-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Reporting] Config flag to escape formula CSV values #63645
Changes from 5 commits
2ab046e
8ac2a32
8ca9b05
b49c39c
c45b8a6
39bf9f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { startsWith } from 'lodash'; | ||
import { CSV_FORMULA_CHARS } from '../../../../common/constants'; | ||
|
||
export const cellHasFormulas = (val: string) => | ||
CSV_FORMULA_CHARS.some(formulaChar => startsWith(val, formulaChar)); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { i18n } from '@kbn/i18n'; | ||
import { Logger } from '../../../../types'; | ||
import { GenerateCsvParams, SavedSearchGeneratorResult } from '../../types'; | ||
import { createFlattenHit } from './flatten_hit'; | ||
|
@@ -26,14 +27,17 @@ export function createGenerateCsv(logger: Logger) { | |
cancellationToken, | ||
settings, | ||
}: GenerateCsvParams): Promise<SavedSearchGeneratorResult> { | ||
const escapeValue = createEscapeValue(settings.quoteValues); | ||
const escapeValue = createEscapeValue(settings.quoteValues, settings.escapeFormulaValues); | ||
const builder = new MaxSizeStringBuilder(settings.maxSizeBytes); | ||
const header = `${fields.map(escapeValue).join(settings.separator)}\n`; | ||
const warnings: string[] = []; | ||
|
||
if (!builder.tryAppend(header)) { | ||
return { | ||
size: 0, | ||
content: '', | ||
maxSizeReached: true, | ||
warnings: [], | ||
}; | ||
} | ||
|
||
|
@@ -82,11 +86,20 @@ export function createGenerateCsv(logger: Logger) { | |
const size = builder.getSizeInBytes(); | ||
logger.debug(`finished generating, total size in bytes: ${size}`); | ||
|
||
if (csvContainsFormulas && settings.escapeFormulaValues) { | ||
warnings.push( | ||
i18n.translate('xpack.reporting.exportTypes.csv.generateCsv.escapedFormulaValues', { | ||
defaultMessage: 'CSV may contain formulas whose values have been escaped', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we can probably just remove that second line since we have more information in the Job details. We have a task to come back and consolidate all of these warning messages since we now have a method of sending generic messages. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'll follow up with this in another PR |
||
}) | ||
); | ||
} | ||
|
||
return { | ||
content: builder.getString(), | ||
csvContainsFormulas, | ||
csvContainsFormulas: csvContainsFormulas && !settings.escapeFormulaValues, | ||
maxSizeReached, | ||
size, | ||
warnings, | ||
}; | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -114,6 +114,7 @@ const CaptureSchema = schema.object({ | |
|
||
const CsvSchema = schema.object({ | ||
checkForFormulas: schema.boolean({ defaultValue: true }), | ||
escapeFormulaValues: schema.boolean({ defaultValue: false }), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need to be added to This is likely something we'll need to whitelist on Cloud as well There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Looks like the kibana-docker whitelisted settings are out of date for Reporting. I will file and issue and get those synced up. For Cloud reference, here is a template PR for updating the whitelist for cloud: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks Tim! I'll work on whitelisting this in cloud There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. related PR, since the docker stuff was out of date: #63766 |
||
enablePanelActionDownload: schema.boolean({ defaultValue: true }), | ||
maxSizeBytes: schema.number({ | ||
defaultValue: 1024 * 1024 * 10, // 10MB | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about consolidating this with the existing set of characters defined here, and in general consolidating that logic so we check for formulas in a consistent manner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++, will consolidate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consolidated!