Closed gmac closed 2 years ago
Merging #85 (a09d53b) into main (b054273) will increase coverage by
0.09%
. The diff coverage is88.88%
.
@@ Coverage Diff @@
## main #85 +/- ##
==========================================
+ Coverage 67.01% 67.11% +0.09%
==========================================
Files 24 24
Lines 2747 2749 +2
==========================================
+ Hits 1841 1845 +4
+ Misses 750 749 -1
+ Partials 156 155 -1
Impacted Files | Coverage Δ | |
---|---|---|
plan.go | 82.04% <88.88%> (+0.97%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update b054273...a09d53b. Read the comment docs.
@nmaquet FYI – looks like the map enumeration I did in https://github.com/movio/bramble/pull/81 has introduced a spurious test failure with the print order of fragments. I'll noodle on fixing it.
@nmaquet FYI – looks like the map enumeration I did in #81 has introduced a spurious test failure with the print order of fragments. I'll noodle on fixing it.
@gmac You've potentially already figured it out but this is due to this line https://github.com/movio/bramble/blob/main/plan.go#L228, as the iteration ordering over maps is intentionally random in golang. Sorting the output feels wrong because we don't actually care about ordering other than for the test. At the same time due to the fact the selection set is a string here makes it a difficult to fix comparison.
It might pay to create something custom for that test. If we can parse the selection set string back into the AST format we could simply check for the presence of each fragment.
@lucianjon
It might pay to create something custom for that test. If we can parse the selection set string back into the AST format we could simply check for the presence of each fragment.
Good call! Yep, I learned this morning about randomized map orders... it was a surprise to me coming from the land of Ruby and JS where enumerable keys maintain definition order. Thanks for the info and the tip. Will have another PR for that fix in the next day or two.
@gmac Great find, looks good from my perspective. The fix and added tests make sense, I think the collect and then create approach is cleaner too.
As described in https://github.com/movio/bramble/issues/73, deeply-nested selection paths that transitioned between services were broken. This was a fairly significant issue.
The problem
While extracting selection sets, the
mergedWithExistingStep
check was treating all consolidated fields as leaf values. That means that when a composite field was encountered with its own nested selections, that entire selection set was merged into the current planning step without traversing its children to delegate them properly. This resulted in fields belonging to foreign services getting lumped into the present service selection, and the query failing with a GraphQL validation message:The fix
This adjusts the flow of
extractSelectionSet
so that all remote fields are collected up front and then create steps once together later in the process. This eliminates the need for the previous step-merging pattern, and is computationally simpler to create steps all at once rather than creating them incrementally for each remote step in sequence.Note that this refactor looks bigger than it really is: this simplification was able to remove some conditional nesting and thus decrease the indent on a few existing code blocks.
Tests
Tests have been added. There's one update to an existing test where a benign array order has changed due to the revised sequencing of extracting remote fields.
Resolves https://github.com/movio/bramble/issues/73.