WaterWayCheck: Add autofix suggestion (geometry) (osmlab#477)

* MapRoulette: Allow upload of OSC to MapRoulette Signed-off-by: Taylor Smock <[email protected]> * WaterWayCheck: Add autofix suggestion (geometry) Signed-off-by: Taylor Smock <[email protected]>
martapako · Dec 8, 2021 · eb41b22 · eb41b22
1 parent 8632e73
commit eb41b22
Show file tree

Hide file tree

Showing 14 changed files with 584 additions and 21 deletions.
diff --git a/build.gradle b/build.gradle
@@ -101,6 +101,7 @@ dependencies
 
     // Support Junit 5 tests
     testImplementation packages.junit.api
+    testImplementation packages.junit.params
     testRuntimeOnly packages.junit.engine
     // Support JUnit 3/4 tests
     testCompileOnly packages.junit.junit4

diff --git a/dependencies.gradle b/dependencies.gradle
@@ -30,6 +30,7 @@ project.ext.packages = [
         vintage: "org.junit.vintage:junit-vintage-engine:${versions.junit}",
         api: "org.junit.jupiter:junit-jupiter-api:${versions.junit}",
         engine: "org.junit.jupiter:junit-jupiter-engine:${versions.junit}",
+        params: "org.junit.jupiter:junit-jupiter-params:${versions.junit}",
     ],
     sqlite: "org.xerial:sqlite-jdbc:${versions.sqlite}",
     log4j: "log4j:log4j:${versions.log4j}"

diff --git a/src/main/java/org/openstreetmap/atlas/checks/flag/serializer/CheckFlagDeserializer.java b/src/main/java/org/openstreetmap/atlas/checks/flag/serializer/CheckFlagDeserializer.java
@@ -354,11 +354,16 @@ private Optional<FeatureChange> getFixSuggestion(final AtlasEntity beforeView,
         {
             return Optional.empty();
         }
+        // Get the OSC if present
+        final Optional<String> osc = Optional.ofNullable(fixSuggestion.get("osc"))
+                .map(JsonElement::getAsString);
         // Create a remove FeatureChange from a shallow copy of the before view
         if (ChangeDescriptorType.REMOVE.equals(suggestionType))
         {
-            return Optional.of(new FeatureChange(ChangeType.REMOVE,
-                    CompleteEntity.shallowFrom(beforeView), beforeView));
+            final FeatureChange featureChange = new FeatureChange(ChangeType.REMOVE,
+                    CompleteEntity.shallowFrom(beforeView), beforeView);
+            osc.ifPresent(featureChange::withOsc);
+            return Optional.of(featureChange);
         }
 
         // Create a full copy for the after view
@@ -402,6 +407,9 @@ private Optional<FeatureChange> getFixSuggestion(final AtlasEntity beforeView,
             this.applyGeometryChanges(afterView, geometryChangeDescriptors);
         }
 
-        return Optional.of(new FeatureChange(ChangeType.ADD, (AtlasEntity) afterView, beforeView));
+        final var featureChange = new FeatureChange(ChangeType.ADD, (AtlasEntity) afterView,
+                beforeView);
+        osc.ifPresent(featureChange::withOsc);
+        return Optional.of(featureChange);
     }
 }
diff --git a/src/main/java/org/openstreetmap/atlas/checks/maproulette/data/Task.java b/src/main/java/org/openstreetmap/atlas/checks/maproulette/data/Task.java
@@ -8,7 +8,7 @@
 import java.util.stream.Collectors;
 
 import org.apache.commons.codec.binary.StringUtils;
-import org.openstreetmap.atlas.checks.maproulette.data.cooperative_challenge.TagChangeOperation;
+import org.openstreetmap.atlas.checks.maproulette.data.cooperative_challenge.CooperativeChallengeOperation;
 import org.openstreetmap.atlas.exception.CoreException;
 import org.openstreetmap.atlas.geography.Location;
 import org.openstreetmap.atlas.geography.atlas.change.FeatureChange;
@@ -73,7 +73,18 @@ private List<JsonObject> convert()
             for (final FeatureChange featureChange : this.featureChanges)
             {
                 final ChangeDescription whatChanged = featureChange.explain();
-                operationsList.add(new TagChangeOperation(whatChanged).create().getJson());
+                final Class<? extends CooperativeChallengeOperation> classType = CooperativeChallengeOperation
+                        .getAppropriateChangeOperation(whatChanged);
+                try
+                {
+                    operationsList.add(classType.getConstructor(ChangeDescription.class)
+                            .newInstance(whatChanged).create().getJson());
+                }
+                catch (final ReflectiveOperationException reflectiveOperationException)
+                {
+                    throw new CoreException("Class {0} does not have an appropriate constructor",
+                            reflectiveOperationException, classType.getName());
+                }
             }
             // convert each featureChange to a cooperative work object
             return operationsList;
@@ -233,11 +244,29 @@ private JsonObject generateTaskCooperativeWork(final List<JsonObject> cooperativ
         final JsonObject cooperativeWorkObject = new JsonObject();
         final JsonObject meta = new JsonObject();
         meta.add("version", new JsonPrimitive(2));
-        meta.add("type", new JsonPrimitive(1));
+        final byte type;
+        if (cooperativeWork.size() == 1 && cooperativeWork.get(0).has("format"))
+        {
+            // This is an OSC cooperative work challenge type
+            type = 2;
+        }
+        else
+        {
+            // Default to the tag fix cooperative work challenge type
+            type = 1;
+        }
+        meta.add("type", new JsonPrimitive(type));
         cooperativeWorkObject.add("meta", meta);
-        final JsonArray cooperativeWorkArray = new JsonArray();
-        cooperativeWork.forEach(cooperativeWorkArray::add);
-        cooperativeWorkObject.add("operations", cooperativeWorkArray);
+        if (type == 2)
+        {
+            cooperativeWorkObject.add("file", cooperativeWork.get(0));
+        }
+        else
+        {
+            final var cooperativeWorkArray = new JsonArray();
+            cooperativeWork.forEach(cooperativeWorkArray::add);
+            cooperativeWorkObject.add("operations", cooperativeWorkArray);
+        }
         return cooperativeWorkObject;
     }
 

diff --git a/...ap/atlas/checks/maproulette/data/cooperative_challenge/CooperativeChallengeOperation.java b/...ap/atlas/checks/maproulette/data/cooperative_challenge/CooperativeChallengeOperation.java
@@ -4,6 +4,7 @@
 import java.util.function.Predicate;
 import java.util.stream.Collectors;
 
+import org.openstreetmap.atlas.exception.CoreException;
 import org.openstreetmap.atlas.geography.atlas.change.description.ChangeDescription;
 import org.openstreetmap.atlas.geography.atlas.change.description.ChangeDescriptorType;
 import org.openstreetmap.atlas.geography.atlas.change.description.descriptors.ChangeDescriptor;
@@ -13,24 +14,46 @@
 
 /***
  * This represents an operation that is embedded in a cooperative challenge. Example: Given a
- * FeatureCollection geojson uploaded to MapRoulette:
- * 
+ * FeatureCollection geojson uploaded to MapRoulette: <br>
+ * For Tag changes:
+ *
  * <pre>
  * {
  *   "type": "FeatureCollection",
  *   "features": [ ... ],            // omitted for readability
- *   "cooperativeWork": {            // special `cooperativeWork` property
+ *   "cooperativeWork": {            // special `cooperativeWork` property
  *     "meta": {
- *       "version": 2,               // must be format version `2`
- *       "type": 1                   // `1` for tag fix type
+ *       "version": 2,               // must be format version `2`
+ *       "type": 1                   // `1` for tag fix type
  *     },
  *     "operations": [               // Operations section (see below)
  *       ...
  *     ]
  *   }
  * }
  * </pre>
- * 
+ *
+ * For geometry changes:
+ *
+ * <pre>
+ * {
+ *   "type": "FeatureCollection",
+ *   "features": [ ... ],            // omitted for readability
+ *   "cooperativeWork": {            // special `cooperativeWork` property
+ *     "meta": {
+ *       "version": 2,               // must be format version `2`
+ *       "type": 2                   // `2` for change file type
+ *     },
+ *     "file": {                     // Operations section (see below)
+ *       "type": "xml",              // only xml is supported at this time
+ *       "format": "osc",            // only osc is supported at this time
+ *       "encoding": "base64",       // only base64 is supported at this time
+ *       "content": "..."            // The base64 encoded OSC file
+ *     }
+ *   }
+ * }
+ * </pre>
+ *
  * This class is an abstract representation of the json material to be found under "operations". Its
  * subclasses should handle the various ChangeDescriptor types, including GeometryChangeDescriptor,
  * LongElementChangeDescriptor, RelationMemberChangeDescriptor, TagChangeDescriptor
@@ -51,6 +74,35 @@ public abstract class CooperativeChallengeOperation
     private static final String DELIMITER = "/";
     private static final int ATLAS_SECTIONING_IDENTIFIER_LENGTH = 6;
 
+    /**
+     * Get the appropriate change operation class
+     *
+     * @param changeDescription
+     *            The change descriptor
+     * @return A class appropriate for the change descriptor
+     */
+    public static Class<? extends CooperativeChallengeOperation> getAppropriateChangeOperation(
+            final ChangeDescription changeDescription)
+    {
+        for (final ChangeDescriptor changeDescriptor : changeDescription.getChangeDescriptors())
+        {
+            switch (changeDescriptor.getName())
+            {
+                case GEOMETRY:
+                    // GEOMETRY requires OSC, which covers just about everything else.
+                    return GeometryChangeOperation.class;
+                case TAG:
+                    break;
+                default:
+                    throw new CoreException(
+                            "No cooperative challenge converter is available for {0}",
+                            changeDescriptor.getName());
+            }
+        }
+        return changeDescription.getOsc().isPresent() ? GeometryChangeOperation.class
+                : TagChangeOperation.class;
+    }
+
     /**
      * Return the string representation of the OSM data type best representing the itemType
      *
@@ -114,5 +166,4 @@ protected void setJson(final JsonObject json)
     {
         this.json = json;
     }
-
 }
diff --git a/...treetmap/atlas/checks/maproulette/data/cooperative_challenge/GeometryChangeOperation.java b/...treetmap/atlas/checks/maproulette/data/cooperative_challenge/GeometryChangeOperation.java
@@ -0,0 +1,69 @@
+package org.openstreetmap.atlas.checks.maproulette.data.cooperative_challenge;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.function.Predicate;
+
+import org.openstreetmap.atlas.geography.atlas.change.description.ChangeDescription;
+import org.openstreetmap.atlas.geography.atlas.change.description.descriptors.ChangeDescriptor;
+import org.openstreetmap.atlas.geography.atlas.change.description.descriptors.ChangeDescriptorName;
+
+import com.google.gson.JsonObject;
+import com.google.gson.JsonPrimitive;
+
+/**
+ * Support geometry change operations
+ *
+ * @author Taylor Smock
+ */
+public class GeometryChangeOperation extends CooperativeChallengeOperation
+{
+    private static final String CONTENT = "content";
+    private static final Map<String, String> FILE_MAP = new HashMap<>(3);
+    static
+    {
+        FILE_MAP.put("type", "xml");
+        FILE_MAP.put("format", "osc");
+        FILE_MAP.put("encoding", "base64");
+    }
+
+    /** The OSC for the geometry change */
+    private final String osc;
+
+    public GeometryChangeOperation(final ChangeDescription changeDescription)
+    {
+        super(changeDescription);
+        final var optionalOsc = changeDescription.getOsc();
+        if (optionalOsc.isPresent())
+        {
+            this.osc = optionalOsc.get();
+        }
+        else if (changeDescription.toJsonElement().getAsJsonObject().has("osc"))
+        {
+            this.osc = changeDescription.toJsonElement().getAsJsonObject().get("osc").getAsString();
+        }
+        else
+        {
+            this.osc = null;
+        }
+    }
+
+    @Override
+    public GeometryChangeOperation create()
+    {
+        final var json = new JsonObject();
+        if (this.osc != null && !this.osc.isBlank())
+        {
+            FILE_MAP.forEach((key, value) -> json.add(key, new JsonPrimitive(value)));
+            json.add(CONTENT, new JsonPrimitive(this.osc));
+        }
+        this.setJson(json);
+        return this;
+    }
+
+    @Override
+    protected Predicate<ChangeDescriptor> operationFilter()
+    {
+        return changeDescriptor -> changeDescriptor.getName() == ChangeDescriptorName.GEOMETRY;
+    }
+}
diff --git a/src/main/java/org/openstreetmap/atlas/checks/utility/OpenStreetMapCheckFlagConverter.java b/src/main/java/org/openstreetmap/atlas/checks/utility/OpenStreetMapCheckFlagConverter.java
@@ -128,6 +128,8 @@ private static void convertFixSuggestions(final Map<String, Set<FeatureChange>>
                     final Optional<PolyLine> concatenatedAfterPolyline = entry.getValue().stream()
                             .map(FeatureChange::getAfterView)
                             .sorted(Comparator.comparing(AtlasEntity::getIdentifier))
+                            // Removed edges may not have a polyline
+                            .filter(entity -> ((Edge) entity).asPolyLine() != null)
                             .map(entity -> ((Edge) entity).asPolyLine()).reduce(PolyLine::append);
                     if (concatenatedBeforePolyline.isPresent()
                             && concatenatedAfterPolyline.isPresent())

diff --git a/src/main/java/org/openstreetmap/atlas/checks/validation/linear/lines/WaterWayCheck.java b/src/main/java/org/openstreetmap/atlas/checks/validation/linear/lines/WaterWayCheck.java
@@ -23,6 +23,9 @@
 import org.openstreetmap.atlas.geography.PolyLine;
 import org.openstreetmap.atlas.geography.Segment;
 import org.openstreetmap.atlas.geography.atlas.Atlas;
+import org.openstreetmap.atlas.geography.atlas.change.FeatureChange;
+import org.openstreetmap.atlas.geography.atlas.complete.CompleteEntity;
+import org.openstreetmap.atlas.geography.atlas.items.AtlasEntity;
 import org.openstreetmap.atlas.geography.atlas.items.AtlasObject;
 import org.openstreetmap.atlas.geography.atlas.items.LineItem;
 import org.openstreetmap.atlas.geography.atlas.items.LocationItem;
@@ -494,15 +497,26 @@ private CheckFlag flagIncline(final CheckFlag flag, final LineItem line, final L
         if (uphill && this.minResolutionDistance
                 .isGreaterThanOrEqualTo(this.elevationUtils.getResolution(first)))
         {
-            final CheckFlag returnFlag = flag;
             final String instruction = this.getLocalizedInstruction(
                     FALLBACK_INSTRUCTIONS.indexOf(GOES_UPHILL), line.getOsmIdentifier(),
                     this.elevationUtils.getResolution(first).asMeters());
-            if (returnFlag == null)
+            final CheckFlag returnFlag;
+            if (flag == null)
+            {
+                returnFlag = this.createFlag(line, instruction);
+            }
+            else
             {
-                return this.createFlag(line, instruction);
+                returnFlag = flag;
+                returnFlag.addInstruction(instruction);
             }
-            returnFlag.addInstruction(instruction);
+            final CompleteEntity<?> entity = (CompleteEntity<?>) CompleteEntity.shallowFrom(line);
+            final List<Location> points = Iterables.stream(line).collectToList();
+            Collections.reverse(points);
+            entity.withGeometry(points);
+            final var featureChange = FeatureChange.add((AtlasEntity) entity, line.getAtlas(),
+                    FeatureChange.Options.OSC_IF_POSSIBLE);
+            returnFlag.addFixSuggestion(featureChange);
             return returnFlag;
         }
         return flag;

diff --git a/src/test/java/org/openstreetmap/atlas/checks/flag/serializer/CheckFlagDeserializerTest.java b/src/test/java/org/openstreetmap/atlas/checks/flag/serializer/CheckFlagDeserializerTest.java
@@ -3,6 +3,7 @@
 import java.io.BufferedReader;
 import java.io.FileReader;
 import java.io.IOException;
+import java.lang.reflect.Field;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.Date;
@@ -24,6 +25,7 @@
 import org.openstreetmap.atlas.geography.atlas.change.FeatureChange;
 import org.openstreetmap.atlas.geography.atlas.complete.CompleteArea;
 import org.openstreetmap.atlas.geography.atlas.complete.CompleteEdge;
+import org.openstreetmap.atlas.geography.atlas.complete.CompleteLine;
 import org.openstreetmap.atlas.geography.atlas.complete.CompleteNode;
 import org.openstreetmap.atlas.geography.atlas.complete.CompletePoint;
 import org.openstreetmap.atlas.geography.atlas.complete.CompleteRelation;
@@ -167,6 +169,36 @@ public void instructionsDeserializationTest() throws IOException
         Assert.assertNotEquals(0, instructions.length());
     }
 
+    @Test
+    public void oscDeserializationTest() throws ReflectiveOperationException
+    {
+        final Line line = this.rule.atlas().line(1000000L);
+        final List<Location> locations = Iterables.asList(line.asPolyLine());
+        Collections.reverse(locations);
+        final CheckFlag lineFlag = new CheckFlag("1000000", Collections.singleton(line),
+                Collections.singletonList("Instruction"), locations,
+                Collections.singleton(FeatureChange.add(
+                        (AtlasEntity) CompleteLine.from(line).withGeometry(locations),
+                        this.rule.atlas(), FeatureChange.Options.OSC_IF_POSSIBLE)));
+
+        final Map<String, String> contextualProperties = new HashMap<>();
+        contextualProperties.put("generator", "NullCheck");
+        contextualProperties.put("timestamp", new Date().toString());
+
+        final String serializedJson = CheckFlagEvent.flagToJson(lineFlag, contextualProperties)
+                .toString();
+        final CheckFlag deserializedFlag = gson.fromJson(
+                CheckFlagEvent.flagToJson(lineFlag, contextualProperties), CheckFlag.class);
+        final Field oscField = FeatureChange.class.getDeclaredField("osc");
+        Assert.assertTrue(oscField.trySetAccessible());
+        Assert.assertEquals(1, deserializedFlag.getFixSuggestions().size());
+        Assert.assertEquals(1, lineFlag.getFixSuggestions().size());
+        Assert.assertEquals(
+                lineFlag.getFixSuggestions().iterator().next().explain().toJsonElement()
+                        .getAsJsonObject().get("osc").getAsString(),
+                oscField.get(deserializedFlag.getFixSuggestions().iterator().next()));
+    }
+
     @Test
     public void pointDeserializationTest()
     {