Skip to content

Commit

Permalink
New check source maxspeed (osmlab#445)
Browse files Browse the repository at this point in the history
* Created SourceMaxspeedCheck

- started check structure

* First version for SourceMaxspeedCheck completed

* Unit-tests

- added unit-tests
- changed regex matching to use patterns and find a match instead
of matching the whole string for edge cases

* Instruction text fix

* Extracted optional in variable to avoid sonar complaint

* Added missing doc files for sourceMaxspeedCheck

* Added configurables and edge walking

- used already existing configurable country.denylist
- added configurables for values, context and country exceptions
- grouped edges with same id
- added unittest for the changes

* Described configurables and added them to config file

- added values for configurables in configuration.json
- added description for each new configurable in check MD file

* Added more unit tests for untouched conditions

* Added missing iso_country_code tag in test rule
  • Loading branch information
mm-ciub authored Jan 12, 2021
1 parent 8d1a5b6 commit 31e9f38
Show file tree
Hide file tree
Showing 6 changed files with 611 additions and 0 deletions.
37 changes: 37 additions & 0 deletions config/configuration.json
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,43 @@
"tags":"tags"
}
},
"SourceMaxspeedCheck": {
"countries.denylist": ["UK"],
"values": [
"sign",
"markings"
],
"context.values": [
"urban",
"rural",
"bicycle_road",
"trunk",
"motorway",
"living_street",
"school",
"pedestrian_zone",
"urban_motorway",
"urban_trunk",
"nsl",
"express",
"nsl_restricted",
"nsl_dual",
"nsl_single",
"zone"
],
"country.exceptions": [
"BE-VLG",
"BE-WAL",
"BE-BRU"
],
"challenge": {
"description": "Tasks containing features with tag source:maxspeed with incorrect value format.",
"blurb": "Features with invalid source:maxspeed tags",
"instruction": "Open your favorite editor and check that the listed tags are correct.",
"difficulty": "NORMAL",
"tags":"tags"
}
},
"InvalidTurnRestrictionCheck": {
"challenge": {
"description": "Tasks containing invalid turn restrictions",
Expand Down
1 change: 1 addition & 0 deletions docs/available_checks.md
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ This document is a list of tables with a description and link to documentation f
| [TunnelBridgeHeightLimitCheck](checks/tunnelBridgeHeightLimitCheck.md) | The purpose of this check is to identify roads with limited vertical clearance which do not have a maxheight tag. |
| [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. |
| [SourceMaxspeedCheck](checks/sourceMaxspeedCheck.md) | The purpose of this check is to identify elements that have a source:maxspeed tag that does not follow the tagging rules. |

## Ways
| Check Name | Check Description |
Expand Down
37 changes: 37 additions & 0 deletions docs/checks/sourceMaxspeedCheck.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# Source Maxspeed Check

This check flags features that contain a `source:maxspeed` tag that is not constructed following the rules set in the
OSM wiki: [Key:source:maxspeed](https://wiki.openstreetmap.org/wiki/Key:source:maxspeed).

#### Live Examples

1. Way [id:24358163](https://www.openstreetmap.org/way/24358163) has the value `implicit` instead of `RO:urban` as expected
for a default maxspeed on an urban road in Romania.
2. Way [id:31362404](https://www.openstreetmap.org/way/31362404) has a url instead of one of the following expected:
`sign`, `markings` or `<country_code>:<context>`.

#### 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).

The check verifies that all the values for `source:maxspeed` tags respect the following rules:
* = `sign` where the speed limit is defined by a numeric sign
* = `markings` where the speed limit is defined by painted road markings
* = `<country_code>:<context>` where the speed limit is defined by a particular context, for example urban/rural/motorway/etc.,
and no maxspeed is signposted

#### Configurables
The following values can be configured based on country legislation:
* `country_denylist` - this expects a list of countries for which the check should not be run; for example UK does not follow
the rules of this check;
* `values` - a list of defined values besides the zone rule. Current possible options are `sign` and `markings`;
* `context.values` - a set of possible values that the context can take in the `<country_code>:<context>` rule
* `country.exceptions` - list of exceptions that do not follow the ISO code format; for example, `BE-VLG`
is an accepted value because the region has it's own speed standard;

To learn more about the code, please look at the comments in the source code for the check:
[SourceMaxspeedCheck](../../src/main/java/org/openstreetmap/atlas/checks/validation/tag/SourceMaxspeedCheck.java)

Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
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 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.geography.atlas.items.Edge;
import org.openstreetmap.atlas.geography.atlas.items.Point;
import org.openstreetmap.atlas.geography.atlas.walker.OsmWayWalker;
import org.openstreetmap.atlas.tags.ISOCountryTag;
import org.openstreetmap.atlas.tags.annotations.validation.ISO2CountryValidator;
import org.openstreetmap.atlas.utilities.configuration.Configuration;

/**
* This check verifies that the source:maxspeed tag follows the official tagging rules.
* https://wiki.openstreetmap.org/wiki/Key:source:maxspeed
*
* @author mm-ciub
*/
public class SourceMaxspeedCheck extends BaseCheck<Long>
{

private static final long serialVersionUID = -7004341564141771203L;
private static final String GENERAL_INSTRUCTION = "The element with id {0,number,#} does not follow the source:maxspeed tagging rules";
private static final String WRONG_VALUE_INSTRUCTION = "The value must be 'sign', 'markings' or follow the country_code:context format.";
private static final String WRONG_COUNTRY_CODE_INSTRUCTION = "{0} is not a valid country code.";
private static final String WRONG_CONTEXT_INSTRUCTION = "{0} is not a valid context for the maxspeed source. (valid examples: urban, 30 etc.)";
private static final List<String> FALLBACK_INSTRUCTIONS = Arrays.asList(GENERAL_INSTRUCTION,
WRONG_VALUE_INSTRUCTION, WRONG_COUNTRY_CODE_INSTRUCTION, WRONG_CONTEXT_INSTRUCTION);
private static final String SOURCE_MAXSPEED = "source:maxspeed";
private static final List<String> POSSIBLE_VALUES = Arrays.asList("sign", "markings");
private static final Pattern COUNTRY_CONTEXT_PATTERN = Pattern.compile("[a-zA-Z]{2}:.+");
private static final Set<String> CONTEXT_VALUES = new HashSet<>(
Arrays.asList("urban", "rural", "bicycle_road", "trunk", "motorway", "living_street",
"school", "pedestrian_zone", "urban_motorway", "urban_trunk", "nsl", "express",
"nsl_restricted", "nsl_dual", "nsl_single", "zone"));
// Belgium has these 3 regions that are accepted because they have a different default rural or
// urban maxspeed
private static final List<String> COUNTRY_EXCEPTIONS = Arrays.asList("BE-VLG", "BE-WAL",
"BE-BRU");
// besides the default possible values, there are some accepted variations of "zone"
private static final String ZONE = "zone";
private static final int GENERAL_INSTRUCTION_INDEX = 0;
private static final int VALUE_INSTRUCTION_INDEX = 1;
private static final int COUNTRY_INSTRUCTION_INDEX = 2;
private static final int CONTEXT_INSTRUCTION_INDEX = 3;

private final List<String> exceptedCountries;
private final List<String> possibleValues;
private final Set<String> contextValues;
private final List<String> countryExceptions;

/**
* @param configuration
* the JSON configuration for this check
*/
public SourceMaxspeedCheck(final Configuration configuration)
{
super(configuration);
this.exceptedCountries = this.getDenylistCountries();
this.possibleValues = this.configurationValue(configuration, "values", POSSIBLE_VALUES);
this.contextValues = Set
.copyOf(this.configurationValue(configuration, "context.values", CONTEXT_VALUES));
this.countryExceptions = this.configurationValue(configuration, "country.exceptions",
COUNTRY_EXCEPTIONS);
}

/**
* Valid objects for this check are Points and Edges with a source:maxspeed tag and are not part
* of the excepted countries.
*
* @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 !this.isFlagged(object.getOsmIdentifier())
&& (object instanceof Edge || object instanceof Point)
&& this.hasSourceMaxspeed(object)
&& (object.getTags().containsKey(ISOCountryTag.KEY) && !this.exceptedCountries
.contains(object.tag(ISOCountryTag.KEY).toUpperCase()));
}

/**
* 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
*/
@Override
protected Optional<CheckFlag> flag(final AtlasObject object)
{
final Optional<String> sourceMaxspeed = object.getTag(SOURCE_MAXSPEED);
if (sourceMaxspeed.isPresent())
{
final String sourceValue = sourceMaxspeed.get();
final Set<String> instructions = this.testSourceValue(sourceValue);
if (!instructions.isEmpty())
{
this.markAsFlagged(object.getOsmIdentifier());
final String instruction = this.getLocalizedInstruction(GENERAL_INSTRUCTION_INDEX,
object.getOsmIdentifier());
final CheckFlag flag = object instanceof Edge
? this.createFlag(new OsmWayWalker((Edge) object).collectEdges(),
instruction)
: this.createFlag(object, instruction);
instructions.forEach(flag::addInstruction);
return Optional.of(flag);
}
}
return Optional.empty();
}

@Override
protected List<String> getFallbackInstructions()
{
return FALLBACK_INSTRUCTIONS;
}

private boolean hasSourceMaxspeed(final AtlasObject object)
{
return object.getOsmTags().keySet().stream().anyMatch(key -> key.contains(SOURCE_MAXSPEED));
}

private boolean isContextValid(final String context)
{
final boolean isNumber = context.matches("[0-9].+");
final boolean isZone = context.contains(ZONE);
final boolean isHighwayType = this.contextValues.contains(context);
return isNumber || isZone || isHighwayType;
}

private boolean isCountryValid(final String countryCode)
{
final ISO2CountryValidator validator = new ISO2CountryValidator();
return validator.isValid(countryCode) || this.countryExceptions.contains(countryCode);
}

private Set<String> testSourceValue(final String sourceValue)
{
final Set<String> instructions = new HashSet<>();
final Matcher matcher = COUNTRY_CONTEXT_PATTERN.matcher(sourceValue);
if (matcher.find())
{
final String[] parts = sourceValue.split(COLON);
if (!this.isCountryValid(parts[0]))
{
instructions.add(this.getLocalizedInstruction(COUNTRY_INSTRUCTION_INDEX, parts[0]));
}
if (!this.isContextValid(parts[1]))
{
instructions.add(this.getLocalizedInstruction(CONTEXT_INSTRUCTION_INDEX, parts[1]));
}

}
else if (!this.possibleValues.contains(sourceValue) && !sourceValue.contains(ZONE))
{
instructions.add(this.getLocalizedInstruction(VALUE_INSTRUCTION_INDEX));
}
return instructions;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
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;
import org.openstreetmap.atlas.utilities.configuration.Configuration;

/**
* @author mm-ciub
*/
public class SourceMaxspeedCheckTest
{

@Rule
public SourceMaxspeedCheckTestRule setup = new SourceMaxspeedCheckTestRule();

@Rule
public ConsumerBasedExpectedCheckVerifier verifier = new ConsumerBasedExpectedCheckVerifier();

private final Configuration inlineConfiguration = ConfigurationResolver.inlineConfiguration(
"{\"SourceMaxspeedCheck\":{\"countries.denylist\":[\"UK\"], \"context.values\":[\"urban\"], \"values\":[\"implied\"], \"country.exceptions\":[\"RO-TST\"]}}");

@Test
public void countryExceptionConfigTest()
{
this.verifier.actual(this.setup.countryExceptionConfigAtlas(),
new SourceMaxspeedCheck(this.inlineConfiguration));
this.verifier.verifyEmpty();
}

@Test
public void edgeWalkerTest()
{
this.verifier.actual(this.setup.edgeWalkerAtlas(),
new SourceMaxspeedCheck(ConfigurationResolver.emptyConfiguration()));
this.verifier.globallyVerify(flags -> Assert.assertEquals(1, flags.size()));
}

@Test
public void exceptionCountryTest()
{
this.verifier.actual(this.setup.exceptionCountryAtlas(),
new SourceMaxspeedCheck(this.inlineConfiguration));
this.verifier.verifyEmpty();
}

@Test
public void invalidContextTest()
{
this.verifier.actual(this.setup.invalidContextAtlas(),
new SourceMaxspeedCheck(ConfigurationResolver.emptyConfiguration()));
this.verifier.globallyVerify(flags -> Assert.assertEquals(2, flags.size()));
}

@Test
public void invalidCountryTest()
{
this.verifier.actual(this.setup.invalidCountryCodeAtlas(),
new SourceMaxspeedCheck(ConfigurationResolver.emptyConfiguration()));
this.verifier.globallyVerify(flags -> Assert.assertEquals(1, flags.size()));
}

@Test
public void invalidValueTest()
{
this.verifier.actual(this.setup.invalidValueAtlas(),
new SourceMaxspeedCheck(ConfigurationResolver.emptyConfiguration()));
this.verifier.globallyVerify(flags -> Assert.assertEquals(1, flags.size()));
}

@Test
public void noSourceMaxspeedTagTest()
{
this.verifier.actual(this.setup.noSourceMaxspeedAtlas(),
new SourceMaxspeedCheck(ConfigurationResolver.emptyConfiguration()));
this.verifier.verifyEmpty();
}

@Test
public void validContextConfigTest()
{
this.verifier.actual(this.setup.validContextAtlas(),
new SourceMaxspeedCheck(this.inlineConfiguration));
this.verifier.verifyEmpty();
}

@Test
public void validElementTest()
{
this.verifier.actual(this.setup.validAtlas(),
new SourceMaxspeedCheck(ConfigurationResolver.emptyConfiguration()));
this.verifier.verifyEmpty();
}

@Test
public void validNumberContextTest()
{
this.verifier.actual(this.setup.numberContextAtlas(),
new SourceMaxspeedCheck(ConfigurationResolver.emptyConfiguration()));
this.verifier.verifyEmpty();
}

@Test
public void validValueConfigTest()
{
this.verifier.actual(this.setup.validValueImpliedAtlas(),
new SourceMaxspeedCheck(this.inlineConfiguration));
this.verifier.verifyEmpty();
}

@Test
public void validZone()
{
this.verifier.actual(this.setup.zoneAtlas(),
new SourceMaxspeedCheck(ConfigurationResolver.emptyConfiguration()));
this.verifier.verifyEmpty();
}
}
Loading

0 comments on commit 31e9f38

Please sign in to comment.