Move search source parsing and serializing to data by flash1293 · Pull Request #59919 · elastic/kibana

flash1293 · 2020-03-11T15:51:22Z

This PR is moving the logic taking care of serializing and deserializing a search source from the saved_object plugin to the data_plugin. The current infrastructure using the SavedObject class stays in place, but this move makes it possible to progressively move away from the class based approach to just calling the utility functions when necessary.

TODOs:

Check whether removed safety checks are actually safe to remove (especially for legacy import)
Add tests
Finalize API exposed by data plugin

Changes:

The clearSavedIndexPattern option was removed because it seems like it's completely unused
The legacy parseSearchSource function would initialize a search source and start parsing, but in some cases throwing an error and leaving a half-initialized search source object not complying to the typescript type (index being a string instead of an actual index pattern object). The helper introduced by this PR only returns valid search source instances, but because of that the legacy import had to be changed to not rely on the inconsistent search source object to do the conflict resolution. I chose this route because I didn't want to introduce functionality specifically for conflict resolution in a general helper like parseSearchSource and the legacy import will be deprecated with 8.0 IIRC
parseSearchSource is also doing the job of hydrateIndexPattern now - this is part of the previous point.
In terms of exposing the functionality, I followed @lukeelmers advice and exposed a pre-wired version of the parsee helper via contract as well as a static export.

flash1293 · 2020-03-11T17:42:03Z

src/plugins/data/public/index.ts

  SearchStrategyProvider,
  ISearchSource,
  SearchSource,
+  parseSearchSource,


@lukeelmers This PR is not ready for review from the app arch team yet, but before cleaning up the implementation I want to make sure the approach is sound. The two utility functions added to the data plugin are:

async function parseSearchSource( searchSourceJson: string, references: SavedObjectReference[], indexPatterns: IndexPatternsContract ): SearchSource; function serializeSearchSource(searchSource: ISearchSource): { searchSourceJSON: string, references: SavedObjectReference[] };

Should these be exposed via contract instead (especially because parsing the search source requires the index patterns service, so we could wire that up within the data plugin instead of passing it in from the consumer)?

IMO either approach works, though I do agree it would be nice to provide a version that already works with index patterns.

This is a bit subjective, but in general I think we often sacrifice a clean contract by statically exporting so many things, so I’m personally not opposed to putting both in the lifecycle contract (even the static one). If things change in the future, it is easier to move something from stateful > static than it is to go from static > stateful.

Plus, from a DX perspective it feels like this would be nice living alongside the other SearchSource stuff in the lifecycle contract

That said, another way we’ve addressed these scenarios is to build it to be stateless/static, but then offer a “prewired” version in the lifecycle contract. (So basically combining both approaches). In the rare case that someone prefers the static version which requires additional config, they can use it.

i prefer having it on the contract but i agree with luke that both approaches work

flash1293 · 2020-03-23T16:09:00Z

I will park this PR for now to focus on moving things over, will revisit in the next minor.

stacey-gammon · 2020-03-27T02:04:55Z

Why are you abandoning this PR @flash1293? I was looking for the same kind of functionality. Is this a bit rabbit hold to go down? Or it was parked from changing priorities? What do you think the effort would be to see it through?

flash1293 · 2020-03-27T08:38:19Z

@stacey-gammon this was just about changed priorities - @ppisljar already mentioned this would unblock work of the app arch team, I will work on this today and hopefully get it in a mergeable state.

…urce-serialization

elasticmachine · 2020-03-27T15:38:36Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

flash1293 · 2020-03-27T17:23:28Z

src/plugins/saved_objects/public/saved_object/helpers/hydrate_index_pattern.ts

  indexPatterns: IndexPatternsContract,
  config: SavedObjectConfig
 ) {
-  const clearSavedIndexPattern = !!config.clearSavedIndexPattern;


I couldn't find any reference to this functionality, seems like this is safe to remove

…urce-serialization

ppisljar

code LGTM

ppisljar · 2020-04-06T10:37:37Z

src/plugins/data/public/search/search_service.ts

        // TODO: should an intercepror have a destroy method?
        this.searchInterceptor = searchInterceptor;
      },
+      parseSearchSource: parseSearchSource(indexPatterns),


nit: what about calling this createSearchSource on the contract ?

nreese

maps changes lgtm
code review

peteharverson

Edit to transform plugin LGTM

cjcenizal

It looks like ES UI got pinged because of the change to Transforms. We don't maintain this so I'm just approving to unblock the PR.

…urce-serialization

flash1293 · 2020-04-07T14:00:16Z

@elasticmachine merge upstream

flash1293 · 2020-04-07T14:08:37Z

Ping @pgayvallet - this is touching the legacy saved object import, IIRC this falls under platforms umbrella. Would you mind taking a look?

pgayvallet · 2020-04-07T15:20:27Z

@flash1293 ack, will take a look

kertal · 2020-04-08T06:13:45Z

will review this today

pgayvallet

LGTM for (futur) SavedObjectsManagement plugin changes.

Linking to #61700, as the changes in core_plugins/kibana/public/management/sections/objects/...will need to be ported to it once this PR is merged.

pgayvallet · 2020-04-07T15:29:35Z

src/plugins/saved_objects/public/types.ts

 export interface SavedObjectKibanaServices {
  savedObjectsClient: SavedObjectsClientContract;
  indexPatterns: IndexPatternsContract;
+  search: DataPublicPluginStart['search'];


NIT: use ISearchStart instead.

pgayvallet · 2020-04-08T08:08:39Z

src/legacy/core_plugins/kibana/public/management/sections/objects/lib/resolve_saved_objects.ts

-    ...doc,
+    ...cloneDeep(doc),


Was this necessary due to a change in this PR, or is it just a fix/cleanup?

This was necessary because of this PR - as applyESResp now errors out without creating an inconsistent search source instance (as in the old implementation), https://github.com/elastic/kibana/blob/master/src/plugins/saved_objects/public/saved_object/helpers/apply_es_resp.ts#L44 becomes a problem because this information is now lost for https://github.com/elastic/kibana/pull/59919/files/babb4d6ee332d3836ba81e213073a46b986dc913#diff-c5230517ea9644f8395ec776249ed4bbR174

I'm also fine with moving the clone into applyESResp

probably fine to keep it there

pgayvallet · 2020-04-08T08:09:44Z

src/plugins/saved_objects/public/saved_object/helpers/apply_es_resp.ts

  savedObject: SavedObject,
-  config: SavedObjectConfig
+  config: SavedObjectConfig,
+  createSearchSource: DataPublicPluginStart['search']['createSearchSource']


NIT: ISearchStart['createSearchSource']

Tried to do this initially, but ISearchStart is not exported from the data plugin.

@elastic/kibana-app-arch ^ any reason for that?

kertal

Code LGTM 👍 , tested locally in Chrome 80, OsX Mojave, by working with Saved Objects in Discover / Visualize / Dashboard. Everything works as expected.

src/plugins/saved_objects/public/types.ts

…urce-serialization

…h1293/kibana into migrate-search-source-serialization

kibanamachine · 2020-04-09T09:21:36Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 8ec0021

History

💚 Build #39251 succeeded babb4d6
💔 Build #39219 failed 8a5fc1b
💚 Build #38753 succeeded 732e990
💔 Build #37048 failed 40aeded
💔 Build #37013 failed aa57ebc

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

…chore/put-all-xjson-together * 'master' of github.com:elastic/kibana: [EPM] Update UI copy to use `integration` (elastic#63077) [NP] Inline buildPointSeriesData and buildHierarchicalData dependencies (elastic#61575) [Maps] create NOT EXISTS filter for tooltip property with no value (elastic#62849) [Endpoint] Add link to Logs UI to the Host Details view (elastic#62852) [UI COPY] Fixes typo in max_shingle_size for search_as_you_type (elastic#63071) [APM] docs: add alerting examples for APM (elastic#62864) [EPM] Change PACKAGES_SAVED_OBJECT_TYPE id (elastic#62818) docs: fix rendering of bulleted list (elastic#62855) Exposed AddMessageVariables as separate component (elastic#63007) Add Data - Adding cloud reset password link to cloud instructions (elastic#62835) [ML] DF Analytics: update memory estimate after adding exclude fields (elastic#62850) [Table Vis] Fix visualization overflow (elastic#62630) [Endpoint][EPM] Endpoint depending on ingest manager to initialize (elastic#62871) [Remote clusters] Fix flaky jest tests (elastic#58768) [Discover] Hide time picker when an indexpattern without timefield is selected (elastic#62134) Move search source parsing and serializing to data (elastic#59919) [ML] Functional tests - stabilize typing in mml input (elastic#63091) [data.search.aggs]: Clean up TimeBuckets implementation (elastic#62123) [ML] Functional transform tests - stabilize source selection (elastic#63087) add embed flag to saved object url as well (elastic#62926) # Conflicts: # x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/es_index.tsx

move search source parsing and serializing to data

56edb43

flash1293 added release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0 labels Mar 11, 2020

only initialize search source if it is enabled in config

01c1ca7

flash1293 commented Mar 11, 2020

View reviewed changes

make functions state and add tests for parsing

37cd60e

flash1293 removed the v7.7.0 label Mar 23, 2020

Merge remote-tracking branch 'upstream/master' into migrate-search-so…

4490ae6

…urce-serialization

botelastic bot added the Team:AppArch label Mar 27, 2020

flash1293 added 2 commits March 27, 2020 17:33

fix referenecs

4f9cc2e

add tests and fix bugs

72f969d

flash1293 commented Mar 27, 2020

View reviewed changes

flash1293 added 8 commits March 29, 2020 11:02

Merge remote-tracking branch 'upstream/master' into migrate-search-so…

2079c32

…urce-serialization

fix some problems

e07546a

fix legacy imporrts

aa57ebc

Merge remote-tracking branch 'upstream/master' into migrate-search-so…

d4e69ee

…urce-serialization

finalize

3360b12

move serialize into search source

40aeded

Merge remote-tracking branch 'upstream/master' into migrate-search-so…

37b8510

…urce-serialization

fix tests

732e990

flash1293 marked this pull request as ready for review April 6, 2020 10:09

flash1293 requested a review from a team as a code owner April 6, 2020 10:09

flash1293 requested a review from a team April 6, 2020 10:09

flash1293 requested review from a team as code owners April 6, 2020 10:09

flash1293 requested a review from a team as a code owner April 6, 2020 10:09

flash1293 added v7.8.0 Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// Feature:Saved Objects labels Apr 6, 2020

ppisljar approved these changes Apr 6, 2020

View reviewed changes

nreese approved these changes Apr 6, 2020

View reviewed changes

peteharverson approved these changes Apr 6, 2020

View reviewed changes

cjcenizal approved these changes Apr 6, 2020

View reviewed changes

flash1293 added 2 commits April 7, 2020 11:42

Merge remote-tracking branch 'upstream/master' into migrate-search-so…

b200ae1

…urce-serialization

rename parseSearchSource to createSearchSource

8a5fc1b

Merge branch 'master' into migrate-search-source-serialization

babb4d6

pgayvallet approved these changes Apr 8, 2020

View reviewed changes

kertal approved these changes Apr 8, 2020

View reviewed changes

src/plugins/saved_objects/public/types.ts Show resolved Hide resolved

flash1293 added 3 commits April 9, 2020 09:32

Merge remote-tracking branch 'upstream/master' into migrate-search-so…

a1a9fd4

…urce-serialization

remove unused property from type

40a4e97

Merge branch 'migrate-search-source-serialization' of github.com:flas…

8ec0021

…h1293/kibana into migrate-search-source-serialization

flash1293 merged commit 8d21b6b into elastic:master Apr 9, 2020

flash1293 deleted the migrate-search-source-serialization branch April 9, 2020 12:06

flash1293 added a commit to flash1293/kibana that referenced this pull request Apr 9, 2020

Move search source parsing and serializing to data (elastic#59919)

f63a678

flash1293 mentioned this pull request Apr 9, 2020

[7.x] Move search source parsing and serializing to data (#59919) #63104

Merged

flash1293 added a commit that referenced this pull request Apr 9, 2020

Move search source parsing and serializing to data (#59919) (#63104)

3078bfa

Conversation

flash1293 commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flash1293 commented Mar 23, 2020

Uh oh!

stacey-gammon commented Mar 27, 2020

Uh oh!

flash1293 commented Mar 27, 2020

Uh oh!

elasticmachine commented Mar 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ppisljar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nreese left a comment

Choose a reason for hiding this comment

Uh oh!

peteharverson left a comment

Choose a reason for hiding this comment

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

flash1293 commented Apr 7, 2020

Uh oh!

flash1293 commented Apr 7, 2020

Uh oh!

pgayvallet commented Apr 7, 2020

Uh oh!

kertal commented Apr 8, 2020

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet Apr 8, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kertal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibanamachine commented Apr 9, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

flash1293 commented Mar 11, 2020 •

edited

Loading

pgayvallet Apr 8, 2020 •

edited

Loading