From f74b6241e2d34d65be23acc19a01d32dff9f9e6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Gabriel=20Quaresma=20de=20Almeida?= Date: Wed, 2 Oct 2024 09:24:13 -0300 Subject: [PATCH 1/3] fix: ByteFor::NUMBER case error message --- lib/graphql/language/lexer.rb | 11 ++++++++++- spec/graphql/language/parser_spec.rb | 4 ++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/lib/graphql/language/lexer.rb b/lib/graphql/language/lexer.rb index 871824465b..0e1a444a6b 100644 --- a/lib/graphql/language/lexer.rb +++ b/lib/graphql/language/lexer.rb @@ -72,7 +72,7 @@ def advance # Check for a matched decimal: @scanner[1] ? :FLOAT : :INT else - raise_parse_error("Expected a number, but it was malformed (#{@string[@pos].inspect})") + raise_parse_error(invalid_byte_for_number_error_message) end when ByteFor::ELLIPSIS if @string.getbyte(@pos + 1) != 46 || @string.getbyte(@pos + 2) != 46 @@ -116,6 +116,15 @@ def debug_token_value(token_name) end end + def invalid_byte_for_number_error_message + match_data = @string.match(/\{\s*(\w+)\((\w+):\s*([^\)]+)\)\s*\}/) + field = match_data[1] + argument = match_data[2] + value = match_data[3] + + "Argument '#{argument}' on Field '#{field}' has an invalid value (#{value}). Expected type 'number'." + end + ESCAPES = /\\["\\\/bfnrt]/ ESCAPES_REPLACE = { '\\"' => '"', diff --git a/spec/graphql/language/parser_spec.rb b/spec/graphql/language/parser_spec.rb index c3feafcc5a..503f252b68 100644 --- a/spec/graphql/language/parser_spec.rb +++ b/spec/graphql/language/parser_spec.rb @@ -116,7 +116,7 @@ expected_message = if USING_C_PARSER "syntax error, unexpected invalid token (\"-\") at [1, 8]" else - "Expected a number, but it was malformed (\"-\")" + "Argument 'b' on Field 'a' has an invalid value (-c). Expected type 'number'." end assert_equal expected_message, err.message end @@ -192,7 +192,7 @@ expected_msg = if USING_C_PARSER "syntax error, unexpected invalid token (\"-\") at [1, 19]" else - "Expected a number, but it was malformed (\"-\")" + "Argument 'argument' on Field 'field' has an invalid value (a-b). Expected type 'number'." end assert_equal expected_msg, err.message From f11fe5e11f25bbe813e220c25fbde48190e9f7d9 Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Wed, 2 Oct 2024 09:30:59 -0400 Subject: [PATCH 2/3] Add a few more tests for invalid minus sign parsing --- spec/graphql/language/parser_spec.rb | 52 ++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/spec/graphql/language/parser_spec.rb b/spec/graphql/language/parser_spec.rb index 503f252b68..420e0a9d99 100644 --- a/spec/graphql/language/parser_spec.rb +++ b/spec/graphql/language/parser_spec.rb @@ -121,6 +121,58 @@ assert_equal expected_message, err.message end + it "handles invalid minus signs in variable default values" do + err = assert_raises GraphQL::ParseError do + GraphQL.parse("query($something: Int = -foo) { }") + end + # TODO: this currently raises a NoMethodError: + # graphql-ruby/lib/graphql/language/lexer.rb:121:in `invalid_byte_for_number_error_message' + # graphql-ruby/lib/graphql/language/lexer.rb:75:in `advance' + # graphql-ruby/lib/graphql/language/parser.rb:97:in `advance_token' + # graphql-ruby/lib/graphql/language/parser.rb:166:in `definition' + # graphql-ruby/lib/graphql/language/parser.rb:108:in `document' + # graphql-ruby/lib/graphql/language/parser.rb:45:in `block in parse' + # graphql-ruby/lib/graphql/tracing/trace.rb:24:in `parse' + # graphql-ruby/lib/graphql/language/parser.rb:44:in `parse' + # graphql-ruby/lib/graphql/language/parser.rb:16:in `parse' + # graphql-ruby/lib/graphql.rb:39:in `parse' + # graphql-ruby/spec/graphql/language/parser_spec.rb:126:in `block (3 levels) in ' + assert_equal "TODO: add expected error messages here", err.message + end + + it "handles invalid minus signs in deeply nested input objects" do + err = assert_raises GraphQL::ParseError do + GraphQL.parse("{ doSomething(a: { b: { c: { d: -foo } } }) }") + end + # TODO: this currently removes `a:` from the error message: + # "Argument 'a' on Field 'doSomething' has an invalid value ({ b: { c: { d: -foo } } }). Expected type 'number'." + assert_equal "TODO: add expected error messages here", err.message + end + + it "handles invalid minus signs in schema definitions" do + err = assert_raises GraphQL::ParseError do + GraphQL.parse(" + type Query { + someField(a: Int = -foo): Int + } + ") + end + + # TODO: this currently raises the same NoMethodError as the other test + assert_equal "TODO: add expected error messages here", err.message + end + + it "handles invalid minus signs in list literals" do + err = assert_raises GraphQL::ParseError do + GraphQL.parse("{ + a1: a(b: [1,2,3]) + a2: a(b: [1, 2, -foo]) + }") + end + # TODO: this currently raises the same NoMethodError as the other test + assert_equal "TODO: add expected error messages here", err.message + end + it "allows operation names to match operation types" do doc = GraphQL.parse("query subscription { foo }") assert_equal "subscription", doc.definitions.first.name From 1c8ded0e0a1a140b74f1c4ca1d41a235e5fd3f88 Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Fri, 29 Nov 2024 10:13:07 -0500 Subject: [PATCH 3/3] Attempt to find the part after the minus --- lib/graphql/language/lexer.rb | 12 ++----- spec/graphql/language/parser_spec.rb | 49 +++++++++++++++------------- 2 files changed, 29 insertions(+), 32 deletions(-) diff --git a/lib/graphql/language/lexer.rb b/lib/graphql/language/lexer.rb index 0e1a444a6b..1aeb1da4c0 100644 --- a/lib/graphql/language/lexer.rb +++ b/lib/graphql/language/lexer.rb @@ -72,6 +72,9 @@ def advance # Check for a matched decimal: @scanner[1] ? :FLOAT : :INT else + # Attempt to find the part after the `-` + value = @scanner.scan(/-\s?[a-z0-9]*/i) + invalid_byte_for_number_error_message = "Expected type 'number', but it was malformed#{value.nil? ? "" : ": #{value.inspect}"}." raise_parse_error(invalid_byte_for_number_error_message) end when ByteFor::ELLIPSIS @@ -116,15 +119,6 @@ def debug_token_value(token_name) end end - def invalid_byte_for_number_error_message - match_data = @string.match(/\{\s*(\w+)\((\w+):\s*([^\)]+)\)\s*\}/) - field = match_data[1] - argument = match_data[2] - value = match_data[3] - - "Argument '#{argument}' on Field '#{field}' has an invalid value (#{value}). Expected type 'number'." - end - ESCAPES = /\\["\\\/bfnrt]/ ESCAPES_REPLACE = { '\\"' => '"', diff --git a/spec/graphql/language/parser_spec.rb b/spec/graphql/language/parser_spec.rb index 420e0a9d99..b3fad1adfd 100644 --- a/spec/graphql/language/parser_spec.rb +++ b/spec/graphql/language/parser_spec.rb @@ -116,7 +116,7 @@ expected_message = if USING_C_PARSER "syntax error, unexpected invalid token (\"-\") at [1, 8]" else - "Argument 'b' on Field 'a' has an invalid value (-c). Expected type 'number'." + "Expected type 'number', but it was malformed: \"-c\"." end assert_equal expected_message, err.message end @@ -125,28 +125,24 @@ err = assert_raises GraphQL::ParseError do GraphQL.parse("query($something: Int = -foo) { }") end - # TODO: this currently raises a NoMethodError: - # graphql-ruby/lib/graphql/language/lexer.rb:121:in `invalid_byte_for_number_error_message' - # graphql-ruby/lib/graphql/language/lexer.rb:75:in `advance' - # graphql-ruby/lib/graphql/language/parser.rb:97:in `advance_token' - # graphql-ruby/lib/graphql/language/parser.rb:166:in `definition' - # graphql-ruby/lib/graphql/language/parser.rb:108:in `document' - # graphql-ruby/lib/graphql/language/parser.rb:45:in `block in parse' - # graphql-ruby/lib/graphql/tracing/trace.rb:24:in `parse' - # graphql-ruby/lib/graphql/language/parser.rb:44:in `parse' - # graphql-ruby/lib/graphql/language/parser.rb:16:in `parse' - # graphql-ruby/lib/graphql.rb:39:in `parse' - # graphql-ruby/spec/graphql/language/parser_spec.rb:126:in `block (3 levels) in ' - assert_equal "TODO: add expected error messages here", err.message + expected_message = if USING_C_PARSER + "syntax error, unexpected invalid token (\"-\") at [1, 25]" + else + "Expected type 'number', but it was malformed: \"-foo\"." + end + assert_equal expected_message, err.message end it "handles invalid minus signs in deeply nested input objects" do err = assert_raises GraphQL::ParseError do GraphQL.parse("{ doSomething(a: { b: { c: { d: -foo } } }) }") end - # TODO: this currently removes `a:` from the error message: - # "Argument 'a' on Field 'doSomething' has an invalid value ({ b: { c: { d: -foo } } }). Expected type 'number'." - assert_equal "TODO: add expected error messages here", err.message + expected_message = if USING_C_PARSER + "syntax error, unexpected invalid token (\"-\") at [1, 33]" + else + "Expected type 'number', but it was malformed: \"-foo\"." + end + assert_equal expected_message, err.message end it "handles invalid minus signs in schema definitions" do @@ -157,9 +153,12 @@ } ") end - - # TODO: this currently raises the same NoMethodError as the other test - assert_equal "TODO: add expected error messages here", err.message + expected_message = if USING_C_PARSER + "syntax error, unexpected invalid token (\"-\") at [3, 28]" + else + "Expected type 'number', but it was malformed: \"-foo\"." + end + assert_equal expected_message, err.message end it "handles invalid minus signs in list literals" do @@ -169,8 +168,12 @@ a2: a(b: [1, 2, -foo]) }") end - # TODO: this currently raises the same NoMethodError as the other test - assert_equal "TODO: add expected error messages here", err.message + expected_message = if USING_C_PARSER + "syntax error, unexpected invalid token (\"-\") at [3, 25]" + else + "Expected type 'number', but it was malformed: \"-foo\"." + end + assert_equal expected_message, err.message end it "allows operation names to match operation types" do @@ -244,7 +247,7 @@ expected_msg = if USING_C_PARSER "syntax error, unexpected invalid token (\"-\") at [1, 19]" else - "Argument 'argument' on Field 'field' has an invalid value (a-b). Expected type 'number'." + "Expected type 'number', but it was malformed: \"-b\"." end assert_equal expected_msg, err.message