-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix 3120 #7277
Fix 3120 #7277
Conversation
This new option allows to create relation without using foreign keys Closes: typeorm#3120
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't joinColumns
and inverseJoinColumns
still be set here, even if there are no foreign keys?
typeorm/src/metadata/RelationMetadata.ts
Lines 490 to 500 in c4a36da
registerForeignKeys(...foreignKeys: ForeignKeyMetadata[]) { | |
this.foreignKeys.push(...foreignKeys); | |
this.joinColumns = this.foreignKeys[0] ? this.foreignKeys[0].columns : []; | |
this.inverseJoinColumns = this.foreignKeys[1] ? this.foreignKeys[1].columns : []; | |
this.isOwning = this.isManyToOne || ((this.isManyToMany || this.isOneToOne) && this.joinColumns.length > 0); | |
this.isOneToOneOwner = this.isOneToOne && this.isOwning; | |
this.isOneToOneNotOwner = this.isOneToOne && !this.isOwning; | |
this.isManyToManyOwner = this.isManyToMany && this.isOwning; | |
this.isManyToManyNotOwner = this.isManyToMany && !this.isOwning; | |
this.isWithJoinColumn = this.isManyToOne || this.isOneToOneOwner; | |
} |
If so, last paragraph of my comment describes the necessary changes.
@nebkat you're right, I missed that... PR updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now!
@@ -56,13 +56,14 @@ export class RelationJoinColumnBuilder { | |||
*/ | |||
build(joinColumns: JoinColumnMetadataArgs[], relation: RelationMetadata): { | |||
foreignKey: ForeignKeyMetadata|undefined, | |||
columns: ColumnMetadata[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just rename this field to joinColumns
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it - and I'm worried that it might be confusing, as this function already takes joinColumns
(JoinColumnMetadataArgs
type) as argument, but in the result columns type is ColumnMetadata[]
- so I've decided to follow same naming as for types. Please let me know, if you have another idea how to resolve that naming conflict
action: string; | ||
|
||
@ManyToOne(type => Person, { | ||
createForeignKeyConstraints: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think if we just call it constraint: false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could there be confusion with unique constraints? I think there is some driver that requires a unique on foreign keys.
Thank you for contribution! 🎉 |
Description of change
Fixes #3120 by adding a new relation option "createForeignKeyConstraints". That's updated version of #7112 PR by @alpharder, with fixes suggested by @nebkat and updated tests & docs.
Pull-Request Checklist
master
branchnpm run lint
passes with this changenpm run test
passes with this change (there are some linter errors not related to those changes - mainly in exxisting test cases)Fixes #0000