-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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: fix entityManager.getId for custom join table #8676
Conversation
Please add tests to this change |
ff3dad7
to
1ff25c8
Compare
Test added |
Maybe we can simply add check for: && !(entity[this.relationMetadata.propertyName] instanceof Promise) instead of I think it will make more clear what is happening this way. |
That will unintentionally call the getter function and thus make a database request in case it's a lazy relation |
@ranjan-purbey but how come |
@pleerock because lazy-relations are not normal functions. Instead they are getters. JavaScript getters are called as soon as the property is accessed. For example, consider the following: const todos = {
get items () {
console.log("Items property was accessed");
return ["task 1", "task 2"];
}
}; If we try to check if |
@ranjan-purbey thanks for the information. If so, we can merge this PR, however I would like to ask to add a bold comment on why this approach was used instead of |
Yes I agree that a comment clarifying this will be helpful for future reference. I will add one |
1ff25c8
to
8e14960
Compare
@pleerock comment added |
getId currently returns undefined for an entity with composite primary key if primary key columns also foreign keys with lazy relations, for e.g., in a custom join table. This commit tries to fix that Closes: typeorm#7736 (maybe)
8e14960
to
0bd9641
Compare
Thanks a lot for the contribution! |
Description of change
entityManager.getId
currently returns undefined for an entity with composite primary key if primary key columns are also foreign keys with lazy relations, for e.g., in a custom join table. This commit tries to fix that.The reason for
getId
returningundefined
is thatcolumnMetadata.getEntityValueMap
doesn't take into account the fact that the value for lazy-relations is a getter which returns aPromise
. This PR adds a check for this condition.To reproduce the issue, refer https://github.com/ranjan-purbey/typeorm-composite-primary-key-test
Closes: #7736 (maybe)
Pull-Request Checklist
master
branchnpm run lint
passes with this changenpm run test
passes with this changeFixes #0000