Skip to content

Commit

Permalink
Fix #23860: Duplicate key+value in preset causes an ISE in TagChecker
Browse files Browse the repository at this point in the history 
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.

git-svn-id: https://josm.openstreetmap.de/svn/trunk@19195 0c6e7542-c601-0410-84e7-c038aed88b3b
  • Loading branch information
taylor.smock committed Aug 14, 2024
1 parent 75a5206 commit eed40d1
Show file tree
Hide file tree
Showing 4 changed files with 130 additions and 6 deletions.
4 changes: 1 addition & 3 deletions resources/data/defaultpresets.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3272,7 +3272,6 @@
<optional>
<check key="ferry:cable" text="Reaction ferry" />
<space />
<reference ref="oh" />
<reference ref="public_transport_route_optionals" />
<reference ref="pt_route_opt2" />
<space />
Expand Down Expand Up @@ -3808,7 +3807,6 @@
<check key="group_only" text="Group only access" />
</checkgroup>
<combo key="sanitary_dump_station" text="Dump Station" values="yes,public,customers,no" />
<reference ref="wheelchair" />
<reference ref="internet" />
<space />
<combo key="nudism" text="Nudism" values="yes,no,obligatory,designated,customary,permissive" />
Expand Down Expand Up @@ -6982,11 +6980,11 @@
<combo key="generator:method" text="Method" values_context="generator method" values="combustion,gasification,pyrolysis" />
<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 -->
<item name="Water Turbine" icon="presets/power/power_source-water.svg" type="node,closedway,multipolygon" preset_name_label="true">
Expand Down
4 changes: 2 additions & 2 deletions src/org/openstreetmap/josm/data/validation/tests/TagChecker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -718,8 +718,8 @@ private void checkPresetsTypes(OsmPrimitive p, Collection<TaggingPreset> matchin
// Potential error, unless matching tags are all known by a supported preset
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(
e -> tp2.data.stream().anyMatch(
Expand Down
54 changes: 54 additions & 0 deletions test/unit/org/openstreetmap/josm/data/validation/tests/TagCheckerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -414,4 +420,52 @@ void testTicket21348() throws IOException {
final List<TestError> errors = test(OsmUtils.createPrimitive("node power=tower ref=12"));
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);
}
}
}
74 changes: 73 additions & 1 deletion test/unit/org/openstreetmap/josm/gui/preferences/map/TaggingPresetPreferenceTestIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;

/**
Expand Down Expand Up @@ -106,7 +116,7 @@ private void testPresets(Set<String> 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();
Expand All @@ -120,6 +130,9 @@ private void testPresets(Set<String> messages, ExtendedSourceEntry source, List<
Logging.error(e);
}
});
presets.parallelStream().flatMap(TaggingPresetPreferenceTestIT::checkForDuplicates)
.filter(Objects::nonNull)
.forEach(message -> addOrIgnoreError(source, messages, message, ignoredErrors));
Collection<String> errorsAndWarnings = Logging.getLastErrorAndWarnings();
boolean error = false;
for (String message : errorsAndWarnings) {
Expand All @@ -140,4 +153,63 @@ void addOrIgnoreError(ExtendedSourceEntry source, Set<String> 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<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();
}
}

0 comments on commit eed40d1

Please sign in to comment.