Modify

Opened 9 months ago

Closed 9 months ago

Last modified 8 months ago

#19312 closed enhancement (fixed)

detect circular dependencies in relations

Reported by: GerdP Owned by: GerdP
Priority: normal Milestone: 20.06
Component: Core validator Version:
Keywords: Cc:

Description

The shortest possible loop is a relation that contains itself as a member. Bigger loops may be formed when a relation A contains relation B as member and B contains A. Or a seqeance like A-B-C-D-A.
I don't think that there is any situation where this makes sense, so this should be flagged as an error.
Of, JOSM can only detect the loops when the corresponding relations are loaded.
I coded this test to find a loop in type=waterway relations and thought it might be useful.
Two new messages are produced:
Relation contains itself as a member
or for the complexer cases:
Relations build a dependency loop
I think the first is clear, the 2nd might be improved. Possible alternative:
Relations build a parent/child dependency loop

For the simple case we can offer an autofix, for the complex case this needs expert knowledge.

Attachments (7)

19312-sample.osm (1.5 KB) - added by GerdP 9 months ago.
test data
19312.patch (4.5 KB) - added by GerdP 9 months ago.
19312.2.patch (11.8 KB) - added by GerdP 9 months ago.
19312.3.patch (11.6 KB) - added by GerdP 9 months ago.
improved i18n strings
tworels.PNG (16.8 KB) - added by GerdP 9 months ago.
19312-popup.PNG (11.7 KB) - added by GerdP 9 months ago.
19312-allow.complex.dependency.patch (1.9 KB) - added by GerdP 8 months ago.
implement preference key validator.relation.allow.complex.dependency to disable warnings from validator or relation editor

Download all attachments as: .zip

Change History (29)

Changed 9 months ago by GerdP

Attachment: 19312-sample.osm added

test data

Changed 9 months ago by GerdP

Attachment: 19312.patch added

comment:1 Changed 9 months ago by simon04

Let's call it "Circular dependency" as this is the commonly used term, see https://en.wikipedia.org/wiki/Circular_dependency

See also: org.openstreetmap.josm.gui.dialogs.relation.GenericRelationEditor#warnOfCircularReferences (maybe we can extract the test logic from org.openstreetmap.josm.gui.dialogs.relation.GenericRelationEditor#addPrimitivesToRelation?)

comment:2 Changed 9 months ago by GerdP

OK, I'll have a closer look at that.

Off Topic: I've just seen r16543. I think you have effectively removed a unit test regarding the iterator implementation of QuadBuckets, but maybe it is done in one of the other unit tests?

comment:3 Changed 9 months ago by GerdP

The code in org.openstreetmap.josm.gui.dialogs.relation.GenericRelationEditor#addPrimitivesToRelation only finds simple circular dependencies. It doesn't detect r1->r2->r3->r1, so I'd rather try to use the new code in the GenericRelationEditor, too

comment:4 Changed 9 months ago by GerdP

Summary: detect dependency loops in relationsdetect circular dependencies in relations

comment:5 Changed 9 months ago by GerdP

Owner: changed from team to GerdP
Status: newassigned

Changed 9 months ago by GerdP

Attachment: 19312.2.patch added

comment:6 Changed 9 months ago by GerdP

With the relation editor I think we have three cases of circular dependencies.
1) User tries to add a relation to itself. This was already handled.
2) User tries to add relation B to relation A and B or one of its children refers to A, e.g. A-B-A or A-B-C-A
3) User tries to add relation B to relation A and B is already part of circular dependency, e.g. A-B-B or A-B-C-B

The patch doesn't handle the 3rd case. I found it is too confusing to bother the user with this. It is quite complex to understand the problem with the 2nd variant.
Please review esp. the new i18n strings in warnOfCircularReferences().
I probably have to improve the javadoc.

comment:7 Changed 9 months ago by GerdP

Any comments on the i18n strings?

comment:8 Changed 9 months ago by Klumbumbus

I too think "circular dependency" is a more common term. And I don't know if "build" is a good choice. Maybe Relations generate a circular dependency of parent/child elements

Changed 9 months ago by GerdP

Attachment: 19312.3.patch added

improved i18n strings

comment:9 Changed 9 months ago by GerdP

Ok, how about these texts?

        if (loop.size() <= 2) {
            msg = tr("<html>You are trying to add a relation to itself.<br>"
                    + "<br>"
                    + "This generates a circular dependency of parent/child elements and is therefore discouraged.<br>"
                    + "Skipping relation ''{0}''.</html>",
                    Utils.escapeReservedCharactersHTML(primitive.getDisplayName(df)));
        } else {
            msg = tr("<html>You are trying to add a child relation which refers to the parent relation.<br>"
                    + "<br>"
                    + "This generates a circular dependency of parent/child elements and is therefore discouraged.<br>"
                    + "Skipping relation ''{0}''." + "<br>"
                    + "Relations that would generate the circular dependency:<br>{1}</html>",
                    Utils.escapeReservedCharactersHTML(primitive.getDisplayName(df)),
                    ...

comment:10 Changed 9 months ago by simon04

"a child relation", "the parent relation": I'm unsure whether it's always clear for the user which relation is meant. Maybe they are not aware of the relevant parent relation at all. What about dropping the terms "child"/"parent", and instead, writing out the relation names (using DefaultNameFormatter)? For instance, "Adding X to Y will create a circular dependency, which is discouraged"

comment:11 Changed 9 months ago by GerdP

The loop itself is reported with the code that I left out with the ...:

                    loop.stream().map(p -> Utils.escapeReservedCharactersHTML(p.getDisplayName(df)))
                            .collect(Collectors.joining(" -> <br>"))); 

The popup is shown in two different situations:
1) in Relation Editor window
2) when you right click on a relation in the relation list and choose "add selection to relation"

Last edited 9 months ago by GerdP (previous) (diff)

comment:12 Changed 9 months ago by GerdP

In an already existing circular depency situation it is of course unclear which relation is parent and which is child, but when one or more relations are added to a relation it should be clear that the added relations are the children.

Changed 9 months ago by GerdP

Attachment: tworels.PNG added

comment:13 Changed 9 months ago by GerdP

Example for a loop with just two relations:

comment:14 Changed 9 months ago by GerdP

Resolution: fixed
Status: assignedclosed

In 16651/josm:

fix #19312: detect circular dependencies in relations

  • add test in RelationChecker to detect them
  • show popup when user is about to create one and refuse the action

I am not happy with the texts but such a rather complex problem probably requires a complex message :(

comment:15 Changed 9 months ago by GerdP

In 16652/josm:

see #19312: fix unit tests

Changed 9 months ago by GerdP

Attachment: 19312-popup.PNG added

comment:16 Changed 9 months ago by GerdP

I've just noticed that there is another method GenericRelationEditor.cleanSelfReferences which might show this when you open a relation that already contains itself as a member:

Can we keep the existing text or should the 2nd sentence be changed to e.g.
This generates a circular dependency of parent/child elements and is therefore discouraged.

comment:17 Changed 8 months ago by anonymous

The changes associated with this seem to make it impossible to create circular dependencies. In my case, I am using josm to edit Lanelet2 maps which use circular dependencies.
When I try to create a circular dependency a warning now pops up saying that it is "discouraged".

Is there a way to disable this pop-up? if not, can the warning pop up with two options?

  1. Continue and create circular dependency
  2. Cancel creation of circular dependency

Just wondering what options I have other than permanently down-grading to an old version.

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

comment:18 Changed 8 months ago by GerdP

Before r16551 JOSM only complained when a relation was added to itself and it wasn't possible to do that.
If I got you right you only need to be able to create more complex circular dependencies?

comment:19 Changed 8 months ago by anonymous

Correct, The relations I'm creating never point to themselves, but it's common for 2 relations to have roles that point to each other.
I believe it's done this way to simply speed up queries on really large maps.

I understand that my use case doesn't really fall under the design scope of JOSM, since I'm not updating OSM, but rather other maps that use the OSM format.
That being said, I see no harm in adding an option to ignore the pop-up, or adding a boolean in the settings somewhere to disable circular dependency warnings.

Thanks for hearing me out :)

comment:20 Changed 8 months ago by GerdP

OK, so for your data you want to be able to create circular complex dependencies and you don't want to see the warning about them, right? Do you use the same JOSM installation/configuration to edit real OSM data as well? If not I think an expert preference would be sufficient.

Changed 8 months ago by GerdP

implement preference key validator.relation.allow.complex.dependency to disable warnings from validator or relation editor

comment:21 Changed 8 months ago by anonymous

OK, so for your data you want to be able to create circular complex dependencies and you don't want to see the warning about them, right?

Correct

Do you use the same JOSM installation/configuration to edit real OSM data as well?

No, I rarely edit OSM data, only on special occasions.

I agree 100% with the expert preference since it would be best if the pop-up simply didn't appear at all instead of having to disregard/cancel it constantly.

Thanks!

comment:22 Changed 8 months ago by GerdP

In 16793/josm:

see #19312: detect circular dependencies in relations

  • implement preference key validator.relation.allow.complex.dependency to disable warnings from validator or relation editor when multiple relations generate a dependency loop

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain GerdP.
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.