Ignore missing references on saved object exports by pgayvallet · Pull Request #47685 · elastic/kibana

pgayvallet · 2019-10-09T12:07:32Z

Release notes

When exporting saved objects with "include related objects" enabled, the saved objects management UI will now display a warning and continue instead of failing if some related objects could not be found.

Fixes #43876

Breaking changes

This PR changes the behaviour of the /api/saved_objects/_export experimental endpoint

no longer returns error when includeReferencesDeep option is true and some references are non-existing (ignore them instead)
returns an export summary at the end of the ndjson stream containing the number of exported objects and the number and id's of any missing references (this is a breaking change for consumers of the endpoint)
add an excludeExportDetails option to disable the previous behaviour

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~

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~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2019-10-09T12:33:36Z

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

elasticmachine · 2019-10-09T13:14:17Z

💔 Build Failed

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

elasticmachine · 2019-10-09T14:29:08Z

💔 Build Failed

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

elasticmachine · 2019-10-09T15:14:09Z

💔 Build Failed

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

elasticmachine · 2019-10-09T17:00:19Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 916b5d2841e35bb354c569f9116a542e6b809596

elasticmachine · 2019-10-09T18:07:55Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 0ac506e3691a8b114f882cc5d29086e533294d88

elasticmachine · 2019-10-09T20:13:58Z

💔 Build Failed

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

elasticmachine · 2019-10-09T22:00:25Z

💚 Build Succeeded

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

elasticmachine · 2019-10-10T11:18:24Z

💔 Build Failed

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

elasticmachine · 2019-10-10T12:58:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 5c42fe42fb090894a53324a30633ef87b03c0d78

rudolf · 2019-10-10T15:01:11Z

@mikecote Would be good to get your opinion on the design but I think what Pierre did is more elegant than my previous suggestion of failing the export and then retrying with ?ignoreMissingReferences=true.

At the end of the NDJSON stream we include a status entry with all the references which couldn't be found. I think if we move to a truly streaming API this will be really useful so that as someone only consuming the API you can be sure that the stream "completed" and that there weren't any errors (like if an exception occurred after exporting 1GB's of objects).

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

elasticmachine · 2019-10-10T17:19:06Z

💔 Build Failed

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

mikecote · 2019-10-10T17:53:49Z

@rudolf the approach makes sense to me 👍. The one drawback I can see is the entire file may have to be read (in memory) in order to gather the status entry. This is more a problem in the future with 1GB files and I'm sure there's a stream way to do this on the browser without reading the whole file. Though I think that problem can be deferred to the future as only 10,000 objects are supported at this time (and I think < 10MB imports).

elasticmachine · 2019-10-10T18:33:19Z

💚 Build Succeeded

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

pgayvallet · 2019-10-11T09:09:08Z

@mikecote The blob is more or less a reader interface, we need to read the whole blob to get the last entry, but we can only keep the last / current reading line in memory. This will definitely makes sense once we properly stream the content from server to browser. For now, I think reading (max) 10MB in memory is fine.

rudolf

Left a couple of copy and docs comments but overall this looks very good

src/core/server/saved_objects/export/get_sorted_objects_for_export.ts

docs/api/saved-objects/export.asciidoc

docs/development/core/server/kibana-plugin-server.md

src/core/server/saved_objects/export/get_sorted_objects_for_export.ts

..._plugins/kibana/public/management/sections/objects/components/objects_table/objects_table.js

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

elasticmachine · 2019-10-11T14:22:48Z

💔 Build Failed

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

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

elasticmachine · 2019-10-11T16:39:28Z

💚 Build Succeeded

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

pgayvallet · 2019-10-14T16:03:08Z

@rudolf all changes has been made, PTAL 4 last commits

rudolf

🎉

pgayvallet · 2019-10-15T05:31:31Z

@legrego Could you review the changes impacting spaces ?

legrego · 2019-10-15T11:54:58Z

@pgayvallet will do! I have one PR to review ahead of this one, but I'll get to it as soon as possible

legrego · 2019-10-15T14:11:57Z

returns an export summary at the end of the ndjson stream containing number of exported objects and number / id of missing references if any. (this is a breaking change for consumers of the endpoint)

question: Is there a reason why we have to make this a breaking change for consumers?

add a excludeExportDetails option to disable the previous behaviour

If we change this to includeExportDetails, and default the value to false, then existing consumers will continue to work without changes. We'd need to update the UI to include this query parameter, but all other things equal, it's easier for us to add this parameter than it is to ask all other consumers to add the exclude option.

Also, sorry for the merge conflict - we started moving the spaces plugin to the new platform. I don't expect the changes to be all that difficult though. Let me know if you need help resolving!

pgayvallet · 2019-10-15T14:58:12Z

Well, the first point would be that this API is flagged as experimental, meaning that breaking changes may occurs.

But the actual reason is that the ndjson format being that containing, we really wanted a way to add Metadata to the format. As an extra line is the only approach, we want to change the format as soon as possible to avoid doing it later. The Metadata should now be a part of the default export. The 'exclude' option is here for rétro-compatibility. Not the other way around.

I hope that make sense

pgayvallet · 2019-10-15T15:10:01Z

@rudolf what do you think about @legrego proposal?

legrego

Pending merge conflicts, the changes to spaces LGTM! Tested locally.

rudolf · 2019-10-16T09:06:35Z

In retrospect it might have been better to go with @legrego's proposal since not introducing a breaking change would have made it easy to include this in 7.4.1 but I think it's too late to squeeze it into the release.

I think including the metadata in the response is a better default for this API. If you're an API consumer you shouldn't be relying on the HTTP status code, as when the internals have eventually been converted to be completely stream based we'll send a 200 before we've completed the whole export. So I think we should introduce the breaking change at some point.

Since this is an experimental API and we have an easy flag to opt-out of the breaking behaviour I agree with @pgayvallet that doing this sooner is probably preferable.

elasticmachine · 2019-10-16T10:22:52Z

💚 Build Succeeded

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

* add saved object export details in ndjson response Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * update core doc Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * exclude export details for space copy Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * fixing tests Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * display warning instead of success if export contains missing refs Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * nits/typo Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * properly updates api integration tests Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * fix typings Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * add test on objects_table component Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * remove added translations from jp/cn bundles Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * restoring line feeds Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * improve doc and user alert message Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * restoring line feeds on server.api.md Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co> * warning test label Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

joshdover · 2019-10-16T14:54:11Z

Removing the breaking label as this is not a stable API.

pgayvallet added Feature:Saved Objects Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// labels Oct 9, 2019

pgayvallet force-pushed the kbn-43876-ignore-missing-refs branch from e537b42 to cdb38be Compare October 9, 2019 13:21

pgayvallet force-pushed the kbn-43876-ignore-missing-refs branch from c2b2e24 to 916b5d2 Compare October 9, 2019 15:48

pgayvallet added release_note:skip Skip the PR/issue when compiling release notes v7.5.0 v8.0.0 labels Oct 9, 2019

pgayvallet force-pushed the kbn-43876-ignore-missing-refs branch from 916b5d2 to 0ac506e Compare October 9, 2019 16:52

pgayvallet force-pushed the kbn-43876-ignore-missing-refs branch from 0ac506e to d64ebc9 Compare October 9, 2019 18:54

pgayvallet marked this pull request as ready for review October 10, 2019 13:17

pgayvallet requested review from a team as code owners October 10, 2019 13:17

pgayvallet requested review from legrego and rudolf October 10, 2019 13:18

pgayvallet added release_note:breaking release_note:fix and removed release_note:skip Skip the PR/issue when compiling release notes labels Oct 10, 2019

pgayvallet added 2 commits October 10, 2019 17:51

add saved object export details in ndjson response

e4fcacb

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

update core doc

446a1fa

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

remove added translations from jp/cn bundles

cf86141

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

rudolf requested changes Oct 11, 2019

View reviewed changes

pgayvallet added 3 commits October 11, 2019 14:34

restoring line feeds

112115c

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

improve doc and user alert message

08476c3

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

restoring line feeds on server.api.md

c60d199

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

warning test label

e181461

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

rudolf approved these changes Oct 14, 2019

View reviewed changes

legrego approved these changes Oct 15, 2019

View reviewed changes

Merge branch 'master' into kbn-43876-ignore-missing-refs

6879092

rudolf merged commit 7df981f into elastic:master Oct 16, 2019

joshdover removed the release_note:breaking label Oct 16, 2019

This was referenced Oct 31, 2019

Object export error handling #49769

Closed

unable to download log from kibana dashboard #49836

Closed

Conversation

pgayvallet commented Oct 9, 2019 • edited by rudolf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release notes

Breaking changes

Checklist

For maintainers

Uh oh!

elasticmachine commented Oct 9, 2019

Uh oh!

elasticmachine commented Oct 9, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Oct 9, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Oct 9, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Oct 9, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Oct 9, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Oct 9, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Oct 9, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Oct 10, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Oct 10, 2019

💚 Build Succeeded

Uh oh!

rudolf commented Oct 10, 2019

Uh oh!

elasticmachine commented Oct 10, 2019

💔 Build Failed

Uh oh!

mikecote commented Oct 10, 2019

Uh oh!

elasticmachine commented Oct 10, 2019

💚 Build Succeeded

Uh oh!

pgayvallet commented Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rudolf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elasticmachine commented Oct 11, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Oct 11, 2019

💚 Build Succeeded

Uh oh!

pgayvallet commented Oct 14, 2019

Uh oh!

rudolf left a comment

Choose a reason for hiding this comment

Uh oh!

pgayvallet commented Oct 15, 2019

Uh oh!

legrego commented Oct 15, 2019

Uh oh!

legrego commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pgayvallet commented Oct 15, 2019

Uh oh!

pgayvallet commented Oct 15, 2019

Uh oh!

pgayvallet commented Oct 9, 2019 •

edited by rudolf

Loading

pgayvallet commented Oct 11, 2019 •

edited

Loading

legrego commented Oct 15, 2019 •

edited

Loading