diff --git a/config/configuration.json b/config/configuration.json index 5678cc7e9..2b64bed25 100644 --- a/config/configuration.json +++ b/config/configuration.json @@ -1029,5 +1029,14 @@ "instruction": "Open your favorite editor and edit the features overlapping the ocean so they either validly overlap or do not overlap.", "difficulty": "EASY" } + }, + "ConditionalRestrictionCheck": { + "challenge": { + "description": "The conditional restriction tag and value should follow a predefined scheme", + "blurb": "Improper conditional restriction tags", + "instruction": "Open your favorite editor and check the instruction for the task and then update the conditional tag to the appropriate scheme.", + "difficulty": "EASY", + "defaultPriority": "MEDIUM" + } } } diff --git a/docs/available_checks.md b/docs/available_checks.md index 8623e884f..cb566589f 100644 --- a/docs/available_checks.md +++ b/docs/available_checks.md @@ -81,6 +81,7 @@ This document is a list of tables with a description and link to documentation f | ShortNameCheck | The short name check will validate that any and all names contain at least 2 letters in the name. | | [StreetNameIntegersOnlyCheck](checks/streetNameIntegersOnlyCheck.md) | The purpose of this check is to identify streets whose names contain integers only. | | [UnusualLayerTagsCheck](checks/unusualLayerTagsCheck.md) | The purpose of this check is to identify layer tag values when accompanied by invalid tunnel and bridge tags. | +| [ConditionalRestrictionCheck](checks/conditionalRestrictionCheck.md) | The purpose of this check is to identify elements that have a :conditional tag that does not respect the established format. | ## Ways | Check Name | Check Description | diff --git a/docs/checks/conditionalRestrictionCheck.md b/docs/checks/conditionalRestrictionCheck.md new file mode 100644 index 000000000..21424dfa4 --- /dev/null +++ b/docs/checks/conditionalRestrictionCheck.md @@ -0,0 +1,32 @@ +# Conditional Restriction Check + +This check flags features that contain a `:conditional` tag that is not constructed following the rules set in the +OSM wiki: [Conditional restrictions](https://wiki.openstreetmap.org/wiki/Conditional_restrictions). + +#### Live Examples + +1. Way [id:19696701](https://www.openstreetmap.org/way/19696701) has the value `no (maxstay<3 hours)` which does not +respect the ` @ ` format. +2. Way [id:525881134](https://www.openstreetmap.org/way/525881134) has the key `psv:lanes:backward:conditional` which +does not respect the `[:][:]` format. + +#### Code Review + +In [Atlas](https://github.com/osmlab/atlas), OSM elements are represented as Edges, Points, Lines, Nodes, Areas & Relations; +in our case, we’re are looking at +[Points](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/geography/atlas/items/Point.java) and +[Edges](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/geography/atlas/items/Edge.java). + + This check verifies all elements that have at least one tag containing `:conditional`. For these cases it verifies that + the key follows either one of the two acceptable forms: + 1. `[:][:]:conditional` + 2. `[:]:conditional` - in case of restriction type as access. + + For each part there are lists of possible values that are compared with the checked element. + + The values are also verified to ensure they are following the ` @ [; @ ]` + format. In case of a restriction of type access, the value is also compared with a list of predefined values. + + To learn more about the code, please look at the comments in the source code for the check: + [ConditionalRestrictionCheck](../../src/main/java/org/openstreetmap/atlas/checks/validation/tag/ConditionalRestrictionCheck.java) + \ No newline at end of file diff --git a/src/main/java/org/openstreetmap/atlas/checks/validation/tag/ConditionalRestrictionCheck.java b/src/main/java/org/openstreetmap/atlas/checks/validation/tag/ConditionalRestrictionCheck.java new file mode 100644 index 000000000..097863376 --- /dev/null +++ b/src/main/java/org/openstreetmap/atlas/checks/validation/tag/ConditionalRestrictionCheck.java @@ -0,0 +1,224 @@ +package org.openstreetmap.atlas.checks.validation.tag; + +import static org.openstreetmap.atlas.checks.constants.CommonConstants.COLON; + +import java.util.Arrays; +import java.util.HashSet; +import java.util.List; +import java.util.Optional; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +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.utilities.configuration.Configuration; + +/** + * Flags conditional restriction tags that do not follow the scheme provided on the OSM wiki. + * {@literal [:][:]:conditional + * = @ [; @ ]} + * + * @author laura + * @see wiki for more + * information. + */ +public class ConditionalRestrictionCheck extends BaseCheck +{ + + private static final long serialVersionUID = 6726352951073801440L; + + public static final String CONDITIONAL = ":conditional"; + private static final List RESTRICTION_TYPES = List.of("access", "restriction", + "maxspeed", "minspeed", "maxweight", "maxaxleload", "maxheight", "maxlength", "maxstay", + "maxgcweight", "maxgcweightrating", "interval", "duration", "overtaking", "oneway", + "fee", "toll", "noexit", "snowplowing", "disabled", "lanes", "parking"); + private static final List TRANSPORTATION_MODE = List.of("foot", "ski", "inline_skates", + "horse", "vehicle", "bicycle", "carriage", "trailer", "caravan", "motor_vehicle", + "motorcycle", "moped", "mofa", "motorcar", "motorhome", "tourist_bus", "coach", "goods", + "hgv", "hgv_articulated", "agricultural", "golf_cart", "atv", "snowmobile", "psv", + "bus", "minibus", "share_taxi", "taxi", "hov", "hazmat", "emergency", "canoe", + "electric_vehicle", "cycleway", "busway"); + private static final List DIRECTION = List.of("forward", "backward", "left", "right", + "both"); + private static final List ACCESS_RESTRICTION_VALUE = List.of("yes", "no", "private", + "permissive", "destination", "delivery", "customers", "designated", "use_sidepath", + "dismount", "agricultural", "forestry", "discouraged", "official", "lane", + "share_busway", "opposite_share_busway"); + + private static final List FALLBACK_INSTRUCTIONS = Arrays.asList( + "The conditional key \"{0}\" does not respect the \"[:][:]:conditional\" format", + "The conditional value \"{0}\" does not respect the format \" @ [; @ ]\" ", + "The element with id {0,number,#} does not follow the conditional restriction pattern."); + private static final int TWO_PARTS = 2; + private static final int THREE_PARTS = 3; + private static final int FOUR_PARTS = 4; + + private static final Pattern VALUE_PATTERN = Pattern.compile( + "([a-zA-Z0-9_.-|\\s]*?)\\s@\\s(\\([^)\\s][^)]+?\\)|[^();\\s][^();]*)\\s*(;\\s*([^@\\s][^@]*?)\\s*@\\s*(\\([^)\\s][^)]+?\\)|[^();\\s][^();]*?)\\s*)*"); + + public ConditionalRestrictionCheck(final Configuration configuration) + { + super(configuration); + } + + @Override + public boolean validCheckForObject(final AtlasObject object) + { + // all elements that have a tag containing ":conditional" + return object.getOsmTags().keySet().stream().anyMatch(key -> key.contains(CONDITIONAL)); + } + + /** + * Checks if the conditional restrictions respects the format + * + * @param object + * the atlas object containing a conditional tag + * @return an optional {@link CheckFlag} object + */ + @Override + protected Optional flag(final AtlasObject object) + { + final Set instructions = new HashSet<>(); + + final List conditionalKeys = object.getOsmTags().keySet().stream() + .filter(key -> key.contains(CONDITIONAL)).collect(Collectors.toList()); + for (final String key : conditionalKeys) + { + if (!this.isKeyValid(key)) + { + instructions.add(this.getLocalizedInstruction(0, key)); + } + final String value = object.getOsmTags().get(key); + if (!this.isValueValid(value, key)) + { + instructions.add(this.getLocalizedInstruction(1, value)); + } + } + if (!instructions.isEmpty()) + { + final CheckFlag flag = this.createFlag(object, + this.getLocalizedInstruction(2, object.getOsmIdentifier())); + instructions.forEach(flag::addInstruction); + return Optional.of(flag); + } + return Optional.empty(); + } + + @Override + protected List getFallbackInstructions() + { + return FALLBACK_INSTRUCTIONS; + } + + private boolean containsTransportationMode(final String key) + { + final String[] parts = key.split(COLON); + for (final String part : parts) + { + if (this.isTransportationMode(part)) + { + return true; + } + } + return false; + } + + private boolean isAccessType(final String value) + { + return "access".equals(value); + } + + private boolean isAccessValue(final String value) + { + return ACCESS_RESTRICTION_VALUE.contains(value); + } + + private boolean isDirection(final String value) + { + return DIRECTION.contains(value); + } + + private boolean isKeyValid(final String key) + { + final String[] parts = key.split(COLON); + // access:lanes is a valid exception for lanes on second position + final boolean isAccessLanes = parts.length > 2 + && (this.isAccessType(parts[0]) && this.isLanes(parts[1])); + switch (parts.length) + { + // starts with 2 because they all contain the conditional part + case TWO_PARTS: + return this.isRestrictionType(parts[0]) || this.isTransportationMode(parts[0]); + case THREE_PARTS: + final boolean isRestrictionTypeFormat = this.isRestrictionType(parts[0]) + && (this.isTransportationMode(parts[1]) || this.isDirection(parts[1])); + final boolean isTransportTypeFormat = this.isTransportationMode(parts[0]) + && (this.isDirection(parts[1]) || this.isLanes(parts[1])); + return isRestrictionTypeFormat || isTransportTypeFormat || isAccessLanes; + case FOUR_PARTS: + final boolean isRestrictionTransport = this.isRestrictionType(parts[0]) + && this.isTransportationMode(parts[1]); + final boolean isTransportLanes = this.isTransportationMode(parts[0]) + && this.isLanes(parts[1]); + return (isRestrictionTransport || isTransportLanes || isAccessLanes) + && this.isDirection(parts[2]); + default: + return false; + } + } + + private boolean isLanes(final String value) + { + return "lanes".equals(value); + } + + private boolean isNotLanesType(final String key) + { + final String[] parts = key.split(COLON); + return !this.isLanes(parts[0]); + } + + private boolean isRestrictionType(final String value) + { + return RESTRICTION_TYPES.contains(value); + } + + private boolean isTransportationMode(final String value) + { + return TRANSPORTATION_MODE.contains(value); + } + + private boolean isValueValid(final String value, final String key) + { + final Matcher matcher = VALUE_PATTERN.matcher(value); + if (matcher.matches()) + { + if (this.containsTransportationMode(key) && this.isNotLanesType(key)) + { + final String[] parts = value.split("@"); + for (int i = 0; i < parts.length - 1; i += 2) + { + // the character | is used to display lanes and can accompany a access value in + // cases of lanes + final String[] subParts = parts[i].split("\\|"); + for (final String part : subParts) + { + final String trimmedPart = part.trim(); + if (!trimmedPart.isEmpty() && !this.isAccessValue(trimmedPart)) + { + return false; + } + } + } + } + } + else + { + return false; + } + return true; + } +} diff --git a/src/test/java/org/openstreetmap/atlas/checks/validation/tag/ConditionalRestrictionCheckTest.java b/src/test/java/org/openstreetmap/atlas/checks/validation/tag/ConditionalRestrictionCheckTest.java new file mode 100644 index 000000000..2dc2da637 --- /dev/null +++ b/src/test/java/org/openstreetmap/atlas/checks/validation/tag/ConditionalRestrictionCheckTest.java @@ -0,0 +1,88 @@ +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; + +/** + * @author laura on 09/06/2020. + */ +public class ConditionalRestrictionCheckTest +{ + + @Rule + public ConditionalRestrictionCheckTestRule setup = new ConditionalRestrictionCheckTestRule(); + + @Rule + public ConsumerBasedExpectedCheckVerifier verifier = new ConsumerBasedExpectedCheckVerifier(); + + @Test + public void accessLanesType() + { + this.verifier.actual(this.setup.getAccessLanes(), + new ConditionalRestrictionCheck(ConfigurationResolver.emptyConfiguration())); + this.verifier.globallyVerify(flags -> Assert.assertEquals(0, flags.size())); + } + + @Test + public void invalidAccessLanes() + { + this.verifier.actual(this.setup.getInvalidAccessLanes(), + new ConditionalRestrictionCheck(ConfigurationResolver.emptyConfiguration())); + this.verifier.globallyVerify(flags -> Assert.assertEquals(1, flags.size())); + this.verifier.verify(flag -> Assert.assertTrue(flag.getInstructions().contains( + "The conditional value \"1 @ (Mo-Fr 06:30-09:00,16:00-18:30)\" does not respect the format \" @ [; @ ]\" "))); + + } + + @Test + public void invalidAccessType() + { + this.verifier.actual(this.setup.getInvalidAccessType(), + new ConditionalRestrictionCheck(ConfigurationResolver.emptyConfiguration())); + this.verifier.globallyVerify(flags -> Assert.assertEquals(1, flags.size())); + this.verifier.verify(flag -> Assert.assertTrue(flag.getInstructions().contains( + "The conditional value \"notallowed @ (Sa 08:00-16:00)\" does not respect the format \" @ [; @ ]\" "))); + + } + + @Test + public void invalidConditionFormat() + { + this.verifier.actual(this.setup.getInvalidConditionFormat(), + new ConditionalRestrictionCheck(ConfigurationResolver.emptyConfiguration())); + this.verifier.globallyVerify(flags -> Assert.assertEquals(1, flags.size())); + this.verifier.verify(flag -> Assert.assertTrue(flag.getInstructions().contains( + "The conditional value \"no@(Sa 08:00-16:00)\" does not respect the format \" @ [; @ ]\" "))); + + } + + @Test + public void invalidConditionalKey() + { + this.verifier.actual(this.setup.getInvalidConditionalKey(), + new ConditionalRestrictionCheck(ConfigurationResolver.emptyConfiguration())); + this.verifier.globallyVerify(flags -> Assert.assertEquals(1, flags.size())); + this.verifier.verify(flag -> Assert.assertTrue(flag.getInstructions().contains( + "The conditional key \"hgv:maxweight:conditional\" does not respect the \"[:][:]:conditional\" format"))); + } + + @Test + public void reversedLanesTransportationMode() + { + this.verifier.actual(this.setup.getReversedLanesTransport(), + new ConditionalRestrictionCheck(ConfigurationResolver.emptyConfiguration())); + this.verifier.globallyVerify(flags -> Assert.assertEquals(0, flags.size())); + } + + @Test + public void validRestriction() + { + this.verifier.actual(this.setup.getConditionalWay(), + new ConditionalRestrictionCheck(ConfigurationResolver.emptyConfiguration())); + this.verifier.globallyVerify(flags -> Assert.assertEquals(0, flags.size())); + } + +} diff --git a/src/test/java/org/openstreetmap/atlas/checks/validation/tag/ConditionalRestrictionCheckTestRule.java b/src/test/java/org/openstreetmap/atlas/checks/validation/tag/ConditionalRestrictionCheckTestRule.java new file mode 100644 index 000000000..7a7e96cd3 --- /dev/null +++ b/src/test/java/org/openstreetmap/atlas/checks/validation/tag/ConditionalRestrictionCheckTestRule.java @@ -0,0 +1,109 @@ +package org.openstreetmap.atlas.checks.validation.tag; + +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; + +/** + * @author laura on 09/06/2020. + */ +public class ConditionalRestrictionCheckTestRule extends CoreTestRule +{ + + private static final String TEST_1 = "47.2136626201459,-122.443275382856"; + private static final String TEST_2 = "47.2138327316739,-122.44258668766"; + + @TestAtlas(nodes = { @Node(coordinates = @Loc(value = TEST_1)), + @Node(coordinates = @Loc(value = TEST_2)) }, edges = { + @Edge(id = "1001000001", coordinates = { @Loc(value = TEST_1), + @Loc(value = TEST_2) }, tags = { "highway=pedestrian", + "access:lanes:left:conditional=|yes|no|yes @ (Mo-Fr 06:00-11:00,17:00-19:00;Sa 03:30-19:00)", + "bicycle=yes", "bicycle:conditional=no @ (Sa 08:00-16:00)" }) }) + private Atlas accessLanes; + + @TestAtlas(nodes = { @Node(coordinates = @Loc(value = TEST_1)), + @Node(coordinates = @Loc(value = TEST_2)) }, edges = { + @Edge(id = "1001000001", coordinates = { @Loc(value = TEST_1), + @Loc(value = TEST_2) }, tags = { "highway=pedestrian", + "motor_vehicle:conditional=delivery @ (Mo-Fr 06:00-11:00,17:00-19:00;Sa 03:30-19:00)", + "bicycle=yes", "bicycle:conditional=no @ (Sa 08:00-16:00)" }) }) + private Atlas conditionalWay; + + @TestAtlas(nodes = { @Node(coordinates = @Loc(value = TEST_1)), + @Node(coordinates = @Loc(value = TEST_2)) }, edges = { + @Edge(id = "1001000001", coordinates = { @Loc(value = TEST_1), + @Loc(value = TEST_2) }, tags = { "highway=pedestrian", + "hgv:maxweight:conditional=none @ delivery" }) }) + private Atlas invalidConditionalKey; + + @TestAtlas(nodes = { @Node(coordinates = @Loc(value = TEST_1)), + @Node(coordinates = @Loc(value = TEST_2)) }, edges = { + @Edge(id = "1001000001", coordinates = { @Loc(value = TEST_1), + @Loc(value = TEST_2) }, tags = { "highway=pedestrian", + "motor_vehicle:conditional=delivery @ (Mo-Fr 06:00-11:00,17:00-19:00;Sa 03:30-19:00)", + "bicycle=yes", "bicycle:conditional=no@(Sa 08:00-16:00)" }) }) + private Atlas invalidConditionFormat; + + @TestAtlas(nodes = { @Node(coordinates = @Loc(value = TEST_1)), + @Node(coordinates = @Loc(value = TEST_2)) }, edges = { + @Edge(id = "1001000001", coordinates = { @Loc(value = TEST_1), + @Loc(value = TEST_2) }, tags = { "highway=pedestrian", + "psv:lanes:conditional:=1 @ (Mo-Fr 06:30-09:00,16:00-18:30)", + "bicycle=yes", "bicycle:conditional=no @ (Sa 08:00-16:00)" }) }) + private Atlas invalidAccessLanes; + + @TestAtlas(nodes = { @Node(coordinates = @Loc(value = TEST_1)), + @Node(coordinates = @Loc(value = TEST_2)) }, edges = { + @Edge(id = "1001000001", coordinates = { @Loc(value = TEST_1), + @Loc(value = TEST_2) }, tags = { "highway=pedestrian", + "motor_vehicle:conditional=delivery @ (Mo-Fr 06:00-11:00,17:00-19:00;Sa 03:30-19:00)", + "bicycle=yes", + "bicycle:conditional=notallowed @ (Sa 08:00-16:00)" }) }) + private Atlas invalidAccessType; + + @TestAtlas(nodes = { @Node(coordinates = @Loc(value = TEST_1)), + @Node(coordinates = @Loc(value = TEST_2)) }, edges = { + @Edge(id = "1001000001", coordinates = { @Loc(value = TEST_1), + @Loc(value = TEST_2) }, tags = { "highway=pedestrian", + "lanes:psv:conditional:=1 @ (Mo-Fr 06:30-09:00,16:00-18:30)", + "bicycle=yes", "bicycle:conditional=no @ (Sa 08:00-16:00)" }) }) + private Atlas reversedLanesTransport; + + public Atlas getAccessLanes() + { + return this.accessLanes; + } + + public Atlas getConditionalWay() + { + return this.conditionalWay; + } + + public Atlas getInvalidAccessLanes() + { + return this.invalidAccessLanes; + } + + public Atlas getInvalidAccessType() + { + return this.invalidAccessType; + } + + public Atlas getInvalidConditionFormat() + { + return this.invalidConditionFormat; + } + + public Atlas getInvalidConditionalKey() + { + return this.invalidConditionalKey; + } + + public Atlas getReversedLanesTransport() + { + return this.reversedLanesTransport; + } +}