From 64356583fb76c5ab7fac97bcb0da16a86d137667 Mon Sep 17 00:00:00 2001 From: Vladimir Lemberg <62575608+vladlemberg@users.noreply.github.com> Date: Wed, 30 Dec 2020 10:23:06 -0800 Subject: [PATCH] Missing relation type check (#455) * MissingRelationType new Atlas check. * added javadoc, fixed arrangement * LeftCurly fix --- config/configuration.json | 9 ++ docs/available_checks.md | 1 + docs/checks/missingRelationType.md | 26 +++++ .../relations/MissingRelationTypeCheck.java | 62 ++++++++++++ .../checks/utility/CommonMethodsTest.java | 20 +++- .../checks/utility/CommonMethodsTestRule.java | 42 ++++++-- .../MissingRelationTypeCheckTest.java | 56 +++++++++++ .../MissingRelationTypeCheckTestRule.java | 99 +++++++++++++++++++ 8 files changed, 306 insertions(+), 9 deletions(-) create mode 100644 docs/checks/missingRelationType.md create mode 100644 src/main/java/org/openstreetmap/atlas/checks/validation/relations/MissingRelationTypeCheck.java create mode 100644 src/test/java/org/openstreetmap/atlas/checks/validation/relations/MissingRelationTypeCheckTest.java create mode 100644 src/test/java/org/openstreetmap/atlas/checks/validation/relations/MissingRelationTypeCheckTestRule.java diff --git a/config/configuration.json b/config/configuration.json index 2727f51e7..3194dc236 100644 --- a/config/configuration.json +++ b/config/configuration.json @@ -612,6 +612,15 @@ "difficulty": "MEDIUM" } }, + "MissingRelationTypeCheck": { + "ignore.tags.filter":"disused:type->!&disabled:type->!", + "challenge": { + "description": "Tasks contain Relations with missing type tag.", + "blurb": "Relations with missing type tag.", + "instruction": "Open your favorite editor and add Relation type tag.", + "difficulty": "NORMAL" + } + }, "NodeValenceCheck": { "connections.maximum": 10, "challenge": { diff --git a/docs/available_checks.md b/docs/available_checks.md index c0d12a4a6..7d6a4bdea 100644 --- a/docs/available_checks.md +++ b/docs/available_checks.md @@ -64,6 +64,7 @@ This document is a list of tables with a description and link to documentation f | :--------- | :---------------- | | InvalidMultiPolygonRelationCheck | This check is designed to scan through MultiPolygon relations and flag them for invalid geometry. | | [InvalidTurnRestrictionCheck](checks/invalidTurnRestrictionCheck.md) | The purpose of this check is to identify invalid turn restrictions in OSM. Invalid turn restrictions occur in a variety of ways from invalid members, Edge geometry issues, not being routable, or wrong topology. | +| [MissingRelationType](checks/missingRelationType.md) | The purpose of this check is to identify Relations without relation type. | | InvalidSignBoardRelationCheck | Identifies signboard relations that do not meet all the requirements for a signboard relation. | | [OneMemberRelationCheck](checks/oneMemberRelationCheck.md) | The purpose of this check is to identify Relations in OSM which only have one Member. | | [OpenBoundaryCheck](checks/openBoundaryCheck.md) | This check attempts to check for Admin Boundary Relations that should be closed polygons but are not closed. | diff --git a/docs/checks/missingRelationType.md b/docs/checks/missingRelationType.md new file mode 100644 index 000000000..01283d8ce --- /dev/null +++ b/docs/checks/missingRelationType.md @@ -0,0 +1,26 @@ +# Missing Relation Type Check + +#### Description +This check identifies relations without relation type. In OSM, a [Relation](https://wiki.openstreetmap.org/wiki/Elements#Relation) +suppose to have one of proposed relation [Type](https://wiki.openstreetmap.org/wiki/Types_of_relation). + +#### Live Example +The following examples illustrate two cases where a Relation is missing relation type. +1) This relation [id:7508794](https://www.openstreetmap.org/relation/7508794) is missing "multipolygon" relation type. +2) This relation [id:9769731](https://www.openstreetmap.org/relation/9769731) is missing "boundary" relation type. + +#### Code Review +In [Atlas](https://github.com/osmlab/atlas), OSM elements are represented as Edges, Points, Lines, +Nodes & Relations; in our case, we’re working with [Relations](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/geography/atlas/items/Relation.java). + +Our first goal is to validate the incoming Atlas Object. We use some preliminary filtering to target +Relation objects. Therefore, we use: +* Must be a Relation +* Must not have a Relation Type tag +* Must not be a "One Member Relation" +* Must not have disused:type or disabled:type tags [reference](https://wiki.openstreetmap.org/wiki/Key:disused:) + +After the preliminary filtering, Relation will be flagged. + +To learn more about the code, please look at the comments in the source code for the check. +[MissingRelationTypeCheck.java](../../src/main/java/org/openstreetmap/atlas/checks/validation/relations/MissingRelationTypeCheck.java) \ No newline at end of file diff --git a/src/main/java/org/openstreetmap/atlas/checks/validation/relations/MissingRelationTypeCheck.java b/src/main/java/org/openstreetmap/atlas/checks/validation/relations/MissingRelationTypeCheck.java new file mode 100644 index 000000000..fc4ed8095 --- /dev/null +++ b/src/main/java/org/openstreetmap/atlas/checks/validation/relations/MissingRelationTypeCheck.java @@ -0,0 +1,62 @@ +package org.openstreetmap.atlas.checks.validation.relations; + +import java.util.Collections; +import java.util.List; +import java.util.Optional; + +import org.openstreetmap.atlas.checks.base.BaseCheck; +import org.openstreetmap.atlas.checks.flag.CheckFlag; +import org.openstreetmap.atlas.checks.utility.CommonMethods; +import org.openstreetmap.atlas.geography.atlas.items.AtlasObject; +import org.openstreetmap.atlas.geography.atlas.items.Relation; +import org.openstreetmap.atlas.tags.RelationTypeTag; +import org.openstreetmap.atlas.tags.filters.TaggableFilter; +import org.openstreetmap.atlas.utilities.configuration.Configuration; + +/** + * This check flags {@link Relation}s with missing Type tag. Refer to: + * https://wiki.openstreetmap.org/wiki/Types_of_relation + * + * @author Vladimir Lemberg + */ +public class MissingRelationTypeCheck extends BaseCheck +{ + public static final String MISSING_TYPE_INSTRUCTIONS = "This relation, {0,number,#}, is missing type tag"; + private static final List FALLBACK_INSTRUCTIONS = Collections + .singletonList(MISSING_TYPE_INSTRUCTIONS); + private static final String TAG_FILTER_IGNORE_DEFAULT = "disused:type->!&disabled:type->!"; + private static final long serialVersionUID = 5171171744111206430L; + private final TaggableFilter tagFilterIgnore; + + public MissingRelationTypeCheck(final Configuration configuration) + { + super(configuration); + this.tagFilterIgnore = this.configurationValue(configuration, "ignore.tags.filter", + TAG_FILTER_IGNORE_DEFAULT, TaggableFilter::forDefinition); + } + + @Override + public boolean validCheckForObject(final AtlasObject object) + { + return object instanceof Relation + // Missing relation Type tag + && object.getTag(RelationTypeTag.KEY).isEmpty() + // Is not "One Member Relation" + && CommonMethods.getOSMRelationMemberSize((Relation) object) > 1 + // Relation is not disused or disabled + && this.tagFilterIgnore.test(object); + } + + @Override + protected Optional flag(final AtlasObject object) + { + return Optional.of(this.createFlag(object, + this.getLocalizedInstruction(0, object.getOsmIdentifier()))); + } + + @Override + protected List getFallbackInstructions() + { + return FALLBACK_INSTRUCTIONS; + } +} diff --git a/src/test/java/org/openstreetmap/atlas/checks/utility/CommonMethodsTest.java b/src/test/java/org/openstreetmap/atlas/checks/utility/CommonMethodsTest.java index b27b50080..5c4978917 100644 --- a/src/test/java/org/openstreetmap/atlas/checks/utility/CommonMethodsTest.java +++ b/src/test/java/org/openstreetmap/atlas/checks/utility/CommonMethodsTest.java @@ -18,17 +18,31 @@ public class CommonMethodsTest public CommonMethodsTestRule setup = new CommonMethodsTestRule(); @Test - public void testOneMemberRelationReversed() + public void testOneMemberRelationEdge() + { + assertEquals(1, CommonMethods + .getOSMRelationMemberSize(this.setup.getOneMemberRelationEdge().relation(123))); + } + + @Test + public void testOneMemberRelationNode() { assertEquals(1, CommonMethods - .getOSMRelationMemberSize(this.setup.getOneMemberRelationReversed().relation(123))); + .getOSMRelationMemberSize(this.setup.getOneMemberRelationNode().relation(123))); + } + + @Test + public void testOneMemberRelationReversed() + { + assertEquals(1, CommonMethods.getOSMRelationMemberSize( + this.setup.getOneMemberRelationReversedEdge().relation(123))); } @Test public void testOneMemberRelationSectioned() { assertEquals(1, CommonMethods.getOSMRelationMemberSize( - this.setup.getOneMemberRelationSectioned().relation(123))); + this.setup.getOneMemberRelationSectionedEdge().relation(123))); } @Test diff --git a/src/test/java/org/openstreetmap/atlas/checks/utility/CommonMethodsTestRule.java b/src/test/java/org/openstreetmap/atlas/checks/utility/CommonMethodsTestRule.java index a37ffbb35..abc272a2d 100644 --- a/src/test/java/org/openstreetmap/atlas/checks/utility/CommonMethodsTestRule.java +++ b/src/test/java/org/openstreetmap/atlas/checks/utility/CommonMethodsTestRule.java @@ -41,6 +41,26 @@ public class CommonMethodsTestRule extends CoreTestRule @Member(id = "23000001", type = "edge", role = "") }) }) private Atlas validRelation; + @TestAtlas( + // nodes + nodes = { @Node(id = "1", coordinates = @Loc(value = ONE)), + @Node(id = "2", coordinates = @Loc(value = TWO)) }, + // edges + edges = { @Edge(id = "12000001", coordinates = { @Loc(value = ONE), + @Loc(value = TWO) }) }, + // relations + relations = { @Relation(id = "123", members = { + @Member(id = "12000001", type = "edge", role = ""), }) }) + private Atlas oneMemberRelationEdge; + + @TestAtlas( + // nodes + nodes = { @Node(id = "1", coordinates = @Loc(value = ONE)) }, + // relations + relations = { @Relation(id = "123", members = { + @Member(id = "1", type = "node", role = ""), }) }) + private Atlas oneMemberRelationNode; + @TestAtlas( // nodes nodes = { @Node(id = "1", coordinates = @Loc(value = ONE)), @@ -54,7 +74,7 @@ public class CommonMethodsTestRule extends CoreTestRule relations = { @Relation(id = "123", members = { @Member(id = "12000001", type = "edge", role = ""), @Member(id = "-12000001", type = "edge", role = "") }) }) - private Atlas oneMemberRelationReversed; + private Atlas oneMemberRelationReversedEdge; @TestAtlas( // nodes @@ -73,16 +93,26 @@ public class CommonMethodsTestRule extends CoreTestRule @Member(id = "12000001", type = "edge", role = ""), @Member(id = "12000002", type = "edge", role = ""), @Member(id = "12000003", type = "edge", role = "") }) }) - private Atlas oneMemberRelationSectioned; + private Atlas oneMemberRelationSectionedEdge; + + public Atlas getOneMemberRelationEdge() + { + return this.oneMemberRelationEdge; + } + + public Atlas getOneMemberRelationNode() + { + return this.oneMemberRelationNode; + } - public Atlas getOneMemberRelationReversed() + public Atlas getOneMemberRelationReversedEdge() { - return this.oneMemberRelationReversed; + return this.oneMemberRelationReversedEdge; } - public Atlas getOneMemberRelationSectioned() + public Atlas getOneMemberRelationSectionedEdge() { - return this.oneMemberRelationSectioned; + return this.oneMemberRelationSectionedEdge; } public Atlas getValidRelation() diff --git a/src/test/java/org/openstreetmap/atlas/checks/validation/relations/MissingRelationTypeCheckTest.java b/src/test/java/org/openstreetmap/atlas/checks/validation/relations/MissingRelationTypeCheckTest.java new file mode 100644 index 000000000..db2d49000 --- /dev/null +++ b/src/test/java/org/openstreetmap/atlas/checks/validation/relations/MissingRelationTypeCheckTest.java @@ -0,0 +1,56 @@ +package org.openstreetmap.atlas.checks.validation.relations; + +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 MissingRelationTypeCheck} + * + * @author vlemberg + */ +public class MissingRelationTypeCheckTest +{ + @Rule + public final MissingRelationTypeCheckTestRule setup = new MissingRelationTypeCheckTestRule(); + + @Rule + public final ConsumerBasedExpectedCheckVerifier verifier = new ConsumerBasedExpectedCheckVerifier(); + + private final Configuration inlineConfiguration = ConfigurationResolver.inlineConfiguration( + "{\"MissingRelationTypeCheck\":{\"ignore.tags.filter\":\"disused:type->!&disabled:type->!\"}}"); + + @Test + public void missingRelationType() + { + this.verifier.actual(this.setup.getMissingRelationType(), + new MissingRelationTypeCheck(this.inlineConfiguration)); + this.verifier.verifyExpectedSize(1); + } + + @Test + public void missingRelationTypeDisabled() + { + this.verifier.actual(this.setup.getMissingRelationTypeDisabled(), + new MissingRelationTypeCheck(this.inlineConfiguration)); + this.verifier.verifyEmpty(); + } + + @Test + public void missingRelationTypeDisused() + { + this.verifier.actual(this.setup.getMissingRelationTypeDisused(), + new MissingRelationTypeCheck(this.inlineConfiguration)); + this.verifier.verifyEmpty(); + } + + @Test + public void validRelation() + { + this.verifier.actual(this.setup.getValidRelation(), + new MissingRelationTypeCheck(this.inlineConfiguration)); + this.verifier.verifyEmpty(); + } +} diff --git a/src/test/java/org/openstreetmap/atlas/checks/validation/relations/MissingRelationTypeCheckTestRule.java b/src/test/java/org/openstreetmap/atlas/checks/validation/relations/MissingRelationTypeCheckTestRule.java new file mode 100644 index 000000000..08ed7dbc3 --- /dev/null +++ b/src/test/java/org/openstreetmap/atlas/checks/validation/relations/MissingRelationTypeCheckTestRule.java @@ -0,0 +1,99 @@ +package org.openstreetmap.atlas.checks.validation.relations; + +import org.openstreetmap.atlas.geography.atlas.Atlas; +import org.openstreetmap.atlas.utilities.testing.CoreTestRule; +import org.openstreetmap.atlas.utilities.testing.TestAtlas; +import org.openstreetmap.atlas.utilities.testing.TestAtlas.Edge; +import org.openstreetmap.atlas.utilities.testing.TestAtlas.Loc; +import org.openstreetmap.atlas.utilities.testing.TestAtlas.Node; +import org.openstreetmap.atlas.utilities.testing.TestAtlas.Relation; +import org.openstreetmap.atlas.utilities.testing.TestAtlas.Relation.Member; + +/** + * {@link MissingRelationTypeCheckTest} data generator + * + * @author vlemberg + */ +public class MissingRelationTypeCheckTestRule extends CoreTestRule +{ + private static final String ONE = "18.4360044, -71.7194204"; + private static final String TWO = "18.4360737, -71.6970306"; + private static final String THREE = "18.4273807, -71.7052283"; + @TestAtlas(nodes = { @Node(id = "1", coordinates = @Loc(value = ONE)), + @Node(id = "2", coordinates = @Loc(value = TWO)), + @Node(id = "3", coordinates = @Loc(value = THREE)) }, edges = { + @Edge(id = "12", coordinates = { @Loc(value = ONE), @Loc(value = TWO) }), + @Edge(id = "23", coordinates = { @Loc(value = TWO), @Loc(value = THREE) }), + @Edge(id = "31", coordinates = { @Loc(value = THREE), + @Loc(value = ONE) }) }, relations = { + @Relation(id = "123", members = { + @Member(id = "12", type = "edge", role = "any"), + @Member(id = "2", type = "node", role = "any"), + @Member(id = "23", type = "edge", role = "any") }, tags = { + "type=any" }) }) + private Atlas validRelation; + + @TestAtlas(nodes = { @Node(id = "1", coordinates = @Loc(value = ONE)), + @Node(id = "2", coordinates = @Loc(value = TWO)), + @Node(id = "3", coordinates = @Loc(value = THREE)) }, edges = { + @Edge(id = "12", coordinates = { @Loc(value = ONE), @Loc(value = TWO) }), + @Edge(id = "23", coordinates = { @Loc(value = TWO), @Loc(value = THREE) }), + @Edge(id = "31", coordinates = { @Loc(value = THREE), + @Loc(value = ONE) }) }, relations = { + @Relation(id = "123", members = { + @Member(id = "12", type = "edge", role = "any"), + @Member(id = "2", type = "node", role = "any"), + @Member(id = "23", type = "edge", role = "any") }) }) + private Atlas missingRelationType; + + @TestAtlas(nodes = { @Node(id = "1", coordinates = @Loc(value = ONE)), + @Node(id = "2", coordinates = @Loc(value = TWO)), + @Node(id = "3", coordinates = @Loc(value = THREE)) }, edges = { + @Edge(id = "12", coordinates = { @Loc(value = ONE), @Loc(value = TWO) }), + @TestAtlas.Edge(id = "23", coordinates = { @Loc(value = TWO), + @Loc(value = THREE) }), + @TestAtlas.Edge(id = "31", coordinates = { @Loc(value = THREE), + @Loc(value = ONE) }) }, relations = { + @Relation(id = "123", members = { + @Member(id = "12", type = "edge", role = "any"), + @Member(id = "2", type = "node", role = "any"), + @Member(id = "23", type = "edge", role = "any") }, tags = { + "disused:type=any" }) }) + private Atlas missingRelationTypeDisused; + + @TestAtlas(nodes = { @Node(id = "1", coordinates = @Loc(value = ONE)), + @Node(id = "2", coordinates = @Loc(value = TWO)), + @Node(id = "3", coordinates = @Loc(value = THREE)) }, edges = { + @TestAtlas.Edge(id = "12", coordinates = { @Loc(value = ONE), + @Loc(value = TWO) }), + @TestAtlas.Edge(id = "23", coordinates = { @Loc(value = TWO), + @Loc(value = THREE) }), + @TestAtlas.Edge(id = "31", coordinates = { @Loc(value = THREE), + @Loc(value = ONE) }) }, relations = { + @Relation(id = "123", members = { + @Member(id = "12", type = "edge", role = "any"), + @Member(id = "2", type = "node", role = "any"), + @Member(id = "23", type = "edge", role = "any") }, tags = { + "disabled:type=any" }) }) + private Atlas missingRelationTypeDisabled; + + public Atlas getMissingRelationType() + { + return this.missingRelationType; + } + + public Atlas getMissingRelationTypeDisabled() + { + return this.missingRelationTypeDisabled; + } + + public Atlas getMissingRelationTypeDisused() + { + return this.missingRelationTypeDisused; + } + + public Atlas getValidRelation() + { + return this.validRelation; + } +}