-
-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Use updated_at
to prevent overwriting new data with old data.
#5599
Comments
@ErisDS — if this is still necessary, I'd like to take it on myself! |
@psvet For sure it's still needed 👍 |
@ErisDS, a few questions: I have the basic Also, I added the functionality to |
@psvet looks like you've updated your PR since that comment - sorry for the slow reply. I think the key thing to check is always the markdown content which is edited and we definitely shouldn't count the |
Closes TryGhost#5599 - refactored `models/base/index#edit` to return a transaction - added `safeToSave` validation method to check `updated_at` in client and DB objects - if client-side `updated_at` is behind DB, and other changes have been made, throw `409 Conflict` error - added `deep-diff` module for calculating changes between objects
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)
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)
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 * Ensure error code is forwarded to the client - there was no support for error codes yet - added a simpler condition into our LTS error handler * 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
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
Ghost's editor is not yet collaborative, which is a sizeable issue that we want to resolve properly in the not too distant future. However, I think in the short-term we could be preventing issues like #4999 and #5450 by preventing overwrites of data when we know we already have a newer version in the database.
When a PUT request is made, it is made with a complete resource that includes the
updated_at
timestamp. With our Ember admin, that data is returned from the server, and simply resent with the next save - the timestamp is not modified by the client (and nor should it be).On the server, we could use this information to determine whether or not it is safe to save. If the
updated_at
timestamp is the same or newer than the one currently in the table, then we can safely save the data, however, if theupdated_at
timestamp is older than the one in the table then we have a more recent version of the data in the database and should not overwrite it with the data that was sent to the API.It is also possible to implement a slightly smarter version of this which also checks if any of the other data in the table is different. If the two versions are the same, there's no need to do anything.
The request to lookup what is in the table, and the request to update it, if safe, should be contained within a transaction, to ensure that we don't end up with race conditions.
I believe the correct thing to do, in the case that the
updated_at
timestamp is out of date and the data cannot be saved, is to return a409 Conflict
error. The recommendation is to return enough data to resolve the conflict, but I'm not aware of any recommendations on the form of the response. I think using our normal error format is fine.Short term, there's no need to do fancy conflict resolution handling, as long term we will hopefully solve all of this with a more advanced solution like Operational Transforms.
For now, showing an error and warning the user to save their version elsewhere and then refresh is one option, perhaps a slightly nicer option is showing the warning in a modal and giving the user the option to save anyway. This would require adding a force save flag to the PUT request.
This is a slightly dirty short term solution, but my thought is that it is better to prevent overwrites, than to mask that they are happening by doing them blindly.
Although this mostly applies to posts, I think it makes sense to implement it in such a way that overriding
updated_at
is prevented across all models by default.The text was updated successfully, but these errors were encountered: