Index: src/org/openstreetmap/josm/data/validation/TestError.java =================================================================== --- src/org/openstreetmap/josm/data/validation/TestError.java (revision 16178) +++ src/org/openstreetmap/josm/data/validation/TestError.java (working copy) @@ -219,11 +219,12 @@ * Returns a new test error with the specified values * * @return a new test error with the specified values - * @throws IllegalArgumentException when {@link #message} or {@link #primitives} is null. + * @throws IllegalArgumentException when {@link #message} or {@link #primitives} is null/empty. */ public TestError build() { CheckParameterUtil.ensureParameterNotNull(message, "message not set"); CheckParameterUtil.ensureParameterNotNull(primitives, "primitives not set"); + CheckParameterUtil.ensureThat(!primitives.isEmpty(), "primitives is empty"); if (this.highlighted == null) { this.highlighted = Collections.emptySet(); } Index: src/org/openstreetmap/josm/data/validation/tests/ConditionalKeys.java =================================================================== --- src/org/openstreetmap/josm/data/validation/tests/ConditionalKeys.java (revision 16178) +++ src/org/openstreetmap/josm/data/validation/tests/ConditionalKeys.java (working copy) @@ -8,6 +8,7 @@ import java.util.Collection; import java.util.HashSet; import java.util.List; +import java.util.Locale; import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -122,16 +123,6 @@ return parts.length == 1 && (isRestrictionType(parts[0]) || isTransportationMode(parts[0])); } - /** - * Check if a value is valid - * @param key The key the value is for - * @param value The value - * @return true if it is valid - */ - public boolean isValueValid(String key, String value) { - return validateValue(key, value) == null; - } - static class ConditionalParsingException extends RuntimeException { ConditionalParsingException(String message) { super(message); @@ -190,9 +181,10 @@ * Validate a key/value pair * @param key The key * @param value The value + * @param p the primitive * @return The error message for that value or null to indicate valid */ - public String validateValue(String key, String value) { + public String validateValue(String key, String value, OsmPrimitive p) { try { for (final ConditionalValue conditional : ConditionalValue.parse(value)) { // validate restriction value @@ -202,7 +194,8 @@ // validate opening hour if the value contains an hour (heuristic) for (final String condition : conditional.conditions) { if (condition.matches(".*[0-9]:[0-9]{2}.*")) { - final List errors = openingHourTest.checkOpeningHourSyntax("", condition); + final List errors = openingHourTest.checkOpeningHourSyntax("", condition, p, + Locale.ENGLISH); if (!errors.isEmpty()) { return errors.get(0).getDescription(); } @@ -233,7 +226,7 @@ continue; } final String value = p.get(key); - final String error = validateValue(key, value); + final String error = validateValue(key, value, p); if (error != null) { errors.add(TestError.builder(this, Severity.WARNING, 3202) .message(tr("Error in {0} value: {1}", key, error)) Index: src/org/openstreetmap/josm/data/validation/tests/OpeningHourTest.java =================================================================== --- src/org/openstreetmap/josm/data/validation/tests/OpeningHourTest.java (revision 16178) +++ src/org/openstreetmap/josm/data/validation/tests/OpeningHourTest.java (working copy) @@ -11,16 +11,18 @@ import java.util.Locale; import java.util.Objects; -import ch.poole.openinghoursparser.OpeningHoursParser; -import ch.poole.openinghoursparser.ParseException; -import ch.poole.openinghoursparser.Rule; -import ch.poole.openinghoursparser.Util; import org.openstreetmap.josm.command.ChangePropertyCommand; import org.openstreetmap.josm.data.osm.OsmPrimitive; import org.openstreetmap.josm.data.validation.Severity; import org.openstreetmap.josm.data.validation.Test.TagTest; import org.openstreetmap.josm.data.validation.TestError; +import org.openstreetmap.josm.tools.CheckParameterUtil; +import ch.poole.openinghoursparser.OpeningHoursParser; +import ch.poole.openinghoursparser.ParseException; +import ch.poole.openinghoursparser.Rule; +import ch.poole.openinghoursparser.Util; + /** * Tests the correct usage of the opening hour syntax of the tags * {@code opening_hours}, {@code collection_times}, {@code service_times} according to @@ -45,16 +47,15 @@ * @param severity The error severity * @param message The error message * @param key The incriminated key, used for display. - * @param value The incriminated value, used for comparison with prettified value. * @param prettifiedValue The prettified value * @param p The incriminated OSM primitive. * @return The real test error given to JOSM validator. Can be fixable or not if a prettified values has been determined. */ - private TestError createTestError(Severity severity, String message, String key, String value, String prettifiedValue, OsmPrimitive p) { + private TestError createTestError(Severity severity, String message, String key, String prettifiedValue, OsmPrimitive p) { final TestError.Builder error = TestError.builder(this, severity, 2901) .message(tr("Opening hours syntax"), message) // todo obtain English message for ignore functionality - .primitives(p != null ? new OsmPrimitive[] {p} : new OsmPrimitive[] {}); - if (p == null || prettifiedValue == null || prettifiedValue.equals(value)) { + .primitives(p); + if (prettifiedValue == null || prettifiedValue.equals(p.get(key))) { return error.build(); } else { return error.fix(() -> new ChangePropertyCommand(p, key, prettifiedValue)).build(); @@ -64,17 +65,6 @@ /** * Checks for a correct usage of the opening hour syntax of the {@code value} given, * and returns a list containing validation errors or an empty list. Null values result in an empty list. - * @param key the OSM key (should be "opening_hours", "collection_times" or "service_times"). Used in error message - * @param value the opening hour value to be checked. - * @return a list of {@link TestError} or an empty list - */ - public List checkOpeningHourSyntax(final String key, final String value) { - return checkOpeningHourSyntax(key, value, null, Locale.getDefault()); - } - - /** - * Checks for a correct usage of the opening hour syntax of the {@code value} given, - * and returns a list containing validation errors or an empty list. Null values result in an empty list. * @param key the OSM key (should be "opening_hours", "collection_times" or "service_times"). * @param value the opening hour value to be checked. * @param p the primitive to check/fix. @@ -82,6 +72,7 @@ * @return a list of {@link TestError} or an empty list */ List checkOpeningHourSyntax(final String key, final String value, OsmPrimitive p, Locale locale) { + CheckParameterUtil.ensureParameterNotNull(p, "primitive"); if (value == null || value.isEmpty()) { return Collections.emptyList(); } @@ -96,7 +87,7 @@ new OpeningHoursParser(new StringReader(value)).rules(true); } } catch (ParseException e) { - return Collections.singletonList(createTestError(Severity.WARNING, e.getMessage(), key, value, prettifiedValue, p)); + return Collections.singletonList(createTestError(Severity.WARNING, e.getMessage(), key, prettifiedValue, p)); } if (!includeOtherSeverityChecks() || Objects.equals(value, prettifiedValue)) { @@ -103,7 +94,7 @@ return Collections.emptyList(); } else { final String message = tr("{0} value can be prettified", key); - return Collections.singletonList(createTestError(Severity.OTHER, message, key, value, prettifiedValue, p)); + return Collections.singletonList(createTestError(Severity.OTHER, message, key, prettifiedValue, p)); } } Index: test/unit/org/openstreetmap/josm/data/validation/tests/ConditionalKeysTest.java =================================================================== --- test/unit/org/openstreetmap/josm/data/validation/tests/ConditionalKeysTest.java (revision 16178) +++ test/unit/org/openstreetmap/josm/data/validation/tests/ConditionalKeysTest.java (working copy) @@ -7,6 +7,9 @@ import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.openstreetmap.josm.data.coor.LatLon; +import org.openstreetmap.josm.data.osm.DataSet; +import org.openstreetmap.josm.data.osm.Node; import org.openstreetmap.josm.testutils.JOSMTestRules; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; @@ -51,22 +54,36 @@ } /** - * Unit test of {@link ConditionalKeys#isValueValid}. + * Unit test of {@link ConditionalKeys#validateValue}. */ @Test public void testValueValid() { - assertTrue(test.isValueValid("maxspeed:conditional", "120 @ (06:00-19:00)")); - assertFalse(test.isValueValid("maxspeed:conditional", " @ (06:00-19:00)")); - assertFalse(test.isValueValid("maxspeed:conditional", "120 (06:00-19:00)")); - assertFalse(test.isValueValid("maxspeed:conditional", "120 @ ()")); - assertFalse(test.isValueValid("maxspeed:conditional", "120 @ ")); - assertFalse(test.isValueValid("maxspeed:conditional", "120 @ (06:00/19:00)")); - assertTrue(test.isValueValid("maxspeed:conditional", "120 @ (06:00-20:00); 100 @ (22:00-06:00)")); - assertTrue(test.isValueValid("motor_vehicle:conditional", "delivery @ (Mo-Fr 06:00-11:00,17:00-19:00;Sa 03:30-19:00)")); - assertTrue(test.isValueValid("motor_vehicle:conditional", "no @ (10:00-18:00 AND length>5)")); - assertFalse(test.isValueValid("motor_vehicle:conditional", "foo @ (10:00-18:00 AND length>5)")); - assertFalse(test.isValueValid("motor_vehicle:conditional", "no @ (10:00until18:00 AND length>5)")); - assertTrue(test.isValueValid("maxspeed:hgv:conditional", "60 @ (weight>7.5)")); - assertTrue(test.isValueValid("restriction:conditional", "no_left_turn @ (Mo-Fr 16:00-18:00)")); + assertTrue(isValueValid("maxspeed:conditional", "120 @ (06:00-19:00)")); + assertFalse(isValueValid("maxspeed:conditional", " @ (06:00-19:00)")); + assertFalse(isValueValid("maxspeed:conditional", "120 (06:00-19:00)")); + assertFalse(isValueValid("maxspeed:conditional", "120 @ ()")); + assertFalse(isValueValid("maxspeed:conditional", "120 @ ")); + assertFalse(isValueValid("maxspeed:conditional", "120 @ (06:00/19:00)")); + assertTrue(isValueValid("maxspeed:conditional", "120 @ (06:00-20:00); 100 @ (22:00-06:00)")); + assertTrue(isValueValid("motor_vehicle:conditional", "delivery @ (Mo-Fr 06:00-11:00,17:00-19:00;Sa 03:30-19:00)")); + assertTrue(isValueValid("motor_vehicle:conditional", "no @ (10:00-18:00 AND length>5)")); + assertFalse(isValueValid("motor_vehicle:conditional", "foo @ (10:00-18:00 AND length>5)")); + assertFalse(isValueValid("motor_vehicle:conditional", "no @ (10:00until18:00 AND length>5)")); + assertTrue(isValueValid("maxspeed:hgv:conditional", "60 @ (weight>7.5)")); + assertTrue(isValueValid("restriction:conditional", "no_left_turn @ (Mo-Fr 16:00-18:00)")); } + + /** + * Check if a value is valid + * @param key The key the value is for + * @param value The value + * @return true if it is valid + */ + private boolean isValueValid(String key, String value) { + final Node node = new Node(LatLon.ZERO); + node.put(key, value); + new DataSet(node); + return test.validateValue(key, value, node) == null; + } + }