Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat(GraphQL): This PR allow multiple @id fields in a type. #7235

Merged
merged 48 commits into from
Feb 22, 2021

Conversation

JatinDev543
Copy link
Contributor

@JatinDev543 JatinDev543 commented Dec 31, 2020

Currently, we only allow single xid or external id fields in a type with the @id directive.
Now, we are extending this to have multiple external @id fields in a type. Every such field will be unique over all the nodes of that type. As of now, fields with @id directive will be immutable. get query will work for all the fields with the @id type.
RFC:https://discuss.dgraph.io/t/wip-rfc-allow-multiple-unique-fields-in-graphql-schema/12008


This change is Reviewable

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Dec 31, 2020
@netlify
Copy link

netlify bot commented Jan 4, 2021

✔️ Deploy preview for dgraph-docs ready!

🔨 Explore the source changes: 3864368

🔍 Inspect the deploy logs: https://app.netlify.com/sites/dgraph-docs/deploys/5ff2c148df37d00007136457

😎 Browse the preview: https://deploy-preview-7235--dgraph-docs.netlify.app

@JatinDev543 JatinDev543 changed the title Feat(GraphQL): This PR adds allow multiple @id fields in a type. Feat(GraphQL): This PR allow multiple @id fields in a type. Jan 4, 2021
Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good.
We also need to add some e2e tests for mutations and queries using multiple XID fields.

For mutations, we should also write tests for error cases where we first add one object with multiple XIDs, then try to add another object keeping only one of the XIDs same, then another with some other XID same, then another with all the XIDs same. All the trials apart from the first one should result in errors.

Reviewed 6 of 7 files at r1.
Reviewable status: 6 of 9 files reviewed, 7 unresolved discussions (waiting on @jatindevdg, @MichaelJCompton, and @pawanrawal)


graphql/resolve/query_rewriter.go, line 476 at r2 (raw file):

xidArgName := query.XIDArg()

xidArgNameToDgPredMap := query.XIDArgs()


graphql/resolve/query_test.yaml, line 3167 at r2 (raw file):

        Author : Book.Author
      }
    }

we should also add a case where there are multiple @id fields in args but no ID field.


graphql/schema/rules.go, line 684 at r2 (raw file):

var idDirectiveFields []*ast.FieldDefinition

should be removed now


graphql/schema/rules.go, line 690 at r2 (raw file):

		if d := field.Directives.ForName(idDirective); d != nil {
			idDirectiveFields = append(idDirectiveFields, field)
		}

should be removed now


graphql/schema/wrappers.go, line 124 at r2 (raw file):

XIDArg()

We should name it XIDArgs() now that there are multiple XIDs.


graphql/schema/wrappers.go, line 1028 at r2 (raw file):

 0

this can be skipped


graphql/schema/wrappers.go, line 1052 at r2 (raw file):

0

can be skipped

Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 6 of 9 files reviewed, 12 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, @MichaelJCompton, and @pawanrawal)


graphql/resolve/query_test.yaml, line 3152 at r2 (raw file):

  gqlquery: |
    query {
      getBook(id: "0x1",title:"GraphQL",ISBN:"001HB") {

If all of these are nullable, then what happens if I don't give any of them? That is

getBook {
  id
  title
}

graphql/resolve/query_test.yaml, line 3167 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

we should also add a case where there are multiple @id fields in args but no ID field.

yup


graphql/schema/gqlschema.go, line 1472 at r2 (raw file):

// fieldMulti returns true if multiple field in fields satisfies pred
func fieldMulti(fields ast.FieldList, pred func(*ast.FieldDefinition) bool) bool {

What is pred, should be cond or condition? Do we even need this function if its just being used at one place?


graphql/schema/gqlschema.go, line 1749 at r2 (raw file):

func addGetQuery(schema *ast.Schema, defn *ast.Definition, generateSubscription bool) {
	hasIDField := hasID(defn)
	hasXIDField := hasXID(defn)

this and the below function are both checking for xids, would have been better if there was just function which returned number of xid fields and then the result could have been used to do whatever you are doing below


graphql/schema/wrappers.go, line 1048 at r2 (raw file):

}

func (f *field) IDArgValue() (xids map[string]*string, uid uint64, err error) {

Why are we returning a pointer to the string, we can just return map[string]string


graphql/schema/wrappers.go, line 1052 at r2 (raw file):

	passwordField := f.Type().PasswordField()
	xidArgName := ""
	xidArgToVal := make(map[string]*string, 0)

Do you need this? Why not just assign directly to xids?

Copy link
Contributor Author

@JatinDev543 JatinDev543 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 15 files reviewed, 12 unresolved discussions (waiting on @abhimanyusinghgaur, @filter, @MichaelJCompton, and @pawanrawal)


graphql/resolve/query_rewriter.go, line 476 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
xidArgName := query.XIDArg()

xidArgNameToDgPredMap := query.XIDArgs()

done.


graphql/resolve/query_test.yaml, line 3152 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

If all of these are nullable, then what happens if I don't give any of them? That is

getBook {
  id
  title
}

That will be an empty query.
Corresponding Dgraph query will be
query {
getBook(func: uid(0x0)) @filter(type(Book)) {
Book.id : uid
}
}


graphql/resolve/query_test.yaml, line 3167 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

yup

done.


graphql/schema/gqlschema.go, line 1472 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

What is pred, should be cond or condition? Do we even need this function if its just being used at one place?

removed.


graphql/schema/gqlschema.go, line 1749 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

this and the below function are both checking for xids, would have been better if there was just function which returned number of xid fields and then the result could have been used to do whatever you are doing below

done.


graphql/schema/rules.go, line 684 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
var idDirectiveFields []*ast.FieldDefinition

should be removed now

removed.


graphql/schema/rules.go, line 690 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
		if d := field.Directives.ForName(idDirective); d != nil {
			idDirectiveFields = append(idDirectiveFields, field)
		}

should be removed now

removed.


graphql/schema/wrappers.go, line 124 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
XIDArg()

We should name it XIDArgs() now that there are multiple XIDs.

changed.


graphql/schema/wrappers.go, line 1028 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
 0

this can be skipped

skipped.


graphql/schema/wrappers.go, line 1048 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Why are we returning a pointer to the string, we can just return map[string]string

yeah, changed.
Previously also we are returning address of it, so thought of using the same.


graphql/schema/wrappers.go, line 1052 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
0

can be skipped

skipped.


graphql/schema/wrappers.go, line 1052 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Do you need this? Why not just assign directly to xids?

changed.

@JatinDev543
Copy link
Contributor Author

Can you also add more tests for update_mutation_test.yaml, delete_mutation_test.yaml .
It will also be good to have a one test each for auth .yaml files and auth e2e tests (if possible). It will ensure robustness of code.

Reviewable status: 14 of 19 files reviewed, 18 unresolved discussions (waiting on @abhimanyusinghgaur, @filter, @jatindevdg, @manishrjain, @MichaelJCompton, and @pawanrawal)

added update and delete mutation tests. Auth tests not required.

Copy link
Contributor

@vmrajas vmrajas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 18 files at r3, 5 of 15 files at r4, 11 of 11 files at r5.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @abhimanyusinghgaur, @filter, @jatindevdg, @manishrjain, @MichaelJCompton, and @pawanrawal)


graphql/e2e/common/mutation.go, line 5825 at r5 (raw file):

                     }`,
			error: `couldn't rewrite mutation updateEmployer because failed to rewrite mutation payload because field reg_No cannot be empty`,
		},

Good job with fixing indentation.


graphql/e2e/directives/schema.graphql, line 368 at r3 (raw file):

Previously, vmrajas wrote…

To bring more diversity in tests. It will be better if one of reg_No or emp_Id is of type Int! . IDs can now also be integers.

I believe this is also done.


graphql/e2e/directives/schema.graphql, line 374 at r5 (raw file):

    company: String! @id
    worker: [Worker]
}

nit: Add new line at end of file.


graphql/resolve/add_mutation_test.yaml, line 3332 at r3 (raw file):

Previously, JatinDevDG (Jatin Dev) wrote…

added tests

Add two more tests similar to "Add type with multiple XID fields at deep level".
1.: Book1 exists. The author should be linked to existing book and deletejson should be formed.
2. Book1 and Book2 both exist and point to two different books. the mutation should link it to one of the existing book.


graphql/resolve/mutation_rewriter.go, line 1268 at r5 (raw file):

	xids := typ.XIDFields()
	if len(xids) != 0 {
		var xidsWithoutNodes int

Can this be renamed to nonExistingXIDs . Also can you add a comment over here over what these variables store.


graphql/resolve/mutation_rewriter.go, line 1317 at r5 (raw file):

					// Node with XIDs does not exist. It means this is a new node.
					// This node will be created later.
					if xidsWithoutNodes == len(xids)-1 {

Add a comment here explaining why only in this case we do this processing.


graphql/resolve/mutation_rewriter.go, line 1336 at r5 (raw file):

						// Set existenceQueryResult to _:variable. This is to make referencing to
						// this node later easier.
						idExistence[variable] = fmt.Sprintf("_:%s", variable)

We need to consider a test case in which the node is created and referenced by a different XID (other than last). The following test case may fail if this is not handled.
I know a way to handle this. Lets discuss this offline.
Consider this schema and mutation:

type Person {
id: String! @id
name: String! @id
friends: [Person]
closeFriends: [Person]
}

addPerson( input: [ { id: "1", name: "Person1", closeFriends: [ { add a new friend}]friends: [ { reference by id } ] } ]) {
person {
id
}
}


graphql/resolve/mutation_rewriter.go, line 1352 at r5 (raw file):

				err := errors.Errorf("field %s cannot be empty", xid.Name())
				retErrors = append(retErrors, err)
				missingXid = true

Add a comment here on why this is done and error thrown after the for loop.


graphql/schema/wrappers.go, line 2409 at r5 (raw file):

		}
	}
	sort.Slice(xids, func(i, j int) bool { return xids[i].Name() < xids[j].Name() })

Add a comment that XIDs are sorted by name to ensure consistency.

Copy link
Contributor Author

@JatinDev543 JatinDev543 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 16 of 21 files reviewed, 20 unresolved discussions (waiting on @abhimanyusinghgaur, @filter, @jatindevdg, @manishrjain, @MichaelJCompton, @pawanrawal, and @vmrajas)


graphql/e2e/common/mutation.go, line 5825 at r5 (raw file):

Previously, vmrajas wrote…

Good job with fixing indentation.

thanks.


graphql/e2e/directives/schema.graphql, line 368 at r3 (raw file):

Previously, vmrajas wrote…

I believe this is also done.

yeah.


graphql/e2e/directives/schema.graphql, line 374 at r5 (raw file):

Previously, vmrajas wrote…

nit: Add new line at end of file.

done.


graphql/resolve/add_mutation_test.yaml, line 3332 at r3 (raw file):

Previously, vmrajas wrote…

Add two more tests similar to "Add type with multiple XID fields at deep level".
1.: Book1 exists. The author should be linked to existing book and deletejson should be formed.
2. Book1 and Book2 both exist and point to two different books. the mutation should link it to one of the existing book.

added.


graphql/resolve/mutation_rewriter.go, line 1268 at r5 (raw file):

Previously, vmrajas wrote…

Can this be renamed to nonExistingXIDs . Also can you add a comment over here over what these variables store.

done.


graphql/resolve/mutation_rewriter.go, line 1317 at r5 (raw file):

Previously, vmrajas wrote…

Add a comment here explaining why only in this case we do this processing.

done.


graphql/resolve/mutation_rewriter.go, line 1352 at r5 (raw file):

Previously, vmrajas wrote…

Add a comment here on why this is done and error thrown after the for loop.

done.


graphql/schema/wrappers.go, line 2409 at r5 (raw file):

Previously, vmrajas wrote…

Add a comment that XIDs are sorted by name to ensure consistency.

done.

Copy link
Contributor Author

@JatinDev543 JatinDev543 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 6 of 21 files reviewed, 20 unresolved discussions (waiting on @abhimanyusinghgaur, @filter, @id, @manishrjain, @MichaelJCompton, @pawanrawal, and @vmrajas)


graphql/resolve/mutation_rewriter.go, line 1336 at r5 (raw file):

Previously, vmrajas wrote…

We need to consider a test case in which the node is created and referenced by a different XID (other than last). The following test case may fail if this is not handled.
I know a way to handle this. Lets discuss this offline.
Consider this schema and mutation:

type Person {
id: String! @id
name: String! @id
friends: [Person]
closeFriends: [Person]
}

addPerson( input: [ { id: "1", name: "Person1", closeFriends: [ { add a new friend}]friends: [ { reference by id } ] } ]) {
person {
id
}
}

done

Copy link
Contributor

@vmrajas vmrajas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update and adding a test case for upsert with multiple XIDs.
Can you add one more test case, a test case in which only one of the variable for XID existence query exists and the other does not. It may fail this test case. The failure has more to do with upsert, I can take the fix for this if needed.

Reviewed 2 of 5 files at r7, 9 of 12 files at r8, 6 of 6 files at r9.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @abhimanyusinghgaur, @filter, @id, @manishrjain, @MichaelJCompton, @pawanrawal, and @vmrajas)

@JatinDev543
Copy link
Contributor Author

Thanks for the update and adding a test case for upsert with multiple XIDs.
Can you add one more test case, a test case in which only one of the variable for XID existence query exists and the other does not. It may fail this test case. The failure has more to do with upsert, I can take the fix for this if needed.

Reviewed 2 of 5 files at r7, 9 of 12 files at r8, 6 of 6 files at r9.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @abhimanyusinghgaur, @filter, @id, @manishrjain, @MichaelJCompton, @pawanrawal, and @vmrajas)

added test case for upsert mutation.

Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 21 of 22 files reviewed, 13 unresolved discussions (waiting on @abhimanyusinghgaur, @id, @jatindevdg, @manishrjain, @MichaelJCompton, and @vmrajas)


graphql/resolve/query_test.yaml, line 3266 at r9 (raw file):

  gqlquery: |
    query {
    	getBook(id: "0x1", title: "GraphQL", ISBN: "001HB") {

What's up with the indentation here?

Copy link
Contributor Author

@JatinDev543 JatinDev543 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 20 of 22 files reviewed, 13 unresolved discussions (waiting on @abhimanyusinghgaur, @id, @manishrjain, @MichaelJCompton, @pawanrawal, and @vmrajas)


graphql/resolve/query_test.yaml, line 3266 at r9 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

What's up with the indentation here?

fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

4 participants