Feat(GraphQL): Refactor Mutation Rewriting for AddRewriter

[vmrajas]

This PR refactors mutation_rewritier.go for Add Mutations. The approach followed is discussed in:
Related Discuss Post: https://discuss.dgraph.io/t/refactoring-graphql-mutation-rewriting/12114
Testing:
Unit tests (Changed add_mutation_test.yaml)
e2e tests. Added e2e tests under graphql/e2e/common .
Fixes GRAPHQL-872
Also fixes GRAPHQL-623, GRAPHQL-803 .

---

This change is 

[pawanrawal]

Reviewable status: 0 of 9 files reviewed, 20 unresolved discussions (waiting on @abhimanyusinghgaur and @vmrajas)

---

ee/acl/acl_test.go, line 232 at r1 (raw file):

	resp := createUser(t, token, userid, userpassword)
	require.Equal(t, x.GqlErrorList{{
		Message: "couldn't rewrite mutation addUser because failed to rewrite mutation payload because id" +

Can this be kept the same as before for backward compatibility?

---

graphql/e2e/auth/auth_test.go, line 308 at r1 (raw file):

/*
func TestAddMutationWithXid(t *testing.T) {

Do a t.Skip() instead of commenting.

---

graphql/resolve/mutation.go, line 243 at r1 (raw file):

		}
	}
	var upserts []*UpsertMutation

Space before this line to indicate a new block of code is starting.

---

graphql/resolve/mutation.go, line 260 at r1 (raw file):

		req.Query = qry
		var mutResp *dgoapi.Response
		if req.Query != "" {

Add a comment about in what cases would this be empty. I guess the cases where no id or xid fields are part of the mutation?

---

graphql/resolve/mutation.go, line 269 at r1 (raw file):

		}

		// Parse the result now.

add a comment showing a potential dgraph response here to make things easier to visualize.

---

graphql/resolve/mutation.go, line 274 at r1 (raw file):

			err = json.Unmarshal(mutResp.Json, &queryResultMap)
		}

newline not required here.

---

graphql/resolve/mutation.go, line 281 at r1 (raw file):

		}

		existenceQueriesResult := make(map[string]string)

Add a comment about what this map holds and rename it to something smaller like qnameToUid because it maps a query name to a uid.

---

graphql/resolve/mutation.go, line 287 at r1 (raw file):

				existenceQueriesResult[key] = result[0]["uid"]
			}
			// TODO: Handle and throw proper error if len(result) > 1. Ideally this should not happen.

let's do this here

---

graphql/resolve/mutation.go, line 291 at r1 (raw file):

		// Create mutations and new nodes depending on result of queries.
		upserts, frags, err = NewCreateMutations(ctx, mutation, varGen, xidMd, existenceQueriesResult)

Is this part of the same transaction yet as the query above?

---

graphql/resolve/mutation_rewriter.go, line 84 at r1 (raw file):

	seenUIDs map[string]bool
	// iDToVariable map stores XID / UID -> the variable map used in the query.
	idToVariable map[string]string

not required?

---

graphql/resolve/mutation_rewriter.go, line 370 at r1 (raw file):

	for _, i := range val {
		obj := i.(map[string]interface{})
		queries, errs := getExistenceQueries(ctx, mutatedType, nil, varGen, obj, xidMetadata)

existenceQueries is enough. In Go usually, getters don't have get in front of them. https://golang.org/doc/effective_go.html#Getters

---

graphql/resolve/mutation_rewriter.go, line 374 at r1 (raw file):

			var gqlErrors x.GqlErrorList
			for _, err := range errs {
				gqlErrors = append(gqlErrors, schema.AsGQLErrors(err)...)

Why don't we append the errors to retErrors here itself?

---

graphql/resolve/mutation_rewriter.go, line 400 at r1 (raw file):

	for _, i := range val {
		obj := i.(map[string]interface{})
		fragment, errs := newRewriteObject(ctx, mutatedType, nil, "", varGen, obj, xidMetadata, existenceQueriesResult)

The last variable can be shortened a bit to idExistence?

---

graphql/resolve/mutation_rewriter.go, line 1599 at r1 (raw file):

}

func newRewriteObject(

It feels to me like this should the method of an object with a few things like xidMetadata, varGen, existenceQueriesResult being part of the object and the rest of the things being passed in as variables.

---

graphql/resolve/mutation_rewriter.go, line 1650 at r1 (raw file):

						return nil, retErrors
					} else {
						return newAsIDReference(ctx, idVal, srcField, srcUID, varGen, idVal.(string)), nil

Why are we passing both idVal and idVal.(string)? Can't we just pass the string version of the id?

---

graphql/resolve/mutation_rewriter.go, line 1663 at r1 (raw file):

			// As the type has not been referenced by ID field, remove it so that it does
			// not interfere with further processing.
			delete(obj, id.Name())

looks like we are here if id is null, why do we have to delete it from the object?

---

graphql/resolve/mutation_rewriter.go, line 1770 at r1 (raw file):

	var fields []string
	for field := range obj {
		fields = append(fields, field)

Is this required?

---

graphql/resolve/mutation_rewriter.go, line 1790 at r1 (raw file):

				fieldMutationFragment, err := newRewriteUnionField(ctx, fieldDef, myUID, varGen, val, xidMetadata, existenceQueriesResult)
				if fieldMutationFragment != nil {
					newObj[fieldName] = fieldMutationFragment.fragment

this part looks repeated below, lets move it to an inline function?

---

graphql/resolve/mutation_rewriter.go, line 1832 at r1 (raw file):

					}
					if fieldMutationFragment != nil {
						mutationFragments = append(mutationFragments, fieldMutationFragment.fragment)

looks repeated again

---

graphql/resolve/mutation_rewriter.go, line 2367 at r1 (raw file):

	}

	addDelete(ctx, frag, varGen, variable, srcUID, invField, srcField, isNew)

It's called isNew here but isQryVarID below. What is it supposed to represent?

[vmrajas]

Reviewable status: 0 of 11 files reviewed, 20 unresolved discussions (waiting on @abhimanyusinghgaur, @pawanrawal, and @vmrajas)

---

graphql/e2e/auth/auth_test.go, line 308 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Do a t.Skip() instead of commenting.

Done.

---

graphql/resolve/mutation.go, line 243 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Space before this line to indicate a new block of code is starting.

Done.

---

graphql/resolve/mutation.go, line 260 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Add a comment about in what cases would this be empty. I guess the cases where no id or xid fields are part of the mutation?

Done.

---

graphql/resolve/mutation.go, line 269 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

add a comment showing a potential dgraph response here to make things easier to visualize.

Done.

---

graphql/resolve/mutation.go, line 274 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

newline not required here.

Done.

---

graphql/resolve/mutation.go, line 281 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Add a comment about what this map holds and rename it to something smaller like qnameToUid because it maps a query name to a uid.

Done.

---

graphql/resolve/mutation.go, line 287 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

let's do this here

Done.

---

graphql/resolve/mutation.go, line 291 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Is this part of the same transaction yet as the query above?

Yes. This function does not execute the mutations. rewrites the mutations. I will rename the function name as well.

---

graphql/resolve/mutation_rewriter.go, line 84 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

not required?

Done.

---

graphql/resolve/mutation_rewriter.go, line 370 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

existenceQueries is enough. In Go usually, getters don't have get in front of them. https://golang.org/doc/effective_go.html#Getters

Done.

---

graphql/resolve/mutation_rewriter.go, line 400 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

The last variable can be shortened a bit to idExistence?

Done.

---

graphql/resolve/mutation_rewriter.go, line 1599 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

It feels to me like this should the method of an object with a few things like xidMetadata, varGen, existenceQueriesResult being part of the object and the rest of the things being passed in as variables.

As discussed, this will be handled after refactoring is done for UpdateRewriter (GRAPHQL-962)

---

graphql/resolve/mutation_rewriter.go, line 1650 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Why are we passing both idVal and idVal.(string)? Can't we just pass the string version of the id?

Good catch

---

graphql/resolve/mutation_rewriter.go, line 1663 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

looks like we are here if id is null, why do we have to delete it from the object?

removed it

---

graphql/resolve/mutation_rewriter.go, line 1770 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Is this required?

No. Removed it.

---

graphql/resolve/mutation_rewriter.go, line 1790 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

this part looks repeated below, lets move it to an inline function?

Done.

---

graphql/resolve/mutation_rewriter.go, line 1832 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

looks repeated again

Done.

---

graphql/resolve/mutation_rewriter.go, line 2367 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

It's called isNew here but isQryVarID below. What is it supposed to represent?

It represents if the qryVar is an ID. It is ID only in the newRewriteObject. I have renamed it everywhere to reflect this.

[pawanrawal]

Reviewed 1 of 9 files at r1, 2 of 10 files at r2.
Reviewable status: 3 of 11 files reviewed, 8 unresolved discussions (waiting on @abhimanyusinghgaur and @vmrajas)

---

graphql/resolve/mutation_rewriter.go, line 374 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Why don't we append the errors to retErrors here itself?

can you respond to this one?

---

graphql/resolve/mutation_rewriter.go, line 400 at r1 (raw file):

Previously, vmrajas wrote…

Done.

don't see this one updated?

---

graphql/resolve/mutation_rewriter.go, line 390 at r2 (raw file):

	val, _ := m.ArgValue(schema.InputArgName).([]interface{})

	var ret []*UpsertMutation

Add comments about what do these variables ret, fragments, and frags hold? And why we need both frags and fragments

---

graphql/resolve/mutation_rewriter.go, line 409 at r2 (raw file):

			fragments = append(fragments, fragment)
			var tmp []*mutationFragment
			tmp = append(tmp, fragment)

Why do we need temp here and why can't we just append to frags directly?

---

graphql/resolve/mutation_rewriter.go, line 446 at r2 (raw file):

	}

	newNodes := make(map[string]schema.Type)

Add a comment about what this holds.

---

graphql/resolve/mutation_rewriter.go, line 448 at r2 (raw file):

	newNodes := make(map[string]schema.Type)
	for _, frag := range fragments {
		// squashFragments puts all the new nodes into the first fragment, so we only

Comment not valid anymore. Update it to reflect the new logic.

---

graphql/resolve/mutation_rewriter.go, line 483 at r2 (raw file):

		node := strings.TrimPrefix(frag[0].
			fragment.(map[string]interface{})["uid"].(string), "_:")

Is this safe to do or can it result in a panic? Would the first element of the fragment always be an object and would it always have a string uid?

---

graphql/resolve/mutation_rewriter.go, line 1946 at r2 (raw file):

	// Iterate on fields and call the same function recursively.

newline not required

[vmrajas]

Reviewable status: 2 of 16 files reviewed, 8 unresolved discussions (waiting on @abhimanyusinghgaur and @pawanrawal)

---

ee/acl/acl_test.go, line 232 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Can this be kept the same as before for backward compatibility?

As discussed, not keeping it the same

---

graphql/resolve/mutation_rewriter.go, line 374 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

can you respond to this one?

All graphql errors reported per input value need to be collated into "gqlErrors" which are then appended to retErrors. This helps in maintaining backward compatibility at a few places in .yaml files.

---

graphql/resolve/mutation_rewriter.go, line 400 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

don't see this one updated?

I renamed at one of the places. Forgot to rename it here.

---

graphql/resolve/mutation_rewriter.go, line 390 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Add comments about what do these variables ret, fragments, and frags hold? And why we need both frags and fragments

Done. Its needed because frags is used by authorizeNodes function. fragments is used in making upserts. These things will become cleaner after refactoring Update Rewriter as then frags could become part of the Object on which it acts.

---

graphql/resolve/mutation_rewriter.go, line 409 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Why do we need temp here and why can't we just append to frags directly?

Done.

---

graphql/resolve/mutation_rewriter.go, line 446 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Add a comment about what this holds.

Done.

---

graphql/resolve/mutation_rewriter.go, line 448 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Comment not valid anymore. Update it to reflect the new logic.

Done.

---

graphql/resolve/mutation_rewriter.go, line 483 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Is this safe to do or can it result in a panic? Would the first element of the fragment always be an object and would it always have a string uid?

This is old code. It will be removed. The corresponding function NewFromMutationResult also has the same code snippet. Yes, it is guranteed. This is because at this stage we know that each root level node will have a variable name associated on account of it being a blank UID.

---

graphql/resolve/mutation_rewriter.go, line 1946 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

newline not required

Done.

[pawanrawal]

Reviewed 1 of 9 files at r1, 4 of 10 files at r2, 9 of 10 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @abhimanyusinghgaur and @vmrajas)

---

graphql/e2e/common/mutation.go, line 3432 at r3 (raw file):

	require.NoError(t, err)

	require.Equal(t, actualResult.AddStudent.Student[0].Xid, "HS1")

instead of all these equal statements would have been easier to compare the JSON?

---

graphql/e2e/common/mutation.go, line 4860 at r3 (raw file):

	// Add Owner
	query := &GraphQLParams{
		Query: `mutation {

What's up with the indentation of this one?

---

graphql/e2e/common/mutation.go, line 4897 at r3 (raw file):

      						},
      						name: "project",
      						datasets:

indentation is off

---

graphql/e2e/common/mutation.go, line 4912 at r3 (raw file):

    			project  {
      				id
					owner {

indent

---

graphql/resolve/mutation_test.go, line 276 at r3 (raw file):

			}
			mut := test.GetMutation(t, op)
			// rewriterToTest := rewriterFactory()

can be removed?

[vmrajas]

Reviewable status: 9 of 16 files reviewed, 5 unresolved discussions (waiting on @abhimanyusinghgaur and @pawanrawal)

---

graphql/e2e/common/mutation.go, line 3432 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

instead of all these equal statements would have been easier to compare the JSON?

Done.

---

graphql/e2e/common/mutation.go, line 4860 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

What's up with the indentation of this one?

Done.

---

graphql/e2e/common/mutation.go, line 4897 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

indentation is off

Done.

---

graphql/e2e/common/mutation.go, line 4912 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

indent

Done.

---

graphql/resolve/mutation_test.go, line 276 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

can be removed?

Done.