SavedObjectsRepository code cleanup #157154

pgayvallet · 2023-05-09T12:45:26Z

Summary

Structural cleanup of the SavedObjectsRepository code, by extracting the actual implementation of each API to their individual file (as it was initiated for some by Joe a while ago, e.g updateObjectsSpaces)

Why doing that, and why now?

I remember discussing about this extraction with Joe for the first time like, what, almost 3 years ago? The 2.5k line SOR is a beast, and the only reason we did not refactor that yet is because of (the lack of) priorization of tech debt (and lack of courage, probably).

So, why now? Well, with the changes we're planning to perform to the SOR code for serverless, I thought that doing a bit of cleanup beforehand was probably a wise thing. So I took this on-week time to tackle this (I know, so much for an on-week project, right?)

API extraction

All of these APIs in the SOR class now look like:

  /**
   * {@inheritDoc ISavedObjectsRepository.create}
   */
  public async create<T = unknown>(
    type: string,
    attributes: T,
    options: SavedObjectsCreateOptions = {}
  ): Promise<SavedObject<T>> {
    return await performCreate(
      {
        type,
        attributes,
        options,
      },
      this.apiExecutionContext
    );
  }

This separation allows a better isolation, testability, readability and therefore maintainability overall.

Structure

@kbn/core-saved-objects-api-server-internal
  - /src/lib
    - repository.ts
    - /apis
      - create.ts
      - delete.ts
      - ....
      - /helpers
      - /utils
      - /internals

There was a massive amount of helpers, utilities and such, both as internal functions on the SOR, and as external utilities. Some being stateless, some requiring access to parts of the SOR to perform calls...

I introduced 3 concepts here, as you can see on the structure:

utils

Base utility functions, receiving (mostly) parameters from a given API call's option (e.g the type or id of a document, but not the type registry).

helpers

'Stateful' helpers. These helpers were mostly here to receive the utility functions that were extracted from the SOR. They are instantiated with the SOR's context (e.g type registry, mappings and so on), to avoid the caller to such helpers to have to pass all the parameters again.

internals

I would call them 'utilities with business logic'. These are the 'big' chunks of logic called by the APIs. E.g preflightCheckForCreate, internalBulkResolve and so on.

Note that given the legacy of the code, the frontier between those concept is quite thin sometimes, but I wanted to regroups things a bit, and also I aimed at increasing the developer experience by avoiding to call methods with 99 parameters (which is why the helpers were created).

Tests

Test coverage was not altered by this PR. The base repository tests (packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts) and the extension tests (packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.{ext}_extension.test.ts) were remain untouched. These tests are performing 'almost unmocked' tests, somewhat close to integration tests, so it would probably be worth keeping them.

The new structure allow more low-level, unitary testing of the individual APIs. I did NOT add proper unit test coverage for the extracted APIs, as the amount of work it represent is way more significant than the refactor itself (and, once again, the existing coverage still applies / function here).

The testing utilities and mocks were added in the PR though, and an example of what the per-API unit test could look like was also added (packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/remove_references_to.test.ts).

Overall, I think it of course would be beneficial to add the missing unit test coverage, but given the amount of work it represent, and the fact that the code is already tested by the repository test and the (quite exhaustive) FTR test suites, I don't think it's worth the effort right now given our other priorities.

…fix'

elasticmachine · 2023-05-10T09:59:28Z

Pinging @elastic/kibana-core (Team:Core)

pgayvallet

Self-review

pgayvallet · 2023-05-10T11:39:41Z

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/find.ts

+export const performFind = async <T = unknown, A = unknown>(
+  { options, internalOptions }: PerformFindParams,
+  {
+    registry,
+    helpers,
+    allowedTypes: rawAllowedTypes,
+    mappings,
+    client,


All the APIs were extracted without any behavioural change. Only modifications were to fix the calls to the helper methods and/or access to the SOR properties (now passed via the context)

pgayvallet · 2023-05-10T11:42:57Z

...ved-objects/core-saved-objects-api-server-internal/src/lib/apis/remove_references_to.test.ts

+describe('performRemoveReferencesTo', () => {
+  const namespace = 'some_ns';
+  const indices = ['.kib_1', '.kib_2'];
+  let apiExecutionContext: ApiExecutionContextMock;
+
+  beforeEach(() => {
+    apiExecutionContext = apiContextMock.create();
+    apiExecutionContext.registry.registerType(fooType);
+    apiExecutionContext.registry.registerType(barType);


As explained in the issue's description, I did not add full test coverage for all the newly created files, as the repository tests are still properly covering them (in addition to the FTR tests).

The mocks and tools are ready though, as shown by this test file

pgayvallet · 2023-05-10T11:46:24Z

...ges/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/common.ts

+export class CommonHelper {
+  private registry: ISavedObjectTypeRegistry;
+  private spaceExtension?: ISavedObjectsSpacesExtension;
+  private encryptionExtension?: ISavedObjectsEncryptionExtension;
+  private defaultIndex: string;
+  private kibanaVersion: string;
+
+  public readonly createPointInTimeFinder: CreatePointInTimeFinderFn;
+
+  constructor({
+    registry,
+    createPointInTimeFinder,


The 'helpers' passed down to all API execution handlers. As explained in the issue's description, those are mostly used to replace calls to internal SOR helpers, and to avoid calls from the API handlers to pass the full list of parameters (e.g registry extensions and so on) by keeping reference of those dependencies in the helpers.

pgayvallet · 2023-05-10T11:56:15Z

...ages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/index.ts

+import type { ICommonHelper } from './common';
+import type { IEncryptionHelper } from './encryption';
+import type { IValidationHelper } from './validation';
+import type { IPreflightCheckHelper } from './preflight_check';
+import type { ISerializerHelper } from './serializer';


NIT: I was forced to add these interfaces for the mocked versions (stupid TS still considering that private attributes are part of an instance's type)

Interesting, how did this manifest as an issue? Were you able to access private members in the repository functions?

TS complained because the mocked version (that only implements the methods) could not be coerced as the concrete type because it was missing the private properties (when using the mocked api internal context in tests)

pgayvallet · 2023-05-10T12:01:50Z

...jects/core-saved-objects-api-server-internal/src/lib/apis/internals/internal_bulk_resolve.ts

  client: RepositoryEsClient;
-  serializer: SavedObjectsSerializer;
+  serializer: ISavedObjectsSerializer;


NIT: lot of methods were using the concrete type instead of the interface. Fixed all those usages

pgayvallet · 2023-05-10T12:08:27Z

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/constants.ts

+export const DEFAULT_REFRESH_SETTING = 'wait_for';
+export const DEFAULT_RETRY_COUNT = 3;
+export const MAX_CONCURRENT_ALIAS_DELETIONS = 10;


NIT: extracted the constants that were living in the repository.ts file.

pgayvallet · 2023-05-10T12:09:09Z

...ges/core/saved-objects/core-saved-objects-api-server-internal/src/lib/priority_collection.ts

This file was plain unused. Not even sure for how long it has been a dead file.

The code was implemented during Spaces Phase 1 and the last implementation code change was ~ 3 years ago. I guess it became redundant since then.

pgayvallet · 2023-05-10T12:10:23Z

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts

+    return await performCheckConflicts(
+      {
+        objects,
+        options,
+      },
+      this.apiExecutionContext
+    );


Look at this beauty

pgayvallet · 2023-05-10T12:12:08Z

...ages/core/saved-objects/core-saved-objects-api-server-internal/src/mocks/api_context.mock.ts

+const createApiExecutionContextMock = (): ApiExecutionContextMock => {
+  return {
+    registry: new SavedObjectTypeRegistry(),
+    helpers: apiHelperMocks.create(),
+    extensions: savedObjectsExtensionsMock.create(),
+    client: elasticsearchClientMock.createElasticsearchClient(),


The ApiExecutionContext mock sure was something...

pgayvallet · 2023-05-10T12:15:26Z

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

@@ -11,7 +11,7 @@ import type {
  SavedObjectsResolveResponse,
 } from '@kbn/core-saved-objects-api-server';
 import type { SavedObjectsClient } from '@kbn/core-saved-objects-api-server-internal';
-import { isBulkResolveError } from '@kbn/core-saved-objects-api-server-internal/src/lib/internal_bulk_resolve';
+import { isBulkResolveError } from '@kbn/core-saved-objects-api-server-internal/src/lib/apis/internals/internal_bulk_resolve';


@elastic/kibana-security 🙈 nested import for internal core package.

jloleysens

Great work @pgayvallet !

I read this in my local editor and I really like how these functions are broken out. There is a lot of commonality in structure for this actions (namespaces, preflight, auth check, do the thing then encrypt/redact) but the way they are now still affords a lot of flexibility to tweak things over time.

The structure of utils and helpers makes sense off the bat, but I had to spend a bit of time thinking about how internals fits in -- like when would I choose to put something there? Then I saw the commonality of using the client and how these are shared extensions of the clients functionality + business logic. This is just my paraphrasing of what you said. Overall I like it!

Left a few non-blocker comments, approving to unblock progress 🚢

jloleysens · 2023-05-10T14:19:01Z

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/bulk_create.ts

+      return {
+        tag: 'Left',
+        value: { id: requestId, type, error: errorContent(error) },
+      };


nit: WDYT about a util left(v: L): Left<L> and right(v: R): Right<R>? Could remove some boilerplate. This would become: return left({ id... })

Yeah, we should add this to our own implementation of either. will take a shot in current PR

done in 32f9866

jloleysens · 2023-05-10T14:24:25Z

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/bulk_create.ts

This function still seems like such a beast 😄

Yeah. I was planning on using the remaining time this week trying to do some quick wins simplifying some APIs

jloleysens · 2023-05-10T15:07:47Z

...ages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/index.ts

+import type { ICommonHelper } from './common';
+import type { IEncryptionHelper } from './encryption';
+import type { IValidationHelper } from './validation';
+import type { IPreflightCheckHelper } from './preflight_check';
+import type { ISerializerHelper } from './serializer';


Interesting, how did this manifest as an issue? Were you able to access private members in the repository functions?

jloleysens · 2023-05-10T15:12:19Z

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/index.ts

jloleysens · 2023-05-10T15:15:20Z

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/types.ts

+export interface ApiExecutionContext {
+  registry: ISavedObjectTypeRegistry;
+  helpers: RepositoryHelpers;
+  extensions: SavedObjectsExtensions;
+  client: RepositoryEsClient;
+  allowedTypes: string[];
+  serializer: ISavedObjectsSerializer;
+  migrator: IKibanaMigrator;
+  logger: Logger;
+  mappings: IndexMapping;
+}


How about ApiInternalContext with a doc comment 😉

jloleysens · 2023-05-10T15:21:46Z

...ges/core/saved-objects/core-saved-objects-api-server-internal/src/lib/priority_collection.ts

jloleysens · 2023-05-10T15:24:22Z

...re/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/utils/internal_utils.ts

Could/should we move this file + test to ../internals/utils.ts + ../internals/utils.test.ts?

On second thought, that does not really fit the current pattern... Just thought seeing "internal" again was a bit confusing.

I just kept the old file name tbh, it's not only used by the /api/internals folder...

legrego · 2023-05-10T16:59:09Z

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

@@ -11,7 +11,7 @@ import type {
  SavedObjectsResolveResponse,
 } from '@kbn/core-saved-objects-api-server';
 import type { SavedObjectsClient } from '@kbn/core-saved-objects-api-server-internal';
-import { isBulkResolveError } from '@kbn/core-saved-objects-api-server-internal/src/lib/internal_bulk_resolve';
+import { isBulkResolveError } from '@kbn/core-saved-objects-api-server-internal/src/lib/apis/internals/internal_bulk_resolve';


TinaHeiligers · 2023-05-10T20:01:22Z

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts

-    await pMap(objectsToDeleteAliasesFor, mapper, { concurrency: MAX_CONCURRENT_ALIAS_DELETIONS });
-
-    return { statuses: [...savedObjects] };
+    return await performBulkDelete(


TinaHeiligers

Love it!

TinaHeiligers · 2023-05-10T20:12:31Z

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts

-// BEWARE: The SavedObjectClient depends on the implementation details of the SavedObjectsRepository
-// so any breaking changes to this repository are considered breaking changes to the SavedObjectsClient.
+  type ApiExecutionContext,
+  performCreate,


I love the consistency!

TinaHeiligers · 2023-05-10T20:24:52Z

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/index.ts

+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+


WDYT about adding a descriptor of what each folder contains as part of the whole repository code organization?
We could use the README as a start.

A copy-paste version of the PR description would work!

TinaHeiligers · 2023-05-10T20:50:35Z

...ges/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/helpers/common.ts

+
+export type ICommonHelper = PublicMethodsOf<CommonHelper>;
+
+export class CommonHelper {


WDYT about ApiExecutionContextGetters? Most of the helpers are getters.

Naming is hard... Not sure these methods are strictly speaking 'getters' given they all hold some internal logic.

TinaHeiligers · 2023-05-10T21:16:59Z

...ects/core-saved-objects-api-server-internal/src/lib/repository_bulk_delete_internal_types.ts

@@ -12,7 +12,7 @@ import type {
  ErrorCause,
 } from '@elastic/elasticsearch/lib/api/types';
 import type { estypes, TransportResult } from '@elastic/elasticsearch';
-import type { Either } from './internal_utils';
+import type { Either } from './apis/utils';
 import type { DeleteLegacyUrlAliasesParams } from './legacy_url_aliases';

 /**


With the new structure, these should probably move somewhere else. Now that the "rules" are less strict, we could move them to core-saved-objects-api-server or to base.
Alternatively, just add them to the file that needs them.

Yeah, I agree that our Either utils could be moved elsewhere. Although it's only used within the api-server-internal package (and will likely stay that way given we're using fp-ts in other parts of the code like the migration).

Ihmo the next step would probably be to delete those and using fp-ts too.

For the other utils thats harder. Utils are always a mess. Some are using only by one file, some are used by multiple APIs....

kibana-ci · 2023-05-11T07:25:28Z

💚 Build Succeeded

Buildkite Build
Commit: 69bab51

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
`@kbn/core-saved-objects-api-server-internal`	55	0	-55

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-saved-objects-api-server-internal`	77	0	-77

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	400	404	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	480	484	+4
total			+6

History

💚 Build #126827 succeeded 37ccab3
💚 Build #126794 succeeded 5eac343
💛 Build #126777 was flaky 48a1be0
💔 Build #126763 failed b1e3b3ef5b74d5fa6aff4787acef81d8d192d6e8
💔 Build #126760 failed b63a1b9
💚 Build #126676 succeeded ba8563a

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

## Summary Follow-up of #157154 Continue the cleanup started in the previous PR, by moving more things around. - Move everything search-related (dsl, aggregations, kql utils) to a dedicated `search` folder - Move a few more things to `/apis/internals` - Remove the 'v5' field compatibility in the field list generation (see comment) - Cleanup some files a bit. --------- Co-authored-by: kibanamachine <[email protected]>

## Summary Structural cleanup of the `SavedObjectsRepository`, continued from where #157154 left off. This PR holds changes to `repository.test.ts`, and splits the api unit tests into dedicated test files next to the api implementation. ### Tests This PR does not alter test coverage, its primary aim is to improve DevEx when it comes to SOR api unit testing. Hopefully, it'll make life easier for everyone touching the code. --------- Co-authored-by: Kibana Machine <[email protected]>

pgayvallet added 20 commits May 9, 2023 10:25

start extracting all the things

66c2db2

extract 'create'

1baed7a

extract bulk_create

3104023

extract delete

e9c8d57

extract checkConflicts

2e1add9

extracting bulkDelete

bd51cd3

extract delete by namespace

e268959

extract find

7b5a8b8

extract bulk_get

2dc4634

extract get

a2f90e8

extract update

1ed1f1c

extract bulk_update

8b550de

extract remove_references_to

cb47abb

extract open point in time

d3b338f

extract increment_counter

aeed912

implement resolve and bulk resolve

109017d

move internals

e7f1e34

delete dead code

aef0cfa

remove obsolete snapshot

91cfdc0

some more cleanup

81ea647

pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes v8.9.0 labels May 9, 2023

pgayvallet added 7 commits May 9, 2023 16:23

fix import path

ccfe1c9

fix import path

65b4bfa

fix binding

b6007d8

this was a tricky one

ba8563a

implement remaining missing methods

f640226

Merge remote-tracking branch 'upstream/main' into kbn-xxx-SOR-cleanup

b63a1b9

small repository cleanup

85a9c00

pgayvallet added 4 commits May 10, 2023 09:08

move ALL the things again

c83830d

fix constructor error

eec0fc1

add mocks and test example

2464efd

lint

7b8e966

pgayvallet force-pushed the kbn-xxx-SOR-cleanup branch from b1e3b3e to 7b8e966 Compare May 10, 2023 08:33

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

48a1be0

…fix'

pgayvallet marked this pull request as ready for review May 10, 2023 09:59

pgayvallet requested review from a team as code owners May 10, 2023 09:59

pgayvallet added 2 commits May 10, 2023 12:00

Merge remote-tracking branch 'upstream/main' into kbn-xxx-SOR-cleanup

5eac343

merge util files

37ccab3

pgayvallet commented May 10, 2023

View reviewed changes

jloleysens approved these changes May 10, 2023

View reviewed changes

legrego approved these changes May 10, 2023

View reviewed changes

TinaHeiligers reviewed May 10, 2023

View reviewed changes

TinaHeiligers approved these changes May 10, 2023

View reviewed changes

pgayvallet added 2 commits May 11, 2023 08:07

add left and right helpers

32f9866

Merge remote-tracking branch 'upstream/main' into kbn-xxx-SOR-cleanup

04e5ac4

pgayvallet added the backport:skip This commit does not require backporting label May 11, 2023

add description to the package's readme

69bab51

pgayvallet enabled auto-merge (squash) May 11, 2023 06:24

pgayvallet merged commit 3b6b7ad into elastic:main May 11, 2023

pgayvallet mentioned this pull request May 11, 2023

SavedObjectsRepository code cleanup (bis) #157363

Merged

TinaHeiligers mentioned this pull request Dec 12, 2023

Splits repository api unit tests into individual tests #173218

Merged


		export type ICommonHelper = PublicMethodsOf<CommonHelper>;

		export class CommonHelper {

SavedObjectsRepository code cleanup #157154

SavedObjectsRepository code cleanup #157154

Conversation

pgayvallet commented May 9, 2023 • edited Loading

Summary

Why doing that, and why now?

API extraction

Structure

utils

helpers

internals

Tests

elasticmachine commented May 10, 2023

pgayvallet 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

pgayvallet May 10, 2023 • 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

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

jloleysens 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

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

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

TinaHeiligers May 10, 2023 • edited Loading

Choose a reason for hiding this comment

TinaHeiligers 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

Choose a reason for hiding this comment

kibana-ci commented May 11, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

API count

ESLint disabled line counts

Total ESLint disabled count

History

pgayvallet commented May 9, 2023 •

edited

Loading

pgayvallet May 10, 2023 •

edited

Loading

jloleysens left a comment •

edited

Loading

TinaHeiligers May 10, 2023 •

edited

Loading