From c319edb8e0ecd13d086652147667916e5abab2d8 Mon Sep 17 00:00:00 2001 From: Adrien de Peretti Date: Tue, 20 Feb 2024 12:07:39 +0100 Subject: [PATCH] chore(utils): Add default ordering to the internal service factory list/listAndCount (#6436) **What** Default ordering. By default, only the top level entity ordering is applied using the primary keys, the relations are default ordered by the foreign keys. It include tests fixing for deterministic data ordering --- .changeset/giant-berries-rush.md | 5 ++ .../services/auth-user/index.spec.ts | 24 +++--- .../services/module/auth-user.spec.ts | 24 +++--- .../fulfillment-module-service.spec.ts | 77 +++++++++++++++---- .../__tests__/services/currency/index.spec.ts | 26 +++---- .../services/money-amount/index.spec.ts | 4 +- .../services/pricing-module/currency.spec.ts | 24 +++--- .../pricing-module/money-amount.spec.ts | 2 +- .../price-set-money-amount-rules.spec.ts | 18 ++--- .../src/services/__tests__/currency.spec.ts | 12 +++ .../src/services/__tests__/product.spec.ts | 12 +++ .../services/__tests__/sales-channle.spec.ts | 3 + .../internal-module-service-factory.spec.ts | 35 ++++++++- .../internal-module-service-factory.ts | 20 +++++ 14 files changed, 208 insertions(+), 78 deletions(-) create mode 100644 .changeset/giant-berries-rush.md diff --git a/.changeset/giant-berries-rush.md b/.changeset/giant-berries-rush.md new file mode 100644 index 0000000000..36fe5f9924 --- /dev/null +++ b/.changeset/giant-berries-rush.md @@ -0,0 +1,5 @@ +--- +"@medusajs/utils": patch +--- + +chore(utils): Add default ordering to the internal service factory list/listAndCount diff --git a/packages/auth/integration-tests/__tests__/services/auth-user/index.spec.ts b/packages/auth/integration-tests/__tests__/services/auth-user/index.spec.ts index 32dd1aa819..3aae9c7e48 100644 --- a/packages/auth/integration-tests/__tests__/services/auth-user/index.spec.ts +++ b/packages/auth/integration-tests/__tests__/services/auth-user/index.spec.ts @@ -38,15 +38,15 @@ describe("AuthUser Service", () => { const serialized = JSON.parse(JSON.stringify(authUsers)) expect(serialized).toEqual([ - expect.objectContaining({ - provider: "manual", - }), - expect.objectContaining({ - provider: "manual", - }), expect.objectContaining({ provider: "store", }), + expect.objectContaining({ + provider: "manual", + }), + expect.objectContaining({ + provider: "manual", + }), ]) }) @@ -87,15 +87,15 @@ describe("AuthUser Service", () => { expect(count).toEqual(3) expect(serialized).toEqual([ - expect.objectContaining({ - provider: "manual", - }), - expect.objectContaining({ - provider: "manual", - }), expect.objectContaining({ provider: "store", }), + expect.objectContaining({ + provider: "manual", + }), + expect.objectContaining({ + provider: "manual", + }), ]) }) diff --git a/packages/auth/integration-tests/__tests__/services/module/auth-user.spec.ts b/packages/auth/integration-tests/__tests__/services/module/auth-user.spec.ts index 2a45edeeb6..263ee6268f 100644 --- a/packages/auth/integration-tests/__tests__/services/module/auth-user.spec.ts +++ b/packages/auth/integration-tests/__tests__/services/module/auth-user.spec.ts @@ -43,15 +43,15 @@ describe("AuthModuleService - AuthUser", () => { const authUsers = await service.list() expect(authUsers).toEqual([ - expect.objectContaining({ - provider: "manual", - }), - expect.objectContaining({ - provider: "manual", - }), expect.objectContaining({ provider: "store", }), + expect.objectContaining({ + provider: "manual", + }), + expect.objectContaining({ + provider: "manual", + }), ]) }) @@ -89,15 +89,15 @@ describe("AuthModuleService - AuthUser", () => { expect(count).toEqual(3) expect(authUsers).toEqual([ - expect.objectContaining({ - provider: "manual", - }), - expect.objectContaining({ - provider: "manual", - }), expect.objectContaining({ provider: "store", }), + expect.objectContaining({ + provider: "manual", + }), + expect.objectContaining({ + provider: "manual", + }), ]) }) diff --git a/packages/fulfillment/integration-tests/__tests__/fulfillment-module-service.spec.ts b/packages/fulfillment/integration-tests/__tests__/fulfillment-module-service.spec.ts index 5922f8e9b6..a1aaa62d8b 100644 --- a/packages/fulfillment/integration-tests/__tests__/fulfillment-module-service.spec.ts +++ b/packages/fulfillment/integration-tests/__tests__/fulfillment-module-service.spec.ts @@ -44,12 +44,44 @@ moduleIntegrationTestRunner({ }, ], }, + { + name: "test2", + geo_zones: [ + { + type: GeoZoneType.COUNTRY, + country_code: "fr", + }, + ], + }, + { + name: "_test", + geo_zones: [ + { + type: GeoZoneType.COUNTRY, + country_code: "fr", + }, + ], + }, ], }) - let listedSets = await service.list({ - type: createdSet1.type, - }) + let listedSets = await service.list( + { + type: createdSet1.type, + }, + { + relations: ["service_zones"], + } + ) + + const listedSets2 = await service.list( + { + type: createdSet1.type, + }, + { + relations: ["service_zones"], + } + ) expect(listedSets).toEqual( expect.arrayContaining([ @@ -58,6 +90,15 @@ moduleIntegrationTestRunner({ ]) ) + // Respecting order id by default + expect(listedSets[1].service_zones).toEqual([ + expect.objectContaining({ name: "test" }), + expect.objectContaining({ name: "test2" }), + expect.objectContaining({ name: "_test" }), + ]) + + expect(listedSets2).toEqual(listedSets2) + listedSets = await service.list({ name: createdSet2.name, }) @@ -1206,9 +1247,11 @@ moduleIntegrationTestRunner({ expect(updatedFulfillmentSets).toHaveLength(2) - let i = 0 for (const data_ of updateData) { - expect(updatedFulfillmentSets[i]).toEqual( + const expectedFulfillmentSet = updatedFulfillmentSets.find( + (f) => f.id === data_.id + ) + expect(expectedFulfillmentSet).toEqual( expect.objectContaining({ id: data_.id, name: data_.name, @@ -1231,7 +1274,6 @@ moduleIntegrationTestRunner({ ]), }) ) - ++i } const serviceZones = await service.listServiceZones() @@ -1310,16 +1352,18 @@ moduleIntegrationTestRunner({ expect(updatedFulfillmentSets).toHaveLength(2) - let i = 0 for (const data_ of updateData) { - expect(updatedFulfillmentSets[i]).toEqual( + const expectedFulfillmentSet = updatedFulfillmentSets.find( + (f) => f.id === data_.id + ) + expect(expectedFulfillmentSet).toEqual( expect.objectContaining({ id: data_.id, name: data_.name, type: data_.type, service_zones: expect.arrayContaining([ expect.objectContaining({ - id: createdFulfillmentSets[i].service_zones[0].id, + id: expect.any(String), }), expect.objectContaining({ id: expect.any(String), @@ -1338,7 +1382,6 @@ moduleIntegrationTestRunner({ ]), }) ) - ++i } const serviceZones = await service.listServiceZones() @@ -1470,9 +1513,11 @@ moduleIntegrationTestRunner({ expect(updatedServiceZones).toHaveLength(2) - let i = 0 for (const data_ of updateData) { - expect(updatedServiceZones[i]).toEqual( + const expectedServiceZone = updatedServiceZones.find( + (serviceZone) => serviceZone.id === data_.id + ) + expect(expectedServiceZone).toEqual( expect.objectContaining({ id: data_.id, name: data_.name, @@ -1485,7 +1530,6 @@ moduleIntegrationTestRunner({ ]), }) ) - ++i } }) @@ -1617,16 +1661,17 @@ moduleIntegrationTestRunner({ expect(updatedGeoZones).toHaveLength(2) - let i = 0 for (const data_ of updateData) { - expect(updatedGeoZones[i]).toEqual( + const expectedGeoZone = updatedGeoZones.find( + (geoZone) => geoZone.id === data_.id + ) + expect(expectedGeoZone).toEqual( expect.objectContaining({ id: data_.id, type: data_.type, country_code: data_.country_code, }) ) - ++i } }) }) diff --git a/packages/pricing/integration-tests/__tests__/services/currency/index.spec.ts b/packages/pricing/integration-tests/__tests__/services/currency/index.spec.ts index cf054ed3ae..fc14191baa 100644 --- a/packages/pricing/integration-tests/__tests__/services/currency/index.spec.ts +++ b/packages/pricing/integration-tests/__tests__/services/currency/index.spec.ts @@ -1,18 +1,18 @@ import { SqlEntityManager } from "@mikro-orm/postgresql" import { Currency } from "@models" -import { CurrencyService } from "@services" import { createCurrencies } from "../../../__fixtures__/currency" import { MikroOrmWrapper } from "../../../utils" import { createMedusaContainer } from "@medusajs/utils" import { asValue } from "awilix" import ContainerLoader from "../../../../src/loaders/container" +import { ModulesSdkTypes } from "@medusajs/types" jest.setTimeout(30000) describe("Currency Service", () => { - let service: CurrencyService + let service: ModulesSdkTypes.InternalModuleService let testManager: SqlEntityManager let repositoryManager: SqlEntityManager let data!: Currency[] @@ -57,14 +57,14 @@ describe("Currency Service", () => { const currenciesResult = await service.list() expect(currenciesResult).toEqual([ - expect.objectContaining({ - code: "USD", - name: "US Dollar", - }), expect.objectContaining({ code: "CAD", name: "Canadian Dollar", }), + expect.objectContaining({ + code: "USD", + name: "US Dollar", + }), ]) }) @@ -86,14 +86,14 @@ describe("Currency Service", () => { expect(count).toEqual(2) expect(currenciesResult).toEqual([ - expect.objectContaining({ - code: "USD", - name: "US Dollar", - }), expect.objectContaining({ code: "CAD", name: "Canadian Dollar", }), + expect.objectContaining({ + code: "USD", + name: "US Dollar", + }), ]) }) @@ -120,8 +120,8 @@ describe("Currency Service", () => { expect(count).toEqual(2) expect(currenciesResult).toEqual([ expect.objectContaining({ - code: "CAD", - name: "Canadian Dollar", + code: "USD", + name: "US Dollar", }), ]) }) @@ -140,7 +140,7 @@ describe("Currency Service", () => { expect(count).toEqual(2) expect(serialized).toEqual([ { - code: "USD", + code: "CAD", }, ]) }) diff --git a/packages/pricing/integration-tests/__tests__/services/money-amount/index.spec.ts b/packages/pricing/integration-tests/__tests__/services/money-amount/index.spec.ts index 618857e6db..592c060644 100644 --- a/packages/pricing/integration-tests/__tests__/services/money-amount/index.spec.ts +++ b/packages/pricing/integration-tests/__tests__/services/money-amount/index.spec.ts @@ -214,8 +214,8 @@ describe("MoneyAmount Service", () => { expect(count).toEqual(3) expect(serialized).toEqual([ { - id: "money-amount-USD", - amount: 500, + id: "money-amount-CAD", + amount: 600, }, ]) }) diff --git a/packages/pricing/integration-tests/__tests__/services/pricing-module/currency.spec.ts b/packages/pricing/integration-tests/__tests__/services/pricing-module/currency.spec.ts index ae2865a504..2ecd9a880d 100644 --- a/packages/pricing/integration-tests/__tests__/services/pricing-module/currency.spec.ts +++ b/packages/pricing/integration-tests/__tests__/services/pricing-module/currency.spec.ts @@ -46,10 +46,6 @@ describe("PricingModule Service - Currency", () => { const currenciesResult = await service.listCurrencies() expect(currenciesResult).toEqual([ - expect.objectContaining({ - code: "USD", - name: "US Dollar", - }), expect.objectContaining({ code: "CAD", name: "Canadian Dollar", @@ -58,6 +54,10 @@ describe("PricingModule Service - Currency", () => { code: "EUR", name: "Euro", }), + expect.objectContaining({ + code: "USD", + name: "US Dollar", + }), ]) }) @@ -79,10 +79,6 @@ describe("PricingModule Service - Currency", () => { expect(count).toEqual(3) expect(currenciesResult).toEqual([ - expect.objectContaining({ - code: "USD", - name: "US Dollar", - }), expect.objectContaining({ code: "CAD", name: "Canadian Dollar", @@ -91,6 +87,10 @@ describe("PricingModule Service - Currency", () => { code: "EUR", name: "Euro", }), + expect.objectContaining({ + code: "USD", + name: "US Dollar", + }), ]) }) @@ -117,8 +117,10 @@ describe("PricingModule Service - Currency", () => { expect(count).toEqual(3) expect(currenciesResult).toEqual([ expect.objectContaining({ - code: "CAD", - name: "Canadian Dollar", + code: "EUR", + name: "Euro", + symbol: "€", + symbol_native: "€", }), ]) }) @@ -137,7 +139,7 @@ describe("PricingModule Service - Currency", () => { expect(count).toEqual(3) expect(serialized).toEqual([ { - code: "USD", + code: "CAD", }, ]) }) diff --git a/packages/pricing/integration-tests/__tests__/services/pricing-module/money-amount.spec.ts b/packages/pricing/integration-tests/__tests__/services/pricing-module/money-amount.spec.ts index c1e47a73a9..6bec0199c1 100644 --- a/packages/pricing/integration-tests/__tests__/services/pricing-module/money-amount.spec.ts +++ b/packages/pricing/integration-tests/__tests__/services/pricing-module/money-amount.spec.ts @@ -201,7 +201,7 @@ describe("PricingModule Service - MoneyAmount", () => { expect(count).toEqual(3) expect(serialized).toEqual([ { - id: "money-amount-USD", + id: "money-amount-CAD", amount: null, }, ]) diff --git a/packages/pricing/integration-tests/__tests__/services/pricing-module/price-set-money-amount-rules.spec.ts b/packages/pricing/integration-tests/__tests__/services/pricing-module/price-set-money-amount-rules.spec.ts index db74001808..909399ce56 100644 --- a/packages/pricing/integration-tests/__tests__/services/pricing-module/price-set-money-amount-rules.spec.ts +++ b/packages/pricing/integration-tests/__tests__/services/pricing-module/price-set-money-amount-rules.spec.ts @@ -273,16 +273,14 @@ describe("PricingModule Service - PriceSetMoneyAmountRules", () => { }, ]) - const priceSetMoneyAmountRules = - await service.listPriceSetMoneyAmountRules( - {}, - { - relations: ["price_set_money_amount", "rule_type"], - } - ) - - const created = - priceSetMoneyAmountRules[priceSetMoneyAmountRules.length - 1] + const [created] = await service.listPriceSetMoneyAmountRules( + { + value: ["New priceSetMoneyAmountRule"], + }, + { + relations: ["price_set_money_amount", "rule_type"], + } + ) expect(created).toEqual( expect.objectContaining({ diff --git a/packages/pricing/src/services/__tests__/currency.spec.ts b/packages/pricing/src/services/__tests__/currency.spec.ts index 46cc0f789c..125d5c1a86 100644 --- a/packages/pricing/src/services/__tests__/currency.spec.ts +++ b/packages/pricing/src/services/__tests__/currency.spec.ts @@ -92,6 +92,9 @@ describe("Currency service", function () { fields: undefined, limit: 15, offset: 0, + orderBy: { + code: "ASC", + }, populate: [], withDeleted: undefined, }, @@ -130,6 +133,9 @@ describe("Currency service", function () { fields: undefined, limit: 15, offset: 0, + orderBy: { + code: "ASC", + }, populate: [], withDeleted: undefined, }, @@ -168,6 +174,9 @@ describe("Currency service", function () { fields: undefined, limit: 15, offset: 0, + orderBy: { + code: "ASC", + }, withDeleted: undefined, populate: ["tags"], }, @@ -206,6 +215,9 @@ describe("Currency service", function () { fields: undefined, limit: 15, offset: 0, + orderBy: { + code: "ASC", + }, withDeleted: undefined, populate: ["tags"], }, diff --git a/packages/product/src/services/__tests__/product.spec.ts b/packages/product/src/services/__tests__/product.spec.ts index fb1a6819d3..5d3a3e1707 100644 --- a/packages/product/src/services/__tests__/product.spec.ts +++ b/packages/product/src/services/__tests__/product.spec.ts @@ -91,6 +91,9 @@ describe("Product service", function () { fields: undefined, limit: 15, offset: 0, + orderBy: { + id: "ASC", + }, populate: [], withDeleted: undefined, }, @@ -129,6 +132,9 @@ describe("Product service", function () { fields: undefined, limit: 15, offset: 0, + orderBy: { + id: "ASC", + }, populate: [], withDeleted: undefined, }, @@ -167,6 +173,9 @@ describe("Product service", function () { fields: undefined, limit: 15, offset: 0, + orderBy: { + id: "ASC", + }, withDeleted: undefined, populate: ["tags"], }, @@ -205,6 +214,9 @@ describe("Product service", function () { fields: undefined, limit: 15, offset: 0, + orderBy: { + id: "ASC", + }, withDeleted: undefined, populate: ["tags"], }, diff --git a/packages/sales-channel/src/services/__tests__/sales-channle.spec.ts b/packages/sales-channel/src/services/__tests__/sales-channle.spec.ts index 61e5bb1a1f..22ec375ba1 100644 --- a/packages/sales-channel/src/services/__tests__/sales-channle.spec.ts +++ b/packages/sales-channel/src/services/__tests__/sales-channle.spec.ts @@ -34,6 +34,9 @@ describe("Sales channel service", function () { fields: ["id", "name"], limit: 15, offset: 0, + orderBy: { + id: "ASC", + }, withDeleted: undefined, populate: [], }, diff --git a/packages/utils/src/modules-sdk/__tests__/internal-module-service-factory.spec.ts b/packages/utils/src/modules-sdk/__tests__/internal-module-service-factory.spec.ts index b8825dd3e6..d5633294e3 100644 --- a/packages/utils/src/modules-sdk/__tests__/internal-module-service-factory.spec.ts +++ b/packages/utils/src/modules-sdk/__tests__/internal-module-service-factory.spec.ts @@ -3,7 +3,12 @@ import { lowerCaseFirst } from "../../common" const defaultContext = { __type: "MedusaContext" } -class Model {} +class Model { + static __meta = { + primaryKeys: ["id"], + } +} + describe("Internal Module Service Factory", () => { const modelRepositoryName = `${lowerCaseFirst(Model.name)}Repository` @@ -114,6 +119,34 @@ describe("Internal Module Service Factory", () => { expect(result).toEqual(entities) }) + it("should list entities and relation with default ordering successfully", async () => { + const entities = [ + { id: "1", name: "Item" }, + { id: "2", name: "Item2" }, + ] + containerMock[modelRepositoryName].find.mockResolvedValueOnce(entities) + + const result = await instance.list( + {}, + { + relations: ["relation"], + } + ) + + expect(result).toEqual(entities) + expect(containerMock[modelRepositoryName].find).toHaveBeenCalledWith( + expect.objectContaining({ + options: expect.objectContaining({ + populate: ["relation"], + orderBy: { + id: "ASC", + }, + }), + }), + expect.any(Object) + ) + }) + it("should list and count entities successfully", async () => { const entities = [ { id: "1", name: "Item" }, diff --git a/packages/utils/src/modules-sdk/internal-module-service-factory.ts b/packages/utils/src/modules-sdk/internal-module-service-factory.ts index 58af0431fc..d792d75992 100644 --- a/packages/utils/src/modules-sdk/internal-module-service-factory.ts +++ b/packages/utils/src/modules-sdk/internal-module-service-factory.ts @@ -63,6 +63,24 @@ export function internalModuleServiceFactory< return keys.map((k) => data[k]).join("_") } + /** + * Only apply top level default ordering as the relation + * default ordering is already applied through the foreign key + * @param config + */ + static applyDefaultOrdering(config: FindConfig) { + if (config.order) { + return + } + + config.order = {} + + const primaryKeys = AbstractService_.retrievePrimaryKeys(model) + primaryKeys.forEach((primaryKey) => { + config.order![primaryKey] = "ASC" + }) + } + @InjectManager(propertyRepositoryName) async retrieve( idOrObject: string | object, @@ -129,6 +147,7 @@ export function internalModuleServiceFactory< config: FindConfig = {}, @MedusaContext() sharedContext: Context = {} ): Promise { + AbstractService_.applyDefaultOrdering(config) const queryOptions = buildQuery(filters, config) return await this[propertyRepositoryName].find( @@ -143,6 +162,7 @@ export function internalModuleServiceFactory< config: FindConfig = {}, @MedusaContext() sharedContext: Context = {} ): Promise<[TEntity[], number]> { + AbstractService_.applyDefaultOrdering(config) const queryOptions = buildQuery(filters, config) return await this[propertyRepositoryName].findAndCount(