ENH: RenameAction and Filter Add Overwrite Option, Result Changes (#912)

Co-authored-by: Matthew Marine <matthew.marine@bluequartz.net> Added new overwrite functionality Works by just deleting other object and renaming accordingly (no move semantics) Small change in MergeResults()
BlueQuartzSoftware · May 2, 2024 · f0e0eb7 · f0e0eb7
1 parent 5bb8213
commit f0e0eb7
Show file tree

Hide file tree

Showing 9 changed files with 217 additions and 22 deletions.
diff --git a/...ins/ITKImageProcessing/src/ITKImageProcessing/Filters/Algorithms/ITKImportFijiMontage.cpp b/...ins/ITKImageProcessing/src/ITKImageProcessing/Filters/Algorithms/ITKImportFijiMontage.cpp
@@ -269,7 +269,7 @@ class IOHandler
         // rename grayscale array to reflect original
         {
           auto& gray = m_DataStructure.getDataRefAs<IDataArray>(imageDataPath.replaceName("gray" + imageDataPath.getTargetName()));
-          if(!gray.canRename(imageDataPath.getTargetName()))
+          if(gray.canRename(imageDataPath.getTargetName()) == false)
           {
             return MakeErrorResult(-18543, fmt::format("Unable to rename the grayscale array to {}", imageDataPath.getTargetName()));
           }

diff --git a/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/RenameDataObjectFilter.cpp b/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/RenameDataObjectFilter.cpp
@@ -56,6 +56,8 @@ Parameters RenameDataObjectFilter::parameters() const
   Parameters params;
 
   params.insertSeparator(Parameters::Separator{"Input Parameters"});
+  params.insert(std::make_unique<BoolParameter>(k_AllowOverwrite_Key, "Allow Overwrite",
+                                                "If true existing object with `New Name` and all of its nested objects will be removed to free up the name for the target DataObject", false));
   params.insert(std::make_unique<DataPathSelectionParameter>(k_SourceDataObjectPath_Key, "DataObject to Rename", "DataPath pointing to the target DataObject", DataPath()));
   params.insert(std::make_unique<StringParameter>(k_NewName_Key, "New Name", "Target DataObject name", ""));
   return params;
@@ -71,10 +73,11 @@ IFilter::UniquePointer RenameDataObjectFilter::clone() const
 IFilter::PreflightResult RenameDataObjectFilter::preflightImpl(const DataStructure& dataStructure, const Arguments& filterArgs, const MessageHandler& messageHandler,
                                                                const std::atomic_bool& shouldCancel) const
 {
+  auto allowOverwrite = filterArgs.value<bool>(k_AllowOverwrite_Key);
   auto dataGroupPath = filterArgs.value<DataPath>(k_SourceDataObjectPath_Key);
   auto newName = filterArgs.value<std::string>(k_NewName_Key);
 
-  auto action = std::make_unique<RenameDataAction>(dataGroupPath, newName);
+  auto action = std::make_unique<RenameDataAction>(dataGroupPath, newName, allowOverwrite);
 
   OutputActions actions;
   actions.appendAction(std::move(action));

diff --git a/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/RenameDataObjectFilter.hpp b/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/RenameDataObjectFilter.hpp
@@ -25,6 +25,7 @@ class SIMPLNXCORE_EXPORT RenameDataObjectFilter : public IFilter
   RenameDataObjectFilter& operator=(RenameDataObjectFilter&&) noexcept = delete;
 
   // Parameter Keys
+  static inline constexpr StringLiteral k_AllowOverwrite_Key = "allow_overwrite";
   static inline constexpr StringLiteral k_SourceDataObjectPath_Key = "source_data_object_path";
   static inline constexpr StringLiteral k_NewName_Key = "new_name";
 

diff --git a/src/Plugins/SimplnxCore/test/RenameDataObjectTest.cpp b/src/Plugins/SimplnxCore/test/RenameDataObjectTest.cpp
@@ -66,3 +66,88 @@ TEST_CASE("SimplnxCore::RenameDataAction(Valid Parameters)", "[SimplnxCore][Rena
 
   REQUIRE(dataObject->getName() == k_NewName);
 }
+
+TEST_CASE("SimplnxCore::RenameDataAction(Valid Overwrite)", "[SimplnxCore][RenameDataAction]")
+{
+  static constexpr StringLiteral k_NewName = Constants::k_GroupHName;
+  static const DataPath k_DataPath({Constants::k_GroupAName, Constants::k_GroupCName, Constants::k_GroupDName, Constants::k_ArrayIName});
+
+  RenameDataObjectFilter filter;
+  DataStructure dataStructure = UnitTest::CreateComplexMultiLevelDataGraph();
+  Arguments args;
+
+  args.insert(RenameDataObjectFilter::k_AllowOverwrite_Key, std::make_any<bool>(true));
+  args.insert(RenameDataObjectFilter::k_NewName_Key, std::make_any<std::string>(k_NewName));
+  args.insert(RenameDataObjectFilter::k_SourceDataObjectPath_Key, std::make_any<DataPath>(k_DataPath));
+
+  auto preflightResult = filter.preflight(dataStructure, args);
+  SIMPLNX_RESULT_REQUIRE_VALID(preflightResult.outputActions);
+
+  // There is a warning clause, but under current implementation it won't be reached
+  //  bool warningFound = false;
+  //  for(const auto& warning : preflightResult.outputActions.warnings())
+  //  {
+  //    if(warning.code == -6602)
+  //    {
+  //      warningFound = true;
+  //    }
+  //  }
+  //  REQUIRE(warningFound);
+
+  auto result = filter.execute(dataStructure, args);
+  SIMPLNX_RESULT_REQUIRE_VALID(result.result);
+
+  // Verify rename was successful
+  {
+    DataPath newPath({Constants::k_GroupAName, Constants::k_GroupCName, Constants::k_GroupDName, k_NewName});
+    auto* dataObject = dataStructure.getData(newPath);
+    REQUIRE(dataObject != nullptr);
+
+    REQUIRE(dataObject->getName() == k_NewName);
+  }
+
+  // Verify old DataGroup (`H`) was removed
+  {
+    DataPath oldHPath({Constants::k_GroupAName, Constants::k_GroupHName});
+    auto* dataObject = dataStructure.getData(oldHPath);
+    REQUIRE(dataObject == nullptr);
+  }
+
+  // Verify old DataGroup (`H`) sub-array (`N`) was removed
+  {
+    DataPath oldHChildPath({Constants::k_GroupAName, Constants::k_GroupHName, Constants::k_ArrayNName});
+    auto* dataObject = dataStructure.getData(oldHChildPath);
+    REQUIRE(dataObject == nullptr);
+  }
+}
+
+TEST_CASE("SimplnxCore::RenameDataAction(InValid Overwrite)", "[SimplnxCore][RenameDataAction]")
+{
+  static constexpr StringLiteral k_NewName = Constants::k_GroupDName;
+  static const DataPath k_DataPath({Constants::k_GroupAName, Constants::k_GroupCName, Constants::k_GroupDName, Constants::k_ArrayIName});
+
+  RenameDataObjectFilter filter;
+
+  DataStructure dataStructure = UnitTest::CreateComplexMultiLevelDataGraph();
+  Arguments args;
+
+  args.insert(RenameDataObjectFilter::k_AllowOverwrite_Key, std::make_any<bool>(true));
+  args.insert(RenameDataObjectFilter::k_NewName_Key, std::make_any<std::string>(k_NewName));
+  args.insert(RenameDataObjectFilter::k_SourceDataObjectPath_Key, std::make_any<DataPath>(k_DataPath));
+
+  auto preflightResult = filter.preflight(dataStructure, args);
+  SIMPLNX_RESULT_REQUIRE_VALID(preflightResult.outputActions);
+
+  auto result = filter.execute(dataStructure, args);
+  SIMPLNX_RESULT_REQUIRE_INVALID(result.result);
+
+  bool errorFound = false;
+  for(const auto& error : result.result.errors())
+  {
+    if(error.code == -6601)
+    {
+      errorFound = true;
+    }
+  }
+  REQUIRE(errorFound);
+}
diff --git a/src/simplnx/Common/Result.hpp b/src/simplnx/Common/Result.hpp
@@ -259,19 +259,6 @@ inline bool ExtractResult(Result<> result, std::vector<Error>& errors, std::vect
 template <typename T = void>
 inline Result<T> MergeResults(Result<T> first, Result<T> second)
 {
-  usize warningsSize = first.warnings().size() + second.warnings().size();
-  std::vector<Warning> warnings;
-  warnings.reserve(warningsSize);
-
-  for(auto&& warning : first.warnings())
-  {
-    warnings.push_back(std::move(warning));
-  }
-  for(auto&& warning : second.warnings())
-  {
-    warnings.push_back(std::move(warning));
-  }
-
   usize errorsSize = 0;
   if(first.invalid())
   {
@@ -299,8 +286,18 @@ inline Result<T> MergeResults(Result<T> first, Result<T> second)
     }
   }
 
-  Result<T> result = errors.empty() ? Result<T>{} : Result<T>{nonstd::make_unexpected(std::move(errors))};
-  result.warnings() = std::move(warnings);
+  Result<T> result = errors.empty() ? Result<T>{} : Result<T>{{nonstd::make_unexpected(std::move(errors))}};
+
+  result.m_Warnings.reserve(first.warnings().size() + second.warnings().size());
+  for(auto&& warning : first.warnings())
+  {
+    result.m_Warnings.push_back(std::move(warning));
+  }
+  for(auto&& warning : second.warnings())
+  {
+    result.m_Warnings.push_back(std::move(warning));
+  }
+
   return result;
 }
 

diff --git a/src/simplnx/DataStructure/BaseGroup.cpp b/src/simplnx/DataStructure/BaseGroup.cpp
@@ -246,3 +246,8 @@ void BaseGroup::checkUpdatedIdsImpl(const std::vector<std::pair<IdType, IdType>>
 {
   m_DataMap.updateIds(updatedIds);
 }
+
+std::vector<DataObject::IdType> BaseGroup::GetChildrenIds()
+{
+  return m_DataMap.getKeys();
+}
diff --git a/src/simplnx/DataStructure/BaseGroup.hpp b/src/simplnx/DataStructure/BaseGroup.hpp
@@ -328,6 +328,11 @@ class SIMPLNX_EXPORT BaseGroup : public DataObject
    */
   std::vector<std::string> GetChildrenNames();
 
+  /**
+   * @brief Querys the DataMap for the object ids in m_DataMap
+   */
+  std::vector<DataObject::IdType> GetChildrenIds();
+
 protected:
   /**
    * @brief Creates a BaseGroup with the target DataStructure and name.

diff --git a/src/simplnx/Filter/Actions/RenameDataAction.cpp b/src/simplnx/Filter/Actions/RenameDataAction.cpp
@@ -7,11 +7,47 @@
 
 using namespace nx::core;
 
+namespace
+{
+Result<> TerminateNodesRecursively(DataStructure& dataStructure, DataObject::IdType id, IDataAction::Mode mode, const bool checkDependence)
+{
+  Result<> result = {};
+  DataObject* node = dataStructure.getData(id);
+
+  if(checkDependence)
+  {
+    auto parentIds = node->getParentIds();
+    if(parentIds.size() > 1)
+    {
+      return {};
+    }
+  }
+
+  auto* baseGroup = dataStructure.getDataAs<BaseGroup>(id);
+  if(baseGroup != nullptr)
+  {
+    auto childIds = baseGroup->GetChildrenIds();
+    if(!childIds.empty())
+    {
+      for(const auto& childId : childIds)
+      {
+        result = MergeResults(result, std::move(TerminateNodesRecursively(dataStructure, childId, mode, checkDependence)));
+      }
+    }
+  }
+
+  dataStructure.removeData(node->getId());
+
+  return result;
+}
+} // namespace
+
 namespace nx::core
 {
-RenameDataAction::RenameDataAction(const DataPath& path, const std::string& newName)
+RenameDataAction::RenameDataAction(const DataPath& path, const std::string& newName, bool overwrite)
 : m_NewName(newName)
 , m_Path(path)
+, m_Overwrite(overwrite)
 {
 }
 
@@ -27,14 +63,64 @@ Result<> RenameDataAction::apply(DataStructure& dataStructure, Mode mode) const
     return MakeErrorResult(-6601, ss);
   }
 
-  if(!dataObject->canRename(m_NewName))
+  Result<> result = {};
+
+  if(m_Overwrite)
+  {
+    if(dataObject->getName() == m_NewName)
+    {
+      return result;
+    }
+
+    std::vector<std::string> pathVec = m_Path.getPathVector();
+    for(const auto& name : pathVec)
+    {
+      if(name == m_NewName)
+      {
+        std::string ss = fmt::format("{}The object that would be overwritten is a parent container to {} cannot rename to {}", prefix, m_Path.getTargetName(), m_NewName);
+        return MakeErrorResult(-6601, ss);
+      }
+    }
+
+    DataObject::IdType targetId = std::numeric_limits<DataObject::IdType>::max();
+    for(auto dataObjectID : dataStructure.getAllDataObjectIds())
+    {
+      if(dataStructure.getData(dataObjectID)->getName() == m_NewName)
+      {
+        targetId = dataObjectID;
+        break;
+      }
+    }
+
+    if(targetId != std::numeric_limits<DataObject::IdType>::max())
+    {
+      if(mode == Mode::Preflight)
+      {
+        std::string ss = fmt::format("{}Another object exists with that name, will overwrite destroying other DataObject at '{}' and replacing it with '{}'", prefix, m_NewName, m_Path.toString());
+        result.warnings().emplace_back(Warning{-6602, ss});
+      }
+      else
+      {
+        if(dataStructure.getDataAs<BaseGroup>(targetId) != nullptr)
+        {
+          // Recursive removal of overwritten object
+          result = MergeResults(result, ::TerminateNodesRecursively(dataStructure, targetId, mode, true));
+        }
+        else
+        {
+          dataStructure.removeData(targetId);
+        }
+      }
+    }
+  }
+  else if(!dataObject->canRename(m_NewName))
   {
     std::string ss = fmt::format("{}Could not rename DataObject at '{}' to '{}'", prefix, m_Path.toString(), m_NewName);
-    return MakeErrorResult(-6602, ss);
+    return MakeErrorResult(-6603, ss);
   }
 
   dataObject->rename(m_NewName);
-  return {};
+  return result;
 }
 
 IDataAction::UniquePointer RenameDataAction::clone() const
@@ -51,4 +137,10 @@ const DataPath& RenameDataAction::path() const
 {
   return m_Path;
 }
+
+bool RenameDataAction::overwrite() const
+{
+  return m_Overwrite;
+}
+
 } // namespace nx::core
diff --git a/src/simplnx/Filter/Actions/RenameDataAction.hpp b/src/simplnx/Filter/Actions/RenameDataAction.hpp
@@ -13,7 +13,7 @@ class SIMPLNX_EXPORT RenameDataAction : public IDataAction
 public:
   RenameDataAction() = delete;
 
-  RenameDataAction(const DataPath& path, const std::string& newName);
+  RenameDataAction(const DataPath& path, const std::string& newName, bool overwrite = false);
 
   ~RenameDataAction() noexcept override;
 
@@ -48,8 +48,15 @@ class SIMPLNX_EXPORT RenameDataAction : public IDataAction
    */
   const DataPath& path() const;
 
+  /**
+   * @brief Returns the overwrite value
+   * @return
+   */
+  bool overwrite() const;
+
 private:
   std::string m_NewName;
   DataPath m_Path;
+  bool m_Overwrite;
 };
 } // namespace nx::core