Skip to content

Commit

Permalink
Fix #23290: Exclude incomplete relations from region checks
Browse files Browse the repository at this point in the history 
Also rework `TagCheckerTest` to use parameterized tests.

git-svn-id: https://josm.openstreetmap.de/svn/trunk@19201 0c6e7542-c601-0410-84e7-c038aed88b3b
  • Loading branch information
taylor.smock committed Aug 19, 2024
1 parent ffa514d commit cabf5bf
Show file tree
Hide file tree
Showing 2 changed files with 176 additions and 257 deletions.
115 changes: 64 additions & 51 deletions src/org/openstreetmap/josm/data/validation/tests/TagChecker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,11 @@ public class TagChecker extends TagTest implements TaggingPresetListener {

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;

protected boolean checkKeys;
Expand Down Expand Up @@ -292,46 +297,7 @@ private static void initializeData() throws IOException {
CachedFile cf = new CachedFile(source);
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);
errorSources.append(source).append('\n');
Expand All @@ -344,6 +310,49 @@ private static void initializeData() throws IOException {
"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));
}
}
}
}

/**
* Parse a line found in a configuration file
* @param source name of configuration file
Expand Down Expand Up @@ -434,7 +443,7 @@ private static void initAdditionalPresetsValueData() {
additionalPresetsValueData.addAll(AbstractPrimitive.getUninterestingKeys());
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")));
}

private static void addPresetValue(KeyedItem ky) {
Expand Down Expand Up @@ -583,7 +592,7 @@ private static Set<String> getPresetValues(String key) {
* @since 9023
* @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);
}
Expand Down Expand Up @@ -666,7 +675,7 @@ public void check(OsmPrimitive p) {
}
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());
withErrors.put(p, "FIXME");
Expand Down Expand Up @@ -866,7 +875,11 @@ private static boolean primitiveInRegions(IPrimitive primitive, Collection<Strin
} else if (primitive instanceof IWay) {
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);
}
Expand Down Expand Up @@ -903,7 +916,7 @@ private void checkMultipolygonTags(OsmPrimitive p) {
if (p.hasKey("surface")) {
// 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<String, String> filteredTags = p.getInterestingTags();
filteredTags.remove("type");
Expand All @@ -912,14 +925,14 @@ private void checkMultipolygonTags(OsmPrimitive p) {

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());

}
}
if (builder == null) {
// 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());
}
Expand Down Expand Up @@ -983,9 +996,9 @@ private static boolean hasAcceptedPrimaryTagForMultipolygon(OsmPrimitive p) {
|| p.hasTag("waterway", "riverbank", "dam", "rapids", "dock", "boatyard", "fuel")
|| 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)
|| p.hasTag("flood_prone", OsmUtils.TRUE_VALUE)
Expand Down Expand Up @@ -1258,8 +1271,8 @@ private static boolean isNum(String harmonizedValue) {
}

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");
}

private static String harmonizeKey(String key) {
Expand Down
Loading

0 comments on commit cabf5bf

Please sign in to comment.