Modify

Opened 10 years ago

Closed 10 years ago

#4467 closed defect (fixed)

Don't silently drop locally deleted member primitives from downloaded ways and relation

Reported by: mjulius Owned by: team
Priority: critical Milestone:
Component: Core Version: latest
Keywords: r-2010-01-blocker Cc: nakor.wp@…, skyper

Description

JOSM is currently dropping locally deleted way nodes or relation members from downloaded ways and relations.

I believe this is dangerous because it alters the downloaded data without a really good way of restoring that and maybe even without the user noticing. Instead, this should raise a conflict or maybe a warning message with the option to undelete those nodes.

Attachments (0)

Change History (25)

comment:1 Changed 10 years ago by mjulius

Priority: normalcritical

comment:2 Changed 10 years ago by mjulius

Ticket #4403 has been marked as a duplicate of this ticket.

comment:3 Changed 10 years ago by Nakor

Cc: nakor.wp@… added

comment:4 Changed 10 years ago by Nakor

Just copying more explanation from a ticket I opened 2 weeks ago and which was closed as a duplicate of this one. The conflict is actually pointed out but incorrectly silently resolved.

Upload attached file, you will get a conflict that the deleted node still exists in the DB. Choose "Prepare conflict resolution" The resolution window comes up empty Try to upload again (DO NOT actually do it)

JOSM simply deletes the node from the two ways it was in, effectively destroying data without letting the user now.

comment:5 in reply to:  4 Changed 10 years ago by mjulius

Replying to Nakor:

Just copying more explanation from a ticket I opened 2 weeks ago and which was closed as a duplicate of this one. The conflict is actually pointed out but incorrectly silently resolved.

The conflict is pointed out by the API - not by JOSM.

Upload attached file, you will get a conflict that the deleted node still exists in the DB. Choose "Prepare conflict resolution" The resolution window comes up empty Try to upload again (DO NOT actually do it)

JOSM simply deletes the node from the two ways it was in, effectively destroying data without letting the user now.

And this is the issue described in this ticket.

The way it works is this:

  • delete a node that is used by a way JOSM doesn't know about
  • upload => API reports a conflict => JOSM offers to resolve that
  • to do this JOSM loads the way from API which contains the now deleted node
  • because your deleted node and the node downloaded from the API have the same version JOSM does not recognize this as a conflict and keeps the deleted node.
  • and, because the node is deleted it is removed from the way

In short: JOSM never had a conflict in its data - it only has reported to you that the API has detected a conflict. That's why this is not a matter of conflict resolving but one of conflict detecting.

comment:6 Changed 10 years ago by Nakor

Sorry about that. The message in JOSM is confusing then because it says it is going to prepare a conflict resolution.

comment:7 Changed 10 years ago by mjulius

Resolution: fixed
Status: newclosed

(In [2933]) fixes #4467 - Don't silently drop locally deleted member primitives from downloaded ways and relation
conflicts are created instead

comment:8 Changed 10 years ago by Ldp

Seems my #3761 is the same as this?

comment:9 in reply to:  8 Changed 10 years ago by mjulius

Replying to Ldp:

Seems my #3761 is the same as this?

It became the same with your last comment there. I guess we can close that one. JOSM should not modify silently downloaded data.

It's not perfect, yet. See #4476. Also. The conflict dialog should mention when there are referring objects.

comment:10 Changed 10 years ago by jttt

mjulius, please check r2939. I'm not sure if it's related to this ticket, but it's reverting your code and I couldn't find your private e-mail.

comment:11 Changed 10 years ago by mjulius

Thanks for actually doing that. This was a thinking error on my part.

comment:12 Changed 10 years ago by bastiK

Resolution: fixed
Status: closedreopened

I tried, but it wasn't fixed:

  • delete some nodes that are still used by a way, but this way is not known to JOSM
  • upload -> "Upload failed because you tried to delete node ... which is still in use in way ..."
  • click "Prepare conflict resolution"
  • -> nothing, no conflicts displayed
  • Ctrl-a -> error from the measurement plugin (1)
  • when trying to upload again, the same api error comes. In earlier versions, JOSM downloaded the conflicting way. And the issue was gone. (For this single node.)

(Maybe connected to the recent API-reimplementation? (2))

(1)

Build-Date: 2010-02-07 18:56:33		
Revision: 2948
Is-Local-Build: true

Memory Usage: 63 MB / 297 MB (22 MB allocated, but free)
Java version: 1.6.0_15, Sun Microsystems Inc., Linux

Dataset consistency test:
[DELETED REFERENCED] {Way id=49794163 version=1 VT nodes=[{Node id=632652219 version=0 IV }, {Node id=632652215 version=0 IV }, {Node id=632652216 version=1 MVD lat=54.0969967,lon=13.3507683}, {Node id=632652211 version=0 IV }, {Node id=632652212 version=0 IV }]} refers to deleted primitive {Node id=632652216 version=1 MVD lat=54.0969967,lon=13.3507683}


Plugins: measurement,osmarender,photo_geotagging,remotecontrol,tageditor,tagging-preset-tester,terracer,validator,wmsplugin
Plugin wmsplugin Version: 19626
Plugin tageditor Version: 19587
Plugin remotecontrol Version: 19471
Plugin validator Version: 19688
Plugin osmarender Version: 19419
Plugin tagging-preset-tester Version: 19481
Plugin terracer Version: 19678
Plugin photo_geotagging Version: 19672
Plugin measurement Version: 19681

java.lang.NullPointerException
	at org.openstreetmap.josm.plugins.measurement.MeasurementLayer.calcDistance(MeasurementLayer.java:151)
	at org.openstreetmap.josm.plugins.measurement.MeasurementDialog$1.selectionChanged(MeasurementDialog.java:130)
	at org.openstreetmap.josm.data.osm.DataSet.fireSelectionChanged(DataSet.java:285)
	at org.openstreetmap.josm.data.osm.DataSet.addSelected(DataSet.java:445)
	at org.openstreetmap.josm.data.osm.DataSet.setSelected(DataSet.java:390)
	at org.openstreetmap.josm.data.osm.DataSet.setSelected(DataSet.java:403)
	at org.openstreetmap.josm.actions.SelectAllAction.actionPerformed(SelectAllAction.java:23)
	at javax.swing.SwingUtilities.notifyAction(SwingUtilities.java:1636)
	at javax.swing.JComponent.processKeyBinding(JComponent.java:2851)
	at javax.swing.KeyboardManager.fireBinding(KeyboardManager.java:267)
	at javax.swing.KeyboardManager.fireKeyboardAction(KeyboardManager.java:216)
	at javax.swing.JComponent.processKeyBindingsForAllComponents(JComponent.java:2928)
	at javax.swing.JComponent.processKeyBindings(JComponent.java:2920)
	at javax.swing.JComponent.processKeyEvent(JComponent.java:2814)
	at java.awt.Component.processEvent(Component.java:6040)
	at java.awt.Container.processEvent(Container.java:2041)
	at java.awt.Component.dispatchEventImpl(Component.java:4630)
	at java.awt.Container.dispatchEventImpl(Container.java:2099)
	at java.awt.Component.dispatchEvent(Component.java:4460)
	at java.awt.KeyboardFocusManager.redispatchEvent(KeyboardFocusManager.java:1848)
	at java.awt.DefaultKeyboardFocusManager.dispatchKeyEvent(DefaultKeyboardFocusManager.java:704)
	at java.awt.DefaultKeyboardFocusManager.preDispatchKeyEvent(DefaultKeyboardFocusManager.java:969)
	at java.awt.DefaultKeyboardFocusManager.typeAheadAssertions(DefaultKeyboardFocusManager.java:841)
	at java.awt.DefaultKeyboardFocusManager.dispatchEvent(DefaultKeyboardFocusManager.java:668)
	at java.awt.Component.dispatchEventImpl(Component.java:4502)
	at java.awt.Container.dispatchEventImpl(Container.java:2099)
	at java.awt.Window.dispatchEventImpl(Window.java:2475)
	at java.awt.Component.dispatchEvent(Component.java:4460)
	at java.awt.EventQueue.dispatchEvent(EventQueue.java:599)
	at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:269)
	at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:184)
	at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:174)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:169)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:161)
	at java.awt.EventDispatchThread.run(EventDispatchThread.java:122)

(2)
http://lists.openstreetmap.org/pipermail/talk-de/2010-February/062703.html

comment:13 in reply to:  12 Changed 10 years ago by Ldp

Replying to bastiK:

  • when trying to upload again, the same api error comes. In earlier versions, JOSM downloaded the conflicting way. And the issue was gone. (For this single node.)

That's because earlier it silently deleted the node from the other way as well, when doing that conflict 'resolution' thing. I argued earlier (in #3761) that this is not always the desired behaviour.

As both options seem to be equally valid, but applicable to different circumstances, to me it seems that JOSM should ask

"The node {node} you deleted is still present in way {way} on the server.
[Delete from other way as well] [Leave in other way] [Download way and let me handle this]".

Where in the latter case ('let me handle this') it enters a conflict in the conflict list.

comment:14 Changed 10 years ago by mjulius

Resolution: fixed
Status: reopenedclosed

This is a different issue.

The problem here is that when downloading referrers the API only returns the referrers themselves and not the other primitives the referrers refer to. In your example, JOSM downloads the ways that are referring to the deleted node, but not their other nodes. That's why JOSM does not create a conflict and why it cannot display the way. And if you select such a way (by 'Select all' for example) the Measurement plugin throws a NPE when it encounters a way with incomplete nodes.

There need to be separate tickets created for these issues.

comment:15 Changed 10 years ago by mjulius

(In [2950]) when downloading referrers also download way nodes for referring ways
see #4467

comment:16 Changed 10 years ago by bastiK

Nice, that's how it should work... :)

comment:17 Changed 10 years ago by jttt

Resolution: fixed
Status: closedreopened

This is still not perfect. When my dataset contains deleted node that is referenced by way in their dataset, the result is merged dataset with way that reference deleted node. This produce incorrect rendering and might cause problems in other parts of JOSM.

This get fixed after conflict resolution, but user is not forced to do conflict resolution immediately, so for some time JOSM works with inconsistent dataset.

Simple solution would be setting all referenced nodes or members with conflict in my dataset to non deleted state.

comment:18 Changed 10 years ago by Gubaer

Keywords: r-2010-01-blocker added

comment:19 in reply to:  17 Changed 10 years ago by mjulius

Replying to jttt:

This is still not perfect. When my dataset contains deleted node that is referenced by way in their dataset, the result is merged dataset with way that reference deleted node. This produce incorrect rendering and might cause problems in other parts of JOSM.

This get fixed after conflict resolution, but user is not forced to do conflict resolution immediately, so for some time JOSM works with inconsistent dataset.

Maybe we should prevent any data modification when there is a conflict in the dataset.

Simple solution would be setting all referenced nodes or members with conflict in my dataset to non deleted state.

and not produce a conflict at all and simply merge the non-deleted primitive from theirs if it is referenced? Otherwise we would need to remember somehow that the primitive was deleted.

We could also introduce another flag into OsmPrimitive to mark primitives with conflicts. The renderer can then display those and they can be made selectable. This would also allow the renderer to easily highlight all primitives with conflicts.

That might be the best way to handle conflicts in deleted state.

comment:20 Changed 10 years ago by anonymous

We could also introduce another flag into OsmPrimitive to mark primitives with conflicts.

I think we don't have to introduce a flag.

Each OsmLayer maintains a ConflictCollection. We can ask it whether an OsmPrimitive participates in a conflict. Rendering performance could be an issue, though.

comment:21 in reply to:  20 Changed 10 years ago by mjulius

Replying to anonymous:

We could also introduce another flag into OsmPrimitive to mark primitives with conflicts.

I think we don't have to introduce a flag.

Each OsmLayer maintains a ConflictCollection. We can ask it whether an OsmPrimitive participates in a conflict. Rendering performance could be an issue, though.

Exactly. It might be a bet less so if the renderer first builds a HashSet of OsmPrimitives with conflicts. But, looking up a flag is probably still faster.

A performance penalty would be less significant if we only do that for deleted primitives.

Of course, all this only needs to be done when there are any conflicts. Therefore, this would not effect normal operation. It might serve as an incentive to resolve conflicts early. ;-)

comment:22 Changed 10 years ago by jttt

Replying to mjulius:

Simple solution would be setting all referenced nodes or members with conflict in my dataset to non deleted state.

and not produce a conflict at all and simply merge the non-deleted primitive from theirs if it is referenced? Otherwise we would need to remember somehow that the primitive was deleted.

No, there will still be a conflict. Special type of conflict, that will contain reference to primitive and information, that primitive is in fact deleted.

The renderer can then display those and they can be made selectable.

This problem isn't only about rendering, also other parts of JOSM shouldn't be forced to test for deleted nodes/members.

On the other hand, special rendering of primitives in conflict will be useful.

comment:23 Changed 10 years ago by skyper

Cc: skyper added

comment:24 in reply to:  22 Changed 10 years ago by mjulius

Replying to jttt:

On the other hand, special rendering of primitives in conflict will be useful.

Really cool would be if JOSM somehow rendered both versions of a conflicting primitive when there was a change in location.

comment:25 Changed 10 years ago by jttt

Resolution: fixed
Status: reopenedclosed

(In [3034]) Fix #4467 Don't silently drop locally deleted member primitives from downloaded ways and relation (fix the issue when deleted primitive is referenced)

Modify Ticket

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