Ticket #21423: 21423.3.patch

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

@@ -112 +112 @@
     @SuppressWarnings("unchecked")
     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
         UntaggedNode.class, // ID  201 ..  299
@@ -217 +218 @@
 
     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");
             try {
                 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);
 
@@ -246 +246 @@
 
     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) {
             Iterator<Entry<String, String>> iter = ignoredErrors.entrySet().iterator();
             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();
                     wasChanged = true;
                 }
             }
         }
-        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;
     }
 
     /**
@@ -502 +510 @@
         List<Map<String, String>> list = new ArrayList<>();
         cleanupIgnoredErrors();
         ignoredErrors.remove("3000"); // see #19053
+        ignoredErrors.remove("3701"); // see #18230
         list.add(ignoredErrors);
         int i = 0;
         while (i < list.size()) {
@@ -607 +616 @@
     }
 
     /**
-     * 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)
      */
@@ -636 +645 @@
     private static boolean testsInitialized;
 
     /**
-     * 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() {
         if (!testsInitialized) {

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

@@ -4 +4 @@
 import java.awt.geom.Area;
 import java.awt.geom.PathIterator;
 import java.text.MessageFormat;
+import java.time.Instant;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.Locale;
+import java.util.Map;
 import java.util.TreeSet;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
@@ -33 +35 @@
  * @since 3669
  */
 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;
     /** Severity */
@@ -50 +58 @@
     private final Test tester;
     /** 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;
     /** Supplying a command to fix the error */
@@ -63 +73 @@
         private final Test tester;
         private final Severity severity;
         private final int code;
+        private final int uniqueCode;
         private String message;
         private String description;
         private String descriptionEn;
@@ -74 +85 @@
             this.tester = tester;
             this.severity = severity;
             this.code = code;
+            this.uniqueCode = this.tester != null ? this.tester.getClass().getName().hashCode() : code;
         }
 
         /**
@@ -233 +245 @@
         }
     }
 
+    /**
+     * 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;
+    }
+
     /**
      * Starts building a new {@code TestError}
      * @param tester The tester
@@ -254 +274 @@
         this.primitives = builder.primitives;
         this.highlighted = builder.highlighted;
         this.code = builder.code;
+        this.uniqueCode = builder.uniqueCode;
         this.fixingCommand = builder.fixingCommand;
     }
 
@@ -306 +327 @@
      * @return the ignore state for this error or null if any primitive is new
      */
     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) {
             // ignore data not yet uploaded
@@ -321 +351 @@
             }
             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), ""));
     }
 
     /**
@@ -335 +365 @@
     }
 
     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);
+            }
+        }
+    }
+
     /**
      * Gets the ignores subgroup that is more specialized than {@link #getIgnoreGroup()}
      * @return The ignore sub group
      */
     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;
         }
@@ -365 +434 @@
      * @see TestError#getIgnoreSubGroup()
      */
     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;
     }
 
     /**
@@ -404 +486 @@
         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;
+    }
+
     /**
      * Returns true if the error can be fixed automatically
      *
@@ -546 +637 @@
      * @return true if two errors are similar
      */
     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()
                 && getPrimitives().containsAll(other.getPrimitives())
@@ -570 +662 @@
 
     @Override
     public String toString() {
-        return "TestError [tester=" + tester + ", code=" + code + ", message=" + message + ']';
+        return "TestError [tester=" + tester + ", unique code=" + this.uniqueCode +
+                ", code=" + code + ", message=" + message + ']';
     }
 
 }

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

@@ -45 +45 @@
         final JsonObjectBuilder propertiesBuilder = Json.createObjectBuilder();
         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());
         propertiesBuilder.add("severityInteger", testError.getSeverity().getLevel());

new file test/unit/org/openstreetmap/josm/data/validation/TestErrorTest.java

@@ +1 @@
+// License: GPL. For details, see LICENSE file.
+package org.openstreetmap.josm.data.validation;
+
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Stream;
+
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.openstreetmap.josm.TestUtils;
+import org.openstreetmap.josm.data.osm.OsmPrimitive;
+import org.openstreetmap.josm.data.validation.tests.InternetTags;
+import org.openstreetmap.josm.gui.progress.NullProgressMonitor;
+import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
+
+/**
+ * Test class for {@link TestError}
+ */
+@BasicPreferences
+class TestErrorTest {
+    static Stream<Arguments> testCodeCompatibility() {
+        return Stream.of(Arguments.of(InternetTags.class, 3301, 1166507262, false, Collections.singletonList(TestUtils.newNode("url=invalid"))),
+                Arguments.of(InternetTags.class, 3301, 1166507262, true, Collections.singletonList(TestUtils.newNode("url=invalid"))));
+    }
+
+    /**
+     * See #18230/#21423: Keep error codes unique
+     *
+     * @param testClass The test class to use
+     * @param originalCode The expected error code (original)
+     * @param expectedCode The expected error code (new, should be {@code testClass.getName().hashCode()})
+     * @param switchOver {@code true} if the new code should be saved instead of the original code
+     * @param primitiveCollection The primitives to run the test on
+     * @throws ReflectiveOperationException If the test class could not be instantiated (no-op constructor)
+     */
+    @ParameterizedTest
+    @MethodSource
+    void testCodeCompatibility(Class<? extends Test> testClass, int originalCode, int expectedCode,
+                               boolean switchOver, List<OsmPrimitive> primitiveCollection) throws ReflectiveOperationException {
+        // Ensure that this test always works
+        TestError.setUpdateErrorCodes(switchOver);
+        assertEquals(expectedCode, testClass.getName().hashCode());
+        // Run the test
+        final Test test = testClass.getConstructor().newInstance();
+        test.startTest(NullProgressMonitor.INSTANCE);
+        test.visit(primitiveCollection);
+        test.endTest();
+        assertFalse(test.getErrors().isEmpty());
+        assertEquals(1, test.getErrors().size());
+        final TestError testError = test.getErrors().get(0);
+        final String ignoreGroup = testError.getIgnoreGroup();
+        final String ignoreSubGroup = testError.getIgnoreSubGroup();
+        if (primitiveCollection.size() == 1 && primitiveCollection.get(0).isNew()) {
+            assertNull(testError.getIgnoreState());
+            primitiveCollection.get(0).setOsmId(1, 1);
+        }
+        final String ignoreState = testError.getIgnoreState();
+        final String startUniqueCode = expectedCode + "_";
+        assertAll(() -> assertTrue(ignoreGroup.startsWith(startUniqueCode + originalCode)),
+                () -> assertTrue(ignoreSubGroup.startsWith(startUniqueCode + originalCode)),
+                () -> assertTrue(ignoreState.startsWith(startUniqueCode + originalCode)));
+        for (String ignore : Arrays.asList(ignoreGroup, ignoreSubGroup, ignoreState)) {
+            OsmValidator.clearIgnoredErrors();
+            final String oldIgnore = ignore.replace(startUniqueCode, "");
+            OsmValidator.addIgnoredError(oldIgnore);
+            // Add the ignored error
+            assertTrue(testError.updateIgnored());
+            assertAll(() -> assertEquals(switchOver, OsmValidator.hasIgnoredError(ignore)),
+                    () -> assertNotEquals(switchOver, OsmValidator.hasIgnoredError(oldIgnore)));
+
+            OsmValidator.clearIgnoredErrors();
+            OsmValidator.addIgnoredError(ignore);
+            // Add the ignored error
+            assertTrue(testError.updateIgnored());
+            assertAll(() -> assertTrue(OsmValidator.hasIgnoredError(ignore)),
+                    () -> assertFalse(OsmValidator.hasIgnoredError(oldIgnore)));
+        }
+    }
+}

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

@@ -3 +3 @@
 
 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;
 
 import java.awt.GraphicsEnvironment;
@@ -16 +17 @@
 import java.util.List;
 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;
 
 import org.junit.jupiter.api.BeforeAll;
 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;
 import org.openstreetmap.josm.data.gpx.GpxData;
@@ -73 +78 @@
 
         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();
         Map<String, List<String>> invalidManifestEntries = loadedPlugins.stream().filter(pi -> !pi.invalidManifestEntries.isEmpty())
@@ -92 +97 @@
             testPlugin(MainApplication.getLayerManager()::removeLayer, layer, layerExceptions, loadedPlugins);
         }
 
+        Map<String, String> testCodeHashCollisions = checkForHashCollisions();
+
         Map<String, Throwable> noRestartExceptions = new HashMap<>();
         testCompletelyRestartlessPlugins(loadedPlugins, noRestartExceptions);
 
@@ -99 +106 @@
         debugPrint(loadingExceptions);
         debugPrint(layerExceptions);
         debugPrint(noRestartExceptions);
+        debugPrint(testCodeHashCollisions);
 
         invalidManifestEntries = filterKnownErrors(invalidManifestEntries);
         loadingExceptions = filterKnownErrors(loadingExceptions);
         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);
     }
 
     private static String errMsg(String type, Map<String, ?> map) {
@@ -121 +132 @@
 
     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()
                     .filter(info -> PluginHandler.getPlugin(info.name) instanceof Destroyable)
@@ -141 +166 @@
             Throwable root = Utils.getRootCause(t);
             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())));
+    }
 
     private static <T> Map<String, T> filterKnownErrors(Map<String, T> errorMap) {
         return errorMap.entrySet().parallelStream()
@@ -153 +210 @@
     private static void debugPrint(Map<String, ?> invalidManifestEntries) {
         System.out.println(invalidManifestEntries.entrySet()
                 .stream()
-                .map(e -> convertEntryToString(e))
+                .map(PluginHandlerTestIT::convertEntryToString)
                 .collect(Collectors.joining(", ")));
     }
 
@@ -203 +260 @@
         if (GraphicsEnvironment.isHeadless()) {
             for (Iterator<PluginInformation> it = plugins.iterator(); it.hasNext();) {
                 PluginInformation pi = it.next();
-                if (pi.isExternal()) {
+                if (pi.isExternal() && (pi.name.equals("jogl") || pi.requires == null || pi.requires.contains("jogl"))) {
                     System.out.println("Ignoring " + pi.name + " (unofficial plugin in headless mode)");
                     it.remove();
                 }
@@ -241 +298 @@
         for (PluginInformation p : plugins) {
             try {
                 ClassLoader cl = PluginHandler.getPluginClassLoader(p.getName());
+                assertNotNull(cl);
                 String pluginPackage = cl.loadClass(p.className).getPackage().getName();
                 for (StackTraceElement e : root.getStackTrace()) {
                     try {