diff --git a/graphql/e2e/common/common.go b/graphql/e2e/common/common.go index 40aeea213be..d584b68fb1a 100644 --- a/graphql/e2e/common/common.go +++ b/graphql/e2e/common/common.go @@ -900,6 +900,8 @@ func RunAll(t *testing.T) { t.Run("Update language tag fields", updateLangTagFields) t.Run("mutation with @id field and interface arg", mutationWithIDFieldHavingInterfaceArg) t.Run("xid update and nullable tests", xidUpdateAndNullableTests) + t.Run("Referencing same node containing multiple XIDs", + referencingSameNodeWithMultipleXIds) // error tests t.Run("graphql completion on", graphQLCompletionOn) diff --git a/graphql/e2e/common/mutation.go b/graphql/e2e/common/mutation.go index 8c34acc15f4..aef1166c935 100644 --- a/graphql/e2e/common/mutation.go +++ b/graphql/e2e/common/mutation.go @@ -6620,3 +6620,77 @@ func xidUpdateAndNullableTests(t *testing.T) { DeleteGqlType(t, "Employer", filterEmployer, 2, nil) DeleteGqlType(t, "Worker", filterWorker, 4, nil) } + +func referencingSameNodeWithMultipleXIds(t *testing.T) { + params := &GraphQLParams{ + Query: `mutation($input: [AddPerson1Input!]!) { + addPerson1(input: $input) { + person1 { + regId + name + friends { + regId + name + } + closeFriends { + regId + name + } + } + } + }`, + Variables: map[string]interface{}{ + "input": []interface{}{ + map[string]interface{}{ + "regId": "7", + "name": "7th Person", + "name1": "seventh Person", + "friends": []interface{}{ + map[string]interface{}{ + "regId": "8", + "name": "8th Person", + "name1": "eighth Person", + }, + }, + "closeFriends": []interface{}{ + map[string]interface{}{ + "regId": "8", + "name": "8th Person", + }, + }, + }, + }, + }, + } + + gqlResponse := postExecutor(t, GraphqlURL, params) + RequireNoGQLErrors(t, gqlResponse) + + expected := `{ + "addPerson1": + { + "person1": [ + { + "closeFriends": [ + { + "name": "8th Person", + "regId": "8" + } + ], + "friends": [ + { + "name": "8th Person", + "regId": "8" + } + ], + "name": "7th Person", + "regId": "7" + } + ] + } + }` + testutil.CompareJSON(t, expected, string(gqlResponse.Data)) + + // cleanup + DeleteGqlType(t, "Person1", map[string]interface{}{}, 2, nil) +} diff --git a/graphql/e2e/directives/schema.graphql b/graphql/e2e/directives/schema.graphql index f7e6718608b..9d1fa987a43 100644 --- a/graphql/e2e/directives/schema.graphql +++ b/graphql/e2e/directives/schema.graphql @@ -206,7 +206,9 @@ type post1{ type Person1 { id: ID! - name: String! + name: String! @id + name1: String @id + regId: String @id closeFriends: [Person1] @hasInverse(field: closeFriends) friends: [Person1] @hasInverse(field: friends) } diff --git a/graphql/e2e/directives/schema_response.json b/graphql/e2e/directives/schema_response.json index b41cb67668b..de786b23d43 100644 --- a/graphql/e2e/directives/schema_response.json +++ b/graphql/e2e/directives/schema_response.json @@ -469,7 +469,30 @@ }, { "predicate": "Person1.name", - "type": "string" + "type": "string", + "upsert": true, + "tokenizer": [ + "hash" + ], + "index": true + }, + { + "predicate": "Person1.regId", + "type": "string", + "upsert": true, + "tokenizer": [ + "hash" + ], + "index": true + }, + { + "predicate": "Person1.name1", + "type": "string", + "upsert": true, + "tokenizer": [ + "hash" + ], + "index": true }, { "predicate": "Plant.breed", @@ -1209,6 +1232,12 @@ }, { "name": "Person1.closeFriends" + }, + { + "name": "Person1.name1" + }, + { + "name": "Person1.regId" } ], "name": "Person1" diff --git a/graphql/e2e/normal/schema.graphql b/graphql/e2e/normal/schema.graphql index 396dea4318b..36a486771a3 100644 --- a/graphql/e2e/normal/schema.graphql +++ b/graphql/e2e/normal/schema.graphql @@ -219,7 +219,9 @@ type post1{ type Person1 { id: ID! - name: String! + name: String! @id + name1: String @id + regId: String @id closeFriends: [Person1] @hasInverse(field: closeFriends) friends: [Person1] @hasInverse(field: friends) } diff --git a/graphql/e2e/normal/schema_response.json b/graphql/e2e/normal/schema_response.json index f409ecdedef..868483caff7 100644 --- a/graphql/e2e/normal/schema_response.json +++ b/graphql/e2e/normal/schema_response.json @@ -523,7 +523,30 @@ }, { "predicate": "Person1.name", - "type": "string" + "index": true, + "type": "string", + "tokenizer": [ + "hash" + ], + "upsert": true + }, + { + "predicate": "Person1.name1", + "index": true, + "type": "string", + "tokenizer": [ + "hash" + ], + "upsert": true + }, + { + "predicate": "Person1.regId", + "index": true, + "type": "string", + "tokenizer": [ + "hash" + ], + "upsert": true }, { "predicate": "Plant.breed", @@ -1237,6 +1260,12 @@ { "name": "Person1.name" }, + { + "name": "Person1.name1" + }, + { + "name": "Person1.regId" + }, { "name": "Person1.friends" }, diff --git a/graphql/resolve/add_mutation_test.yaml b/graphql/resolve/add_mutation_test.yaml index 42135b978b4..a647f87cd6e 100644 --- a/graphql/resolve/add_mutation_test.yaml +++ b/graphql/resolve/add_mutation_test.yaml @@ -661,8 +661,10 @@ } explanation: "A reference must be a valid UID" error: - { "message": - "failed to rewrite mutation payload because ID argument (HI!) was not able to be parsed" } + { + "message": + "failed to rewrite mutation payload because ID argument (HI!) was not able to be parsed" + } - name: "Add mutation with inverse reference" @@ -2741,43 +2743,6 @@ message: |- failed to rewrite mutation payload because duplicate XID found: S1 -- name: "Duplicate XIDs in single mutation for Interface" - gqlmutation: | - mutation addStudent($input: [AddStudentInput!]!) { - addStudent(input: $input) { - student { - xid - name - taughtBy { - xid - name - subject - } - } - } - } - gqlvariables: | - { - "input": [ - { - "xid": "S1", - "name": "Stud1" - }, - { - "xid": "S2", - "name": "Stud2", - "taughtBy": [ - {"xid": "S1", "name": "Teacher1", "subject": "Sub1"} - ] - } - ] - } - explanation: "When duplicate XIDs are given as input for an Interface in a single mutation, it - should return error." - error: - message: |- - failed to rewrite mutation payload because duplicate XID found: S1 - # Additional Deletes # # If we have @@ -5500,3 +5465,42 @@ { "message": "failed to rewrite mutation payload because field id cannot be empty" } + +- + name: "Add Mutation referencing same XID in different types" + gqlmutation: | + mutation($input: [AddT1Input!]!) { + addT1(input: $input) { + t1 { + name + name1 + name2 + link { + name + name1 + name3 + } + } + } + } + gqlvariables: | + { + "input": [ + { + "name": "Bob", + "name1": "Bob11", + "name2": "Bob2", + "link": { + "name": "Bob" + } + } + ] + } + explanation: "As the link and top level object contain the same XID, Bob, this should throw an error" + error: + { + "message": + "failed to rewrite mutation payload because using duplicate XID value: Bob for XID: name for two different + implementing types of same interfaces: T1 and T2" + } + diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index 2a33a892627..f5ecce2a0e9 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -107,6 +107,9 @@ type xidMetadata struct { seenAtTopLevel map[string]bool // seenUIDs tells whether the UID is previously been seen during DFS traversal seenUIDs map[string]bool + // interfaceVariableToTypes stores a map from interface variable to type to which the node + // belongs + interfaceVariableToTypes map[string]string } // A mutationBuilder can build a json mutation []byte from a mutationFragment @@ -136,12 +139,16 @@ func (v *VariableGenerator) Next(typ schema.Type, xidName, xidVal string, auth b // return previously allocated variable name for repeating xidVal var key string flagAndXidName := xidName + // isInterfaceVariable is true if Next function is being used to generate variable for an + // interface. This is used for XIDs which are part of interface with interface=true flag. + isIntefaceVariable := false // We pass the xidName as "Int.xidName" to generate variable for existence query // of interface type when id filed is inherited from interface and have interface Argument set // Here we handle that case if strings.Contains(flagAndXidName, ".") { xidName = strings.Split(flagAndXidName, ".")[1] + isIntefaceVariable = true } if xidName == "" || xidVal == "" { @@ -163,6 +170,11 @@ func (v *VariableGenerator) Next(typ schema.Type, xidName, xidVal string, auth b // here we are using the assertion that field name or type name can't have "." in them xidType, _ := typ.FieldOriginatedFrom(xidName) key = xidType.Name() + "." + flagAndXidName + "." + xidVal + if !isIntefaceVariable { + // This is done to ensure that two implementing types get a different variable + // assigned in case they are not inheriting the same XID with interface=true flag. + key = key + typ.Name() + } } if varName, ok := v.xidVarNameMap[key]; ok { @@ -204,9 +216,10 @@ func NewDeleteRewriter() MutationRewriter { // NewXidMetadata returns a new empty *xidMetadata for storing the metadata. func NewXidMetadata() *xidMetadata { return &xidMetadata{ - variableObjMap: make(map[string]map[string]interface{}), - seenAtTopLevel: make(map[string]bool), - seenUIDs: make(map[string]bool), + variableObjMap: make(map[string]map[string]interface{}), + seenAtTopLevel: make(map[string]bool), + seenUIDs: make(map[string]bool), + interfaceVariableToTypes: make(map[string]string), } } @@ -215,10 +228,10 @@ func NewXidMetadata() *xidMetadata { // 2. we are in a deep mutation and: // a. this newXidObj has a field which is inverse of srcField and that // invField is not of List type, OR -// b. newXidObj has some values other than xid and isn't equal to existingXidObject +// b. newXidObj has some values other than xids and isn't equal to existingXidObject // It is used in places where we don't want to allow duplicates. func (xidMetadata *xidMetadata) isDuplicateXid(atTopLevel bool, xidVar string, - newXidObj map[string]interface{}, srcField schema.FieldDefinition) bool { + newXidObj map[string]interface{}, srcField schema.FieldDefinition, isXID map[string]bool) bool { if atTopLevel && xidMetadata.seenAtTopLevel[xidVar] { return true } @@ -230,15 +243,43 @@ func (xidMetadata *xidMetadata) isDuplicateXid(atTopLevel bool, xidVar string, } } - // We return an error if both occurrences of xid contain more than one values - // and are not equal. - // XID should be defined with all its values at one of the places and references with its - // XID from other places. - if len(newXidObj) > 1 && len(xidMetadata.variableObjMap[xidVar]) > 1 && - !reflect.DeepEqual(xidMetadata.variableObjMap[xidVar], newXidObj) { - return true - } + // We return an error if both occurrences of xid contain any non-XID value and are not equal. + // We don't return an error in case some of the XID values are missing from one of the + // references. This is perfectly fine. + // Stores if newXidObj contains any non XID value. + containsNonXID1 := false + // Stores if xidMetadata.variableObjMap[xidVar] contains any non XID value. + containsNonXID2 := false + for key, val := range newXidObj { + if !isXID[key] { + containsNonXID1 = true + } + // The value should either be nil in other map. If it is not nil, it should be completely + // equal. + if otherVal, ok := xidMetadata.variableObjMap[xidVar][key]; ok { + if !reflect.DeepEqual(val, otherVal) { + return true + } + } + } + for key, val := range xidMetadata.variableObjMap[xidVar] { + if !isXID[key] { + containsNonXID2 = true + } + // The value should either be nil in other map. If it is not nil, it should be completely + // equal. + if otherVal, ok := newXidObj[key]; ok { + if !reflect.DeepEqual(val, otherVal) { + return true + } + } + } + // If both contain non XID values, check for equality. We return an true if both maps are not + // equal. + if containsNonXID1 && containsNonXID2 { + return !reflect.DeepEqual(xidMetadata.variableObjMap[xidVar], newXidObj) + } return false } @@ -1785,6 +1826,11 @@ func existenceQueries( } xids := typ.XIDFields() + // xidNames[fieldName] is set to true if fieldName is XID. + isXID := make(map[string]bool) + for _, xid := range xids { + isXID[xid.Name()] = true + } var xidString string var err error if len(xids) != 0 { @@ -1807,12 +1853,7 @@ func existenceQueries( // if we already encountered an object with same xid earlier, and this object is // considered a duplicate of the existing object, then return error. - if xidMetadata.isDuplicateXid(atTopLevel, variable, obj, srcField) { - // TODO(Jatin): Add this error for inherited @id field with interface arg. - // Currently we don't return this error for the nested case when - // at both root and nested level we have same value of @id fields - // which have interface arg set and are inherited from same interface - // but are in different implementing type, we currently treat that as reference. + if xidMetadata.isDuplicateXid(atTopLevel, variable, obj, srcField, isXID) { err := errors.Errorf("duplicate XID found: %s", xidString) retErrors = append(retErrors, err) return nil, nil, retErrors @@ -1824,9 +1865,7 @@ func existenceQueries( // xidMetadata.variableObjMap[variable] = { "id": "1" } // In this case, as obj is the correct definition of the object, we update variableObjMap oldObj := xidMetadata.variableObjMap[variable] - // TODO(Jatin): This condition also needs to change in accordance with multiple xids. - // Also consider the case when @id fields can be nullable. - if len(oldObj) == 1 && len(obj) > 1 { + if len(obj) > len(oldObj) { // Continue execution to perform dfs in this case. There may be more nodes // in the subtree of this node. xidMetadata.variableObjMap[variable] = obj @@ -1854,6 +1893,21 @@ func existenceQueries( interfaceTyp, varInterface := interfaceVariable(typ, varGen, xid.Name(), xidString) if interfaceTyp != nil { + if typeName, ok := xidMetadata.interfaceVariableToTypes[varInterface]; ok { + // If we have reached this state, it means the interface XID has been + // referenced before. We throw an error if it has previously been + // referenced with different implementing type. + if typeName != typ.Name() { + err := errors.Errorf( + "using duplicate XID value: %s for XID: %s "+ + "for two different implementing"+ + " types of same interfaces: %s and"+ + " %s", xidString, xid.Name(), typeName, typ.Name()) + retErrors = append(retErrors, err) + return nil, nil, retErrors + } + } + xidMetadata.interfaceVariableToTypes[varInterface] = typ.Name() queryInterface := checkXIDExistsQuery(varInterface, xidString, xid.Name(), typ) ret = append(ret, queryInterface) diff --git a/graphql/resolve/schema.graphql b/graphql/resolve/schema.graphql index 6d8b2a1588d..0e06633b433 100644 --- a/graphql/resolve/schema.graphql +++ b/graphql/resolve/schema.graphql @@ -499,4 +499,21 @@ type CricketTeam implements Team { type LibraryManager { name: String! @id manages: [LibraryMember] -} \ No newline at end of file +} + +interface T { + name: String! @id(interface:true) + name1: String + +} + +type T1 implements T { + name2:String + link:T2 + +} + +type T2 implements T { + name3:String + +} diff --git a/graphql/resolve/update_mutation_test.yaml b/graphql/resolve/update_mutation_test.yaml index 054b915e091..c3d599b119c 100644 --- a/graphql/resolve/update_mutation_test.yaml +++ b/graphql/resolve/update_mutation_test.yaml @@ -1457,48 +1457,6 @@ message: |- failed to rewrite mutation payload because duplicate XID found: T1 -- name: "Duplicate XIDs in single mutation for Interface" - gqlmutation: | - mutation updateStudent($input: UpdateStudentInput!) { - updateStudent(input: $input) { - student { - xid - name - taughtBy { - xid - name - subject - } - } - } - } - gqlvariables: | - { - "input": { - "filter": { - "xid": {"eq": "S1"} - }, - "set": { - "taughtBy": [ - { - "xid": "T1", - "name": "Teacher1", - "subject": "Sub1", - "teaches": [ - {"xid": "T1", "name": "Stud2"} - ] - } - ] - } - } - } - explanation: "When duplicate XIDs are given as input for an Interface in a single mutation, it - should return error." - error: - message: |- - failed to rewrite mutation payload because duplicate XID found: T1 - - # Additional Deletes # # If we have