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 #8362

Merged
merged 1 commit into from
Apr 19, 2017

Conversation

kirrg001
Copy link
Contributor

refs #5599

Same as #8328, just for master.
I had to adapt some smaller things like error handling, tests.

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

@kirrg001 kirrg001 changed the title ✨ post update collision detection (#8328) ✨ post update collision detection 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 kevinansfield merged commit c93f03b into TryGhost:master Apr 19, 2017
@kevinansfield kevinansfield deleted the 1.0.0-dev/post-update-coll branch April 19, 2017 13:53
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.

None yet

2 participants