Ticket #18956: 18956-alternative.patch

src/org/openstreetmap/josm/data/validation/TestError.java

@@ -219 +219 @@
          * 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();
             }

src/org/openstreetmap/josm/data/validation/tests/ConditionalKeys.java

@@ -8 +8 @@
 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 +123 @@
         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 <code>true</code> 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 +181 @@
      * 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 <code>null</code> 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 +194 @@
                 // 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<TestError> errors = openingHourTest.checkOpeningHourSyntax("", condition);
+                        final List<TestError> errors = openingHourTest.checkOpeningHourSyntax("", condition, p,
+                                Locale.ENGLISH);
                         if (!errors.isEmpty()) {
                             return errors.get(0).getDescription();
                         }
@@ -233 +226 @@
                 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))

src/org/openstreetmap/josm/data/validation/tests/OpeningHourTest.java

@@ -11 +11 @@
 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 +47 @@
      * @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 +65 @@
     /**
      * 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<TestError> 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 +72 @@
      * @return a list of {@link TestError} or an empty list
      */
     List<TestError> checkOpeningHourSyntax(final String key, final String value, OsmPrimitive p, Locale locale) {
+        CheckParameterUtil.ensureParameterNotNull(p, "primitive");
         if (value == null || value.isEmpty()) {
             return Collections.emptyList();
         }
@@ -96 +87 @@
                 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 +94 @@
             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));
         }
     }
 

test/unit/org/openstreetmap/josm/data/validation/tests/ConditionalKeysTest.java

@@ -7 +7 @@
 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 +54 @@
     }
 
     /**
-     * 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 <code>true</code> 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;
+    }
+
 }