shurcooL / graphql

Package graphql provides a GraphQL client implementation.
MIT License
708 stars 282 forks source link

Error while generating graphql query with variables #6

Closed alexander-myltsev closed 6 years ago

alexander-myltsev commented 6 years ago

The query and variables as follows:

var query struct {
    NameStringsByUuid []struct {
        InputId graphql.String `graphql:"inputId"`
    } `graphql:"nameStringsByUuid(uuids:[$someid1, $someid2])"`
}

    variables := map[string]interface{}{
        "someid1": graphql.ID("7db4f8a2-aafe-56b6-8838-89522c67d9f0"),
        "someid2": graphql.ID("a5cd7171-03e3-5d36-88a4-8eb921f4d159"),
    }

Produces query with errors (no commas between query parameters):

query($someid1:ID!$someid2:ID!){nameStringsByUuid(uuids:[$someid1, $someid2]){inputId}}

On the other hand, when I'm using queries as follows:

var query struct {
    NameStringsByUuid []struct {
        InputId graphql.String `graphql:"inputId"`
    } `graphql:"nameStringsByUuid(uuids:[$someids])"`
}

    variables := map[string]interface{}{
        "someids": []graphql.ID{
            graphql.ID("7db4f8a2-aafe-56b6-8838-89522c67d9f0"),
            graphql.ID("a5cd7171-03e3-5d36-88a4-8eb921f4d159"),
        },
    }

It produces query with errors too (variable type should be $someids:[ID!]! with bang in the end):

query($someids:[ID!]){nameStringsByUuid(uuids:[$someids]){inputId}}

Is it a bug, or am I doing something wrong?

dmitshur commented 6 years ago

Thanks for reporting this, I will investigate.

I don't see you doing anything wrong, so if the issue is valid, I suspect it's a bug in the code.

dmitshur commented 6 years ago

Produces query with errors (no commas between query parameters):

This part might be intentional. My intention was to produce a minified query. When I tested against GitHub's GraphQL server, it accepted comma-less query parameters without issues.

See the following test-case:

https://github.com/shurcooL/graphql/blob/671b933639ae10534edc12900803e2c1d7dfb9b5/query_test.go#L163-L168

Can you tell me what you're basing the statement that omitting commas is an error on? Is it because it looks wrong, or is it failing to work? If the latter, how can I reproduce it?

Of course, our best bet is to look into the GraphQL spec to see whether omitting commas is considered valid or not.

dmitshur commented 6 years ago

http://facebook.github.io/graphql/October2016/#sec-Insignificant-Commas and http://facebook.github.io/graphql/October2016/#sec-List-Value seem to suggest that omitting commas is indeed a valid thing to do:

Similar to white space and line terminators, commas (,) are used to improve the legibility of source text and separate lexical tokens but are otherwise syntactically and semantically insignificant within GraphQL query documents.

Commas are optional throughout GraphQL so trailing commas are allowed and repeated commas do not represent missing values.

dmitshur commented 6 years ago

It produces query with errors too (variable type should be $someids:[ID!]! with bang in the end):

query($someids:[ID!]){nameStringsByUuid(uuids:[$someids]){inputId}}

That part seems to be a legitimate issue. Basically, I hadn't run into that situation yet, so the code that handles it was left incomplete.

dmitshur commented 6 years ago

@alexander-myltsev, please see PR #7, it resolves this issue.

alexander-myltsev commented 6 years ago

@shurcooL works for me now.

dmitshur commented 6 years ago

Thanks for testing, and for reporting this issue! I'll merge the PR now.