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

Functional behavior of _removed revisions #927

Closed
zgramana opened this issue Jun 16, 2015 · 10 comments
Closed

Functional behavior of _removed revisions #927

zgramana opened this issue Jun 16, 2015 · 10 comments

Comments

@zgramana
Copy link
Contributor

zgramana commented Jun 16, 2015

In the process of discussing the behavior of _removed revisions (see couchbase/couchbase-lite-ios#671) questions arose regarding the expected behavior for certain scenarios:

  • Re-granting read access to a doc, via the sync function, that previously _removed but had not yet been purged from a user's CBL database.
  • Loss of read access to a document due to administrative removal of access to a channel.
@zgramana
Copy link
Contributor Author

Also related to #264.

@amazkovoi
Copy link

Hi Guys,

I hope you do not mind me jumping in here.

#264 was raised by us (SafetyCulture). jnordberg has moved to another company since then.

We are very much in need of documents to be completely removed from mobile devices when the user loses access to a channel or when a document is removed from a channel.

Are you considering implementing this behaviour?

If I understand correctly, the current behaviour is that documents are left on the device. They are not updated with latest revisions, but are not removed. We would like them to be removed.

@adamcfraser
Copy link
Collaborator

After reviewing with the team, the fix for this issue will be as follows:

To restate the problem: Currently, when Sync Gateway identifies as revision as being removed for a user, it sends a _changes-only copy of that revision with a _removed property added. If the user later gets access to that document without a change being made to the document itself, the old '_removed' revision isn't replaced on the client.

Sample use case:

  1. Doc foo is in channels A, B
  2. User alice has access to channel A, replicates and gets doc foo, rev 1-abc
  3. Doc foo is updated and removed from channel A.
  4. User alice replicates foo 2-def, with removed=true over the changes feed
  5. User alice gets granted access to channel B by document 'bar'
  6. User alice gets a backfill of channel B over the changes feed, which includes a non-removed version of foo rev 2-def. Couchbase Lite sees that it already has rev 2-def (from step 4), so the copy of the revision with removed=true remains in the client DB.

Instead, SG should do the following at step 4:

  • Create a new (transient) revision with body {"_deleted":true, "_removed"=true}
  • Generate a new revision number based on that JSON (2-ghi)
  • Send that new revision on the changes feed (but don't persist it to the server DB)
  • If the client later gets access to revision 2-def, it will be treated as the winning revision on the client.

This doesn't address the scenario where users lose channel access - that's still going to be tracked with #264.

@amazkovoi
Copy link

Thanks for the update, Adam.

Sorry, I did not mean to derail this issue.

@adamcfraser
Copy link
Collaborator

@pasin identified a scenario that's not covered by the above - where a client is the originator of the revision that causes them to lose access, and so they already have a local copy of 2-def.

That client will still pull the new 2-ghi, but the already-existing 2-def will be treated as the winning revision by the client. Needs further discussion to identify how to handle this case.

@adamcfraser
Copy link
Collaborator

After followup review with the team, the issue discussed in the previous comment (users making a change that would revoke access to a revision they created) will need to be addressed in future, potentially as part of #264.

Implementation notes on this issue:

  • We currently set the removed property in makeChangeEntry() in changes.go. The bulk of the work can be done there - to set the deleted flag, and update the revision.
  • The new revision number can be generated using the standard algorithm based on a body with only the _deleted and _removed properties. I don't think we can otherwise rely on having the revision body available at this point.

However, there's a tricky scenario needs to be considered and handled:

  1. The document is originally in two channels (A and B)
  2. The user has access to both A and B
  3. The document has a new revision and is removed from B
  4. The document should be sent with the removed property (for removal from B), but without the deleted flag, and using the real revision number.

We're already doing some merge handling of the 'removed' properties (https://github.com/couchbase/sync_gateway/blob/master/src/github.com/couchbase/sync_gateway/db/changes.go#L373). I think this will need to be enhanced to ensure that the correct value of the deleted flag and revision number are sent.

@adamcfraser adamcfraser assigned ajres and unassigned adamcfraser Jun 26, 2015
@ajres ajres added ready and removed in progress labels Jul 6, 2015
@atom992
Copy link

atom992 commented Jan 14, 2016

I am using

    com.couchbase.lite:couchbase-lite-android:1.1.0
    com.couchbase.lite:couchbase-lite-java-listener:1.1.0
    com.couchbase.lite:couchbase-lite-java-javascript:1.1.0

on android 5.0.1 and Sync Gateway 1.1.0-28 on Ubuntu 12.0.4 64 bit

I have the similar issue, when the user lose access of doc1's channel A,the changed of doc1 will not sync, and from cbl, the doc1 don't have _removed=true property.
any plan to fixed the issue ?

@adamcfraser
Copy link
Collaborator

@atom992 That doesn't sound like the same issue described above. You should be getting removal notification in that scenario currently, unless the user has access to the doc via another channel.

@zgramana zgramana modified the milestones: 1.3, 1.2.0 Jan 15, 2016
@zgramana zgramana added backlog and removed ready labels Apr 18, 2016
@adamcfraser adamcfraser modified the milestones: 1.3, Future May 26, 2016
@adamcfraser
Copy link
Collaborator

@pasin I'd like to sync up to review whether this is still a valid issue/potential enhancement for SG, or it should be closed.

@adamcfraser adamcfraser removed this from the Future milestone Oct 12, 2016
@djpongh djpongh added this to the 2.2.0 milestone Jan 31, 2018
@djpongh djpongh modified the milestones: Iridium, Cobalt Aug 28, 2018
@djpongh djpongh removed the P3: low label Aug 30, 2018
@djpongh djpongh modified the milestones: Cobalt, Mercury Dec 13, 2018
@adamcfraser
Copy link
Collaborator

Incorporated into https://issues.couchbase.com/browse/CBG-639

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants