diff --git a/config/configuration.json b/config/configuration.json index 1d408bb1d..3a3da7a73 100644 --- a/config/configuration.json +++ b/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.", diff --git a/dependencies.gradle b/dependencies.gradle index 1eca119dc..5b7eaea1f 100644 --- a/dependencies.gradle +++ b/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', ] diff --git a/docs/checks/conflictingAreaTagCombination.md b/docs/checks/conflictingAreaTagCombination.md new file mode 100644 index 000000000..da03b8300 --- /dev/null +++ b/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 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) diff --git a/src/main/java/org/openstreetmap/atlas/checks/validation/tag/ConflictingAreaTagCombination.java b/src/main/java/org/openstreetmap/atlas/checks/validation/tag/ConflictingAreaTagCombination.java new file mode 100644 index 000000000..fdafa33c1 --- /dev/null +++ b/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 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 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 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 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 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 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 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 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 getFallbackInstructions() + { + return FALLBACK_INSTRUCTIONS; + } +} diff --git a/src/test/java/org/openstreetmap/atlas/checks/validation/tag/ConflictingAreaTagCombinationCheckTest.java b/src/test/java/org/openstreetmap/atlas/checks/validation/tag/ConflictingAreaTagCombinationCheckTest.java new file mode 100644 index 000000000..9c9926536 --- /dev/null +++ b/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(); + } +} diff --git a/src/test/java/org/openstreetmap/atlas/checks/validation/tag/ConflictingAreaTagCombinationCheckTestRule.java b/src/test/java/org/openstreetmap/atlas/checks/validation/tag/ConflictingAreaTagCombinationCheckTestRule.java new file mode 100644 index 000000000..6fad1bb19 --- /dev/null +++ b/src/test/java/org/openstreetmap/atlas/checks/validation/tag/ConflictingAreaTagCombinationCheckTestRule.java @@ -0,0 +1,150 @@ +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.Area; +import org.openstreetmap.atlas.utilities.testing.TestAtlas.Loc; + +/** + * ConflictingAreaTagCombination test Atlas. + * + * @author danielbaah + */ +public class ConflictingAreaTagCombinationCheckTestRule extends CoreTestRule +{ + public static final String INVALID_AREA_ID = "127005"; + private static final String AREA_LOCATION_ONE = "37.320524859664474, -122.03601479530336"; + private static final String AREA_LOCATION_TWO = "37.320524859664474, -122.03530669212341"; + private static final String AREA_LOCATION_THREE = "37.32097706357857, -122.03530669212341"; + + // building=* and natural=* + @TestAtlas(areas = { + @Area(id = INVALID_AREA_ID, coordinates = { @Loc(value = AREA_LOCATION_ONE), + @Loc(value = AREA_LOCATION_TWO), @Loc(value = AREA_LOCATION_THREE), + @Loc(value = AREA_LOCATION_ONE) }, tags = { "building=yes", "natural=rock" }) }) + private Atlas buildingNaturalTagAtlas; + + // building=* and highway=* + @TestAtlas(areas = { @Area(id = INVALID_AREA_ID, coordinates = { + @Loc(value = AREA_LOCATION_ONE), @Loc(value = AREA_LOCATION_TWO), + @Loc(value = AREA_LOCATION_THREE), + @Loc(value = AREA_LOCATION_ONE) }, tags = { "building=yes", "highway=pedestrian" }) }) + private Atlas buildingHighwayTagAtlas; + + // building=* and highway=* + @TestAtlas(areas = { @Area(id = INVALID_AREA_ID, coordinates = { + @Loc(value = AREA_LOCATION_ONE), @Loc(value = AREA_LOCATION_TWO), + @Loc(value = AREA_LOCATION_THREE), + @Loc(value = AREA_LOCATION_ONE) }, tags = { "building=yes", "highway=services" }) }) + private Atlas buildingHighwayServicesAtlas; + + // natural=* and man_made=* valid tag combinations + @TestAtlas(areas = { @Area(id = INVALID_AREA_ID, coordinates = { + @Loc(value = AREA_LOCATION_ONE), @Loc(value = AREA_LOCATION_TWO), + @Loc(value = AREA_LOCATION_THREE), @Loc(value = AREA_LOCATION_ONE) }, tags = { + "natural=water", "man_made=reservoir_covered" }) }) + private Atlas naturalManMadeTagAtlas; + + // natural=* and highway=* + @TestAtlas(areas = { @Area(id = INVALID_AREA_ID, coordinates = { + @Loc(value = AREA_LOCATION_ONE), @Loc(value = AREA_LOCATION_TWO), + @Loc(value = AREA_LOCATION_THREE), + @Loc(value = AREA_LOCATION_ONE) }, tags = { "natural=water", "highway=primary" }) }) + private Atlas naturalHighwayTagAtlas; + + // natural=* and leisure=* valid tag combinations + @TestAtlas(areas = { @Area(id = INVALID_AREA_ID, coordinates = { + @Loc(value = AREA_LOCATION_ONE), @Loc(value = AREA_LOCATION_TWO), + @Loc(value = AREA_LOCATION_THREE), + @Loc(value = AREA_LOCATION_ONE) }, tags = { "natural=grassland", "leisure=park" }) }) + private Atlas naturalLeisureTagAtlas; + + // natural=water and landuse=* + @TestAtlas(areas = { @Area(id = INVALID_AREA_ID, coordinates = { + @Loc(value = AREA_LOCATION_ONE), @Loc(value = AREA_LOCATION_TWO), + @Loc(value = AREA_LOCATION_THREE), + @Loc(value = AREA_LOCATION_ONE) }, tags = { "natural=water", "landuse=port" }) }) + private Atlas waterLandUseTagAtlas; + + // natural=water and landuse=aquaculture + @TestAtlas(areas = { @Area(id = INVALID_AREA_ID, coordinates = { + @Loc(value = AREA_LOCATION_ONE), @Loc(value = AREA_LOCATION_TWO), + @Loc(value = AREA_LOCATION_THREE), + @Loc(value = AREA_LOCATION_ONE) }, tags = { "natural=water", "landuse=aquaculture" }) }) + private Atlas waterLandUseAquacultureAtlas; + + // natural=water and man_made=chimney + @TestAtlas(areas = { @Area(id = INVALID_AREA_ID, coordinates = { + @Loc(value = AREA_LOCATION_ONE), @Loc(value = AREA_LOCATION_TWO), + @Loc(value = AREA_LOCATION_THREE), + @Loc(value = AREA_LOCATION_ONE) }, tags = { "natural=water", "man_made=chimney" }) }) + private Atlas waterManMadeChimneyAtlas; + + @TestAtlas(areas = { @Area(id = INVALID_AREA_ID, coordinates = { + @Loc(value = AREA_LOCATION_ONE), @Loc(value = AREA_LOCATION_TWO), + @Loc(value = AREA_LOCATION_THREE), + @Loc(value = AREA_LOCATION_ONE) }, tags = { "landuse=disused", "highway=primary" }) }) + private Atlas landUseHighwayAtlas; + + @TestAtlas(areas = { @Area(id = INVALID_AREA_ID, coordinates = { + @Loc(value = AREA_LOCATION_ONE), @Loc(value = AREA_LOCATION_TWO), + @Loc(value = AREA_LOCATION_THREE), @Loc(value = AREA_LOCATION_ONE) }, tags = { + "area=no", "natural=water", "landuse=port" }) }) + private Atlas areaNoNaturalLandUseTagAtlas; + + public Atlas getBuildingNaturalTagAtlas() + { + return buildingNaturalTagAtlas; + } + + public Atlas getBuildingHighwayTagAtlas() + { + return buildingHighwayTagAtlas; + } + + public Atlas getBuildingHighwayServicesAtlas() + { + return buildingHighwayServicesAtlas; + } + + public Atlas getNaturalManMadeTagAtlas() + { + return naturalManMadeTagAtlas; + } + + public Atlas getNaturalHighwayTagAtlas() + { + return naturalHighwayTagAtlas; + } + + public Atlas getNaturalLeisureTagAtlas() + { + return naturalLeisureTagAtlas; + } + + public Atlas getWaterLandUseTagAtlas() + { + return waterLandUseTagAtlas; + } + + public Atlas getWaterLandUseAquacultureAtlas() + { + return waterLandUseAquacultureAtlas; + } + + public Atlas getWaterManMadeChimneyAtlas() + { + return waterManMadeChimneyAtlas; + } + + public Atlas getLandUseHighwayAtlas() + { + return landUseHighwayAtlas; + } + + public Atlas getAreaNoNaturalLandUseTagAtlas() + { + return areaNoNaturalLandUseTagAtlas; + } +}