Ticket #21788: 21788.patch

src/com/vividsolutions/jcs/conflate/polygonmatch/AngleHistogramMatcher.java

@@ -84 +84 @@
      * weighted by segment length
      */
     protected Histogram angleHistogram(Geometry g, int binCount) {
-        Geometry clone = (Geometry) g.clone();
+        Geometry clone = g.copy();
         //#normalize makes linestrings and polygons use a standard orientation.
         //[Jon Aquino]
         clone.normalize();

src/com/vividsolutions/jcs/conflate/polygonmatch/CentroidAligner.java

@@ -16 +16 @@
     }
 
     private Geometry align(Geometry original) {
-        Geometry aligned = (Geometry) original.clone();
+        Geometry aligned = original.copy();
         MatcherUtil.align(aligned, aligned.getCentroid().getCoordinate());
         return aligned;
     }

src/com/vividsolutions/jcs/conflate/polygonmatch/Matches.java

@@ -31 +31 @@
  */
 package com.vividsolutions.jcs.conflate.polygonmatch;
 
+import java.util.AbstractMap;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Map;
+import java.util.Set;
 
 import org.locationtech.jts.geom.Envelope;
 import org.locationtech.jts.util.Assert;
@@ -47 +51 @@
  * A FeatureCollection that stores the "score" of each Feature.  The score is
  * a number between 0.0 and 1.0 that indicates the confidence of a match.
  */
-public class Matches implements FeatureCollection, Cloneable {
-
+public class Matches extends AbstractMap<Feature, Double> implements FeatureCollection, Cloneable {
+    private final Set<Map.Entry<Feature, Double>> entrySet = new HashSet<>();
     /**
      * Creates a Matches object.
      * @param schema metadata applicable to the features that will be stored in
@@ -80 +84 @@
         }
     }
 
-    private FeatureDataset dataset;
-    private List<Double> scores = new ArrayList<>();
+    private final FeatureDataset dataset;
+    private final List<Double> scores = new ArrayList<>();
 
     /**
      * This method is not supported, because added features need to be associated
@@ -135 +139 @@
         throw new UnsupportedOperationException();
     }
 
+    @Override
+    public Set<Entry<Feature, Double>> entrySet() {
+        if (this.size() == this.entrySet.size()) {
+            return this.entrySet;
+        }
+        return updateEntrySet();
+    }
+
+    private synchronized Set<Entry<Feature, Double>> updateEntrySet() {
+        if (this.size() != this.entrySet.size()) {
+            this.entrySet.clear();
+            for (int i = 0; i < this.size(); i++) {
+                this.entrySet.add(new SimpleEntry<>(this.getFeature(i), this.getScore(i)));
+            }
+        }
+        return this.entrySet;
+    }
+
     /**
      * This method is not supported, because Matches should not normally need to
      * have matches removed.
@@ -167 +189 @@
         if (score == 0) {
             return;
         }
-        scores.add(Double.valueOf(score));
+        scores.add(score);
         dataset.add(feature);
         if (score > topScore) {
             topScore = score;
@@ -195 +217 @@
      * @return the confidence of the ith match
      */
     public double getScore(int i) {
-        return scores.get(i).doubleValue();
+        return scores.get(i);
     }
 
     @Override

src/com/vividsolutions/jcs/conflate/polygonmatch/SymDiffMatcher.java

@@ -53 +53 @@
    */
   @Override
   public double match(Geometry target, Geometry candidate) {
-    Geometry targetGeom = (Geometry) target.clone();
-    Geometry candidateGeom = (Geometry) candidate.clone();
+    Geometry targetGeom = target.copy();
+    Geometry candidateGeom = candidate.copy();
     if (targetGeom.isEmpty() || candidateGeom.isEmpty()) {
       return 0; //avoid div by 0 in centre-of-mass calc [Jon Aquino]
     }

src/com/vividsolutions/jcs/conflate/polygonmatch/WeightedMatcher.java

@@ -33 +33 @@
 
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Objects;
 import java.util.TreeMap;
 
 import org.locationtech.jts.util.Assert;
@@ -48 +49 @@
 
     /**
    * Creates a WeightedMatcher with the given matchers and their weights.
-   * @param matchersAndWeights alternates between FeatureMatchers and Doubles
+   * @param matchersAndWeights alternates between Doubles and FeatureMatchers
    */
-  public WeightedMatcher(Object[] matchersAndWeights) {
+  public WeightedMatcher(Object... matchersAndWeights) {
+    Objects.requireNonNull(matchersAndWeights);
     Assert.isTrue(matchersAndWeights.length % 2 == 0);
     for (int i = 0; i < matchersAndWeights.length; i += 2) {
       add((FeatureMatcher) matchersAndWeights[i+1],
@@ -70 +72 @@
     if (weight == 0) {
         return;
     }
-    matcherToWeightMap.put(matcher, Double.valueOf(weight));
+    this.matcherToWeightMap.put(matcher, weight);
   }
 
-  private Map<FeatureMatcher, Double> matcherToWeightMap = new HashMap<>();
+  private final Map<FeatureMatcher, Double> matcherToWeightMap = new HashMap<>();
 
   /**
    * Searches a collection of candidate features for those that match the given
@@ -93 +95 @@
 
   private Matches toMatches(Map<Feature, Double> featureToScoreMap, FeatureSchema schema) {
     Matches matches = new Matches(schema);
-    for (Feature feature : featureToScoreMap.keySet()) {
-      double score = featureToScoreMap.get(feature).doubleValue();
-      matches.add(feature, score);
-    }
+    featureToScoreMap.forEach(matches::add);
     return matches;
   }
 
@@ -111 +110 @@
 
   private Map<Feature, Double> featureToScoreMap(Map<FeatureMatcher, Matches> matcherToMatchesMap) {
     Map<Feature, Double> featureToScoreMap = new TreeMap<>();
-    for (FeatureMatcher matcher : matcherToMatchesMap.keySet()) {
-      Matches matches = matcherToMatchesMap.get(matcher);
-      addToFeatureToScoreMap(matches, matcher, featureToScoreMap);
-    }
+    matcherToMatchesMap.forEach((matcher, matches) -> this.addToFeatureToScoreMap(matches, matcher, featureToScoreMap));
     return featureToScoreMap;
   }
 
   private void addToFeatureToScoreMap(Matches matches, FeatureMatcher matcher,
                                       Map<Feature, Double> featureToScoreMap) {
-    for (int i = 0; i < matches.size(); i++) {
-      double score = matches.getScore(i) * normalizedWeight(matcher);
-      addToFeatureToScoreMap(matches.getFeature(i), score, featureToScoreMap);
+
+    for (Map.Entry<Feature, Double> entry : matches.entrySet()) {
+      double score = entry.getValue() * this.normalizedWeight(matcher);
+      featureToScoreMap.compute(entry.getKey(), (key, value) -> value == null ? score : value + score);
     }
   }
 
-  private void addToFeatureToScoreMap(Feature feature, double score, Map<Feature, Double> featureToScoreMap) {
-    Double oldScore = featureToScoreMap.get(feature);
-    if (oldScore == null) { oldScore = Double.valueOf(0); }
-    featureToScoreMap.put(feature, Double.valueOf(oldScore.doubleValue() + score));
-  }
-
   private double normalizedWeight(FeatureMatcher matcher) {
-    return matcherToWeightMap.get(matcher).doubleValue() / weightTotal();
+    return this.matcherToWeightMap.get(matcher) / this.weightTotal();
   }
 
   private double weightTotal() {
-    double weightTotal = 0;
-    for (Double weight : matcherToWeightMap.values()) {
-      weightTotal += weight.doubleValue();
-    }
-    return weightTotal;
+    return this.matcherToWeightMap.values().stream().mapToDouble(Double::doubleValue).sum();
   }
 }

src/com/vividsolutions/jump/feature/AbstractBasicFeature.java

@@ -182 +182 @@
         for (int i = 0; i < schema.getAttributeCount(); i++) {
             if (schema.getAttributeType(i) == AttributeType.GEOMETRY) {
                 clone.setAttribute(i,
-                    deep ? getGeometry().clone() : getGeometry());
+                    deep ? getGeometry().copy() : getGeometry());
             } else {
                 clone.setAttribute(i, getAttribute(i));
             }

src/com/vividsolutions/jump/util/CoordinateArrays.java

@@ -34 +34 @@
 import java.util.ArrayList;
 import java.util.List;
 
-import org.locationtech.jts.algorithm.CGAlgorithms;
+import org.locationtech.jts.algorithm.Orientation;
 import org.locationtech.jts.geom.Coordinate;
 import org.locationtech.jts.geom.Geometry;
 import org.locationtech.jts.geom.GeometryCollection;
@@ -128 +128 @@
             Coordinate[] shell = poly.getExteriorRing().getCoordinates();
 
             if (orientPolygons) {
-                shell = ensureOrientation(shell, CGAlgorithms.CLOCKWISE);
+                shell = ensureOrientation(shell, Orientation.CLOCKWISE);
             }
 
             coordArrayList.add(shell);
@@ -137 +137 @@
                 Coordinate[] hole = poly.getInteriorRingN(i).getCoordinates();
 
                 if (orientPolygons) {
-                    hole = ensureOrientation(hole, CGAlgorithms.COUNTERCLOCKWISE);
+                    hole = ensureOrientation(hole, Orientation.COUNTERCLOCKWISE);
                 }
 
                 coordArrayList.add(hole);
@@ -168 +168 @@
             return coord;
         }
 
-        int orientation = CGAlgorithms.isCCW(coord) ? CGAlgorithms.COUNTERCLOCKWISE
-                                           : CGAlgorithms.CLOCKWISE;
+        int orientation = Orientation.isCCW(coord) ? Orientation.COUNTERCLOCKWISE
+                                           : Orientation.CLOCKWISE;
 
         if (orientation != desiredOrientation) {
             Coordinate[] reverse = coord.clone();

src/org/openstreetmap/josm/plugins/conflation/OsmFeature.java

@@ -3 +3 @@
 package org.openstreetmap.josm.plugins.conflation;
 
 import java.util.Map;
+import java.util.Objects;
 
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
 import org.openstreetmap.josm.plugins.jts.JTSConverter;
@@ -9 +10 @@
 
 import com.vividsolutions.jump.feature.AbstractBasicFeature;
 import com.vividsolutions.jump.feature.AttributeType;
+import com.vividsolutions.jump.feature.Feature;
 import com.vividsolutions.jump.feature.FeatureSchema;
 
 public class OsmFeature extends AbstractBasicFeature {
     private Object[] attributes;
-    private OsmPrimitive primitive;
-    private JTSConverter converter;
+    private final OsmPrimitive primitive;
 
     /**
      * Create a copy of the OSM geometry
@@ -22 +23 @@
      */
     public OsmFeature(OsmPrimitive prim, JTSConverter jtsConverter) {
         super(new FeatureSchema());
-        primitive = prim;
+        this.primitive = Objects.requireNonNull(prim);
         Map<String, String> keys = prim.getKeys();
-        attributes = new Object[keys.size() + 1];
-        getSchema().addAttribute("__GEOMETRY__", AttributeType.GEOMETRY);
-        for (String key : keys.keySet()) {
-            getSchema().addAttribute(key, AttributeType.STRING);
-            setAttribute(key, keys.get(key));
-        }
+        this.attributes = new Object[keys.size() + 1];
+        this.getSchema().addAttribute("__GEOMETRY__", AttributeType.GEOMETRY);
+        keys.forEach((key, value) -> {
+            this.getSchema().addAttribute(key, AttributeType.STRING);
+            setAttribute(key, value);
+        });
+        final JTSConverter converter;
         if (jtsConverter != null)
             converter = jtsConverter;
         else
@@ -68 +70 @@
         // objects with the same id
         return (int) primitive.getUniqueId();
     }
+
+    @Override
+    public int compareTo(Feature abstractBasicFeature) {
+        // Rather unfortunately, we cannot implement the interface with OsmFeature
+        // So we are going to special case osm features, and compare ids.
+        // The super.compareTo only looks at geometry. If the geometry is the same, then it returns 0.
+        final int superCompare = super.compareTo(abstractBasicFeature);
+        if (superCompare == 0 && abstractBasicFeature instanceof OsmFeature) {
+            final OsmFeature other = (OsmFeature) abstractBasicFeature;
+            return this.primitive.compareTo(other.primitive);
+        }
+        return superCompare;
+    }
+
+    @Override
+    public boolean equals(Object other) {
+        return other != null && this.getClass().equals(other.getClass())
+                && Objects.equals(((OsmFeature) other).primitive, this.primitive);
+    }
+
+    @Override
+    public int hashCode() {
+        // No superclasses implement hashCode
+        return this.primitive.hashCode();
+    }
 }

test/unit/com/vividsolutions/jcs/conflate/polygonmatch/WeightedMatcherTest.java

@@ +1 @@
+package com.vividsolutions.jcs.conflate.polygonmatch;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+
+import java.util.Arrays;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.openstreetmap.josm.data.coor.LatLon;
+import org.openstreetmap.josm.data.osm.Node;
+import org.openstreetmap.josm.plugins.conflation.OsmFeature;
+import org.openstreetmap.josm.plugins.jts.JTSConverter;
+import org.openstreetmap.josm.testutils.JOSMTestRules;
+import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
+
+import com.vividsolutions.jump.feature.Feature;
+import com.vividsolutions.jump.feature.FeatureCollection;
+import com.vividsolutions.jump.feature.FeatureDataset;
+import com.vividsolutions.jump.feature.FeatureSchema;
+import com.vividsolutions.jump.feature.IndexedFeatureCollection;
+
+/**
+ * Test class for {@link WeightedMatcher}
+ * @author Taylor Smock
+ */
+@BasicPreferences
+class WeightedMatcherTest {
+    @RegisterExtension
+    JOSMTestRules josmTestRules = new JOSMTestRules().projection();
+    /**
+     * Non-regression test for JOSM #21788
+     * This occurred when two {@link org.openstreetmap.josm.plugins.conflation.OsmFeature} objects had {@link Comparable}
+     * equality.
+     */
+    @Test
+    void testNonRegression21788() {
+        final Node target = new Node();
+        target.setCoor(LatLon.ZERO);
+        final Node node1 = new Node(1, 1);
+        node1.setCoor(LatLon.ZERO);
+        final Node node2 = new Node(2, 1);
+        node2.setCoor(LatLon.ZERO);
+        final JTSConverter converter = new JTSConverter(true);
+        final WeightedMatcher weightedMatcher = new WeightedMatcher(1, new UnityMatcher());
+        assertDoesNotThrow(() -> weightedMatcher.match(new OsmFeature(target, converter),
+                new FeatureDataset(Arrays.asList(new OsmFeature(node1, converter), new OsmFeature(node2, converter)), new FeatureSchema())));
+    }
+
+    /**
+     * This matcher always gives a score of 1. This is to make it easier for getting out of bounds.
+     */
+    private static final class UnityMatcher implements FeatureMatcher {
+        @Override
+        public Matches match(Feature target, FeatureCollection candidates) {
+            final Matches matches = new Matches(target.getSchema());
+            candidates.forEach(feature -> matches.add(feature, 1));
+            return matches;
+        }
+    }
+}

test/unit/org/openstreetmap/josm/plugins/conflation/OsmFeatureTest.java

@@ +1 @@
+package org.openstreetmap.josm.plugins.conflation;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.Arrays;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.openstreetmap.josm.data.coor.LatLon;
+import org.openstreetmap.josm.data.osm.Node;
+import org.openstreetmap.josm.data.osm.OsmPrimitive;
+import org.openstreetmap.josm.plugins.jts.JTSConverter;
+import org.openstreetmap.josm.testutils.JOSMTestRules;
+import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
+
+import com.vividsolutions.jump.feature.AttributeType;
+import com.vividsolutions.jump.feature.FeatureSchema;
+import nl.jqno.equalsverifier.EqualsVerifier;
+
+/**
+ * Test class for {@link OsmFeature}
+ * @author Taylor Smock
+ */
+@BasicPreferences
+class OsmFeatureTest {
+    @RegisterExtension
+    JOSMTestRules josmTestRules = new JOSMTestRules().projection();
+    /**
+     * This checks that two osm features (points) do not match if they have the same geometry but different ids.
+     * This is a partial non-regression test for JOSM #21788. The root of the issue is that TreeMap uses the Comparable
+     * interface in put operations, and the {@link Comparable} interface specifies that {@code 0} implies equality.
+     */
+    @Test
+    void testCompareTo() {
+        final JTSConverter converter = new JTSConverter(true);
+
+        final Node node1 = new Node(1, 1);
+        final Node node2 = new Node(2, 1);
+        // This is deliberate -- we want to make certain that different equal objects work (i.e., no one accidentally
+        // uses == ).
+        final Node node3 = new Node(1, 1);
+        Arrays.asList(node1, node2, node3).forEach(node -> node.setCoor(LatLon.ZERO));
+
+        final OsmFeature osmFeature1 = new OsmFeature(node1, converter);
+        final OsmFeature osmFeature2 = new OsmFeature(node2, converter);
+        final OsmFeature osmFeature3 = new OsmFeature(node3, converter);
+
+        assertEquals(osmFeature1, osmFeature3, "The two nodes are equal");
+        assertNotEquals(osmFeature1, osmFeature2, "The two nodes are not equal");
+        assertEquals(0, osmFeature1.compareTo(osmFeature3));
+        assertEquals(0, osmFeature3.compareTo(osmFeature1));
+        assertNotEquals(0, osmFeature1.compareTo(osmFeature2));
+        assertNotEquals(0, osmFeature2.compareTo(osmFeature3));
+    }
+
+    @Test
+    void testEqualsContract() {
+        final Node redNode = new Node(1, 1);
+        final Node blueNode = new Node(2, 2);
+        redNode.setCoor(LatLon.NORTH_POLE);
+        blueNode.setCoor(LatLon.SOUTH_POLE);
+
+        final FeatureSchema redSchema = new FeatureSchema();
+        final FeatureSchema blueSchema = new FeatureSchema();
+        redSchema.addAttribute("red", AttributeType.STRING);
+        blueSchema.addAttribute("blue", AttributeType.STRING);
+        EqualsVerifier.forClass(OsmFeature.class)
+                .usingGetClass()
+                .withNonnullFields("primitive")
+                .withIgnoredFields("attributes" /* mutable */,
+                        "schema" /* mutable */,
+                        "id" /* not used in class */)
+                .withPrefabValues(OsmPrimitive.class, redNode, blueNode)
+                .withPrefabValues(FeatureSchema.class, redSchema, blueSchema)
+                .verify();
+    }
+}