Use savedObjectsClient.resolve in saved query service #111229

lukasolson · 2021-09-04T00:06:08Z

Summary

Updates the saved query service to use savedObjectsClient.resolve rather than savedObjectsClient.get.

For background, see https://www.elastic.co/guide/en/kibana/master/sharing-saved-objects.html#sharing-saved-objects-faq-resolve-instead-of-get. Saved queries can be deep-linked inside URLs, so we need to use the new resolve method to handle conflicts. Currently, conflicts will throw an error... (though I'm not sure if that's the best behavior... should they log an error and return the exact match?)

Checklist

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

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2021-09-04T00:06:10Z

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

jportner

Code review only --

One nit on an error message, and one problem with the type conversion, see individual comments

Edit: and this PR should be backported to 7.16

jportner · 2021-09-07T20:03:13Z

src/plugins/data/public/query/saved_query/saved_query_service.ts

  const getSavedQuery = async (id: string): Promise<SavedQuery> => {
-    const savedObject = await savedObjectsClient.get<SerializedSavedQueryAttributes>('query', id);
-    if (savedObject.error) {
+    const {
+      saved_object: savedObject,
+      outcome,
+    } = await savedObjectsClient.resolve<SerializedSavedQueryAttributes>('query', id);
+    if (outcome === 'conflict') {
+      throw new Error(`Multiple saved queries found with ID: ${id}`);
+    } else if (savedObject.error) {
      throw new Error(savedObject.error.message);
    }
    return parseSavedQueryObject(savedObject);


Just to let you know, when a saved query is not found, this error checking logic won't work as expected.
Currently if you get an object with the public SOC, it batches the request into a bulkGet under the hood - if it can't find the object, it doesn't throw an error, rather it just returns an object with an error field.

The resolve API doesn't work the same way, so it will actually throw an error when an object is not found.

That said, I have an open issue to add a bulkResolve with the same type of transparent batching, so after that is merged the behavior will be back to how you expect it: #111201

TL;DR no changes needed here 👍

jportner · 2021-09-07T20:06:04Z

src/plugins/data/server/saved_objects/query.ts

-  namespaceType: 'single',
+  namespaceType: 'multiple-isolated',


The rest of this PR is Step 2 of the Sharing Saved Objects Dev Guide, but this bit is Step 4.
Step 2 should be backported to 7.x, but Step 4 should not be backported, it should only land in 8.0.

You should split that into a separate PR that is not backported

In addition, you can't make this change without also using the convertToMultiNamespaceTypeVersion field.

src/plugins/data/public/query/saved_query/saved_query_service.ts

Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>

jportner

LGTM, thanks!

tsullivan

LGTM reviewed the code only.

kibanamachine · 2021-09-13T22:37:45Z

💚 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
`data`	777.8KB	777.9KB	+149.0B

History

💚 Build #152663 succeeded 08ed1c1
💛 Build #152051 was flaky 5f2596b
💔 Build #151643 failed 09e3b7b
💔 Build #151108 failed 7b85d04

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

cc @lukasolson

* Use resolve instead of get for saved query service * Update tests * Update src/plugins/data/public/query/saved_query/saved_query_service.ts Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com> * Revert step 4 * Fix test Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>

kibanamachine · 2021-09-13T23:03:31Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

) * Use resolve instead of get for saved query service * Update tests * Update src/plugins/data/public/query/saved_query/saved_query_service.ts Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com> * Revert step 4 * Fix test Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com> Co-authored-by: Lukas Olson <olson.lukas@gmail.com> Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>

lukasolson added 2 commits September 3, 2021 16:57

Use resolve instead of get for saved query service

872facd

Update tests

7b85d04

lukasolson added review Feature:Filters Feature:Query Bar Querying and query bar features v8.0.0 Team:AppServices release_note:skip Skip the PR/issue when compiling release notes labels Sep 4, 2021

lukasolson requested review from jportner and ppisljar September 4, 2021 00:06

lukasolson requested a review from a team as a code owner September 4, 2021 00:06

lukasolson self-assigned this Sep 4, 2021

Merge branch 'master' into saved-query-resolve

6aa49a2

jportner suggested changes Sep 7, 2021

View reviewed changes

lukasolson added auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 labels Sep 7, 2021

lukasolson and others added 2 commits September 7, 2021 13:30

Update src/plugins/data/public/query/saved_query/saved_query_service.ts

1e14b3b

Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>

Revert step 4

09e3b7b

lukasolson mentioned this pull request Sep 7, 2021

Make saved queries share-capable #111462

Merged

lukasolson requested a review from jportner September 7, 2021 20:37

jportner approved these changes Sep 7, 2021

View reviewed changes

lukasolson added 3 commits September 8, 2021 14:03

Fix test

5f2596b

Merge branch 'master' into saved-query-resolve

08ed1c1

Merge branch 'master' into saved-query-resolve

f38c895

tsullivan approved these changes Sep 13, 2021

View reviewed changes

lukasolson merged commit a8f9468 into elastic:master Sep 13, 2021

kibanamachine mentioned this pull request Sep 13, 2021

[7.x] Use savedObjectsClient.resolve in saved query service (#111229) #112031

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use savedObjectsClient.resolve in saved query service #111229

Use savedObjectsClient.resolve in saved query service #111229

lukasolson commented Sep 4, 2021 •

edited by ppisljar

Loading

elasticmachine commented Sep 4, 2021

jportner left a comment •

edited

Loading

jportner Sep 7, 2021

jportner Sep 7, 2021 •

edited

Loading

jportner left a comment

tsullivan left a comment

kibanamachine commented Sep 13, 2021

kibanamachine commented Sep 13, 2021

Use savedObjectsClient.resolve in saved query service #111229

Use savedObjectsClient.resolve in saved query service #111229

Conversation

lukasolson commented Sep 4, 2021 • edited by ppisljar Loading

Summary

Checklist

For maintainers

elasticmachine commented Sep 4, 2021

jportner left a comment • edited Loading

Choose a reason for hiding this comment

jportner Sep 7, 2021

Choose a reason for hiding this comment

jportner Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

jportner left a comment

Choose a reason for hiding this comment

tsullivan left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 13, 2021

💚 Build Succeeded

Metrics [docs]

Page load bundle

History

kibanamachine commented Sep 13, 2021

💚 Backport successful

lukasolson commented Sep 4, 2021 •

edited by ppisljar

Loading

jportner left a comment •

edited

Loading

jportner Sep 7, 2021 •

edited

Loading