New check: Missing bridge structure (#341) (#342)

* New check: Missing bridge structure (#341)

* No duplicate flagging of bidirectional Ways

* Replacing Atlas ID in the violation info with OSM ID

* Updating documentation

* Adding MapRoulette challenge; minor refactoring to accommodate code review comments

* More refactoring after code review

* Reverting accidentally changed 'enabled' flags

* Fixing one more checkstyle problem

config/configuration.json

@@ -1058,5 +1058,15 @@
       "difficulty": "EASY",
       "defaultPriority": "MEDIUM"
     }
+  },
+  "BridgeDetailedInfoCheck": {
+    "bridge.length.minimum.meters": 500.0,
+    "challenge": {
+      "description": "Bridge is long enough to deserve a more detailed description than just bridge=yes",
+      "blurb": "Bridge without structure",
+      "instruction": "Open your favorite editor and add a more specific 'bridge=*' or 'bridge:structure=*' tag.",
+      "difficulty": "EASY",
+      "defaultPriority": "LOW"
+    }
   }
 }

docs/available_checks.md

@@ -68,6 +68,7 @@ This document is a list of tables with a description and link to documentation f
 | Check Name | Check Description |
 | :--------- | :---------------- |
 | AbbreviatedNameCheck |  The purpose of this check is to flag names that have abbreviations in them. |
+| [BridgeDetailedInfoCheck](checks/bridgeDetailedInfoCheck.md) | The purpose of this check is to identify prominent bridges with unspecified type or structure. |
 | [ConflictingAreaTagCombination](checks/conflictingAreaTagCombination.md) | The purpose of this check is to identify Areas with invalid tag combinations. |
 | ConflictingTagCombinationCheck | This check verifies whether an atlas object has a conflicting tag combination or not. |
 | [HighwayToFerryTagCheck](checks/highwayToFerryTagCheck.md) | The purpose of this check is to identify all Edges with route=FERRY and highway=PATH (or higher). |

docs/checks/bridgeDetailedInfoCheck.md

@@ -0,0 +1,25 @@
+# Bridge Detailed Info Check
+
+#### Description
+
+This check flags railway and major highway bridges longer than X meters (configurable) which only have a generic 'bridge=yes' tag without any details (bridge type or structure).
+
+The minimum bridge length to qualify for this check is configurable. The default is 500 meters.
+
+This is a port of Osmose check #7012.
+
+#### Live examples
+
+Way [id:4407849](https://www.openstreetmap.org/way/4407849) represents a prominent bridge - it is a section of a motorway and its length is greater than 500m - but it only has a generic `bridge=yes` tag.  
+
+#### Code review
+
+The validation section ensures that the Atlas object being evaluated is an [Edge](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/geography/atlas/items/Edge.java) with the following tags:
+* `bridge=yes`
+* `railway` -or- `highway` with one of the values: `motorway`, `trunk`, `primary`, `secondary`.
+Only _master_ Edges are valid candidates for this check, to avoid duplicate flags on a single bi-directional Way.
+
+The check section flags a valid candidate Edge if it is longer than the configurable minimum but does not have a `bridge:structure` tag.
+
+To learn more, please look at the source code for the check:
+[BridgeDetailedInfoCheck.java](../../src/main/java/org/openstreetmap/atlas/checks/validation/tag/BridgeDetailedInfoCheck.java)

src/main/java/org/openstreetmap/atlas/checks/validation/tag/BridgeDetailedInfoCheck.java

@@ -0,0 +1,95 @@
+package org.openstreetmap.atlas.checks.validation.tag;
+
+import java.util.Collections;
+import java.util.List;
+import java.util.Optional;
+import java.util.function.Predicate;
+
+import org.openstreetmap.atlas.checks.base.BaseCheck;
+import org.openstreetmap.atlas.checks.flag.CheckFlag;
+import org.openstreetmap.atlas.geography.atlas.items.AtlasObject;
+import org.openstreetmap.atlas.geography.atlas.items.Edge;
+import org.openstreetmap.atlas.tags.BridgeTag;
+import org.openstreetmap.atlas.tags.HighwayTag;
+import org.openstreetmap.atlas.tags.RailwayTag;
+import org.openstreetmap.atlas.tags.annotations.validation.Validators;
+import org.openstreetmap.atlas.utilities.configuration.Configuration;
+import org.openstreetmap.atlas.utilities.scalars.Distance;
+
+/**
+ * Flags railway and major highway bridges which are longer than configured minimum and have
+ * unspecified structure. This is a port of Osmose check 7012.
+ * 
+ * @author ladwlo
+ */
+public class BridgeDetailedInfoCheck extends BaseCheck<Long>
+{
+
+    private static final long serialVersionUID = -8915653487119336836L;
+
+    private static final Predicate<AtlasObject> IS_MAJOR_HIGHWAY = object -> Validators.isOfType(
+            object, HighwayTag.class, HighwayTag.MOTORWAY, HighwayTag.TRUNK, HighwayTag.PRIMARY,
+            HighwayTag.SECONDARY);
+    private static final Double MINIMUM_LENGTH = 500.0;
+    private static final String BRIDGE_STRUCTURE_TAG = "bridge:structure";
+    private static final List<String> FALLBACK_INSTRUCTIONS = Collections.singletonList(
+            "The length of this bridge (OSM ID: {0,number,#}) makes it deserve more details than just 'bridge=yes'. Add an appropriate 'bridge=*' or 'bridge:structure=*' tag.");
+    private final Distance minimumLength;
+
+    /**
+     * The default constructor that must be supplied. The Atlas Checks framework will generate the
+     * checks with this constructor, supplying a configuration that can be used to adjust any
+     * parameters that the check uses during operation.
+     *
+     * @param configuration
+     *            the JSON configuration for this check
+     */
+    public BridgeDetailedInfoCheck(final Configuration configuration)
+    {
+        super(configuration);
+        this.minimumLength = configurationValue(configuration, "bridge.length.minimum.meters",
+                MINIMUM_LENGTH, Distance::meters);
+    }
+
+    /**
+     * This function will validate if the supplied atlas object is valid for the check.
+     *
+     * @param object
+     *            the atlas object supplied by the Atlas-Checks framework for evaluation
+     * @return {@code true} if this object should be checked
+     */
+    @Override
+    public boolean validCheckForObject(final AtlasObject object)
+    {
+        // only master edges shall be flagged to avoid duplicate flags on the same OSM Way
+        return object instanceof Edge && ((Edge) object).isMasterEdge()
+                && Validators.isOfType(object, BridgeTag.class, BridgeTag.YES)
+                && (RailwayTag.isRailway(object) || IS_MAJOR_HIGHWAY.test(object));
+    }
+
+    /**
+     * This is the actual function that will check to see whether the object needs to be flagged.
+     *
+     * @param object
+     *            the atlas object supplied by the Atlas-Checks framework for evaluation
+     * @return an optional {@link CheckFlag} object that
+     */
+    @Override
+    protected Optional<CheckFlag> flag(final AtlasObject object)
+    {
+        final Optional<String> bridgeStructureTag = object.getTag(BRIDGE_STRUCTURE_TAG);
+        if (((Edge) object).length().isGreaterThan(this.minimumLength)
+                && bridgeStructureTag.isEmpty())
+        {
+            return Optional
+                    .of(createFlag(object, getLocalizedInstruction(0, object.getOsmIdentifier())));
+        }
+        return Optional.empty();
+    }
+
+    @Override
+    protected List<String> getFallbackInstructions()
+    {
+        return FALLBACK_INSTRUCTIONS;
+    }
+}

src/test/java/org/openstreetmap/atlas/checks/validation/tag/BridgeDetailedInfoCheckTest.java

@@ -0,0 +1,98 @@
+package org.openstreetmap.atlas.checks.validation.tag;
+
+import org.junit.Assert;
+import org.junit.Rule;
+import org.junit.Test;
+import org.openstreetmap.atlas.checks.configuration.ConfigurationResolver;
+import org.openstreetmap.atlas.checks.validation.verifier.ConsumerBasedExpectedCheckVerifier;
+import org.openstreetmap.atlas.utilities.configuration.Configuration;
+
+/**
+ * Tests for {@link BridgeDetailedInfoCheck}
+ *
+ * @author ladwlo
+ */
+public class BridgeDetailedInfoCheckTest
+{
+
+    @Rule
+    public BridgeDetailedInfoCheckTestRule setup = new BridgeDetailedInfoCheckTestRule();
+
+    @Rule
+    public ConsumerBasedExpectedCheckVerifier verifier = new ConsumerBasedExpectedCheckVerifier();
+
+    private final Configuration inlineConfiguration = ConfigurationResolver.inlineConfiguration(
+            "{\"BridgeDetailedInfoCheck\":{\"bridge.length.minimum.meters\":500.0}}");
+
+    @Test
+    public void longEdgeThatIsNotABridgeIsIgnored()
+    {
+        this.verifier.actual(this.setup.getLongEdgeThatIsNotABridge(),
+                new BridgeDetailedInfoCheck(this.inlineConfiguration));
+        this.verifier.globallyVerify(flags -> Assert.assertEquals(0, flags.size()));
+    }
+
+    @Test
+    public void longGenericMajorHighwayBridgeIsFlagged()
+    {
+        this.verifier.actual(this.setup.longGenericMajorHighwayBridge(),
+                new BridgeDetailedInfoCheck(this.inlineConfiguration));
+        this.verifier.globallyVerify(flags -> Assert.assertEquals(1, flags.size()));
+    }
+
+    @Test
+    public void longGenericMinorHighwayBridgeIsIgnored()
+    {
+        this.verifier.actual(this.setup.longGenericMinorHighwayBridge(),
+                new BridgeDetailedInfoCheck(this.inlineConfiguration));
+        this.verifier.globallyVerify(flags -> Assert.assertEquals(0, flags.size()));
+    }
+
+    @Test
+    public void longGenericNonRailwayOrHighwayBridgeIsIgnored()
+    {
+        this.verifier.actual(this.setup.longGenericBridge(),
+                new BridgeDetailedInfoCheck(this.inlineConfiguration));
+        this.verifier.globallyVerify(flags -> Assert.assertEquals(0, flags.size()));
+    }
+
+    @Test
+    public void longGenericRailwayBridgeIsFlagged()
+    {
+        this.verifier.actual(this.setup.longGenericRailwayBridge(),
+                new BridgeDetailedInfoCheck(this.inlineConfiguration));
+        this.verifier.globallyVerify(flags -> Assert.assertEquals(1, flags.size()));
+    }
+
+    @Test
+    public void longRailwayBridgeWithStructureIsAccepted()
+    {
+        this.verifier.actual(this.setup.longRailwayBridgeWithStructure(),
+                new BridgeDetailedInfoCheck(this.inlineConfiguration));
+        this.verifier.globallyVerify(flags -> Assert.assertEquals(0, flags.size()));
+    }
+
+    @Test
+    public void longRailwayBridgeWithTypeIsAccepted()
+    {
+        this.verifier.actual(this.setup.longRailwayBridgeWithType(),
+                new BridgeDetailedInfoCheck(this.inlineConfiguration));
+        this.verifier.globallyVerify(flags -> Assert.assertEquals(0, flags.size()));
+    }
+
+    @Test
+    public void masterAndReversedEdgesAreOnlyFlaggedOnce()
+    {
+        this.verifier.actual(this.setup.masterAndReversedEdges(),
+                new BridgeDetailedInfoCheck(this.inlineConfiguration));
+        this.verifier.globallyVerify(flags -> Assert.assertEquals(1, flags.size()));
+    }
+
+    @Test
+    public void shortGenericHighwayBridgeIsIgnored()
+    {
+        this.verifier.actual(this.setup.shortGenericHighwayBridge(),
+                new BridgeDetailedInfoCheck(this.inlineConfiguration));
+        this.verifier.globallyVerify(flags -> Assert.assertEquals(0, flags.size()));
+    }
+}