movio / bramble

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

Abstract type boundary federation #166

Closed mai00cmi closed 2 years ago

mai00cmi commented 2 years ago

I think there are two ways to fix this a) a886f69b5fc7cd0dd2ab924e8477d21b61bb9d6a just assume if the _bramble_id is missing that it's not a boundary type b) 2dcec5a23e7b3366c0bbc533ee90c2818f46b612 always ask for __typename and check the types before merging.

I'm not sure which one is the correct one.

codecov-commenter commented 2 years ago

Codecov Report

Merging #166 (2dcec5a) into main (3091cea) will decrease coverage by 0.06%. The diff coverage is 75.00%.

@@            Coverage Diff             @@
##             main     #166      +/-   ##
==========================================
- Coverage   70.80%   70.74%   -0.07%     
==========================================
  Files          26       26              
  Lines        2672     2690      +18     
==========================================
+ Hits         1892     1903      +11     
- Misses        660      664       +4     
- Partials      120      123       +3     
Impacted Files Coverage Δ
execution_result.go 70.92% <57.14%> (-0.71%) :arrow_down:
execution.go 83.27% <85.71%> (-0.39%) :arrow_down:
plan.go 84.00% <100.00%> (+0.07%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

pkqk commented 2 years ago

We have the type information, so checking that makes more sense to me rather than just making assumptions.

nmaquet commented 2 years ago

Thanks for spending some time on this @mai00cmi! Great to see you digging into Bramble 🤘

pkqk commented 2 years ago

@mai00cmi if we squash and remove the error checks in fe1681f and a886f69 does your final commit solve the issue?

Happy to merge after that if that solves the error.