Index: /trunk/src/org/openstreetmap/josm/data/validation/tests/TagChecker.java =================================================================== --- /trunk/src/org/openstreetmap/josm/data/validation/tests/TagChecker.java (revision 19200) +++ /trunk/src/org/openstreetmap/josm/data/validation/tests/TagChecker.java (revision 19201) @@ -184,4 +184,9 @@ private static final int MAX_LEVENSHTEIN_DISTANCE = 2; + /* Common string values */ + private static final String FIXME_STR = marktr("fixme"); + private static final String MULTIPOLYGON_TAGS = marktr("Multipolygon tags"); + private static final String SEAMARK_TYPE = "seamark:type"; + protected boolean includeOtherSeverity; @@ -293,44 +298,5 @@ BufferedReader reader = cf.getContentReader() ) { - String okValue = null; - boolean tagcheckerfile = false; - boolean ignorefile = false; - boolean isFirstLine = true; - String line; - while ((line = reader.readLine()) != null) { - if (line.isEmpty()) { - // ignore - } else if (line.startsWith("#")) { - if (line.startsWith("# JOSM TagChecker")) { - tagcheckerfile = true; - Logging.error(tr("Ignoring {0}. Support was dropped", source)); - } else - if (line.startsWith("# JOSM IgnoreTags")) { - ignorefile = true; - if (!DEFAULT_SOURCES.contains(source)) { - Logging.info(tr("Adding {0} to ignore tags", source)); - } - } - } else if (ignorefile) { - parseIgnoreFileLine(source, line); - } else if (tagcheckerfile) { - // ignore - } else if (line.charAt(0) == '+') { - okValue = line.substring(1); - } else if (line.charAt(0) == '-' && okValue != null) { - String hk = harmonizeKey(line.substring(1)); - if (!okValue.equals(hk) && harmonizedKeys.put(hk, okValue) != null && Logging.isDebugEnabled()) { - Logging.debug("Line was ignored: " + line); - } - } else { - Logging.error(tr("Invalid spellcheck line: {0}", line)); - } - if (isFirstLine) { - isFirstLine = false; - if (!(tagcheckerfile || ignorefile) && !DEFAULT_SOURCES.contains(source)) { - Logging.info(tr("Adding {0} to spellchecker", source)); - } - } - } + parseSource(source, reader); } catch (IOException e) { Logging.error(e); @@ -343,4 +309,47 @@ "Could not access data file:\n{0}", "Could not access data files:\n{0}", errorSources.length(), errorSources)); + } + + private static void parseSource(String source, BufferedReader reader) throws IOException { + String okValue = null; + boolean tagcheckerfile = false; + boolean ignorefile = false; + boolean isFirstLine = true; + String line; + while ((line = reader.readLine()) != null) { + if (line.isEmpty()) { + // ignore + } else if (line.startsWith("#")) { + if (line.startsWith("# JOSM TagChecker")) { + tagcheckerfile = true; + Logging.error(tr("Ignoring {0}. Support was dropped", source)); + } else + if (line.startsWith("# JOSM IgnoreTags")) { + ignorefile = true; + if (!DEFAULT_SOURCES.contains(source)) { + Logging.info(tr("Adding {0} to ignore tags", source)); + } + } + } else if (ignorefile) { + parseIgnoreFileLine(source, line); + } else if (tagcheckerfile) { + // ignore + } else if (line.charAt(0) == '+') { + okValue = line.substring(1); + } else if (line.charAt(0) == '-' && okValue != null) { + String hk = harmonizeKey(line.substring(1)); + if (!okValue.equals(hk) && harmonizedKeys.put(hk, okValue) != null && Logging.isDebugEnabled()) { + Logging.debug("Line was ignored: " + line); + } + } else { + Logging.error(tr("Invalid spellcheck line: {0}", line)); + } + if (isFirstLine) { + isFirstLine = false; + if (!(tagcheckerfile || ignorefile) && !DEFAULT_SOURCES.contains(source)) { + Logging.info(tr("Adding {0} to spellchecker", source)); + } + } + } } @@ -435,5 +444,5 @@ additionalPresetsValueData.addAll(Config.getPref().getList( ValidatorPrefHelper.PREFIX + ".knownkeys", - Arrays.asList("is_in", "int_ref", "fixme", "population"))); + Arrays.asList("is_in", "int_ref", FIXME_STR, "population"))); } @@ -584,5 +593,5 @@ * @deprecated since 18281 -- use {@link TaggingPresets#isKeyInPresets(String)} instead */ - @Deprecated + @Deprecated(since = "18281", forRemoval = true) public static boolean isKeyInPresets(String key) { return TaggingPresets.isKeyInPresets(key); @@ -667,5 +676,5 @@ if (checkFixmes && key != null && !Utils.isEmpty(value) && isFixme(key, value) && !withErrors.contains(p, "FIXME")) { errors.add(TestError.builder(this, Severity.OTHER, FIXME) - .message(tr("fixme")) + .message(tr(FIXME_STR)) .primitives(p) .build()); @@ -867,5 +876,9 @@ return ((IWay) primitive).getNodes().stream().anyMatch(n -> primitiveInRegions(n, regions, excludeRegions)); } else if (primitive instanceof IRelation) { - return ((IRelation) primitive).getMemberPrimitivesList().stream().anyMatch(p -> primitiveInRegions(p, regions, excludeRegions)); + final IRelation relation = (IRelation) primitive; + if (relation.hasIncompleteMembers()) { + return true; // Assume that the relation has members in a valid area. See #23290. + } + return relation.getMemberPrimitivesList().stream().anyMatch(p -> primitiveInRegions(p, regions, excludeRegions)); } throw new IllegalArgumentException("Unknown primitive type: " + primitive); @@ -904,5 +917,5 @@ // accept often used tag surface=* as area tag builder = TestError.builder(this, Severity.OTHER, MULTIPOLYGON_INCOMPLETE) - .message(tr("Multipolygon tags"), marktr("only {0} tag"), "surface"); + .message(tr(MULTIPOLYGON_TAGS), marktr("only {0} tag"), "surface"); } else { Map filteredTags = p.getInterestingTags(); @@ -913,5 +926,5 @@ if (filteredTags.isEmpty()) { builder = TestError.builder(this, Severity.ERROR, MULTIPOLYGON_NO_AREA) - .message(tr("Multipolygon tags"), marktr("tag describing the area is missing"), new Object()); + .message(tr(MULTIPOLYGON_TAGS), marktr("tag describing the area is missing"), new Object()); } @@ -920,5 +933,5 @@ // multipolygon has either no area tag or a rarely used one builder = TestError.builder(this, Severity.WARNING, MULTIPOLYGON_MAYBE_NO_AREA) - .message(tr("Multipolygon tags"), marktr("tag describing the area might be missing"), new Object()); + .message(tr(MULTIPOLYGON_TAGS), marktr("tag describing the area might be missing"), new Object()); } errors.add(builder.primitives(p).build()); @@ -984,7 +997,7 @@ || p.hasTag("indoor", "corridor", "room", "area") || p.hasTag("power", "substation", "generator", "plant", "switchgear", "converter", "sub_station") - || p.hasTag("seamark:type", "harbour", "fairway", "anchorage", "landmark", "berth", "harbour_basin", + || p.hasTag(SEAMARK_TYPE, "harbour", "fairway", "anchorage", "landmark", "berth", "harbour_basin", "separation_zone") - || (p.get("seamark:type") != null && p.get("seamark:type").matches(".*\\_(area|zone)$"))) + || (p.get(SEAMARK_TYPE) != null && p.get(SEAMARK_TYPE).matches(".*\\_(area|zone)$"))) return true; return p.hasTag("harbour", OsmUtils.TRUE_VALUE) @@ -1259,6 +1272,6 @@ private static boolean isFixme(String key, String value) { - return key.toLowerCase(Locale.ENGLISH).contains("fixme") || key.contains("todo") - || value.toLowerCase(Locale.ENGLISH).contains("fixme") || value.contains("check and delete"); + return key.toLowerCase(Locale.ENGLISH).contains(FIXME_STR) || key.contains("todo") + || value.toLowerCase(Locale.ENGLISH).contains(FIXME_STR) || value.contains("check and delete"); } Index: /trunk/test/unit/org/openstreetmap/josm/data/validation/tests/TagCheckerTest.java =================================================================== --- /trunk/test/unit/org/openstreetmap/josm/data/validation/tests/TagCheckerTest.java (revision 19200) +++ /trunk/test/unit/org/openstreetmap/josm/data/validation/tests/TagCheckerTest.java (revision 19201) @@ -7,16 +7,26 @@ import java.io.IOException; +import java.text.MessageFormat; import java.util.Collections; import java.util.List; import java.util.function.Consumer; import java.util.stream.Collectors; +import java.util.stream.Stream; import org.junit.jupiter.api.Assertions; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; +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.coor.LatLon; +import org.openstreetmap.josm.data.osm.DataSet; +import org.openstreetmap.josm.data.osm.Node; import org.openstreetmap.josm.data.osm.OsmPrimitive; import org.openstreetmap.josm.data.osm.OsmUtils; +import org.openstreetmap.josm.data.osm.Relation; +import org.openstreetmap.josm.data.osm.RelationMember; import org.openstreetmap.josm.data.osm.Tag; +import org.openstreetmap.josm.data.osm.Way; import org.openstreetmap.josm.data.preferences.sources.ValidatorPrefHelper; import org.openstreetmap.josm.data.validation.Severity; @@ -44,153 +54,90 @@ checker.initialize(); checker.startTest(null); - checker.check(TestUtils.addFakeDataSet(primitive)); + if (primitive.getDataSet() == null) { + TestUtils.addFakeDataSet(primitive); + } + checker.check(primitive); return checker.getErrors(); } - /** - * Check for misspelled key. - * @throws IOException if any I/O error occurs - */ - @Test - void testMisspelledKey1() throws IOException { - final List errors = test(OsmUtils.createPrimitive("node Name=Main")); + static Stream testTags() { + final String misspelled = "Misspelled property key"; + final String unknown = "Unknown property value"; + final String noPropertyValue = "Presets do not contain property value"; + final String looksLike = "Key ''{0}'' looks like ''{1}''."; + final String invalidPreset = "Key from a preset is invalid in this region"; + return Stream.of( + // Check for misspelled key. + Arguments.of("testMisspelledKey1", "node Name=Main", misspelled, + MessageFormat.format(looksLike, "Name", "name"), Severity.WARNING, true), + // Check for misspelled key. + Arguments.of("testMisspelledKey2", "node landuse;=forest", misspelled, + MessageFormat.format(looksLike, "landuse;", "landuse"), Severity.WARNING, true), + // Check for misspelled key where the suggested alternative is in use. The error should not be fixable. + Arguments.of("testMisspelledKeyButAlternativeInUse", "node amenity=fuel brand=bah Brand=foo", misspelled, + MessageFormat.format(looksLike, "Brand", "brand"), Severity.WARNING, false), + // Check for misspelled key where the suggested alternative is given with prefix E: in ignoreTags.cfg. + // The error should be fixable. + // ticket 17468 + Arguments.of("testUpperCaseIgnoredKey", "node wheelchair:Description=bla", misspelled, + MessageFormat.format(looksLike, "wheelchair:Description", "wheelchair:description"), Severity.WARNING, true), + // Check for misspelled key where the suggested alternative is given with prefix K: in ignoreTags.cfg. + // The error should be fixable. + // ticket 17468 + Arguments.of("testUpperCaseInKeyIgnoredTag", "node land_Area=administrative", misspelled, + MessageFormat.format(looksLike, "land_Area", "land_area"), Severity.WARNING, true), + // Check for unknown key + Arguments.of("testTranslatedNameKey", "node namez=Baz", + "Presets do not contain property key", "Key 'namez' not in presets.", Severity.OTHER, false), + // Check for misspelled value + Arguments.of("testMisspelledTag", "node landuse=forrest", unknown, + "Value 'forrest' for key 'landuse' is unknown, maybe 'forest' is meant?", Severity.WARNING, false), + // Check for misspelled value with multiple alternatives in presets. + Arguments.of("testMisspelledTag2", "node highway=servics", unknown, + "Value 'servics' for key 'highway' is unknown, maybe one of [service, services] is meant?", Severity.WARNING, false), + // Check for misspelled value. + Arguments.of("testMisspelledTag3", "node highway=residentail", unknown, + "Value 'residentail' for key 'highway' is unknown, maybe 'residential' is meant?", Severity.WARNING, false), + // Check for misspelled value. + Arguments.of("testShortValNotInPreset2", "node shop=abs", noPropertyValue, + "Value 'abs' for key 'shop' not in presets.", Severity.OTHER, false), + // Check regression: Don't fix surface=u -> surface=mud. + Arguments.of("testTooShortToFix", "node surface=u", noPropertyValue, + "Value 'u' for key 'surface' not in presets.", Severity.OTHER, false), + // Check value with upper case + Arguments.of("testValueDifferentCase", "node highway=Residential", unknown, + "Value 'Residential' for key 'highway' is unknown, maybe 'residential' is meant?", Severity.WARNING, false), + Arguments.of("testRegionKey", "node payment:ep_avant=yes", invalidPreset, + "Preset Payment Methods should not have the key payment:ep_avant", Severity.WARNING, false), + Arguments.of("testRegionTag", "relation type=waterway gnis:feature_id=123456", invalidPreset, + "Preset Waterway should not have the key gnis:feature_id", Severity.WARNING, false), + // Key in presets but not in ignored.cfg. Caused a NPE with r14727. + Arguments.of("testRegression17246", "node access=privat", unknown, + "Value 'privat' for key 'access' is unknown, maybe 'private' is meant?", Severity.WARNING, false) + ); + } + + /** + * Test tags + * @param name The name of the test + * @param primitive The primitive definition to use + * @param expectedMessage The expected error message + * @param expectedDescription The expected error description + * @param severity The expected severity + * @param isFixable {@code true} if the error should be fixable + * @throws IOException See {@link #test(OsmPrimitive)} + */ + @ParameterizedTest(name = "{0}") + @MethodSource("testTags") + void testTags(String name, String primitive, String expectedMessage, String expectedDescription, Severity severity, boolean isFixable) + throws IOException { + final List errors = test(OsmUtils.createPrimitive(primitive)); assertEquals(1, errors.size()); - assertEquals("Misspelled property key", errors.get(0).getMessage()); - assertEquals("Key 'Name' looks like 'name'.", errors.get(0).getDescription()); - assertTrue(errors.get(0).isFixable()); - } - - /** - * Check for misspelled key. - * @throws IOException if any I/O error occurs - */ - @Test - void testMisspelledKey2() throws IOException { - final List errors = test(OsmUtils.createPrimitive("node landuse;=forest")); - assertEquals(1, errors.size()); - assertEquals("Misspelled property key", errors.get(0).getMessage()); - assertEquals("Key 'landuse;' looks like 'landuse'.", errors.get(0).getDescription()); - assertTrue(errors.get(0).isFixable()); - } - - /** - * Check for misspelled key where the suggested alternative is in use. The error should not be fixable. - * @throws IOException if any I/O error occurs - */ - @Test - void testMisspelledKeyButAlternativeInUse() throws IOException { - // ticket 12329 - final List errors = test(OsmUtils.createPrimitive("node amenity=fuel brand=bah Brand=foo")); - assertEquals(1, errors.size()); - assertEquals("Misspelled property key", errors.get(0).getMessage()); - assertEquals("Key 'Brand' looks like 'brand'.", errors.get(0).getDescription()); - assertEquals(Severity.WARNING, errors.get(0).getSeverity()); - assertFalse(errors.get(0).isFixable()); - } - - /** - * Check for misspelled key where the suggested alternative is given with prefix E: in ignoreTags.cfg. - * The error should be fixable. - * @throws IOException if any I/O error occurs - */ - @Test - void testUpperCaseIgnoredKey() throws IOException { - // ticket 17468 - final List errors = test(OsmUtils.createPrimitive("node wheelchair:Description=bla")); - assertEquals(1, errors.size()); - assertEquals("Misspelled property key", errors.get(0).getMessage()); - assertEquals("Key 'wheelchair:Description' looks like 'wheelchair:description'.", errors.get(0).getDescription()); - assertEquals(Severity.WARNING, errors.get(0).getSeverity()); - assertTrue(errors.get(0).isFixable()); - } - - /** - * Check for misspelled key where the suggested alternative is given with prefix K: in ignoreTags.cfg. - * The error should be fixable. - * @throws IOException if any I/O error occurs - */ - @Test - void testUpperCaseInKeyIgnoredTag() throws IOException { - // ticket 17468 - final List errors = test(OsmUtils.createPrimitive("node land_Area=administrative")); - assertEquals(1, errors.size()); - assertEquals("Misspelled property key", errors.get(0).getMessage()); - assertEquals("Key 'land_Area' looks like 'land_area'.", errors.get(0).getDescription()); - assertEquals(Severity.WARNING, errors.get(0).getSeverity()); - assertTrue(errors.get(0).isFixable()); - } - - /** - * Check for unknown key. - * @throws IOException if any I/O error occurs - */ - @Test - void testTranslatedNameKey() throws IOException { - final List errors = test(OsmUtils.createPrimitive("node namez=Baz")); - assertEquals(1, errors.size()); - assertEquals("Presets do not contain property key", errors.get(0).getMessage()); - assertEquals("Key 'namez' not in presets.", errors.get(0).getDescription()); - assertEquals(Severity.OTHER, errors.get(0).getSeverity()); - assertFalse(errors.get(0).isFixable()); - } - - /** - * Check for misspelled value. - * @throws IOException if any I/O error occurs - */ - @Test - void testMisspelledTag() throws IOException { - final List errors = test(OsmUtils.createPrimitive("node landuse=forrest")); - assertEquals(1, errors.size()); - assertEquals("Unknown property value", errors.get(0).getMessage()); - assertEquals("Value 'forrest' for key 'landuse' is unknown, maybe 'forest' is meant?", errors.get(0).getDescription()); - assertEquals(Severity.WARNING, errors.get(0).getSeverity()); - assertFalse(errors.get(0).isFixable()); - } - - /** - * Check for misspelled value with multiple alternatives in presets. - * @throws IOException if any I/O error occurs - */ - @Test - void testMisspelledTag2() throws IOException { - final List errors = test(OsmUtils.createPrimitive("node highway=servics")); - assertEquals(1, errors.size()); - assertEquals("Unknown property value", errors.get(0).getMessage()); - assertEquals( - "Value 'servics' for key 'highway' is unknown, maybe one of [service, services] is meant?", - errors.get(0).getDescription()); - assertEquals(Severity.WARNING, errors.get(0).getSeverity()); - assertFalse(errors.get(0).isFixable()); - } - - /** - * Check for misspelled value. - * @throws IOException if any I/O error occurs - */ - @Test - void testMisspelledTag3() throws IOException { - final List errors = test(OsmUtils.createPrimitive("node highway=residentail")); - assertEquals(1, errors.size()); - assertEquals("Unknown property value", errors.get(0).getMessage()); - assertEquals("Value 'residentail' for key 'highway' is unknown, maybe 'residential' is meant?", - errors.get(0).getDescription()); - assertEquals(Severity.WARNING, errors.get(0).getSeverity()); - assertFalse(errors.get(0).isFixable()); - } - - /** - * Check for misspelled value. - * @throws IOException if any I/O error occurs - */ - @Test - void testShortValNotInPreset2() throws IOException { - final List errors = test(OsmUtils.createPrimitive("node shop=abs")); - assertEquals(1, errors.size()); - assertEquals("Presets do not contain property value", errors.get(0).getMessage()); - assertEquals("Value 'abs' for key 'shop' not in presets.", errors.get(0).getDescription()); - assertEquals(Severity.OTHER, errors.get(0).getSeverity()); - assertFalse(errors.get(0).isFixable()); - } + assertEquals(expectedMessage, errors.get(0).getMessage()); + assertEquals(expectedDescription, errors.get(0).getDescription()); + assertEquals(severity, errors.get(0).getSeverity()); + assertEquals(isFixable, errors.get(0).isFixable()); + } + /** @@ -206,69 +153,4 @@ .collect(Collectors.toList()); assertTrue(errors.isEmpty(), errors::toString); - } - - /** - * Check regression: Don't fix surface=u -> surface=mud. - * @throws IOException if any I/O error occurs - */ - @Test - void testTooShortToFix() throws IOException { - final List errors = test(OsmUtils.createPrimitive("node surface=u")); - assertEquals(1, errors.size()); - assertEquals("Presets do not contain property value", errors.get(0).getMessage()); - assertEquals("Value 'u' for key 'surface' not in presets.", errors.get(0).getDescription()); - assertEquals(Severity.OTHER, errors.get(0).getSeverity()); - assertFalse(errors.get(0).isFixable()); - } - - /** - * Check value with upper case - * @throws IOException if any I/O error occurs - */ - @Test - void testValueDifferentCase() throws IOException { - final List errors = test(OsmUtils.createPrimitive("node highway=Residential")); - assertEquals(1, errors.size()); - assertEquals("Unknown property value", errors.get(0).getMessage()); - assertEquals("Value 'Residential' for key 'highway' is unknown, maybe 'residential' is meant?", - errors.get(0).getDescription()); - assertEquals(Severity.WARNING, errors.get(0).getSeverity()); - assertFalse(errors.get(0).isFixable()); - } - - @Test - void testRegionKey() throws IOException { - final List errors = test(OsmUtils.createPrimitive("node payment:ep_avant=yes")); - assertEquals(1, errors.size()); - assertEquals("Key from a preset is invalid in this region", errors.get(0).getMessage()); - assertEquals("Preset Payment Methods should not have the key payment:ep_avant", errors.get(0).getDescription()); - assertEquals(Severity.WARNING, errors.get(0).getSeverity()); - assertFalse(errors.get(0).isFixable()); - - } - - @Test - void testRegionTag() throws IOException { - final List errors = test(OsmUtils.createPrimitive("relation type=waterway gnis:feature_id=123456")); - assertEquals(1, errors.size()); - assertEquals("Key from a preset is invalid in this region", errors.get(0).getMessage()); - assertEquals("Preset Waterway should not have the key gnis:feature_id", errors.get(0).getDescription()); - assertEquals(Severity.WARNING, errors.get(0).getSeverity()); - assertFalse(errors.get(0).isFixable()); - } - - /** - * Key in presets but not in ignored.cfg. Caused a NPE with r14727. - * @throws IOException if any I/O error occurs - */ - @Test - void testRegression17246() throws IOException { - final List errors = test(OsmUtils.createPrimitive("node access=privat")); - assertEquals(1, errors.size()); - assertEquals("Unknown property value", errors.get(0).getMessage()); - assertEquals("Value 'privat' for key 'access' is unknown, maybe 'private' is meant?", - errors.get(0).getDescription()); - assertEquals(Severity.WARNING, errors.get(0).getSeverity()); - assertFalse(errors.get(0).isFixable()); } @@ -388,5 +270,4 @@ */ @Test - @Disabled("broken, see #19519") void testTicket19519() throws IOException { List errors = test(OsmUtils.createPrimitive("node amenity=restaurant cuisine=bavarian;beef_bowl")); @@ -420,4 +301,29 @@ final List errors = test(OsmUtils.createPrimitive("node power=tower ref=12")); assertEquals(0, errors.size()); + } + + /** + * Non-regression test for Bug #23290 + */ + @Test + void testTicket23290() throws IOException { + final Node france1 = new Node(new LatLon(48.465638, 7.3677049)); + final Node france2 = new Node(new LatLon(48.4191768, 7.7275072)); + final Node german1 = new Node(new LatLon(48.3398223, 8.1683322)); + final Node german2 = new Node(new LatLon(48.4137076, 7.754287)); + final Way frenchRiver = TestUtils.newWay("waterway=river", france1, france2); + final Way germanRiver = TestUtils.newWay("waterway=river", german1, german2); + final Way incompleteWay = new Way(123, 0); + final Relation riverRelation = TestUtils.newRelation("type=waterway waterway=river ref:sandre=A---0000 ref:fgkz=2", + new RelationMember("", germanRiver)); + // Ensure they have the same dataset + new DataSet(france1, france2, frenchRiver, german1, german2, germanRiver, incompleteWay, riverRelation); + assertEquals(1, test(riverRelation).size()); + riverRelation.addMember(new RelationMember("", frenchRiver)); + assertEquals(0, test(riverRelation).size()); + riverRelation.removeMembersFor(frenchRiver); + assertEquals(1, test(riverRelation).size()); + riverRelation.addMember(new RelationMember("", incompleteWay)); + assertEquals(0, test(riverRelation).size()); }