Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JSONSelection] Fix precedence of parsing NamedPathSelection within NamedSelection rule #5156

Merged
merged 3 commits into from
May 13, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 45 additions & 30 deletions apollo-federation/src/sources/connect/json_selection/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,10 @@ below.
JSONSelection ::= NakedSubSelection | PathSelection
NakedSubSelection ::= NamedSelection* StarSelection?
SubSelection ::= "{" NakedSubSelection "}"
NamedSelection ::= NamedFieldSelection | NamedQuotedSelection | NamedPathSelection | NamedGroupSelection
NamedSelection ::= NamedPathSelection | NamedFieldSelection | NamedQuotedSelection | NamedGroupSelection
NamedPathSelection ::= Alias PathSelection
NamedFieldSelection ::= Alias? Identifier SubSelection?
NamedQuotedSelection ::= Alias StringLiteral SubSelection?
NamedPathSelection ::= Alias PathSelection
NamedGroupSelection ::= Alias SubSelection
Alias ::= Identifier ":"
PathSelection ::= (VarPath | KeyPath) SubSelection?
Expand Down Expand Up @@ -112,17 +112,21 @@ operator. Every `CamelCase` identifier on the left side of the `::=` operator
can be recursively expanded into one of its right-side alternatives.

Methodically trying out all these alternatives is the fundamental job of the
parser. While this grammar is not believed to have any ambiguities, ambiguities
can be resolved by applying the alternatives left to right, accepting the first
set of expansions that fully matches the input tokens. Parsing succeeds when
only terminal tokens remain (quoted text or regular expression character
classes).

Much like regular expression syntax, the `*` and `+` operators denote repetition
(_zero or more_ and _one or more_, respectively), `?` denotes optionality (_zero
or one_), parentheses allow grouping, `"quoted"` or `'quoted'` text represents
raw characters that cannot be expanded further, and `[...]` specifies character
ranges.
parser. Parsing succeeds when only terminal tokens remain (quoted text or
regular expression character classes).

Ambiguities can be resolved by applying the alternatives left to right,
accepting the first set of expansions that fully matches the input tokens. An
example where this kind of ordering matters is the `NamedSelection` rule, which
specifies parsing `NamedPathSelection` before `NamedFieldSelection` and
`NamedQuotedSelection`, so the entire path will be consumed, rather than
mistakenly consuming only the first key in the path as a field name.

As in many regular expression syntaxes, the `*` and `+` operators denote
repetition (_zero or more_ and _one or more_, respectively), `?` denotes
optionality (_zero or one_), parentheses allow grouping, `"quoted"` or
`'quoted'` text represents raw characters that cannot be expanded further, and
`[...]` specifies character ranges.

### Whitespace, comments, and `NO_SPACE`

Expand Down Expand Up @@ -253,6 +257,34 @@ 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.

### `NamedPathSelection ::=`

![NamedPathSelection](./grammar/NamedPathSelection.svg)

Since `PathSelection` returns an anonymous value extracted from the given path,
if you want to use a `PathSelection` alongside other `NamedSelection` items, you
have to 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:

```graphql
position { x y }
pathName: some.nested.path { a b c }
scalarField
```

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.

### `NamedFieldSelection ::=`

![NamedFieldSelection](./grammar/NamedFieldSelection.svg)
Expand Down Expand Up @@ -290,23 +322,6 @@ 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`.

### `NamedPathSelection ::=`

![NamedPathSelection](./grammar/NamedPathSelection.svg)

Since `PathSelection` returns an anonymous value extracted from the given path,
if you want to use a `PathSelection` alongside other `NamedSelection` items, you
have to 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:

```graphql
position { x y }
pathName: some.nested.path { a b c }
scalarField
```

### `NamedGroupSelection ::=`

![NamedGroupSelection](./grammar/NamedGroupSelection.svg)
Expand Down
14 changes: 11 additions & 3 deletions apollo-federation/src/sources/connect/json_selection/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,10 @@ impl JSONSelection {
}
}

// NamedSelection ::= NamedFieldSelection | NamedQuotedSelection | NamedPathSelection | NamedGroupSelection
// NamedSelection ::= NamedPathSelection | NamedFieldSelection | NamedQuotedSelection | NamedGroupSelection
// NamedPathSelection ::= Alias PathSelection
// NamedFieldSelection ::= Alias? Identifier SubSelection?
// NamedQuotedSelection ::= Alias StringLiteral SubSelection?
// NamedPathSelection ::= Alias PathSelection
// NamedGroupSelection ::= Alias SubSelection

#[derive(Debug, PartialEq, Clone, Serialize)]
Expand All @@ -67,9 +67,17 @@ pub enum NamedSelection {
impl NamedSelection {
fn parse(input: &str) -> IResult<&str, Self> {
alt((
// We must try parsing NamedPathSelection before NamedFieldSelection
// and NamedQuotedSelection because a NamedPathSelection without a
// leading `.`, such as `alias: some.nested.path` has a prefix that
// can be parsed as a NamedFieldSelection: `alias: some`. Parsing
// then fails when it finds the remaining `.nested.path` text. Some
// parsers would solve this by forbidding `.` in the "lookahead" for
// Named{Field,Quoted}Selection, but negative lookahead is tricky in
// nom, so instead we greedily parse NamedPathSelection first.
Self::parse_path,
Self::parse_field,
Self::parse_quoted,
Self::parse_path,
Self::parse_group,
Comment on lines 67 to 81
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the operative change that fixes the NamedPathSelection parsing ambiguity.

))(input)
}
Expand Down
Loading