From 705f74afa03d51ef51050d33a03a32b59848dcb1 Mon Sep 17 00:00:00 2001 From: harshil Date: Sat, 23 May 2020 11:48:48 +0530 Subject: [PATCH 01/22] graphql: Updated mutation rewriting to fix OOM issue --- graphql/admin/add_group.go | 2 +- graphql/admin/schema.go | 2 +- graphql/admin/update_group.go | 7 +- graphql/resolve/add_mutation_test.yaml | 158 ++++++------ graphql/resolve/mutation.go | 56 +++-- graphql/resolve/mutation_rewriter.go | 291 +++++++++++++++------- graphql/resolve/mutation_test.go | 41 +-- graphql/resolve/schema.graphql | 5 + graphql/resolve/update_mutation_test.yaml | 143 +++++------ 9 files changed, 417 insertions(+), 288 deletions(-) diff --git a/graphql/admin/add_group.go b/graphql/admin/add_group.go index 02c77ff1a69..8b4dba87e69 100644 --- a/graphql/admin/add_group.go +++ b/graphql/admin/add_group.go @@ -21,7 +21,7 @@ func NewAddGroupRewriter() resolve.MutationRewriter { // A rule is duplicate if it has same predicate name as another rule. func (mrw *addGroupRewriter) Rewrite( ctx context.Context, - m schema.Mutation) (*resolve.UpsertMutation, error) { + m schema.Mutation) ([]*resolve.UpsertMutation, error) { addGroupInput, _ := m.ArgValue(schema.InputArgName).([]interface{}) diff --git a/graphql/admin/schema.go b/graphql/admin/schema.go index be7fcb3abac..b4769ab464d 100644 --- a/graphql/admin/schema.go +++ b/graphql/admin/schema.go @@ -59,7 +59,7 @@ type updateGQLSchemaInput struct { func (asr *updateSchemaResolver) Rewrite( ctx context.Context, - m schema.Mutation) (*resolve.UpsertMutation, error) { + m schema.Mutation) ([]*resolve.UpsertMutation, error) { glog.Info("Got updateGQLSchema request") diff --git a/graphql/admin/update_group.go b/graphql/admin/update_group.go index eb082fb71e5..8aa0b908dc4 100644 --- a/graphql/admin/update_group.go +++ b/graphql/admin/update_group.go @@ -3,6 +3,7 @@ package admin import ( "context" "fmt" + dgoapi "github.com/dgraph-io/dgo/v2/protos/api" "github.com/dgraph-io/dgraph/gql" "github.com/dgraph-io/dgraph/graphql/resolve" @@ -23,7 +24,7 @@ func NewUpdateGroupRewriter() resolve.MutationRewriter { // name as another rule. func (urw *updateGroupRewriter) Rewrite( ctx context.Context, - m schema.Mutation) (*resolve.UpsertMutation, error) { + m schema.Mutation) ([]*resolve.UpsertMutation, error) { inp := m.ArgValue(schema.InputArgName).(map[string]interface{}) setArg := inp["set"] @@ -125,10 +126,10 @@ func (urw *updateGroupRewriter) Rewrite( return nil, nil } - return &resolve.UpsertMutation{ + return []*resolve.UpsertMutation{{ Query: &gql.GraphQuery{Children: []*gql.GraphQuery{upsertQuery}}, Mutations: append(mutSet, mutDel...), - }, schema.GQLWrapf(schema.AppendGQLErrs(errSet, errDel), "failed to rewrite mutation payload") + }}, schema.GQLWrapf(schema.AppendGQLErrs(errSet, errDel), "failed to rewrite mutation payload") } // FromMutationResult rewrites the query part of a GraphQL update mutation into a Dgraph query. diff --git a/graphql/resolve/add_mutation_test.yaml b/graphql/resolve/add_mutation_test.yaml index 5b6f273d382..492e18501ab 100644 --- a/graphql/resolve/add_mutation_test.yaml +++ b/graphql/resolve/add_mutation_test.yaml @@ -837,27 +837,31 @@ State3 as State3(func: eq(State.code, "dg")) @filter(type(State)) { uid } - var(func: uid(State3)) { - Country4 as State.country - } } + dgmutations: - setjson: | { - "uid": "_:Country1", - "dgraph.type": ["Country"], - "Country.name": "Dgraph Land", - "Country.states": [ { - "uid": "_:State3", - "dgraph.type": ["State"], - "State.code": "dg", - "State.name": "Dgraph", - "State.country": { - "uid": "_:Country1" - } - } ] + "State.code": "dg", + "State.name": "Dgraph", + "dgraph.type": [ + "State" + ], + "uid": "_:State3" } cond: "@if(eq(len(State3), 0))" + + dgquerysec: |- + query { + State3 as State3(func: eq(State.code, "dg")) @filter(type(State)) { + uid + } + var(func: uid(State3)) { + Country4 as State.country + } + } + + dgmutationssec: - setjson: | { "uid": "_:Country1", @@ -1072,21 +1076,25 @@ uid } } + dgquerysec: |- + query { + District3 as District3(func: eq(District.code, "D1")) @filter(type(District)) { + uid + } + } dgmutations: - setjson: | { - "City.name":"Bengaluru", - "City.district":{ - "District.code":"D1", - "District.cities":[{"uid":"_:City1"}], - "District.name":"Dist1", - "dgraph.type":["District"], - "uid":"_:District3" - }, - "dgraph.type":["City"], - "uid":"_:City1" + "District.code": "D1", + "District.name": "Dist1", + "dgraph.type": [ + "District" + ], + "uid": "_:District3" } cond: "@if(eq(len(District3), 0))" + + dgmutationssec: - setjson: | { "City.name":"Bengaluru", @@ -1098,18 +1106,6 @@ "uid":"_:City1" } cond: "@if(eq(len(District3), 1))" - - setjson: | - { - "City.name":"NY", - "City.district":{ - "District.cities":[{"uid":"_:City4"}], - "dgraph.type":["District"], - "uid":"_:District3" - }, - "dgraph.type":["City"], - "uid":"_:City4" - } - cond: "@if(eq(len(District3), 0))" - setjson: | { "City.name":"NY", @@ -1122,6 +1118,8 @@ } cond: "@if(eq(len(District3), 1))" + + - name: "Deep Mutation Duplicate XIDs with same object with @hasInverse Test" gqlmutation: | mutation addCountry($input: [AddCountryInput!]!) { @@ -1166,7 +1164,6 @@ does not have the @hasInverse field of List type, it should return error." error: message: |- - failed to rewrite mutation payload because duplicate XID found: S1 failed to rewrite mutation payload because duplicate XID found: S1 failed to rewrite mutation payload because duplicate XID found: S2 @@ -1379,23 +1376,35 @@ "states": [ { "code": "abc", "name": "Alphabet" } ] } } + + dgquery: |- + query { + State3 as State3(func: eq(State.code, "abc")) @filter(type(State)) { + uid + } + } dgmutations: - setjson: | - { - "uid" : "_:Country1", - "dgraph.type": ["Country"], - "Country.name": "A Country", - "Country.states": [ - { - "uid": "_:State3", - "dgraph.type": ["State"], - "State.code": "abc", - "State.name": "Alphabet", - "State.country": { "uid": "_:Country1" } - } - ] + { + "State.code": "abc", + "State.name": "Alphabet", + "dgraph.type": [ + "State" + ], + "uid": "_:State3" } cond: "@if(eq(len(State3), 0))" + + dgquerysec: |- + query { + State3 as State3(func: eq(State.code, "abc")) @filter(type(State)) { + uid + } + var(func: uid(State3)) { + Country4 as State.country + } + } + dgmutationssec: - setjson: | { "uid" : "_:Country1", @@ -1416,15 +1425,6 @@ } ] cond: "@if(eq(len(State3), 1))" - dgquery: |- - query { - State3 as State3(func: eq(State.code, "abc")) @filter(type(State)) { - uid - } - var(func: uid(State3)) { - Country4 as State.country - } - } - name: "Additional Deletes - deep mutation" gqlmutation: | @@ -1447,26 +1447,17 @@ } dgmutations: - setjson: | - { - "uid":"_:Author1", - "dgraph.type":["Author"], - "Author.name":"A.N. Author", - "Author.country": { - "uid": "_:Country2", - "dgraph.type": ["Country"], - "Country.name": "A Country", - "Country.states": [ - { - "uid": "_:State4", - "dgraph.type": ["State"], - "State.code": "abc", - "State.name": "Alphabet", - "State.country": { "uid": "_:Country2" } - } - ] - } + { + "State.code": "abc", + "State.name": "Alphabet", + "dgraph.type": [ + "State" + ], + "uid": "_:State4" } cond: "@if(eq(len(State4), 0))" + + dgmutationssec: - setjson: | { "uid":"_:Author1", @@ -1492,7 +1483,8 @@ } ] cond: "@if(eq(len(State4), 1))" - dgquery: |- + + dgquerysec: |- query { State4 as State4(func: eq(State.code, "abc")) @filter(type(State)) { uid @@ -1500,4 +1492,10 @@ var(func: uid(State4)) { Country5 as State.country } - } \ No newline at end of file + } + dgquery: |- + query { + State4 as State4(func: eq(State.code, "abc")) @filter(type(State)) { + uid + } + } diff --git a/graphql/resolve/mutation.go b/graphql/resolve/mutation.go index 5082fb7094d..68a70630094 100644 --- a/graphql/resolve/mutation.go +++ b/graphql/resolve/mutation.go @@ -85,7 +85,7 @@ type MutationRewriter interface { // Rewrite rewrites GraphQL mutation m into a Dgraph mutation - that could // be as simple as a single DelNquads, or could be a Dgraph upsert mutation // with a query and multiple mutations guarded by conditions. - Rewrite(ctx context.Context, m schema.Mutation) (*UpsertMutation, error) + Rewrite(ctx context.Context, m schema.Mutation) ([]*UpsertMutation, error) // FromMutationResult takes a GraphQL mutation and the results of a Dgraph // mutation and constructs a Dgraph query. It's used to find the return @@ -206,32 +206,36 @@ func (mr *dgraphResolver) rewriteAndExecute( } } - upsert, err := mr.mutationRewriter.Rewrite(ctx, mutation) + upserts, err := mr.mutationRewriter.Rewrite(ctx, mutation) if err != nil { return emptyResult(schema.GQLWrapf(err, "couldn't rewrite mutation %s", mutation.Name())), resolverFailed } - req := &dgoapi.Request{ - Query: dgraph.AsString(upsert.Query), - CommitNow: true, - Mutations: upsert.Mutations, - } + result := make(map[string]interface{}) + req := &dgoapi.Request{} - mutResp, err = mr.executor.Execute(ctx, req) - if err != nil { - gqlErr := schema.GQLWrapLocationf( - err, mutation.Location(), "mutation %s failed", mutation.Name()) - return emptyResult(gqlErr), resolverFailed + for _, upsert := range upserts { + if len(upsert.Mutations) == 0 { + continue + } + req.Query = dgraph.AsString(upsert.Query) + req.Mutations = upsert.Mutations - } + mutResp, err = mr.executor.Execute(ctx, req) + if err != nil { + gqlErr := schema.GQLWrapLocationf( + err, mutation.Location(), "mutation %s failed", mutation.Name()) + return emptyResult(gqlErr), resolverFailed - result := make(map[string]interface{}) - if req.Query != "" && len(mutResp.GetJson()) != 0 { - if err := json.Unmarshal(mutResp.GetJson(), &result); err != nil { - return emptyResult( - schema.GQLWrapf(err, "Couldn't unmarshal response from Dgraph mutation")), - resolverFailed + } + + if req.Query != "" && len(mutResp.GetJson()) != 0 { + if err := json.Unmarshal(mutResp.GetJson(), &result); err != nil { + return emptyResult( + schema.GQLWrapf(err, "Couldn't unmarshal response from Dgraph mutation")), + resolverFailed + } } } @@ -243,13 +247,15 @@ func (mr *dgraphResolver) rewriteAndExecute( return emptyResult(errs), resolverFailed } - err = mr.executor.CommitOrAbort(ctx, mutResp.Txn) - if err != nil { - return emptyResult( - schema.GQLWrapf(err, "mutation failed, couldn't commit transaction")), - resolverFailed + if mutResp != nil { + err = mr.executor.CommitOrAbort(ctx, mutResp.Txn) + if err != nil { + return emptyResult( + schema.GQLWrapf(err, "mutation failed, couldn't commit transaction")), + resolverFailed + } + commit = true } - commit = true qryResp, err := mr.executor.Execute(ctx, &dgoapi.Request{Query: dgraph.AsString(dgQuery), ReadOnly: true}) diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index e572a401424..d551506c331 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -225,8 +225,7 @@ func newXidMetadata() *xidMetadata { // } ], // "Author.friends":[ {"uid":"0x123"} ], // } -func (mrw *AddRewriter) Rewrite(ctx context.Context, m schema.Mutation) (*UpsertMutation, error) { - +func (mrw *AddRewriter) Rewrite(ctx context.Context, m schema.Mutation) ([]*UpsertMutation, error) { mutatedType := m.MutatedType() if m.IsArgListType(schema.InputArgName) { @@ -236,8 +235,8 @@ func (mrw *AddRewriter) Rewrite(ctx context.Context, m schema.Mutation) (*Upsert varGen := NewVariableGenerator() val := m.ArgValue(schema.InputArgName).(map[string]interface{}) xidMd := newXidMetadata() - mrw.frags = [][]*mutationFragment{rewriteObject(mutatedType, nil, "", varGen, true, val, - xidMd)} + obj := rewriteObject(mutatedType, nil, "", varGen, true, val, xidMd) + mrw.frags = [][]*mutationFragment{obj.get()} mutations, err := mutationsFromFragments( mrw.frags[0], func(frag *mutationFragment) ([]byte, error) { @@ -255,24 +254,25 @@ func (mrw *AddRewriter) Rewrite(ctx context.Context, m schema.Mutation) (*Upsert Mutations: mutations, } - return upsert, schema.GQLWrapf(err, "failed to rewrite mutation payload") + return []*UpsertMutation{upsert}, schema.GQLWrapf(err, "failed to rewrite mutation payload") } -func (mrw *AddRewriter) handleMultipleMutations(m schema.Mutation) (*UpsertMutation, error) { +func (mrw *AddRewriter) handleMultipleMutations(m schema.Mutation) ([]*UpsertMutation, error) { mutatedType := m.MutatedType() val, _ := m.ArgValue(schema.InputArgName).([]interface{}) varGen := NewVariableGenerator() xidMd := newXidMetadata() var errs error - var mutationsAll []*dgoapi.Mutation - queries := &gql.GraphQuery{} - for _, i := range val { - obj := i.(map[string]interface{}) - frag := rewriteObject(mutatedType, nil, "", varGen, true, obj, xidMd) - mrw.frags = append(mrw.frags, frag) + mutationsAllSec := []*dgoapi.Mutation{} + queriesSec := &gql.GraphQuery{} + + mutationsAll := []*dgoapi.Mutation{} + queries := &gql.GraphQuery{} + buildMutations := func(mutationsAll []*dgoapi.Mutation, queries *gql.GraphQuery, + frag []*mutationFragment, keepError bool) []*dgoapi.Mutation { mutations, err := mutationsFromFragments( frag, func(frag *mutationFragment) ([]byte, error) { @@ -293,18 +293,44 @@ func (mrw *AddRewriter) handleMultipleMutations(m schema.Mutation) (*UpsertMutat if qry != nil { queries.Children = append(queries.Children, qry.Children...) } + + return mutationsAll + } + + for _, i := range val { + obj := i.(map[string]interface{}) + frag := rewriteObject(mutatedType, nil, "", varGen, true, obj, xidMd) + mrw.frags = append(mrw.frags, frag.get()) + + mutationsAll = buildMutations(mutationsAll, queries, frag.firstPass, false) + mutationsAllSec = buildMutations(mutationsAllSec, queriesSec, frag.secondPass, true) } if len(queries.Children) == 0 { queries = nil } - upsert := &UpsertMutation{ - Query: queries, - Mutations: mutationsAll, + if len(queriesSec.Children) == 0 { + queriesSec = nil + } + + result := []*UpsertMutation{} + + if len(mutationsAll) > 0 { + result = append(result, &UpsertMutation{ + Query: queries, + Mutations: mutationsAll, + }) + } + + if len(mutationsAllSec) > 0 { + result = append(result, &UpsertMutation{ + Query: queriesSec, + Mutations: mutationsAllSec, + }) } - return upsert, errs + return result, errs } // FromMutationResult rewrites the query part of a GraphQL add mutation into a Dgraph query. @@ -373,7 +399,7 @@ func (mrw *AddRewriter) FromMutationResult( // See AddRewriter for how the set and remove fragments get created. func (urw *UpdateRewriter) Rewrite( ctx context.Context, - m schema.Mutation) (*UpsertMutation, error) { + m schema.Mutation) ([]*UpsertMutation, error) { mutatedType := m.MutatedType() @@ -389,62 +415,97 @@ func (urw *UpdateRewriter) Rewrite( srcUID := MutationQueryVarUID xidMd := newXidMetadata() - var errSet, errDel error - var mutSet, mutDel []*dgoapi.Mutation + var errs error varGen := NewVariableGenerator() + buildMutation := func(setFrag, delFrag []*mutationFragment, keepError bool) *UpsertMutation { + var mutSet, mutDel []*dgoapi.Mutation + queries := []*gql.GraphQuery{upsertQuery} + + if setArg != nil { + urw.setFrags = append(urw.setFrags, setFrag...) + addUpdateCondition(setFrag) + var errSet error + mutSet, errSet = mutationsFromFragments( + setFrag, + func(frag *mutationFragment) ([]byte, error) { + return json.Marshal(frag.fragment) + }, + func(frag *mutationFragment) ([]byte, error) { + if len(frag.deletes) > 0 { + return json.Marshal(frag.deletes) + } + return nil, nil + }) + + if keepError { + errs = schema.AppendGQLErrs(errs, errSet) + } + + q1 := queryFromFragments(setFrag) + if q1 != nil { + queries = append(queries, q1.Children...) + } + } + + if delArg != nil { + urw.delFrags = append(urw.delFrags, delFrag...) + addUpdateCondition(delFrag) + var errDel error + mutDel, errDel = mutationsFromFragments( + urw.delFrags, + func(frag *mutationFragment) ([]byte, error) { + return nil, nil + }, + func(frag *mutationFragment) ([]byte, error) { + return json.Marshal(frag.fragment) + }) + + if keepError { + errs = schema.AppendGQLErrs(errs, errDel) + } + + q2 := queryFromFragments(delFrag) + if q2 != nil { + queries = append(queries, q2.Children...) + } + } + + return &UpsertMutation{ + Query: &gql.GraphQuery{Children: queries}, + Mutations: append(mutSet, mutDel...), + } + } + + var setFragF, setFragS, delFragF, delFragS []*mutationFragment + if setArg != nil { - urw.setFrags = - rewriteObject(mutatedType, nil, srcUID, varGen, true, - setArg.(map[string]interface{}), xidMd) - addUpdateCondition(urw.setFrags) - mutSet, errSet = mutationsFromFragments( - urw.setFrags, - func(frag *mutationFragment) ([]byte, error) { - return json.Marshal(frag.fragment) - }, - func(frag *mutationFragment) ([]byte, error) { - if len(frag.deletes) > 0 { - return json.Marshal(frag.deletes) - } - return nil, nil - }) + setFrag := rewriteObject(mutatedType, nil, srcUID, varGen, true, + setArg.(map[string]interface{}), xidMd) + setFragF = setFrag.firstPass + setFragS = setFrag.secondPass } if delArg != nil { - urw.delFrags = - rewriteObject(mutatedType, nil, srcUID, varGen, false, - delArg.(map[string]interface{}), xidMd) - addUpdateCondition(urw.delFrags) - mutDel, errDel = mutationsFromFragments( - urw.delFrags, - func(frag *mutationFragment) ([]byte, error) { - return nil, nil - }, - func(frag *mutationFragment) ([]byte, error) { - return json.Marshal(frag.fragment) - }) + delFrag := rewriteObject(mutatedType, nil, srcUID, varGen, false, + delArg.(map[string]interface{}), xidMd) + delFragF = delFrag.firstPass + delFragS = delFrag.secondPass } - queries := []*gql.GraphQuery{upsertQuery} - - q1 := queryFromFragments(urw.setFrags) - if q1 != nil { - queries = append(queries, q1.Children...) - } + result := []*UpsertMutation{} - q2 := queryFromFragments(urw.delFrags) - if q2 != nil { - queries = append(queries, q2.Children...) + firstPass := buildMutation(setFragF, delFragF, false) + if len(firstPass.Mutations) > 0 { + result = append(result, firstPass) } - upsert := &UpsertMutation{ - Query: &gql.GraphQuery{Children: queries}, - Mutations: append(mutSet, mutDel...), + secondPass := buildMutation(setFragS, delFragS, true) + if len(secondPass.Mutations) > 0 { + result = append(result, secondPass) } - return upsert, - schema.GQLWrapf(schema.AppendGQLErrs(errSet, errDel), "failed to rewrite mutation payload") + return result, schema.GQLWrapf(errs, "failed to rewrite mutation payload") } // FromMutationResult rewrites the query part of a GraphQL update mutation into a Dgraph query. @@ -567,7 +628,7 @@ func RewriteUpsertQueryFromMutation(m schema.Mutation) *gql.GraphQuery { func (drw *deleteRewriter) Rewrite( ctx context.Context, - m schema.Mutation) (*UpsertMutation, error) { + m schema.Mutation) ([]*UpsertMutation, error) { if m.MutationType() != schema.DeleteMutation { return nil, errors.Errorf( @@ -621,7 +682,7 @@ func (drw *deleteRewriter) Rewrite( Mutations: []*dgoapi.Mutation{{DeleteJson: b}}, } - return upsert, err + return []*UpsertMutation{upsert}, err } func (drw *deleteRewriter) FromMutationResult( @@ -707,6 +768,17 @@ func queryFromFragments(frags []*mutationFragment) *gql.GraphQuery { return qry } +type mutationRes struct { + firstPass []*mutationFragment + secondPass []*mutationFragment +} + +func (mr *mutationRes) get() []*mutationFragment { + k := mr.firstPass + k = append(k, mr.secondPass...) + return k +} + // rewriteObject rewrites obj to a list of mutation fragments. See AddRewriter.Rewrite // for a description of what those fragments look like. // @@ -729,7 +801,7 @@ func rewriteObject( varGen *VariableGenerator, withAdditionalDeletes bool, obj map[string]interface{}, - xidMetadata *xidMetadata) []*mutationFragment { + xidMetadata *xidMetadata) *mutationRes { atTopLevel := srcField == nil topLevelAdd := srcUID == "" @@ -740,8 +812,9 @@ func rewriteObject( if id != nil { if idVal, ok := obj[id.Name()]; ok { if idVal != nil { - return []*mutationFragment{ - asIDReference(idVal, srcField, srcUID, variable, withAdditionalDeletes, varGen)} + return &mutationRes{secondPass: []*mutationFragment{ + asIDReference(idVal, srcField, srcUID, variable, + withAdditionalDeletes, varGen)}} } delete(obj, id.Name()) } @@ -757,7 +830,7 @@ func rewriteObject( if !ok { errFrag := newFragment(nil) errFrag.err = errors.New("encountered an XID that isn't a string") - return []*mutationFragment{errFrag} + return &mutationRes{secondPass: []*mutationFragment{errFrag}} } // if the object has an xid, the variable name will be formed from the xidValue in order // to handle duplicate object addition/updation @@ -779,7 +852,7 @@ func rewriteObject( xidObj, obj) || (invField != nil && invField.Type().ListType() == nil) { errFrag := newFragment(nil) errFrag.err = errors.Errorf("duplicate XID found: %s", xidString) - return []*mutationFragment{errFrag} + return &mutationRes{secondPass: []*mutationFragment{errFrag}} } } else { // if not encountered till now, add it to the map @@ -805,8 +878,8 @@ func rewriteObject( } else { name = id.Name() } - return invalidObjectFragment(fmt.Errorf("%s is not provided", name), - xidFrag, variable, xidString) + return &mutationRes{secondPass: invalidObjectFragment(fmt.Errorf("%s is not provided", name), + xidFrag, variable, xidString)} } } @@ -822,14 +895,18 @@ func rewriteObject( if err := typ.EnsureNonNulls(obj, exclude); err != nil { // This object is either an invalid deep mutation or it's an xid reference // and asXIDReference must to apply or it's an error. - return invalidObjectFragment(err, xidFrag, variable, xidString) + return &mutationRes{secondPass: invalidObjectFragment(err, xidFrag, variable, xidString)} } } if !atTopLevel && !withAdditionalDeletes { // For remove op (!withAdditionalDeletes), we don't need to generate a new // blank node. - return []*mutationFragment{xidFrag} + if xidFrag != nil { + return &mutationRes{secondPass: []*mutationFragment{xidFrag}} + } else { + return &mutationRes{} + } } var myUID string @@ -841,15 +918,16 @@ func rewriteObject( newObj["dgraph.type"] = dgraphTypes myUID = fmt.Sprintf("_:%s", variable) - addInverseLink(newObj, srcField, srcUID) + if xid == nil { + addInverseLink(newObj, srcField, srcUID) + } } else { myUID = srcUID } newObj["uid"] = myUID - frag := newFragment(newObj) - results := []*mutationFragment{frag} + results := &mutationRes{secondPass: []*mutationFragment{frag}} // if xidString != "", then we are adding with an xid. In which case, we have to ensure // as part of the upsert that the xid doesn't already exist. @@ -867,11 +945,22 @@ func rewriteObject( nil) } + if xid != nil && !atTopLevel { + results = &mutationRes{ + firstPass: []*mutationFragment{frag}, + } + if xidFrag != nil { + frag.queries = []*gql.GraphQuery{ + xidQuery(variable, xidString, xid.Name(), typ), + } + } + } + // if this object has an xid, then we don't need to rewrite its children if we have encountered // it earlier if xidString == "" || xidEncounteredFirstTime { for field, val := range obj { - var frags []*mutationFragment + var frags *mutationRes fieldDef := typ.Field(field) fieldName := typ.DgraphPredicate(field) @@ -888,6 +977,7 @@ func rewriteObject( frags = rewriteObject(fieldDef.Type(), fieldDef, myUID, varGen, withAdditionalDeletes, val, xidMetadata) + case []interface{}: // This field is either: // 1) A list of objects: e.g. if the schema said `categories: [Categories]` @@ -912,15 +1002,24 @@ func rewriteObject( // e.g. to remove the text or // { "friends": null, ... } // to remove all friends - frags = []*mutationFragment{newFragment(val)} + if xid != nil && !atTopLevel { + frags = &mutationRes{firstPass: []*mutationFragment{newFragment(val)}} + } else { + frags = &mutationRes{secondPass: []*mutationFragment{newFragment(val)}} + } } - results = squashFragments(squashIntoObject(fieldName), results, frags) + results.firstPass = squashFragments(squashIntoObject(fieldName), results.firstPass, frags.firstPass) + results.secondPass = squashFragments(squashIntoObject(fieldName), results.secondPass, frags.secondPass) } } if xidFrag != nil { - results = append(results, xidFrag) + results.secondPass = append(results.secondPass, xidFrag) + } + + if xid != nil && !atTopLevel && !xidEncounteredFirstTime { + results.firstPass = []*mutationFragment{} } return results @@ -1256,25 +1355,28 @@ func rewriteList( varGen *VariableGenerator, withAdditionalDeletes bool, objects []interface{}, - xidMetadata *xidMetadata) []*mutationFragment { + xidMetadata *xidMetadata) *mutationRes { + + result := &mutationRes{} - frags := []*mutationFragment{newFragment(make([]interface{}, 0))} + result.secondPass = []*mutationFragment{newFragment(make([]interface{}, 0))} for _, obj := range objects { switch obj := obj.(type) { case map[string]interface{}: - frags = squashFragments(squashIntoList, frags, - rewriteObject(typ, srcField, srcUID, varGen, withAdditionalDeletes, obj, xidMetadata)) + frag := rewriteObject(typ, srcField, srcUID, varGen, withAdditionalDeletes, obj, xidMetadata) + result.firstPass = appendFragments(result.firstPass, frag.firstPass) + result.secondPass = squashFragments(squashIntoList, result.secondPass, frag.secondPass) default: // All objects in the list must be of the same type. GraphQL validation makes sure // of that. So this must be a list of scalar values (lists of lists aren't allowed). - return []*mutationFragment{ + return &mutationRes{secondPass: []*mutationFragment{ newFragment(objects), - } + }} } } - return frags + return result } func newFragment(f interface{}) *mutationFragment { @@ -1312,6 +1414,24 @@ func squashIntoObject(label string) func(interface{}, interface{}, bool) interfa } } +func appendFragments(left, right []*mutationFragment) []*mutationFragment { + if len(left) == 0 { + return right + } + + if len(right) == 0 { + return left + } + + for _, i := range right { + left[0].queries = append(left[0].queries, i.queries...) + i.queries = []*gql.GraphQuery{} + } + + left = append(left, right...) + return left +} + // squashFragments takes two lists of mutationFragments and produces a single list // that has all the right fragments squashed into the left. // @@ -1420,6 +1540,7 @@ func squashFragments( for _, r := range right { queries = append(queries, r.queries...) } + result[0].queries = queries return result diff --git a/graphql/resolve/mutation_test.go b/graphql/resolve/mutation_test.go index 626d752dae5..1ea0aeeb260 100644 --- a/graphql/resolve/mutation_test.go +++ b/graphql/resolve/mutation_test.go @@ -23,6 +23,7 @@ import ( "strings" "testing" + dgoapi "github.com/dgraph-io/dgo/v2/protos/api" "github.com/dgraph-io/dgraph/graphql/dgraph" "github.com/dgraph-io/dgraph/graphql/schema" "github.com/dgraph-io/dgraph/graphql/test" @@ -45,7 +46,9 @@ type testCase struct { GQLVariables string Explanation string DGMutations []*dgraphMutation + DGMutationsSec []*dgraphMutation DGQuery string + DGQuerySec string Error *x.GqlError ValidationError *x.GqlError } @@ -112,6 +115,19 @@ func mutationRewriting(t *testing.T, file string, rewriterFactory func() Mutatio gqlSchema := test.LoadSchemaFromFile(t, "schema.graphql") + compareMutations := func(t *testing.T, test []*dgraphMutation, generated []*dgoapi.Mutation) { + require.Len(t, generated, len(test)) + for i, expected := range test { + require.Equal(t, expected.Cond, generated[i].Cond) + if len(generated[i].SetJson) > 0 || expected.SetJSON != "" { + require.JSONEq(t, expected.SetJSON, string(generated[i].SetJson)) + } + if len(generated[i].DeleteJson) > 0 || expected.DeleteJSON != "" { + require.JSONEq(t, expected.DeleteJSON, string(generated[i].DeleteJson)) + } + } + } + for _, tcase := range tests { t.Run(tcase.Name, func(t *testing.T) { // -- Arrange -- @@ -132,24 +148,21 @@ func mutationRewriting(t *testing.T, file string, rewriterFactory func() Mutatio // -- Act -- upsert, err := rewriterToTest.Rewrite(context.Background(), mut) - q := upsert.Query - muts := upsert.Mutations // -- Assert -- if tcase.Error != nil || err != nil { + require.NotNil(t, err) + require.NotNil(t, tcase.Error) require.Equal(t, tcase.Error.Error(), err.Error()) - } else { - require.Equal(t, tcase.DGQuery, dgraph.AsString(q)) - require.Len(t, muts, len(tcase.DGMutations)) - for i, expected := range tcase.DGMutations { - require.Equal(t, expected.Cond, muts[i].Cond) - if len(muts[i].SetJson) > 0 || expected.SetJSON != "" { - require.JSONEq(t, expected.SetJSON, string(muts[i].SetJson)) - } - if len(muts[i].DeleteJson) > 0 || expected.DeleteJSON != "" { - require.JSONEq(t, expected.DeleteJSON, string(muts[i].DeleteJson)) - } - } + return + } + + require.Equal(t, tcase.DGQuery, dgraph.AsString(upsert[0].Query)) + compareMutations(t, tcase.DGMutations, upsert[0].Mutations) + + if len(upsert) > 1 { + require.Equal(t, tcase.DGQuerySec, dgraph.AsString(upsert[1].Query)) + compareMutations(t, tcase.DGMutationsSec, upsert[1].Mutations) } }) } diff --git a/graphql/resolve/schema.graphql b/graphql/resolve/schema.graphql index 50e64ee5e17..0a2113b07d4 100644 --- a/graphql/resolve/schema.graphql +++ b/graphql/resolve/schema.graphql @@ -39,6 +39,11 @@ type Post { postType: PostType @search author: Author! category: Category @hasInverse(field: posts) + ps: PostSecret +} + +type PostSecret { + title: String! @id } type Category { diff --git a/graphql/resolve/update_mutation_test.yaml b/graphql/resolve/update_mutation_test.yaml index 9264df76850..226f9d29f59 100644 --- a/graphql/resolve/update_mutation_test.yaml +++ b/graphql/resolve/update_mutation_test.yaml @@ -670,23 +670,16 @@ explanation: "The update has a choice of linking to new or existing state" dgmutations: - setjson: | - { "uid" : "uid(x)", - "Author.country": { - "uid": "_:Country2", - "dgraph.type": ["Country"], - "Country.name": "New Country", - "Country.states": [ { - "uid": "_:State4", - "dgraph.type": ["State"], - "State.code": "dg", - "State.name": "Dgraph", - "State.country": { - "uid": "_:Country2" - } - } ] - } + { + "State.code": "dg", + "State.name": "Dgraph", + "dgraph.type": [ + "State" + ], + "uid": "_:State4" } cond: "@if(eq(len(State4), 0) AND gt(len(x), 0))" + dgmutationssec: - setjson: | { "uid" : "uid(x)", "Author.country": { @@ -710,6 +703,15 @@ ] cond: "@if(eq(len(State4), 1) AND gt(len(x), 0))" dgquery: |- + query { + x as updateAuthor(func: type(Author)) @filter(uid(0x123)) { + uid + } + State4 as State4(func: eq(State.code, "dg")) @filter(type(State)) { + uid + } + } + dgquerysec: |- query { x as updateAuthor(func: type(Author)) @filter(uid(0x123)) { uid @@ -964,54 +966,28 @@ uid } } + dgquerysec: |- + query { + x as updateStudent(func: type(Student)) @filter(uid(0x123)) { + uid + } + Teacher3 as Teacher3(func: eq(People.xid, "T1")) @filter(type(Teacher)) { + uid + } + } dgmutations: - setjson: | { - "Student.taughtBy": [ - { - "People.name": "Teacher1", - "People.xid": "T1", - "dgraph.type": [ "Teacher", "People" ], - "uid": "_:Teacher3" - }, - { - "dgraph.type": [ "Teacher", "People" ], - "uid": "_:Teacher3" - } + "People.name": "Teacher1", + "People.xid": "T1", + "dgraph.type": [ + "Teacher", + "People" ], - "uid": "uid(x)" + "uid": "_:Teacher3" } - cond: "@if(eq(len(Teacher3), 0) AND eq(len(Teacher3), 0) AND gt(len(x), 0))" - - setjson: | - { - "Student.taughtBy": [ - { - "People.name": "Teacher1", - "People.xid": "T1", - "dgraph.type": [ "Teacher", "People" ], - "uid": "_:Teacher3" - }, - { - "uid": "uid(Teacher3)" - } - ], - "uid": "uid(x)" - } - cond: "@if(eq(len(Teacher3), 0) AND eq(len(Teacher3), 1) AND gt(len(x), 0))" - - setjson: | - { - "Student.taughtBy": [ - { - "uid": "uid(Teacher3)" - }, - { - "dgraph.type": [ "Teacher", "People" ], - "uid": "_:Teacher3" - } - ], - "uid": "uid(x)" - } - cond: "@if(eq(len(Teacher3), 1) AND eq(len(Teacher3), 0) AND gt(len(x), 0))" + cond: "@if(eq(len(Teacher3), 0) AND gt(len(x), 0))" + dgmutationssec: - setjson: | { "Student.taughtBy": [ @@ -1061,7 +1037,6 @@ error: message: |- failed to rewrite mutation payload because duplicate XID found: S1 - failed to rewrite mutation payload because duplicate XID found: S1 - name: "Deep Mutation Duplicate XIDs with different object Test" gqlmutation: | @@ -1097,7 +1072,6 @@ error: message: |- failed to rewrite mutation payload because duplicate XID found: T1 - failed to rewrite mutation payload because duplicate XID found: T1 - name: "Duplicate XIDs in single mutation for Interface" @@ -1305,19 +1279,14 @@ } dgmutations: - setjson: | - { - "uid" : "uid(x)", - "Country.states": [ - { - "uid": "_:State3", - "dgraph.type": ["State"], - "State.code": "abc", - "State.name": "Alphabet", - "State.country": { "uid": "uid(x)" } - } - ] + { + "uid": "_:State3", + "dgraph.type": ["State"], + "State.code": "abc", + "State.name": "Alphabet" } cond: "@if(eq(len(State3), 0) AND gt(len(x), 0))" + dgmutationssec: - setjson: | { "uid" : "uid(x)", @@ -1337,6 +1306,15 @@ ] cond: "@if(eq(len(State3), 1) AND gt(len(x), 0))" dgquery: |- + query { + x as updateCountry(func: type(Country)) @filter(uid(0x123)) { + uid + } + State3 as State3(func: eq(State.code, "abc")) @filter(type(State)) { + uid + } + } + dgquerysec: |- query { x as updateCountry(func: type(Country)) @filter(uid(0x123)) { uid @@ -1368,15 +1346,13 @@ } dgmutations: - setjson: | - { "uid" : "uid(x)", - "ComputerOwner.computers": { - "uid": "_:Computer3", - "dgraph.type": ["Computer"], - "Computer.name": "Comp", - "Computer.owners": [ { "uid": "uid(x)" } ] - } + { + "uid": "_:Computer3", + "dgraph.type": ["Computer"], + "Computer.name": "Comp" } cond: "@if(eq(len(Computer3), 0) AND gt(len(x), 0))" + dgmutationssec: - setjson: | { "uid" : "uid(x)", "ComputerOwner.computers": { @@ -1393,6 +1369,15 @@ ] cond: "@if(eq(len(Computer3), 1) AND gt(len(x), 0))" dgquery: |- + query { + x as updateComputerOwner(func: type(ComputerOwner)) @filter(eq(ComputerOwner.name, "A.N. Owner")) { + uid + } + Computer3 as Computer3(func: eq(Computer.name, "Comp")) @filter(type(Computer)) { + uid + } + } + dgquerysec: |- query { x as updateComputerOwner(func: type(ComputerOwner)) @filter(eq(ComputerOwner.name, "A.N. Owner")) { uid @@ -1403,4 +1388,4 @@ var(func: uid(x)) { Computer4 as ComputerOwner.computers @filter(NOT (uid(Computer3))) } - } \ No newline at end of file + } From 2855adb1b2f3fe7d2fcab7c0fabcef3ec8d02736 Mon Sep 17 00:00:00 2001 From: harshil Date: Thu, 28 May 2020 20:47:51 +0530 Subject: [PATCH 02/22] added a big test --- graphql/resolve/add_mutation_test.yaml | 259 +++++++++++++++++++++++++ 1 file changed, 259 insertions(+) diff --git a/graphql/resolve/add_mutation_test.yaml b/graphql/resolve/add_mutation_test.yaml index 492e18501ab..4430ef8a8f4 100644 --- a/graphql/resolve/add_mutation_test.yaml +++ b/graphql/resolve/add_mutation_test.yaml @@ -26,6 +26,265 @@ "Author.posts":[] } +- + name: "Add deep mutation with variables" + gqlmutation: | + mutation addAuthor($auth: AddAuthorInput!) { + addAuthor(input: [$auth]) { + author { + name + } + } + } + gqlvariables: | + { "auth": + { "name": "A.N. Author", + "posts": [{ + "title": "post1", + "ps": {"title": "ps1"} + }, { + "title": "post2", + "ps": {"title": "ps2"} + }, { + "title": "post3", + "ps": {"title": "ps3"} + }, { + "title": "post4", + "ps": {"title": "ps4"} + }, { + "title": "post5", + "ps": {"title": "ps5"} + }, { + "title": "post6", + "ps": {"title": "ps6"} + }, { + "title": "post7", + "ps": {"title": "ps7"} + }, { + "title": "post8", + "ps": {"title": "ps8"} + }] + } + } + explanation: "A uid and type should get injected and all data transformed to + underlying Dgraph edge names" + dgquery: |- + query { + PostSecret4 as PostSecret4(func: eq(PostSecret.title, "ps1")) @filter(type(PostSecret)) { + uid + } + PostSecret7 as PostSecret7(func: eq(PostSecret.title, "ps2")) @filter(type(PostSecret)) { + uid + } + PostSecret10 as PostSecret10(func: eq(PostSecret.title, "ps3")) @filter(type(PostSecret)) { + uid + } + PostSecret13 as PostSecret13(func: eq(PostSecret.title, "ps4")) @filter(type(PostSecret)) { + uid + } + PostSecret16 as PostSecret16(func: eq(PostSecret.title, "ps5")) @filter(type(PostSecret)) { + uid + } + PostSecret19 as PostSecret19(func: eq(PostSecret.title, "ps6")) @filter(type(PostSecret)) { + uid + } + PostSecret22 as PostSecret22(func: eq(PostSecret.title, "ps7")) @filter(type(PostSecret)) { + uid + } + PostSecret25 as PostSecret25(func: eq(PostSecret.title, "ps8")) @filter(type(PostSecret)) { + uid + } + } + dgquerysec: |- + query { + PostSecret4 as PostSecret4(func: eq(PostSecret.title, "ps1")) @filter(type(PostSecret)) { + uid + } + PostSecret7 as PostSecret7(func: eq(PostSecret.title, "ps2")) @filter(type(PostSecret)) { + uid + } + PostSecret10 as PostSecret10(func: eq(PostSecret.title, "ps3")) @filter(type(PostSecret)) { + uid + } + PostSecret13 as PostSecret13(func: eq(PostSecret.title, "ps4")) @filter(type(PostSecret)) { + uid + } + PostSecret16 as PostSecret16(func: eq(PostSecret.title, "ps5")) @filter(type(PostSecret)) { + uid + } + PostSecret19 as PostSecret19(func: eq(PostSecret.title, "ps6")) @filter(type(PostSecret)) { + uid + } + PostSecret22 as PostSecret22(func: eq(PostSecret.title, "ps7")) @filter(type(PostSecret)) { + uid + } + PostSecret25 as PostSecret25(func: eq(PostSecret.title, "ps8")) @filter(type(PostSecret)) { + uid + } + } + dgmutations: + - setjson: | + { + "PostSecret.title": "ps1", + "dgraph.type": [ + "PostSecret" + ], + "uid": "_:PostSecret4" + } + cond: "@if(eq(len(PostSecret4), 0))" + - setjson: | + { + "PostSecret.title": "ps2", + "dgraph.type": [ + "PostSecret" + ], + "uid": "_:PostSecret7" + } + cond: "@if(eq(len(PostSecret7), 0))" + - setjson: | + { + "PostSecret.title": "ps3", + "dgraph.type": [ + "PostSecret" + ], + "uid": "_:PostSecret10" + } + cond: "@if(eq(len(PostSecret10), 0))" + - setjson: | + { + "PostSecret.title": "ps4", + "dgraph.type": [ + "PostSecret" + ], + "uid": "_:PostSecret13" + } + cond: "@if(eq(len(PostSecret13), 0))" + - setjson: | + { + "PostSecret.title": "ps5", + "dgraph.type": [ + "PostSecret" + ], + "uid": "_:PostSecret16" + } + cond: "@if(eq(len(PostSecret16), 0))" + - setjson: | + { + "PostSecret.title": "ps6", + "dgraph.type": [ + "PostSecret" + ], + "uid": "_:PostSecret19" + } + cond: "@if(eq(len(PostSecret19), 0))" + - setjson: | + { + "PostSecret.title": "ps7", + "dgraph.type": [ + "PostSecret" + ], + "uid": "_:PostSecret22" + } + cond: "@if(eq(len(PostSecret22), 0))" + - setjson: | + { + "PostSecret.title": "ps8", + "dgraph.type": [ + "PostSecret" + ], + "uid": "_:PostSecret25" + } + cond: "@if(eq(len(PostSecret25), 0))" + dgmutationssec: + - setjson: | + { + "Author.name": "A.N. Author", + "Author.posts": [{ + "Post.author": { + "uid": "_:Author1" + }, + "Post.ps": { + "uid": "uid(PostSecret4)" + }, + "Post.title": "post1", + "dgraph.type": ["Post"], + "uid": "_:Post2" + }, { + "Post.author": { + "uid": "_:Author1" + }, + "Post.ps": { + "uid": "uid(PostSecret7)" + }, + "Post.title": "post2", + "dgraph.type": ["Post"], + "uid": "_:Post5" + }, { + "Post.author": { + "uid": "_:Author1" + }, + "Post.ps": { + "uid": "uid(PostSecret10)" + }, + "Post.title": "post3", + "dgraph.type": ["Post"], + "uid": "_:Post8" + }, { + "Post.author": { + "uid": "_:Author1" + }, + "Post.ps": { + "uid": "uid(PostSecret13)" + }, + "Post.title": "post4", + "dgraph.type": ["Post"], + "uid": "_:Post11" + }, { + "Post.author": { + "uid": "_:Author1" + }, + "Post.ps": { + "uid": "uid(PostSecret16)" + }, + "Post.title": "post5", + "dgraph.type": ["Post"], + "uid": "_:Post14" + }, { + "Post.author": { + "uid": "_:Author1" + }, + "Post.ps": { + "uid": "uid(PostSecret19)" + }, + "Post.title": "post6", + "dgraph.type": ["Post"], + "uid": "_:Post17" + }, { + "Post.author": { + "uid": "_:Author1" + }, + "Post.ps": { + "uid": "uid(PostSecret22)" + }, + "Post.title": "post7", + "dgraph.type": ["Post"], + "uid": "_:Post20" + }, { + "Post.author": { + "uid": "_:Author1" + }, + "Post.ps": { + "uid": "uid(PostSecret25)" + }, + "Post.title": "post8", + "dgraph.type": ["Post"], + "uid": "_:Post23" + }], + "dgraph.type": ["Author"], + "uid": "_:Author1" + } + cond: "@if(eq(len(PostSecret4), 1) AND eq(len(PostSecret7), 1) AND eq(len(PostSecret10), 1) AND eq(len(PostSecret13), 1) AND eq(len(PostSecret16), 1) AND eq(len(PostSecret19), 1) AND eq(len(PostSecret22), 1) AND eq(len(PostSecret25), 1))" + - name: "Add multiple mutation with variables" gqlmutation: | From 058fcdf8f77f3d0fefc1a5d779a709b5b6ee78a7 Mon Sep 17 00:00:00 2001 From: harshil Date: Thu, 28 May 2020 22:09:19 +0530 Subject: [PATCH 03/22] fixed e2e tests --- graphql/resolve/mutation_rewriter.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index d551506c331..c60be032041 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -300,7 +300,7 @@ func (mrw *AddRewriter) handleMultipleMutations(m schema.Mutation) ([]*UpsertMut for _, i := range val { obj := i.(map[string]interface{}) frag := rewriteObject(mutatedType, nil, "", varGen, true, obj, xidMd) - mrw.frags = append(mrw.frags, frag.get()) + mrw.frags = append(mrw.frags, frag.secondPass) mutationsAll = buildMutations(mutationsAll, queries, frag.firstPass, false) mutationsAllSec = buildMutations(mutationsAllSec, queriesSec, frag.secondPass, true) @@ -423,7 +423,6 @@ func (urw *UpdateRewriter) Rewrite( queries := []*gql.GraphQuery{upsertQuery} if setArg != nil { - urw.setFrags = append(urw.setFrags, setFrag...) addUpdateCondition(setFrag) var errSet error mutSet, errSet = mutationsFromFragments( @@ -439,6 +438,7 @@ func (urw *UpdateRewriter) Rewrite( }) if keepError { + urw.setFrags = append(urw.setFrags, setFrag...) errs = schema.AppendGQLErrs(errs, errSet) } @@ -449,11 +449,10 @@ func (urw *UpdateRewriter) Rewrite( } if delArg != nil { - urw.delFrags = append(urw.delFrags, delFrag...) addUpdateCondition(delFrag) var errDel error mutDel, errDel = mutationsFromFragments( - urw.delFrags, + delFrag, func(frag *mutationFragment) ([]byte, error) { return nil, nil }, @@ -462,6 +461,7 @@ func (urw *UpdateRewriter) Rewrite( }) if keepError { + urw.delFrags = append(urw.delFrags, delFrag...) errs = schema.AppendGQLErrs(errs, errDel) } From e4cf0b3e186ce48a4389c0e984524335bb6ff85d Mon Sep 17 00:00:00 2001 From: harshil Date: Tue, 2 Jun 2020 18:08:49 +0530 Subject: [PATCH 04/22] fixed nested uid issue --- graphql/resolve/add_mutation_test.yaml | 100 +++++++++++++++++++++++++ graphql/resolve/mutation_rewriter.go | 22 ++++-- graphql/resolve/schema.graphql | 5 ++ 3 files changed, 122 insertions(+), 5 deletions(-) diff --git a/graphql/resolve/add_mutation_test.yaml b/graphql/resolve/add_mutation_test.yaml index 4430ef8a8f4..dd69d69a4a8 100644 --- a/graphql/resolve/add_mutation_test.yaml +++ b/graphql/resolve/add_mutation_test.yaml @@ -1685,6 +1685,106 @@ ] cond: "@if(eq(len(State3), 1))" +- name: "Deep XID Add" + gqlmutation: | + mutation addAuthor($auth: AddLabInput!) { + addLab(input: [$auth]) { + lab { + name + } + } + } + gqlvariables: | + { + "auth": { + "name": "Lab1", + "computers": [{ + "name": "computer1", + "owners": [{ + "name": "owner1" + }] + }] + } + } + dgmutations: + - setjson: | + { + "Computer.name": "computer1", + "dgraph.type": [ + "Computer" + ], + "uid": "_:Computer4" + } + cond: "@if(eq(len(Computer4), 0))" + + - setjson: | + { + "ComputerOwner.name": "owner1", + "dgraph.type": [ + "ComputerOwner" + ], + "uid": "_:ComputerOwner6" + } + cond: "@if(eq(len(ComputerOwner6), 0))" + + dgmutationssec: + - setjson: |- + { + "Lab.computers": [ + { + "Computer.owners": [ + { + "ComputerOwner.computers": { + "uid": "_:Computer4" + }, + "uid": "uid(ComputerOwner6)" + } + ], + "uid": "uid(Computer4)" + } + ], + "Lab.name": "Lab1", + "dgraph.type": [ + "Lab" + ], + "uid": "_:Lab2" + } + cond: "@if(eq(len(Lab2), 0) AND eq(len(Computer4), 1) AND eq(len(ComputerOwner6), 1))" + deletejson: |- + [{ + "Computer.owners":[{ + "uid":"uid(ComputerOwner6)" + }], + "uid":"uid(Computer7)" + }] + + + dgquerysec: |- + query { + Lab2 as Lab2(func: eq(Lab.name, "Lab1")) @filter(type(Lab)) { + uid + } + Computer4 as Computer4(func: eq(Computer.name, "computer1")) @filter(type(Computer)) { + uid + } + ComputerOwner6 as ComputerOwner6(func: eq(ComputerOwner.name, "owner1")) @filter(type(ComputerOwner)) { + uid + } + var(func: uid(ComputerOwner6)) { + Computer7 as ComputerOwner.computers + } + } + + dgquery: |- + query { + Computer4 as Computer4(func: eq(Computer.name, "computer1")) @filter(type(Computer)) { + uid + } + ComputerOwner6 as ComputerOwner6(func: eq(ComputerOwner.name, "owner1")) @filter(type(ComputerOwner)) { + uid + } + } + - name: "Additional Deletes - deep mutation" gqlmutation: | mutation addAuthor($auth: AddAuthorInput!) { diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index c60be032041..12c3ef4f08e 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -956,6 +956,12 @@ func rewriteObject( } } + var additionalFrag []*mutationFragment + + if xidFrag != nil { + results.secondPass = append(results.secondPass, xidFrag) + } + // if this object has an xid, then we don't need to rewrite its children if we have encountered // it earlier if xidString == "" || xidEncounteredFirstTime { @@ -965,6 +971,8 @@ func rewriteObject( fieldDef := typ.Field(field) fieldName := typ.DgraphPredicate(field) + strategy := "squash" + switch val := val.(type) { case map[string]interface{}: // This field is another GraphQL object, which could either be linking to an @@ -978,6 +986,8 @@ func rewriteObject( rewriteObject(fieldDef.Type(), fieldDef, myUID, varGen, withAdditionalDeletes, val, xidMetadata) + strategy = "merge1" + case []interface{}: // This field is either: // 1) A list of objects: e.g. if the schema said `categories: [Categories]` @@ -993,6 +1003,7 @@ func rewriteObject( frags = rewriteList(fieldDef.Type(), fieldDef, myUID, varGen, withAdditionalDeletes, val, xidMetadata) + strategy = "merge2" default: // This field is either: // 1) a scalar value: e.g. @@ -1009,14 +1020,16 @@ func rewriteObject( } } - results.firstPass = squashFragments(squashIntoObject(fieldName), results.firstPass, frags.firstPass) + if strategy == "squash" { + results.firstPass = squashFragments(squashIntoObject(fieldName), results.firstPass, frags.firstPass) + } else { + additionalFrag = appendFragments(additionalFrag, frags.firstPass) + } results.secondPass = squashFragments(squashIntoObject(fieldName), results.secondPass, frags.secondPass) } } - if xidFrag != nil { - results.secondPass = append(results.secondPass, xidFrag) - } + results.firstPass = append(results.firstPass, additionalFrag...) if xid != nil && !atTopLevel && !xidEncounteredFirstTime { results.firstPass = []*mutationFragment{} @@ -1542,6 +1555,5 @@ func squashFragments( } result[0].queries = queries - return result } diff --git a/graphql/resolve/schema.graphql b/graphql/resolve/schema.graphql index 0a2113b07d4..52b8fa8b9db 100644 --- a/graphql/resolve/schema.graphql +++ b/graphql/resolve/schema.graphql @@ -102,6 +102,11 @@ type MovieDirector { directed: [Movie] @dgraph(pred: "directed.movies") } +type Lab { + name: String! @id + computers: [Computer] +} + # just for testing XID remove in list type Computer { owners: [ComputerOwner!] From 653f2e06a200d3a7a221382ce05d5250747217a8 Mon Sep 17 00:00:00 2001 From: harshil Date: Wed, 3 Jun 2020 19:46:08 +0530 Subject: [PATCH 05/22] fixed rewrite issue --- graphql/resolve/add_mutation_test.yaml | 73 ++++++++++---------- graphql/resolve/mutation_rewriter.go | 83 +++++++++++++++++++---- graphql/resolve/mutation_test.go | 4 ++ graphql/resolve/update_mutation_test.yaml | 3 +- 4 files changed, 111 insertions(+), 52 deletions(-) diff --git a/graphql/resolve/add_mutation_test.yaml b/graphql/resolve/add_mutation_test.yaml index dd69d69a4a8..54fd935b2ab 100644 --- a/graphql/resolve/add_mutation_test.yaml +++ b/graphql/resolve/add_mutation_test.yaml @@ -1629,8 +1629,8 @@ } } gqlvariables: | - { - "inp": { + { + "inp": { "name": "A Country", "states": [ { "code": "abc", "name": "Alphabet" } ] } @@ -1665,7 +1665,7 @@ } dgmutationssec: - setjson: | - { + { "uid" : "_:Country1", "dgraph.type": ["Country"], "Country.name": "A Country", @@ -1687,8 +1687,8 @@ - name: "Deep XID Add" gqlmutation: | - mutation addAuthor($auth: AddLabInput!) { - addLab(input: [$auth]) { + mutation addLab($lab: AddLabInput!) { + addLab(input: [$lab]) { lab { name } @@ -1696,7 +1696,7 @@ } gqlvariables: | { - "auth": { + "lab": { "name": "Lab1", "computers": [{ "name": "computer1", @@ -1719,45 +1719,45 @@ - setjson: | { - "ComputerOwner.name": "owner1", - "dgraph.type": [ - "ComputerOwner" - ], - "uid": "_:ComputerOwner6" + "ComputerOwner.computers":{"uid":"_:Computer4"}, + "ComputerOwner.name":"owner1", + "dgraph.type":["ComputerOwner"], + "uid":"_:ComputerOwner6" } - cond: "@if(eq(len(ComputerOwner6), 0))" + cond: "@if(eq(len(ComputerOwner6), 0) AND eq(len(Computer4), 0))" + + - setjson: | + { + "ComputerOwner.computers": { + "uid": "_:Computer4" + }, + "uid": "uid(ComputerOwner6)" + } + deletejson: |- + [{ + "Computer.owners": [ + { + "uid": "uid(ComputerOwner6)" + } + ], + "uid": "uid(Computer7)" + }] + cond: "@if(eq(len(ComputerOwner6), 1) AND eq(len(Computer4), 0))" dgmutationssec: - setjson: |- { "Lab.computers": [ { - "Computer.owners": [ - { - "ComputerOwner.computers": { - "uid": "_:Computer4" - }, - "uid": "uid(ComputerOwner6)" - } - ], + "Computer.owners": [], "uid": "uid(Computer4)" } ], "Lab.name": "Lab1", - "dgraph.type": [ - "Lab" - ], + "dgraph.type": ["Lab"], "uid": "_:Lab2" } - cond: "@if(eq(len(Lab2), 0) AND eq(len(Computer4), 1) AND eq(len(ComputerOwner6), 1))" - deletejson: |- - [{ - "Computer.owners":[{ - "uid":"uid(ComputerOwner6)" - }], - "uid":"uid(Computer7)" - }] - + cond: "@if(eq(len(Lab2), 0) AND eq(len(Computer4), 1))" dgquerysec: |- query { @@ -1767,12 +1767,6 @@ Computer4 as Computer4(func: eq(Computer.name, "computer1")) @filter(type(Computer)) { uid } - ComputerOwner6 as ComputerOwner6(func: eq(ComputerOwner.name, "owner1")) @filter(type(ComputerOwner)) { - uid - } - var(func: uid(ComputerOwner6)) { - Computer7 as ComputerOwner.computers - } } dgquery: |- @@ -1783,6 +1777,9 @@ ComputerOwner6 as ComputerOwner6(func: eq(ComputerOwner.name, "owner1")) @filter(type(ComputerOwner)) { uid } + var(func: uid(ComputerOwner6)) { + Computer7 as ComputerOwner.computers + } } - name: "Additional Deletes - deep mutation" diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index 12c3ef4f08e..d5380d3e987 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -26,6 +26,7 @@ import ( dgoapi "github.com/dgraph-io/dgo/v2/protos/api" "github.com/dgraph-io/dgraph/gql" + "github.com/dgraph-io/dgraph/graphql/dgraph" "github.com/dgraph-io/dgraph/graphql/schema" "github.com/dgraph-io/dgraph/x" "github.com/pkg/errors" @@ -96,6 +97,30 @@ func NewVariableGenerator() *VariableGenerator { } } +func (mf *mutationFragment) print() { + fmt.Println("------------") + fmt.Println(dgraph.AsString(&gql.GraphQuery{Children: mf.queries})) + fmt.Println("Condition: ", mf.conditions) + byt, _ := json.MarshalIndent(mf.fragment, "", " ") + fmt.Println("Frag:", string(byt)) + byt, _ = json.MarshalIndent(mf.deletes, "", " ") + fmt.Println("Dels:", string(byt)) + fmt.Println("------------") +} + +func (mr *mutationRes) print() { + fmt.Println("======first pass======") + for _, i := range mr.firstPass { + i.print() + } + fmt.Println("======second pass======") + for _, i := range mr.secondPass { + i.print() + } + fmt.Println("======end pass======") + +} + // Next gets the Next variable name for the given type and xid. // So, if two objects of the same type have same value for xid field, // then they will get same variable name. @@ -235,7 +260,7 @@ func (mrw *AddRewriter) Rewrite(ctx context.Context, m schema.Mutation) ([]*Upse varGen := NewVariableGenerator() val := m.ArgValue(schema.InputArgName).(map[string]interface{}) xidMd := newXidMetadata() - obj := rewriteObject(mutatedType, nil, "", varGen, true, val, xidMd) + obj := rewriteObject(mutatedType, nil, "", varGen, true, val, 0, xidMd) mrw.frags = [][]*mutationFragment{obj.get()} mutations, err := mutationsFromFragments( mrw.frags[0], @@ -299,7 +324,8 @@ func (mrw *AddRewriter) handleMultipleMutations(m schema.Mutation) ([]*UpsertMut for _, i := range val { obj := i.(map[string]interface{}) - frag := rewriteObject(mutatedType, nil, "", varGen, true, obj, xidMd) + frag := rewriteObject(mutatedType, nil, "", varGen, true, obj, 0, xidMd) + frag.print() mrw.frags = append(mrw.frags, frag.secondPass) mutationsAll = buildMutations(mutationsAll, queries, frag.firstPass, false) @@ -481,14 +507,14 @@ func (urw *UpdateRewriter) Rewrite( if setArg != nil { setFrag := rewriteObject(mutatedType, nil, srcUID, varGen, true, - setArg.(map[string]interface{}), xidMd) + setArg.(map[string]interface{}), 0, xidMd) setFragF = setFrag.firstPass setFragS = setFrag.secondPass } if delArg != nil { delFrag := rewriteObject(mutatedType, nil, srcUID, varGen, false, - delArg.(map[string]interface{}), xidMd) + delArg.(map[string]interface{}), 0, xidMd) delFragF = delFrag.firstPass delFragS = delFrag.secondPass } @@ -801,6 +827,7 @@ func rewriteObject( varGen *VariableGenerator, withAdditionalDeletes bool, obj map[string]interface{}, + deepXID int, xidMetadata *xidMetadata) *mutationRes { atTopLevel := srcField == nil @@ -864,6 +891,8 @@ func rewriteObject( xidMetadata.seenAtTopLevel[variable] = atTopLevel } } + + deepXID += 1 } if !atTopLevel { // top level is never a reference - it's adding/updating @@ -918,7 +947,7 @@ func rewriteObject( newObj["dgraph.type"] = dgraphTypes myUID = fmt.Sprintf("_:%s", variable) - if xid == nil { + if xid == nil || deepXID > 1 { addInverseLink(newObj, srcField, srcUID) } } else { @@ -949,7 +978,7 @@ func rewriteObject( results = &mutationRes{ firstPass: []*mutationFragment{frag}, } - if xidFrag != nil { + if xidFrag != nil && deepXID <= 2 { frag.queries = []*gql.GraphQuery{ xidQuery(variable, xidString, xid.Name(), typ), } @@ -958,7 +987,7 @@ func rewriteObject( var additionalFrag []*mutationFragment - if xidFrag != nil { + if xidFrag != nil && deepXID <= 2 { results.secondPass = append(results.secondPass, xidFrag) } @@ -984,9 +1013,8 @@ func rewriteObject( // like here ^^ frags = rewriteObject(fieldDef.Type(), fieldDef, myUID, varGen, withAdditionalDeletes, - val, xidMetadata) - - strategy = "merge1" + val, deepXID, xidMetadata) + strategy = "merge" case []interface{}: // This field is either: @@ -1002,8 +1030,8 @@ func rewriteObject( // like here ^^ frags = rewriteList(fieldDef.Type(), fieldDef, myUID, varGen, withAdditionalDeletes, val, - xidMetadata) - strategy = "merge2" + deepXID, xidMetadata) + strategy = "merge" default: // This field is either: // 1) a scalar value: e.g. @@ -1029,12 +1057,40 @@ func rewriteObject( } } + //fmt.Println("") + //fmt.Println("") + //fmt.Println("") + + //fmt.Println(obj) + //fmt.Println("=========PRE=========") + //results.print() + + conditions := []string{} + + for _, i := range results.firstPass { + conditions = append(conditions, i.conditions...) + } + + for _, i := range additionalFrag { + i.conditions = append(i.conditions, conditions...) + } + results.firstPass = append(results.firstPass, additionalFrag...) + if xidFrag != nil && deepXID > 2 { + results.firstPass = append(results.firstPass, xidFrag) + } if xid != nil && !atTopLevel && !xidEncounteredFirstTime { results.firstPass = []*mutationFragment{} } + //fmt.Println("=========POST=========") + //results.print() + + //fmt.Println("") + //fmt.Println("") + //fmt.Println("") + return results } @@ -1368,6 +1424,7 @@ func rewriteList( varGen *VariableGenerator, withAdditionalDeletes bool, objects []interface{}, + deepXID int, xidMetadata *xidMetadata) *mutationRes { result := &mutationRes{} @@ -1377,7 +1434,7 @@ func rewriteList( for _, obj := range objects { switch obj := obj.(type) { case map[string]interface{}: - frag := rewriteObject(typ, srcField, srcUID, varGen, withAdditionalDeletes, obj, xidMetadata) + frag := rewriteObject(typ, srcField, srcUID, varGen, withAdditionalDeletes, obj, deepXID, xidMetadata) result.firstPass = appendFragments(result.firstPass, frag.firstPass) result.secondPass = squashFragments(squashIntoList, result.secondPass, frag.secondPass) default: diff --git a/graphql/resolve/mutation_test.go b/graphql/resolve/mutation_test.go index 1ea0aeeb260..622e9b28c27 100644 --- a/graphql/resolve/mutation_test.go +++ b/graphql/resolve/mutation_test.go @@ -19,6 +19,7 @@ package resolve import ( "context" "encoding/json" + "fmt" "io/ioutil" "strings" "testing" @@ -118,11 +119,14 @@ func mutationRewriting(t *testing.T, file string, rewriterFactory func() Mutatio compareMutations := func(t *testing.T, test []*dgraphMutation, generated []*dgoapi.Mutation) { require.Len(t, generated, len(test)) for i, expected := range test { + fmt.Println(i) require.Equal(t, expected.Cond, generated[i].Cond) if len(generated[i].SetJson) > 0 || expected.SetJSON != "" { + fmt.Println(string(generated[i].SetJson)) require.JSONEq(t, expected.SetJSON, string(generated[i].SetJson)) } if len(generated[i].DeleteJson) > 0 || expected.DeleteJSON != "" { + fmt.Println(string(generated[i].DeleteJson)) require.JSONEq(t, expected.DeleteJSON, string(generated[i].DeleteJson)) } } diff --git a/graphql/resolve/update_mutation_test.yaml b/graphql/resolve/update_mutation_test.yaml index 226f9d29f59..cc320ea6720 100644 --- a/graphql/resolve/update_mutation_test.yaml +++ b/graphql/resolve/update_mutation_test.yaml @@ -1349,7 +1349,8 @@ { "uid": "_:Computer3", "dgraph.type": ["Computer"], - "Computer.name": "Comp" + "Computer.name": "Comp", + "Computer.owners":[ { "uid":"uid(x)" } ] } cond: "@if(eq(len(Computer3), 0) AND gt(len(x), 0))" dgmutationssec: From eb77dcafb296ec0649153ec9b9128726896f9649 Mon Sep 17 00:00:00 2001 From: harshil Date: Mon, 8 Jun 2020 17:21:35 +0530 Subject: [PATCH 06/22] fixed commnets --- graphql/resolve/add_mutation_test.yaml | 14 +++++++ graphql/resolve/mutation.go | 23 ++++++----- graphql/resolve/mutation_rewriter.go | 57 +++++++++++--------------- graphql/resolve/mutation_test.go | 11 +++-- 4 files changed, 57 insertions(+), 48 deletions(-) diff --git a/graphql/resolve/add_mutation_test.yaml b/graphql/resolve/add_mutation_test.yaml index 54fd935b2ab..e24fac9cccd 100644 --- a/graphql/resolve/add_mutation_test.yaml +++ b/graphql/resolve/add_mutation_test.yaml @@ -1744,6 +1744,20 @@ }] cond: "@if(eq(len(ComputerOwner6), 1) AND eq(len(Computer4), 0))" + - setjson: | + { + "Computer.owners":[{"uid":"uid(ComputerOwner6)"}], + "uid":"_:Computer4" + } + cond: "@if(eq(len(ComputerOwner6), 1) AND eq(len(Computer4), 0))" + + - setjson: | + { + "Computer.owners":[{"uid":"_:ComputerOwner6"}], + "uid":"_:Computer4" + } + cond: "@if(eq(len(ComputerOwner6), 0) AND eq(len(Computer4), 0))" + dgmutationssec: - setjson: |- { diff --git a/graphql/resolve/mutation.go b/graphql/resolve/mutation.go index 68a70630094..0634d1b6c77 100644 --- a/graphql/resolve/mutation.go +++ b/graphql/resolve/mutation.go @@ -216,9 +216,6 @@ func (mr *dgraphResolver) rewriteAndExecute( req := &dgoapi.Request{} for _, upsert := range upserts { - if len(upsert.Mutations) == 0 { - continue - } req.Query = dgraph.AsString(upsert.Query) req.Mutations = upsert.Mutations @@ -247,15 +244,19 @@ func (mr *dgraphResolver) rewriteAndExecute( return emptyResult(errs), resolverFailed } - if mutResp != nil { - err = mr.executor.CommitOrAbort(ctx, mutResp.Txn) - if err != nil { - return emptyResult( - schema.GQLWrapf(err, "mutation failed, couldn't commit transaction")), - resolverFailed - } - commit = true + if mutResp == nil { + return emptyResult( + schema.GQLWrapf(err, "mutation failed, couldn't get result")), + resolverFailed + } + + err = mr.executor.CommitOrAbort(ctx, mutResp.Txn) + if err != nil { + return emptyResult( + schema.GQLWrapf(err, "mutation failed, couldn't commit transaction")), + resolverFailed } + commit = true qryResp, err := mr.executor.Execute(ctx, &dgoapi.Request{Query: dgraph.AsString(dgQuery), ReadOnly: true}) diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index d5380d3e987..36be6c74055 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -252,38 +252,6 @@ func newXidMetadata() *xidMetadata { // } func (mrw *AddRewriter) Rewrite(ctx context.Context, m schema.Mutation) ([]*UpsertMutation, error) { mutatedType := m.MutatedType() - - if m.IsArgListType(schema.InputArgName) { - return mrw.handleMultipleMutations(m) - } - - varGen := NewVariableGenerator() - val := m.ArgValue(schema.InputArgName).(map[string]interface{}) - xidMd := newXidMetadata() - obj := rewriteObject(mutatedType, nil, "", varGen, true, val, 0, xidMd) - mrw.frags = [][]*mutationFragment{obj.get()} - mutations, err := mutationsFromFragments( - mrw.frags[0], - func(frag *mutationFragment) ([]byte, error) { - return json.Marshal(frag.fragment) - }, - func(frag *mutationFragment) ([]byte, error) { - if len(frag.deletes) > 0 { - return json.Marshal(frag.deletes) - } - return nil, nil - }) - - upsert := &UpsertMutation{ - Query: queryFromFragments(mrw.frags[0]), - Mutations: mutations, - } - - return []*UpsertMutation{upsert}, schema.GQLWrapf(err, "failed to rewrite mutation payload") -} - -func (mrw *AddRewriter) handleMultipleMutations(m schema.Mutation) ([]*UpsertMutation, error) { - mutatedType := m.MutatedType() val, _ := m.ArgValue(schema.InputArgName).([]interface{}) varGen := NewVariableGenerator() @@ -325,7 +293,7 @@ func (mrw *AddRewriter) handleMultipleMutations(m schema.Mutation) ([]*UpsertMut for _, i := range val { obj := i.(map[string]interface{}) frag := rewriteObject(mutatedType, nil, "", varGen, true, obj, 0, xidMd) - frag.print() + //frag.print() mrw.frags = append(mrw.frags, frag.secondPass) mutationsAll = buildMutations(mutationsAll, queries, frag.firstPass, false) @@ -895,10 +863,21 @@ func rewriteObject( deepXID += 1 } + var parentFrags []*mutationFragment + if !atTopLevel { // top level is never a reference - it's adding/updating if xid != nil && xidString != "" { xidFrag = asXIDReference(srcField, srcUID, typ, xid.Name(), xidString, variable, withAdditionalDeletes, varGen, xidMetadata) + if deepXID > 2 { + res := make(map[string]interface{}, 1) + res["uid"] = srcUID + addInverseLink(res, srcField.Inverse(), fmt.Sprintf("uid(%s)", variable)) + + parentFrag := newFragment(res) + parentFrag.conditions = append(parentFrag.conditions, xidFrag.conditions...) + parentFrags = append(parentFrags, parentFrag) + } } else if !withAdditionalDeletes { // In case of delete, id/xid is required var name string @@ -978,10 +957,18 @@ func rewriteObject( results = &mutationRes{ firstPass: []*mutationFragment{frag}, } - if xidFrag != nil && deepXID <= 2 { + if deepXID <= 2 { frag.queries = []*gql.GraphQuery{ xidQuery(variable, xidString, xid.Name(), typ), } + } else { + res := make(map[string]interface{}, 1) + res["uid"] = srcUID + addInverseLink(res, srcField.Inverse(), fmt.Sprintf("_:%s", variable)) + + parentFrag := newFragment(res) + parentFrag.conditions = append(parentFrag.conditions, frag.conditions...) + parentFrags = append(parentFrags, parentFrag) } } @@ -1080,6 +1067,8 @@ func rewriteObject( results.firstPass = append(results.firstPass, xidFrag) } + results.firstPass = append(results.firstPass, parentFrags...) + if xid != nil && !atTopLevel && !xidEncounteredFirstTime { results.firstPass = []*mutationFragment{} } diff --git a/graphql/resolve/mutation_test.go b/graphql/resolve/mutation_test.go index 622e9b28c27..a8cfe6bc1a2 100644 --- a/graphql/resolve/mutation_test.go +++ b/graphql/resolve/mutation_test.go @@ -117,16 +117,19 @@ func mutationRewriting(t *testing.T, file string, rewriterFactory func() Mutatio gqlSchema := test.LoadSchemaFromFile(t, "schema.graphql") compareMutations := func(t *testing.T, test []*dgraphMutation, generated []*dgoapi.Mutation) { + for _, i := range generated { + fmt.Println(string(i.SetJson)) + fmt.Println(string(i.DeleteJson)) + fmt.Println(i.Cond) + } + require.Len(t, generated, len(test)) for i, expected := range test { - fmt.Println(i) require.Equal(t, expected.Cond, generated[i].Cond) if len(generated[i].SetJson) > 0 || expected.SetJSON != "" { - fmt.Println(string(generated[i].SetJson)) require.JSONEq(t, expected.SetJSON, string(generated[i].SetJson)) } if len(generated[i].DeleteJson) > 0 || expected.DeleteJSON != "" { - fmt.Println(string(generated[i].DeleteJson)) require.JSONEq(t, expected.DeleteJSON, string(generated[i].DeleteJson)) } } @@ -161,10 +164,12 @@ func mutationRewriting(t *testing.T, file string, rewriterFactory func() Mutatio return } + fmt.Println(dgraph.AsString(upsert[0].Query)) require.Equal(t, tcase.DGQuery, dgraph.AsString(upsert[0].Query)) compareMutations(t, tcase.DGMutations, upsert[0].Mutations) if len(upsert) > 1 { + fmt.Println(dgraph.AsString(upsert[1].Query)) require.Equal(t, tcase.DGQuerySec, dgraph.AsString(upsert[1].Query)) compareMutations(t, tcase.DGMutationsSec, upsert[1].Mutations) } From 1550a4f3ed56fc65dd69c8630e250b588868dced Mon Sep 17 00:00:00 2001 From: harshil Date: Mon, 8 Jun 2020 19:02:17 +0530 Subject: [PATCH 07/22] removed print statements --- graphql/resolve/mutation_rewriter.go | 41 ---------------------------- 1 file changed, 41 deletions(-) diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index 36be6c74055..948c6b275e0 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -26,7 +26,6 @@ import ( dgoapi "github.com/dgraph-io/dgo/v2/protos/api" "github.com/dgraph-io/dgraph/gql" - "github.com/dgraph-io/dgraph/graphql/dgraph" "github.com/dgraph-io/dgraph/graphql/schema" "github.com/dgraph-io/dgraph/x" "github.com/pkg/errors" @@ -97,30 +96,6 @@ func NewVariableGenerator() *VariableGenerator { } } -func (mf *mutationFragment) print() { - fmt.Println("------------") - fmt.Println(dgraph.AsString(&gql.GraphQuery{Children: mf.queries})) - fmt.Println("Condition: ", mf.conditions) - byt, _ := json.MarshalIndent(mf.fragment, "", " ") - fmt.Println("Frag:", string(byt)) - byt, _ = json.MarshalIndent(mf.deletes, "", " ") - fmt.Println("Dels:", string(byt)) - fmt.Println("------------") -} - -func (mr *mutationRes) print() { - fmt.Println("======first pass======") - for _, i := range mr.firstPass { - i.print() - } - fmt.Println("======second pass======") - for _, i := range mr.secondPass { - i.print() - } - fmt.Println("======end pass======") - -} - // Next gets the Next variable name for the given type and xid. // So, if two objects of the same type have same value for xid field, // then they will get same variable name. @@ -293,7 +268,6 @@ func (mrw *AddRewriter) Rewrite(ctx context.Context, m schema.Mutation) ([]*Upse for _, i := range val { obj := i.(map[string]interface{}) frag := rewriteObject(mutatedType, nil, "", varGen, true, obj, 0, xidMd) - //frag.print() mrw.frags = append(mrw.frags, frag.secondPass) mutationsAll = buildMutations(mutationsAll, queries, frag.firstPass, false) @@ -1044,14 +1018,6 @@ func rewriteObject( } } - //fmt.Println("") - //fmt.Println("") - //fmt.Println("") - - //fmt.Println(obj) - //fmt.Println("=========PRE=========") - //results.print() - conditions := []string{} for _, i := range results.firstPass { @@ -1073,13 +1039,6 @@ func rewriteObject( results.firstPass = []*mutationFragment{} } - //fmt.Println("=========POST=========") - //results.print() - - //fmt.Println("") - //fmt.Println("") - //fmt.Println("") - return results } From 816aa7380be4d5fc5888d27a4f75356187975b44 Mon Sep 17 00:00:00 2001 From: harshil Date: Wed, 10 Jun 2020 02:14:22 +0530 Subject: [PATCH 08/22] fixed some tests --- graphql/e2e/common/common.go | 1 + graphql/e2e/common/mutation.go | 83 ++++++++++++++- graphql/e2e/normal/schema.graphql | 2 +- graphql/resolve/add_mutation_test.yaml | 104 +++++++++++++++++- graphql/resolve/mutation_rewriter.go | 124 +++++++++++++++++++--- graphql/resolve/mutation_test.go | 2 + graphql/resolve/schema.graphql | 2 +- graphql/resolve/update_mutation_test.yaml | 6 ++ 8 files changed, 301 insertions(+), 23 deletions(-) diff --git a/graphql/e2e/common/common.go b/graphql/e2e/common/common.go index afb86319f2b..d01e891601c 100644 --- a/graphql/e2e/common/common.go +++ b/graphql/e2e/common/common.go @@ -298,6 +298,7 @@ func RunAll(t *testing.T) { t.Run("password in mutation", passwordTest) t.Run("duplicate xid in single mutation", deepMutationDuplicateXIDsSameObjectTest) t.Run("alias works for mutations", mutationsWithAlias) + t.Run("three level deep", threeLevelDeepMutation) // error tests t.Run("graphql completion on", graphQLCompletionOn) diff --git a/graphql/e2e/common/mutation.go b/graphql/e2e/common/mutation.go index c7ddba99644..1a4fb941691 100644 --- a/graphql/e2e/common/mutation.go +++ b/graphql/e2e/common/mutation.go @@ -1292,8 +1292,8 @@ func updateMutationOnlyUpdatesRefsIfDifferent(t *testing.T, executeRequest reque set: $set } ) { - post { - postID + post { + postID text author { id } } @@ -1313,9 +1313,9 @@ func updateMutationOnlyUpdatesRefsIfDifferent(t *testing.T, executeRequest reque // The text is updated as expected // The author is unchanged expected := fmt.Sprintf(` - { "updatePost": { "post": [ - { - "postID": "%s", + { "updatePost": { "post": [ + { + "postID": "%s", "text": "The Updated Text", "author": { "id": "%s" } } @@ -2863,6 +2863,79 @@ func passwordTest(t *testing.T) { deleteUser(t, *newUser) } +func threeLevelDeepMutation(t *testing.T) { + newStudent := &student{ + Xid: "HS1", + Name: "Stud1", + TaughtBy: []*teacher{ + { + Xid: "HT0", + Name: "Teacher0", + Subject: "English", + Teaches: []*student{{ + Xid: "HS2", + Name: "Stud2", + }}, + }, + }, + } + + newStudents := []*student{newStudent} + + addStudentParams := &GraphQLParams{ + Query: `mutation addStudent($input: [AddStudentInput!]!) { + addStudent(input: $input) { + student { + xid + name + taughtBy { + id + xid + name + subject + teaches (order: {asc:xid}) { + xid + taughtBy { + name + xid + subject + } + } + } + } + } + }`, + Variables: map[string]interface{}{"input": newStudents}, + } + + gqlResponse := postExecutor(t, graphqlURL, addStudentParams) + fmt.Println(string(gqlResponse.Data)) + requireNoGQLErrors(t, gqlResponse) + + var actualResult struct { + AddStudent struct { + Student []*student + } + } + + err := json.Unmarshal(gqlResponse.Data, &actualResult) + require.NoError(t, err) + + require.Equal(t, actualResult.AddStudent.Student[0].Xid, "HS1") + require.Equal(t, actualResult.AddStudent.Student[0].TaughtBy[0].Xid, "HT0") + require.Equal(t, actualResult.AddStudent.Student[0].TaughtBy[0].Teaches[0].Xid, "HS1") + require.Equal(t, actualResult.AddStudent.Student[0].TaughtBy[0].Teaches[0].TaughtBy[0].Xid, "HT0") + require.Equal(t, actualResult.AddStudent.Student[0].TaughtBy[0].Teaches[1].Xid, "HS2") + require.Equal(t, actualResult.AddStudent.Student[0].TaughtBy[0].Teaches[1].TaughtBy[0].Xid, "HT0") + + // cleanup + filter := getXidFilter("xid", []string{"HS1", "HS2"}) + deleteGqlType(t, "Student", filter, 2, nil) + filter = getXidFilter("xid", []string{"HT0"}) + deleteGqlType(t, "Teacher", filter, 1, nil) + +} + func deepMutationDuplicateXIDsSameObjectTest(t *testing.T) { newStudents := []*student{ { diff --git a/graphql/e2e/normal/schema.graphql b/graphql/e2e/normal/schema.graphql index 14141a87a74..ce7037b6013 100644 --- a/graphql/e2e/normal/schema.graphql +++ b/graphql/e2e/normal/schema.graphql @@ -136,6 +136,6 @@ type Teacher implements People { } type Student implements People { - taughtBy: [Teacher] + taughtBy: [Teacher] @hasInverse(field: teaches) } diff --git a/graphql/resolve/add_mutation_test.yaml b/graphql/resolve/add_mutation_test.yaml index 35741ef09e6..3f7f2629294 100644 --- a/graphql/resolve/add_mutation_test.yaml +++ b/graphql/resolve/add_mutation_test.yaml @@ -1705,7 +1705,108 @@ ] cond: "@if(eq(len(State3), 1))" -- name: "Deep XID Add" +- name: "Deep XID Add top level hasInverse" + gqlmutation: | + mutation addStudent($student: AddStudentInput!) { + addStudent(input: [$student]) { + student { + name + } + } + } + gqlvariables: | + { + "student": { + "xid": "S0", + "name": "Student0", + "taughtBy": [{ + "xid": "T0", + "name": "teacher0", + "teaches": [{ + "xid": "S1", + "name": "Student1" + }] + }] + } + } + dgquery: |- + query { + Teacher4 as Teacher4(func: eq(People.xid, "T0")) @filter(type(Teacher)) { + uid + } + Student6 as Student6(func: eq(People.xid, "S1")) @filter(type(Student)) { + uid + } + } + + dgmutations: + - setjson: | + { + "People.name":"teacher0", + "People.xid":"T0", + "dgraph.type":["Teacher","People"], + "uid":"_:Teacher4" + } + + cond: "@if(eq(len(Teacher4), 0))" + + - setjson: | + { + "People.name":"Student1", + "People.xid":"S1", + "Student.taughtBy":[{"uid":"_:Teacher4"}], + "dgraph.type":["Student","People"], + "uid":"_:Student6" + } + cond: "@if(eq(len(Student6), 0) AND eq(len(Teacher4), 0))" + + - setjson: | + { + "Student.taughtBy":[{"uid":"_:Teacher4"}], + "uid":"uid(Student6)" + } + deletejson: | + cond: "@if(eq(len(Student6), 1) AND eq(len(Teacher4), 0))" + + - setjson: | + { + "Teacher.teaches":[{"uid":"uid(Student6)"}], + "uid":"_:Teacher4" + } + deletejson: | + cond: "@if(eq(len(Student6), 1) AND eq(len(Teacher4), 0))" + + - setjson: | + { + "Teacher.teaches":[{"uid":"_:Student6"}], + "uid":"_:Teacher4" + } + deletejson: | + cond: "@if(eq(len(Student6), 0) AND eq(len(Teacher4), 0))" + + dgquerysec: |- + query { + Student2 as Student2(func: eq(People.xid, "S0")) @filter(type(Student)) { + uid + } + Teacher4 as Teacher4(func: eq(People.xid, "T0")) @filter(type(Teacher)) { + uid + } + } + + dgmutationssec: + - setjson: | + { + "People.name":"Student0", + "People.xid":"S0", + "Student.taughtBy":[{"Teacher.teaches":[{"uid":"_:Student2"}],"uid":"uid(Teacher4)"}], + "dgraph.type":["Student","People"], + "uid":"_:Student2" + } + cond: "@if(eq(len(Student2), 0) AND eq(len(Teacher4), 1))" + + +- name: "Deep XID Add lower level hasInvsere" gqlmutation: | mutation addLab($lab: AddLabInput!) { addLab(input: [$lab]) { @@ -1783,7 +1884,6 @@ { "Lab.computers": [ { - "Computer.owners": [], "uid": "uid(Computer4)" } ], diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index 8cb64f6f78a..a8f89016390 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -26,6 +26,7 @@ import ( dgoapi "github.com/dgraph-io/dgo/v2/protos/api" "github.com/dgraph-io/dgraph/gql" + "github.com/dgraph-io/dgraph/graphql/dgraph" "github.com/dgraph-io/dgraph/graphql/schema" "github.com/dgraph-io/dgraph/x" "github.com/pkg/errors" @@ -96,6 +97,30 @@ func NewVariableGenerator() *VariableGenerator { } } +func (mf *mutationFragment) print() { + fmt.Println("------------") + fmt.Println(dgraph.AsString(&gql.GraphQuery{Children: mf.queries})) + fmt.Println("Condition: ", mf.conditions) + byt, _ := json.MarshalIndent(mf.fragment, "", " ") + fmt.Println("Frag:", string(byt)) + byt, _ = json.MarshalIndent(mf.deletes, "", " ") + fmt.Println("Dels:", string(byt)) + fmt.Println("------------") +} + +func (mr *mutationRes) print() { + fmt.Println("======first pass======") + for _, i := range mr.firstPass { + i.print() + } + fmt.Println("======second pass======") + for _, i := range mr.secondPass { + i.print() + } + fmt.Println("======end pass======") + +} + // Next gets the Next variable name for the given type and xid. // So, if two objects of the same type have same value for xid field, // then they will get same variable name. @@ -268,6 +293,7 @@ func (mrw *AddRewriter) Rewrite(ctx context.Context, m schema.Mutation) ([]*Upse for _, i := range val { obj := i.(map[string]interface{}) frag := rewriteObject(mutatedType, nil, "", varGen, true, obj, 0, xidMd) + frag.print() mrw.frags = append(mrw.frags, frag.secondPass) mutationsAll = buildMutations(mutationsAll, queries, frag.firstPass, false) @@ -405,8 +431,8 @@ func (urw *UpdateRewriter) Rewrite( return nil, nil }) + urw.setFrags = append(urw.setFrags, setFrag...) if keepError { - urw.setFrags = append(urw.setFrags, setFrag...) errs = schema.AppendGQLErrs(errs, errSet) } @@ -428,8 +454,8 @@ func (urw *UpdateRewriter) Rewrite( return json.Marshal(frag.fragment) }) + urw.delFrags = append(urw.delFrags, delFrag...) if keepError { - urw.delFrags = append(urw.delFrags, delFrag...) errs = schema.AppendGQLErrs(errs, errDel) } @@ -450,6 +476,17 @@ func (urw *UpdateRewriter) Rewrite( if setArg != nil { setFrag := rewriteObject(mutatedType, nil, srcUID, varGen, true, setArg.(map[string]interface{}), 0, xidMd) + + fmt.Println("") + fmt.Println("") + fmt.Println("") + fmt.Println("=======set frags======") + setFrag.print() + fmt.Println("=======set frags end======") + fmt.Println("") + fmt.Println("") + fmt.Println("") + setFragF = setFrag.firstPass setFragS = setFrag.secondPass } @@ -463,12 +500,12 @@ func (urw *UpdateRewriter) Rewrite( result := []*UpsertMutation{} - firstPass := buildMutation(setFragF, delFragF, false) + firstPass := buildMutation(setFragF, delFragF, true) if len(firstPass.Mutations) > 0 { result = append(result, firstPass) } - secondPass := buildMutation(setFragS, delFragS, true) + secondPass := buildMutation(setFragS, delFragS, false) if len(secondPass.Mutations) > 0 { result = append(result, secondPass) } @@ -680,7 +717,8 @@ func asUID(val interface{}) (uint64, error) { func mutationsFromFragments( frags []*mutationFragment, - setBuilder, delBuilder mutationBuilder) ([]*dgoapi.Mutation, error) { + setBuilder mutationBuilder, + delBuilder mutationBuilder) ([]*dgoapi.Mutation, error) { mutations := make([]*dgoapi.Mutation, 0, len(frags)) var errs x.GqlErrorList @@ -798,7 +836,7 @@ func rewriteObject( if !ok { errFrag := newFragment(nil) errFrag.err = errors.New("encountered an XID that isn't a string") - return &mutationRes{secondPass: []*mutationFragment{errFrag}} + return &mutationRes{firstPass: []*mutationFragment{errFrag}} } // if the object has an xid, the variable name will be formed from the xidValue in order // to handle duplicate object addition/updation @@ -820,7 +858,7 @@ func rewriteObject( xidObj, obj) || (invField != nil && invField.Type().ListType() == nil) { errFrag := newFragment(nil) errFrag.err = errors.Errorf("duplicate XID found: %s", xidString) - return &mutationRes{secondPass: []*mutationFragment{errFrag}} + return &mutationRes{firstPass: []*mutationFragment{errFrag}} } } else { // if not encountered till now, add it to the map @@ -859,7 +897,7 @@ func rewriteObject( } else { name = id.Name() } - return &mutationRes{secondPass: invalidObjectFragment(fmt.Errorf("%s is not provided", name), + return &mutationRes{firstPass: invalidObjectFragment(fmt.Errorf("%s is not provided", name), xidFrag, variable, xidString)} } } @@ -899,7 +937,7 @@ func rewriteObject( newObj["dgraph.type"] = dgraphTypes myUID = fmt.Sprintf("_:%s", variable) - if xid == nil || deepXID > 1 { + if xid == nil || deepXID > 2 { addInverseLink(newObj, srcField, srcUID) } } else { @@ -948,7 +986,14 @@ func rewriteObject( var additionalFrag []*mutationFragment if xidFrag != nil && deepXID <= 2 { + fmt.Println("Frag") + xidFrag.print() + fmt.Println("Frag") results.secondPass = append(results.secondPass, xidFrag) + } else if xidFrag != nil { + fmt.Println("Frag E") + xidFrag.print() + fmt.Println("Frag E") } // if this object has an xid, then we don't need to rewrite its children if we have encountered @@ -959,10 +1004,6 @@ func rewriteObject( fieldDef := typ.Field(field) fieldName := typ.DgraphPredicate(field) - // This fixes mutation when dgraph predicate has special characters. PR #5526 - if strings.HasPrefix(fieldName, "<") && strings.HasSuffix(fieldName, ">") { - fieldName = fieldName[1 : len(fieldName)-1] - } strategy := "squash" @@ -1012,12 +1053,39 @@ func rewriteObject( } } + fmt.Println("") + fmt.Println("") + fmt.Println("") + fmt.Println("====frags=====") + fmt.Println(fieldName) + fmt.Println(val) + fmt.Println(strategy) + fmt.Println(obj) + fmt.Println(deepXID) + frags.print() + fmt.Println("====result=====") + results.print() + fmt.Println("====post results=====") + if strategy == "squash" { results.firstPass = squashFragments(squashIntoObject(fieldName), results.firstPass, frags.firstPass) } else { additionalFrag = appendFragments(additionalFrag, frags.firstPass) } - results.secondPass = squashFragments(squashIntoObject(fieldName), results.secondPass, frags.secondPass) + + if xid == nil || (deepXID < 2 && xid != nil) { + results.secondPass = squashFragments(squashIntoObject(fieldName), results.secondPass, frags.secondPass) + } else { + fmt.Println(deepXID) + fmt.Println(xid == nil) + fmt.Println("Here") + } + + results.print() + fmt.Println("====end=====") + fmt.Println("") + fmt.Println("") + fmt.Println("") } } @@ -1421,6 +1489,13 @@ func squashIntoList(list, v interface{}, makeCopy bool) interface{} { func squashIntoObject(label string) func(interface{}, interface{}, bool) interface{} { return func(object, v interface{}, makeCopy bool) interface{} { + fmt.Println("") + fmt.Println("") + fmt.Println("") + fmt.Println("======== squash into object =======") + fmt.Println(object) + fmt.Println(v) + fmt.Println(label) asObject := object.(map[string]interface{}) if makeCopy { cpy := make(map[string]interface{}, len(asObject)+1) @@ -1430,6 +1505,11 @@ func squashIntoObject(label string) func(interface{}, interface{}, bool) interfa asObject = cpy } asObject[label] = v + fmt.Println(asObject) + fmt.Println("======== squash into object =======") + fmt.Println("") + fmt.Println("") + fmt.Println("") return asObject } } @@ -1536,6 +1616,15 @@ func squashFragments( deletes = make([]interface{}, len(l.deletes), len(l.deletes)+len(r.deletes)) copy(deletes, l.deletes) } + fmt.Println("") + fmt.Println("") + fmt.Println("") + fmt.Println("============DEBUUUGGGGG START============") + + fmt.Println("l") + l.print() + fmt.Println("r") + r.print() result = append(result, &mutationFragment{ conditions: append(conds, r.conditions...), @@ -1548,6 +1637,13 @@ func squashFragments( }(l.check, r.check), err: schema.AppendGQLErrs(l.err, r.err), }) + + fmt.Println("result") + result[len(result)-1].print() + fmt.Println("============DEBUUUGGGGG END============") + fmt.Println("") + fmt.Println("") + fmt.Println("") } } diff --git a/graphql/resolve/mutation_test.go b/graphql/resolve/mutation_test.go index a8cfe6bc1a2..fbbc8dd2487 100644 --- a/graphql/resolve/mutation_test.go +++ b/graphql/resolve/mutation_test.go @@ -127,6 +127,8 @@ func mutationRewriting(t *testing.T, file string, rewriterFactory func() Mutatio for i, expected := range test { require.Equal(t, expected.Cond, generated[i].Cond) if len(generated[i].SetJson) > 0 || expected.SetJSON != "" { + fmt.Println(expected.SetJSON) + fmt.Println(string(generated[i].SetJson)) require.JSONEq(t, expected.SetJSON, string(generated[i].SetJson)) } if len(generated[i].DeleteJson) > 0 || expected.DeleteJSON != "" { diff --git a/graphql/resolve/schema.graphql b/graphql/resolve/schema.graphql index fc81e030367..5f3e3e08b28 100644 --- a/graphql/resolve/schema.graphql +++ b/graphql/resolve/schema.graphql @@ -149,7 +149,7 @@ type Teacher implements People { } type Student implements People { - taughtBy: [Teacher] + taughtBy: [Teacher] @hasInverse(field: "teaches") } type Message { diff --git a/graphql/resolve/update_mutation_test.yaml b/graphql/resolve/update_mutation_test.yaml index cc320ea6720..206a00875bc 100644 --- a/graphql/resolve/update_mutation_test.yaml +++ b/graphql/resolve/update_mutation_test.yaml @@ -992,9 +992,15 @@ { "Student.taughtBy": [ { + "Teacher.teaches": [{ + "uid": "uid(x)" + }], "uid": "uid(Teacher3)" }, { + "Teacher.teaches": [{ + "uid": "uid(x)" + }], "uid": "uid(Teacher3)" } ], From 4b274167c57fe9ad089e84cc04722a186cd0866a Mon Sep 17 00:00:00 2001 From: harshil Date: Wed, 10 Jun 2020 02:46:24 +0530 Subject: [PATCH 09/22] added a benchmark --- graphql/resolve/mutation_test.go | 52 ++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/graphql/resolve/mutation_test.go b/graphql/resolve/mutation_test.go index fbbc8dd2487..15c1f56dd6f 100644 --- a/graphql/resolve/mutation_test.go +++ b/graphql/resolve/mutation_test.go @@ -30,6 +30,9 @@ import ( "github.com/dgraph-io/dgraph/graphql/test" "github.com/dgraph-io/dgraph/x" "github.com/stretchr/testify/require" + "github.com/vektah/gqlparser/v2/ast" + "github.com/vektah/gqlparser/v2/parser" + "github.com/vektah/gqlparser/v2/validator" "gopkg.in/yaml.v2" ) @@ -106,6 +109,55 @@ func mutationValidation(t *testing.T, file string, rewriterFactory func() Mutati } } +func benchmark3LevelDeep(num int, b *testing.B) { + gql, _ := ioutil.ReadFile("schema.graphql") + handler, _ := schema.NewHandler(string(gql)) + doc, _ := parser.ParseSchemas(validator.Prelude, &ast.Source{Input: handler.GQLSchema()}) + gqlSch, _ := validator.ValidateSchemaDocument(doc) + gqlSchema := schema.AsSchema(gqlSch) + + innerTeachers := make([]map[string]interface{}, num) + for i := 1; i <= num; i++ { + innerTeachers = append(innerTeachers, map[string]interface{}{ + "xid": fmt.Sprintf("S$%d", i), + "name": fmt.Sprintf("Name%d", i), + }) + } + + vars := map[string]interface{}{ + "input": []map[string]interface{}{{ + "xid": "S0", + "name": "Name0", + "taughtBy": []map[string]interface{}{{ + "xid": "T0", + "name": "Teacher0", + "teaches": innerTeachers, + }}, + }}, + } + + for n := 0; n < b.N; n++ { + op, _ := gqlSchema.Operation( + &schema.Request{ + Query: ` + mutation addStudent($input: [AddStudentInput!]!) { + addStudent(input: $input) { + student { + xid + } + } + }`, + Variables: vars, + }) + + NewAddRewriter().Rewrite(context.Background(), op.Mutations()[0]) + } +} + +func Benchmark3LevelDeep5(b *testing.B) { benchmark3LevelDeep(5, b) } +func Benchmark3LevelDeep10(b *testing.B) { benchmark3LevelDeep(10, b) } +func Benchmark3LevelDeep100(b *testing.B) { benchmark3LevelDeep(100, b) } + func mutationRewriting(t *testing.T, file string, rewriterFactory func() MutationRewriter) { b, err := ioutil.ReadFile(file) require.NoError(t, err, "Unable to read test file") From a986ed0ba070b92803dd8e1e2347af81021d7af1 Mon Sep 17 00:00:00 2001 From: harshil Date: Wed, 10 Jun 2020 02:49:36 +0530 Subject: [PATCH 10/22] removed print statement --- graphql/resolve/mutation_rewriter.go | 95 ---------------------------- graphql/resolve/mutation_test.go | 10 --- 2 files changed, 105 deletions(-) diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index a8f89016390..8ad7efbfad6 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -26,7 +26,6 @@ import ( dgoapi "github.com/dgraph-io/dgo/v2/protos/api" "github.com/dgraph-io/dgraph/gql" - "github.com/dgraph-io/dgraph/graphql/dgraph" "github.com/dgraph-io/dgraph/graphql/schema" "github.com/dgraph-io/dgraph/x" "github.com/pkg/errors" @@ -97,30 +96,6 @@ func NewVariableGenerator() *VariableGenerator { } } -func (mf *mutationFragment) print() { - fmt.Println("------------") - fmt.Println(dgraph.AsString(&gql.GraphQuery{Children: mf.queries})) - fmt.Println("Condition: ", mf.conditions) - byt, _ := json.MarshalIndent(mf.fragment, "", " ") - fmt.Println("Frag:", string(byt)) - byt, _ = json.MarshalIndent(mf.deletes, "", " ") - fmt.Println("Dels:", string(byt)) - fmt.Println("------------") -} - -func (mr *mutationRes) print() { - fmt.Println("======first pass======") - for _, i := range mr.firstPass { - i.print() - } - fmt.Println("======second pass======") - for _, i := range mr.secondPass { - i.print() - } - fmt.Println("======end pass======") - -} - // Next gets the Next variable name for the given type and xid. // So, if two objects of the same type have same value for xid field, // then they will get same variable name. @@ -293,7 +268,6 @@ func (mrw *AddRewriter) Rewrite(ctx context.Context, m schema.Mutation) ([]*Upse for _, i := range val { obj := i.(map[string]interface{}) frag := rewriteObject(mutatedType, nil, "", varGen, true, obj, 0, xidMd) - frag.print() mrw.frags = append(mrw.frags, frag.secondPass) mutationsAll = buildMutations(mutationsAll, queries, frag.firstPass, false) @@ -477,16 +451,6 @@ func (urw *UpdateRewriter) Rewrite( setFrag := rewriteObject(mutatedType, nil, srcUID, varGen, true, setArg.(map[string]interface{}), 0, xidMd) - fmt.Println("") - fmt.Println("") - fmt.Println("") - fmt.Println("=======set frags======") - setFrag.print() - fmt.Println("=======set frags end======") - fmt.Println("") - fmt.Println("") - fmt.Println("") - setFragF = setFrag.firstPass setFragS = setFrag.secondPass } @@ -986,14 +950,7 @@ func rewriteObject( var additionalFrag []*mutationFragment if xidFrag != nil && deepXID <= 2 { - fmt.Println("Frag") - xidFrag.print() - fmt.Println("Frag") results.secondPass = append(results.secondPass, xidFrag) - } else if xidFrag != nil { - fmt.Println("Frag E") - xidFrag.print() - fmt.Println("Frag E") } // if this object has an xid, then we don't need to rewrite its children if we have encountered @@ -1053,20 +1010,6 @@ func rewriteObject( } } - fmt.Println("") - fmt.Println("") - fmt.Println("") - fmt.Println("====frags=====") - fmt.Println(fieldName) - fmt.Println(val) - fmt.Println(strategy) - fmt.Println(obj) - fmt.Println(deepXID) - frags.print() - fmt.Println("====result=====") - results.print() - fmt.Println("====post results=====") - if strategy == "squash" { results.firstPass = squashFragments(squashIntoObject(fieldName), results.firstPass, frags.firstPass) } else { @@ -1075,17 +1018,7 @@ func rewriteObject( if xid == nil || (deepXID < 2 && xid != nil) { results.secondPass = squashFragments(squashIntoObject(fieldName), results.secondPass, frags.secondPass) - } else { - fmt.Println(deepXID) - fmt.Println(xid == nil) - fmt.Println("Here") } - - results.print() - fmt.Println("====end=====") - fmt.Println("") - fmt.Println("") - fmt.Println("") } } @@ -1489,13 +1422,6 @@ func squashIntoList(list, v interface{}, makeCopy bool) interface{} { func squashIntoObject(label string) func(interface{}, interface{}, bool) interface{} { return func(object, v interface{}, makeCopy bool) interface{} { - fmt.Println("") - fmt.Println("") - fmt.Println("") - fmt.Println("======== squash into object =======") - fmt.Println(object) - fmt.Println(v) - fmt.Println(label) asObject := object.(map[string]interface{}) if makeCopy { cpy := make(map[string]interface{}, len(asObject)+1) @@ -1505,11 +1431,6 @@ func squashIntoObject(label string) func(interface{}, interface{}, bool) interfa asObject = cpy } asObject[label] = v - fmt.Println(asObject) - fmt.Println("======== squash into object =======") - fmt.Println("") - fmt.Println("") - fmt.Println("") return asObject } } @@ -1616,15 +1537,6 @@ func squashFragments( deletes = make([]interface{}, len(l.deletes), len(l.deletes)+len(r.deletes)) copy(deletes, l.deletes) } - fmt.Println("") - fmt.Println("") - fmt.Println("") - fmt.Println("============DEBUUUGGGGG START============") - - fmt.Println("l") - l.print() - fmt.Println("r") - r.print() result = append(result, &mutationFragment{ conditions: append(conds, r.conditions...), @@ -1637,13 +1549,6 @@ func squashFragments( }(l.check, r.check), err: schema.AppendGQLErrs(l.err, r.err), }) - - fmt.Println("result") - result[len(result)-1].print() - fmt.Println("============DEBUUUGGGGG END============") - fmt.Println("") - fmt.Println("") - fmt.Println("") } } diff --git a/graphql/resolve/mutation_test.go b/graphql/resolve/mutation_test.go index 15c1f56dd6f..02dbc96f044 100644 --- a/graphql/resolve/mutation_test.go +++ b/graphql/resolve/mutation_test.go @@ -169,18 +169,10 @@ func mutationRewriting(t *testing.T, file string, rewriterFactory func() Mutatio gqlSchema := test.LoadSchemaFromFile(t, "schema.graphql") compareMutations := func(t *testing.T, test []*dgraphMutation, generated []*dgoapi.Mutation) { - for _, i := range generated { - fmt.Println(string(i.SetJson)) - fmt.Println(string(i.DeleteJson)) - fmt.Println(i.Cond) - } - require.Len(t, generated, len(test)) for i, expected := range test { require.Equal(t, expected.Cond, generated[i].Cond) if len(generated[i].SetJson) > 0 || expected.SetJSON != "" { - fmt.Println(expected.SetJSON) - fmt.Println(string(generated[i].SetJson)) require.JSONEq(t, expected.SetJSON, string(generated[i].SetJson)) } if len(generated[i].DeleteJson) > 0 || expected.DeleteJSON != "" { @@ -218,12 +210,10 @@ func mutationRewriting(t *testing.T, file string, rewriterFactory func() Mutatio return } - fmt.Println(dgraph.AsString(upsert[0].Query)) require.Equal(t, tcase.DGQuery, dgraph.AsString(upsert[0].Query)) compareMutations(t, tcase.DGMutations, upsert[0].Mutations) if len(upsert) > 1 { - fmt.Println(dgraph.AsString(upsert[1].Query)) require.Equal(t, tcase.DGQuerySec, dgraph.AsString(upsert[1].Query)) compareMutations(t, tcase.DGMutationsSec, upsert[1].Mutations) } From e7fa2ff59b1a72e768a01f7d87ec75569f687879 Mon Sep 17 00:00:00 2001 From: harshil Date: Wed, 10 Jun 2020 03:21:07 +0530 Subject: [PATCH 11/22] fixed a bug in benchmark --- graphql/resolve/mutation_test.go | 35 ++++++++++++++------------------ 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/graphql/resolve/mutation_test.go b/graphql/resolve/mutation_test.go index 02dbc96f044..6ab9ebd2a1e 100644 --- a/graphql/resolve/mutation_test.go +++ b/graphql/resolve/mutation_test.go @@ -30,9 +30,6 @@ import ( "github.com/dgraph-io/dgraph/graphql/test" "github.com/dgraph-io/dgraph/x" "github.com/stretchr/testify/require" - "github.com/vektah/gqlparser/v2/ast" - "github.com/vektah/gqlparser/v2/parser" - "github.com/vektah/gqlparser/v2/validator" "gopkg.in/yaml.v2" ) @@ -110,25 +107,22 @@ func mutationValidation(t *testing.T, file string, rewriterFactory func() Mutati } func benchmark3LevelDeep(num int, b *testing.B) { - gql, _ := ioutil.ReadFile("schema.graphql") - handler, _ := schema.NewHandler(string(gql)) - doc, _ := parser.ParseSchemas(validator.Prelude, &ast.Source{Input: handler.GQLSchema()}) - gqlSch, _ := validator.ValidateSchemaDocument(doc) - gqlSchema := schema.AsSchema(gqlSch) + t := &testing.T{} + gqlSchema := test.LoadSchemaFromFile(t, "schema.graphql") - innerTeachers := make([]map[string]interface{}, num) + innerTeachers := make([]interface{}, 0) for i := 1; i <= num; i++ { innerTeachers = append(innerTeachers, map[string]interface{}{ - "xid": fmt.Sprintf("S$%d", i), + "xid": fmt.Sprintf("S%d", i), "name": fmt.Sprintf("Name%d", i), }) } vars := map[string]interface{}{ - "input": []map[string]interface{}{{ + "input": []interface{}{map[string]interface{}{ "xid": "S0", "name": "Name0", - "taughtBy": []map[string]interface{}{{ + "taughtBy": []interface{}{map[string]interface{}{ "xid": "T0", "name": "Teacher0", "teaches": innerTeachers, @@ -136,10 +130,9 @@ func benchmark3LevelDeep(num int, b *testing.B) { }}, } - for n := 0; n < b.N; n++ { - op, _ := gqlSchema.Operation( - &schema.Request{ - Query: ` + op, _ := gqlSchema.Operation( + &schema.Request{ + Query: ` mutation addStudent($input: [AddStudentInput!]!) { addStudent(input: $input) { student { @@ -147,15 +140,17 @@ func benchmark3LevelDeep(num int, b *testing.B) { } } }`, - Variables: vars, - }) + Variables: vars, + }) + mut := test.GetMutation(t, op) - NewAddRewriter().Rewrite(context.Background(), op.Mutations()[0]) + for n := 0; n < b.N; n++ { + NewAddRewriter().Rewrite(context.Background(), mut) } } func Benchmark3LevelDeep5(b *testing.B) { benchmark3LevelDeep(5, b) } -func Benchmark3LevelDeep10(b *testing.B) { benchmark3LevelDeep(10, b) } +func Benchmark3LevelDeep19(b *testing.B) { benchmark3LevelDeep(19, b) } func Benchmark3LevelDeep100(b *testing.B) { benchmark3LevelDeep(100, b) } func mutationRewriting(t *testing.T, file string, rewriterFactory func() MutationRewriter) { From 4a06fef9d494db766fe268a2067e2cb49e4cb071 Mon Sep 17 00:00:00 2001 From: harshil Date: Wed, 10 Jun 2020 13:12:21 +0530 Subject: [PATCH 12/22] added benchmarks --- graphql/resolve/mutation_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/graphql/resolve/mutation_test.go b/graphql/resolve/mutation_test.go index 6ab9ebd2a1e..9ee0fc300ad 100644 --- a/graphql/resolve/mutation_test.go +++ b/graphql/resolve/mutation_test.go @@ -149,9 +149,11 @@ func benchmark3LevelDeep(num int, b *testing.B) { } } -func Benchmark3LevelDeep5(b *testing.B) { benchmark3LevelDeep(5, b) } -func Benchmark3LevelDeep19(b *testing.B) { benchmark3LevelDeep(19, b) } -func Benchmark3LevelDeep100(b *testing.B) { benchmark3LevelDeep(100, b) } +func Benchmark3LevelDeep5(b *testing.B) { benchmark3LevelDeep(5, b) } +func Benchmark3LevelDeep19(b *testing.B) { benchmark3LevelDeep(19, b) } +func Benchmark3LevelDeep100(b *testing.B) { benchmark3LevelDeep(100, b) } +func Benchmark3LevelDeep1000(b *testing.B) { benchmark3LevelDeep(1000, b) } +func Benchmark3LevelDeep10000(b *testing.B) { benchmark3LevelDeep(10000, b) } func mutationRewriting(t *testing.T, file string, rewriterFactory func() MutationRewriter) { b, err := ioutil.ReadFile(file) From 705aba672c1b8f82cd5232646841b9e5665c3acc Mon Sep 17 00:00:00 2001 From: harshil Date: Wed, 10 Jun 2020 14:04:12 +0530 Subject: [PATCH 13/22] fixed some tests --- graphql/resolve/mutation_rewriter.go | 5 +++++ graphql/resolve/update_mutation_test.yaml | 24 ++++++++--------------- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index 8ad7efbfad6..f38c56b642d 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -962,6 +962,11 @@ func rewriteObject( fieldDef := typ.Field(field) fieldName := typ.DgraphPredicate(field) + // This fixes mutation when dgraph predicate has special characters. PR #5526 + if strings.HasPrefix(fieldName, "<") && strings.HasSuffix(fieldName, ">") { + fieldName = fieldName[1 : len(fieldName)-1] + } + strategy := "squash" switch val := val.(type) { diff --git a/graphql/resolve/update_mutation_test.yaml b/graphql/resolve/update_mutation_test.yaml index 206a00875bc..a16fccadd58 100644 --- a/graphql/resolve/update_mutation_test.yaml +++ b/graphql/resolve/update_mutation_test.yaml @@ -990,20 +990,13 @@ dgmutationssec: - setjson: | { - "Student.taughtBy": [ - { - "Teacher.teaches": [{ - "uid": "uid(x)" - }], - "uid": "uid(Teacher3)" - }, - { - "Teacher.teaches": [{ - "uid": "uid(x)" - }], - "uid": "uid(Teacher3)" - } - ], + "Student.taughtBy":[{ + "Teacher.teaches":[{"uid":"uid(x)"}], + "uid":"uid(Teacher3)" + },{ + "Teacher.teaches":[{"uid":"uid(x)"}], + "uid":"uid(Teacher3)" + }], "uid": "uid(x)" } cond: "@if(eq(len(Teacher3), 1) AND eq(len(Teacher3), 1) AND gt(len(x), 0))" @@ -1355,8 +1348,7 @@ { "uid": "_:Computer3", "dgraph.type": ["Computer"], - "Computer.name": "Comp", - "Computer.owners":[ { "uid":"uid(x)" } ] + "Computer.name": "Comp" } cond: "@if(eq(len(Computer3), 0) AND gt(len(x), 0))" dgmutationssec: From 9d1e9b7c5bd4319d29476f084cfc3477f4325b06 Mon Sep 17 00:00:00 2001 From: harshil Date: Wed, 10 Jun 2020 14:23:59 +0530 Subject: [PATCH 14/22] fixed all tests --- graphql/e2e/directives/schema.graphql | 3 +-- graphql/resolve/mutation_rewriter.go | 8 ++------ 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/graphql/e2e/directives/schema.graphql b/graphql/e2e/directives/schema.graphql index 20da0595555..3aaa2d8c6f9 100644 --- a/graphql/e2e/directives/schema.graphql +++ b/graphql/e2e/directives/schema.graphql @@ -136,9 +136,8 @@ type Teacher implements People { } type Student implements People { - taughtBy: [Teacher] + taughtBy: [Teacher] @hasInverse(field: "teaches") } - type Message { content: String! @dgraph(pred: "post") author: String @dgraph(pred: "<职业>") diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index f38c56b642d..ce9819f3a16 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -406,9 +406,7 @@ func (urw *UpdateRewriter) Rewrite( }) urw.setFrags = append(urw.setFrags, setFrag...) - if keepError { - errs = schema.AppendGQLErrs(errs, errSet) - } + errs = schema.AppendGQLErrs(errs, errSet) q1 := queryFromFragments(setFrag) if q1 != nil { @@ -429,9 +427,7 @@ func (urw *UpdateRewriter) Rewrite( }) urw.delFrags = append(urw.delFrags, delFrag...) - if keepError { - errs = schema.AppendGQLErrs(errs, errDel) - } + errs = schema.AppendGQLErrs(errs, errDel) q2 := queryFromFragments(delFrag) if q2 != nil { From bfffbbbb416a37c2518954475bbc1aa4e18cc209 Mon Sep 17 00:00:00 2001 From: harshil Date: Wed, 10 Jun 2020 15:44:14 +0530 Subject: [PATCH 15/22] added a failing test --- graphql/resolve/add_mutation_test.yaml | 70 ++++++++++++++++++++++++++ graphql/resolve/mutation_rewriter.go | 6 --- graphql/resolve/mutation_test.go | 6 +++ 3 files changed, 76 insertions(+), 6 deletions(-) diff --git a/graphql/resolve/add_mutation_test.yaml b/graphql/resolve/add_mutation_test.yaml index 3f7f2629294..18a87e11d3d 100644 --- a/graphql/resolve/add_mutation_test.yaml +++ b/graphql/resolve/add_mutation_test.yaml @@ -1916,6 +1916,76 @@ } } +- name: "Deep mutation alternate id xid" + gqlmutation: | + mutation addAuthor($city: AddCityInput!) { + addCity(input: [$city]) { + city { + name + district { + code + name + cities { + name + district { + code + name + } + } + } + } + } + } + gqlvariables: | + { + "city": { + "name": "c1", + "district":{ + "name":"d1", + "code":"d1", + "cities":[{"name": "c2"}] + } + } + } + dgquery: |- + query { + District3 as District3(func: eq(District.code, "d1")) @filter(type(District)) { + uid + } + } + dgmutations: + - setjson: | + {"District.code":"d1","District.name":"d1","dgraph.type":["District"],"uid":"_:District3"} + cond: "@if(eq(len(District3), 0))" + + dgquerysec: |- + query { + District3 as District3(func: eq(District.code, "d1")) @filter(type(District)) { + uid + } + } + + dgmutationssec: + - setjson: | + { + "City.district": { + "District.cities": [{ + "City.district": { + "uid": "uid(District3)" + }, + "City.name": "c2", + "dgraph.type": ["City"], + "uid": "_:City4" + }], + "uid": "uid(District3)" + }, + "City.name": "c1", + "dgraph.type": ["City"], + "uid": "_:City1" + } + cond: "@if(eq(len(District3), 1))" + + - name: "Additional Deletes - deep mutation" gqlmutation: | mutation addAuthor($auth: AddAuthorInput!) { diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index ce9819f3a16..60c642c0ab8 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -738,12 +738,6 @@ type mutationRes struct { secondPass []*mutationFragment } -func (mr *mutationRes) get() []*mutationFragment { - k := mr.firstPass - k = append(k, mr.secondPass...) - return k -} - // rewriteObject rewrites obj to a list of mutation fragments. See AddRewriter.Rewrite // for a description of what those fragments look like. // diff --git a/graphql/resolve/mutation_test.go b/graphql/resolve/mutation_test.go index 9ee0fc300ad..ddc16bc457c 100644 --- a/graphql/resolve/mutation_test.go +++ b/graphql/resolve/mutation_test.go @@ -166,6 +166,10 @@ func mutationRewriting(t *testing.T, file string, rewriterFactory func() Mutatio gqlSchema := test.LoadSchemaFromFile(t, "schema.graphql") compareMutations := func(t *testing.T, test []*dgraphMutation, generated []*dgoapi.Mutation) { + for _, i := range generated { + fmt.Println(string(i.SetJson)) + } + require.Len(t, generated, len(test)) for i, expected := range test { require.Equal(t, expected.Cond, generated[i].Cond) @@ -207,10 +211,12 @@ func mutationRewriting(t *testing.T, file string, rewriterFactory func() Mutatio return } + fmt.Println(dgraph.AsString(upsert[0].Query)) require.Equal(t, tcase.DGQuery, dgraph.AsString(upsert[0].Query)) compareMutations(t, tcase.DGMutations, upsert[0].Mutations) if len(upsert) > 1 { + fmt.Println(dgraph.AsString(upsert[1].Query)) require.Equal(t, tcase.DGQuerySec, dgraph.AsString(upsert[1].Query)) compareMutations(t, tcase.DGMutationsSec, upsert[1].Mutations) } From b1540ca663ff20f757d50e1efc77d3e2f07b1a7a Mon Sep 17 00:00:00 2001 From: harshil Date: Thu, 11 Jun 2020 20:38:56 +0530 Subject: [PATCH 16/22] refactored code --- graphql/resolve/add_mutation_test.yaml | 93 ++++++++++------- graphql/resolve/mutation_rewriter.go | 132 ++++++++++++++++++------- 2 files changed, 157 insertions(+), 68 deletions(-) diff --git a/graphql/resolve/add_mutation_test.yaml b/graphql/resolve/add_mutation_test.yaml index 18a87e11d3d..1bd524ddaec 100644 --- a/graphql/resolve/add_mutation_test.yaml +++ b/graphql/resolve/add_mutation_test.yaml @@ -1744,6 +1744,7 @@ { "People.name":"teacher0", "People.xid":"T0", + "Teacher.teaches":[], "dgraph.type":["Teacher","People"], "uid":"_:Teacher4" } @@ -1762,27 +1763,27 @@ - setjson: | { - "Student.taughtBy":[{"uid":"_:Teacher4"}], - "uid":"uid(Student6)" + "Teacher.teaches":[{"uid":"uid(Student6)"}], + "uid":"_:Teacher4" } deletejson: | cond: "@if(eq(len(Student6), 1) AND eq(len(Teacher4), 0))" - setjson: | { - "Teacher.teaches":[{"uid":"uid(Student6)"}], + "Teacher.teaches":[{"uid":"_:Student6"}], "uid":"_:Teacher4" } deletejson: | - cond: "@if(eq(len(Student6), 1) AND eq(len(Teacher4), 0))" + cond: "@if(eq(len(Student6), 0) AND eq(len(Teacher4), 0))" - setjson: | { - "Teacher.teaches":[{"uid":"_:Student6"}], - "uid":"_:Teacher4" + "Student.taughtBy":[{"uid":"_:Teacher4"}], + "uid":"uid(Student6)" } deletejson: | - cond: "@if(eq(len(Student6), 0) AND eq(len(Teacher4), 0))" + cond: "@if(eq(len(Student6), 1) AND eq(len(Teacher4), 0))" dgquerysec: |- query { @@ -1831,6 +1832,7 @@ - setjson: | { "Computer.name": "computer1", + "Computer.owners":[], "dgraph.type": [ "Computer" ], @@ -1847,6 +1849,20 @@ } cond: "@if(eq(len(ComputerOwner6), 0) AND eq(len(Computer4), 0))" + - setjson: | + { + "Computer.owners":[{"uid":"uid(ComputerOwner6)"}], + "uid":"_:Computer4" + } + cond: "@if(eq(len(ComputerOwner6), 1) AND eq(len(Computer4), 0))" + + - setjson: | + { + "Computer.owners":[{"uid":"_:ComputerOwner6"}], + "uid":"_:Computer4" + } + cond: "@if(eq(len(ComputerOwner6), 0) AND eq(len(Computer4), 0))" + - setjson: | { "ComputerOwner.computers": { @@ -1865,19 +1881,6 @@ }] cond: "@if(eq(len(ComputerOwner6), 1) AND eq(len(Computer4), 0))" - - setjson: | - { - "Computer.owners":[{"uid":"uid(ComputerOwner6)"}], - "uid":"_:Computer4" - } - cond: "@if(eq(len(ComputerOwner6), 1) AND eq(len(Computer4), 0))" - - - setjson: | - { - "Computer.owners":[{"uid":"_:ComputerOwner6"}], - "uid":"_:Computer4" - } - cond: "@if(eq(len(ComputerOwner6), 0) AND eq(len(Computer4), 0))" dgmutationssec: - setjson: |- @@ -1955,7 +1958,27 @@ } dgmutations: - setjson: | - {"District.code":"d1","District.name":"d1","dgraph.type":["District"],"uid":"_:District3"} + { + "District.cities": [ + { + "City.district": { + "uid": "_:District3" + }, + "City.name": "c2", + "dgraph.type": [ + "City" + ], + "uid": "_:City4" + } + ], + "District.code": "d1", + "District.name": "d1", + "dgraph.type": [ + "District" + ], + "uid": "_:District3" + } + cond: "@if(eq(len(District3), 0))" dgquerysec: |- @@ -1968,21 +1991,21 @@ dgmutationssec: - setjson: | { - "City.district": { - "District.cities": [{ - "City.district": { - "uid": "uid(District3)" - }, - "City.name": "c2", - "dgraph.type": ["City"], - "uid": "_:City4" - }], - "uid": "uid(District3)" - }, - "City.name": "c1", - "dgraph.type": ["City"], - "uid": "_:City1" + "City.district": { + "District.cities": [ + { + "uid": "_:City1" + } + ], + "uid": "uid(District3)" + }, + "City.name": "c1", + "dgraph.type": [ + "City" + ], + "uid": "_:City1" } + cond: "@if(eq(len(District3), 1))" diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index 60c642c0ab8..d71a0cf5771 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -26,6 +26,7 @@ import ( dgoapi "github.com/dgraph-io/dgo/v2/protos/api" "github.com/dgraph-io/dgraph/gql" + "github.com/dgraph-io/dgraph/graphql/dgraph" "github.com/dgraph-io/dgraph/graphql/schema" "github.com/dgraph-io/dgraph/x" "github.com/pkg/errors" @@ -738,6 +739,31 @@ type mutationRes struct { secondPass []*mutationFragment } +func (mf *mutationFragment) print() { + fmt.Println(dgraph.AsString(&gql.GraphQuery{Children: mf.queries})) + byt, _ := json.MarshalIndent(mf.fragment, "", " ") + fmt.Println(string(byt)) + fmt.Println(mf.conditions) +} + +func (mr *mutationRes) print() { + fmt.Println("") + fmt.Println("") + fmt.Println("") + fmt.Println("========First Pass=====") + for _, i := range mr.firstPass { + i.print() + } + fmt.Println("========Second Pass=====") + for _, i := range mr.secondPass { + i.print() + } + fmt.Println("========End Pass=====") + fmt.Println("") + fmt.Println("") + fmt.Println("") +} + // rewriteObject rewrites obj to a list of mutation fragments. See AddRewriter.Rewrite // for a description of what those fragments look like. // @@ -835,6 +861,7 @@ func rewriteObject( xidFrag = asXIDReference(srcField, srcUID, typ, xid.Name(), xidString, variable, withAdditionalDeletes, varGen, xidMetadata) if deepXID > 2 { + // Create an inverse link for the xidFrag res := make(map[string]interface{}, 1) res["uid"] = srcUID addInverseLink(res, srcField.Inverse(), fmt.Sprintf("uid(%s)", variable)) @@ -919,14 +946,13 @@ func rewriteObject( } if xid != nil && !atTopLevel { - results = &mutationRes{ - firstPass: []*mutationFragment{frag}, - } - if deepXID <= 2 { + if deepXID <= 2 { // elements in firstPass or not + // duplicate query in elements >= 2, as the pair firstPass element would already have the same query. frag.queries = []*gql.GraphQuery{ xidQuery(variable, xidString, xid.Name(), typ), } } else { + // for elements in firstPass, we need to create an inverse link res := make(map[string]interface{}, 1) res["uid"] = srcUID addInverseLink(res, srcField.Inverse(), fmt.Sprintf("_:%s", variable)) @@ -939,12 +965,11 @@ func rewriteObject( var additionalFrag []*mutationFragment - if xidFrag != nil && deepXID <= 2 { - results.secondPass = append(results.secondPass, xidFrag) - } + // we build the mutation to add object here. If XID != nil, we would then move it to + // firstPass from secondPass (frag). - // if this object has an xid, then we don't need to rewrite its children if we have encountered - // it earlier + // if this object has an xid, then we don't need to + // rewrite its children if we have encountered it earlier. if xidString == "" || xidEncounteredFirstTime { for field, val := range obj { var frags *mutationRes @@ -957,8 +982,6 @@ func rewriteObject( fieldName = fieldName[1 : len(fieldName)-1] } - strategy := "squash" - switch val := val.(type) { case map[string]interface{}: // This field is another GraphQL object, which could either be linking to an @@ -971,7 +994,6 @@ func rewriteObject( frags = rewriteObject(fieldDef.Type(), fieldDef, myUID, varGen, withAdditionalDeletes, val, deepXID, xidMetadata) - strategy = "merge" case []interface{}: // This field is either: @@ -988,7 +1010,6 @@ func rewriteObject( frags = rewriteList(fieldDef.Type(), fieldDef, myUID, varGen, withAdditionalDeletes, val, deepXID, xidMetadata) - strategy = "merge" default: // This field is either: // 1) a scalar value: e.g. @@ -998,46 +1019,86 @@ func rewriteObject( // e.g. to remove the text or // { "friends": null, ... } // to remove all friends - if xid != nil && !atTopLevel { - frags = &mutationRes{firstPass: []*mutationFragment{newFragment(val)}} - } else { - frags = &mutationRes{secondPass: []*mutationFragment{newFragment(val)}} - } - } - - if strategy == "squash" { - results.firstPass = squashFragments(squashIntoObject(fieldName), results.firstPass, frags.firstPass) - } else { - additionalFrag = appendFragments(additionalFrag, frags.firstPass) - } - - if xid == nil || (deepXID < 2 && xid != nil) { - results.secondPass = squashFragments(squashIntoObject(fieldName), results.secondPass, frags.secondPass) + frags = &mutationRes{secondPass: []*mutationFragment{newFragment(val)}} } + fmt.Println("") + fmt.Println("") + fmt.Println("") + fmt.Println("========Rewrite object Pass=====") + fmt.Println(fieldName) + fmt.Println(val) + fmt.Println(obj) + fmt.Println(xid == nil) + fmt.Println("========Frag=====") + frags.print() + fmt.Println("========Result=====") + results.print() + + additionalFrag = appendFragments(additionalFrag, frags.firstPass) + results.secondPass = squashFragments(squashIntoObject(fieldName), results.secondPass, frags.secondPass) + + fmt.Println("=======Post Result======") + results.print() + + fmt.Println("========End Pass=====") + fmt.Println("") + fmt.Println("") + fmt.Println("") } } - conditions := []string{} + if xid != nil && !atTopLevel { + results.firstPass = append(results.firstPass, results.secondPass...) + results.secondPass = []*mutationFragment{} + } + // add current conditions to all the new fragments from children. + // children add should only happen when this level is true. + conditions := []string{} for _, i := range results.firstPass { conditions = append(conditions, i.conditions...) } - for _, i := range additionalFrag { i.conditions = append(i.conditions, conditions...) } - results.firstPass = append(results.firstPass, additionalFrag...) + + // parentFrags are reverse links to parents. only applicable for when deepXID > 2 + results.firstPass = append(results.firstPass, parentFrags...) + + // xidFrag contains the mutation to update object if it is present. + // add it to secondPass if deepXID <= 2, otherwise firstPass for relevant hasInverse links. if xidFrag != nil && deepXID > 2 { results.firstPass = append(results.firstPass, xidFrag) + } else if xidFrag != nil { + results.secondPass = append(results.secondPass, xidFrag) } - results.firstPass = append(results.firstPass, parentFrags...) - + // if !xidEncounteredFirstTime, we have already seen the relevant fragments. if xid != nil && !atTopLevel && !xidEncounteredFirstTime { results.firstPass = []*mutationFragment{} } + fmt.Println("==========Final=======") + fmt.Println(obj) + fmt.Println(deepXID) + fmt.Println("==========additional frag==========") + for _, i := range additionalFrag { + i.print() + } + fmt.Println("==========parent frag==========") + for _, i := range parentFrags { + i.print() + } + fmt.Println("==========xidfrag==========") + if xidFrag != nil { + xidFrag.print() + } + fmt.Println("==========result==========") + + results.print() + fmt.Println("==========End Final=======") + return results } @@ -1381,6 +1442,9 @@ func rewriteList( switch obj := obj.(type) { case map[string]interface{}: frag := rewriteObject(typ, srcField, srcUID, varGen, withAdditionalDeletes, obj, deepXID, xidMetadata) + fmt.Println("========rewrite list=========") + frag.print() + fmt.Println("========rewrite end list========") result.firstPass = appendFragments(result.firstPass, frag.firstPass) result.secondPass = squashFragments(squashIntoList, result.secondPass, frag.secondPass) default: @@ -1417,6 +1481,8 @@ func squashIntoList(list, v interface{}, makeCopy bool) interface{} { func squashIntoObject(label string) func(interface{}, interface{}, bool) interface{} { return func(object, v interface{}, makeCopy bool) interface{} { + fmt.Println(object) + fmt.Println(v, label) asObject := object.(map[string]interface{}) if makeCopy { cpy := make(map[string]interface{}, len(asObject)+1) From 2e53e78b599e81355164ba69777eea4efc2095fb Mon Sep 17 00:00:00 2001 From: harshil Date: Thu, 11 Jun 2020 20:55:25 +0530 Subject: [PATCH 17/22] removed print statements --- graphql/resolve/mutation_rewriter.go | 71 ---------------------------- graphql/resolve/mutation_test.go | 6 --- 2 files changed, 77 deletions(-) diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index d71a0cf5771..91508c7c976 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -26,7 +26,6 @@ import ( dgoapi "github.com/dgraph-io/dgo/v2/protos/api" "github.com/dgraph-io/dgraph/gql" - "github.com/dgraph-io/dgraph/graphql/dgraph" "github.com/dgraph-io/dgraph/graphql/schema" "github.com/dgraph-io/dgraph/x" "github.com/pkg/errors" @@ -739,31 +738,6 @@ type mutationRes struct { secondPass []*mutationFragment } -func (mf *mutationFragment) print() { - fmt.Println(dgraph.AsString(&gql.GraphQuery{Children: mf.queries})) - byt, _ := json.MarshalIndent(mf.fragment, "", " ") - fmt.Println(string(byt)) - fmt.Println(mf.conditions) -} - -func (mr *mutationRes) print() { - fmt.Println("") - fmt.Println("") - fmt.Println("") - fmt.Println("========First Pass=====") - for _, i := range mr.firstPass { - i.print() - } - fmt.Println("========Second Pass=====") - for _, i := range mr.secondPass { - i.print() - } - fmt.Println("========End Pass=====") - fmt.Println("") - fmt.Println("") - fmt.Println("") -} - // rewriteObject rewrites obj to a list of mutation fragments. See AddRewriter.Rewrite // for a description of what those fragments look like. // @@ -1021,29 +995,9 @@ func rewriteObject( // to remove all friends frags = &mutationRes{secondPass: []*mutationFragment{newFragment(val)}} } - fmt.Println("") - fmt.Println("") - fmt.Println("") - fmt.Println("========Rewrite object Pass=====") - fmt.Println(fieldName) - fmt.Println(val) - fmt.Println(obj) - fmt.Println(xid == nil) - fmt.Println("========Frag=====") - frags.print() - fmt.Println("========Result=====") - results.print() additionalFrag = appendFragments(additionalFrag, frags.firstPass) results.secondPass = squashFragments(squashIntoObject(fieldName), results.secondPass, frags.secondPass) - - fmt.Println("=======Post Result======") - results.print() - - fmt.Println("========End Pass=====") - fmt.Println("") - fmt.Println("") - fmt.Println("") } } @@ -1079,26 +1033,6 @@ func rewriteObject( results.firstPass = []*mutationFragment{} } - fmt.Println("==========Final=======") - fmt.Println(obj) - fmt.Println(deepXID) - fmt.Println("==========additional frag==========") - for _, i := range additionalFrag { - i.print() - } - fmt.Println("==========parent frag==========") - for _, i := range parentFrags { - i.print() - } - fmt.Println("==========xidfrag==========") - if xidFrag != nil { - xidFrag.print() - } - fmt.Println("==========result==========") - - results.print() - fmt.Println("==========End Final=======") - return results } @@ -1442,9 +1376,6 @@ func rewriteList( switch obj := obj.(type) { case map[string]interface{}: frag := rewriteObject(typ, srcField, srcUID, varGen, withAdditionalDeletes, obj, deepXID, xidMetadata) - fmt.Println("========rewrite list=========") - frag.print() - fmt.Println("========rewrite end list========") result.firstPass = appendFragments(result.firstPass, frag.firstPass) result.secondPass = squashFragments(squashIntoList, result.secondPass, frag.secondPass) default: @@ -1481,8 +1412,6 @@ func squashIntoList(list, v interface{}, makeCopy bool) interface{} { func squashIntoObject(label string) func(interface{}, interface{}, bool) interface{} { return func(object, v interface{}, makeCopy bool) interface{} { - fmt.Println(object) - fmt.Println(v, label) asObject := object.(map[string]interface{}) if makeCopy { cpy := make(map[string]interface{}, len(asObject)+1) diff --git a/graphql/resolve/mutation_test.go b/graphql/resolve/mutation_test.go index ddc16bc457c..9ee0fc300ad 100644 --- a/graphql/resolve/mutation_test.go +++ b/graphql/resolve/mutation_test.go @@ -166,10 +166,6 @@ func mutationRewriting(t *testing.T, file string, rewriterFactory func() Mutatio gqlSchema := test.LoadSchemaFromFile(t, "schema.graphql") compareMutations := func(t *testing.T, test []*dgraphMutation, generated []*dgoapi.Mutation) { - for _, i := range generated { - fmt.Println(string(i.SetJson)) - } - require.Len(t, generated, len(test)) for i, expected := range test { require.Equal(t, expected.Cond, generated[i].Cond) @@ -211,12 +207,10 @@ func mutationRewriting(t *testing.T, file string, rewriterFactory func() Mutatio return } - fmt.Println(dgraph.AsString(upsert[0].Query)) require.Equal(t, tcase.DGQuery, dgraph.AsString(upsert[0].Query)) compareMutations(t, tcase.DGMutations, upsert[0].Mutations) if len(upsert) > 1 { - fmt.Println(dgraph.AsString(upsert[1].Query)) require.Equal(t, tcase.DGQuerySec, dgraph.AsString(upsert[1].Query)) compareMutations(t, tcase.DGMutationsSec, upsert[1].Mutations) } From 6fb43db026f615891a7603ad7e13dcc6dc62de4b Mon Sep 17 00:00:00 2001 From: harshil Date: Thu, 11 Jun 2020 22:30:07 +0530 Subject: [PATCH 18/22] refactored a little bit --- graphql/resolve/mutation_rewriter.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index 91508c7c976..100bf4e50ff 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -240,7 +240,7 @@ func (mrw *AddRewriter) Rewrite(ctx context.Context, m schema.Mutation) ([]*Upse queries := &gql.GraphQuery{} buildMutations := func(mutationsAll []*dgoapi.Mutation, queries *gql.GraphQuery, - frag []*mutationFragment, keepError bool) []*dgoapi.Mutation { + frag []*mutationFragment) []*dgoapi.Mutation { mutations, err := mutationsFromFragments( frag, func(frag *mutationFragment) ([]byte, error) { @@ -270,8 +270,8 @@ func (mrw *AddRewriter) Rewrite(ctx context.Context, m schema.Mutation) ([]*Upse frag := rewriteObject(mutatedType, nil, "", varGen, true, obj, 0, xidMd) mrw.frags = append(mrw.frags, frag.secondPass) - mutationsAll = buildMutations(mutationsAll, queries, frag.firstPass, false) - mutationsAllSec = buildMutations(mutationsAllSec, queriesSec, frag.secondPass, true) + mutationsAll = buildMutations(mutationsAll, queries, frag.firstPass) + mutationsAllSec = buildMutations(mutationsAllSec, queriesSec, frag.secondPass) } if len(queries.Children) == 0 { @@ -386,7 +386,7 @@ func (urw *UpdateRewriter) Rewrite( var errs error varGen := NewVariableGenerator() - buildMutation := func(setFrag, delFrag []*mutationFragment, keepError bool) *UpsertMutation { + buildMutation := func(setFrag, delFrag []*mutationFragment) *UpsertMutation { var mutSet, mutDel []*dgoapi.Mutation queries := []*gql.GraphQuery{upsertQuery} @@ -460,12 +460,12 @@ func (urw *UpdateRewriter) Rewrite( result := []*UpsertMutation{} - firstPass := buildMutation(setFragF, delFragF, true) + firstPass := buildMutation(setFragF, delFragF) if len(firstPass.Mutations) > 0 { result = append(result, firstPass) } - secondPass := buildMutation(setFragS, delFragS, false) + secondPass := buildMutation(setFragS, delFragS) if len(secondPass.Mutations) > 0 { result = append(result, secondPass) } @@ -790,7 +790,7 @@ func rewriteObject( if !ok { errFrag := newFragment(nil) errFrag.err = errors.New("encountered an XID that isn't a string") - return &mutationRes{firstPass: []*mutationFragment{errFrag}} + return &mutationRes{secondPass: []*mutationFragment{errFrag}} } // if the object has an xid, the variable name will be formed from the xidValue in order // to handle duplicate object addition/updation @@ -812,7 +812,7 @@ func rewriteObject( xidObj, obj) || (invField != nil && invField.Type().ListType() == nil) { errFrag := newFragment(nil) errFrag.err = errors.Errorf("duplicate XID found: %s", xidString) - return &mutationRes{firstPass: []*mutationFragment{errFrag}} + return &mutationRes{secondPass: []*mutationFragment{errFrag}} } } else { // if not encountered till now, add it to the map @@ -852,7 +852,7 @@ func rewriteObject( } else { name = id.Name() } - return &mutationRes{firstPass: invalidObjectFragment(fmt.Errorf("%s is not provided", name), + return &mutationRes{secondPass: invalidObjectFragment(fmt.Errorf("%s is not provided", name), xidFrag, variable, xidString)} } } From b9aeb258013b80cd1b55d93c3c6bb3b38a7d1594 Mon Sep 17 00:00:00 2001 From: harshil Date: Fri, 12 Jun 2020 00:23:12 +0530 Subject: [PATCH 19/22] minor refactor --- graphql/resolve/mutation_rewriter.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index 100bf4e50ff..475c22baf29 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -677,8 +677,7 @@ func asUID(val interface{}) (uint64, error) { func mutationsFromFragments( frags []*mutationFragment, - setBuilder mutationBuilder, - delBuilder mutationBuilder) ([]*dgoapi.Mutation, error) { + setBuilder, delBuilder mutationBuilder) ([]*dgoapi.Mutation, error) { mutations := make([]*dgoapi.Mutation, 0, len(frags)) var errs x.GqlErrorList From 63a0f953422368990e5eb11d94d140e3f3c9f9cf Mon Sep 17 00:00:00 2001 From: harshil Date: Fri, 12 Jun 2020 14:48:10 +0530 Subject: [PATCH 20/22] added little bit more comments --- graphql/resolve/mutation_rewriter.go | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index 475c22baf29..ac1fc6f1162 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -752,6 +752,11 @@ type mutationRes struct { // an author might make the `author: Author!` field of a bunch of Posts invalid. // (That might actually be helpful if you want to run one mutation to remove something // and then another to correct it.) +// +// rewriteObject returns two set of mutations, firstPass and secondPass. We start +// building mutations recursively in the secondPass. Whenever we encounter an XID object, +// we push it to firstPass. We need to make sure that the XID doesn't refer hasInverse links +// to secondPass, and then to make those links ourselves. func rewriteObject( typ schema.Type, srcField schema.FieldDefinition, @@ -834,7 +839,7 @@ func rewriteObject( xidFrag = asXIDReference(srcField, srcUID, typ, xid.Name(), xidString, variable, withAdditionalDeletes, varGen, xidMetadata) if deepXID > 2 { - // Create an inverse link for the xidFrag + // We need to link the parent to the already existing child res := make(map[string]interface{}, 1) res["uid"] = srcUID addInverseLink(res, srcField.Inverse(), fmt.Sprintf("uid(%s)", variable)) @@ -925,7 +930,7 @@ func rewriteObject( xidQuery(variable, xidString, xid.Name(), typ), } } else { - // for elements in firstPass, we need to create an inverse link + // We need to link the parent to the element we are just creating res := make(map[string]interface{}, 1) res["uid"] = srcUID addInverseLink(res, srcField.Inverse(), fmt.Sprintf("_:%s", variable)) @@ -936,7 +941,7 @@ func rewriteObject( } } - var additionalFrag []*mutationFragment + var childrenFirstPass []*mutationFragment // we build the mutation to add object here. If XID != nil, we would then move it to // firstPass from secondPass (frag). @@ -995,26 +1000,27 @@ func rewriteObject( frags = &mutationRes{secondPass: []*mutationFragment{newFragment(val)}} } - additionalFrag = appendFragments(additionalFrag, frags.firstPass) + childrenFirstPass = appendFragments(childrenFirstPass, frags.firstPass) results.secondPass = squashFragments(squashIntoObject(fieldName), results.secondPass, frags.secondPass) } } + // In the case of an XID, move the secondPass (creation mutation) to firstPass if xid != nil && !atTopLevel { results.firstPass = append(results.firstPass, results.secondPass...) results.secondPass = []*mutationFragment{} } // add current conditions to all the new fragments from children. - // children add should only happen when this level is true. + // childrens should only be addded when this level is true conditions := []string{} for _, i := range results.firstPass { conditions = append(conditions, i.conditions...) } - for _, i := range additionalFrag { + for _, i := range childrenFirstPass { i.conditions = append(i.conditions, conditions...) } - results.firstPass = append(results.firstPass, additionalFrag...) + results.firstPass = append(results.firstPass, childrenFirstPass...) // parentFrags are reverse links to parents. only applicable for when deepXID > 2 results.firstPass = append(results.firstPass, parentFrags...) From 83772179456e4dbca5e838a82de85cd1a736b5cf Mon Sep 17 00:00:00 2001 From: harshil Date: Fri, 12 Jun 2020 22:37:01 +0530 Subject: [PATCH 21/22] added another test --- graphql/resolve/add_mutation_test.yaml | 140 ++++++++++++++++++++++++- graphql/resolve/mutation_rewriter.go | 9 +- 2 files changed, 146 insertions(+), 3 deletions(-) diff --git a/graphql/resolve/add_mutation_test.yaml b/graphql/resolve/add_mutation_test.yaml index 1bd524ddaec..a1a56b17b69 100644 --- a/graphql/resolve/add_mutation_test.yaml +++ b/graphql/resolve/add_mutation_test.yaml @@ -1705,6 +1705,144 @@ ] cond: "@if(eq(len(State3), 1))" +- name: "Deep XID 4 level deep" + gqlmutation: | + mutation addStudent($student: AddStudentInput!) { + addStudent(input: [$student]) { + student { + name + } + } + } + gqlvariables: | + { + "student": { + "xid": "S0", + "name": "Student0", + "taughtBy": [{ + "xid": "T0", + "name": "teacher0", + "teaches": [{ + "xid": "S1", + "name": "Student1", + "taughtBy": [{ + "xid": "T1", + "name": "teacher1" + }] + }] + }] + } + } + dgquery: |- + query { + Teacher4 as Teacher4(func: eq(People.xid, "T0")) @filter(type(Teacher)) { + uid + } + Teacher8 as Teacher8(func: eq(People.xid, "T1")) @filter(type(Teacher)) { + uid + } + Student6 as Student6(func: eq(People.xid, "S1")) @filter(type(Student)) { + uid + } + } + + dgmutations: + - setjson: | + { + "People.name":"teacher0", + "People.xid":"T0", + "dgraph.type":["Teacher","People"], + "uid":"_:Teacher4" + } + + cond: "@if(eq(len(Teacher4), 0))" + + - setjson: | + { + "People.name": "Student1", + "People.xid": "S1", + "Student.taughtBy": [{"uid": "_:Teacher4"}], + "dgraph.type": ["Student","People"], + "uid": "_:Student6" + } + cond: "@if(eq(len(Student6), 0) AND eq(len(Teacher4), 0))" + + - setjson: | + { + "People.name": "teacher1", + "People.xid": "T1", + "Teacher.teaches": [{"uid": "_:Student6"}], + "dgraph.type": ["Teacher","People"], + "uid": "_:Teacher8" + } + cond: "@if(eq(len(Teacher8), 0) AND eq(len(Student6), 0) AND eq(len(Teacher4), 0))" + + - setjson: | + { + "Student.taughtBy": [{"uid": "uid(Teacher8)"}], + "uid": "_:Student6" + } + cond: "@if(eq(len(Teacher8), 1) AND eq(len(Student6), 0) AND eq(len(Teacher4), 0))" + + - setjson: | + { + "Student.taughtBy": [{"uid": "_:Teacher8"}], + "uid": "_:Student6" + } + cond: "@if(eq(len(Teacher8), 0) AND eq(len(Student6), 0) AND eq(len(Teacher4), 0))" + + - setjson: | + { + "Teacher.teaches": [{"uid": "_:Student6"}], + "uid": "uid(Teacher8)" + } + cond: "@if(eq(len(Teacher8), 1) AND eq(len(Student6), 0) AND eq(len(Teacher4), 0))" + + - setjson: | + { + "Teacher.teaches": [{"uid": "uid(Student6)"}], + "uid": "_:Teacher4" + } + cond: "@if(eq(len(Student6), 1) AND eq(len(Teacher4), 0))" + + - setjson: | + { + "Teacher.teaches": [{"uid": "_:Student6"}], + "uid": "_:Teacher4" + } + cond: "@if(eq(len(Student6), 0) AND eq(len(Teacher4), 0))" + + - setjson: | + { + "Student.taughtBy": [{"uid": "_:Teacher4"}], + "uid": "uid(Student6)" + } + cond: "@if(eq(len(Student6), 1) AND eq(len(Teacher4), 0))" + + dgquerysec: |- + query { + Student2 as Student2(func: eq(People.xid, "S0")) @filter(type(Student)) { + uid + } + Teacher4 as Teacher4(func: eq(People.xid, "T0")) @filter(type(Teacher)) { + uid + } + } + + dgmutationssec: + - setjson: | + { + "People.name": "Student0", + "People.xid": "S0", + "Student.taughtBy": [{ + "Teacher.teaches": [{"uid": "_:Student2"}], + "uid": "uid(Teacher4)" + }], + "dgraph.type": ["Student","People"], + "uid": "_:Student2" + } + cond: "@if(eq(len(Student2), 0) AND eq(len(Teacher4), 1))" + - name: "Deep XID Add top level hasInverse" gqlmutation: | mutation addStudent($student: AddStudentInput!) { @@ -1744,7 +1882,6 @@ { "People.name":"teacher0", "People.xid":"T0", - "Teacher.teaches":[], "dgraph.type":["Teacher","People"], "uid":"_:Teacher4" } @@ -1832,7 +1969,6 @@ - setjson: | { "Computer.name": "computer1", - "Computer.owners":[], "dgraph.type": [ "Computer" ], diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index ac1fc6f1162..f101b2857b2 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -1374,13 +1374,16 @@ func rewriteList( xidMetadata *xidMetadata) *mutationRes { result := &mutationRes{} - result.secondPass = []*mutationFragment{newFragment(make([]interface{}, 0))} + foundSecondPass := false for _, obj := range objects { switch obj := obj.(type) { case map[string]interface{}: frag := rewriteObject(typ, srcField, srcUID, varGen, withAdditionalDeletes, obj, deepXID, xidMetadata) + if len(frag.secondPass) != 0 { + foundSecondPass = true + } result.firstPass = appendFragments(result.firstPass, frag.firstPass) result.secondPass = squashFragments(squashIntoList, result.secondPass, frag.secondPass) default: @@ -1392,6 +1395,10 @@ func rewriteList( } } + if len(objects) != 0 && !foundSecondPass { + result.secondPass = nil + } + return result } From 6858556a332f94c1e2fb3e18d54821f81c169f31 Mon Sep 17 00:00:00 2001 From: harshil Date: Tue, 16 Jun 2020 13:36:25 +0530 Subject: [PATCH 22/22] removed a error handling to expose panics --- graphql/resolve/mutation.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/graphql/resolve/mutation.go b/graphql/resolve/mutation.go index 67c0c9ee963..575319f98ac 100644 --- a/graphql/resolve/mutation.go +++ b/graphql/resolve/mutation.go @@ -244,12 +244,6 @@ func (mr *dgraphResolver) rewriteAndExecute( return emptyResult(errs), resolverFailed } - if mutResp == nil { - return emptyResult( - schema.GQLWrapf(err, "mutation failed, couldn't get result")), - resolverFailed - } - err = mr.executor.CommitOrAbort(ctx, mutResp.Txn) if err != nil { return emptyResult(