Changeset 18636 in josm

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

Files:

• src/org/openstreetmap/josm/data/validation/OsmValidator.java (modified) (9 diffs)
• src/org/openstreetmap/josm/data/validation/TestError.java (modified) (16 diffs)
• src/org/openstreetmap/josm/io/GeoJSONMapRouletteWriter.java (modified) (1 diff)
• test/unit/org/openstreetmap/josm/data/validation/TestErrorTest.java (added)
• test/unit/org/openstreetmap/josm/plugins/PluginHandlerTestIT.java (modified) (11 diffs)

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());

trunk/test/unit/org/openstreetmap/josm/plugins/PluginHandlerTestIT.java

@@ -4 +4 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
@@ -17 +18 @@
 import java.util.Map;
 import java.util.Map.Entry;
+import java.util.Objects;
 import java.util.Set;
 import java.util.function.Consumer;
+import java.util.logging.Handler;
+import java.util.logging.LogRecord;
 import java.util.stream.Collectors;
 
@@ -24 +28 @@
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.platform.commons.util.ReflectionUtils;
 import org.openstreetmap.josm.TestUtils;
 import org.openstreetmap.josm.data.Preferences;
@@ -74 +79 @@
         Map<String, Throwable> loadingExceptions = PluginHandler.pluginLoadingExceptions.entrySet().stream()
                 .filter(e -> !(Utils.getRootCause(e.getValue()) instanceof HeadlessException))
-                .collect(Collectors.toMap(e -> e.getKey(), e -> Utils.getRootCause(e.getValue())));
+                .collect(Collectors.toMap(Map.Entry::getKey, e -> Utils.getRootCause(e.getValue())));
 
         List<PluginInformation> loadedPlugins = PluginHandler.getPlugins();
@@ -93 +98 @@
         }
 
+        Map<String, String> testCodeHashCollisions = checkForHashCollisions();
+
         Map<String, Throwable> noRestartExceptions = new HashMap<>();
         testCompletelyRestartlessPlugins(loadedPlugins, noRestartExceptions);
@@ -100 +107 @@
         debugPrint(layerExceptions);
         debugPrint(noRestartExceptions);
+        debugPrint(testCodeHashCollisions);
 
         invalidManifestEntries = filterKnownErrors(invalidManifestEntries);
@@ -105 +113 @@
         layerExceptions = filterKnownErrors(layerExceptions);
         noRestartExceptions = filterKnownErrors(noRestartExceptions);
+        testCodeHashCollisions = filterKnownErrors(testCodeHashCollisions);
 
         String msg = errMsg("invalidManifestEntries", invalidManifestEntries) + '\n' +
                 errMsg("loadingExceptions", loadingExceptions) + '\n' +
                 errMsg("layerExceptions", layerExceptions) + '\n' +
-                errMsg("noRestartExceptions", noRestartExceptions);
+                errMsg("noRestartExceptions", noRestartExceptions) + '\n' +
+                errMsg("testCodeHashCollisions", testCodeHashCollisions);
         assertTrue(invalidManifestEntries.isEmpty()
                 && loadingExceptions.isEmpty()
                 && layerExceptions.isEmpty()
-                && noRestartExceptions.isEmpty(), msg);
+                && noRestartExceptions.isEmpty()
+                && testCodeHashCollisions.isEmpty(), msg);
     }
 
@@ -122 +133 @@
     private static void testCompletelyRestartlessPlugins(List<PluginInformation> loadedPlugins,
             Map<String, Throwable> noRestartExceptions) {
+        final List<LogRecord> records = new ArrayList<>();
+        Handler tempHandler = new Handler() {
+            @Override
+            public void publish(LogRecord record) {
+                records.add(record);
+            }
+
+            @Override
+            public void flush() { /* Do nothing */ }
+
+            @Override
+            public void close() throws SecurityException { /* Do nothing */ }
+        };
+        Logging.getLogger().addHandler(tempHandler);
         try {
             List<PluginInformation> restartable = loadedPlugins.parallelStream()
@@ -142 +167 @@
             root.printStackTrace();
             noRestartExceptions.put(findFaultyPlugin(loadedPlugins, root), root);
-        }
+            records.removeIf(record -> Objects.equals(Utils.getRootCause(record.getThrown()), root));
+        } catch (AssertionError assertionError) {
+            noRestartExceptions.put("Plugin load/unload failed", assertionError);
+        } finally {
+            Logging.getLogger().removeHandler(tempHandler);
+            for (LogRecord record : records) {
+                if (record.getThrown() != null) {
+                    Throwable root = Utils.getRootCause(record.getThrown());
+                    root.printStackTrace();
+                    noRestartExceptions.put(findFaultyPlugin(loadedPlugins, root), root);
+                }
+            }
+        }
+    }
+
+    private static Map<String, String> checkForHashCollisions() {
+        Map<Integer, List<String>> codes = new HashMap<>();
+        for (Class<?> clazz : ReflectionUtils.findAllClassesInPackage("org.openstreetmap",
+                org.openstreetmap.josm.data.validation.Test.class::isAssignableFrom, s -> true)) {
+            if (org.openstreetmap.josm.data.validation.Test.class.isAssignableFrom(clazz)
+            && !Objects.equals(org.openstreetmap.josm.data.validation.Test.class, clazz)) {
+                // clazz.getName().hashCode() is how the base error codes are calculated since xxx
+                // We want to avoid cases where the hashcode is too close, so we want to
+                // ensure that there is at least 1m available codes after the hashCode.
+                // This is needed since some plugins pick some really large number, and count up from there.
+                int hashCeil = (int) Math.ceil(clazz.getName().hashCode() / 1_000_000d);
+                int hashFloor = (int) Math.floor(clazz.getName().hashCode() / 1_000_000d);
+                codes.computeIfAbsent(hashCeil, k -> new ArrayList<>()).add(clazz.getName());
+                codes.computeIfAbsent(hashFloor, k -> new ArrayList<>()).add(clazz.getName());
+            }
+        }
+        return codes.entrySet().stream().filter(entry -> entry.getValue().size() > 1).collect(
+                Collectors.toMap(entry -> entry.getKey().toString(), entry -> String.join(", ", entry.getValue())));
     }
 
@@ -154 +211 @@
         System.out.println(invalidManifestEntries.entrySet()
                 .stream()
-                .map(e -> convertEntryToString(e))
+                .map(PluginHandlerTestIT::convertEntryToString)
                 .collect(Collectors.joining(", ")));
     }
@@ -242 +299 @@
             try {
                 ClassLoader cl = PluginHandler.getPluginClassLoader(p.getName());
+                assertNotNull(cl);
                 String pluginPackage = cl.loadClass(p.className).getPackage().getName();
                 for (StackTraceElement e : root.getStackTrace()) {