fix: filter null keys when including belongs-to relations in queries #4795
Conversation
| }); | ||
|
|
||
| let results = await orderRepo.find({include: [{relation: 'customer'}]}); | ||
| expect(results.length).to.equal(2); |
There was a problem hiding this comment.
What are we trying to fix in the PR? I didn't see the code changes, and new added test doesn't seem to test the null fk.
There was a problem hiding this comment.
Hi @agnes512, I moved in with only the tests first to check all the datasources that fail for this condition. I am adding the code changes now.
There was a problem hiding this comment.
I should have opened a draft PR, sorry about the false alarm.
There was a problem hiding this comment.
@agnes512 the new tests do check for null fk , because the order 'Dine in' has no customer Id.
There was a problem hiding this comment.
Oh haha I see. Sorry that I got the notification and just reviewed it.
And I have a few questions. I checked other tests in this file, the test in #L 97 checks the case that source instance does not have the foreign key. Why is that one passing before the changes? Is the bug caused by the inclusion? If so, I think file belongs-to.inclusion-resolver.relation.acceptance.ts fits this test more. From my understanding, order.customer(order.id) is different from order.find({include customer}, because the later one uses the inclusion resolver, where the problem lies in. It'd also be better if we can check content of result to see if there is only one instance has the related model.
There was a problem hiding this comment.
@agnes512 the other test passes because it directly queries the target relation. You are right, I will move this test to the inclusion resolver tests. it suites better there.
deepakrkris
changed the title
|
Does this PR make the behavior of all the connectors uniform now? Is there any difference between Postgres and others? If yes, we should add the details on the website docs. |
@hacksparrow this is not a behavioral change, users will not see any difference in querying. It only removes null keys before querying a relation. |
deepakrkris
force-pushed
the
fix4372
branch
2 times, most recently
from
0bc94f4 to
0d03047
Compare
| targetRepo, | ||
| targetKey, | ||
| deduplicate(sourceIds), | ||
| dedupedSourceIds.filter(e => e), |
There was a problem hiding this comment.
Is it possible to have a valid id value that's not truthy? For example, I can imagine 0 being a valid primary key. Your condition will filter such value out.
fixes #4372
fix querying related entities with null keys and add tests to repository-test
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm testpasses on your machinepackages/cliwere updatedexamples/*were updated👉 Check out how to submit a PR 👈