Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SOR: Add attr override option for update and bulkUpdate #183267

Merged
merged 5 commits into from
May 15, 2024
Merged

SOR: Add attr override option for update and bulkUpdate #183267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aa5e2bf
SOR: Add attr override option for `update` and `bulkUpdate`
pgayvallet May 13, 2024
a276821
add unit tests
pgayvallet May 14, 2024
f1cff3b
add integration tests
pgayvallet May 14, 2024
5857447
rename option
pgayvallet May 15, 2024
fe90427
Merge remote-tracking branch 'upstream/main' into kbn-183112-full-update
pgayvallet May 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 68 additions & 8 deletions ...ore/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/bulk_update.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,22 @@ describe('#bulkUpdate', () => {
const originId = 'some-origin-id';
const namespace = 'foo-namespace';

const getBulkIndexEntry = (
method: string,
{ type, id }: TypeIdTuple,
_index = expect.any(String),
getId: (type: string, id: string) => string = () => expect.any(String),
overrides: Record<string, unknown> = {}
) => {
return {
[method]: {
_index,
_id: getId(type, id),
...overrides,
},
};
};

// bulk index calls have two objects for each source -- the action, and the source
const expectClientCallArgsAction = (
objects: TypeIdTuple[],
Expand All @@ -148,14 +164,8 @@ describe('#bulkUpdate', () => {
}
) => {
const body = [];
for (const { type, id } of objects) {
body.push({
[method]: {
_index,
_id: getId(type, id),
...overrides,
},
});
for (const object of objects) {
body.push(getBulkIndexEntry(method, object, _index, getId, overrides));
body.push(expect.any(Object));
}
expect(client.bulk).toHaveBeenCalledWith(
Expand Down Expand Up @@ -220,6 +230,55 @@ describe('#bulkUpdate', () => {
expectClientCallArgsAction([obj1, _obj2], { method: 'index' });
});

it('should use the ES bulk action with the merged attributes when mergeAttributes is not false', async () => {
const _obj1 = { ...obj1, attributes: { foo: 'bar' } };
const _obj2 = { ...obj2, attributes: { hello: 'dolly' }, mergeAttributes: true };
await bulkUpdateSuccess(client, repository, registry, [_obj1, _obj2]);

expect(client.bulk).toHaveBeenCalledTimes(1);
expect(client.bulk).toHaveBeenCalledWith(
expect.objectContaining({
body: [
getBulkIndexEntry('index', _obj1),
expect.objectContaining({
[obj1.type]: {
title: 'Testing',
foo: 'bar',
},
}),
getBulkIndexEntry('index', _obj2),
expect.objectContaining({
[obj2.type]: {
title: 'Testing',
hello: 'dolly',
},
}),
],
}),
expect.any(Object)
);
});

it('should use the ES bulk action only with the provided attributes when mergeAttributes is false', async () => {
const _obj1 = { ...obj1, attributes: { hello: 'dolly' }, mergeAttributes: false };
await bulkUpdateSuccess(client, repository, registry, [_obj1]);

expect(client.bulk).toHaveBeenCalledTimes(1);
expect(client.bulk).toHaveBeenCalledWith(
expect.objectContaining({
body: [
getBulkIndexEntry('index', _obj1),
expect.objectContaining({
[obj1.type]: {
hello: 'dolly',
},
}),
],
}),
expect.any(Object)
);
});

it(`doesnt call Elasticsearch if there are no valid objects to update`, async () => {
const objects = [obj1, obj2].map((x) => ({ ...x, type: 'unknownType' }));
await repository.bulkUpdate(objects);
Expand Down Expand Up @@ -507,6 +566,7 @@ describe('#bulkUpdate', () => {
await bulkUpdateError(obj, true, expectedErrorResult);
});
});

describe('migration', () => {
it('migrates the fetched documents from Mget', async () => {
const modifiedObj2 = { ...obj2, coreMigrationVersion: '8.0.0' };
Expand Down
52 changes: 37 additions & 15 deletions ...ges/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/bulk_update.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ type ExpectedBulkGetResult = Either<
objectNamespace?: string;
esRequestIndex: number;
migrationVersionCompatibility?: 'raw' | 'compatible';
mergeAttributes: boolean;
}
>;

Expand Down Expand Up @@ -89,7 +90,15 @@ export const performBulkUpdate = async <T>(

let bulkGetRequestIndexCounter = 0;
const expectedBulkGetResults = objects.map<ExpectedBulkGetResult>((object) => {
const { type, id, attributes, references, version, namespace: objectNamespace } = object;
const {
type,
id,
attributes,
references,
version,
namespace: objectNamespace,
mergeAttributes = true,
} = object;
let error: DecoratedError | undefined;

if (!allowedTypes.includes(type)) {
Expand Down Expand Up @@ -122,6 +131,7 @@ export const performBulkUpdate = async <T>(
objectNamespace,
esRequestIndex: bulkGetRequestIndexCounter++,
migrationVersionCompatibility,
mergeAttributes,
});
});

Expand Down Expand Up @@ -205,8 +215,15 @@ export const performBulkUpdate = async <T>(
return expectedBulkGetResult;
}

const { esRequestIndex, id, type, version, documentToSave, objectNamespace } =
expectedBulkGetResult.value;
const {
esRequestIndex,
id,
type,
version,
documentToSave,
objectNamespace,
mergeAttributes,
} = expectedBulkGetResult.value;

let namespaces: string[] | undefined;
const versionProperties = getExpectedVersionProperties(version);
Expand Down Expand Up @@ -261,18 +278,23 @@ export const performBulkUpdate = async <T>(
}

const typeDefinition = registry.getType(type)!;
const updatedAttributes = mergeForUpdate({
targetAttributes: {
...(migrated!.attributes as Record<string, unknown>),
},
updatedAttributes: await encryptionHelper.optionallyEncryptAttributes(
type,
id,
objectNamespace || namespace,
documentToSave[type]
),
typeMappings: typeDefinition.mappings,
});

const encryptedUpdatedAttributes = await encryptionHelper.optionallyEncryptAttributes(
type,
id,
objectNamespace || namespace,
documentToSave[type]
);

const updatedAttributes = mergeAttributes
? mergeForUpdate({
targetAttributes: {
...(migrated!.attributes as Record<string, unknown>),
},
updatedAttributes: encryptedUpdatedAttributes,
typeMappings: typeDefinition.mappings,
})
: encryptedUpdatedAttributes;

const migratedUpdatedSavedObjectDoc = migrationHelper.migrateInputDocument({
...migrated!,
Expand Down
51 changes: 51 additions & 0 deletions ...ges/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/update.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,55 @@ describe('#update', () => {
expect(client.index).toHaveBeenCalledTimes(1);
});

it(`should use the ES index action with the merged attributes when mergeAttributes is not false`, async () => {
migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));

await updateSuccess(client, repository, registry, NAMESPACE_AGNOSTIC_TYPE, id, {
foo: 'bar',
});

expect(client.index).toHaveBeenCalledTimes(1);
expect(client.index).toHaveBeenCalledWith(
expect.objectContaining({
body: expect.objectContaining({
globalType: {
foo: 'bar',
title: 'Testing',
},
}),
}),
expect.any(Object)
);
});

it(`should use the ES index action only with the provided attributes when mergeAttributes is false`, async () => {
migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));

await updateSuccess(
client,
repository,
registry,
NAMESPACE_AGNOSTIC_TYPE,
id,
{
foo: 'bar',
},
{ mergeAttributes: false }
);

expect(client.index).toHaveBeenCalledTimes(1);
expect(client.index).toHaveBeenCalledWith(
expect.objectContaining({
body: expect.objectContaining({
globalType: {
foo: 'bar',
},
}),
}),
expect.any(Object)
);
});

it(`should check for alias conflicts if a new multi-namespace object before create action would be created then create action to create the object`, async () => {
migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
await updateSuccess(
Expand Down Expand Up @@ -218,6 +267,7 @@ describe('#update', () => {
};
await test(references);
});

it(`accepts custom references array 2`, async () => {
const test = async (references: SavedObjectReference[]) => {
migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
Expand All @@ -232,6 +282,7 @@ describe('#update', () => {
};
await test([{ type: 'foo', id: '42', name: 'some ref' }]);
});

it(`accepts custom references array 3`, async () => {
const test = async (references: SavedObjectReference[]) => {
migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
Expand Down
30 changes: 18 additions & 12 deletions packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/update.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,18 +246,24 @@ export const executeUpdate = async <T>(
// at this point, we already know 1. the document exists 2. we're not doing an upsert
// therefor we can safely process with the "standard" update sequence.

const updatedAttributes = mergeForUpdate({
targetAttributes: {
...(migrated!.attributes as Record<string, unknown>),
},
updatedAttributes: await encryptionHelper.optionallyEncryptAttributes(
type,
id,
namespace,
attributes
),
typeMappings: typeDefinition.mappings,
});
const mergeAttributes = options.mergeAttributes ?? true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT consistency: All other options are extracted on lines 87-93, and the optional ones have default values that are stored in ../constants.ts.

const encryptedUpdatedAttributes = await encryptionHelper.optionallyEncryptAttributes(
type,
id,
namespace,
attributes
);

const updatedAttributes = mergeAttributes
? mergeForUpdate({
targetAttributes: {
...(migrated!.attributes as Record<string, unknown>),
},
updatedAttributes: encryptedUpdatedAttributes,
typeMappings: typeDefinition.mappings,
})
: encryptedUpdatedAttributes;

const migratedUpdatedSavedObjectDoc = migrationHelper.migrateInputDocument({
...migrated!,
id,
Expand Down
7 changes: 7 additions & 0 deletions packages/core/saved-objects/core-saved-objects-api-server/src/apis/bulk_update.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,13 @@ export interface SavedObjectsBulkUpdateObject<T = unknown>
* Note: the default namespace's string representation is `'default'`, and its ID representation is `undefined`.
**/
namespace?: string;
/**
* By default, update will merge the provided attributes with the ones present on the document
* (performing a standard partial update). Setting this option to `false` will change the behavior, performing
* a "full" update instead, where the provided attributes will fully replace the existing ones.
* Defaults to `true`.
*/
mergeAttributes?: boolean;
}

/**
Expand Down
7 changes: 7 additions & 0 deletions packages/core/saved-objects/core-saved-objects-api-server/src/apis/update.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,13 @@ export interface SavedObjectsUpdateOptions<Attributes = unknown> extends SavedOb
retryOnConflict?: number;
/** {@link SavedObjectsRawDocParseOptions.migrationVersionCompatibility} */
migrationVersionCompatibility?: 'compatible' | 'raw';
/**
* By default, update will merge the provided attributes with the ones present on the document
* (performing a standard partial update). Setting this option to `false` will change the behavior, performing
* a "full" update instead, where the provided attributes will fully replace the existing ones.
* Defaults to `true`.
*/
mergeAttributes?: boolean;
}

/**
Expand Down
Loading