[JSONSelection] Support -> method syntax for inline data transforms
#5762
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a follow-up to PR #5156, which reordered some rules in the JSONSelection grammar but neglected to regenerate the SVG diagrams.
Although it may seem like a minor thing, we need an outer PathSelection wrapper around an inner PathList so we can apply certain logic only at the beginning of the path, which was not previously possible using only the PathSelection enum. For example, during the recursion of pretty_print_with_indentation, we don't know if a given PathList::Key is the first key in the path, in which case it _might_ need a leading '.' (depending on is_single_key), or some subsequent key, in which case it always needs a leading '.'. If we make a distinction between PathSelection and PathList, we can easily apply that special logic only at the beginning of the path in PathSelection::pretty_print_with_indentation. As a side benefit, we can keep the PathSelection::path field private, since PathSelection is now a struct rather than an enum, which should help maintain the various invariants we want to enforce upon the path (e.g. variables can only come at the beginning, and selections can only come at the end), because code outside the json_selection directory can no longer create any arbitrary chain of PathSelection enum variants.
I originally thought we might allow integer literals in path selections, such as data.results.0, but that functionality will be ultimately/better achieved using `->` methods that operate on arrays (`->first`, `->last`, `->at(n)`, `->slice(start, end)`, etc), leaving the PathList::Key variant to represent only string-valued keys.
We want the single-PathSelection version of JSONSelection to be capable
of operating directly on arrays (e.g. `$->slice(0, 5) { id }`), so that
decision should not be forced at the JSONSelection level.
|
CI performance tests
|
-> method syntax for inline data transforms and flexible value injectionJSONSelection] Support -> method syntax for inline data transforms
This commit also allows ->first and ->last to operate on string characters, rather than returning the whole string.
fdb1408 to
61566c7
Compare
dylan-apollo
reviewed
Aug 8, 2024
There was a problem hiding this comment.
Here are some comments on just the docs, in thinking about the methods themselves from a users' perspective. I do think before we stabilize any of these, we need to try them out on some full examples, which means we should add expansion support for them before continuing. In particular, expansion needs to know if a method resolves to an object, and if so, what the shape of that object is.
|
@nicholascioli @dylan-apollo Heads up that I grouped all the experimental methods we are not yet making public into a |
apollo-federation/src/sources/connect/json_selection/methods.rs
Outdated
Show resolved
Hide resolved
apollo-federation/src/sources/connect/json_selection/methods.rs
Outdated
Show resolved
Hide resolved
apollo-federation/src/sources/connect/json_selection/apply_to.rs
Outdated
Show resolved
Hide resolved
The only public methods we want to expose related to the `ApplyTo[Internal]` traits are the top-level `apply_to` and `apply_with_vars` methods, not internal methods like `apply_to_path` or `apply_to_array`. This commit hoists the `apply_to` and `apply_with_vars` methods up to the `JSONSelection` namespace (only), and requires each other AST type to implement its own `ApplyToInternal` trait / `apply_to_path` method, which is only visible within the `json_selection` module.
The "External" naming emphasizes that we are only collecting variables like $this, $args, and $context, and not $ or @ variables.
benjamn
commented
Aug 26, 2024
dylan-apollo
approved these changes
Aug 27, 2024
There was a problem hiding this comment.
I think we've reached peak iteration on this PR, and I'm happy to merge it if you are @benjamn! Discussed in a few places but, for posterity, we won't be documenting these as stable until we tackle a few follow-up items (all tracked in Jira, mostly validation-related). Merging it lets us sneak it into the release, though, so we can start experimenting with methods before letting users go wild (and start working on those follow-ups ASAP).
Thanks for putting all the effort into this, especially with all our questions and feedback!
benjamn
added a commit
that referenced
this pull request
Aug 27, 2024
benjamn
added a commit
that referenced
this pull request
Sep 13, 2024
Following up on this suggestion from @nicholascioli: #5762 (comment)
benjamn
added a commit
that referenced
this pull request
Sep 13, 2024
Following up on this suggestion from @nicholascioli: #5762 (comment)
benjamn
added a commit
that referenced
this pull request
Sep 17, 2024
Following up on this suggestion from @nicholascioli: #5762 (comment)
benjamn
added a commit
that referenced
this pull request
Feb 11, 2025
We previously decided to allow leading 0s, per this discussion: #5762 (comment) However, since we use the parsing logic of `serde_json::Number` to produce the internal representation, and that logic fails given leading zeros, we need to normalize the input `&str` a bit before calling `number.parse()`. We already perform some similar normalizations, e.g. ensuring the fractional part is at least a 0 (not empty after the `.`), so there's precedent.
benjamn
added a commit
that referenced
this pull request
Feb 11, 2025
We previously decided to allow leading 0s, per this discussion: #5762 (comment) However, since we use the parsing logic of `serde_json::Number` to produce the internal representation, and that logic fails given leading zeros, we need to normalize the input `&str` a bit before calling `number.parse()`. We already perform some similar normalizations, e.g. ensuring the fractional part is at least a 0 (not empty after the `.`), so there's precedent.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR extends
JSONSelectionsyntax to allow any field value (including fields selected bynested.path.selections) to be transformed in-line using a "method" syntax that looks like eitherfield->methodorfield->method(arg1, arg2, ...), wheremethodis an identifier that specifies which method implementation to invoke against the precedingfield's value.When a
->method takes arguments, those arguments can be any JSON literal expression, with an additional allowance forPathSelectionvalues, so you can refer to$variableslike$thisor$argsor$(see #5142) within the JSON literal. This additional capability is why I introduced theJSLiteralenum and relatedparse*/ApplyTomethods, rather than just reusingserde_json_bytes::Value. In some sense, this extension brings theJSLiteralsyntax closer to JavaScript literal syntax (which can refer to variables) than to JSON syntax, which is why I called itJSLiteralrather thanJSONLiteral.The implementation of a method has access not only to its arguments, but also to the input value preceding the
->. Sometimes, you may need to refer to that input value within the method arguments, which is possible using the newly introduced@symbol. This syntax was directly inspired byJSONPathsyntax, which uses the@character for analogous purposes (representing the current value within filter expressions).This syntax was first inspired by the need to operate on arrays, using methods like
array->first,array->last, andarray->slice(0, 3), but the set of potential use cases for the syntax has grown rapidly since then, and is very much still growing. You can find examples of all the currently supported methods in theREADME.md, and their actual implementations can be found in thejson_selection/methods.rsfile.For now, the set of available methods is fixed by the Rust implementation, and registered in a
lazy_static!map. In the future, we might make the method list configurable, with a flexible type system to validate inputs and outputs, but for now it seems simpler to restrict this functionality to methods we know about and have carefully reviewed.The most important commit is the final one, but I included a number of supporting refactoring commits beforehand.