Skip to content

Conversation

@ErimTuzcuoglu
Copy link

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

  • Code is up-to-date with the master branch
  • npm run format to apply prettier formatting
  • npm run test passes with this change
  • This pull request links relevant issues as Fixes #0000
  • There are new or updated unit tests validating the change
  • Documentation has been updated to reflect this change
  • The new commits follow conventions explained in CONTRIBUTING.md

@pleerock
Copy link
Member

Tests are failing. Also please make sure to merge latest master into your branch since latest master contains fixes for the tests.

@ErimTuzcuoglu ErimTuzcuoglu force-pushed the postgres-same-column-unique-and-primary-constraint-fix branch from 044472d to 072a566 Compare August 24, 2022 21:07
@ErimTuzcuoglu ErimTuzcuoglu force-pushed the postgres-same-column-unique-and-primary-constraint-fix branch from 072a566 to b08a604 Compare August 24, 2022 21:09
@ErimTuzcuoglu ErimTuzcuoglu force-pushed the postgres-same-column-unique-and-primary-constraint-fix branch from b08a604 to 7e15dcc Compare August 24, 2022 21:19
@PrimaryColumn()
@OneToOne("User", "id")
@JoinColumn({ name: "userId" })
public userId: string
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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.

Copy link
Contributor

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Why is second migration generated?

3 participants