Currently we post to /api/preview/{entity_type}/{entity} when the model changes and we need to udpate the preview.
This saves to the auto-save without discrimination.
This means that last user who triggers an auto-save can wipe out the previous work of other users. They can also wipe out their previous work if they happen to have an open tab editing the same entity in XB that they left open days ago.
Example
Another example could be when user 1 and 2 are trying to edit the page at the same time. Everytime 1 user makes a change and triggers an auto-save it wipe the changes the other user just made.
\Drupal\experience_builder\Controller\ApiLayoutController::get(), \Drupal\experience_builder\Controller\ApiLayoutController::patch and \Drupal\experience_builder\Controller\ApiLayoutController::post include a unique UUI for each auto-save in their response, for the main entity and all regions. \Drupal\experience_builder\Controller\ApiLayoutController::patch(). and \Drupal\experience_builder\Controller\ApiLayoutController::post. If the auto-save hashes do not match the match the current auto-save hashes return an error code to the client. While not perfect it will prevent any user from losing more that there edit in XB. Because we trigger an auto-save for each edit they will not be able do more than 1 edit before they find out they can't edit. If another user is not currently editing the same entity, for instance if the current user just left a tab open for an hour or a few days, refreshing the browser tab would allow to continue editing.
Not in this issue but in the future once #3492065: Enable concurrent editing: Replace the postPreview action with atomic equivalents is done we can look into....
For each item in the model, keep a version number, initialize this to 1 when we first write to the auto-save entry for that entity.
With all patchModelAndLayout and patchComponent operations, check the value sent by the client against the currently stored value.
If they don't match, return a HTTP Conflict error.
If they do, increment the version when updating the value.
Work out how to do something similar for the layout structure.
If another user has triggered an auto-save since the current user has made changes then the current user will get an error
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
larowlanComment #3
wim leers#3502902: Only auto-save content entities/PageRegion config entities when there are actual changes: simply previewing incorrectly causes them to appear in "Review x changes" already added the essential missing piece: auto-save hashes. See @larowlan's comment at https://git.drupalcode.org/project/experience_builder/-/merge_requests/7....
Comment #4
wim leers… which AFAICT would not need everything written up in the issue summary — because an even more rudimentary implementation is now possible.
Comment #5
larowlanThe auto save hashes are at the whole entity level. But we need them at the component level
Comment #6
tedbowre #4 and #5
I chatted with @lauriii, @effulgentsia and @balintbrews about this on yesterday about this.
The decision was that as first pass we should only check that the user has the latest version of auto-save, probably by comparing that last known auto-save.
so re #5
I agree this is not an as ideal as what you have suggested, it still would allow us to add more advanced conflict detection later.
Comment #7
tedbowComment #8
tedbowworking on this
Comment #10
tedbowRight now in the MR I have only worked on getting the 3
\Drupal\Tests\experience_builder\Kernel\ApiLayoutController*Testclasses to passa bunch of other things should break because they are not sending the new 'autoSaves' key back to the server. Probably all of the e2e tests should fail
Comment #11
tedbowThis is still very rough. I will pick it back up tomorrow
Comment #12
tedbowI am wondering if there will be any security concerns with sending the actual hash of the auto-save to the client.
Right now I don't think there are because we pretty much send the contents of the auto save to the client via
\Drupal\experience_builder\Controller\ApiLayoutController::getanyways, so having the hash is not giving any more information.but after #3525829: If user with less/different permissions edits a page after a user with more/different permissions data loss could occur we may store fields in the auto-save that user does not view or edit access to. We will then probably fitler what's in the auto-save before we send it to the client.
In that case imagine this scenario
Would it be possible for the user to reconstruct the complete auto-save data from the information they have andgenerate the has themselves, 1 time with the boolean field FALSE and 1 time with the boolean field TRUE and then just check which one matches the hash they were sent?
I know it might be an edge case but this could also work with any field that has limited number of possible values
My other thought is to just send a auto-save version number.
Initially in this issue @larowlan suggested
I am not sure he was considering this or he just suggested using a version number because of security concerns or just he was referring to sub-items of the auto-save we don't have hashes already for.
Comment #13
tedbowTo address #12 I am just going to use UUID that will get saved with auto-save each time.
I didn't want to use an auto-incremented number because a malicious user could just guess the current ones to purposefully wipe of another users changes and save there's.
Comment #14
tedbowComment #15
tedbowre #12 and #13 I actually realized that
modules/contrib/experience_builder/src/Controller/ApiAutoSaveController.php:107already returns hash of auto-save entries so this is not new problem.So I have reverted back to using the hash
Comment #16
tedbowI think this could probably use some other eyes on before I work more on it.
So far it is the back-end implementation I mentioned in #6.
I gave a first pass(with AI help at the front). The front-end changes work with the caveat I mention in the MR about rapid fire requests.
The last test that is failing for me locally is
entity-form-field-types-test.cy.jswhen the test loop through many different fields, so I am pretty sure this is because of the same problem. I could probably get the tests passing with the correct waiting on requests. I have fixed a few test this way in this issue already.Comment #17
tedbowComment #18
tedbowso I don't forget
Comment #19
tedbowTo address @larowlan's feedback I have made this a backend only issue and follow-up handle the Frontend work. I create #3526907: For content entities add client support conflict detection via the `autoSaves' key as follow-up
In that issue I pointed to the commits that remove the
/uiif the person tackling that wants to start from my workComment #20
wim leersDidn't @lauriii consider this a beta blocker?
Discussed with @effulgentsia, and we both agree @larowlan is the ideal reviewer for this.
Comment #21
larowlanAddressed my own review and will merge if this comes back green
Comment #22
larowlanComment #24
larowlanMerged to 0.x - thanks!
Comment #25
wim leersNice!
Next up: make the XB UI use this in #3526907: For content entities add client support conflict detection via the `autoSaves' key.