Conditional restriction check (osmlab#313)

* Conditional Restriction check - started check for conditional restriction tag format - key format check implemented - conditional value format check to be done - added corresponding test class * Conditional Restriction check value - finished implementing the value check - added corresponding test cases * Added missing values - upon searching existing tags, added some missing values that can comprise a correct conditional restriction * Conditional Restriction check - checkstyle fix - fixed code style to pass the spotless checks * Conditional Restriction check minor fix - added missing restriction type and dot character as possible in the value format * Conditional Restriction check - PR fixes - changes per PR comments - added md files - added multiple instructions per object to capture any possible issue with it * Test update - updated tests with code changes * Validation changes Upon validating, some exceptions from the existing rules had to be added: - transportation and lanes can be reversed and represent the number of lanes or which can be used accordingly - | is a valid value character - added missing restriction types and access values - added exception for access:lanes * PR comment changes Made minor code changes based on the pull request comments * Bug fix Removed unnecessary variable creation
sayas01 · Jul 6, 2020 · b663d8e · b663d8e
1 parent d070ac4
commit b663d8e
Show file tree

Hide file tree

Showing 6 changed files with 463 additions and 0 deletions.
diff --git 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
@@ -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
@@ -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 `<restriction-value> @ <condition>` format.
+2. Way [id:525881134](https://www.openstreetmap.org/way/525881134) has the key `psv:lanes:backward:conditional` which
+does not respect the `<restriction-type>[:<transportation mode>][:<direction>]` 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. `<restriction-type>[:<transportation mode>][:<direction>]:conditional`
+ 2. `<transportation mode>[:<direction>]: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 `<restriction-value> @ <condition>[;<restriction-value> @ <condition>]`
+ 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)
+
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
@@ -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 <restriction-type>[:<transportation mode>][:<direction>]:conditional
+ * = <restriction-value> @ <condition>[;<restriction-value> @ <condition>]}
+ *
+ * @author laura
+ * @see <a href="https://wiki.openstreetmap.org/wiki/Conditional_restrictions">wiki</a> for more
+ *      information.
+ */
+public class ConditionalRestrictionCheck extends BaseCheck<String>
+{
+
+    private static final long serialVersionUID = 6726352951073801440L;
+
+    public static final String CONDITIONAL = ":conditional";
+    private static final List<String> 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<String> 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<String> DIRECTION = List.of("forward", "backward", "left", "right",
+            "both");
+    private static final List<String> 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<String> FALLBACK_INSTRUCTIONS = Arrays.asList(
+            "The conditional key \"{0}\" does not respect the \"<restriction-type>[:<transportation mode>][:<direction>]:conditional\" format",
+            "The conditional value \"{0}\" does not respect the format \"<restriction-value> @ <condition>[;<restriction-value> @ <condition>]\" ",
+            "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<CheckFlag> flag(final AtlasObject object)
+    {
+        final Set<String> instructions = new HashSet<>();
+
+        final List<String> 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<String> 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/...t/java/org/openstreetmap/atlas/checks/validation/tag/ConditionalRestrictionCheckTest.java b/...t/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 \"<restriction-value> @ <condition>[;<restriction-value> @ <condition>]\" ")));
+
+    }
+
+    @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 \"<restriction-value> @ <condition>[;<restriction-value> @ <condition>]\" ")));
+
+    }
+
+    @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 \"<restriction-value> @ <condition>[;<restriction-value> @ <condition>]\" ")));
+
+    }
+
+    @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 \"<restriction-type>[:<transportation mode>][:<direction>]: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()));
+    }
+
+}