-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 allows updatable and nullable @id fields. #7736
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 29 of 29 files at r1.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @jatindevdg)
graphql/e2e/auth/schema.graphql, line 855 at r1 (raw file):
) { company: String! @id company_Id: String @id
companyId
graphql/e2e/auth/schema.graphql, line 861 at r1 (raw file):
type Worker { reg_No: Int @id
regNo
graphql/e2e/auth/schema.graphql, line 862 at r1 (raw file):
type Worker { reg_No: Int @id UniqueID: Int @id
uniqueID
graphql/e2e/common/mutation.go, line 6499 at r1 (raw file):
} }`, error: "mutation updateEmployer failed because multiple nodes are selected in filter" +
failed because only one node is allowed in the filter while updating fields with @id directive
graphql/e2e/directives/schema.graphql, line 384 at r1 (raw file):
type Worker { name: String! reg_No: Int @id
regNo
graphql/e2e/directives/schema.graphql, line 385 at r1 (raw file):
name: String! reg_No: Int @id UniqueID: Int @id
uniqueId
graphql/e2e/directives/schema.graphql, line 391 at r1 (raw file):
type Employer { company: String! @id company_Id: String @id
companyId
graphql/e2e/normal/schema.graphql, line 322 at r1 (raw file):
name: String! reg_No: Int @id UniqueID: Int @id
uniqueId
graphql/e2e/normal/schema.graphql, line 323 at r1 (raw file):
reg_No: Int @id UniqueID: Int @id emp_Id: String! @id
empId
graphql/e2e/normal/schema.graphql, line 328 at r1 (raw file):
type Employer { company: String! @id company_Id: String @id
companyId
graphql/resolve/mutation_rewriter.go, line 1815 at r1 (raw file):
// In this case, as obj is the correct definition of the object, we update variableObjMap oldObj := xidMetadata.variableObjMap[variable] if len(oldObj) == 1 && len(obj) > 1 {
if there is a test case that was broken earlier, lets add that here. Otherwise lets keep it as it is
graphql/resolve/schema.graphql, line 396 at r1 (raw file):
title: String @id ISBN: String @id BookId: Int @id
bookedId
graphql/resolve/update_mutation_test.yaml, line 1972 at r1 (raw file):
updateBook(input: $patch) { book { title
spacing seems a bit messed up
graphql/schema/gqlschema.go, line 1290 at r1 (raw file):
if len(nonIDFields) == 0 { // The user might just have an predicate with reverse edge id field and nothing else. //We don't generate patch type in that case.
// We
graphql/schema/gqlschema_test.yml, line 633 at r1 (raw file):
input: | type X { f1: Float! @id
this should remain?
graphql/schema/rules.go, line 2076 at r1 (raw file):
dir *ast.Directive, secrets map[string]x.Sensitive) gqlerror.List { if field.Type.NamedType == "String" ||
Isn't this check applicable for non-nullable id fields as well?
graphql/schema/testdata/schemagen/output/custom-dql-query-with-subscription.graphql, line 439 at r1 (raw file):
input UserPatch { screen_name: String
screenName
There was a problem hiding this 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 31 files reviewed, 17 unresolved discussions (waiting on @id, @jatindevdg, and @pawanrawal)
graphql/e2e/auth/schema.graphql, line 855 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
companyId
done.
graphql/e2e/auth/schema.graphql, line 861 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
regNo
done
graphql/e2e/auth/schema.graphql, line 862 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
uniqueID
changed.
graphql/e2e/common/mutation.go, line 6499 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
failed because only one node is allowed in the filter while updating fields with @id directive
changed.
graphql/e2e/directives/schema.graphql, line 384 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
regNo
done.
graphql/e2e/directives/schema.graphql, line 385 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
uniqueId
done.
graphql/e2e/directives/schema.graphql, line 391 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
companyId
done.
graphql/e2e/normal/schema.graphql, line 322 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
uniqueId
done.
graphql/e2e/normal/schema.graphql, line 323 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
empId
done.
graphql/e2e/normal/schema.graphql, line 328 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
companyId
done.
graphql/resolve/mutation_rewriter.go, line 1815 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
if there is a test case that was broken earlier, lets add that here. Otherwise lets keep it as it is
currently, it doesn't have any effect, because if we have more than one xid fields and we try to add reference in single mutation then that gives duplicate xid error. That itself is a bug. i have created ticket for it. we need to do some refactoring for this. There are couple of scenarios where duplicate xid error is not behaving correctly. while fixing that we can fix this also. Added TODO for this in code.
graphql/resolve/schema.graphql, line 396 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
bookedId
changed.
graphql/schema/gqlschema.go, line 1290 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
// We
changed.
graphql/schema/gqlschema_test.yml, line 633 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
this should remain?
added.
graphql/schema/rules.go, line 2076 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Isn't this check applicable for non-nullable id fields as well?
Actually, NamedType, cover both nullable and non-nullable values.
graphql/schema/testdata/schemagen/output/custom-dql-query-with-subscription.graphql, line 439 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
screenName
changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 13 of 31 files reviewed, 17 unresolved discussions (waiting on @id and @pawanrawal)
graphql/resolve/update_mutation_test.yaml, line 1972 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
spacing seems a bit messed up
fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 14 of 15 files at r2, 4 of 4 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jatindevdg)
graphql/resolve/mutation_rewriter.go, line 1832 at r3 (raw file):
// In this case, as obj is the correct definition of the object, we update variableObjMap oldObj := xidMetadata.variableObjMap[variable] // TODO(GRAOHQL): This condition also needs to change in accordance with multiple xids.
TODO(jatin)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 29 of 31 files reviewed, 1 unresolved discussion (waiting on @pawanrawal)
graphql/resolve/mutation_rewriter.go, line 1832 at r3 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
TODO(jatin)
changed
Currently, @id fields are not updatable and also we enforce them to be non-null. In this PR we are allowing @id fields to be updatable and nullable. RFC: https://discuss.dgraph.io/t/rfc-mutable-and-null-able-id-field-in-graphql/13663 Related Posts: https://discuss.dgraph.io/t/nullable-id-field/13296 https://discuss.dgraph.io/t/editable-id-fields/13295 https://discuss.dgraph.io/t/rfc-allow-multiple-unique-fields-in-graphql-schema/12008
Currently, @id fields are not updatable and also we enforce them to be non-null. In this PR we are allowing @id fields to be updatable and nullable. RFC: https://discuss.dgraph.io/t/rfc-mutable-and-null-able-id-field-in-graphql/13663 Related Posts: https://discuss.dgraph.io/t/nullable-id-field/13296 https://discuss.dgraph.io/t/editable-id-fields/13295 https://discuss.dgraph.io/t/rfc-allow-multiple-unique-fields-in-graphql-schema/12008
Currently, @id fields are not updatable and also we enforce them to be non-null. In this PR we are allowing @id fields to be updatable and nullable. RFC: https://discuss.dgraph.io/t/rfc-mutable-and-null-able-id-field-in-graphql/13663 Related Posts: https://discuss.dgraph.io/t/nullable-id-field/13296 https://discuss.dgraph.io/t/editable-id-fields/13295 https://discuss.dgraph.io/t/rfc-allow-multiple-unique-fields-in-graphql-schema/12008
Currently, @id fields are not updatable and also we enforce them to be non-null. In this PR we are allowing @id fields to be updatable and nullable. RFC: https://discuss.dgraph.io/t/rfc-mutable-and-null-able-id-field-in-graphql/13663 Related Posts: https://discuss.dgraph.io/t/nullable-id-field/13296 https://discuss.dgraph.io/t/editable-id-fields/13295 https://discuss.dgraph.io/t/rfc-allow-multiple-unique-fields-in-graphql-schema/12008
Currently, @id fields are not updatable and also we enforce them to be non-null.
In this PR we are allowing @id fields to be updatable and nullable.
RFC:https://discuss.dgraph.io/t/rfc-mutable-and-null-able-id-field-in-graphql/13663
Related Posts:
https://discuss.dgraph.io/t/nullable-id-field/13296
https://discuss.dgraph.io/t/editable-id-fields/13295
https://discuss.dgraph.io/t/rfc-allow-multiple-unique-fields-in-graphql-schema/12008
This change is