graphql: @dgraph(pred: "...") with @search #5019
Conversation
abhimanyusinghgaur
added
the
area/graphql
There was a problem hiding this comment.
The validation code needs to be moved to before schema generation. See the notes inline.
Reviewable status: 0 of 3 files reviewed, 9 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @MichaelJCompton, and @pawanrawal)
graphql/schema/gqlschema_test.yml, line 711 at r2 (raw file):
- name: "Conflicting GraphQL types for predicate"
generally I prefer all the test cases to be in distinct tests, or all in one test.
I'd be ok with one test here called something like "@dgraph type validation" or a different test for each case. I couldn't see a reason to have this test and the "Conflicting dgraph types for predicate" one split up.
The line I'd draw is if the test fails, will the test name + its output direct us to exactly what needs looking at.
I also think theres a couple of other cases
- one with String and the other String! (that should pass)
- same thing with variations on [String], [String!], [String]! and [String!]! (again should pass)
graphql/schema/gqlschema_test.yml, line 722 at r2 (raw file):
} errlist: [ {"message": "Type: Y; Field: f1 has its GraphQL Type: Int; which is different from a previous field with same dgraph predicate.",
Let's try and add all the info into the error message and a way that they can fix or a reason. Here I think something like
Type Y; Field f1: has type Int, which is different to type X, field f1, which has the same @dgraph directive but type String. These fields must have either the same GraphQL types, or use different Dgraph predicates.
graphql/schema/gqlschema_test.yml, line 731 at r2 (raw file):
name: "Conflicting dgraph types for predicate" input: | type X @secret(field: "f", pred: "pwd"){
Great catch!!! I hadn't thought of that one.
graphql/schema/gqlschema_test.yml, line 741 at r2 (raw file):
{"message": "Type: Y; Field: f1 has its dgraph Type: [string]; which is different from a previous field with same dgraph predicate.", "locations":[{"line":5, "column":3}]}, {"message": "Type: Y; Field: f2 has its dgraph Type: string; which is different from a previous field with same dgraph predicate.",
This one will need some sort of different error message to note that it's about a secret. Something like...
Type Y; Field f1: has the @dgraph predicate, but that conflicts with type X @secret directive on the same predicate. @secret predicates are stored encrypted and so the same predicate can't be used as a String.
...my general rule about error messages is that it should point you to exactly where the error is and tell you how to fix it. If they find out that there's an error and have to google docs, or seriously eyeball a whole schema, then it needs a more helpful error.
graphql/schema/schemagen.go, line 111 at r2 (raw file):
} gqlErrList := preGQLValidation(doc)
you can probably do the error checking/validation in here
graphql/schema/schemagen.go, line 197 at r2 (raw file):
// genDgSchema generates Dgraph schema from a valid graphql schema. func genDgSchema(gqlSch *ast.Schema, definitions []string) (string, gqlerror.List) {
The error checking doesn't belong here. It should be in the validation code.
graphql/schema/schemagen.go, line 250 at r2 (raw file):
if edge, ok := dgPreds[fname]; ok && edge.gqlType != f.Type.Name() && edge. reverse == "" { errs = append(errs, gqlerror.ErrorPosf(f.Position,
This doesn't belong here. If this is a validation check, then it should be in the rules.go file. By the time we get to here we should be assuming a valid schema.
graphql/schema/schemagen.go, line 299 at r2 (raw file):
id := f.Directives.ForName(idDirective) if id != nil { upsertStr = "@upsert "
let's add another rule for @id. eg
type A {
f1 : String! @dgraph(pred: "f") @id
...
}
type B {
f2: String @dgraph(pred: "f")
...
}
doesn't make sense, right? How could we ensure the uniqueness of the id?
There was a problem hiding this comment.
Reviewable status: 0 of 5 files reviewed, 9 unresolved discussions (waiting on @dgraph, @golangcibot, @id, @manishrjain, @MichaelJCompton, @pawanrawal, and @secret)
graphql/schema/gqlschema_test.yml, line 711 at r2 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
generally I prefer all the test cases to be in distinct tests, or all in one test.
I'd be ok with one test here called something like "@dgraph type validation" or a different test for each case. I couldn't see a reason to have this test and the "Conflicting dgraph types for predicate" one split up.
The line I'd draw is if the test fails, will the test name + its output direct us to exactly what needs looking at.
I also think theres a couple of other cases
- one with String and the other String! (that should pass)
- same thing with variations on [String], [String!], [String]! and [String!]! (again should pass)
Done.
graphql/schema/gqlschema_test.yml, line 722 at r2 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
Let's try and add all the info into the error message and a way that they can fix or a reason. Here I think something like
Type Y; Field f1: has type Int, which is different to type X, field f1, which has the same @dgraph directive but type String. These fields must have either the same GraphQL types, or use different Dgraph predicates.
Done.
graphql/schema/gqlschema_test.yml, line 731 at r2 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
Great catch!!! I hadn't thought of that one.
Done.
graphql/schema/gqlschema_test.yml, line 741 at r2 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
This one will need some sort of different error message to note that it's about a secret. Something like...
Type Y; Field f1: has the @dgraph predicate, but that conflicts with type X @secret directive on the same predicate. @secret predicates are stored encrypted and so the same predicate can't be used as a String.
...my general rule about error messages is that it should point you to exactly where the error is and tell you how to fix it. If they find out that there's an error and have to google docs, or seriously eyeball a whole schema, then it needs a more helpful error.
Done.
graphql/schema/schemagen.go, line 405 at r1 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
File is not
gofmt-ed with-s(fromgofmt)for index := range f.indexes {
Done.
graphql/schema/schemagen.go, line 111 at r2 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
you can probably do the error checking/validation in here
Done.
Doing it in postGQLValidation as they were easier with *ast.Schema instead of *ast.SchemaDocument
graphql/schema/schemagen.go, line 197 at r2 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
The error checking doesn't belong here. It should be in the validation code.
Done.
graphql/schema/schemagen.go, line 250 at r2 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
This doesn't belong here. If this is a validation check, then it should be in the rules.go file. By the time we get to here we should be assuming a valid schema.
Done.
graphql/schema/schemagen.go, line 299 at r2 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
let's add another rule for @id. eg
type A { f1 : String! @dgraph(pred: "f") @id ... } type B { f2: String @dgraph(pred: "f") ... }doesn't make sense, right? How could we ensure the uniqueness of the id?
Done.
There was a problem hiding this comment.
couple more fixes and should be good
Reviewable status: 0 of 5 files reviewed, 10 unresolved discussions (waiting on @abhimanyusinghgaur, @golangcibot, @manishrjain, @MichaelJCompton, and @pawanrawal)
graphql/schema/gqlschema_test.yml, line 727 at r3 (raw file):
f4: String @dgraph(pred: "f4") f5: String @dgraph(pred: "pwd") }
How about this
type X {
f1: String
}
type Y {
f2: Int @dgraph(pred: "X.f1")
}
Looks like your code could handle this ... can you add a case for that
graphql/schema/gqlschema_test.yml, line 735 at r3 (raw file):
{"message": "Type Y; Field f3: has type X, which is different to type X; field f3, which has the same @dgraph directive but type Y. These fields must have either the same GraphQL types, or use different Dgraph predicates.", "locations":[{"line":12, "column":3}]}, {"message": "Type Y; Field f4: doesn't have @id directive, which conflicts with type X; field f4, which has the same @dgraph directive along with @id directive. Both these fields must either use @id directive, or use different Dgraph predicates.",
I actually think we should just disallow this case altogether.
Think about your @id PR and uniqueness - we couldn't guarantee uniqueness because the types would be different so we'd allow through mutations that set 2x the same xid value.
Just disallow any other @dgraph to reference a field that has @id on it (like in the secret case).
graphql/schema/gqlschema_test.yml, line 845 at r3 (raw file):
- name: "@dgraph predicate type validation gives no errors with non-null variations"
nice
graphql/schema/rules.go, line 98 at r3 (raw file):
// implements. If we get a parent interface, that means this field gets validated // during the validation of that interface. So, no need to validate this field here. if fname != typName+"."+f.Name && parentInterface(gqlSch, def, f.Name) == nil {
Didn't understand this test.
Also why can't a field with @dgraph map to "x.y"?
Should it just test directly if there is an @dgraph?
graphql/schema/schemagen.go, line 299 at r2 (raw file):
Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
Done.
and a test to go with it. nice job!
graphql/schema/schemagen.go, line 287 at r3 (raw file):
if parentInt == nil { edge, ok := dgPreds[fname]
edge -> pred
graphql/schema/schemagen.go, line 312 at r3 (raw file):
} } if parentInt == nil {
this looks like the same bit of code twice. can we abstract it to a function so we don't repeat ... how about one that returns the right pred and we can put it straight in the spot
graphql/schema/schemagen.go, line 328 at r3 (raw file):
} } if fd != nil {
unreleated, but can we rename this please ... fd isn't great
graphql/schema/schemagen.go, line 358 at r3 (raw file):
indexStr := "" if len(f.indexes) > 0 { indexes := make([]string, 0)
why is f.indexes a string->bool map ... why not just put it in a a list of string and sort it directly without this loop?
graphql/schema/schemagen_test.yml, line 458 at r3 (raw file):
} B.name: string .
please add a test where the indexes overlap.
Also we need some testing around interfaces .
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 11 unresolved discussions (waiting on @golangcibot, @manishrjain, @MichaelJCompton, and @pawanrawal)
graphql/schema/gqlschema_test.yml, line 727 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
How about this
type X { f1: String } type Y { f2: Int @dgraph(pred: "X.f1") }Looks like your code could handle this ... can you add a case for that
Done.
graphql/schema/gqlschema_test.yml, line 735 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
I actually think we should just disallow this case altogether.
Think about your
@idPR and uniqueness - we couldn't guarantee uniqueness because the types would be different so we'd allow through mutations that set 2x the same xid value.Just disallow any other
@dgraphto reference a field that has@idon it (like in the secret case).
Done.
Didn't do anything for this, as you suggested.
graphql/schema/gqlschema_test.yml, line 845 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
nice
Done.
graphql/schema/rules.go, line 98 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
Didn't understand this test.
Also why can't a field with
@dgraphmap to "x.y"?Should it just test directly if there is an
@dgraph?
Done.
I hadn't thought of @dgraph(pred: "X.y") case earlier!
graphql/schema/rules.go, line 124 at r4 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
File is not
gofmt-ed with-s(fromgofmt)for fname := range interfacePreds[intr2] {
Done.
graphql/schema/schemagen.go, line 287 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
edge -> pred
Done.
graphql/schema/schemagen.go, line 312 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
this looks like the same bit of code twice. can we abstract it to a function so we don't repeat ... how about one that returns the right pred and we can put it straight in the spot
Done.
graphql/schema/schemagen.go, line 328 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
unreleated, but can we rename this please ... fd isn't great
Done.
graphql/schema/schemagen.go, line 358 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
why is f.indexes a string->bool map ... why not just put it in a a list of string and sort it directly without this loop?
That's because, when merging two predicates which have different @search but same @dgraph(pred: ...), its easier with a map. Otherwise, with a list I will have to find duplicates, if @search had same indexes.
graphql/schema/schemagen_test.yml, line 458 at r3 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
please add a test where the indexes overlap.
Also we need some testing around interfaces .
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 7 unresolved discussions (waiting on @abhimanyusinghgaur, @golangcibot, @manishrjain, @MichaelJCompton, and @pawanrawal)
graphql/schema/gqlschema_test.yml, line 706 at r5 (raw file):
} errlist: [ {"message": "Type X; has a secret directive and field of the same name f1",
We should only have one or other of these errors
graphql/schema/gqlschema_test.yml, line 708 at r5 (raw file):
Type X; Field f1: has the @dgraph predicate
...no it doesn't
graphql/schema/gqlschema_test.yml, line 739 at r5 (raw file):
} errlist: [ {"message": "Type X; implements interfaces [V W], all of which have fields with @dgraph predicate: ff1. These fields must use different Dgraph predicates.",
let's chat about this one ... I didn't understand
graphql/schema/gqlschema_test.yml, line 741 at r5 (raw file):
{"message": "Type X; implements interfaces [V W], all of which have fields with @dgraph predicate: ff1. These fields must use different Dgraph predicates.", "locations":[{"line":10, "column":6}]}, {"message": "Type X; Field f9: has the @dgraph directive, which conflicts with interface W; field f4, that this type implements. These fields must use different Dgraph predicates.",
so you can't use the same dgraph predicate within a type?
graphql/schema/gqlschema_test.yml, line 859 at r5 (raw file):
f1: [X] @dgraph(pred: "f1") }
add a passing X.f one like
type X { f: String }
type Y { f: String @dgraph(pred: "X.f")
and a passing one with @id in two types mapped to the same underlying field.
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 7 unresolved discussions (waiting on @dgraph, @golangcibot, @manishrjain, @MichaelJCompton, and @pawanrawal)
graphql/schema/gqlschema_test.yml, line 706 at r5 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
We should only have one or other of these errors
Done.
graphql/schema/gqlschema_test.yml, line 708 at r5 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
Type X; Field f1: has the @dgraph predicate...no it doesn't
Done.
graphql/schema/gqlschema_test.yml, line 739 at r5 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
let's chat about this one ... I didn't understand
ok
graphql/schema/gqlschema_test.yml, line 741 at r5 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
so you can't use the same dgraph predicate within a type?
No, as it will result in two fields having same name in a dgraph type, like below:
type X {
...
ff4
...
ff4
...
}
which is a wrong dgraph schema
graphql/schema/gqlschema_test.yml, line 859 at r5 (raw file):
Previously, MichaelJCompton (Michael Compton) wrote…
add a passing X.f one like
type X { f: String }
type Y { f: String @dgraph(pred: "X.f")and a passing one with
@idin two types mapped to the same underlying field.
Done.
We aren’t quite handling indexes correctly when the `@dgraph` predicate has a `@search`.
```graphql
type Country {
...
name: String! @dgraph(pred: "name")
}
type Author {
name: String! @dgraph(pred: "name") @search(by: [term])
...
}
```
This will write the predicate into the schema twice and only one will apply. We should be smarter about this and do the following things:
* make sure we only write each predicate once.
* make sure all the predicate versions follow the below rules:
1. all versions have the same dgraph type
2. all versions either use @id, or none of them uses it
3. all versions either use @secret, or none of them uses it
4. a field from a type is not using the same predicate as another field in the interface that the type implements
5. Two different interfaces, which are implemented by the same type, can't use same dgraph predicate for their fields
* set the indexes to the union of all the @search on the same pred name.
This PR takes care of all the above things.
We aren’t quite handling indexes correctly when the
@dgraphpredicate has a@search.This will write the predicate into the schema twice and only one will apply. We should be smarter about this and do the following things:
This PR takes care of all the above things.
This change is