Skip to content
This repository has been archived by the owner on Mar 31, 2024. It is now read-only.

Commit

Permalink
Remove dependency on doc versions (elastic#29906)
Browse files Browse the repository at this point in the history
See elastic/elasticsearch#38254

Using the `version` parameter to implement optimistic concurrency is not going to be supported in 7.0, so we need to replace our usage of document version with the new `_seq_no` and `_primary_term` parameters. These fields are returned in the same way that `_version` was returned on all read/write requests except for search, where it needs to be requested by sending `seq_no_primary_term: true` in the body of the search request. These parameters are sent back to Elasticsearch on write requests with the `if_seq_no` and `if_primary_term` parameters, and are functionally equivalent to sending a `version` in a write request before elastic/elasticsearch#38254.

To make these updates I searched the code base for uses of a `version` and `_version`, then triaged each usage, so I'm fairly confident that I got everything but it's possible something slipped through the cracks, so if you know of any usage of the document version field please help me out by double checking that I converted it.

- [x] **Saved Objects**:  @elastic/kibana-platform, @elastic/es-security - for BWC and ergonomics the `version` provided by the Saved Objects client/API was not removed, it was converted from a number to a string whose value is `base64(json([_seq_no, _primary_term]))`. This allows the Saved Objects API and its consumers to remain mostly unmodified, as long as the underlying value in the version field is irrelevant. This was the case for all usages in Kibana, only thing that needed updating was tests and TS types.

- [x] **Reporting/esqueue**: @joelgriffith, @tsullivan - the version parameter was used here specifically for implementing optimistic concurrency, and since its usage was contained within the esqueue module I just updated it to use the new `_seq_no` and `_primary_term` fields.

- [x] **Task Manager**: @tsullivan @njd5475 - Like esqueue this module uses version for optimistic concurrency but the usage is contained with the module so I just updated it to use, store, and request the `_seq_no` and `_primary_term` fields.

- [ ] **ML**: @elastic/ml-ui - Best I could tell the only "version" in the ML code refers to the stack version, elastic@077245f

- [ ] **Beats CM**: @elastic/beats - Looks like the references to `_version` in the code is only in the types but not in the code itself. I updated the types to use `_seq_no` and `_primary_term`, and their camelCase equivalents where appropriate. I did find a method that used one of the types referencing version but when investigating its usage it seemed the only consumer of that method was itself so i removed it. elastic@52d890f

- [x] **Spaces (tests)**: @elastic/kibana-security - The spaces test helpers use saved objects with versions in a number of places, so I updated them to use the new string versions where the version was predictable, and removed the assertion on version where it wasn't. We test the version in the saved objects code so this should be fine.
  • Loading branch information
Spencer committed Feb 5, 2019
1 parent db38e66 commit 9af2fd6
Show file tree
Hide file tree
Showing 58 changed files with 773 additions and 247 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ describe('plugins/elasticsearch', () => {

cluster = { callWithInternalUser: sinon.stub() };
cluster.callWithInternalUser.withArgs('index', sinon.match.any).returns(Promise.resolve());
cluster.callWithInternalUser.withArgs('create', sinon.match.any).returns(Promise.resolve({ _id: '1', _version: 1 }));
cluster.callWithInternalUser.withArgs('mget', sinon.match.any).returns(Promise.resolve({ ok: true }));
cluster.callWithInternalUser.withArgs('get', sinon.match.any).returns(Promise.resolve({ found: false }));
cluster.callWithInternalUser.withArgs('search', sinon.match.any).returns(Promise.resolve({ hits: { hits: [] } }));
Expand Down
2 changes: 1 addition & 1 deletion src/server/saved_objects/routes/bulk_create.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export const createBulkCreateRoute = prereqs => ({
type: Joi.string().required(),
id: Joi.string(),
attributes: Joi.object().required(),
version: Joi.number(),
version: Joi.string(),
migrationVersion: Joi.object().optional(),
}).required()
),
Expand Down
2 changes: 1 addition & 1 deletion src/server/saved_objects/routes/bulk_get.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ describe('POST /api/saved_objects/_bulk_get', () => {
id: 'abc123',
type: 'index-pattern',
title: 'logstash-*',
version: 2
version: 'foo',
}]
};

Expand Down
2 changes: 1 addition & 1 deletion src/server/saved_objects/routes/update.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export const createUpdateRoute = (prereqs) => {
}).required(),
payload: Joi.object({
attributes: Joi.object().required(),
version: Joi.number().min(1)
version: Joi.string(),
}).required()
},
handler(request) {
Expand Down
2 changes: 1 addition & 1 deletion src/server/saved_objects/routes/update.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ describe('PUT /api/saved_objects/{type}/{id?}', () => {

it('calls upon savedObjectClient.update', async () => {
const attributes = { title: 'Testing' };
const options = { version: 2 };
const options = { version: 'foo' };
const request = {
method: 'PUT',
url: '/api/saved_objects/index-pattern/logstash-*',
Expand Down
18 changes: 13 additions & 5 deletions src/server/saved_objects/serialization/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

import uuid from 'uuid';
import { SavedObjectsSchema } from '../schema';
import { decodeVersion, encodeVersion } from '../version';

/**
* The root document type. In 7.0, this needs to change to '_doc'.
Expand All @@ -37,7 +38,8 @@ export interface RawDoc {
_id: string;
_source: any;
_type?: string;
_version?: number;
_seq_no?: number;
_primary_term?: number;
}

/**
Expand All @@ -60,7 +62,7 @@ export interface SavedObjectDoc {
type: string;
namespace?: string;
migrationVersion?: MigrationVersion;
version?: number;
version?: string;
updated_at?: Date;

[rootProp: string]: any;
Expand Down Expand Up @@ -99,16 +101,22 @@ export class SavedObjectsSerializer {
*
* @param {RawDoc} rawDoc - The raw ES document to be converted to saved object format.
*/
public rawToSavedObject({ _id, _source, _version }: RawDoc): SavedObjectDoc {
public rawToSavedObject({ _id, _source, _seq_no, _primary_term }: RawDoc): SavedObjectDoc {
const { type, namespace } = _source;

const version =
_seq_no != null || _primary_term != null
? encodeVersion(_seq_no!, _primary_term!)
: undefined;

return {
type,
id: this.trimIdPrefix(namespace, type, _id),
...(namespace && !this.schema.isNamespaceAgnostic(type) && { namespace }),
attributes: _source[type],
...(_source.migrationVersion && { migrationVersion: _source.migrationVersion }),
...(_source.updated_at && { updated_at: _source.updated_at }),
...(_version != null && { version: _version }),
...(version && { version }),
};
}

Expand All @@ -131,7 +139,7 @@ export class SavedObjectsSerializer {
_id: this.generateRawId(namespace, type, id),
_source: source,
_type: ROOT_TYPE,
...(version != null && { _version: version }),
...(version != null && decodeVersion(version)),
};
}

Expand Down
67 changes: 56 additions & 11 deletions src/server/saved_objects/serialization/serialization.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import _ from 'lodash';
import { ROOT_TYPE, SavedObjectsSerializer } from '.';
import { SavedObjectsSchema } from '../schema';
import { encodeVersion } from '../version';

describe('saved object conversion', () => {
describe('#rawToSavedObject', () => {
Expand Down Expand Up @@ -69,7 +70,8 @@ describe('saved object conversion', () => {
const actual = serializer.rawToSavedObject({
_id: 'hello:world',
_type: ROOT_TYPE,
_version: 3,
_seq_no: 3,
_primary_term: 1,
_source: {
type: 'hello',
hello: {
Expand All @@ -86,7 +88,7 @@ describe('saved object conversion', () => {
const expected = {
id: 'world',
type: 'hello',
version: 3,
version: encodeVersion(3, 1),
attributes: {
a: 'b',
c: 'd',
Expand All @@ -112,17 +114,46 @@ describe('saved object conversion', () => {
expect(actual).not.toHaveProperty('version');
});

test(`if specified it copies _version to version`, () => {
test(`if specified it encodes _seq_no and _primary_term to version`, () => {
const serializer = new SavedObjectsSerializer(new SavedObjectsSchema());
const actual = serializer.rawToSavedObject({
_id: 'foo:bar',
_version: 4,
_seq_no: 4,
_primary_term: 1,
_source: {
type: 'foo',
hello: {},
},
});
expect(actual).toHaveProperty('version', 4);
expect(actual).toHaveProperty('version', encodeVersion(4, 1));
});

test(`if only _seq_no is specified it throws`, () => {
const serializer = new SavedObjectsSerializer(new SavedObjectsSchema());
expect(() =>
serializer.rawToSavedObject({
_id: 'foo:bar',
_seq_no: 4,
_source: {
type: 'foo',
hello: {},
},
})
).toThrowErrorMatchingInlineSnapshot(`"_primary_term from elasticsearch must be an integer"`);
});

test(`if only _primary_term is throws`, () => {
const serializer = new SavedObjectsSerializer(new SavedObjectsSchema());
expect(() =>
serializer.rawToSavedObject({
_id: 'foo:bar',
_primary_term: 1,
_source: {
type: 'foo',
hello: {},
},
})
).toThrowErrorMatchingInlineSnapshot(`"_seq_no from elasticsearch must be an integer"`);
});

test('if specified it copies the _source.updated_at property to updated_at', () => {
Expand Down Expand Up @@ -206,7 +237,8 @@ describe('saved object conversion', () => {
const raw = {
_id: 'foo-namespace:foo:bar',
_type: ROOT_TYPE,
_version: 24,
_primary_term: 24,
_seq_no: 42,
_source: {
type: 'foo',
foo: {
Expand Down Expand Up @@ -440,25 +472,38 @@ describe('saved object conversion', () => {
expect(actual._source).not.toHaveProperty('migrationVersion');
});

test('it copies the version property to _version', () => {
test('it decodes the version property to _seq_no and _primary_term', () => {
const serializer = new SavedObjectsSerializer(new SavedObjectsSchema());
const actual = serializer.savedObjectToRaw({
type: '',
attributes: {},
version: 4,
version: encodeVersion(1, 2),
} as any);

expect(actual).toHaveProperty('_version', 4);
expect(actual).toHaveProperty('_seq_no', 1);
expect(actual).toHaveProperty('_primary_term', 2);
});

test(`if unspecified it doesn't add _version property`, () => {
test(`if unspecified it doesn't add _seq_no or _primary_term properties`, () => {
const serializer = new SavedObjectsSerializer(new SavedObjectsSchema());
const actual = serializer.savedObjectToRaw({
type: '',
attributes: {},
} as any);

expect(actual).not.toHaveProperty('_version');
expect(actual).not.toHaveProperty('_seq_no');
expect(actual).not.toHaveProperty('_primary_term');
});

test(`if version invalid it throws`, () => {
const serializer = new SavedObjectsSerializer(new SavedObjectsSchema());
expect(() =>
serializer.savedObjectToRaw({
type: '',
attributes: {},
version: 'foo',
} as any)
).toThrowErrorMatchingInlineSnapshot(`"Invalid version [foo]"`);
});

test('it copies attributes to _source[type]', () => {
Expand Down
3 changes: 3 additions & 0 deletions src/server/saved_objects/service/lib/errors.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,6 @@ export function isNotFoundError(maybeError: any): boolean;
export function isConflictError(maybeError: any): boolean;
export function isEsUnavailableError(maybeError: any): boolean;
export function isEsAutoCreateIndexError(maybeError: any): boolean;

export function createInvalidVersionError(version: any): Error;
export function isInvalidVersionError(maybeError: Error): boolean;
9 changes: 9 additions & 0 deletions src/server/saved_objects/service/lib/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,15 @@ export function isBadRequestError(error) {
return error && error[code] === CODE_BAD_REQUEST;
}

// 400 - invalid version
const CODE_INVALID_VERSION = 'SavedObjectsClient/invalidVersion';
export function createInvalidVersionError(versionInput) {
return decorate(Boom.badRequest(`Invalid version [${versionInput}]`), CODE_INVALID_VERSION, 400);
}
export function isInvalidVersionError(error) {
return error && error[code] === CODE_INVALID_VERSION;
}


// 401 - Not Authorized
const CODE_NOT_AUTHORIZED = 'SavedObjectsClient/notAuthorized';
Expand Down
23 changes: 12 additions & 11 deletions src/server/saved_objects/service/lib/repository.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { getSearchDsl } from './search_dsl';
import { includedFields } from './included_fields';
import { decorateEsError } from './decorate_es_error';
import * as errors from './errors';
import { decodeRequestVersion, encodeVersion, encodeHitVersion } from '../../version';

// BEWARE: The SavedObjectClient depends on the implementation details of the SavedObjectsRepository
// so any breaking changes to this repository are considered breaking changes to the SavedObjectsClient.
Expand Down Expand Up @@ -169,7 +170,8 @@ export class SavedObjectsRepository {
const {
error,
_id: responseId,
_version: version,
_seq_no: seqNo,
_primary_term: primaryTerm,
} = Object.values(response)[0];

const {
Expand Down Expand Up @@ -199,8 +201,8 @@ export class SavedObjectsRepository {
id,
type,
updated_at: time,
version,
attributes
version: encodeVersion(seqNo, primaryTerm),
attributes,
};
})
};
Expand Down Expand Up @@ -252,7 +254,6 @@ export class SavedObjectsRepository {
* @returns {promise} - { took, timed_out, total, deleted, batches, version_conflicts, noops, retries, failures }
*/
async deleteByNamespace(namespace) {

if (!namespace || typeof namespace !== 'string') {
throw new TypeError(`namespace is required, and must be a string`);
}
Expand Down Expand Up @@ -324,7 +325,7 @@ export class SavedObjectsRepository {
ignore: [404],
rest_total_hits_as_int: true,
body: {
version: true,
seq_no_primary_term: true,
...getSearchDsl(this._mappings, this._schema, {
search,
searchFields,
Expand Down Expand Up @@ -407,7 +408,7 @@ export class SavedObjectsRepository {
id,
type,
...time && { updated_at: time },
version: doc._version,
version: encodeHitVersion(doc),
attributes: doc._source[type],
migrationVersion: doc._source.migrationVersion,
};
Expand Down Expand Up @@ -449,7 +450,7 @@ export class SavedObjectsRepository {
id,
type,
...updatedAt && { updated_at: updatedAt },
version: response._version,
version: encodeHitVersion(response),
attributes: response._source[type],
migrationVersion: response._source.migrationVersion,
};
Expand All @@ -461,7 +462,7 @@ export class SavedObjectsRepository {
* @param {string} type
* @param {string} id
* @param {object} [options={}]
* @property {integer} options.version - ensures version matches that of persisted object
* @property {string} options.version - ensures version matches that of persisted object
* @property {string} [options.namespace]
* @returns {promise}
*/
Expand All @@ -476,7 +477,7 @@ export class SavedObjectsRepository {
id: this._serializer.generateRawId(namespace, type, id),
type: this._type,
index: this._index,
version,
...(version && decodeRequestVersion(version)),
refresh: 'wait_for',
ignore: [404],
body: {
Expand All @@ -496,7 +497,7 @@ export class SavedObjectsRepository {
id,
type,
updated_at: time,
version: response._version,
version: encodeHitVersion(response),
attributes
};
}
Expand Down Expand Up @@ -570,7 +571,7 @@ export class SavedObjectsRepository {
id,
type,
updated_at: time,
version: response._version,
version: encodeHitVersion(response),
attributes: response.get._source[type],
};

Expand Down
Loading

0 comments on commit 9af2fd6

Please sign in to comment.