Handle invalid serialized CheckFlags: solves osmlab#415 (osmlab#435)

* update waterAreaCheck and CheckFlagDeserializer * fix javadoc warning * add null check test Co-authored-by: Sean Coulter <[email protected]>
sayas01 · Dec 2, 2020 · b8e8502 · b8e8502
1 parent bffa74e
commit b8e8502
Show file tree

Hide file tree

Showing 6 changed files with 35 additions and 1 deletion.
diff --git a/src/main/java/org/openstreetmap/atlas/checks/flag/CheckFlag.java b/src/main/java/org/openstreetmap/atlas/checks/flag/CheckFlag.java
@@ -494,7 +494,7 @@ public Task getMapRouletteTask()
      * 
      * @param includeFixSuggestions
      *            true if we want to upload fix suggestions, false if not
-     * @return @return a {@link Task}
+     * @return a {@link Task}
      */
     public Task getMapRouletteTask(final boolean includeFixSuggestions)
     {

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
@@ -94,6 +94,12 @@ public CheckFlag deserialize(final JsonElement json, final Type typeOfT,
             final JsonDeserializationContext context)
     {
         final JsonObject full = json.getAsJsonObject();
+        if (full.get("type") == null)
+        {
+            // Consumers of this deserializer will have to handle this null result. Indicates the
+            // serialized flag is not a FeatureCollection, so it might not have flagged objects
+            return null;
+        }
         final JsonObject properties = full.get(PROPERTIES).getAsJsonObject();
         final String checkName = properties.get(GENERATOR).getAsString();
         // Split the instructions using the new line character and remove the prepended instruction

diff --git a/src/main/java/org/openstreetmap/atlas/checks/maproulette/MapRouletteUploadCommand.java b/src/main/java/org/openstreetmap/atlas/checks/maproulette/MapRouletteUploadCommand.java
@@ -136,6 +136,11 @@ protected void execute(final CommandMap commandMap,
                     {
                         final CheckFlag flagRecoveredFromLine = new CheckFlagDeserializer()
                                 .deserialize(new JsonParser().parse(line), null, null);
+                        if (flagRecoveredFromLine == null)
+                        {
+                            // an issue deserializing the flag
+                            return;
+                        }
                         final CheckFlag uploadFlag = OpenStreetMapCheckFlagConverter
                                 .openStreetMapify(flagRecoveredFromLine)
                                 .orElse(flagRecoveredFromLine);

diff --git a/src/main/java/org/openstreetmap/atlas/checks/validation/areas/WaterAreaCheck.java b/src/main/java/org/openstreetmap/atlas/checks/validation/areas/WaterAreaCheck.java
@@ -195,6 +195,7 @@ private CheckFlag checkForMissingWaterway(final CheckFlag flag, final Area area,
             returnFlag.addInstruction(this.getLocalizedInstruction(
                     FALLBACK_INSTRUCTIONS.indexOf(INSTRUCTION_MISSING_WATERWAY),
                     area.getOsmIdentifier()));
+            returnFlag.addObject(area);
         }
         return returnFlag;
     }
@@ -228,6 +229,10 @@ private CheckFlag checkForNoExitingWays(final CheckFlag flag, final Polygon area
                 {
                     returnFlag = new CheckFlag(this.getTaskIdentifier(area));
                 }
+                if (returnFlag.getFlaggedObjects().isEmpty())
+                {
+                    returnFlag.addObject(area);
+                }
                 returnFlag.addInstruction(this.getLocalizedInstruction(
                         FALLBACK_INSTRUCTIONS.indexOf(INSTRUCTION_NO_EXITING_WATERWAY),
                         area.getOsmIdentifier()));
@@ -277,6 +282,11 @@ && matchesFilter(this.waterwayCrossingIgnore, area)
             {
                 returnFlag = new CheckFlag(this.getTaskIdentifier(area));
             }
+            // At this point, the flag should hold the area or nothing
+            if (returnFlag.getFlaggedObjects().isEmpty())
+            {
+                returnFlag.addObject(area);
+            }
             returnFlag.addPoints(possibleAreaIntersections.stream()
                     .filter(pair -> !this.alreadyFlagged(pair.getRight())).map(Pair::getLeft)
                     .map(Segment::middle).collect(Collectors.toList()));
@@ -304,6 +314,12 @@ && matchesFilter(this.waterwayCrossingIgnore, area)
             {
                 returnFlag = new CheckFlag(this.getTaskIdentifier(area));
             }
+            // At this point, the flag should hold any one of: the area, the area and some other
+            // area(s), or nothing. So we just check for the last case
+            if (returnFlag.getFlaggedObjects().isEmpty())
+            {
+                returnFlag.addObject(area);
+            }
             returnFlag.addInstruction(this.getLocalizedInstruction(
                     FALLBACK_INSTRUCTIONS.indexOf(INSTRUCTION_WATERWAY_INTERSECTION),
                     area.getOsmIdentifier(),

diff --git a/src/test/java/org/openstreetmap/atlas/checks/maproulette/MapRouletteUploadCommandTest.java b/src/test/java/org/openstreetmap/atlas/checks/maproulette/MapRouletteUploadCommandTest.java
@@ -106,6 +106,7 @@ public void writeFiles()
             unzippedProcessor.process(this.setup.getTwoCountryFlag());
             unzippedProcessor.process(this.setup.getAnotherBasicFlag());
             unzippedProcessor.process(this.setup.getFlagSameCheck());
+            unzippedProcessor.process(this.setup.getFlagNoObjects());
             unzippedProcessor.process(new ShutdownEvent());
 
             // Create a zipped file

diff --git a/...est/java/org/openstreetmap/atlas/checks/maproulette/MapRouletteUploadCommandTestRule.java b/...est/java/org/openstreetmap/atlas/checks/maproulette/MapRouletteUploadCommandTestRule.java
@@ -22,6 +22,7 @@ public class MapRouletteUploadCommandTestRule extends CoreTestRule
     private static final String IDENTIFIER_2 = "2";
     private static final String IDENTIFIER_3 = "3";
     private static final String IDENTIFIER_4 = "4";
+    private static final String IDENTIFIER_5 = "5";
     private static final String CHALLENGE_1 = "SomeCheck";
     private static final String CHALLENGE_2 = "SomeOtherCheck";
     private static final String CHALLENGE_3 = "AnotherCheck";
@@ -41,6 +42,11 @@ public CheckFlagEvent getAnotherBasicFlag()
         return this.getBasicFlag(IDENTIFIER_2, this.basicAtlas.point(2L), CHALLENGE_2);
     }
 
+    public CheckFlagEvent getFlagNoObjects()
+    {
+        return this.getBasicFlag(IDENTIFIER_5, null, CHALLENGE_1);
+    }
+
     public CheckFlagEvent getFlagSameCheck()
     {
         return this.getBasicFlag(IDENTIFIER_4, this.basicAtlas.point(4L), CHALLENGE_1);