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

Display error when multiple mutation blocks #2817

Merged

Conversation

srfrog
Copy link
Contributor

@srfrog srfrog commented Dec 10, 2018

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 Reviewable

…sue-2815_server_executes_only_one_of_two_mutations
Copy link
Contributor

@manishrjain manishrjain left a 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

Copy link
Contributor Author

@srfrog srfrog left a 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

Copy link
Contributor Author

@srfrog srfrog left a 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.

Copy link
Contributor

@manishrjain manishrjain left a 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.

Copy link
Contributor Author

@srfrog srfrog left a 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
Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@srfrog srfrog merged commit c5d9320 into master Jan 3, 2019
@srfrog srfrog deleted the feature/issue-2815_server_executes_only_one_of_two_mutations branch January 3, 2019 00:44
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants