Add SavedObjectsClient.bulkResolve #112025

jportner · 2021-09-13T20:30:33Z

Testing

In addition to the included automated tests, some manual testing steps:

Start Kibana
Add sample data
View sample dashboard

You should see a single _bulk_resolve API call that contains many lens objects to populate the dashboard.

Notes

In the saved objects core code, I added an internalBulkResolve module. Then I replaced the existing SavedObjectsRepository.resolve method implementation with that, and I also used it to add a new bulkResolve method. So now resolve and bulkResolve use the same implementation, reducing our testing burden and reducing the size of the SOR itself.

See also the commit message from 1e61891:

In abstracting this out to a separate module, I was able to reuse this instead of the existing resolve code, and I was able to simplify the unit tests.
Note, there is currently an inconsistent API response between get and bulkGet (an unsupported type for the former returns 404, and the latter returns 400). I opted to return a 400 for bulkResolveInternal as that is the most consistent response, so that means I had to change the integration tests for resolve accordingly.

In abstracting this out to a separate module, I was able to reuse this instead of the existing resolve code, and I was able to simplify the unit tests. Note, there is currently an inconsistent API response between get and bulkGet (an unsupported type for the former returns 404, and the latter returns 400). I opted to return a 400 for bulkResolveInternal as that is the most consistent response, so that means I had to change the integration tests for resolve accordingly.

jportner

Author's notes for reviewers

jportner · 2021-09-14T20:17:05Z

src/core/server/saved_objects/service/lib/internal_bulk_resolve.ts

+      resolveCounter.recordOutcome(REPOSITORY_RESOLVE_OUTCOME_STATS.NOT_FOUND);
+      return {
+        type,
+        id,
+        error: SavedObjectsErrorHelpers.createGenericNotFoundError(type, id),
+      };


(Part 1 of 3)
I'm not thrilled with this, but I view it as an acceptable compromise.

Here is the current behavior of get vs resolve:

get

found: returns { type, id, attributes, references, ... }

not found: throws error

bulkGet

found: returns [{ type, id, attributes, references, ... }]

not found: returns [{ type, id, error }]

resolve

found: returns { saved_object: { type, id, attributes, references, ... }, outcome }

not found: throws error

bulkResolve - what to do about this? (see other comment below)

Note that the SavedObject type requires the attributes and references fields and has an optional error field that is only used for bulk responses. But the current behavior of bulkGet (and the other bulk* methods) doesn't return a valid SavedObject type, because only the type, id, and error fields are present, because the SOR uses any under the hood.

When writing the internalBulkResolve module, I wanted to be able to reuse it for both SOR.resolve and SOR.bulkResolve -- that meant that I needed to return the actual DecoratedError object so that the SOR.resolve method could find it and throw the error.

When writing the internalBulkResolve module, I wanted to be able to reuse it for both SOR.resolve and SOR.bulkResolve

That makes sense

that meant that I needed to return the actual DecoratedError object so that the SOR.resolve method could find it and throw the error.

Can't we just use a higher level of abstraction by having internalBulkResolve returning the type of the error and letting the resolve and bulkResolve callers reformat/wrap them?

Can't we just use a higher level of abstraction by having internalBulkResolve returning the type of the error and letting the resolve and bulkResolve callers reformat/wrap them?

Well resolve needs to throw a DecoratedError, while bulkResolve needs to extract the payload.output field from it. So I figured the least common denominator is to have the internalBulkResolve return the DecoratedError itself.

If you think it's better to return the error type instead that should be fine and I can make that change (we really just have two types, "unsupported" and "not found"). I'll wait til you and Rudolf get a chance to reply before doing anything else though.

From a pure design standpoint, returning a real Error (or subclass) instance as an attribute of a return value is imho bad practice. Now, honestly, if that answers our current need, that's very fine with me.

jportner · 2021-09-14T20:18:56Z

src/core/server/saved_objects/service/lib/repository.ts

+    const { resolved_objects: bulkResults } = await internalBulkResolve<T>({
+      registry: this._registry,
+      allowedTypes: this._allowedTypes,
+      client: this.client,
+      serializer: this._serializer,
+      getIndexForType: this.getIndexForType.bind(this),
+      incrementCounterInternal: this.incrementCounterInternal.bind(this),
+      objects: [{ type, id }],
+      options,
+    });
+    const [result] = bulkResults;
+    if ((result as InternalBulkResolveError).error) {
+      throw (result as InternalBulkResolveError).error;
    }
-    throw SavedObjectsErrorHelpers.createGenericNotFoundError(type, id);
+    return result as SavedObjectsResolveResponse<T>;


(Part 2 of 3)

In this section, the SOR.resolve method 1. calls internalBulkResolve, 2. checks to see if there was an error (throwing if need be), and 3. if there was no error, returns the result, casting it as a SavedObjectsResolveResponse type. This matches the existing behavior.

jportner · 2021-09-14T20:25:20Z

src/core/server/saved_objects/service/lib/repository.ts

+    const { resolved_objects: bulkResults } = await internalBulkResolve<T>({
+      registry: this._registry,
+      allowedTypes: this._allowedTypes,
+      client: this.client,
+      serializer: this._serializer,
+      getIndexForType: this.getIndexForType.bind(this),
+      incrementCounterInternal: this.incrementCounterInternal.bind(this),
+      objects,
+      options,
+    });
+    const resolvedObjects = bulkResults.map<SavedObjectsResolveResponse<T>>((result) => {
+      // extract payloads from saved object errors
+      if ((result as InternalBulkResolveError).error) {
+        const errorResult = result as InternalBulkResolveError;
+        const { type, id, error } = errorResult;
+        return {
+          saved_object: ({ type, id, error: errorContent(error) } as unknown) as SavedObject<T>,
+          outcome: 'exactMatch',
+        };
+      }
+      return result as SavedObjectsResolveResponse<T>;
+    });
+    return { resolved_objects: resolvedObjects };


(Part 3 of 3)

In this section, the SOR.bulkResolve method 1. calls internalBulkResolve, 2. iterates over the results, extracting error content if need be and casting it as a SavedObjectsResolveResponse type, and 3. returns the result (which is now a valid SavedObjectsBulkResolveResponse type).

This means that any bulkResolve errors will have the shape:

{ saved_object: { type, id, error }, outcome: 'exactMatch' }

I can see two other alternatives to this behavior:

Add a special 'error' outcome that will only be used for bulkResolve responses

Omit the outcome field entirely for errors (casting this to a SavedObjectsResolveResponse type anyway)

The current approach is crappy, but it seems the least crappy because of the way that existing consumers on the client side use SOC.get; that API actually batches those calls into a single bulkGet on behalf of the consumer, and each consumer has some way of checking if the call succeeded (either by checking to see if an expected saved object field is present [positive test], or by checking to see if the error field is present [negative test]).

I'm a bit worried that if we return an unexpected outcome, or if we don't return an outcome at all, then the impact on consumer code will be worse.
I'm curious to see what the Core team thinks of this, or if there are any other suggestions.

I don't really like the exactMatch outcome for errors, as it imho doesn't make a lot of sense. I would have expected something like error or unknown.

But that's probably not that significant, so if you think this is better and easier to handle for the client-side consumers, I' fine keeping it that way. We probably want to document this behavior in the API's tsdoc though (at least on the server-side).

jportner · 2021-09-14T20:31:03Z

x-pack/test/saved_object_api_integration/spaces_only/apis/resolve.ts

-  { ...CASES.HIDDEN, ...fail404() },
+  { ...CASES.HIDDEN, ...fail400() },


Changed internalBulkResolve method to return an UnsupportedObjectType error instead of a NotFound error. See issue description; resolve was originally modeled after get, but there is a discrepancy between how get and bulkGet handle unsupported types. I figured that it made more sense for resolve and bulkResolve to both return an UnsupportedObjectType error.

pgayvallet · 2021-09-15T12:18:44Z

ack: will review today/tomorrow

pgayvallet

LGTM. A few remarks and questions, but overall nothing really significant.

Gave my opinion about the outcome issue for bulkResolve (see my comment), but I think it's probably fine either way.

src/core/public/saved_objects/saved_objects_client.ts

src/core/server/core_usage_data/types.ts

...ugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts

pgayvallet · 2021-09-15T14:52:39Z

src/core/server/saved_objects/service/lib/internal_bulk_resolve.ts

+interface Right<R> {
+  tag: 'Right';
+  value: R;
+}
+type Either<L = unknown, R = L> = Left<L> | Right<R>;
+const isLeft = <L, R>(either: Either<L, R>): either is Left<L> => either.tag === 'Left';
+const isRight = <L, R>(either: Either<L, R>): either is Right<R> => either.tag === 'Right';


We really need to extract this out of the repository file to avoid such duplication

pgayvallet · 2021-09-15T15:09:31Z

src/core/server/saved_objects/service/lib/internal_bulk_resolve.ts

+      resolveCounter.recordOutcome(REPOSITORY_RESOLVE_OUTCOME_STATS.NOT_FOUND);
+      return {
+        type,
+        id,
+        error: SavedObjectsErrorHelpers.createGenericNotFoundError(type, id),
+      };


When writing the internalBulkResolve module, I wanted to be able to reuse it for both SOR.resolve and SOR.bulkResolve

That makes sense

that meant that I needed to return the actual DecoratedError object so that the SOR.resolve method could find it and throw the error.

Can't we just use a higher level of abstraction by having internalBulkResolve returning the type of the error and letting the resolve and bulkResolve callers reformat/wrap them?

src/core/server/saved_objects/service/lib/internal_bulk_resolve.ts

pgayvallet · 2021-09-15T15:28:55Z

src/core/server/saved_objects/service/lib/repository.ts

+    const { resolved_objects: bulkResults } = await internalBulkResolve<T>({
+      registry: this._registry,
+      allowedTypes: this._allowedTypes,
+      client: this.client,
+      serializer: this._serializer,
+      getIndexForType: this.getIndexForType.bind(this),
+      incrementCounterInternal: this.incrementCounterInternal.bind(this),
+      objects,
+      options,
+    });
+    const resolvedObjects = bulkResults.map<SavedObjectsResolveResponse<T>>((result) => {
+      // extract payloads from saved object errors
+      if ((result as InternalBulkResolveError).error) {
+        const errorResult = result as InternalBulkResolveError;
+        const { type, id, error } = errorResult;
+        return {
+          saved_object: ({ type, id, error: errorContent(error) } as unknown) as SavedObject<T>,
+          outcome: 'exactMatch',
+        };
+      }
+      return result as SavedObjectsResolveResponse<T>;
+    });
+    return { resolved_objects: resolvedObjects };


I don't really like the exactMatch outcome for errors, as it imho doesn't make a lot of sense. I would have expected something like error or unknown.

But that's probably not that significant, so if you think this is better and easier to handle for the client-side consumers, I' fine keeping it that way. We probably want to document this behavior in the API's tsdoc though (at least on the server-side).

pgayvallet

Approving a LGTM is always better.

legrego

Haven't tested locally yet, will do so before approving. No red flags, just some nits and questions from me below

src/core/public/saved_objects/saved_objects_client.ts

src/core/server/saved_objects/service/saved_objects_client.ts

src/core/server/saved_objects/service/lib/repository.ts

legrego · 2021-09-15T15:51:07Z

src/core/server/saved_objects/service/lib/internal_utils.ts

+ * @internal
+ */
+export function getCurrentTime() {
+  return new Date(Date.now()).toISOString();


The old implementation was return new Date().toISOString();. What is the reason for this change?

The only reason is so that I can easily mock the output in unit tests.

x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts

x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts

legrego · 2021-09-15T16:07:50Z

x-pack/test/saved_object_api_integration/common/suites/bulk_resolve.ts

+            expect(resolvedObject.alias_target_id).to.eql(undefined);
+          }
+        }
+        // TODO: add assertions for redacted namespaces (this already exists in the bulkCreate test suite)


Is this something we want to do in this PR, or should we open an issue to track this work? If the latter, can we add a link to the issue here?

Good point, I'll open an issue for follow-on work and link it here.

Created an issue: #112455

Unrelated to this PR but I noticed that this needed to be changed.

legrego

LGTM on green CI!

kibanamachine · 2021-09-16T21:12:15Z

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`core`	1006	1014	+8

Page load bundle

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

id	before	after	diff
`core`	408.1KB	409.6KB	+1.5KB

Unknown metric groups

API count

id	before	after	diff
`core`	2265	2283	+18

References to deprecated APIs

id	before	after	diff
`security`	60	61	+1

History

💚 Build #153337 succeeded cc26c0a
💔 Build #153320 failed ba1b64b1331e4a9e369cb203b669f7080fa78139
💔 Build #153031 failed 2191f9b5e08e329e7039b9e00b202dba58d35101

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

kibanamachine · 2021-09-16T21:15:22Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

* Add SavedObjectsClient.bulkResolve (#112025) * Fix type error Co-authored-by: Joe Portner <[email protected]>

jportner added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.16.0 labels Sep 13, 2021

jportner added 4 commits September 14, 2021 15:57

Refactor some internal utils out of the SOR

0b6fc98

Fix jest integration tests

9a85fc9

Add bulkResolve API

cc26c0a

jportner force-pushed the issue-111201-add-bulkresolve branch from ba1b64b to cc26c0a Compare September 14, 2021 19:58

jportner commented Sep 14, 2021

View reviewed changes

jportner marked this pull request as ready for review September 14, 2021 22:16

jportner requested review from a team as code owners September 14, 2021 22:16

pgayvallet reviewed Sep 15, 2021

View reviewed changes

pgayvallet approved these changes Sep 15, 2021

View reviewed changes

legrego reviewed Sep 15, 2021

View reviewed changes

jportner added 6 commits September 15, 2021 18:40

Merge branch 'master' into issue-111201-add-bulkresolve

244022b

Abstract out discriminated union types and type guards

bbea46f

Remove redundant TSDocs from spaces SOC wrapper

6579f70

Address remaining review feedback

f09ae54

Update docs

d09765c

Update sharing saved objects dev docs

db3aee5

Unrelated to this PR but I noticed that this needed to be changed.

jportner requested a review from legrego September 16, 2021 19:37

legrego approved these changes Sep 16, 2021

View reviewed changes

jportner enabled auto-merge (squash) September 16, 2021 20:37

jportner added the auto-backport Deprecated - use backport:version if exact versions are needed label Sep 16, 2021

jportner merged commit 1ca8771 into elastic:master Sep 16, 2021

kibanamachine mentioned this pull request Sep 16, 2021

[7.x] Add SavedObjectsClient.bulkResolve (#112025) #112486

Merged

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 16, 2021

Add SavedObjectsClient.bulkResolve (elastic#112025)

0cbc524

jportner deleted the issue-111201-add-bulkresolve branch September 16, 2021 21:28

kibanamachine added a commit that referenced this pull request Sep 16, 2021

[7.x] Add SavedObjectsClient.bulkResolve (#112025) (#112486)

b34f986

* Add SavedObjectsClient.bulkResolve (#112025) * Fix type error Co-authored-by: Joe Portner <[email protected]>

dmlemeshko mentioned this pull request Sep 17, 2021

update dashboard methods to have bulk_resolve elastic/kibana-load-testing#168

Merged

2 tasks

This was referenced Sep 27, 2021

Public SavedObjectsClient.resolve fails for "aliasMatch" outcome #113160

Closed

Public SavedObjectsClient.resolve fails when batching sometimes #113791

Closed

rudolf mentioned this pull request Feb 13, 2023

Data views usage collection causes large amount of Elasticsearch queries #151064

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SavedObjectsClient.bulkResolve #112025

Add SavedObjectsClient.bulkResolve #112025

jportner commented Sep 13, 2021 •

edited

Loading

jportner left a comment

jportner Sep 14, 2021 •

edited

Loading

pgayvallet Sep 15, 2021

jportner Sep 15, 2021

pgayvallet Sep 16, 2021

jportner Sep 14, 2021

jportner Sep 14, 2021 •

edited

Loading

pgayvallet Sep 15, 2021

jportner Sep 14, 2021

pgayvallet commented Sep 15, 2021

pgayvallet left a comment •

edited

Loading

pgayvallet Sep 15, 2021

pgayvallet Sep 15, 2021

pgayvallet Sep 15, 2021

pgayvallet left a comment

legrego left a comment

legrego Sep 15, 2021

jportner Sep 15, 2021

legrego Sep 15, 2021

jportner Sep 15, 2021

jportner Sep 16, 2021

legrego left a comment

kibanamachine commented Sep 16, 2021

API count

References to deprecated APIs

kibanamachine commented Sep 16, 2021

		{ ...CASES.HIDDEN, ...fail404() },
		{ ...CASES.HIDDEN, ...fail400() },

Add SavedObjectsClient.bulkResolve #112025

Add SavedObjectsClient.bulkResolve #112025

Conversation

jportner commented Sep 13, 2021 • edited Loading

Testing

Notes

jportner left a comment

Choose a reason for hiding this comment

jportner Sep 14, 2021 • edited Loading

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

jportner Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet commented Sep 15, 2021

pgayvallet left a comment • edited Loading

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

pgayvallet left a comment

Choose a reason for hiding this comment

legrego 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

legrego left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 16, 2021

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Page load bundle

API count

References to deprecated APIs

History

kibanamachine commented Sep 16, 2021

💚 Backport successful

jportner commented Sep 13, 2021 •

edited

Loading

jportner Sep 14, 2021 •

edited

Loading

jportner Sep 14, 2021 •

edited

Loading

pgayvallet left a comment •

edited

Loading