diff --git a/config/configuration.json b/config/configuration.json index cf3801939..8826bce59 100644 --- a/config/configuration.json +++ b/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" + } } } diff --git a/docs/available_checks.md b/docs/available_checks.md index 727e8e16f..8f87edebf 100644 --- a/docs/available_checks.md +++ b/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). | diff --git a/docs/checks/bridgeDetailedInfoCheck.md b/docs/checks/bridgeDetailedInfoCheck.md new file mode 100644 index 000000000..d05aa871d --- /dev/null +++ b/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) diff --git a/src/main/java/org/openstreetmap/atlas/checks/validation/tag/BridgeDetailedInfoCheck.java b/src/main/java/org/openstreetmap/atlas/checks/validation/tag/BridgeDetailedInfoCheck.java new file mode 100644 index 000000000..c3824b279 --- /dev/null +++ b/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 +{ + + private static final long serialVersionUID = -8915653487119336836L; + + private static final Predicate 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 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 flag(final AtlasObject object) + { + final Optional 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 getFallbackInstructions() + { + return FALLBACK_INSTRUCTIONS; + } +} diff --git a/src/test/java/org/openstreetmap/atlas/checks/validation/tag/BridgeDetailedInfoCheckTest.java b/src/test/java/org/openstreetmap/atlas/checks/validation/tag/BridgeDetailedInfoCheckTest.java new file mode 100644 index 000000000..eabbcd5e1 --- /dev/null +++ b/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())); + } +} diff --git a/src/test/java/org/openstreetmap/atlas/checks/validation/tag/BridgeDetailedInfoCheckTestRule.java b/src/test/java/org/openstreetmap/atlas/checks/validation/tag/BridgeDetailedInfoCheckTestRule.java new file mode 100644 index 000000000..e63c5c14f --- /dev/null +++ b/src/test/java/org/openstreetmap/atlas/checks/validation/tag/BridgeDetailedInfoCheckTestRule.java @@ -0,0 +1,155 @@ +package org.openstreetmap.atlas.checks.validation.tag; + +import static org.openstreetmap.atlas.utilities.testing.TestAtlas.Edge; +import static org.openstreetmap.atlas.utilities.testing.TestAtlas.Loc; +import static org.openstreetmap.atlas.utilities.testing.TestAtlas.Node; + +import org.openstreetmap.atlas.geography.atlas.Atlas; +import org.openstreetmap.atlas.utilities.testing.CoreTestRule; +import org.openstreetmap.atlas.utilities.testing.TestAtlas; + +/** + * Test atlases for {@link BridgeDetailedInfoCheckTest} + * + * @author ladwlo + */ +public class BridgeDetailedInfoCheckTestRule extends CoreTestRule +{ + + // short bridge + private static final String LOC_1 = "47.222,-122.444"; + private static final String LOC_2 = "47.225,-122.441"; + // long bridge + private static final String LOC_3 = "47.111,-122.666"; + private static final String LOC_4 = "47.115,-122.661"; + + @TestAtlas( + // nodes + nodes = { @Node(coordinates = @Loc(value = LOC_3)), + @Node(coordinates = @Loc(value = LOC_4)) }, + // edges + edges = { @Edge(id = "1000000001", coordinates = { @Loc(value = LOC_3), + @Loc(value = LOC_4) }, tags = { "railway=rail" }) }) + private Atlas longEdgeThatIsNotABridge; + + @TestAtlas( + // nodes + nodes = { @Node(coordinates = @Loc(value = LOC_3)), + @Node(coordinates = @Loc(value = LOC_4)) }, + // edges + edges = { @Edge(id = "1000000001", coordinates = { @Loc(value = LOC_3), + @Loc(value = LOC_4) }, tags = { "bridge=yes" }) }) + private Atlas longGenericBridge; + + @TestAtlas( + // nodes + nodes = { @Node(coordinates = @Loc(value = LOC_3)), + @Node(coordinates = @Loc(value = LOC_4)) }, + // edges + edges = { @Edge(id = "1000000001", coordinates = { @Loc(value = LOC_3), + @Loc(value = LOC_4) }, tags = { "highway=motorway", "bridge=yes" }) }) + private Atlas longGenericMajorHighwayBridge; + + @TestAtlas( + // nodes + nodes = { @Node(coordinates = @Loc(value = LOC_3)), + @Node(coordinates = @Loc(value = LOC_4)) }, + // edges + edges = { @Edge(id = "1000000001", coordinates = { @Loc(value = LOC_3), + @Loc(value = LOC_4) }, tags = { "highway=tertiary", "bridge=yes" }) }) + private Atlas longGenericMinorHighwayBridge; + + @TestAtlas( + // nodes + nodes = { @Node(coordinates = @Loc(value = LOC_3)), + @Node(coordinates = @Loc(value = LOC_4)) }, + // edges + edges = { @Edge(id = "1000000001", coordinates = { @Loc(value = LOC_3), + @Loc(value = LOC_4) }, tags = { "railway=rail", "bridge=yes" }) }) + private Atlas longGenericRailwayBridge; + + @TestAtlas( + // nodes + nodes = { @Node(coordinates = @Loc(value = LOC_3)), + @Node(coordinates = @Loc(value = LOC_4)) }, + // edges + edges = { @Edge(id = "1000000001", coordinates = { @Loc(value = LOC_3), + @Loc(value = LOC_4) }, tags = { "highway=motorway", "bridge=yes", + "bridge:structure=arch" }) }) + private Atlas longRailwayBridgeWithStructure; + + @TestAtlas( + // nodes + nodes = { @Node(coordinates = @Loc(value = LOC_3)), + @Node(coordinates = @Loc(value = LOC_4)) }, + // edges + edges = { @Edge(id = "1000000001", coordinates = { @Loc(value = LOC_3), + @Loc(value = LOC_4) }, tags = { "railway=rail", "bridge=cantilever" }) }) + private Atlas longRailwayBridgeWithType; + + @TestAtlas( + // nodes + nodes = { @Node(coordinates = @Loc(value = LOC_3)), + @Node(coordinates = @Loc(value = LOC_4)) }, + // edges + edges = { + @Edge(id = "1000000001", coordinates = { @Loc(value = LOC_3), + @Loc(value = LOC_4) }, tags = { "railway=rail", "bridge=yes" }), + @Edge(id = "-1000000001", coordinates = { @Loc(value = LOC_4), + @Loc(value = LOC_3) }, tags = { "railway=rail", "bridge=yes" }) }) + private Atlas masterAndReversedEdges; + + @TestAtlas( + // nodes + nodes = { @Node(coordinates = @Loc(value = LOC_1)), + @Node(coordinates = @Loc(value = LOC_2)) }, + // edges + edges = { @Edge(id = "1000000001", coordinates = { @Loc(value = LOC_1), + @Loc(value = LOC_2) }, tags = { "highway=motorway", "bridge=yes" }) }) + private Atlas shortGenericHighwayBridge; + + public Atlas getLongEdgeThatIsNotABridge() + { + return this.longEdgeThatIsNotABridge; + } + + public Atlas longGenericBridge() + { + return this.longGenericBridge; + } + + public Atlas longGenericMajorHighwayBridge() + { + return this.longGenericMajorHighwayBridge; + } + + public Atlas longGenericMinorHighwayBridge() + { + return this.longGenericMinorHighwayBridge; + } + + public Atlas longGenericRailwayBridge() + { + return this.longGenericRailwayBridge; + } + + public Atlas longRailwayBridgeWithStructure() + { + return this.longRailwayBridgeWithStructure; + } + + public Atlas longRailwayBridgeWithType() + { + return this.longRailwayBridgeWithType; + } + + public Atlas masterAndReversedEdges() + { + return this.masterAndReversedEdges; + } + + public Atlas shortGenericHighwayBridge() + { + return this.shortGenericHighwayBridge; + } +}