[SOM] Preserve saved object references when saving the object #66584

pgayvallet · 2020-05-14T15:28:36Z

Summary

Fix a bug causing the edition of a saved objects from the management section to removes all its references, resulting in a corrupted object.

elasticmachine · 2020-05-14T15:29:55Z

Pinging @elastic/kibana-platform (Team:Platform)

…erences-corruption

pgayvallet · 2020-05-14T20:12:07Z

src/plugins/saved_objects_management/public/lib/create_field_list.ts

+  // Special handling for references which isn't within "attributes"
+  fields = [...fields, ...createFields('references', object.references)];


This was missed when migrating from legacy.

Ideally, we would create fields with full path instead of assuming all fields are in attributes to avoid that kind of hack (that would break for example if a given editable SO type had a references attributes), but this is outside of the scope of this mere fix.

pgayvallet · 2020-05-14T20:16:30Z

test/functional/apps/saved_objects_management/edit_saved_object.ts

+    return browser.execute(
+      (editor: string, value: string) => {
+        return (window as any).ace.edit(editor).setValue(value);
+      },
+      editorId,


Using the ace API from the browser side instead of performing 'real' user interactions is far from ideal, however as ace autocompletes user input (for example, user type [, ace automatically adds the closing ]), it was just over complicated to use focus + sendKeys, so this will have to be good enough.

Same things with getAceEditorFieldValue: parsing the text content from the ace editor structure is a pain.

pgayvallet · 2020-05-14T20:17:17Z

test/functional/apps/saved_objects_management/edit_saved_object.ts

@@ -99,5 +121,48 @@ export default function({ getPageObjects, getService }: FtrProviderContext) {
      const objects = await PageObjects.settings.getSavedObjectsInTable();
      expect(objects.includes('A Dashboard')).to.be(false);
    });
+
+    it('preserves the object references when saving', async () => {


testing that references are properly displayed and preserved during save, then testing that updating the references actually works.

joshdover

Tested locally and it worked! Few nits / questions on test, but otherwise LGTM

joshdover · 2020-05-14T20:59:33Z

test/functional/apps/saved_objects_management/edit_saved_object.ts

+      expect(JSON.parse(await getAceEditorFieldValue('references'))).to.eql(
+        JSON.parse(referencesValue)
+      );


Are we just parsing to ensure that key order doesn't effect the test outcome? Might be good to comment as such.

Correct assumption. We were getting the key in arbitrary order here.

Unfortunately this check doesn't prevent again this regression. The references text was rendered already as an empty array before this patch, clicking saving and checking the results will not discover if we are facing this issue again.
IMHO I think it's better to check, if possible, the presence of the references array directly on the saved object from ES, or somewhere else on the UI

if possible, the presence of the references array directly on the saved object from ES, or somewhere else on the UI

from ES: Unfortunately this is not possible from an e2e test.

somewhere else on the UI: AFAIK we are not displaying references anywhere expect in the SOM edition page.

As we control the source data for the test however, I can assert that the displayed relations matches the references from the dataset.

dataset/assertion modified in d453272

joshdover · 2020-05-14T21:00:29Z

test/functional/apps/saved_objects_management/edit_saved_object.ts

+      await PageObjects.settings.navigateTo();
+      await PageObjects.settings.clickKibanaSavedObjects();
+
+      let objects = await PageObjects.settings.getSavedObjectsInTable();
+      expect(objects.includes('A Pie')).to.be(true);


These steps don't seem necessary for this test?

This was mostly to insert webdriver implicit timing to wait for the listing page.

I will replace this with just await PageObjects.settings.getSavedObjectsInTable()

joshdover · 2020-05-14T21:01:37Z

test/functional/apps/saved_objects_management/edit_saved_object.ts

+      objects = await PageObjects.settings.getSavedObjectsInTable();
+      expect(objects.includes('A Pie')).to.be(true);


Do we need to do this again?

…erences-corruption

markov00

I've left a comment on the functional test, we should probably revisit it to avoid this regression again in the future

markov00 · 2020-05-15T08:52:06Z

test/functional/apps/saved_objects_management/edit_saved_object.ts

+      expect(JSON.parse(await getAceEditorFieldValue('references'))).to.eql(
+        JSON.parse(referencesValue)
+      );


Unfortunately this check doesn't prevent again this regression. The references text was rendered already as an empty array before this patch, clicking saving and checking the results will not discover if we are facing this issue again.
IMHO I think it's better to check, if possible, the presence of the references array directly on the saved object from ES, or somewhere else on the UI

kibanamachine · 2020-05-15T17:48:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: d453272

History

💚 Build #48225 succeeded c7f6304
💚 Build #48105 succeeded f109ec8
💔 Build #47992 failed dab3b5c

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…c#66584) * create field for references and add comments * add FTR test * remove comments * address comments * use real reference in dataset and assert against it.

…c#66584) * create field for references and add comments * add FTR test * remove comments * address comments * use real reference in dataset and assert against it. # Conflicts: # src/plugins/saved_objects_management/public/lib/create_field_list.test.ts # src/plugins/saved_objects_management/public/lib/create_field_list.ts # src/plugins/saved_objects_management/public/management_section/object_view/components/field.tsx # src/plugins/saved_objects_management/public/management_section/object_view/components/form.tsx

#66843) * create field for references and add comments * add FTR test * remove comments * address comments * use real reference in dataset and assert against it.

#66844) * create field for references and add comments * add FTR test * remove comments * address comments * use real reference in dataset and assert against it.

#66845) * create field for references and add comments * add FTR test * remove comments * address comments * use real reference in dataset and assert against it. # Conflicts: # src/plugins/saved_objects_management/public/lib/create_field_list.test.ts # src/plugins/saved_objects_management/public/lib/create_field_list.ts # src/plugins/saved_objects_management/public/management_section/object_view/components/field.tsx # src/plugins/saved_objects_management/public/management_section/object_view/components/form.tsx

…ine-editor * 'master' of github.com:elastic/kibana: (157 commits) [ML] fix url assertion (#66850) Skip failing lens test(s). #66779 [SOM] Preserve saved object references when saving the object (#66584) Use ES API from start contract (#66157) Reorganize Management apps into Ingest, Data, Alerts and Insights, Security, Kibana, and Stack groups (#65796) [Uptime] Fix flaky navigation to certs page in tests (#66806) [Maps] Do not check count for blended layers when layer is not visible (#66460) [SIEM] Fixes glob patterns from directory changes recently for GraphQL chore(NA): bump static-fs to 1.0.2 (#66775) [Maps] Handle cross cluster index _settings resp (#66797) [SIEM][Lists] Adds 90% of the REST API and client API for exception lists and exception items allow any type for customResponseHeaders config (#66689) [APM] Disable map layout animation (#66763) [ML] Add linking to dataframe from job management tab (#65778) [Maps] Get number of categories from palette (#66454) move oss features registration to KP (#66524) [kbn/plugin-helpers] typescript-ify (#66513) Add kibana-operations as codeowners for .ci/es-snapshots and vars/ (#66746) FTR: move basic services under common folder (#66563) Migrate Beats Management UI to KP (#65791) ... # Conflicts: # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_fields.tsx

* master: [ML] fix url assertion (elastic#66850) Skip failing lens test(s). elastic#66779 [SOM] Preserve saved object references when saving the object (elastic#66584) Use ES API from start contract (elastic#66157) Reorganize Management apps into Ingest, Data, Alerts and Insights, Security, Kibana, and Stack groups (elastic#65796) [Uptime] Fix flaky navigation to certs page in tests (elastic#66806) [Maps] Do not check count for blended layers when layer is not visible (elastic#66460) [SIEM] Fixes glob patterns from directory changes recently for GraphQL chore(NA): bump static-fs to 1.0.2 (elastic#66775) [Maps] Handle cross cluster index _settings resp (elastic#66797) [SIEM][Lists] Adds 90% of the REST API and client API for exception lists and exception items allow any type for customResponseHeaders config (elastic#66689) [APM] Disable map layout animation (elastic#66763) [ML] Add linking to dataframe from job management tab (elastic#65778)

create field for references and add comments

dab3b5c

pgayvallet added v7.7.0 v7.8.0 v7.9.0 v8.0.0 v7.7.1 and removed v7.7.0 labels May 14, 2020

pgayvallet added release_note:fix Feature:Saved Objects Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels May 14, 2020

pgayvallet added the Feature:Kibana Management Feature label for Data Views, Advanced Setting, Saved Object management pages label May 14, 2020

pgayvallet changed the title ~~create field for references and add comments~~ [SOM] Preserve saved object references when saving the object May 14, 2020

pgayvallet added 3 commits May 14, 2020 22:07

add FTR test

bc85d91

Merge remote-tracking branch 'upstream/master' into kbn-66542-SOM-ref…

8fa82c0

…erences-corruption

remove comments

f109ec8

pgayvallet commented May 14, 2020

View reviewed changes

pgayvallet marked this pull request as ready for review May 14, 2020 20:17

pgayvallet requested a review from a team as a code owner May 14, 2020 20:17

pgayvallet removed v7.9.0 v8.0.0 labels May 14, 2020

joshdover approved these changes May 14, 2020

View reviewed changes

pgayvallet added 2 commits May 15, 2020 08:09

address comments

c2f2730

Merge remote-tracking branch 'upstream/master' into kbn-66542-SOM-ref…

c7f6304

…erences-corruption

markov00 reviewed May 15, 2020

View reviewed changes

use real reference in dataset and assert against it.

d453272

pgayvallet added the v7.9.0 label May 16, 2020

pgayvallet merged commit 61936a1 into elastic:master May 16, 2020

pgayvallet mentioned this pull request May 16, 2020

[7.x] [SOM] Preserve saved object references when saving the object (#66584) #66843

Merged

This was referenced May 16, 2020

[7.8] [SOM] Preserve saved object references when saving the object (#66584) #66844

Merged

Editing a saved object from Management UI can corrupt the saved object #66542

Closed

pgayvallet mentioned this pull request May 16, 2020

[7.7] [SOM] Preserve saved object references when saving the object (#66584) #66845

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SOM] Preserve saved object references when saving the object #66584

[SOM] Preserve saved object references when saving the object #66584

pgayvallet commented May 14, 2020

elasticmachine commented May 14, 2020

pgayvallet May 14, 2020

pgayvallet May 14, 2020

pgayvallet May 14, 2020

joshdover left a comment

joshdover May 14, 2020

pgayvallet May 15, 2020

markov00 May 15, 2020

pgayvallet May 15, 2020

pgayvallet May 15, 2020

joshdover May 14, 2020

pgayvallet May 15, 2020

joshdover May 14, 2020

markov00 left a comment

markov00 May 15, 2020

kibanamachine commented May 15, 2020

		// Special handling for references which isn't within "attributes"
		fields = [...fields, ...createFields('references', object.references)];

		objects = await PageObjects.settings.getSavedObjectsInTable();
		expect(objects.includes('A Pie')).to.be(true);

[SOM] Preserve saved object references when saving the object #66584

[SOM] Preserve saved object references when saving the object #66584

Conversation

pgayvallet commented May 14, 2020

Summary

elasticmachine commented May 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshdover left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markov00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented May 15, 2020

💚 Build Succeeded

History