From 9dcdc0041a2b08cc0723343dd8d9127d9977b086 Mon Sep 17 00:00:00 2001 From: Adrien de Peretti Date: Thu, 29 Jun 2023 15:26:41 +0200 Subject: [PATCH] fix(medusa, utils): fix the way selects are consumed alongside the relations (#4389) **What** There is actually an issue with using the `fields` query params with the way the repositories are using our custom query strategy. This pr aims to fix this issue by reworking the strategy. What we had to do was to rework the way the selects are built for each subquery in order to follow the aliasing convention and to be taken into consideration. Alongside these changes, the join used to always select everything, this needed to be changed so that if there are any selects provided for a join, the join should not select everything and let the query select the fields that are requested. Another notable change is that all the repositories are now using the repository util in order to centralize the customization and to have a single place to update when this kind of issue arises. This means that the eager relations when using the query builder are not necessarily taken into account. For that reason, I have removed the `shipping_option` eager option in favor of explicitly asking for the relations like we started to do it in some places. FIXES CORE-1413 --- .changeset/great-panthers-call.md | 12 ++ .../api/__tests__/batch-jobs/order/export.js | 2 - .../__tests__/batch-jobs/product/export.js | 5 +- .../api/__tests__/database/index.js | 2 +- .../api/__tests__/store/customer.js | 23 ++-- .../api/__tests__/store/products.js | 33 +++-- .../__tests__/inventory/order/order.js | 1 - .../utils/use-admin-expand-paramter.ts | 1 + .../src/services/brightpearl.js | 10 +- .../src/subscribers/order.js | 2 + .../src/subscribers/order.js | 6 + .../src/services/sendgrid.js | 15 +-- .../src/services/slack.js | 2 + .../routes/admin/swaps/__tests__/get-swap.js | 1 + .../src/api/routes/admin/swaps/index.ts | 1 + .../src/api/routes/store/orders/index.ts | 1 + .../src/api/routes/store/products/index.ts | 2 - .../src/api/routes/store/swaps/index.ts | 2 + packages/medusa/src/models/shipping-method.ts | 2 +- packages/medusa/src/repositories/cart.ts | 40 +++--- .../medusa/src/repositories/customer-group.ts | 8 +- packages/medusa/src/repositories/order.ts | 23 ++-- .../src/repositories/product-category.ts | 9 +- packages/medusa/src/repositories/product.ts | 33 ++--- packages/medusa/src/repositories/tax-rate.ts | 3 +- .../medusa/src/services/__tests__/cart.js | 4 +- .../medusa/src/services/__tests__/swap.ts | 71 +++++----- packages/medusa/src/services/cart.ts | 14 +- packages/medusa/src/services/claim.ts | 1 + packages/medusa/src/services/order-edit.ts | 1 + packages/medusa/src/services/return.ts | 1 + packages/medusa/src/services/swap.ts | 4 +- .../medusa/src/strategies/cart-completion.ts | 1 + packages/medusa/src/types/orders.ts | 5 + packages/medusa/src/utils/repository.ts | 125 +++++++++++------- .../src/common/__tests__/build-query.spec.ts | 91 +++++++++++++ .../__tests__/object-to-string-path.spec.ts | 42 ++++++ .../utils/src/common/object-to-string-path.ts | 51 +++++-- 38 files changed, 443 insertions(+), 207 deletions(-) create mode 100644 .changeset/great-panthers-call.md create mode 100644 packages/utils/src/common/__tests__/build-query.spec.ts create mode 100644 packages/utils/src/common/__tests__/object-to-string-path.spec.ts diff --git a/.changeset/great-panthers-call.md b/.changeset/great-panthers-call.md new file mode 100644 index 0000000000..66ab384b1f --- /dev/null +++ b/.changeset/great-panthers-call.md @@ -0,0 +1,12 @@ +--- +"@medusajs/medusa": patch +"@medusajs/admin-ui": patch +"@medusajs/admin": patch +"medusa-plugin-brightpearl": patch +"medusa-plugin-segment": patch +"medusa-plugin-sendgrid": patch +"medusa-plugin-slack-notification": patch +"@medusajs/utils": patch +--- + +fix(medusa, utils): fix the way selects are consumed alongside the relations diff --git a/integration-tests/api/__tests__/batch-jobs/order/export.js b/integration-tests/api/__tests__/batch-jobs/order/export.js index c52282ba71..63be7859f6 100644 --- a/integration-tests/api/__tests__/batch-jobs/order/export.js +++ b/integration-tests/api/__tests__/batch-jobs/order/export.js @@ -109,8 +109,6 @@ describe("Batchjob with type order-export", () => { expect(batchJob.status).toBe("completed") - expect(batchJob.status).toBe("completed") - exportFilePath = path.resolve(__dirname, batchJob.result.file_key) const isFileExists = (await fs.stat(exportFilePath)).isFile() diff --git a/integration-tests/api/__tests__/batch-jobs/product/export.js b/integration-tests/api/__tests__/batch-jobs/product/export.js index defbed9b1f..69255c34d2 100644 --- a/integration-tests/api/__tests__/batch-jobs/product/export.js +++ b/integration-tests/api/__tests__/batch-jobs/product/export.js @@ -60,6 +60,7 @@ describe("Batch job of product-export type", () => { const db = useDb() await db.teardown() + // @ts-ignore try { const isFileExists = (await fs.stat(exportFilePath))?.isFile() @@ -74,8 +75,8 @@ describe("Batch job of product-export type", () => { await fs.unlink(exportFilePath) } - } catch (e) { - console.log(e) + } catch (err) { + // noop } }) diff --git a/integration-tests/api/__tests__/database/index.js b/integration-tests/api/__tests__/database/index.js index 7a86d6f6b7..27d3ae4b19 100644 --- a/integration-tests/api/__tests__/database/index.js +++ b/integration-tests/api/__tests__/database/index.js @@ -40,7 +40,7 @@ describe("Database options", () => { // Idle time is 1000 ms so this should timeout await new Promise((resolve) => - setTimeout(() => resolve(console.log("")), 2000) + setTimeout(() => resolve(undefined), 2000) ) // This query should fail with a QueryRunnerAlreadyReleasedError diff --git a/integration-tests/api/__tests__/store/customer.js b/integration-tests/api/__tests__/store/customer.js index 65a5fe2938..d93e828491 100644 --- a/integration-tests/api/__tests__/store/customer.js +++ b/integration-tests/api/__tests__/store/customer.js @@ -293,16 +293,19 @@ describe("/store/customers", () => { }) expect(response.status).toEqual(200) - expect(response.data.orders).toEqual([ - expect.objectContaining({ - display_id: 3, - status: "canceled", - }), - expect.objectContaining({ - display_id: 1, - status: "completed", - }), - ]) + expect(response.data.orders.length).toEqual(2) + expect(response.data.orders).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + display_id: 3, + status: "canceled", + }), + expect.objectContaining({ + display_id: 1, + status: "completed", + }), + ]) + ) expect(response.data.orders.length).toEqual(2) }) }) diff --git a/integration-tests/api/__tests__/store/products.js b/integration-tests/api/__tests__/store/products.js index 3d6583791a..bc765f1ce5 100644 --- a/integration-tests/api/__tests__/store/products.js +++ b/integration-tests/api/__tests__/store/products.js @@ -10,6 +10,10 @@ const { const productSeeder = require("../../helpers/store-product-seeder") const adminSeeder = require("../../helpers/admin-seeder") +const { + allowedStoreProductsFields, + defaultStoreProductsRelations, +} = require("@medusajs/medusa/dist") jest.setTimeout(30000) @@ -988,23 +992,28 @@ describe("/store/products", () => { it("response contains only fields defined with `fields` param", async () => { const api = useApi() + const fields = allowedStoreProductsFields + const response = await api.get( - "/store/products/test-product?fields=handle" + `/store/products/test-product?fields=${fields.join(",")}` ) expect(response.status).toEqual(200) - expect(Object.keys(response.data.product)).toEqual([ - // fields - "handle", - // relations - "variants", - "options", - "images", - "tags", - "collection", - "type", - ]) + const expectedProperties = [...fields, ...defaultStoreProductsRelations] + const actualProperties = [ + ...Object.keys(response.data.product), + ...Object.keys(response.data.product.variants[0]).map( + (key) => `variants.${key}` + ), + "variants.prices.amount", + "options.values", + ] + + expect(Object.keys(response.data.product).length).toEqual(31) + expect(actualProperties).toEqual( + expect.arrayContaining(expectedProperties) + ) }) }) }) diff --git a/integration-tests/plugins/__tests__/inventory/order/order.js b/integration-tests/plugins/__tests__/inventory/order/order.js index aff714af85..1d675ceacb 100644 --- a/integration-tests/plugins/__tests__/inventory/order/order.js +++ b/integration-tests/plugins/__tests__/inventory/order/order.js @@ -5,7 +5,6 @@ const { initDb, useDb } = require("../../../../helpers/use-db") const { setPort, useApi } = require("../../../../helpers/use-api") const adminSeeder = require("../../../helpers/admin-seeder") -const cartSeeder = require("../../../helpers/cart-seeder") const { simpleProductFactory, simpleCustomerFactory, diff --git a/packages/admin-ui/ui/src/domain/orders/details/utils/use-admin-expand-paramter.ts b/packages/admin-ui/ui/src/domain/orders/details/utils/use-admin-expand-paramter.ts index 12af7c0baf..b39bbc4a3e 100644 --- a/packages/admin-ui/ui/src/domain/orders/details/utils/use-admin-expand-paramter.ts +++ b/packages/admin-ui/ui/src/domain/orders/details/utils/use-admin-expand-paramter.ts @@ -7,6 +7,7 @@ const orderRelations = [ "discounts", "discounts.rule", "shipping_methods", + "shipping_methods.shipping_option", "payments", "items", "fulfillments", diff --git a/packages/medusa-plugin-brightpearl/src/services/brightpearl.js b/packages/medusa-plugin-brightpearl/src/services/brightpearl.js index b3eb85870b..1095ff894f 100644 --- a/packages/medusa-plugin-brightpearl/src/services/brightpearl.js +++ b/packages/medusa-plugin-brightpearl/src/services/brightpearl.js @@ -1,8 +1,5 @@ -import { MedusaError, humanizeAmount } from "medusa-core-utils" -import { - ReservationType, - updateInventoryAndReservations, -} from "@medusajs/medusa" +import { humanizeAmount, MedusaError } from "medusa-core-utils" +import { updateInventoryAndReservations } from "@medusajs/medusa" import { BaseService } from "medusa-interfaces" import Brightpearl from "../utils/brightpearl" @@ -111,7 +108,7 @@ class BrightpearlService extends BaseService { httpMethod: "POST", uriTemplate: `${this.options.backend_url}/brightpearl/goods-out`, bodyTemplate: - "{\"account\": \"${account-code}\", \"lifecycle_event\": \"${lifecycle-event}\", \"resource_type\": \"${resource-type}\", \"id\": \"${resource-id}\" }", + '{"account": "${account-code}", "lifecycle_event": "${lifecycle-event}", "resource_type": "${resource-type}", "id": "${resource-id}" }', contentType: "application/json", idSetAccepted: false, }, @@ -1008,6 +1005,7 @@ class BrightpearlService extends BaseService { "shipping_address", "billing_address", "shipping_methods", + "shipping_methods.shipping_option", "payments", "sales_channel", ], diff --git a/packages/medusa-plugin-brightpearl/src/subscribers/order.js b/packages/medusa-plugin-brightpearl/src/subscribers/order.js index 732905a96d..4e5df3c97f 100644 --- a/packages/medusa-plugin-brightpearl/src/subscribers/order.js +++ b/packages/medusa-plugin-brightpearl/src/subscribers/order.js @@ -102,6 +102,7 @@ class OrderSubscriber { "additional_items.tax_lines", "shipping_address", "shipping_methods", + "shipping_methods.shipping_option", "shipping_methods.tax_lines", ], }) @@ -148,6 +149,7 @@ class OrderSubscriber { "additional_items.variant.product", "additional_items.tax_lines", "shipping_address", + "shipping_methods.shipping_option", "shipping_methods", ], }) diff --git a/packages/medusa-plugin-segment/src/subscribers/order.js b/packages/medusa-plugin-segment/src/subscribers/order.js index 49961fc8c5..3c13b334a5 100644 --- a/packages/medusa-plugin-segment/src/subscribers/order.js +++ b/packages/medusa-plugin-segment/src/subscribers/order.js @@ -46,6 +46,7 @@ class OrderSubscriber { "discounts", "discounts.rule", "shipping_methods", + "shipping_methods.shipping_option", "payments", "fulfillments", "returns", @@ -56,6 +57,7 @@ class OrderSubscriber { "swaps.return_order", "swaps.payment", "swaps.shipping_methods", + "swaps.shipping_methods.shipping_option", "swaps.shipping_address", "swaps.additional_items", "swaps.fulfillments", @@ -149,6 +151,7 @@ class OrderSubscriber { "discounts", "discounts.rule", "shipping_methods", + "shipping_methods.shipping_option", "payments", "fulfillments", "returns", @@ -159,6 +162,7 @@ class OrderSubscriber { "swaps.return_order", "swaps.payment", "swaps.shipping_methods", + "swaps.shipping_methods.shipping_option", "swaps.shipping_address", "swaps.additional_items", "swaps.fulfillments", @@ -257,6 +261,7 @@ class OrderSubscriber { "discounts", "discounts.rule", "shipping_methods", + "shipping_methods.shipping_option", "payments", "fulfillments", "items", @@ -267,6 +272,7 @@ class OrderSubscriber { "swaps.return_order", "swaps.payment", "swaps.shipping_methods", + "swaps.shipping_methods.shipping_option", "swaps.shipping_address", "swaps.additional_items", "swaps.fulfillments", diff --git a/packages/medusa-plugin-sendgrid/src/services/sendgrid.js b/packages/medusa-plugin-sendgrid/src/services/sendgrid.js index 95dca0344f..8ac063c6b9 100644 --- a/packages/medusa-plugin-sendgrid/src/services/sendgrid.js +++ b/packages/medusa-plugin-sendgrid/src/services/sendgrid.js @@ -850,7 +850,9 @@ class SendGridService extends NotificationService { } async swapCreatedData({ id }) { - const store = await this.storeService_.retrieve({ where: { id: Not(IsNull()) } }) + const store = await this.storeService_.retrieve({ + where: { id: Not(IsNull()) }, + }) const swap = await this.swapService_.retrieve(id, { relations: [ "additional_items", @@ -913,7 +915,7 @@ class SendGridService extends NotificationService { "shipping_total", "subtotal", ], - relations: ["items", "items.variant", "items.variant.product"] + relations: ["items", "items.variant", "items.variant.product"], }) const currencyCode = order.currency_code.toUpperCase() @@ -993,6 +995,7 @@ class SendGridService extends NotificationService { relations: [ "shipping_address", "shipping_methods", + "shipping_methods.shipping_option", "shipping_methods.tax_lines", "additional_items", "additional_items.variant", @@ -1028,11 +1031,7 @@ class SendGridService extends NotificationService { "shipping_total", "subtotal", ], - relations: [ - "items", - "items.variant", - "items.variant.product", - ] + relations: ["items", "items.variant", "items.variant.product"], }) const returnRequest = swap.return_order @@ -1152,7 +1151,7 @@ class SendGridService extends NotificationService { "order.items", "order.items.variant", "order.items.variant.product", - "order.shipping_address" + "order.shipping_address", ], }) diff --git a/packages/medusa-plugin-slack-notification/src/services/slack.js b/packages/medusa-plugin-slack-notification/src/services/slack.js index b7e948d0e1..4c7ec8b463 100644 --- a/packages/medusa-plugin-slack-notification/src/services/slack.js +++ b/packages/medusa-plugin-slack-notification/src/services/slack.js @@ -42,6 +42,7 @@ class SlackService extends BaseService { "discounts", "discounts.rule", "shipping_methods", + "shipping_methods.shipping_option", "payments", "fulfillments", "returns", @@ -51,6 +52,7 @@ class SlackService extends BaseService { "swaps.return_order", "swaps.payment", "swaps.shipping_methods", + "swaps.shipping_methods.shipping_option", "swaps.shipping_address", "swaps.additional_items", "swaps.fulfillments", diff --git a/packages/medusa/src/api/routes/admin/swaps/__tests__/get-swap.js b/packages/medusa/src/api/routes/admin/swaps/__tests__/get-swap.js index cc39fbcc8c..3c55487958 100644 --- a/packages/medusa/src/api/routes/admin/swaps/__tests__/get-swap.js +++ b/packages/medusa/src/api/routes/admin/swaps/__tests__/get-swap.js @@ -15,6 +15,7 @@ const defaultRelations = [ "return_order", "shipping_address", "shipping_methods", + "shipping_methods.shipping_option", ] const defaultFields = [ diff --git a/packages/medusa/src/api/routes/admin/swaps/index.ts b/packages/medusa/src/api/routes/admin/swaps/index.ts index 10a9d43f25..ed67ce031f 100644 --- a/packages/medusa/src/api/routes/admin/swaps/index.ts +++ b/packages/medusa/src/api/routes/admin/swaps/index.ts @@ -34,6 +34,7 @@ export const defaultAdminSwapRelations = [ "return_order", "shipping_address", "shipping_methods", + "shipping_methods.shipping_option", ] export const defaultAdminSwapFields = [ diff --git a/packages/medusa/src/api/routes/store/orders/index.ts b/packages/medusa/src/api/routes/store/orders/index.ts index ab86fb08c1..eca95e5bdb 100644 --- a/packages/medusa/src/api/routes/store/orders/index.ts +++ b/packages/medusa/src/api/routes/store/orders/index.ts @@ -83,6 +83,7 @@ export const defaultStoreOrdersRelations = [ "items", "items.variant", "shipping_methods", + "shipping_methods.shipping_option", "discounts", "discounts.rule", "customer", diff --git a/packages/medusa/src/api/routes/store/products/index.ts b/packages/medusa/src/api/routes/store/products/index.ts index 5410cfc35c..48b63e77c7 100644 --- a/packages/medusa/src/api/routes/store/products/index.ts +++ b/packages/medusa/src/api/routes/store/products/index.ts @@ -104,9 +104,7 @@ export const allowedStoreProductsFields = [ export const allowedStoreProductsRelations = [ ...defaultStoreProductsRelations, - "variants.title", "variants.inventory_items", - "variants.prices.amount", "sales_channels", ] diff --git a/packages/medusa/src/api/routes/store/swaps/index.ts b/packages/medusa/src/api/routes/store/swaps/index.ts index 2215ef2fc3..18f7c3cf74 100644 --- a/packages/medusa/src/api/routes/store/swaps/index.ts +++ b/packages/medusa/src/api/routes/store/swaps/index.ts @@ -24,10 +24,12 @@ export const defaultStoreSwapRelations = [ "additional_items.variant", "return_order", "return_order.shipping_method", + "return_order.shipping_method.shipping_option", "fulfillments", "payment", "shipping_address", "shipping_methods", + "shipping_methods.shipping_option", "cart", ] export const defaultStoreSwapFields: FindConfig["select"] = [ diff --git a/packages/medusa/src/models/shipping-method.ts b/packages/medusa/src/models/shipping-method.ts index 273b701f19..cc2bffe403 100644 --- a/packages/medusa/src/models/shipping-method.ts +++ b/packages/medusa/src/models/shipping-method.ts @@ -76,7 +76,7 @@ export class ShippingMethod { @JoinColumn({ name: "return_id" }) return_order: Return - @ManyToOne(() => ShippingOption, { eager: true }) + @ManyToOne(() => ShippingOption) @JoinColumn({ name: "shipping_option_id" }) shipping_option: ShippingOption diff --git a/packages/medusa/src/repositories/cart.ts b/packages/medusa/src/repositories/cart.ts index c70f9b8f04..d60c3d0930 100644 --- a/packages/medusa/src/repositories/cart.ts +++ b/packages/medusa/src/repositories/cart.ts @@ -1,8 +1,12 @@ import { objectToStringPath } from "@medusajs/utils" -import { flatten, groupBy, map, merge } from "lodash" -import { FindManyOptions, FindOptionsRelations, In } from "typeorm" +import { FindManyOptions, FindOptionsRelations } from "typeorm" import { dataSource } from "../loaders/database" import { Cart } from "../models" +import { + getGroupedRelations, + mergeEntitiesWithRelations, + queryEntityWithIds, +} from "../utils/repository" export const CartRepository = dataSource.getRepository(Cart).extend({ async findWithRelations( @@ -12,31 +16,17 @@ export const CartRepository = dataSource.getRepository(Cart).extend({ const entities = await this.find(optionsWithoutRelations) const entitiesIds = entities.map(({ id }) => id) - const groupedRelations = {} - for (const rel of objectToStringPath(relations)) { - const [topLevel] = rel.split(".") - if (groupedRelations[topLevel]) { - groupedRelations[topLevel].push(rel) - } else { - groupedRelations[topLevel] = [rel] - } - } + const groupedRelations = getGroupedRelations(objectToStringPath(relations)) - const entitiesIdsWithRelations = await Promise.all( - Object.entries(groupedRelations).map(async ([_, rels]) => { - return this.find({ - where: { id: In(entitiesIds) }, - select: ["id"], - relations: rels as string[], - }) - }) - ).then(flatten) - const entitiesAndRelations = entitiesIdsWithRelations.concat(entities) + const entitiesIdsWithRelations = await queryEntityWithIds({ + repository: this, + entityIds: entitiesIds, + groupedRelations, + select: ["id"], + }) - const entitiesAndRelationsById = groupBy(entitiesAndRelations, "id") - return map(entitiesAndRelationsById, (entityAndRelations) => - merge({}, ...entityAndRelations) - ) + const entitiesAndRelations = entities.concat(entitiesIdsWithRelations) + return mergeEntitiesWithRelations(entitiesAndRelations) }, async findOneWithRelations( diff --git a/packages/medusa/src/repositories/customer-group.ts b/packages/medusa/src/repositories/customer-group.ts index c9a6635a82..f269037eff 100644 --- a/packages/medusa/src/repositories/customer-group.ts +++ b/packages/medusa/src/repositories/customer-group.ts @@ -82,9 +82,9 @@ export const CustomerGroupRepository = dataSource : { ...idsOrOptionsWithoutRelations.order } const originalSelect = isOptionsArray ? undefined - : (objectToStringPath( - idsOrOptionsWithoutRelations.select - ) as (keyof CustomerGroup)[]) + : (objectToStringPath(idsOrOptionsWithoutRelations.select, { + includeParentPropertyFields: false, + }) as (keyof CustomerGroup)[]) const clonedOptions = isOptionsArray ? idsOrOptionsWithoutRelations : cloneDeep(idsOrOptionsWithoutRelations) @@ -169,7 +169,7 @@ export const CustomerGroupRepository = dataSource withDeleted, }) - const entitiesAndRelations = entitiesIdsWithRelations.concat(entities) + const entitiesAndRelations = entities.concat(entitiesIdsWithRelations) const entitiesToReturn = mergeEntitiesWithRelations(entitiesAndRelations) diff --git a/packages/medusa/src/repositories/order.ts b/packages/medusa/src/repositories/order.ts index e0e153b1fe..0ad6bd905a 100644 --- a/packages/medusa/src/repositories/order.ts +++ b/packages/medusa/src/repositories/order.ts @@ -1,8 +1,12 @@ import { objectToStringPath } from "@medusajs/utils" -import { flatten, groupBy, map, merge } from "lodash" +import { flatten } from "lodash" import { FindManyOptions, FindOptionsRelations, In } from "typeorm" import { dataSource } from "../loaders/database" import { Order } from "../models" +import { + getGroupedRelations, + mergeEntitiesWithRelations, +} from "../utils/repository" const ITEMS_REL_NAME = "items" const REGION_REL_NAME = "region" @@ -15,15 +19,7 @@ export const OrderRepository = dataSource.getRepository(Order).extend({ const entities = await this.find(optionsWithoutRelations) const entitiesIds = entities.map(({ id }) => id) - const groupedRelations: { [topLevel: string]: string[] } = {} - for (const rel of objectToStringPath(relations)) { - const [topLevel] = rel.split(".") - if (groupedRelations[topLevel]) { - groupedRelations[topLevel].push(rel) - } else { - groupedRelations[topLevel] = [rel] - } - } + const groupedRelations = getGroupedRelations(objectToStringPath(relations)) const entitiesIdsWithRelations = await Promise.all( Object.entries(groupedRelations).map(async ([topLevel, rels]) => { @@ -39,11 +35,8 @@ export const OrderRepository = dataSource.getRepository(Order).extend({ }) ).then(flatten) - const entitiesAndRelations = entitiesIdsWithRelations.concat(entities) - - const entitiesAndRelationsById = groupBy(entitiesAndRelations, "id") - - return map(entities, (e) => merge({}, ...entitiesAndRelationsById[e.id])) + const entitiesAndRelations = entities.concat(entitiesIdsWithRelations) + return mergeEntitiesWithRelations(entitiesAndRelations) }, async findOneWithRelations( diff --git a/packages/medusa/src/repositories/product-category.ts b/packages/medusa/src/repositories/product-category.ts index 537f8d6ded..b680e581ba 100644 --- a/packages/medusa/src/repositories/product-category.ts +++ b/packages/medusa/src/repositories/product-category.ts @@ -1,10 +1,9 @@ import { - Brackets, + DeleteResult, + FindOneOptions, FindOptionsWhere, ILike, - DeleteResult, In, - FindOneOptions, } from "typeorm" import { ProductCategory } from "../models/product-category" import { ExtendedFindConfig, QuerySelector } from "../types/common" @@ -44,7 +43,9 @@ export const ProductCategoryRepository = dataSource const options_ = { ...options } options_.where = options_.where as FindOptionsWhere - const columnsSelected = objectToStringPath(options_.select) + const columnsSelected = objectToStringPath(options_.select, { + includeParentPropertyFields: false, + }) const relationsSelected = objectToStringPath(options_.relations) const fetchSelectColumns = (relationName: string): string[] => { diff --git a/packages/medusa/src/repositories/product.ts b/packages/medusa/src/repositories/product.ts index d11a49f992..d9cab503d4 100644 --- a/packages/medusa/src/repositories/product.ts +++ b/packages/medusa/src/repositories/product.ts @@ -18,10 +18,11 @@ import { ExtendedFindConfig } from "@medusajs/types" import { applyOrdering, getGroupedRelations, + mergeEntitiesWithRelations, queryEntityWithIds, queryEntityWithoutRelations, } from "../utils/repository" -import { cloneDeep, groupBy, map, merge } from "lodash" +import { cloneDeep } from "lodash" export type DefaultWithoutRelations = Omit< ExtendedFindConfig, @@ -213,11 +214,13 @@ export const ProductRepository = dataSource.getRepository(Product).extend({ customJoinBuilders: [ (queryBuilder, alias, topLevel) => { if (topLevel === "variants") { - queryBuilder.leftJoinAndSelect( - `${alias}.${topLevel}`, - topLevel, - `${topLevel}.deleted_at IS NULL` - ) + const joinMethod = select.filter( + (key) => !!key.match(/^variants\.\w+$/i) + ).length + ? "leftJoin" + : "leftJoinAndSelect" + + queryBuilder[joinMethod](`${alias}.${topLevel}`, topLevel) if ( !Object.keys(order!).some((key) => key.startsWith("variants")) @@ -228,7 +231,8 @@ export const ProductRepository = dataSource.getRepository(Product).extend({ return false } - return true + + return }, (queryBuilder, alias, topLevel) => { if (topLevel === "categories") { @@ -245,7 +249,7 @@ export const ProductRepository = dataSource.getRepository(Product).extend({ return false } - return true + return }, ], }) @@ -479,9 +483,9 @@ export const ProductRepository = dataSource.getRepository(Product).extend({ : { ...idsOrOptionsWithoutRelations.order } const originalSelect = isOptionsArray ? undefined - : (objectToStringPath( - idsOrOptionsWithoutRelations.select - ) as (keyof Product)[]) + : (objectToStringPath(idsOrOptionsWithoutRelations.select, { + includeParentPropertyFields: false, + }) as (keyof Product)[]) const clonedOptions = isOptionsArray ? idsOrOptionsWithoutRelations : cloneDeep(idsOrOptionsWithoutRelations) @@ -543,10 +547,9 @@ export const ProductRepository = dataSource.getRepository(Product).extend({ withDeleted, }) - const entitiesAndRelations = groupBy(entitiesIdsWithRelations, "id") - const entitiesToReturn = map(entitiesIds, (id) => - merge({}, ...entitiesAndRelations[id]) - ) + const entitiesAndRelations = entities.concat(entitiesIdsWithRelations) + const entitiesToReturn = + mergeEntitiesWithRelations(entitiesAndRelations) return [entitiesToReturn, count] }, diff --git a/packages/medusa/src/repositories/tax-rate.ts b/packages/medusa/src/repositories/tax-rate.ts index 254b1077d4..5a323ebe65 100644 --- a/packages/medusa/src/repositories/tax-rate.ts +++ b/packages/medusa/src/repositories/tax-rate.ts @@ -34,7 +34,8 @@ export const TaxRateRepository = dataSource.getRepository(TaxRate).extend({ if (isDefined(findOptions.select)) { const selectableCols: (keyof TaxRate)[] = [] const legacySelect = objectToStringPath( - findOptions.select as FindOptionsSelect + findOptions.select as FindOptionsSelect, + { includeParentPropertyFields: false } ) as (keyof TaxRate)[] for (const k of legacySelect) { if (!resolveableFields.includes(k)) { diff --git a/packages/medusa/src/services/__tests__/cart.js b/packages/medusa/src/services/__tests__/cart.js index c26370aad6..212f0a45c7 100644 --- a/packages/medusa/src/services/__tests__/cart.js +++ b/packages/medusa/src/services/__tests__/cart.js @@ -865,7 +865,9 @@ describe("CartService", () => { payment_sessions: true, region: { countries: true }, shipping_address: true, - shipping_methods: true, + shipping_methods: { + shipping_option: true, + }, }), expect.objectContaining({ select: undefined, diff --git a/packages/medusa/src/services/__tests__/swap.ts b/packages/medusa/src/services/__tests__/swap.ts index 9a2419590f..a168eae7a3 100644 --- a/packages/medusa/src/services/__tests__/swap.ts +++ b/packages/medusa/src/services/__tests__/swap.ts @@ -12,16 +12,12 @@ import { ProductVariantInventoryService, ReturnService, ShippingOptionService, - TotalsService + TotalsService, } from "../index" import LineItemAdjustmentService from "../line-item-adjustment" import SwapService from "../swap" -import { - LineItemAdjustmentServiceMock -} from "../__mocks__/line-item-adjustment" -import { - ProductVariantInventoryServiceMock -} from "../__mocks__/product-variant-inventory" +import { LineItemAdjustmentServiceMock } from "../__mocks__/line-item-adjustment" +import { ProductVariantInventoryServiceMock } from "../__mocks__/product-variant-inventory" /* ******************** DEFAULT REPOSITORY MOCKS ******************** */ @@ -273,32 +269,33 @@ describe("SwapService", () => { expect(swapRepo.findOne).toHaveBeenCalledWith({ relations: { additional_items: { - variant: true + variant: true, }, order: { claims: { - additional_items: true + additional_items: true, }, discounts: { - rule: true + rule: true, }, items: { variant: { - product: true - } + product: true, + }, }, swaps: { - additional_items: true - } + additional_items: true, + }, }, return_order: { items: true, shipping_method: { - tax_lines: true - } - } + shipping_option: true, + tax_lines: true, + }, + }, }, - where: { id: IdMap.getId("swap-1") } + where: { id: IdMap.getId("swap-1") }, }) expect(lineItemService.createReturnLines).toHaveBeenCalledTimes(1) @@ -413,15 +410,13 @@ describe("SwapService", () => { describe("success", () => { const lineItemService = { - generate: jest - .fn() - .mockImplementation(({ variantId, quantity }) => { - return { - unit_price: 100, - variant_id: variantId, - quantity, - } - }), + generate: jest.fn().mockImplementation(({ variantId, quantity }) => { + return { + unit_price: 100, + variant_id: variantId, + quantity, + } + }), retrieve: () => Promise.resolve({}), list: () => Promise.resolve([]), withTransaction: function () { @@ -457,13 +452,16 @@ describe("SwapService", () => { ) expect(lineItemService.generate).toHaveBeenCalledTimes(1) - expect(lineItemService.generate).toHaveBeenCalledWith({ - quantity: 1, - variantId: IdMap.getId("new-variant") - }, { - "cart": undefined, - region_id: IdMap.getId("region") - }) + expect(lineItemService.generate).toHaveBeenCalledWith( + { + quantity: 1, + variantId: IdMap.getId("new-variant"), + }, + { + cart: undefined, + region_id: IdMap.getId("region"), + } + ) }) it("creates swap", async () => { @@ -605,7 +603,7 @@ describe("SwapService", () => { shipping_methods: existing.shipping_methods, }, [{ item_id: "1234", quantity: 2 }], - { swap_id: IdMap.getId("swap"), metadata: {} }, + { swap_id: IdMap.getId("swap"), metadata: {} } ) }) }) @@ -798,8 +796,7 @@ describe("SwapService", () => { describe("failure", () => { const swapRepo = MockRepository({ - findOne: () => - Promise.resolve({ canceled_at: new Date() }), + findOne: () => Promise.resolve({ canceled_at: new Date() }), }) const swapService = new SwapService({ diff --git a/packages/medusa/src/services/cart.ts b/packages/medusa/src/services/cart.ts index 2bcdaa4756..1abd23d29f 100644 --- a/packages/medusa/src/services/cart.ts +++ b/packages/medusa/src/services/cart.ts @@ -1004,7 +1004,7 @@ class CartService extends TransactionBaseService { * shipping discount * If a free shipping is present, we set shipping methods price to 0 * if a free shipping was present, we set shipping methods to original amount - * @param cart - the the cart to adjust free shipping for + * @param cart - the cart to adjust free shipping for * @param shouldAdd - flag to indicate, if we should add or remove * @return void */ @@ -1058,6 +1058,7 @@ class CartService extends TransactionBaseService { "items.variant", "items.variant.product", "shipping_methods", + "shipping_methods.shipping_option", "shipping_address", "billing_address", "gift_cards", @@ -1530,6 +1531,7 @@ class CartService extends TransactionBaseService { "discounts.rule", "payment_sessions", "shipping_methods", + "shipping_methods.shipping_option", ], }) @@ -1829,6 +1831,7 @@ class CartService extends TransactionBaseService { "discounts.rule", "gift_cards", "shipping_methods", + "shipping_methods.shipping_option", "billing_address", "shipping_address", "region", @@ -2170,7 +2173,12 @@ class CartService extends TransactionBaseService { } const updatedCart = await this.retrieve(cart.id, { - relations: ["discounts", "discounts.rule", "shipping_methods"], + relations: [ + "discounts", + "discounts.rule", + "shipping_methods", + "shipping_methods.shipping_option", + ], }) // if cart has freeshipping, adjust price @@ -2537,6 +2545,7 @@ class CartService extends TransactionBaseService { "region.tax_rates", "shipping_address", "shipping_methods", + "shipping_methods.shipping_option", ], }) @@ -2559,6 +2568,7 @@ class CartService extends TransactionBaseService { "items", "items.tax_lines", "shipping_methods", + "shipping_methods.shipping_option", "shipping_methods.tax_lines", ], }) diff --git a/packages/medusa/src/services/claim.ts b/packages/medusa/src/services/claim.ts index dee72aab6b..978ced59b2 100644 --- a/packages/medusa/src/services/claim.ts +++ b/packages/medusa/src/services/claim.ts @@ -530,6 +530,7 @@ export default class ClaimService extends TransactionBaseService { "additional_items.variant", "additional_items.variant.product", "shipping_methods", + "shipping_methods.shipping_option", "shipping_methods.tax_lines", "shipping_address", "order", diff --git a/packages/medusa/src/services/order-edit.ts b/packages/medusa/src/services/order-edit.ts index 8280931ca2..af3ae17b43 100644 --- a/packages/medusa/src/services/order-edit.ts +++ b/packages/medusa/src/services/order-edit.ts @@ -514,6 +514,7 @@ export default class OrderEditService extends TransactionBaseService { "items.variant", "region.tax_rates", "shipping_methods", + "shipping_methods.shipping_option", "shipping_methods.tax_lines", ], }) diff --git a/packages/medusa/src/services/return.ts b/packages/medusa/src/services/return.ts index 985bf07a7d..d9231288e6 100644 --- a/packages/medusa/src/services/return.ts +++ b/packages/medusa/src/services/return.ts @@ -596,6 +596,7 @@ class ReturnService extends TransactionBaseService { "discounts.rule", "refunds", "shipping_methods", + "shipping_methods.shipping_option", "region", "swaps", "swaps.additional_items", diff --git a/packages/medusa/src/services/swap.ts b/packages/medusa/src/services/swap.ts index f083e16eed..6c01b21495 100644 --- a/packages/medusa/src/services/swap.ts +++ b/packages/medusa/src/services/swap.ts @@ -29,7 +29,7 @@ import { } from "./index" import { EntityManager, In } from "typeorm" import { FindConfig, Selector, WithRequiredProperty } from "../types/common" -import { MedusaError, isDefined } from "medusa-core-utils" +import { isDefined, MedusaError } from "medusa-core-utils" import { buildQuery, setMetadata, validateId } from "../utils" import { CreateShipmentConfig } from "../types/fulfillment" @@ -579,6 +579,7 @@ class SwapService extends TransactionBaseService { "return_order", "return_order.items", "return_order.shipping_method", + "return_order.shipping_method.shipping_option", "return_order.shipping_method.tax_lines", ], }) @@ -918,6 +919,7 @@ class SwapService extends TransactionBaseService { "additional_items.variant", "additional_items.variant.product", "shipping_methods", + "shipping_methods.shipping_option", "shipping_methods.tax_lines", "order", "order.region", diff --git a/packages/medusa/src/strategies/cart-completion.ts b/packages/medusa/src/strategies/cart-completion.ts index c14411c5a4..e4d4bfdbaf 100644 --- a/packages/medusa/src/strategies/cart-completion.ts +++ b/packages/medusa/src/strategies/cart-completion.ts @@ -197,6 +197,7 @@ class CartCompletionStrategy extends AbstractCartCompletionStrategy { "region.tax_rates", "shipping_address", "shipping_methods", + "shipping_methods.shipping_option", ], }) diff --git a/packages/medusa/src/types/orders.ts b/packages/medusa/src/types/orders.ts index de03e47588..c3d99240c4 100644 --- a/packages/medusa/src/types/orders.ts +++ b/packages/medusa/src/types/orders.ts @@ -133,9 +133,12 @@ export const defaultAdminOrdersRelations = [ "returns.items", "returns.items.reason", "returns.shipping_method", + "returns.shipping_method.shipping_option", "returns.shipping_method.tax_lines", "shipping_address", "shipping_methods", + "shipping_methods.shipping_option", + "shipping_methods.tax_lines", "swaps", "swaps.additional_items", "swaps.additional_items.variant", @@ -144,9 +147,11 @@ export const defaultAdminOrdersRelations = [ "swaps.payment", "swaps.return_order", "swaps.return_order.shipping_method", + "swaps.return_order.shipping_method.shipping_option", "swaps.return_order.shipping_method.tax_lines", "swaps.shipping_address", "swaps.shipping_methods", + "swaps.shipping_methods.shipping_option", "swaps.shipping_methods.tax_lines", // "claims.claim_items.tags", ] diff --git a/packages/medusa/src/utils/repository.ts b/packages/medusa/src/utils/repository.ts index b20f718a3c..3f51d0dde7 100644 --- a/packages/medusa/src/utils/repository.ts +++ b/packages/medusa/src/utils/repository.ts @@ -7,6 +7,11 @@ import { } from "typeorm" import { ExtendedFindConfig } from "../types/common" +// Regex matches all '.' except the rightmost +export const positiveLookaheadDotReplacer = new RegExp(/\.(?=[^.]*\.)/, "g") +// Replace all '.' with '__' to avoid typeorm's automatic aliasing +export const dotReplacer = new RegExp(/\./, "g") + /** * Custom query entity, it is part of the creation of a custom findWithRelationsAndCount needs. * Allow to query the relations for the specified entity ids @@ -35,62 +40,86 @@ export async function queryEntityWithIds({ qb: SelectQueryBuilder, alias: string, toplevel: string - ) => boolean)[] + ) => false | undefined)[] }): Promise { const alias = repository.metadata.name.toLowerCase() return await Promise.all( - Object.entries(groupedRelations).map(async ([toplevel, rels]) => { - let querybuilder = repository.createQueryBuilder(alias) + Object.entries(groupedRelations).map( + async ([toplevel, topLevelRelations]) => { + let querybuilder = repository.createQueryBuilder(alias) - if (select && select.length) { - querybuilder.select(select.map((f) => `${alias}.${f as string}`)) - } - - let shouldAttachDefault = true - for (const customJoinBuilder of customJoinBuilders) { - const result = customJoinBuilder(querybuilder, alias, toplevel) - shouldAttachDefault = shouldAttachDefault && result - } - - // If the toplevel relation has been attached with a customJoinBuilder and the function return false then - // do not attach the toplevel join bellow. - if (shouldAttachDefault) { - querybuilder = querybuilder.leftJoinAndSelect( - `${alias}.${toplevel}`, - toplevel - ) - } - - for (const rel of rels) { - const [_, rest] = rel.split(".") - if (!rest) { - continue + if (select?.length) { + querybuilder.select( + (select as string[]) + .filter(function (s) { + return s.startsWith(toplevel) || !s.includes(".") + }) + .map((column) => { + // In case the column is the toplevel relation, we need to replace the dot with a double underscore if it also contains top level relations + if (column.includes(toplevel)) { + return topLevelRelations.some((rel) => column.includes(rel)) + ? column.replace(positiveLookaheadDotReplacer, "__") + : column + } + return `${alias}.${column}` + }) + ) } - querybuilder = querybuilder.leftJoinAndSelect( - // Regex matches all '.' except the rightmost - rel.replace(/\.(?=[^.]*\.)/g, "__"), - // Replace all '.' with '__' to avoid typeorm's automatic aliasing - rel.replace(/\./g, "__") - ) - } - if (withDeleted) { - querybuilder = querybuilder - .where(`${alias}.id IN (:...entitiesIds)`, { - entitiesIds: entityIds, - }) - .withDeleted() - } else { - querybuilder = querybuilder.where( - `${alias}.deleted_at IS NULL AND ${alias}.id IN (:...entitiesIds)`, - { - entitiesIds: entityIds, + let shouldAttachDefault: boolean | undefined = true + for (const customJoinBuilder of customJoinBuilders) { + const result = customJoinBuilder(querybuilder, alias, toplevel) + if (result === undefined) { + continue } - ) - } - return querybuilder.getMany() - }) + shouldAttachDefault = shouldAttachDefault && result + } + + if (shouldAttachDefault) { + const regexp = new RegExp(`^${toplevel}\\.\\w+$`) + const joinMethod = (select as string[]).filter( + (key) => !!key.match(regexp) + ).length + ? "leftJoin" + : "leftJoinAndSelect" + + querybuilder = querybuilder[joinMethod]( + `${alias}.${toplevel}`, + toplevel + ) + } + + for (const rel of topLevelRelations) { + const [_, rest] = rel.split(".") + if (!rest) { + continue + } + + const regexp = new RegExp(`^${rel}\\.\\w+$`) + const joinMethod = (select as string[]).filter( + (key) => !!key.match(regexp) + ).length + ? "leftJoin" + : "leftJoinAndSelect" + + querybuilder = querybuilder[joinMethod]( + rel.replace(positiveLookaheadDotReplacer, "__"), + rel.replace(dotReplacer, "__") + ) + } + + querybuilder = querybuilder.where(`${alias}.id IN (:...entitiesIds)`, { + entitiesIds: entityIds, + }) + + if (withDeleted) { + querybuilder.withDeleted() + } + + return querybuilder.getMany() + } + ) ).then(flatten) } diff --git a/packages/utils/src/common/__tests__/build-query.spec.ts b/packages/utils/src/common/__tests__/build-query.spec.ts new file mode 100644 index 0000000000..9ddc53dcf0 --- /dev/null +++ b/packages/utils/src/common/__tests__/build-query.spec.ts @@ -0,0 +1,91 @@ +import { buildSelects } from "../build-query" + +describe("buildSelects", () => { + it("successfully build back select object shape to list", () => { + const q = buildSelects([ + "order", + "order.items", + "order.swaps", + "order.swaps.additional_items", + "order.discounts", + "order.discounts.rule", + "order.claims", + "order.claims.additional_items", + "additional_items", + "additional_items.variant", + "return_order", + "return_order.items", + "return_order.shipping_method", + "return_order.shipping_method.tax_lines", + ]) + + expect(q).toEqual({ + order: { + items: true, + swaps: { + additional_items: true, + }, + discounts: { + rule: true, + }, + claims: { + additional_items: true, + }, + }, + additional_items: { + variant: true, + }, + return_order: { + items: true, + shipping_method: { + tax_lines: true, + }, + }, + }) + }) +}) + +describe("buildSelects", () => { + it("successfully build back select object shape to list", () => { + const q = buildSelects([ + "order", + "order.items", + "order.swaps", + "order.swaps.additional_items", + "order.discounts", + "order.discounts.rule", + "order.claims", + "order.claims.additional_items", + "additional_items", + "additional_items.variant", + "return_order", + "return_order.items", + "return_order.shipping_method", + "return_order.shipping_method.tax_lines", + ]) + + expect(q).toEqual({ + order: { + items: true, + swaps: { + additional_items: true, + }, + discounts: { + rule: true, + }, + claims: { + additional_items: true, + }, + }, + additional_items: { + variant: true, + }, + return_order: { + items: true, + shipping_method: { + tax_lines: true, + }, + }, + }) + }) +}) diff --git a/packages/utils/src/common/__tests__/object-to-string-path.spec.ts b/packages/utils/src/common/__tests__/object-to-string-path.spec.ts new file mode 100644 index 0000000000..b36b6d02bb --- /dev/null +++ b/packages/utils/src/common/__tests__/object-to-string-path.spec.ts @@ -0,0 +1,42 @@ +import { objectToStringPath } from "../object-to-string-path" + +describe("objectToStringPath", function () { + it("should return only the properties path of the properties that are set to true", function () { + const res = objectToStringPath( + { + product: true, + variants: { + title: true, + prices: { + amount: true, + }, + }, + }, + { + includeParentPropertyFields: false, + } + ) + + expect(res).toEqual(["product", "variants.title", "variants.prices.amount"]) + }) + + it("should return a string path from an object including properties that are object and contains other properties set to true", function () { + const res = objectToStringPath({ + product: true, + variants: { + title: true, + prices: { + amount: true, + }, + }, + }) + + expect(res).toEqual([ + "product", + "variants", + "variants.title", + "variants.prices", + "variants.prices.amount", + ]) + }) +}) diff --git a/packages/utils/src/common/object-to-string-path.ts b/packages/utils/src/common/object-to-string-path.ts index b1010df1ab..c3f64e56be 100644 --- a/packages/utils/src/common/object-to-string-path.ts +++ b/packages/utils/src/common/object-to-string-path.ts @@ -4,7 +4,8 @@ import { isObject } from "./is-object" * Converts a structure of find options to an * array of string paths * @example - * input: { + * // With `includeTruePropertiesOnly` default value set to false + * const result = objectToStringPath({ * test: { * test1: true, * test2: true, @@ -13,20 +14,50 @@ import { isObject } from "./is-object" * }, * }, * test2: true - * } - * output: ['test.test1', 'test.test2', 'test.test3.test4', 'test2'] - * @param input + * }) + * console.log(result) + * // output: ['test', 'test.test1', 'test.test2', 'test.test3', 'test.test3.test4', 'test2'] + * + * @example + * // With `includeTruePropertiesOnly` set to true + * const result = objectToStringPath({ + * test: { + * test1: true, + * test2: true, + * test3: { + * test4: true + * }, + * }, + * test2: true + * }, { + * includeTruePropertiesOnly: true + * }) + * console.log(result) + * // output: ['test.test1', 'test.test2', 'test.test3.test4', 'test2'] + * + * @param {InputObject} input + * @param {boolean} includeParentPropertyFields If set to true (example 1), all properties will be included as well as the parents, + * otherwise (example 2) all properties path set to true will included, excluded the parents */ -export function objectToStringPath(input: object = {}): string[] { +export function objectToStringPath( + input: object = {}, + { includeParentPropertyFields }: { includeParentPropertyFields: boolean } = { + includeParentPropertyFields: true, + } +): string[] { if (!isObject(input) || !Object.keys(input).length) { return [] } - const output: Set = new Set(Object.keys(input)) + const output: Set = includeParentPropertyFields + ? new Set(Object.keys(input)) + : new Set() for (const key of Object.keys(input)) { - if (input[key] != undefined && typeof input[key] === "object") { - const deepRes = objectToStringPath(input[key]) + if (isObject(input[key])) { + const deepRes = objectToStringPath(input[key], { + includeParentPropertyFields, + }) const items = deepRes.reduce((acc, val) => { acc.push(`${key}.${val}`) @@ -37,7 +68,9 @@ export function objectToStringPath(input: object = {}): string[] { continue } - output.add(key) + if (isObject(key) || input[key] === true) { + output.add(key) + } } return Array.from(output)