-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
fix: Preventing both unique and primary constraint query building on … #9282
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: Preventing both unique and primary constraint query building on … #9282
Conversation
|
Tests are failing. Also please make sure to merge latest master into your branch since latest master contains fixes for the tests. |
044472d to
072a566
Compare
072a566 to
b08a604
Compare
b08a604 to
7e15dcc
Compare
| @PrimaryColumn() | ||
| @OneToOne("User", "id") | ||
| @JoinColumn({ name: "userId" }) | ||
| public userId: string |
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 just left a comment in #9229. It has invalid decorators usage. Same issue in your test.
Use case is not valid. Relational decorators must be applied to the properties with the type of the object to which relation maps, e.g.:
import { Column, Entity, JoinColumn, OneToOne, PrimaryColumn } from 'typeorm';
@Entity('testEntity')
export class TestEntity {
@PrimaryColumn()
public userId: string;
@OneToOne('User', 'test')
@JoinColumn({ name: 'userId' })
public user: User;
@Column({ type: 'jsonb', nullable: false })
public x: string[];
}Change the test, and run the test without your changes in the RelationJoinColumnBuilder.ts file.
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.
Yeah actually I know too this use case is not valid. But when user added both PrimaryColumn and JoinColumn accidentally like this example, our query builder generates table query as below.
CREATE TABLE "testEntity" (
"userId" uuid NOT NULL,
"x" jsonb NOT NULL,
CONSTRAINT "REL_3138077fc5fcc8a4e4e3c175f4" UNIQUE ("userId"),
CONSTRAINT "PK_3138077fc5fcc8a4e4e3c175f40" PRIMARY KEY ("userId")
)
I manually ran this query and similar queries which includes both primary and unique constraints on same field in our db and I saw postgresql added only the last constraint. My code includes the change that only prevent this condition.
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.
we shouldn't bring fixes for invalid use cases. Other parts of the codebase might not expect invalid use cases, thus they can break or lead to incorrect behavior. We need to update the test cases to valid code and check if we still have this issue with a valid entity definitions.
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.
Relational decorators must be applied to the properties with the type of the object to which relation maps
It may be useful to do some kind assertion there.
| export class User { | ||
| @PrimaryColumn() | ||
| @OneToOne("TestEntity", "userId") | ||
| public id: string |
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.
here it should be:
@PrimaryColumn()
public id: string
@OneToOne("TestEntity", "user")
public test: TestEntity
Description of change
Fixes #9229
Added postgres option to RelationJoinColumnBuilder to prevent both unique and primary constraints on same column. When I tested with executing raw query on postgres, I realized that it just add last constraint in the query while table creating.
Pull-Request Checklist
masterbranchnpm run formatto apply prettier formattingnpm run testpasses with this changeFixes #0000