Modify

Opened 5 weeks ago

Closed 2 weeks ago

Last modified 2 weeks 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 (6)

19312-sample.osm (1.5 KB) - added by GerdP 5 weeks ago.
test data
19312.patch (4.5 KB) - added by GerdP 5 weeks ago.
19312.2.patch (11.8 KB) - added by GerdP 4 weeks ago.
19312.3.patch (11.6 KB) - added by GerdP 3 weeks ago.
improved i18n strings
tworels.PNG (16.8 KB) - added by GerdP 3 weeks ago.
19312-popup.PNG (11.7 KB) - added by GerdP 2 weeks ago.

Download all attachments as: .zip

Change History (22)

Changed 5 weeks ago by GerdP

Attachment: 19312-sample.osm added

test data

Changed 5 weeks ago by GerdP

Attachment: 19312.patch added

comment:1 Changed 4 weeks 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 4 weeks 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 4 weeks 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 4 weeks ago by GerdP

Summary: detect dependency loops in relationsdetect circular dependencies in relations

comment:5 Changed 4 weeks ago by GerdP

Owner: changed from team to GerdP
Status: newassigned

Changed 4 weeks ago by GerdP

Attachment: 19312.2.patch added

comment:6 Changed 4 weeks 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 3 weeks ago by GerdP

Any comments on the i18n strings?

comment:8 Changed 3 weeks 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 3 weeks ago by GerdP

Attachment: 19312.3.patch added

improved i18n strings

comment:9 Changed 3 weeks 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 3 weeks 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 3 weeks 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 3 weeks ago by GerdP (previous) (diff)

comment:12 Changed 3 weeks 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 3 weeks ago by GerdP

Attachment: tworels.PNG added

comment:13 Changed 3 weeks ago by GerdP

Example for a loop with just two relations:

comment:14 Changed 2 weeks 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 2 weeks ago by GerdP

In 16652/josm:

see #19312: fix unit tests

Changed 2 weeks ago by GerdP

Attachment: 19312-popup.PNG added

comment:16 Changed 2 weeks 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.

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.