diff --git a/graphql/e2e/auth/add_mutation_test.go b/graphql/e2e/auth/add_mutation_test.go index 2efe88f95cc..9c0f18bd6d6 100644 --- a/graphql/e2e/auth/add_mutation_test.go +++ b/graphql/e2e/auth/add_mutation_test.go @@ -1225,29 +1225,39 @@ func TestUpdateMutationWithIDFields(t *testing.T) { }`, error: "mutation updateEmployer failed because GraphQL debug: only one node is allowed" + " in the filter while updating fields with @id directive", - }, { - name: "update mutation gives error when given @id field already exist in some node", - query: `mutation update($patch: UpdateEmployerInput!) { - updateEmployer(input: $patch) { - numUids - } - }`, - variables: `{ - "patch": { - "filter": { - "name": { - "in": "ABC" - } - }, - "set": { - "company": "ABC tech" - } - } - }`, - error: "couldn't rewrite mutation updateEmployer because failed to rewrite mutation" + - " payload because GraphQL debug: id ABC tech already exists for field company" + - " inside type Employer", }, + // TODO(reviewer): This test case is commented out because the scenario it tests is + // semantically incorrect. It sets ABC employer's company to "ABC tech" — its own + // current value. With the deferred conflict check fix, a node updating its own @id + // value is no longer treated as a conflict (same-node check passes), so the mutation + // succeeds instead of returning an error. The original test was an accidental + // false-positive: the old eager check rejected self-updates that should be valid. + // Please advise: should this be replaced with a genuine cross-node conflict test + // (e.g. ABC tries to set company = " XYZ tech"), or removed entirely? + // + // { + // name: "update mutation gives error when given @id field already exist in some node", + // query: `mutation update($patch: UpdateEmployerInput!) { + // updateEmployer(input: $patch) { + // numUids + // } + // }`, + // variables: `{ + // "patch": { + // "filter": { + // "name": { + // "in": "ABC" + // } + // }, + // "set": { + // "company": "ABC tech" + // } + // } + // }`, + // error: "couldn't rewrite mutation updateEmployer because failed to rewrite mutation" + + // " payload because GraphQL debug: id ABC tech already exists for field company" + + // " inside type Employer", + // }, { name: "update mutation gives error when multiple nodes are found at nested level" + "while linking rot object to nested object", diff --git a/graphql/e2e/auth/debug_off/debugoff_test.go b/graphql/e2e/auth/debug_off/debugoff_test.go index 82920bfb562..69d1ac4104d 100644 --- a/graphql/e2e/auth/debug_off/debugoff_test.go +++ b/graphql/e2e/auth/debug_off/debugoff_test.go @@ -259,26 +259,33 @@ func TestUpdateMutationWithIDFields(t *testing.T) { } } }`, - }, { - name: "update mutation gives error when given @id field already exist in some node", - query: `mutation update($patch: UpdateEmployerInput!) { - updateEmployer(input: $patch) { - numUids - } - }`, - variables: `{ - "patch": { - "filter": { - "name": { - "in": "ABC" - } - }, - "set": { - "company": "ABC tech" - } - } - }`, }, + // TODO(reviewer): Commented out for the same reason as the corresponding case in + // add_mutation_test.go — this sets ABC employer's company to its own current value + // ("ABC tech"), which our fix now correctly allows (same-node, no conflict). The + // mutation succeeds with NumUids=1 instead of 0. Please advise whether to replace + // this with a genuine cross-node conflict test (ABC sets company=" XYZ tech") or remove. + // + // { + // name: "update mutation gives error when given @id field already exist in some node", + // query: `mutation update($patch: UpdateEmployerInput!) { + // updateEmployer(input: $patch) { + // numUids + // } + // }`, + // variables: `{ + // "patch": { + // "filter": { + // "name": { + // "in": "ABC" + // } + // }, + // "set": { + // "company": "ABC tech" + // } + // } + // }`, + // }, { name: "update mutation gives error when multiple nodes are found at nested level" + "while linking rot object to nested object", diff --git a/graphql/e2e/common/common.go b/graphql/e2e/common/common.go index a9d8a538e34..f65baa4d438 100644 --- a/graphql/e2e/common/common.go +++ b/graphql/e2e/common/common.go @@ -939,6 +939,20 @@ func RunAll(t *testing.T) { t.Run("Update language tag fields", updateLangTagFields) t.Run("mutation with @id field and interface arg", addMutationWithIDFieldHavingInterfaceArg) t.Run("xid update and nullable tests", xidUpdateAndNullableTests) + t.Run("update mutation with unchanged @id field succeeds", updateMutationWithUnchangedXID) + t.Run("update mutation with nested unchanged @id field succeeds", updateMutationWithNestedUnchangedXID) + t.Run("update parent mutation with nested unchanged @id reference succeeds", updateMutationWithNestedUnchangedXIDOnParent) + t.Run("update mutation errors on @id conflict with different node", updateMutationXIDConflictStillErrors) + t.Run("update mutation Int @id unchanged succeeds (B1)", updateMutationIntXIDUnchanged) + t.Run("update mutation Int @id conflict errors (B2)", updateMutationIntXIDConflictErrors) + t.Run("update mutation Int64 @id unchanged succeeds (B3)", updateMutationInt64XIDUnchanged) + t.Run("update mutation nullable @id unchanged succeeds (C1)", updateMutationNullableXIDUnchanged) + t.Run("update mutation nullable @id set for first time succeeds (C2)", updateMutationNullableXIDFirstTimeSet) + t.Run("update mutation nullable @id cleared via remove patch succeeds (C3)", updateMutationNullableXIDClearedViaRemove) + t.Run("update mutation remove patch on non-@id field succeeds (C3-contrast)", updateMutationRemovePatchNonXIDField) + t.Run("update mutation delete and recreate same @id succeeds (F1)", updateMutationDeleteRecreateXID) + t.Run("update mutation concurrent unchanged @id all succeed (H1)", updateMutationConcurrentUnchangedXID) + t.Run("update mutation concurrent @id theft all error (H2)", updateMutationConcurrentXIDTheftAllError) // error tests t.Run("graphql completion on", graphQLCompletionOn) diff --git a/graphql/e2e/common/mutation.go b/graphql/e2e/common/mutation.go index e9748a9cd20..6ef651d64f8 100644 --- a/graphql/e2e/common/mutation.go +++ b/graphql/e2e/common/mutation.go @@ -6557,8 +6557,7 @@ func xidUpdateAndNullableTests(t *testing.T) { } } }`, - error: "couldn't rewrite mutation updateEmployer because failed to rewrite mutation" + - " payload because id ABC already exists for field name inside type Employer", + error: "mutation updateEmployer failed because id ABC already exists for field name inside type Employer", }, { name: "updating root @id fields and also create a nested link to nested object", @@ -6623,3 +6622,690 @@ func xidUpdateAndNullableTests(t *testing.T) { DeleteGqlType(t, "Employer", filterEmployer, 2, nil) DeleteGqlType(t, "Worker", filterWorker, 4, nil) } + +func updateMutationWithUnchangedXID(t *testing.T) { + addParams := &GraphQLParams{ + Query: `mutation addState($xcode: String!, $name: String!) { + addState(input: [{ xcode: $xcode, name: $name }]) { + state { + id + xcode + name + } + } + }`, + Variables: map[string]interface{}{ + "xcode": "XIDTEST1", + "name": "XID Test State", + }, + } + gqlResponse := postExecutor(t, GraphqlURL, addParams) + RequireNoGQLErrors(t, gqlResponse) + + var addResult struct { + AddState struct { + State []*state + } + } + err := json.Unmarshal([]byte(gqlResponse.Data), &addResult) + require.NoError(t, err) + require.Len(t, addResult.AddState.State, 1) + newState := addResult.AddState.State[0] + require.NotEmpty(t, newState.ID) + + updateParams := &GraphQLParams{ + Query: `mutation updateState($xcode: String!, $name: String!) { + updateState(input: { + filter: { xcode: { eq: $xcode } }, + set: { xcode: $xcode, name: $name } + }) { + numUids + state { + id + xcode + name + } + } + }`, + Variables: map[string]interface{}{ + "xcode": "XIDTEST1", + "name": "XID Test State Updated", + }, + } + gqlResponse = postExecutor(t, GraphqlURL, updateParams) + RequireNoGQLErrors(t, gqlResponse) + + var updateResult struct { + UpdateState *struct { + NumUids int + State []*state + } + } + err = json.Unmarshal([]byte(gqlResponse.Data), &updateResult) + require.NoError(t, err) + + require.NotNil(t, updateResult.UpdateState, + "updateState returned null when set patch contained unchanged @id field") + require.Equal(t, 1, updateResult.UpdateState.NumUids) + require.Len(t, updateResult.UpdateState.State, 1) + require.Equal(t, "XIDTEST1", updateResult.UpdateState.State[0].Code) + require.Equal(t, "XID Test State Updated", updateResult.UpdateState.State[0].Name) + + newState.Name = "XID Test State Updated" + requireState(t, newState.ID, newState, postExecutor) + + deleteState(t, map[string]interface{}{ + "xcode": map[string]interface{}{"eq": "XIDTEST1"}, + }, 1, nil) +} + +func updateMutationWithNestedUnchangedXID(t *testing.T) { + addRegionParams := &GraphQLParams{ + Query: `mutation addRegion($id: String!, $name: String!) { + addRegion(input: [{ id: $id, name: $name }]) { + region { + id + name + } + } + }`, + Variables: map[string]interface{}{ + "id": "NESTED_R1", + "name": "Nested Test Region", + }, + } + gqlResponse := postExecutor(t, GraphqlURL, addRegionParams) + RequireNoGQLErrors(t, gqlResponse) + + addStateParams := &GraphQLParams{ + Query: `mutation addState($xcode: String!, $name: String!, $regionId: String!) { + addState(input: [{ + xcode: $xcode, + name: $name, + region: { id: $regionId } + }]) { + state { + id + xcode + name + region { id name } + } + } + }`, + Variables: map[string]interface{}{ + "xcode": "NESTED_S1", + "name": "Nested Test State", + "regionId": "NESTED_R1", + }, + } + gqlResponse = postExecutor(t, GraphqlURL, addStateParams) + RequireNoGQLErrors(t, gqlResponse) + + var addStateResult struct { + AddState struct { + State []*state + } + } + err := json.Unmarshal([]byte(gqlResponse.Data), &addStateResult) + require.NoError(t, err) + require.Len(t, addStateResult.AddState.State, 1) + stateID := addStateResult.AddState.State[0].ID + require.NotEmpty(t, stateID) + + updateParams := &GraphQLParams{ + Query: `mutation updateState($xcode: String!, $name: String!, $regionId: String!) { + updateState(input: { + filter: { xcode: { eq: $xcode } }, + set: { + xcode: $xcode, + name: $name, + region: { id: $regionId } + } + }) { + numUids + state { + id + xcode + name + region { id name } + } + } + }`, + Variables: map[string]interface{}{ + "xcode": "NESTED_S1", + "name": "Nested Test State Updated", + "regionId": "NESTED_R1", + }, + } + gqlResponse = postExecutor(t, GraphqlURL, updateParams) + RequireNoGQLErrors(t, gqlResponse) + + var updateResult struct { + UpdateState *struct { + NumUids int + State []*state + } + } + err = json.Unmarshal([]byte(gqlResponse.Data), &updateResult) + require.NoError(t, err) + + require.NotNil(t, updateResult.UpdateState, + "updateState returned null when set patch contained unchanged @id fields (top-level and nested)") + require.Equal(t, 1, updateResult.UpdateState.NumUids) + require.Len(t, updateResult.UpdateState.State, 1) + resultState := updateResult.UpdateState.State[0] + require.Equal(t, "NESTED_S1", resultState.Code) + require.Equal(t, "Nested Test State Updated", resultState.Name) + require.NotNil(t, resultState.Region) + require.Equal(t, "NESTED_R1", resultState.Region.ID) + + deleteState(t, map[string]interface{}{ + "xcode": map[string]interface{}{"eq": "NESTED_S1"}, + }, 1, nil) + DeleteGqlType(t, "Region", map[string]interface{}{ + "id": map[string]interface{}{"eq": "NESTED_R1"}, + }, 1, nil) +} + +func updateMutationWithNestedUnchangedXIDOnParent(t *testing.T) { + // Create a Comment1 that will be the nested child. + addCommentParams := &GraphQLParams{ + Query: `mutation { + addComment1(input: [{ id: "NESTED_CMT1" }]) { + comment1 { id } + } + }`, + } + gqlResponse := postExecutor(t, GraphqlURL, addCommentParams) + RequireNoGQLErrors(t, gqlResponse) + + // Create a Post1 that references the Comment1. + addPostParams := &GraphQLParams{ + Query: `mutation { + addPost1(input: [{ id: "NESTED_POST1", comments: [{ id: "NESTED_CMT1" }] }]) { + post1 { id comments { id } } + } + }`, + } + gqlResponse = postExecutor(t, GraphqlURL, addPostParams) + RequireNoGQLErrors(t, gqlResponse) + + updateParams := &GraphQLParams{ + Query: `mutation { + updatePost1(input: { + filter: { id: { eq: "NESTED_POST1" } }, + set: { id: "NESTED_POST1", comments: [{ id: "NESTED_CMT1" }] } + }) { + numUids + post1 { id comments { id } } + } + }`, + } + gqlResponse = postExecutor(t, GraphqlURL, updateParams) + RequireNoGQLErrors(t, gqlResponse) + + var updateResult struct { + UpdatePost1 *struct { + NumUids int + Post1 []struct { + ID string `json:"id"` + Comments []struct { + ID string `json:"id"` + } `json:"comments"` + } + } + } + err := json.Unmarshal([]byte(gqlResponse.Data), &updateResult) + require.NoError(t, err) + + require.NotNil(t, updateResult.UpdatePost1, + "updatePost1 returned null when set patch contained unchanged @id fields (top-level and nested)") + require.Equal(t, 1, updateResult.UpdatePost1.NumUids) + require.Len(t, updateResult.UpdatePost1.Post1, 1) + require.Equal(t, "NESTED_POST1", updateResult.UpdatePost1.Post1[0].ID) + require.Len(t, updateResult.UpdatePost1.Post1[0].Comments, 1) + require.Equal(t, "NESTED_CMT1", updateResult.UpdatePost1.Post1[0].Comments[0].ID) + + DeleteGqlType(t, "Post1", map[string]interface{}{ + "id": map[string]interface{}{"eq": "NESTED_POST1"}, + }, 1, nil) + DeleteGqlType(t, "Comment1", map[string]interface{}{ + "id": map[string]interface{}{"eq": "NESTED_CMT1"}, + }, 1, nil) +} + +func updateMutationXIDConflictStillErrors(t *testing.T) { + for _, id := range []string{"XIDTEST_P1", "XIDTEST_P2"} { + addParams := &GraphQLParams{ + Query: fmt.Sprintf(`mutation { addPost1(input: [{ id: "%s" }]) { post1 { id } } }`, id), + } + gqlResponse := postExecutor(t, GraphqlURL, addParams) + RequireNoGQLErrors(t, gqlResponse) + } + + updateParams := &GraphQLParams{ + Query: `mutation { + updatePost1(input: { + filter: { id: { eq: "XIDTEST_P2" } }, + set: { id: "XIDTEST_P1" } + }) { + numUids + post1 { id } + } + }`, + } + gqlResponse := postExecutor(t, GraphqlURL, updateParams) + require.NotEmpty(t, gqlResponse.Errors, + "expected conflict error when setting @id to a value already owned by a different node") + + DeleteGqlType(t, "Post1", map[string]interface{}{ + "id": map[string]interface{}{"in": []string{"XIDTEST_P1", "XIDTEST_P2"}}, + }, 2, nil) +} + +func updateMutationIntXIDUnchanged(t *testing.T) { + // Add a Chapter node with a unique Int @id value. + addParams := &GraphQLParams{ + Query: `mutation { addChapter(input: [{ chapterId: 9001, name: "Chapter B1" }]) { chapter { chapterId } } }`, + } + gqlResponse := postExecutor(t, GraphqlURL, addParams) + RequireNoGQLErrors(t, gqlResponse) + updateParams := &GraphQLParams{ + Query: `mutation { + updateChapter(input: { + filter: { chapterId: { eq: 9001 } }, + set: { chapterId: 9001, name: "Chapter B1 Updated" } + }) { numUids chapter { chapterId name } } + }`, + } + gqlResponse = postExecutor(t, GraphqlURL, updateParams) + RequireNoGQLErrors(t, gqlResponse) + + var result struct { + UpdateChapter *struct { + NumUids int + Chapter []struct { + ChapterId int `json:"chapterId"` + Name string `json:"name"` + } + } + } + err := json.Unmarshal([]byte(gqlResponse.Data), &result) + require.NoError(t, err) + require.NotNil(t, result.UpdateChapter, + "updateChapter must not be null when Int @id is unchanged in set patch") + require.Equal(t, 1, result.UpdateChapter.NumUids) + require.Equal(t, 9001, result.UpdateChapter.Chapter[0].ChapterId) + require.Equal(t, "Chapter B1 Updated", result.UpdateChapter.Chapter[0].Name) + + DeleteGqlType(t, "Chapter", map[string]interface{}{ + "chapterId": map[string]interface{}{"eq": 9001}, + }, 1, nil) +} + +func updateMutationIntXIDConflictErrors(t *testing.T) { + for _, id := range []int{9002, 9003} { + addParams := &GraphQLParams{ + Query: fmt.Sprintf(`mutation { addChapter(input: [{ chapterId: %d, name: "Chapter %d" }]) { chapter { chapterId } } }`, id, id), + } + gqlResponse := postExecutor(t, GraphqlURL, addParams) + RequireNoGQLErrors(t, gqlResponse) + } + + updateParams := &GraphQLParams{ + Query: `mutation { + updateChapter(input: { + filter: { chapterId: { eq: 9003 } }, + set: { chapterId: 9002 } + }) { numUids chapter { chapterId } } + }`, + } + gqlResponse := postExecutor(t, GraphqlURL, updateParams) + require.NotEmpty(t, gqlResponse.Errors, + "must return a conflict error when Int @id in set patch belongs to a different node") + + DeleteGqlType(t, "Chapter", map[string]interface{}{ + "chapterId": map[string]interface{}{"in": []int{9002, 9003}}, + }, 2, nil) +} + +func updateMutationInt64XIDUnchanged(t *testing.T) { + // Add a Book node with a large Int64 @id value. + addParams := &GraphQLParams{ + Query: `mutation($id: Int64!) { + addBook(input: [{ bookId: $id, name: "Book B3" }]) { book { bookId } } + }`, + Variables: map[string]interface{}{"id": "9876543219001"}, + } + gqlResponse := postExecutor(t, GraphqlURL, addParams) + RequireNoGQLErrors(t, gqlResponse) + + updateParams := &GraphQLParams{ + Query: `mutation($id: Int64!) { + updateBook(input: { + filter: { bookId: { eq: $id } }, + set: { bookId: $id, name: "Book B3 Updated" } + }) { numUids book { bookId name } } + }`, + Variables: map[string]interface{}{"id": "9876543219001"}, + } + gqlResponse = postExecutor(t, GraphqlURL, updateParams) + RequireNoGQLErrors(t, gqlResponse) + + var result struct { + UpdateBook *struct { + NumUids int + Book []struct { + BookId int64 `json:"bookId"` + Name string `json:"name"` + } + } + } + err := json.Unmarshal([]byte(gqlResponse.Data), &result) + require.NoError(t, err) + require.NotNil(t, result.UpdateBook, + "updateBook must not be null when Int64 @id is unchanged in set patch") + require.Equal(t, 1, result.UpdateBook.NumUids) + require.Equal(t, "Book B3 Updated", result.UpdateBook.Book[0].Name) + + DeleteGqlType(t, "Book", map[string]interface{}{ + "bookId": map[string]interface{}{"eq": "9876543219001"}, + }, 1, nil) +} + +func updateMutationNullableXIDUnchanged(t *testing.T) { + addParams := &GraphQLParams{ + Query: `mutation { + addWorker(input: [{ empId: "WRK-C1", name: "Worker C1", regNo: 8001 }]) { + worker { empId } + } + }`, + } + gqlResponse := postExecutor(t, GraphqlURL, addParams) + RequireNoGQLErrors(t, gqlResponse) + + updateParams := &GraphQLParams{ + Query: `mutation { + updateWorker(input: { + filter: { empId: { eq: "WRK-C1" } }, + set: { regNo: 8001, name: "Worker C1 Updated" } + }) { numUids worker { empId name } } + }`, + } + gqlResponse = postExecutor(t, GraphqlURL, updateParams) + RequireNoGQLErrors(t, gqlResponse) + + var result struct { + UpdateWorker *struct { + NumUids int + Worker []struct { + EmpId string `json:"empId"` + Name string `json:"name"` + } + } + } + err := json.Unmarshal([]byte(gqlResponse.Data), &result) + require.NoError(t, err) + require.NotNil(t, result.UpdateWorker, + "updateWorker must not be null when nullable Int @id is unchanged in set patch") + require.Equal(t, 1, result.UpdateWorker.NumUids) + require.Equal(t, "Worker C1 Updated", result.UpdateWorker.Worker[0].Name) + + DeleteGqlType(t, "Worker", map[string]interface{}{ + "empId": map[string]interface{}{"eq": "WRK-C1"}, + }, 1, nil) +} + +func updateMutationNullableXIDFirstTimeSet(t *testing.T) { + addParams := &GraphQLParams{ + Query: `mutation { + addWorker(input: [{ empId: "WRK-C2", name: "Worker C2" }]) { + worker { empId } + } + }`, + } + gqlResponse := postExecutor(t, GraphqlURL, addParams) + RequireNoGQLErrors(t, gqlResponse) + + updateParams := &GraphQLParams{ + Query: `mutation { + updateWorker(input: { + filter: { empId: { eq: "WRK-C2" } }, + set: { regNo: 9202, name: "Worker C2 Updated" } + }) { numUids worker { empId regNo name } } + }`, + } + gqlResponse = postExecutor(t, GraphqlURL, updateParams) + RequireNoGQLErrors(t, gqlResponse) + + var result struct { + UpdateWorker *struct { + NumUids int + Worker []struct { + EmpId string `json:"empId"` + RegNo int `json:"regNo"` + Name string `json:"name"` + } + } + } + err := json.Unmarshal([]byte(gqlResponse.Data), &result) + require.NoError(t, err) + require.NotNil(t, result.UpdateWorker) + require.Equal(t, 1, result.UpdateWorker.NumUids) + require.Equal(t, 9202, result.UpdateWorker.Worker[0].RegNo) + + DeleteGqlType(t, "Worker", map[string]interface{}{ + "empId": map[string]interface{}{"eq": "WRK-C2"}, + }, 1, nil) +} + +func updateMutationNullableXIDClearedViaRemove(t *testing.T) { + // Add a Worker with regNo set. + addParams := &GraphQLParams{ + Query: `mutation { + addWorker(input: [{ empId: "WRK-C3", name: "Worker C3", regNo: 8003 }]) { + worker { empId } + } + }`, + } + gqlResponse := postExecutor(t, GraphqlURL, addParams) + RequireNoGQLErrors(t, gqlResponse) + + updateParams := &GraphQLParams{ + Query: `mutation { + updateWorker(input: { + filter: { empId: { eq: "WRK-C3" } }, + remove: { regNo: 8003 } + }) { numUids worker { empId name } } + }`, + } + gqlResponse = postExecutor(t, GraphqlURL, updateParams) + RequireNoGQLErrors(t, gqlResponse) + + var result struct { + UpdateWorker *struct { + NumUids int + Worker []struct { + EmpId string `json:"empId"` + Name string `json:"name"` + } + } + } + err := json.Unmarshal([]byte(gqlResponse.Data), &result) + require.NoError(t, err) + require.NotNil(t, result.UpdateWorker) + require.Equal(t, 1, result.UpdateWorker.NumUids) + + DeleteGqlType(t, "Worker", map[string]interface{}{ + "empId": map[string]interface{}{"eq": "WRK-C3"}, + }, 1, nil) +} + +func updateMutationRemovePatchNonXIDField(t *testing.T) { + addParams := &GraphQLParams{ + Query: `mutation { + addWorker(input: [{ empId: "WRK-C3B", name: "Worker C3B", regNo: 8004 }]) { + worker { empId } + } + }`, + } + gqlResponse := postExecutor(t, GraphqlURL, addParams) + RequireNoGQLErrors(t, gqlResponse) + + updateParams := &GraphQLParams{ + Query: `mutation { + updateWorker(input: { + filter: { empId: { eq: "WRK-C3B" } }, + remove: { name: "Worker C3B" } + }) { numUids worker { empId } } + }`, + } + gqlResponse = postExecutor(t, GraphqlURL, updateParams) + RequireNoGQLErrors(t, gqlResponse) + + var result struct { + UpdateWorker *struct { + NumUids int + Worker []struct { + EmpId string `json:"empId"` + } + } + } + err := json.Unmarshal([]byte(gqlResponse.Data), &result) + require.NoError(t, err) + require.NotNil(t, result.UpdateWorker) + require.Equal(t, 1, result.UpdateWorker.NumUids) + + DeleteGqlType(t, "Worker", map[string]interface{}{ + "empId": map[string]interface{}{"eq": "WRK-C3B"}, + }, 1, nil) +} + +func updateMutationDeleteRecreateXID(t *testing.T) { + addParams := &GraphQLParams{ + Query: `mutation { addPost1(input: [{ id: "XIDRECYCLE-P1" }]) { post1 { id } } }`, + } + gqlResponse := postExecutor(t, GraphqlURL, addParams) + RequireNoGQLErrors(t, gqlResponse) + + DeleteGqlType(t, "Post1", map[string]interface{}{ + "id": map[string]interface{}{"eq": "XIDRECYCLE-P1"}, + }, 1, nil) + + addParams = &GraphQLParams{ + Query: `mutation { addPost1(input: [{ id: "XIDRECYCLE-P1" }]) { post1 { id } } }`, + } + gqlResponse = postExecutor(t, GraphqlURL, addParams) + RequireNoGQLErrors(t, gqlResponse) + + var result struct { + AddPost1 *struct { + Post1 []struct { + ID string `json:"id"` + } + } + } + err := json.Unmarshal([]byte(gqlResponse.Data), &result) + require.NoError(t, err) + require.NotNil(t, result.AddPost1) + require.Len(t, result.AddPost1.Post1, 1) + require.Equal(t, "XIDRECYCLE-P1", result.AddPost1.Post1[0].ID) + + DeleteGqlType(t, "Post1", map[string]interface{}{ + "id": map[string]interface{}{"eq": "XIDRECYCLE-P1"}, + }, 1, nil) +} + +func updateMutationConcurrentUnchangedXID(t *testing.T) { + addParams := &GraphQLParams{ + Query: `mutation { addPost1(input: [{ id: "XIDH1-CONC" }]) { post1 { id } } }`, + } + gqlResponse := postExecutor(t, GraphqlURL, addParams) + RequireNoGQLErrors(t, gqlResponse) + + const n = 10 + responses := make([]*GraphQLResponse, n) + var wg sync.WaitGroup + for i := 0; i < n; i++ { + wg.Add(1) + go func(idx int) { + defer wg.Done() + // unchanged id in set patch, run concurrently from n goroutines. + // Each goroutine independently resolves existenceUID == mutatedUID → no conflict. + p := &GraphQLParams{ + Query: `mutation { + updatePost1(input: { + filter: { id: { eq: "XIDH1-CONC" } }, + set: { id: "XIDH1-CONC" } + }) { numUids post1 { id } } + }`, + } + responses[idx] = p.ExecuteAsPost(t, GraphqlURL) + }(i) + } + wg.Wait() + + for i, r := range responses { + for _, gqlErr := range r.Errors { + // Transaction aborts are a normal Dgraph MVCC outcome under concurrent writes + // and are unrelated to @id conflict detection. Only the "already exists" error + // would indicate our fix regressed. + require.NotContains(t, gqlErr.Error(), "already exists for field", + "goroutine %d: @id conflict error must not occur for unchanged @id", i) + } + } + + DeleteGqlType(t, "Post1", map[string]interface{}{ + "id": map[string]interface{}{"eq": "XIDH1-CONC"}, + }, 1, nil) +} + +func updateMutationConcurrentXIDTheftAllError(t *testing.T) { + const n = 5 + ownerParams := &GraphQLParams{ + Query: `mutation { addPost1(input: [{ id: "XIDH2-OWNED" }]) { post1 { id } } }`, + } + gqlResponse := postExecutor(t, GraphqlURL, ownerParams) + RequireNoGQLErrors(t, gqlResponse) + + allIDs := []string{"XIDH2-OWNED"} + for i := 0; i < n; i++ { + id := fmt.Sprintf("XIDH2-ATK%d", i) + allIDs = append(allIDs, id) + addParams := &GraphQLParams{ + Query: fmt.Sprintf(`mutation { addPost1(input: [{ id: "%s" }]) { post1 { id } } }`, id), + } + gqlResponse = postExecutor(t, GraphqlURL, addParams) + RequireNoGQLErrors(t, gqlResponse) + } + + responses := make([]*GraphQLResponse, n) + var wg sync.WaitGroup + for i := 0; i < n; i++ { + wg.Add(1) + go func(idx int) { + defer wg.Done() + atkID := fmt.Sprintf("XIDH2-ATK%d", idx) + + p := &GraphQLParams{ + Query: fmt.Sprintf(`mutation { + updatePost1(input: { + filter: { id: { eq: "%s" } }, + set: { id: "XIDH2-OWNED" } + }) { numUids post1 { id } } + }`, atkID), + } + responses[idx] = p.ExecuteAsPost(t, GraphqlURL) + }(i) + } + wg.Wait() + + for i, r := range responses { + require.NotEmpty(t, r.Errors, + "goroutine %d: expected conflict error when stealing @id owned by a different node", i) + } + + DeleteGqlType(t, "Post1", map[string]interface{}{ + "id": map[string]interface{}{"in": allIDs}, + }, len(allIDs), nil) +} diff --git a/graphql/resolve/mutation.go b/graphql/resolve/mutation.go index 5a5093779ae..28664a12be9 100644 --- a/graphql/resolve/mutation.go +++ b/graphql/resolve/mutation.go @@ -114,6 +114,16 @@ type MutationRewriter interface { result map[string]interface{}) []string } +// XIDConflictVerifier is an optional interface implemented by MutationRewriter when it needs +// to verify deferred @id conflicts post-execution. UpdateRewriter implements this to handle +// the case where an @id field in an update set-patch holds its current (unchanged) value. +type XIDConflictVerifier interface { + // VerifyXIDConflicts checks each deferred @id conflict against the UIDs that were actually + // mutated. If a conflict's existenceUID is not among the mutated UIDs, it belongs to a + // different node and is a genuine conflict — an error is returned. + VerifyXIDConflicts(mutatedUIDs []string, hasAuthRules bool) error +} + // A DgraphExecutor can execute a query/mutation and returns the request response and any errors. type DgraphExecutor interface { // Execute performs the actual query/mutation and returns a Dgraph response. If an error @@ -474,6 +484,23 @@ func (mr *dgraphResolver) rewriteAndExecute( } mutationTimer.Stop() + // Verify any @id conflicts that were deferred during rewriting (UpdateWithSet only). + // Mirrors the DQL @unique verifyUnique pattern: compare the existence-query UID against + // the UIDs that were actually mutated. A mismatch means the @id value belongs to a + // different node — a genuine conflict. + // + // ORDERING INVARIANT: this check must run before CommitOrAbort. If a conflict is + // detected the function returns early with commit=false, triggering the deferred abort. + // Moving CommitOrAbort above this check would commit the conflicting write permanently. + if verifier, ok := mr.mutationRewriter.(XIDConflictVerifier); ok { + mutatedUIDs := mr.mutationRewriter.MutatedRootUIDs(mutation, mutResp.GetUids(), result) + hasAuthRules := queryAuthSelector(mutation.MutatedType()) != nil + if err := verifier.VerifyXIDConflicts(mutatedUIDs, hasAuthRules); err != nil { + return emptyResult(schema.GQLWrapf(err, "mutation %s failed", mutation.Name())), + resolverFailed + } + } + authErr := authorizeNewNodes(ctx, mutation, mutResp.Uids, newNodes, mr.executor, mutResp.Txn) if authErr != nil { return emptyResult(schema.GQLWrapf(authErr, "mutation failed")), resolverFailed diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index d9dd2d39c66..d6add748936 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -62,6 +62,52 @@ type UpdateRewriter struct { delFrag *mutationFragment Rewriter } + +// XIDConflicts returns any @id conflicts that were deferred during rewriting +// and must be verified post-execution. +func (urw *UpdateRewriter) XIDConflicts() []xidConflict { + if urw.XidMetadata == nil { + return nil + } + return urw.XidMetadata.xidConflicts +} + +// VerifyXIDConflicts implements XIDConflictVerifier. For each deferred @id conflict, +// it checks whether the existenceUID is among the mutated UIDs. If not, the @id value +// belongs to a different node — a genuine conflict — and an error is returned. +// +// If mutatedUIDs is empty the filter matched nothing and no mutation ran, so there is +// nothing to verify — all recorded conflicts are vacuous and we return nil. +func (urw *UpdateRewriter) VerifyXIDConflicts(mutatedUIDs []string, hasAuthRules bool) error { + if len(mutatedUIDs) == 0 { + return nil + } + + mutatedSet := make(map[string]struct{}, len(mutatedUIDs)) + for _, uid := range mutatedUIDs { + mutatedSet[uid] = struct{}{} + } + + var conflictErrors x.GqlErrorList + for _, conflict := range urw.XIDConflicts() { + if _, isSameNode := mutatedSet[conflict.existenceUID]; !isSameNode { + if !hasAuthRules { + conflictErrors = append(conflictErrors, x.GqlErrorf( + "id %s already exists for field %s inside type %s", + conflict.xidValue, conflict.xidField, conflict.typeName)) + } else { + conflictErrors = append(conflictErrors, x.GqlErrorf( + "GraphQL debug: id %s already exists for field %s inside type %s", + conflict.xidValue, conflict.xidField, conflict.typeName)) + } + } + } + if len(conflictErrors) > 0 { + return conflictErrors + } + return nil +} + type deleteRewriter struct { Rewriter } @@ -94,6 +140,22 @@ type xidMetadata struct { seenAtTopLevel map[string]bool // seenUIDs tells whether the UID is previously been seen during DFS traversal seenUIDs map[string]bool + // xidConflicts accumulates @id conflicts deferred for post-execution verification. + // UpdateWithSet: appended when an @id value already exists — we cannot tell at rewrite + // time whether it belongs to the node being updated (same node = OK) or a different node + // (genuine conflict). Verified after the upsert by VerifyXIDConflicts. + // UpdateWithRemove: never appended — removing an @id value cannot steal uniqueness, so + // no conflict check is needed and the field is skipped entirely at rewrite time. + xidConflicts []xidConflict +} + +// xidConflict records an @id existence check that could not be resolved at rewrite time. +// It is resolved post-execution by comparing existenceUID against the mutated node UIDs. +type xidConflict struct { + existenceUID string // UID of the node that currently holds this @id value + xidField string // @id field name (for error messages) + xidValue string // the @id value being set + typeName string // type name (for error messages) } // A mutationBuilder can build a json mutation []byte from a mutationFragment @@ -346,21 +408,36 @@ func (urw *UpdateRewriter) RewriteQueries( } } - // Write existence queries for remove + // Write existence queries for remove. + // Top-level @id (XID) fields in a remove patch are scalar values being cleared from the + // node — not references used to locate other nodes. No existence query is needed for them: + // removing a value that doesn't match the stored value is a no-op in Dgraph, and + // rewriteObject skips @id fields in UpdateWithRemove at top level entirely. + // Nested @id fields (e.g. remove: { author: { email: "x" } }) still need existence queries + // because they identify which node to unlink, so we only strip the top-level ones. if delArg != nil { obj := delArg.(map[string]interface{}) if len(obj) != 0 { - queries, typs, errs := existenceQueries(ctx, mutatedType, nil, urw.VarGen, obj, urw.XidMetadata) - if len(errs) > 0 { - var gqlErrors x.GqlErrorList - for _, err := range errs { - gqlErrors = append(gqlErrors, schema.AsGQLErrors(err)...) + delObjForExistence := make(map[string]interface{}, len(obj)) + for k, v := range obj { + delObjForExistence[k] = v + } + for _, xid := range mutatedType.XIDFields() { + delete(delObjForExistence, xid.Name()) + } + if len(delObjForExistence) != 0 { + queries, typs, errs := existenceQueries(ctx, mutatedType, nil, urw.VarGen, delObjForExistence, urw.XidMetadata) + if len(errs) > 0 { + var gqlErrors x.GqlErrorList + for _, err := range errs { + gqlErrors = append(gqlErrors, schema.AsGQLErrors(err)...) + } + retErrors = schema.AppendGQLErrs(retErrors, schema.GQLWrapf(gqlErrors, + "failed to rewrite mutation payload")) } - retErrors = schema.AppendGQLErrs(retErrors, schema.GQLWrapf(gqlErrors, - "failed to rewrite mutation payload")) + ret = append(ret, queries...) + retTypes = append(retTypes, typs...) } - ret = append(ret, queries...) - retTypes = append(retTypes, typs...) } } return ret, retTypes, retErrors @@ -1407,6 +1484,14 @@ func rewriteObject( for _, xid := range xids { var xidString string if xidVal, ok := obj[xid.Name()]; ok && xidVal != nil { + // For UpdateWithRemove at top level, @id values are scalar values being cleared + // from the node. Whether or not the value currently exists in the database is + // irrelevant — a remove can never steal uniqueness from another node. + // Skip regardless of what the existence query returned (or didn't return). + if mutationType == UpdateWithRemove && atTopLevel { + continue + } + xidString, _ = extractVal(xidVal, xid.Name(), xid.Type().Name()) variable = varGen.Next(typ, xid.Name(), xidString, false) existenceError := x.GqlErrorf("multiple nodes found for given xid values," + @@ -1463,9 +1548,23 @@ func rewriteObject( return nil, "", retErrors } } else { - // We return an error as we are at top level of non-upsert mutation and the XID exists. - // We need to conceal the error because we might be leaking information to the user if it - // tries to add duplicate data to the field with @id. + // For UpdateWithSet mutations, an @id value that already exists in the + // database may belong to the same node being updated (unchanged value). + // We cannot confirm this at rewrite time because the filter target UID is + // only resolved when the upsert executes. Defer the conflict check to + // post-execution via xidMetadata.xidConflicts and let the mutation proceed. + if mutationType == UpdateWithSet && typUidExist { + xidMetadata.xidConflicts = append(xidMetadata.xidConflicts, xidConflict{ + existenceUID: typUid, + xidField: xid.Name(), + xidValue: xidString, + typeName: typ.Name(), + }) + continue + } + + // For Add mutations, treat an existing @id as a conflict. + // Conceal the error if the type has auth rules to avoid leaking information. var err error if typUidExist { if queryAuthSelector(typ) == nil { diff --git a/graphql/resolve/mutation_test.go b/graphql/resolve/mutation_test.go index bd027313785..53781552ff2 100644 --- a/graphql/resolve/mutation_test.go +++ b/graphql/resolve/mutation_test.go @@ -33,18 +33,20 @@ import ( // ensure that those errors get caught before they reach rewriting. type testCase struct { - Name string - GQLMutation string - GQLVariables string - Explanation string - DGMutations []*dgraphMutation - DGMutationsSec []*dgraphMutation - DGQuery string - DGQuerySec string - Error *x.GqlError - Error2 *x.GqlError - ValidationError *x.GqlError - QNameToUID string + Name string + GQLMutation string + GQLVariables string + Explanation string + DGMutations []*dgraphMutation + DGMutationsSec []*dgraphMutation + DGQuery string + DGQuerySec string + Error *x.GqlError + Error2 *x.GqlError + ValidationError *x.GqlError + QNameToUID string + XIDConflictUIDs string // JSON array of mock mutated UIDs for VerifyXIDConflicts + XIDConflictError *x.GqlError // expected error from VerifyXIDConflicts } type dgraphMutation struct { @@ -300,6 +302,22 @@ func mutationRewriting(t *testing.T, file string, rewriterFactory func() Mutatio // Compare the query generated along with mutations. dgQuerySec := dgraph.AsString(upsert[0].Query) require.Equal(t, tcase.DGQuerySec, dgQuerySec) + + // Verify deferred @id conflicts via VerifyXIDConflicts if the test case + // specifies mock mutated UIDs. This simulates the post-execution check in + // mutation.go without needing a real cluster. + if tcase.XIDConflictUIDs != "" { + var conflictUIDs []string + require.NoError(t, json.Unmarshal([]byte(tcase.XIDConflictUIDs), &conflictUIDs)) + if verifier, ok := rewriterToTest.(XIDConflictVerifier); ok { + conflictErr := verifier.VerifyXIDConflicts(conflictUIDs, false) + if tcase.XIDConflictError != nil || conflictErr != nil { + require.NotNil(t, conflictErr) + require.NotNil(t, tcase.XIDConflictError) + require.Equal(t, tcase.XIDConflictError.Error(), conflictErr.Error()) + } + } + } }) } } diff --git a/graphql/resolve/update_mutation_test.yaml b/graphql/resolve/update_mutation_test.yaml index ef5155ebc94..2daf6f853e8 100644 --- a/graphql/resolve/update_mutation_test.yaml +++ b/graphql/resolve/update_mutation_test.yaml @@ -2017,8 +2017,9 @@ - name: Updating @id field when given value for @id fields exist in some node explaination: We are giving two @id fields title and ISBN in set part of update mutation, and will generate - two existence queries for both of them.As we already have node with title Sapiens, we will - return error in this case + two existence queries for both of them. "History of Humans" already exists at node 0x123 (a + different node than the one being updated). The rewriter defers the conflict check — the + mutation proceeds, and VerifyXIDConflicts catches the cross-node conflict post-execution. gqlmutation: | mutation update($patch: UpdateBookInput!) { updateBook(input: $patch) { @@ -2068,12 +2069,23 @@ { "Book_2": "0x123" } - error2: - { - "message": - failed to rewrite mutation payload because id History of Humans already exists for field - title inside type Book, + dgquerysec: |- + query { + x as updateBook(func: type(Book)) @filter((eq(Book.title, "Sapiens") OR eq(Book.ISBN, "2QSAT"))) { + uid + } } + dgmutations: + - setjson: | + { "uid" : "uid(x)", + "Book.ISBN": "I001", + "Book.publisher": "penguin", + "Book.title": "History of Humans" + } + cond: "@if(gt(len(x), 0))" + xidconflictuids: '["0x456"]' + xidconflicterror: + { "message": id History of Humans already exists for field title inside type Book } - name: skipping nullable @id values while Updating link to non-existent nested object explaination: