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

Error while generating graphql query with variables #6

Closed
alexander-myltsev opened this Issue Nov 30, 2017 · 7 comments

Comments

2 participants
@alexander-myltsev

alexander-myltsev commented Nov 30, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Nov 30, 2017

Member

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.

Member

dmitshur commented Nov 30, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Nov 30, 2017

Member

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:

graphql/query_test.go

Lines 163 to 168 in 671b933

inVariables: map[string]interface{}{
"repositoryOwner": String("shurcooL-test"),
"repositoryName": String("test-repo"),
"issueNumber": Int(1),
},
want: `query($issueNumber:Int!$repositoryName:String!$repositoryOwner:String!){repository(owner: $repositoryOwner, name: $repositoryName){issue(number: $issueNumber){body}}}`,

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.

Member

dmitshur commented Nov 30, 2017

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:

graphql/query_test.go

Lines 163 to 168 in 671b933

inVariables: map[string]interface{}{
"repositoryOwner": String("shurcooL-test"),
"repositoryName": String("test-repo"),
"issueNumber": Int(1),
},
want: `query($issueNumber:Int!$repositoryName:String!$repositoryOwner:String!){repository(owner: $repositoryOwner, name: $repositoryName){issue(number: $issueNumber){body}}}`,

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

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Nov 30, 2017

Member

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.

Member

dmitshur commented Nov 30, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Nov 30, 2017

Member

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.

Member

dmitshur commented Nov 30, 2017

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 added a commit that referenced this issue Nov 30, 2017

Implement missing edge cases in queryArguments.
Previously, queryArguments was partially implemented to handle the most
common types I had run into, with a TODO to complete the rest.

This change implements the support for the remaining argument types.
Now, slices of T are mapped to "[T]!", and pointers to slices of T are
mapped to "[T]", as per GraphQL specification.

Add a comment pointing out that commas are omitted because they're
insignificant, and we're looking to produce minified output (less bytes
to send over the network).

Fixes #6.
@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Nov 30, 2017

Member

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

Member

dmitshur commented Nov 30, 2017

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

@alexander-myltsev

This comment has been minimized.

Show comment
Hide comment
@alexander-myltsev

alexander-myltsev commented Dec 1, 2017

@shurcooL works for me now.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Dec 1, 2017

Member

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

Member

dmitshur commented Dec 1, 2017

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

@dmitshur dmitshur closed this in #7 Dec 1, 2017

dmitshur added a commit that referenced this issue Dec 1, 2017

Implement missing edge cases in queryArguments. (#7)
Previously, queryArguments was partially implemented to handle the most
common types I had run into, with a TODO to complete the rest.

This change implements the support for the remaining argument types.
Now, slices of T are mapped to "[T]!", and pointers to slices of T are
mapped to "[T]", as per GraphQL specification.

Add a comment pointing out that commas are omitted because they're
insignificant, and we're looking to produce minified output (less bytes
to send over the network).

Fixes #6.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment