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

[SOR] use initialNamespaces when checking for conflict for create and bulkCreate #111023

Merged
merged 3 commits into from
Sep 3, 2021
Merged

[SOR] use initialNamespaces when checking for conflict for create and bulkCreate #111023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
86a806c
use initialNamespaces when checking for conflict
pgayvallet Sep 2, 2021
ae809a2
nits
pgayvallet Sep 2, 2021
4d1b75e
Merge remote-tracking branch 'upstream/master' into kbn-xxx-SO-create…
pgayvallet Sep 2, 2021
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
81 changes: 78 additions & 3 deletions src/core/server/saved_objects/service/lib/repository.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,16 @@ describe('SavedObjectsRepository', () => {
{ type, id, references, namespace: objectNamespace, originId },
namespace
) => {
const namespaceId = objectNamespace === 'default' ? undefined : objectNamespace ?? namespace;
let namespaces;
if (objectNamespace) {
namespaces = [objectNamespace];
} else if (namespace) {
namespaces = Array.isArray(namespace) ? namespace : [namespace];
} else {
namespaces = ['default'];
}
const namespaceId = namespaces[0] === 'default' ? undefined : namespaces[0];

return {
// NOTE: Elasticsearch returns more fields (_index, _type) but the SavedObjectsRepository method ignores these
found: true,
Expand All @@ -207,7 +216,7 @@ describe('SavedObjectsRepository', () => {
...mockVersionProps,
_source: {
...(registry.isSingleNamespace(type) && { namespace: namespaceId }),
...(registry.isMultiNamespace(type) && { namespaces: [namespaceId ?? 'default'] }),
...(registry.isMultiNamespace(type) && { namespaces }),
...(originId && { originId }),
type,
[type]: { title: 'Testing' },
Expand All @@ -219,7 +228,9 @@ describe('SavedObjectsRepository', () => {
};

const getMockMgetResponse = (objects, namespace) => ({
docs: objects.map((obj) => (obj.found === false ? obj : getMockGetResponse(obj, namespace))),
docs: objects.map((obj) =>
obj.found === false ? obj : getMockGetResponse(obj, obj.initialNamespaces ?? namespace)
),
});

expect.extend({
Expand Down Expand Up @@ -797,6 +808,54 @@ describe('SavedObjectsRepository', () => {
});
});

it(`returns error when there is an unresolvable conflict with an existing multi-namespace saved object when using initialNamespaces (get)`, async () => {
const obj = {
...obj3,
type: MULTI_NAMESPACE_TYPE,
initialNamespaces: ['foo-namespace', 'default'],
};
const response1 = {
status: 200,
docs: [
{
found: true,
_source: {
type: obj.type,
namespaces: ['bar-namespace'],
},
},
],
};
client.mget.mockResolvedValueOnce(
elasticsearchClientMock.createSuccessTransportRequestPromise(response1)
);
const response2 = getMockBulkCreateResponse([obj1, obj2]);
client.bulk.mockResolvedValueOnce(
elasticsearchClientMock.createSuccessTransportRequestPromise(response2)
);

const options = { overwrite: true };
const result = await savedObjectsRepository.bulkCreate([obj1, obj, obj2], options);

expect(client.bulk).toHaveBeenCalled();
expect(client.mget).toHaveBeenCalled();

const body1 = { docs: [expect.objectContaining({ _id: `${obj.type}:${obj.id}` })] };
expect(client.mget).toHaveBeenCalledWith(
expect.objectContaining({ body: body1 }),
expect.anything()
);
const body2 = [...expectObjArgs(obj1), ...expectObjArgs(obj2)];
expect(client.bulk).toHaveBeenCalledWith(
expect.objectContaining({ body: body2 }),
expect.anything()
);
const expectedError = expectErrorConflict(obj, { metadata: { isNotOverwritable: true } });
expect(result).toEqual({
saved_objects: [expectSuccess(obj1), expectedError, expectSuccess(obj2)],
});
});

it(`returns bulk error`, async () => {
const expectedErrorResult = { type: obj3.type, id: obj3.id, error: 'Oh no, a bulk error!' };
await bulkCreateError(obj3, true, expectedErrorResult);
Expand Down Expand Up @@ -2197,6 +2256,22 @@ describe('SavedObjectsRepository', () => {
expect(client.get).toHaveBeenCalled();
});

it(`throws when there is an unresolvable conflict with an existing multi-namespace saved object when using initialNamespaces (get)`, async () => {
const response = getMockGetResponse({ type: MULTI_NAMESPACE_ISOLATED_TYPE, id }, namespace);
client.get.mockResolvedValueOnce(
elasticsearchClientMock.createSuccessTransportRequestPromise(response)
);
await expect(
savedObjectsRepository.create(MULTI_NAMESPACE_TYPE, attributes, {
id,
overwrite: true,
initialNamespaces: ['bar-ns', 'dolly-ns'],
namespace,
})
).rejects.toThrowError(createConflictError(MULTI_NAMESPACE_TYPE, id));
expect(client.get).toHaveBeenCalled();
});

it.todo(`throws when automatic index creation fails`);

it.todo(`throws when an unexpected failure occurs`);
Expand Down
34 changes: 26 additions & 8 deletions src/core/server/saved_objects/service/lib/repository.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,8 +308,12 @@ export class SavedObjectsRepository {
if (id && overwrite) {
// we will overwrite a multi-namespace saved object if it exists; if that happens, ensure we preserve its included namespaces
// note: this check throws an error if the object is found but does not exist in this namespace
const existingNamespaces = await this.preflightGetNamespaces(type, id, namespace);
savedObjectNamespaces = initialNamespaces || existingNamespaces;
savedObjectNamespaces = await this.preflightGetNamespaces(
type,
id,
namespace,
initialNamespaces
);
} else {
savedObjectNamespaces = initialNamespaces || getSavedObjectNamespaces(namespace);
}
Expand Down Expand Up @@ -455,8 +459,14 @@ export class SavedObjectsRepository {
const indexFound = bulkGetResponse?.statusCode !== 404;
const actualResult = indexFound ? bulkGetResponse?.body.docs[esRequestIndex] : undefined;
const docFound = indexFound && actualResult?.found === true;
// @ts-expect-error MultiGetHit._source is optional
if (docFound && !this.rawDocExistsInNamespace(actualResult!, namespace)) {
if (
docFound &&
!this.rawDocExistsInNamespaces(
// @ts-expect-error MultiGetHit._source is optional
actualResult!,
initialNamespaces ?? [SavedObjectsUtils.namespaceIdToString(namespace)]
)
) {
const { id, type } = object;
return {
tag: 'Left' as 'Left',
Expand Down Expand Up @@ -2140,12 +2150,18 @@ export class SavedObjectsRepository {
* @param type The type of the saved object.
* @param id The ID of the saved object.
* @param namespace The target namespace.
* @param initialNamespaces The target namespace(s) we intend to create the object in, if specified.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The difference between namespace and initialNamespaces is subtle and it might be worth adding a bit more info here about when each is used.

* @returns Array of namespaces that this saved object currently includes, or (if the object does not exist yet) the namespaces that a
* newly-created object will include. Value may be undefined if an existing saved object has no namespaces attribute; this should not
* happen in normal operations, but it is possible if the Elasticsearch document is manually modified.
* @throws Will throw an error if the saved object exists and it does not include the target namespace.
*/
private async preflightGetNamespaces(type: string, id: string, namespace?: string) {
private async preflightGetNamespaces(
type: string,
id: string,
namespace: string | undefined,
initialNamespaces?: string[]
) {
if (!this._registry.isMultiNamespace(type)) {
throw new Error(`Cannot make preflight get request for non-multi-namespace type '${type}'.`);
}
Expand All @@ -2160,17 +2176,19 @@ export class SavedObjectsRepository {
}
);

const namespaces = initialNamespaces ?? [SavedObjectsUtils.namespaceIdToString(namespace)];

const indexFound = statusCode !== 404;
if (indexFound && isFoundGetResponse(body)) {
if (!this.rawDocExistsInNamespace(body, namespace)) {
if (!this.rawDocExistsInNamespaces(body, namespaces)) {
throw SavedObjectsErrorHelpers.createConflictError(type, id);
}
return getSavedObjectNamespaces(namespace, body);
return initialNamespaces ?? getSavedObjectNamespaces(namespace, body);
} else if (isNotFoundFromUnsupportedServer({ statusCode, headers })) {
// checking if the 404 is from Elasticsearch
throw SavedObjectsErrorHelpers.createGenericNotFoundEsUnavailableError(type, id);
}
return getSavedObjectNamespaces(namespace);
return initialNamespaces ?? getSavedObjectNamespaces(namespace);
}

/**
Expand Down