feat(router): Response Reformatting Errors#8441
Conversation
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: bc302854ab0c298aeb5c5cef |
This comment has been minimized.
This comment has been minimized.
TylerBloom
left a comment
TylerBloom
left a comment
There was a problem hiding this comment.
In addition to clearing up the wording for some of the errors, there is also an open question around if all possible invalid objects need to be validated. That is, do all elements in arrays and fields in object need to be validated if an invalid, non-nullable object is found?
TylerBloom
force-pushed
the
tylerb/router-1425
branch
from
1f2ebd8 to
8d36908
Compare
aaronArinder
left a comment
aaronArinder
left a comment
There was a problem hiding this comment.
first pass! talking synchronously about the logic will help me understand it better; then, I'll spend some time with the tests--it looks like there's good coverage, but I'll need to open it in an editor to really see what's covered
| } | ||
| } | ||
| #[inline] | ||
| fn format_non_nullable_value<'a: 'b, 'b>( |
There was a problem hiding this comment.
more me learning about rust than about this pr; but, it looks like these two lifetimes are just the same lifetime, do we need 'a: 'b, could 'b just be used?
There was a problem hiding this comment.
Yes, we do need 'a to outlive 'b (i.e. 'a: 'b) due to some lifetime rules around subtyping and variance. Notably, if this didn't require &mut Vec<_> and just had &[_] instead, we wouldn't be needed to specify any lifetimes and everything could just be elided.
| // NOTE: The subtype logic is strange. We are trying to determine if a fragment | ||
| // should be applied, but we don't have the __typename of the selection set | ||
| // (otherwise, we would be on a different branch). Consider the following query | ||
| // for a union Thing = Foo | Bar: | ||
| // { thing { ... on Foo { foo }, ... on Bar { bar } } } | ||
| // | ||
| // As we process the `... on Foo` fragment, `Foo` is `type_condition` and | ||
| // `Thing` is `current_type`, we *could* reverse the order in calling | ||
| // `is_subtype` and apply the fragment; however, the same is true for the `Bar` | ||
| // fragment. Without the type info of the data we have in our response, we | ||
| // can't know which to apply (or if both should apply in the case of | ||
| // interfaces). | ||
| // | ||
| // Without that information, this is the best we can do without construction a | ||
| // much more complicated reformatting heuristic. |
There was a problem hiding this comment.
I kind of like the field Thing.i as a way to understand where to find the invalid value; is there a way to keep the directional help for the field but also include the type that went wrong? Something like Invalid value found for field Thing.i, type Int?
There was a problem hiding this comment.
The reason the Type.field was changed is because we include the full path with the message. It also makes the message consistent between objects and lists while allowing full info to be available in the path in both cases.
There was a problem hiding this comment.
Notably, Renee and I came to an agreement on this message convention.
TylerBloom
force-pushed
the
tylerb/router-1425
branch
from
eb8c67d to
e0dfa52
Compare
Co-authored-by: Aaron <aaronarinder@gmail.com>
2 participants
This PR adds logic to insert errors into response if parts of a subgraph response are incorrect and get reformatted. This corrects and addresses prior response reformatting tests.
Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
A lot of (if not most) features benefit from built-in observability and
debug-level logs. Please read this guidance on metrics best-practices. ↩Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩