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

Raise ParseError for some queries that are considered invalid by the reference implementation #2344

Merged
merged 17 commits into from Jun 28, 2019

Conversation

@daemonsy
Copy link
Contributor

@daemonsy daemonsy commented Jun 21, 2019

Update: Refer to new write up below

What's up?

This is an attempt to resolve #2243.

Both libgraphqlparser and graphql-js raises EOF errors when the only token in a query is a comment.

a.k.a documents with no selection sets are not possible, as far as the reference implementations tell.

What this does

  • Raise ParseError on empty sets
  • Add documentation about ragel/colm

Update 26 June 2019:

  • Also raising errors on node() { id } where the node() is not considered legal form by libgraphqlparser and graphql-js.
graphql.gemspec Outdated
@@ -40,7 +40,7 @@ Gem::Specification.new do |s|
s.add_development_dependency "minitest", "~> 5.9.0"
s.add_development_dependency "minitest-focus", "~> 1.1"
s.add_development_dependency "minitest-reporters", "~>1.0"
s.add_development_dependency "racc", "~> 1.4"
s.add_development_dependency "racc", "1.4.14"
Copy link
Contributor Author

@daemonsy daemonsy Jun 21, 2019

Choose a reason for hiding this comment

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

because we pin exact versions of racc in the rake task.

Loading

@@ -1,6 +1,6 @@
#
# DO NOT MODIFY!!!!
# This file is automatically generated by Racc 1.4.15
Copy link
Contributor Author

@daemonsy daemonsy Jun 21, 2019

Choose a reason for hiding this comment

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

The rake task pinning might be outdated. Happy to change it to 1.4.15 everywhere (including docs).

Loading

Copy link
Owner

@rmosolgo rmosolgo Jun 25, 2019

Choose a reason for hiding this comment

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

Oops, yeah, I'm assuming that newer is better, let's go for it!

Loading

@rmosolgo
Copy link
Owner

@rmosolgo rmosolgo commented Jun 25, 2019

Thanks for taking this on and making the case for it. I'm game to make the change, but I want to make sure, will this raise a Ruby error when people call Schema.execute(...) in their controllers? I want to make sure that these client errors don't become noise in our bug trackers.

On a related note, I see that a lot of tests about empty query strings and comments were removed altogether. Could we keep those tests but update the assertion to describe this improved behavior? That would also help me answer my question above.

Loading

@daemonsy
Copy link
Contributor Author

@daemonsy daemonsy commented Jun 25, 2019

Thanks for taking this on and making the case for it.

😅 actually I haven't explained it well. In general, my thought is that it's best to stay close to behavior of reference implementation / spec to avoid future problems where someone might be relying on non spec compliant behavior.

Hence taking on this issue and the one where { node(id:) { id } } is considered a valid query. Actually on their own, they are kind of a "trivial breaking change", so I don't mind it being deferred to a major release.

will this raise a Ruby error when people call Schema.execute(...) in their controllers? I want to make sure that these client errors don't become noise in our bug trackers.

Not so sure what you mean here, it raises an error if GraphQL.parse is called and becomes part of the errors key if passed directly into Schema.execute(...).

Could we keep those tests but update the assertion to describe this improved behavior?

Good idea 👍 will do. I was wondering the context behind those tests in the first place, do you have recollection?

Loading

@@ -165,7 +165,6 @@ rule

arguments_opt:
/* none */ { return EMPTY_ARRAY }
| LPAREN RPAREN { return EMPTY_ARRAY }
Copy link
Contributor Author

@daemonsy daemonsy Jun 26, 2019

Choose a reason for hiding this comment

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

I believe this changes removes matching field() if I got the understanding of racc correctly.

Loading

@@ -10,7 +10,7 @@
}
fragment cheeseFields on Cheese {
similarCheese() { __typename }
similarCheese { __typename }
Copy link
Contributor Author

@daemonsy daemonsy Jun 26, 2019

Choose a reason for hiding this comment

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

I think static validation assumes that the query can be parsed, this test was failing because context.visitor.visit is failing. But if you took the whole query in this test and either parsed it or tried executing it, it raises the ParseError as expected.

Loading

@rmosolgo
Copy link
Owner

@rmosolgo rmosolgo commented Jun 26, 2019

context behind those tests in the first place

I don't remember specifically, but in general, they serve as a specification for behavior in those cases. If they weren't tested, then they'd basically be undefined behavior and it would be very easy to break GraphQL-Ruby's behavior in those cases. By testing them, we can be sure to maintain the existing behavior, or if we mean to change the behavior, we can keep a close eye on how we change it. There are enough apps using this gem that the slightest unexpected change will surprise someone 😆

Loading

daemonsy added 2 commits Jun 27, 2019
{ node(id:) { id } } was considered a valid query. After turning verbose mode on, discovered a weird rule:

rule 79 input_value:
rule 80 input_value: literal_value
rule 81 input_value: variable
rule 82 input_value: object_value

rule 79 was caused by the extra pipe which allowed input_value to be empty
@daemonsy
Copy link
Contributor Author

@daemonsy daemonsy commented Jun 27, 2019

Update: PR Writeup 2.0

Please ignore the original write up.

This is an attempt to resolve #2243, #1772, namely GraphQL.parse treating the queries below as valid

All the "invalid" queries are tested with graphql-js and libgraphqlparser to verify that they are considered invalid.

Only comment in a query (a.k.a empty query)

# this is a comment 

Empty argument set

{
  node() { name }
}

Malformed argument

{
  node(id:) { name } 
} 

The changes

  • raise ParseError with EOF as the other graphql parsers when the query is empty
  • Remove () grammar from argument sets
  • Remove implicit /* none */ with | in front of literal_value 👓
  • Pin down racc to latest stable version
  • Add documentation about racc, colm and ragel

Loading

@@ -138,7 +138,7 @@ def foobar
let(:analyzers) { [AstTypeCollector, AstNodeCounter] }
let(:reduce_result) { GraphQL::Analysis::AST.analyze_query(query, analyzers) }
let(:variables) { {} }
let(:query) { GraphQL::Query.new(Dummy::Schema, query_string, variables: variables) }
let(:query) { GraphQL::Query.new(Dummy::Schema.graphql_definition, query_string, variables: variables) }
Copy link
Contributor Author

@daemonsy daemonsy Jun 27, 2019

Choose a reason for hiding this comment

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

👀

Loading

Copy link
Contributor Author

@daemonsy daemonsy left a comment

I had to add .graphql_definition so gets converted to the graphql type, where schema.parse_error is defined

Loading

@@ -32,7 +32,7 @@
let(:operation_name) { nil }
let(:max_depth) { nil }
let(:query_variables) { {"cheeseId" => 2} }
let(:schema) { Dummy::Schema }
let(:schema) { Dummy::Schema.graphql_definition }
Copy link
Contributor Author

@daemonsy daemonsy Jun 27, 2019

Choose a reason for hiding this comment

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

👀

Loading

@daemonsy
Copy link
Contributor Author

@daemonsy daemonsy commented Jun 27, 2019

@rmosolgo I think this is ready for a second review. I'll also appreciate if you can build the parser locally and see if there's any diff, not sure if I got racc/ragel/colm working correctly.

When it's all ready, maybe this can be squash merged because of a lot of small noisy intermediate commits.

Loading

@daemonsy daemonsy changed the title Raise ParseError when the only token in the document is a comment Raise ParseError for some queries that are considered invalid by the reference implementation Jun 27, 2019
spec/graphql/analysis/ast_spec.rb Outdated Show resolved Hide resolved
Loading
guides/development.md Outdated Show resolved Hide resolved
Loading
@rmosolgo rmosolgo changed the base branch from master to 1.10-dev Jun 28, 2019
@rmosolgo
Copy link
Owner

@rmosolgo rmosolgo commented Jun 28, 2019

Oops, I updated it to go into 1.10-dev but it seems confused because of the commits from master. Sorry about the noise, but I think the merge will be fine. (I tried merging master into 1.10-dev but it seems like that didn't help.)

Loading

Co-Authored-By: Robert Mosolgo <rmosolgo@github.com>
@daemonsy
Copy link
Contributor Author

@daemonsy daemonsy commented Jun 28, 2019

I merged 1.10-dev in and also pulled the remote changes. Only see my own changes, so 🤞

If the build goes green, should be ready to merge

Loading

@rmosolgo rmosolgo merged commit e7e45e0 into rmosolgo:1.10-dev Jun 28, 2019
1 check passed
Loading
@rmosolgo
Copy link
Owner

@rmosolgo rmosolgo commented Jun 28, 2019

🤘 !

Loading

@rmosolgo rmosolgo added this to the 1.10.0 milestone Jun 28, 2019
@daemonsy
Copy link
Contributor Author

@daemonsy daemonsy commented Jun 28, 2019

🙏 thank you, I learned quite a bit (but only skimped the surface) about lexing and parsing in this PR. 😄

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants