diff --git a/lib/doc/pivot-table.js b/lib/doc/pivot-table.js index 9de9688f6..8fad3f7b1 100644 --- a/lib/doc/pivot-table.js +++ b/lib/doc/pivot-table.js @@ -33,10 +33,14 @@ function makePivotTable(worksheet, model) { let {rows, columns, values, pages = []} = model; const {metric, pageDefaults = {}} = model; - // Generate sharedItems for ALL fields in the source, not just the ones used by this pivot table - // This ensures Excel can properly display any field configuration + // Generate sharedItems only for fields used as pivot axes (rows, columns, pages). + // Value fields and unused fields use lightweight sharedItems (no enumeration) — Excel + // flags pivot caches that materialize sharedItems for unused/long-text fields with a + // "Repaired Records: PivotTable report from /xl/pivotCache/pivotCacheDefinition*.xml" + // warning on open (see protobi/exceljs#42). const allHeaderNames = sourceSheet.getRow(1).values.slice(1); - const cacheFields = makeCacheFields(sourceSheet, allHeaderNames); + const axialFieldNames = [...rows, ...columns, ...pages]; + const cacheFields = makeCacheFields(sourceSheet, axialFieldNames, allHeaderNames); // let {rows, columns, values, pages} use indices instead of names; // names can then be accessed via `pivotTable.cacheFields[index].name`. @@ -124,15 +128,14 @@ function validate(worksheet, model) { const allFields = [...model.rows, ...model.columns, ...model.values]; for (const pageField of pages) { if (allFields.includes(pageField)) { - throw new Error( - `Page field "${pageField}" cannot also be used as a row, column, or value field.`, - ); + const msg = `Page field "${pageField}" cannot also be used as a row, column, or value field.`; + throw new Error(msg); } } // Validate pageDefaults reference valid page fields and values if (model.pageDefaults) { - for (const [fieldName, defaultValue] of Object.entries(model.pageDefaults)) { + for (const fieldName of Object.keys(model.pageDefaults)) { if (!pages.includes(fieldName)) { throw new Error(`pageDefaults field "${fieldName}" is not in the pages array.`); } @@ -140,7 +143,8 @@ function validate(worksheet, model) { } } -function makeCacheFields(worksheet, fieldNamesWithSharedItems) { +function makeCacheFields(worksheet, fieldNamesWithSharedItems, allFieldNames /* reserved */) { + // `allFieldNames` is reserved for future use; iteration uses worksheet.getRow(1) // Cache fields are used in pivot tables to reference source data. // // Example @@ -193,12 +197,60 @@ function makeCacheFields(worksheet, fieldNamesWithSharedItems) { return toSortedArray(uniqueValues); }; + // Inspect a column's body values to derive lightweight sharedItems hints + // (containsBlank / containsString / containsNumber / longText) WITHOUT enumerating + // every unique value. Used for fields that aren't pivot axes — Excel can rebuild + // the index on refresh from the inline values written into pivotCacheRecords. + const inspect = columnIndex => { + const columnValues = worksheet.getColumn(columnIndex).values.slice(2); + const hints = { + containsBlank: false, + containsString: false, + containsNumber: false, + containsInteger: true, // assume true until proven false + hasNumber: false, + longText: false, + }; + for (const value of columnValues) { + if (value === null || value === undefined) { + hints.containsBlank = true; + continue; + } + if (typeof value === 'number') { + hints.containsNumber = true; + hints.hasNumber = true; + if (!Number.isInteger(value)) hints.containsInteger = false; + } else if (typeof value === 'string') { + hints.containsString = true; + if (value.length > 255) hints.longText = true; + } else { + // fallback for dates/booleans/etc — treat as string-ish + hints.containsString = true; + } + } + if (!hints.hasNumber) hints.containsInteger = false; + return hints; + }; + // make result const result = []; for (const columnIndex of range(1, names.length)) { const name = names[columnIndex]; - const sharedItems = nameToHasSharedItems[name] ? aggregate(columnIndex) : null; - result.push({name, sharedItems}); + let sharedItems; + if (nameToHasSharedItems[name]) { + // Axial field — needs full enumeration so pivotCacheRecords can reference + // values via indices. + sharedItems = aggregate(columnIndex); + } else { + // Non-axial field — emit lightweight sharedItems (type hints only). + // pivotCacheRecords will inline values rather than reference indices. + sharedItems = null; + } + const cacheField = {name, sharedItems}; + if (sharedItems === null) { + cacheField.hints = inspect(columnIndex); + } + result.push(cacheField); } return result; } diff --git a/lib/xlsx/xform/pivot-table/cache-field.js b/lib/xlsx/xform/pivot-table/cache-field.js index 8dcea835e..361b1de01 100644 --- a/lib/xlsx/xform/pivot-table/cache-field.js +++ b/lib/xlsx/xform/pivot-table/cache-field.js @@ -1,6 +1,6 @@ class CacheField { - constructor({name, sharedItems}) { - // string type + constructor({name, sharedItems, hints}) { + // string type (axial field — used as pivot row/column/page) // // { // 'name': 'A', @@ -9,14 +9,22 @@ class CacheField { // // or // - // integer type + // non-axial field (value field or unused) — sharedItems is null and `hints` + // describes the column's value types so we can emit a lightweight + // declaration without enumerating every unique value. + // This avoids triggering Excel's "Repaired Records: PivotTable report" + // warning on pivot caches with high-cardinality / long-text non-axial + // fields (see protobi/exceljs#42). // // { // 'name': 'D', - // 'sharedItems': null + // 'sharedItems': null, + // 'hints': { containsBlank, containsString, containsNumber, + // containsInteger, hasNumber, longText } // } this.name = name; this.sharedItems = sharedItems; + this.hints = hints || null; } // Helper function to escape XML special characters @@ -34,20 +42,40 @@ class CacheField { // PivotCache Field: http://www.datypic.com/sc/ooxml/e-ssml_cacheField-1.html // Shared Items: http://www.datypic.com/sc/ooxml/e-ssml_sharedItems-1.html - // integer types + // Non-axial field — emit lightweight based on hints from + // column inspection. pivotCacheRecords inlines values for these fields + // (see pivot-cache-records-xform.js + lib/doc/pivot-table.js). if (this.sharedItems === null) { - // TK(2023-07-18): left out attributes... minValue="5" maxValue="45" + const hints = this.hints || {}; + const attrs = []; + // Pure-numeric column: declare it explicitly so Excel doesn't infer mixed. + if (hints.containsNumber && !hints.containsString) { + attrs.push('containsSemiMixedTypes="0"'); + attrs.push('containsString="0"'); + attrs.push('containsNumber="1"'); + if (hints.containsInteger) attrs.push('containsInteger="1"'); + if (hints.containsBlank) attrs.push('containsBlank="1"'); + } else { + // Pure-string or mixed column. Excel's parser is strict about which + // attributes appear together; the safest minimal form is just the + // hints that matter — no count, no enumerated items. + if (hints.containsBlank) attrs.push('containsBlank="1"'); + if (hints.longText) attrs.push('longText="1"'); + // Mixed string+number: omit containsString/containsNumber to let Excel + // infer from the inline records; declaring both can disagree with the + // record contents and trigger repair. + } + const attrStr = attrs.length ? ` ${attrs.join(' ')}` : ''; return ` - + `; } // Check if all items are numeric - const allNumeric = this.sharedItems.every( - item => - typeof item === 'number' || - (typeof item === 'string' && !isNaN(item) && item.trim() !== ''), - ); + const isNumericItem = item => + typeof item === 'number' || + (typeof item === 'string' && !Number.isNaN(Number(item)) && item.trim() !== ''); + const allNumeric = this.sharedItems.every(isNumericItem); if (allNumeric) { // numeric types - use tags diff --git a/lib/xlsx/xform/pivot-table/pivot-cache-definition-xform.js b/lib/xlsx/xform/pivot-table/pivot-cache-definition-xform.js index 18f4ef379..880815193 100644 --- a/lib/xlsx/xform/pivot-table/pivot-cache-definition-xform.js +++ b/lib/xlsx/xform/pivot-table/pivot-cache-definition-xform.js @@ -21,6 +21,13 @@ class PivotCacheDefinitionXform extends BaseXform { render(xmlStream, model) { const {sourceSheet, cacheFields} = model; + // recordCount must match the count attribute of — the + // number of data rows in the source sheet (header excluded). Excel uses + // this to validate the cache; a mismatch is one of the triggers for the + // "Repaired Records: PivotTable report from /xl/pivotCache/ + // pivotCacheDefinition*.xml" warning (see protobi/exceljs#42). + const recordCount = Math.max(0, sourceSheet.getSheetValues().length - 2); + xmlStream.openXml(XmlStream.StdDocAttributes); xmlStream.openNode(this.tag, { ...PivotCacheDefinitionXform.PIVOT_CACHE_DEFINITION_ATTRIBUTES, @@ -31,7 +38,7 @@ class PivotCacheDefinitionXform extends BaseXform { createdVersion: '8', refreshedVersion: '8', minRefreshableVersion: '3', - recordCount: cacheFields.length + 1, + recordCount, }); xmlStream.openNode('cacheSource', {type: 'worksheet'}); @@ -43,7 +50,10 @@ class PivotCacheDefinitionXform extends BaseXform { xmlStream.openNode('cacheFields', {count: cacheFields.length}); // Note: keeping this pretty-printed for now to ease debugging. - xmlStream.writeXml(cacheFields.map(cacheField => new CacheField(cacheField).render()).join('\n ')); + const cacheFieldsXml = cacheFields + .map(cacheField => new CacheField(cacheField).render()) + .join('\n '); + xmlStream.writeXml(cacheFieldsXml); xmlStream.closeNode(); xmlStream.closeNode(); diff --git a/spec/integration/issues/issue-42-pivot-cache-repair.spec.js b/spec/integration/issues/issue-42-pivot-cache-repair.spec.js new file mode 100644 index 000000000..c20a53b79 --- /dev/null +++ b/spec/integration/issues/issue-42-pivot-cache-repair.spec.js @@ -0,0 +1,162 @@ +// Regression test for protobi/exceljs#42: +// "[BUG] Pivot cache triggers Excel repair" +// +// When a workbook contains a pivot table whose source sheet has fields that +// are NOT used as pivot axes (rows/columns/pages) and contain high-cardinality +// or long-text values (e.g. a "Description" column), Excel opens the file with: +// +// "Repaired Records: PivotTable report from /xl/pivotCache/ +// pivotCacheDefinition1.xml part (PivotTable cache)" +// +// Root cause: pivotCacheDefinition fully materialized for every +// source column, including non-axial fields. Excel's strict cache validator +// flags this — and it's also wasteful (1500-row long-text columns produced +// ~900KB cache definitions that were never indexed by anything). +// +// Fix: only enumerate sharedItems for fields actually used as pivot axes. +// Non-axial fields get a lightweight declaration with type +// hints; pivotCacheRecords inlines their values (//) rather than +// referencing them by index. recordCount on the cache definition is also +// corrected to match the data row count (was previously cacheFields.length+1). + +const fs = require('fs'); +const {promisify} = require('util'); + +const fsReadFileAsync = promisify(fs.readFile); + +const JSZip = require('jszip'); + +const ExcelJS = verquire('exceljs'); + +const TEST_XLSX_FILEPATH = './spec/out/wb.issue-42.xlsx'; + +function findCacheField(xml, name) { + const re = new RegExp(`]*>[\\s\\S]*?`); + return xml.match(re); +} + +describe('Workbook', () => { + describe('Pivot Tables (issue #42 — Excel repair on cache definition)', () => { + let zip; + let cacheDefXml; + let cacheRecordsXml; + const ROW_COUNT = 200; + + before(async () => { + const workbook = new ExcelJS.Workbook(); + const dataSheet = workbook.addWorksheet('Data'); + dataSheet.columns = [ + {header: 'Repo', key: 'repo', width: 20}, + {header: 'Group', key: 'group', width: 20}, + {header: 'Severity', key: 'severity', width: 20}, + // Description: long-text, high-cardinality, NOT used as a pivot axis. + // This is the field that triggers the bug. + {header: 'Description', key: 'description', width: 80}, + {header: 'Counter', key: 'count', width: 10}, + ]; + + for (let i = 0; i < ROW_COUNT; i++) { + dataSheet.addRow({ + repo: `repo-${i % 10}`, + group: `group-${i % 20}`, + severity: ['CRITICAL', 'HIGH', 'MEDIUM', 'LOW', 'INFO'][i % 5], + description: `long-description-text-${i}-${'x'.repeat(300)}`, + count: 1, + }); + } + + const pivotSheet = workbook.addWorksheet('Summary'); + pivotSheet.addPivotTable({ + sourceSheet: dataSheet, + rows: ['Repo', 'Group'], + columns: ['Severity'], + values: ['Counter'], + metric: 'sum', + }); + + await workbook.xlsx.writeFile(TEST_XLSX_FILEPATH); + const buffer = await fsReadFileAsync(TEST_XLSX_FILEPATH); + zip = await JSZip.loadAsync(buffer); + cacheDefXml = await zip.file('xl/pivotCache/pivotCacheDefinition1.xml').async('string'); + cacheRecordsXml = await zip.file('xl/pivotCache/pivotCacheRecords1.xml').async('string'); + }); + + it('emits a pivot cache definition', () => { + expect(cacheDefXml).to.be.a('string'); + expect(cacheDefXml).to.include(' { + // Was previously `cacheFields.length + 1` (= 6 for this fixture); + // Excel expects it to match the attribute, + // which is the number of data rows. + const match = cacheDefXml.match(/recordCount="(\d+)"/); + expect(match).to.not.be.null(); + expect(Number(match[1])).to.equal(ROW_COUNT); + + const recordsCountMatch = cacheRecordsXml.match(/]*count="(\d+)"/); + expect(recordsCountMatch).to.not.be.null(); + expect(Number(recordsCountMatch[1])).to.equal(ROW_COUNT); + }); + + it('does NOT enumerate sharedItems for the non-axial Description field', () => { + // The Description cacheField should appear with a lightweight + // (self-closing or with attributes only) — it must + // NOT contain children. + const descMatch = findCacheField(cacheDefXml, 'Description'); + expect(descMatch, 'Description cacheField missing').to.not.be.null(); + const descBlock = descMatch[0]; + expect(descBlock).to.not.include('/); + expect(counterBlock).to.match(/]*\/>/); + }); + + it('keeps full sharedItems enumeration for axial Repo/Group/Severity fields', () => { + for (const fieldName of ['Repo', 'Group', 'Severity']) { + const block = findCacheField(cacheDefXml, fieldName); + expect(block, `${fieldName} cacheField missing`).to.not.be.null(); + // Axial fields keep full ... + expect(block[0]).to.include(''); + expect(block[0]).to.match(/ indices) in pivotCacheRecords for non-axial fields', () => { + // Each row should have 5 cells: (Repo) (Group) (Severity) (Desc) (Counter) + // Pull the first ... record and verify shape. + const firstRecord = cacheRecordsXml.match(/([\s\S]*?)<\/r>/); + expect(firstRecord, 'no record found').to.not.be.null(); + const body = firstRecord[1]; + // Three indices for the three axial fields: + const xCount = (body.match(/ for Description: + expect(body).to.match(/ { + // Pre-fix, this fixture (200 rows × ~325-char descriptions) produced a + // ~75 KB pivotCacheDefinition. Post-fix it should be a few KB at most, + // because the giant Description sharedItems block is gone. + expect(cacheDefXml.length).to.be.lessThan(20000); + }); + + it('round-trip-loads the resulting workbook without throwing', async () => { + const reloaded = new ExcelJS.Workbook(); + await reloaded.xlsx.readFile(TEST_XLSX_FILEPATH); + expect(reloaded.worksheets.map(ws => ws.name)).to.include('Data'); + expect(reloaded.worksheets.map(ws => ws.name)).to.include('Summary'); + }); + }); +});