Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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);
}
}),
Comment on lines +31 to +39
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Soooo, long story short:

Uncatched errors from streams (either unhandled promises or plain errors) are causing the process to crash due to unhandled errors (forget streams were THAT much of a pain since I've been using observables)

The implementation of createPromiseFromStreams that is used here

const collectedObjects: Array<SavedObject<{ title?: string }>> = await createPromiseFromStreams([
readStream,

relies on stream.pipeline under the hood, that is supposed to intercept errors occurring during the stream/transform chain, however errors that occurred within a pipe of the argument observable are not.

The direct consequence is that any error thrown from the piping chain that is done in createSavedObjectsStreamFromNdJson was uncatched, therefor killing the server.

The correct solution would probably be to stop using streams and moving to observable instead, but these are significant changes (and collectSavedObjects is also used by the spaces plugin when copying to space), so the only solution I found for now was to collect the initial saved objects inside createSavedObjectsStreamFromNdJson and recreate a stream from that. That way, errors during the createSavedObjectsStreamFromNdJson stream chain are properly thrown via the added await createPromiseFromStreams. A FTR test has been added to assert that.

Note that this doesn't change the performances or memory usage: we were already collecting all emissions of the SO stream here before:

const collectedObjects: Array<SavedObject<{ title?: string }>> = await createPromiseFromStreams([

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

I suspect if we're adding more saved object types to the export (to allow us to deprecate multi-tenancy) we might want to support real streaming imports/exports again and that will maybe allow us to remove the import payload size limit completely. But that's something to revisit once we know this is a requirement.

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"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks 😄

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