[Discover] Removing SavedObject usage for savedSearch by alexwizp · Pull Request #112983 · elastic/kibana

alexwizp · 2021-09-23T14:26:38Z

Get rid of savedSearch SavedObject in Discover. This work is done because the SavedObject class has been deprecated and will be removed in Kibana 8

To avoid the PR affecting many files, I suggest breaking it down into several parts:

Step 1 (7.16 and 8.0):
The goal of this step is to get rid of legacy dependencies in all external plugins. What should be done:
- 🏁 Mark all code related to savedSearch SavedObject as deprecated and move into legacy folder.
- 🏁 Create a simple function for getting savedSearch and use it from all external plugins instead of SavedObjectLoader;
- 🏁 remove lastSavedTitle from SavedSearch
- 🏁 savedObject.get should be replaced to savedObject.resolve
Step 2 (7.16 and 8.0):
- 🏁 Discover works with ISearchSource, SearchSource type should be replaced to ISearchSource
- 🏁 Remove getSavedSearchById and getSavedSearchUrlById from the DiscoverServices API;
- 🏁 Refactor Discover to work with savedSearch'es without SavedObject.
Step 3 (7.16 and 8.0):
- 🏁 Update client-side code to correctly handle the three different resolve() outcomes. See https://www.elastic.co/guide/en/kibana/master/sharing-saved-objects.html#sharing-saved-objects-step-3
- 🏁 Update Discover to correctly handle cases on Index Pattern conflicts.
- 🏁 Add unit tests for new code
Step 4 (only for 8.0): 🏃
- completely remove SavedObjectLoader

Testing notes:

This PR contains changes with replacing get() to resolve() for support of Sharing Saved Objects. To test it follow the following guide #107099 (comment)

Screens:

SavedSearchURLConflictCallout:

Redirect:

Conflict index pattern:

Transform plugin:

alexwizp · 2021-09-24T15:28:01Z

@elasticmachine merge upstream

alexwizp · 2021-09-27T15:43:42Z

@elasticmachine merge upstream

jportner · 2021-10-10T05:44:49Z

src/plugins/discover/public/saved_searches/saved_searches_utils.ts

+export const getSavedSearchUrlConflictMessage = async (savedSearch: SavedSearch) =>
+  i18n.translate('discover.savedSearchEmbeddable.legacyURLConflict.errorMessage', {
+    defaultMessage: `This search has the same URL as a legacy alias. Disable the alias to resolve this error : {json}`,
+    values: {
+      json: savedSearch.sharingSavedObjectProps?.errorJSON,
+    },
+  });
+
+export const throwErrorOnSavedSearchUrlConflict = async (savedSearch: SavedSearch) => {
+  if (savedSearch.sharingSavedObjectProps?.errorJSON) {
+    throw new Error(await getSavedSearchUrlConflictMessage(savedSearch));
+  }
+};


The Spaces plugin now provides a component for this embeddable error. Whether you want to change it in this PR or leave it for a follow-on PR, I just wanted to leave a note and let you know about it.

You can load this component with spaces.ui.components.getEmbeddableLegacyUrlConflict; you can see how Lens consumes it here:

kibana/x-pack/plugins/lens/public/embeddable/embeddable.tsx

Lines 283 to 296 in 961fe75

if (sharingSavedObjectProps?.outcome === 'conflict' && this.deps.spaces) {

const conflictError = {

shortMessage: i18n.translate('xpack.lens.embeddable.legacyURLConflict.shortMessage', {

defaultMessage: `You've encountered a URL conflict`,

}),

longMessage: (

<this.deps.spaces.ui.components.getEmbeddableLegacyUrlConflict

targetType={DOC_TYPE}

sourceId={sharingSavedObjectProps.sourceId!}

/>

),

};

this.errors = this.errors ? [...this.errors, conflictError] : [conflictError];

}

The sourceId is the ID of the saved object.

@jportner yep... we saw your PR, but unfortunately right now we cannot do it like in Lens. it's a blocker isssue #114305 for that. I think we can create a follow up PR and fix that in future.

alexwizp · 2021-10-11T07:02:16Z

@elasticmachine merge upstream

flash1293

Found two minor issues while re-testing. The rest looks good to me.

When trying to save a search "as new" (switch in the modal) and pick a name which exists already, the warning about duplicate names is shown, but pressing the button a second time doesn't save the search under this name

When accessing discover with a conflicting index pattern, the error page is rendered. But when loading a saved search via deeplink, it's stuck at the loading spinner:

…ject-loader

kertal

Code LGTM 👍 ... code review part 1, will continue later on testing

kertal · 2021-10-11T11:26:32Z

src/plugins/discover/public/application/apps/main/discover_main_route.tsx

+const DiscoverError = ({ error }: { error: Error }) => (
+  <EuiEmptyPrompt
+    iconType="alert"
+    iconColor="danger"
+    title={
+      <h2>
+        {i18n.translate('discover.discoverError.title', {
+          defaultMessage: 'Error loading Discover',
+        })}
+      </h2>
+    }
+    body={<p>{error.message}</p>}
+  />
+);
+


Nice improvement 👍

walterra

Transform code LGTM, also did some local testing.

flash1293 · 2021-10-11T12:38:13Z

@elastic/kibana-reporting-services could we get a review on this?

VladLasitsa · 2021-10-11T16:03:26Z

Found two minor issues while re-testing. The rest looks good to me.

When trying to save a search "as new" (switch in the modal) and pick a name which exists already, the warning about duplicate names is shown, but pressing the button a second time doesn't save the search under this name

When accessing discover with a conflicting index pattern, the error page is rendered. But when loading a saved search via deeplink, it's stuck at the loading spinner:

@flash1293 I think I fixed these issues.

src/plugins/discover/public/saved_searches/saved_search_url_conflict_callout.ts

flash1293

Comments got addressed, works fine for me 👍

VladLasitsa · 2021-10-12T08:38:38Z

@elasticmachine merge upstream

kertal

Is there a way we could improve showing a conflicting saved search? Because currently users need to navigate to a different application, return to Discover, to have it removed. This is confusing, because first you'll try to reload the page, but the last link of Discover is still in session, which means, still the conflict screen is displayed. So I think showing the default view with the default index pattern and showing an error message about the conflicting saved object would be a better experience in this case.

flash1293 · 2021-10-12T09:43:57Z

Is there a way we could improve showing a conflicting saved search? Because currently users need to navigate to a different application, return to Discover, to have it removed

This should not be the case, a conflicted saved search loads just fine (and shows a warning callout above the app). I'm guessing you are talking about the case with a conflicted index pattern (if you follow Joe Portners script with an export containing both index pattern and saved search, it will actually put both into a conflicting state). We discussed this with Joe and the current guidance is to fail on conflicts on "secondary saved objects" (SOs managed by some other part of Kibana, but used in the current view) - this is also how dashboard behaves (even though it's just per panel there). I don't have a strong opinion on this - @jportner what do you think?

kertal · 2021-10-12T09:50:06Z

Is there a way we could improve showing a conflicting saved search? Because currently users need to navigate to a different application, return to Discover, to have it removed

This should not be the case, a conflicted saved search loads just fine (and shows a warning callout above the app). I'm guessing you are talking about the case with a conflicted index pattern (if you follow Joe Portners script with an export containing both index pattern and saved search, it will actually put both into a conflicting state).

yes, exactly that's what I did. fine with failing here, just to remove the failure in this special case could be improved

flash1293 · 2021-10-12T09:58:28Z

We do the "show toast and go to default view of the app" in some other error cases (e.g. Visualize). Could be done here as well, but we would have to make sure to not enter a loop (what if the default index pattern is conflicted?)

kertal · 2021-10-12T10:46:49Z

(what if the default index pattern is conflicted?)

yes, this could be the case. Is there a way to remove or clean the last discover URL in the session in case there's an conflicting error?

jportner · 2021-10-12T12:03:27Z

We discussed this with Joe and the current guidance is to fail on conflicts on "secondary saved objects" (SOs managed by some other part of Kibana, but used in the current view) - this is also how dashboard behaves (even though it's just per panel there). I don't have a strong opinion on this - @jportner what do you think?

Yeah, for the most consistent user experience we should do this. FWIW, a more recent development is #113335, in which we will add validation before objects are created to prevent legacy URL alias conflict scenarios from happening in the first place.

We hope no users will ever run into this in the real world, but we have this error handling out of an abundance of caution. I wouldn't worry too much about making sure this screen has a really nice workflow to help users fix the issue. If this does happen, users will need to disable the legacy URL alias using our API. With #114172 that was recently merged, the LegacyUrlConflict and EmbeddableLegacyUrlConflict components now point to the docs where users can learn more about that.

VladLasitsa

LGTM, tested locally.

…ct-loader # Conflicts: # src/plugins/discover/public/application/apps/context/services/context.ts

kibanamachine · 2021-10-12T21:46:58Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`discover`	394	399	+5

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`discover`	56	77	+21

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`discover`	327.7KB	328.5KB	+770.0B
`transform`	322.1KB	322.3KB	+183.0B
`visDefaultEditor`	152.6KB	152.7KB	+33.0B
`visualizations`	72.7KB	72.8KB	+134.0B
`visualize`	53.1KB	53.1KB	+36.0B
total			+1.1KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`discover`	6	7	+1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`discover`	19.6KB	23.6KB	+4.0KB
`reporting`	40.9KB	40.9KB	+4.0B
`visDefaultEditor`	19.2KB	19.3KB	+138.0B
`visualizations`	39.0KB	38.9KB	-107.0B
total			+4.0KB

Unknown metric groups

API count

id	before	after	diff
`discover`	82	103	+21

References to deprecated APIs

id	before	after	diff
`discover`	1821	1818	-3
`visDefaultEditor`	223	213	-10
`visualize`	99	86	-13
total			-26

History

💚 Build #159844 succeeded 4814a0a
💔 Build #159643 failed 5bf1a0e
💔 Build #159593 failed ff973c5
💚 Build #159336 succeeded 54a9508
💚 Build #159232 succeeded 150a2ca

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

cc @alexwizp

kertal

LGTM, tested and works as expected ... talked with @alexwizp and decided to move forward here and solve the problem with the special case of conflicts in saves search and index pattern in a follow up PR.

alexwizp force-pushed the discover-saved-object-loader branch 2 times, most recently from 7a5fe7f to 7000224 Compare September 24, 2021 10:24

alexwizp requested a review from VladLasitsa September 24, 2021 10:39

alexwizp force-pushed the discover-saved-object-loader branch 3 times, most recently from 2af74a6 to bb32a6a Compare September 24, 2021 14:20

alexwizp changed the title ~~[Discover] Step 1 - remove SavedObjectLoader~~ [Discover] Removing SavedObject usage for savedSearch Sep 27, 2021

alexwizp force-pushed the discover-saved-object-loader branch 4 times, most recently from ab44b21 to 61d82f7 Compare September 29, 2021 10:58

elastic deleted a comment from kibanamachine Sep 29, 2021

alexwizp force-pushed the discover-saved-object-loader branch 3 times, most recently from 269ae6b to 5a6e2f0 Compare September 29, 2021 16:43

elastic deleted a comment from kibanamachine Sep 29, 2021

alexwizp force-pushed the discover-saved-object-loader branch from 5a6e2f0 to 479c5a9 Compare September 29, 2021 18:31

elastic deleted a comment from kibanamachine Sep 29, 2021

alexwizp requested a review from kertal September 30, 2021 08:56

alexwizp force-pushed the discover-saved-object-loader branch from 479c5a9 to 702607e Compare September 30, 2021 15:27

alexwizp self-assigned this Sep 30, 2021

alexwizp added v7.16.0 v8.0.0 Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// release_note:enhancement labels Sep 30, 2021

alexwizp force-pushed the discover-saved-object-loader branch 2 times, most recently from 0aa4101 to 9ce16e6 Compare September 30, 2021 19:13

Merge branch 'master' into discover-saved-object-loader

150a2ca

jportner reviewed Oct 10, 2021

View reviewed changes

jportner mentioned this pull request Oct 10, 2021

Legacy url conflict UI improvements #114172

Merged

Merge branch 'master' into discover-saved-object-loader

54a9508

flash1293 reviewed Oct 11, 2021

View reviewed changes

vadimkibana approved these changes Oct 11, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into discover-saved-ob…

a86d821

…ject-loader

kertal reviewed Oct 11, 2021

View reviewed changes

walterra approved these changes Oct 11, 2021

View reviewed changes

vadimkibana approved these changes Oct 11, 2021

View reviewed changes

Fix comments

ff973c5

kertal reviewed Oct 11, 2021

View reviewed changes

src/plugins/discover/public/saved_searches/saved_search_url_conflict_callout.ts Outdated Show resolved Hide resolved

Fix lint

5bf1a0e

flash1293 approved these changes Oct 12, 2021

View reviewed changes

Merge branch 'master' into discover-saved-object-loader

4814a0a

kertal reviewed Oct 12, 2021

View reviewed changes

VladLasitsa approved these changes Oct 12, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/main' into discover-saved-obje…

76ee791

…ct-loader # Conflicts: # src/plugins/discover/public/application/apps/context/services/context.ts

kertal approved these changes Oct 13, 2021

View reviewed changes

alexwizp mentioned this pull request Oct 13, 2021

[7.x] [Discover] Removing SavedObject usage for savedSearch (#112983) #114767

Merged

	if (sharingSavedObjectProps?.outcome === 'conflict' && this.deps.spaces) {
	const conflictError = {
	shortMessage: i18n.translate('xpack.lens.embeddable.legacyURLConflict.shortMessage', {
	defaultMessage: `You've encountered a URL conflict`,
	}),
	longMessage: (
	<this.deps.spaces.ui.components.getEmbeddableLegacyUrlConflict
	targetType={DOC_TYPE}
	sourceId={sharingSavedObjectProps.sourceId!}
	/>
	),
	};
	this.errors = this.errors ? [...this.errors, conflictError] : [conflictError];
	}

Conversation

alexwizp commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing notes:

Screens:

Uh oh!

alexwizp commented Sep 24, 2021

Uh oh!

alexwizp commented Sep 27, 2021

Uh oh!

jportner Oct 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexwizp Oct 10, 2021

Choose a reason for hiding this comment

Uh oh!

alexwizp commented Oct 11, 2021

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

kertal left a comment

Choose a reason for hiding this comment

Uh oh!

kertal Oct 11, 2021

Choose a reason for hiding this comment

Uh oh!

walterra left a comment

Choose a reason for hiding this comment

Uh oh!

flash1293 commented Oct 11, 2021

Uh oh!

VladLasitsa commented Oct 11, 2021

Uh oh!

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

VladLasitsa commented Oct 12, 2021

Uh oh!

kertal left a comment

Choose a reason for hiding this comment

Uh oh!

flash1293 commented Oct 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kertal commented Oct 12, 2021

Uh oh!

flash1293 commented Oct 12, 2021

Uh oh!

kertal commented Oct 12, 2021

Uh oh!

jportner commented Oct 12, 2021

Uh oh!

VladLasitsa left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Oct 12, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

References to deprecated APIs

History

Uh oh!

kertal left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

alexwizp commented Sep 23, 2021 •

edited

Loading

jportner Oct 10, 2021 •

edited

Loading

flash1293 commented Oct 12, 2021 •

edited

Loading