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

GraphQL parser doesn't treat commas as ignored tokens #1713

Closed
skevy opened this issue Oct 28, 2019 · 0 comments · Fixed by #1714

Comments

@skevy
Copy link

@skevy skevy commented Oct 28, 2019

In the process of upgrading the Airbnb app to use 1.2.0, I encountered some issues parsing GraphQL files.

The file in question has a GraphQL query as follows:

mutation CreateGuidebook($title: String!) {
  [redacted] {
   [redacted](request: {
      title: $title,
    }) {
      id
    }
  }
}

The dangling comma on the 4th line causes the parser to throw:

Unsupported token `}`
----------------------------------------------------
[68]:      title: $title,
[69]:    }) {
[70]:      id
----------------------------------------------------

Taking a look at the grammar, I see the following

arguments
   : '(' argument ( ','? argument )* ')'
   ;

As per the GraphQL spec here, commas are ignored tokens.

For instance, this minified GraphQL query is fully valid (notice the lack of spaces in the mutation arguments, and the spaces instead of commas in the field arguments for bar):

mutation Foo($arg1:String!$arg2:String!){foo{bar(arg1:$arg1 arg2:$arg2)}}

We should fix the grammar accordingly, and ensure it matches the spec in all areas.

I'm happy to send a PR (or PRs) to do so eventually, but don't have time at the moment, so I wanted to make sure I flagged the issue in case someone had time to get to this first.

Thanks in advance!

sav007 added a commit to sav007/apollo-android that referenced this issue Oct 28, 2019
sav007 added a commit to sav007/apollo-android that referenced this issue Oct 28, 2019
@sav007 sav007 closed this in #1714 Oct 28, 2019
sav007 added a commit that referenced this issue Oct 28, 2019
Closes #1713
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.