From 225d00cd09e40012ba7c8f6e5ab5e7524e2871ec Mon Sep 17 00:00:00 2001 From: Adrien de Peretti Date: Thu, 3 Oct 2024 08:22:11 +0200 Subject: [PATCH] chore: improve mikro orm serializer circular ref and link serialization (#9411) --- .../__tests__/product/admin/product.spec.ts | 18 --- .../product/helpers/normalize-for-export.ts | 14 +- .../src/product/steps/get-all-products.ts | 1 + .../src/dal/mikro-orm/mikro-orm-serializer.ts | 151 ++++++++++++------ .../integration-tests/__tests__/enum.spec.ts | 2 +- .../__tests__/has-one-belongs-to.spec.ts | 4 +- .../__tests__/many-to-many.spec.ts | 8 +- .../__tests__/many-to-one.spec.ts | 4 +- packages/medusa/src/commands/start.ts | 26 +-- .../src/services/dynamic-service-class.ts | 2 +- .../src/services/link-module-service.ts | 12 +- .../services/payment-module/index.spec.ts | 5 +- 12 files changed, 142 insertions(+), 105 deletions(-) diff --git a/integration-tests/http/__tests__/product/admin/product.spec.ts b/integration-tests/http/__tests__/product/admin/product.spec.ts index 587f8f40ff..31a856d9c7 100644 --- a/integration-tests/http/__tests__/product/admin/product.spec.ts +++ b/integration-tests/http/__tests__/product/admin/product.spec.ts @@ -1277,16 +1277,10 @@ medusaIntegrationTestRunner({ expect.objectContaining({ id: expect.stringMatching(/^optval_*/), value: "large", - option: expect.objectContaining({ - title: "size", - }), }), expect.objectContaining({ id: expect.stringMatching(/^optval_*/), value: "green", - option: expect.objectContaining({ - title: "color", - }), }), ]), }), @@ -1557,9 +1551,6 @@ medusaIntegrationTestRunner({ expect.objectContaining({ id: expect.stringMatching(/^optval_*/), value: "large", - option: expect.objectContaining({ - title: "size", - }), }), ]), origin_country: null, @@ -2660,9 +2651,6 @@ medusaIntegrationTestRunner({ updatedProduct.variants.find((v) => v.id === baseVariant.id).options ).toEqual([ expect.objectContaining({ - option: expect.objectContaining({ - title: "size", - }), value: "small", }), ]) @@ -2692,15 +2680,9 @@ medusaIntegrationTestRunner({ expect(updatedOptions).toEqual( expect.arrayContaining([ expect.objectContaining({ - option: expect.objectContaining({ - title: "size", - }), value: "small", }), expect.objectContaining({ - option: expect.objectContaining({ - title: "color", - }), value: "green", }), ]) diff --git a/packages/core/core-flows/src/product/helpers/normalize-for-export.ts b/packages/core/core-flows/src/product/helpers/normalize-for-export.ts index 8547e8aa44..dad6d716ef 100644 --- a/packages/core/core-flows/src/product/helpers/normalize-for-export.ts +++ b/packages/core/core-flows/src/product/helpers/normalize-for-export.ts @@ -1,8 +1,8 @@ import { - RegionTypes, BigNumberInput, HttpTypes, PricingTypes, + RegionTypes, } from "@medusajs/framework/types" import { MedusaError, upperCaseFirst } from "@medusajs/framework/utils" @@ -24,7 +24,7 @@ export const normalizeForExport = ( variants.forEach((v) => { const toPush = { ...normalizeProductForExport(product), - ...normalizeVariantForExport(v, regionsMap), + ...normalizeVariantForExport(v, regionsMap, product), } as any delete toPush["Product Variants"] @@ -101,7 +101,8 @@ const normalizeVariantForExport = ( variant: HttpTypes.AdminProductVariant & { price_set?: PricingTypes.PriceSetDTO }, - regionsMap: Map + regionsMap: Map, + product: HttpTypes.AdminProduct ): object => { const flattenedPrices = variant.price_set?.prices ?.sort((a, b) => b.currency_code!.localeCompare(a.currency_code!)) @@ -133,9 +134,14 @@ const normalizeVariantForExport = ( return acc }, {}) + const options = product.options ?? [] + const flattenedOptions = variant.options?.reduce( (acc: Record, option, idx) => { - acc[beautifyKey(`variant_option_${idx + 1}_name`)] = option.option?.title! + const prodOptions = options.find( + (prodOption) => prodOption.id === option.option_id + ) + acc[beautifyKey(`variant_option_${idx + 1}_name`)] = prodOptions?.title! acc[beautifyKey(`variant_option_${idx + 1}_value`)] = option.value return acc }, diff --git a/packages/core/core-flows/src/product/steps/get-all-products.ts b/packages/core/core-flows/src/product/steps/get-all-products.ts index 5e87db1508..855150d6a7 100644 --- a/packages/core/core-flows/src/product/steps/get-all-products.ts +++ b/packages/core/core-flows/src/product/steps/get-all-products.ts @@ -36,6 +36,7 @@ export const getAllProductsStep = createStep( }, fields: data.select, }) + allProducts.push(...products) if (products.length < pageSize) { diff --git a/packages/core/utils/src/dal/mikro-orm/mikro-orm-serializer.ts b/packages/core/utils/src/dal/mikro-orm/mikro-orm-serializer.ts index e59c3983ed..4581543a61 100644 --- a/packages/core/utils/src/dal/mikro-orm/mikro-orm-serializer.ts +++ b/packages/core/utils/src/dal/mikro-orm/mikro-orm-serializer.ts @@ -8,8 +8,8 @@ import { Platform, Reference, ReferenceType, - SerializeOptions, SerializationContext, + SerializeOptions, Utils, } from "@mikro-orm/core" @@ -64,20 +64,26 @@ function isPopulated( } /** - * Customer property filtering for the serialization which takes into account the parent entity to filter out circular references if configured for. + * Custom property filtering for the serialization which takes into account circular references to not return them. * @param propName * @param meta * @param options - * @param parent + * @param parents */ -function filterEntityPropToSerialize( - propName: string, - meta: EntityMetadata, +function filterEntityPropToSerialize({ + propName, + meta, + options, + parents, +}: { + propName: string + meta: EntityMetadata options: SerializeOptions & { preventCircularRef?: boolean - } = {}, - parent?: object -): boolean { + } + parents?: string[] +}): boolean { + parents ??= [] const isVisibleRes = isVisible(meta, propName, options) const prop = meta.properties[propName] @@ -86,12 +92,16 @@ function filterEntityPropToSerialize( prop && options.preventCircularRef && isVisibleRes && - parent && prop.reference !== ReferenceType.SCALAR ) { // mapToPk would represent a foreign key and we want to keep them - return !!prop.mapToPk || parent.constructor.name !== prop.type + if (!!prop.mapToPk) { + return true + } + + return !parents.some((parent) => parent === prop.type) } + return isVisibleRes } @@ -99,8 +109,10 @@ export class EntitySerializer { static serialize( entity: T, options: SerializeOptions & { preventCircularRef?: boolean } = {}, - parent?: object + parents: string[] = [] ): EntityDTO> { + const parents_ = Array.from(new Set(parents)) + const wrapped = helper(entity) const meta = wrapped.__meta let contextCreated = false @@ -116,20 +128,39 @@ export class EntitySerializer { contextCreated = true } - const root = wrapped.__serializationContext.root! + const root = wrapped.__serializationContext + .root! as SerializationContext & { + visitedSerialized?: Map + } + const ret = {} as EntityDTO> const keys = new Set(meta.primaryKeys) Object.keys(entity).forEach((prop) => keys.add(prop)) - const visited = root.visited.has(entity) + const visited = root.visited.has(entity) if (!visited) { root.visited.add(entity) } + // Virtually augment the serialization context + root.visitedSerialized ??= new Map() + const primaryKeysValues = Array.from(keys) + .map((key) => entity[key]) + .join("-") + + if (root.visitedSerialized.has(primaryKeysValues)) { + return root.visitedSerialized.get(primaryKeysValues) + } + ;[...keys] /** Medusa Custom properties filtering **/ .filter((prop) => - filterEntityPropToSerialize(prop, meta, options, parent) + filterEntityPropToSerialize({ + propName: prop, + meta, + options, + parents: parents_, + }) ) .map((prop) => { const cycle = root.visit(meta.className, prop) @@ -141,7 +172,8 @@ export class EntitySerializer { const val = this.processProperty( prop as keyof T & string, entity, - options + options, + parents_ ) if (!cycle) { @@ -189,7 +221,7 @@ export class EntitySerializer { .forEach( (prop) => (ret[this.propertyName(meta, prop.name, wrapped.__platform)] = - this.processProperty(prop.name, entity, options)) + this.processProperty(prop.name, entity, options, parents_)) ) // decorated get methods @@ -206,10 +238,11 @@ export class EntitySerializer { this.processProperty( prop.getterName as keyof T & string, entity, - options + options, + parents_ )) ) - + root.visitedSerialized.set(primaryKeysValues, ret) return ret } @@ -233,8 +266,11 @@ export class EntitySerializer { private static processProperty( prop: keyof T & string, entity: T, - options: SerializeOptions + options: SerializeOptions, + parents: string[] = [] ): T[keyof T] | undefined { + const parents_ = [...parents, entity.constructor.name] + const parts = prop.split(".") prop = parts[0] as string & keyof T const wrapped = helper(entity) @@ -258,11 +294,17 @@ export class EntitySerializer { } if (Utils.isCollection(entity[prop])) { - return this.processCollection(prop, entity, options) + return this.processCollection(prop, entity, options, parents_) } if (Utils.isEntity(entity[prop], true)) { - return this.processEntity(prop, entity, wrapped.__platform, options) + return this.processEntity( + prop, + entity, + wrapped.__platform, + options, + parents_ + ) } /* istanbul ignore next */ @@ -314,8 +356,11 @@ export class EntitySerializer { prop: keyof T & string, entity: T, platform: Platform, - options: SerializeOptions + options: SerializeOptions, + parents: string[] = [] ): T[keyof T] | undefined { + const parents_ = [...parents, entity.constructor.name] + const child = Reference.unwrapReference(entity[prop] as T) const wrapped = helper(child) const populated = @@ -326,8 +371,7 @@ export class EntitySerializer { return this.serialize( child, this.extractChildOptions(options, prop), - /** passing the entity as the parent for circular filtering **/ - entity + parents_ ) as T[keyof T] } @@ -339,8 +383,10 @@ export class EntitySerializer { private static processCollection( prop: keyof T & string, entity: T, - options: SerializeOptions + options: SerializeOptions, + parents: string[] = [] ): T[keyof T] | undefined { + const parents_ = [...parents, entity.constructor.name] const col = entity[prop] as unknown as Collection if (!col.isInitialized()) { @@ -352,8 +398,7 @@ export class EntitySerializer { return this.serialize( item, this.extractChildOptions(options, prop), - /** passing the entity as the parent for circular filtering **/ - entity + parents_ ) } @@ -367,34 +412,36 @@ export const mikroOrmSerializer = ( options?: Parameters[1] & { preventCircularRef?: boolean } -): TOutput => { - options ??= {} +): Promise => { + return new Promise((resolve) => { + options ??= {} - const data_ = (Array.isArray(data) ? data : [data]).filter(Boolean) + const data_ = (Array.isArray(data) ? data : [data]).filter(Boolean) - const forSerialization: unknown[] = [] - const notForSerialization: unknown[] = [] + const forSerialization: unknown[] = [] + const notForSerialization: unknown[] = [] - data_.forEach((object) => { - if (object.__meta) { - return forSerialization.push(object) + data_.forEach((object) => { + if (object.__meta) { + return forSerialization.push(object) + } + + return notForSerialization.push(object) + }) + + let result: any = forSerialization.map((entity) => + EntitySerializer.serialize(entity, { + forceObject: true, + populate: true, + preventCircularRef: true, + ...options, + } as SerializeOptions) + ) as TOutput[] + + if (notForSerialization.length) { + result = result.concat(notForSerialization) } - return notForSerialization.push(object) + resolve(Array.isArray(data) ? result : result[0]) }) - - let result: any = forSerialization.map((entity) => - EntitySerializer.serialize(entity, { - forceObject: true, - populate: true, - preventCircularRef: true, - ...options, - } as SerializeOptions) - ) as TOutput[] - - if (notForSerialization.length) { - result = result.concat(notForSerialization) - } - - return Array.isArray(data) ? result : result[0] } diff --git a/packages/core/utils/src/dml/integration-tests/__tests__/enum.spec.ts b/packages/core/utils/src/dml/integration-tests/__tests__/enum.spec.ts index 5ce2b0dab1..dd36df9def 100644 --- a/packages/core/utils/src/dml/integration-tests/__tests__/enum.spec.ts +++ b/packages/core/utils/src/dml/integration-tests/__tests__/enum.spec.ts @@ -82,7 +82,7 @@ describe("EntityBuilder | enum", () => { id: user1.id, }) - expect(mikroOrmSerializer>(user)).toEqual({ + expect(await mikroOrmSerializer>(user)).toEqual({ id: user1.id, username: "User 1", role: "admin", diff --git a/packages/core/utils/src/dml/integration-tests/__tests__/has-one-belongs-to.spec.ts b/packages/core/utils/src/dml/integration-tests/__tests__/has-one-belongs-to.spec.ts index 4e9682c474..17a8ea6b2f 100644 --- a/packages/core/utils/src/dml/integration-tests/__tests__/has-one-belongs-to.spec.ts +++ b/packages/core/utils/src/dml/integration-tests/__tests__/has-one-belongs-to.spec.ts @@ -92,7 +92,7 @@ describe("hasOne - belongTo", () => { } ) - expect(mikroOrmSerializer>(user)).toEqual({ + expect(await mikroOrmSerializer>(user)).toEqual({ id: user1.id, username: "User 1", created_at: expect.any(Date), @@ -137,7 +137,7 @@ describe("hasOne - belongTo", () => { } ) - expect(mikroOrmSerializer>(user)).toEqual({ + expect(await mikroOrmSerializer>(user)).toEqual({ id: user1.id, username: "User 1", created_at: expect.any(Date), diff --git a/packages/core/utils/src/dml/integration-tests/__tests__/many-to-many.spec.ts b/packages/core/utils/src/dml/integration-tests/__tests__/many-to-many.spec.ts index 4d65a4eef4..36e47099c5 100644 --- a/packages/core/utils/src/dml/integration-tests/__tests__/many-to-many.spec.ts +++ b/packages/core/utils/src/dml/integration-tests/__tests__/many-to-many.spec.ts @@ -129,7 +129,9 @@ describe("manyToMany - manyToMany", () => { } ) - const serializedSquad = mikroOrmSerializer>(team) + const serializedSquad = await mikroOrmSerializer>( + team + ) expect(serializedSquad.users).toHaveLength(2) expect(serializedSquad).toEqual({ @@ -166,7 +168,9 @@ describe("manyToMany - manyToMany", () => { } ) - const serializedUser = mikroOrmSerializer>(user) + const serializedUser = await mikroOrmSerializer>( + user + ) expect(serializedUser.squads).toHaveLength(1) expect(serializedUser).toEqual({ diff --git a/packages/core/utils/src/dml/integration-tests/__tests__/many-to-one.spec.ts b/packages/core/utils/src/dml/integration-tests/__tests__/many-to-one.spec.ts index ec0ab4b329..d668a2ad76 100644 --- a/packages/core/utils/src/dml/integration-tests/__tests__/many-to-one.spec.ts +++ b/packages/core/utils/src/dml/integration-tests/__tests__/many-to-one.spec.ts @@ -104,7 +104,7 @@ describe("manyToOne - belongTo", () => { } ) - expect(mikroOrmSerializer>(team)).toEqual({ + expect(await mikroOrmSerializer>(team)).toEqual({ id: team1.id, name: "Team 1", created_at: expect.any(Date), @@ -130,7 +130,7 @@ describe("manyToOne - belongTo", () => { } ) - expect(mikroOrmSerializer>(user)).toEqual({ + expect(await mikroOrmSerializer>(user)).toEqual({ id: user1.id, username: "User 1", created_at: expect.any(Date), diff --git a/packages/medusa/src/commands/start.ts b/packages/medusa/src/commands/start.ts index 219b45a1be..59a74ed759 100644 --- a/packages/medusa/src/commands/start.ts +++ b/packages/medusa/src/commands/start.ts @@ -60,20 +60,20 @@ async function start({ port, directory, types }) { const app = express() const http_ = http.createServer(async (req, res) => { - if (traceRequestHandler) { - await traceRequestHandler( - async () => { - return new Promise((resolve) => { - res.on("finish", resolve) + await new Promise((resolve) => { + res.on("finish", resolve) + if (traceRequestHandler) { + void traceRequestHandler( + async () => { app(req, res) - }) - }, - req, - res - ) - } else { - app(req, res) - } + }, + req, + res + ) + } else { + app(req, res) + } + }) }) try { diff --git a/packages/modules/link-modules/src/services/dynamic-service-class.ts b/packages/modules/link-modules/src/services/dynamic-service-class.ts index 3766fb33a0..3785360f02 100644 --- a/packages/modules/link-modules/src/services/dynamic-service-class.ts +++ b/packages/modules/link-modules/src/services/dynamic-service-class.ts @@ -22,7 +22,7 @@ export function getModuleService( ) } - return class LinkService extends LinkModuleService { + return class LinkService extends LinkModuleService { override __joinerConfig(): ModuleJoinerConfig { return joinerConfig_ as ModuleJoinerConfig } diff --git a/packages/modules/link-modules/src/services/link-module-service.ts b/packages/modules/link-modules/src/services/link-module-service.ts index 84860bf38a..c716b5d432 100644 --- a/packages/modules/link-modules/src/services/link-module-service.ts +++ b/packages/modules/link-modules/src/services/link-module-service.ts @@ -35,9 +35,9 @@ type InjectedDependencies = { [Modules.EVENT_BUS]?: IEventBusModuleService } -export default class LinkModuleService implements ILinkModule { +export default class LinkModuleService implements ILinkModule { protected baseRepository_: DAL.RepositoryService - protected readonly linkService_: LinkService + protected readonly linkService_: LinkService protected readonly eventBusModuleService_?: IEventBusModuleService protected readonly entityName_: string protected readonly serviceName_: string @@ -151,7 +151,7 @@ export default class LinkModuleService implements ILinkModule { const rows = await this.linkService_.list(filters, config, sharedContext) - return await this.baseRepository_.serialize(rows) + return rows.map((row) => row.toJSON()) } @InjectManager() @@ -170,7 +170,7 @@ export default class LinkModuleService implements ILinkModule { sharedContext ) - return [await this.baseRepository_.serialize(rows), count] + return [rows.map((row) => row.toJSON()), count] } @InjectTransactionManager(shouldForceTransaction, "baseRepository_") @@ -219,7 +219,7 @@ export default class LinkModuleService implements ILinkModule { })) ) - return await this.baseRepository_.serialize(links) + return links.map((row) => row.toJSON()) } @InjectTransactionManager(shouldForceTransaction, "baseRepository_") @@ -244,7 +244,7 @@ export default class LinkModuleService implements ILinkModule { const links = await this.linkService_.dismiss(data, sharedContext) - return await this.baseRepository_.serialize(links) + return links.map((row) => row.toJSON()) } @InjectTransactionManager(shouldForceTransaction, "baseRepository_") diff --git a/packages/modules/payment/integration-tests/__tests__/services/payment-module/index.spec.ts b/packages/modules/payment/integration-tests/__tests__/services/payment-module/index.spec.ts index d3e9338715..d0e22efc8a 100644 --- a/packages/modules/payment/integration-tests/__tests__/services/payment-module/index.spec.ts +++ b/packages/modules/payment/integration-tests/__tests__/services/payment-module/index.spec.ts @@ -4,8 +4,8 @@ import { PaymentModuleService } from "@services" import { moduleIntegrationTestRunner } from "medusa-test-utils" import { createPaymentCollections, - createPaymentSessions, createPayments, + createPaymentSessions, } from "../../../__fixtures__" jest.setTimeout(30000) @@ -519,9 +519,6 @@ moduleIntegrationTestRunner({ data: {}, status: "authorized", authorized_at: expect.any(Date), - payment_collection: expect.objectContaining({ - id: expect.any(String), - }), payment_collection_id: expect.any(String), }), })