Skip to content
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

Merged
merged 3 commits into from
Oct 20, 2020

Conversation

amaranth
Copy link
Contributor

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.

describe("github issues > #2984 Discriminator conflict reported even for non-inherited tables", () => {
let connection: Connection;

before(async () => connection = await createConnection(setupSingleTestingConnection("postgres", {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

@pleerock
Copy link
Member

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 entityMetadata.inheritanceTree property for this purpose.

@amaranth
Copy link
Contributor Author

So while adding the change to make it only fail conflicts in the same inheritanceTree I discovered the original check was actually broken. Only the class with @TableInheritance gets marked as "STI", the rest are just "entity-child" type. That means the original check would block situations that should be allowed (like what this PR was originally about) but would allow situations that should be blocked (like having two children with the same discriminatorValue).

Copy link
Contributor

@imnotjames imnotjames left a 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.
@amaranth
Copy link
Contributor Author

Rebased and all the tests passed, should be good to go.

@imnotjames imnotjames merged commit 06903d1 into typeorm:master Oct 20, 2020
@imnotjames imnotjames added the hacktoberfest-accepted label hacktoberfest label Oct 20, 2020
zaro pushed a commit to zaro/typeorm that referenced this pull request Jan 12, 2021
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted label hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discriminator conflict reported even for non-inherited tables
4 participants