From 3e3d0fd82f348c5f90c16b32f6e2d1b4bcae3fcd Mon Sep 17 00:00:00 2001 From: mm-ciub <68378600+mm-ciub@users.noreply.github.com> Date: Thu, 28 Jan 2021 01:17:02 +0200 Subject: [PATCH] Added fix suggestion to RoadNameGapCheck (#475) * Added fix suggestion for RoadNameGapCheck * Fix suggestion: Store previous name in old_name tag * Complex fix suggestion changes - if bridge and no bridge:name, store old name value under it - otherwise store the old value under alt_name (with increased index if already existing eg. alt_name_1) * Add tunnel condition in fix suggestion Co-authored-by: atiannicelli * Added missing TunnelTag import * code smell Co-authored-by: atiannicelli Co-authored-by: Sean Coulter --- docs/checks/RoadNameGapCheck.md | 8 ++- .../validation/tag/RoadNameGapCheck.java | 65 ++++++++++++++++--- .../validation/tag/RoadNameGapCheckTest.java | 46 ++++++++++++- .../tag/RoadNameGapCheckTestRule.java | 56 ++++++++++++++++ 4 files changed, 163 insertions(+), 12 deletions(-) diff --git a/docs/checks/RoadNameGapCheck.md b/docs/checks/RoadNameGapCheck.md index 6ad44faef..04710a65d 100644 --- a/docs/checks/RoadNameGapCheck.md +++ b/docs/checks/RoadNameGapCheck.md @@ -14,14 +14,18 @@ i) This check should flag the edges who has NO name tag with below conditions: 1. TERTIARY_LINK, SECONDARY_LINK, PRIMARY_LINK, TRUNK_LINK, or MOTORWAY_LINK c. junction=ROUNDABOUT + For this case a fix suggestion to add the name tag with the same value as the connected edges is issued. + ii) This check should flag the edges which has different Name Tag: 1. Is an Edge 2. Has the Tag, 1. highway=TERTIARY, SECONDARY, PRIMARY, TRUNK, or MOTORWAY. 3. name=* but is not the same as the other Edges it is connected to 4. Does NOT have the Tag, - a. highway=*_LINK - b. junction=ROUNDABOUT + a. highway=*_LINK + b. junction=ROUNDABOUT + +For this case a fix suggestion to modify the name tag with the value from the connected edges is issued. ### Live Examples diff --git a/src/main/java/org/openstreetmap/atlas/checks/validation/tag/RoadNameGapCheck.java b/src/main/java/org/openstreetmap/atlas/checks/validation/tag/RoadNameGapCheck.java index 9683049b9..5ba0d1fac 100644 --- a/src/main/java/org/openstreetmap/atlas/checks/validation/tag/RoadNameGapCheck.java +++ b/src/main/java/org/openstreetmap/atlas/checks/validation/tag/RoadNameGapCheck.java @@ -7,12 +7,23 @@ import java.util.stream.Collectors; import org.openstreetmap.atlas.checks.base.BaseCheck; +import org.openstreetmap.atlas.checks.constants.CommonConstants; import org.openstreetmap.atlas.checks.flag.CheckFlag; +import org.openstreetmap.atlas.geography.atlas.change.FeatureChange; +import org.openstreetmap.atlas.geography.atlas.complete.CompleteEdge; +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.Edge; +import org.openstreetmap.atlas.tags.BridgeTag; import org.openstreetmap.atlas.tags.HighwayTag; import org.openstreetmap.atlas.tags.JunctionTag; +import org.openstreetmap.atlas.tags.TunnelTag; import org.openstreetmap.atlas.tags.annotations.validation.Validators; +import org.openstreetmap.atlas.tags.names.AlternativeNameTag; +import org.openstreetmap.atlas.tags.names.BridgeNameTag; +import org.openstreetmap.atlas.tags.names.NameTag; +import org.openstreetmap.atlas.tags.names.TunnelNameTag; import org.openstreetmap.atlas.utilities.configuration.Configuration; import org.openstreetmap.atlas.utilities.direction.EdgeDirectionComparator; @@ -21,6 +32,7 @@ * Tag, OR the Edge has a name Tag but does not equal the name Tag of the Edges that it is between. * * @author sugandhimaheshwaram + * @author mm-ciub */ public class RoadNameGapCheck extends BaseCheck { @@ -113,16 +125,23 @@ protected Optional flag(final AtlasObject object) outEdges); if (matchingInAndOutEdgeNames.isEmpty()) { - // There is no pair of inedge and out edge with same name. + // There is no pair of in edge and out edge with same name. return Optional.empty(); } - + final String nameSuggestion = matchingInAndOutEdgeNames.iterator().next(); // Create flag when we have in edge and out edge with same name but intermediate edge // doesn't have a name. - if (!edge.getName().isPresent()) + if (edge.getName().isEmpty()) { - return Optional.of(this.createFlag(object, - this.getLocalizedInstruction(0, edge.getOsmIdentifier()))); + return Optional + .of(this.createFlag(object, + this.getLocalizedInstruction(0, edge.getOsmIdentifier())) + .addFixSuggestion( + FeatureChange.add( + (AtlasEntity) ((CompleteEntity) CompleteEntity + .from((AtlasEntity) object)).withAddedTag( + NameTag.KEY, nameSuggestion), + object.getAtlas()))); } final Optional edgeName = edge.getName(); @@ -132,9 +151,12 @@ protected Optional flag(final AtlasObject object) .filter(this::validCheckForObject).collect(Collectors.toSet()); return this.findMatchingEdgeNameWithConnectedEdges(connectedEdges, edgeName.get()) ? Optional.empty() - : Optional.of(this.createFlag(object, this.getLocalizedInstruction(1, - edge.getOsmIdentifier(), edgeName.get()))); - + : Optional.of(this + .createFlag(object, + this.getLocalizedInstruction(1, edge.getOsmIdentifier(), + edgeName.get())) + .addFixSuggestion(this.getComplexFixSuggestion(object, edgeName.get(), + nameSuggestion))); } return Optional.empty(); } @@ -162,6 +184,33 @@ private boolean findMatchingEdgeNameWithConnectedEdges(final Set connected && connectedEdge.getName().get().equals(edgeName)); } + private FeatureChange getComplexFixSuggestion(final AtlasObject object, + final String originalName, final String nameSuggestion) + { + final CompleteEntity completeEntity = (CompleteEntity) CompleteEntity + .from((AtlasEntity) object); + completeEntity.withReplacedTag(NameTag.KEY, NameTag.KEY, nameSuggestion); + if (BridgeTag.isBridge(object) && object.getTag(BridgeNameTag.KEY).isEmpty()) + { + completeEntity.withAddedTag(BridgeNameTag.KEY, originalName); + } + else if (TunnelTag.isTunnel(object) && object.getTag(TunnelNameTag.KEY).isEmpty()) + { + completeEntity.withAddedTag(TunnelNameTag.KEY, originalName); + } + else + { + final long altNameCount = object.getOsmTags().keySet().stream() + .filter(key -> key.startsWith(AlternativeNameTag.KEY)).count(); + final String altNameTag = altNameCount > 0 + ? (AlternativeNameTag.KEY + CommonConstants.UNDERSCORE + altNameCount) + : AlternativeNameTag.KEY; + completeEntity.withAddedTag(altNameTag, originalName); + + } + return FeatureChange.add((AtlasEntity) completeEntity, object.getAtlas()); + } + /** * Find matching in and out edge names by comparing every name of the edge. * diff --git a/src/test/java/org/openstreetmap/atlas/checks/validation/tag/RoadNameGapCheckTest.java b/src/test/java/org/openstreetmap/atlas/checks/validation/tag/RoadNameGapCheckTest.java index 0798ffd0e..64175ee5b 100644 --- a/src/test/java/org/openstreetmap/atlas/checks/validation/tag/RoadNameGapCheckTest.java +++ b/src/test/java/org/openstreetmap/atlas/checks/validation/tag/RoadNameGapCheckTest.java @@ -5,6 +5,8 @@ import org.junit.Test; import org.openstreetmap.atlas.checks.configuration.ConfigurationResolver; import org.openstreetmap.atlas.checks.validation.verifier.ConsumerBasedExpectedCheckVerifier; +import org.openstreetmap.atlas.tags.names.AlternativeNameTag; +import org.openstreetmap.atlas.tags.names.BridgeNameTag; import org.openstreetmap.atlas.utilities.configuration.Configuration; /** @@ -23,12 +25,48 @@ public class RoadNameGapCheckTest private final Configuration inlineConfiguration = ConfigurationResolver.inlineConfiguration( "{\"RoadNameGapCheck\":{\"valid.highway.tag\":[\"primary\",\"tertiary\",\"trunk\",\"motorway\",\"secondary\"]}}"); + @Test + public void testForBridgeEdgeWithDifferentNameTag() + { + this.verifier.actual(this.setup.getBridgeEdge(), + new RoadNameGapCheck(this.inlineConfiguration)); + this.verifier.globallyVerify(flags -> + { + Assert.assertEquals(1, flags.size()); + Assert.assertEquals(1, flags.get(0).getFixSuggestions().size()); + flags.get(0).getFixSuggestions() + .forEach(s -> Assert.assertTrue(s.getTag(BridgeNameTag.KEY).isPresent())); + }); + } + + @Test + public void testForEdgeWithAltName() + { + this.verifier.actual(this.setup.getEdgeWithAltName(), + new RoadNameGapCheck(this.inlineConfiguration)); + this.verifier.globallyVerify(flags -> + { + Assert.assertEquals(1, flags.size()); + Assert.assertEquals(1, flags.get(0).getFixSuggestions().size()); + flags.get(0).getFixSuggestions() + .forEach(s -> Assert.assertTrue(s.getTag(AlternativeNameTag.KEY).isPresent())); + flags.get(0).getFixSuggestions().forEach( + s -> Assert.assertTrue(s.getTag(AlternativeNameTag.KEY + "_1").isPresent())); + }); + } + @Test public void testForEdgeWithDifferentNameTag() { this.verifier.actual(this.setup.getEdgeWithDifferentNameTag(), new RoadNameGapCheck(this.inlineConfiguration)); - this.verifier.globallyVerify(flags -> Assert.assertEquals(1, flags.size())); + this.verifier.globallyVerify(flags -> + { + Assert.assertEquals(1, flags.size()); + Assert.assertEquals(1, flags.get(0).getFixSuggestions().size()); + flags.get(0).getFixSuggestions() + .forEach(s -> Assert.assertTrue(s.getTag(AlternativeNameTag.KEY).isPresent())); + }); } @Test @@ -44,7 +82,11 @@ public void testForEdgeWithNoNameTag() { this.verifier.actual(this.setup.getEdgeWithNoNameTag(), new RoadNameGapCheck(this.inlineConfiguration)); - this.verifier.globallyVerify(flags -> Assert.assertEquals(1, flags.size())); + this.verifier.globallyVerify(flags -> + { + Assert.assertEquals(1, flags.size()); + Assert.assertEquals(1, flags.get(0).getFixSuggestions().size()); + }); } @Test diff --git a/src/test/java/org/openstreetmap/atlas/checks/validation/tag/RoadNameGapCheckTestRule.java b/src/test/java/org/openstreetmap/atlas/checks/validation/tag/RoadNameGapCheckTestRule.java index 08c214029..e8c92cc19 100644 --- a/src/test/java/org/openstreetmap/atlas/checks/validation/tag/RoadNameGapCheckTestRule.java +++ b/src/test/java/org/openstreetmap/atlas/checks/validation/tag/RoadNameGapCheckTestRule.java @@ -23,6 +23,52 @@ public class RoadNameGapCheckTestRule extends CoreTestRule private static final String TEST_7 = "9.8344387,56.0243522"; private static final String TEST_8 = "9.8343536,56.0243326"; + @TestAtlas( + // nodes + nodes = { @Node(coordinates = @Loc(value = TEST_1), id = "0"), + @Node(coordinates = @Loc(value = TEST_2), id = "1"), + @Node(coordinates = @Loc(value = TEST_3), id = "2"), + @Node(coordinates = @Loc(value = TEST_4), id = "3"), + @Node(coordinates = @Loc(value = TEST_5), id = "4"), + @Node(coordinates = @Loc(value = TEST_6), id = "5"), + @Node(coordinates = @Loc(value = TEST_7), id = "6"), + @Node(coordinates = @Loc(value = TEST_8), id = "7") }, + // edges + edges = { + @Edge(id = "1001000000", coordinates = { @Loc(value = TEST_5), + @Loc(value = TEST_6) }, tags = { "highway=primary", + "name=Tsing Long Highway" }), + @Edge(id = "1002000001", coordinates = { @Loc(value = TEST_6), + @Loc(value = TEST_7) }, tags = { "highway=primary", "name=failingName", + "bridge=yes" }), + @Edge(id = "1003000002", coordinates = { @Loc(value = TEST_7), + @Loc(value = TEST_8) }, tags = { "highway=primary", + "name=Tsing Long Highway" }) }) + private Atlas bridgeEdge; + + @TestAtlas( + // nodes + nodes = { @Node(coordinates = @Loc(value = TEST_1), id = "0"), + @Node(coordinates = @Loc(value = TEST_2), id = "1"), + @Node(coordinates = @Loc(value = TEST_3), id = "2"), + @Node(coordinates = @Loc(value = TEST_4), id = "3"), + @Node(coordinates = @Loc(value = TEST_5), id = "4"), + @Node(coordinates = @Loc(value = TEST_6), id = "5"), + @Node(coordinates = @Loc(value = TEST_7), id = "6"), + @Node(coordinates = @Loc(value = TEST_8), id = "7") }, + // edges + edges = { + @Edge(id = "1001000000", coordinates = { @Loc(value = TEST_5), + @Loc(value = TEST_6) }, tags = { "highway=primary", + "name=Tsing Long Highway" }), + @Edge(id = "1002000001", coordinates = { @Loc(value = TEST_6), + @Loc(value = TEST_7) }, tags = { "highway=primary", "name=failingName", + "alt_name=firstFailure" }), + @Edge(id = "1003000002", coordinates = { @Loc(value = TEST_7), + @Loc(value = TEST_8) }, tags = { "highway=primary", + "name=Tsing Long Highway" }) }) + private Atlas edgeWithAltName; + @TestAtlas( // nodes nodes = { @Node(coordinates = @TestAtlas.Loc(value = TEST_3), id = "2"), @@ -116,6 +162,16 @@ public class RoadNameGapCheckTestRule extends CoreTestRule "name=Tsing Long Highway" }) }) private Atlas edgeWithNoNameTag; + public Atlas getBridgeEdge() + { + return this.bridgeEdge; + } + + public Atlas getEdgeWithAltName() + { + return this.edgeWithAltName; + } + public Atlas getEdgeWithDifferentNameTag() { return this.edgeWithDifferentNameTag;