From 82e262b89646ce79d1fcfdffc2529104d2e65116 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 23 Oct 2024 13:01:31 -0400 Subject: [PATCH] [JSONSelection] Remove `rest: * { a b }` star selection syntax (#6185) --- .../tests/schemas/expand/steelthread.graphql | 3 +- .../tests/schemas/expand/steelthread.yaml | 2 - ...expand_supergraph@carryover.graphql-2.snap | 3 - ..._expand_supergraph@circular.graphql-2.snap | 2 - .../it_expand_supergraph@keys.graphql-2.snap | 9 - ..._supergraph@normalize_names.graphql-2.snap | 2 - ...expand_supergraph@realistic.graphql-2.snap | 12 - ...it_expand_supergraph@simple.graphql-2.snap | 3 - ...pand_supergraph@steelthread.graphql-2.snap | 26 +- ...pand_supergraph@steelthread.graphql-3.snap | 3 - ...expand_supergraph@steelthread.graphql.snap | 1 - ...supergraph@types_used_twice.graphql-2.snap | 4 - .../sources/connect/expand/visitors/mod.rs | 31 -- .../connect/expand/visitors/selection.rs | 4 - .../sources/connect/json_selection/README.md | 179 +++-------- .../connect/json_selection/apply_to.rs | 275 +---------------- .../json_selection/grammar/JSONSelection.svg | 32 +- .../grammar/NakedSubSelection.svg | 52 ---- .../json_selection/grammar/StarSelection.svg | 60 ---- .../json_selection/grammar/SubSelection.svg | 36 +-- .../connect/json_selection/location.rs | 12 - .../sources/connect/json_selection/parser.rs | 280 +----------------- .../sources/connect/json_selection/pretty.rs | 57 +--- .../connect/json_selection/selection_set.rs | 57 ---- ...n__location__tests__arrow_path_ranges.snap | 1 - ...on__tests__parse_with_range_snapshots.snap | 52 +--- ...__parser__tests__expr_path_selections.snap | 1 - ...rser__tests__path_with_subselection-2.snap | 2 - ...rser__tests__path_with_subselection-3.snap | 3 - ...rser__tests__path_with_subselection-5.snap | 2 - ...rser__tests__path_with_subselection-6.snap | 4 - ...rser__tests__path_with_subselection-7.snap | 4 - ...parser__tests__path_with_subselection.snap | 1 - ...n_selection__parser__tests__selection.snap | 4 - .../src/sources/connect/models.rs | 2 - .../src/sources/connect/spec/directives.rs | 2 - .../sources/connect/validation/selection.rs | 126 +------- .../validation_tests@rest-good.graphql.snap | 6 - .../validation_tests@rest.graphql.snap | 38 --- .../validation/test_data/rest-good.graphql | 18 -- .../connect/validation/test_data/rest.graphql | 53 ---- .../src/plugins/connectors/make_requests.rs | 20 -- .../connectors/testdata/steelthread.graphql | 3 +- .../connectors/testdata/steelthread.yaml | 2 - apollo-router/src/plugins/connectors/tests.rs | 16 +- 45 files changed, 106 insertions(+), 1399 deletions(-) delete mode 100644 apollo-federation/src/sources/connect/json_selection/grammar/NakedSubSelection.svg delete mode 100644 apollo-federation/src/sources/connect/json_selection/grammar/StarSelection.svg delete mode 100644 apollo-federation/src/sources/connect/validation/snapshots/validation_tests@rest-good.graphql.snap delete mode 100644 apollo-federation/src/sources/connect/validation/snapshots/validation_tests@rest.graphql.snap delete mode 100644 apollo-federation/src/sources/connect/validation/test_data/rest-good.graphql delete mode 100644 apollo-federation/src/sources/connect/validation/test_data/rest.graphql diff --git a/apollo-federation/src/sources/connect/expand/tests/schemas/expand/steelthread.graphql b/apollo-federation/src/sources/connect/expand/tests/schemas/expand/steelthread.graphql index b234783089..3e4b8ba6ab 100644 --- a/apollo-federation/src/sources/connect/expand/tests/schemas/expand/steelthread.graphql +++ b/apollo-federation/src/sources/connect/expand/tests/schemas/expand/steelthread.graphql @@ -71,7 +71,7 @@ type Query @join__type(graph: GRAPHQL) { users: [User] @join__field(graph: CONNECTORS) @join__directive(graphs: [CONNECTORS], name: "connect", args: {source: "json", http: {GET: "/users"}, selection: "id name"}) - user(id: ID!): User @join__field(graph: CONNECTORS) @join__directive(graphs: [CONNECTORS], name: "connect", args: {source: "json", http: {GET: "/users/{$args.id}"}, selection: "id\nname\nusername\nrest: *", entity: true}) + user(id: ID!): User @join__field(graph: CONNECTORS) @join__directive(graphs: [CONNECTORS], name: "connect", args: {source: "json", http: {GET: "/users/{$args.id}"}, selection: "id\nname\nusername", entity: true}) } type User @@ -81,7 +81,6 @@ type User id: ID! name: String @join__field(graph: CONNECTORS) username: String @join__field(graph: CONNECTORS) - rest: JSON @join__field(graph: CONNECTORS) c: String @join__field(graph: CONNECTORS, external: true) @join__field(graph: GRAPHQL) d: String @join__field(graph: CONNECTORS, requires: "c") @join__directive(graphs: [CONNECTORS], name: "connect", args: {source: "json", http: {GET: "/users/{$this.c}"}, selection: "$.phone"}) } diff --git a/apollo-federation/src/sources/connect/expand/tests/schemas/expand/steelthread.yaml b/apollo-federation/src/sources/connect/expand/tests/schemas/expand/steelthread.yaml index 058efe1a2e..f78235ff63 100644 --- a/apollo-federation/src/sources/connect/expand/tests/schemas/expand/steelthread.yaml +++ b/apollo-federation/src/sources/connect/expand/tests/schemas/expand/steelthread.yaml @@ -31,7 +31,6 @@ subgraphs: id name username - rest: * """ entity: true ) @@ -46,7 +45,6 @@ subgraphs: id: ID! name: String username: String - rest: JSON c: String @external d: String @requires(fields: "c") diff --git a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@carryover.graphql-2.snap b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@carryover.graphql-2.snap index 3cb495356b..bfe51c13ac 100644 --- a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@carryover.graphql-2.snap +++ b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@carryover.graphql-2.snap @@ -152,7 +152,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/ca None, ), ], - star: None, range: Some( 0..70, ), @@ -321,7 +320,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/ca None, ), ], - star: None, range: Some( 0..70, ), @@ -415,7 +413,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/ca None, ), ], - star: None, range: Some( 0..2, ), diff --git a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@circular.graphql-2.snap b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@circular.graphql-2.snap index 7e2fb32988..28d0ba77f0 100644 --- a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@circular.graphql-2.snap +++ b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@circular.graphql-2.snap @@ -79,7 +79,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/ci None, ), ], - star: None, range: Some( 0..2, ), @@ -171,7 +170,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/ci None, ), ], - star: None, range: Some( 0..2, ), diff --git a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@keys.graphql-2.snap b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@keys.graphql-2.snap index 2c5f8b658e..66fa8d11b2 100644 --- a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@keys.graphql-2.snap +++ b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@keys.graphql-2.snap @@ -89,7 +89,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/ke None, ), ], - star: None, range: Some( 0..6, ), @@ -204,7 +203,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/ke None, ), ], - star: None, range: Some( 0..6, ), @@ -301,7 +299,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/ke None, ), ], - star: None, range: Some( 0..6, ), @@ -416,7 +413,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/ke None, ), ], - star: None, range: Some( 0..6, ), @@ -554,7 +550,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/ke }, ), ], - star: None, range: Some( 0..17, ), @@ -668,7 +663,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/ke }, ), ], - star: None, range: Some( 0..12, ), @@ -704,7 +698,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/ke None, ), ], - star: None, range: Some( 0..6, ), @@ -818,7 +811,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/ke }, ), ], - star: None, range: Some( 0..12, ), @@ -895,7 +887,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/ke }, ), ], - star: None, range: Some( 0..17, ), diff --git a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@normalize_names.graphql-2.snap b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@normalize_names.graphql-2.snap index 69383df6a6..c8815a48cd 100644 --- a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@normalize_names.graphql-2.snap +++ b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@normalize_names.graphql-2.snap @@ -72,7 +72,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/no None, ), ], - star: None, range: Some( 0..4, ), @@ -174,7 +173,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/no None, ), ], - star: None, range: Some( 0..6, ), diff --git a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@realistic.graphql-2.snap b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@realistic.graphql-2.snap index 8c441fce20..b5cbdc6dfe 100644 --- a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@realistic.graphql-2.snap +++ b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@realistic.graphql-2.snap @@ -229,7 +229,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/re None, ), ], - star: None, range: Some( 81..92, ), @@ -237,7 +236,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/re ), ), ], - star: None, range: Some( 49..94, ), @@ -245,7 +243,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/re ), ), ], - star: None, range: Some( 12..96, ), @@ -285,7 +282,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/re None, ), ], - star: None, range: Some( 0..2, ), @@ -406,7 +402,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/re }, ), ], - star: None, range: Some( 0..24, ), @@ -442,7 +437,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/re None, ), ], - star: None, range: Some( 0..7, ), @@ -588,7 +582,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/re None, ), ], - star: None, range: Some( 16..45, ), @@ -596,7 +589,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/re ), ), ], - star: None, range: Some( 0..45, ), @@ -808,7 +800,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/re None, ), ], - star: None, range: Some( 73..94, ), @@ -816,7 +807,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/re ), ), ], - star: None, range: Some( 31..96, ), @@ -909,7 +899,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/re None, ), ], - star: None, range: Some( 119..156, ), @@ -917,7 +906,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/re ), ), ], - star: None, range: Some( 0..156, ), diff --git a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@simple.graphql-2.snap b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@simple.graphql-2.snap index 9807ad9764..710787097a 100644 --- a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@simple.graphql-2.snap +++ b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@simple.graphql-2.snap @@ -72,7 +72,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/si None, ), ], - star: None, range: Some( 0..4, ), @@ -174,7 +173,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/si None, ), ], - star: None, range: Some( 0..6, ), @@ -301,7 +299,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/si }, ), ], - star: None, range: Some( 0..15, ), diff --git a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@steelthread.graphql-2.snap b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@steelthread.graphql-2.snap index 95c4f11aa2..1b012f007a 100644 --- a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@steelthread.graphql-2.snap +++ b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@steelthread.graphql-2.snap @@ -80,7 +80,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/st None, ), ], - star: None, range: Some( 0..7, ), @@ -189,31 +188,8 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/st None, ), ], - star: Some( - StarSelection { - alias: Some( - Alias { - name: WithRange { - node: Field( - "rest", - ), - range: Some( - 17..21, - ), - }, - range: Some( - 17..22, - ), - }, - ), - selection: None, - range: Some( - 17..24, - ), - }, - ), range: Some( - 0..24, + 0..16, ), }, ), diff --git a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@steelthread.graphql-3.snap b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@steelthread.graphql-3.snap index fc570a24fc..f4f8e63c29 100644 --- a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@steelthread.graphql-3.snap +++ b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@steelthread.graphql-3.snap @@ -47,13 +47,10 @@ enum join__Graph { GRAPHQL @join__graph(name: "graphql", url: "https://localhost:4001") } -scalar JSON @join__type(graph: CONNECTORS_QUERY_USER_0) - type User @join__type(graph: CONNECTORS_QUERY_USER_0, key: "id") @join__type(graph: CONNECTORS_QUERY_USERS_0) @join__type(graph: CONNECTORS_USER_D_1, key: "c") @join__type(graph: GRAPHQL, key: "id") { id: ID! @join__field(graph: CONNECTORS_QUERY_USER_0) @join__field(graph: CONNECTORS_QUERY_USERS_0) @join__field(graph: GRAPHQL) name: String @join__field(graph: CONNECTORS_QUERY_USER_0) @join__field(graph: CONNECTORS_QUERY_USERS_0) username: String @join__field(graph: CONNECTORS_QUERY_USER_0) - rest: JSON @join__field(graph: CONNECTORS_QUERY_USER_0) d: String @join__field(graph: CONNECTORS_USER_D_1) c: String @join__field(graph: CONNECTORS_USER_D_1) @join__field(graph: GRAPHQL) } diff --git a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@steelthread.graphql.snap b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@steelthread.graphql.snap index 0b9fc82244..3431ded5db 100644 --- a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@steelthread.graphql.snap +++ b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@steelthread.graphql.snap @@ -21,7 +21,6 @@ type User { id: ID! name: String username: String - rest: JSON c: String d: String } diff --git a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@types_used_twice.graphql-2.snap b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@types_used_twice.graphql-2.snap index a8d3d0a249..c40ac1b587 100644 --- a/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@types_used_twice.graphql-2.snap +++ b/apollo-federation/src/sources/connect/expand/tests/snapshots/it_expand_supergraph@types_used_twice.graphql-2.snap @@ -73,7 +73,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/ty None, ), ], - star: None, range: Some( 2..8, ), @@ -119,7 +118,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/ty None, ), ], - star: None, range: Some( 15..21, ), @@ -127,7 +125,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/ty ), ), ], - star: None, range: Some( 11..23, ), @@ -135,7 +132,6 @@ input_file: apollo-federation/src/sources/connect/expand/tests/schemas/expand/ty ), ), ], - star: None, range: Some( 0..23, ), diff --git a/apollo-federation/src/sources/connect/expand/visitors/mod.rs b/apollo-federation/src/sources/connect/expand/visitors/mod.rs index 7369616172..2d2c46fa7d 100644 --- a/apollo-federation/src/sources/connect/expand/visitors/mod.rs +++ b/apollo-federation/src/sources/connect/expand/visitors/mod.rs @@ -198,7 +198,6 @@ mod tests { use crate::sources::connect::expand::visitors::GroupVisitor; use crate::sources::connect::json_selection::NamedSelection; use crate::sources::connect::JSONSelection; - use crate::sources::connect::Key; use crate::sources::connect::SubSelection; /// Visitor for tests. @@ -263,19 +262,6 @@ mod tests { .selections_iter() .sorted_by_key(|s| s.names()) .cloned() - .chain( - group - .star_iter() - // We just need a field name here - // This relies on validation to enforce the presence of an alias - .map(|s| { - NamedSelection::Field( - s.alias().cloned(), - Key::field("").into_with_range(), - None, - ) - }), - ) .collect()) } @@ -335,23 +321,6 @@ mod tests { "###); } - #[test] - fn it_iterates_rest() { - let mut visited = Vec::new(); - let visitor = TestVisitor::new(&mut visited); - let (unmatched, selection) = JSONSelection::parse("a b rest: *").unwrap(); - assert!(unmatched.is_empty()); - - visitor - .walk(selection.next_subselection().cloned().unwrap()) - .unwrap(); - assert_snapshot!(print_visited(visited), @r###" - a - b - rest - "###); - } - #[test] fn it_iterates_over_nested_selection() { let mut visited = Vec::new(); diff --git a/apollo-federation/src/sources/connect/expand/visitors/selection.rs b/apollo-federation/src/sources/connect/expand/visitors/selection.rs index bcb14bf6b3..18bc549f81 100644 --- a/apollo-federation/src/sources/connect/expand/visitors/selection.rs +++ b/apollo-federation/src/sources/connect/expand/visitors/selection.rs @@ -20,7 +20,6 @@ use crate::error::FederationError; use crate::schema::position::ObjectTypeDefinitionPosition; use crate::schema::position::TypeDefinitionPosition; use crate::sources::connect::json_selection::NamedSelection; -use crate::sources::connect::Key; use crate::sources::connect::SubSelection; /// Type alias for JSONSelection group info @@ -184,9 +183,6 @@ impl GroupVisitor .selections_iter() .sorted_by_key(|s| s.names()) .cloned() - .chain(group.star_iter().map(|s| { - NamedSelection::Field(s.alias().cloned(), Key::field("").into_with_range(), None) - })) .collect()) } diff --git a/apollo-federation/src/sources/connect/json_selection/README.md b/apollo-federation/src/sources/connect/json_selection/README.md index 9dee51f574..a77b4d9a99 100644 --- a/apollo-federation/src/sources/connect/json_selection/README.md +++ b/apollo-federation/src/sources/connect/json_selection/README.md @@ -81,9 +81,8 @@ worry if it doesn't seem helpful yet, as every rule will be explained in detail below. ```ebnf -JSONSelection ::= PathSelection | NakedSubSelection -NakedSubSelection ::= NamedSelection* StarSelection? -SubSelection ::= "{" NakedSubSelection "}" +JSONSelection ::= PathSelection | NamedSelection* +SubSelection ::= "{" NamedSelection* "}" NamedSelection ::= NamedPathSelection | PathWithSubSelection | NamedFieldSelection | NamedGroupSelection NamedPathSelection ::= Alias PathSelection NamedFieldSelection ::= Alias? Key SubSelection? @@ -107,7 +106,6 @@ LitNumber ::= "-"? ([0-9]+ ("." [0-9]*)? | "." [0-9]+) LitObject ::= "{" (LitProperty ("," LitProperty)* ","?)? "}" LitProperty ::= Key ":" LitExpr LitArray ::= "[" (LitExpr ("," LitExpr)* ","?)? "]" -StarSelection ::= Alias? "*" SubSelection? NO_SPACE ::= !SpacesOrComments SpacesOrComments ::= (Spaces | Comment)+ Spaces ::= ("⎵" | "\t" | "\r" | "\n")+ @@ -222,7 +220,7 @@ feel free to take your time and enjoy the journey. The `JSONSelection` non-terminal is the top-level entry point for the grammar, and appears nowhere else within the rest of the grammar. It can be either a `PathSelection` (for selecting a single anonymous value from a given path) or a -`NakedSubSelection` (for selecting multiple named items). +`NamedSelection*` (for selecting zero or more named items). When the `PathSelection` syntax is chosen at this level, and the path does not have a trailing `SubSelection` (which ensures the result is an object with @@ -230,36 +228,15 @@ statically known properties), the entire `JSONSelection` must be that single path, without any other named selections. If the `PathSelection` does have a trailing `SubSelection`, it may be mixed together with other named selections, though in that case it will be parsed as a `PathWithSubSelection` within a -`NakedSubSelection`, instead of a `PathSelection`. - -### `NakedSubSelection ::=` - -![NakedSubSelection](./grammar/NakedSubSelection.svg) - -A `NakedSubSelection` is a `SubSelection` without the surrounding `{` and `}` -braces. It can appear at the top level of a `JSONSelection`, but otherwise -appears only as part of the `SubSelection` rule, meaning it must have braces -everywhere except at the top level. - -Because a `NakedSubSelection` can contain any number of `NamedSelection` items -(including zero), and may have no `StarSelection`, it's possible for the -`NakedSelection` to be fully empty. In these unusual cases, whitespace and -comments are still allowed, and the result of the selection will always be an -empty object. - -In the Rust implementation, there is no dedicated `NakedSubSelection` struct, as -we use the `SubSelection` struct to represent the meaningful contents of the -selection, regardless of whether it has braces. The `NakedSubSelection` -non-terminal is just a grammatical convenience, to avoid repetition between -`JSONSelection` and `SubSelection`. +`SubSelection`, instead of a standalone `PathSelection`. ### `SubSelection ::=` ![SubSelection](./grammar/SubSelection.svg) -A `SubSelection` is a `NakedSubSelection` surrounded by `{` and `}`, and is used -to select specific properties from the preceding object, much like a nested -selection set in a GraphQL operation. +A `SubSelection` is a sequence of zero or more `NamedSelection` items surrounded +by `{` and `}`, and is used to select specific properties from the preceding +object, much like a nested selection set in a GraphQL operation. Note that `SubSelection` may appear recursively within itself, as part of one of the various `NamedSelection` rules. This recursion allows for arbitrarily deep @@ -269,9 +246,9 @@ nesting of selections, which is necessary to handle complex JSON structures. ![NamedSelection](./grammar/NamedSelection.svg) -Every possible production of the `NamedSelection` non-terminal corresponds to a -named property in the output object, though each one obtains its value from the -input object in a slightly different way. +Every production of the `NamedSelection` non-terminal adds named properties to +the output object, though they obtain their properties/values from the input +object in different ways. ### `NamedPathSelection ::=` @@ -281,63 +258,49 @@ Since `PathSelection` returns an anonymous value extracted from the given path, if you want to use a `PathSelection` alongside other `NamedSelection` items, you can prefix it with an `Alias`, turning it into a `NamedPathSelection`. -For example, you cannot omit the `pathName:` alias in the following -`NakedSubSelection`, because `some.nested.path` has no output name by itself: +For example, the `abc:` alias in this example causes the `{ a b c }` object +selected from `some.nested.path` to be nested under an `abc` output key: ```graphql -position { x y } -pathName: some.nested.path { a b c } -scalarField +id +name +abc: some.nested.path { a b c } ``` -The ordering of alternatives in the `NamedSelection` rule is important, so the -`NamedPathSelection` alternative can be considered before `NamedFieldSelection` -and `NamedQuotedSelection`, because a `NamedPathSelection` such as `pathName: -some.nested.path` has a prefix that looks like a `NamedFieldSelection`: -`pathName: some`, causing an error when the parser encounters the remaining -`.nested.path` text. Some parsers would resolve this ambiguity by forbidding `.` -in the lookahead for `Named{Field,Quoted}Selection`, but negative lookahead is -tricky for this parser (see similar discussion regarding `NO_SPACE`), so instead -we greedily parse `NamedPathSelection` first, when possible, since that ensures -the whole path will be consumed. +This selection produces an output object with keys `id`, `name`, and `abc`, +where `abc` is an object with keys `a`, `b`, and `c`. + +#### Related syntax: `PathWithSubSelection` + +You can also flatten the `{ a b c }` properties to the top level by omitting the +alias, but this syntax parses as a `PathWithSubSelection` instead of a +`NamedPathSelection`: + +```graphql +id +name +some.nested.path { a b c } +``` + +This selection produces an output object with keys `id`, `name`, `a`, `b`, and +`c`. Additionally, `some.nested.path` must evaluate to a single object, rather +than an array objects or a scalar value, which is not a limitation of the +`NamedPathSelection` syntax. ### `NamedFieldSelection ::=` ![NamedFieldSelection](./grammar/NamedFieldSelection.svg) The `NamedFieldSelection` non-terminal is the option most closely resembling -GraphQL field selections, where the field name must be an `Identifier`, may have -an `Alias`, and may have a `SubSelection` to select nested properties (which -requires the field's value to be an object rather than a scalar). +GraphQL field selections, where the field name must be `Key` (`Identifier` or +quoted string literal), may have an `Alias`, and may have a `SubSelection` to +select nested properties (assuming the field's value is an object). In practice, whitespace is often required to keep multiple consecutive `NamedFieldSelection` identifiers separate, but is not strictly necessary when there is no ambiguity, as when an identifier follows a preceding subselection: `a{b}c`. -### `NamedQuotedSelection ::=` - -![NamedQuotedSelection](./grammar/NamedQuotedSelection.svg) - -Since arbitrary JSON objects can have properties that are not identifiers, we -need a version of `NamedFieldSelection` that allows for quoted property names as -opposed to identifiers. - -However, since our goal is always to produce an output that is safe for GraphQL -consumption, an `Alias` is strictly required in this case, and it must be a -valid GraphQL `Identifier`: - -```graphql -first -second: "second property" { x y z } -third { a b } -``` - -Besides extracting the `first` and `third` fields in typical GraphQL fashion, -this selection extracts the `second property` field as `second`, subselecting -`x`, `y`, and `z` from the extracted object. The final object will have the -properties `first`, `second`, and `third`. - ### `NamedGroupSelection ::=` ![NamedGroupSelection](./grammar/NamedGroupSelection.svg) @@ -386,8 +349,7 @@ Analogous to a GraphQL alias, the `Alias` syntax allows for renaming properties from the input JSON to match the desired output shape. In addition to renaming, `Alias` can provide names to otherwise anonymous -structures, such as those selected by `PathSelection`, `NamedGroupSelection`, or -`StarSelection` syntax. +structures, such as those selected by `PathSelection` or `NamedGroupSelection`. ### `Path ::=` @@ -432,7 +394,7 @@ type Query { ``` If you need to select other named properties, you can still use a -`PathSelection` as part of a `NakedSubSelection`, as long as you give it an +`PathSelection` within a `NamedSelection*` sequence, as long as you give it an `Alias`: ```graphql @@ -604,7 +566,7 @@ $.data { id name } This will produce a single object with `id` and `name` fields, without the enclosing `data` property. Equivalently, you could manually unroll this example -to the following `NakedSubSelection`: +to the following `NamedSelection*` sequence: ```graphql id: data.id @@ -614,12 +576,6 @@ name: data.name In this case, the `$.` is no longer necessary because `data.id` and `data.name` are unambiguously `KeyPath` selections. -> For backwards compatibility with earlier versions of the `JSONSelection` -syntax that did not support the `$` variable, you can also use a leading `.` -character (so `.data { id name }`, or even `.data.id` or `.data.name`) to mean -the same thing as `$.`, but this is no longer recommended, since `.data` is easy -to mistype and misread, compared to `$.data`. - ### `AtPath ::=` ![AtPath](./grammar/AtPath.svg) @@ -925,61 +881,6 @@ A list of `LitExpr` items within square brackets, as in JavaScript. Trailing commas are not currently allowed, but could be supported in the future. -### `StarSelection ::=` - -![StarSelection](./grammar/StarSelection.svg) - -The `StarSelection` non-terminal is uncommon when working with GraphQL, since it -selects all remaining properties of an object, which can be difficult to -represent using static GraphQL types, without resorting to the catch-all `JSON` -scalar type. Still, a `StarSelection` can be useful for consuming JSON -dictionaries with dynamic keys, or for capturing unexpected properties for -debugging purposes. - -When used, a `StarSelection` must come after any `NamedSelection` items within a -given `NakedSubSelection`. - -A common use case for `StarSelection` is capturing all properties not otherwise -selected using a field called `allOtherFields`, which must have a generic `JSON` -type in the GraphQL schema: - -```graphql -knownField -anotherKnownField -allOtherFields: * -``` - -Note that `knownField` and `anotherKnownField` will not be included in the -`allOtherFields` output, since they are selected explicitly. In this sense, the -`*` functions a bit like object `...rest` syntax in JavaScript. - -If you happen to know these other fields all have certain properties, you can -restrict the `*` selection to just those properties: - -```graphql -knownField { id name } -allOtherFields: * { id } -``` - -Sometimes a REST API will return a dictionary result with an unknown set of -dynamic keys but values of some known type, such as a map of ISBN numbers to -`Book` objects: - -```graphql -booksByISBN: result.books { * { title author { name } } -``` - -Because the set of ISBN numbers is statically unknowable, the type of -`booksByISBN` would have to be `JSON` in the GraphQL schema, but it can still be -useful to select known properties from the `Book` objects within the -`result.books` dictionary, so you don't return more GraphQL data than necessary. - -The grammar technically allows a `StarSelection` with neither an `Alias` nor a -`SubSelection`, but this is not a useful construct from a GraphQL perspective, -since it provides no output fields that can be reliably typed by a GraphQL -schema. This form has some use cases when working with `JSONSelection` outside -of GraphQL, but they are not relevant here. - ### `NO_SPACE ::= !SpacesOrComments` The `NO_SPACE` non-terminal is used to enforce the absence of whitespace or diff --git a/apollo-federation/src/sources/connect/json_selection/apply_to.rs b/apollo-federation/src/sources/connect/json_selection/apply_to.rs index 5fd93ff291..a0cb741f9b 100644 --- a/apollo-federation/src/sources/connect/json_selection/apply_to.rs +++ b/apollo-federation/src/sources/connect/json_selection/apply_to.rs @@ -511,112 +511,22 @@ impl ApplyToInternal for SubSelection { vars }; - let (data_map, data_really_primitive) = match data { - JSON::Object(data_map) => (data_map.clone(), false), - _primitive => (JSONMap::new(), true), - }; - let mut output = JSONMap::new(); let mut errors = Vec::new(); - let mut input_names = IndexSet::default(); for named_selection in self.selections.iter() { let (value, apply_errors) = named_selection.apply_to_path(data, &vars, input_path); errors.extend(apply_errors); - // If value is an object, extend output with its keys and their values. - if let Some(JSON::Object(key_and_value)) = value { - output.extend(key_and_value); - } - - // If there is a star selection, we need to keep track of the - // *original* names of the fields that were explicitly selected, - // because we will need to omit them from what the * matches. - if self.star.is_some() { - match named_selection { - NamedSelection::Field(_, name, _) => { - input_names.insert(name.as_str()); - } - NamedSelection::Path(_, path_selection) => { - if let PathList::Key(key, _) = path_selection.path.as_ref() { - input_names.insert(key.as_str()); - } - } - // The contents of groups do not affect the keys matched by - // * selections in the parent object (outside the group). - NamedSelection::Group(_, _) => {} - }; + if let Some(JSON::Object(named_properties)) = value { + output.extend(named_properties); } } - match &self.star { - // Aliased but not subselected, e.g. "a b c rest: *" - Some(StarSelection { - alias: Some(alias), - selection: None, - .. - }) => { - let mut star_output = JSONMap::new(); - for (key, value) in &data_map { - if !input_names.contains(key.as_str()) { - star_output.insert(key.clone(), value.clone()); - } - } - output.insert(alias.name(), JSON::Object(star_output)); - } - // Aliased and subselected, e.g. "alias: * { hello }" - Some(StarSelection { - alias: Some(alias), - selection: Some(selection), - .. - }) => { - let mut star_output = JSONMap::new(); - for (key, value) in &data_map { - if !input_names.contains(key.as_str()) { - let (selected_opt, apply_errors) = - selection.apply_to_path(value, &vars, input_path); - errors.extend(apply_errors); - if let Some(selected) = selected_opt { - star_output.insert(key.clone(), selected); - } - } - } - output.insert(alias.name(), JSON::Object(star_output)); - } - // Not aliased but subselected, e.g. "parent { * { hello } }" - Some(StarSelection { - alias: None, - selection: Some(selection), - .. - }) => { - for (key, value) in &data_map { - if !input_names.contains(key.as_str()) { - let (selected_opt, apply_errors) = - selection.apply_to_path(value, &vars, input_path); - errors.extend(apply_errors); - if let Some(selected) = selected_opt { - output.insert(key.clone(), selected); - } - } - } - } - // Neither aliased nor subselected, e.g. "parent { * }" or just "*" - Some(StarSelection { - alias: None, - selection: None, - .. - }) => { - for (key, value) in &data_map { - if !input_names.contains(key.as_str()) { - output.insert(key.clone(), value.clone()); - } - } - } - // No * selection present, e.g. "parent { just some properties }" - None => {} - }; - - if data_really_primitive && output.is_empty() { + // If data was a primitive value (neither array nor object), and no + // output properties were generated, return data as is, along with any + // errors that occurred. + if !matches!(data, JSON::Object(_)) && output.is_empty() { return (Some(data.clone()), errors); } @@ -779,179 +689,6 @@ mod tests { ); } - #[test] - fn test_apply_to_star_selections() { - let data = json!({ - "englishAndGreekLetters": { - "a": { "en": "ay", "gr": "alpha" }, - "b": { "en": "bee", "gr": "beta" }, - "c": { "en": "see", "gr": "gamma" }, - "d": { "en": "dee", "gr": "delta" }, - "e": { "en": "ee", "gr": "epsilon" }, - "f": { "en": "eff", "gr": "phi" }, - }, - "englishAndSpanishNumbers": [ - { "en": "one", "es": "uno" }, - { "en": "two", "es": "dos" }, - { "en": "three", "es": "tres" }, - { "en": "four", "es": "cuatro" }, - { "en": "five", "es": "cinco" }, - { "en": "six", "es": "seis" }, - ], - "asciiCharCodes": { - "A": 65, - "B": 66, - "C": 67, - "D": 68, - "E": 69, - "F": 70, - "G": 71, - }, - "books": { - "9780262533751": { - "title": "The Geometry of Meaning", - "author": "Peter Gärdenfors", - }, - "978-1492674313": { - "title": "P is for Pterodactyl: The Worst Alphabet Book Ever", - "author": "Raj Haldar", - }, - "9780262542456": { - "title": "A Biography of the Pixel", - "author": "Alvy Ray Smith", - }, - } - }); - - let check_ok = |selection: JSONSelection, expected_json: JSON| { - let (actual_json, errors) = selection.apply_to(&data); - assert_eq!(actual_json, Some(expected_json)); - assert_eq!(errors, vec![]); - }; - - check_ok( - selection!("englishAndGreekLetters { * { en }}"), - json!({ - "englishAndGreekLetters": { - "a": { "en": "ay" }, - "b": { "en": "bee" }, - "c": { "en": "see" }, - "d": { "en": "dee" }, - "e": { "en": "ee" }, - "f": { "en": "eff" }, - }, - }), - ); - - check_ok( - selection!("englishAndGreekLetters { C: c.en * { gr }}"), - json!({ - "englishAndGreekLetters": { - "a": { "gr": "alpha" }, - "b": { "gr": "beta" }, - "C": "see", - "d": { "gr": "delta" }, - "e": { "gr": "epsilon" }, - "f": { "gr": "phi" }, - }, - }), - ); - - check_ok( - selection!("englishAndGreekLetters { A: a B: b rest: * }"), - json!({ - "englishAndGreekLetters": { - "A": { "en": "ay", "gr": "alpha" }, - "B": { "en": "bee", "gr": "beta" }, - "rest": { - "c": { "en": "see", "gr": "gamma" }, - "d": { "en": "dee", "gr": "delta" }, - "e": { "en": "ee", "gr": "epsilon" }, - "f": { "en": "eff", "gr": "phi" }, - }, - }, - }), - ); - - check_ok( - selection!("$.'englishAndSpanishNumbers' { en rest: * }"), - json!([ - { "en": "one", "rest": { "es": "uno" } }, - { "en": "two", "rest": { "es": "dos" } }, - { "en": "three", "rest": { "es": "tres" } }, - { "en": "four", "rest": { "es": "cuatro" } }, - { "en": "five", "rest": { "es": "cinco" } }, - { "en": "six", "rest": { "es": "seis" } }, - ]), - ); - - // To include/preserve all remaining properties from an object in the output - // object, we support a naked * selection (no alias or subselection). This - // is useful when the values of the properties are scalar, so a subselection - // isn't possible, and we want to preserve all properties of the original - // object. These unnamed properties may not be useful for GraphQL unless the - // whole object is considered as opaque JSON scalar data, but we still need - // to support preserving JSON when it has scalar properties. - check_ok( - selection!("asciiCharCodes { ay: A bee: B * }"), - json!({ - "asciiCharCodes": { - "ay": 65, - "bee": 66, - "C": 67, - "D": 68, - "E": 69, - "F": 70, - "G": 71, - }, - }), - ); - - check_ok( - selection!("asciiCharCodes { * } gee: asciiCharCodes.G"), - json!({ - "asciiCharCodes": data.get("asciiCharCodes").unwrap(), - "gee": 71, - }), - ); - - check_ok( - selection!("books { * { title } }"), - json!({ - "books": { - "9780262533751": { - "title": "The Geometry of Meaning", - }, - "978-1492674313": { - "title": "P is for Pterodactyl: The Worst Alphabet Book Ever", - }, - "9780262542456": { - "title": "A Biography of the Pixel", - }, - }, - }), - ); - - check_ok( - selection!("books { authorsByISBN: * { author } }"), - json!({ - "books": { - "authorsByISBN": { - "9780262533751": { - "author": "Peter Gärdenfors", - }, - "978-1492674313": { - "author": "Raj Haldar", - }, - "9780262542456": { - "author": "Alvy Ray Smith", - }, - }, - }, - }), - ); - } - #[test] fn test_apply_to_errors() { let data = json!({ diff --git a/apollo-federation/src/sources/connect/json_selection/grammar/JSONSelection.svg b/apollo-federation/src/sources/connect/json_selection/grammar/JSONSelection.svg index b4e7a0b653..d12580f0e9 100644 --- a/apollo-federation/src/sources/connect/json_selection/grammar/JSONSelection.svg +++ b/apollo-federation/src/sources/connect/json_selection/grammar/JSONSelection.svg @@ -1,5 +1,5 @@ - + - - + + - - - PathSelection + xlink:href="#NamedSelection" + xlink:title="NamedSelection"> + + + NamedSelection - - - NakedSubSelection + xlink:href="#PathSelection" + xlink:title="PathSelection"> + + + PathSelection - - + d="m17 51 h2 m40 0 h10 m0 0 h132 m-162 0 l20 0 m-1 0 q-9 0 -9 -10 l0 -14 q0 -10 10 -10 m142 34 l20 0 m-20 0 q10 0 10 -10 l0 -14 q0 -10 -10 -10 m-142 0 h10 m122 0 h10 m-182 34 h20 m182 0 h20 m-222 0 q10 0 10 10 m202 0 q0 -10 10 -10 m-212 10 v12 m202 0 v-12 m-202 12 q0 10 10 10 m182 0 q10 0 10 -10 m-192 10 h10 m106 0 h10 m0 0 h56 m23 -32 h-3"/> + + diff --git a/apollo-federation/src/sources/connect/json_selection/grammar/NakedSubSelection.svg b/apollo-federation/src/sources/connect/json_selection/grammar/NakedSubSelection.svg deleted file mode 100644 index ffcfa089f1..0000000000 --- a/apollo-federation/src/sources/connect/json_selection/grammar/NakedSubSelection.svg +++ /dev/null @@ -1,52 +0,0 @@ - - - - - - - - - - - NamedSelection - - - - - StarSelection - - - - - diff --git a/apollo-federation/src/sources/connect/json_selection/grammar/StarSelection.svg b/apollo-federation/src/sources/connect/json_selection/grammar/StarSelection.svg deleted file mode 100644 index 4e09fae7df..0000000000 --- a/apollo-federation/src/sources/connect/json_selection/grammar/StarSelection.svg +++ /dev/null @@ -1,60 +0,0 @@ - - - - - - - - - - - Alias - - - - * - - - - SubSelection - - - - - diff --git a/apollo-federation/src/sources/connect/json_selection/grammar/SubSelection.svg b/apollo-federation/src/sources/connect/json_selection/grammar/SubSelection.svg index 913b60aa0c..6c912b7439 100644 --- a/apollo-federation/src/sources/connect/json_selection/grammar/SubSelection.svg +++ b/apollo-federation/src/sources/connect/json_selection/grammar/SubSelection.svg @@ -1,5 +1,5 @@ - + - - - + + + - { + { - - - NakedSubSelection + xlink:href="#NamedSelection" + xlink:title="NamedSelection"> + + + NamedSelection - - + - } + } - - + d="m17 51 h2 m0 0 h10 m28 0 h10 m20 0 h10 m0 0 h132 m-162 0 l20 0 m-1 0 q-9 0 -9 -10 l0 -14 q0 -10 10 -10 m142 34 l20 0 m-20 0 q10 0 10 -10 l0 -14 q0 -10 -10 -10 m-142 0 h10 m122 0 h10 m20 34 h10 m28 0 h10 m3 0 h-3"/> + + diff --git a/apollo-federation/src/sources/connect/json_selection/location.rs b/apollo-federation/src/sources/connect/json_selection/location.rs index ffeed11989..7aee13945f 100644 --- a/apollo-federation/src/sources/connect/json_selection/location.rs +++ b/apollo-federation/src/sources/connect/json_selection/location.rs @@ -207,22 +207,11 @@ pub(crate) mod strip_ranges { fn strip_ranges(&self) -> Self { SubSelection { selections: self.selections.iter().map(|s| s.strip_ranges()).collect(), - star: self.star.as_ref().map(|s| s.strip_ranges()), ..Default::default() } } } - impl StripRanges for StarSelection { - fn strip_ranges(&self) -> Self { - StarSelection { - alias: self.alias.as_ref().map(|a| a.strip_ranges()), - selection: self.selection.as_ref().map(|s| Box::new(s.strip_ranges())), - range: None, - } - } - } - impl StripRanges for Alias { fn strip_ranges(&self) -> Self { Alias { @@ -326,7 +315,6 @@ mod tests { group: { a b c } arg: $args . arg field - rest: * { data } } "#, ) diff --git a/apollo-federation/src/sources/connect/json_selection/parser.rs b/apollo-federation/src/sources/connect/json_selection/parser.rs index b0264995c4..3a96bd5e94 100644 --- a/apollo-federation/src/sources/connect/json_selection/parser.rs +++ b/apollo-federation/src/sources/connect/json_selection/parser.rs @@ -53,9 +53,7 @@ impl JSONSelection { pub fn is_empty(&self) -> bool { match self { - JSONSelection::Named(subselect) => { - subselect.selections.is_empty() && subselect.star.is_none() - } + JSONSelection::Named(subselect) => subselect.selections.is_empty(), JSONSelection::Path(path) => *path.path == PathList::Empty, } } @@ -688,7 +686,6 @@ impl ExternalVarPaths for PathList { #[derive(Debug, PartialEq, Eq, Clone, Default)] pub struct SubSelection { pub(super) selections: Vec, - pub(super) star: Option, pub(super) range: OffsetRange, } @@ -716,7 +713,6 @@ impl SubSelection { remainder, Self { selections: sub.selections, - star: sub.star, range, }, ) @@ -724,34 +720,15 @@ impl SubSelection { } fn parse_naked(input: Span) -> IResult { - tuple(( - spaces_or_comments, - many0(NamedSelection::parse), - // Note that when a * selection is used, it must be the last - // selection in the SubSelection, since it does not count as a - // NamedSelection, and is stored as a separate field from the - // selections vector. - opt(StarSelection::parse), - ))(input) - .map(|(remainder, (_, selections, star))| { - let range = merge_ranges( - selections.first().and_then(|first| first.range()), - selections.last().and_then(|last| last.range()), - ); - let range = if let Some(star) = star.as_ref() { - merge_ranges(range, star.range()) - } else { - range - }; - ( - remainder, - Self { - selections, - star, - range, - }, - ) - }) + preceded(spaces_or_comments, many0(NamedSelection::parse))(input).map( + |(remainder, selections)| { + let range = merge_ranges( + selections.first().and_then(|first| first.range()), + selections.last().and_then(|last| last.range()), + ); + (remainder, Self { selections, range }) + }, + ) } // Returns an Iterator over each &NamedSelection that contributes a single @@ -791,18 +768,6 @@ impl SubSelection { selections.into_iter() } - pub fn has_star(&self) -> bool { - self.star.is_some() - } - - pub fn set_star(&mut self, star: Option) { - self.star = star; - } - - pub fn star_iter(&self) -> impl Iterator { - self.star.iter() - } - pub fn append_selection(&mut self, selection: NamedSelection) { self.selections.push(selection); } @@ -822,60 +787,6 @@ impl ExternalVarPaths for SubSelection { } } -// StarSelection ::= Alias? "*" SubSelection? - -#[derive(Debug, PartialEq, Eq, Clone, Default)] -pub struct StarSelection { - pub(super) alias: Option, - pub(super) selection: Option>, - pub(super) range: OffsetRange, -} - -impl Ranged for StarSelection { - fn range(&self) -> OffsetRange { - self.range.clone() - } -} - -impl StarSelection { - pub(crate) fn alias(&self) -> Option<&Alias> { - self.alias.as_ref() - } - - pub(crate) fn parse(input: Span) -> IResult { - tuple(( - // The spaces_or_comments separators are necessary here because - // Alias::parse and SubSelection::parse only consume surrounding - // spaces when they match, and they are both optional here. - opt(Alias::parse), - spaces_or_comments, - ranged_span("*"), - opt(SubSelection::parse), - ))(input) - .map(|(remainder, (alias, _, star, selection))| { - let range = star.range(); - let range = if let Some(alias) = alias.as_ref() { - merge_ranges(alias.range(), range) - } else { - range - }; - let range = if let Some(selection) = selection.as_ref() { - merge_ranges(range, selection.range()) - } else { - range - }; - ( - remainder, - Self { - alias, - selection: selection.map(Box::new), - range, - }, - ) - }) - } -} - // Alias ::= Key ":" #[derive(Debug, PartialEq, Eq, Clone)] @@ -2624,171 +2535,6 @@ mod tests { ); } - #[test] - fn test_star_selection() { - fn check_parsed(input: &str, expected: StarSelection) { - let (remainder, parsed) = StarSelection::parse(Span::new(input)).unwrap(); - assert!(span_is_all_spaces_or_comments(remainder)); - assert_eq!(parsed.strip_ranges(), expected); - } - - check_parsed( - "rest: *", - StarSelection { - alias: Some(Alias::new("rest")), - ..Default::default() - }, - ); - - check_parsed( - "*", - StarSelection { - ..Default::default() - }, - ); - - check_parsed( - " * ", - StarSelection { - ..Default::default() - }, - ); - check_parsed( - " * { hello } ", - StarSelection { - selection: Some(Box::new(SubSelection { - selections: vec![NamedSelection::Field( - None, - Key::field("hello").into_with_range(), - None, - )], - ..Default::default() - })), - ..Default::default() - }, - ); - - check_parsed( - "hi: * { hello }", - StarSelection { - alias: Some(Alias::new("hi")), - selection: Some(Box::new(SubSelection { - selections: vec![NamedSelection::Field( - None, - Key::field("hello").into_with_range(), - None, - )], - ..Default::default() - })), - ..Default::default() - }, - ); - - check_parsed( - "alias: * { x y z rest: * }", - StarSelection { - alias: Some(Alias::new("alias")), - selection: Some(Box::new(SubSelection { - selections: vec![ - NamedSelection::Field(None, Key::field("x").into_with_range(), None), - NamedSelection::Field(None, Key::field("y").into_with_range(), None), - NamedSelection::Field(None, Key::field("z").into_with_range(), None), - ], - star: Some(StarSelection { - alias: Some(Alias::new("rest")), - ..Default::default() - }), - ..Default::default() - })), - ..Default::default() - }, - ); - - assert_eq!( - selection!(" before alias: * { * { a b c } } ").strip_ranges(), - JSONSelection::Named(SubSelection { - selections: vec![NamedSelection::Field( - None, - Key::field("before").into_with_range(), - None - )], - star: Some(StarSelection { - alias: Some(Alias::new("alias")), - selection: Some(Box::new(SubSelection { - selections: vec![], - star: Some(StarSelection { - selection: Some(Box::new(SubSelection { - selections: vec![ - NamedSelection::Field( - None, - Key::field("a").into_with_range(), - None - ), - NamedSelection::Field( - None, - Key::field("b").into_with_range(), - None - ), - NamedSelection::Field( - None, - Key::field("c").into_with_range(), - None - ), - ], - ..Default::default() - })), - ..Default::default() - }), - ..Default::default() - })), - ..Default::default() - }), - ..Default::default() - }), - ); - - assert_eq!( - selection!(" before group: { * { a b c } } after ").strip_ranges(), - JSONSelection::Named(SubSelection { - selections: vec![ - NamedSelection::Field(None, Key::field("before").into_with_range(), None), - NamedSelection::Group( - Alias::new("group"), - SubSelection { - selections: vec![], - star: Some(StarSelection { - selection: Some(Box::new(SubSelection { - selections: vec![ - NamedSelection::Field( - None, - Key::field("a").into_with_range(), - None - ), - NamedSelection::Field( - None, - Key::field("b").into_with_range(), - None - ), - NamedSelection::Field( - None, - Key::field("c").into_with_range(), - None - ), - ], - ..Default::default() - })), - ..Default::default() - }), - ..Default::default() - }, - ), - NamedSelection::Field(None, Key::field("after").into_with_range(), None), - ], - ..Default::default() - }), - ); - } - #[test] fn test_external_var_paths() { fn parse(input: &str) -> PathSelection { @@ -2877,7 +2623,6 @@ mod tests { None, )], range: Some(0..5), - ..Default::default() }), ); @@ -2890,7 +2635,6 @@ mod tests { None, )], range: Some(2..7), - ..Default::default() }), ); @@ -2914,11 +2658,9 @@ mod tests { ), ], range: Some(9..20), - ..Default::default() }), )], range: Some(2..20), - ..Default::default() }), ); @@ -3014,7 +2756,6 @@ mod tests { ), ], range: Some(28..37), - ..Default::default() }), Some(28..37), ), @@ -3033,7 +2774,6 @@ mod tests { ), ], range: Some(0..42), - ..Default::default() }), ); } diff --git a/apollo-federation/src/sources/connect/json_selection/pretty.rs b/apollo-federation/src/sources/connect/json_selection/pretty.rs index 395f38f739..847dc91aa9 100644 --- a/apollo-federation/src/sources/connect/json_selection/pretty.rs +++ b/apollo-federation/src/sources/connect/json_selection/pretty.rs @@ -13,7 +13,6 @@ use crate::sources::connect::json_selection::MethodArgs; use crate::sources::connect::json_selection::NamedSelection; use crate::sources::connect::json_selection::PathList; use crate::sources::connect::json_selection::PathSelection; -use crate::sources::connect::json_selection::StarSelection; use crate::sources::connect::json_selection::SubSelection; impl std::fmt::Display for JSONSelection { @@ -77,17 +76,10 @@ impl PrettyPrintable for SubSelection { impl SubSelection { /// Prints all of the selections in a subselection fn print_subselections(&self, indentation: usize) -> String { - let selections = self - .selections + self.selections .iter() - .map(|s| s.pretty_print_with_indentation(false, indentation)); - - let star = self - .star - .as_ref() - .map(|s| s.pretty_print_with_indentation(false, indentation)); - - selections.chain(star).join("\n") + .map(|s| s.pretty_print_with_indentation(false, indentation)) + .join("\n") } } @@ -306,38 +298,12 @@ impl PrettyPrintable for NamedSelection { } } -impl PrettyPrintable for StarSelection { - fn pretty_print_with_indentation(&self, inline: bool, indentation: usize) -> String { - let mut result = String::new(); - - if !inline { - result.push_str(indent_chars(indentation).as_str()); - } - - if let Some(alias) = &self.alias { - result.push_str(alias.name.as_str()); - result.push_str(": "); - } - - result.push('*'); - - if let Some(sub) = &self.selection { - let sub = sub.pretty_print_with_indentation(true, indentation); - result.push(' '); - result.push_str(sub.as_str()); - } - - result - } -} - #[cfg(test)] mod tests { use super::super::location::Span; use crate::sources::connect::json_selection::pretty::indent_chars; use crate::sources::connect::json_selection::NamedSelection; use crate::sources::connect::json_selection::PrettyPrintable; - use crate::sources::connect::json_selection::StarSelection; use crate::sources::connect::JSONSelection; use crate::sources::connect::PathSelection; use crate::sources::connect::SubSelection; @@ -372,23 +338,6 @@ mod tests { ); } - #[test] - fn it_prints_a_star_selection() { - let (unmatched, star_selection) = StarSelection::parse(Span::new("rest: *")).unwrap(); - assert!(unmatched.is_empty()); - - test_permutations(star_selection, "rest: *"); - } - - #[test] - fn it_prints_a_star_selection_with_subselection() { - let (unmatched, star_selection) = - StarSelection::parse(Span::new("rest: * { a b }")).unwrap(); - assert!(unmatched.is_empty()); - - test_permutations(star_selection, "rest: * {\n a\n b\n}"); - } - #[test] fn it_prints_a_named_selection() { let selections = [ diff --git a/apollo-federation/src/sources/connect/json_selection/selection_set.rs b/apollo-federation/src/sources/connect/json_selection/selection_set.rs index 414ba58cc8..5bb317c87b 100644 --- a/apollo-federation/src/sources/connect/json_selection/selection_set.rs +++ b/apollo-federation/src/sources/connect/json_selection/selection_set.rs @@ -16,9 +16,6 @@ ) )] -use std::collections::HashSet; - -use apollo_compiler::collections::IndexSet; use apollo_compiler::executable::Field; use apollo_compiler::executable::Selection; use apollo_compiler::executable::SelectionSet; @@ -35,7 +32,6 @@ use super::parser::PathList; use crate::sources::connect::json_selection::Alias; use crate::sources::connect::json_selection::NamedSelection; use crate::sources::connect::JSONSelection; -use crate::sources::connect::Key; use crate::sources::connect::PathSelection; use crate::sources::connect::SubSelection; @@ -61,8 +57,6 @@ impl SubSelection { selection_set: &SelectionSet, ) -> Self { let mut new_selections = Vec::new(); - let mut dropped_fields = IndexSet::default(); - let mut referenced_fields = HashSet::new(); let field_map = map_fields_by_name(document, selection_set); // When the operation contains __typename, it might be used to complete @@ -109,9 +103,6 @@ impl SubSelection { .map(|a| a.name.as_str()) .unwrap_or(name.as_str()); if let Some(fields) = field_map.get_vec(key) { - if self.star.is_some() { - referenced_fields.insert(key); - } for field in fields { let field_response_key = field.response_key().as_str(); new_selections.push(NamedSelection::Field( @@ -126,8 +117,6 @@ impl SubSelection { }), )); } - } else if self.star.is_some() { - dropped_fields.insert(key); } } NamedSelection::Path(alias, path_selection) => { @@ -155,14 +144,6 @@ impl SubSelection { path_selection.apply_selection_set(document, selection_set), )); } - - if self.star.is_some() { - if let Some(name) = key_name(path_selection) { - referenced_fields.insert(name); - } - } else if let Some(name) = key_name(path_selection) { - dropped_fields.insert(name); - } } NamedSelection::Group(alias, sub) => { let key = alias.name.as_str(); @@ -177,24 +158,9 @@ impl SubSelection { } } } - let new_star = self.star.as_ref().cloned(); - if new_star.is_some() { - // Alias fields that were dropped from the original selection to prevent them from - // being picked up by the star. - dropped_fields.retain(|key| !referenced_fields.contains(key)); - for dropped in dropped_fields { - let name = format!("__unused__{dropped}"); - new_selections.push(NamedSelection::Field( - Some(Alias::new(name.as_str())), - WithRange::new(Key::field(dropped), None), - None, - )); - } - } Self { selections: new_selections, - star: new_star, // Keep the old range even though it may be inaccurate after the // removal of selections, since it still indicates where the // original SubSelection came from. @@ -263,14 +229,6 @@ impl PathList { } } -#[inline] -fn key_name(path_selection: &PathSelection) -> Option<&str> { - match path_selection.path.as_ref() { - PathList::Key(key, _) => Some(key.as_str()), - _ => None, - } -} - fn map_fields_by_name<'a>( document: &'a ExecutableDocument, set: &'a SelectionSet, @@ -405,10 +363,8 @@ mod tests { j k } - rest: * } path_to_f: c.f - rest: * } "###, ) @@ -462,12 +418,8 @@ mod tests { group: { j } - __unused__d: d - __unused__i: i - rest: * } path_to_f: c.f - rest: * }"### ); @@ -501,17 +453,8 @@ mod tests { "group": { "j": "j" }, - "__unused__d": "d", - "__unused__i": "i", - "rest": { - "f": "f", - "g": "g", - "j": "j", - "k": "k", - } }, "path_to_f": "f", - "rest": {} })), vec![] ) diff --git a/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__location__tests__arrow_path_ranges.snap b/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__location__tests__arrow_path_ranges.snap index b3974d5916..0c134ec583 100644 --- a/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__location__tests__arrow_path_ranges.snap +++ b/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__location__tests__arrow_path_ranges.snap @@ -74,7 +74,6 @@ Named( }, ), ], - star: None, range: Some( 2..36, ), diff --git a/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__location__tests__parse_with_range_snapshots.snap b/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__location__tests__parse_with_range_snapshots.snap index 9b6414b77e..3b953a90ef 100644 --- a/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__location__tests__parse_with_range_snapshots.snap +++ b/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__location__tests__parse_with_range_snapshots.snap @@ -94,7 +94,6 @@ Named( None, ), ], - star: None, range: Some( 46..54, ), @@ -102,7 +101,6 @@ Named( ), ), ], - star: None, range: Some( 32..55, ), @@ -381,7 +379,6 @@ Named( None, ), ], - star: None, range: Some( 250..259, ), @@ -453,60 +450,15 @@ Named( None, ), ], - star: Some( - StarSelection { - alias: Some( - Alias { - name: WithRange { - node: Field( - "rest", - ), - range: Some( - 319..323, - ), - }, - range: Some( - 319..324, - ), - }, - ), - selection: Some( - SubSelection { - selections: [ - Field( - None, - WithRange { - node: Field( - "data", - ), - range: Some( - 329..333, - ), - }, - None, - ), - ], - star: None, - range: Some( - 327..335, - ), - }, - ), - range: Some( - 319..335, - ), - }, - ), range: Some( - 91..345, + 91..316, ), }, ), ), ], - star: None, range: Some( - 9..345, + 9..316, ), }, ) diff --git a/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__expr_path_selections.snap b/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__expr_path_selections.snap index 89d1b79405..ac3c41b9df 100644 --- a/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__expr_path_selections.snap +++ b/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__expr_path_selections.snap @@ -163,7 +163,6 @@ Named( }, ), ], - star: None, range: Some( 1..71, ), diff --git a/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection-2.snap b/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection-2.snap index 9f530fbaf4..d7e45855f3 100644 --- a/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection-2.snap +++ b/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection-2.snap @@ -90,7 +90,6 @@ Named( None, ), ], - star: None, range: Some( 71..87, ), @@ -130,7 +129,6 @@ Named( None, ), ], - star: None, range: Some( 13..105, ), diff --git a/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection-3.snap b/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection-3.snap index 0781fab6c9..cc1a45c465 100644 --- a/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection-3.snap +++ b/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection-3.snap @@ -90,7 +90,6 @@ Named( None, ), ], - star: None, range: Some( 71..87, ), @@ -192,7 +191,6 @@ Named( None, ), ], - star: None, range: Some( 140..164, ), @@ -220,7 +218,6 @@ Named( }, ), ], - star: None, range: Some( 13..164, ), diff --git a/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection-5.snap b/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection-5.snap index 5cc8d290b6..fee1908412 100644 --- a/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection-5.snap +++ b/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection-5.snap @@ -111,7 +111,6 @@ Ok( None, ), ], - star: None, range: Some( 50..108, ), @@ -134,7 +133,6 @@ Ok( }, ), ], - star: None, range: Some( 13..108, ), diff --git a/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection-6.snap b/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection-6.snap index 4f2a32b35f..9351e02fd7 100644 --- a/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection-6.snap +++ b/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection-6.snap @@ -36,7 +36,6 @@ Ok( None, ), ], - star: None, range: Some( 19..25, ), @@ -118,7 +117,6 @@ Ok( None, ), ], - star: None, range: Some( 54..68, ), @@ -141,7 +139,6 @@ Ok( }, ), ], - star: None, range: Some( 44..70, ), @@ -159,7 +156,6 @@ Ok( }, ), ], - star: None, range: Some( 13..70, ), diff --git a/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection-7.snap b/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection-7.snap index 334a3ce5e7..65eca95ca2 100644 --- a/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection-7.snap +++ b/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection-7.snap @@ -36,7 +36,6 @@ Ok( None, ), ], - star: None, range: Some( 60..66, ), @@ -158,7 +157,6 @@ Ok( None, ), ], - star: None, range: Some( 285..299, ), @@ -193,7 +191,6 @@ Ok( None, ), ], - star: None, range: Some( 86..336, ), @@ -264,7 +261,6 @@ Ok( }, ), ], - star: None, range: Some( 54..362, ), diff --git a/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection.snap b/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection.snap index 49e1ae4eed..c48cdec378 100644 --- a/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection.snap +++ b/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__path_with_subselection.snap @@ -62,7 +62,6 @@ Path( None, ), ], - star: None, range: Some( 36..52, ), diff --git a/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__selection.snap b/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__selection.snap index c920fe3f08..86f998836c 100644 --- a/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__selection.snap +++ b/apollo-federation/src/sources/connect/json_selection/snapshots/apollo_federation__sources__connect__json_selection__parser__tests__selection.snap @@ -198,7 +198,6 @@ Named( None, ), ], - star: None, range: Some( 489..595, ), @@ -266,14 +265,12 @@ Named( None, ), ], - star: None, range: Some( 745..763, ), }, ), ], - star: None, range: Some( 115..777, ), @@ -281,7 +278,6 @@ Named( ), ), ], - star: None, range: Some( 86..777, ), diff --git a/apollo-federation/src/sources/connect/models.rs b/apollo-federation/src/sources/connect/models.rs index 5e87ce2c89..c27a303586 100644 --- a/apollo-federation/src/sources/connect/models.rs +++ b/apollo-federation/src/sources/connect/models.rs @@ -353,7 +353,6 @@ mod tests { None, ), ], - star: None, range: Some( 0..7, ), @@ -475,7 +474,6 @@ mod tests { None, ), ], - star: None, range: Some( 0..13, ), diff --git a/apollo-federation/src/sources/connect/spec/directives.rs b/apollo-federation/src/sources/connect/spec/directives.rs index fe75ee87c1..1da002ed4e 100644 --- a/apollo-federation/src/sources/connect/spec/directives.rs +++ b/apollo-federation/src/sources/connect/spec/directives.rs @@ -611,7 +611,6 @@ mod tests { None, ), ], - star: None, range: Some( 0..7, ), @@ -681,7 +680,6 @@ mod tests { None, ), ], - star: None, range: Some( 0..13, ), diff --git a/apollo-federation/src/sources/connect/validation/selection.rs b/apollo-federation/src/sources/connect/validation/selection.rs index e598db1c82..9cc4665b31 100644 --- a/apollo-federation/src/sources/connect/validation/selection.rs +++ b/apollo-federation/src/sources/connect/validation/selection.rs @@ -11,7 +11,6 @@ use apollo_compiler::schema::ExtendedType; use apollo_compiler::schema::ObjectType; use apollo_compiler::Node; use apollo_compiler::Schema; -use either::Either; use itertools::Itertools; use super::coordinates::ConnectDirectiveCoordinate; @@ -25,7 +24,6 @@ use crate::sources::connect::expand::visitors::FieldVisitor; use crate::sources::connect::expand::visitors::GroupVisitor; use crate::sources::connect::json_selection::NamedSelection; use crate::sources::connect::json_selection::Ranged; -use crate::sources::connect::json_selection::StarSelection; use crate::sources::connect::spec::schema::CONNECT_SELECTION_ARGUMENT_NAME; use crate::sources::connect::validation::coordinates::connect_directive_http_body_coordinate; use crate::sources::connect::validation::graphql::GraphQLString; @@ -240,23 +238,17 @@ impl SelectionValidator<'_, '_> { #[derive(Clone, Copy, Debug)] struct Field<'schema> { - selection: Either<&'schema NamedSelection, &'schema StarSelection>, + selection: &'schema NamedSelection, definition: &'schema Node, } impl<'schema> Field<'schema> { fn next_subselection(&self) -> Option<&'schema SubSelection> { - match self.selection { - Either::Left(selection) => selection.next_subselection(), - Either::Right(_) => None, - } + self.selection.next_subselection() } fn inner_range(&self) -> Option> { - match self.selection { - Either::Left(selection) => selection.range(), - Either::Right(selection) => selection.range(), - } + self.selection.range() } } @@ -325,7 +317,7 @@ impl<'schema> GroupVisitor, Field<'schema>> for SelectionValidato for field_name in selection.names() { if let Some(definition) = group.ty.fields.get(field_name) { results.push(Ok(Field { - selection: Either::Left(selection), + selection, definition, })); } else { @@ -341,9 +333,7 @@ impl<'schema> GroupVisitor, Field<'schema>> for SelectionValidato } } results - }).chain( - self.validate_star_selection(group) - ).collect() + }).collect() } fn exit_group(&mut self) -> Result<(), Self::Error> { @@ -431,110 +421,4 @@ impl<'schema, 'a> SelectionValidator<'schema, 'a> { fn last_field(&self) -> &PathPart { self.path.last().unwrap_or(&self.root) } - - fn validate_star_selection( - &self, - group: &'a Group<'schema>, - ) -> impl Iterator, Message>> + '_ { - group.selection.star_iter().map(|star| { - let Some(alias) = star.alias() else { - return Err(Message { - code: Code::InvalidStarSelection, - message: format!( - "{coordinate} contains `*` without an alias. Use `fieldName: *` to map properties to a field.", - coordinate = self.selection_arg.coordinate, - ), - locations: self.get_selection_location(star).collect(), - }); - }; - - let field_name = alias.name(); - - let Some(definition) = group.ty.fields.get(field_name) else { - return Err(Message { - code: Code::SelectedFieldNotFound, - message: format!( - "{coordinate} contains field `{field_name}`, which does not exist on `{parent_type}`.", - coordinate = self.selection_arg.coordinate, - parent_type = group.ty.name, - ), - locations: self.get_selection_location(alias).collect(), - }); - }; - - let locations = self.get_selection_location(star).chain(definition.line_column_range(&self.schema.sources)); - - if definition.ty.is_list() { - return Err(Message { - code: Code::InvalidStarSelection, - message: format!( - "{coordinate} contains `{field_name}: *` but the field `{parent_type}.{field_name}: {ty}` returns a list. It must be a non-list custom scalar.", - coordinate = self.selection_arg.coordinate, - field_name = field_name, - parent_type = group.ty.name, - ty = definition.ty - ), - locations: locations.collect(), - }); - } - - let Some(ty) = self.schema.types.get(definition.ty.inner_named_type()) else { - return Err(Message { - code: Code::GraphQLError, - message: format!( - "{coordinate} contains field `{field_name}`, which has undefined type `{type_name}.", - coordinate = self.selection_arg.coordinate, - type_name = definition.ty.inner_named_type() - ), - locations: locations.collect(), - }); - }; - - if !ty.is_scalar() { - return Err(Message { - code: Code::InvalidStarSelection, - message: format!( - "{coordinate} contains `{field_name}: *` but the field `{parent_type}.{field_name}: {ty}` returns {ty_kind} type. It must be a non-list custom scalar.", - coordinate = self.selection_arg.coordinate, - field_name = field_name, - parent_type = group.ty.name, - ty = definition.ty, - ty_kind = type_sentence_part(ty) - ), - locations: locations.collect(), - }); - } - - if ty.is_built_in() { - return Err(Message { - code: Code::InvalidStarSelection, - message: format!( - "{coordinate} contains `{field_name}: *` but the field `{parent_type}.{field_name}: {ty}` returns a built-in scalar type. It must be a non-list custom scalar.", - coordinate = self.selection_arg.coordinate, - field_name = field_name, - parent_type = group.ty.name, - ty = definition.ty - ), - locations: locations.collect(), - }); - } - - Ok(Field { - selection: Either::Right(star), - definition, - }) - }) - } -} - -fn type_sentence_part(ty: &ExtendedType) -> String { - match ty { - ExtendedType::Object(_) => "an object", - ExtendedType::Interface(_) => "an interface", - ExtendedType::Union(_) => "a union", - ExtendedType::Enum(_) => "an enum", - ExtendedType::InputObject(_) => "an input object", - ExtendedType::Scalar(_) => "a scalar", - } - .to_string() } diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@rest-good.graphql.snap b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@rest-good.graphql.snap deleted file mode 100644 index f0a892b407..0000000000 --- a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@rest-good.graphql.snap +++ /dev/null @@ -1,6 +0,0 @@ ---- -source: apollo-federation/src/sources/connect/validation/mod.rs -expression: "format!(\"{:#?}\", errors)" -input_file: apollo-federation/src/sources/connect/validation/test_data/rest-good.graphql ---- -[] diff --git a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@rest.graphql.snap b/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@rest.graphql.snap deleted file mode 100644 index 71292b14c1..0000000000 --- a/apollo-federation/src/sources/connect/validation/snapshots/validation_tests@rest.graphql.snap +++ /dev/null @@ -1,38 +0,0 @@ ---- -source: apollo-federation/src/sources/connect/validation/mod.rs -expression: "format!(\"{:#?}\", errors)" -input_file: apollo-federation/src/sources/connect/validation/test_data/rest.graphql ---- -[ - Message { - code: InvalidStarSelection, - message: "`@connect(selection:)` on `Query.notScalar` contains `rest: *` but the field `NotScalar.rest: Obj` returns an object type. It must be a non-list custom scalar.", - locations: [ - 13:63..13:70, - 34:3..34:12, - ], - }, - Message { - code: InvalidStarSelection, - message: "`@connect(selection:)` on `Query.builtIn` contains `rest: *` but the field `BuiltIn.rest: String` returns a built-in scalar type. It must be a non-list custom scalar.", - locations: [ - 16:63..16:70, - 43:3..43:15, - ], - }, - Message { - code: InvalidStarSelection, - message: "`@connect(selection:)` on `Query.list` contains `rest: *` but the field `List.rest: [JSON]` returns a list. It must be a non-list custom scalar.", - locations: [ - 19:63..19:70, - 48:3..48:15, - ], - }, - Message { - code: SelectedFieldNotFound, - message: "`@connect(selection:)` on `Query.notFound` contains field `rest`, which does not exist on `NotFound`.", - locations: [ - 22:63..22:68, - ], - }, -] diff --git a/apollo-federation/src/sources/connect/validation/test_data/rest-good.graphql b/apollo-federation/src/sources/connect/validation/test_data/rest-good.graphql deleted file mode 100644 index 5f99572b07..0000000000 --- a/apollo-federation/src/sources/connect/validation/test_data/rest-good.graphql +++ /dev/null @@ -1,18 +0,0 @@ -extend schema - @link( - url: "https://specs.apollo.dev/connect/v0.1" - import: ["@connect", "@source"] - ) - @source(name: "v2", http: { baseURL: "http://127.0.0.1" }) - -type Query { - good: [Good!]! - @connect(source: "v2", http: { GET: "/" }, selection: "id rest: *") -} - -type Good { - id: ID! - rest: JSON -} - -scalar JSON diff --git a/apollo-federation/src/sources/connect/validation/test_data/rest.graphql b/apollo-federation/src/sources/connect/validation/test_data/rest.graphql deleted file mode 100644 index 00e4c55df7..0000000000 --- a/apollo-federation/src/sources/connect/validation/test_data/rest.graphql +++ /dev/null @@ -1,53 +0,0 @@ -extend schema - @link( - url: "https://specs.apollo.dev/connect/v0.1" - import: ["@connect", "@source"] - ) - @source(name: "v2", http: { baseURL: "http://127.0.0.1" }) - -type Query { - good: [Good!]! - @connect(source: "v2", http: { GET: "/" }, selection: "id rest: *") - - notScalar: [NotScalar!]! - @connect(source: "v2", http: { GET: "/" }, selection: "id rest: *") - - builtIn: [BuiltIn!]! - @connect(source: "v2", http: { GET: "/" }, selection: "id rest: *") - - list: [List!]! - @connect(source: "v2", http: { GET: "/" }, selection: "id rest: *") - - notFound: [NotFound!]! - @connect(source: "v2", http: { GET: "/" }, selection: "id rest: *") -} - -type Good { - id: ID! - rest: JSON -} - -scalar JSON - -type NotScalar { - id: ID! - rest: Obj -} - -type Obj { - hi: String -} - -type BuiltIn { - id: ID! - rest: String -} - -type List { - id: ID! - rest: [JSON] -} - -type NotFound { - id: ID! -} diff --git a/apollo-router/src/plugins/connectors/make_requests.rs b/apollo-router/src/plugins/connectors/make_requests.rs index 7739dc7712..9beccbff2a 100644 --- a/apollo-router/src/plugins/connectors/make_requests.rs +++ b/apollo-router/src/plugins/connectors/make_requests.rs @@ -600,7 +600,6 @@ mod tests { None, ), ], - star: None, range: Some( 0..1, ), @@ -642,7 +641,6 @@ mod tests { None, ), ], - star: None, range: Some( 0..1, ), @@ -1201,7 +1199,6 @@ mod tests { None, ), ], - star: None, range: Some( 0..5, ), @@ -1312,7 +1309,6 @@ mod tests { None, ), ], - star: None, range: Some( 0..5, ), @@ -1522,7 +1518,6 @@ mod tests { None, ), ], - star: None, range: Some( 0..5, ), @@ -1633,7 +1628,6 @@ mod tests { None, ), ], - star: None, range: Some( 0..5, ), @@ -1765,7 +1759,6 @@ mod tests { None, ), ], - star: None, range: Some( 6..15, ), @@ -1773,7 +1766,6 @@ mod tests { ), ), ], - star: None, range: Some( 0..15, ), @@ -1832,7 +1824,6 @@ mod tests { None, ), ], - star: None, range: Some( 6..15, ), @@ -1840,7 +1831,6 @@ mod tests { ), ), ], - star: None, range: Some( 0..15, ), @@ -1979,7 +1969,6 @@ mod tests { None, ), ], - star: None, range: Some( 0..8, ), @@ -2023,7 +2012,6 @@ mod tests { None, ), ], - star: None, range: Some( 0..8, ), @@ -2067,7 +2055,6 @@ mod tests { None, ), ], - star: None, range: Some( 0..8, ), @@ -2111,7 +2098,6 @@ mod tests { None, ), ], - star: None, range: Some( 0..8, ), @@ -2258,7 +2244,6 @@ mod tests { None, ), ], - star: None, range: Some( 0..8, ), @@ -2302,7 +2287,6 @@ mod tests { None, ), ], - star: None, range: Some( 0..8, ), @@ -2346,7 +2330,6 @@ mod tests { None, ), ], - star: None, range: Some( 0..8, ), @@ -2390,7 +2373,6 @@ mod tests { None, ), ], - star: None, range: Some( 0..8, ), @@ -2532,7 +2514,6 @@ mod tests { None, ), ], - star: None, range: Some( 0..8, ), @@ -2574,7 +2555,6 @@ mod tests { None, ), ], - star: None, range: Some( 0..8, ), diff --git a/apollo-router/src/plugins/connectors/testdata/steelthread.graphql b/apollo-router/src/plugins/connectors/testdata/steelthread.graphql index e045b1f77f..13430cd85a 100644 --- a/apollo-router/src/plugins/connectors/testdata/steelthread.graphql +++ b/apollo-router/src/plugins/connectors/testdata/steelthread.graphql @@ -73,7 +73,7 @@ type Query { users: [User] @join__field(graph: CONNECTORS) @join__directive(graphs: [CONNECTORS], name: "connect", args: {source: "json", http: {GET: "/users", headers: [{name: "x-new-name", from: "x-rename-connect"}, {name: "x-insert-multi-value", value: "first,second"}, {name: "x-config-variable-connect", value: "before {$config.connect.val} after"}, {name: "x-context-value-connect", value: "before {$context.val} after"}]}, selection: "id name"}) me: User @join__field(graph: CONNECTORS) @join__directive(graphs: [CONNECTORS], name: "connect", args: {source: "json", http: {GET: "/users/{$config.id}"}, selection: "id\nname\nusername"}) - user(id: ID!): User @join__field(graph: CONNECTORS) @join__directive(graphs: [CONNECTORS], name: "connect", args: {source: "json", http: {GET: "/users/{$args.id}"}, selection: "id\nname\nusername\nrest: *", entity: true}) + user(id: ID!): User @join__field(graph: CONNECTORS) @join__directive(graphs: [CONNECTORS], name: "connect", args: {source: "json", http: {GET: "/users/{$args.id}"}, selection: "id\nname\nusername", entity: true}) posts: [Post] @join__field(graph: CONNECTORS) @join__directive(graphs: [CONNECTORS], name: "connect", args: {source: "json", http: {GET: "/posts"}, selection: "id title user: { id: userId }"}) } @@ -84,7 +84,6 @@ type User id: ID! name: String @join__field(graph: CONNECTORS) username: String @join__field(graph: CONNECTORS) - rest: JSON @join__field(graph: CONNECTORS) c: String @join__field(graph: CONNECTORS, external: true) @join__field(graph: GRAPHQL) d: String @join__field(graph: CONNECTORS, requires: "c") @join__directive(graphs: [CONNECTORS], name: "connect", args: {source: "json", http: {GET: "/users/{$this.c}"}, selection: "$.phone"}) } diff --git a/apollo-router/src/plugins/connectors/testdata/steelthread.yaml b/apollo-router/src/plugins/connectors/testdata/steelthread.yaml index ac25da35b8..d7cbb5b07e 100644 --- a/apollo-router/src/plugins/connectors/testdata/steelthread.yaml +++ b/apollo-router/src/plugins/connectors/testdata/steelthread.yaml @@ -62,7 +62,6 @@ subgraphs: id name username - rest: * """ entity: true ) @@ -79,7 +78,6 @@ subgraphs: id: ID! name: String username: String - rest: JSON c: String @external d: String @requires(fields: "c") diff --git a/apollo-router/src/plugins/connectors/tests.rs b/apollo-router/src/plugins/connectors/tests.rs index daa5564ee5..e92b2eb414 100644 --- a/apollo-router/src/plugins/connectors/tests.rs +++ b/apollo-router/src/plugins/connectors/tests.rs @@ -442,7 +442,7 @@ async fn test_root_field_plus_entity_plus_requires() { let response = execute( STEEL_THREAD_SCHEMA, &mock_server.uri(), - "query { users { id name username d rest } }", + "query { users { id name username d } }", Default::default(), None, |_| {}, @@ -457,23 +457,13 @@ async fn test_root_field_plus_entity_plus_requires() { "id": 1, "name": "Leanne Graham", "username": "Bret", - "d": "1-770-736-8031 x56442", - "rest": { - "phone": "1-770-736-8031 x56442", - "email": "Sincere@april.biz", - "website": "hildegard.org" - } + "d": "1-770-736-8031 x56442" }, { "id": 2, "name": "Ervin Howell", "username": "Antonette", - "d": "1-770-736-8031 x56442", - "rest": { - "phone": "1-770-736-8031 x56442", - "email": "Shanna@melissa.tv", - "website": "anastasia.net" - } + "d": "1-770-736-8031 x56442" } ] }