[PATCH] Validator rules for connectivity relations

[Traaker_L]

Validator rules that check for references to non-existent lanes in the connectivity tag, no connectivity tag in the relation, or an unknown role in the relation.

[Don-vip]

Thanks! For the final patch, please also make sure to run ant pmd checkstyle and check there's no violation.

[Don-vip]

Is it still WIP?

[anonymous]

Replying to Don-vip:

Is it still WIP?

It is not. Sorry, I forgot to remove that when I added the new patch file. It's now ready to go!

[Traaker_L]

Also, credit where credit is due, the unit tests were written by taylor.smock.

[Don-vip]

Remarks:

• New presets items need an SVG icon.
• conflict with r15405. Test error codes range must be increased
• why return a new TreeMap for relations without connectivity instead of Collections.emptyMap()?
• please catch NumberFormatException in case the number parsing fails
• don't compile patterns in a loop. Please make them static final fields
• use Boolean.TRUE / Boolean.FALSE for the constants you put in the map
• when using equals with a constant, please use <constant>.equals(<variable>)

[Traaker_L]

Replying to Don-vip:

Remarks:

• New presets items need an SVG icon.
• conflict with r15405. Test error codes range must be increased
• why return a new TreeMap for relations without connectivity instead of Collections.emptyMap()?
• please catch NumberFormatException in case the number parsing fails
• don't compile patterns in a loop. Please make them static final fields
• use Boolean.TRUE / Boolean.FALSE for the constants you put in the map
• when using equals with a constant, please use <constant>.equals(<variable>)

Thank you very much for the feedback! I will make sure to address those issues as soon as possible.

[Traaker_L]

The patch is ready to go!

[Traaker_L]

Any updates on when this could be incorporated?

[Traaker_L]

Just verified that the patch still works with the latest version of JOSM. Any updates on when we could incorporate this?

[Klumbumbus]

The icon is not really recognizable at a size of 16px:

Could you please rework it?
(see wiki:/DevelopersGuide/DefaultPresets)

[Traaker_L]

Here's a new svg that should be more recognizable at lower resolutions. It is public domain, and is taken from the wiki.

[Klumbumbus]

Replying to Traaker_L:

Here's a new svg that should be more recognizable at lower resolutions.

Not really, looks like an eating giraffe ;)

[floscher]

Alternative suggestion that should be a bit more recognizable at 16×16px

[floscher]

this is a slight variation of the proposed icon, just a bit more pixel-aligned for 16×16px.

this would be in my opinion a better alternative, much more recognizable than a traffic sign at such a small resolution.

[floscher]

Alternative suggestion that should be a bit more recognizable at 16×16px

[Traaker_L]

Thank you kindly for the alternative icon, floscher!

[Klumbumbus]

In the preset <text key="connectivity" text="Lane Connectivity" /> should not be optional. That can be fixed during commit, no need for another patch just for this. Apart from that the preset and icon is ok.
@Gerd, could you have a look at the rest of the patch and commit if ok as Vincent is on holiday?

[GerdP]

I have to learn what a conectivity relation is about...

[GerdP]

I've used overpass api wizard to download all connectivity relations:

type:relation and type=connectivity global

When I run validator on this I get a bug report:

Build-Date:2019-12-05 08:29:00
Revision:15558
Is-Local-Build:true

Identification: JOSM/1.5 (15558 SVN en) Windows 10 64-Bit
OS Build number: Windows 10 Home 1903 (18362)
Memory Usage: 756 MB / 1753 MB (544 MB allocated, but free)
Java version: 1.8.0_191-b12, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: \Display0 1920x1080
Maximum Screen Size: 1920x1080
VM arguments: [-agentlib:jdwp=transport=dt_socket,suspend=y,address=localhost:59992, -ea, -Dfile.encoding=UTF-8]
Program arguments: [--debug]
Dataset consistency test: No problems found

Plugins:
+ OpeningHoursEditor (35242)
+ apache-commons (35092)
+ buildings_tools (35171)
+ continuosDownload (82)
+ ejml (35122)
+ geotools (35169)
+ jaxb (35014)
+ jts (35122)
+ merge-overlap (35072)
+ o5m (34908)
+ opendata (35179)
+ pbf (35033)
+ poly (34991)
+ reverter (35226)
+ undelete (34977)
+ utilsplugin2 (35238)

Last errors/warnings:
- W: No configuration settings found.  Using hardcoded default values for all pools.
- E: Handled by bug report queue: java.lang.NumberFormatException: For input string: "bw"
- E: Handled by bug report queue: java.lang.NumberFormatException: For input string: "bw"
- E: Handled by bug report queue: java.lang.NumberFormatException: For input string: "bw"


=== REPORTED CRASH DATA ===
BugReportExceptionHandler#handleException:
No data collected.

Warning issued by: BugReportExceptionHandler#handleException

=== STACK TRACE ===
Thread: AWT-EventQueue-0 (20) of main
java.lang.NumberFormatException: For input string: "bw"
	at java.lang.NumberFormatException.forInputString(NumberFormatException.java:65)
	at java.lang.Integer.parseInt(Integer.java:580)
	at java.lang.Integer.parseInt(Integer.java:615)
	at org.openstreetmap.josm.data.validation.tests.ConnectivityRelations.parseConnectivityTag(ConnectivityRelations.java:69)
	at org.openstreetmap.josm.data.validation.tests.ConnectivityRelations.checkForInconsistentLanes(ConnectivityRelations.java:108)
	at org.openstreetmap.josm.data.validation.tests.ConnectivityRelations.visit(ConnectivityRelations.java:101)
	at org.openstreetmap.josm.data.osm.Relation.accept(Relation.java:175)
	at org.openstreetmap.josm.data.validation.Test.visit(Test.java:212)
	at org.openstreetmap.josm.actions.ValidateAction$ValidationTask.realRun(ValidateAction.java:168)
	at org.openstreetmap.josm.gui.PleaseWaitRunnable.doRealRun(PleaseWaitRunnable.java:94)
	at org.openstreetmap.josm.gui.PleaseWaitRunnable.run(PleaseWaitRunnable.java:142)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

[GerdP]

I think I'd prefer to have

protected static final int UNKNOWN_CONNECTIVITY_ROLE = 3901;

instead of

protected static final int UNKNOWN_CONNECTIVITY_ROLE = INCONSISTENT_LANE_COUNT + 1;

Two reasons:

• It is easier to find the source which uses error code 3901
• The error UNKNOWN_CONNECTIVITY_ROLE is not at all related to INCONSISTENT_LANE_COUNT

[GerdP]

I get a NPE when checking relation 10342984

[Traaker_L]

Thank you for the feedback, will get this all figured out and addressed!

[Traaker_L]

New patch addressing the crashes encountered when validating all connectivity relations

[GerdP]

• Please use trn() to translate plurals like "Relation {0} member(s) missing lanes tag"
• Sonarlint complains about lane[0].equals("bw"), change it to "bw".equals(lane[0])
• The relation 9960890 has connectivity="1:1|2:2(3)" and produces "Connectivity tag contains unusual data (1)". Would it be possible to give a hint like "comma is missing" ?
• I created a relation with two ways having lanes=2 and the rel with connectivity="1:1|2:2". My understanding is that such a relation is not needed as it describes the default connectivity. Maybe produce a warning like "Obsolete connectivity relation" or "connectivity relation describes the default connectivity"?
• The wiki states that you can use multiple via nodes, but I get a warning "Relation contains 2 via roles. (1)" when I do that. For example, I replaced the via way in relation 8555466 by the two end nodes of that way to produce this warning. The wiki says "Via members should be nodes when possible". There is no example showing more than 1 via member.
• Not sure if relation 9496171 makes any sense? It seems obsolete to me.

To be honest, I do not yet understand most of the text in the wiki. It often tries to describe a very complex relation type with examples. I guess I can create dozens of special cases where I get no warning or a false positive. Let me know if I should continue.

[taylor.smock]

Replying to GerdP:

• Please use trn() to translate plurals like "Relation {0} member(s) missing lanes tag"
• Sonarlint complains about lane[0].equals("bw"), change it to "bw".equals(lane[0])
• The relation 9960890 has connectivity="1:1|2:2(3)" and produces "Connectivity tag contains unusual data (1)". Would it be possible to give a hint like "comma is missing" ?
• I created a relation with two ways having lanes=2 and the rel with connectivity="1:1|2:2". My understanding is that such a relation is not needed as it describes the default connectivity. Maybe produce a warning like "Obsolete connectivity relation" or "connectivity relation describes the default connectivity"?
• The wiki states that you can use multiple via nodes, but I get a warning "Relation contains 2 via roles. (1)" when I do that. For example, I replaced the via way in relation 8555466 by the two end nodes of that way to produce this warning. The wiki says "Via members should be nodes when possible". There is no example showing more than 1 via member.

IIRC, when we were originally coding the check, it was assumed that the relation followed the standard from-via-to understanding of the via role, where you can have one node, one way, or multiple ways as the via role. I've asked for and got clarification on the Talk page of the wiki, and it is one node, one way, or multiple ways.

• Not sure if relation 9496171 makes any sense? It seems obsolete to me.

It looks like Connectivity Relations may be needed if (a) the lane is used in another connectivity relation or (b) the connectivity is non-default.

To be honest, I do not yet understand most of the text in the wiki. It often tries to describe a very complex relation type with examples. I guess I can create dozens of special cases where I get no warning or a false positive. Let me know if I should continue.

As far as default connectivity goes, from ​https://wiki.openstreetmap.org/wiki/Relation:connectivity#Default_connectivity:

• If not at an intersection, and lanes from == lanes to and connectivity=1:1|2:2|...
• There are placement tags and it is not an intersection, and the placement tags can fill in for connectivity
• If it is a merging highway, and the incoming lanes == the outgoing lanes, then connectivity is not needed (if things don't get complex)

[Traaker_L]

Currently working on next patch. It will fully address the trn(), Sonarlint, unusual data specificity, and obsolete connectivity relation issues.

[Traaker_L]

New patch, am still working on addressing some sonar lint cognitive complexity issues with a few functions, and I'm still adding javadoc stuff, but functionally the patch is good to go. It checks for default connectivity based on placement tags, lane tags, and intersections as described on the wiki page, and I added a specific warning for missing commas in the connectivity tag.

[GerdP]

Looks much better now :)

• I've tried my previous example (relation 8555466) with two via nodes. The message still says "Relation contains 2 via roles." This seems to imply that only one via role is allowed. I think "Relation contains 2 'via' nodes" would be better here, and maybe "Multiple 'via' roles only allowed with ways" for the case that via-nodes and via-ways are mixed?
• Some problems are also reported by RelationChecker, e.g. two from members. I think you should remove those duplicate messages. Note that RelationChecker puts the role in quotes, please do the same.
• I created a nonsense type=connectivity relation with two ways tagged only highway=residential and connectivity=1:4. I only get a Severity.OTHER message "Relation 'from', 'to' members missing lanes tag (1)". This message is a bit cryptic and I am missing a warning "Inconsistent lane numbering between relation and members". Not sure but I think lanes=1 is default if no lanes tag is given? OTOH a connectivity relation seems to be pointless when there is only one lane?

[Traaker_L]

More clarification for warning messages about multiple/mixed via members, removal of redundant role checks covered by relation checker, changing of "inconsistent lane warning" severity to WARNING, assume that way has at least one way and don't throw error if connectivity relation only seems to deal with one way.

[Traaker_L]

Replying to GerdP:

Looks much better now :)

• I've tried my previous example (relation 8555466) with two via nodes. The message still says "Relation contains 2 via roles." This seems to imply that only one via role is allowed. I think "Relation contains 2 'via' nodes" would be better here, and maybe "Multiple 'via' roles only allowed with ways" for the case that via-nodes and via-ways are mixed?
• Some problems are also reported by RelationChecker, e.g. two from members. I think you should remove those duplicate messages. Note that RelationChecker puts the role in quotes, please do the same.

I've covered these two matters!

• I created a nonsense type=connectivity relation with two ways tagged only highway=residential and connectivity=1:4. I only get a Severity.OTHER message "Relation 'from', 'to' members missing lanes tag (1)". This message is a bit cryptic and I am missing a warning "Inconsistent lane numbering between relation and members". Not sure but I think lanes=1 is default if no lanes tag is given? OTOH a connectivity relation seems to be pointless when there is only one lane?

It makes sense that the presence of at least single lane can be assumed if a connectivity relation has it as a member, so I went ahead and prevented the lane numbering warning from showing if the relation is only dealing with that first lane. In regards to the lanes warning, the "Inconsistent lane numbering" warning doesn't show up in the case of a member that isn't tagged with its lanes, since there isn't an exact community-defined default for assumed number of lanes on the way in this case. I also meant to have the severity of the "missing lanes" be the issue that is raised with the relation, in the case of your nonsense relation. I have altered the severity for that warning appropriately.

[GerdP]

Please update your local version before creating a new patch.
The javadoc still seems in work? I see "Check the relation to see if the connectivity described is already implied by the relation members' tags" multiple times.

Found a few more special cases:

• r10579524 still produces 4 warnings, the two from the connectivity test seem obsolete:

  + Connectivity relation is missing at least one necessary role (1)
    Role verification problem (2)
      Role 'through' is not in templates 'from/via/to' (1)
      Role 'to' missing (1)
  + Unkown role in connectivity relation (1)
  

• It seems that a mapper did not understand the connectivity tag. Validate e.g. these two relations: r10460738 r10460739. No idea if it is worth to catch these? JOSM already creates a "Relations with same members" warning.

[Traaker_L]

Patch for latest version, finished up javadoc, removed redundant role verification checks

[Traaker_L]

I think we can just let JOSM's "same members" warning catch a case like ​osmwww:relation/10460738 and ​osmwww:relation/10460739.

[GerdP]

In 16295/josm:

fix #18138: Validator rules for connectivity relations
18138.8.patch by Traaker_L with small modifications

• new preset Lane Connectivity
• new validator test ConnectivityRelations

[GerdP]

Sorry, almost forgot this patch.

[GerdP]

And now I remember why I didn't commit it: One unit test fails:
https://josm.openstreetmap.de/jenkins/job/JOSM/lastCompletedBuild/jdk=JDK8/testReport/org.openstreetmap.josm.data.validation.tests/ConnectivityRelationsTest/testForBadRole/

[GerdP]

In 16298/josm:

see #18138: remove obsolete unit test

[Don-vip]

We should abstain to introduce new i18n strings with quotes ('):

• Nobody translate them correctly on Launchpad
• Nobody fix the issues reported by Jenkins on Launchpad

I just fixed manually 51 translation errors affecting almost all languages translated the last week... :(

Or we should find a way to automatically fix these errors. I'm tired of fixing them manually for years.

[Don-vip]

Ah it's not even good in English:

[exec] JAVA translation issue for language en: Mismatching single quotes:
     [exec] Translated text: Multiple 'via' roles only allowed with ways
     [exec] Original text: Multiple 'via' roles only allowed with ways

     [exec] JAVA translation issue for language en: Mismatching single quotes:
     [exec] Translated text: No 'connectivity' tag in connectivity relation
     [exec] Original text: No 'connectivity' tag in connectivity relation

     [exec] JAVA translation issue for language en: Mismatching single quotes:
     [exec] Translated text: Relation should not contain mixed 'via' ways and nodes
     [exec] Original text: Relation should not contain mixed 'via' ways and nodes

     [exec] JAVA translation issue for language en: Mismatching single quotes:
     [exec] Translated text: Relation {0} members missing 'lanes' or '*:lanes' tag
     [exec] Original text: Relation {0} member missing lanes tag

@Gerd: please fix these strings. They're not correct, so everyone translated them for nothing. Also, please check at Jenkins for i18n failures after you add new i18n strings

[GerdP]

@Vincent: Do you want me to remove the quotes or duplicate them? Reg. "not good English": Please suggest improvements.

[GerdP]

Also, please check at Jenkins for i18n failures after you add new i18n strings

Yes, sorry, forgot about that part again. It would be much easier if I could run that check on my (Windows) machine.

[Don-vip]

Replying to GerdP:

@Vincent: Do you want me to remove the quotes or duplicate them? Reg. "not good English": Please suggest improvements.

See Translations#Software

[GerdP]

Please review the patch.
In comment:28 I suggested to

Note that RelationChecker puts the role in quotes, please do the same.

I think this is still true, so I duplicated the quotes in those strings.

[GerdP]

allternative using U+2019 RIGHT SINGLE QUOTATION MARK instead of duplicated quotes

[stoecker]

Simply use two single quotes: ''. Translators are used to that and copy it when translating.

NOTE: Two single quotes are displayed as one single quote! One single quote is not displayed: ​https://tonnygaric.com/blog/escape-single-quotes-when-using-java-messageformat

That's a really strange design decision of Java.

[simon04]

Replying to stoecker:

That's a really strange design decision of Java.

They chose ' to be the escape character, see ​https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/text/MessageFormat.html –

Within a String, a pair of single quotes can be used to quote any arbitrary characters except single quotes. For example, pattern string "'{0}'" represents string "{0}", not a FormatElement. A single quote itself must be represented by doubled single quotes '' throughout a String.

[stoecker]

Replying to simon04:

Replying to stoecker:

That's a really strange design decision of Java.

They chose ' to be the escape character, see ​https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/text/MessageFormat.html –

Within a String, a pair of single quotes can be used to quote any arbitrary characters except single quotes. For example, pattern string "'{0}'" represents string "{0}", not a FormatElement. A single quote itself must be represented by doubled single quotes '' throughout a String.

As said. Very strange design. Who uses such a common character as a single quote as escape character (especiall as it's even part of normal sentences as in don't). Usually you choose a seldom used character for such purpose.

[GerdP]

In Translations#Software I am requested to use U+2019 RIGHT SINGLE QUOTATION MARK instead of duplicated quotes.
I've found only one source using this for translated string: PTAssistantPluginPreferences.java

[simon04]

Replying to stoecker:

Usually you choose a seldom used character for such purpose.

We can be lucky that they haven't chosen e U+0065 (LATIN SMALL LETTER E) since it would be the obvious choice for escape. ;-)

[simon04]

Replying to GerdP:

In Translations#Software I am requested to use U+2019 RIGHT SINGLE QUOTATION MARK instead of duplicated quotes.
I've found only one source using this for translated string: PTAssistantPluginPreferences.java

Translations?action=diff&version=45

[stoecker]

Replying to GerdP:

In Translations#Software I am requested to use U+2019 RIGHT SINGLE QUOTATION MARK instead of duplicated quotes.
I've found only one source using this for translated string: PTAssistantPluginPreferences.java

In the translation. In the source we don't do this yet as it may cause to much issues (although UTF-8 is more common nowadays, so these problems probably will be much less likely then when I first made that suggestion).

[GerdP]

OK, so 18138.i18n.patch is the preferred choice. I've tried to improve the text reg. good English. Any more improvements needed?

[simon04]

I think you've misread Don-vip's statement "not even good in English" -- I think he was implying that the quotation marks are wrong in the English texts, not that the wordings of the English texts is bad/wrong.

[GerdP]

Maybe. I wait for comments from Vincent and Traaker_L. If neither complains I'll commit 18138.i18n.patch tomorrow because I think it improves also the texts.

[stoecker]

Patch looks good to me. New wording is better I'd say.

[GerdP]

In 16372/josm:

fix #18138: Validator rules for connectivity relations
fix single quotes in i18n string, improve wording

[Klumbumbus]

Milestone renamed