Skip to content

Commit 5ed2957

Browse files
caio-pizzolcaio-pizzol
committed
fix(super-converter): keep the document when a style record is unreadable
The style catalogue is built outside the importer's per-node recovery boundary, so one unreadable `w:style` aborted createDocumentJson. getSchema then returned null and createDocument fell back to an empty document, discarding the whole body instead of a single node. Guarding the crash was not enough on its own: an outline level with a missing or non-numeric w:val still parsed to NaN, and a tab stop missing w:val or w:pos was still emitted half-formed. Both attributes are required in ECMA-376, so neither record carries anything to apply and both are now dropped. Also skips style records with no w:styleId, since nothing in the document can reference them, and removes the latent-style match loop that collected into a list nothing ever read.
1 parent 1f5e584 commit 5ed2957

4 files changed

Lines changed: 237 additions & 99 deletions

File tree

‎packages/super-editor/src/editors/v1/core/super-converter/docx-helpers/get-default-style-definition.empty-style-properties.test.js‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
// @ts-check
2+
import { describe, expect, it } from 'vitest';
3+
import * as xmljs from 'xml-js';
4+
import { getDefaultStyleDefinition } from './get-default-style-definition.js';
5+
6+
/**
7+
* Incomplete leaf properties in `word/styles.xml` (issue #3861).
8+
*
9+
* AIDEV-NOTE: Asserts the parsed values, not just that parsing survived. An
10+
* earlier pass returned NaN for outlineLevel and half-formed tab stops, both of
11+
* which a survival-only test would have missed. Fixtures parse real XML because
12+
* xml-js omits the `elements` key on empty elements.
13+
*/
14+
15+
const WORDPROCESSING_NS = 'http://schemas.openxmlformats.org/wordprocessingml/2006/main';
16+
17+
/** @param {string} styleChildren Raw XML placed inside `<w:style w:styleId="Target">`. */
18+
const parseStyle = (styleChildren) =>
19+
getDefaultStyleDefinition('Target', {
20+
'word/styles.xml': xmljs.xml2js(
21+
`<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
22+
<w:styles xmlns:w="${WORDPROCESSING_NS}">
23+
<w:style w:type="paragraph" w:styleId="Target">${styleChildren}</w:style>
24+
</w:styles>`,
25+
{ compact: false },
26+
),
27+
});
28+
29+
describe('getDefaultStyleDefinition with incomplete properties', () => {
30+
describe('w:outlineLvl', () => {
31+
it('reads a valid level', () => {
32+
expect(parseStyle('<w:pPr><w:outlineLvl w:val="2"/></w:pPr>').attrs.outlineLevel).toBe(2);
33+
});
34+
35+
// w:val is required on CT_DecimalNumber; parseInt of a missing value yielded NaN.
36+
it.each([
37+
['no w:val', '<w:outlineLvl/>'],
38+
['a non-numeric w:val', '<w:outlineLvl w:val="abc"/>'],
39+
])('reports null rather than NaN for %s', (_label, outlineLvl) => {
40+
expect(parseStyle(`<w:pPr>${outlineLvl}</w:pPr>`).attrs.outlineLevel).toBeNull();
41+
});
42+
});
43+
44+
describe('w:tab', () => {
45+
it('reads a complete tab stop', () => {
46+
expect(parseStyle('<w:pPr><w:tabs><w:tab w:val="left" w:pos="720"/></w:tabs></w:pPr>').styles.tabStops).toEqual([
47+
{ val: 'start', pos: 48, leader: undefined },
48+
]);
49+
});
50+
51+
// w:val and w:pos are both required on CT_TabStop.
52+
it.each([
53+
['no attributes', '<w:tab/>'],
54+
['only w:val', '<w:tab w:val="left"/>'],
55+
['only w:pos', '<w:tab w:pos="720"/>'],
56+
])('drops a tab stop with %s', (_label, tab) => {
57+
expect(parseStyle(`<w:pPr><w:tabs>${tab}</w:tabs></w:pPr>`).styles.tabStops).toBeNull();
58+
});
59+
60+
it('keeps complete stops alongside incomplete ones', () => {
61+
const styles = parseStyle(
62+
'<w:pPr><w:tabs><w:tab w:val="left"/><w:tab w:val="right" w:pos="1440"/></w:tabs></w:pPr>',
63+
).styles;
64+
65+
expect(styles.tabStops).toEqual([{ val: 'end', pos: 96, leader: undefined }]);
66+
});
67+
});
68+
69+
describe('duplicate w:styleId records', () => {
70+
const duplicated = `
71+
<w:style w:type="paragraph" w:styleId="Target"><w:qFormat/></w:style>
72+
<w:style w:type="paragraph" w:styleId="Target">
73+
<w:name w:val="FromSecond"/>
74+
<w:basedOn w:val="BaseFromSecond"/>
75+
<w:pPr><w:jc w:val="center"/><w:ind w:left="720"/></w:pPr>
76+
</w:style>`;
77+
78+
const parsed = () =>
79+
getDefaultStyleDefinition('Target', {
80+
'word/styles.xml': xmljs.xml2js(
81+
`<?xml version="1.0"?><w:styles xmlns:w="${WORDPROCESSING_NS}">${duplicated}</w:styles>`,
82+
{ compact: false },
83+
),
84+
});
85+
86+
it('takes identity properties from whichever record declares them', () => {
87+
expect(parsed().attrs).toMatchObject({ name: 'FromSecond', basedOn: 'BaseFromSecond', qFormat: true });
88+
});
89+
90+
// Documents the current boundary: only the identity children above look past
91+
// the first record. Paragraph properties are not merged across duplicates.
92+
it('still reads paragraph properties from the first record only', () => {
93+
expect(parsed().styles.textAlign).toBeUndefined();
94+
});
95+
});
96+
});

‎packages/super-editor/src/editors/v1/core/super-converter/docx-helpers/get-default-style-definition.js‎

Lines changed: 56 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,20 @@
11
import { parseMarks } from '@converter/v2/importer/index.js';
22
import { twipsToLines, twipsToPixels } from '@converter/helpers.js';
33
import { kebabCase } from '@superdoc/common';
4+
import { attrValue, childElements, findChild, findChildren } from './xml-node-access.js';
5+
6+
/**
7+
* First child with the given name across every record sharing a styleId.
8+
*
9+
* Applies only to the identity children read below (w:name, w:basedOn,
10+
* w:qFormat). Paragraph and run properties still come from the first matching
11+
* record alone; this does not merge duplicate style records.
12+
*
13+
* @param {import('./xml-node-access.js').XmlNode[]} records Style elements sharing one w:styleId.
14+
* @param {string} name Qualified child name, e.g. `w:basedOn`.
15+
* @returns {import('./xml-node-access.js').XmlNode | undefined}
16+
*/
17+
const findInAnyRecord = (records, name) => records.map((record) => findChild(record, name)).find(Boolean);
418

519
/**
620
* Gets the default style definition.
@@ -15,32 +29,24 @@ export const getDefaultStyleDefinition = (defaultStyleId, docx) => {
1529
const styles = docx['word/styles.xml'];
1630
if (!styles) return result;
1731

18-
const { elements } = styles.elements[0];
19-
const elementsWithId = elements.filter((el) => {
20-
const { attributes } = el;
21-
return attributes && attributes['w:styleId'] === defaultStyleId;
22-
});
32+
const elementsWithId = childElements(childElements(styles)[0]).filter(
33+
(el) => attrValue(el, 'w:styleId') === defaultStyleId,
34+
);
2335

2436
const firstMatch = elementsWithId[0];
2537
if (!firstMatch) return result;
2638

2739
if (!firstMatch.elements) return result;
2840

29-
const qFormat = elementsWithId.find((el) => {
30-
const qFormat = el.elements.find((innerEl) => innerEl.name === 'w:qFormat');
31-
return qFormat;
32-
});
33-
34-
const name = elementsWithId
35-
.find((el) => el.elements.some((inner) => inner.name === 'w:name'))
36-
?.elements.find((inner) => inner.name === 'w:name')?.attributes['w:val'];
41+
const qFormat = findInAnyRecord(elementsWithId, 'w:qFormat');
42+
const name = attrValue(findInAnyRecord(elementsWithId, 'w:name'), 'w:val');
3743

3844
// pPr
39-
const pPr = firstMatch.elements.find((el) => el.name === 'w:pPr');
40-
const spacing = pPr?.elements?.find((el) => el.name === 'w:spacing');
41-
const justify = pPr?.elements?.find((el) => el.name === 'w:jc');
42-
const indent = pPr?.elements?.find((el) => el.name === 'w:ind');
43-
const tabs = pPr?.elements?.find((el) => el.name === 'w:tabs');
45+
const pPr = findChild(firstMatch, 'w:pPr');
46+
const spacing = findChild(pPr, 'w:spacing');
47+
const justify = findChild(pPr, 'w:jc');
48+
const indent = findChild(pPr, 'w:ind');
49+
const tabs = findChild(pPr, 'w:tabs');
4450

4551
let lineSpaceBefore, lineSpaceAfter, line;
4652
if (spacing?.attributes) {
@@ -57,56 +63,55 @@ export const getDefaultStyleDefinition = (defaultStyleId, docx) => {
5763
firstLine = twipsToPixels(indent.attributes['w:firstLine']);
5864
}
5965

60-
let tabStops = [];
61-
if (tabs) {
62-
tabStops = (tabs.elements || [])
63-
.filter((el) => el.name === 'w:tab')
64-
.map((tab) => {
65-
let val = tab.attributes['w:val'];
66-
if (val == 'left') {
67-
val = 'start';
68-
} else if (val == 'right') {
69-
val = 'end';
70-
}
71-
return {
72-
val,
73-
pos: twipsToPixels(tab.attributes['w:pos']),
74-
leader: tab.attributes['w:leader'],
75-
};
76-
});
77-
}
78-
79-
const keepNext = pPr?.elements?.find((el) => el.name === 'w:keepNext');
80-
const keepLines = pPr?.elements?.find((el) => el.name === 'w:keepLines');
81-
82-
const outlineLevel = pPr?.elements?.find((el) => el.name === 'w:outlineLvl');
83-
const outlineLvlValue = outlineLevel?.attributes['w:val'];
84-
85-
const pageBreakBefore = pPr?.elements?.find((el) => el.name === 'w:pageBreakBefore');
66+
// ECMA-376 marks w:val and w:pos required on w:tab (CT_TabStop). A record missing
67+
// either cannot place a stop, so it is dropped rather than emitted half-formed.
68+
const tabStops = findChildren(tabs, 'w:tab')
69+
.filter((tab) => attrValue(tab, 'w:val') != null && attrValue(tab, 'w:pos') != null)
70+
.map((tab) => {
71+
let val = attrValue(tab, 'w:val');
72+
if (val == 'left') {
73+
val = 'start';
74+
} else if (val == 'right') {
75+
val = 'end';
76+
}
77+
return {
78+
val,
79+
pos: twipsToPixels(attrValue(tab, 'w:pos')),
80+
leader: attrValue(tab, 'w:leader'),
81+
};
82+
});
83+
84+
const keepNext = findChild(pPr, 'w:keepNext');
85+
const keepLines = findChild(pPr, 'w:keepLines');
86+
87+
// w:val is required on w:outlineLvl (CT_DecimalNumber). Without a usable number
88+
// there is no level to report, so it stays null rather than becoming NaN.
89+
const outlineLevel = findChild(pPr, 'w:outlineLvl');
90+
const outlineLvlValue = Number.parseInt(attrValue(outlineLevel, 'w:val') ?? '', 10);
91+
92+
const pageBreakBefore = findChild(pPr, 'w:pageBreakBefore');
8693
let pageBreakBeforeVal = 0;
8794
if (pageBreakBefore) {
8895
if (!pageBreakBefore.attributes?.['w:val']) pageBreakBeforeVal = 1;
8996
else pageBreakBeforeVal = Number(pageBreakBefore?.attributes?.['w:val']);
9097
}
91-
const pageBreakAfter = pPr?.elements?.find((el) => el.name === 'w:pageBreakAfter');
98+
const pageBreakAfter = findChild(pPr, 'w:pageBreakAfter');
9299
let pageBreakAfterVal;
93100
if (pageBreakAfter) {
94101
if (!pageBreakAfter.attributes?.['w:val']) pageBreakAfterVal = 1;
95102
else pageBreakAfterVal = Number(pageBreakAfter?.attributes?.['w:val']);
96103
}
97104

98-
const basedOn = elementsWithId
99-
.find((el) => el.elements.some((inner) => inner.name === 'w:basedOn'))
100-
?.elements.find((inner) => inner.name === 'w:basedOn')?.attributes['w:val'];
105+
const basedOn = attrValue(findInAnyRecord(elementsWithId, 'w:basedOn'), 'w:val');
101106

102-
const linkToCharacterStyle = firstMatch.elements.find((el) => el.name === 'w:link')?.attributes?.['w:val'] ?? null;
107+
const linkToCharacterStyle = attrValue(findChild(firstMatch, 'w:link'), 'w:val') ?? null;
103108

104109
const parsedAttrs = {
105110
name,
106111
qFormat: qFormat ? true : false,
107112
keepNext: keepNext ? true : false,
108113
keepLines: keepLines ? true : false,
109-
outlineLevel: outlineLevel ? parseInt(outlineLvlValue) : null,
114+
outlineLevel: Number.isInteger(outlineLvlValue) ? outlineLvlValue : null,
110115
pageBreakBefore: pageBreakBeforeVal ? true : false,
111116
pageBreakAfter: pageBreakAfterVal ? true : false,
112117
basedOn: basedOn ?? null,
@@ -115,7 +120,7 @@ export const getDefaultStyleDefinition = (defaultStyleId, docx) => {
115120
};
116121

117122
// rPr
118-
const rPr = firstMatch.elements.find((el) => el.name === 'w:rPr');
123+
const rPr = findChild(firstMatch, 'w:rPr');
119124
const parsedMarks = parseMarks(rPr, [], docx) || [];
120125
const parsedStyles = {
121126
spacing: { lineSpaceAfter, lineSpaceBefore, line },

‎packages/super-editor/src/editors/v1/core/super-converter/v2/importer/docxImporter.empty-style-properties.test.js‎

Lines changed: 76 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -30,21 +30,28 @@ const DOCUMENT_XML = `<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
3030
</w:body>
3131
</w:document>`;
3232

33-
/** Styles.xml where Table Grid inherits from a bare base and carries an empty w:rPr. */
34-
const STYLES_XML = `<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
35-
<w:styles xmlns:w="${WORDPROCESSING_NS}">
36-
<w:style w:type="paragraph" w:default="1" w:styleId="Normal"><w:name w:val="Normal"/></w:style>
33+
/** Table Grid inherits from a bare base and carries an empty w:rPr. */
34+
const TABLE_STYLES = `
3735
<w:style w:type="table" w:styleId="TableNormal"/>
3836
<w:style w:type="table" w:styleId="TableGrid">
3937
<w:name w:val="Table Grid"/>
4038
<w:basedOn w:val="TableNormal"/>
4139
<w:rPr/>
42-
</w:style>
43-
</w:styles>`;
40+
</w:style>`;
4441

45-
const buildDocx = () => ({
42+
/**
43+
* @param {string} [extraStyles] Raw XML for additional `<w:style>` elements.
44+
*/
45+
const buildDocx = (extraStyles = '') => ({
4646
'word/document.xml': parse(DOCUMENT_XML),
47-
'word/styles.xml': parse(STYLES_XML),
47+
'word/styles.xml': parse(
48+
`<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
49+
<w:styles xmlns:w="${WORDPROCESSING_NS}">
50+
<w:style w:type="paragraph" w:default="1" w:styleId="Normal"><w:name w:val="Normal"/></w:style>
51+
${TABLE_STYLES}
52+
${extraStyles}
53+
</w:styles>`,
54+
),
4855
'word/_rels/document.xml.rels': parse(
4956
`<?xml version="1.0"?><Relationships xmlns="http://schemas.openxmlformats.org/package/2006/relationships"/>`,
5057
),
@@ -65,20 +72,69 @@ const textOf = (node) => {
6572
return (node.content ?? []).map(textOf).join('');
6673
};
6774

75+
/**
76+
* Import a document whose body is always `before` / table / `after`.
77+
* @param {string} [extraStyles] Raw XML for additional `<w:style>` elements.
78+
*/
79+
const importDocument = (extraStyles) => {
80+
const exceptions = [];
81+
const editor = {
82+
emit: (name, payload) => {
83+
if (name === 'exception') exceptions.push(payload.error);
84+
},
85+
options: {},
86+
};
87+
88+
const result = createDocumentJson(buildDocx(extraStyles), buildConverter(), editor);
89+
90+
return {
91+
body: (result?.pmDoc?.content ?? []).map((node) => `${node.type}:${textOf(node)}`),
92+
styleIds: (result?.linkedStyles ?? []).map((style) => style.id),
93+
exceptions,
94+
};
95+
};
96+
97+
const FULL_BODY = ['paragraph:before', 'table:cell', 'paragraph:after'];
98+
6899
describe('DOCX import with empty style property containers', () => {
69100
it('keeps a table whose style has an empty w:rPr and a bare basedOn target', () => {
70-
const exceptions = [];
71-
const editor = {
72-
emit: (name, payload) => {
73-
if (name === 'exception') exceptions.push(payload.error);
74-
},
75-
options: {},
76-
};
77-
78-
const result = createDocumentJson(buildDocx(), buildConverter(), editor);
79-
80-
const body = (result?.pmDoc?.content ?? []).map((node) => `${node.type}:${textOf(node)}`);
81-
expect(body).toEqual(['paragraph:before', 'table:cell', 'paragraph:after']);
101+
const { body, exceptions } = importDocument();
102+
103+
expect(body).toEqual(FULL_BODY);
82104
expect(exceptions).toEqual([]);
83105
});
106+
107+
// AIDEV-NOTE: The style catalogue is built outside the importer's per-node recovery
108+
// boundary, so one unreadable w:style used to abort createDocumentJson entirely.
109+
// getSchema then returned null and createDocument fell back to an empty document,
110+
// losing the whole body rather than one node. These cases must stay import-level.
111+
describe('incomplete style records do not discard the document', () => {
112+
it.each([
113+
[
114+
'w:outlineLvl without w:val',
115+
'<w:style w:type="paragraph" w:styleId="A"><w:pPr><w:outlineLvl/></w:pPr></w:style>',
116+
],
117+
[
118+
'w:tab without attributes',
119+
'<w:style w:type="paragraph" w:styleId="B"><w:pPr><w:tabs><w:tab/></w:tabs></w:pPr></w:style>',
120+
],
121+
[
122+
'a duplicate w:styleId whose second record is empty',
123+
'<w:style w:type="paragraph" w:styleId="C"><w:qFormat/></w:style><w:style w:type="paragraph" w:styleId="C"/>',
124+
],
125+
['a w:style without attributes', '<w:style><w:name w:val="Orphan"/></w:style>'],
126+
])('imports the full body when styles.xml contains %s', (_label, extraStyles) => {
127+
const { body, exceptions } = importDocument(extraStyles);
128+
129+
expect(body).toEqual(FULL_BODY);
130+
expect(exceptions).toEqual([]);
131+
});
132+
133+
it('omits unusable style records from the catalogue but keeps the rest', () => {
134+
const { styleIds } = importDocument('<w:style><w:name w:val="Orphan"/></w:style>');
135+
136+
expect(styleIds).toContain('TableGrid');
137+
expect(styleIds).not.toContain(undefined);
138+
});
139+
});
84140
});

0 commit comments

Comments
 (0)