Skip to content

Commit

Permalink
SavedObjects management: display explicit import error in case of fai…
Browse files Browse the repository at this point in the history
…lure and avoid invalid file to crash the server (#82406)

* display error cause in case of import failure

* avoid server crash when importing invalid file

* remove unused translations

* fix unit tests

* change savedObjects.maxImportPayloadBytes default to 25mb

* fix types and logic
  • Loading branch information
pgayvallet authored Nov 4, 2020
1 parent c3f4024 commit e2cbde3
Show file tree
Hide file tree
Showing 24 changed files with 165 additions and 46 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ const createStartContractMock = () => {
},
savedObjects: {
maxImportExportSizeBytes: 10000,
maxImportPayloadBytes: 10485760,
maxImportPayloadBytes: 26214400,
},
},
environment: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ describe('CoreUsageDataService', () => {
},
"savedObjects": Object {
"maxImportExportSizeBytes": 10000,
"maxImportPayloadBytes": 10485760,
"maxImportPayloadBytes": 26214400,
},
},
"environment": Object {
Expand Down
2 changes: 1 addition & 1 deletion src/core/server/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ export function pluginInitializerContextConfigMock<T>(config: T) {
},
path: { data: '/tmp' },
savedObjects: {
maxImportPayloadBytes: new ByteSizeValue(10485760),
maxImportPayloadBytes: new ByteSizeValue(26214400),
},
};

Expand Down
2 changes: 1 addition & 1 deletion src/core/server/plugins/plugin_context.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ describe('createPluginInitializerContext', () => {
pingTimeout: duration(30, 's'),
},
path: { data: fromRoot('data') },
savedObjects: { maxImportPayloadBytes: new ByteSizeValue(10485760) },
savedObjects: { maxImportPayloadBytes: new ByteSizeValue(26214400) },
});
});

Expand Down
11 changes: 10 additions & 1 deletion src/core/server/saved_objects/routes/import.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,19 @@ export const registerImportRoute = (router: IRouter, config: SavedObjectConfig)
return res.badRequest({ body: `Invalid file extension ${fileExtension}` });
}

let readStream: Readable;
try {
readStream = await createSavedObjectsStreamFromNdJson(file);
} catch (e) {
return res.badRequest({
body: e,
});
}

const result = await importSavedObjectsFromStream({
savedObjectsClient: context.core.savedObjects.client,
typeRegistry: context.core.savedObjects.typeRegistry,
readStream: createSavedObjectsStreamFromNdJson(file),
readStream,
objectLimit: maxImportExportSize,
overwrite,
createNewCopies,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ type SetupServerReturn = UnwrapPromise<ReturnType<typeof setupServer>>;
const exportSavedObjectsToStream = exportMock.exportSavedObjectsToStream as jest.Mock;
const allowedTypes = ['index-pattern', 'search'];
const config = {
maxImportPayloadBytes: 10485760,
maxImportPayloadBytes: 26214400,
maxImportExportSize: 10000,
} as SavedObjectConfig;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ type SetupServerReturn = UnwrapPromise<ReturnType<typeof setupServer>>;

const { v4: uuidv4 } = jest.requireActual('uuid');
const allowedTypes = ['index-pattern', 'visualization', 'dashboard'];
const config = { maxImportPayloadBytes: 10485760, maxImportExportSize: 10000 } as SavedObjectConfig;
const config = { maxImportPayloadBytes: 26214400, maxImportExportSize: 10000 } as SavedObjectConfig;
const URL = '/internal/saved_objects/_import';

describe(`POST ${URL}`, () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ type SetupServerReturn = UnwrapPromise<ReturnType<typeof setupServer>>;

const { v4: uuidv4 } = jest.requireActual('uuid');
const allowedTypes = ['index-pattern', 'visualization', 'dashboard'];
const config = { maxImportPayloadBytes: 10485760, maxImportExportSize: 10000 } as SavedObjectConfig;
const config = { maxImportPayloadBytes: 26214400, maxImportExportSize: 10000 } as SavedObjectConfig;
const URL = '/api/saved_objects/_resolve_import_errors';

describe(`POST ${URL}`, () => {
Expand Down
11 changes: 10 additions & 1 deletion src/core/server/saved_objects/routes/resolve_import_errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,19 @@ export const registerResolveImportErrorsRoute = (router: IRouter, config: SavedO
return res.badRequest({ body: `Invalid file extension ${fileExtension}` });
}

let readStream: Readable;
try {
readStream = await createSavedObjectsStreamFromNdJson(file);
} catch (e) {
return res.badRequest({
body: e,
});
}

const result = await resolveSavedObjectsImportErrors({
typeRegistry: context.core.savedObjects.typeRegistry,
savedObjectsClient: context.core.savedObjects.client,
readStream: createSavedObjectsStreamFromNdJson(file),
readStream,
retries: req.body.retries,
objectLimit: maxImportExportSize,
createNewCopies: req.query.createNewCopies,
Expand Down
6 changes: 3 additions & 3 deletions src/core/server/saved_objects/routes/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ async function readStreamToCompletion(stream: Readable) {

describe('createSavedObjectsStreamFromNdJson', () => {
it('transforms an ndjson stream into a stream of saved objects', async () => {
const savedObjectsStream = createSavedObjectsStreamFromNdJson(
const savedObjectsStream = await createSavedObjectsStreamFromNdJson(
new Readable({
read() {
this.push('{"id": "foo", "type": "foo-type"}\n');
Expand All @@ -52,7 +52,7 @@ describe('createSavedObjectsStreamFromNdJson', () => {
});

it('skips empty lines', async () => {
const savedObjectsStream = createSavedObjectsStreamFromNdJson(
const savedObjectsStream = await createSavedObjectsStreamFromNdJson(
new Readable({
read() {
this.push('{"id": "foo", "type": "foo-type"}\n');
Expand All @@ -79,7 +79,7 @@ describe('createSavedObjectsStreamFromNdJson', () => {
});

it('filters the export details entry from the stream', async () => {
const savedObjectsStream = createSavedObjectsStreamFromNdJson(
const savedObjectsStream = await createSavedObjectsStreamFromNdJson(
new Readable({
read() {
this.push('{"id": "foo", "type": "foo-type"}\n');
Expand Down
39 changes: 23 additions & 16 deletions src/core/server/saved_objects/routes/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,23 +19,30 @@

import { Readable } from 'stream';
import { SavedObject, SavedObjectsExportResultDetails } from 'src/core/server';
import { createSplitStream, createMapStream, createFilterStream } from '../../utils/streams';
import {
createSplitStream,
createMapStream,
createFilterStream,
createPromiseFromStreams,
createListStream,
createConcatStream,
} from '../../utils/streams';

export function createSavedObjectsStreamFromNdJson(ndJsonStream: Readable) {
return ndJsonStream
.pipe(createSplitStream('\n'))
.pipe(
createMapStream((str: string) => {
if (str && str.trim() !== '') {
return JSON.parse(str);
}
})
)
.pipe(
createFilterStream<SavedObject | SavedObjectsExportResultDetails>(
(obj) => !!obj && !(obj as SavedObjectsExportResultDetails).exportedCount
)
);
export async function createSavedObjectsStreamFromNdJson(ndJsonStream: Readable) {
const savedObjects = await createPromiseFromStreams([
ndJsonStream,
createSplitStream('\n'),
createMapStream((str: string) => {
if (str && str.trim() !== '') {
return JSON.parse(str);
}
}),
createFilterStream<SavedObject | SavedObjectsExportResultDetails>(
(obj) => !!obj && !(obj as SavedObjectsExportResultDetails).exportedCount
),
createConcatStream([]),
]);
return createListStream(savedObjects);
}

export function validateTypes(types: string[], supportedTypes: string[]): string | undefined {
Expand Down
2 changes: 1 addition & 1 deletion src/core/server/saved_objects/saved_objects_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export type SavedObjectsConfigType = TypeOf<typeof savedObjectsConfig.schema>;
export const savedObjectsConfig = {
path: 'savedObjects',
schema: schema.object({
maxImportPayloadBytes: schema.byteSize({ defaultValue: 10485760 }),
maxImportPayloadBytes: schema.byteSize({ defaultValue: 26214400 }),
maxImportExportSize: schema.byteSize({ defaultValue: 10000 }),
}),
};
Expand Down
15 changes: 15 additions & 0 deletions src/core/server/utils/streams/map_stream.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,4 +58,19 @@ describe('createMapStream()', () => {

expect(result).toEqual([0, 2, 6]);
});

test('handles errors in async mappers', async () => {
await expect(
createPromiseFromStreams([
createListStream([1, 2, 3]),
createMapStream(async (n: number, i: number) => {
if (n === 2) {
await Promise.reject(new Error('that went bad'));
}
return n;
}),
createConcatStream([]),
])
).rejects.toMatchInlineSnapshot(`[Error: that went bad]`);
});
});

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,9 @@ describe('Flyout', () => {
// Ensure all promises resolve
await new Promise((resolve) => process.nextTick(resolve));

expect(component.state('error')).toEqual('foobar');
expect(component.state('error')).toMatchInlineSnapshot(
`"The file could not be processed due to error: \\"foobar\\""`
);
expect(component.find('EuiFlyoutBody EuiCallOut')).toMatchSnapshot();
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,17 @@ interface ConflictingRecord {
done: (result: [boolean, string | undefined]) => void;
}

const getErrorMessage = (e: any) => {
const errorMessage =
e.body?.error && e.body?.message ? `${e.body.error}: ${e.body.message}` : e.message;
return i18n.translate('savedObjectsManagement.objectsTable.flyout.importFileErrorMessage', {
defaultMessage: 'The file could not be processed due to error: "{error}"',
values: {
error: errorMessage,
},
});
};

export class Flyout extends Component<FlyoutProps, FlyoutState> {
constructor(props: FlyoutProps) {
super(props);
Expand Down Expand Up @@ -183,9 +194,7 @@ export class Flyout extends Component<FlyoutProps, FlyoutState> {
} catch (e) {
this.setState({
status: 'error',
error: i18n.translate('savedObjectsManagement.objectsTable.flyout.importFileErrorMessage', {
defaultMessage: 'The file could not be processed.',
}),
error: getErrorMessage(e),
});
return;
}
Expand Down Expand Up @@ -241,10 +250,7 @@ export class Flyout extends Component<FlyoutProps, FlyoutState> {
} catch (e) {
this.setState({
status: 'error',
error: i18n.translate(
'savedObjectsManagement.objectsTable.flyout.resolveImportErrorsFileErrorMessage',
{ defaultMessage: 'The file could not be processed.' }
),
error: getErrorMessage(e),
});
}
};
Expand Down Expand Up @@ -437,8 +443,8 @@ export class Flyout extends Component<FlyoutProps, FlyoutState> {
);
} catch (e) {
this.setState({
error: e.message,
status: 'error',
error: getErrorMessage(e),
loadingMessage: undefined,
});
return;
Expand Down Expand Up @@ -605,7 +611,7 @@ export class Flyout extends Component<FlyoutProps, FlyoutState> {
}
color="danger"
>
<p>{error}</p>
<p data-test-subj="importSavedObjectsErrorText">{error}</p>
</EuiCallOut>
<EuiSpacer size="s" />
</Fragment>
Expand Down Expand Up @@ -759,6 +765,7 @@ export class Flyout extends Component<FlyoutProps, FlyoutState> {
}
>
<EuiFilePicker
accept=".ndjson, .json"
fullWidth
initialPromptText={
<FormattedMessage
Expand Down
38 changes: 38 additions & 0 deletions test/functional/apps/management/_import_objects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,44 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {

expect(selectedIdForMissingIndexPattern7).to.eql('f1e4c910-a2e6-11e7-bb30-233be9be6a87');
});

it('should display an explicit error message when importing object from a higher Kibana version', async () => {
await PageObjects.savedObjects.importFile(
path.join(__dirname, 'exports', '_import_higher_version.ndjson')
);

await PageObjects.savedObjects.checkImportError();

const errorText = await PageObjects.savedObjects.getImportErrorText();

expect(errorText).to.contain(
`has property "visualization" which belongs to a more recent version of Kibana [9.15.82]`
);
});

it('should display an explicit error message when importing a file bigger than allowed', async () => {
await PageObjects.savedObjects.importFile(
path.join(__dirname, 'exports', '_import_too_big.ndjson')
);

await PageObjects.savedObjects.checkImportError();

const errorText = await PageObjects.savedObjects.getImportErrorText();

expect(errorText).to.contain(`Payload content length greater than maximum allowed`);
});

it('should display an explicit error message when importing an invalid file', async () => {
await PageObjects.savedObjects.importFile(
path.join(__dirname, 'exports', '_import_invalid_format.ndjson')
);

await PageObjects.savedObjects.checkImportError();

const errorText = await PageObjects.savedObjects.getImportErrorText();

expect(errorText).to.contain(`Unexpected token T in JSON at position 0`);
});
});
});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"attributes":{"description":"","kibanaSavedObjectMeta":{"searchSourceJSON":"{\"filter\":[],\"query\":{\"query\":\"\",\"language\":\"lucene\"},\"indexRefName\":\"kibanaSavedObjectMeta.searchSourceJSON.index\"}"},"title":"Log Agents","uiStateJSON":"{}","visState":"{\"title\":\"Log Agents\",\"type\":\"area\",\"params\":{\"type\":\"area\",\"grid\":{\"categoryLines\":false,\"style\":{\"color\":\"#eee\"}},\"categoryAxes\":[{\"id\":\"CategoryAxis-1\",\"type\":\"category\",\"position\":\"bottom\",\"show\":true,\"style\":{},\"scale\":{\"type\":\"linear\"},\"labels\":{\"show\":true,\"truncate\":100},\"title\":{\"text\":\"agent.raw: Descending\"}}],\"valueAxes\":[{\"id\":\"ValueAxis-1\",\"name\":\"LeftAxis-1\",\"type\":\"value\",\"position\":\"left\",\"show\":true,\"style\":{},\"scale\":{\"type\":\"linear\",\"mode\":\"normal\"},\"labels\":{\"show\":true,\"rotate\":0,\"filter\":false,\"truncate\":100},\"title\":{\"text\":\"Count\"}}],\"seriesParams\":[{\"show\":\"true\",\"type\":\"area\",\"mode\":\"stacked\",\"data\":{\"label\":\"Count\",\"id\":\"1\"},\"drawLinesBetweenPoints\":true,\"showCircles\":true,\"interpolate\":\"linear\",\"valueAxis\":\"ValueAxis-1\"}],\"addTooltip\":true,\"addLegend\":true,\"legendPosition\":\"right\",\"times\":[],\"addTimeMarker\":false},\"aggs\":[{\"id\":\"1\",\"enabled\":true,\"type\":\"count\",\"schema\":\"metric\",\"params\":{}},{\"id\":\"2\",\"enabled\":true,\"type\":\"terms\",\"schema\":\"segment\",\"params\":{\"field\":\"agent.raw\",\"size\":5,\"order\":\"desc\",\"orderBy\":\"1\"}}]}"},"id":"082f1d60-a2e7-11e7-bb30-233be9be6a15","migrationVersion":{"visualization":"9.15.82"},"references":[],"type":"visualization","version":1}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
This is NOT a ndjson file!
Loading

0 comments on commit e2cbde3

Please sign in to comment.