graphql: ensure @id uniqueness within a mutation #4959
Conversation
abhimanyusinghgaur
requested review from
harshil-goel,
MichaelJCompton and
pawanrawal
abhimanyusinghgaur
added
the
area/graphql
There was a problem hiding this comment.
Its looking good. Got some comments for your consideration and would like to see some more tests in add_mutation_test.yaml atleast for the success cases if errors are not possible to be tested there.
Reviewed 9 of 10 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @abhimanyusinghgaur, @harshil-goel, @manishrjain, and @MichaelJCompton)
graphql/e2e/common/mutation.go, line 3038 at r2 (raw file):
// cleanup deleteStateExpected := `{"deleteState" : { "msg": "Deleted" } }` filter := map[string]interface{}{
Ouch, this looks very verbose. Perhaps we should support taking in a list of xids like we do for ids.
graphql/resolve/mutation_rewriter.go, line 101 at r2 (raw file):
// would be easier as the variable name can be easily determined by a human :) xidVal = strings.ReplaceAll(xidVal, " ", "__space__") return fmt.Sprintf("%s%s", typ.Name(), xidVal)
What happens if there are other characters in the xidVal like /, are they allowed in variable names? I was thinking that we'd have a map of
typ.Name() + xidVal => variable (this could just have been generated as before)
Everytime we want to assign a new variable we can just check that map to see if we have already looked at the type and xidVal combination. Then we don't need special handling for any characters.
graphql/resolve/mutation_rewriter.go, line 821 at r2 (raw file):
} // if this object has an xid, then we don't need to rewrite its children if we have encountered
This is because they'll have the same properties as when we encountered it for the first time?
There was a problem hiding this comment.
I didn't get a chance to go through main algorithm, but made some comments on the tests and variables.
Yes, also add some tests to add_mutation_test.yaml, so we can track what mutations this actually makes.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @abhimanyusinghgaur, @harshil-goel, @manishrjain, and @pawanrawal)
graphql/e2e/common/mutation.go, line 2891 at r2 (raw file):
} func topLevelUniqueXIDsTest(t *testing.T) {
this doesn't look like a test for what this PR adds. It's a test that adding existing XIDs results in errors.
Check if we have an equivalent test elsewhere and if we do remove this. If we don't, please move it out of duplicateXidInSingleMutation.
graphql/e2e/common/mutation.go, line 2936 at r2 (raw file):
// cleanup deleteStateExpected := `{"deleteState" : { "msg": "Deleted" } }`
this should test how man states were deleted
graphql/e2e/common/mutation.go, line 2955 at r2 (raw file):
newStates := []*state{ {Name: "State1", Code: "S1"}, {Name: "State2", Code: "S1"},
what happens toplevel if there's two objects that are exactly the same?
graphql/e2e/common/mutation.go, line 2986 at r2 (raw file):
} func deepMutationUniqueXIDsTest(t *testing.T) {
I don't get what this is testing? Don't we already have tests that deep mutations work?
What does this test add that isn't tested by existing deep mutation tests?
graphql/e2e/common/mutation.go, line 3038 at r2 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Ouch, this looks very verbose. Perhaps we should support taking in a list of xids like we do for ids.
I've added a Jira card
graphql/e2e/common/mutation.go, line 3133 at r2 (raw file):
} // creating duplicate xids with different structure should give errors
break into a different test
graphql/e2e/common/mutation.go, line 3160 at r2 (raw file):
gqlResponse = postExecutor(t, graphqlURL, addCountryParams) expectedErrors := getDuplicateXIDErrors("addCountry", []string{"S4", "S5", "S6", "S6"})
why s6 in here twice?
graphql/resolve/add_mutation_test.yaml, line 295 at r2 (raw file):
dgmutations: - setjson: | { "uid" : "_:Statensw",
I'd prefer keeping State1. This is gunna get unreadable - particularly 'UserA.N.__space__Author'.
How about mapping xid value to a structure that has the variable name and the properties, so we can keep State1
graphql/resolve/mutation_rewriter.go, line 101 at r2 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
What happens if there are other characters in the xidVal like
/, are they allowed in variable names? I was thinking that we'd have a map of
typ.Name() + xidVal=> variable (this could just have been generated as before)
Everytime we want to assign a new variable we can just check that map to see if we have already looked at the type and xidVal combination. Then we don't need special handling for any characters.
Yep, I made a similar comment as well.
There was a problem hiding this comment.
Reviewable status: 5 of 11 files reviewed, 12 unresolved discussions (waiting on @abhimanyusinghgaur, @golangcibot, @harshil-goel, @manishrjain, @MichaelJCompton, and @pawanrawal)
graphql/e2e/common/mutation.go, line 2891 at r2 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
this doesn't look like a test for what this PR adds. It's a test that adding existing XIDs results in errors.
Check if we have an equivalent test elsewhere and if we do remove this. If we don't, please move it out of duplicateXidInSingleMutation.
Done.
There was a test for this.
graphql/e2e/common/mutation.go, line 2955 at r2 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
what happens toplevel if there's two objects that are exactly the same?
Done.
It will return error for that case as well.
Added a test for this.
graphql/e2e/common/mutation.go, line 2986 at r2 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
I don't get what this is testing? Don't we already have tests that deep mutations work?
What does this test add that isn't tested by existing deep mutation tests?
Done.
Removed this, there was an existing test.
graphql/e2e/common/mutation.go, line 3038 at r2 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
I've added a Jira card
Done.
graphql/e2e/common/mutation.go, line 3133 at r2 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
break into a different test
Done.
graphql/e2e/common/mutation.go, line 3160 at r2 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
why s6 in here twice?
It's because how squash fragments works currently. When squashing two lists of mutation fragments of size m and n, it will give back a list of size m*n. And, as a result, we are getting error two times for S6 because the first S6 creates two mutation fragments, and when squashing that with error fragment for S6, it results in two fragments with errors.
graphql/resolve/add_mutation_test.yaml, line 295 at r2 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
I'd prefer keeping State1. This is gunna get unreadable - particularly 'UserA.N.__space__Author'.
How about mapping xid value to a structure that has the variable name and the properties, so we can keep State1
Done.
graphql/resolve/mutation_rewriter.go, line 125 at r1 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
S1019: should use make(map[string]interface{}) instead (from
gosimple)
Done.
graphql/resolve/mutation_rewriter.go, line 126 at r1 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
S1019: should use make(map[string]bool) instead (from
gosimple)
Done.
graphql/resolve/mutation_rewriter.go, line 101 at r2 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
Yep, I made a similar comment as well.
Done.
graphql/resolve/mutation_rewriter.go, line 821 at r2 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
This is because they'll have the same properties as when we encountered it for the first time?
Yes!
There was a problem hiding this comment.
Got one comment about start index for variables and then it should be good to go.
Reviewed 6 of 6 files at r3.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @abhimanyusinghgaur, @golangcibot, @harshil-goel, @manishrjain, and @MichaelJCompton)
graphql/resolve/add_mutation_test.yaml, line 286 at r3 (raw file):
dgquery: |- query { State2 as State2(func: eq(State.code, "nsw")) @filter(type(State)) {
Did we change something so that now everything starts with 2 instead of 1?
graphql/resolve/add_mutation_test.yaml, line 1073 at r3 (raw file):
uid } var(func: uid(State10)) {
Strange that there are two queries doing the same thing here, also above. Is this expected?
graphql/resolve/add_mutation_test.yaml, line 1082 at r3 (raw file):
dgmutations: - setjson: | { "Country.name":"Country0",
Wow, this a long test output and very hard to verify. Hope you've had a good look at this.
There was a problem hiding this comment.
Generally looks fine. I just had some points inline about testing.
Another bit of testing that doesn't seem to be here is when the object with an xid has deeper stuff hanging off it - I can see you are handling that in the code, but it's not tested anywhere.
Another question is about interfaces. I'm always asking on PRs if there's any interaction with interfaces because they turn out to be subtle. So how about this ... interface A has an xid in it. A is implemented by B and C. It looks to me like the code here would allow through a mutation that created both a B and a C in the one mutation with the same xid and that would in turn be two A's with the same xid. ...prove me wrong with a test :-)
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @abhimanyusinghgaur, @golangcibot, @harshil-goel, @manishrjain, @MichaelJCompton, and @pawanrawal)
graphql/e2e/common/mutation.go, line 2955 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
Done.
It will return error for that case as well.
Added a test for this.
Split this up please - looks like there's two tests going on at once here. One with name and code same and one with same code, but different names.
graphql/e2e/common/mutation.go, line 3160 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
It's because how
squash fragmentsworks currently. When squashing two lists of mutation fragments of sizemandn, it will give back a list of sizem*n. And, as a result, we are getting error two times forS6because the firstS6creates two mutation fragments, and when squashing that with error fragment forS6, it results in two fragments with errors.
Fair enough. Feels wrong to me - the errors they get should be proportional to the number of errors in their input, not proportional to implementation. But let's leave it till someone complains.
graphql/e2e/common/mutation.go, line 2884 at r3 (raw file):
deleteUser(t, *newUser) } func duplicateXidInSingleMutation(t *testing.T) {
what if the duplicate xids are at different levels in the mutation tree. For example: addAuthor
with Michael as the added author, but then Michael comes up again deeper in the tree?
graphql/e2e/common/mutation.go, line 2929 at r3 (raw file):
Quoted 4 lines of code…
newState := addState(t, "California", postExecutor) requireState(t, newState.ID, newState, postExecutor) newState.ID = "" newState.Country = nil
not sure what using the addState bit adds here? Again, it looks like testing something to do with xids and things that are already in the DB, but these tests shouldn't be doing that.
These tests should be only about this new behaviour and as far as I can see, we reject before ever getting to the DB.
graphql/e2e/common/mutation.go, line 2935 at r3 (raw file):
Name: "Country0", States: []*state{ {Code: newState.Code, Name: "State0"},
the test is also really hard to read because of newState ... hard to see what its code is without going elsewhere.
graphql/e2e/common/mutation.go, line 2945 at r3 (raw file):
{Code: newState.Code, Name: "State0"}, {Code: "S1", Name: "State1", Capital: "Cap1"}, {Code: "S2", Name: "State2", Capital: "Cap2"},
I don't really get why there's "newState.Code", "S1" and "S2" - looks like we are testing exactly the same thing, three times.
graphql/e2e/common/mutation.go, line 2953 at r3 (raw file):
{Code: "S3", Name: "State3", Capital: "Cap3"}, {Code: "S3", Name: "State3", Capital: "Cap3"}, {Code: "S3", Name: "State3", Capital: "Cap3"},
why have three copies of this?
graphql/e2e/common/mutation.go, line 3005 at r3 (raw file):
// cleanup deleteStateExpected := `{"deleteState" : { "msg": "Deleted" } }`
please check that the right number of nodes are deleted here.
graphql/resolve/add_mutation_test.yaml, line 286 at r3 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Did we change something so that now everything starts with 2 instead of 1?
Yeah, just from the point of view of reviewing the PR, it feels wrong that so many unrelated tests just changed.
graphql/resolve/add_mutation_test.yaml, line 995 at r3 (raw file):
- name: "Top Level Duplicate XIDs Test"
the error cases really belong here, so delete the e2e versions of the same tests and put them here.
graphql/resolve/add_mutation_test.yaml, line 1082 at r3 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Wow, this a long test output and very hard to verify. Hope you've had a good look at this.
I think the problem is that this is two tests rolled together. Please split it into two cases: the "S1" and "S2" bits. Needs to be simple enough that it's just one or two cases, so it's easy enough to check.
graphql/resolve/add_mutation_test.yaml, line 1324 at r3 (raw file):
- name: "Deep Mutation Duplicate XIDs with different object Test"
again no need for this test in the e2e
graphql/resolve/update_mutation_test.yaml, line 375 at r3 (raw file):
} dgmutations: - deletejson: |
there probably should be an update test to show that the same behaviour is happening when an update creates new nodes.
There was a problem hiding this comment.
Reviewable status: 0 of 12 files reviewed, 22 unresolved discussions (waiting on @golangcibot, @harshil-goel, @manishrjain, @MichaelJCompton, and @pawanrawal)
graphql/e2e/common/mutation.go, line 2936 at r2 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
this should test how man states were deleted
Done.
graphql/e2e/common/mutation.go, line 2955 at r2 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
Split this up please - looks like there's two tests going on at once here. One with name and code same and one with same code, but different names.
Removed the error case from e2
graphql/e2e/common/mutation.go, line 3160 at r2 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
Fair enough. Feels wrong to me - the errors they get should be proportional to the number of errors in their input, not proportional to implementation. But let's leave it till someone complains.
Ok.
graphql/e2e/common/mutation.go, line 2884 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
what if the duplicate xids are at different levels in the mutation tree. For example: addAuthor
with Michael as the added author, but then Michael comes up again deeper in the tree?
Currently, there is following behaviour:
- first at top level, then deeper in the tree: allowed only if both the objects are same
- first deeper in the tree, then at top level: rejected as a duplicate
graphql/e2e/common/mutation.go, line 2929 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
newState := addState(t, "California", postExecutor) requireState(t, newState.ID, newState, postExecutor) newState.ID = "" newState.Country = nilnot sure what using the addState bit adds here? Again, it looks like testing something to do with xids and things that are already in the DB, but these tests shouldn't be doing that.
These tests should be only about this new behaviour and as far as I can see, we reject before ever getting to the DB.
Done.
Minimized the test.
graphql/e2e/common/mutation.go, line 2935 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
the test is also really hard to read because of newState ... hard to see what its code is without going elsewhere.
Done.
graphql/e2e/common/mutation.go, line 2945 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
I don't really get why there's "newState.Code", "S1" and "S2" - looks like we are testing exactly the same thing, three times.
Done.
Minimized the test.
graphql/e2e/common/mutation.go, line 2953 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
why have three copies of this?
Done.
Minimized the test.
graphql/e2e/common/mutation.go, line 3005 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
please check that the right number of nodes are deleted here.
Done.
graphql/e2e/common/mutation.go, line 3002 at r4 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
func
getDuplicateXIDErrorsis unused (fromunused)
Done.
graphql/resolve/add_mutation_test.yaml, line 286 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
Yeah, just from the point of view of reviewing the PR, it feels wrong that so many unrelated tests just changed.
For every object which contains xid, always 2 calls happen to VariableGenerator.Next(), and the variable from the 2nd call gets used. So, thats why this behaviour is there.
graphql/resolve/add_mutation_test.yaml, line 995 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
the error cases really belong here, so delete the e2e versions of the same tests and put them here.
Done.
graphql/resolve/add_mutation_test.yaml, line 1073 at r3 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Strange that there are two queries doing the same thing here, also above. Is this expected?
Yes!
Although, the queries are the same thing, but they are used during delete mutation for different country objects.
graphql/resolve/add_mutation_test.yaml, line 1082 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
I think the problem is that this is two tests rolled together. Please split it into two cases: the "S1" and "S2" bits. Needs to be simple enough that it's just one or two cases, so it's easy enough to check.
Done.
graphql/resolve/add_mutation_test.yaml, line 1324 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
again no need for this test in the e2e
Done.
graphql/resolve/update_mutation_test.yaml, line 375 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
there probably should be an update test to show that the same behaviour is happening when an update creates new nodes.
Done.
There was a problem hiding this comment.
looks good. Just give me a day to think about test cases to make sure we are covering everything.
Reviewable status: 0 of 12 files reviewed, 13 unresolved discussions (waiting on @abhimanyusinghgaur, @golangcibot, @harshil-goel, @manishrjain, @MichaelJCompton, and @pawanrawal)
graphql/e2e/common/mutation.go, line 2884 at r3 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
Currently, there is following behaviour:
- first at top level, then deeper in the tree: allowed only if both the objects are same
- first deeper in the tree, then at top level: rejected as a duplicate
I don't understand?
graphql/e2e/common/mutation.go, line 2372 at r5 (raw file):
} func deleteState(
could this now call deleteGqlType?
graphql/e2e/common/mutation.go, line 2431 at r5 (raw file):
int(deleteType["numUids"].(float64)
why do we have to do this?
graphql/resolve/add_mutation_test.yaml, line 1078 at r5 (raw file):
- setjson: | { "City.name":"City0",
I found this really confusing with name City0 and uid _:City1 :-(
can we give the name something else so it's easier to keep track.
graphql/resolve/add_mutation_test.yaml, line 1107 at r5 (raw file):
"District.cities":[{"uid":"_:City4"}], "dgraph.type":["District"], "uid":"_:District3"
so blank nodes work across setjson blocks like this - cool
There was a problem hiding this comment.
Reviewable status: 0 of 12 files reviewed, 13 unresolved discussions (waiting on @golangcibot, @harshil-goel, @manishrjain, @MichaelJCompton, and @pawanrawal)
graphql/e2e/common/mutation.go, line 2884 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
I don't understand?
I can explain in discussion.
graphql/e2e/common/mutation.go, line 2372 at r5 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
could this now call deleteGqlType?
Yes!
No need to write any delete mutations for any graphql types now!!!
graphql/e2e/common/mutation.go, line 2431 at r5 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
int(deleteType["numUids"].(float64)why do we have to do this?
because of json unmarshalling. Json by default unmarshals any number to float64, which then needs to be converted to int in order to compare.
graphql/resolve/add_mutation_test.yaml, line 1078 at r5 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
I found this really confusing with name City0 and uid _:City1 :-(
can we give the name something else so it's easier to keep track.
Done.
graphql/resolve/add_mutation_test.yaml, line 1107 at r5 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
so blank nodes work across setjson blocks like this - cool
Done.
abhimanyusinghgaur
force-pushed
the
abhimanyu/@id-bug
branch
from
51031cf to
e0379d7
Compare
There was a problem hiding this comment.
I'm 99% of the way there.
-
Can you please add an e2e that's similar to "Deep Mutation Duplicate XIDs with same object Test" the way that it leaves extra bits in the mutation is interesting. I really just want to ensure that Dgraph does the right thing with a single mutation that adds a blank node twice.
-
Can you add a unit test case like addState with S1 - country -> C1 - states -> [ {S1} ] ... this'll fail right? because the top level object and the internal S1 won't turn out to be equal. That means there's no way to add a circular xid mutation like this? Is that OK?
-
what about what we discussed yesterday for your commit "reject at top level only if seen previously at top level". So something like addState [ {s1 -country-> C1 -states-> [{s2}] }, {s2} ] Vs the same thing with the list order reversed (just unit tests is fine to see that it rewrites to something sensible)
These conditions are a bit strange cond: "@if(eq(len(Teacher3), 0) AND eq(len(Teacher3), 1) AND gt(len(x), 0))", but it's an edge case, so should be fine.
Reviewable status: 0 of 12 files reviewed, 13 unresolved discussions (waiting on @golangcibot, @harshil-goel, @manishrjain, @MichaelJCompton, and @pawanrawal)
There was a problem hiding this comment.
Has thrown up another deep mutation xid error that we'll fix in a separate PR.
Reviewable status: 0 of 12 files reviewed, 13 unresolved discussions (waiting on @golangcibot, @harshil-goel, @manishrjain, @MichaelJCompton, and @pawanrawal)
Fixes #4882.
This change is