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

✨ post update collision detection #8328

Merged
merged 10 commits into from
Apr 17, 2017

Conversation

kirrg001
Copy link
Contributor

@kirrg001 kirrg001 commented Apr 13, 2017

closes #5599

If two users edit the same post, it can happen that they override each others content or post settings. With this PR this won't happen anymore.
This PR can detect post changes and tag changes and if a collision is detected, the user will receive an error back. Each commit should be clear and has enough context to understand the required changes. Some tests i have added are quite complex because they simulate collision cases we have to care about. These tests are not sooo good to read. But can't change this, because they simulate a complex use case. You will see if you go through the commits.

What i didn't test is the rest of the functionality in Ghost. Will do before we release the new LTS version.

  • test on postgres
  • test on node 6
  • more comments?
  • go over code pieces where we use find and edit together
  • add more context to the commits
  • ensure client receives error code
  • tested with 1.0 - have a branch for it as well
  • tested with Ghost Desktop
  • add more tests?
  • Ghost-Admin: don't auto save if user isn't actively changing anything, see Auto-save improvements #8331
  • optimise 1-2 comments
  • make another project search for editing posts

@kirrg001 kirrg001 added the LTS label Apr 13, 2017
@kirrg001 kirrg001 force-pushed the feature/update-collision branch 2 times, most recently from 4a2f307 to 807afc1 Compare April 14, 2017 11:22
closes TryGhost#5599

- add a new bookshelf plugin to detect these changes
- use the `changed` object of bookshelf -> we don't have to create our own diff
- compare client and server updated_at field
- run editing posts in a transaction (see comments in code base)
- `updateTags` for adding posts on `onCreated` - happens after the post was inserted
   --> it's "okay" to attach the tags afterwards on insert
   --> there is no need to add collision for inserting data
   --> it's very hard to move the updateTags call to `onCreating`, because the `updateTags` function queries the database to look up the affected post
- `updateTags` while editing posts on `onSaving` - all operations run in a transactions and are rolled back if something get's rejected
- there was no support for error codes yet
- added a simpler condition into our LTS error handler
- if two queries happening in a transaction we have to signalise knex/mysql that we select for an update
- otherwise the following case happens:
  >> you fetch posts for an update
  >> a user requests comes in and updates the post (e.g. sets title to "X")
  >> you update the fetched posts, title would get overriden to the old one
- use a transaction for listener updates
- signalise forUpdate
- write a complex test
- publish endpoint runs in a transaction
- add complex test
- @todo: right now scheduling api uses posts api, therefor we had to extend the options for api's
  >> allowed to pass transactions through it
  >> but these are only allowed if defined from outside {opts: [...]}
  >> so i think this is fine and not dirty
  >> will wait for opinions
  >> alternatively we have to re-write the scheduling endpoint to use the models directly
@kirrg001 kirrg001 changed the title [WIP] ✨ post update collision detection ✨ post update collision detection Apr 14, 2017
@kirrg001
Copy link
Contributor Author

This is ready for review 🙀

Copy link
Member

@ErisDS ErisDS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a couple of small questions and one minor change to the error message.

I tested this and it seemed to work well 👍

if (Object.keys(changed).length) {
if (clientUpdatedAt.diff(serverUpdatedAt) !== 0) {
err = new errors.InternalServerError('Uh-oh. We already have a newer version of this post saved.' +
'To prevent losing your text, please copy your changes somewhere else and then refresh this page.');

This comment was marked as abuse.

This comment was marked as abuse.

serverUpdatedAt = moment(self.serverData.updated_at);

if (Object.keys(changed).length) {
if (clientUpdatedAt.diff(serverUpdatedAt) !== 0) {

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@@ -459,7 +462,7 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({

return model.fetch(options).then(function then(object) {
if (object) {
return object.save(data, options);
return object.save(data, _.merge({method: 'update'}, options));

This comment was marked as abuse.

This comment was marked as abuse.

options = options || {};

if (options.transacting) {
return editPost(data, options);

This comment was marked as abuse.

if (publishedAtMoment.diff(moment(), 'minutes') > publishAPostBySchedulerToleranceInMinutes) {
return Promise.reject(new errors.NotFoundError(i18n.t('errors.api.job.notFound')));
}
// CASE: apiPosts.read and apiPosts.edit happen in a transaction, signalise `forUpdate` to knex

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

kevinansfield added a commit to kevinansfield/Ghost-Admin that referenced this pull request Apr 17, 2017
refs TryGhost/Ghost#8331, TryGhost/Ghost#8328
- prevents auto-save save if nothing has changed, this avoides unnecessary collision errors when reading a post that another user is editing
@kevinansfield kevinansfield merged commit ae7ad56 into TryGhost:lts Apr 17, 2017
@kevinansfield kevinansfield deleted the feature/update-collision branch April 17, 2017 17:58
kevinansfield added a commit to kevinansfield/Ghost-Admin that referenced this pull request Apr 18, 2017
refs TryGhost/Ghost#8331, TryGhost/Ghost#8328
- prevents auto-save save if nothing has changed, this avoides unnecessary collision errors when reading a post that another user is editing
kirrg001 pushed a commit to TryGhost/Admin that referenced this pull request Apr 18, 2017
refs TryGhost/Ghost#8331, TryGhost/Ghost#8328

- prevents auto-save save if nothing has changed, this avoides unnecessary collision errors when reading a post that another user is editing
kirrg001 added a commit to kirrg001/Ghost that referenced this pull request Apr 19, 2017
closes TryGhost#5599

If two users edit the same post, it can happen that they override each others content or post settings. With this change this won't happen anymore.

✨ Update collision for posts
- add a new bookshelf plugin to detect these changes
- use the `changed` object of bookshelf -> we don't have to create our own diff
- compare client and server updated_at field
- run editing posts in a transaction (see comments in code base)

🙀  update collision for tags
- `updateTags` for adding posts on `onCreated` - happens after the post was inserted
   --> it's "okay" to attach the tags afterwards on insert
   --> there is no need to add collision for inserting data
   --> it's very hard to move the updateTags call to `onCreating`, because the `updateTags` function queries the database to look up the affected post
- `updateTags` while editing posts on `onSaving` - all operations run in a transactions and are rolled back if something get's rejected

- Post model edit: if we push a transaction from outside, take this one

✨  introduce options.forUpdate
- if two queries happening in a transaction we have to signalise knex/mysql that we select for an update
- otherwise the following case happens:
  >> you fetch posts for an update
  >> a user requests comes in and updates the post (e.g. sets title to "X")
  >> you update the fetched posts, title would get overriden to the old one

use options.forUpdate and protect internal post updates: model listeners
- use a transaction for listener updates
- signalise forUpdate
- write a complex test

use options.forUpdate and protect internal post updates: scheduling
- publish endpoint runs in a transaction
- add complex test
- @todo: right now scheduling api uses posts api, therefor we had to extend the options for api's
  >> allowed to pass transactions through it
  >> but these are only allowed if defined from outside {opts: [...]}
  >> so i think this is fine and not dirty
  >> will wait for opinions
  >> alternatively we have to re-write the scheduling endpoint to use the models directly
kevinansfield pushed a commit that referenced this pull request Apr 19, 2017
closes #5599

If two users edit the same post, it can happen that they override each others content or post settings. With this change this won't happen anymore.

✨ Update collision for posts
- add a new bookshelf plugin to detect these changes
- use the `changed` object of bookshelf -> we don't have to create our own diff
- compare client and server updated_at field
- run editing posts in a transaction (see comments in code base)

🙀  update collision for tags
- `updateTags` for adding posts on `onCreated` - happens after the post was inserted
   --> it's "okay" to attach the tags afterwards on insert
   --> there is no need to add collision for inserting data
   --> it's very hard to move the updateTags call to `onCreating`, because the `updateTags` function queries the database to look up the affected post
- `updateTags` while editing posts on `onSaving` - all operations run in a transactions and are rolled back if something get's rejected

- Post model edit: if we push a transaction from outside, take this one

✨  introduce options.forUpdate
- if two queries happening in a transaction we have to signalise knex/mysql that we select for an update
- otherwise the following case happens:
  >> you fetch posts for an update
  >> a user requests comes in and updates the post (e.g. sets title to "X")
  >> you update the fetched posts, title would get overriden to the old one

use options.forUpdate and protect internal post updates: model listeners
- use a transaction for listener updates
- signalise forUpdate
- write a complex test

use options.forUpdate and protect internal post updates: scheduling
- publish endpoint runs in a transaction
- add complex test
- @todo: right now scheduling api uses posts api, therefor we had to extend the options for api's
  >> allowed to pass transactions through it
  >> but these are only allowed if defined from outside {opts: [...]}
  >> so i think this is fine and not dirty
  >> will wait for opinions
  >> alternatively we have to re-write the scheduling endpoint to use the models directly
@ErisDS ErisDS removed their assignment Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants