From 1e679a568e2c274419c31204c1e51bf4362fd225 Mon Sep 17 00:00:00 2001 From: Joscha Feth Date: Wed, 11 Sep 2024 03:51:06 +0100 Subject: [PATCH 01/19] fix: allow named types in unions The Avro specification defines: > Unions may not contain more than one schema with the same type, except for the named types record, fixed and enum see https://avro.apache.org/docs/1.11.1/specification/#unions meaning that if a `record` type has a name, it is valid to mix multiple of them in a union, unwrapped. --- lib/types.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/types.js b/lib/types.js index c1aba225..74ff70dc 100644 --- a/lib/types.js +++ b/lib/types.js @@ -3044,7 +3044,7 @@ function getTypeBucket(type) { case 'map': case 'error': case 'record': - return 'object'; + return type.name ? maybeQualify(type.name, type.namespace) : 'object'; default: return typeName; } From 626ab67ec2597e667d2de09accccae8b068e93f8 Mon Sep 17 00:00:00 2001 From: Joscha Feth Date: Wed, 11 Sep 2024 03:54:20 +0100 Subject: [PATCH 02/19] Update lib/types.js --- lib/types.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/types.js b/lib/types.js index 74ff70dc..e0a9b887 100644 --- a/lib/types.js +++ b/lib/types.js @@ -3043,6 +3043,7 @@ function getTypeBucket(type) { return 'string'; case 'map': case 'error': + return 'object'; case 'record': return type.name ? maybeQualify(type.name, type.namespace) : 'object'; default: From 3d9efd5bccc07db57071ed6f59b2d35fa5ccf794 Mon Sep 17 00:00:00 2001 From: Joscha Feth Date: Wed, 11 Sep 2024 12:01:10 +0100 Subject: [PATCH 03/19] fix: allow named types in unions --- lib/types.js | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/lib/types.js b/lib/types.js index e0a9b887..b7c6b752 100644 --- a/lib/types.js +++ b/lib/types.js @@ -67,6 +67,7 @@ class Type { this.name = undefined; this.aliases = undefined; this.doc = (schema && schema.doc) ? '' + schema.doc : undefined; + this.opts = Object.assign({}, opts); if (schema) { // This is a complex (i.e. non-primitive) type. @@ -351,7 +352,7 @@ class Type { // Group types by category, similar to the logic for unwrapped unions. let bucketized = {}; expanded.forEach((type) => { - let bucket = getTypeBucket(type); + let bucket = getTypeBucket(type, true); let bucketTypes = bucketized[bucket]; if (!bucketTypes) { bucketized[bucket] = bucketTypes = []; @@ -1258,7 +1259,7 @@ class UnwrappedUnionType extends UnionType { } _getIndex (val) { - let index = this._bucketIndices[getValueBucket(val)]; + let index = this._bucketIndices[getValueBucket(val, this.opts.namespace)]; if (this._dynamicBranches) { // Slower path, we must run the value through all branches. index = this._getBranchIndex(val, index); @@ -3027,8 +3028,9 @@ function maybeQualify(name, ns) { * Get a type's bucket when included inside an unwrapped union. * * @param type {Type} Any type. + * @param isExpanded {Boolean} Whether the union is expanded - if yes, we don't use the name to determine the bucket, as the parent type is already in the right bucket. */ -function getTypeBucket(type) { +function getTypeBucket(type, isExpanded) { let typeName = type.typeName; switch (typeName) { case 'double': @@ -3045,7 +3047,11 @@ function getTypeBucket(type) { case 'error': return 'object'; case 'record': - return type.name ? maybeQualify(type.name, type.namespace) : 'object'; + if (isExpanded || !type.name) { + return 'object'; + } else { + return maybeQualify(type.name, type.namespace) + } default: return typeName; } @@ -3055,8 +3061,9 @@ function getTypeBucket(type) { * Infer a value's bucket (see unwrapped unions for more details). * * @param val {...} Any value. + * @param namespace {String} Optional namespace. */ -function getValueBucket(val) { +function getValueBucket(val, namespace) { if (val === null) { return 'null'; } @@ -3067,6 +3074,12 @@ function getValueBucket(val) { return 'array'; } else if (Buffer.isBuffer(val)) { return 'buffer'; + } else if(typeof val.constructor !== 'undefined' && val.constructor.name) { + const isBuiltin = Object.values(module.exports.builtins).some(b => val instanceof b); + if (isBuiltin || val.constructor === Object) { + return bucket; + } + return maybeQualify(val.constructor.name, namespace); } } return bucket; From ee6aecdc024808e3cd70af705c87018da2c96b98 Mon Sep 17 00:00:00 2001 From: Joscha Feth Date: Wed, 11 Sep 2024 12:08:14 +0100 Subject: [PATCH 04/19] Update lib/types.js --- lib/types.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/types.js b/lib/types.js index b7c6b752..450292c9 100644 --- a/lib/types.js +++ b/lib/types.js @@ -3074,7 +3074,7 @@ function getValueBucket(val, namespace) { return 'array'; } else if (Buffer.isBuffer(val)) { return 'buffer'; - } else if(typeof val.constructor !== 'undefined' && val.constructor.name) { + } else if (typeof val.constructor !== 'undefined' && val.constructor.name) { const isBuiltin = Object.values(module.exports.builtins).some(b => val instanceof b); if (isBuiltin || val.constructor === Object) { return bucket; From 6ca39f816f4a933e5009557ec9ef04a46a349712 Mon Sep 17 00:00:00 2001 From: Joscha Feth Date: Mon, 23 Sep 2024 13:15:03 +0100 Subject: [PATCH 05/19] Revert "fix: allow named types in unions" This reverts commit 3d9efd5bccc07db57071ed6f59b2d35fa5ccf794. --- lib/types.js | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/lib/types.js b/lib/types.js index d3bb2662..d8c6bf1c 100644 --- a/lib/types.js +++ b/lib/types.js @@ -67,7 +67,6 @@ class Type { this.name = undefined; this.aliases = undefined; this.doc = (schema && schema.doc) ? '' + schema.doc : undefined; - this.opts = Object.assign({}, opts); if (schema) { // This is a complex (i.e. non-primitive) type. @@ -352,7 +351,7 @@ class Type { // Group types by category, similar to the logic for unwrapped unions. let bucketized = {}; expanded.forEach((type) => { - let bucket = getTypeBucket(type, true); + let bucket = getTypeBucket(type); let bucketTypes = bucketized[bucket]; if (!bucketTypes) { bucketized[bucket] = bucketTypes = []; @@ -1271,7 +1270,7 @@ class UnwrappedUnionType extends UnionType { } _getIndex (val) { - let index = this._bucketIndices[getValueBucket(val, this.opts.namespace)]; + let index = this._bucketIndices[getValueBucket(val)]; if (this._dynamicBranches) { // Slower path, we must run the value through all branches. index = this._getBranchIndex(val, index); @@ -3037,9 +3036,8 @@ function maybeQualify(name, ns) { * Get a type's bucket when included inside an unwrapped union. * * @param type {Type} Any type. - * @param isExpanded {Boolean} Whether the union is expanded - if yes, we don't use the name to determine the bucket, as the parent type is already in the right bucket. */ -function getTypeBucket(type, isExpanded) { +function getTypeBucket(type) { let typeName = type.typeName; switch (typeName) { case 'double': @@ -3056,11 +3054,7 @@ function getTypeBucket(type, isExpanded) { case 'error': return 'object'; case 'record': - if (isExpanded || !type.name) { - return 'object'; - } else { - return maybeQualify(type.name, type.namespace) - } + return type.name ? maybeQualify(type.name, type.namespace) : 'object'; default: return typeName; } @@ -3070,9 +3064,8 @@ function getTypeBucket(type, isExpanded) { * Infer a value's bucket (see unwrapped unions for more details). * * @param val {...} Any value. - * @param namespace {String} Optional namespace. */ -function getValueBucket(val, namespace) { +function getValueBucket(val) { if (val === null) { return 'null'; } @@ -3083,12 +3076,6 @@ function getValueBucket(val, namespace) { return 'array'; } else if (isBufferLike(val)) { return 'buffer'; - } else if(typeof val.constructor !== 'undefined' && val.constructor.name) { - const isBuiltin = Object.values(module.exports.builtins).some(b => val instanceof b); - if (isBuiltin || val.constructor === Object) { - return bucket; - } - return maybeQualify(val.constructor.name, namespace); } } return bucket; From 19028d8381ab24a55b5ba0d30a82ab40eee2b6e8 Mon Sep 17 00:00:00 2001 From: Joscha Feth Date: Mon, 23 Sep 2024 16:07:13 +0100 Subject: [PATCH 06/19] feat: projection function for unwrapped unions --- lib/types.js | 32 +++++++++++++++++++++++++++---- test/test_types.js | 48 ++++++++++++++++++++++++++++++++++++++++++++++ types/index.d.ts | 11 ++++++++++- 3 files changed, 86 insertions(+), 5 deletions(-) diff --git a/lib/types.js b/lib/types.js index d8c6bf1c..10b0e931 100644 --- a/lib/types.js +++ b/lib/types.js @@ -111,6 +111,8 @@ class Type { wrapUnions = 'auto'; } else if (typeof wrapUnions == 'string') { wrapUnions = wrapUnions.toLowerCase(); + } else if (typeof wrapUnions === 'function') { + wrapUnions = 'auto'; } switch (wrapUnions) { case 'always': @@ -197,7 +199,20 @@ class Type { return Type.forSchema(obj, opts); }); if (!UnionType) { - UnionType = isAmbiguous(types) ? WrappedUnionType : UnwrappedUnionType; + // either automatic detection or we have a projection function + if (typeof opts.wrapUnions === 'function') { + // we have a projection function + try { + UnionType = typeof opts.wrapUnions(types) !== 'undefined' + // projection function yields a function, we can use an Unwrapped type + ? UnwrappedUnionType + : WrappedUnionType; + } catch(e) { + throw new Error(`Union projection function errored: ${e}`); + } + } else { + UnionType = isAmbiguous(types) ? WrappedUnionType : UnwrappedUnionType; + } } LOGICAL_TYPE = logicalType; type = new UnionType(types, opts); @@ -341,10 +356,10 @@ class Type { return branchTypes[name]; }), opts); } catch (err) { - opts.wrapUnions = wrapUnions; throw err; + } finally { + opts.wrapUnions = wrapUnions; } - opts.wrapUnions = wrapUnions; return unionType; } @@ -1265,12 +1280,21 @@ class UnwrappedUnionType extends UnionType { this._bucketIndices[bucket] = index; } }, this); + this.projectionFunction = opts && typeof opts.wrapUnions === 'function' + ? ((fn) => (val) => { + const index = fn(val); + if (typeof index !== 'number' || index >= this._bucketIndices.length) { + throw new Error(`Projected index ${index} is not valid`); + } + return index; + })(opts.wrapUnions(this.types)) + : (val) => this._bucketIndices[getValueBucket(val)] Object.freeze(this); } _getIndex (val) { - let index = this._bucketIndices[getValueBucket(val)]; + let index = this.projectionFunction(val); if (this._dynamicBranches) { // Slower path, we must run the value through all branches. index = this._getBranchIndex(val, index); diff --git a/test/test_types.js b/test/test_types.js index 888a2143..28073a62 100644 --- a/test/test_types.js +++ b/test/test_types.js @@ -3492,6 +3492,54 @@ suite('types', () => { assert(Type.isType(t.field('unwrapped').type, 'union:unwrapped')); }); + test.only('union projection', () => { + const Dog = { + type: 'record', + name: 'Dog', + fields: [ + { type: 'string', name: 'bark' } + ], + }; + const Cat = { + type: 'record', + name: 'Cat', + fields: [ + { type: 'string', name: 'meow' } + ], + }; + const animalTypes = [Dog, Cat]; + + const wrapUnions = (types) => { + assert.deepEqual(types.map(t => t.name), ['Dog', 'Cat']); + return (animal) => { + const animalType = ((animal) => { + if ('bark' in animal) { + return 'Dog'; + } else if ('meow' in animal) { + return 'Cat'; + } + throw new Error('Unkown animal'); + })(animal); + return types.indexOf(types.find(type => type.name === animalType)); + } + }; + + // TODO: replace this with a mock when available + // currently we're on mocha without sinon + function mockWrapUnions() { + mockWrapUnions.calls = typeof mockWrapUnions.calls === 'undefined' + ? 1 + : ++mockWrapUnions.calls; + return wrapUnions.apply(null, arguments); + } + + // Ambiguous, but we have a projection function + const Animal = Type.forSchema(animalTypes, { wrapUnions: mockWrapUnions }); + Animal.toBuffer({ meow: '🐈' }); + assert.equal(mockWrapUnions.calls, 2); + assert.throws(() => Animal.toBuffer({ snap: '🐊' }), /Unkown animal/) + }); + test('invalid wrap unions option', () => { assert.throws(() => { Type.forSchema('string', {wrapUnions: 'FOO'}); diff --git a/types/index.d.ts b/types/index.d.ts index 33611dd7..6f4a5ad9 100644 --- a/types/index.d.ts +++ b/types/index.d.ts @@ -96,6 +96,15 @@ interface EncoderOptions { syncMarker: Buffer; } +/** + * A projection function that is used when unwrapping unions. + * This function is called at schema parsing time on each union with its branches' types. + * If it returns a non-null (function) value, that function will be called each time a value's branch needs to be inferred and should return the branch's index. + * The index muss be a number between 0 and length-1 of the passed types. + * In this case (a branch index) the union will use an unwrapped representation. Otherwise (undefined), the union will be wrapped. + */ +type ProjectionFn = (types: ReadonlyArray) => ((val: unknown) => number) | undefined; + interface ForSchemaOptions { assertLogicalTypes: boolean; logicalTypes: { [type: string]: new (schema: Schema, opts?: any) => types.LogicalType; }; @@ -104,7 +113,7 @@ interface ForSchemaOptions { omitRecordMethods: boolean; registry: { [name: string]: Type }; typeHook: (schema: Schema | string, opts: ForSchemaOptions) => Type | undefined; - wrapUnions: boolean | 'auto' | 'always' | 'never'; + wrapUnions: ProjectionFn | boolean | 'auto' | 'always' | 'never'; } interface TypeOptions extends ForSchemaOptions { From c3ac1b31f8af01706c48cfd1421f903c5085a6b5 Mon Sep 17 00:00:00 2001 From: Joscha Feth Date: Mon, 23 Sep 2024 16:10:25 +0100 Subject: [PATCH 07/19] fix: tests --- lib/types.js | 3 +-- test/test_types.js | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/types.js b/lib/types.js index 10b0e931..e38f6cf7 100644 --- a/lib/types.js +++ b/lib/types.js @@ -3076,9 +3076,8 @@ function getTypeBucket(type) { return 'string'; case 'map': case 'error': - return 'object'; case 'record': - return type.name ? maybeQualify(type.name, type.namespace) : 'object'; + return 'object'; default: return typeName; } diff --git a/test/test_types.js b/test/test_types.js index 28073a62..01ca882b 100644 --- a/test/test_types.js +++ b/test/test_types.js @@ -3492,7 +3492,7 @@ suite('types', () => { assert(Type.isType(t.field('unwrapped').type, 'union:unwrapped')); }); - test.only('union projection', () => { + test('union projection', () => { const Dog = { type: 'record', name: 'Dog', From b21aee84c1081e7b33e7488fea9a530dc37df0dc Mon Sep 17 00:00:00 2001 From: Joscha Feth Date: Mon, 23 Sep 2024 16:11:12 +0100 Subject: [PATCH 08/19] style: typo --- test/test_types.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_types.js b/test/test_types.js index 01ca882b..4c23863f 100644 --- a/test/test_types.js +++ b/test/test_types.js @@ -3518,7 +3518,7 @@ suite('types', () => { } else if ('meow' in animal) { return 'Cat'; } - throw new Error('Unkown animal'); + throw new Error('Unknown animal'); })(animal); return types.indexOf(types.find(type => type.name === animalType)); } @@ -3537,7 +3537,7 @@ suite('types', () => { const Animal = Type.forSchema(animalTypes, { wrapUnions: mockWrapUnions }); Animal.toBuffer({ meow: '🐈' }); assert.equal(mockWrapUnions.calls, 2); - assert.throws(() => Animal.toBuffer({ snap: '🐊' }), /Unkown animal/) + assert.throws(() => Animal.toBuffer({ snap: '🐊' }), /Unknown animal/) }); test('invalid wrap unions option', () => { From ad181b67296fd1d1ac51d4e5967ad84ec724d673 Mon Sep 17 00:00:00 2001 From: Joscha Feth Date: Mon, 23 Sep 2024 16:20:45 +0100 Subject: [PATCH 09/19] fix: duped buckets --- lib/types.js | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/lib/types.js b/lib/types.js index e38f6cf7..95a560bf 100644 --- a/lib/types.js +++ b/lib/types.js @@ -1266,13 +1266,31 @@ class UnwrappedUnionType extends UnionType { this._dynamicBranches = null; this._bucketIndices = {}; + + this.projectionFunction = (val) => this._bucketIndices[getValueBucket(val)]; + let hasWrapUnionsFn = opts && typeof opts.wrapUnions === 'function'; + if (hasWrapUnionsFn) { + const projectionFunction = opts.wrapUnions(this.types); + if (typeof projectionFunction === 'undefined') { + hasWrapUnionsFn = false; + } else { + this.projectionFunction = (val) => { + const index = projectionFunction(val); + if (typeof index !== 'number' || index >= this._bucketIndices.length) { + throw new Error(`Projected index ${index} is not valid`); + } + return index; + } + } + } + this.types.forEach(function (type, index) { if (Type.isType(type, 'abstract', 'logical')) { if (!this._dynamicBranches) { this._dynamicBranches = []; } this._dynamicBranches.push({index, type}); - } else { + } else if (!hasWrapUnionsFn) { let bucket = getTypeBucket(type); if (this._bucketIndices[bucket] !== undefined) { throw new Error(`ambiguous unwrapped union: ${j(this)}`); @@ -1280,15 +1298,6 @@ class UnwrappedUnionType extends UnionType { this._bucketIndices[bucket] = index; } }, this); - this.projectionFunction = opts && typeof opts.wrapUnions === 'function' - ? ((fn) => (val) => { - const index = fn(val); - if (typeof index !== 'number' || index >= this._bucketIndices.length) { - throw new Error(`Projected index ${index} is not valid`); - } - return index; - })(opts.wrapUnions(this.types)) - : (val) => this._bucketIndices[getValueBucket(val)] Object.freeze(this); } From 040aed7c91c5983e9726b19f8e10d73a8e46f558 Mon Sep 17 00:00:00 2001 From: Joscha Feth Date: Tue, 24 Sep 2024 10:22:51 +0100 Subject: [PATCH 10/19] ProjectionFn -> BranchProjection --- types/index.d.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/types/index.d.ts b/types/index.d.ts index 6f4a5ad9..125bc5e7 100644 --- a/types/index.d.ts +++ b/types/index.d.ts @@ -103,7 +103,9 @@ interface EncoderOptions { * The index muss be a number between 0 and length-1 of the passed types. * In this case (a branch index) the union will use an unwrapped representation. Otherwise (undefined), the union will be wrapped. */ -type ProjectionFn = (types: ReadonlyArray) => ((val: unknown) => number) | undefined; +type BranchProjection = (types: ReadonlyArray) => + | ((val: unknown) => number) + | undefined; interface ForSchemaOptions { assertLogicalTypes: boolean; @@ -113,7 +115,7 @@ interface ForSchemaOptions { omitRecordMethods: boolean; registry: { [name: string]: Type }; typeHook: (schema: Schema | string, opts: ForSchemaOptions) => Type | undefined; - wrapUnions: ProjectionFn | boolean | 'auto' | 'always' | 'never'; + wrapUnions: BranchProjection | boolean | 'auto' | 'always' | 'never'; } interface TypeOptions extends ForSchemaOptions { From 510d272e3724c43b72d9352db1bb082ed0af5a90 Mon Sep 17 00:00:00 2001 From: Joscha Feth Date: Tue, 24 Sep 2024 10:56:57 +0100 Subject: [PATCH 11/19] changes as requested --- lib/types.js | 113 ++++++++++++++++++++++++++------------------- test/test_types.js | 2 +- 2 files changed, 66 insertions(+), 49 deletions(-) diff --git a/lib/types.js b/lib/types.js index 95a560bf..a2d243ab 100644 --- a/lib/types.js +++ b/lib/types.js @@ -198,24 +198,26 @@ class Type { let types = schema.map((obj) => { return Type.forSchema(obj, opts); }); + let projectionFn; if (!UnionType) { // either automatic detection or we have a projection function if (typeof opts.wrapUnions === 'function') { // we have a projection function try { - UnionType = typeof opts.wrapUnions(types) !== 'undefined' - // projection function yields a function, we can use an Unwrapped type - ? UnwrappedUnionType - : WrappedUnionType; + projectionFn = opts.wrapUnions(types); + UnionType = typeof projectionFn !== 'undefined' + // projection function yields a function, we can use an Unwrapped type + ? UnwrappedUnionType + : WrappedUnionType; } catch(e) { - throw new Error(`Union projection function errored: ${e}`); + throw new Error(`Error generating projection function: ${e}`); } } else { UnionType = isAmbiguous(types) ? WrappedUnionType : UnwrappedUnionType; } } LOGICAL_TYPE = logicalType; - type = new UnionType(types, opts); + type = new UnionType(types, opts, projectionFn); } else { // New type definition. type = (function (typeName) { let Type = TYPES[typeName]; @@ -1241,6 +1243,44 @@ UnionType.prototype._branchConstructor = function () { throw new Error('unions cannot be directly wrapped'); }; + +function generateProjectionIndexer(projectionFn) { + return (val) => { + const index = projectionFn(val); + if (typeof index !== 'number') { + throw new Error(`Projected index '${index}' is not valid`); + } + return index; + }; +} + +function generateDefaultIndexer() { + this._dynamicBranches = null; + this._bucketIndices = {}; + this.types.forEach(function (type, index) { + if (Type.isType(type, 'abstract', 'logical')) { + if (!this._dynamicBranches) { + this._dynamicBranches = []; + } + this._dynamicBranches.push({index, type}); + } else { + let bucket = getTypeBucket(type); + if (this._bucketIndices[bucket] !== undefined) { + throw new Error(`ambiguous unwrapped union: ${j(this)}`); + } + this._bucketIndices[bucket] = index; + } + }, this); + return (val) => { + let index = this._bucketIndices[getValueBucket(val)]; + if (this._dynamicBranches) { + // Slower path, we must run the value through all branches. + index = this._getBranchIndex(val, index); + } + return index; + }; +} + /** * "Natural" union type. * @@ -1261,56 +1301,33 @@ UnionType.prototype._branchConstructor = function () { * + `map`, `record` */ class UnwrappedUnionType extends UnionType { - constructor (schema, opts) { + /** + * + * @param {*} schema + * @param {*} opts + * @param {Function|undefined} projectionFn The projection function used + * to determine the bucket for the + * Union. Falls back to generate + * from `wrapUnions` parameter + * if given. + */ + constructor (schema, opts, projectionFn) { super(schema, opts); - this._dynamicBranches = null; - this._bucketIndices = {}; - - this.projectionFunction = (val) => this._bucketIndices[getValueBucket(val)]; - let hasWrapUnionsFn = opts && typeof opts.wrapUnions === 'function'; - if (hasWrapUnionsFn) { - const projectionFunction = opts.wrapUnions(this.types); - if (typeof projectionFunction === 'undefined') { - hasWrapUnionsFn = false; - } else { - this.projectionFunction = (val) => { - const index = projectionFunction(val); - if (typeof index !== 'number' || index >= this._bucketIndices.length) { - throw new Error(`Projected index ${index} is not valid`); - } - return index; - } + if (!projectionFn && opts && typeof opts.wrapUnions === 'function') { + try { + projectionFn = opts.wrapUnions(this.types); + } catch(e) { + throw new Error(`Error generating projection function: ${e}`); } } - - this.types.forEach(function (type, index) { - if (Type.isType(type, 'abstract', 'logical')) { - if (!this._dynamicBranches) { - this._dynamicBranches = []; - } - this._dynamicBranches.push({index, type}); - } else if (!hasWrapUnionsFn) { - let bucket = getTypeBucket(type); - if (this._bucketIndices[bucket] !== undefined) { - throw new Error(`ambiguous unwrapped union: ${j(this)}`); - } - this._bucketIndices[bucket] = index; - } - }, this); + this._getIndex = projectionFn + ? generateProjectionIndexer(projectionFn) + : generateDefaultIndexer.bind(this)(this.types); Object.freeze(this); } - _getIndex (val) { - let index = this.projectionFunction(val); - if (this._dynamicBranches) { - // Slower path, we must run the value through all branches. - index = this._getBranchIndex(val, index); - } - return index; - } - _getBranchIndex (any, index) { let logicalBranches = this._dynamicBranches; for (let i = 0, l = logicalBranches.length; i < l; i++) { diff --git a/test/test_types.js b/test/test_types.js index 0d99d5f3..7d5d2380 100644 --- a/test/test_types.js +++ b/test/test_types.js @@ -3549,7 +3549,7 @@ suite('types', () => { // Ambiguous, but we have a projection function const Animal = Type.forSchema(animalTypes, { wrapUnions: mockWrapUnions }); Animal.toBuffer({ meow: '🐈' }); - assert.equal(mockWrapUnions.calls, 2); + assert.equal(mockWrapUnions.calls, 1); assert.throws(() => Animal.toBuffer({ snap: '🐊' }), /Unknown animal/) }); From 5ecec5e8d128d59a26cdb128b7b4d114a62adc09 Mon Sep 17 00:00:00 2001 From: Joscha Feth Date: Sat, 28 Sep 2024 22:17:59 +0100 Subject: [PATCH 12/19] refactor: `callsToWrapUnions` --- test/test_types.js | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/test/test_types.js b/test/test_types.js index 7d5d2380..4578d7d2 100644 --- a/test/test_types.js +++ b/test/test_types.js @@ -3522,7 +3522,9 @@ suite('types', () => { }; const animalTypes = [Dog, Cat]; + let callsToWrapUnions = 0; const wrapUnions = (types) => { + callsToWrapUnions++; assert.deepEqual(types.map(t => t.name), ['Dog', 'Cat']); return (animal) => { const animalType = ((animal) => { @@ -3537,19 +3539,10 @@ suite('types', () => { } }; - // TODO: replace this with a mock when available - // currently we're on mocha without sinon - function mockWrapUnions() { - mockWrapUnions.calls = typeof mockWrapUnions.calls === 'undefined' - ? 1 - : ++mockWrapUnions.calls; - return wrapUnions.apply(null, arguments); - } - // Ambiguous, but we have a projection function - const Animal = Type.forSchema(animalTypes, { wrapUnions: mockWrapUnions }); + const Animal = Type.forSchema(animalTypes, { wrapUnions }); Animal.toBuffer({ meow: '🐈' }); - assert.equal(mockWrapUnions.calls, 1); + assert.equal(callsToWrapUnions, 1); assert.throws(() => Animal.toBuffer({ snap: '🐊' }), /Unknown animal/) }); From 52873d07bcaec4de91a5767ee970ae5c021797e5 Mon Sep 17 00:00:00 2001 From: Joscha Feth Date: Sat, 28 Sep 2024 22:27:02 +0100 Subject: [PATCH 13/19] style: 80 cols --- types/index.d.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/types/index.d.ts b/types/index.d.ts index 8aa282d3..3c1f5aba 100644 --- a/types/index.d.ts +++ b/types/index.d.ts @@ -97,10 +97,14 @@ interface EncoderOptions { /** * A projection function that is used when unwrapping unions. - * This function is called at schema parsing time on each union with its branches' types. - * If it returns a non-null (function) value, that function will be called each time a value's branch needs to be inferred and should return the branch's index. + * This function is called at schema parsing time on each union with its branches' + * types. + * If it returns a non-null (function) value, that function will be called each + * time a value's branch needs to be inferred and should return the branch's + * index. * The index muss be a number between 0 and length-1 of the passed types. - * In this case (a branch index) the union will use an unwrapped representation. Otherwise (undefined), the union will be wrapped. + * In this case (a branch index) the union will use an unwrapped representation. + * Otherwise (undefined), the union will be wrapped. */ type BranchProjection = (types: ReadonlyArray) => | ((val: unknown) => number) From 7629f6b6961ee9f8a55a26fe9ae4191caca91551 Mon Sep 17 00:00:00 2001 From: Joscha Feth Date: Sat, 28 Sep 2024 22:41:51 +0100 Subject: [PATCH 14/19] test: add test for `wrapUnions` returning `undefined` --- test/test_types.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/test_types.js b/test/test_types.js index 4578d7d2..e7675761 100644 --- a/test/test_types.js +++ b/test/test_types.js @@ -3546,6 +3546,16 @@ suite('types', () => { assert.throws(() => Animal.toBuffer({ snap: '🐊' }), /Unknown animal/) }); + test('union projection with fallback', () => { + let t = Type.forSchema({ + type: 'record', + fields: [ + {name: 'wrapped', type: ['int', 'double' ]}, // Ambiguous. + ] + }, {wrapUnions: () => undefined }); + assert(Type.isType(t.field('wrapped').type, 'union:wrapped')); + }); + test('invalid wrap unions option', () => { assert.throws(() => { Type.forSchema('string', {wrapUnions: 'FOO'}); From c0ead8bbc63bd1b8c77230c74ae019917c4961d0 Mon Sep 17 00:00:00 2001 From: Joscha Feth Date: Sat, 28 Sep 2024 22:43:00 +0100 Subject: [PATCH 15/19] style: remove comments --- lib/types.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/types.js b/lib/types.js index a2d243ab..068fa9cf 100644 --- a/lib/types.js +++ b/lib/types.js @@ -200,13 +200,11 @@ class Type { }); let projectionFn; if (!UnionType) { - // either automatic detection or we have a projection function if (typeof opts.wrapUnions === 'function') { // we have a projection function try { projectionFn = opts.wrapUnions(types); UnionType = typeof projectionFn !== 'undefined' - // projection function yields a function, we can use an Unwrapped type ? UnwrappedUnionType : WrappedUnionType; } catch(e) { From 0e6c4cd194882119e0d083ac6c0294d8c9e9629a Mon Sep 17 00:00:00 2001 From: Joscha Feth Date: Sat, 28 Sep 2024 22:48:41 +0100 Subject: [PATCH 16/19] docs: remove jsdoc --- lib/types.js | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/lib/types.js b/lib/types.js index 068fa9cf..f149fd95 100644 --- a/lib/types.js +++ b/lib/types.js @@ -1299,27 +1299,17 @@ function generateDefaultIndexer() { * + `map`, `record` */ class UnwrappedUnionType extends UnionType { - /** - * - * @param {*} schema - * @param {*} opts - * @param {Function|undefined} projectionFn The projection function used - * to determine the bucket for the - * Union. Falls back to generate - * from `wrapUnions` parameter - * if given. - */ - constructor (schema, opts, projectionFn) { + constructor (schema, opts, /* @private parameter */ _projectionFn) { super(schema, opts); - if (!projectionFn && opts && typeof opts.wrapUnions === 'function') { + if (!_projectionFn && opts && typeof opts.wrapUnions === 'function') { try { - projectionFn = opts.wrapUnions(this.types); + _projectionFn = opts.wrapUnions(this.types); } catch(e) { throw new Error(`Error generating projection function: ${e}`); } } - this._getIndex = projectionFn + this._getIndex = _projectionFn ? generateProjectionIndexer(projectionFn) : generateDefaultIndexer.bind(this)(this.types); From bd2c6ddbff0eb1b864e3445bcd834a4027b84210 Mon Sep 17 00:00:00 2001 From: Joscha Feth Date: Sat, 28 Sep 2024 23:03:09 +0100 Subject: [PATCH 17/19] refactor: move out of class --- lib/types.js | 74 ++++++++++++++++++++++++++-------------------------- 1 file changed, 37 insertions(+), 37 deletions(-) diff --git a/lib/types.js b/lib/types.js index f149fd95..5521296c 100644 --- a/lib/types.js +++ b/lib/types.js @@ -1252,28 +1252,44 @@ function generateProjectionIndexer(projectionFn) { }; } -function generateDefaultIndexer() { - this._dynamicBranches = null; - this._bucketIndices = {}; - this.types.forEach(function (type, index) { - if (Type.isType(type, 'abstract', 'logical')) { - if (!this._dynamicBranches) { - this._dynamicBranches = []; +function generateDefaultIndexer(types) { + const dynamicBranches = []; + const bucketIndices = {}; + + const getBranchIndex = (any, index) => { + let logicalBranches = dynamicBranches; + for (let i = 0, l = logicalBranches.length; i < l; i++) { + let branch = logicalBranches[i]; + if (branch.type._check(any)) { + if (index === undefined) { + index = branch.index; + } else { + // More than one branch matches the value so we aren't guaranteed to + // infer the correct type. We throw rather than corrupt data. This can + // be fixed by "tightening" the logical types. + throw new Error('ambiguous conversion'); + } } - this._dynamicBranches.push({index, type}); + } + return index; + } + + types.forEach(function (type, index) { + if (Type.isType(type, 'abstract', 'logical')) { + dynamicBranches.push({index, type}); } else { let bucket = getTypeBucket(type); - if (this._bucketIndices[bucket] !== undefined) { + if (bucketIndices[bucket] !== undefined) { throw new Error(`ambiguous unwrapped union: ${j(this)}`); } - this._bucketIndices[bucket] = index; + bucketIndices[bucket] = index; } }, this); return (val) => { - let index = this._bucketIndices[getValueBucket(val)]; - if (this._dynamicBranches) { + let index = bucketIndices[getValueBucket(val)]; + if (dynamicBranches.length) { // Slower path, we must run the value through all branches. - index = this._getBranchIndex(val, index); + index = getBranchIndex(val, index); } return index; }; @@ -1310,30 +1326,12 @@ class UnwrappedUnionType extends UnionType { } } this._getIndex = _projectionFn - ? generateProjectionIndexer(projectionFn) + ? generateProjectionIndexer(_projectionFn) : generateDefaultIndexer.bind(this)(this.types); Object.freeze(this); } - _getBranchIndex (any, index) { - let logicalBranches = this._dynamicBranches; - for (let i = 0, l = logicalBranches.length; i < l; i++) { - let branch = logicalBranches[i]; - if (branch.type._check(any)) { - if (index === undefined) { - index = branch.index; - } else { - // More than one branch matches the value so we aren't guaranteed to - // infer the correct type. We throw rather than corrupt data. This can - // be fixed by "tightening" the logical types. - throw new Error('ambiguous conversion'); - } - } - } - return index; - } - _check (val, flags, hook, path) { let index = this._getIndex(val); let b = index !== undefined; @@ -1393,16 +1391,18 @@ class UnwrappedUnionType extends UnionType { // Using the `coerceBuffers` option can cause corruption and erroneous // failures with unwrapped unions (in rare cases when the union also // contains a record which matches a buffer's JSON representation). - if (isJsonBuffer(val) && this._bucketIndices.buffer !== undefined) { - index = this._bucketIndices.buffer; - } else { - index = this._getIndex(val); + if (isJsonBuffer(val)) { + let bufIndex = this.types.findIndex(t => getTypeBucket(t) === 'buffer'); + if (bufIndex !== -1) { + index = bufIndex; + } } + index ??= this._getIndex(val); break; case 2: // Decoding from JSON, we must unwrap the value. if (val === null) { - index = this._bucketIndices['null']; + index = this._getIndex(null); } else if (typeof val === 'object') { let keys = Object.keys(val); if (keys.length === 1) { From 9f6eaffec66396cd891e39eca93a41e71cf8d321 Mon Sep 17 00:00:00 2001 From: Joscha Feth Date: Sun, 29 Sep 2024 07:35:59 +0100 Subject: [PATCH 18/19] remove wrapping --- lib/types.js | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/lib/types.js b/lib/types.js index 5521296c..cba76fd2 100644 --- a/lib/types.js +++ b/lib/types.js @@ -202,14 +202,10 @@ class Type { if (!UnionType) { if (typeof opts.wrapUnions === 'function') { // we have a projection function - try { - projectionFn = opts.wrapUnions(types); - UnionType = typeof projectionFn !== 'undefined' - ? UnwrappedUnionType - : WrappedUnionType; - } catch(e) { - throw new Error(`Error generating projection function: ${e}`); - } + projectionFn = opts.wrapUnions(types); + UnionType = typeof projectionFn !== 'undefined' + ? UnwrappedUnionType + : WrappedUnionType; } else { UnionType = isAmbiguous(types) ? WrappedUnionType : UnwrappedUnionType; } @@ -1319,11 +1315,7 @@ class UnwrappedUnionType extends UnionType { super(schema, opts); if (!_projectionFn && opts && typeof opts.wrapUnions === 'function') { - try { - _projectionFn = opts.wrapUnions(this.types); - } catch(e) { - throw new Error(`Error generating projection function: ${e}`); - } + _projectionFn = opts.wrapUnions(this.types); } this._getIndex = _projectionFn ? generateProjectionIndexer(_projectionFn) From d6692e16e5593d8bd48778e336e07a8fd7b8820a Mon Sep 17 00:00:00 2001 From: Joscha Feth Date: Sun, 29 Sep 2024 07:36:47 +0100 Subject: [PATCH 19/19] remove binding --- lib/types.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/types.js b/lib/types.js index cba76fd2..1e15acfc 100644 --- a/lib/types.js +++ b/lib/types.js @@ -1280,7 +1280,7 @@ function generateDefaultIndexer(types) { } bucketIndices[bucket] = index; } - }, this); + }); return (val) => { let index = bucketIndices[getValueBucket(val)]; if (dynamicBranches.length) { @@ -1319,7 +1319,7 @@ class UnwrappedUnionType extends UnionType { } this._getIndex = _projectionFn ? generateProjectionIndexer(_projectionFn) - : generateDefaultIndexer.bind(this)(this.types); + : generateDefaultIndexer(this.types); Object.freeze(this); }