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

Attachments still stored for documents rejected by sync function #1728

Closed
adamcfraser opened this issue Apr 29, 2016 · 0 comments · Fixed by #1898
Closed

Attachments still stored for documents rejected by sync function #1728

adamcfraser opened this issue Apr 29, 2016 · 0 comments · Fixed by #1898
Assignees
Milestone

Comments

@adamcfraser
Copy link
Collaborator

adamcfraser commented Apr 29, 2016

If a document with attachments gets rejected by the sync function, the attachments are still being stored to the bucket.

@adamcfraser adamcfraser added this to the 1.2.2 milestone Apr 29, 2016
@adamcfraser adamcfraser self-assigned this Apr 29, 2016
@adamcfraser adamcfraser modified the milestones: 1.3, 1.2.2 May 27, 2016
adamcfraser added a commit that referenced this issue Jun 22, 2016
Previously attachments that were sent inline would be persisted to the DB even if the owning document failed sync function validation.

Inline attachment data in the document needs to be removed and replaced with digest references before the revision ID is calculated (since it's a change to the document body), so it's not possible to just move the entire storeAttachments processing after Sync Function execution.  The fix is to split the functionality - the updateDoc callback now updates the attachments to digest in the doc body, then returns any attachment bodies for later storage.  After the document passes Sync Function validation, the attachments are stored in the DB.

This doesn't change the handling for multiple invocations of updateDoc (for concurrent writes).  The previous implementation already had the potential to try to store a document multiple times: if the attachment already existed, it proceeds without error.  The new implementation does the same - the only difference is that the attempted attachment writes happen after Sync Function validation.

There's still a corner case where:
(a) Document passes Sync Function validation on first attempt
(b) Attachments are written
(c) Document write fails and retries (someone else has updated the doc)
(d) Document fails Sync Function validation on second attempt (because oldDoc has changed)

For this corner case the risk/overhead associated with the attachments written in (b) is minor.  In particular, it's not possible to repeatedly trigger this scenario in the same way as the previous issue.

Fixes #1728.
adamcfraser added a commit that referenced this issue Jun 22, 2016
Previously attachments that were sent inline would be persisted to the DB even if the owning document failed sync function validation.

Inline attachment data in the document needs to be removed and replaced with digest references before the revision ID is calculated (since it's a change to the document body), so it's not possible to just move the entire storeAttachments processing after Sync Function execution.  The fix is to split the functionality - the updateDoc callback now updates the attachments to digest in the doc body, then returns any attachment bodies for later storage.  After the document passes Sync Function validation, the attachments are stored in the DB.

This doesn't change the handling for multiple invocations of updateDoc (for concurrent writes).  The previous implementation already had the potential to try to store a document multiple times: if the attachment already existed, it proceeds without error.  The new implementation does the same - the only difference is that the attempted attachment writes happen after Sync Function validation.

There's still a corner case where:
(a) Document passes Sync Function validation on first attempt
(b) Attachments are written
(c) Document write fails and retries (someone else has updated the doc)
(d) Document fails Sync Function validation on second attempt (because oldDoc has changed)

For this corner case the risk/overhead associated with the attachments written in (b) is minor.  In particular, it's not possible to repeatedly trigger this scenario in the same way as the previous issue.

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

Successfully merging a pull request may close this issue.

2 participants