Skip to content

Commit

Permalink
use initialNamespaces when checking for conflict
Browse files Browse the repository at this point in the history
  • Loading branch information
@pgayvallet
pgayvallet committed Sep 2, 2021
1 parent fecdba7 commit 86a806c
Show file tree
Hide file tree
Showing 2 changed files with 105 additions and 11 deletions.
82 changes: 79 additions & 3 deletions src/core/server/saved_objects/service/lib/repository.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,17 @@ 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];

// const namespaceId = objectNamespace === 'default' ? undefined : objectNamespace ?? namespace;
return {
// NOTE: Elasticsearch returns more fields (_index, _type) but the SavedObjectsRepository method ignores these
found: true,
Expand All @@ -207,7 +217,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 +229,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 @@ -778,6 +790,54 @@ describe('SavedObjectsRepository', () => {
});
});

it(`returns error when there is a conflict with an existing multi-namespace saved object with 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 @@ -2160,6 +2220,22 @@ describe('SavedObjectsRepository', () => {
expect(client.get).toHaveBeenCalled();
});

it(`throws when there is a 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
Original file line number Diff line number Diff line change
Expand Up @@ -306,8 +306,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 @@ -453,8 +457,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 @@ -2136,12 +2146,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.
* @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 @@ -2156,17 +2172,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

0 comments on commit 86a806c

Please sign in to comment.