Make auto-save manager only support passing and returning entity objects, remove support for arbitrary data, use deterministic hashing to ensure entries are actually updates

Reproduced.

Per the diagram in #3505118: The status badge should indicate if there are changes to the page, Changed is shown when an auto-save is created.

And sure enough:

  if (hasAutoSave) {
    return (
      <Badge size="1" variant="solid" color="amber">
        Changed
      </Badge>
    );
  }

… is triggered because merely doing Click on Preview and switch the view 2-3 times between - Full - Tablet - Mobile then Exit preview

… which explains it.

@tedbow: Can you please investigate when/why/how an auto-save entry gets created? 🙏

Not sure why this happens but appears but for some reason when viewing the preview that the auto-save is triggered.

I pushed up a debug branch I can see by retrieving /xb/api/v0/auto-saves/pending the auto-save is made during the preview before the exit.

I can also see in the call to \Drupal\experience_builder\AutoSave\AutoSaveManager::recordInitialClientSideRepresentation which is used to save a comparison to determine if an auto-save should be made `form_build_id` and other form fields are missing. this could be the problem. Have idea to fix it

I think 2 things are going on

1. For some reason just Previewing triggers a request to the Layout patch or post which can trigger an auto-save. Not sure why this is happening but it should not itself trigger an auto-save if nothing has actually changed. I might be an unnecessary call to the server but it should be safe
2. I think that actually any of our production calls to \Drupal\experience_builder\AutoSave\AutoSaveManager::save() from ApiLayoutController, at least for the main entity that has entity_form_fields will always result in a new auto-save entity. Here is why
   • In \Drupal\experience_builder\Controller\ApiLayoutController::get we have the call to record the initial has for the entity.
     

     // Remember the initial client-side representation of this XB-enabled
           // content entity (prior to auto-saves existing), to allow detecting when
           // an auto-save request from the client should actually be stored (i.e.
           // when changes are detected).
           $this->autoSaveManager->recordInitialClientSideRepresentation($entity, [
             'layout' => [$content_layout],
             'model' => self::extractModelForSubtree($content_layout, $model),
             'entity_form_fields' => $entity_form_fields,
           ]);
     

     Here is example result

     {
         "layout": [
             {
                 "nodeType": "region",
                 "id": "content",
                 "name": "Content",
                 "components": []
             }
         ],
         "model": [],
         "entity_form_fields": {
             "langcode[0][value]": "en",
             "revision_log[0][value]": "",
             "title[0][value]": "Untitled page",
             "path[0][alias]": "",
             "path[0][source]": "\/page\/5",
             "path[0][langcode]": "en",
             "image[media_library_selection]": "",
             "description[0][value]": ""
         }
     }
     

     Notice here we don't have form_id or form_* fields

     entity_form_fields is set via $entity_form_fields = $this->getEntityData($entity);

   • Then when you have made a change and auto-save is triggered it called from \Drupal\experience_builder\Controller\ApiLayoutController::buildPreviewRenderable

     If we look at the actual auto-save entity we have

     "xb_page:5:en": {
     "owner": {
     "name": "root",
     "avatar": null,
     "uri": "/user/1",
     "id": 1
     },
     "entity_type": "xb_page",
     "entity_id": "5",
     "data": {
     "layout": [
     {
     "nodeType": "region",
     "id": "content",
     "name": "Content",
     "components": []
     }
     ],
     "model": [],
     "entity_form_fields": {
     "form_build_id": "form-mmx4qbE8iQ2NvDfGMf605hckQT70LUYXRW0_OsCcaMY",
     "langcode[0][value]": "en",
     "revision_log[0][value]": "",
     "title[0][value]": "Untitled page11",
     "path[0][alias]": "/untitled-page11",
     "path[0][source]": "/page/5",
     "path[0][langcode]": "en",
     "image[media_library_selection]": "",
     "description[0][value]": "",
     "form_token": "Qcfk9lkWdJKbHsM87vruI-x-7SguVTEVrUCJODdGIyo",
     "form_id": "xb_page_form",
     "image-media-library-update": "Update widget",
     "image-media-library-open-button": "Add media",
     "advanced__active_tab": ""
     }
     },
     "data_hash": "67a515423ee87311",
     "langcode": "en",
     "label": "Untitled page11",
     "updated": 1749740209
     }
     

     Notice form_build_id is under entity_form_fields

     So because we compare the has made from recordInitialClientSideRepresentation() without the form fields we should always get a new auto-save entry because they will never match.

so the question would be why don't we always get an auto-save entry when there is not changes. I think this is because we normally don't trigger a call to \Drupal\experience_builder\AutoSave\AutoSaveManager::save if there are no actual changes. The call to save() should only add a entry if there are changes.

To confirm this you can do

1. Create a new page
2. Change the title to "Untitled Page1"
3. This results in change to the title and adds a "URL alias"
4. Wait and see "Review 1 changes"
5. Rename the page back to ""Untitled Page" and delete the "URL alias"
6. Wait and see if "Review 1 changes" changes back to "No changes"
7. It will not revert back even though the values have changed back

The auto-save entry is

"xb_page:5:en": {
"owner": {
"name": "root",
"avatar": null,
"uri": "/user/1",
"id": 1
},
"entity_type": "xb_page",
"entity_id": "5",
"data": {
"layout": [
{
"nodeType": "region",
"id": "content",
"name": "Content",
"components": []
}
],
"model": [],
"entity_form_fields": {
"form_build_id": "form-mmx4qbE8iQ2NvDfGMf605hckQT70LUYXRW0_OsCcaMY",
"langcode[0][value]": "en",
"revision_log[0][value]": "",
"title[0][value]": "Untitled page",
"path[0][alias]": "",
"path[0][source]": "/page/5",
"path[0][langcode]": "en",
"image[media_library_selection]": "",
"description[0][value]": "",
"form_token": "Qcfk9lkWdJKbHsM87vruI-x-7SguVTEVrUCJODdGIyo",
"form_id": "xb_page_form",
"image-media-library-update": "Update widget",
"image-media-library-open-button": "Add media",
"advanced__active_tab": ""
}
},
"data_hash": "bd84b24727dbf6d0",
"langcode": "en",
"label": "Untitled page",
"updated": 1749740800
}

The values are back to the "initial state" except now we have form_build_id, form_token etc

hmm. actually I didn't notice \Drupal\experience_builder\AutoSave\AutoSaveManager::generateHash has

// Remove values that only exist for retrieving form state cache during
    // entity conversion and should not influence the hash.
    unset(
      $data['entity_form_fields']['form_build_id'],
      $data['entity_form_fields']['form_id'],
      $data['entity_form_fields']['form_token'],
    );

which I think I added. So it must be some other key that is different. looking....

I think the solution I pushed should fix this for xb_pages, both problem initially reported and the problem I describe in #6 where you change the title of page and then change it back but the "Review 1 change" does not go away

but it does not fix it for articles using the xb_dev_standard module. This is because the article have more properties that are exposed in entity_form_fields. So this just happens to fix it for xb_pages but if we say added the menu property and widget to pages this would break it again.

Maybe the problem is the logic in \Drupal\experience_builder\Controller\ApiLayoutController::getEntityData and \Drupal\experience_builder\ClientDataToEntityConverter::setEntityFields() are too different

Here is a comparison between the data used to make the hashes between the initial and then call to save an updated.

Ah, that's because the form state gets set from input behaviours in the FE which does things like mapping booleans from 0/1 to true/false
Tricky 🤔

I wonder if we can track why we're posting to the layout endpoint - the preview comes back from the get controller now so we shouldn't be making any post requests _until_ we change something.

@larowlan re #10 I agree we shouldn't need to make post requests until something changes but also we should be able to make post requests and not create an auto-save entries if the form represents that same data as the save entity. Extra posts requests should not matter in that regard.

We should be able to change the form to something different to what is saved, and then change the form back to what is saved in the entity and then that should delete auto-save entry because the system detects the match.

The extra post requests seem like a minor problem but the inability to detect when a state matches the saved entity seems critical

re #9
I can't remember if we consider not relying on the form state but instead relying what would be saved if we published. Would it be possible to run \Drupal\experience_builder\ClientDataToEntityConverter::convert() on auto-saved entry, like we do in \Drupal\experience_builder\Controller\ApiAutoSaveController::post() and then compare against the saved entity?

Something like

// Pluck out only the content region.
          $content_region = \array_values(\array_filter($auto_save['data']['layout'], static fn(array $region) => $region['id'] === XbPageVariant::MAIN_CONTENT_REGION));
          $this->clientDataToEntityConverter->convert([
            'layout' => reset($content_region),
            'model' => $auto_save['data']['model'],
            'entity_form_fields' => $auto_save['data']['entity_form_fields'],
          ], $entity);
   // $entity now has been updated by the auto-save state
  $saved_entity = loadUnchanged($entity->id())
$auto_save_entity_hash =  \hash('xxh64', \json_encode($entity->toArray(), JSON_THROW_ON_ERROR));
 $saved_entity_hash =  \hash('xxh64', \json_encode($saved_entity->toArray(), JSON_THROW_ON_ERROR));
// or instead of hashes could we compare all fields using `$original_field->equals($received_field)` like we do in \Drupal\experience_builder\ClientDataToEntityConverter::checkPatchFieldAccess
if ($auto_save_entity_hash !== $saved_entity hash) {
// entity should be considered changed
}

I know we there would be more to it than this and we probably have massage things, but also the form values method also seem to need a lot of massaging and seems really tricky

Just trying to think of alternatives.

re #11 another way I am trying to think about the problem is imaging if we had been able to use Workspaces instead of I current auto-save.

Just considering Content entities., if we had workspaces and were able to make real entity revisions in the workspaces would we have the problem of not being able to detect when the revisions in the workspace are actually the same as the revision in the live site?

Not considering XB, in standard workspaces if you edited the title of node in a workspace from "Original title" to "Updated title" back to "Original title". Not sure?

will update summary too

I created #3530068: When in Preview mode on published page an unneeded layout POST to the back end is made to handle the specific problem. This issue will handle problem of creating a new auto-save entry because the system does not recognize it is the same as the initial state

Updated summary. I changed it to major but I think it is really a product decision as to whether this acceptable for beta1 or 1.0. I definitely want surface it because I don't think we aware this problem still exists.

Unlike the data model, I don't think this likely something we couldn't fix later if the behavior is acceptable for now.

Re #12 and how this kind of problem compares to something like Workspaces that allows staging of content

I don't think Workspaces would detect if you changed an entity in workspace but then changed it back so it matched the original. So you would see "changed" entity that could be published to Live.

I think the difference you would have to submit a form at least once for this to happen, where as XB auto-save on keystrokes, so I think the editor would have much harder time figuring out what happen for unintentional changes where just want to "view" an entity XB. Even if you don't want to change a page if just want to see what the page would look like if say a JS component it uses that has changed, you have to open it up in the editor. There is no other way to just "View"

Because its Ted's weekend I'll try and push this forward a bit

I've started a new branch named 3529622-convert-before-autosave that changes the signatures on autosave to only take the entity, i.e. you have to convert before you ask the autosave manager to save it.
I've also done some work to make hashing of the entity determinate and have APiLayoutControllerPostTest::post passing. I expect there will be a lot of failure and I will check in with @tedbow when I start tomorrow to get a handover to pick it back up again. No pressure to work on this @tedbow

I pushed this a little more forward I hope.

Updating the issue title to reflect the approach.

Adding beta-blocker tag as this blocks #3524298: Auto-save data should update itself when the Fallback plugin is (de)activated and that itself has the tag

Should be down to just failures in ApiConfigAutoSaveControllersTest now, will continue next week

It was a journey, but this is now green and ready for review.

I think this makes our auto-save much more robust and reliable.

It makes it clear that we probably want to do #3505018: Return validation errors from LayoutController::post and ::patch sooner than later too

More accurate issue title

Postponed #3514900: EntityFormController should load auto-save state if it exists on top of this. It is critical because of data-loss, so that makes this critical too.

This blocks #3527244: Add test coverage that ensure fields values the user has view but not edit access to can NOT be saved to AutoSave and will likely allow us to close #3509267: [PP-1] Only auto-save JavaScriptComponent/AssetLibrary config entities when there are actual changes and #3525829: If user with less/different permissions edits a page after a user with more/different permissions data loss could occur.

Closed #3509267 as a duplicate at #3509267-6: [PP-1] Only auto-save JavaScriptComponent/AssetLibrary config entities when there are actual changes.

Crediting @justafish for her clarifying response at https://git.drupalcode.org/project/experience_builder/-/merge_requests/1..., important given the recent turmoil at #3532130: `/xb/api/v0/config/component` crashes when `article` node type does not exist due to hardcoded assumption in `GeneratedFieldExplicitInputUxComponentSourceBase::getClientSideInfo()` 🙏

Exciting to see this approached looks like it is going to work out! ➕1 from me. Going look at the auto-save portions

I think this looks good but I don't we have actual test coverage for the problem in #6, Where you change the title and change it back to original and the system can't detect that current state matches the save entity state. If you look at #8 you can see that form values are different, like in one case "menu" is include and in the other it is not. Since we not rely on the form values to detect if we to save or clear the auto-save this should fix it but in a follow-up it would be good to write an e2e test to prove the current entity would be removed from the "review x changes". You can check the summary where I put "Why this is worse than it seems!" section that details how this could be bad. Also the instructions in the summary under

To confirm that data sent from the client that matches the saved entity will actually result in a auto-save entry you can test:

Shows how to manually test this. So it might be good start for writing a test.

but for this issue I have manually tested the MR and it does fix the problem!

@tedbow in #32: I believe between https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... and the few lines I added at https://git.drupalcode.org/project/experience_builder/-/merge_requests/1..., there is the necessary test coverage already. You're right though that this does not necessarily reflect all the possible such bool-as-int, int-as-string etc scenarios. But I'm not sure an e2e test could ever test all of those scenarios anyway, since we won't be able to test with every possible widget and every possible input for each widget.

I did some tightening of the "collapse/uncollapse" logic because I was worried the duplication of complex logic would make XB overall less reliable/harder to maintain.

In doing so, I uncovered a long-existing bug … which I first tried to fix in here, but it requires careful analysis, so: #3532414: Follow-up for #3500386: tighten `::collapse()` to improve data integrity, because new props added to auto-saved code components cannot have a widget anyway.

This is a HUGE, EPIC MR. It hardens countless things and fixes a number of bugs, some recent, some old. Impressive! 👏

That also means this took >5 hours to review 😅, because since this affects data integrity (due to the introduction of ComponentSourceInterface::preSaveItem() etc.), I wanted to be absolutely certain I understood all those pieces. Which explains #34.

I'm less concerned about auto-save, because that can't affect data integrity — losing auto-saves ain't the end of the world, corrupted component trees on live sites is.

Given this blocks so many things and clearly improves the status quo: RTBC! But I do see a number of things that need either a follow-up issue (or MR here) or that aren't fully clear to me yet:

1. preSaveItem(ComponentTreeItem $item) should be optimizeExplicitInput(array $inputValues) before Component Source plugins become a public API
2. definite regression, but with no user impact today since the UI doesn't yet tie it back to the model it sent
3. possible regression
4. likely simplification
5. Q by Ted
6. another Q by Ted, with a tentative answer by me
7. one tiny bit that I don't quite understand

Follow ups all ready for review

1. #3532485: Change ComponentSourceInterface::preSaveItem($item) to ::optimizeExplicitInput($values)
2. #3532487: ApiAutoSaveController should transpose XB field errors to the model property path
3. #3532490: Add tests to ensure duplicated pages don't retain path alias of original entity
4. #3532495: Simplify ApiAutoSaveController::post for page regions
5. Left an answer and opened #3532496: Assert fields the user can't access aren't sent in ApiLayoutController::get
6. Yep, answer is correct
7. Replied in comment

This passes tests on SQLite & MariaDB, but introduced a regression on MySQL & PostgreSQL. 🫠

HPUnit Test failed to complete; Error: PHPUnit 10.5.47 by Sebastian Bergmann and contributors.
    
    Runtime:       PHP 8.3.22
    Configuration: /builds/project/experience_builder/web/core/phpunit.xml.dist
    
    ...FFF......                                                      12 / 12 (100%)
    
    Time: 05:39.478, Memory: 8.00 MB
    
    Api Layout Controller Post (Drupal\Tests\experience_builder\Kernel\ApiLayoutControllerPost)
     ✔ Entity access required
     ✔ Empty
     ✔ Missing slot
     ✘ 
       ┐
       ├ Failed asserting that actual size 1 matches expected size 0.
       │
       │ /builds/project/experience_builder/tests/src/Kernel/ApiLayoutControllerTestBase.php:98
       │ /builds/project/experience_builder/tests/src/Kernel/ApiLayoutControllerPostTest.php:176
       ┴
     ✘ With global
       ┐
       ├ Failed asserting that false is true.
       │
       │ /builds/project/experience_builder/tests/src/Kernel/ApiLayoutControllerPostTest.php:253
       ┴
     ✘ Without page region permission
       ┐
       ├ Failed asserting that false is true.
       │
       │ /builds/project/experience_builder/tests/src/Kernel/ApiLayoutControllerPostTest.php:307
       ┴
     ✔ With draft code component
     ✔ Image component permutations with 0
     ✔ Image component permutations with 1
     ✔ Image component permutations with 2
     ✔ Image component permutations with 3
     ✔ Invalid form values are returned
    
    FAILURES!
    Tests: 12, Assertions: 219, Failures: 3.

+

---- Drupal\Tests\experience_builder\Kernel\ApiLayoutControllerPatchTest ----
Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Fail      Other      phpunit-19.xml       0 Drupal\Tests\experience_builder\Ker
    PHPUnit Test failed to complete; Error: PHPUnit 10.5.47 by Sebastian Bergmann and contributors.
    
    Runtime:       PHP 8.3.22
    Configuration: /builds/project/experience_builder/web/core/phpunit.xml.dist
    
    ..........FF.                                                     13 / 13 (100%)
    
    Time: 05:53.443, Memory: 10.00 MB
    
    Api Layout Controller Patch (Drupal\Tests\experience_builder\Kernel\ApiLayoutControllerPatch)
     ✔ Entity access required
     ✔ Invalid with no·component·instance·uuid
     ✔ Invalid with no·component·type
     ✔ Invalid with no·model
     ✔ Invalid with No·such·component·in·model
     ✔ Invalid with No·such·component
     ✔ Invalid with No·version·provided
     ✔ Invalid with Invalid·version·provided
     ✔ 
     ✔  with fresh·state,·global
     ✘  with existing·auto-save,·no·global
       ┐
       ├ Failed asserting that actual size 1 matches expected size 0.
       │
       │ /builds/project/experience_builder/tests/src/Kernel/ApiLayoutControllerTestBase.php:98
       │ /builds/project/experience_builder/tests/src/Kernel/ApiLayoutControllerPatchTest.php:206
       ┴
     ✘  with existing·auto-save,·global
       ┐
       ├ Failed asserting that actual size 1 matches expected size 0.
       │
       │ /builds/project/experience_builder/tests/src/Kernel/ApiLayoutControllerTestBase.php:98
       │ /builds/project/experience_builder/tests/src/Kernel/ApiLayoutControllerPatchTest.php:206
       ┴
     ✔ Without page region permission
    
    FAILURES!
    Tests: 13, Assertions: 294, Failures: 2.

— MySQL 8

Same on PostgreSQL 16

#38: merged almost all of those! 🚢🚢🚢🚢 Except #3532495: Simplify ApiAutoSaveController::post for page regions, which contained a bit I didn't understand, so I made adjustments and approved, awaiting you or @penyaskito to +1 or not 🙏

Fixes in new MR

Thanks!