When deleting a local entity, remote version change causes endless loop of conflict resolution

[guyarad]

Repro steps:

1. Delete a node (or other entities) locally.
2. From browser/computer (possibly with another user) make a change to that entity, causing a version bump.
3. Attempt to save your changed.

Actual behavior:

• If you had other modifications, besides deletion, you will see endless loop of upload and then nodes requests to the server (and the UI will display loading).
• otherwise, you will see an endless of loop of upload requests.

Expected behavior:

• A conflict resolution screen is displayed
• Local change, if selected, is saved

[guyarad]

I have done a preliminary analysis of the issue. It seems like the client isn't asking the server for the deleted node as part of the node request, in order to detect the conflicts.

The reason it's not asking for is this line.
However, simply asking for deleted as well won't do the trick, because later when detecting conflicts here, it will fetch the entity from the coreGraph, but due to weird code here it will simply fetch the base entity, which will in turn resolve the conflicts easily without involving the user, and without even knowing it was deleted, so the end result is local changes are essentially discarded.

Seems like there needs to be a flow that's aware of locally deleted nodes and offers the resolve these conflicts properly.

[tyrasd]

Nice catch!

but due to weird code here

Yeah, that was a workaround for a bug in some versions of Chrome/Chromium on Linux. I believe it can be removed now since the underlying bug has been fixed in newer versions of Chrome by now (I assume?!).

[guyarad]

@tyrasd I'm good like that :)

but it can't be really removed easily.
Because if removed, when attempting to get an entity from the coreGraph, it will throw if the entity is deleted locally.
That being said, it's probably a good thing because it will identify places which attempted getting an entity without checking if it's there first.

[guyarad]

Also, in my opinion, hasEntity is a little broken for locally deleted entities. It will return false even thought the graph does have the entity, in a sense.

[tyrasd]

I agree that in order to fix the reported problem, there must be done more than just removing the odd workaround. Maybe it will be necessary to store a visible=false version for deleted objects (instead of just setting them to undefined in the graph) in order to fix the issue. 🤔

[bhousel]

Yeah, that was a workaround for a bug in some versions of Chrome/Chromium on Linux. I believe it can be removed now since the underlying bug has been fixed in newer versions of Chrome by now (I assume?!).

yeah I agree - It's been a year since we needed to patch Chrome 58/59.. we can probably remove it.

Also, in my opinion, hasEntity is a little broken for locally deleted entities. It will return false even thought the graph does have the entity, in a sense.

This is the design of how it works. hasEntity is supposed to return undefined if the entity has been deleted.

That said, the save.js code can be improved - it already checks the modified objects, and it should check the deleted objects too.

[erkinsergey]

I also have this issue in version 2.12.2 when editing and deleting the same objects.
I also saw in source codes that deleted objects are not checked, only modified.
Please inform if something changes.