[Files] Adds bulk get method #155636

vadimkibana · 2023-04-24T14:52:09Z

Summary

Closes #155369

Checklist

Delete any items that are not applicable to this PR.

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 · 2023-04-24T14:53:36Z

Pinging @elastic/appex-sharedux (Team:SharedUX)

…-fix'

vadimkibana · 2023-04-25T08:30:21Z

@elasticmachine merge upstream

jonathan-buttner · 2023-04-25T18:32:24Z

src/plugins/files/server/file_client/file_metadata_client/adapters/es_index.ts

@@ -120,6 +121,10 @@ export class EsIndexFilesMetadataClient<M = unknown> implements FileMetadataClie
    };
  }

+  async bulkGet({ ids }: BulkGetArg): Promise<FileDescriptor[]> {
+    return await Promise.all(ids.map((id) => this.get({ id })));


I don't believe cases will be using this adapter but could this use elasticsearch's mget API?

If that's not an option, we've used p-map in cases to limit the number of concurrent requests: https://github.com/sindresorhus/p-map

Good point. I've added p-limit in 0a65355

jonathan-buttner · 2023-04-25T18:39:47Z

src/plugins/files/server/file_service/internal_file_service.ts

+      const metadatas = await this.metadataClient.bulkGet({ ids });
+      const result = metadatas.map(({ id, metadata }) => {
+        if (metadata.Status === 'DELETED') {
+          throw new FileNotFoundError('File has been deleted');


I don't think this will provide the functionality we need in cases. Cases is intentionally doing a promise settled to allow for some files to not exist but still not have the entire api call throw an error.

https://github.com/elastic/kibana/blob/main/x-pack/plugins/cases/server/client/attachments/bulk_delete.ts#L137

This way we can continue with our deletion logic even when some files don't exist. Could we still return normally here but provide a way to see which ids failed and which succeeded?

Good points 👍
In c1030a0 I've added 2 options:

throwIfNotFound - If set to false then null is returned

format ("array" | "map") - If set to map then a map of id/File (or null if throwIfNotFound is false) is returned

it('returns the files under a map of id/File', async () => { const file1 = await createDisposableFile({ fileKind, name: 'test' }); const unknownID = 'foo'; const myFiles = await fileService.bulkGetById({ ids: [file1.id, unknownID], throwIfNotFound: false, // do not throw format: 'map', // returns a map }); expect(myFiles[file1?.id]?.id).toBe(file1?.id); // true expect(myFiles[unknownID]).toBe(null); // true });

would that work?

jonathan-buttner · 2023-04-25T18:40:43Z

src/plugins/files/server/file_service/internal_file_service.ts

+      });
+      return result;
+    } catch (e) {
+      if (SavedObjectsErrorHelpers.isNotFoundError(e)) {


Will the saved object client throw an error if one of the files does not exist? I think it will just mark that item in the array with the error field.

Yes you are right, that's how it is returned from the saved object bulkGet

if (docNotFound) { return { id, type, error: errorContent(SavedObjectsErrorHelpers.createGenericNotFoundError(type, id)), } as any as SavedObject<T>; }

In b97a0f1 I removed this condition and made sure we return null in case of an error when fetching one of the saved object.

sebelga

Great job @vadimkibana ! I made some changes based on @jonathan-buttner feedback. Jonathan can you have another look and let me know if the new options would help for cases? Cheers 👍

sebelga · 2023-04-26T13:17:06Z

src/plugins/files/server/file_client/file_metadata_client/adapters/es_index.ts

@@ -120,6 +121,10 @@ export class EsIndexFilesMetadataClient<M = unknown> implements FileMetadataClie
    };
  }

+  async bulkGet({ ids }: BulkGetArg): Promise<FileDescriptor[]> {
+    return await Promise.all(ids.map((id) => this.get({ id })));


Good point. I've added p-limit in 0a65355

sebelga · 2023-04-26T14:32:05Z

src/plugins/files/server/file_service/internal_file_service.ts

+      const metadatas = await this.metadataClient.bulkGet({ ids });
+      const result = metadatas.map(({ id, metadata }) => {
+        if (metadata.Status === 'DELETED') {
+          throw new FileNotFoundError('File has been deleted');


Good points 👍
In c1030a0 I've added 2 options:

throwIfNotFound - If set to false then null is returned

format ("array" | "map") - If set to map then a map of id/File (or null if throwIfNotFound is false) is returned

it('returns the files under a map of id/File', async () => { const file1 = await createDisposableFile({ fileKind, name: 'test' }); const unknownID = 'foo'; const myFiles = await fileService.bulkGetById({ ids: [file1.id, unknownID], throwIfNotFound: false, // do not throw format: 'map', // returns a map }); expect(myFiles[file1?.id]?.id).toBe(file1?.id); // true expect(myFiles[unknownID]).toBe(null); // true });

would that work?

sebelga · 2023-04-26T15:16:51Z

src/plugins/files/server/file_service/internal_file_service.ts

+      });
+      return result;
+    } catch (e) {
+      if (SavedObjectsErrorHelpers.isNotFoundError(e)) {


Yes you are right, that's how it is returned from the saved object bulkGet

if (docNotFound) { return { id, type, error: errorContent(SavedObjectsErrorHelpers.createGenericNotFoundError(type, id)), } as any as SavedObject<T>; }

In b97a0f1 I removed this condition and made sure we return null in case of an error when fetching one of the saved object.

jonathan-buttner

Thanks for the changes!

kibana-ci · 2023-04-27T13:23:21Z

💚 Build Succeeded

Buildkite Build
Commit: d3268fd

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
`files`	10	23	+13

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`files`	6	8	+2

Unknown metric groups

API count

id	before	after	diff
`files`	219	235	+16

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	17	19	+2
`securitySolution`	399	402	+3
total			+5

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	18	20	+2
`securitySolution`	479	482	+3
total			+5

History

💔 Build #123548 failed 1098547
💔 Build #123514 failed c1030a0
💚 Build #123470 succeeded 0a65355
💚 Build #122822 succeeded 8369cda
💔 Build #122527 failed 75f4444

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

sebelga

Approving!

## Summary Closes elastic#155369 ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### For maintainers - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit a0cd724)

kibanamachine · 2023-04-27T13:31:03Z

💚 All backports created successfully

Status	Branch	Result
✅	8.8

Note: Successful backport PRs will be merged automatically after passing CI.