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.

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

SavedObjects bulkCreate API should return migrationVersion and strip the type & namespace from the id #65150

Merged
merged 12 commits into from
May 8, 2020
Merged
39 changes: 34 additions & 5 deletions src/core/server/saved_objects/service/lib/repository.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -424,9 +424,17 @@ describe('SavedObjectsRepository', () => {

const getMockBulkCreateResponse = (objects, namespace) => {
return {
items: objects.map(({ type, id }) => ({
items: objects.map(({ type, id, attributes, references, migrationVersion }) => ({
create: {
_id: `${namespace ? `${namespace}:` : ''}${type}:${id}`,
_source: {
[type]: attributes,
type,
namespace,
references,
rudolf marked this conversation as resolved.
Show resolved Hide resolved
...mockTimestampFields,
migrationVersion: migrationVersion || { [type]: '1.1.1' },
},
...mockVersionProps,
},
})),
Expand Down Expand Up @@ -474,7 +482,7 @@ describe('SavedObjectsRepository', () => {

const expectSuccessResult = obj => ({
...obj,
migrationVersion: undefined,
migrationVersion: { [obj.type]: '1.1.1' },
version: mockVersion,
...mockTimestampFields,
});
Expand Down Expand Up @@ -619,13 +627,16 @@ describe('SavedObjectsRepository', () => {
};

const bulkCreateError = async (obj, esError, expectedError) => {
const objects = [obj1, obj, obj2];
const response = getMockBulkCreateResponse(objects);
let response;
if (esError) {
response = getMockBulkCreateResponse([obj1, obj, obj2]);
response.items[1].create = { error: esError };
} else {
response = getMockBulkCreateResponse([obj1, obj2]);
}
callAdminCluster.mockResolvedValue(response); // this._writeToCluster('bulk', ...)

const objects = [obj1, obj, obj2];
const result = await savedObjectsRepository.bulkCreate(objects);
expectClusterCalls('bulk');
const objCall = esError ? expectObjArgs(obj) : [];
Expand Down Expand Up @@ -781,7 +792,7 @@ describe('SavedObjectsRepository', () => {
id: 'three',
};
const objects = [obj1, obj, obj2];
const response = getMockBulkCreateResponse(objects);
const response = getMockBulkCreateResponse([obj1, obj2]);
callAdminCluster.mockResolvedValue(response); // this._writeToCluster('bulk', ...)
const result = await savedObjectsRepository.bulkCreate(objects);
expect(callAdminCluster).toHaveBeenCalledTimes(1);
Expand All @@ -790,6 +801,24 @@ describe('SavedObjectsRepository', () => {
});
});
});

it(`deserializes the raw ES response into a saved object`, async () => {
rudolf marked this conversation as resolved.
Show resolved Hide resolved
// Test for fix to https://github.com/elastic/kibana/issues/65088 where
// we returned raw ID's when an object without an id was created.
const response = getMockBulkCreateResponse([obj1, obj2]);
callAdminCluster.mockResolvedValueOnce(response); // this._writeToCluster('bulk', ...)

// Bulk create one object with id unspecified, and one with id specified
const result = await savedObjectsRepository.bulkCreate([{ ...obj1, id: undefined }, obj2]);

// Assert that both raw docs from the ES response are deserialized
expect(serializer.rawToSavedObject).toHaveBeenNthCalledWith(1, response.items[0].create);
expect(serializer.rawToSavedObject).toHaveBeenNthCalledWith(2, response.items[1].create);

// Assert that ID's are deserialized and doesn't include the namespace
// and type.
expect(result.saved_objects.map(so => so.id)).toEqual([obj1.id, obj2.id]);
});
});

describe('#bulkGet', () => {
Expand Down
37 changes: 12 additions & 25 deletions src/core/server/saved_objects/service/lib/repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -404,35 +404,22 @@ export class SavedObjectsRepository {
}

const { requestedId, rawMigratedDoc, esRequestIndex } = expectedResult.value;
const response = bulkResponse.items[esRequestIndex];
const {
error,
_id: responseId,
_seq_no: seqNo,
_primary_term: primaryTerm,
} = Object.values(response)[0] as any;

const {
_source: { type, [type]: attributes, references = [], namespaces },
} = rawMigratedDoc;

const id = requestedId || responseId;
const { error, ...rawResponse } = Object.values(
bulkResponse.items[esRequestIndex]
)[0] as any;

if (error) {
return {
id,
type,
error: getBulkOperationError(error, type, id),
id: requestedId,
type: rawMigratedDoc._source.type,
error: getBulkOperationError(error, rawMigratedDoc._source.type, requestedId),
};
}
return {
id,
type,
...(namespaces && { namespaces }),
updated_at: time,
version: encodeVersion(seqNo, primaryTerm),
attributes,
references,
};

// When method == 'index' the bulkResponse doesn't include the indexed
// _source so we return rawMigratedDoc but have to spread rawResponse
// to return the latest _seq_no and _primary_term values from ES.
return this._serializer.rawToSavedObject({ ...rawMigratedDoc, ...rawResponse });
rudolf marked this conversation as resolved.
Show resolved Hide resolved
}),
};
}
Expand Down
9 changes: 9 additions & 0 deletions test/api_integration/apis/saved_objects/bulk_create.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ export default function({ getService }) {
attributes: {
title: 'A great new dashboard',
},
migrationVersion: {
dashboard: resp.body.saved_objects[1].migrationVersion.dashboard,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This value will change anytime a new dashboard migration is added. This seems like the best workaround when using kbn-expect to prevent tests from failing anytime a new dashboard migration is added.

We should probably "modernize" these integration tests, but it felt like it's not enough of a priority to attempt this right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, having jest toolbox for FTR tests would we really great. kbn-expect feels very old when compared to jest.

},
references: [],
},
],
Expand Down Expand Up @@ -106,6 +109,9 @@ export default function({ getService }) {
title: 'An existing visualization',
},
references: [],
migrationVersion: {
visualization: resp.body.saved_objects[0].migrationVersion.visualization,
},
rudolf marked this conversation as resolved.
Show resolved Hide resolved
},
{
type: 'dashboard',
Expand All @@ -116,6 +122,9 @@ export default function({ getService }) {
title: 'A great new dashboard',
},
references: [],
migrationVersion: {
dashboard: resp.body.saved_objects[1].migrationVersion.dashboard,
},
},
],
});
Expand Down