#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)
Change History (29)
by , 5 years ago
| Attachment: | 19312-sample.osm added |
|---|
by , 5 years ago
| Attachment: | 19312.patch added |
|---|
comment:1 by , 5 years ago
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 by , 5 years ago
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 by , 5 years ago
comment:4 by , 5 years ago
| Summary: | detect dependency loops in relations → detect circular dependencies in relations |
|---|
comment:5 by , 5 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
by , 5 years ago
| Attachment: | 19312.2.patch added |
|---|
comment:6 by , 5 years ago
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:8 by , 5 years ago
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
comment:9 by , 5 years ago
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 by , 5 years ago
"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 by , 5 years ago
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"
comment:12 by , 5 years ago
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.
by , 5 years ago
| Attachment: | tworels.PNG added |
|---|
by , 5 years ago
| Attachment: | 19312-popup.PNG added |
|---|
comment:16 by , 5 years ago
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 by , 4 years ago
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?
- Continue and create circular dependency
- Cancel creation of circular dependency
Just wondering what options I have other than permanently down-grading to an old version.
comment:18 by , 4 years ago
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 by , 4 years ago
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 by , 4 years ago
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.
by , 4 years ago
| Attachment: | 19312-allow.complex.dependency.patch added |
|---|
implement preference key validator.relation.allow.complex.dependency to disable warnings from validator or relation editor
comment:21 by , 4 years ago
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!
test data