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

graphql: Fix mutation on predicate with special characters having dgraph directive. #5526

Merged
merged 9 commits into from
May 29, 2020

Conversation

arijitAD
Copy link

@arijitAD arijitAD commented May 27, 2020

Fixes GRAPHQL-411.
Fixes #5296


This change is Reviewable

Docs Preview: Dgraph Preview

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label May 27, 2020
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.

Could we also add an end to test which mutates some data for such a predicate and then queries it back? You can even update one of such existing mutation e2e tests if that is easier.

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @arijitAD and @MichaelJCompton)


graphql/resolve/mutation_rewriter.go, line 1006 at r1 (raw file):

			fieldDef := typ.Field(field)
			fieldName := typ.DgraphPredicate(field)
			if strings.HasPrefix(fieldName, "<") && strings.HasSuffix(fieldName, ">") {

This should probably not be here as this happens at mutation rewriting time. We should have this in wrappers.go where we compute this mapping on schema update.


graphql/resolve/schema.graphql, line 212 at r1 (raw file):

type Message {
  content: String! @dgraph(pred: "post")

Could you please fix the indentation on this one?


graphql/resolve/schema.graphql, line 213 at r1 (raw file):

type Message {
  content: String! @dgraph(pred: "post")
  author: String @dgraph(pred: "<职业>")

What happens if this didn't have <> and just had special characters, would that work fine?


graphql/schema/gqlschema_test.yml, line 1993 at r1 (raw file):

      }

  - name: "@dgraph predicate type validation gives no errors with non-null variations"

Look like this was a duplicate test?

Copy link
Author

@arijitAD arijitAD 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: 4 of 9 files reviewed, 4 unresolved discussions (waiting on @MichaelJCompton and @pawanrawal)


graphql/resolve/mutation_rewriter.go, line 1006 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This should probably not be here as this happens at mutation rewriting time. We should have this in wrappers.go where we compute this mapping on schema update.

We would still be using <> for the predicate when querying. Hence, if I make the change in wrapper.go it might query the wrong predicate.


graphql/resolve/schema.graphql, line 212 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Could you please fix the indentation on this one?

Done.


graphql/resolve/schema.graphql, line 213 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

What happens if this didn't have <> and just had special characters, would that work fine?

This is something that Dgraph recommends.
https://dgraph.io/docs/query-language/#predicates-i18n


graphql/schema/gqlschema_test.yml, line 1993 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Look like this was a duplicate test?

Yes

@arijitAD arijitAD requested a review from pawanrawal May 28, 2020 16:45
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.

Approving but ideally the mutation in e2e tests should be a GraphQL mutation and not a GraphQL+- one. We do that for other data also like starship etc, so you should be able to add that easily.

Reviewed 7 of 7 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @arijitAD and @MichaelJCompton)


graphql/e2e/common/query.go, line 1478 at r2 (raw file):

		}`,
	}
	result := "{\"queryMessage\":[{\"content\":\"content1\",\"author\":\"author1\"}]}"

You can use `` and then you don't need to escape the double quotes. Makes for cleaner code.


graphql/e2e/directives/test_data.json, line 104 at r2 (raw file):

    },
    {
        "uid":"_:Message1",

We should be adding this test data using a GraphQL mutation (like addMessage) to test the mutation logic end to end. Right now it's just doing a Dgraph mutation which doesn't do any of the query rewritings.


graphql/resolve/mutation_rewriter.go, line 1006 at r1 (raw file):

Previously, arijitAD (Arijit Das) wrote…

We would still be using <> for the predicate when querying. Hence, if I make the change in wrapper.go it might query the wrong predicate.

Yeah, agreed.

Copy link
Author

@arijitAD arijitAD 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: 6 of 9 files reviewed, 3 unresolved discussions (waiting on @MichaelJCompton and @pawanrawal)


graphql/e2e/common/query.go, line 1478 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

You can use `` and then you don't need to escape the double quotes. Makes for cleaner code.

Done.


graphql/e2e/directives/test_data.json, line 104 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

We should be adding this test data using a GraphQL mutation (like addMessage) to test the mutation logic end to end. Right now it's just doing a Dgraph mutation which doesn't do any of the query rewritings.

Done.


graphql/resolve/mutation_rewriter.go, line 1006 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Yeah, agreed.

Done.

@arijitAD arijitAD merged commit 92232c0 into master May 29, 2020
arijitAD pushed a commit that referenced this pull request Jun 4, 2020
…aph directive. (#5526)

* Fix Dgraph directive for multiple language.

(cherry picked from commit 92232c0)
arijitAD pushed a commit that referenced this pull request Jun 8, 2020
#5577)

* graphql: Fix mutation on predicate with special characters having dgraph directive. (#5526)

* Fix Dgraph directive for multiple languages.

(cherry picked from commit 92232c0)
arijitAD pushed a commit that referenced this pull request Jul 6, 2020
#5577)

* graphql: Fix mutation on predicate with special characters having dgraph directive. (#5526)

* Fix Dgraph directive for multiple languages.

(cherry picked from commit 92232c0)
(cherry picked from commit 829cbb0)
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
…aph directive. (dgraph-io#5526)

* Fix Dgraph directive for multiple language.
@joshua-goldstein joshua-goldstein deleted the arijitAD/Fix-dgraph-directive branch August 11, 2022 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

GraphQL Schema Not support <> using @dgraph(pred: "<other language>")
2 participants