Exception when merging positive id entity into negative id entity (#5655

) * exception when merging positive id entity into negative id entity * Fix assertion failure for element merger * Update element merge server * add failing test using positive id merged into negative expects the hoot:merge:target tag to be removed and the hoot:status=3 tag to be added * Update merge functions to return the correct ElementId of the remaining element --------- Co-authored-by: Ben Marchant <[email protected]>
ngageoint · May 26, 2023 · d67c5b4 · d67c5b4
1 parent af525ae
commit d67c5b4
Show file tree

Hide file tree

Showing 10 changed files with 136 additions and 71 deletions.
diff --git a/hoot-core/src/main/cpp/hoot/core/conflate/polygon/BuildingMerger.cpp b/hoot-core/src/main/cpp/hoot/core/conflate/polygon/BuildingMerger.cpp
@@ -22,7 +22,7 @@
  * This will properly maintain the copyright information. Maxar
  * copyrights will be updated automatically.
  *
- * @copyright Copyright (C) 2015, 2016, 2017, 2018, 2019, 2020, 2021, 2022 Maxar (http://www.maxar.com/)
+ * @copyright Copyright (C) 2015-2023 Maxar (http://www.maxar.com/)
  */
 #include "BuildingMerger.h"
 
@@ -39,8 +39,8 @@
 #include <hoot/core/elements/ElementIdUtils.h>
 #include <hoot/core/elements/InMemoryElementSorter.h>
 #include <hoot/core/elements/OsmUtils.h>
-#include <hoot/core/elements/TagUtils.h>
 #include <hoot/core/elements/Relation.h>
+#include <hoot/core/elements/TagUtils.h>
 #include <hoot/core/io/OsmMapWriterFactory.h>
 #include <hoot/core/ops/IdSwapOp.h>
 #include <hoot/core/ops/RecursiveElementRemover.h>
@@ -308,17 +308,15 @@ ElementId BuildingMerger::_getIdOfMoreComplexBuilding(const ElementPtr& building
   if (building1.get())
   {
     LOG_VART(building1);
-    nodeCount1 =
-      (int)FilteredVisitor::getStat(std::make_shared<NodeCriterion>(), std::make_shared<ElementCountVisitor>(), map, building1);
+    nodeCount1 = (int)FilteredVisitor::getStat(std::make_shared<NodeCriterion>(), std::make_shared<ElementCountVisitor>(), map, building1);
   }
   LOG_VART(nodeCount1);
 
   int nodeCount2 = 0;
   if (building2.get())
   {
     LOG_VART(building2);
-    nodeCount2 =
-      (int)FilteredVisitor::getStat(std::make_shared<NodeCriterion>(), std::make_shared<ElementCountVisitor>(), map, building2);
+    nodeCount2 = (int)FilteredVisitor::getStat(std::make_shared<NodeCriterion>(), std::make_shared<ElementCountVisitor>(), map, building2);
   }
   LOG_VART(nodeCount2);
 
@@ -690,7 +688,7 @@ void BuildingMerger::_fixStatuses(OsmMapPtr map)
   }
 }
 
-void BuildingMerger::merge(OsmMapPtr map, const ElementId& mergeTargetId)
+ElementId BuildingMerger::merge(OsmMapPtr map, const ElementId& mergeTargetId)
 {
   LOG_INFO("Merging buildings...");
 
@@ -716,6 +714,7 @@ void BuildingMerger::merge(OsmMapPtr map, const ElementId& mergeTargetId)
 
   int buildingsMerged = 0;
   BuildingCriterion buildingCrit;
+  std::vector<std::pair<ElementId, ElementId>> replacedElements;
   //  Copy the way map so the original can be modified
   const WayMap ways = map->getWays();
   for (auto wayItr = ways.begin(); wayItr != ways.end(); ++wayItr)
@@ -728,7 +727,6 @@ void BuildingMerger::merge(OsmMapPtr map, const ElementId& mergeTargetId)
       pairs.emplace(mergeTargetId, way->getElementId());
       BuildingMerger merger(pairs);
       LOG_VART(pairs.size());
-      std::vector<std::pair<ElementId, ElementId>> replacedElements;
       merger.apply(map, replacedElements);
       buildingsMerged++;
     }
@@ -745,13 +743,23 @@ void BuildingMerger::merge(OsmMapPtr map, const ElementId& mergeTargetId)
       pairs.emplace(mergeTargetId, relation->getElementId());
       BuildingMerger merger(pairs);
       LOG_VART(pairs.size());
-      std::vector<std::pair<ElementId, ElementId>> replacedElements;
       merger.apply(map, replacedElements);
       buildingsMerged++;
     }
   }
 
   LOG_INFO("Merged " << buildingsMerged << " buildings.");
+
+  //  Check if the merge target wasn't changed
+  if (map->containsElement(mergeTargetId))
+    return mergeTargetId;
+  //  Find the correct object that was merged
+  for (const auto& p : replacedElements)
+  {
+    if (p.first == mergeTargetId && map->containsElement(p.second))
+      return p.second;
+  }
+  return mergeTargetId;
 }
 
 QString BuildingMerger::toString() const

diff --git a/hoot-core/src/main/cpp/hoot/core/conflate/polygon/BuildingMerger.h b/hoot-core/src/main/cpp/hoot/core/conflate/polygon/BuildingMerger.h
@@ -22,7 +22,7 @@
  * This will properly maintain the copyright information. Maxar
  * copyrights will be updated automatically.
  *
- * @copyright Copyright (C) 2015, 2017, 2018, 2019, 2020, 2021, 2022 Maxar (http://www.maxar.com/)
+ * @copyright Copyright (C) 2015-2023 Maxar (http://www.maxar.com/)
  */
 #ifndef BUILDINGMERGER_H
 #define BUILDINGMERGER_H
@@ -76,7 +76,7 @@ class BuildingMerger : public MergerBase
    * @param map a map containing the buildings to be merged
    * @param mergeTargetId the ID of the building which all other buildings should be merged into
    */
-  static void merge(OsmMapPtr map, const ElementId& mergeTargetId);
+  static ElementId merge(OsmMapPtr map, const ElementId& mergeTargetId);
 
   /**
    * Adds multiple buildings to the same relation

diff --git a/hoot-js/src/main/cpp/hoot/js/conflate/merging/AreaMergerJs.cpp b/hoot-js/src/main/cpp/hoot/js/conflate/merging/AreaMergerJs.cpp
@@ -22,7 +22,7 @@
  * This will properly maintain the copyright information. Maxar
  * copyrights will be updated automatically.
  *
- * @copyright Copyright (C) 2016, 2018, 2019, 2021, 2022 Maxar (http://www.maxar.com/)
+ * @copyright Copyright (C) 2016-2023 Maxar (http://www.maxar.com/)
  */
 #include "AreaMergerJs.h"
 
@@ -41,7 +41,7 @@ using namespace v8;
 namespace hoot
 {
 
-void AreaMergerJs::merge(OsmMapPtr map, const ElementId& mergeTargetId, Isolate* current)
+ElementId AreaMergerJs::merge(OsmMapPtr map, const ElementId& mergeTargetId, Isolate* current)
 {
   LOG_INFO("Merging areas...");
 
@@ -63,6 +63,7 @@ void AreaMergerJs::merge(OsmMapPtr map, const ElementId& mergeTargetId, Isolate*
   int areasMerged = 0;
 
   NonBuildingAreaCriterion nonBuildingAreaCrit;
+  std::vector<std::pair<ElementId, ElementId>> replacedElements;
   //  Make a copy of the way map so that the mergers can modify the original
   const WayMap ways = map->getWays();
   for (auto it = ways.begin(); it != ways.end(); ++it)
@@ -78,9 +79,8 @@ void AreaMergerJs::merge(OsmMapPtr map, const ElementId& mergeTargetId, Isolate*
       matches.emplace(mergeTargetId, ElementId::way(way->getId()));
       // apply script merging
       ScriptMerger merger(script, plugin, matches);
-      std::vector<std::pair<ElementId, ElementId>> replacedWays;
-      merger.apply(map, replacedWays);
-      LOG_VART(replacedWays.size());
+      merger.apply(map, replacedElements);
+      LOG_VART(replacedElements.size());
 
       areasMerged++;
     }
@@ -100,15 +100,25 @@ void AreaMergerJs::merge(OsmMapPtr map, const ElementId& mergeTargetId, Isolate*
       matches.emplace(mergeTargetId, ElementId::relation(relation->getId()));
       // apply script merging
       ScriptMerger merger(script, plugin, matches);
-      std::vector<std::pair<ElementId, ElementId>> replacedRelations;
-      merger.apply(map, replacedRelations);
-      LOG_VART(replacedRelations.size());
+      merger.apply(map, replacedElements);
+      LOG_VART(replacedElements.size());
 
       areasMerged++;
     }
   }
 
   LOG_INFO("Merged " << areasMerged << " areas.");
+
+  //  Check if the merge target wasn't changed
+  if (map->containsElement(mergeTargetId))
+    return mergeTargetId;
+  //  Find the correct object that was merged
+  for (const auto& p : replacedElements)
+  {
+    if (p.first == mergeTargetId && map->containsElement(p.second))
+      return p.second;
+  }
+  return mergeTargetId;
 }
 
 }
diff --git a/hoot-js/src/main/cpp/hoot/js/conflate/merging/AreaMergerJs.h b/hoot-js/src/main/cpp/hoot/js/conflate/merging/AreaMergerJs.h
@@ -22,7 +22,7 @@
  * This will properly maintain the copyright information. Maxar
  * copyrights will be updated automatically.
  *
- * @copyright Copyright (C) 2016, 2018, 2019, 2021 Maxar (http://www.maxar.com/)
+ * @copyright Copyright (C) 2016-2023 Maxar (http://www.maxar.com/)
  */
 
 #ifndef AREAMERGERJS_H
@@ -32,8 +32,8 @@
 #include <hoot/core/elements/OsmMap.h>
 
 #include <hoot/js/HootJsStable.h>
-#include <hoot/js/SystemNodeJs.h>
 #include <hoot/js/PluginContext.h>
+#include <hoot/js/SystemNodeJs.h>
 
 namespace hoot
 {
@@ -57,7 +57,7 @@ class AreaMergerJs
    * @param mergeTargetId the ID of the area which all other areas should be merged into
    * @param current the context this method should run under
    */
-  static void merge(OsmMapPtr map, const ElementId& mergeTargetId, v8::Isolate* current);
+  static ElementId merge(OsmMapPtr map, const ElementId& mergeTargetId, v8::Isolate* current);
 };
 
 }

diff --git a/hoot-js/src/main/cpp/hoot/js/conflate/merging/ElementMergerJs.cpp b/hoot-js/src/main/cpp/hoot/js/conflate/merging/ElementMergerJs.cpp
@@ -22,7 +22,7 @@
  * This will properly maintain the copyright information. Maxar
  * copyrights will be updated automatically.
  *
- * @copyright Copyright (C) 2018, 2019, 2020, 2021, 2022 Maxar (http://www.maxar.com/)
+ * @copyright Copyright (C) 2018-2023 Maxar (http://www.maxar.com/)
  */
 #include "ElementMergerJs.h"
 
@@ -77,8 +77,7 @@ void ElementMergerJs::Init(Local<Object> exports)
   Isolate* current = exports->GetIsolate();
   HandleScope scope(current);
   Local<Context> context = current->GetCurrentContext();
-  Maybe<bool> success = exports->Set(context, toV8("merge"),
-                                 FunctionTemplate::New(current, merge)->GetFunction(context).ToLocalChecked());
+  Maybe<bool> success = exports->Set(context, toV8("merge"), FunctionTemplate::New(current, merge)->GetFunction(context).ToLocalChecked());
   (void) success; // unused variable
 }
 
@@ -93,8 +92,7 @@ void ElementMergerJs::merge(const FunctionCallbackInfo<Value>& args)
   {
     if (args.Length() != 1)
     {
-      args.GetReturnValue().Set(
-        current->ThrowException(HootExceptionJs::create(IllegalArgumentException("Expected one argument passed to 'merge'."))));
+      args.GetReturnValue().Set(current->ThrowException(HootExceptionJs::create(IllegalArgumentException("Expected one argument passed to 'merge'."))));
       return;
     }
 
@@ -149,26 +147,24 @@ void ElementMergerJs::_merge(OsmMapPtr map, Isolate* current)
   }
 
   // We're using a mix of generic conflation scripts and custom built classes to merge features
-  // here, depending on the feature type.
+  // here, depending on the feature type, let the merger determine the merge target ID.
   switch (mergeType)
   {
   case MergeType::Building:
-    BuildingMerger::merge(map, mergeTargetId);
+    mergeTargetId = BuildingMerger::merge(map, mergeTargetId);
     break;
   case MergeType::PoiToPolygon:
-    // POI/Poly always merges into the polygon and there's only one of them, so we let the
-    // routine determine the merge target ID.
     mergeTargetId = PoiPolygonMerger::mergeOnePoiAndOnePolygon(map);
     break;
   case MergeType::Poi:
-    PoiMergerJs::merge(map, mergeTargetId, current);
+    mergeTargetId = PoiMergerJs::merge(map, mergeTargetId, current);
     break;
   case MergeType::Area:
-    AreaMergerJs::merge(map, mergeTargetId, current);
+    mergeTargetId = AreaMergerJs::merge(map, mergeTargetId, current);
     break;
   case MergeType::Railway:
     MapProjector::projectToPlanar(map);
-    RailwayMergerJs::merge(map, mergeTargetId, current);
+    mergeTargetId = RailwayMergerJs::merge(map, mergeTargetId, current);
     SuperfluousNodeRemover::removeNodes(map);
     MapProjector::projectToWgs84(map);
     break;
@@ -179,11 +175,13 @@ void ElementMergerJs::_merge(OsmMapPtr map, Isolate* current)
   // By convention, we're setting the status of any element that gets merged with something to
   // conflated, even if its yet to be reviewed against something else.
   ElementPtr mergedElement = map->getElement(mergeTargetId);
-  assert(mergedElement);
-  mergedElement->setStatus(Status(Status::Conflated));
-  mergedElement->getTags()[MetadataTags::HootStatus()] = "3";
-  mergedElement->getTags().remove(MetadataTags::HootMergeTarget());
-  LOG_VART(mergedElement);
+  if (mergedElement)
+  {
+    mergedElement->setStatus(Status(Status::Conflated));
+    mergedElement->getTags()[MetadataTags::HootStatus()] = "3";
+    mergedElement->getTags().remove(MetadataTags::HootMergeTarget());
+    LOG_VART(mergedElement);
+  }
 }
 
 ElementId ElementMergerJs::_getMergeTargetFeatureId(ConstOsmMapPtr map)
@@ -210,10 +208,7 @@ ElementId ElementMergerJs::_getMergeTargetFeatureId(ConstOsmMapPtr map)
           std::make_shared<StatusCriterion>(Status::Conflated)),
         std::make_shared<ElementCountVisitor>(), map);
     if (numStatus1Elements != 1)
-    {
-      throw IllegalArgumentException(
-        "Input map must have at exactly one feature with status=1 or status=3.");
-    }
+      throw IllegalArgumentException("Input map must have at exactly one feature with status=1 or status=3.");
 
     // Return the ref or already conflated feature's ID as the target ID.
     ConstElementPtr status1Element = MapUtils::getFirstElementWithStatus(map, Status::Unknown1);
@@ -232,10 +227,7 @@ ElementId ElementMergerJs::_getMergeTargetFeatureId(ConstOsmMapPtr map)
       std::make_shared<ElementCountVisitor>(), map);
   LOG_VART(numMergeTargets);
   if (numMergeTargets != 1)
-  {
-    throw IllegalArgumentException(
-      QString("Input map must have exactly one feature marked with a %1 tag.").arg(MetadataTags::HootMergeTarget()));
-  }
+    throw IllegalArgumentException(QString("Input map must have exactly one feature marked with a %1 tag.").arg(MetadataTags::HootMergeTarget()));
 
   const long numNonMergeTargets =
     (long)FilteredVisitor::getStat(
@@ -244,10 +236,7 @@ ElementId ElementMergerJs::_getMergeTargetFeatureId(ConstOsmMapPtr map)
       std::make_shared<ElementCountVisitor>(), map);
   LOG_VART(numMergeTargets);
   if (numNonMergeTargets < 1)
-  {
-    throw IllegalArgumentException(
-      QString("Input map must have at least one feature not marked with a %1 tag.").arg(MetadataTags::HootMergeTarget()));
-  }
+    throw IllegalArgumentException(QString("Input map must have at least one feature not marked with a %1 tag.").arg(MetadataTags::HootMergeTarget()));
 
   TagKeyCriterion mergeTagCrit(MetadataTags::HootMergeTarget());
   UniqueElementIdVisitor idSetVis;

diff --git a/hoot-js/src/main/cpp/hoot/js/conflate/merging/PoiMergerJs.cpp b/hoot-js/src/main/cpp/hoot/js/conflate/merging/PoiMergerJs.cpp
@@ -22,7 +22,7 @@
  * This will properly maintain the copyright information. Maxar
  * copyrights will be updated automatically.
  *
- * @copyright Copyright (C) 2015, 2017, 2018, 2019, 2020, 2021, 2022 Maxar (http://www.maxar.com/)
+ * @copyright Copyright (C) 2015-2023 Maxar (http://www.maxar.com/)
  */
 #include "PoiMergerJs.h"
 
@@ -41,7 +41,7 @@ using namespace v8;
 namespace hoot
 {
 
-void PoiMergerJs::merge(OsmMapPtr map, const ElementId& mergeTargetId, Isolate* current)
+ElementId PoiMergerJs::merge(OsmMapPtr map, const ElementId& mergeTargetId, Isolate* current)
 {
   LOG_INFO("Merging POIs...");
 
@@ -68,6 +68,7 @@ void PoiMergerJs::merge(OsmMapPtr map, const ElementId& mergeTargetId, Isolate*
   //
   // ...then pass those pairs one at a time through the merger, since it only merges pairs
   int poisMerged = 0;
+  std::vector<std::pair<ElementId, ElementId>> replacedNodes;
   //  Make a copy of the node map so that the iterators work while merging
   const NodeMap nodes = map->getNodes();
   for (auto it = nodes.begin(); it != nodes.end(); ++it)
@@ -81,14 +82,24 @@ void PoiMergerJs::merge(OsmMapPtr map, const ElementId& mergeTargetId, Isolate*
       matches.emplace(mergeTargetId, node->getElementId());
       // apply script merging
       ScriptMerger merger(script, plugin, matches);
-      std::vector<std::pair<ElementId, ElementId>> replacedNodes;
       merger.apply(map, replacedNodes);
       LOG_VART(replacedNodes.size());
 
       poisMerged++;
     }
   }
   LOG_INFO("Merged " << poisMerged << " POIs.");
+
+  //  Check if the merge target wasn't changed
+  if (map->containsElement(mergeTargetId))
+    return mergeTargetId;
+  //  Find the correct object that was merged
+  for (const auto& p : replacedNodes)
+  {
+    if (p.first == mergeTargetId && map->containsElement(p.second))
+      return p.second;
+  }
+  return mergeTargetId;
 }
 
 }
diff --git a/hoot-js/src/main/cpp/hoot/js/conflate/merging/PoiMergerJs.h b/hoot-js/src/main/cpp/hoot/js/conflate/merging/PoiMergerJs.h
@@ -22,7 +22,7 @@
  * This will properly maintain the copyright information. Maxar
  * copyrights will be updated automatically.
  *
- * @copyright Copyright (C) 2015, 2018, 2019, 2021 Maxar (http://www.maxar.com/)
+ * @copyright Copyright (C) 2015-2023 Maxar (http://www.maxar.com/)
  */
 
 #ifndef POIMERGERJS_H
@@ -32,8 +32,8 @@
 #include <hoot/core/elements/OsmMap.h>
 
 #include <hoot/js/HootJsStable.h>
-#include <hoot/js/SystemNodeJs.h>
 #include <hoot/js/PluginContext.h>
+#include <hoot/js/SystemNodeJs.h>
 
 namespace hoot
 {
@@ -55,7 +55,7 @@ class PoiMergerJs
    * @param mergeTargetId the ID of the area which all other POIs should be merged into
    * @param current the context this method should run under
    */
-  static void merge(OsmMapPtr map, const ElementId& mergeTargetId, v8::Isolate* current);
+  static ElementId merge(OsmMapPtr map, const ElementId& mergeTargetId, v8::Isolate* current);
 };
 
 }