From 52e394055cbee1066aebf8e993689a2e49a32177 Mon Sep 17 00:00:00 2001 From: Adrien de Peretti Date: Wed, 28 Aug 2024 17:45:48 +0200 Subject: [PATCH] fix(utils): DML hasOne - belongsTo not behaving correctly (#8813) FIXES FRMW-2676 **What** ref: https://discord.com/channels/876835651130097704/1023889804544458752/threads/1276979858781503528 Currently, when providing the following ```ts const user1 = manager.create(User, { username: "User 1", team: { name: "Team 1", }, }) ``` It would result in an error inserting into the database because the foreign key will be sent twice as part of the insert, one for the relation and one for the foreign key that both relate to the foreign key property. To fix that and allow both approaches (providing the entity to cascade persist or just providing the foreign key in case of another side nullable relation) we need to handle it a bit differently. now both approaches would be valid. the entities for the example might not be the best ones but it is just to illustrate option 1 - we create both the user and the team: ```ts const user1 = manager.create(User, { username: "User 1", team: { name: "Team 1", }, }) ``` option 2 - the team already exists (for example the previous user have been detached from the team but we kept the team alive and assign a new user to that team) : ```ts const user1 = manager.create(User, { username: "User 1", team_id: team.id }) ``` --- .../src/dml/__tests__/entity-builder.spec.ts | 5 + .../entity-builder/define-relationship.ts | 33 +++- .../__tests__/has-one-belongs-to.spec.ts | 156 ++++++++++++++++++ 3 files changed, 186 insertions(+), 8 deletions(-) create mode 100644 packages/core/utils/src/dml/integration-tests/__tests__/has-one-belongs-to.spec.ts diff --git a/packages/core/utils/src/dml/__tests__/entity-builder.spec.ts b/packages/core/utils/src/dml/__tests__/entity-builder.spec.ts index 07125e7923..3a15cf391e 100644 --- a/packages/core/utils/src/dml/__tests__/entity-builder.spec.ts +++ b/packages/core/utils/src/dml/__tests__/entity-builder.spec.ts @@ -2361,6 +2361,7 @@ describe("Entity builder", () => { setter: false, type: "string", isForeignKey: true, + persist: false, }, created_at: { reference: "scalar", @@ -3242,6 +3243,7 @@ describe("Entity builder", () => { getter: false, setter: false, isForeignKey: true, + persist: false, }, created_at: { reference: "scalar", @@ -3422,6 +3424,7 @@ describe("Entity builder", () => { getter: false, setter: false, isForeignKey: true, + persist: false, }, created_at: { reference: "scalar", @@ -4026,6 +4029,7 @@ describe("Entity builder", () => { getter: false, setter: false, isForeignKey: true, + persist: false, }, created_at: { reference: "scalar", @@ -4214,6 +4218,7 @@ describe("Entity builder", () => { getter: false, setter: false, isForeignKey: true, + persist: false, }, created_at: { reference: "scalar", diff --git a/packages/core/utils/src/dml/helpers/entity-builder/define-relationship.ts b/packages/core/utils/src/dml/helpers/entity-builder/define-relationship.ts index 4337f1a96d..fe7fc23d97 100644 --- a/packages/core/utils/src/dml/helpers/entity-builder/define-relationship.ts +++ b/packages/core/utils/src/dml/helpers/entity-builder/define-relationship.ts @@ -13,6 +13,7 @@ import { OneToOne, OnInit, Property, + rel, } from "@mikro-orm/core" import { camelToSnakeCase, pluralize } from "../../../common" import { ForeignKey } from "../../../dal/mikro-orm/decorators/foreign-key" @@ -116,6 +117,23 @@ export function defineBelongsToRelationship( * Hook to handle foreign key assignation */ MikroORMEntity.prototype[hookName] = function () { + /** + * In case of has one relation, in order to be able to have both ways + * to associate a relation (through the relation or the foreign key) we need to handle it + * specifically + */ + if (HasOne.isHasOne(otherSideRelation)) { + const relationMeta = this.__meta.relations.find( + (relation) => relation.name === relationship.name + ).targetMeta + this[relationship.name] ??= rel( + relationMeta.class, + this[foreignKeyName] + ) + this[relationship.name] ??= this[relationship.name]?.id + return + } + this[relationship.name] ??= this[foreignKeyName] this[foreignKeyName] ??= this[relationship.name]?.id } @@ -179,19 +197,18 @@ export function defineBelongsToRelationship( onDelete: shouldCascade ? "cascade" : undefined, })(MikroORMEntity.prototype, relationship.name) - if (relationship.nullable) { - Object.defineProperty(MikroORMEntity.prototype, foreignKeyName, { - value: null, - configurable: true, - enumerable: true, - writable: true, - }) - } + Object.defineProperty(MikroORMEntity.prototype, foreignKeyName, { + value: null, + configurable: true, + enumerable: true, + writable: true, + }) Property({ type: "string", columnType: "text", nullable: relationship.nullable, + persist: false, })(MikroORMEntity.prototype, foreignKeyName) ForeignKey()(MikroORMEntity.prototype, foreignKeyName) diff --git a/packages/core/utils/src/dml/integration-tests/__tests__/has-one-belongs-to.spec.ts b/packages/core/utils/src/dml/integration-tests/__tests__/has-one-belongs-to.spec.ts new file mode 100644 index 0000000000..4e9682c474 --- /dev/null +++ b/packages/core/utils/src/dml/integration-tests/__tests__/has-one-belongs-to.spec.ts @@ -0,0 +1,156 @@ +import { MetadataStorage, MikroORM } from "@mikro-orm/core" +import { model } from "../../entity-builder" +import { toMikroOrmEntities } from "../../helpers/create-mikro-orm-entity" +import { createDatabase, dropDatabase } from "pg-god" +import { CustomTsMigrationGenerator, mikroOrmSerializer } from "../../../dal" +import { EntityConstructor } from "@medusajs/types" +import { pgGodCredentials } from "../utils" +import { FileSystem } from "../../../common" +import { join } from "path" + +export const fileSystem = new FileSystem( + join(__dirname, "../../integration-tests-migrations-has-one-belongs-to") +) + +describe("hasOne - belongTo", () => { + const dbName = "EntityBuilder-HasOneBelongsTo" + + let orm!: MikroORM + let Team: EntityConstructor, User: EntityConstructor + + afterAll(async () => { + await fileSystem.cleanup() + }) + + beforeEach(async () => { + MetadataStorage.clear() + + const team = model.define("team", { + id: model.id().primaryKey(), + name: model.text(), + user: model.belongsTo(() => user, { mappedBy: "team" }), + }) + + const user = model.define("user", { + id: model.id().primaryKey(), + username: model.text(), + team: model.hasOne(() => team, { mappedBy: "user" }).nullable(), + }) + + ;[User, Team] = toMikroOrmEntities([user, team]) + + await createDatabase({ databaseName: dbName }, pgGodCredentials) + + orm = await MikroORM.init({ + entities: [Team, User], + tsNode: true, + dbName, + password: pgGodCredentials.password, + host: pgGodCredentials.host, + user: pgGodCredentials.user, + type: "postgresql", + migrations: { + generator: CustomTsMigrationGenerator, + path: fileSystem.basePath, + }, + }) + + const migrator = orm.getMigrator() + await migrator.createMigration() + await migrator.up() + }) + + afterEach(async () => { + await orm.close() + + await dropDatabase( + { databaseName: dbName, errorIfNonExist: false }, + pgGodCredentials + ) + }) + + it(`should handle the relation properly`, async () => { + let manager = orm.em.fork() + + const user1 = manager.create(User, { + username: "User 1", + team: { + name: "Team 1", + }, + }) + + await manager.persistAndFlush([user1]) + manager = orm.em.fork() + + const user = await manager.findOne( + User, + { + id: user1.id, + }, + { + populate: ["team"], + } + ) + + expect(mikroOrmSerializer>(user)).toEqual({ + id: user1.id, + username: "User 1", + created_at: expect.any(Date), + updated_at: expect.any(Date), + deleted_at: null, + team: { + id: expect.any(String), + name: "Team 1", + created_at: expect.any(Date), + updated_at: expect.any(Date), + deleted_at: null, + user_id: user1.id, + }, + }) + }) + + it(`should handle the relation properly with just the relation id`, async () => { + let manager = orm.em.fork() + + const user1 = manager.create(User, { + username: "User 1", + }) + + await manager.persistAndFlush([user1]) + manager = orm.em.fork() + + const team1 = manager.create(Team, { + name: "Team 1", + user_id: user1.id, + }) + + await manager.persistAndFlush([team1]) + manager = orm.em.fork() + + const user = await manager.findOne( + User, + { + id: user1.id, + }, + { + populate: ["team"], + } + ) + + expect(mikroOrmSerializer>(user)).toEqual({ + id: user1.id, + username: "User 1", + created_at: expect.any(Date), + updated_at: expect.any(Date), + deleted_at: null, + team: { + id: expect.any(String), + name: "Team 1", + created_at: expect.any(Date), + updated_at: expect.any(Date), + deleted_at: null, + user_id: user1.id, + }, + }) + }) +})