Skip to content

Commit

Permalink
Include alias checks for upserts
Browse files Browse the repository at this point in the history 
The current implementation causes an extra round trip to ES if a
new object would be created, but given the infrequency of the
affected functions and our current time constraints, it seems
acceptable because it is the simplest path forward.
  • Loading branch information
@jportner
jportner committed Oct 25, 2021
1 parent 2d3b344 commit d3383bd
Show file tree
Hide file tree
Showing 3 changed files with 161 additions and 10 deletions.
6 changes: 3 additions & 3 deletions src/core/server/saved_objects/service/lib/preflight_check_for_create.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,10 @@ export interface PreflightCheckForCreateObject {
type: string;
/** The ID of the object. */
id: string;
/** Whether or not the object should be overwritten if it would encounter a regular conflict. */
overwrite: boolean;
/** The namespaces that the consumer intends to create this object in. */
namespaces: string[];
/** Whether or not the object should be overwritten if it would encounter a regular conflict. */
overwrite?: boolean;
}

export interface PreflightCheckForCreateParams {
Expand Down Expand Up @@ -190,7 +190,7 @@ async function optionallyFindAliases(
) {
// Make a discriminated union based on the spaces the objects should be created in (Left=mget aliases, Right=find aliases)
const objectsToGetOrObjectsToFind = objects.map<Either<ParsedObject>>((object) => {
const { type, id, overwrite, namespaces } = object;
const { type, id, namespaces, overwrite = false } = object;
const spaces = new Set(namespaces);
const tag =
spaces.size > FIND_ALIASES_THRESHOLD || spaces.has(ALL_NAMESPACES_STRING) ? 'Right' : 'Left';
Expand Down
128 changes: 121 additions & 7 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 @@ -3731,10 +3731,12 @@ describe('SavedObjectsRepository', () => {
const namespace = 'foo-namespace';
const originId = 'some-origin-id';

const incrementCounterSuccess = async (type, id, fields, options) => {
const incrementCounterSuccess = async (type, id, fields, options, internalOptions = {}) => {
const { mockGetResponseValue } = internalOptions;
const isMultiNamespace = registry.isMultiNamespace(type);
if (isMultiNamespace) {
const response = getMockGetResponse({ type, id }, options?.namespace);
const response =
mockGetResponseValue ?? getMockGetResponse({ type, id }, options?.namespace);
client.get.mockResolvedValueOnce(
elasticsearchClientMock.createSuccessTransportRequestPromise(response)
);
Expand Down Expand Up @@ -3766,9 +3768,18 @@ describe('SavedObjectsRepository', () => {
return result;
};

beforeEach(() => {
mockPreflightCheckForCreate.mockReset();
mockPreflightCheckForCreate.mockImplementation(({ objects }) => {
return objects.map(({ type, id }) => ({ type, id })); // respond with no errors by default
});
});

describe('client calls', () => {
it(`should use the ES update action if type is not multi-namespace`, async () => {
await incrementCounterSuccess(type, id, counterFields, { namespace });
expect(client.get).not.toHaveBeenCalled();
expect(mockPreflightCheckForCreate).not.toHaveBeenCalled();
expect(client.update).toHaveBeenCalledTimes(1);
});

Expand All @@ -3777,6 +3788,20 @@ describe('SavedObjectsRepository', () => {
namespace,
});
expect(client.get).toHaveBeenCalledTimes(1);
expect(mockPreflightCheckForCreate).not.toHaveBeenCalled();
expect(client.update).toHaveBeenCalledTimes(1);
});

it(`should check for alias conflicts if a new multi-namespace object would be created`, async () => {
await incrementCounterSuccess(
MULTI_NAMESPACE_ISOLATED_TYPE,
id,
counterFields,
{ namespace },
{ mockGetResponseValue: { found: false } }
);
expect(client.get).toHaveBeenCalledTimes(1);
expect(mockPreflightCheckForCreate).toHaveBeenCalledTimes(1);
expect(client.update).toHaveBeenCalledTimes(1);
});

Expand Down Expand Up @@ -3945,6 +3970,43 @@ describe('SavedObjectsRepository', () => {
)
).rejects.toThrowError(createConflictError(MULTI_NAMESPACE_ISOLATED_TYPE, id));
expect(client.get).toHaveBeenCalledTimes(1);
expect(mockPreflightCheckForCreate).not.toHaveBeenCalled();
expect(client.update).not.toHaveBeenCalled();
});

it(`throws when there is an alias conflict from preflightCheckForCreate`, async () => {
client.get.mockResolvedValueOnce(
elasticsearchClientMock.createSuccessTransportRequestPromise({ found: false })
);
mockPreflightCheckForCreate.mockResolvedValue([{ error: { type: 'aliasConflict' } }]);
await expect(
savedObjectsRepository.incrementCounter(
MULTI_NAMESPACE_ISOLATED_TYPE,
id,
counterFields,
{ namespace }
)
).rejects.toThrowError(createConflictError(MULTI_NAMESPACE_ISOLATED_TYPE, id));
expect(client.get).toHaveBeenCalledTimes(1);
expect(mockPreflightCheckForCreate).toHaveBeenCalledTimes(1);
expect(client.update).not.toHaveBeenCalled();
});

it(`does not throw when there is a different error from preflightCheckForCreate`, async () => {
client.get.mockResolvedValueOnce(
elasticsearchClientMock.createSuccessTransportRequestPromise({ found: false })
);
mockPreflightCheckForCreate.mockResolvedValue([{ error: { type: 'something-else' } }]);
await incrementCounterSuccess(
MULTI_NAMESPACE_ISOLATED_TYPE,
id,
counterFields,
{ namespace },
{ mockGetResponseValue: { found: false } }
);
expect(client.get).toHaveBeenCalledTimes(1);
expect(mockPreflightCheckForCreate).toHaveBeenCalledTimes(1);
expect(client.update).toHaveBeenCalledTimes(1);
});
});

Expand Down Expand Up @@ -4090,9 +4152,11 @@ describe('SavedObjectsRepository', () => {
);
};

const updateSuccess = async (type, id, attributes, options, includeOriginId) => {
const updateSuccess = async (type, id, attributes, options, internalOptions = {}) => {
const { mockGetResponseValue, includeOriginId } = internalOptions;
if (registry.isMultiNamespace(type)) {
const mockGetResponse = getMockGetResponse({ type, id }, options?.namespace);
const mockGetResponse =
mockGetResponseValue ?? getMockGetResponse({ type, id }, options?.namespace);
client.get.mockResolvedValueOnce(
elasticsearchClientMock.createSuccessTransportRequestPromise(
{ ...mockGetResponse },
Expand All @@ -4106,15 +4170,38 @@ describe('SavedObjectsRepository', () => {
return result;
};

beforeEach(() => {
mockPreflightCheckForCreate.mockReset();
mockPreflightCheckForCreate.mockImplementation(({ objects }) => {
return objects.map(({ type, id }) => ({ type, id })); // respond with no errors by default
});
});

describe('client calls', () => {
it(`should use the ES update action when type is not multi-namespace`, async () => {
await updateSuccess(type, id, attributes);
expect(client.get).not.toHaveBeenCalled();
expect(mockPreflightCheckForCreate).not.toHaveBeenCalled();
expect(client.update).toHaveBeenCalledTimes(1);
});

it(`should use the ES get action then update action when type is multi-namespace`, async () => {
await updateSuccess(MULTI_NAMESPACE_ISOLATED_TYPE, id, attributes);
expect(client.get).toHaveBeenCalledTimes(1);
expect(mockPreflightCheckForCreate).not.toHaveBeenCalled();
expect(client.update).toHaveBeenCalledTimes(1);
});

it(`should use the ES update action when type is not multi-namespace`, async () => {
await updateSuccess(type, id, attributes);
it(`should check for alias conflicts if a new multi-namespace object would be created`, async () => {
await updateSuccess(
MULTI_NAMESPACE_ISOLATED_TYPE,
id,
attributes,
{ upsert: true },
{ mockGetResponseValue: { found: false } }
);
expect(client.get).toHaveBeenCalledTimes(1);
expect(mockPreflightCheckForCreate).toHaveBeenCalledTimes(1);
expect(client.update).toHaveBeenCalledTimes(1);
});

Expand Down Expand Up @@ -4390,6 +4477,33 @@ describe('SavedObjectsRepository', () => {
expect(client.get).toHaveBeenCalledTimes(1);
});

it(`throws when there is an alias conflict from preflightCheckForCreate`, async () => {
client.get.mockResolvedValueOnce(
elasticsearchClientMock.createSuccessTransportRequestPromise({ found: false })
);
mockPreflightCheckForCreate.mockResolvedValue([{ error: { type: 'aliasConflict' } }]);
await expect(
savedObjectsRepository.update(MULTI_NAMESPACE_ISOLATED_TYPE, id, {}, { upsert: true })
).rejects.toThrowError(createConflictError(MULTI_NAMESPACE_ISOLATED_TYPE, id));
expect(client.get).toHaveBeenCalledTimes(1);
expect(mockPreflightCheckForCreate).toHaveBeenCalledTimes(1);
expect(client.update).not.toHaveBeenCalled();
});

it(`does not throw when there is a different error from preflightCheckForCreate`, async () => {
mockPreflightCheckForCreate.mockResolvedValue([{ error: { type: 'something-else' } }]);
await updateSuccess(
MULTI_NAMESPACE_ISOLATED_TYPE,
id,
attributes,
{ upsert: true },
{ mockGetResponseValue: { found: false } }
);
expect(client.get).toHaveBeenCalledTimes(1);
expect(mockPreflightCheckForCreate).toHaveBeenCalledTimes(1);
expect(client.update).toHaveBeenCalledTimes(1);
});

it(`throws when ES is unable to find the document during update`, async () => {
const notFoundError = new EsErrors.ResponseError(
elasticsearchClientMock.createApiResponse({
Expand Down Expand Up @@ -4437,7 +4551,7 @@ describe('SavedObjectsRepository', () => {
});

it(`includes originId property if present in cluster call response`, async () => {
const result = await updateSuccess(type, id, attributes, {}, true);
const result = await updateSuccess(type, id, attributes, {}, { includeOriginId: true });
expect(result).toMatchObject({ originId });
});
});
Expand Down
37 changes: 37 additions & 0 deletions src/core/server/saved_objects/service/lib/repository.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1265,6 +1265,12 @@ export class SavedObjectsRepository {
) {
throw SavedObjectsErrorHelpers.createGenericNotFoundError(type, id);
}
if (upsert && preflightResult.checkResult === 'not_found') {
// If an upsert would result in the creation of a new object, we need to check for alias conflicts too.
// This takes an extra round trip to Elasticsearch, but this won't happen often.
// TODO: improve performance by combining these into a single preflight check
await this.preflightCheckForUpsertAliasConflict(type, id, namespace);
}
}

const time = getCurrentTime();
Expand Down Expand Up @@ -1816,6 +1822,14 @@ export class SavedObjectsRepository {
if (preflightResult.checkResult === 'found_outside_namespace') {
throw SavedObjectsErrorHelpers.createConflictError(type, id);
}

if (preflightResult.checkResult === 'not_found') {
// If an upsert would result in the creation of a new object, we need to check for alias conflicts too.
// This takes an extra round trip to Elasticsearch, but this won't happen often.
// TODO: improve performance by combining these into a single preflight check
await this.preflightCheckForUpsertAliasConflict(type, id, namespace);
}

savedObjectNamespaces = preflightResult.savedObjectNamespaces;
}

Expand Down Expand Up @@ -2153,6 +2167,29 @@ export class SavedObjectsRepository {
};
}

/**
* Pre-flight check to ensure that an upsert which would create a new object does not result in an alias conflict.
*/
private async preflightCheckForUpsertAliasConflict(
type: string,
id: string,
namespace: string | undefined
) {
const namespaceString = SavedObjectsUtils.namespaceIdToString(namespace);
const [{ error }] = await preflightCheckForCreate({
registry: this._registry,
client: this.client,
serializer: this._serializer,
getIndexForType: this.getIndexForType.bind(this),
createPointInTimeFinder: this.createPointInTimeFinder.bind(this),
objects: [{ type, id, namespaces: [namespaceString] }],
});
if (error?.type === 'aliasConflict') {
throw SavedObjectsErrorHelpers.createConflictError(type, id);
}
// any other error from this check does not matter
}

/** The `initialNamespaces` field (create, bulkCreate) is used to create an object in an initial set of spaces. */
private validateInitialNamespaces(type: string, initialNamespaces: string[] | undefined) {
if (!initialNamespaces) {
Expand Down

0 comments on commit d3383bd

Please sign in to comment.