Validator does not find overlapping buildings anymore

[jfd553]

What steps will reproduce the problem?

1. Have a bunch of crossing buildings
2. Run validator
3. Look at found errors, the "Building crossing" warnings does not appear anymore.

What is the expected result?

Validator build a list of all crossing buildings.

What happens instead?

The "Building crossing" warning does not appear anymore.

Please provide any additional information below. Attach a screenshot if possible.

It was running correctly in the previous version of JOSM, stop working after upgrading to latest version.

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-04-06 02:17:07 +0200 (Mon, 06 Apr 2020)
Build-Date:2020-04-06 00:18:43
Revision:16239
Relative:URL: ^/trunk

Identification: JOSM/1.5 (16239 en) Windows 10 64-Bit
OS Build number: Windows 10 Enterprise 1903 (18362)
Memory Usage: 283 MB / 989 MB (150 MB allocated, but free)
Java version: 1.8.0_241-b07, Oracle Corporation, Java HotSpot(TM) Client VM
Screen: \Display0 1680x1050
Maximum Screen Size: 1680x1050
Dataset consistency test: No problems found

Plugins:
+ PicLayer (35405)
+ reverter (35409)
+ turnrestrictions (35405)
+ utilsplugin2 (35405)

[GerdP]

The error is detected by a different test now. Look for "Overlapping buildings"

[GerdP]

See #16707

[jfd553]

False, there is no "Overlapping buildings" error/warning/other message that appear in JOSM in the example I sent you!
I can send you the .osm file I am working on if you whish...

[jfd553]

False, there is no "Overlapping buildings" error/warning/other message that appear in JOSM in the example I sent you!
I can send you the .osm file I am working on if you whish...

[GerdP]

Please check if the message is ignored or if the checkk geometry.mapcss is disabled. If not, please attach the .osm file.

[GerdP]

Sorry, I can reproduce the problem. Should have tried it first.

[jfd553]

+1

[GerdP]

Strange. I was able to reproduce because I had configured a different file for the geometry test. After resetting to default all works fine. Please check your preferences regarding "Tag checker rules" for validator.

[jfd553]

Here are my preferences regarding the "tag checker rules" for the validator. I reset them just in case but that didn't change anything. Specifically for the geometry item, it points to resource://data/validator/geometry.mapcss. Is "resource:" located on my computer? If yes, where is it expected to be?

true Addresses
true Tag combinations
true Deprecated features
true Geometry
true Highways
true Multiple values
true Numeric values
true Religion
true Relations
true Territories
true Unnecessary tags
true Wikipedia

[Klumbumbus]

It works for me too. Please attach your .osm file and check your validator ignore list (the button bottom right in your screenshot).

[jfd553]

Did not find anything in the ignore list that seem related to this problem :-( However,I'll attach both the .osm file and a print screen of my validator ignore list.

[jfd553]

Validator ignore list

[jfd553]

Working osm file

[GerdP]

I see Overlapping buildings (78)
If you did the same as I your preferences file will not contain this block:

    <map>
      <tag key='active' value='true'/>
      <tag key='title' value='Geometry'/>
      <tag key='url' value='resource://data/validator/geometry.mapcss'/>
    </map>

Important part:resource://data/validator/geometry.mapcss

[Klumbumbus]

Replying to GerdP:

I see Overlapping buildings (78)

Me too.

[jfd553]

You are right, seems my preference file does not contain the block you provided. Then I have two questions...
1- In order to make it works, I add the block seomwhere between two other block?
2- Since it worked yesterday with the previous version, and did nothing else since, why did it stop to work?

[jfd553]

I supposed the missing block should be inserted in a specific parent block like
<maps key=Something that must be specific>

<map>

<tag key='active' value='true'/>
<tag key='title' value='Geometry'/>
<tag key='url' value='resource://data/validator/geometry.mapcss'/>

</map>

</maps>

[GerdP]

In comment:10 you confirmed that the correct file is used. Maybe it doesn't appear in the preferences because you never changed it.
Please check the item "ignore list" within your ignore list, maybe that contains the problematic item.
The ignore list is very long, maybe there is an error in the handling of the ignore list.
You can try this: stop JOSM, create a copy of the preferences.xml somewhere and start it again. Clear the ignore list.
If that brings back the warning the problem is in the ignore list.
The ignore list is stored in the block

  <maps key='validator.ignorelist'>
    <map>
...
    </map>
  </maps>

Please extract that block from the copy and attach it.

[jfd553]

The "ignore list" within myignore list has four items: 103, 113, 1604, 1608
When clearing the whole ignore list I get the "Overlapping buildings" (78) I was looking for!!!
I'll attach The ignore list soon :-)

[jfd553]

The ignore list block

[GerdP]

This entry in your list might be the problem:

<tag key='3000' value='missing tag'/>

[jfd553]

I remove that <tag key='3000' value='missing tag'/> entry and it will work again?
Is it something I did over hte years (contributing to OSM since 2009)?

[GerdP]

I can confirm that this entry is the problem. I added it to my preferences and a lot of warnings disappeared.
I don't know how it was created, though.
Anyway, something is wrong in the ignore list handler, I'll look at this tomorrow.

[jfd553]

+1

[GerdP]

It would be good to know how this bad entry made it into the list.
If you have backups of the preferences.xml made before updaing to r16239, please check if they alreay contain this.
Another possible source would be the migration process from file validator/ignorederrors, but that should have happened around Jan 2019. Anyway, it would help to know the current content of validator/ignorederrors if you still have it.

BTW: Your list contains lots of entries which are probably many years old. For some of them the corresponding tests no longer exist. I think it would be a good idea to start from scratch with the cleared list.

[GerdP]

Forget above. I can reproduce it. Details will follow.

[GerdP]

simple sample

[GerdP]

Steps to reproduce:
1) start JOSM with a cleared ignore list
2) load attached file 19053-sample.osm
3) run validator, make sure you get an entry missing tag (2) which contains two different(!) entries
4) select entry "missing tag (2)" and click Ignore
5) chose "whole group" in the popup (this produces the bad entry)
6) click "Manage Ignore" and look at the details.

The same problem can be produced with two different amenity inside amenity problems (e.g amenity=parking and amenity=fuel)
Once the bad entry exists all(!) errors/warnings produced by mapcss tests are ignored.

[Klumbumbus]

Uh, thats bad, indeed.

[GerdP]

My current thinking is that the "Ignore" button should be disabled when selected entry is a mapcss group like "deprecated tagging" or "missing tag".

[GerdP]

I think the problem was introduced with r14828 for #17268.

[Klumbumbus]

Also I guess we need a way JOSM detects and removes bad entries in the ignore list, i.e. those which block all mapcss tests and the old ones before the restructure with numbers only.

[GerdP]

We just have to remove the entry with 3000. The others are okay, they were migrated from the older ignorederrors file.
r14828 was still okay, probably error was introduced with r14858, josm-snapshot-14867.jar shows the error.

[GerdP]

The bug itself is older. I just tried r14500 and it shows the same problem. With this version the value 3000 is stored in the ignorederrors file and that was even worse compared to the current version.
So, an entry of 3000 without any further info must be ignored.

Open question is if I should simply disable the Ignore button for groups or if we should store a corrected value which allows to identify what the user wanted.

[GerdP]

The patch implements the simple solution (remove bad entry, disable Ignore button for groups).

[Klumbumbus]

Another strange thing I just noticed:

1. load 19053-sample.osm​
2. validate
3. select a single element in the validator list
4. click ignore
5. validate again
6. the previous ignored element is back (because nothing was added to the ignore list two steps before)

[GerdP]

Yes, that's confusing. The objects in the sample file are new, so it makes no sense to add the ids to the ignore list.
Still, the errors are removed from the list.

[Klumbumbus]

Replying to GerdP:

The objects in the sample file are new, so it makes no sense to add the ids to the ignore list.

Ah, right, negative ids. So this behavior is ok.

[Klumbumbus]

Replying to GerdP:

The patch implements the simple solution (remove bad entry, disable Ignore button for groups).

I think this solution is OK.

[GerdP]

I am not yet happy with the items that are displayed in the ignore list manager.
I have e.g.

missing tag (2,067)
 + alternative name without name (1836)
 + amenity=place_of_worship without religion (1)
 + amenity=recycling without ^recycling: (4)
 + amenity=social_facility without social_facility (2)

When I select "alternative name without name (1836)" and click Ignore -> Whole group the manager shows this:

missing tag
3000_*[_name$][!name][!old_name][!loc_name][!uic_name][!artist_name][!lock_name][!osak:municipality_name][!osak:street_name][noname'NEQ'yes]

The decriptive name is gone. Not sure if this was always the case.

[GerdP]

I just looked at the original ticket #17268. Seems I only asked for that structure but did not notice that it was not implemented. Now it's too late.

[GerdP]

In 16247/josm:

fix #19053: Validator does not find overlapping buildings anymore
Ignore list handling added an invalid entry for key "3000" so that all issues found by the MapCSSTagChecker were ignored

• remove invalid entry when loading the ignore List
• disable the Ignore button when it would produce the invalid entry

[taylor.smock]

Replying to GerdP:

I just looked at the original ticket #17268. Seems I only asked for that structure but did not notice that it was not implemented. Now it's too late.

I wish I could remember what I was thinking back then. I know I didn't know as much then as I do now though.

That being said, I probably used the mapcss match since that was not going to change on a whim (third party validators). Now that I'm thinking about it again, it may not have been the best thing to do. It may have also been to avoid situations like this : way[highway][!name] { throwError: "Way without name"} way[!name] { throwError: "Way without name" }, and I needed some way to consistently differentiate between mapcss errors, so I think I used the mapcss matcher as the method to differentiate.

[GerdP]

I think the problem existed before #17268. Here is a very ugly example:
Tree shows

amenity inside amenity (4)
 + amenity=fuel inside amenity=fuel (1)
 + amenity=parking inside amenity=parking (2)
 + amenity=school inside amenity=school (1)

When I ignore the "amenity=fuel inside amenity=fuel (1)" entry I find this in preferences:

  <maps key='validator.ignorelist'>
    <map>
      <tag key='3000_node[ParameterFunction~equal(class java.lang.Object ParameterFunction~tag(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String &lt;amenity&gt;),class java.lang.Object ParameterFunction~parent_tag(class org.openstreetmap.josm.gui.mappaint.Environment,class java.lang.String &lt;amenity&gt;))] &gt;LinkSelector{conditions=[]} area[amenity][amenity&apos;NEQ&apos;parking]' value='amenity inside amenity'/>
    </map>
  </maps>

I wonder why the key doesn't look like the rule, e.g.

3000_node[tag("amenity") = parent_tag("amenity")] ∈ area[amenity][amenity != parking]

[GerdP]

different problem

[GerdP]

I found a different problem case:

1. Run validator on attached shop-inside-shop.osm, should find two different warnings:

   shop inside shop (2)
   + shop=bookmaker inside shop=bookmaker (1)
   + shop=shoes inside shop=shoes (1)
   

2. Ignore "shop=bookmaker inside shop=bookmaker"
3. run validator again and see that both warnings are ignored

I think in this case it would really be better to use the (maybe untranslated) message "shop=bookmaker inside shop=bookmaker" as key

[Klumbumbus]

...and if you ignore "shop inside shop (2)" -> whole group, it still adds the bad 3000 entry (with r16247)

[GerdP]

In 16248/josm:

fix #19053:Validator does not find overlapping buildings anymore

• don't store invalid key "3000" which would suppress all MapCSSTagChecker messages

[GerdP]

Replying to Klumbumbus:

...and if you ignore "shop inside shop (2)" -> whole group, it still adds the bad 3000 entry (with r16247)

Yes, shit. I've added code so that this bad key is never stored.
Means "Ignore" button works in that it removes the errors from the list but they re-appear with the next validation.
We need a different way to store the ignored errors, the concept is - and always was - error prone reg. the mapcss tests.

[GerdP]

A possible solution might be to add a unique ignore-id to each mapcss rule.

[Klumbumbus]

Replying to GerdP:

the concept is - and always was - error prone reg. the mapcss tests.

Probably since we use wiki:/Help/Validator/MapCSSTagChecker#Grouping it is even more error prone.

[Klumbumbus]

Replying to GerdP:

add a unique ignore-id to each mapcss rule.

That wouldn't help for comment:45 as it is the same rule, would it?

[GerdP]

Replying to Klumbumbus:

Replying to GerdP:

add a unique ignore-id to each mapcss rule.

That wouldn't help for comment:45 as it is the same rule, would it?

Yes, so a combination of ignore-id and message is needed.

[GerdP]

Forget that patch. Is it possible to remove it?

[Klumbumbus]

yes. click on it and then delete attachement.

[GerdP]

I found ticket:9414#comment:35 where Dirk writes:

otherwise you can't ignore error groups.

At that time the group: statement was not yet implemented, it came much later with r10710 (#12570)

IIGTR we should still allow to ignore groups of errors NOT produced by mapcss tests, e.g. "Overlapping ways"
"Opening hours syntax", or "Role verification problem"?

[GerdP]

I think with this patch the Ignore button and also Manage Ignore works quite well:

• enable Ignore button for groups NOT produced by mapcss tests, e.g. "Overlapping ways", "Opening hours syntax", or "Role verification problem"
• add most specific warning message to the ignore key when code is 3000, e.g. 3000_shop=bookmaker inside shop=bookmaker
• don't add the ugly selector string to the key

The patch probably requires some changes to unit tests, so please let me know what you think about it.
We also may need some more cleanup code for the existing ignore lists.

[GerdP]

v3 changes the logic so that 3000: is used instead of 3000_
This allows to distinguish between old and new entries in the ignore list
It also removes all entries starting with 3000_ and prints a warning for each.
It would be possible to convert some of the old entries to the new format but that would require quite a lot of work. We would have to find all mapcss rules which do NOT contain the group: statement. Only those can be converted. The corresponding code might be generated by a script.

[GerdP]

The corresponding code might be generated by a script.

A script would not work, we have to do it with all the loaded rules.
The migration of the old ignore list entries would require quite a lot of work, and only a few entries don't have a group, so I think it is not worth to do that. For those who encountered the original problem in this ticket the information is lost anyway.

[GerdP]

With 19053.3.patch it was no longer possible to add certain errors to the ignore list, .e.g unclosed ways.

The code is hard to understand, some separator characters have special meanings as they are used in regular expressions somewhere else.

[GerdP]

I am still struggling with the "group" concept.
See for example

*[ele][ele =~ /^-?[0-9]+\.[0-9][0-9][0-9]+$/] {
  throwWarning: tr("{0}", "{0.tag}");
  group: tr("Unnecessary amount of decimal places");
}

It produces messages like this

Unnecessary amount of decimal places (208)
 + ele=1.8082275 (1)
 + ele=2.5291748 (1)
 + ele=6.866 (3)
...

Since r16247 you cannot ignore group "Unnecessary amount of decimal places", but it makes no sense to ignore the individual ele values.

OTOH we have what I would call "master" groups like

group: tr("deprecated tagging");
group: tr("unnecessary tag");
group: tr("missing tag");
group: tr("suspicious tag combination");

which are used in various mapcss tests.

Other checks like java code in UnclosedWays also create groups, e.g.

Unclosed way (2)
 + boundary type administrative (1)
 + building (1)

With latest tested version r16239 you can ignore "Unclosed way" but this will not ignore all kinds of unclosed ways, only those which appear in the tested data. If you add a new unclosed way with e.g. "landuse=residential" it will not be ignored.
My current understanding is that we want to allow to ignore all kind of groups and that the code either fails to "understand" what the group is or how to store the information in the preferences, maybe both. This should be fixed.
r16247 and r16248 didn't improve this situation, they made it worse.

Open question: Do we want to forbid to ignore "master" groups?

[GerdP]

19053.4.patch reverts most of the changes in r16247

• allow again to ignore groups like "Unnecessary amount of decimal places", also "master" groups like "unnecessary tag"
• am old bad ignore lists entry like

  <tag key='3000' value='missing tag'/>
  

is converted to

<tag key='3000_missing tag' value='missing tag'/>

This is also the entry that is store when you ignore the master group "missing tag".
Compared to versions before r16247 only the handling of MapCss tests is changed.

[Klumbumbus]

With patch v4 and this validator tree and ignoring the whole group "foot=yes ist unnötig für highway=pedestrian (5)" and revalidation also the two groups abouve disappear. This is probably not what the user expects. Especially as after ignoring the two lines above are still in the tree. (only after revalidation they disappear.)

(Anyway I would appreciate if someone else would also test as I never use the ignore list.)

[skyper]

I have never ignored a group. In my eyes, it was suspicious. Do not know who told me.

I would exclude "master" groups from ignoring. You can always disable it through preferences. A shortcut to validator preferences from context menu like in Map paint styles panel could help the user.

[GerdP]

How can you ignore a "master" group in preferences? The only way I know is to remove the corresponding rules in all *.mapcss files

[Klumbumbus]

There is not really such a thing as "master" groups.
group: tr("deprecated tagging"); and group: tr("Unnecessary amount of decimal places"); are the same, just different texts.

[GerdP]

Yes, that's why I used the quotes. Some group: statements are used in only one rule, others are used in many. The latter I call "master" group.

[GerdP]

19053.5.patch changes again the way how the ignore key is created for mapcss tests. When you ignore a group that has a "master" group the name of the group is used, e.g.

      <tag key='3000_ele=1.8082275' value='Unnecessary amount of decimal places'/>
      <tag key='3000_foot=yes is unnecessary for highway=pedestrian' value='unnecessary tag'/>

[Klumbumbus]

Replying to GerdP:

Yes, that's why I used the quotes. Some group: statements are used in only one rule, others are used in many. The latter I call "master" group.

I would't handle them different or distinguish at all. Because there could be a "rule specific group" split into two rules, one with and one without autofix. Then it is "rules specific" but also used on several rules and not clear if it is a "master" group or not.

[GerdP]

OK, that's what I implemented.

[GerdP]

There is still a problem with rules that have multiple selectors but no group:, e.g.

*[addr:street =~ /(?i).*Straße.*/][inside("LI,CH")],
*[name =~ /(?i).*Straße.*/][inside("LI,CH")] {
  throwError: tr("street name contains ß");
...
}

If the error is ignored for an addr:street tag it might reoccur when a street with name="Hauptstraße" is checked because they have different selectors and the selector is still sometimes used to build the key.

[GerdP]

never use selector to build key

[GerdP]

I think 19053.6.patch works fine but is incompatible to the old ignorelist entries stored in preferences.xml.
Did we have that problem before? If yes, what was the solution? I could check the preference key josm.version and ignore incompatible entries written with any older version.

[GerdP]

19053.7.patch introduces new preference validator.ignorelist.version. If not set, entries starting with "3000_" are ignored.
For each entry a warning like below is produced

2020-04-13 18:11:00.419 WARNING: Cannot handle ignore list entry 3000_node[exit_to]:n_46440554=deprecated tagging
2020-04-13 18:11:00.419 WARNING: Cannot handle ignore list entry 3000_node[exit_to]:n_885659107=deprecated tagging
2020-04-13 18:11:00.419 WARNING: Cannot handle ignore list entry 3000_node[exit_to]:n_896347559=deprecated tagging
2020-04-13 18:11:00.419 WARNING: Cannot handle ignore list entry 3000_node[leisure=park][natural'NEQ'tree]:n_2562892753=leisure=park on a node. Should be drawn as an area.
2020-04-13 18:11:00.420 WARNING: Cannot handle ignore list entry 3000_way[highway=unclassified][!name][noname'NEQ'yes]=Ignore list

If nobody has a better idea I'll commit this patch tomorrow.

[GerdP]

In 16296/josm:

fix #19053:Validator does not find overlapping buildings anymore

• improve handling of ignore action for messages produced by MapCSSTagCheckerTest. This requires a change in the entries stored in preferences.xml.
• partly revert r16247 to re-enable ignore button for groups
• add code to recognize and remove ignore list entries produced with older version of JOSM. Users have to re-add those items by ignoring the errors again.

[Klumbumbus]

Milestone renamed

[simon04]

In 16443/josm:

see #19053 - fix cleanup3000, add non-regression test

[GerdP]

With this change I cannot ignore the group "missing tag" forever because the corresponding entry is removed from the ignore list. With the old code this was possible.

[GerdP]

The old code did exactly what I wanted. I'll add a unit test to show what that is.

[GerdP]

Forget it. The code is completely obsolete. It was added in 19053.6.patch (where it made sense). In 19053.7.patch I changed the key so that the code in cleanup3000 is obsolete, but forgot to remove it.

[GerdP]

In 16447/josm:

see #19053: remove wrong and previously obsolete method clean3000 and corresponding unit test
We want to be able to ignore groups like "shop inside shop"