Invalid tags check highway inconsistency (osmlab#397)

* Highway inconsistency extension and specific instructions

- added filters that look for inconsistency in the usage
of the highway tag (issue osmlab#377)
- made necessary changes so that when passing filters,
specific instructions can be passed as well

* Testing and documentation update

- updated documentation to reflect the addition of
specific instructions
- updated highway inconsistency instructions
- minor edits: removed magic number, fixed default instructions
and their usage
- added tests for the new cases

* Highway inconsistency filter fix

- fixed taggable filters to reflect the expected results

* Removed code smell in read config method

- split the readConfigurationFilter method in two,
one for simple and one for regex filters

config/configuration.json

@@ -441,7 +441,8 @@
     "filters.classes.tags": [
       ["area","boundary->protected_area&protect_class->!"],
       ["relation","boundary->protected_area&protect_class->!"],
-      ["edge","highway->motorway,trunk,primary,secondary,tertiary,unclassified,residential,service,motorway_link,trunk_link,primary_link,secondary_link,tertiary_link,living_street,track&junction->roundabout&area->*"]
+      ["edge","highway->motorway,trunk,primary,secondary,tertiary,unclassified,residential,service,motorway_link,trunk_link,primary_link,secondary_link,tertiary_link,living_street,track&junction->roundabout&area->*"],
+      ["node","highway->emergency_access_point&ref->!", "The element is an emergency_access_point with no ref tag."]
     ],
     "challenge": {
       "description": "Tasks containing features with tags containing missing, conflicting, incorrect or illegal values",

docs/checks/invalidTagsCheck.md

@@ -11,10 +11,10 @@ both the configurable filters and filters passed through the resource files will
 
 Filters for this check are either passed through config or/and through resource files.
 File "invalidTags.txt" contains the mapping of AtlasEntity to its corresponding resource file.
-Each configurable filter has 2 parts. The first is AtlasEntity class (node, edge, area, etc.). The second is a 
+Each configurable filter has 2 mandatory parts. The first is AtlasEntity class (node, edge, area, etc.). The second is a 
 [TaggableFilter](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/tags/filters/TaggableFilter.java)
 or a [RegexTaggableFilter](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/tags/filters/RegexTaggableFilter.java).
-
+Optionally, a specific instruction string can be passed as the third part.
 If a feature is one of the classes given and passes the filter then it is flagged.
 
 ** Configurable Filter Example:**
@@ -28,6 +28,8 @@ Filters to flag this would look like the following:
 The first string is the AtlasEntity class we want to look for. 
 The second string is the taggable filter that is looking for the combination of `boundary=protected_area`
 without a `protect_class` tag.
+For a specific instruction, the filter would look like this:
+`["area,"boundary->protected_area&protect_class->!","Area missing protected_class tag."]`
 
 This would flag an osm feature like the following: [Way 673787307](https://www.openstreetmap.org/way/673787307).
 
@@ -39,7 +41,9 @@ The RegexTaggableFilter can be configured in two ways. First, the inline configu
        ["node", ["source"],["illegal value regex"]]
      ]`
 The first string is the AtlasEntity. The next array of string represents the tag names for which the value must be checked. The last 
-array of strings represents the regex patterns that are matched with the tag values.
+array of strings represents the regex patterns that are matched with the tag values. Optionally, after the regex array a specific
+instruction can be passed: `["node", ["source"],["illegal value regex"], "The element has an illegal source."]`
+
 The second option is to create the filter through a resource file. This is a more complete option allowing also the configuration
 of an exception map containing certain tag-value pairs that are excepted by the regex match. For this, the filter is passed 
 in json format: `{
@@ -55,10 +59,12 @@ in json format: `{
                            "tagName" : "source",
                            "values": ["open version map", "public map"]
                          }
-                       ]
+                       ],
+                       "instruction" : "The element has an illegal source."
                      }
                    ]
-                 }` 
+                 }`
+The `instruction` attribute is optional. 
 To be mentioned here that the resource file for the `RegexTaggableFilter` must contain the word `regex` in it's name.
 
 #### Code Review

src/main/resources/org/openstreetmap/atlas/checks/validation/tag/invalid-tags-check-inconsistent-highway-filter.json

@@ -0,0 +1,18 @@
+{
+  "filters": [
+    "junction->*&junction->!yes&highway->!",
+    "oneway->*&highway->!&railway->!&aerialway->!&waterway->!&aeroway->!&piste:type->!",
+    "highway->!&disused:highway->!&abandoned:highway->!&construction:highway->!&proposed:highway->!&planned:highway->!&leisure->!track&tracktype->*||lanes->*",
+    "highway->emergency_access_point&ref->!",
+    "footway->sidewalk&highway->!footway&highway->!path&highway->!construction",
+    "highway->motorway&ref->!&nat_ref->!&int_ref->!&noref->!"
+  ],
+  "instructions": [
+    "The element has a junction tag but no highway tag.",
+    "The element has a oneway tag but no highway tag.",
+    "The element is missing a highway tag.",
+    "The element is an emergency access point but has no ref tag.",
+    "The element is a footway=sidewalk but has no appropriate highway tag.",
+    "The element is a motorway but has no ref tag."
+  ]
+}

src/main/resources/org/openstreetmap/atlas/checks/validation/tag/invalid-tags-check-regex-filter.json

@@ -8,7 +8,8 @@
         ".*(?i)\\bvworld\\b.*",
         ".*(?i)\\bxdworld\\b.*"
       ],
-      "exceptions":[]
+      "exceptions":[],
+      "instruction": "The following element has an illegal source."
     }
   ]
 }

src/main/resources/org/openstreetmap/atlas/checks/validation/tag/invalidTags.txt

@@ -1,8 +1,4 @@
-area:invalid-tags-check-filter.json
-relation:invalid-tags-check-filter.json
-line:invalid-tags-check-filter.json
-point:invalid-tags-check-filter.json
-node:invalid-tags-check-filter.json
+edge:invalid-tags-check-inconsistent-highway-filter.json
 area:invalid-tags-check-regex-filter.json
 relation:invalid-tags-check-regex-filter.json
 line:invalid-tags-check-regex-filter.json

src/test/java/org/openstreetmap/atlas/checks/validation/tag/InvalidTagsCheckTest.java

@@ -29,6 +29,31 @@ public void appendConfigFiltersTest()
         this.verifier.globallyVerify(flags -> Assert.assertEquals(2, flags.size()));
     }
 
+    @Test
+    public void configRegexInstruction()
+    {
+        this.verifier.actual(this.setup.getIllegalSourceLinkNode(),
+                new InvalidTagsCheck(ConfigurationResolver.inlineConfiguration(
+                        "{\"InvalidTagsCheck\":{\"filters.resource.override\": true,\"filters.classes.regex\": ["
+                                + "      [\"node\", [\"source\"],[\".*(?i)\\\\bgoogle\\\\b.*\", \".*(?i)\\\\bhere\\\\b(?=.*map|.com)\",\n"
+                                + "        \".*(?i)\\\\bvworld\\\\b.*\", \".*(?i)\\\\bxdworld\\\\b.*\"], \"Illegal tag: {0}.\"]"
+                                + "    ]}}")));
+        this.verifier.globallyVerify(flags -> Assert.assertEquals(
+                "1. OSM feature 1 has invalid tags.\n" + "2. Illegal tag: source.",
+                flags.get(0).getInstructions()));
+    }
+
+    @Test
+    public void configTaggableInstruction()
+    {
+        this.verifier.actual(this.setup.testAtlas(),
+                new InvalidTagsCheck(ConfigurationResolver.inlineConfiguration(
+                        "{\"InvalidTagsCheck\":{\"filters.resource.override\": true,\"filters.classes.tags\":[[\"edge\",\"route->ferry&highway->*\",\"The feature has tag issues.\"]]}}")));
+        this.verifier.globallyVerify(flags -> Assert.assertEquals(
+                "1. OSM feature 5 has invalid tags.\n" + "2. The feature has tag issues.",
+                flags.get(0).getInstructions()));
+    }
+
     @Test
     public void illegalSourceEdge()
     {
@@ -117,6 +142,34 @@ public void invalidRoundaboutAreaTest()
                 flags.get(0).getInstructions()));
     }
 
+    @Test
+    public void resourceRegexInstruction()
+    {
+        this.verifier.actual(this.setup.getIllegalSourceLinkNode(),
+                new InvalidTagsCheck(ConfigurationResolver.inlineConfiguration(
+                        "{\"InvalidTagsCheck\":{\"filters.resource.override\": false}}")));
+        this.verifier.globallyVerify(flags -> Assert.assertEquals(
+                "1. OSM feature 1 has invalid tags.\n"
+                        + "2. The following element has an illegal source.",
+                flags.get(0).getInstructions()));
+        this.verifier.globallyVerify(flags -> Assert.assertEquals(
+                "1. OSM feature 2 has invalid tags.\n"
+                        + "2. The following element has an illegal source.",
+                flags.get(1).getInstructions()));
+    }
+
+    @Test
+    public void resourceTaggableInstruction()
+    {
+        this.verifier.actual(this.setup.getInconsistentHighwayAtlas(),
+                new InvalidTagsCheck(ConfigurationResolver.inlineConfiguration(
+                        "{\"InvalidTagsCheck\":{\"filters.resource.override\": false}}")));
+        this.verifier.globallyVerify(flags -> Assert.assertEquals(
+                "1. OSM feature 1001 has invalid tags.\n"
+                        + "2. The element has a junction tag but no highway tag.",
+                flags.get(0).getInstructions()));
+    }
+
     @Test
     public void validEmptyConfigTest()
     {

src/test/java/org/openstreetmap/atlas/checks/validation/tag/InvalidTagsCheckTestRule.java

@@ -40,6 +40,12 @@ public class InvalidTagsCheckTestRule extends CoreTestRule
                                     @Loc(value = TEST_2) }) })
     private Atlas illegalSourceLinkNode;
 
+    @TestAtlas(nodes = { @Node(id = "1000000", coordinates = @Loc(value = TEST_1)),
+            @Node(id = "2000000", coordinates = @Loc(value = TEST_2)) }, edges = {
+                    @Edge(id = "1001000001", coordinates = { @Loc(value = TEST_1),
+                            @Loc(value = TEST_2) }, tags = { "junction=roundabout" }) })
+    private Atlas inconsistentHighwayAtlas;
+
     @TestAtlas(
             // nodes
             nodes = {
@@ -86,6 +92,11 @@ public Atlas getIllegalSourceLinkNode()
         return this.illegalSourceLinkNode;
     }
 
+    public Atlas getInconsistentHighwayAtlas()
+    {
+        return this.inconsistentHighwayAtlas;
+    }
+
     public Atlas testAtlas()
     {
         return this.testAtlas;