From 27150959ffa70819cf3c4ad176288a1c70287942 Mon Sep 17 00:00:00 2001 From: Oliver Windall Juhl <59018053+olivermrbl@users.noreply.github.com> Date: Fri, 10 Sep 2021 16:03:44 +0200 Subject: [PATCH] fix: Ensure uniqueness for products, variants, collections and discounts (#382) --- .../api/__tests__/admin/discount.js | 172 ++++++++++-------- .../api/__tests__/admin/product.js | 83 ++++++++- .../1631261634964-enforce_uniqueness.ts | 41 +++++ packages/medusa/src/models/discount.ts | 2 +- .../medusa/src/models/product-collection.ts | 2 +- packages/medusa/src/models/product-variant.ts | 10 +- packages/medusa/src/models/product.ts | 2 +- packages/medusa/src/utils/db-aware-column.ts | 18 +- 8 files changed, 241 insertions(+), 89 deletions(-) create mode 100644 packages/medusa/src/migrations/1631261634964-enforce_uniqueness.ts diff --git a/integration-tests/api/__tests__/admin/discount.js b/integration-tests/api/__tests__/admin/discount.js index 44bf7f0a1e..a731cec416 100644 --- a/integration-tests/api/__tests__/admin/discount.js +++ b/integration-tests/api/__tests__/admin/discount.js @@ -1,46 +1,46 @@ -const path = require("path"); -const { Region, DiscountRule, Discount } = require("@medusajs/medusa"); +const path = require("path") +const { Region, DiscountRule, Discount } = require("@medusajs/medusa") -const setupServer = require("../../../helpers/setup-server"); -const { useApi } = require("../../../helpers/use-api"); -const { initDb, useDb } = require("../../../helpers/use-db"); -const adminSeeder = require("../../helpers/admin-seeder"); +const setupServer = require("../../../helpers/setup-server") +const { useApi } = require("../../../helpers/use-api") +const { initDb, useDb } = require("../../../helpers/use-db") +const adminSeeder = require("../../helpers/admin-seeder") -jest.setTimeout(30000); +jest.setTimeout(30000) describe("/admin/discounts", () => { - let medusaProcess; - let dbConnection; + let medusaProcess + let dbConnection beforeAll(async () => { - const cwd = path.resolve(path.join(__dirname, "..", "..")); - dbConnection = await initDb({ cwd }); - medusaProcess = await setupServer({ cwd }); - }); + const cwd = path.resolve(path.join(__dirname, "..", "..")) + dbConnection = await initDb({ cwd }) + medusaProcess = await setupServer({ cwd }) + }) afterAll(async () => { - const db = useDb(); - await db.shutdown(); - medusaProcess.kill(); - }); + const db = useDb() + await db.shutdown() + medusaProcess.kill() + }) describe("POST /admin/discounts", () => { beforeEach(async () => { try { - await adminSeeder(dbConnection); + await adminSeeder(dbConnection) } catch (err) { - console.log(err); - throw err; + console.log(err) + throw err } - }); + }) afterEach(async () => { - const db = useDb(); - await db.teardown(); - }); + const db = useDb() + await db.teardown() + }) it("creates a discount and updates it", async () => { - const api = useApi(); + const api = useApi() const response = await api .post( @@ -62,16 +62,16 @@ describe("/admin/discounts", () => { } ) .catch((err) => { - console.log(err); - }); + console.log(err) + }) - expect(response.status).toEqual(200); + expect(response.status).toEqual(200) expect(response.data.discount).toEqual( expect.objectContaining({ code: "HELLOWORLD", usage_limit: 10, }) - ); + ) const updated = await api .post( @@ -86,51 +86,51 @@ describe("/admin/discounts", () => { } ) .catch((err) => { - console.log(err); - }); + console.log(err) + }) - expect(updated.status).toEqual(200); + expect(updated.status).toEqual(200) expect(updated.data.discount).toEqual( expect.objectContaining({ code: "HELLOWORLD", usage_limit: 20, }) - ); - }); - }); + ) + }) + }) describe("testing for soft-deletion + uniqueness on discount codes", () => { - let manager; + let manager beforeEach(async () => { - manager = dbConnection.manager; + manager = dbConnection.manager try { - await adminSeeder(dbConnection); + await adminSeeder(dbConnection) await manager.insert(DiscountRule, { id: "test-discount-rule", description: "Test discount rule", type: "percentage", value: 10, allocation: "total", - }); + }) await manager.insert(Discount, { id: "test-discount", code: "TESTING", rule_id: "test-discount-rule", is_dynamic: false, is_disabled: false, - }); + }) } catch (err) { - throw err; + throw err } - }); + }) afterEach(async () => { - const db = useDb(); - await db.teardown(); - }); + const db = useDb() + await db.teardown() + }) it("successfully creates discount with soft-deleted discount code", async () => { - const api = useApi(); + const api = useApi() // First we soft-delete the discount await api @@ -140,8 +140,8 @@ describe("/admin/discounts", () => { }, }) .catch((err) => { - console.log(err); - }); + console.log(err) + }) // Lets try to create a discount with same code as deleted one const response = await api @@ -164,51 +164,81 @@ describe("/admin/discounts", () => { } ) .catch((err) => { - console.log(err); - }); + console.log(err) + }) - expect(response.status).toEqual(200); + expect(response.status).toEqual(200) expect(response.data.discount).toEqual( expect.objectContaining({ code: "TESTING", usage_limit: 10, }) - ); - }); - }); + ) + }) + + it("should fails when creating a discount with already existing code", async () => { + const api = useApi() + + // Lets try to create a discount with same code as deleted one + try { + await api.post( + "/admin/discounts", + { + code: "TESTING", + rule: { + description: "test", + type: "percentage", + value: 10, + allocation: "total", + }, + usage_limit: 10, + }, + { + headers: { + Authorization: "Bearer test_token", + }, + } + ) + } catch (error) { + expect(error.response.data.message).toMatch( + /duplicate key value violates unique constraint/i + ) + } + }) + }) describe("POST /admin/discounts/:discount_id/dynamic-codes", () => { beforeEach(async () => { - const manager = dbConnection.manager; + const manager = dbConnection.manager try { - await adminSeeder(dbConnection); + await adminSeeder(dbConnection) await manager.insert(DiscountRule, { id: "test-discount-rule", description: "Dynamic rule", type: "percentage", value: 10, allocation: "total", - }); + }) await manager.insert(Discount, { id: "test-discount", code: "DYNAMIC", is_dynamic: true, is_disabled: false, rule_id: "test-discount-rule", - }); + }) } catch (err) { - console.log(err); - throw err; + console.log(err) + throw err } - }); + }) afterEach(async () => { - const db = useDb(); - await db.teardown(); - }); + const db = useDb() + await db.teardown() + }) it("creates a dynamic discount", async () => { - const api = useApi(); + const api = useApi() const response = await api .post( @@ -223,10 +253,10 @@ describe("/admin/discounts", () => { } ) .catch((err) => { - console.log(err); - }); + console.log(err) + }) - expect(response.status).toEqual(200); - }); - }); -}); + expect(response.status).toEqual(200) + }) + }) +}) diff --git a/integration-tests/api/__tests__/admin/product.js b/integration-tests/api/__tests__/admin/product.js index 8d43c2aac2..9c17d99cf1 100644 --- a/integration-tests/api/__tests__/admin/product.js +++ b/integration-tests/api/__tests__/admin/product.js @@ -7,7 +7,7 @@ const { initDb, useDb } = require("../../../helpers/use-db") const adminSeeder = require("../../helpers/admin-seeder") const productSeeder = require("../../helpers/product-seeder") -jest.setTimeout(30000) +jest.setTimeout(50000) describe("/admin/products", () => { let medusaProcess @@ -263,7 +263,7 @@ describe("/admin/products", () => { const api = useApi() const payload = { - title: "Test product", + title: "Test", description: "test-product-description", type: { value: "test-type" }, images: ["test-image.png", "test-image-2.png"], @@ -293,10 +293,10 @@ describe("/admin/products", () => { expect(response.status).toEqual(200) expect(response.data.product).toEqual( expect.objectContaining({ - title: "Test product", + title: "Test", discountable: true, is_giftcard: false, - handle: "test-product", + handle: "test", images: expect.arrayContaining([ expect.objectContaining({ url: "test-image.png", @@ -646,7 +646,7 @@ describe("/admin/products", () => { ) }) - it("successfully creates product with soft-deleted product handle", async () => { + it("successfully creates product with soft-deleted product handle and deletes it again", async () => { const api = useApi() // First we soft-delete the product @@ -691,6 +691,56 @@ describe("/admin/products", () => { expect(res.status).toEqual(200) expect(res.data.product.handle).toEqual("test-product") + + // Delete product again to ensure uniqueness is enforced in all cases + const response2 = await api + .delete("/admin/products/test-product", { + headers: { + Authorization: "Bearer test_token", + }, + }) + .catch((err) => { + console.log(err) + }) + + expect(response2.status).toEqual(200) + expect(response2.data.id).toEqual("test-product") + }) + + it("should fail when creating a product with a handle that already exists", async () => { + const api = useApi() + + // Lets try to create a product with same handle as deleted one + const payload = { + title: "Test product", + handle: "test-product", + description: "test-product-description", + type: { value: "test-type" }, + images: ["test-image.png", "test-image-2.png"], + collection_id: "test-collection", + tags: [{ value: "123" }, { value: "456" }], + options: [{ title: "size" }, { title: "color" }], + variants: [ + { + title: "Test variant", + inventory_quantity: 10, + prices: [{ currency_code: "usd", amount: 100 }], + options: [{ value: "large" }, { value: "green" }], + }, + ], + } + + try { + await api.post("/admin/products", payload, { + headers: { + Authorization: "Bearer test_token", + }, + }) + } catch (error) { + expect(error.response.data.message).toMatch( + /duplicate key value violates unique constraint/i + ) + } }) it("successfully deletes product collection", async () => { @@ -743,6 +793,28 @@ describe("/admin/products", () => { expect(res.data.collection.handle).toEqual("test-collection") }) + it("should fail when creating a collection with a handle that already exists", async () => { + const api = useApi() + + // Lets try to create a collection with same handle as deleted one + const payload = { + title: "Another test collection", + handle: "test-collection", + } + + try { + await api.post("/admin/collections", payload, { + headers: { + Authorization: "Bearer test_token", + }, + }) + } catch (error) { + expect(error.response.data.message).toMatch( + /duplicate key value violates unique constraint/i + ) + } + }) + it("successfully creates soft-deleted product variant", async () => { const api = useApi() @@ -769,7 +841,6 @@ describe("/admin/products", () => { expect(response.status).toEqual(200) expect(response.data.variant_id).toEqual("test-variant") - // Lets try to create a product collection with same handle as deleted one const payload = { title: "Second variant", sku: "test-sku", diff --git a/packages/medusa/src/migrations/1631261634964-enforce_uniqueness.ts b/packages/medusa/src/migrations/1631261634964-enforce_uniqueness.ts new file mode 100644 index 0000000000..1ec7b44c5c --- /dev/null +++ b/packages/medusa/src/migrations/1631261634964-enforce_uniqueness.ts @@ -0,0 +1,41 @@ +import {MigrationInterface, QueryRunner} from "typeorm"; + +export class enforceUniqueness1631261634964 implements MigrationInterface { + name = 'enforceUniqueness1631261634964' + + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query(`DROP INDEX "IDX_e08af711f3493df1e921c4c9ef"`); + await queryRunner.query(`DROP INDEX "IDX_77c4073c30ea7793f484750529"`); + await queryRunner.query(`DROP INDEX "IDX_0683952543d7d3f4fffc427034"`); + await queryRunner.query(`DROP INDEX "IDX_410649600ce31c10c4b667ca10"`); + await queryRunner.query(`DROP INDEX "IDX_5248fda27b9f16ef818604bb6f"`); + await queryRunner.query(`DROP INDEX "IDX_832f86daf8103491d634a967da"`); + await queryRunner.query(`DROP INDEX "IDX_ae3e22c67d7c7a969a363533c0"`); + await queryRunner.query(`CREATE UNIQUE INDEX "IDX_6f234f058bbbea810dce1d04d0" ON "product_collection" ("handle") WHERE deleted_at IS NULL`); + await queryRunner.query(`CREATE UNIQUE INDEX "IDX_cf9cc6c3f2e6414b992223fff1" ON "product" ("handle") WHERE deleted_at IS NULL`); + await queryRunner.query(`CREATE UNIQUE INDEX "IDX_2ca8cfbdafb998ecfd6d340389" ON "product_variant" ("sku") WHERE deleted_at IS NULL`); + await queryRunner.query(`CREATE UNIQUE INDEX "IDX_045d4a149c09f4704e0bc08dd4" ON "product_variant" ("barcode") WHERE deleted_at IS NULL`); + await queryRunner.query(`CREATE UNIQUE INDEX "IDX_b5b6225539ee8501082fbc0714" ON "product_variant" ("ean") WHERE deleted_at IS NULL`); + await queryRunner.query(`CREATE UNIQUE INDEX "IDX_aa16f61348be02dd07ce3fc54e" ON "product_variant" ("upc") WHERE deleted_at IS NULL`); + await queryRunner.query(`CREATE UNIQUE INDEX "IDX_f65bf52e2239ace276ece2b2f4" ON "discount" ("code") WHERE deleted_at IS NULL`); + } + + public async down(queryRunner: QueryRunner): Promise { + + await queryRunner.query(`DROP INDEX "IDX_f65bf52e2239ace276ece2b2f4"`); + await queryRunner.query(`DROP INDEX "IDX_aa16f61348be02dd07ce3fc54e"`); + await queryRunner.query(`DROP INDEX "IDX_b5b6225539ee8501082fbc0714"`); + await queryRunner.query(`DROP INDEX "IDX_045d4a149c09f4704e0bc08dd4"`); + await queryRunner.query(`DROP INDEX "IDX_2ca8cfbdafb998ecfd6d340389"`); + await queryRunner.query(`DROP INDEX "IDX_cf9cc6c3f2e6414b992223fff1"`); + await queryRunner.query(`DROP INDEX "IDX_6f234f058bbbea810dce1d04d0"`); + await queryRunner.query(`CREATE UNIQUE INDEX "IDX_ae3e22c67d7c7a969a363533c0" ON "discount" ("code") WHERE (deleted_at IS NOT NULL)`); + await queryRunner.query(`CREATE UNIQUE INDEX "IDX_832f86daf8103491d634a967da" ON "product_variant" ("upc") WHERE (deleted_at IS NOT NULL)`); + await queryRunner.query(`CREATE UNIQUE INDEX "IDX_5248fda27b9f16ef818604bb6f" ON "product_variant" ("ean") WHERE (deleted_at IS NOT NULL)`); + await queryRunner.query(`CREATE UNIQUE INDEX "IDX_410649600ce31c10c4b667ca10" ON "product_variant" ("barcode") WHERE (deleted_at IS NOT NULL)`); + await queryRunner.query(`CREATE UNIQUE INDEX "IDX_0683952543d7d3f4fffc427034" ON "product_variant" ("sku") WHERE (deleted_at IS NOT NULL)`); + await queryRunner.query(`CREATE UNIQUE INDEX "IDX_77c4073c30ea7793f484750529" ON "product" ("handle") WHERE (deleted_at IS NOT NULL)`); + await queryRunner.query(`CREATE UNIQUE INDEX "IDX_e08af711f3493df1e921c4c9ef" ON "product_collection" ("handle") WHERE (deleted_at IS NOT NULL)`); + } + +} diff --git a/packages/medusa/src/models/discount.ts b/packages/medusa/src/models/discount.ts index 34c033a4df..708fdb9110 100644 --- a/packages/medusa/src/models/discount.ts +++ b/packages/medusa/src/models/discount.ts @@ -24,7 +24,7 @@ export class Discount { @PrimaryColumn() id: string - @Index({ unique: true, where: "deleted_at IS NOT NULL" }) + @Index({ unique: true, where: "deleted_at IS NULL" }) @Column() code: string diff --git a/packages/medusa/src/models/product-collection.ts b/packages/medusa/src/models/product-collection.ts index e0fc3c72a2..d161c5e06c 100644 --- a/packages/medusa/src/models/product-collection.ts +++ b/packages/medusa/src/models/product-collection.ts @@ -24,7 +24,7 @@ export class ProductCollection { @Column() title: string - @Index({ unique: true, where: "deleted_at IS NOT NULL" }) + @Index({ unique: true, where: "deleted_at IS NULL" }) @Column({ nullable: true }) handle: string diff --git a/packages/medusa/src/models/product-variant.ts b/packages/medusa/src/models/product-variant.ts index 94c10fa104..1538575d33 100644 --- a/packages/medusa/src/models/product-variant.ts +++ b/packages/medusa/src/models/product-variant.ts @@ -48,22 +48,22 @@ export class ProductVariant { prices: MoneyAmount[] @Column({ nullable: true }) - @Index({ unique: true, where: "deleted_at IS NOT NULL" }) + @Index({ unique: true, where: "deleted_at IS NULL" }) sku: string @Column({ nullable: true }) - @Index({ unique: true, where: "deleted_at IS NOT NULL" }) + @Index({ unique: true, where: "deleted_at IS NULL" }) barcode: string @Column({ nullable: true }) - @Index({ unique: true, where: "deleted_at IS NOT NULL" }) + @Index({ unique: true, where: "deleted_at IS NULL" }) ean: string @Column({ nullable: true }) - @Index({ unique: true, where: "deleted_at IS NOT NULL" }) + @Index({ unique: true, where: "deleted_at IS NULL" }) upc: string - @Column({ nullable: true, default: 0, select:false }) + @Column({ nullable: true, default: 0, select: false }) variant_rank: number @Column({ type: "int" }) diff --git a/packages/medusa/src/models/product.ts b/packages/medusa/src/models/product.ts index 506e7ebc8a..c1ff8d713f 100644 --- a/packages/medusa/src/models/product.ts +++ b/packages/medusa/src/models/product.ts @@ -40,7 +40,7 @@ export class Product { @Column({ nullable: true }) description: string - @Index({ unique: true, where: "deleted_at IS NOT NULL" }) + @Index({ unique: true, where: "deleted_at IS NULL" }) @Column({ nullable: true }) handle: string diff --git a/packages/medusa/src/utils/db-aware-column.ts b/packages/medusa/src/utils/db-aware-column.ts index ea6023286f..216aa60a6e 100644 --- a/packages/medusa/src/utils/db-aware-column.ts +++ b/packages/medusa/src/utils/db-aware-column.ts @@ -18,8 +18,13 @@ const pgSqliteGenerationMapping: { let dbType: string export function resolveDbType(pgSqlType: ColumnType): ColumnType { if (!dbType) { - const { configModule } = getConfigFile(path.resolve("."), `medusa-config`) - dbType = configModule.projectConfig.database_type + try { + const { configModule } = getConfigFile(path.resolve("."), `medusa-config`) + dbType = configModule.projectConfig.database_type + } catch (error) { + // Default to Postgres to allow for e.g. migrations to run + dbType = "postgres" + } } if (dbType === "sqlite" && pgSqlType in pgSqliteTypeMapping) { @@ -32,8 +37,13 @@ export function resolveDbGenerationStrategy( pgSqlType: "increment" | "uuid" | "rowid" ): "increment" | "uuid" | "rowid" { if (!dbType) { - const { configModule } = getConfigFile(path.resolve("."), `medusa-config`) - dbType = configModule.projectConfig.database_type + try { + const { configModule } = getConfigFile(path.resolve("."), `medusa-config`) + dbType = configModule.projectConfig.database_type + } catch (error) { + // Default to Postgres to allow for e.g. migrations to run + dbType = "postgres" + } } if (dbType === "sqlite" && pgSqlType in pgSqliteTypeMapping) {