[Patch] Warn about multiple access values

[skyper]

The use of multiple access values separated with semi-colon is discouraged and either distinct access modes and/or :conditional should be used.

Could we, please, have a validator warning for e.g. vehicle=delivery;agricultural or hgv=destination;forestry or access=destination;agricultural;forestry. Thanks

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-06-18 19:17:52 +0200 (Thu, 18 Jun 2020)
Revision:16678
Build-Date:2020-06-19 01:30:47
URL:https://josm.openstreetmap.de/svn/trunk

[skyper]

See also #17998.

[skyper]

Below seems to work. Any hints how to get it shorter? Actually, railway is still missing. For the :lanes-part, I do not need ships, but still the list is already long.

/* Access */
/* multiple values (#19419) */

node[access][access =~ /^.*;.*$/][number_of_tags() > 1],
way[access][access =~ /^.*;.*$/][number_of_tags() > 1],
relation[access][access =~ /^.*;.*$/],
*["4wd_only"]["4wd_only" =~ /^.*;.*$/],
*[agricultural][agricultural =~ /^.*;.*$/],
*[bdouble][bdouble =~ /^.*;.*$/],
*[bicycle][bicycle =~ /^.*;.*$/],
*[boat][boat =~ /^.*;.*$/],
*[bus][bus =~ /^.*;.*$/],
*[canoe][canoe =~ /^.*;.*$/],
*[carriage][carriage =~ /^.*;.*$/],
*[disabled][disabled =~ /^.*;.*$/],
*[dog][dog =~ /^.*;.*$/],
*[emergency][emergency =~ /^.*;.*$/],
*[foot][foot =~ /^.*;.*$/],
*[golf_cart][golf_cart =~ /^.*;.*$/],
*[goods][goods =~ /^.*;.*$/],
*[hazmat][hazmat =~ /^.*;.*$/],
*[hgv][hgv =~ /^.*;.*$/],
*[horse][horse =~ /^.*;.*$/],
*[hov][hov =~ /^.*;.*$/],
*[mofa][mofa =~ /^.*;.*$/],
*[moped][moped =~ /^.*;.*$/],
*[motor_vehicle][motor_vehicle =~ /^.*;.*$/],
*[motorboat][motorboat =~ /^.*;.*$/],
*[motorcar][motorcar =~ /^.*;.*$/],
*[motorcycle][motorcycle =~ /^.*;.*$/],
*[psv][psv =~ /^.*;.*$/],
*[ship][ship =~ /^.*;.*$/],
*[snowmobile][snowmobile =~ /^.*;.*$/],
*[ski][ski =~ /^.*;.*$/],
*[taxi][taxi =~ /^.*;.*$/],
*[tourist_bus][tourist_bus =~ /^.*;.*$/],
*[vehicle][vehicle =~ /^.*;.*$/],
*[wheelchair][wheelchair =~ /^.*;.*$/] {
  throwWarning: tr("{1} for {0}", "{0.key}", "{0.value}");
  group: tr("Multiple values in access tag");
  suggestAlternative: tr("only one value and additional {0}", "{0.key}:conditional");
  assertMatch:   "relation access=agricultural;forestry";
  assertNoMatch: "relation access=no";
  assertMatch:   "way access=agricultural;forestry a=b";
  assertNoMatch: "way access=agricultural;forestry";
}

[Famlam]

Any hints how to get it shorter?

Shorter on each line: [key*=";"] instead of the regex

Shorter in general, all I can think of is the following

*[/^(4wd_only|access|agricultural|bdouble|bicycle|boat|bus|canoe|carriage|disabled|dog|emergency|foot|golf_cart|goods|hazmat|hgv|horse|hov|mofa|moped|motor_vehicle|motorboat|motorcar|motorcycle|psv|ship|snowmobile|ski|taxi|tourist_bus|vehicle|wheelchair)$/][/^(4wd_only|access|agricultural|bdouble|bicycle|boat|bus|canoe|carriage|disabled|dog|emergency|foot|golf_cart|goods|hazmat|hgv|horse|hov|mofa|moped|motor_vehicle|motorboat|motorcar|motorcycle|psv|ship|snowmobile|ski|taxi|tourist_bus|vehicle|wheelchair)$/=~/;/] {
  throwWarning: tr("{0}", "{0.tag}");
  suggestAlternative: tr("only one value and additional {0}", "{0.key}:conditional");
  group: tr("Multiple values in an access tag");
  assertMatch:   "relation access=agricultural;forestry";
  assertNoMatch: "relation access=no";
  assertMatch:   "way access=agricultural;forestry a=b";
}

(I didn't understand the need for the last 'assertNoMatch': what's a way without tags except for an access tag?)

[skyper]

Replying to Famlam:

Shorter in general, all I can think of is the following

*[/^(4wd_only|access|agricultural|bdouble|bicycle|boat|bus|canoe|carriage|disabled|dog|emergency|foot|golf_cart|goods|hazmat|hgv|horse|hov|mofa|moped|motor_vehicle|motorboat|motorcar|motorcycle|psv|ship|snowmobile|ski|taxi|tourist_bus|vehicle|wheelchair)$/][/^(4wd_only|access|agricultural|bdouble|bicycle|boat|bus|canoe|carriage|disabled|dog|emergency|foot|golf_cart|goods|hazmat|hgv|horse|hov|mofa|moped|motor_vehicle|motorboat|motorcar|motorcycle|psv|ship|snowmobile|ski|taxi|tourist_bus|vehicle|wheelchair)$/=~/;/] {
  throwWarning: tr("{0}", "{0.tag}");
  suggestAlternative: tr("only one value and additional {0}", "{0.key}:conditional");
  group: tr("Multiple values in an access tag");
  assertMatch:   "relation access=agricultural;forestry";
  assertNoMatch: "relation access=no";
  assertMatch:   "way access=agricultural;forestry a=b";
}

Thanks, I did not think about regex for the keys. Probably a bit too short but I can split it into some groups. That should work. Have to test how I get nice messages.

(I didn't understand the need for the last 'assertNoMatch': what's a way without tags except for an access tag?)

There is an individual test for nodes and ways with only access=* and I did not like to get two warnings, e.g. I only warn with two or more tags. Have to take a look at the individual test and see how it can be integrated.

[skyper]

Ok, the individual test for only access is in combinations.mapcss​. Not sure if I should rework it as all access tags without additional tag could be warned about and not only access=*. A single motor_vehicle=no or foot=private might be a problem for routers.

I now have two versions for the simple access tags as the condensed version does not give much information and it only warns about one object once where as the longer form does display the key and adds a warning each tag. e.g. possibly multiple per object.

Well, *:lanes is more tricky, I will try.

/* Access */
/* multiple values (#19419) */
/* there is an individual test for only "access=*" for nodes and ways. */

node[access][access *= ";"][number_of_tags() > 1],
way[access][access *= ";"][number_of_tags() > 1],
relation[access][access *= ";"],
*[/^(foot|dog|horse|ski|wheelchair)$/ =~ /;/],
*[/^(vehicle|bicycle|carriage|caravan|trailer)$/ =~ /;/],
*[/^(motor_vehicle|bdouble|bus|goods|hgv|minibus|mofa|moped|motorcar|motorcycle|motorhome|taxi|tourist_bus)$/ =~ /;/],
*[/^(4wd_only|agricultural|atv|disabled|emergency|golf_cart|hazmat|hov|psv|snowmobile)$/ =~ /;/],
*[/^(train|light_train|monorail|tram|subway)$/ =~ /;/],
*[/^(boat|canoe|cargo|motorboat|sailboat|ship)$/ =~ /;/] {
  group: tr("Invalid access tag");
  throwWarning: tr("Multiple values in access tag");
  suggestAlternative: tr("only one value and additional {0}", "*:conditional");
  assertMatch:   "relation access=agricultural;forestry";
  assertNoMatch: "relation access=no";
  assertMatch:   "way access=agricultural;forestry a=b";
  assertNoMatch: "way access=agricultural;forestry";
}

/* second, more informative version */

node[access][access *= ";"][number_of_tags() > 1],
way[access][access *= ";"][number_of_tags() > 1],
relation[access][access *= ";"],
*[foot][foot *= ";"],
*[wheelchair][wheelchair *= ";"],
*[dog][dog *= ";"],
*[horse][horse *= ";"],
*[ski][ski *= ";"],
*[vehicle][vehicle *= ";"],
*[bicycle][bicycle *= ";"],
*[carriage][carriage *= ";"],
*[trailer][trailer *= ";"],
*[caravan][caravan *= ";"],
*[motor_vehicle][motor_vehicle *= ";"],
*[motorcar][motorcar *= ";"],
*[motorcycle][motorcycle *= ";"],
*[motorhome][motorhome *= ";"],
*[bus][bus *= ";"],
*[minibus][minibus *= ";"],
*[tourist_bus][tourist_bus *= ";"],
*[taxi][taxi *= ";"],
*[goods][goods *= ";"],
*[hgv][hgv *= ";"],
*[bdouble][bdouble *= ";"],
*[mofa][mofa *= ";"],
*[moped][moped *= ";"],
*[atv][atv *= ";"],
*[golf_cart][golf_cart *= ";"],
*[snowmobile][snowmobile *= ";"],
*[4wd_only][4wd_only *= ";"],
*[agricultural][agricultural *= ";"],
*[disabled][disabled *= ";"],
*[emergency][emergency *= ";"],
*[hazmat][hazmat *= ";"],
*[hov][hov *= ";"],
*[psv][psv *= ";"],
*[light_train][light_train *= ";"],
*[monorail][monorail *= ";"],
*[train][train *= ";"],
*[tram][tram *= ";"],
*[subway][subway *= ";"],
*[boat][boat *= ";"],
*[canoe][canoe *= ";"],
*[cargo][cargo *= ";"],
*[motorboat][motorboat *= ";"],
*[sailboat][sailboat *= ";"],
*[ship][ship *= ";"] {
  group: tr("Invalid access tag");
  throwWarning: tr("Multiple values in {0}", "{0.tag}");
  suggestAlternative: tr("only one value and additional {0}", "{0.key}:conditional");
  assertMatch:   "relation access=agricultural;forestry";
  assertNoMatch: "relation access=no";
  assertMatch:   "way access=agricultural;forestry a=b";
  assertNoMatch: "way access=agricultural;forestry";
}

[skyper]

Below should do it for almost all access-tags combinations and the line in "combinations.mapcss" should be removed. Please have a look, I gonna continue with #17998 and might have to set more class:

/* only invalid access tag */
*[/^(access|foot|dog|horse|ski|wheelchair)(:.*)*$/][/^(access|foot|dog|horse|ski|wheelchair)(:.*)*$/ =~ /;/][number_of_tags() == 1],
*[/^(vehicle|bicycle|carriage|caravan|trailer)(:.*)*$/][/^(vehicle|bicycle|carriage|caravan|trailer)(:.*)*$/ =~ /;/][number_of_tags() == 1],
*[/^(motor_vehicle|bdouble|bus|goods|hgv|minibus|mofa|moped|motorcar|motorcycle|motorhome|taxi|tourist_bus)(:.*)*$/][/^(motor_vehicle|bdouble|bus|goods|hgv|minibus|mofa|moped|motorcar|motorcycle|motorhome|taxi|tourist_bus)(:.*)*$/ =~ /;/][number_of_tags() == 1],
*[/^(4wd_only|agricultural|atv|disabled|emergency|golf_cart|hazmat|hov|psv|snowmobile)(:.*)*$/][/^(4wd_only|agricultural|atv|disabled|emergency|golf_cart|hazmat|hov|psv|snowmobile)(:.*)*$/ =~ /;/][number_of_tags() == 1],
*[/^(train|light_train|monorail|tram|subway)(:.*)*$/][/^(train|light_train|monorail|tram|subway)(:.*)*$/ =~ /;/][number_of_tags() == 1],
*[/^(boat|canoe|cargo|motorboat|sailboat|ship)(:.*)*$/][/^(boat|canoe|cargo|motorboat|sailboat|ship)(:.*)*$/ =~ /;/][number_of_tags() == 1] {
  throwWarning: tr("incomplete object: only invalid {0}, add a primary tag and", "{0.tag}");
  suggestAlternative: tr("only one value and additional {0}", "{0.key}:conditional");
  set only_invalid_access_tag;
  group: tr("missing tag");
  assertMatch:   "way foot=no;yes";
  assertNoMatch: "way foot=no;yes a=b";
  assertMatch:   "way bicycle:lanes:forward=no;yes|designated";
  assertNoMatch: "way bicycle:lanes:forward=no;yes|designated a=b";
}

/* only valid access tag */
*[/^(access|foot|dog|horse|ski|wheelchair)(:.*)*$/][number_of_tags() == 1]!.only_invalid_access_tag,
*[/^(vehicle|bicycle|carriage|caravan|trailer)(:.*)*$/][number_of_tags() == 1]!.only_invalid_access_tag,
*[/^(motor_vehicle|bdouble|bus|goods|hgv|minibus|mofa|moped|motorcar|motorcycle|motorhome|taxi|tourist_bus)(:.*)*$/][number_of_tags() == 1]!.only_invalid_access_tag,
*[/^(4wd_only|agricultural|atv|disabled|emergency|golf_cart|hazmat|hov|psv|snowmobile)(:.*)*$/][number_of_tags() == 1]!.only_invalid_access_tag,
*[/^(train|light_train|monorail|tram|subway)(:.*)*$/][number_of_tags() == 1]!.only_invalid_access_tag,
*[/^(boat|canoe|cargo|motorboat|sailboat|ship)(:.*)*$/][number_of_tags() == 1]!.only_invalid_access_tag {
  throwWarning: tr("incomplete object: only {0}", "{0.tag}");
  group: tr("missing tag");
  assertMatch:   "way foot=no";
  assertNoMatch: "way foot=no a=b";
  assertMatch:   "way bicycle:lanes:forward=yes|designated";
  assertNoMatch: "way bicycle:lanes:forward=yes|designated a=b";
}

/* multiple values (#19419) */
*[number_of_tags() > 1][/^(access|foot|dog|horse|ski|wheelchair)(:.*)*$/][/^(access|foot|dog|horse|ski|wheelchair)(:.*)*$/ =~ /;/],
*[number_of_tags() > 1][/^(vehicle|bicycle|carriage|caravan|trailer)(:.*)*$/][/^(vehicle|bicycle|carriage|caravan|trailer)(:.*)*$/ =~ /;/],
*[number_of_tags() > 1][/^(motor_vehicle|bdouble|bus|goods|hgv|minibus|mofa|moped|motorcar|motorcycle|motorhome|taxi|tourist_bus)(:.*)*$/][/^(motor_vehicle|bdouble|bus|goods|hgv|minibus|mofa|moped|motorcar|motorcycle|motorhome|taxi|tourist_bus)(:.*)*$/ =~ /;/],
*[number_of_tags() > 1][/^(4wd_only|agricultural|atv|disabled|emergency|golf_cart|hazmat|hov|psv|snowmobile)(:.*)*$/][/^(4wd_only|agricultural|atv|disabled|emergency|golf_cart|hazmat|hov|psv|snowmobile)(:.*)*$/ =~ /;/],
*[number_of_tags() > 1][/^(train|light_train|monorail|tram|subway)(:.*)*$/][/^(train|light_train|monorail|tram|subway)(:.*)*$/ =~ /;/],
*[number_of_tags() > 1][/^(boat|canoe|cargo|motorboat|sailboat|ship)(:.*)*$/][/^(boat|canoe|cargo|motorboat|sailboat|ship)(:.*)*$/ =~ /;/] {
  throwWarning: tr("Multiple values in access tag {0}", "{1.tag}");
  suggestAlternative: tr("only one value and additional {0}", "{1.key}:conditional");
  group: tr("Invalid access tag");
  assertMatch:   "way access=agricultural;forestry a=b";
  assertNoMatch: "way access=agricultural;forestry";
  assertMatch:   "way bicycle:lanes:forward=yes;no|designated a=b";
  assertNoMatch: "way bicycle:lanes:forward=yes;no|designated";
}

[skyper]

I ran into problems trying to get different warnings for multiple values with valid values vs. multiple values with an invalid value. Another problem is that there might be quite some access tags and I'd like to get the key and the value. The advantage of no regex or a minimal one instead of the long one seems to be more control.

Have to rethink and probably start with only some common keys and not the whole list.

[skyper]

I decided to go with the long form but only with some primary tags.
Please, find attached patch file​.

On the way, I harmonized the indent to follow the common two white spaces.

[skyper]

patch adds warning for almost all access tags with multiple values with specific primary tags

[skyper]

Additionally, I tried to add *:lanes[:*] but I stumbled over getting the proper key from regex:

2021-07-15 18:43:02.349 SEVERE: Unable to replace argument ^bus:lanes(:both_ways)?(:(backward|forward))?$ in : Illegal group reference: group index is missing: java.lang.IllegalArgumentException: Illegal group reference: group index is missing

with

way[highway][/^bus:lanes(:both_ways)?(:(backward|forward))?$/ =~ /^.*;.*$/] {
  throwWarning: tr("{0} with multiple values", "{1.key}");
  group: tr("Multiple values in access tag");
  suggestAlternative: tr("only one value and additional {0}", "{1.key}:conditional");
  assertMatch:   "way highway=trunk bus:lanes:both_ways:forward=designated;yes|no";
  assertNoMatch: "way highway=trunk bus:lanes:both_ways:forward=designated|no";
}

Without the assertMatch it is accepted but the warning includes placeholders:

Multiple values in access tag - {1.key} with multiple values, use only one value and additional {1.key}:conditional instead

Any hints?

[Famlam]

@Skyper:

A hint about the regexes:
[something =~ /^.*;.*$/] equals [something =~ /;/] or [something *=";"] (and probably it's also faster as you don't need to evaluate the full string, just until you find a ;)

Also, why check for empty strings?
|| in *[/^(amenity||building(:part)?|entrance|highway|leisure)$/]?

[skyper]

Replying to Famlam:

@Skyper:

A hint about the regexes:
[something =~ /^.*;.*$/] equals [something =~ /;/] or [something *=";"] (and probably it's also faster as you don't need to evaluate the full string, just until you find a ;)

Right, I simply copied regexes and did not think about the easy solution. Only need to look for ;.

Also, why check for empty strings?
|| in *[/^(amenity||building(:part)?|entrance|highway|leisure)$/]?

Thanks, stupid typo working in block mode.

Attached version 2​ with the changes.

[skyper]

version 2: removed empty values and shorter regex

[Don-vip]

In 18105/josm:

fix #19419 - Warn about multiple access values (patch by skyper)

[skyper]

Replying to skyper:

Additionally, I tried to add *:lanes[:*]

See #21192.

Any hints?

I have to try if regexp_match() can help.

[skyper]

Despite two up-votes and no direct demands, please, revert r18105 and r18132 (#21192).

It is not clear if multiple values for access tags are allowed and motor_vehicle=agricultural;forestry is quite in use in the German speaking area. On the ​German forum there is the opinion this is a false positive.

I thought about lowering the warning level but as presets use combos and no multiselect there is already an informal warning about "value not in preset".

A correct test would be on conflicting multiple values like private;designated or yes;no which I find in the wild. Though, this test does not work in MapCSS as I fear and would need own Java code.

[Don-vip]

In 18182/josm:

see #19419 - see #21192 - revert "Multiple access values" checks

[skyper]

See #21249 for a new try.