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

fix(GraphQL): Link xids properly if there are duplicate xids within the same add request. #6265

Merged
merged 4 commits into from
Aug 27, 2020

Conversation

pawanrawal
Copy link
Contributor

@pawanrawal pawanrawal commented Aug 24, 2020

Fixes GRAPHQL-641

See https://discuss.dgraph.io/t/residual-issue-linking-grandchild-in-mutation-with-custom-ids/9613 for the issue details.

If the same nested xid was used as a leaf node within an addType mutation, it was only linked the first time that it was used and not subsequently for later objects. This change fixes that by adding the linkage mutations and only removing the mutation which was creating the object with the xid again.


This change is Reviewable

Docs Preview: Dgraph Preview

…weren't linked properly beyond the first object.
@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Aug 24, 2020
Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

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

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


graphql/resolve/add_mutation_test.yaml, line 2286 at r1 (raw file):

cond: "@if(eq(len(Comment16), 0) AND eq(len(Comment14), 0))"

we are creating relpy1 only if comment1 doesn't exist. What if comment1 existed already? In that case we won't be creating reply1 and the later mutation for comment2 won't know what is reply1.

If comment1 existed already would we error out the whole mutation? I feel like it should still work.

Also, we can add some e2e tests.


graphql/resolve/add_mutation_test.yaml, line 2309 at r1 (raw file):

 comment1

comment2

Copy link
Contributor Author

@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.

Reviewed 10 of 10 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @abhimanyusinghgaur and @MichaelJCompton)


graphql/e2e/common/mutation.go, line 768 at r2 (raw file):

		ExpectedNumDeletedComments int
	}{
		"2nd level nodes don't exist but third level does": {

8 cases that I could think of


graphql/e2e/directives/schema_response.json, line 15 at r2 (raw file):

        {
            "predicate": "Category.name",
            "type": "string"

This is a just a move from inline to the file.


graphql/e2e/normal/schema_response.json, line 371 at r2 (raw file):

                {
                    "name": "Category.name"
                },

Moved this to a separate file to make formatting easy, I can just use my editor's formatting now. Its harder to do that when its a string.


graphql/resolve/add_mutation_test.yaml, line 2286 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
cond: "@if(eq(len(Comment16), 0) AND eq(len(Comment14), 0))"

we are creating relpy1 only if comment1 doesn't exist. What if comment1 existed already? In that case we won't be creating reply1 and the later mutation for comment2 won't know what is reply1.

If comment1 existed already would we error out the whole mutation? I feel like it should still work.

Also, we can add some e2e tests.

So we do create reply1 if comment2 doesn't exist as well now. Nice catch. If comment1 existed, then we won't link it to reply1. That is the behavior for deep mutations with xids. Added e2e tests for all the 8 cases possible here.


graphql/resolve/add_mutation_test.yaml, line 2309 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
 comment1

comment2

Done.

Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur 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 10 of 10 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @MichaelJCompton and @pawanrawal)


graphql/e2e/common/mutation.go, line 768 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

8 cases that I could think of

Nice set of cases, they cover all the scenarios for this 3 level test.


graphql/e2e/directives/schema_response.json, line 15 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This is a just a move from inline to the file.

we will finally get rid of those formatting errors which were hard to find in the previous JSON string 😌

@pawanrawal pawanrawal merged commit 12b2716 into master Aug 27, 2020
@pawanrawal pawanrawal deleted the pawanrawal/deep-xid-another-fix branch August 27, 2020 08:41
pawanrawal added a commit that referenced this pull request Aug 27, 2020
…he same add request. (#6265)

Fixes GRAPHQL-641

See https://discuss.dgraph.io/t/residual-issue-linking-grandchild-in-mutation-with-custom-ids/9613 for the issue details.

If the same nested xid was used as a leaf node within an addType mutation, it was only linked the first time that it was used and not subsequently for later objects. This change fixes that by adding the linkage mutations and only removing the mutation which was creating the object with the xid again.

(cherry picked from commit 12b2716)
pawanrawal added a commit that referenced this pull request Aug 27, 2020
…he same add request. (#6265) (#6297)

* fix(GraphQL): Link xids properly if there are duplicate xids within the same add request. (#6265)

Fixes GRAPHQL-641

See https://discuss.dgraph.io/t/residual-issue-linking-grandchild-in-mutation-with-custom-ids/9613 for the issue details.

If the same nested xid was used as a leaf node within an addType mutation, it was only linked the first time that it was used and not subsequently for later objects. This change fixes that by adding the linkage mutations and only removing the mutation which was creating the object with the xid again.

(cherry picked from commit 12b2716)
abhimanyusinghgaur added a commit that referenced this pull request Sep 21, 2020
…he same add request. (#6265)

Fixes GRAPHQL-641

See https://discuss.dgraph.io/t/residual-issue-linking-grandchild-in-mutation-with-custom-ids/9613 for the issue details.

If the same nested xid was used as a leaf node within an addType mutation, it was only linked the first time that it was used and not subsequently for later objects. This change fixes that by adding the linkage mutations and only removing the mutation which was creating the object with the xid again.

(cherry picked from commit 12b2716)

# Conflicts:
#	graphql/e2e/directives/dgraph_directives_test.go
#	graphql/e2e/normal/normal_test.go
#	graphql/resolve/add_mutation_test.yaml
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.

2 participants