fix(type): make sure handled constructor properties are not set twice

Previously even though a property was handled in the constructor it was additionally set on the object as well. This is wrong, since the constructor can arbitrarily change the behaviour of the property. This fixes the behaviour also in deepkit/bson.
deepkit · May 4, 2024 · 2e82eb6 · 2e82eb6
1 parent 4e6f06f
commit 2e82eb6
Show file tree

Hide file tree

Showing 7 changed files with 139 additions and 10 deletions.
diff --git a/packages/bson/src/bson-deserializer-templates.ts b/packages/bson/src/bson-deserializer-templates.ts
@@ -9,6 +9,7 @@ import {
     excludedAnnotation,
     executeTemplates,
     extendTemplateLiteral,
+    getDeepConstructorProperties,
     getIndexCheck,
     getNameExpression,
     getStaticDefaultCodeForProperty,
@@ -778,15 +779,20 @@ export function deserializeObjectLiteral(type: TypeClass | TypeObjectLiteral, st
     if (type.kind === ReflectionKind.class && type.classType !== Object) {
         const reflection = ReflectionClass.from(type.classType);
         const constructor = reflection.getConstructorOrUndefined();
+
         if (constructor && constructor.parameters.length) {
             const constructorArguments: string[] = [];
             for (const parameter of constructor.getParameters()) {
                 const name = getNameExpression(parameter.getName(), state);
                 constructorArguments.push(parameter.getVisibility() === undefined ? 'undefined' : `${object}[${name}]`);
             }
 
+            const constructorProperties = getDeepConstructorProperties(type).map(v => String(v.name));
+            const resetDefaultSets = constructorProperties.map(v => `delete ${object}.${v};`);
+
             createClassInstance = `
                 ${state.setter} = new ${state.compilerContext.reserveConst(type.classType, 'classType')}(${constructorArguments.join(', ')});
+                ${resetDefaultSets.join('\n')}
                 Object.assign(${state.setter}, ${object});
             `;
         } else {

diff --git a/packages/bson/tests/type-spec.spec.ts b/packages/bson/tests/type-spec.spec.ts
@@ -1068,3 +1068,40 @@ test('Map part of union', () => {
         expect(roundTrip<T2>({ tags: map })).toEqual({ tags: map });
     }
 });
+
+test('constructor property not assigned as property', () => {
+    //when a constructor property is assigned, it must be set via the constructor only
+    class Base {
+        constructor(public id: string) {
+        }
+    }
+
+    class Store {
+        id: string = '';
+    }
+
+    class Derived extends Base {
+        constructor(public store: Store) {
+            super(store.id.split(':')[0]);
+        }
+    }
+
+    const clazz = ReflectionClass.from(Derived);
+    expect(clazz.getConstructorOrUndefined()?.getParameter('store').isProperty()).toBe(true);
+    const parentConstructor = clazz.parent!.getConstructorOrUndefined();
+    expect(parentConstructor!.getParameter('id').isProperty()).toBe(true);
+
+    const store = new Store;
+    store.id = 'foo:bar';
+    const derived = new Derived(store);
+    expect(derived.id).toBe('foo');
+
+    const json = serializeToJson<Derived>(derived);
+    expect(json).toEqual({ id: 'foo', store: { id: 'foo:bar' } });
+
+    const back = deserializeFromJson<Derived>({
+        id: 'unrelated',
+        store: { id: 'foo:bar' },
+    });
+    expect(back).toEqual(derived);
+});
diff --git a/packages/core/src/core.ts b/packages/core/src/core.ts
@@ -698,6 +698,15 @@ export function getParentClass(classType: ClassType): ClassType | undefined {
     return parent;
 }
 
+export function getInheritanceChain(classType: ClassType): ClassType[] {
+    const chain: ClassType[] = [classType];
+    let current = classType;
+    while (current = getParentClass(current) as ClassType) {
+        chain.push(current);
+    }
+    return chain;
+}
+
 declare var v8debug: any;
 
 export function inDebugMode() {

diff --git a/packages/type/src/reflection/reflection.ts b/packages/type/src/reflection/reflection.ts
@@ -383,6 +383,23 @@ export class ReflectionParameter {
     isPrivate(): boolean {
         return this.parameter.visibility === ReflectionVisibility.private;
     }
+
+    isReadonly(): boolean {
+        return this.parameter.readonly === true;
+    }
+
+    /**
+     * True if the parameter becomes a property in the class.
+     * This is the case for parameters in constructors with visibility or readonly.
+     *
+     * ```typescript
+     * class User {
+     *    constructor(public name: string) {}
+     * }
+     */
+    isProperty(): boolean {
+        return this.parameter.readonly === true || this.parameter.visibility !== undefined;
+    }
 }
 
 /**

diff --git a/packages/type/src/reflection/type.ts b/packages/type/src/reflection/type.ts
@@ -13,13 +13,14 @@ import {
     arrayRemoveItem,
     ClassType,
     getClassName,
+    getInheritanceChain,
     getParentClass,
     indent,
     isArray,
     isClass,
 } from '@deepkit/core';
 import { TypeNumberBrand } from '@deepkit/type-spec';
-import { getProperty, ReceiveType, reflect, ReflectionClass, toSignature } from './reflection.js';
+import { getProperty, ReceiveType, reflect, ReflectionClass, resolveReceiveType, toSignature } from './reflection.js';
 import { isExtendable } from './extends.js';
 import { state } from './state.js';
 import { resolveRuntimeType } from './processor.js';
@@ -2315,6 +2316,27 @@ export function stringifyShortResolvedType(type: Type, stateIn: Partial<Stringif
     return stringifyType(type, { ...stateIn, showNames: false, showFullDefinition: false, });
 }
 
+/**
+ * Returns all (including inherited) constructor properties of a class.
+ */
+export function getDeepConstructorProperties(type: TypeClass): TypeParameter[] {
+    const chain = getInheritanceChain(type.classType);
+    const res: TypeParameter[] = [];
+    for (const classType of chain) {
+        const type = resolveReceiveType(classType) as TypeClass;
+        if (type.kind !== ReflectionKind.class) continue;
+        const constructor = findMember('constructor', type.types);
+        if (!constructor || constructor.kind !== ReflectionKind.method) continue;
+        for (const param of constructor.parameters) {
+            if (param.kind !== ReflectionKind.parameter) continue;
+            if (param.readonly === true || param.visibility !== undefined) {
+                res.push(param);
+            }
+        }
+    }
+    return res;
+}
+
 interface StringifyTypeOptions {
     //show type alias names
     showNames: boolean;

diff --git a/packages/type/src/serializer.ts b/packages/type/src/serializer.ts
@@ -37,6 +37,7 @@ import {
     excludedAnnotation,
     FindType,
     getConstructorProperties,
+    getDeepConstructorProperties,
     getTypeJitContainer,
     getTypeObjectLiteralFromTypeClass,
     groupAnnotation,
@@ -1147,9 +1148,10 @@ export function serializeObjectLiteral(type: TypeObjectLiteral | TypeClass, stat
         const clazz = ReflectionClass.from(type.classType);
         const constructor = clazz.getConstructorOrUndefined();
         if (!clazz.disableConstructor && constructor) {
+            handledPropertiesInConstructor.push(...getDeepConstructorProperties(type).map(v => String(v.name)));
             const parameters = constructor.getParameters();
             for (const parameter of parameters) {
-                if (parameter.getVisibility() === undefined) {
+                if (!parameter.isProperty()) {
                     constructorArguments.push('undefined');
                     continue;
                 }
@@ -1160,7 +1162,6 @@ export function serializeObjectLiteral(type: TypeObjectLiteral | TypeClass, stat
                 if (property.isSerializerExcluded(state.registry.serializer.name)) {
                     continue;
                 }
-                handledPropertiesInConstructor.push(parameter.getName());
                 const argumentName = state.compilerContext.reserveVariable('c_' + parameter.getName());
 
                 const readName = getNameExpression(state.namingStrategy.getPropertyName(property.property, state.registry.serializer.name), state);

diff --git a/packages/type/tests/type-spec.spec.ts b/packages/type/tests/type-spec.spec.ts
@@ -5,7 +5,7 @@ import { cast, cloneClass, serialize } from '../src/serializer-facade.js';
 import { createReference } from '../src/reference.js';
 import { unpopulatedSymbol } from '../src/core.js';
 
-(BigInt.prototype as any).toJSON = function () {
+(BigInt.prototype as any).toJSON = function() {
     return this.toString();
 };
 
@@ -244,7 +244,7 @@ test('model 1', () => {
             filter: undefined,
             skip: undefined,
             limit: undefined,
-            sort: undefined
+            sort: undefined,
         };
         expect(roundTrip<Model>(model as any)).toEqual(model);
     }
@@ -357,7 +357,7 @@ test('relation 2', () => {
                 name: 'Marc 1',
                 id: 3,
                 version: 0,
-            })
+            }),
         ];
 
         expect(roundTrip<User[]>(items)).toEqual(items);
@@ -436,7 +436,7 @@ test('partial returns the model at second level', () => {
 
     expect(roundTrip<Partial<Model>>({ id: 23, config: config } as any)).toEqual({
         id: 23,
-        config: { big: false, color: 'red' }
+        config: { big: false, color: 'red' },
     });
     expect(roundTrip<Partial<Model>>({ id: 23, config: config } as any).config).toBeInstanceOf(Config);
 });
@@ -587,7 +587,7 @@ test('omit circular reference 1', () => {
         another?: Model;
 
         constructor(
-            public id: number = 0
+            public id: number = 0,
         ) {
         }
     }
@@ -790,11 +790,11 @@ test('array with mongoid', () => {
     }
 
     expect(deserializeFromJson<Model>({ references: [{ cls: 'User', id: '5f3b9b3b9c6b2b1b1c0b1b1b' }] })).toEqual({
-        references: [{ cls: 'User', id: '5f3b9b3b9c6b2b1b1c0b1b1b' }]
+        references: [{ cls: 'User', id: '5f3b9b3b9c6b2b1b1c0b1b1b' }],
     });
 
     expect(serializeToJson<Model>({ references: [{ cls: 'User', id: '5f3b9b3b9c6b2b1b1c0b1b1b' }] })).toEqual({
-        references: [{ cls: 'User', id: '5f3b9b3b9c6b2b1b1c0b1b1b' }]
+        references: [{ cls: 'User', id: '5f3b9b3b9c6b2b1b1c0b1b1b' }],
     });
 });
 
@@ -821,3 +821,40 @@ test('Map part of union', () => {
         expect(roundTrip<T2>({ tags: map })).toEqual({ tags: map });
     }
 });
+
+test('constructor property not assigned as property', () => {
+    //when a constructor property is assigned, it must be set via the constructor only
+    class Base {
+        constructor(public id: string) {
+        }
+    }
+
+    class Store {
+        id: string = '';
+    }
+
+    class Derived extends Base {
+        constructor(public store: Store) {
+            super(store.id.split(':')[0]);
+        }
+    }
+
+    const clazz = ReflectionClass.from(Derived);
+    expect(clazz.getConstructorOrUndefined()?.getParameter('store').isProperty()).toBe(true);
+    const parentConstructor = clazz.parent!.getConstructorOrUndefined();
+    expect(parentConstructor!.getParameter('id').isProperty()).toBe(true);
+
+    const store = new Store;
+    store.id = 'foo:bar';
+    const derived = new Derived(store);
+    expect(derived.id).toBe('foo');
+
+    const json = serializeToJson<Derived>(derived);
+    expect(json).toEqual({ id: 'foo', store: { id: 'foo:bar' } });
+
+    const back = deserializeFromJson<Derived>({
+        id: 'unrelated',
+        store: { id: 'foo:bar' },
+    });
+    expect(back).toEqual(derived);
+});