From 96a96bef155a6304223d1535c6d172d4169dc91a Mon Sep 17 00:00:00 2001 From: Marc Durdin Date: Thu, 27 Feb 2025 13:33:20 +0100 Subject: [PATCH 1/3] fix(developer): add units tests to verify non-BMP numeric entities in XML reader fast-xml-reader has a bug with numeric entities. See: https://github.com/NaturalIntelligence/fast-xml-parser/issues/725 This commit adds a unit test to verify that non-BMP numeric entities will be parsed correctly. It will fail until we update the fast-xml-parser dependency. Relates-to: #13348 --- .../web/utils/test/fixtures/kvks/hex_escape.kvks | 16 ++++++++++++++++ .../web/utils/test/kvks/kvks-file.tests.ts | 15 +++++++++++++++ 2 files changed, 31 insertions(+) create mode 100644 developer/src/common/web/utils/test/fixtures/kvks/hex_escape.kvks diff --git a/developer/src/common/web/utils/test/fixtures/kvks/hex_escape.kvks b/developer/src/common/web/utils/test/fixtures/kvks/hex_escape.kvks new file mode 100644 index 0000000000..74c10b2e71 --- /dev/null +++ b/developer/src/common/web/utils/test/fixtures/kvks/hex_escape.kvks @@ -0,0 +1,16 @@ + + +
+ 10.0 + hex_escape + +
+ + + ሴ + Ā + 𐀀 + 𒍅 + + + diff --git a/developer/src/common/web/utils/test/kvks/kvks-file.tests.ts b/developer/src/common/web/utils/test/kvks/kvks-file.tests.ts index 4c009e9d5c..422a2e4a40 100644 --- a/developer/src/common/web/utils/test/kvks/kvks-file.tests.ts +++ b/developer/src/common/web/utils/test/kvks/kvks-file.tests.ts @@ -40,6 +40,21 @@ describe('kvks-file-reader', function() { const reader = new KvksFileReader(); assert.throws(() => reader.read(input), 'File appears to be a binary .kvk file'); }); + + it('should read non-bmp hex escapes correctly', function() { + const path = makePathToFixture('kvks', 'hex_escape.kvks'); + const input = fs.readFileSync(path); + + const reader = new KvksFileReader(); + const kvks = reader.read(input); + const invalidVkeys: string[] = []; + const vk = reader.transform(kvks, invalidVkeys); + assert.isEmpty(invalidVkeys); + assert.equal(vk.keys[0].text, '\u{1234}'); + assert.equal(vk.keys[1].text, '\u{100}'); + assert.equal(vk.keys[2].text, String.fromCodePoint(0x10000)); + assert.equal(vk.keys[3].text, String.fromCodePoint(0x12345)); + }); }); describe('kvks-file-writer', function() { From 7d5c98ddba4b4d81ad1f6c785e92fddfb12e02e5 Mon Sep 17 00:00:00 2001 From: Marc Durdin Date: Mon, 17 Mar 2025 06:22:29 +0700 Subject: [PATCH 2/3] fix(developer): bump fast-xml-parser to 5.0.9 This is a major dependency version bump (4.5.0 to 5.0.9) which we would normally avoid during beta. However, we need the fix NaturalIntelligence/fast-xml-parser#725 for hex escapes in XML, which is incorporated in 5.0.9. I have assessed the other changes to fast-xml-parser and found no breaking changes for us, but I tightened the types in xml-utils declarations and found some minor inconsistencies which appear to have no impact, and which I have corrected: * wrong type in unused parameter to `tagValueProcessor` * reference to unused property `options.emptyTag` While doing this, I consolidated the common options for the parser in order to verify consistency, but made no changes to the resolved parsing/building options. Added a test to ldml keyboard reading, to verify that numeric strings are treated as strings, given the divergence in the `numberParseOptions` option, and it shows that numeric strings are treated as strings. Fixes: #13348 --- developer/src/common/web/utils/package.json | 2 +- .../src/common/web/utils/src/xml-utils.ts | 106 ++++++++---------- .../fixtures/ldml-keyboard/numeric-id.xml | 10 ++ .../test/fixtures/ldml-keyboard/test-fr.xml | 2 +- .../ldml-keyboard-testdata-reader.tests.ts | 2 +- .../ldml-keyboard-xml-reader.tests.ts | 25 ++++- package-lock.json | 26 +++-- 7 files changed, 93 insertions(+), 80 deletions(-) create mode 100644 developer/src/common/web/utils/test/fixtures/ldml-keyboard/numeric-id.xml rename developer/src/common/web/utils/test/{kmx => ldml}/ldml-keyboard-testdata-reader.tests.ts (96%) rename developer/src/common/web/utils/test/{kmx => ldml}/ldml-keyboard-xml-reader.tests.ts (92%) diff --git a/developer/src/common/web/utils/package.json b/developer/src/common/web/utils/package.json index 4f066313f6..05cba553bb 100644 --- a/developer/src/common/web/utils/package.json +++ b/developer/src/common/web/utils/package.json @@ -12,7 +12,7 @@ "@keymanapp/common-types": "*", "@sentry/node": "^7.57.0", "eventemitter3": "^5.0.0", - "fast-xml-parser": "^4.5.0", + "fast-xml-parser": "^5.0.9", "path-browserify": "^1.0.1", "restructure": "^3.0.1", "sax": ">=0.6.0", diff --git a/developer/src/common/web/utils/src/xml-utils.ts b/developer/src/common/web/utils/src/xml-utils.ts index 022328ff94..d4443c05c3 100644 --- a/developer/src/common/web/utils/src/xml-utils.ts +++ b/developer/src/common/web/utils/src/xml-utils.ts @@ -6,7 +6,7 @@ * Abstraction for XML reading and writing */ -import { XMLParser, XMLBuilder } from 'fast-xml-parser'; +import { XMLParser, XMLBuilder, XmlBuilderOptions, X2jOptions } from 'fast-xml-parser'; export type KeymanXMLType = 'keyboard3' // LDML @@ -17,102 +17,87 @@ export type KeymanXMLType = ; /** Bag of options, maximally one for each KeymanXMLType */ -type KeymanXMLOptionsBag = { - [key in KeymanXMLType]?: any +type KeymanXMLParserOptionsBag = { + [key in KeymanXMLType]?: X2jOptions; +}; + +const commonKeymanXmlParserOptions: X2jOptions = { + attributeNamePrefix: '$', // causes remapping into $: { … } objects + htmlEntities: true, + ignoreAttributes: false, + ignorePiTags: true, + numberParseOptions: { // TODO: query is this option really necessary? + eNotation: null, + hex: null, + leadingZeros: null, + skipLike: /(?:)/, // parse numbers as strings + }, + textNodeName: '_', }; /** map of options for the XML parser */ -const PARSER_OPTIONS: KeymanXMLOptionsBag = { +const PARSER_OPTIONS: KeymanXMLParserOptionsBag = { 'keyboard3': { - ignoreAttributes: false, // We'd like attributes, please attributeNamePrefix: '@__', // We'll use this to convert attributes to strings and subobjects to arrays, when empty. - trimValues: false, // preserve spaces, but: htmlEntities: true, - tagValueProcessor: (tagName: string, tagValue: string /*, jPath, hasAttributes, isLeafNode*/) => { + ignoreAttributes: false, // We'd like attributes, please + tagValueProcessor: (_tagName: string, tagValue: string /*, jPath, hasAttributes, isLeafNode*/) => { // since trimValues: false, we need to zap any element values that would be trimmed. // currently, the LDML spec doesn't have any element values, but this // future-proofs us a little in that element values are allowed, just trimmed. // if we do need elements in the future, we'd check the preserve-space attribute here. return tagValue?.trim(); }, + trimValues: false, // preserve spaces, but see tagValueProcessor }, 'keyboardTest3': { - ignorePiTags: true, + attributeNamePrefix: '', // avoid @_ htmlEntities: true, ignoreAttributes: false, // We'd like attributes, please - attributeNamePrefix: '', // avoid @_ + ignorePiTags: true, preserveOrder: true, // Gives us a 'special' format }, 'kps': { - ignorePiTags: true, - ignoreAttributes: false, - htmlEntities: true, - attributeNamePrefix: '$', // causes remapping into $: { … } objects - textNodeName: '_', - numberParseOptions: { - skipLike: /(?:)/, // parse numbers as strings - hex: null, - leadingZeros: null, - eNotation: null, - }, + ...commonKeymanXmlParserOptions, }, 'kpj': { - ignorePiTags: true, - textNodeName: '_', - htmlEntities: true, - ignoreAttributes: false, // We'd like attributes, please + ...commonKeymanXmlParserOptions, attributeNamePrefix: '', // to avoid '@_' prefixes - numberParseOptions: { - skipLike: /(?:)/, // parse numbers as strings - hex: null, - leadingZeros: null, - eNotation: null, - }, }, 'kvks': { - ignorePiTags: true, - textNodeName: '_', - htmlEntities: true, - ignoreAttributes: false, // We'd like attributes, please - attributeNamePrefix: '$', // causes remapping into $: { … } objects - numberParseOptions: { - skipLike: /(?:)/, // parse numbers as strings - hex: null, - leadingZeros: null, - eNotation: null, - }, - trimValues: false, // preserve spaces, but: - tagValueProcessor: (tagName: string, tagValue: string, jPath: string, hasAttributes: string, isLeafNode: boolean) : string | undefined => { + ...commonKeymanXmlParserOptions, + tagValueProcessor: (_tagName: string, tagValue: string, _jPath: string, _hasAttributes: boolean, isLeafNode: boolean) : string | undefined => { if (!isLeafNode) { return tagValue?.trim(); // trimmed value } else { return null; // no change to leaf nodes } }, + trimValues: false, // preserve spaces }, }; -const GENERATOR_OPTIONS: KeymanXMLOptionsBag = { +type KeymanXMLGeneratorOptionsBag = { + [key in KeymanXMLType]?: XmlBuilderOptions +}; + +const commonKeymanXmlGeneratorOptions: XmlBuilderOptions = { + attributeNamePrefix: '$', + ignoreAttributes: false, + format: true, + textNodeName: '_', + suppressEmptyNode: true, +}; + +const GENERATOR_OPTIONS: KeymanXMLGeneratorOptionsBag = { kvks: { - attributeNamePrefix: '$', - ignoreAttributes: false, - format: true, - textNodeName: '_', - suppressEmptyNode: true, + ...commonKeymanXmlGeneratorOptions, }, kpj: { - attributeNamePrefix: '$', - ignoreAttributes: false, - format: true, - textNodeName: '_', - suppressEmptyNode: true, + ...commonKeymanXmlGeneratorOptions, }, kps: { - attributeNamePrefix: '$', - ignoreAttributes: false, - format: true, - textNodeName: '_', - suppressEmptyNode: true, + ...commonKeymanXmlGeneratorOptions, }, }; @@ -263,9 +248,6 @@ export class KeymanXMLReader { throw Error(`Internal error: unhandled XML type ${this.type}`); } options = Object.assign({}, options); // TODO: xml2js likes to mutate the options here. Shallow clone the object. - if (options.emptyTag) { - options.emptyTag = {}; // TODO: xml2js likes to mutate the options here. Reset it. - } return new XMLParser(options); } } diff --git a/developer/src/common/web/utils/test/fixtures/ldml-keyboard/numeric-id.xml b/developer/src/common/web/utils/test/fixtures/ldml-keyboard/numeric-id.xml new file mode 100644 index 0000000000..404ae55d99 --- /dev/null +++ b/developer/src/common/web/utils/test/fixtures/ldml-keyboard/numeric-id.xml @@ -0,0 +1,10 @@ + + + + + + + + + + diff --git a/developer/src/common/web/utils/test/fixtures/ldml-keyboard/test-fr.xml b/developer/src/common/web/utils/test/fixtures/ldml-keyboard/test-fr.xml index fc0224fcb8..a352e437a6 100644 --- a/developer/src/common/web/utils/test/fixtures/ldml-keyboard/test-fr.xml +++ b/developer/src/common/web/utils/test/fixtures/ldml-keyboard/test-fr.xml @@ -1,5 +1,5 @@ - +