[Discover] Removing SavedObject usage for savedSearch

[alexwizp]

Closes: #105810

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]

@elasticmachine merge upstream

[alexwizp]

@elasticmachine merge upstream

[flash1293]

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

[VladLasitsa]

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.

[flash1293]

Comments got addressed, works fine for me 👍

[VladLasitsa]

@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]

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]

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]

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]

(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]

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.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 76ee791
• Storybooks Preview
• Documentation Changes

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.