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

add support for $ in quoted string #4702

Merged
merged 7 commits into from
Feb 12, 2020
Merged

add support for $ in quoted string #4702

merged 7 commits into from
Feb 12, 2020

Conversation

poonai
Copy link
Contributor

@poonai poonai commented Jan 31, 2020

Fix #4695
Signed-off-by: balaji [email protected]


This change is Reviewable

@poonai poonai requested review from manishrjain and a team as code owners January 31, 2020 07:58
Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

:lgtm: other than a couple minor comments.

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @balajijinnah, @manishrjain, and @pawanrawal)


gql/parser.go, line 357 at r1 (raw file):

		return nil
	}
	// It's a quoated string so unquote it.

s/quoated/quoted

Also, the comment gives the impression that f is always quoted if the code reaches this point. I don't think that's true so the comment should reflect that. Something like "Unquote the variable if it contains quotes".


gql/parser.go, line 394 at r1 (raw file):

		for idx, v := range gq.Func.Args {
			if !v.IsGraphQLVar {
				// It's not a variable. So, unquoate it.

s/unquoate/unquote

Also same comment as above regarding the phrasing of the comment.

Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

There is possibly a cleaner way to do this. Also please add some more tests checking other places where the argument can be a GraphQL variable.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @balajijinnah and @manishrjain)


gql/parser.go, line 359 at r1 (raw file):

	// It's a quoated string so unquote it.
	var err error
	*res, err = unquoteIfQuoted(f)

I see what you are doing here, you are keeping the quoted value for the string and then unquoting it here but this function is only supposed to replace the value of a graphql variable and now its also doing this unquoting stuff.

Could you not have skipped the variable substitution if the arg is not a GraphQLVar? That is in substituteVariablesFilter and other places.

		for idx, v := range f.Func.Args {
			if !v.IsGraphQLVar {
				continue
			}
			if f.Func.Name == uidFunc {

gql/parser.go, line 396 at r1 (raw file):

				// It's not a variable. So, unquoate it.
				var err error
				gq.Func.Args[idx].Value, err = unquoteIfQuoted(gq.Func.Args[idx].Value)

You an just pass in v.Value here to the function.


gql/parser_test.go, line 5143 at r1 (raw file):

	query := `
	{
		q(func: eq(name, "Bob"), first:5)@filter(eq(description, "$yo")) {

Space before @filter.


gql/parser_test.go, line 5152 at r1 (raw file):

		Str: query,
	})
	require.NoError(t, err)

Can you also check that the value of eq argument is $yo?

Signed-off-by: balaji <[email protected]>
Signed-off-by: balaji <[email protected]>
Signed-off-by: balaji <[email protected]>
Signed-off-by: balaji <[email protected]>
Signed-off-by: balaji <[email protected]>
Copy link
Contributor Author

@poonai poonai 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, 6 unresolved discussions (waiting on @manishrjain, @martinmr, and @pawanrawal)


gql/parser.go, line 357 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

s/quoated/quoted

Also, the comment gives the impression that f is always quoted if the code reaches this point. I don't think that's true so the comment should reflect that. Something like "Unquote the variable if it contains quotes".

Done.


gql/parser.go, line 359 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

I see what you are doing here, you are keeping the quoted value for the string and then unquoting it here but this function is only supposed to replace the value of a graphql variable and now its also doing this unquoting stuff.

Could you not have skipped the variable substitution if the arg is not a GraphQLVar? That is in substituteVariablesFilter and other places.

		for idx, v := range f.Func.Args {
			if !v.IsGraphQLVar {
				continue
			}
			if f.Func.Name == uidFunc {

Done.


gql/parser.go, line 394 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

s/unquoate/unquote

Also same comment as above regarding the phrasing of the comment.

Done.


gql/parser.go, line 396 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

You an just pass in v.Value here to the function.

Done.


gql/parser_test.go, line 5143 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Space before @filter.

Done.


gql/parser_test.go, line 5152 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Can you also check that the value of eq argument is $yo?

Done.

Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

:lgtm once CI passes

Reviewed 1 of 2 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @manishrjain and @martinmr)

@poonai poonai merged commit 2f46887 into master Feb 12, 2020
@pawanrawal pawanrawal deleted the balaji/filter branch April 2, 2020 08:51
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.

$ as value is considered variable, despite quotes
4 participants