Subject: [PATCH] Fix #21423: Prevent error codes from clashing --- Index: src/org/openstreetmap/josm/data/validation/OsmValidator.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/org/openstreetmap/josm/data/validation/OsmValidator.java b/src/org/openstreetmap/josm/data/validation/OsmValidator.java --- a/src/org/openstreetmap/josm/data/validation/OsmValidator.java (revision 18633) +++ b/src/org/openstreetmap/josm/data/validation/OsmValidator.java (date 1673546795395) @@ -112,6 +112,7 @@ @SuppressWarnings("unchecked") private static final Class[] 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,14 +218,13 @@ 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 treeSet = new TreeSet<>(); - treeSet.addAll(Files.readAllLines(path, StandardCharsets.UTF_8)); + TreeSet treeSet = new TreeSet<>(Files.readAllLines(path, StandardCharsets.UTF_8)); treeSet.forEach(ignore -> ignoredErrors.putIfAbsent(ignore, "")); removeLegacyEntries(true); @@ -246,28 +246,36 @@ 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> iter = ignoredErrors.entrySet().iterator(); while (iter.hasNext()) { Entry 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,6 +510,7 @@ List> 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,7 +616,7 @@ } /** - * 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&error * until most bugs were discovered while keeping the processing time reasonable) */ @@ -636,7 +645,7 @@ 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) { Index: src/org/openstreetmap/josm/data/validation/TestError.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/org/openstreetmap/josm/data/validation/TestError.java b/src/org/openstreetmap/josm/data/validation/TestError.java --- a/src/org/openstreetmap/josm/data/validation/TestError.java (revision 18633) +++ b/src/org/openstreetmap/josm/data/validation/TestError.java (date 1673550783545) @@ -4,12 +4,14 @@ 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,6 +35,12 @@ * @since 3669 */ public class TestError implements Comparable { + /** + * 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,6 +58,8 @@ 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,6 +73,7 @@ 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,6 +85,7 @@ this.tester = tester; this.severity = severity; this.code = code; + this.uniqueCode = this.tester != null ? this.tester.getClass().getName().hashCode() : code; } /** @@ -233,6 +245,14 @@ } } + /** + * 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,6 +274,7 @@ this.primitives = builder.primitives; this.highlighted = builder.highlighted; this.code = builder.code; + this.uniqueCode = builder.uniqueCode; this.fixingCommand = builder.fixingCommand; } @@ -306,6 +327,15 @@ * @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 strings = new TreeSet<>(); for (OsmPrimitive o : primitives) { // ignore data not yet uploaded @@ -321,7 +351,7 @@ } 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,24 +365,63 @@ } 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 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,11 +434,24 @@ * @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,6 +486,15 @@ 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,7 +637,8 @@ * @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,7 +662,8 @@ @Override public String toString() { - return "TestError [tester=" + tester + ", code=" + code + ", message=" + message + ']'; + return "TestError [tester=" + tester + ", unique code=" + this.uniqueCode + + ", code=" + code + ", message=" + message + ']'; } } Index: src/org/openstreetmap/josm/io/GeoJSONMapRouletteWriter.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/org/openstreetmap/josm/io/GeoJSONMapRouletteWriter.java b/src/org/openstreetmap/josm/io/GeoJSONMapRouletteWriter.java --- a/src/org/openstreetmap/josm/io/GeoJSONMapRouletteWriter.java (revision 18633) +++ b/src/org/openstreetmap/josm/io/GeoJSONMapRouletteWriter.java (date 1673546795418) @@ -45,7 +45,7 @@ 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()); Index: test/unit/org/openstreetmap/josm/data/validation/TestErrorTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/test/unit/org/openstreetmap/josm/data/validation/TestErrorTest.java b/test/unit/org/openstreetmap/josm/data/validation/TestErrorTest.java new file mode 100644 --- /dev/null (date 1673546795425) +++ b/test/unit/org/openstreetmap/josm/data/validation/TestErrorTest.java (date 1673546795425) @@ -0,0 +1,88 @@ +// 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 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 testClass, int originalCode, int expectedCode, + boolean switchOver, List 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))); + } + } +} Index: test/unit/org/openstreetmap/josm/plugins/PluginHandlerTestIT.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/test/unit/org/openstreetmap/josm/plugins/PluginHandlerTestIT.java b/test/unit/org/openstreetmap/josm/plugins/PluginHandlerTestIT.java --- a/test/unit/org/openstreetmap/josm/plugins/PluginHandlerTestIT.java (revision 18633) +++ b/test/unit/org/openstreetmap/josm/plugins/PluginHandlerTestIT.java (date 1673553857660) @@ -3,6 +3,7 @@ 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,13 +17,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,7 +78,7 @@ Map 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 loadedPlugins = PluginHandler.getPlugins(); Map> invalidManifestEntries = loadedPlugins.stream().filter(pi -> !pi.invalidManifestEntries.isEmpty()) @@ -92,6 +97,8 @@ testPlugin(MainApplication.getLayerManager()::removeLayer, layer, layerExceptions, loadedPlugins); } + Map testCodeHashCollisions = checkForHashCollisions(); + Map noRestartExceptions = new HashMap<>(); testCompletelyRestartlessPlugins(loadedPlugins, noRestartExceptions); @@ -99,20 +106,24 @@ 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 map) { @@ -121,6 +132,20 @@ private static void testCompletelyRestartlessPlugins(List loadedPlugins, Map noRestartExceptions) { + final List 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 restartable = loadedPlugins.parallelStream() .filter(info -> PluginHandler.getPlugin(info.name) instanceof Destroyable) @@ -141,8 +166,40 @@ 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 checkForHashCollisions() { + Map> 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 Map filterKnownErrors(Map errorMap) { return errorMap.entrySet().parallelStream() @@ -153,7 +210,7 @@ private static void debugPrint(Map invalidManifestEntries) { System.out.println(invalidManifestEntries.entrySet() .stream() - .map(e -> convertEntryToString(e)) + .map(PluginHandlerTestIT::convertEntryToString) .collect(Collectors.joining(", "))); } @@ -203,7 +260,7 @@ if (GraphicsEnvironment.isHeadless()) { for (Iterator 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,6 +298,7 @@ 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 {