From 58bd8e12fba17986627351b79d032567af2b6e18 Mon Sep 17 00:00:00 2001 From: Asad Ali Date: Mon, 15 Jul 2024 12:03:19 +0500 Subject: [PATCH 1/3] fix: optional and nullable handling in discriminatedSchema --- .../schema/src/types/discriminatedObject.ts | 51 +++-- packages/schema/src/types/object.ts | 5 +- .../test/types/discriminatedObject.test.ts | 181 +++++++++++++----- 3 files changed, 170 insertions(+), 67 deletions(-) diff --git a/packages/schema/src/types/discriminatedObject.ts b/packages/schema/src/types/discriminatedObject.ts index 5005ef68..71f1a120 100644 --- a/packages/schema/src/types/discriminatedObject.ts +++ b/packages/schema/src/types/discriminatedObject.ts @@ -1,4 +1,9 @@ -import { Schema, SchemaMappedType, SchemaType } from '../schema'; +import { + Schema, + SchemaMappedType, + SchemaType, + SchemaValidationError, +} from '../schema'; import { objectEntries } from '../utils'; import { ObjectXmlOptions } from './object'; @@ -14,9 +19,11 @@ export function discriminatedObject< defaultDiscriminator: keyof TDiscrimMap, xmlOptions?: ObjectXmlOptions ): Schema { - const schemaSelector = ( + const allSchemas = Object.values(discriminatorMap).reverse(); + const selectSchema = ( value: unknown, discriminatorProp: string | TDiscrimProp | TDiscrimMappedProp, + checker: (schema: TSchema) => SchemaValidationError[], isAttr: boolean = false ) => { if ( @@ -39,41 +46,53 @@ export function discriminatedObject< return discriminatorMap[discriminatorValue]; } } + for (const key in allSchemas) { + if (checker(allSchemas[key]).length === 0) { + return allSchemas[key]; + } + } return discriminatorMap[defaultDiscriminator]; }; + return { type: () => - `DiscriminatedUnion<${discriminatorPropName},[${objectEntries( + `DiscriminatedUnion<${discriminatorPropName as string},[${objectEntries( discriminatorMap ) .map(([_, v]) => v.type) .join(',')}]>`, map: (value, ctxt) => - schemaSelector(value, discriminatorPropName).map(value, ctxt), + selectSchema(value, discriminatorPropName, (schema) => + schema.validateBeforeMap(value, ctxt) + ).map(value, ctxt), unmap: (value, ctxt) => - schemaSelector(value, discriminatorMappedPropName).unmap(value, ctxt), + selectSchema(value, discriminatorMappedPropName, (schema) => + schema.validateBeforeUnmap(value, ctxt) + ).unmap(value, ctxt), validateBeforeMap: (value, ctxt) => - schemaSelector(value, discriminatorPropName).validateBeforeMap( - value, - ctxt - ), + selectSchema(value, discriminatorPropName, (schema) => + schema.validateBeforeMap(value, ctxt) + ).validateBeforeMap(value, ctxt), validateBeforeUnmap: (value, ctxt) => - schemaSelector(value, discriminatorMappedPropName).validateBeforeUnmap( - value, - ctxt - ), + selectSchema(value, discriminatorMappedPropName, (schema) => + schema.validateBeforeUnmap(value, ctxt) + ).validateBeforeUnmap(value, ctxt), mapXml: (value, ctxt) => - schemaSelector( + selectSchema( value, xmlOptions?.xmlName ?? discriminatorPropName, + (schema) => schema.validateBeforeMapXml(value, ctxt), xmlOptions?.isAttr ).mapXml(value, ctxt), unmapXml: (value, ctxt) => - schemaSelector(value, discriminatorMappedPropName).unmapXml(value, ctxt), + selectSchema(value, discriminatorMappedPropName, (schema) => + schema.validateBeforeUnmap(value, ctxt) + ).unmapXml(value, ctxt), validateBeforeMapXml: (value, ctxt) => - schemaSelector( + selectSchema( value, xmlOptions?.xmlName ?? discriminatorPropName, + (schema) => schema.validateBeforeMapXml(value, ctxt), xmlOptions?.isAttr ).validateBeforeMapXml(value, ctxt), }; diff --git a/packages/schema/src/types/object.ts b/packages/schema/src/types/object.ts index e662e260..3317d4c8 100644 --- a/packages/schema/src/types/object.ts +++ b/packages/schema/src/types/object.ts @@ -377,7 +377,10 @@ function validateValueObject({ ctxt.createChild(propTypePrefix + key, valueObject[key], schema) ) ); - } else if (schema.type().indexOf('Optional<') !== 0) { + } else if ( + !schema.type().startsWith('Optional<') && + !schema.type().startsWith('Nullable<') + ) { // Add to missing keys if it is not an optional property missingProps.add(key); } diff --git a/packages/schema/test/types/discriminatedObject.test.ts b/packages/schema/test/types/discriminatedObject.test.ts index 59152a92..617ad069 100644 --- a/packages/schema/test/types/discriminatedObject.test.ts +++ b/packages/schema/test/types/discriminatedObject.test.ts @@ -2,8 +2,9 @@ import { boolean, discriminatedObject, extendStrictObject, - literal, + nullable, number, + optional, strictObject, string, validateAndMap, @@ -12,21 +13,21 @@ import { describe('Discriminated Object', () => { const baseType = strictObject({ - type: ['type mapped', string()], + type: ['type mapped', optional(string())], baseField: ['base field', number()], }); const childType1 = extendStrictObject(baseType, { - type: ['type mapped', literal('child1')], + type: ['type mapped', optional(string())], child1Field: ['child1 field', boolean()], }); const childType2 = extendStrictObject(baseType, { - type: ['type mapped', literal('child2')], + type: ['type mapped', optional(string())], child2Field: ['child2 field', boolean()], }); - const schema = discriminatedObject( + const discriminatedSchema = discriminatedObject( 'type', 'type mapped', { @@ -37,6 +38,10 @@ describe('Discriminated Object', () => { 'base' ); + const nestedDiscriminatedObject = strictObject({ + innerType: ['inner type', nullable(discriminatedSchema)], + }); + describe('Mapping', () => { it('should map to child type on discriminator match', () => { const input = { @@ -44,7 +49,7 @@ describe('Discriminated Object', () => { 'base field': 123123, 'child1 field': true, }; - const output = validateAndMap(input, schema); + const output = validateAndMap(input, discriminatedSchema); expect(output.errors).toBeFalsy(); expect((output as any).result).toStrictEqual({ type: 'child1', @@ -53,13 +58,67 @@ describe('Discriminated Object', () => { }); }); + it('should map to child type without discriminator match', () => { + const input = { + 'type mapped': 'hello world', + 'base field': 123123, + 'child1 field': true, + }; + const output = validateAndMap(input, discriminatedSchema); + expect(output.errors).toBeFalsy(); + expect((output as any).result).toStrictEqual({ + type: 'hello world', + baseField: 123123, + child1Field: true, + }); + }); + + it('should map to child type with missing discriminator', () => { + const input = { + 'base field': 123123, + 'child1 field': true, + }; + const output = validateAndMap(input, discriminatedSchema); + expect(output.errors).toBeFalsy(); + expect((output as any).result).toStrictEqual({ + baseField: 123123, + child1Field: true, + }); + }); + + it('should map to base type on discriminator match', () => { + const input = { + 'type mapped': 'base', + 'base field': 123123, + }; + const output = validateAndMap(input, discriminatedSchema); + expect(output.errors).toBeFalsy(); + expect((output as any).result).toStrictEqual({ + type: 'base', + baseField: 123123, + }); + }); + + it('should map to base type without discriminator match', () => { + const input = { + 'type mapped': 'hello world', + 'base field': 123123, + }; + const output = validateAndMap(input, discriminatedSchema); + expect(output.errors).toBeFalsy(); + expect((output as any).result).toStrictEqual({ + type: 'hello world', + baseField: 123123, + }); + }); + it('should fail on schema invalidation', () => { const input = { 'type mapped': 'child1', 'base field': 123123, 'child1 field': 101, }; - const output = validateAndMap(input, schema); + const output = validateAndMap(input, discriminatedSchema); expect(output.errors).toBeTruthy(); expect(output.errors).toMatchInlineSnapshot(` Array [ @@ -88,45 +147,93 @@ describe('Discriminated Object', () => { `); }); - it('should map to base type on discriminator match', () => { + it('should map to nestedDiscriminatedObject with null', () => { + const input = {}; + const output = validateAndMap(input, nestedDiscriminatedObject); + expect(output.errors).toBeFalsy(); + expect((output as any).result).toStrictEqual({ + innerType: null, + }); + }); + }); + describe('Unmapping', () => { + it('should unmap child type on discriminator match', () => { const input = { - 'type mapped': 'base', - 'base field': 123123, + type: 'child1', + baseField: 123123, + child1Field: true, }; - const output = validateAndMap(input, schema); + const output = validateAndUnmap(input, discriminatedSchema); expect(output.errors).toBeFalsy(); expect((output as any).result).toStrictEqual({ - type: 'base', - baseField: 123123, + 'type mapped': 'child1', + 'base field': 123123, + 'child1 field': true, }); }); - it('should map to base type on no discriminator match', () => { + it('should unmap child type without discriminator match', () => { const input = { + type: 'hello world', + baseField: 123123, + child1Field: true, + }; + const output = validateAndUnmap(input, discriminatedSchema); + expect(output.errors).toBeFalsy(); + expect((output as any).result).toStrictEqual({ 'type mapped': 'hello world', 'base field': 123123, + 'child1 field': true, + }); + }); + + it('should unmap child type with missing discriminator', () => { + const input = { + baseField: 123123, + child1Field: true, }; - const output = validateAndMap(input, schema); + const output = validateAndUnmap(input, discriminatedSchema); expect(output.errors).toBeFalsy(); expect((output as any).result).toStrictEqual({ + 'base field': 123123, + 'child1 field': true, + }); + }); + + it('should unmap base type on discriminator match', () => { + const input = { + type: 'base', + baseField: 123123, + }; + const output = validateAndUnmap(input, discriminatedSchema); + expect(output.errors).toBeFalsy(); + expect((output as any).result).toStrictEqual({ + 'type mapped': 'base', + 'base field': 123123, + }); + }); + + it('should unmap base type without discriminator match', () => { + const input = { type: 'hello world', baseField: 123123, + }; + const output = validateAndUnmap(input, discriminatedSchema); + expect(output.errors).toBeFalsy(); + expect((output as any).result).toStrictEqual({ + 'type mapped': 'hello world', + 'base field': 123123, }); }); - }); - describe('Unmapping', () => { - it('should map to child type on discriminator match', () => { + + it('should unmap base type with missing discriminator', () => { const input = { - type: 'child1', baseField: 123123, - child1Field: true, }; - const output = validateAndUnmap(input, schema); + const output = validateAndUnmap(input, discriminatedSchema); expect(output.errors).toBeFalsy(); expect((output as any).result).toStrictEqual({ - 'type mapped': 'child1', 'base field': 123123, - 'child1 field': true, }); }); @@ -136,7 +243,7 @@ describe('Discriminated Object', () => { baseField: 123123, child1Field: 101, }; - const output = validateAndUnmap(input, schema); + const output = validateAndUnmap(input, discriminatedSchema); expect(output.errors).toBeTruthy(); expect(output.errors).toMatchInlineSnapshot(` Array [ @@ -164,31 +271,5 @@ describe('Discriminated Object', () => { ] `); }); - - it('should map to base type on discriminator match', () => { - const input = { - type: 'base', - baseField: 123123, - }; - const output = validateAndUnmap(input, schema); - expect(output.errors).toBeFalsy(); - expect((output as any).result).toStrictEqual({ - 'type mapped': 'base', - 'base field': 123123, - }); - }); - - it('should map to base type on no discriminator match', () => { - const input = { - type: 'hello world', - baseField: 123123, - }; - const output = validateAndUnmap(input, schema); - expect(output.errors).toBeFalsy(); - expect((output as any).result).toStrictEqual({ - 'type mapped': 'hello world', - 'base field': 123123, - }); - }); }); }); From 7d7a6a105736cfa6115761bd1d617babf4456ef8 Mon Sep 17 00:00:00 2001 From: Asad Ali Date: Mon, 15 Jul 2024 12:29:40 +0500 Subject: [PATCH 2/3] refactor: refactored for code climate suggestions --- .../schema/src/types/discriminatedObject.ts | 78 ++++++++++--------- 1 file changed, 43 insertions(+), 35 deletions(-) diff --git a/packages/schema/src/types/discriminatedObject.ts b/packages/schema/src/types/discriminatedObject.ts index 71f1a120..0e7b5ea7 100644 --- a/packages/schema/src/types/discriminatedObject.ts +++ b/packages/schema/src/types/discriminatedObject.ts @@ -1,5 +1,6 @@ import { Schema, + SchemaContextCreator, SchemaMappedType, SchemaType, SchemaValidationError, @@ -19,12 +20,10 @@ export function discriminatedObject< defaultDiscriminator: keyof TDiscrimMap, xmlOptions?: ObjectXmlOptions ): Schema { - const allSchemas = Object.values(discriminatorMap).reverse(); - const selectSchema = ( + const selectSchemaWithDisc = ( value: unknown, discriminatorProp: string | TDiscrimProp | TDiscrimMappedProp, - checker: (schema: TSchema) => SchemaValidationError[], - isAttr: boolean = false + isAttr: boolean ) => { if ( typeof value === 'object' && @@ -46,14 +45,47 @@ export function discriminatedObject< return discriminatorMap[discriminatorValue]; } } + return undefined; + }; + const allSchemas = Object.values(discriminatorMap).reverse(); + const selectSchema = ( + value: unknown, + discriminatorProp: string | TDiscrimProp | TDiscrimMappedProp, + checker: (schema: TSchema) => SchemaValidationError[], + isAttr: boolean = false + ) => { + const schema = selectSchemaWithDisc(value, discriminatorProp, isAttr); + if (typeof schema !== 'undefined') { + return schema; + } + // Try checking with discriminator matching for (const key in allSchemas) { if (checker(allSchemas[key]).length === 0) { return allSchemas[key]; } } + // Fallback to default schema return discriminatorMap[defaultDiscriminator]; }; + const mapJsonSchema = (value: unknown, ctxt: SchemaContextCreator) => + selectSchema(value, discriminatorPropName, (schema) => + schema.validateBeforeMap(value, ctxt) + ); + + const mapXmlSchema = (value: unknown, ctxt: SchemaContextCreator) => + selectSchema( + value, + xmlOptions?.xmlName ?? discriminatorPropName, + (schema) => schema.validateBeforeMapXml(value, ctxt), + xmlOptions?.isAttr + ); + + const unmapSchema = (value: unknown, ctxt: SchemaContextCreator) => + selectSchema(value, discriminatorMappedPropName, (schema) => + schema.validateBeforeUnmap(value, ctxt) + ); + return { type: () => `DiscriminatedUnion<${discriminatorPropName as string},[${objectEntries( @@ -61,40 +93,16 @@ export function discriminatedObject< ) .map(([_, v]) => v.type) .join(',')}]>`, - map: (value, ctxt) => - selectSchema(value, discriminatorPropName, (schema) => - schema.validateBeforeMap(value, ctxt) - ).map(value, ctxt), - unmap: (value, ctxt) => - selectSchema(value, discriminatorMappedPropName, (schema) => - schema.validateBeforeUnmap(value, ctxt) - ).unmap(value, ctxt), + map: (value, ctxt) => mapJsonSchema(value, ctxt).map(value, ctxt), + unmap: (value, ctxt) => unmapSchema(value, ctxt).unmap(value, ctxt), validateBeforeMap: (value, ctxt) => - selectSchema(value, discriminatorPropName, (schema) => - schema.validateBeforeMap(value, ctxt) - ).validateBeforeMap(value, ctxt), + mapJsonSchema(value, ctxt).validateBeforeMap(value, ctxt), validateBeforeUnmap: (value, ctxt) => - selectSchema(value, discriminatorMappedPropName, (schema) => - schema.validateBeforeUnmap(value, ctxt) - ).validateBeforeUnmap(value, ctxt), - mapXml: (value, ctxt) => - selectSchema( - value, - xmlOptions?.xmlName ?? discriminatorPropName, - (schema) => schema.validateBeforeMapXml(value, ctxt), - xmlOptions?.isAttr - ).mapXml(value, ctxt), - unmapXml: (value, ctxt) => - selectSchema(value, discriminatorMappedPropName, (schema) => - schema.validateBeforeUnmap(value, ctxt) - ).unmapXml(value, ctxt), + unmapSchema(value, ctxt).validateBeforeUnmap(value, ctxt), + mapXml: (value, ctxt) => mapXmlSchema(value, ctxt).mapXml(value, ctxt), + unmapXml: (value, ctxt) => unmapSchema(value, ctxt).unmapXml(value, ctxt), validateBeforeMapXml: (value, ctxt) => - selectSchema( - value, - xmlOptions?.xmlName ?? discriminatorPropName, - (schema) => schema.validateBeforeMapXml(value, ctxt), - xmlOptions?.isAttr - ).validateBeforeMapXml(value, ctxt), + mapXmlSchema(value, ctxt).validateBeforeMapXml(value, ctxt), }; } From 3b120abaf06568ba886dd2f9cf3ad1ee8b9f1261 Mon Sep 17 00:00:00 2001 From: Asad Ali Date: Thu, 18 Jul 2024 11:14:54 +0500 Subject: [PATCH 3/3] refactor: resolved PR comments --- packages/schema/src/types/discriminatedObject.ts | 8 ++++---- packages/schema/src/types/object.ts | 6 ++---- packages/schema/src/utils.ts | 13 ++++++++++--- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/packages/schema/src/types/discriminatedObject.ts b/packages/schema/src/types/discriminatedObject.ts index 0e7b5ea7..df02987c 100644 --- a/packages/schema/src/types/discriminatedObject.ts +++ b/packages/schema/src/types/discriminatedObject.ts @@ -23,7 +23,7 @@ export function discriminatedObject< const selectSchemaWithDisc = ( value: unknown, discriminatorProp: string | TDiscrimProp | TDiscrimMappedProp, - isAttr: boolean + isAttr?: boolean ) => { if ( typeof value === 'object' && @@ -51,8 +51,8 @@ export function discriminatedObject< const selectSchema = ( value: unknown, discriminatorProp: string | TDiscrimProp | TDiscrimMappedProp, - checker: (schema: TSchema) => SchemaValidationError[], - isAttr: boolean = false + validater: (schema: TSchema) => SchemaValidationError[], + isAttr?: boolean ) => { const schema = selectSchemaWithDisc(value, discriminatorProp, isAttr); if (typeof schema !== 'undefined') { @@ -60,7 +60,7 @@ export function discriminatedObject< } // Try checking with discriminator matching for (const key in allSchemas) { - if (checker(allSchemas[key]).length === 0) { + if (validater(allSchemas[key]).length === 0) { return allSchemas[key]; } } diff --git a/packages/schema/src/types/object.ts b/packages/schema/src/types/object.ts index 3317d4c8..77e9fd94 100644 --- a/packages/schema/src/types/object.ts +++ b/packages/schema/src/types/object.ts @@ -9,6 +9,7 @@ import { OptionalizeObject } from '../typeUtils'; import { isOptional, isOptionalNullable, + isOptionalOrNullableType, literalToString, objectEntries, objectKeyEncode, @@ -377,10 +378,7 @@ function validateValueObject({ ctxt.createChild(propTypePrefix + key, valueObject[key], schema) ) ); - } else if ( - !schema.type().startsWith('Optional<') && - !schema.type().startsWith('Nullable<') - ) { + } else if (!isOptionalOrNullableType(schema.type())) { // Add to missing keys if it is not an optional property missingProps.add(key); } diff --git a/packages/schema/src/utils.ts b/packages/schema/src/utils.ts index 48ece39d..f813f341 100644 --- a/packages/schema/src/utils.ts +++ b/packages/schema/src/utils.ts @@ -161,9 +161,16 @@ export function isOptional(type: string, value: unknown): boolean { } export function isOptionalNullable(type: string, value: unknown): boolean { + return isOptionalAndNullableType(type) && isNullOrMissing(value); +} + +export function isOptionalAndNullableType(type: string): boolean { return ( - (type.startsWith('Optional