Changeset 18636 in josm for trunk/src/org/openstreetmap

Timestamp:

2023-01-17T14:41:59+01:00 (2 years ago)

Author:

taylor.smock

Message:

Fix #21423: Prevent error codes from clashing

This works by creating a unique code using the test class name. The new
format for ignore entries will look like UNIQUECODE_CODE_MESSAGE_STATE.
The switchover date for the new entries is set at 2024-01-01 in order to
give users sufficient time to update, such that if a user has multiple
installations of JOSM, all versions will be able to use the same ignore
list. The compatibility code is intended to be removed in 2025.

Location:

trunk/src/org/openstreetmap/josm

Files:

• data/validation/OsmValidator.java (modified) (9 diffs)
• data/validation/TestError.java (modified) (16 diffs)
• io/GeoJSONMapRouletteWriter.java (modified) (1 diff)

trunk/src/org/openstreetmap/josm/data/validation/OsmValidator.java

@@ -113 +113 @@
     private static final Class<Test>[] CORE_TEST_CLASSES = new Class[] {// NOPMD
         /* FIXME - unique error numbers for tests aren't properly unique - ignoring will not work as expected */
+        /* Error codes are class.getName().hashCode() + "_" + oldCode. There should almost never be a collision. */
         DuplicateNode.class, // ID    1 ..   99
         OverlappingWays.class, // ID  101 ..  199
@@ -218 +219 @@
     private static void loadIgnoredErrors() {
         ignoredErrors.clear();
-        if (ValidatorPrefHelper.PREF_USE_IGNORE.get()) {
+        if (Boolean.TRUE.equals(ValidatorPrefHelper.PREF_USE_IGNORE.get())) {
             Config.getPref().getListOfMaps(ValidatorPrefHelper.PREF_IGNORELIST).forEach(ignoredErrors::putAll);
             Path path = Paths.get(getValidatorDir()).resolve("ignorederrors");
@@ -224 +225 @@
                 if (path.toFile().exists()) {
                     try {
-                        TreeSet<String> treeSet = new TreeSet<>();
-                        treeSet.addAll(Files.readAllLines(path, StandardCharsets.UTF_8));
+                        TreeSet<String> treeSet = new TreeSet<>(Files.readAllLines(path, StandardCharsets.UTF_8));
                         treeSet.forEach(ignore -> ignoredErrors.putIfAbsent(ignore, ""));
                         removeLegacyEntries(true);
@@ -247 +247 @@
     private static void removeLegacyEntries(boolean force) {
         // see #19053:
+        boolean wasChanged = removeLegacyEntry(force, true, "3000");
+        // see #18230 (pt_assistant, RightAngleBuildingTest)
+        wasChanged |= removeLegacyEntry(force, false, "3701");
+
+        if (wasChanged) {
+            saveIgnoredErrors();
+        }
+    }
+
+    private static boolean removeLegacyEntry(boolean force, boolean keep, String prefix) {
         boolean wasChanged = false;
         if (force) {
@@ -252 +262 @@
             while (iter.hasNext()) {
                 Entry<String, String> entry = iter.next();
-                if (entry.getKey().startsWith("3000_")) {
+                if (entry.getKey().startsWith(prefix + "_")) {
                     Logging.warn(tr("Cannot handle ignore list entry {0}", entry));
                     iter.remove();
@@ -259 +269 @@
             }
         }
-        String legacyEntry = ignoredErrors.remove("3000");
-        if (legacyEntry != null) {
+        String legacyEntry = ignoredErrors.remove(prefix);
+        if (keep && legacyEntry != null) {
             if (!legacyEntry.isEmpty()) {
-                addIgnoredError("3000_" + legacyEntry, legacyEntry);
+                addIgnoredError(prefix + "_" + legacyEntry, legacyEntry);
             }
             wasChanged = true;
         }
-        if (wasChanged) {
-            saveIgnoredErrors();
-        }
+        return wasChanged;
     }
 
@@ -503 +511 @@
         cleanupIgnoredErrors();
         ignoredErrors.remove("3000"); // see #19053
+        ignoredErrors.remove("3701"); // see #18230
         list.add(ignoredErrors);
         int i = 0;
@@ -608 +617 @@
 
     /**
-     * Initialize grid details based on current projection system. Values based on
+     * Initialize grid details based on the current projection system. Values based on
      * the original value fixed for EPSG:4326 (10000) using heuristics (that is, test&amp;error
      * until most bugs were discovered while keeping the processing time reasonable)
@@ -637 +646 @@
 
     /**
-     * Initializes all tests if this operations hasn't been performed already.
+     * Initializes all tests if this operation hasn't been performed already.
      */
     public static synchronized void initializeTests() {

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

@@ -5 +5 @@
 import java.awt.geom.PathIterator;
 import java.text.MessageFormat;
+import java.time.Instant;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -11 +12 @@
 import java.util.List;
 import java.util.Locale;
+import java.util.Map;
 import java.util.TreeSet;
 import java.util.function.Supplier;
@@ -34 +36 @@
  */
 public class TestError implements Comparable<TestError> {
+    /**
+     * Used to switch users over to new ignore system, UNIQUE_CODE_MESSAGE_STATE
+     * 1_704_067_200L -> 2024-01-01
+     * We can probably remove this and the supporting code in 2025.
+     */
+    private static boolean switchOver = Instant.now().isAfter(Instant.ofEpochMilli(1_704_067_200L));
     /** is this error on the ignore list */
     private boolean ignored;
@@ -51 +59 @@
     /** Internal code used by testers to classify errors */
     private final int code;
+    /** Internal code used by testers to classify errors. Used for moving between JOSM versions. */
+    private final int uniqueCode;
     /** If this error is selected */
     private boolean selected;
@@ -64 +74 @@
         private final Severity severity;
         private final int code;
+        private final int uniqueCode;
         private String message;
         private String description;
@@ -75 +86 @@
             this.severity = severity;
             this.code = code;
+            this.uniqueCode = this.tester != null ? this.tester.getClass().getName().hashCode() : code;
         }
 
@@ -232 +244 @@
             return new TestError(this);
         }
+    }
+
+    /**
+     * Update error codes on read and save. Used for tests.
+     * @param updateErrorCodes {@code true} to update error codes. See {@link #switchOver} for default.
+     */
+    static void setUpdateErrorCodes(boolean updateErrorCodes) {
+        switchOver = updateErrorCodes;
     }
 
@@ -255 +275 @@
         this.highlighted = builder.highlighted;
         this.code = builder.code;
+        this.uniqueCode = builder.uniqueCode;
         this.fixingCommand = builder.fixingCommand;
     }
@@ -307 +328 @@
      */
     public String getIgnoreState() {
+        return getIgnoreState(false);
+    }
+
+    /**
+     * Get the ignore state
+     * @param useOriginal if {@code true}, use the original code to get the ignore state
+     * @return The ignore state ({@link #getIgnoreGroup} + ignored object list)
+     */
+    private String getIgnoreState(boolean useOriginal) {
         Collection<String> strings = new TreeSet<>();
         for (OsmPrimitive o : primitives) {
@@ -322 +352 @@
             strings.add(type + '_' + o.getId());
         }
-        return strings.stream().map(o -> ':' + o).collect(Collectors.joining("", getIgnoreSubGroup(), ""));
+        return strings.stream().map(o -> ':' + o).collect(Collectors.joining("", getIgnoreSubGroup(useOriginal), ""));
     }
 
@@ -336 +366 @@
 
     private boolean calcIgnored() {
+        // Begin code removal section (backwards compatibility)
+        if (OsmValidator.hasIgnoredError(getIgnoreGroup(true))) {
+            updateIgnoreList(getIgnoreGroup(true), getIgnoreGroup(false));
+            return true;
+        }
+        if (OsmValidator.hasIgnoredError(getIgnoreSubGroup(true))) {
+            updateIgnoreList(getIgnoreSubGroup(true), getIgnoreSubGroup(false));
+            return true;
+        }
+        String oldState = getIgnoreState(true);
+        String state = getIgnoreState(false);
+        if (oldState != null && OsmValidator.hasIgnoredError(oldState)) {
+            updateIgnoreList(oldState, state);
+            return true;
+        }
+        // End code removal section
         if (OsmValidator.hasIgnoredError(getIgnoreGroup()))
             return true;
         if (OsmValidator.hasIgnoredError(getIgnoreSubGroup()))
             return true;
-        String state = getIgnoreState();
         return state != null && OsmValidator.hasIgnoredError(state);
+    }
+
+    /**
+     * Convert old keys to new keys. Only takes effect when {@link #switchOver} is true
+     * @param oldKey The key to replace
+     * @param newKey The new key
+     */
+    private static void updateIgnoreList(String oldKey, String newKey) {
+        if (switchOver) {
+            Map<String, String> errors = OsmValidator.getIgnoredErrors();
+            if (errors.containsKey(oldKey)) {
+                String value = errors.remove(oldKey);
+                errors.put(newKey, value);
+            }
+        }
     }
 
@@ -349 +409 @@
      */
     public String getIgnoreSubGroup() {
+        return getIgnoreSubGroup(false);
+    }
+
+    /**
+     * Get the subgroup for the error
+     * @param useOriginal if {@code true}, use the original code instead of the new unique codes.
+     * @return The ignore subgroup
+     */
+    private String getIgnoreSubGroup(boolean useOriginal) {
         if (code == 3000) {
             // see #19053
             return "3000_" + (description == null ? message : description);
         }
-        String ignorestring = getIgnoreGroup();
+        String ignorestring = getIgnoreGroup(useOriginal);
         if (descriptionEn != null) {
             ignorestring += '_' + descriptionEn;
@@ -366 +435 @@
      */
     public String getIgnoreGroup() {
+        return getIgnoreGroup(false);
+    }
+
+    /**
+     * Get the ignore group
+     * @param useOriginal if {@code true}, use the original code instead of a unique code + original code.
+     *                    Used for reading and understanding old ignore groups.
+     * @return The ignore group.
+     */
+    private String getIgnoreGroup(boolean useOriginal) {
         if (code == 3000) {
             // see #19053
             return "3000_" + getMessage();
         }
-        return Integer.toString(code);
+        if (useOriginal) {
+            return Integer.toString(this.code);
+        }
+        return this.uniqueCode + "_" + this.code;
     }
 
@@ -403 +485 @@
     public int getCode() {
         return code;
+    }
+
+    /**
+     * Get the unique code for this test. Used for ignore lists.
+     * @return The unique code (generated with {@code tester.getClass().getName().hashCode() + code}).
+     * @since xxx
+     */
+    public int getUniqueCode() {
+        return this.uniqueCode;
     }
 
@@ -547 +638 @@
      */
     public boolean isSimilar(TestError other) {
-        return getCode() == other.getCode()
+        return getUniqueCode() == other.getUniqueCode()
+                && getCode() == other.getCode()
                 && getMessage().equals(other.getMessage())
                 && getPrimitives().size() == other.getPrimitives().size()
@@ -571 +663 @@
     @Override
     public String toString() {
-        return "TestError [tester=" + tester + ", code=" + code + ", message=" + message + ']';
+        return "TestError [tester=" + tester + ", unique code=" + this.uniqueCode +
+                ", code=" + code + ", message=" + message + ']';
     }
 

trunk/src/org/openstreetmap/josm/io/GeoJSONMapRouletteWriter.java

@@ -46 +46 @@
         propertiesBuilder.add("message", testError.getMessage());
         Optional.ofNullable(testError.getDescription()).ifPresent(description -> propertiesBuilder.add("description", description));
-        propertiesBuilder.add("code", testError.getCode());
+        propertiesBuilder.add("code", testError.getUniqueCode());
         propertiesBuilder.add("fixable", testError.isFixable());
         propertiesBuilder.add("severity", testError.getSeverity().toString());