From 02bcdba6eececf65faa49468f26451b307a12543 Mon Sep 17 00:00:00 2001 From: Michael Bromley Date: Tue, 17 Dec 2024 14:04:29 +0100 Subject: [PATCH] fix(core): Fix circular reference error in email sending job Fixes to #3277 --- .../core/e2e/entity-serialization.e2e-spec.ts | 11 +++ packages/core/src/config/config.service.ts | 8 ++ .../shipping-eligibility-checker.ts | 8 ++ .../shipping-method/shipping-method.entity.ts | 2 +- packages/core/src/job-queue/job.spec.ts | 94 ++++++++++++++----- packages/core/src/job-queue/job.ts | 64 +++++++++---- 6 files changed, 145 insertions(+), 42 deletions(-) diff --git a/packages/core/e2e/entity-serialization.e2e-spec.ts b/packages/core/e2e/entity-serialization.e2e-spec.ts index 4759a9896c..d685db11e2 100644 --- a/packages/core/e2e/entity-serialization.e2e-spec.ts +++ b/packages/core/e2e/entity-serialization.e2e-spec.ts @@ -214,6 +214,17 @@ describe('Entity serialization', () => { assertCanBeSerialized(shippingLine); }); + it('serialize Order with nested ShippingMethod', async () => { + const ctx = await createCtx(); + const order = await server.app + .get(OrderService) + .findOne(ctx, 1, ['lines', 'shippingLines.shippingMethod']); + + expect(order).not.toBeNull(); + expect(order instanceof Order).toBe(true); + assertCanBeSerialized(order); + }); + function assertCanBeSerialized(entity: any) { try { const json = JSON.stringify(entity); diff --git a/packages/core/src/config/config.service.ts b/packages/core/src/config/config.service.ts index 50367a8239..6f551f3a37 100644 --- a/packages/core/src/config/config.service.ts +++ b/packages/core/src/config/config.service.ts @@ -146,4 +146,12 @@ export class ConfigService implements VendureConfig { } return definedCustomFields; } + + /** + * This is a precaution against attempting to JSON.stringify() a reference to + * this class, which can lead to a circular reference error. + */ + protected toJSON() { + return {}; + } } diff --git a/packages/core/src/config/shipping-method/shipping-eligibility-checker.ts b/packages/core/src/config/shipping-method/shipping-eligibility-checker.ts index 6c5131357a..726d814a30 100644 --- a/packages/core/src/config/shipping-method/shipping-eligibility-checker.ts +++ b/packages/core/src/config/shipping-method/shipping-eligibility-checker.ts @@ -113,6 +113,14 @@ export class ShippingEligibilityChecker< } return true; } + + /** + * This is a precaution against attempting to JSON.stringify() a reference to + * this class, which can lead to a circular reference error. + */ + protected toJSON() { + return {}; + } } /** diff --git a/packages/core/src/entity/shipping-method/shipping-method.entity.ts b/packages/core/src/entity/shipping-method/shipping-method.entity.ts index 3cf5b40653..2f72fded5b 100644 --- a/packages/core/src/entity/shipping-method/shipping-method.entity.ts +++ b/packages/core/src/entity/shipping-method/shipping-method.entity.ts @@ -102,7 +102,7 @@ export class ShippingMethod * This is a fix for https://github.com/vendure-ecommerce/vendure/issues/3277, * to prevent circular references which cause the JSON.stringify() to fail. */ - protected toJSON() { + protected toJSON(): any { return omit(this, ['allCheckers', 'allCalculators'] as any); } } diff --git a/packages/core/src/job-queue/job.spec.ts b/packages/core/src/job-queue/job.spec.ts index ab47e2d44c..82292555b6 100644 --- a/packages/core/src/job-queue/job.spec.ts +++ b/packages/core/src/job-queue/job.spec.ts @@ -116,36 +116,80 @@ describe('Job class', () => { name: 'parent', child: { name: 'child', - parent: { - child: { - name: 'child', - parent: { - child: { - name: 'child', - parent: { - child: { - name: 'child', - parent: { - child: { - name: 'child', - parent: { - child: '[max depth reached]', - name: '[max depth reached]', - }, - }, - name: 'parent', - }, - }, - name: 'parent', - }, - }, - name: 'parent', + parent: '[circular *child.parent]', + }, + }); + }); + + it('handles objects with deep cycles', () => { + const parent = { + name: 'parent', + child1: { + name: 'child1', + child2: { + name: 'child2', + child3: { + name: 'child3', + child4: { + name: 'child4', + parent: {} as any, + }, + }, + }, + }, + }; + parent.child1.child2.child3.child4.parent = parent; + + const job = new Job({ + queueName: 'test', + data: parent, + }); + + expect(job.data).toEqual({ + name: 'parent', + child1: { + name: 'child1', + child2: { + name: 'child2', + child3: { + name: 'child3', + child4: { + name: 'child4', + parent: '[circular *child1.child2.child3.child4.parent]', }, }, - name: 'parent', }, }, }); }); + + it('handles class instances with cycles', async () => { + class Parent { + name = 'parent'; + child = new Child(); + } + + class Child { + name = 'child'; + parent = undefined as Parent | undefined; + } + + const parent = new Parent(); + const child = parent.child; + child.parent = parent; + + const job = new Job({ + queueName: 'test', + data: parent as any, + }); + + expect(job.data).toEqual({ + name: 'parent', + child: { + name: 'child', + parent: '[circular *child.parent]', + }, + }); + }); }); }); diff --git a/packages/core/src/job-queue/job.ts b/packages/core/src/job-queue/job.ts index f45622ef81..13050077d9 100644 --- a/packages/core/src/job-queue/job.ts +++ b/packages/core/src/job-queue/job.ts @@ -231,27 +231,55 @@ export class Job = any> { * already be serializable per the TS type, in practice data can slip through due to loss of * type safety. */ - private ensureDataIsSerializable(data: any, depth = 0): any { + private ensureDataIsSerializable( + data: any, + depth = 0, + seen = new WeakMap(), + path: string[] = [], + ): any { if (10 < depth) { return '[max depth reached]'; } - depth++; - let output: any; + if (data === null || data === undefined) { + return data; + } + // Handle Date objects if (data instanceof Date) { return data.toISOString(); - } else if (isObject(data)) { - if (!output) { - output = {}; - } - for (const key of Object.keys(data)) { - output[key] = this.ensureDataIsSerializable((data as any)[key], depth); + } + + if (typeof data === 'object' && data !== null) { + const seenData = seen.get(data); + if (seenData && seenData.length < path.length) { + return `[circular *${path.join('.')}]`; } - if (isClassInstance(data)) { - const descriptors = Object.getOwnPropertyDescriptors(Object.getPrototypeOf(data)); - for (const name of Object.keys(descriptors)) { - const descriptor = descriptors[name]; - if (typeof descriptor.get === 'function') { - output[name] = (data as any)[name]; + seen.set(data, path); + } + + depth++; + let output: any; + if (isObject(data)) { + output = {}; + // If the object has a `.toJSON()` function defined, then + // prefer it to any other type of serialization. + if (this.hasToJSONFunction(data)) { + output = data.toJSON(); + } else { + for (const key of Object.keys(data)) { + output[key] = this.ensureDataIsSerializable( + (data as any)[key], + depth, + seen, + path.concat(key), + ); + } + if (isClassInstance(data)) { + const descriptors = Object.getOwnPropertyDescriptors(Object.getPrototypeOf(data)); + for (const name of Object.keys(descriptors)) { + const descriptor = descriptors[name]; + if (typeof descriptor.get === 'function') { + output[name] = (data as any)[name]; + } } } } @@ -260,11 +288,15 @@ export class Job = any> { output = []; } data.forEach((item, i) => { - output[i] = this.ensureDataIsSerializable(item, depth); + output[i] = this.ensureDataIsSerializable(item, depth, seen, path.concat(i.toString())); }); } else { return data; } return output; } + + private hasToJSONFunction(obj: any): obj is { toJSON(): any } { + return typeof obj?.toJSON === 'function'; + } }