Skip to content

Commit

Permalink
Treat csvs with formulas differently than those that aren't escaped
Browse files Browse the repository at this point in the history
  • Loading branch information
Joel Griffith committed Apr 16, 2020
1 parent b49c39c commit c45b8a6
Show file tree
Hide file tree
Showing 8 changed files with 65 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ kibana_vars=(
xpack.reporting.capture.viewport.width
xpack.reporting.capture.zoom
xpack.reporting.csv.checkForFormulas
xpack.reporting.csv.escapeFormulaValues
xpack.reporting.csv.enablePanelActionDownload
xpack.reporting.csv.maxSizeBytes
xpack.reporting.csv.scroll.duration
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ describe('CSV Execute Job', function() {
});
});

describe('Cells with formula values', () => {
describe('Warning when cells have formulas', () => {
it('returns `csv_contains_formulas` when cells contain formulas', async function() {
configGetStub.withArgs('csv', 'checkForFormulas').returns(true);
callAsCurrentUserStub.onFirstCall().returns({
Expand Down Expand Up @@ -353,6 +353,7 @@ describe('CSV Execute Job', function() {

it('returns no warnings when cells have no formulas', async function() {
configGetStub.withArgs('csv', 'checkForFormulas').returns(true);
configGetStub.withArgs('csv', 'escapeFormulaValues').returns(false);
callAsCurrentUserStub.onFirstCall().returns({
hits: {
hits: [{ _source: { one: 'foo', two: 'bar' } }],
Expand All @@ -376,6 +377,33 @@ describe('CSV Execute Job', function() {
expect(csvContainsFormulas).toEqual(false);
});

it('returns no warnings when cells have formulas but are escaped', async function() {
configGetStub.withArgs('csv', 'checkForFormulas').returns(true);
configGetStub.withArgs('csv', 'escapeFormulaValues').returns(true);
callAsCurrentUserStub.onFirstCall().returns({
hits: {
hits: [{ _source: { '=SUM(A1:A2)': 'foo', two: 'bar' } }],
},
_scroll_id: 'scrollId',
});

const executeJob = await executeJobFactory(mockReportingPlugin, mockLogger);
const jobParams = getJobDocPayload({
headers: encryptedHeaders,
fields: ['=SUM(A1:A2)', 'two'],
conflictedTypesFields: [],
searchRequest: { index: null, body: null },
});

const { csv_contains_formulas: csvContainsFormulas } = await executeJob(
'job123',
jobParams,
cancellationToken
);

expect(csvContainsFormulas).toEqual(false);
});

it('returns no warnings when configured not to', async () => {
configGetStub.withArgs('csv', 'checkForFormulas').returns(false);
callAsCurrentUserStub.onFirstCall().returns({
Expand Down Expand Up @@ -446,7 +474,7 @@ describe('CSV Execute Job', function() {
});
});

describe('Formula values', () => {
describe('Escaping cells with formulas', () => {
it('escapes values with formulas', async () => {
configGetStub.withArgs('csv', 'escapeFormulaValues').returns(true);
callAsCurrentUserStub.onFirstCall().returns({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ export const executeJobFactory: ExecuteJobFactory<ESQueueWorkerExecuteFn<
const generateCsv = createGenerateCsv(jobLogger);
const bom = config.get('csv', 'useByteOrderMarkEncoding') ? CSV_BOM_CHARS : '';

const { content, maxSizeReached, size, csvContainsFormulas } = await generateCsv({
const { content, maxSizeReached, size, csvContainsFormulas, warnings } = await generateCsv({
searchRequest,
fields,
metaFields,
Expand All @@ -140,12 +140,14 @@ export const executeJobFactory: ExecuteJobFactory<ESQueueWorkerExecuteFn<
},
});

// @TODO: Consolidate these one-off warnings into the warnings array (max-size reached and csv contains formulas)
return {
content_type: 'text/csv',
content: bom + content,
max_size_reached: maxSizeReached,
size,
csv_contains_formulas: csvContainsFormulas,
warnings,
};
};
};
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
Expand Up @@ -5,16 +5,15 @@
*/

import * as _ from 'lodash';

const formulaValues = ['=', '+', '-', '@'];
import { cellHasFormulas } from './cell_has_formula';

interface IFlattened {
[header: string]: string;
}

export const checkIfRowsHaveFormulas = (flattened: IFlattened, fields: string[]) => {
const pruned = _.pick(flattened, fields);
const csvValues = [..._.keys(pruned), ...(_.values(pruned) as string[])];
const cells = [..._.keys(pruned), ...(_.values(pruned) as string[])];

return _.some(csvValues, cell => _.some(formulaValues, char => _.startsWith(cell, char)));
return _.some(cells, cell => cellHasFormulas(cell));
};
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,18 @@
*/

import { RawValue } from './types';
import { CSV_FORMULA_CHARS } from '../../../../common/constants';
import { cellHasFormulas } from './cell_has_formula';

const nonAlphaNumRE = /[^a-zA-Z0-9]/;
const allDoubleQuoteRE = /"/g;

const valHasFormulas = (val: string) =>
CSV_FORMULA_CHARS.some(formulaChar => val.startsWith(formulaChar));

export function createEscapeValue(
quoteValues: boolean,
escapeFormulas: boolean
): (val: RawValue) => string {
return function escapeValue(val: RawValue) {
if (val && typeof val === 'string') {
const formulasEscaped = escapeFormulas && valHasFormulas(val) ? "'" + val : val;
const formulasEscaped = escapeFormulas && cellHasFormulas(val) ? "'" + val : val;
if (quoteValues && nonAlphaNumRE.test(formulasEscaped)) {
return `"${formulasEscaped.replace(allDoubleQuoteRE, '""')}"`;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -29,11 +30,14 @@ export function createGenerateCsv(logger: Logger) {
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: [],
};
}

Expand Down Expand Up @@ -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',
})
);
}

return {
content: builder.getString(),
csvContainsFormulas,
csvContainsFormulas: csvContainsFormulas && !settings.escapeFormulaValues,
maxSizeReached,
size,
warnings,
};
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ export interface SavedSearchGeneratorResult {
size: number;
maxSizeReached: boolean;
csvContainsFormulas?: boolean;
warnings: string[];
}

export interface CsvResultFromSearch {
Expand Down

0 comments on commit c45b8a6

Please sign in to comment.