Remove SO root property index signature #64434

rudolf · 2020-04-24T14:35:53Z

Summary

Remove the root property index signature from SavedObjectsUnsanitizedDoc to increase the type safety of migrations which rely on this type.

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-24T14:35:56Z

Pinging @elastic/kibana-platform (Team:Platform)

rudolf · 2020-04-24T14:38:35Z

src/core/server/saved_objects/migrations/core/document_migrator.test.ts

@@ -165,13 +165,13 @@ describe('DocumentMigrator', () => {
      attributes: { name: 'Tyler' },
      acl: 'anyone',
      migrationVersion: {},
-    });
+    } as SavedObjectUnsanitizedDoc);


our types are stricter than the code... although the code still supports plugins adding their own root property (the acl type can add the acl root property) there's no plugins using this and we should probably remove this functionality.

Yea, I feel like migrations are more permissive than they should, at least for plugins.

pgayvallet · 2020-04-24T20:02:20Z

src/core/server/saved_objects/migrations/core/document_migrator.ts

-    (doc[prop] && doc[prop].latestVersion) ||
+    ((doc as any)[prop] && (doc as any)[prop].latestVersion) ||
    (doc.migrationVersion && (doc as any).migrationVersion[prop])


Wonder if it's worth to introduce an internal type that keeps this [rootProp: string]: any ?

I considered it, but it felt like we're losing type safety instead of increasing it. Core doesn't usually touch any of the root properties except the ones that core itself defines (updated_at namespaces), this section that performs the migration is the only exception where Core basically touches data owned by a plugin. So it felt like we could rather have any override here with slightly more type safety everywhere else.

pgayvallet · 2020-04-24T20:03:29Z

src/core/server/saved_objects/migrations/core/document_migrator.test.ts

@@ -165,13 +165,13 @@ describe('DocumentMigrator', () => {
      attributes: { name: 'Tyler' },
      acl: 'anyone',
      migrationVersion: {},
-    });
+    } as SavedObjectUnsanitizedDoc);


Yea, I feel like migrations are more permissive than they should, at least for plugins.

kibanamachine · 2020-04-27T14:07:23Z

💚 Build Succeeded

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

History

💔 Build #43100 failed 54370cf

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

* Remove SO root property index signature * Update documentation

* master: (78 commits) Remove SO root property index signature (elastic#64434) [ML] Functional tests - stabilize job row details validations (elastic#64503) [Ingest] Add Global settings flyout (elastic#64276) Bump cypress dev-dependency from 4.2.0 to 4.4.1 (elastic#64408) Migrate saved object of type url to kibana platform (elastic#64043) [NP] Migrate ui capabilities (elastic#64185) Bump karma-mocha dev-dependency from 1.3.0 to 2.0.0 (elastic#64407) Migrate kql_telemetry saved object registration to Kibana platform (elastic#64149) Remove SO autocreateindex error and error page (elastic#64037) Fix issue with yarn.lock (elastic#64496) Bump @hapi/boom dependency from 7.4.2 to 7.4.11 (elastic#64433) Bump gonzales-pe dev-dependency from 4.2.4 to 4.3.0 (elastic#64401) Bump karma dev-dependency from 3.1.4 to 5.0.2 (elastic#64403) Bump iedriver dev-dependency from 3.14.1 to 3.14.2 (elastic#64406) Revert "disable flaky suite (elastic#64382) (elastic#64381) (elastic#64480)" Revert "skip flaky suite (elastic#64310)" skip flaky suite (elastic#64310) disable flaky suite (elastic#64382) (elastic#64381) (elastic#64480) [Maps] Include maps styles in embeddables factory so that they're bundled w/ consuming plugins (elastic#64460) update charts library to 18.4.2 (elastic#64438) ...

* Remove SO root property index signature * Update documentation

…bana into pipeline-editor-part-mvp-2 * 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (152 commits) [Ingest pipelines] Simulate pipeline (elastic#64223) Ability to get scoped call cluster from alerting and action executors (elastic#64432) Add editApp and editPath to embeddable (elastic#64297) TSVB validation: Allow numeric values for axes (elastic#63553) [ML] Fixing optional plugin dependency types (elastic#64450) [Logs UI] Alerting (elastic#62806) [Endpoint] Show Policy Status on Host Details using Policy Response API (elastic#64116) [Maps] update LayerWizard previewLayer to take layerDescriptor instead of ISource (elastic#64461) Remove SO root property index signature (elastic#64434) [ML] Functional tests - stabilize job row details validations (elastic#64503) [Ingest] Add Global settings flyout (elastic#64276) Bump cypress dev-dependency from 4.2.0 to 4.4.1 (elastic#64408) Migrate saved object of type url to kibana platform (elastic#64043) [NP] Migrate ui capabilities (elastic#64185) Bump karma-mocha dev-dependency from 1.3.0 to 2.0.0 (elastic#64407) Migrate kql_telemetry saved object registration to Kibana platform (elastic#64149) Remove SO autocreateindex error and error page (elastic#64037) Fix issue with yarn.lock (elastic#64496) Bump @hapi/boom dependency from 7.4.2 to 7.4.11 (elastic#64433) Bump gonzales-pe dev-dependency from 4.2.4 to 4.3.0 (elastic#64401) ... # Conflicts: # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form.tsx # x-pack/plugins/ingest_pipelines/public/shared_imports.ts

rudolf added 2 commits April 24, 2020 16:25

Remove SO root property index signature

feaa4cd

Update documentation

54370cf

rudolf added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Saved Objects v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.8.0 labels Apr 24, 2020

rudolf commented Apr 24, 2020

View reviewed changes

rudolf self-assigned this Apr 24, 2020

rudolf requested a review from pgayvallet April 24, 2020 14:39

rudolf marked this pull request as ready for review April 24, 2020 14:41

rudolf requested a review from a team as a code owner April 24, 2020 14:41

pgayvallet approved these changes Apr 24, 2020

View reviewed changes

Merge branch 'master' into rm-so-index-signature

bf294a4

rudolf mentioned this pull request Apr 27, 2020

add generic typings for SavedObjectMigrationFn #63943

Merged

1 task

rudolf merged commit dfb6a30 into elastic:master Apr 27, 2020

rudolf deleted the rm-so-index-signature branch April 27, 2020 14:08

rudolf mentioned this pull request Apr 27, 2020

[7.x] Remove SO root property index signature (#64434) #64520

Merged

rudolf added a commit to rudolf/kibana that referenced this pull request Apr 27, 2020

Remove SO root property index signature (elastic#64434)

5d6d8a6

* Remove SO root property index signature * Update documentation

rudolf added a commit that referenced this pull request Apr 27, 2020

Remove SO root property index signature (#64434) (#64520)

9932bc6

* Remove SO root property index signature * Update documentation

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove SO root property index signature #64434

Remove SO root property index signature #64434

rudolf commented Apr 24, 2020

elasticmachine commented Apr 24, 2020

rudolf Apr 24, 2020 •

edited

Loading

pgayvallet Apr 24, 2020

pgayvallet Apr 24, 2020

rudolf Apr 27, 2020

pgayvallet Apr 27, 2020

pgayvallet Apr 24, 2020

kibanamachine commented Apr 27, 2020

Remove SO root property index signature #64434

Remove SO root property index signature #64434

Conversation

rudolf commented Apr 24, 2020

Summary

Checklist

For maintainers

elasticmachine commented Apr 24, 2020

rudolf Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

pgayvallet Apr 24, 2020

Choose a reason for hiding this comment

pgayvallet Apr 24, 2020

Choose a reason for hiding this comment

rudolf Apr 27, 2020

Choose a reason for hiding this comment

pgayvallet Apr 27, 2020

Choose a reason for hiding this comment

pgayvallet Apr 24, 2020

Choose a reason for hiding this comment

kibanamachine commented Apr 27, 2020

💚 Build Succeeded

History

rudolf Apr 24, 2020 •

edited

Loading