Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

do not return common revision if local node was marked for deletion #1160

Merged
merged 1 commit into from Feb 6, 2019

Conversation

3 participants
@iLiviu
Copy link
Member

commented Feb 5, 2019

Fixes #1150

If BaseClient.getObject/getFile were called after a file has been removed, but before a successful sync, they were still returning the common revision of the file, if present. This happened because the cachingLayer never checked if the local version exists but was deleted.

This issue was especially problematic when working offline and no sync was done to flush local version.

do not return common revision if local node was marked for deletion
If BaseClient.getObject/getFile were called after a file has been removed, but before a successful sync, they were still returning the common revision of the file, if present. This was especially problematic when working offline.
@skddc

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

I tried to reproduce the actual issue, but I failed. It looks like this is an edge case, which only happens when you call getObject directly in the same session without reloading. But I don't have an app at hand that could do that right now. This change also didn't show any negative side effects and the explanation does make sense to me. So I'm +1 on merging this.

@galfert Could you have a look at the code real quick and see if you have any objections to merging?

@skddc

skddc approved these changes Feb 6, 2019

@iLiviu

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

This can only be reproduced successfully on an application that works offline, because as soon as a successful sync is completed, the local changes are pushed remotely and file removed. When working offline, problem persists even if the page is reloaded.

@skddc

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

I tried it with the latest version of Webmarks, which does work fully offline. But I just realized, the reason for it not loading the bookmark on the edit screen anymore after deletion, is that it keeps the bookmarks collection in memory locally, and that's where it loads the object from.

@galfert

galfert approved these changes Feb 6, 2019

Copy link
Member

left a comment

The explanation makes sense and the code looks good to me.

Great work!

@skddc skddc merged commit 17c9876 into remotestorage:master Feb 6, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@iLiviu iLiviu deleted the iLiviu:bugfix/1150-baseclient.remove_while_offline branch Feb 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.