Validate the regions a tag is expected to be in

[taylor.smock]

Original PR by Sarabjeet108: ​https://github.com/JOSM/josm/pull/122

I've made the following modifications:

• Allow the use of the new region attributes for keys inside a preset
• Basic Tests

regions comes from Vespucci's extensions: ​https://vespucci.io/tutorials/presets/#extensions

[anonymous]

crossing_ref is used worldwide, not sure if that should (effectively) be deprecated everywhere except GB?

[taylor.smock]

@anonymous: See #23251: Crossing preset: Change label for crossing_ref.

I've scheduled the UK limitation for rereview in May 2024.

[Famlam]

Just my two cents: could it be considered to delay this new warning until May too? I use crossing_ref all the time in The Netherlands too.

[taylor.smock]

@Famlam: In order to see the new warning, you have to:

1. Enable Run data validator on user input in Tagging Presets settings
2. Enable Show informational level in Data validator.

You can disable the region check under the Tag checker heading. Or ignore the the group.

[Famlam]

Ah, by scanning it I thought it'd trigger the Severity.WARNING message. Apologies.

[taylor.smock]

No worries. Always feel free to ask for clarification.

In this case, the checks are only run if the informational level is enabled.

[taylor.smock]

In 18918/josm:

Fix #23290: Validate the regions a tag is expected to be in (patch by Sarabjeet108, modified)

Modifications are as follows:

• Allow the use of the new region attributes for keys inside a preset
• Basic tests

regions comes from Vespucci's extensions: ​https://vespucci.io/tutorials/presets/#extensions

[skyper]

It would be nice if the new attribute is documented under wiki:TaggingPresets#Attributes. Thanks in advance.

[skyper]

The unit test jenkins/job/JOSM-Integration/9800/jdk=JDK8/testReport/junit/org.openstreetmap.josm.gui.preferences.map/TaggingPresetPreferenceTestIT/Internal_Preset___https___josm_openstreetmap_de_browser_trunk_data_defaultpresets_xml/ is failing, now:

Internal Preset => [resource://data/defaultpresets.xml => org.openstreetmap.josm.tools.XmlParsingException:
org.openstreetmap.josm.tools.XmlParsingException (at line 1,468, column 140)
java.lang.reflect.InvocationTargetException] ==> expected: <true> but was: <false>

Edit: see #23514

Plus there is a s missing at source:/trunk/resources/data/defaultpresets.xml#L8268​.

[taylor.smock]

I noticed the failing integration test. Unfortunately, it passed locally for me.

[skyper]

I tried the "Shops/Payment Methodes" and I am still offered all regional payments and no validator informational warning is created (unlike crossing_ref where a warning is created).

payment:ep_avant=yes
payment:ep_geldkarte=yes
payment:ep_mep=yes
payment:ep_minicash=yes
payment:ep_minipay=yes
payment:ep_monedero4b=yes
payment:postfinance_card=yes
shop=pet

[taylor.smock]

In 18925/josm:

See #23290 comment 10: Fix missing s in region for payment cards

[skyper]

Replying to skyper:

I tried the "Shops/Payment Methodes" and I am still offered all regional payments and no validator informational warning is created (unlike crossing_ref where a warning is created).

Rereading the ticket and commit carefully, I understand that it is only about validating the values. I was hoping that additionally only appropriate values for the region would be offered in the preset.
Now, I wonder if it would be better to use the code of existing territory selectors inside and outside and even use the same naming instead of adding two new names regions and exclude_regions.

[taylor.smock]

If Vespucci didn't already have the extension, I might be more inclined to go with the MapCSS inside/outside for consistency. However, when adding support for an extension from a third-party project (Vespucci), I would strongly prefer to be consistent with the third-party. With a few functional changes (for example, I don't believe Vespucci supports regions/exclude_regions on a per-tag basis).

We haven't documented it yet, and it is a new feature, so I would be willing to change it to match the MapCSS inside/outside naming scheme. But I would want it to be consistent in the ecosystem, which means getting SimonPoole on board.

Pros of current tagging:

• Existing documentation on how to use it with Vespucci
• Minimal breakage for those who want to bring Vespucci only presets to JOSM
• Doesn't match MapCSS, so it is easier to find documentation (josm regions preset instead of josm inside preset)

Cons of current tagging:

• Doesn't match MapCSS, for those who write both MapCSS and presets
• exclude_regions is a flag instead of a list of regions.

[skyper]

Naming could be solved with aliases and documentation can still be present on the preset part (link). Major questions are if the MapCSS functions could be used for the preset part, too, instead of some duplication, and how to handle the differences between outside and exclude_regions.

[taylor.smock]

After much investigation, I figured out why region wasn't flagged:
We extensively use <anyAttribute processContents="skip" />; this skips any attribute that is not understood.
Of course, that now means I have no idea what is going on with the test.

@stoecker: Are there any customizations to the source code for https://josm.openstreetmap.de/jenkins/job/JOSM-Integration ?
I haven't been able to get the test to fail locally (note: I had to modify source:trunk/test/unit/org/openstreetmap/josm/gui/preferences/AbstractExtendedSourceEntryTestCase.java​ to point to the "right" URL for resources -- the generated URL doesn't actually point at a valid resource).

[stoecker]

Replying to taylor.smock:

We extensively use <anyAttribute processContents="skip" />; this skips any attribute that is not understood.
Of course, that now means I have no idea what is going on with the test.

Yes. This is ugly, but a design flaw of XSD in my eyes. I would like "warn and ignore", but that's not possible. Also the language prefixes would make the warnings hard. XSD is limited...

@stoecker: Are there any customizations to the source code for

Don't think so. You should be able to look yourself :-)

[taylor.smock]

Replying to stoecker:

Replying to taylor.smock:

@stoecker: Are there any customizations to the source code for

Don't think so. You should be able to look yourself :-)

I didn't see anything in Jenkins. I'll change the checkout strategy from Use 'svn update' as much as possible to Always check out a fresh copy for the next run (probably the next few runs; I will change it back tomorrow, assuming I don't forget).

[GerdP]

Not sure if this is the right ticket. I see these warnings
"Invalid region for this preset - Preset Traffic Signal should not have the key crossing_ref" for e.g. ​https://www.osm.org/node/428042469

I think the wording should be improved.
What should this tell the mapper? Are they supposed to change the preset Traffic Signal somehow? Or should they open a ticket because the preset is wrong? Or something in the data is wrong?
If the data should be changed I would expect a message like
"crossing_ref=* should not be used in <....>"
or probably better
"crossing_ref=* should not be used outside of GB".

[taylor.smock]

This is the "right" place. Maybe it should have some kind of conditional (if invalid key else if invalid value else if invalid preset).

• Invalid region for this preset
• Invalid region for a value from a preset
• Invalid region for a key from a preset

Additional notes:
I believe this only occurs when both expert mode is enabled and the informational errors are enabled.

[GerdP]

Sorry, I still don't understand what the warning is about. Maybe I didn't understand what a region is. I thought that is an area (a boundary) and my object is inside or outside of that boundary. If the message says that the region is invalid what am I supposed to do?

I think it is not the region that is invalid, it is the tag or tag value that is considerd invalid in a given region.

[taylor.smock]

The crossing_ref tag is kind of a special case -- I've scheduled a review on that (see #23251) since it doesn't seem to be UK only anymore. I'll probably be dropping the regions check for that.

But there are some tags that are (almost always) limited to a specific region.
For example, ref:gnis or memorial=stolperstein.

[GerdP]

Yes, that's understood. I just complain about the wording of the message. If a tag should not be used outside a special region the message should say that. The actual wording blames the region, not the misused tag.

[taylor.smock]

Ticket retargeted after milestone closed

[taylor.smock]

In 18966/josm:

See #23290: Add missing regions attributes to text, combo and multiselect

simonpoole (Vespucci dev) pointed out that we were using regions for those
attributes on but did not have them documented in the xsd.

[taylor.smock]

Ticket retargeted after milestone closed

[aceman]

Yes, the wording of the message should be improved.

[taylor.smock]

Does the following wording make sense? If it doesn't, I'm open to suggestions. Just keep in mind that we have limited space.

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

@@ -749 +749 @@
                 }
                 if (isInRegion == preset.exclude_regions()) {
                     errors.add(TestError.builder(this, Severity.WARNING, INVALID_REGION)
-                            .message(tr("Invalid region for this preset"),
+                            .message(tr("Preset is invalid in this region"),
                                     marktr("Preset {0} should not be applied in this region"),
                                     preset.getLocaleName())
                             .primitives(p)
@@ -825 +825 @@
                 final TestError.Builder builder = TestError.builder(this, Severity.WARNING, INVALID_REGION)
                         .primitives(p);
                 if (value == null) {
-                    builder.message(tr("Invalid region for this preset"),
+                    builder.message(tr("Key from a preset is invalid in this region"),
                             marktr("Preset {0} should not have the key {1}"),
                             preset.getLocaleName(), key);
                 } else {
-                    builder.message(tr("Invalid region for this preset"),
+                    builder.message(tr("Value from a preset is invalid in this region"),
                             marktr("Preset {0} should not have the tag {1}={2}"),
                             preset.getLocaleName(), key, value);
                 }

[GerdP]

Sorry, I tried to find the reasoning for this change on github, but failed, so I still don't know for sure what this test is expected to find and who is supposed to fix the found "errors". For me, this came "out of the blue", but I assume it was discussed on github. How/Where do I find that discussion?

I didn't try but I think even with the patch I would see lots of these warning messages when validating a city in Germany:
"Preset Pedestrian Crossing should not have the key crossing_ref"
"Preset Traffic Signal should not have the key crossing_ref"
and I am still not sure if that means that the tags of the OSM object should be changed or if the preset has to be fixed or maybe if the preset should not be used by JOSM or something like that.

BTW: I have to enable informational warnings to see these messages, but they are shown with severity warning.

[skyper]

Regarding crossing_ref, see also #23251. Besides I wonder how I can handle the situation if I need to specify different lists of tags with some maybe identical per region/country. And how well the code is prepare for external presets with conflicting definitions compare to the core presets. Have not tried it but what happens if a tag is excluded from a region in core and included in an external preset or vice versa?

[taylor.smock]

I have to enable informational warnings to see these messages, but they are shown with severity warning.

I think that the warnings were initially hidden behind the informational warnings being enabled due to a misunderstanding in the startTest method. Probably my fault.

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

@@ -1266 +1266 @@
             checkPresetsTypes = checkPresetsTypes && Config.getPref().getBoolean(PREF_CHECK_PRESETS_TYPES_BEFORE_UPLOAD, true);
         }
 
-        checkRegions = includeOtherSeverity && Config.getPref().getBoolean(PREF_CHECK_REGIONS, true);
+        checkRegions = Config.getPref().getBoolean(PREF_CHECK_REGIONS, true);
         if (isBeforeUpload) {
             checkRegions = checkRegions && Config.getPref().getBoolean(PREF_CHECK_REGIONS_BEFORE_UPLOAD, true);
         }

but I assume it was discussed on github

I think its been discussed in various locations, but it mostly comes down to decreasing the delta between us and Vespucci (see ​https://vespucci.io/tutorials/presets/#extensions ).

Have not tried it but what happens if a tag is excluded from a region in core and included in an external preset or vice versa?

I would expect there to be a warning; we have no method of having presets override other presets. Whichever one is excluding a tag from a region "wins".

[GerdP]

So, if the "regions extension" was introduced for a mobile OSM editor, I assume the idea was to prevent the situation that a preset is presented to the user when the current location doesn't "allow" to use it. That seems reasonable to me.
I don't know Vespucci, does it also verify existing OSM data and show error messages regarding preset regions? Or maybe other QA tools? What wording do they use?

[taylor.smock]

Replying to GerdP:

I don't know Vespucci, does it also verify existing OSM data and show error messages regarding preset regions? Or maybe other QA tools? What wording do they use?

It looks like it has a basic validator; I don't know all that it does (since I don't use Vespucci). @SimonPoole can probably answer that better.

Or maybe other QA tools?

I don't think other QA tools use it yet, but I would hope they do in the future, assuming they use our presets for validation purposes.

[taylor.smock]

In 18980/josm:

Fix #23290: Improve region check messages and run region checks when informational warnings are disabled

[taylor.smock]

In 18981/josm:

See #23290/r18980: Update test messages

[skyper]

Replying to skyper:

I tried the "Shops/Payment Methodes" and I am still offered all regional payments and no validator informational warning is created (unlike crossing_ref where a warning is created).

payment:ep_avant=yes
payment:ep_geldkarte=yes
payment:ep_mep=yes
payment:ep_minicash=yes
payment:ep_minipay=yes
payment:ep_monedero4b=yes
payment:postfinance_card=yes
shop=pet

Still no warning with a node with above tags in Germany. All, except payment:ep_geldkarte=yes should be warned about.

[Famlam]

Replying to taylor.smock:

No worries. Always feel free to ask for clarification.

In this case, the checks are only run if the informational level is enabled.

I just tried the latest dev version 18997. It also seems that something else changed, because this warning now appears at warning level, e.g. flagging all crossing_refs not on an informational level as before, but on an actual warning level, giving about 10k incorrect actual warnings in NL by default (rather than after manually enabling a specific setting)

(Also the warning is strange: I rarely use the presets, but I get a warning about a key from a preset being wrong, rather than just the key that I added manually)

[taylor.smock]

Ticket retargeted after milestone closed

[skyper]

See #23589 for another improper validator warning.

[Famlam]

Another issue with this check is that some relations cross country borders, so the Rhine river (​relation 123924) also contains ref:fgkz=2, ref:sandre=A---0000 and ref:gwlnr=CH0000010000 because it passes through France, Germany and Switzerland as one river (thus one waterway relation). However, that means that if I'm editing near the river in The Netherlands, I get several validation warnings. I don't think I'm supposed to delete these tags from the relation though, nor that I should split the river relation per country borders.

[stoecker]

That could be easily solved when a feature is not reported when at least partially in the area.

[GerdP]

The current implementation checks only where the center is. An incomplete relation is probably not worth testing.

[Famlam]

The current implementation checks only where the center is. An incomplete relation is probably not worth testing.

Skipping incomplete relations unfortunately won't resolve this issue (although it would help). If I download all members of the relation, I still get the warning for ref:sandre (but indeed, the warning for the German ref - probably the center - is suppressed in that case)

[taylor.smock]

IIRC, the territory checks only take a coordinate.

I'd have to check (via profiling), but I suspect the most complete solution (check every node) would be extremely expensive in terms of CPU and memory.

[skyper]

Problems with relations crossing the national boarder reminds me of #21908.
I guess there are not many ways crossing a national boarder though touching a boarder in more than one consecutive node might still be more common. If you choose a small region like a state or even a city limit, you'd probably find more problematical ways.
For relations I only see a solution for completely downloaded relations by converting the members into a line and checking if the line crosses/touches the boarder. This might work for ways, too, but I do not know how expensive these computations are.

[taylor.smock]

Ticket retargeted after milestone closed

[skyper]

See #23636 for suggests to solve the problem.

[taylor.smock]

Ticket retargeted after milestone closed