diff --git a/resources/data/defaultpresets.xml b/resources/data/defaultpresets.xml index 872aa7fb225..07cda709dd9 100644 --- a/resources/data/defaultpresets.xml +++ b/resources/data/defaultpresets.xml @@ -3272,7 +3272,6 @@ - @@ -3808,7 +3807,6 @@ - @@ -6982,11 +6980,11 @@ + - diff --git a/src/org/openstreetmap/josm/data/validation/tests/TagChecker.java b/src/org/openstreetmap/josm/data/validation/tests/TagChecker.java index d0ba44e1d0d..b365140f3f5 100644 --- a/src/org/openstreetmap/josm/data/validation/tests/TagChecker.java +++ b/src/org/openstreetmap/josm/data/validation/tests/TagChecker.java @@ -718,8 +718,8 @@ private void checkPresetsTypes(OsmPrimitive p, Collection matchin // Potential error, unless matching tags are all known by a supported preset Map 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( e -> tp2.data.stream().anyMatch( diff --git a/test/unit/org/openstreetmap/josm/data/validation/tests/TagCheckerTest.java b/test/unit/org/openstreetmap/josm/data/validation/tests/TagCheckerTest.java index 92857b8a033..d77123cea38 100644 --- a/test/unit/org/openstreetmap/josm/data/validation/tests/TagCheckerTest.java +++ b/test/unit/org/openstreetmap/josm/data/validation/tests/TagCheckerTest.java @@ -6,6 +6,7 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.IOException; +import java.util.Collections; import java.util.List; import java.util.function.Consumer; import java.util.stream.Collectors; @@ -17,8 +18,13 @@ import org.openstreetmap.josm.data.osm.OsmPrimitive; 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; @@ -414,4 +420,52 @@ void testTicket21348() throws IOException { final List errors = test(OsmUtils.createPrimitive("node power=tower ref=12")); assertEquals(0, errors.size()); } + + /** + * Non-regression test for Bug #23860. + * 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 errors = test(OsmUtils.createPrimitive("way highway=bus_stop")); + assertEquals(1, errors.size()); + } finally { + originalBusStop.data.remove(duplicateKey); + } + } + + /** + * Non-regression test for Bug #23860. + * 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 errors = test(OsmUtils.createPrimitive("way highway=bus_stop")); + assertEquals(0, errors.size()); + } finally { + originalBusStop.data.remove(duplicateKey); + } + } } diff --git a/test/unit/org/openstreetmap/josm/gui/preferences/map/TaggingPresetPreferenceTestIT.java b/test/unit/org/openstreetmap/josm/gui/preferences/map/TaggingPresetPreferenceTestIT.java index a91b6f749e5..48552302847 100644 --- a/test/unit/org/openstreetmap/josm/gui/preferences/map/TaggingPresetPreferenceTestIT.java +++ b/test/unit/org/openstreetmap/josm/gui/preferences/map/TaggingPresetPreferenceTestIT.java @@ -10,11 +10,15 @@ import java.net.URL; 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; import org.junit.jupiter.api.Timeout; @@ -24,8 +28,13 @@ import org.openstreetmap.josm.data.preferences.sources.ExtendedSourceEntry; 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; import org.openstreetmap.josm.testutils.annotations.HTTPS; @@ -34,6 +43,7 @@ import org.openstreetmap.josm.tools.HttpClient.Response; import org.openstreetmap.josm.tools.ImageProvider; import org.openstreetmap.josm.tools.Logging; +import org.openstreetmap.josm.tools.Utils; import org.xml.sax.SAXException; /** @@ -106,7 +116,7 @@ private void testPresets(Set messages, ExtendedSourceEntry source, List< assertFalse(presets.isEmpty()); 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(); final int code = cr.getResponseCode(); @@ -120,6 +130,9 @@ private void testPresets(Set messages, ExtendedSourceEntry source, List< Logging.error(e); } }); + presets.parallelStream().flatMap(TaggingPresetPreferenceTestIT::checkForDuplicates) + .filter(Objects::nonNull) + .forEach(message -> addOrIgnoreError(source, messages, message, ignoredErrors)); Collection errorsAndWarnings = Logging.getLastErrorAndWarnings(); boolean error = false; for (String message : errorsAndWarnings) { @@ -140,4 +153,63 @@ void addOrIgnoreError(ExtendedSourceEntry source, Set messages, String m messages.add(message); } } + + /** + * Look for duplicate key/value objects + * @param preset to check + * @return The messages to print to console for fixing + */ + private static Stream checkForDuplicates(TaggingPreset preset) { + final HashMap> 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 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 `` elements which will set default values for the chunk. + ArrayList 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 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(); + } }