Fix: fully traverse nested field selections while planning

[gmac]

As described in #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:

Cannot query field <remoteField> on type <LocalType>

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 #73.

[codecov-commenter]

Codecov Report

Merging #85 (a09d53b) into main (b054273) will increase coverage by 0.09%.
The diff coverage is 88.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%)	⬆️

---

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.

[gmac]

The order of steps changed here because the extractSteps call moved lower in the extractSelectionSet process. However, this reordering of steps should be benign to the execution process.

[gmac]

This is the only actual change to this conditional: transitions in service location now collect remote fields into an array and resolve them all at once later in the process, versus resolving each remote field individually here. There's a lot less churn resolving them together, and it avoids the situation that mergedWithExistingStep was previously trying to address.

The else if and else blocks are identical to their previous implementations, they're simply indented out now that the conditional structure can be flattened.

[gmac]

@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.

[lucianjon]

@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.

[gmac]

@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.

[lucianjon]

@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.