Implement ApplyToInternal::compute_output_shape and JSONSelection::shape returning shape::Shape
#6458
Conversation
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: 61dbe18c073365a3763b2e50 |
|
CI performance tests
|
| NamedSelection ::= NamedPathSelection | PathWithSubSelection | NamedFieldSelection | NamedGroupSelection | ||
| NamedPathSelection ::= Alias PathSelection | ||
| NamedPathSelection ::= (Alias | "...") PathSelection |
There was a problem hiding this comment.
does this allow for future keywords like if or match directly after the ...?
There was a problem hiding this comment.
If the PathSelection started with an if token, that would be a potential concern for making if a keyword in the future, but if we stick with the the idea of a parenthesized test (i.e. ... if (test) {}), the ( character should prevent parsing as a PathSelection, causing the parser to backtrack and consider other ways of parsing ... if (test) {} (which we can define).
Same goes for ... match (value), as long as we use parentheses, or as long as we're willing to make the match field illegal immediately after ... (a breaking change but not unthinkable).
There was a problem hiding this comment.
Postponing this syntax for a later PR/release.
| @@ -69,6 +73,44 @@ impl JSONSelection { | |||
|
|
|||
| (value, errors.into_iter().collect()) | |||
| } | |||
|
|
|||
| pub fn shape(&self) -> Shape { | |||
There was a problem hiding this comment.
what's the reason for having shape code in this file? it feels different enough to warrant a separate file and maybe a separate trait, even if it's structured basically the same
There was a problem hiding this comment.
It made the code a little easier to write to have apply_to_path nearby, but there's probably no harm in moving it to a new file now.
There was a problem hiding this comment.
I'll make this move in a follow-up PR.
|
|
||
| for pair in args { | ||
| if let LitExpr::Array(pair) = pair.as_ref() { | ||
| if pair.len() == 2 { |
There was a problem hiding this comment.
should we return an error if the length isn't exactly 2?
There was a problem hiding this comment.
do you have an example of error handling? since this doesn't return a Result, what should a caller do with a Shape::error_with_range?
There was a problem hiding this comment.
I imagine we'll add a way to collect/traverse/visit any ShapeCase::Error variants within a given Shape, to handle them programmatically or surface them in the debugger, but for now the main point of errors is to fail validation against expected GraphQL schema types.
Since you can embed ShapeCase::Error within a larger shape, shape errors are similar to the GraphQL errors-as-data pattern, where you're able to get back some partial shape information along with contextual errors.
909e897 to
e5eedde
Compare
15b6b8c to
1fde6d8
Compare
e5eedde to
16d0306
Compare
1fde6d8 to
ebb3e95
Compare
16d0306 to
dfeb558
Compare
ebb3e95 to
70486c1
Compare
70486c1 to
2658a33
Compare
| // This output shape is wrong if $root.friend_ids turns out to be an | ||
| // array, and it's tricky to see how to transform the shape to what | ||
| // it would have been if we knew that, where friends: List<{ id: | ||
| // $root.friend_ids.* }> (note the * meaning any array index), | ||
| // because who's to say it's not the id field that should become the | ||
| // List, rather than the friends field? |
There was a problem hiding this comment.
I'm not following the impact of this comment. If the result is potentially incorrect, is this an area that needs future work? Or is the potentially incorrect result not a problem for some reason?
Is there something needed in the syntax to clarify this case? Similarly to how the user can specify <Type>, do we need a way to indicate that there is an array type expected here, like [friend_ids]?
Or should the result be a union of all the possible combinations of things that could be an array (which seems like a potentially large number of possibilities)?
af100bc to
cb6b5ef
Compare
cb6b5ef to
fef9431
Compare
apollo-federation/src/sources/connect/json_selection/methods.rs
Outdated
Show resolved
Hide resolved
|
The |
This PR implements full expressions in URIs and header values (e.g., arrow methods, not just variables) via a few pieces: 1. The new apollo-federation/src/sources/connect/string_template.rs contains shared logic for both URIs and headers, so they can be more consistent behavior (both parsing and interpolation). This comes with support for full JSONSelection rather than just the variable references 2. As a result of 1, headers now work much like URIs in that arrays and objects won't be automatically serialized to JSON, instead it is an error for an expression to result in one of those. This is a breaking change. Users should work around this with ->jsonStringify from ->jsonStringify method #6519 3. Much of the former variable reference code has been removed in favor of this new parsing approach, which leans on the JSONSelection parser instead 4. To validate these more complex expressions, the new shape library is being used. As a result this PR relies on Implement `ApplyToInternal::compute_output_shape` and `JSONSelection::shape` returning `shape::Shape` #6458 There are some expected gaps in the validations where we need to build more infrastructure for the type checker. For example, ->map is completely ignored for now.
9873eb3 to
c6ace9a
Compare
* Remove unused generic parameter from `Ranged` trait. * Implement `Hash` for `WithRange<T>` only when `T: Hash`. * Implement/use `PrettyPrintable` for `Alias` and `Key`. * Remove redundant `NamedSelection::apply_to_path` array base case.
This PR implements full expressions in URIs and header values (e.g., arrow methods, not just variables) via a few pieces: 1. The new apollo-federation/src/sources/connect/string_template.rs contains shared logic for both URIs and headers, so they can be more consistent behavior (both parsing and interpolation). This comes with support for full JSONSelection rather than just the variable references 2. As a result of 1, headers now work much like URIs in that arrays and objects won't be automatically serialized to JSON, instead it is an error for an expression to result in one of those. This is a breaking change. Users should work around this with ->jsonStringify from ->jsonStringify method #6519 3. Much of the former variable reference code has been removed in favor of this new parsing approach, which leans on the JSONSelection parser instead 4. To validate these more complex expressions, the new shape library is being used. As a result this PR relies on Implement `ApplyToInternal::compute_output_shape` and `JSONSelection::shape` returning `shape::Shape` #6458 There are some expected gaps in the validations where we need to build more infrastructure for the type checker. For example, ->map is completely ignored for now.
c6ace9a to
32dafac
Compare
This draft PR uses our newly published
shapecrate to implement theApplyToInternal::compute_output_shapetrait method, which returns ashape::Shape, which is an immutable and reference-counted Rust data structure that describes a set of constraints on JSON data. The "shape" terminology is a close synonym for "type" but I'm using it to emphasize the immutable, reference-free, value-typed nature of the JSON domain.As an important part of this
::compute_output_shapesystem, all->methods now define amethod_shapefunction that allows them to reject unexpected input shapes and compute their own output shape in terms of a given input shape and variable shapes. In other words, the type signatures of->methods are expressed using Rust code which ultimately returns ashape::Shape, which then gets further processed by thecompute_output_shapelogic, allowing->methods to blend seamlessly into the shape processing system.This new ability to compute a static output
Shapefor a given JSONSelection string allows us to relax the constraint that inlinePathSelectionitems must have a static{...}subselection at the end. The static subselection was previously our only means of statically determining the output fields of thePathSelection, but now we can statically/programmatically examine the outputShape, which doesn't distinguish between static subselections and->method results (as long as they have the same computed object shape). We are also now requiring a...token before inlinePathSelectionitems without static subselections, and encouraging the...token for any inlinePathSelection, even if it has a subselection (though enforcing it in that case would be a breaking change).Finally, this PR proposes a way of setting the
__typenamein situations where abstract GraphQL types (interfaces and unions) are involved: by writing a{ <Type> ... }annotation at the beginning of any subselection, developers can indicate to both theShapesystem and the runtime system that the__typenameof this output object should have the literal shape"Type", and the object should include__typename: "Type"at runtime. The underlyingShaperepresentation actually does use a__typenamefield to model this information, but that's a choice made by the JSONSelection code, not something embedded in theShapelibrary.I wish I could have broken this PR up more, though I was able to split out #6455, #6456, and #6457.
I don't believe this PR should introduce any logical runtime differences of behavior in the connectors system, since it's basically adding the
compute_output_shapemethod and then only using it in tests. Once I address a fewTODOs, the output shapes will play a role in checking the validity of...-inlinedPathSelectionitems, but the...syntax is new with this PR, so changing it is still easy.