Modify

Opened 3 years ago

Closed 18 months ago

Last modified 18 months ago

#19419 closed enhancement (fixed)

[Patch] Warn about multiple access values

Reported by: skyper Owned by: team
Priority: normal Milestone: 21.07
Component: Core validator Version:
Keywords: template_report access multiple values Cc: Klumbumbus

Description (last modified by 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

Attachments (2)

josm_19419.patch (4.9 KB) - added by skyper 19 months ago.
patch adds warning for almost all access tags with multiple values with specific primary tags
josm_19419_v2.patch (4.5 KB) - added by skyper 19 months ago.
version 2: removed empty values and shorter regex

Download all attachments as: .zip

Change History (22)

comment:1 Changed 3 years ago by skyper

Description: modified (diff)

comment:2 Changed 3 years ago by skyper

See also #17998.

comment:3 Changed 2 years ago by skyper

Owner: changed from team to skyper

comment:4 Changed 2 years ago by 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";
}

Last edited 2 years ago by skyper (previous) (diff)

comment:5 Changed 2 years ago by 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?)

Last edited 2 years ago by Famlam (previous) (diff)

comment:6 in reply to:  5 Changed 2 years ago by 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.

comment:7 Changed 2 years ago by 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";
}
Last edited 2 years ago by skyper (previous) (diff)

comment:8 Changed 2 years ago by 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";
}

Last edited 2 years ago by skyper (previous) (diff)

comment:9 Changed 2 years ago by 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.

comment:10 Changed 19 months ago by skyper

Cc: Klumbumbus added
Milestone: 21.07
Owner: changed from skyper to team
Summary: Warn about multiple access values.[Patch] Warn about multiple access values.

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.

Last edited 19 months ago by skyper (previous) (diff)

Changed 19 months ago by skyper

Attachment: josm_19419.patch added

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

comment:11 Changed 19 months ago by 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?

comment:12 in reply to:  11 ; Changed 19 months ago by 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)$/]?

comment:13 in reply to:  12 Changed 19 months ago by 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.

Changed 19 months ago by skyper

Attachment: josm_19419_v2.patch added

version 2: removed empty values and shorter regex

comment:14 Changed 19 months ago by Don-vip

Resolution: fixed
Status: newclosed

In 18105/josm:

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

comment:15 Changed 19 months ago by Don-vip

Summary: [Patch] Warn about multiple access values.[Patch] Warn about multiple access values

comment:16 in reply to:  11 Changed 18 months ago by skyper

Replying to skyper:

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

See #21192.

Any hints?

I have to try if regexp_match() can help.

comment:17 Changed 18 months ago by skyper

Resolution: fixed
Status: closedreopened

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.

comment:18 Changed 18 months ago by Don-vip

In 18182/josm:

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

comment:19 Changed 18 months ago by Don-vip

Resolution: fixed
Status: reopenedclosed

comment:20 Changed 18 months ago by skyper

See #21249 for a new try.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.