fix: Handle the querying of secondary relation id fields #1768
Conversation
AndrewSisley
added
bug
AndrewSisley
force-pushed
the
1654-2ndary-related-id
branch
4 times, most recently
from
adf203b to
d3874c1
Compare
AndrewSisley
force-pushed
the
1654-2ndary-related-id
branch
from
d3874c1 to
0bf89b0
Compare
Codecov ReportPatch coverage:
@@ Coverage Diff @@
## develop #1768 +/- ##
===========================================
+ Coverage 75.74% 75.75% +0.01%
===========================================
Files 209 209
Lines 22035 22106 +71
===========================================
+ Hits 16690 16745 +55
- Misses 4194 4204 +10
- Partials 1151 1157 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
AndrewSisley
force-pushed
the
1654-2ndary-related-id
branch
from
0bf89b0 to
f59cc22
Compare
There was a problem hiding this comment.
suggestion: Can you please add a test in one_to_one_to_one with a query on Book joining both Author and Publisher?
There was a problem hiding this comment.
I'm curious, not argumentative - how do you see that breaking/being-relevant?
There was a problem hiding this comment.
I want a test confirming that two or more joins on the one object works as expected. Pretty much to ensure makeTypeJoinOne works in these kinds of situations.
There was a problem hiding this comment.
As-in work in general? I hope we already have tests that properly cover one-one-one joins
There was a problem hiding this comment.
Confirmed, we already cover all possible direction combinations for one-one-one joins.
There was a problem hiding this comment.
Book {
name
publisher {
name
}
author {
name
}
}There was a problem hiding this comment.
Ah okay spreading out from the middle obj? I'll add that in another PR/branch real quick. But not here.
There was a problem hiding this comment.
Well it's the "middle obj" in this case but what matters to me is the dual join on the one object. I think it's quite relevant here so I would like to see it added.
There was a problem hiding this comment.
This PR doesn't change any of the join mechanics. It uses a join, but it doesn't change how they work.
If a bug is found in the test gap you spotted it should not block or impact the merging of this work.
planner/mapper/mapper.go
Outdated
| @@ -928,6 +906,131 @@ sourceLoop: | |||
| return newFields, nil | |||
| } | |||
|
|
|||
| // constructDummyJoin constructs a valid empty join with no requested fields. | |||
| func constructDummyJoin( | |||
There was a problem hiding this comment.
suggestion: I think we should avoid using the work Dummy as it feels more like something you'd use just for testing purposes. In this case this is a function that is used in a critical path. Maybe constructEmptyJoin would be more appropriate.
There was a problem hiding this comment.
Yeah good shout - I'll change it
- Rename Dummy func
AndrewSisley
force-pushed
the
1654-2ndary-related-id
branch
from
f59cc22 to
a72c696
Compare
Whilst other existing tests do document the issue, this is the simplest use-case and needs coverage and will be useful when implementing the fix
It will be called from a second location shortly
Within this commit there is no way to test this, as groupBy clauses cannot traverse relations boundaries at the moment. However it is required to handle secondary relation ids (via joins), which will be added shortly. It is also a likely step towards supporting the traversal of relations in groupBy clauses in the future.
AndrewSisley
force-pushed
the
1654-2ndary-related-id
branch
from
6dd95bc to
65c1844
Compare
…rk#1768) ## Relevant issue(s) Resolves sourcenetwork#1654 ## Description Handles the querying of secondary relation id fields. It does this by joining the related object and copying the dockey value into the secondary related id field.
Relevant issue(s)
Resolves #1654
Description
Handles the querying of secondary relation id fields.
It does this by joining the related object and copying the dockey value into the secondary related id field.
Is currently blocked by Patch one-one relation field to existing schema does not require both id fields #1769 but can still be reviewed nowPR is now based off the fix for that issue (do not review the first commit here). Will rebase off develop branch before merge.Fix merged to develop, this branch has been rebased onto it.