-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Display error when multiple mutation blocks #2817
Display error when multiple mutation blocks #2817
Conversation
removed parse error for multiple mutation ops.
…sue-2815_server_executes_only_one_of_two_mutations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @srfrog and @manishrjain)
gql/parser_mutation.go, line 50 at r1 (raw file):
if item.Typ == itemRightCurl { // mutations must be enclosed in a single block. if it.Next() && it.Item().Typ == itemLeftCurl {
We should check for itemEOF instead.
gql/parser_test.go, line 4296 at r1 (raw file):
func TestParseMutationTooManyBlocks(t *testing.T) { m := `{ set { _:a1 <reg> "a1 content" . }
Can you add a test case like this:
{ set { ...}} something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @manishrjain)
gql/parser_mutation.go, line 50 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
We should check for itemEOF instead.
Done.
gql/parser_test.go, line 4296 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Can you add a test case like this:
{ set { ...}} something
Also added a case for comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @manishrjain)
gql/parser_test.go, line 4296 at r1 (raw file):
Previously, srfrog (Gus) wrote…
Also added a case for comments
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @srfrog)
gql/parser_mutation.go, line 27 at r2 (raw file):
) var ErrMutationTooManyBlocks = errors.New("Too many mutation blocks.")
Don't make a generic error. See below.
gql/parser_mutation.go, line 51 at r2 (raw file):
// mutations must be enclosed in a single block. if it.Next() && it.Item().Typ != lex.ItemEOF { return nil, ErrMutationTooManyBlocks
We can return a very specific error message here, which would be useful for the user. Like the text which we found here.
In general, I think lexer can do a better job of tracking the line and the column number of the item. And then display that in error messages, so users know exactly where their errors are. All of these errors in parsers need to be improved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @manishrjain)
gql/parser_mutation.go, line 27 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Don't make a generic error. See below.
Done.
gql/parser_mutation.go, line 51 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
We can return a very specific error message here, which would be useful for the user. Like the text which we found here.
In general, I think lexer can do a better job of tracking the line and the column number of the item. And then display that in error messages, so users know exactly where their errors are. All of these errors in parsers need to be improved.
I've added some context to the error. I will add line support in the parser in another issue.
…sue-2815_server_executes_only_one_of_two_mutations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved
* added two checks for multiple mutation blocks. * added test for parsing mutation with too many blocks. * added test for mutation with too many blocks. removed parse error for multiple mutation ops. * check that mutation block is ended. * added test case to make sure there's no extra text after the mutation. * check that comments after mutation still work. * more contextual error when parsing an invalid mutation block. * changed test to use contextual errors.
This PR adds a check that there's only a single mutation block per request, otherwise an error is returned.
Closes #2815
This change is