Changeset 19195 in josm for trunk

Timestamp:

2024-08-14T19:44:54+02:00 (3 months ago)

Author:

taylor.smock

Message:

Fix #23860: Duplicate key+value in preset causes an ISE in TagChecker

This fixes the actual issue in TagChecker, but also adds a sanity check to
TaggingPresetPreferenceTestIT since it is usually unintended to have
duplicate key/values (and it is always a problem if they are different and are
Key items).

The fix for TagChecker is just keeping whatever value is last. Not ideal, but
it should work 99% of the time since an object won't match the preset if we have
highway=footway and highway=footway2 as Key objects.

Location:

trunk

Files:

• resources/data/defaultpresets.xml (modified) (3 diffs)
• src/org/openstreetmap/josm/data/validation/tests/TagChecker.java (modified) (1 diff)
• test/unit/org/openstreetmap/josm/data/validation/tests/TagCheckerTest.java (modified) (3 diffs)
• test/unit/org/openstreetmap/josm/gui/preferences/map/TaggingPresetPreferenceTestIT.java (modified) (6 diffs)

trunk/resources/data/defaultpresets.xml

@@ -3273 +3273 @@
                 <check key="ferry:cable" text="Reaction ferry" />
                 <space />
-                <reference ref="oh" />
                 <reference ref="public_transport_route_optionals" />
                 <reference ref="pt_route_opt2" />
@@ -3809 +3808 @@
             </checkgroup>
             <combo key="sanitary_dump_station" text="Dump Station" values="yes,public,customers,no" />
-            <reference ref="wheelchair" />
             <reference ref="internet" />
             <space />
@@ -6983 +6981 @@
                 <combo key="generator:type" text="Generator Type" values_searchable="true">
                     <list_entry value="bioreactor" short_description="gasification" />
+                    <list_entry value="boiler" short_description="" />
                     <list_entry value="pyrolysis" short_description="" />
                     <list_entry value="reciprocating_engine" short_description="combustion" />
                     <list_entry value="steam_generator" short_description="combustion" />
                 </combo>
-                <combo key="generator:type" text="Generator Type" values="bioreactor,boiler,reciprocating_engine,steam_generator" />
                 <reference ref="power_output" />
             </item> <!-- Waste Power Generator -->

trunk/src/org/openstreetmap/josm/data/validation/tests/TagChecker.java

@@ -719 +719 @@
             Map<String, String> matchingTags = tp.data.stream()
                     .filter(i -> Boolean.TRUE.equals(i.matches(tags)))
-                    .filter(i -> i instanceof KeyedItem).map(i -> ((KeyedItem) i).key)
-                    .collect(Collectors.toMap(k -> k, tags::get));
+                    .filter(KeyedItem.class::isInstance).map(i -> ((KeyedItem) i).key)
+                    .collect(Collectors.toMap(k -> k, tags::get, (o, n) -> n));
             if (matchingPresetsOK.stream().noneMatch(
                     tp2 -> matchingTags.entrySet().stream().allMatch(

trunk/test/unit/org/openstreetmap/josm/data/validation/tests/TagCheckerTest.java

@@ -7 +7 @@
 
 import java.io.IOException;
+import java.util.Collections;
 import java.util.List;
 import java.util.function.Consumer;
@@ -18 +19 @@
 import org.openstreetmap.josm.data.osm.OsmUtils;
 import org.openstreetmap.josm.data.osm.Tag;
+import org.openstreetmap.josm.data.preferences.sources.ValidatorPrefHelper;
 import org.openstreetmap.josm.data.validation.Severity;
 import org.openstreetmap.josm.data.validation.TestError;
+import org.openstreetmap.josm.gui.tagging.presets.TaggingPreset;
+import org.openstreetmap.josm.gui.tagging.presets.TaggingPresetType;
+import org.openstreetmap.josm.gui.tagging.presets.items.Key;
+import org.openstreetmap.josm.spi.preferences.Config;
 import org.openstreetmap.josm.testutils.annotations.I18n;
 import org.openstreetmap.josm.testutils.annotations.TaggingPresets;
@@ -415 +421 @@
         assertEquals(0, errors.size());
     }
+
+    /**
+     * Non-regression test for <a href="https://josm.openstreetmap.de/ticket/23860">Bug #23860</a>.
+     * Duplicate key
+     * @throws IOException if any I/O error occurs
+     */
+    @Test
+    void testTicket23860Equal() throws IOException {
+        ValidatorPrefHelper.PREF_OTHER.put(true);
+        Config.getPref().putBoolean(TagChecker.PREF_CHECK_PRESETS_TYPES, true);
+        final TaggingPreset originalBusStop = org.openstreetmap.josm.gui.tagging.presets.TaggingPresets.getMatchingPresets(
+                Collections.singleton(TaggingPresetType.NODE), Collections.singletonMap("highway", "bus_stop"), false)
+                .iterator().next();
+        final Key duplicateKey = new Key();
+        duplicateKey.key = "highway";
+        duplicateKey.value = "bus_stop";
+        try {
+            originalBusStop.data.add(duplicateKey);
+            final List<TestError> errors = test(OsmUtils.createPrimitive("way highway=bus_stop"));
+            assertEquals(1, errors.size());
+        } finally {
+            originalBusStop.data.remove(duplicateKey);
+        }
+    }
+
+    /**
+     * Non-regression test for <a href="https://josm.openstreetmap.de/ticket/23860">Bug #23860</a>.
+     * Duplicate key
+     * @throws IOException if any I/O error occurs
+     */
+    @Test
+    void testTicket23860NonEqual() throws IOException {
+        ValidatorPrefHelper.PREF_OTHER.put(true);
+        Config.getPref().putBoolean(TagChecker.PREF_CHECK_PRESETS_TYPES, true);
+        final TaggingPreset originalBusStop = org.openstreetmap.josm.gui.tagging.presets.TaggingPresets.getMatchingPresets(
+                        Collections.singleton(TaggingPresetType.NODE), Collections.singletonMap("highway", "bus_stop"), false)
+                .iterator().next();
+        final Key duplicateKey = new Key();
+        duplicateKey.key = "highway";
+        duplicateKey.value = "bus_stop2";
+        try {
+            originalBusStop.data.add(duplicateKey);
+            final List<TestError> errors = test(OsmUtils.createPrimitive("way highway=bus_stop"));
+            assertEquals(0, errors.size());
+        } finally {
+            originalBusStop.data.remove(duplicateKey);
+        }
+    }
 }

trunk/test/unit/org/openstreetmap/josm/gui/preferences/map/TaggingPresetPreferenceTestIT.java

@@ -11 +11 @@
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Locale;
+import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import org.junit.jupiter.api.BeforeAll;
@@ -25 +29 @@
 import org.openstreetmap.josm.gui.preferences.AbstractExtendedSourceEntryTestCase;
 import org.openstreetmap.josm.gui.tagging.presets.TaggingPreset;
+import org.openstreetmap.josm.gui.tagging.presets.TaggingPresetItem;
 import org.openstreetmap.josm.gui.tagging.presets.TaggingPresetReader;
 import org.openstreetmap.josm.gui.tagging.presets.TaggingPresetsTest;
+import org.openstreetmap.josm.gui.tagging.presets.items.Check;
+import org.openstreetmap.josm.gui.tagging.presets.items.CheckGroup;
+import org.openstreetmap.josm.gui.tagging.presets.items.Key;
+import org.openstreetmap.josm.gui.tagging.presets.items.KeyedItem;
 import org.openstreetmap.josm.gui.tagging.presets.items.Link;
 import org.openstreetmap.josm.spi.preferences.Config;
@@ -35 +44 @@
 import org.openstreetmap.josm.tools.ImageProvider;
 import org.openstreetmap.josm.tools.Logging;
+import org.openstreetmap.josm.tools.Utils;
 import org.xml.sax.SAXException;
 
@@ -107 +117 @@
         TaggingPresetsTest.waitForIconLoading(presets);
         // check that links are correct and not redirections
-        presets.parallelStream().flatMap(x -> x.data.stream().filter(i -> i instanceof Link).map(i -> ((Link) i).getUrl())).forEach(u -> {
+        presets.parallelStream().flatMap(x -> x.data.stream().filter(Link.class::isInstance).map(i -> ((Link) i).getUrl())).forEach(u -> {
             try {
                 Response cr = HttpClient.create(new URL(u), "HEAD").setMaxRedirects(-1).connect();
@@ -121 +131 @@
             }
         });
+        presets.parallelStream().flatMap(TaggingPresetPreferenceTestIT::checkForDuplicates)
+                .filter(Objects::nonNull)
+                .forEach(message -> addOrIgnoreError(source, messages, message, ignoredErrors));
         Collection<String> errorsAndWarnings = Logging.getLastErrorAndWarnings();
         boolean error = false;
@@ -141 +154 @@
         }
     }
+
+    /**
+     * Look for duplicate key/value objects
+     * @param preset to check
+     * @return The messages to print to console for fixing
+     */
+    private static Stream<String> checkForDuplicates(TaggingPreset preset) {
+        final HashMap<String, List<KeyedItem>> dupMap = preset.data.stream()
+                .flatMap(TaggingPresetPreferenceTestIT::getKeyedItems)
+                .collect(Collectors.groupingBy(i -> i.key, HashMap::new, Collectors.toCollection(ArrayList::new)));
+        dupMap.values().forEach(TaggingPresetPreferenceTestIT::removeUnnecessaryDuplicates);
+        dupMap.values().removeIf(l -> l.size() <= 1);
+        if (!dupMap.isEmpty()) {
+            final StringBuilder prefixBuilder = new StringBuilder();
+            if (preset.group != null && preset.group.name != null) {
+                prefixBuilder.append(preset.group.name).append('/');
+            }
+            if (preset.name != null) {
+                prefixBuilder.append(preset.name).append('/');
+            }
+            final String prefix = prefixBuilder.toString();
+            return dupMap.keySet().stream().map(k -> "Duplicate key: " + prefix + k);
+        }
+        return Stream.empty();
+    }
+
+    /**
+     * Remove keys that are technically duplicates, but are otherwise OK due to working around limitations of the XML.
+     * @param l The list of keyed items to look through
+     */
+    private static void removeUnnecessaryDuplicates(List<KeyedItem> l) {
+        // Remove keys that are "truthy" when a check will be on or off. This seems to be used for setting defaults in chunks.
+        // We might want to extend chunks to have child `<key>` elements which will set default values for the chunk.
+        ArrayList<KeyedItem> toRemove = new ArrayList<>(Math.min(4, l.size() / 10));
+        for (Key first : Utils.filteredCollection(l, Key.class)) {
+            for (Check second : Utils.filteredCollection(l, Check.class)) {
+                if (second.value_off.equals(first.value) || second.value_on.equals(first.value)) {
+                    toRemove.add(first);
+                }
+            }
+        }
+        l.removeAll(toRemove);
+    }
+
+    /**
+     * Convert an item to a collection of items (needed for {@link CheckGroup})
+     * @param item The item to convert
+     * @return The {@link KeyedItem}s to use
+     */
+    private static Stream<? extends KeyedItem> getKeyedItems(TaggingPresetItem item) {
+        // We care about cases where a preset has two separate hardcoded values
+        // Check should use default="on|off" and value_(on|off) to control the default.
+        if (item instanceof Key || item instanceof Check) {
+            return Stream.of((KeyedItem) item);
+        } else if (item instanceof CheckGroup) {
+            return ((CheckGroup) item).checks.stream();
+        }
+        return Stream.empty();
+    }
 }