Skip to content

Commit

Permalink
SavedObjectsRepository code cleanup (#157154)
Browse files Browse the repository at this point in the history
## 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:

```ts
  /**
   * {@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.

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
  • Loading branch information
pgayvallet and kibanamachine committed May 11, 2023
1 parent b70496e commit 3b6b7ad
Show file tree
Hide file tree
Showing 63 changed files with 4,291 additions and 2,801 deletions.
Original file line number Diff line number Diff line change
@@ -1,3 +1,31 @@
# @kbn/core-saved-objects-api-server-internal

This package contains the internal implementation of core's server-side savedObjects client and repository.

## Structure of the package

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

### lib/apis/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).

### lib/apis/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.

### lib/apis/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.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,313 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import type { Payload } from '@hapi/boom';
import {
SavedObjectsErrorHelpers,
type SavedObject,
type SavedObjectSanitizedDoc,
DecoratedError,
AuthorizeCreateObject,
SavedObjectsRawDoc,
} from '@kbn/core-saved-objects-server';
import { SavedObjectsUtils } from '@kbn/core-saved-objects-utils-server';
import {
SavedObjectsCreateOptions,
SavedObjectsBulkCreateObject,
SavedObjectsBulkResponse,
} from '@kbn/core-saved-objects-api-server';
import { DEFAULT_REFRESH_SETTING } from '../constants';
import {
Either,
getBulkOperationError,
getCurrentTime,
getExpectedVersionProperties,
left,
right,
isLeft,
isRight,
normalizeNamespace,
setManaged,
errorContent,
} from './utils';
import { getSavedObjectNamespaces } from './utils';
import { PreflightCheckForCreateObject } from './internals/preflight_check_for_create';
import { ApiExecutionContext } from './types';

export interface PerformBulkCreateParams<T = unknown> {
objects: Array<SavedObjectsBulkCreateObject<T>>;
options: SavedObjectsCreateOptions;
}

type ExpectedResult = Either<
{ type: string; id?: string; error: Payload },
{
method: 'index' | 'create';
object: SavedObjectsBulkCreateObject & { id: string };
preflightCheckIndex?: number;
}
>;

export const performBulkCreate = async <T>(
{ objects, options }: PerformBulkCreateParams<T>,
{
registry,
helpers,
allowedTypes,
client,
serializer,
migrator,
extensions = {},
}: ApiExecutionContext
): Promise<SavedObjectsBulkResponse<T>> => {
const {
common: commonHelper,
validation: validationHelper,
encryption: encryptionHelper,
preflight: preflightHelper,
serializer: serializerHelper,
} = helpers;
const { securityExtension } = extensions;
const namespace = commonHelper.getCurrentNamespace(options.namespace);

const {
migrationVersionCompatibility,
overwrite = false,
refresh = DEFAULT_REFRESH_SETTING,
managed: optionsManaged,
} = options;
const time = getCurrentTime();

let preflightCheckIndexCounter = 0;
const expectedResults = objects.map<ExpectedResult>((object) => {
const { type, id: requestId, initialNamespaces, version, managed } = object;
let error: DecoratedError | undefined;
let id: string = ''; // Assign to make TS happy, the ID will be validated (or randomly generated if needed) during getValidId below
const objectManaged = managed;
if (!allowedTypes.includes(type)) {
error = SavedObjectsErrorHelpers.createUnsupportedTypeError(type);
} else {
try {
id = commonHelper.getValidId(type, requestId, version, overwrite);
validationHelper.validateInitialNamespaces(type, initialNamespaces);
validationHelper.validateOriginId(type, object);
} catch (e) {
error = e;
}
}

if (error) {
return left({ id: requestId, type, error: errorContent(error) });
}

const method = requestId && overwrite ? 'index' : 'create';
const requiresNamespacesCheck = requestId && registry.isMultiNamespace(type);

return right({
method,
object: {
...object,
id,
managed: setManaged({ optionsManaged, objectManaged }),
},
...(requiresNamespacesCheck && { preflightCheckIndex: preflightCheckIndexCounter++ }),
}) as ExpectedResult;
});

const validObjects = expectedResults.filter(isRight);
if (validObjects.length === 0) {
// We only have error results; return early to avoid potentially trying authZ checks for 0 types which would result in an exception.
return {
// Technically the returned array should only contain SavedObject results, but for errors this is not true (we cast to 'unknown' below)
saved_objects: expectedResults.map<SavedObject<T>>(
({ value }) => value as unknown as SavedObject<T>
),
};
}

const namespaceString = SavedObjectsUtils.namespaceIdToString(namespace);
const preflightCheckObjects = validObjects
.filter(({ value }) => value.preflightCheckIndex !== undefined)
.map<PreflightCheckForCreateObject>(({ value }) => {
const { type, id, initialNamespaces } = value.object;
const namespaces = initialNamespaces ?? [namespaceString];
return { type, id, overwrite, namespaces };
});
const preflightCheckResponse = await preflightHelper.preflightCheckForCreate(
preflightCheckObjects
);

const authObjects: AuthorizeCreateObject[] = validObjects.map((element) => {
const { object, preflightCheckIndex: index } = element.value;
const preflightResult = index !== undefined ? preflightCheckResponse[index] : undefined;
return {
type: object.type,
id: object.id,
initialNamespaces: object.initialNamespaces,
existingNamespaces: preflightResult?.existingDocument?._source.namespaces ?? [],
};
});

const authorizationResult = await securityExtension?.authorizeBulkCreate({
namespace,
objects: authObjects,
});

let bulkRequestIndexCounter = 0;
const bulkCreateParams: object[] = [];
type ExpectedBulkResult = Either<
{ type: string; id?: string; error: Payload },
{ esRequestIndex: number; requestedId: string; rawMigratedDoc: SavedObjectsRawDoc }
>;
const expectedBulkResults = await Promise.all(
expectedResults.map<Promise<ExpectedBulkResult>>(async (expectedBulkGetResult) => {
if (isLeft(expectedBulkGetResult)) {
return expectedBulkGetResult;
}

let savedObjectNamespace: string | undefined;
let savedObjectNamespaces: string[] | undefined;
let existingOriginId: string | undefined;
let versionProperties;
const {
preflightCheckIndex,
object: { initialNamespaces, version, ...object },
method,
} = expectedBulkGetResult.value;
if (preflightCheckIndex !== undefined) {
const preflightResult = preflightCheckResponse[preflightCheckIndex];
const { type, id, existingDocument, error } = preflightResult;
if (error) {
const { metadata } = error;
return left({
id,
type,
error: {
...errorContent(SavedObjectsErrorHelpers.createConflictError(type, id)),
...(metadata && { metadata }),
},
});
}
savedObjectNamespaces =
initialNamespaces || getSavedObjectNamespaces(namespace, existingDocument);
versionProperties = getExpectedVersionProperties(version);
existingOriginId = existingDocument?._source?.originId;
} else {
if (registry.isSingleNamespace(object.type)) {
savedObjectNamespace = initialNamespaces
? normalizeNamespace(initialNamespaces[0])
: namespace;
} else if (registry.isMultiNamespace(object.type)) {
savedObjectNamespaces = initialNamespaces || getSavedObjectNamespaces(namespace);
}
versionProperties = getExpectedVersionProperties(version);
}

// 1. If the originId has been *explicitly set* in the options (defined or undefined), respect that.
// 2. Otherwise, preserve the originId of the existing object that is being overwritten, if any.
const originId = Object.keys(object).includes('originId')
? object.originId
: existingOriginId;
const migrated = migrator.migrateDocument({
id: object.id,
type: object.type,
attributes: await encryptionHelper.optionallyEncryptAttributes(
object.type,
object.id,
savedObjectNamespace, // only used for multi-namespace object types
object.attributes
),
migrationVersion: object.migrationVersion,
coreMigrationVersion: object.coreMigrationVersion,
typeMigrationVersion: object.typeMigrationVersion,
...(savedObjectNamespace && { namespace: savedObjectNamespace }),
...(savedObjectNamespaces && { namespaces: savedObjectNamespaces }),
managed: setManaged({ optionsManaged, objectManaged: object.managed }),
updated_at: time,
created_at: time,
references: object.references || [],
originId,
}) as SavedObjectSanitizedDoc<T>;

/**
* If a validation has been registered for this type, we run it against the migrated attributes.
* This is an imperfect solution because malformed attributes could have already caused the
* migration to fail, but it's the best we can do without devising a way to run validations
* inside the migration algorithm itself.
*/
try {
validationHelper.validateObjectForCreate(object.type, migrated);
} catch (error) {
return left({
id: object.id,
type: object.type,
error,
});
}

const expectedResult = {
esRequestIndex: bulkRequestIndexCounter++,
requestedId: object.id,
rawMigratedDoc: serializer.savedObjectToRaw(migrated),
};

bulkCreateParams.push(
{
[method]: {
_id: expectedResult.rawMigratedDoc._id,
_index: commonHelper.getIndexForType(object.type),
...(overwrite && versionProperties),
},
},
expectedResult.rawMigratedDoc._source
);

return right(expectedResult);
})
);

const bulkResponse = bulkCreateParams.length
? await client.bulk({
refresh,
require_alias: true,
body: bulkCreateParams,
})
: undefined;

const result = {
saved_objects: expectedBulkResults.map((expectedResult) => {
if (isLeft(expectedResult)) {
return expectedResult.value as any;
}

const { requestedId, rawMigratedDoc, esRequestIndex } = expectedResult.value;
const rawResponse = Object.values(bulkResponse?.items[esRequestIndex] ?? {})[0] as any;

const error = getBulkOperationError(rawMigratedDoc._source.type, requestedId, rawResponse);
if (error) {
return { type: rawMigratedDoc._source.type, id: requestedId, error };
}

// When method == 'index' the bulkResponse doesn't include the indexed
// _source so we return rawMigratedDoc but have to spread the latest
// _seq_no and _primary_term values from the rawResponse.
return serializerHelper.rawToSavedObject(
{
...rawMigratedDoc,
...{ _seq_no: rawResponse._seq_no, _primary_term: rawResponse._primary_term },
},
{ migrationVersionCompatibility }
);
}),
};
return encryptionHelper.optionallyDecryptAndRedactBulkResult(
result,
authorizationResult?.typeMap,
objects
);
};
Loading

0 comments on commit 3b6b7ad

Please sign in to comment.