movio / bramble

A federated GraphQL API gateway
https://movio.github.io/bramble/
MIT License
498 stars 55 forks source link

Always add __typename to abstract types #91

Closed gmac closed 2 years ago

gmac commented 3 years ago

Preemptively fixing the same issue described in https://github.com/movio/bramble/pull/81, but with __typename...

The (potential) problem

Given that the execution resolver uses __typename while assembling abstract types, we need to assure that a typename is always returned. However, the automated __typename hints are currently only added to user-defined fragments, and those aren't guaranteed to be present when making abstract type selections (case of point: selecting base interface fields).

While I haven't actually made this scenario fail, I strongly suspect that it can be broken.

The (preemptive) fix

This adds __typename to all abstract types. In the event that the user doesn't make a fragment selection on an abstract, the selection will still request typename for resolution purposes.

codecov-commenter commented 3 years ago

Codecov Report

Merging #91 (2c776bd) into main (0813d5d) will increase coverage by 0.02%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #91      +/-   ##
==========================================
+ Coverage   67.15%   67.17%   +0.02%     
==========================================
  Files          24       24              
  Lines        2749     2754       +5     
==========================================
+ Hits         1846     1850       +4     
- Misses        748      749       +1     
  Partials      155      155              
Impacted Files Coverage Δ
plan.go 82.40% <100.00%> (+0.35%) :arrow_up:
auth.go 86.79% <0.00%> (-0.63%) :arrow_down:

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 0813d5d...2c776bd. Read the comment docs.

gmac commented 2 years ago

@nmaquet – this is ready for consideration.

suessflorian commented 2 years ago

I'm thinking; if we guarantee a __typename injection when the parent type presents itself as an abstract type, can we then omit the injection when encountering fragment spreads?

gmac commented 2 years ago

@suessflorian — I had the same thought… I haven’t dug deep enough into the execution pipeline yet for the answer though. The wildcard would be if typename is somehow used for looking up fragments within the resolver, at which time a fragment on a concrete type (which is totally valid) would still need typename via the fragment. Leaving the status quo seemed safest.