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

Fixes #9680 (wrong title when overwriting saved objects) #44441

Closed
wants to merge 5 commits into from
Closed

Fixes #9680 (wrong title when overwriting saved objects) #44441

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
96d3640
Fixes #9680 (wrong title when overwriting saved objects)
friol Aug 29, 2019
cfd882d
As asked by @kobelb, fetching all the titles with one bulk call
friol Aug 30, 2019
5a2e961
Reverted the srcSpaceId changes and modified srcSpaceId in destSpaceI…
friol Sep 5, 2019
ff9c4f0
Merge remote-tracking branch 'upstream/master' into wrongtitlefix
friol Sep 14, 2019
b42bed1
Changes requested in review
friol Sep 15, 2019
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
28 changes: 24 additions & 4 deletions src/core/server/saved_objects/import/extract_errors.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,32 @@ import { SavedObject } from '../types';
import { extractErrors } from './extract_errors';

describe('extractErrors()', () => {
test('returns empty array when no errors exist', () => {
const savedObjectsClient = {
errors: {} as any,
bulkCreate: jest.fn(),
bulkGet: jest.fn(),
create: jest.fn(),
delete: jest.fn(),
find: jest.fn(),
get: jest.fn(),
update: jest.fn(),
};

test('returns empty array when no errors exist', async () => {
const savedObjects: SavedObject[] = [];
const result = extractErrors(savedObjects, savedObjects);
const result = await extractErrors(savedObjects, savedObjects, savedObjectsClient);
expect(result).toMatchInlineSnapshot(`Array []`);
});

test('extracts errors from saved objects', () => {
test('extracts errors from saved objects', async () => {
const clientResponse = {
saved_objects: [
{ id: '2', type: 'dashboard', attributes: { title: 'My Dashboard 2' } },
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change these titles to something like 'Existing Dashboard 2' otherwise we're not testing the existing title finding logic you added. I think this will also surface the the bug where we resolve the existing title of "My Dashboard 3" even though this wasn't a conflict error.

{ id: '3', type: 'dashboard', attributes: { title: 'My Dashboard 3' } },
],
};
savedObjectsClient.bulkGet.mockImplementation(() => Promise.resolve(clientResponse));

const savedObjects: SavedObject[] = [
{
id: '1',
Expand Down Expand Up @@ -62,7 +81,7 @@ describe('extractErrors()', () => {
},
},
];
const result = extractErrors(savedObjects, savedObjects);
const result = await extractErrors(savedObjects, savedObjects, savedObjectsClient);
expect(result).toMatchInlineSnapshot(`
Array [
Object {
Expand All @@ -85,5 +104,6 @@ Array [
},
]
`);
savedObjectsClient.bulkGet.mockReset();
});
});
38 changes: 35 additions & 3 deletions src/core/server/saved_objects/import/extract_errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,47 @@
* specific language governing permissions and limitations
* under the License.
*/
import { SavedObjectsClientContract } from 'src/core/server';
import { SavedObject } from '../types';
import { SavedObjectsImportError } from './types';

export function extractErrors(
export async function extractErrors(
savedObjectResults: SavedObject[],
savedObjectsToImport: SavedObject[]
savedObjectsToImport: SavedObject[],
savedObjectsClient: SavedObjectsClientContract,
namespace?: string
) {
const errors: SavedObjectsImportError[] = [];
const originalSavedObjectsMap = new Map<string, SavedObject>();
const savedObjectsTitlesMap = new Map<string, string>();

for (const savedObject of savedObjectsToImport) {
originalSavedObjectsMap.set(`${savedObject.type}:${savedObject.id}`, savedObject);
}

const errorSavedObjects = new Array();
for (const savedObject of savedObjectResults) {
if (savedObject.error) {
errorSavedObjects.push(savedObject);
}
}

// return an empty array if there are no errors
if (errorSavedObjects.length === 0) {
return [];
}
Comment on lines +37 to +47
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we test for savedObject.error.statusCode === 409 instead of just savedObject.error.

Nit: This code would be much more compact and readable if you used an array.filter like:

const errorSavedObjects = savedObjectResults.filter((so) => so.error && so.error.statusCode === 409)


const bulkGetResult = await savedObjectsClient.bulkGet(errorSavedObjects, {
namespace,
});

for (const savedObject of bulkGetResult.saved_objects) {
savedObjectsTitlesMap.set(
`${savedObject.type}:${savedObject.id}`,
`${savedObject.attributes.title}`
);
}

for (const savedObject of savedObjectResults) {
if (savedObject.error) {
const originalSavedObject = originalSavedObjectsMap.get(
Expand All @@ -37,11 +66,14 @@ export function extractErrors(
originalSavedObject &&
originalSavedObject.attributes &&
originalSavedObject.attributes.title;

if (savedObject.error.statusCode === 409) {
// pick the correct title of the saved object that conflicts
const realTitle = savedObjectsTitlesMap.get(`${savedObject.type}:${savedObject.id}`);
errors.push({
id: savedObject.id,
type: savedObject.type,
title,
title: realTitle,
Copy link
Contributor

Choose a reason for hiding this comment

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

When the import API returns an error, I think the correct response is to include the title of the object that failed to import. The API is saying "I failed while trying to import the title you gave me". Perhaps we can add a new field to the response like existingTitle.

error: {
type: 'conflict',
},
Expand Down
11 changes: 11 additions & 0 deletions src/core/server/saved_objects/import/import_saved_objects.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,17 @@ describe('importSavedObjects()', () => {
},
})),
});

const clientResponse = {
saved_objects: [
{ id: '1', type: 'index-pattern', attributes: { title: 'My Index Pattern' } },
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also change these titles to verify the existing title matching in the test.

{ id: '2', type: 'search', attributes: { title: 'My Search' } },
{ id: '3', type: 'visualization', attributes: { title: 'My Visualization' } },
{ id: '4', type: 'dashboard', attributes: { title: 'My Dashboard' } },
],
};
savedObjectsClient.bulkGet.mockImplementation(() => Promise.resolve(clientResponse));

const result = await importSavedObjects({
readStream,
objectLimit: 4,
Expand Down
11 changes: 7 additions & 4 deletions src/core/server/saved_objects/import/import_saved_objects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,14 @@ export async function importSavedObjects({
overwrite,
namespace,
});
errorAccumulator = [
...errorAccumulator,
...extractErrors(bulkCreateResult.saved_objects, filteredObjects),
];

const soErrors = await extractErrors(
bulkCreateResult.saved_objects,
filteredObjects,
savedObjectsClient,
namespace
);
errorAccumulator = [...errorAccumulator, ...soErrors];
return {
success: errorAccumulator.length === 0,
successCount: bulkCreateResult.saved_objects.filter(obj => !obj.error).length,
Expand Down
11 changes: 11 additions & 0 deletions src/core/server/saved_objects/import/resolve_import_errors.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,17 @@ describe('resolveImportErrors()', () => {
},
})),
});

const clientResponse = {
saved_objects: [
{ id: '1', type: 'index-pattern', attributes: { title: 'My Index Pattern' } },
{ id: '2', type: 'search', attributes: { title: 'My Search' } },
{ id: '3', type: 'visualization', attributes: { title: 'My Visualization' } },
{ id: '4', type: 'dashboard', attributes: { title: 'My Dashboard' } },
],
};

savedObjectsClient.bulkGet.mockImplementation(() => Promise.resolve(clientResponse));
const result = await resolveImportErrors({
readStream,
objectLimit: 4,
Expand Down
22 changes: 14 additions & 8 deletions src/core/server/saved_objects/import/resolve_import_errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,20 +89,26 @@ export async function resolveImportErrors({
overwrite: true,
namespace,
});
errorAccumulator = [
...errorAccumulator,
...extractErrors(bulkCreateResult.saved_objects, objectsToOverwrite),
];

const extractErrs = await extractErrors(
bulkCreateResult.saved_objects,
objectsToOverwrite,
savedObjectsClient
);
errorAccumulator = [...errorAccumulator, ...extractErrs];
successCount += bulkCreateResult.saved_objects.filter(obj => !obj.error).length;
}
if (objectsToNotOverwrite.length) {
const bulkCreateResult = await savedObjectsClient.bulkCreate(objectsToNotOverwrite, {
namespace,
});
errorAccumulator = [
...errorAccumulator,
...extractErrors(bulkCreateResult.saved_objects, objectsToNotOverwrite),
];

const extractErrs = await extractErrors(
bulkCreateResult.saved_objects,
objectsToNotOverwrite,
savedObjectsClient
);
errorAccumulator = [...errorAccumulator, ...extractErrs];
successCount += bulkCreateResult.saved_objects.filter(obj => !obj.error).length;
}

Expand Down