diff --git a/.changeset/cold-dragons-lie.md b/.changeset/cold-dragons-lie.md new file mode 100644 index 0000000000..75752a60d4 --- /dev/null +++ b/.changeset/cold-dragons-lie.md @@ -0,0 +1,5 @@ +--- +"@medusajs/utils": patch +--- + +fix: do not rely on model loading order to find an implicit owner 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 144d559007..31fb8ee1ad 100644 --- a/packages/core/utils/src/dml/__tests__/entity-builder.spec.ts +++ b/packages/core/utils/src/dml/__tests__/entity-builder.spec.ts @@ -5149,9 +5149,9 @@ describe("Entity builder", () => { reference: "m:n", name: "teams", entity: "Team", - owner: true, + owner: false, pivotTable: "team_users", - inversedBy: "users", + mappedBy: "users", }, created_at: { reference: "scalar", @@ -5217,9 +5217,9 @@ describe("Entity builder", () => { users: { reference: "m:n", name: "users", - mappedBy: "teams", + inversedBy: "teams", entity: "User", - owner: false, + owner: true, pivotTable: "team_users", }, created_at: { @@ -5330,9 +5330,9 @@ describe("Entity builder", () => { reference: "m:n", name: "teams", entity: "Team", - owner: true, + owner: false, pivotTable: "team_users", - inversedBy: "users", + mappedBy: "users", }, created_at: { reference: "scalar", @@ -5399,8 +5399,8 @@ describe("Entity builder", () => { reference: "m:n", name: "users", entity: "User", - owner: false, - mappedBy: "teams", + owner: true, + inversedBy: "teams", pivotTable: "team_users", }, created_at: { @@ -5545,9 +5545,9 @@ describe("Entity builder", () => { reference: "m:n", name: "teams", entity: "Team", - owner: true, + owner: false, pivotTable: "team_users", - inversedBy: "users", + mappedBy: "users", }, created_at: { reference: "scalar", @@ -5614,9 +5614,9 @@ describe("Entity builder", () => { reference: "m:n", name: "users", entity: "User", - owner: false, + owner: true, pivotTable: "team_users", - mappedBy: "teams", + inversedBy: "teams", }, created_at: { reference: "scalar", @@ -6125,9 +6125,9 @@ describe("Entity builder", () => { reference: "m:n", name: "teams", entity: "Team", - owner: true, + owner: false, pivotTable: "platform.team_users", - inversedBy: "users", + mappedBy: "users", }, created_at: { reference: "scalar", @@ -6195,8 +6195,8 @@ describe("Entity builder", () => { reference: "m:n", name: "users", entity: "User", - owner: false, - mappedBy: "teams", + owner: true, + inversedBy: "teams", pivotTable: "platform.team_users", }, created_at: { @@ -6719,10 +6719,10 @@ describe("Entity builder", () => { }, }) - const metaData = MetadataStorage.getMetadataFromDecorator(User) - expect(metaData.className).toEqual("User") - expect(metaData.path).toEqual("User") - expect(metaData.properties).toEqual({ + const teamMetaData = MetadataStorage.getMetadataFromDecorator(Team) + expect(teamMetaData.className).toEqual("Team") + expect(teamMetaData.path).toEqual("Team") + expect(teamMetaData.properties).toEqual({ id: { reference: "scalar", type: "number", @@ -6733,23 +6733,23 @@ describe("Entity builder", () => { getter: false, setter: false, }, - username: { + name: { reference: "scalar", type: "string", columnType: "text", - name: "username", - fieldName: "username", + name: "name", + fieldName: "name", nullable: false, getter: false, setter: false, }, - teams: { + users: { reference: "m:n", - name: "teams", - entity: "Team", + name: "users", + entity: "User", owner: true, + inversedBy: "teams", pivotEntity: "TeamUsers", - inversedBy: "users", }, created_at: { reference: "scalar", @@ -6788,10 +6788,10 @@ describe("Entity builder", () => { }, }) - const teamMetaData = MetadataStorage.getMetadataFromDecorator(Team) - expect(teamMetaData.className).toEqual("Team") - expect(teamMetaData.path).toEqual("Team") - expect(teamMetaData.properties).toEqual({ + const metaData = MetadataStorage.getMetadataFromDecorator(User) + expect(metaData.className).toEqual("User") + expect(metaData.path).toEqual("User") + expect(metaData.properties).toEqual({ id: { reference: "scalar", type: "number", @@ -6802,23 +6802,23 @@ describe("Entity builder", () => { getter: false, setter: false, }, - name: { + username: { reference: "scalar", type: "string", columnType: "text", - name: "name", - fieldName: "name", + name: "username", + fieldName: "username", nullable: false, getter: false, setter: false, }, - users: { + teams: { reference: "m:n", - name: "users", - entity: "User", + name: "teams", + entity: "Team", owner: false, - mappedBy: "teams", pivotEntity: "TeamUsers", + mappedBy: "users", }, created_at: { reference: "scalar", @@ -6857,5 +6857,28 @@ describe("Entity builder", () => { }, }) }) + + test("throw error when both sides of relationship defines the pivot table", () => { + const team = model.define("team", { + id: model.number(), + name: model.text(), + users: model.manyToMany(() => user, { + pivotTable: "user_teams", + }), + }) + + const user = model.define("user", { + id: model.number(), + username: model.text(), + teams: model.manyToMany(() => team, { + pivotTable: "team_users", + mappedBy: "users", + }), + }) + + expect(() => toMikroORMEntity(user)).toThrow( + `Invalid relationship reference for "User.teams". Define "pivotTable", "joinColumn", or "inverseJoinColumn" on only one side of the relationship` + ) + }) }) }) 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 8a9afb5f8d..10f8a79823 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 @@ -15,13 +15,13 @@ import { Property, rel, } from "@mikro-orm/core" -import { camelToSnakeCase, pluralize } from "../../../common" import { DmlEntity } from "../../entity" -import { HasMany } from "../../relations/has-many" import { HasOne } from "../../relations/has-one" -import { ManyToMany as DmlManyToMany } from "../../relations/many-to-many" -import { applyEntityIndexes } from "../mikro-orm/apply-indexes" +import { HasMany } from "../../relations/has-many" import { parseEntityName } from "./parse-entity-name" +import { camelToSnakeCase, pluralize } from "../../../common" +import { applyEntityIndexes } from "../mikro-orm/apply-indexes" +import { ManyToMany as DmlManyToMany } from "../../relations/many-to-many" type Context = { MANY_TO_MANY_TRACKED_RELATIONS: Record @@ -375,12 +375,10 @@ export function defineManyToManyRelationship( >, { relatedModelName, - relatedTableName, pgSchema, }: { relatedModelName: string pgSchema: string | undefined - relatedTableName: string }, { MANY_TO_MANY_TRACKED_RELATIONS }: Context ) { @@ -450,11 +448,12 @@ export function defineManyToManyRelationship( pivotEntityName = parseEntityName(pivotEntity).modelName } - if (!pivotEntityName) { - const { tableName } = parseEntityName(entity) - let tableNameWithoutSchema: string - let relatedTableNameWithoutSchema: string + const tableName = parseEntityName(entity).tableNameWithoutSchema + const relatedTableName = parseEntityName(relatedEntity).tableNameWithoutSchema + const sortedTableNames = [tableName, relatedTableName].sort() + const otherSideRelationOptions = otherSideRelationship.parse("").options + if (!pivotEntityName) { /** * Pivot table name is created as follows (when not explicitly provided) * @@ -464,25 +463,10 @@ export function defineManyToManyRelationship( * - And finally pluralizing the second entity name. */ - let [schema, ...tableTokens] = tableName.split(".") - if (!tableTokens.length) { - tableNameWithoutSchema = schema - } else { - tableNameWithoutSchema = tableTokens.join(".") - } - - const [relatedSchema, ...relatedTableTokens] = relatedTableName.split(".") - if (!relatedTableTokens.length) { - relatedTableNameWithoutSchema = relatedSchema - } else { - relatedTableNameWithoutSchema = relatedTableTokens.join(".") - } - pivotTableName = relationship.options.pivotTable ?? otherSideRelationship.parse("").options.pivotTable ?? - [tableNameWithoutSchema, relatedTableNameWithoutSchema] - .sort() + sortedTableNames .map((token, index) => { if (index === 1) { return pluralize(token) @@ -492,22 +476,51 @@ export function defineManyToManyRelationship( .join("_") } - const otherSideRelationOptions = otherSideRelationship.parse("").options + let isOwner: boolean | undefined = undefined - const isOwner = - !!joinColumn || - !!inverseJoinColumn || - !!relationship.options.pivotTable || - /** - * We can't infer it from the current entity so lets - * look at the otherside configuration as well to make a choice - */ - (!otherSideRelationOptions.pivotTable && - !otherSideRelationOptions.joinColumn && - !otherSideRelationOptions.inverseJoinColumn && - !MANY_TO_MANY_TRACKED_RELATIONS[ - `${relatedModelName}.${otherSideRelationshipProperty}` - ]) + const configuresRelationship = !!( + joinColumn || + inverseJoinColumn || + relationship.options.pivotTable + ) + const relatedOneConfiguresRelationship = !!( + otherSideRelationOptions.pivotTable || + otherSideRelationOptions.joinColumn || + otherSideRelationOptions.inverseJoinColumn + ) + + /** + * Both sides are configuring the properties that must be on one + * side only + */ + if (configuresRelationship && relatedOneConfiguresRelationship) { + throw new Error( + `Invalid relationship reference for "${MikroORMEntity.name}.${relationship.name}". Define "pivotTable", "joinColumn", or "inverseJoinColumn" on only one side of the relationship` + ) + } + + /** + * If any of the following properties are provided, we consider + * the current side to be the owner + */ + if (configuresRelationship) { + isOwner = true + } + + /** + * If any of the properties are provided on the other side, + * then we do not expect the current side to be the owner + */ + if (isOwner === undefined && relatedOneConfiguresRelationship) { + isOwner = false + } + + /** + * Finally, we consider the current side as owner, if it is + * the first one in alphabetical order. The same logic is + * applied to pivot table name as well. + */ + isOwner ??= sortedTableNames[0] === tableName const mappedByProp = isOwner ? "inversedBy" : "mappedBy" const mappedByPropValue = diff --git a/packages/core/utils/src/dml/helpers/entity-builder/parse-entity-name.ts b/packages/core/utils/src/dml/helpers/entity-builder/parse-entity-name.ts index 1ec8f4e49f..133d55315b 100644 --- a/packages/core/utils/src/dml/helpers/entity-builder/parse-entity-name.ts +++ b/packages/core/utils/src/dml/helpers/entity-builder/parse-entity-name.ts @@ -25,5 +25,6 @@ export function parseEntityName(entity: DmlEntity) { tableName, modelName: upperCaseFirst(toCamelCase(parsedEntity.name)), pgSchema: rest.length ? pgSchema : undefined, + tableNameWithoutSchema: rest.length ? rest.join(".") : pgSchema, } } diff --git a/packages/core/utils/src/dml/integration-tests/__tests__/many-to-many.spec.ts b/packages/core/utils/src/dml/integration-tests/__tests__/many-to-many.spec.ts index cfece63b5c..bb2731a9d4 100644 --- a/packages/core/utils/src/dml/integration-tests/__tests__/many-to-many.spec.ts +++ b/packages/core/utils/src/dml/integration-tests/__tests__/many-to-many.spec.ts @@ -217,13 +217,13 @@ describe("manyToMany - manyToMany", () => { ;[User, Team] = toMikroOrmEntities([user, team]) const teamMetaData = MetadataStorage.getMetadataFromDecorator(Team) - expect((teamMetaData.properties as any).users.mappedBy).toBe("squads") - expect((teamMetaData.properties as any).users.owner).toBe(false) + expect((teamMetaData.properties as any).users.inversedBy).toBe("squads") + expect((teamMetaData.properties as any).users.owner).toBe(true) const userMetaData = MetadataStorage.getMetadataFromDecorator(User) - expect((userMetaData.properties as any).squads.mappedBy).not.toBeDefined() - expect((userMetaData.properties as any).squads.inversedBy).toBe("users") - expect((userMetaData.properties as any).squads.owner).toBe(true) + expect((userMetaData.properties as any).squads.inversedBy).not.toBeDefined() + expect((userMetaData.properties as any).squads.mappedBy).toBe("users") + expect((userMetaData.properties as any).squads.owner).toBe(false) }) it(`should load the dml's correclty when both side of the relation are specifying the mappedBy options without pivot table`, () => { @@ -248,14 +248,14 @@ describe("manyToMany - manyToMany", () => { let [User, Team] = toMikroOrmEntities([user, team]) const teamMetaData = MetadataStorage.getMetadataFromDecorator(Team) - expect((teamMetaData.properties as any).users.mappedBy).toBe("squads") - expect((teamMetaData.properties as any).users.owner).toBe(false) + expect((teamMetaData.properties as any).users.inversedBy).toBe("squads") + expect((teamMetaData.properties as any).users.owner).toBe(true) expect((teamMetaData.properties as any).users.pivotTable).toBe("team_users") const userMetaData = MetadataStorage.getMetadataFromDecorator(User) - expect((userMetaData.properties as any).squads.mappedBy).not.toBeDefined() - expect((userMetaData.properties as any).squads.inversedBy).toBe("users") - expect((userMetaData.properties as any).squads.owner).toBe(true) + expect((userMetaData.properties as any).squads.inversedBy).not.toBeDefined() + expect((userMetaData.properties as any).squads.mappedBy).toBe("users") + expect((userMetaData.properties as any).squads.owner).toBe(false) expect((userMetaData.properties as any).squads.pivotTable).toBe( "team_users" )