chore(product): Improve product normalization and fix http router with tracing (#11724)

**What**
- Improve product normalization and prevent over fetching data
- Fix HTTP router wrap handler with tracing enabled

.changeset/heavy-items-own.md

@@ -0,0 +1,7 @@
+---
+"@medusajs/product": patch
+"@medusajs/framework": patch
+"@medusajs/medusa": patch
+---
+
+chore(product): Improve product normalization

packages/core/framework/src/http/__fixtures__/routers/admin/fail/route.ts

@@ -0,0 +1,5 @@
+import { Request, Response } from "express"
+
+export function GET(req: Request, res: Response) {
+  throw new Error("Failed")
+}

packages/core/framework/src/http/__tests__/index.spec.ts

@@ -36,6 +36,22 @@ describe("RoutesLoader", function () {
       request = request_
     })
 
+    it("should be handled by the error handler when a route handler fails", async function () {
+      const res = await request("GET", "/admin/fail", {
+        adminSession: {
+          jwt: {
+            userId: "admin_user",
+          },
+        },
+      })
+
+      expect(res.status).toBe(500)
+      console.log(res)
+      expect(res.text).toBe(
+        '{"code":"unknown_error","type":"unknown_error","message":"An unknown error occurred."}'
+      )
+    })
+
     it("should return a status 200 on GET admin/order/:id", async function () {
       const res = await request("GET", "/admin/orders/1000", {
         adminSession: {

packages/core/framework/src/http/__tests__/routes-loader.spec.ts

@@ -9,6 +9,18 @@ describe("Routes loader", () => {
 
     expect(loader.getRoutes()).toMatchInlineSnapshot(`
       [
+        {
+          "absolutePath": "${BASE_DIR}/admin/fail/route.ts",
+          "handler": [Function],
+          "isRoute": true,
+          "matcher": "/admin/fail",
+          "method": "GET",
+          "optedOutOfAuth": false,
+          "relativePath": "/admin/fail/route.ts",
+          "shouldAppendAdminCors": true,
+          "shouldAppendAuthCors": false,
+          "shouldAppendStoreCors": false,
+        },
         {
           "absolutePath": "${BASE_DIR}/admin/orders/[id]/route.ts",
           "handler": [Function],
@@ -205,6 +217,18 @@ describe("Routes loader", () => {
 
     expect(loader.getRoutes()).toMatchInlineSnapshot(`
       [
+        {
+          "absolutePath": "${BASE_DIR}/admin/fail/route.ts",
+          "handler": [Function],
+          "isRoute": true,
+          "matcher": "/admin/fail",
+          "method": "GET",
+          "optedOutOfAuth": false,
+          "relativePath": "/admin/fail/route.ts",
+          "shouldAppendAdminCors": true,
+          "shouldAppendAuthCors": false,
+          "shouldAppendStoreCors": false,
+        },
         {
           "absolutePath": "${BASE_DIR}/admin/orders/[id]/route.ts",
           "handler": [Function],

packages/core/framework/src/http/router.ts

@@ -104,25 +104,25 @@ export class ApiLoader {
     if ("isRoute" in route) {
       logger.debug(`registering route ${route.method} ${route.matcher}`)
       const handler = ApiLoader.traceRoute
-        ? ApiLoader.traceRoute(wrapHandler(route.handler), {
+        ? ApiLoader.traceRoute(route.handler, {
             route: route.matcher,
             method: route.method,
           })
-        : wrapHandler(route.handler)
+        : route.handler
 
-      this.#app[route.method.toLowerCase()](route.matcher, handler)
+      this.#app[route.method.toLowerCase()](route.matcher, wrapHandler(handler))
       return
     }
 
     if (!route.methods) {
       logger.debug(`registering global middleware for ${route.matcher}`)
       const handler = ApiLoader.traceMiddleware
-        ? (ApiLoader.traceMiddleware(wrapHandler(route.handler), {
+        ? (ApiLoader.traceMiddleware(route.handler, {
             route: route.matcher,
           }) as RequestHandler)
-        : (wrapHandler(route.handler) as RequestHandler)
+        : (route.handler as RequestHandler)
 
-      this.#app.use(route.matcher, handler)
+      this.#app.use(route.matcher, wrapHandler(handler))
       return
     }
 

packages/medusa/package.json

@@ -60,6 +60,7 @@
     "@types/multer": "^1.4.7",
     "jest": "^29.7.0",
     "rimraf": "^5.0.1",
+    "supertest": "^4.0.2",
     "typescript": "^5.6.2",
     "yalc": "1.0.0-pre.53"
   },

packages/medusa/src/instrumentation/__fixtures__/mocks/index.ts

@@ -0,0 +1,20 @@
+import { ConfigModule } from "@medusajs/types"
+
+export const customersGlobalMiddlewareMock = jest.fn()
+export const customersCreateMiddlewareMock = jest.fn()
+export const storeGlobalMiddlewareMock = jest.fn()
+
+export const config = {
+  projectConfig: {
+    databaseLogging: false,
+    http: {
+      authCors: "http://localhost:9000",
+      storeCors: "http://localhost:8000",
+      adminCors: "http://localhost:7001",
+      jwtSecret: "supersecret",
+      cookieSecret: "superSecret",
+    },
+  },
+  featureFlags: {},
+  plugins: [],
+} satisfies Partial<ConfigModule>

packages/medusa/src/instrumentation/__fixtures__/routers/admin/fail/route.ts

@@ -0,0 +1,6 @@
+import { MedusaError } from "@medusajs/framework/utils"
+import { Request, Response } from "express"
+
+export function GET(req: Request, res: Response) {
+  throw new MedusaError(MedusaError.Types.INVALID_DATA, "Failed")
+}

packages/medusa/src/instrumentation/__fixtures__/routers/middlewares.ts

@@ -0,0 +1,15 @@
+import { defineMiddlewares } from "@medusajs/framework"
+
+export const errorHandlerMock = jest
+  .fn()
+  .mockImplementation((err, req, res, next) => {
+    console.log("errorHandlerMock", err)
+    return res.status(400).json({
+      type: err.code.toLowerCase(),
+      message: err.message,
+    })
+  })
+
+export default defineMiddlewares({
+  errorHandler: (err, req, res, next) => errorHandlerMock(err, req, res, next),
+})

packages/medusa/src/instrumentation/__fixtures__/server/index.ts

@@ -0,0 +1,183 @@
+import {
+  moduleLoader,
+  ModulesDefinition,
+  registerMedusaModule,
+} from "@medusajs/modules-sdk"
+import { ContainerRegistrationKeys, generateJwtToken } from "@medusajs/utils"
+import { asValue } from "awilix"
+import express from "express"
+import querystring from "querystring"
+import supertest from "supertest"
+
+import { config } from "../mocks"
+import { ConfigModule, MedusaContainer } from "@medusajs/types"
+import { configManager } from "@medusajs/framework/config"
+import {
+  ApiLoader,
+  container,
+  featureFlagsLoader,
+  logger,
+  MedusaRequest,
+} from "@medusajs/framework"
+
+function asArray(resolvers) {
+  return {
+    resolve: (container) =>
+      resolvers.map((resolver) => container.build(resolver)),
+  }
+}
+
+/**
+ * Sets up a test server that injects API Routes using the RoutesLoader
+ *
+ * @param {String} rootDir - The root directory of the project
+ */
+export const createServer = async (rootDir) => {
+  const app = express()
+
+  const moduleResolutions = {}
+  Object.entries(ModulesDefinition).forEach(([moduleKey, module]) => {
+    moduleResolutions[moduleKey] = registerMedusaModule(
+      moduleKey,
+      module.defaultModuleDeclaration,
+      undefined,
+      module
+    )[moduleKey]
+  })
+
+  configManager.loadConfig({
+    projectConfig: config as unknown as ConfigModule,
+    baseDir: rootDir,
+  })
+
+  container.registerAdd = function (this: MedusaContainer, name, registration) {
+    const storeKey = name + "_STORE"
+
+    if (this.registrations[storeKey] === undefined) {
+      this.register(storeKey, asValue([]))
+    }
+    const store = this.resolve(storeKey) as Array<any>
+
+    if (this.registrations[name] === undefined) {
+      this.register(name, asArray(store))
+    }
+    store.unshift(registration)
+
+    return this
+  }.bind(container)
+
+  container.register(ContainerRegistrationKeys.PG_CONNECTION, asValue({}))
+  container.register("configModule", asValue(config))
+  container.register({
+    logger: asValue({
+      error: () => {},
+    }),
+    manager: asValue({}),
+  })
+
+  app.set("trust proxy", 1)
+  app.use((req, _res, next) => {
+    req["session"] = {}
+    const data = req.get("Cookie")
+    if (data) {
+      req["session"] = {
+        ...req["session"],
+        ...JSON.parse(data),
+      }
+    }
+    next()
+  })
+
+  await featureFlagsLoader()
+  await moduleLoader({ container, moduleResolutions, logger })
+
+  app.use((req, res, next) => {
+    ;(req as MedusaRequest).scope = container.createScope() as MedusaContainer
+    next()
+  })
+
+  await new ApiLoader({
+    app,
+    sourceDir: rootDir,
+  }).load()
+
+  const superRequest = supertest(app)
+
+  return {
+    request: async (method, url, opts: any = {}) => {
+      const { payload, query, headers = {} } = opts
+
+      const queryParams = query && querystring.stringify(query)
+      const req = superRequest[method.toLowerCase()](
+        `${url}${queryParams ? "?" + queryParams : ""}`
+      )
+      headers.Cookie = headers.Cookie || ""
+      if (opts.adminSession) {
+        const token = generateJwtToken(
+          {
+            actor_id: opts.adminSession.userId || opts.adminSession.jwt?.userId,
+            actor_type: "user",
+            app_metadata: {
+              user_id:
+                opts.adminSession.userId || opts.adminSession.jwt?.userId,
+            },
+          },
+          {
+            secret: config.projectConfig.http.jwtSecret!,
+            expiresIn: "1d",
+          }
+        )
+
+        headers.Authorization = `Bearer ${token}`
+      }
+
+      if (opts.clientSession) {
+        const token = generateJwtToken(
+          {
+            actor_id:
+              opts.clientSession.customer_id ||
+              opts.clientSession.jwt?.customer_id,
+            actor_type: "customer",
+            app_metadata: {
+              customer_id:
+                opts.clientSession.customer_id ||
+                opts.clientSession.jwt?.customer_id,
+            },
+          },
+          { secret: config.projectConfig.http.jwtSecret!, expiresIn: "1d" }
+        )
+
+        headers.Authorization = `Bearer ${token}`
+      }
+
+      for (const name in headers) {
+        if ({}.hasOwnProperty.call(headers, name)) {
+          req.set(name, headers[name])
+        }
+      }
+
+      if (payload && !req.get("content-type")) {
+        req.set("Content-Type", "application/json")
+      }
+
+      if (!req.get("accept")) {
+        req.set("Accept", "application/json")
+      }
+
+      req.set("Host", "localhost")
+
+      let res
+      try {
+        res = await req.send(JSON.stringify(payload))
+      } catch (e) {
+        if (e.response) {
+          res = e.response
+        } else {
+          throw e
+        }
+      }
+
+      return res
+    },
+  }
+}

packages/medusa/src/instrumentation/__tests__/index.spec.ts

@@ -0,0 +1,50 @@
+import { resolve } from "path"
+import { errorHandlerMock } from "../__fixtures__/routers/middlewares"
+import { createServer } from "../__fixtures__/server"
+import { instrumentHttpLayer } from "../index"
+import { MedusaError } from "@medusajs/framework/utils"
+
+jest.setTimeout(30000)
+
+jest.mock("../../commands/start", () => {
+  return {}
+})
+
+describe("HTTP Instrumentation", () => {
+  let request
+
+  afterEach(function () {
+    jest.clearAllMocks()
+  })
+
+  beforeAll(async function () {
+    instrumentHttpLayer()
+
+    const rootDir = resolve(__dirname, "../__fixtures__/routers")
+
+    const { request: request_ } = await createServer(rootDir)
+
+    request = request_
+  })
+
+  describe("traceRoute", () => {
+    it("should be handled by the error handler when a route fails", async () => {
+      const res = await request("GET", "/admin/fail", {
+        adminSession: {
+          jwt: {
+            userId: "admin_user",
+          },
+        },
+      })
+
+      expect(res.status).toBe(400)
+      expect(errorHandlerMock).toHaveBeenCalled()
+      expect(errorHandlerMock).toHaveBeenCalledWith(
+        new MedusaError(MedusaError.Types.INVALID_DATA, "Failed"),
+        expect.anything(),
+        expect.anything(),
+        expect.anything()
+      )
+    })
+  })
+})

packages/medusa/src/instrumentation/index.ts

@@ -110,26 +110,18 @@ export function instrumentHttpLayer() {
       }`
 
       await HTTPTracer.trace(traceName, async (span) => {
-        return new Promise<void>((resolve, reject) => {
+        try {
-          const _next = (error?: any) => {
+          await handler(req, res, next)
-            if (error) {
+        } catch (error) {
-              span.setStatus({
+          span.setStatus({
-                code: SpanStatusCode.ERROR,
+            code: SpanStatusCode.ERROR,
-                message: error.message || "Failed",
+            message: error.message || "Failed",
-              })
+          })
-              span.end()
+          throw error
-              reject(error)
+        } finally {
-            } else {
+          span.end()
-              span.end()
+        }
-              resolve()
-            }
-          }
-
-          handler(req, res, _next)
-        })
       })
-        .catch(next)
-        .then(next)
     }
   }
 }

packages/modules/product/integration-tests/__tests__/product-module-service/products.spec.ts

@@ -723,10 +723,8 @@ moduleIntegrationTestRunner<IProductModuleService>({
         })
 
         it("should throw because variant doesn't have all options set", async () => {
-          let error
+          const error = await service
-
+            .createProducts([
-          try {
-            await service.createProducts([
               {
                 title: "Product with variants and options",
                 options: [
@@ -741,9 +739,7 @@ moduleIntegrationTestRunner<IProductModuleService>({
                 ],
               },
             ])
-          } catch (e) {
+            .catch((e) => e)
-            error = e
-          }
 
           expect(error.message).toEqual(
             `Product "Product with variants and options" has variants with missing options: [missing option]`

packages/modules/product/src/services/product-module-service.ts

@@ -1558,18 +1558,16 @@ export default class ProductModuleService
     data: ProductTypes.CreateProductDTO[],
     @MedusaContext() sharedContext: Context = {}
   ): Promise<InferEntityType<typeof Product>[]> {
-    const normalizedInput = await promiseAll(
+    const normalizedProducts = await this.normalizeCreateProductInput(
-      data.map(async (d) => {
+      data,
-        const normalized = await this.normalizeCreateProductInput(
+      sharedContext
-          d,
-          sharedContext
-        )
-        this.validateProductCreatePayload(normalized)
-        return normalized
-      })
     )
 
-    const tagIds = normalizedInput
+    for (const product of normalizedProducts) {
+      this.validateProductCreatePayload(product)
+    }
+
+    const tagIds = normalizedProducts
       .flatMap((d) => (d as any).tags ?? [])
       .map((t) => t.id)
     let existingTags: InferEntityType<typeof ProductTag>[] = []
@@ -1586,7 +1584,7 @@ export default class ProductModuleService
 
     const existingTagsMap = new Map(existingTags.map((tag) => [tag.id, tag]))
 
-    const productsToCreate = normalizedInput.map((product) => {
+    const productsToCreate = normalizedProducts.map((product) => {
       const productId = generateEntityId(product.id, "prod")
       product.id = productId
 
@@ -1652,20 +1650,18 @@ export default class ProductModuleService
     data: UpdateProductInput[],
     @MedusaContext() sharedContext: Context = {}
   ): Promise<InferEntityType<typeof Product>[]> {
-    const normalizedInput = await promiseAll(
+    const normalizedProducts = await this.normalizeUpdateProductInput(
-      data.map(async (d) => {
+      data,
-        const normalized = await this.normalizeUpdateProductInput(
+      sharedContext
-          d,
-          sharedContext
-        )
-        this.validateProductUpdatePayload(normalized)
-        return normalized
-      })
     )
 
+    for (const product of normalizedProducts) {
+      this.validateProductUpdatePayload(product)
+    }
+
     const { entities: productData } =
       await this.productService_.upsertWithReplace(
-        normalizedInput,
+        normalizedProducts,
         {
           relations: ["tags", "categories"],
         },
@@ -1675,7 +1671,7 @@ export default class ProductModuleService
     // There is more than 1-level depth of relations here, so we need to handle the options and variants manually
     await promiseAll(
       // Note: It's safe to rely on the order here as `upsertWithReplace` preserves the order of the input
-      normalizedInput.map(async (product, i) => {
+      normalizedProducts.map(async (product, i) => {
         const upsertedProduct: any = productData[i]
         let allOptions: any[] = []
 
@@ -1903,91 +1899,125 @@ export default class ProductModuleService
     this.validateProductPayload(productData)
   }
 
-  protected async normalizeCreateProductInput(
+  protected async normalizeCreateProductInput<
-    product: ProductTypes.CreateProductDTO,
+    T extends ProductTypes.CreateProductDTO | ProductTypes.CreateProductDTO[],
+    TOutput = T extends ProductTypes.CreateProductDTO[]
+      ? ProductTypes.CreateProductDTO[]
+      : ProductTypes.CreateProductDTO
+  >(
+    products: T,
     @MedusaContext() sharedContext: Context = {}
-  ): Promise<ProductTypes.CreateProductDTO> {
+  ): Promise<TOutput> {
-    const productData = (await this.normalizeUpdateProductInput(
+    const products_ = Array.isArray(products) ? products : [products]
-      product as UpdateProductInput,
+
+    const normalizedProducts = (await this.normalizeUpdateProductInput(
+      products_ as UpdateProductInput[],
       sharedContext
-    )) as ProductTypes.CreateProductDTO
+    )) as ProductTypes.CreateProductDTO[]
 
-    if (!productData.handle && productData.title) {
+    for (const productData of normalizedProducts) {
-      productData.handle = toHandle(productData.title)
+      if (!productData.handle && productData.title) {
+        productData.handle = toHandle(productData.title)
+      }
+
+      if (!productData.status) {
+        productData.status = ProductStatus.DRAFT
+      }
+
+      if (!productData.thumbnail && productData.images?.length) {
+        productData.thumbnail = productData.images[0].url
+      }
+
+      if (productData.images?.length) {
+        productData.images = productData.images.map((image, index) =>
+          (image as { rank?: number }).rank != null
+            ? image
+            : {
+                ...image,
+                rank: index,
+              }
+        )
+      }
     }
 
-    if (!productData.status) {
+    return (
-      productData.status = ProductStatus.DRAFT
+      Array.isArray(products) ? normalizedProducts : normalizedProducts[0]
-    }
+    ) as TOutput
-
-    if (!productData.thumbnail && productData.images?.length) {
-      productData.thumbnail = productData.images[0].url
-    }
-
-    if (productData.images?.length) {
-      productData.images = productData.images.map((image, index) =>
-        (image as { rank?: number }).rank != null
-          ? image
-          : {
-              ...image,
-              rank: index,
-            }
-      )
-    }
-
-    return productData
   }
 
-  protected async normalizeUpdateProductInput(
+  protected async normalizeUpdateProductInput<
-    product: UpdateProductInput,
+    T extends UpdateProductInput | UpdateProductInput[],
+    TOutput = T extends UpdateProductInput[]
+      ? UpdateProductInput[]
+      : UpdateProductInput
+  >(
+    products: T,
     @MedusaContext() sharedContext: Context = {}
-  ): Promise<UpdateProductInput> {
+  ): Promise<TOutput> {
-    const productData = { ...product }
+    const products_ = Array.isArray(products) ? products : [products]
-    if (productData.is_giftcard) {
+    const productsIds = products_.map((p) => p.id).filter(Boolean)
-      productData.discountable = false
-    }
 
-    if (productData.options?.length) {
+    let dbOptions: InferEntityType<typeof ProductOption>[] = []
-      // TODO: Instead of fetching per product, this should fetch for all product allowing for only one query instead of X
+
-      const dbOptions = await this.productOptionService_.list(
+    if (productsIds.length) {
-        { product_id: productData.id },
+      dbOptions = await this.productOptionService_.list(
+        { product_id: productsIds },
         { relations: ["values"] },
         sharedContext
       )
-
-      ;(productData as any).options = productData.options?.map((option) => {
-        const dbOption = dbOptions.find((o) => o.title === option.title)
-        return {
-          title: option.title,
-          values: option.values?.map((value) => {
-            const dbValue = dbOption?.values?.find((val) => val.value === value)
-            return {
-              value: value,
-              ...(dbValue ? { id: dbValue.id } : {}),
-            }
-          }),
-          ...(dbOption ? { id: dbOption.id } : {}),
-        }
-      })
     }
 
-    if (productData.tag_ids) {
+    const normalizedProducts: UpdateProductInput[] = []
-      ;(productData as any).tags = productData.tag_ids.map((cid) => ({
-        id: cid,
-      }))
-      delete productData.tag_ids
-    }
 
-    if (productData.category_ids) {
+    for (const product of products_) {
-      ;(productData as any).categories = productData.category_ids.map(
+      const productData = { ...product }
-        (cid) => ({
+      if (productData.is_giftcard) {
-          id: cid,
+        productData.discountable = false
+      }
+
+      if (productData.options?.length) {
+        ;(productData as any).options = productData.options?.map((option) => {
+          const dbOption = dbOptions.find(
+            (o) => o.title === option.title && o.product_id === productData.id
+          )
+          return {
+            title: option.title,
+            values: option.values?.map((value) => {
+              const dbValue = dbOption?.values?.find(
+                (val) => val.value === value
+              )
+              return {
+                value: value,
+                ...(dbValue ? { id: dbValue.id } : {}),
+              }
+            }),
+            ...(dbOption ? { id: dbOption.id } : {}),
+          }
         })
-      )
+      }
-      delete productData.category_ids
+
+      if (productData.tag_ids) {
+        ;(productData as any).tags = productData.tag_ids.map((cid) => ({
+          id: cid,
+        }))
+        delete productData.tag_ids
+      }
+
+      if (productData.category_ids) {
+        ;(productData as any).categories = productData.category_ids.map(
+          (cid) => ({
+            id: cid,
+          })
+        )
+        delete productData.category_ids
+      }
+
+      normalizedProducts.push(productData)
     }
 
-    return productData
+    return (
+      Array.isArray(products) ? normalizedProducts : normalizedProducts[0]
+    ) as TOutput
   }
 
   protected static normalizeCreateProductCollectionInput(

yarn.lock

@@ -6155,6 +6155,7 @@ __metadata:
     request-ip: ^3.3.0
     rimraf: ^5.0.1
     slugify: ^1.6.6
+    supertest: ^4.0.2
     typescript: ^5.6.2
     uuid: ^9.0.0
     yalc: 1.0.0-pre.53