From a8247f28b14b52b93722122b1f3acf9ce303497f Mon Sep 17 00:00:00 2001 From: Savannah Ostrowski Date: Wed, 2 May 2018 10:37:44 -0700 Subject: [PATCH] One Member Relation Check (#40) * Initial logic and unit tests for OMR * Add configuration, documentation * Add information to documentation * Added live examples of relations with one member to documentation * Update documentation; simplified code * Added functionality for Multipolygon * Update to comments * Added multipolygon unit tests * Update to add flagging to all relation entities + gradle formatting * Updates to documentation, and use Method Reference * Fix some small nits * Refactored to remove redundant work * Remove flagging * Handle multi-polygon relation --- config/configuration.json | 8 ++ docs/checks/oneMemberRelationCheck.md | 64 +++++++++ .../relations/OneMemberRelationCheck.java | 74 ++++++++++ .../relations/OneMemberRelationCheckTest.java | 60 ++++++++ .../OneMemberRelationCheckTestRule.java | 129 ++++++++++++++++++ 5 files changed, 335 insertions(+) create mode 100644 docs/checks/oneMemberRelationCheck.md create mode 100644 src/main/java/org/openstreetmap/atlas/checks/validation/relations/OneMemberRelationCheck.java create mode 100644 src/test/java/org/openstreetmap/atlas/checks/validation/relations/OneMemberRelationCheckTest.java create mode 100644 src/test/java/org/openstreetmap/atlas/checks/validation/relations/OneMemberRelationCheckTestRule.java diff --git a/config/configuration.json b/config/configuration.json index 0de4af325..222b079f6 100644 --- a/config/configuration.json +++ b/config/configuration.json @@ -116,6 +116,14 @@ "tags":"highway" } }, + "OneMemberRelationCheck": { + "challenge": { + "description": "Tasks containing relations with only one member.", + "blurb": "One Member Relations", + "instruction": "Correct the relation to add necessary members", + "difficulty": "HARD" + } + }, "RoundaboutClosedLoopCheck": { "challenge": { "description": "A roundabout should be formed by one-way edges with no dead-end nodes.", diff --git a/docs/checks/oneMemberRelationCheck.md b/docs/checks/oneMemberRelationCheck.md new file mode 100644 index 000000000..4f4b41490 --- /dev/null +++ b/docs/checks/oneMemberRelationCheck.md @@ -0,0 +1,64 @@ +# One Member Relation Check + +#### Description + +This check identifies relations that only contain one member. In OSM, a [Relation](https://wiki.openstreetmap.org/wiki/Elements#Relation) +is a multi-purpose data structure that documents a relationship between two or more data elements +(nodes, ways, and/or other relations). + +#### Live Example +The following examples illustrate two cases where a Relation contains only one member. +1) This relation [id:4646212](https://www.openstreetmap.org/relation/4646212) has only one member. +2) This relation [id:7691824](https://www.openstreetmap.org/relation/7691824) has only one member. + +#### 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 been flagged before + +```java + @Override + public boolean validCheckForObject(final AtlasObject object) + { + return object instanceof Relation; + } +``` + +After the preliminary filtering of features, we get the members of that relation. If the number of +members in the relation is 1, then we flag the relation as problematic. One thing to note is that +multi-polygon relations with one member are flagged with a more specific instruction than the +regular one member relation. + +```java + @Override + protected Optional flag(final AtlasObject object) + { + final Relation relation = (Relation) object; + final RelationMemberList members = relation.members(); + + // If the number of members in the relation is 1 + if (members.size() == 1) + { + // If the relation is a multi-polygon, + if (relation.isMultiPolygon()) { + return Optional.of(createFlag( + relation.members().stream().map(RelationMember::getEntity) + .collect(Collectors.toSet()), + this.getLocalizedInstruction(1, relation.getOsmIdentifier()))); + } + return Optional.of(createFlag( + relation.members().stream().map(RelationMember::getEntity) + .collect(Collectors.toSet()), + this.getLocalizedInstruction(0, relation.getOsmIdentifier()))); + } + return Optional.empty(); + } +``` + +To learn more about the code, please look at the comments in the source code for the check. +[OneMemberRelationCheck.java](../../src/main/java/org/openstreetmap/atlas/checks/validation/relations/OneMemberRelationCheck.java) \ No newline at end of file diff --git a/src/main/java/org/openstreetmap/atlas/checks/validation/relations/OneMemberRelationCheck.java b/src/main/java/org/openstreetmap/atlas/checks/validation/relations/OneMemberRelationCheck.java new file mode 100644 index 000000000..bd684fdcf --- /dev/null +++ b/src/main/java/org/openstreetmap/atlas/checks/validation/relations/OneMemberRelationCheck.java @@ -0,0 +1,74 @@ +package org.openstreetmap.atlas.checks.validation.relations; + +import java.util.Arrays; +import java.util.List; +import java.util.Optional; +import java.util.stream.Collectors; + +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.Relation; +import org.openstreetmap.atlas.geography.atlas.items.RelationMember; +import org.openstreetmap.atlas.geography.atlas.items.RelationMemberList; +import org.openstreetmap.atlas.utilities.configuration.Configuration; + +/** + * This check flags {@link Relation}s which contain only one member. + * + * @author savannahostrowski + */ +public class OneMemberRelationCheck extends BaseCheck +{ + public static final String OMR_INSTRUCTIONS = "This relation, {0,number,#}, contains only " + + "one member."; + + public static final String MULTIPOLYGON_OMR_INSTRUCTIONS = "This relation, {0,number,#}, contains only " + + "one member. Multi-polygon relations need multiple polygons."; + + private static final List FALLBACK_INSTRUCTIONS = Arrays.asList(OMR_INSTRUCTIONS, + MULTIPOLYGON_OMR_INSTRUCTIONS); + + @Override + protected List getFallbackInstructions() + { + return FALLBACK_INSTRUCTIONS; + } + + public OneMemberRelationCheck(final Configuration configuration) + { + super(configuration); + } + + @Override + public boolean validCheckForObject(final AtlasObject object) + { + return object instanceof Relation; + } + + @Override + protected Optional flag(final AtlasObject object) + { + final Relation relation = (Relation) object; + final RelationMemberList members = relation.members(); + + // If the number of members in the relation is 1 + if (members.size() == 1) + { + // If the relation is a multi-polygon, + if (relation.isMultiPolygon()) + { + return Optional.of(createFlag( + relation.members().stream().map(RelationMember::getEntity) + .collect(Collectors.toSet()), + this.getLocalizedInstruction(1, relation.getOsmIdentifier()))); + } + + return Optional.of(createFlag( + relation.members().stream().map(RelationMember::getEntity) + .collect(Collectors.toSet()), + this.getLocalizedInstruction(0, relation.getOsmIdentifier()))); + } + return Optional.empty(); + } +} diff --git a/src/test/java/org/openstreetmap/atlas/checks/validation/relations/OneMemberRelationCheckTest.java b/src/test/java/org/openstreetmap/atlas/checks/validation/relations/OneMemberRelationCheckTest.java new file mode 100644 index 000000000..4b57df57a --- /dev/null +++ b/src/test/java/org/openstreetmap/atlas/checks/validation/relations/OneMemberRelationCheckTest.java @@ -0,0 +1,60 @@ +package org.openstreetmap.atlas.checks.validation.relations; + +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; + +/** + * Tests for {@link OneMemberRelationCheck} + * + * @author savannahostrowski + */ +public class OneMemberRelationCheckTest +{ + private static final OneMemberRelationCheck check = new OneMemberRelationCheck( + ConfigurationResolver.emptyConfiguration()); + + @Rule + public OneMemberRelationCheckTestRule setup = new OneMemberRelationCheckTestRule(); + + @Rule + public ConsumerBasedExpectedCheckVerifier verifier = new ConsumerBasedExpectedCheckVerifier(); + + @Test + public void testValidRelation() + { + this.verifier.actual(this.setup.getValidRelation(), check); + this.verifier.globallyVerify(flags -> Assert.assertEquals(0, flags.size())); + } + + @Test + public void testOneMemberRelation() + { + this.verifier.actual(this.setup.getOneMemberRelation(), check); + this.verifier.globallyVerify(flags -> Assert.assertEquals(1, flags.size())); + } + + @Test + public void testOneMemberRelationMultipolygonInner() + { + this.verifier.actual(this.setup.getOneMemberRelationMultipolygonInner(), check); + this.verifier.globallyVerify(flags -> Assert.assertEquals(1, flags.size())); + } + + @Test + public void testOneMemberRelationMultipolygonOuter() + { + this.verifier.actual(this.setup.getOneMemberRelationMultipolygonOuter(), check); + this.verifier.globallyVerify(flags -> Assert.assertEquals(1, flags.size())); + } + + @Test + public void testValidRelationMultipolygon() + { + this.verifier.actual(this.setup.getValidRelationMultipolygon(), check); + this.verifier.globallyVerify(flags -> Assert.assertEquals(0, flags.size())); + } + +} diff --git a/src/test/java/org/openstreetmap/atlas/checks/validation/relations/OneMemberRelationCheckTestRule.java b/src/test/java/org/openstreetmap/atlas/checks/validation/relations/OneMemberRelationCheckTestRule.java new file mode 100644 index 000000000..e46fe06c6 --- /dev/null +++ b/src/test/java/org/openstreetmap/atlas/checks/validation/relations/OneMemberRelationCheckTestRule.java @@ -0,0 +1,129 @@ +package org.openstreetmap.atlas.checks.validation.relations; + +import org.openstreetmap.atlas.geography.atlas.Atlas; +import org.openstreetmap.atlas.tags.RelationTypeTag; +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 OneMemberRelationCheckTest} data generator + * + * @author savannahostrowski + */ +public class OneMemberRelationCheckTestRule 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 + nodes = { @Node(id = "1", coordinates = @Loc(value = ONE)), + @Node(id = "2", coordinates = @Loc(value = TWO)), + @Node(id = "3", coordinates = @Loc(value = THREE)) }, + // edges + edges = { + @Edge(id = "12", coordinates = { @Loc(value = ONE), + @Loc(value = TWO) }, tags = { "highway=road" }), + @Edge(id = "23", coordinates = { @Loc(value = TWO), + @Loc(value = THREE) }, tags = { "highway=road" }), + @Edge(id = "31", coordinates = { @Loc(value = THREE), + @Loc(value = ONE) }, tags = { "highway=road" }) }, + // relations + relations = { @Relation(id = "123", members = { + @Member(id = "12", type = "edge", role = RelationTypeTag.RESTRICTION_ROLE_FROM), + @Member(id = "2", type = "node", role = RelationTypeTag.RESTRICTION_ROLE_VIA), + @Member(id = "23", type = "edge", role = RelationTypeTag.RESTRICTION_ROLE_TO) }, tags = { + "restriction=no_u_turn" }) }) + private Atlas validRelation; + + @TestAtlas( + // nodes + nodes = { @Node(id = "1", coordinates = @Loc(value = ONE)), + @Node(id = "2", coordinates = @Loc(value = TWO)) }, + // edges + edges = { @Edge(id = "12", coordinates = { @Loc(value = ONE), + @Loc(value = TWO) }, tags = { "highway=road" }) }, + // relations + relations = { @Relation(id = "123", members = { + @Member(id = "12", type = "edge", role = RelationTypeTag.RESTRICTION_ROLE_FROM) }, tags = { + "restriction=no_u_turn" }) }) + private Atlas oneMemberRelation; + + @TestAtlas( + // nodes + nodes = { @Node(id = "1", coordinates = @Loc(value = ONE)), + @Node(id = "2", coordinates = @Loc(value = TWO)) }, + // edges + edges = { @Edge(id = "12", coordinates = { @Loc(value = ONE), + @Loc(value = TWO) }, tags = { "highway=road" }) }, + // relations + relations = { @Relation(id = "123", members = { + @Member(id = "12", type = "edge", role = RelationTypeTag.MULTIPOLYGON_ROLE_INNER) }, tags = { + "restriction=no_u_turn", "type=multipolygon" }) }) + private Atlas oneMemberRelationMultipolygonInner; + + @TestAtlas( + // nodes + nodes = { @Node(id = "1", coordinates = @Loc(value = ONE)), + @Node(id = "2", coordinates = @Loc(value = TWO)) }, + // edges + edges = { @Edge(id = "12", coordinates = { @Loc(value = ONE), + @Loc(value = TWO) }, tags = { "highway=road" }) }, + // relations + relations = { @Relation(id = "123", members = { + @Member(id = "12", type = "edge", role = RelationTypeTag.MULTIPOLYGON_ROLE_OUTER) }, tags = { + "restriction=no_u_turn", "type=multipolygon" }) }) + private Atlas oneMemberRelationMultipolygonOuter; + + @TestAtlas( + // nodes + nodes = { @Node(id = "1", coordinates = @Loc(value = ONE)), + @Node(id = "2", coordinates = @Loc(value = TWO)), + @Node(id = "3", coordinates = @Loc(value = THREE)) }, + // edges + edges = { + @Edge(id = "12", coordinates = { @Loc(value = ONE), + @Loc(value = TWO) }, tags = { "highway=road" }), + @Edge(id = "23", coordinates = { @Loc(value = TWO), + @Loc(value = THREE) }, tags = { "highway=road" }), + @Edge(id = "31", coordinates = { @Loc(value = THREE), + @Loc(value = ONE) }, tags = { "highway=road" }) }, + // relations + relations = { @Relation(id = "123", members = { + @Member(id = "12", type = "edge", role = RelationTypeTag.MULTIPOLYGON_ROLE_INNER), + @Member(id = "2", type = "node", role = RelationTypeTag.MULTIPOLYGON_ROLE_OUTER), + @Member(id = "23", type = "edge", role = RelationTypeTag.MULTIPOLYGON_ROLE_OUTER) }, tags = { + "restriction=no_u_turn", "type=multipolygon" }) }) + private Atlas validRelationMultipolygon; + + public Atlas getValidRelation() + { + return this.validRelation; + } + + public Atlas getOneMemberRelation() + { + return this.oneMemberRelation; + } + + public Atlas getOneMemberRelationMultipolygonInner() + { + return this.oneMemberRelationMultipolygonInner; + } + + public Atlas getOneMemberRelationMultipolygonOuter() + { + return this.oneMemberRelationMultipolygonOuter; + } + + public Atlas getValidRelationMultipolygon() + { + return this.validRelationMultipolygon; + } +}