-
-
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
Only check for discriminator conflicts on STI entities #2985
Conversation
describe("github issues > #2984 Discriminator conflict reported even for non-inherited tables", () => { | ||
let connection: Connection; | ||
|
||
before(async () => connection = await createConnection(setupSingleTestingConnection("postgres", { |
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.
Why test it on just one database if fix is not database type specific?
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.
Whoops, that was a copy/paste from another test. I should be able to push a change for that tomorrow if you want.
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 like in this case he doesn't need to do it since he don't really use connection. He just needs to check entity validation process.
I think there is a different problem. Why would you get this error if you don't use entity children? Discriminator values are created for entity children right? In this case your second Node isn't child of that parent. It looks we need to check if your second node is a child of your first node. You probably need to use |
So while adding the change to make it only fail conflicts in the same |
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.
This looks good. Just want to rebase so all tests run and then we are good to go.
If an entity has the same discriminator value as one using @TableInheritance this should be allowed as there is no way for them to conflict. Fixes typeorm#2984.
This commit also fixes the discriminatorColumn check to check all the entities it should be checking. Only the parent in an STI setup has the "STI" inheritancePattern, the children just have the "entity-child" tableType. Before two children with the same discriminatorValue would be (incorrectly) allowed as they'd never even get checked.
Rebased and all the tests passed, should be good to go. |
…2985) If an entity has the same discriminator value as one using `TableInheritance` this should be allowed as there is no way for them to conflict. This commit also fixes the discriminatorColumn check to check all the entities it should be checking. Only the parent in an STI setup has the "STI" inheritancePattern, the children just have the "entity-child" tableType. Before two children with the same discriminatorValue would be (incorrectly) allowed as they'd never even get checked. Fixes typeorm#2984
If an entity has the same discriminator value as one using
@TableInheritance this should be allowed as there is no way for them to
conflict. Fixes #2984.