fixes embeddables migrate function #101470

ppisljar · 2021-06-07T10:41:25Z

Summary

fixes embeddables migrate function to not expect an enhancement migration defined for every version of kibana
resolves #101430

Checklist

Unit or functional tests were updated or added to match the most common scenarios

streamich

LGTM

ThomThomson

I tested locally by importing a dashboard from 7.12.1 which had a by value panel with a drilldown. The import went through, but unfortunately the drilldown was lost. This is because this migration function removes all enhancements which don't have migrations registered.

I'm pretty sure that this behaviour is unintentional. I left a fix in the comments, which I've tested locally. The fix works correctly, but I would strongly encourage you to verify it by testing this fix with an import.

ThomThomson · 2021-06-08T15:24:09Z

src/plugins/embeddable/common/lib/migrate.ts

+      const enhancementDefinition = embeddables.getEnhancement(key);
+      if (enhancementDefinition.migrations && enhancementDefinition.migrations[version]) {
+        (updatedInput.enhancements! as Record<string, any>)[key] = enhancementDefinition.migrations[
+          version
+        ](enhancements[key] as SerializableState);
+      }


I tested this function. It should run migrations on enhancements which have them, and retain enhancements which don't.

Suggested change

const enhancementDefinition = embeddables.getEnhancement(key);

if (enhancementDefinition.migrations && enhancementDefinition.migrations[version]) {

(updatedInput.enhancements! as Record<string, any>)[key] = enhancementDefinition.migrations[

version

](enhancements[key] as SerializableState);

}

const enhancementDefinition = embeddables.getEnhancement(key);

const migratedEnhancement = enhancementDefinition?.migrations?.[version]

? enhancementDefinition.migrations[version](enhancements[key] as SerializableState)

: enhancements[key];

(updatedInput.enhancements! as Record<string, any>)[key] = migratedEnhancement;

@ppisljar - would you be able to add a test that would have caught this issue? Unless I'm missing it, it looks like the only test added was for the original bug, not the secondary bug.

the test was added to catch this as well

Great, thanks Peter!

Actually, did you verify this? I just copied the old code into the file and ran tests, and they all still pass.

It's my understanding (maybe I'm wrong), that your first fix had a bug that Devon found and it wasn't caught by tests? That is the situation I am asking for tests for. Just because the original code didn't show that bug doesn't mean there shouldn't be a test for it.

because original code didn't show that bug you trying to take original code and get the new test failing doesn't work.
you would need to take the intermediate code (where devon found a bug) and then the test would fail

I synced with @ppisljar about this, and we saw how if we take the first iteration of code (pasted below), and run the tests, they all still pass:

const enhancementDefinition = embeddables.getEnhancement(key); if (enhancementDefinition.migrations && enhancementDefinition.migrations[version]) { (updatedInput.enhancements! as Record<string, any>)[key] = enhancementDefinition.migrations[ version ](enhancements[key] as SerializableState); }

@ppisljar - can you follow up when you've added a test that would catch the bug?

This definitely needs unit test coverage.

As some functional coverage for this issue, In this PR I assert that all drilldowns survive the migration process (on by value & by reference panels). The original code removed drilldowns from by value panels, so if we run into something like this again, the smoke test should fail.

Filed #102772 to keep track

Co-authored-by: Devon Thomson <devon.thomson@elastic.co>

…ibana into fix/embeddablemigrate

kibanamachine · 2021-06-09T15:17:13Z

💚 Build Succeeded

Metrics [docs]

Page load bundle

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

id	before	after	diff
`embeddable`	179.1KB	179.4KB	+299.0B

History

💔 Build #130090 failed 04c01bd
💔 Build #130013 failed 987c4ab
💚 Build #129691 succeeded 27a619f
💔 Build #129479 failed 35c3e73
💔 Build #129434 failed 3188923

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

ThomThomson

Fix LGTM. Tested locally again, and the drilldown is properly coming through now. Thanks for implementing this!

* master: (173 commits) [kbnArchiver] convert archive names to root-relative paths (elastic#101839) [Reporting] Make "ScreenCapturePanel" shareable for Canvas (elastic#100623) [Alerting UI] Converted Rules and Connectors management pages to new layout. (elastic#101697) [Fleet] Support granular integrations in policy editor (elastic#101531) [Security Solution][Detections] Update detection alert mappings to ECS v1.10.0 (elastic#101680) [Fleet] Integrations UI: Adjust policies list UI (elastic#101600) chore(NA): moving @kbn/server-route-repository into bazel (elastic#101484) Support owner and description attributes inside the Manifest file, use in API docs (elastic#101786) [Security Solution] fix security empty overview links (elastic#101536) Unskips migration tests now that elastic search is fixed (elastic#101682) Fix endpoint -> integrations onboarding link (elastic#101804) [Alerting] Log warning when rules are not rescheduled due to Saved Object not found error (elastic#101591) Update datafeed_high_count_network_denies.json (elastic#101681) [Index patterns] Field editor example app (elastic#100524) [DOCS] Adding file upload to add data page (elastic#101674) [Security Solution][Endpoint] Adds Endpoint Host Isolation Status common component (elastic#101782) Upgrade ws v7.3.1->v7.4.2 and v6.2.1->v6.2.2 (elastic#101402) fixes embeddables migrate function (elastic#101470) [Canvas] Update slow query in sample ecommerce workpad (elastic#100714) clarify which parts of TM are experimental (elastic#101757) ...

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

elasticmachine · 2021-06-15T13:30:56Z

Pinging @elastic/kibana-app-services (Team:AppServices)

ppisljar requested a review from a team as a code owner June 7, 2021 10:41

botelastic bot added the Feature:Embedding Embedding content via iFrame label Jun 7, 2021

ppisljar changed the title ~~fixing nested serialization/deserialization~~ fixes embeddables migrate function Jun 7, 2021

ppisljar added v7.13.2 v7.14.0 v8.0.0 release_note:fix labels Jun 7, 2021

streamich approved these changes Jun 7, 2021

View reviewed changes

fixing nested serialization/deserialization

35c3e73

ppisljar force-pushed the fix/embeddablemigrate branch from 3188923 to 35c3e73 Compare June 7, 2021 13:03

fixing test

27a619f

ThomThomson requested changes Jun 8, 2021

View reviewed changes

ppisljar and others added 5 commits June 9, 2021 06:45

Update src/plugins/embeddable/common/lib/migrate.ts

987c4ab

Co-authored-by: Devon Thomson <devon.thomson@elastic.co>

fixing test

72ee4a7

Merge branch 'fix/embeddablemigrate' of https://github.com/ppisljar/k…

09509ab

…ibana into fix/embeddablemigrate

Merge branch 'master' into fix/embeddablemigrate

04c01bd

fixing eslint

666604a

ThomThomson approved these changes Jun 9, 2021

View reviewed changes

ppisljar merged commit 07c48a3 into elastic:master Jun 9, 2021

ppisljar mentioned this pull request Jun 9, 2021

[7.x] fixes embeddables migrate function (#101470) #101818

Merged

ppisljar added a commit to ppisljar/kibana that referenced this pull request Jun 9, 2021

fixes embeddables migrate function (elastic#101470)

b314d7e

ppisljar mentioned this pull request Jun 9, 2021

[7.13] fixes embeddables migrate function (#101470) #101819

Merged

ppisljar added a commit to ppisljar/kibana that referenced this pull request Jun 9, 2021

fixes embeddables migrate function (elastic#101470)

24f9c40

ppisljar added a commit that referenced this pull request Jun 9, 2021

fixes embeddables migrate function (#101470) (#101819)

868a713

ppisljar added a commit that referenced this pull request Jun 10, 2021

fixes embeddables migrate function (#101470) (#101818)

5e4c549

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

rashmivkulkarni mentioned this pull request Jun 10, 2021

[DOCS] Adds the 7.13.2 release notes #101924

Merged

timroes added the Team:AppServices label Jun 15, 2021

bhavyarm mentioned this pull request Jun 17, 2021

Kibana should display error message with information instead of internal server error when user tries to import buggy saved objects #102554

Closed

stacey-gammon mentioned this pull request Jun 21, 2021

[PersistableState] Improve unit test coverage of embeddable migrations #102772

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes embeddables migrate function #101470

fixes embeddables migrate function #101470

ppisljar commented Jun 7, 2021 •

edited

Loading

streamich left a comment

ThomThomson left a comment

ThomThomson Jun 8, 2021

stacey-gammon Jun 14, 2021

ppisljar Jun 14, 2021

stacey-gammon Jun 15, 2021

stacey-gammon Jun 15, 2021

stacey-gammon Jun 15, 2021

ppisljar Jun 15, 2021

stacey-gammon Jun 21, 2021

ThomThomson Jun 21, 2021

stacey-gammon Jun 21, 2021

kibanamachine commented Jun 9, 2021

ThomThomson left a comment

elasticmachine commented Jun 15, 2021

fixes embeddables migrate function #101470

fixes embeddables migrate function #101470

Conversation

ppisljar commented Jun 7, 2021 • edited Loading

Summary

Checklist

streamich left a comment

Choose a reason for hiding this comment

ThomThomson 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Jun 9, 2021

💚 Build Succeeded

Metrics [docs]

Page load bundle

History

ThomThomson left a comment

Choose a reason for hiding this comment

elasticmachine commented Jun 15, 2021

ppisljar commented Jun 7, 2021 •

edited

Loading