Conflicting Area Tag Combination check (osmlab#53)

* cat initial commit

* removed / add new combinations & associated unit tests

* minor instruction changes...

* replace ! with isNotOfType & docs

* proper usage of isNotOfType

* clean up docs, add config & additional test

* code cleanup

* bump up atlas version

* another version bump

* Update atlas dependency to 5.1.1

config/configuration.json

@@ -53,6 +53,14 @@
       "tags":"building,highway"
     }
   },
+  "ConflictingAreaTagCombination": {
+    "challenge": {
+      "description": "Task contains Area's with mutually exclusive tag combinations.",
+      "blurb": "Conflicting Area Tag Combination",
+      "instruction": "Open your favorite editor, check the instruction and modify the tags that are considered conflicting",
+      "difficulty": "EASY"
+    }
+  },
   "DuplicateNodeCheck": {
     "challenge": {
       "description": "Tasks contain node locations that have duplicate nodes found.",

dependencies.gradle

@@ -1,6 +1,6 @@
 project.ext.versions = [
     checkstyle: '7.6.1',
-    atlas: '5.0.15',
+    atlas: '5.1.1',
     commons:'2.6',
     atlas_generator: '4.0.4',
 ]

docs/checks/conflictingAreaTagCombination.md

@@ -0,0 +1,49 @@
+# Conflicting Area Tag Check
+
+#### Description
+This check flags Atlas Area Objects that have conflicting tag combinations. An Atlas Area Object is an enclosed Polygon that (by default) uses this [tag filter](https://github.com/osmlab/atlas/blob/dev/src/main/resources/org/openstreetmap/atlas/geography/atlas/pbf/atlas-area.json) to differentiate itself from other Atlas Objects.
+
+#### Live Example
+1) An OSM Editor has mapped this feature ([osm id: 372444940](https://www.openstreetmap.org/way/372444940#map=19/-6.18454/35.74750)) as a building and a tree, using the `nautual=TREE` and `building=YES` tags. These two tags should not logically co-exist. An editor should review the feature and make the appropriate changes.
+
+2) This closed Way ([osm id: 452182969](https://www.openstreetmap.org/way/452182969#map=19/7.77603/81.21694)) is tagged as `natural=WATER` and `man_made=WATER_TOWER`. The `natural=WATER` tag should be used to tag areas of water (e.g. lakes, multipolygon water relations), or water bodies. Water towers are man made structures that contain water, therefore, these two should not co-exist.
+
+#### Code Review
+In [Atlas](https://github.com/osmlab/atlas), OSM elements are represented as Edges, Points, Lines, Nodes & Relations; in our case, we’re are looking at [Areas](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/geography/atlas/items/Area.java).
+
+Our first goal is to validate the incoming Atlas object. Valid features for this check will satisfy the following conditions:
+* Must be an Atlas Area Object
+* Cannot have an `area=NO` tag.
+
+```java
+@Override
+public boolean validCheckForObject(final AtlasObject object)
+{
+    return object instanceof Area
+        && Validators.isNotOfType(object, AreaTag.class, AreaTag.NO);
+}
+```
+
+Using the Validators class, we store each conflicting combination into a [Predicate](https://docs.oracle.com/javase/8/docs/api/java/util/function/Predicate.html) variable that can be used to test its truthiness.
+```java
+private static final Predicate<Taggable> NATURAL_WATER_MANMANDE = object -> 
+        Validators.isOfType(object, NaturalTag.class, NaturalTag.WATER)
+        && Validators.isNotOfType(object, ManMadeTag.class, ManMadeTag.RESERVOIR_COVERED, ManMadeTag.WASTEWATER_PLANT);
+```
+
+For the variable above to be truthy, the following conditions must be true:
+* Area has `natural=WATER` tag
+* Area has a `man_made` tag AND its' value must not equal `RESERVOIR_COVERED` OR `WASTEWATER_PLANT`
+
+Then, we can easily test each combination using Predicate's `test()` function.
+
+```java
+if (NATURAL_WATER_MANMANDE.test(object))
+{
+    flag.addInstruction(this.getLocalizedInstruction(FOUR));
+    hasConflictingCombinations = true;
+}
+```
+
+To learn more about the code, please look at the comments in the source code for the check.
+[ConflictingAreaTagCombination.java](../../src/main/java/org/openstreetmap/atlas/checks/validation/tag/ConflictingAreaTagCombination.java)

src/main/java/org/openstreetmap/atlas/checks/validation/tag/ConflictingAreaTagCombination.java

@@ -0,0 +1,166 @@
+package org.openstreetmap.atlas.checks.validation.tag;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Optional;
+import java.util.function.Predicate;
+
+import org.openstreetmap.atlas.checks.base.BaseCheck;
+import org.openstreetmap.atlas.checks.flag.CheckFlag;
+import org.openstreetmap.atlas.geography.atlas.items.Area;
+import org.openstreetmap.atlas.geography.atlas.items.AtlasObject;
+import org.openstreetmap.atlas.tags.AreaTag;
+import org.openstreetmap.atlas.tags.BuildingTag;
+import org.openstreetmap.atlas.tags.HighwayTag;
+import org.openstreetmap.atlas.tags.LandUseTag;
+import org.openstreetmap.atlas.tags.ManMadeTag;
+import org.openstreetmap.atlas.tags.NaturalTag;
+import org.openstreetmap.atlas.tags.Taggable;
+import org.openstreetmap.atlas.tags.annotations.validation.Validators;
+import org.openstreetmap.atlas.utilities.configuration.Configuration;
+
+/**
+ * Flags Area Objects with conflicting tag combinations.
+ *
+ * @author danielbaah
+ */
+public class ConflictingAreaTagCombination extends BaseCheck
+{
+
+    private static final String INVALID_COMBINATION_INSTRUCTION = "OSM feature {0,number,#} has invalid tag combinations.";
+    private static final String INVALID_BUILDING_NATURAL_INSTRUCTION = "Building tag should not exist with natural tag";
+    private static final String INVALID_BUILDING_HIGHWAY_INSTRUCTION = "Building tag should not exist with highway tag";
+    private static final String INVALID_NATURAL_HIGHWAY_INSTRUCTION = "Natural tag should not exist with highway tag";
+    private static final String INVALID_WATER_LANDUSE_INSTRUCTION = "natural=WATER tag should not exist with any landuse tag other than RESERVOIR, BASIN, or AQUACULTURE";
+    private static final String INVALID_WATER_MANMADE_INSTRUCTION = "natural=WATER tag should not exist with any MAN_MADE tag other than RESERVOIR_COVERED or WASTEWATER_PLANT";
+    private static final String INVALID_LANDUSE_HIGHWAY = "Land use tag should not exist with highway tag";
+
+    private static final List<String> FALLBACK_INSTRUCTIONS = Arrays.asList(
+            INVALID_COMBINATION_INSTRUCTION, INVALID_BUILDING_NATURAL_INSTRUCTION,
+            INVALID_BUILDING_HIGHWAY_INSTRUCTION, INVALID_NATURAL_HIGHWAY_INSTRUCTION,
+            INVALID_WATER_MANMADE_INSTRUCTION, INVALID_WATER_LANDUSE_INSTRUCTION,
+            INVALID_LANDUSE_HIGHWAY);
+    // Building tag should not exist with any natural tags. The only exception is if building=NO.
+    private static final Predicate<Taggable> BUILDING_NATURAL = object -> Validators
+            .isNotOfType(object, BuildingTag.class, BuildingTag.NO)
+            && Validators.hasValuesFor(object, NaturalTag.class);
+    // Building tag should not exist with any highway tags. The only exception is highway=SERVICES.
+    private static final Predicate<Taggable> BUILDING_HIGHWAY = object -> Validators
+            .isNotOfType(object, BuildingTag.class, BuildingTag.NO)
+            && Validators.isNotOfType(object, HighwayTag.class, HighwayTag.SERVICES);
+    // Natural tag should not exist with any highway tags.
+    private static final Predicate<Taggable> NATURAL_HIGHWAY = object -> Validators.hasValuesFor(
+            object, NaturalTag.class) && Validators.hasValuesFor(object, HighwayTag.class);
+    // The natural=WATER tag should not exist with any man_made tags. The exceptions are
+    // man_made=RESERVOIR_COVERED and man_made=WASTEWATER_PLANT.
+    private static final Predicate<Taggable> NATURAL_WATER_MANMANDE = object -> Validators
+            .isOfType(object, NaturalTag.class, NaturalTag.WATER)
+            && Validators.isNotOfType(object, ManMadeTag.class, ManMadeTag.RESERVOIR_COVERED,
+                    ManMadeTag.WASTEWATER_PLANT);
+    // The natural=WATER tag should not exist with any landuse tags. The exceptions are
+    // landuse=BASIN,
+    // landuse=RESERVOIR, and landuse=AQUACULTURE.
+    private static final Predicate<Taggable> WATER_LANDUSE = object -> Validators.isOfType(object,
+            NaturalTag.class, NaturalTag.WATER)
+            && Validators.isNotOfType(object, LandUseTag.class, LandUseTag.RESERVOIR,
+                    LandUseTag.BASIN, LandUseTag.AQUACULTURE);
+    // Landuse should not exist with any highway tags.
+    private static final Predicate<Taggable> LANDUSE_HIGHWAY = object -> Validators.hasValuesFor(
+            object, LandUseTag.class) && Validators.hasValuesFor(object, HighwayTag.class);
+    private static final int THREE = 3;
+    private static final int FOUR = 4;
+    private static final int FIVE = 5;
+    private static final int SIX = 6;
+    private static final long serialVersionUID = 9167816371258788999L;
+
+    /**
+     * The default constructor that must be supplied. The Atlas Checks framework will generate the
+     * checks with this constructor, supplying a configuration that can be used to adjust any
+     * parameters that the check uses during operation.
+     *
+     * @param configuration
+     *            the JSON configuration for this check
+     */
+    public ConflictingAreaTagCombination(final Configuration configuration)
+    {
+        super(configuration);
+    }
+
+    /**
+     * This function will validate if the supplied atlas object is valid for the check.
+     *
+     * @param object
+     *            the atlas object supplied by the Atlas-Checks framework for evaluation
+     * @return {@code true} if this object should be checked
+     */
+    @Override
+    public boolean validCheckForObject(final AtlasObject object)
+    {
+        return object instanceof Area && !Validators.isOfType(object, AreaTag.class, AreaTag.NO);
+    }
+
+    /**
+     * This is the actual function that will check to see whether the object needs to be flagged.
+     *
+     * @param object
+     *            the atlas object supplied by the Atlas-Checks framework for evaluation
+     * @return an optional {@link CheckFlag} object that
+     */
+    @Override
+    protected Optional<CheckFlag> flag(final AtlasObject object)
+    {
+
+        final CheckFlag flag = createFlag(object,
+                this.getLocalizedInstruction(0, object.getOsmIdentifier()));
+        boolean hasConflictingCombinations = false;
+
+        if (BUILDING_NATURAL.test(object))
+        {
+            flag.addInstruction(this.getLocalizedInstruction(1));
+            hasConflictingCombinations = true;
+        }
+
+        if (BUILDING_HIGHWAY.test(object))
+        {
+            flag.addInstruction(this.getLocalizedInstruction(2));
+            hasConflictingCombinations = true;
+        }
+
+        if (NATURAL_HIGHWAY.test(object))
+        {
+            flag.addInstruction(this.getLocalizedInstruction(THREE));
+            hasConflictingCombinations = true;
+        }
+
+        if (NATURAL_WATER_MANMANDE.test(object))
+        {
+            flag.addInstruction(this.getLocalizedInstruction(FOUR));
+            hasConflictingCombinations = true;
+        }
+
+        if (WATER_LANDUSE.test(object))
+        {
+            flag.addInstruction(this.getLocalizedInstruction(FIVE));
+            hasConflictingCombinations = true;
+        }
+
+        if (LANDUSE_HIGHWAY.test(object))
+        {
+            flag.addInstruction(this.getLocalizedInstruction(SIX));
+            hasConflictingCombinations = true;
+        }
+
+        if (hasConflictingCombinations)
+        {
+            return Optional.of(flag);
+        }
+
+        return Optional.empty();
+    }
+
+    @Override
+    protected List<String> getFallbackInstructions()
+    {
+        return FALLBACK_INSTRUCTIONS;
+    }
+}

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

@@ -0,0 +1,108 @@
+package org.openstreetmap.atlas.checks.validation.tag;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.openstreetmap.atlas.checks.configuration.ConfigurationResolver;
+import org.openstreetmap.atlas.checks.validation.verifier.ConsumerBasedExpectedCheckVerifier;
+
+/**
+ * ConflictingAreaTagCombination unit tests
+ *
+ * @author danielbaah
+ */
+public class ConflictingAreaTagCombinationCheckTest
+{
+    @Rule
+    public ConflictingAreaTagCombinationCheckTestRule setup = new ConflictingAreaTagCombinationCheckTestRule();
+
+    @Rule
+    public ConsumerBasedExpectedCheckVerifier verifier = new ConsumerBasedExpectedCheckVerifier();
+
+    @Test
+    public void buildingHighwayTagAtlasTest()
+    {
+        this.verifier.actual(this.setup.getBuildingHighwayTagAtlas(),
+                new ConflictingAreaTagCombination(ConfigurationResolver.emptyConfiguration()));
+        this.verifier.verifyNotEmpty();
+    }
+
+    @Test
+    public void validHighwayServicesAtlasTest()
+    {
+        this.verifier.actual(this.setup.getBuildingHighwayServicesAtlas(),
+                new ConflictingAreaTagCombination(ConfigurationResolver.emptyConfiguration()));
+        this.verifier.verifyEmpty();
+    }
+
+    @Test
+    public void buildingNaturalTagAtlasTest()
+    {
+        this.verifier.actual(this.setup.getBuildingNaturalTagAtlas(),
+                new ConflictingAreaTagCombination(ConfigurationResolver.emptyConfiguration()));
+        this.verifier.verifyNotEmpty();
+    }
+
+    @Test
+    public void naturalHighwayTagAtlasTest()
+    {
+        this.verifier.actual(this.setup.getNaturalHighwayTagAtlas(),
+                new ConflictingAreaTagCombination(ConfigurationResolver.emptyConfiguration()));
+        this.verifier.verifyNotEmpty();
+    }
+
+    @Test
+    public void naturalLeisureTagAtlasTest()
+    {
+        this.verifier.actual(this.setup.getNaturalLeisureTagAtlas(),
+                new ConflictingAreaTagCombination(ConfigurationResolver.emptyConfiguration()));
+        this.verifier.verifyEmpty();
+    }
+
+    @Test
+    public void naturalManMadeTagAtlasTest()
+    {
+        this.verifier.actual(this.setup.getNaturalManMadeTagAtlas(),
+                new ConflictingAreaTagCombination(ConfigurationResolver.emptyConfiguration()));
+        this.verifier.verifyEmpty();
+    }
+
+    @Test
+    public void waterLandUseTagAtlasTest()
+    {
+        this.verifier.actual(this.setup.getWaterLandUseTagAtlas(),
+                new ConflictingAreaTagCombination(ConfigurationResolver.emptyConfiguration()));
+        this.verifier.verifyNotEmpty();
+    }
+
+    @Test
+    public void waterLandUseAquacultureAtlasTest()
+    {
+        this.verifier.actual(this.setup.getWaterLandUseAquacultureAtlas(),
+                new ConflictingAreaTagCombination(ConfigurationResolver.emptyConfiguration()));
+        this.verifier.verifyEmpty();
+    }
+
+    @Test
+    public void waterManMadeChimneyAtlasTest()
+    {
+        this.verifier.actual(this.setup.getWaterManMadeChimneyAtlas(),
+                new ConflictingAreaTagCombination(ConfigurationResolver.emptyConfiguration()));
+        this.verifier.verifyNotEmpty();
+    }
+
+    @Test
+    public void landUseHighwayAtlasTest()
+    {
+        this.verifier.actual(this.setup.getLandUseHighwayAtlas(),
+                new ConflictingAreaTagCombination(ConfigurationResolver.emptyConfiguration()));
+        this.verifier.verifyNotEmpty();
+    }
+
+    @Test
+    public void areaNoNaturalLandUseTagTest()
+    {
+        this.verifier.actual(this.setup.getAreaNoNaturalLandUseTagAtlas(),
+                new ConflictingAreaTagCombination(ConfigurationResolver.emptyConfiguration()));
+        this.verifier.verifyEmpty();
+    }
+}