making vis (completely) serializable #64207

ppisljar · 2020-04-22T15:40:00Z

Summary

Summarize your PR. If it involves visual changes include a screenshot or gif.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-04-22T15:40:03Z

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

lukeelmers

Couple of questions, otherwise this makes sense to me so far

lukeelmers · 2020-04-23T02:14:15Z

src/legacy/ui/public/new_platform/__mocks__/helpers.ts

@@ -48,6 +49,7 @@ export const pluginsMock = {
    visualizations: visualizationsPluginMock.createSetupContract(),
    kibanaLegacy: kibanaLegacyPluginMock.createSetupContract(),
    savedObjectsManagement: savedObjectsManagementPluginMock.createSetupContract(),
+    discover: discoverPluginMock.createSetupContract(),


You will likely need to add mocks to the karma mocks file as well, since it calls setSetupServices and setStartService expecting the same arguments. This is probably contributing to some of the build failures.

lukeelmers · 2020-04-23T02:43:24Z

src/plugins/visualizations/public/saved_visualizations/_saved_vis.ts

 import { ISavedVis, SerializedVis } from '../types';
-import { createSavedSearchesLoader } from '../../../../plugins/discover/public';
-import { getChrome, getOverlays, getIndexPatterns, getSavedObjects, getSearch } from '../services';

 export const convertToSerializedVis = async (savedVis: ISavedVis): Promise<SerializedVis> => {


I know we talked about this a little earlier, but I wonder if this would help you prevent using the bang ! for searchSourceFields below?

Suggested change

export const convertToSerializedVis = async (savedVis: ISavedVis): Promise<SerializedVis> => {

export const convertToSerializedVis = async (savedVis: Required<ISavedVis>): Promise<SerializedVis> => {

Because really the value taken in to convertToSerializedVis isn't ISavedVis, it is the return type of convertFromSerializedVis, in which all the fields should be present.

Oh except ISavedVis has savedSearchRefName, which is not returned by convertFrom SerializedVis...

lukeelmers · 2020-04-23T02:48:03Z

src/plugins/visualizations/public/vis.ts

+
+    searchSource.setParent(savedSearch.searchSource);
+  }
+  searchSource!.setField('size', 0);


Is ! needed here? Because it doesn't appear to be needed in line 78.

lukeelmers · 2020-04-23T02:50:03Z

src/plugins/visualizations/public/vis.ts

+interface PartialVisState extends Omit<SerializedVis, 'data'> {
+  data: Partial<SerializedVisData>;
+}


nit: This could be simplified with the Assign utility, and you wouldn't have to do the weird extends Omit<...> thing:

import { Assign } from '@kbn/utility-types'; type PartialVisState = Assign<SerializedVis, { data: Partial<SerializedVisData> }>;

It's basically Object.assign() for types.

Also, are you sure data shouldn't be optional here? It appears we set a default (lines 96-98). And from what I understand data won't really ever be "populated" with info until setState is called

lukeelmers · 2020-04-23T02:58:56Z

src/plugins/visualizations/public/vis.ts

    if (state.params || typeChanged) {
-      this.params = this.getParams(state.params);
+      this.params = this.getParams(state.params!);


If we are saying we know for sure that params exists (by asserting state.params!), then why do we have a conditional at all on line 134?

If state.params is always truthy, then typeChanged will never be evaluated, and the block will always execute.

So at that point what is typeChanged actually doing?

lukeelmers · 2020-04-23T03:23:30Z

src/plugins/visualizations/public/vis.ts

+    const { data, ...restOfSerialized } = this.serialize();
+    const vis = new Vis(this.type.name, restOfSerialized as any);
+    vis.setState(restOfSerialized as any);
+    const aggs = this.data.indexPattern
+      ? getAggs().createAggConfigs(this.data.indexPattern, data.aggs)
+      : undefined;
+    vis.data = {
+      ...this.data,
+      aggs,
+    };
+    return vis;


I'm wondering if this could be simplified. Since you are relying on mutating vis.data after the fact anyway, why do you need to separate it from restOfSerialized?

Would this do the same thing?

const serializedVis = this.serialize(); const vis = new Vis(this.type.name, serializedVis); vis.setState(serializedVis); vis.data.aggs = this.data.indexPattern ? getAggs().createAggConfigs(this.data.indexPattern, serializedVis.data.aggs) : undefined; return vis;

no, to keep clone method sync we must not call setState without data property set.
we work around loading index pattern and linked search by keeping the same references.

pgayvallet

LGTM for SOM plugin changes

pgayvallet · 2020-04-23T08:10:23Z

src/plugins/saved_objects_management/public/register_services.ts

-      service: discover.savedSearches.createLoader({
-        savedObjectsClient: coreStart.savedObjects.client,
-        indexPatterns: data.indexPatterns,
-        search: data.search,
-        chrome: coreStart.chrome,
-        overlays: coreStart.overlays,
-      }),
+      service: discover.savedSearchLoader,


kindsun

Nice! Maps changes lgtm

# Conflicts: # src/legacy/core_plugins/kibana/public/discover/np_ready/angular/context/api/context.ts # src/legacy/core_plugins/kibana/public/discover/np_ready/angular/context/query/actions.js # src/legacy/core_plugins/kibana/public/discover/np_ready/angular/discover.js # src/plugins/data/public/public.api.md # src/plugins/data/public/search/types.ts # src/plugins/discover/kibana.json # src/plugins/discover/public/mocks.ts # src/plugins/discover/public/plugin.ts # src/plugins/visualize/public/application/legacy_app.js # test/functional/config.js

# Conflicts: # src/plugins/data/public/public.api.md

ppisljar · 2020-05-06T15:18:17Z

retest

lukeelmers

Code changes here LGTM, pending a green build

ppisljar · 2020-05-07T10:02:54Z

@elasticmachine merge upstream

…Response

flash1293

I found an error which is likely related to this change (couldn't make the connection yet). When importing a legacy JSON file via management that contains a broken index pattern reference, selecting a resolution and confirming raises an exception "Cannot read property 'save' of undefined".

You can verify with https://gist.github.com/flash1293/55a6dac0d1e833d9c0a77baf11c78682 - the index pattern reference of the last visualization is the malformed one.

flash1293 · 2020-05-07T15:48:23Z

src/plugins/data/public/search/search_source/create_search_source.test.ts

  });

-  test('should fail if JSON is invalid', () => {


Not a big deal but seems like this test got lost - looks like it should become a unit test of https://github.com/elastic/kibana/pull/64207/files#diff-0138b68230bd9fd9e68b55fe4e8868b1

# Conflicts: # src/plugins/data/public/public.api.md

ppisljar · 2020-05-11T07:36:08Z

retest

ppisljar · 2020-05-11T10:48:43Z

@elasticmachine merge upstream

kibanamachine · 2020-05-12T10:10:40Z

💚 Build Succeeded

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

History

💚 Build #46866 succeeded 4ebe447688be0031c45b5d7979ea2a52784369ef
💚 Build #46827 succeeded 01afd42
💚 Build #46821 succeeded 01afd42
💔 Build #46810 failed 0a6870f
💔 Build #46341 failed 284010f

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

flash1293

Tested in Firefox and works as expected except for legacy import (separate issue created as it's probably not related)

ppisljar added review v8.0.0 Team:AppArch v7.8.0 labels Apr 22, 2020

ppisljar requested a review from a team April 22, 2020 15:40

ppisljar requested review from a team as code owners April 22, 2020 15:40

ppisljar requested a review from lukeelmers April 22, 2020 15:52

ppisljar added the release_note:skip Skip the PR/issue when compiling release notes label Apr 22, 2020

ppisljar requested a review from flash1293 April 22, 2020 15:53

lukeelmers reviewed Apr 23, 2020

View reviewed changes

pgayvallet approved these changes Apr 23, 2020

View reviewed changes

ppisljar force-pushed the searchSource/serialize branch 2 times, most recently from a3ee243 to cb5ed20 Compare April 23, 2020 13:44

making vis serializable

fef7941

ppisljar force-pushed the searchSource/serialize branch from cb5ed20 to af4f9ea Compare April 30, 2020 05:21

fixing and applying review feedback

19c1af9

ppisljar force-pushed the searchSource/serialize branch from af4f9ea to 19c1af9 Compare April 30, 2020 11:39

ppisljar requested a review from a team as a code owner April 30, 2020 11:39

kindsun approved these changes Apr 30, 2020

View reviewed changes

ppisljar force-pushed the searchSource/serialize branch 2 times, most recently from 235b815 to 4b83ca8 Compare May 6, 2020 08:40

fixing after merge master

d5f52d5

ppisljar force-pushed the searchSource/serialize branch from 4b83ca8 to d5f52d5 Compare May 6, 2020 10:50

ppisljar added 2 commits May 6, 2020 04:01

Merge branch 'master' into searchSource/serialize

8e378b9

# Conflicts: # src/plugins/data/public/public.api.md

updating docs

048f89a

lukeelmers reviewed May 7, 2020

View reviewed changes

ppisljar force-pushed the searchSource/serialize branch from ddaf007 to 34c826f Compare May 7, 2020 13:28

fixing management, which expects indexpattern to be loaded in applyEs…

284010f

…Response

ppisljar force-pushed the searchSource/serialize branch from 34c826f to 284010f Compare May 7, 2020 15:02

flash1293 reviewed May 7, 2020

View reviewed changes

ppisljar added 3 commits May 7, 2020 21:19

Merge branch 'master' into searchSource/serialize

0d15ddd

# Conflicts: # src/plugins/data/public/public.api.md

Merge branch 'master' into searchSource/serialize

0a6870f

updating docs again

01afd42

Merge branch 'master' into searchSource/serialize

cbde348

ppisljar force-pushed the searchSource/serialize branch from 4ebe447 to cbde348 Compare May 12, 2020 08:13

flash1293 approved these changes May 12, 2020

View reviewed changes

ppisljar merged commit 15c0644 into elastic:master May 12, 2020

ppisljar mentioned this pull request May 12, 2020

[7.x] making vis (completely) serializable (#64207) #66199

Merged

ppisljar added a commit to ppisljar/kibana that referenced this pull request May 12, 2020

making vis (completely) serializable (elastic#64207)

2afead9

ppisljar added a commit that referenced this pull request May 12, 2020

making vis (completely) serializable (#64207) (#66199)

b7ed53f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

making vis (completely) serializable #64207

making vis (completely) serializable #64207

ppisljar commented Apr 22, 2020 •

edited

Loading

elasticmachine commented Apr 22, 2020

lukeelmers left a comment

lukeelmers Apr 23, 2020

lukeelmers Apr 23, 2020

lukeelmers Apr 23, 2020

lukeelmers Apr 23, 2020

lukeelmers Apr 23, 2020

lukeelmers Apr 23, 2020

lukeelmers Apr 23, 2020

lukeelmers Apr 23, 2020

ppisljar Apr 30, 2020 •

edited

Loading

pgayvallet left a comment

pgayvallet Apr 23, 2020

kindsun left a comment

ppisljar commented May 6, 2020

lukeelmers left a comment

ppisljar commented May 7, 2020

flash1293 left a comment •

edited

Loading

flash1293 May 7, 2020

ppisljar commented May 11, 2020

ppisljar commented May 11, 2020

kibanamachine commented May 12, 2020

flash1293 left a comment

	export const convertToSerializedVis = async (savedVis: ISavedVis): Promise<SerializedVis> => {
	export const convertToSerializedVis = async (savedVis: Required<ISavedVis>): Promise<SerializedVis> => {

making vis (completely) serializable #64207

making vis (completely) serializable #64207

Conversation

ppisljar commented Apr 22, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Apr 22, 2020

lukeelmers 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

ppisljar Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

pgayvallet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kindsun left a comment

Choose a reason for hiding this comment

ppisljar commented May 6, 2020

lukeelmers left a comment

Choose a reason for hiding this comment

ppisljar commented May 7, 2020

flash1293 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppisljar commented May 11, 2020

ppisljar commented May 11, 2020

kibanamachine commented May 12, 2020

💚 Build Succeeded

History

flash1293 left a comment

Choose a reason for hiding this comment

ppisljar commented Apr 22, 2020 •

edited

Loading

ppisljar Apr 30, 2020 •

edited

Loading

flash1293 left a comment •

edited

Loading