Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SavedObjects bulkCreate API should return migrationVersion and strip the type & namespace from the id #65150

Merged
merged 12 commits into from
May 8, 2020

Conversation

rudolf
Copy link
Contributor

@rudolf rudolf commented May 4, 2020

Summary

Fixes #65088

Release notes

When bulk creating saved objects without specifying the id's of the objects to be created, the SavedObjectsClient Javascript API and HTTP API incorrectly returned the raw id in the response. In addition, these API's also didn't return the migrationVersion property of the saved objects that were bulk created. This fix ensures that a valid saved object id and the migrationVersion property are returned in the bulk create response.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@rudolf rudolf added bug Fixes for quality problems that affect the customer experience Feature:Saved Objects Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels May 4, 2020
@elasticmachine
Copy link
Contributor

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

@rudolf rudolf added the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label May 4, 2020
@@ -72,6 +72,9 @@ export default function({ getService }) {
attributes: {
title: 'A great new dashboard',
},
migrationVersion: {
dashboard: resp.body.saved_objects[1].migrationVersion.dashboard,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This value will change anytime a new dashboard migration is added. This seems like the best workaround when using kbn-expect to prevent tests from failing anytime a new dashboard migration is added.

We should probably "modernize" these integration tests, but it felt like it's not enough of a priority to attempt this right now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, having jest toolbox for FTR tests would we really great. kbn-expect feels very old when compared to jest.

@rudolf rudolf marked this pull request as ready for review May 5, 2020 20:26
@rudolf rudolf requested a review from a team as a code owner May 5, 2020 20:26
@@ -72,6 +72,9 @@ export default function({ getService }) {
attributes: {
title: 'A great new dashboard',
},
migrationVersion: {
dashboard: resp.body.saved_objects[1].migrationVersion.dashboard,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, having jest toolbox for FTR tests would we really great. kbn-expect feels very old when compared to jest.

src/core/server/saved_objects/service/lib/repository.ts Outdated Show resolved Hide resolved
@rudolf rudolf requested a review from pgayvallet May 7, 2020 12:26
@rudolf rudolf requested a review from a team as a code owner May 7, 2020 19:06
@jportner jportner self-assigned this May 7, 2020
Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@rudolf rudolf added release_note:fix and removed release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. labels May 7, 2020
@rudolf rudolf changed the title Deserialize bulkCreate response to remove namespace type from id SavedObjects bulkCreate API should return migrationVersion and strip the type & namespace from the id May 7, 2020
Comment on lines 833 to 838
expect(result.saved_objects[0].id).toEqual(
expect.not.stringMatching(/config|index-pattern|myspace/)
);
expect(result.saved_objects[1].id).toEqual(
expect.not.stringMatching(/config|index-pattern|myspace/)
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we use the uuid regexp ([0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}) as done L828 instead of asserting against the names of the types/spaces used in the test suite? I think that's a little more explicit?

Copy link
Contributor Author

@rudolf rudolf May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

obj2's id is logstash-* so I initially thought I'll try to keep these assertions the same. But I agree, a uuid regexp would be better, so I'll change the first match to use a regexp and the second match to match obj2.id.

@rudolf rudolf merged commit 1baf0b0 into elastic:master May 8, 2020
@rudolf rudolf deleted the so-bulk-create-ids branch May 8, 2020 08:08
rudolf added a commit to rudolf/kibana that referenced this pull request May 8, 2020
…the type & namespace from the id (elastic#65150)

* Deserialize bulkCreate response to remove namespace type from id

* Index operations don't return _source in response

* Fix integration tests

* repository: make id generation and seq_no/primary_term spreading more explicit

* API Integration test for bulk create without ids

* Fix copy_to_space snapshot

* Revert "Fix copy_to_space snapshot"

This reverts commit 9c2b743.

* Move test into returns block

* repository.test.js stricter regexp matching
rudolf added a commit to rudolf/kibana that referenced this pull request May 8, 2020
…the type & namespace from the id (elastic#65150)

* Deserialize bulkCreate response to remove namespace type from id

* Index operations don't return _source in response

* Fix integration tests

* repository: make id generation and seq_no/primary_term spreading more explicit

* API Integration test for bulk create without ids

* Fix copy_to_space snapshot

* Revert "Fix copy_to_space snapshot"

This reverts commit 9c2b743.

* Move test into returns block

* repository.test.js stricter regexp matching
jloleysens added a commit to jloleysens/kibana that referenced this pull request May 8, 2020
…or-part-mvp-2

* 'master' of github.com:elastic/kibana: (259 commits)
  SavedObjects bulkCreate API should return migrationVersion and strip the type & namespace from the id (elastic#65150)
  Drilldown count tooltip (elastic#65105)
  plugins logs start with "plugins." prefix (elastic#65710)
  [ML] Fix pagination reset on search query update. (elastic#65668)
  [SIEM] Add types to the mappings objects so extra keys cannot be introduced
  [apm] Update machine learning flyout and service maps docs (elastic#65517)
  change api endpoint and throw error (elastic#65790)
  [Maps] remove SLA percentage metric (elastic#65718)
  [Reporting] APM integration for baseline performance measurements (elastic#59967)
  fix(NA): noParse regex for windows on kbn optimizer (elastic#65755)
  [ML] DFA: ensure at least one field is included in analysis before job can be created (elastic#65320)
  [Data plugin] cleanup - remove unused getRoutes / routes from indexPattern object (elastic#65683)
  Removed skip to enable test. (elastic#65575)
  [Lens] Type safe migrations (elastic#65576)
  [Canvas] Fix nav link behavior in Canvas  (elastic#65590)
  [Event log] Fix flaky test (elastic#65658)
  [Alerting] changes preconfigured actions config from array to object (elastic#65397)
  remove immediate functions from esqueue worker cycles (elastic#65375)
  [Metrics UI] Fix isAbove to only display when threshold set (elastic#65540)
  draft search profiler accessibility tests (elastic#62357)
  ...

# Conflicts:
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_fields.tsx
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

rudolf added a commit that referenced this pull request May 8, 2020
…the type & namespace from the id (#65150) (#65819)

* Deserialize bulkCreate response to remove namespace type from id

* Index operations don't return _source in response

* Fix integration tests

* repository: make id generation and seq_no/primary_term spreading more explicit

* API Integration test for bulk create without ids

* Fix copy_to_space snapshot

* Revert "Fix copy_to_space snapshot"

This reverts commit 9c2b743.

* Move test into returns block

* repository.test.js stricter regexp matching
rudolf added a commit that referenced this pull request May 8, 2020
…the type & namespace from the id (#65150) (#65820)

* Deserialize bulkCreate response to remove namespace type from id

* Index operations don't return _source in response

* Fix integration tests

* repository: make id generation and seq_no/primary_term spreading more explicit

* API Integration test for bulk create without ids

* Fix copy_to_space snapshot

* Revert "Fix copy_to_space snapshot"

This reverts commit 9c2b743.

* Move test into returns block

* repository.test.js stricter regexp matching
@rudolf rudolf mentioned this pull request Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Saved Objects release_note:fix Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SavedObjectsRepository#bulkCreate should not return raw id's
5 participants