For content entities add client support conflict detection via the `autoSaves' key

I brought in the changes from #3490565: Add rudimentary conflict prevention to the preview end-point because I made progress getting the e2e test to pass, so I thought I would the current state

The only 2 e2e tests that are failing now are `entity-form-field-types-test.cy.js` which does pass for me locally and `media-library.cy.js` which did fail locally but I didn't see an 409 request errors in the log. So maybe these are unrelated random test errors? Not sure, retesting

I am actually surprised that no existing e2e tests failed 🎉

2 new tests failed

1. rapid-component-addition.cy.js: I am not sure this can really be test reliability give the random test fails we have in e2e and this test would solely rely on timing. I am removing it
2. auto-save-conflict.cy.js: This was attempt to try to simulate 2 users with 2 different browser sessions having a conflict. I am not sure how to do this in cypress and searched around a little and couldn't get it to work. Someone with more experience might have better luck

I think there is pontential problem with #11 5)

they have come from a single instance and thus can’t be conflicting concurrent changes.

Is this true? What if the client makes 3 request in sequence and for some reason request 1 takes a long time to connect? Basically request 2 and 3 are processed and then request 1 gets processed. Wouldn't that wipe out changes in 2 and 3? Is what I suggesting even possible, maybe on randomly sluggish networks? Maybe ok to handle this edge case?

re #13 and #14 I am currently a -1 on doing #3492065: Enable concurrent editing: Replace the postPreview action with atomic equivalents because

1. There is nothing in #3492065 for "User interface changes"(I just copied that part of #3490565: Add rudimentary conflict prevention to the preview end-point to here now)
   if we "Enable concurrent editing" there probably will be some very tricky UX problems and we should figure the desired outcomes before we start making the solution

   Will concurrent editing just work perfectly and we all agree exactly what that means so there is no describe the UX? 😜

   for instance what happens if I am editing props for component, which should show the updates I as type, at the same time another user does another operation that moved that operation at out of view? This would either be by the other user reorder components at the same level as mine or adding another component above it that puts my out of view. Or it could be changing the props of component above mine that changes the height.

2. Is there no possibility of conflict still in that system? I understand it would be less possible because you are allowing 2 users to edit different parts of the page. But 2 users could still try to edit the same component where 1 user didn't start with the latest changes that were just auto-saved correct?

   If so think a lot of the problems documented in #11 still exist just at a different level down

   I think #12 out of sync requests from the same user could still be problem. Especially because we send auto-save requests as you type for single prop of single component

   So I think you we wouldn't be saving time but not having to solve #11 it would just be a different level, per component

3. #3492065: Enable concurrent editing: Replace the postPreview action with atomic equivalents just seems more complicated so there would be more unknown unknowns. I think we are further along on the current implementation so there would less, less risk

Just wanted to make clear my comments in #16 were all with the idea that we have limited time before Beta1 - re #3515932: Milestone 1.0.0-beta1: Enable creation of non-throwaway sites and probably have a lot of other issues. The product decision has been made that we don't need concurrent editing in Beta1. Not sure about GA. but I assume we have a lot of other features that were prioritized in for Beta1

If we had more development resources and there weren't other features that were deemed beta blockers I would be all for getting concurrent editing in before beta1. But also if also if we made that decision won't the first order of business be defining the UX for that?

@jessebaker I found the problem that is causing #18

It is because during Publish we delete the auto-save entry, including the hashes and the client instance id, for the items that were published. This is because there is no auto-saved information at this point. Everything matches what should have been just saved. We don't want this to show up "Review X changes"

So when the client sends the auto-saved hashes when find a mismatch, client has auto-saved hashes and the server doesn't. Now we could change the server side mechanism to stored this information separate from the actual auto-save information we will need to publish but that would make the server side more complex. Also it is also possible that this particular entity will never be edited within XB again, so this keeps us from having to keep auto-saved hash information for any entity that was ever edited in XB forever.

So I was going to suggest that when submit a publish request and get response from the server that you delete the auto-save hashes. That would solve the problem in #18 but I did make me think of another problem, that actually exists right now

1. User 1 open xb/xb_page/1/editor, does not make any changes, there is no auto-saved hash because ther are not changes yet
2. User 1 leave their browser open
3. User 2 opens xb/xb_page/1/editor makes some changes
4. User 2 publishes xb_page 1
5. User 1, makes a change in their browser session that they never closed

I think the effect would be that User 1 would make an auto-saved version that would not be aware of the published changes and so would wipe out those changes when it was published again.

We could solve this by not deleting the auto-saved entry the server-side when publishing, and therefore throwing a conflict error for User 1. But again this would require us to keep auto-save hash information forever for all entities, which could start to add up on larger sites. Because we don't know if there is some browser session somewhere that was left open or not.

So I don't think this is a solution.

I think what we need to do is have the client be aware of the starting point version of the entity it is editing.

So for example for a page or node this would be version number, vid or changed property.
Basically the server would return to the client, along with the "autosaves" key, a "verison_id" key of the last saved version on the server. The client would always return this key.

So in the example above User 1's last request would properly get a 409 error because it's version number would be 1 version number behind. But this would also require the client to request /xb/api/v0/layout/xb_page/1 after publishing. Though after #3516517: [META] Selective Publishing and Reverting the client could try to be more selective and not request if the current page was not publish it would probably be safer to always just request it.

There are probably unrelated reasons why the client should always call /xb/api/v0/layout/xb_page/1 after publishing. For example Drupal has hook_entity_presave which allows altering an entity before it is saved. Of course XB will not be aware of any changes made in custom code here, therefore if XB doesn't call /xb/api/v0/layout/xb_page/1 after publishing the auto-save request could wipe out any changes that were made in any hook_entity_presave code

I pushed up 86ce2085 which will return the revision id under "auto" . Probably at least autoSaves: Record<string, string>; will need to updated on the front-end because this now an object.

but it seems to work manually testing

Note I have not had time to fix the phpunit tests but the e2e test passed so that is good sign. Just to need to update the phpunit tests to expect to a nested structure with revision info from the client.

re #24 ok might know what is happening.....

@jessebaker thanks for testing this. I was testing this with nodes and it was working.

Basically entity have a seperate revision id but if we create a new revision on every save this won't increment. I guess Page is not incrementing which I guess makes sense because we don't have a way to revert to old revisions or view old revisions AFIACT

So I used the "changed" time also in the revision for the client

changing title to make clear we are only handling content entities, node and pages with layouts

We will need this too #3530827: For config entities add client support conflict detection via the `autoSaves' key

re-rolled, added some more code comments and updating the summary but not finished. Leaving assigned to myself as I am working on the summary

updated more.... but still more to update

leaving assigned to myself

Change the MR so that \Drupal\experience_builder\Controller\ApiLayoutController::patch only validates the region that contains the component being updated.
I chatted with @jessebaker, @wimleers and @effulgentsia about this. Right now we are just going to conflict if all entity auto-saves don't match because it forces the user refresh the browser and we can then certain other parts of the front-end are in-sync after the refresh

I need to add test to demonstrate how this works

Added test coverage for regions to \Drupal\Tests\experience_builder\Kernel\ApiLayoutControllerPostTest::testOutdatedAutoSave. \Drupal\Tests\experience_builder\Kernel\ApiLayoutControllerPatchTest::test already had a test for a conflict with a region

@wim leers Re: #31 I have update the summary. I have had a chance to update #3512616: Document the reasons for not using Workspaces for saving in XB 1.0.0 but I think this issue can be reviewed. Most of the back-end changes are in \Drupal\experience_builder\Controller\ApiLayoutController::validateAutoSaves. \Drupal\Tests\experience_builder\Kernel\ApiLayoutControllerPostTest::testOutdatedAutoSave is the most explicit test of this. \Drupal\Tests\experience_builder\Kernel\ApiLayoutControllerPatchTest::test() also catches ConflictHttpException which is thrown from validateAutoSaves()

Created follow-up #3532056: For less conflict errors only validate region auto-save on layout PATCH, in that issue I mention we might just want to wait until #3492065: Enable concurrent editing: Replace the postPreview action with atomic equivalents. The back-end work for #3532056 would be simple but I am not sure about the front-end changes that would be needed. We would need to send different info from the client but we might need to reload differently.

Also added a todo in the code

I addressed a couple points in @larowlan's review

Assigning back to @larowlan because I answered his question on the MR

adding blocker tag as this blocks #3530827: For config entities add client support conflict detection via the `autoSaves' key and #3530258: Add rudimentary conflict prevention to the Config Auto-save endpoint

I going to RTBC this based on @bnjmnm MR approval and @larowlan saying the backend changes look good

🎉

re #55 @neha_bawankar wow thanks for the detailed testing report!!!