-
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
fix(GraphQL): AND/OR filters now accept an array while also accepting objects. #6801
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.
Reviewable status: 0 of 80 files reviewed, 14 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, and @vvbalaji-dgraph)
go.sum, line 7 at r4 (raw file):
contrib.go.opencensus.io/exporter/prometheus v0.1.0 h1:SByaIoWwNgMdPSgl5sMqM2KDE5H/ukPWBRo314xiDvg= contrib.go.opencensus.io/exporter/prometheus v0.1.0/go.mod h1:cGFniUXGZlKRjzOyuZJ6mgB+PgBcCIa79kEKR8YCW+A= github.com/99designs/gqlgen v0.13.0/go.mod h1:NV130r6f4tpRWuAI+zsrSdooO/eWUv+Gyyoi3rEfXIk=
can you try and delete this line and see if it still comes back after running go mod tidy
?n
graphql/e2e/common/common.go, line 354 at r4 (raw file):
t.Run("alias works for queries", queryWithAlias) t.Run("cascade directive", queryWithCascade) t.Run("Filter Queries", filterQueries)
The cases above start with a small case so any reason to start this one with capital?
Also, this should be more descriptive, say filters queries with array for AND/OR
graphql/e2e/common/common.go, line 406 at r4 (raw file):
t.Run("Geo - Polygon type", mutationPolygonType) t.Run("Geo - MultiPolygon type", mutationMultiPolygonType) t.Run("Mutation With Filter", mutationWithFilter)
mutation with array for AND/OR filter
graphql/e2e/common/common.go, line 407 at r4 (raw file):
t.Run("Geo - MultiPolygon type", mutationMultiPolygonType) t.Run("Mutation With Filter", mutationWithFilter) t.Run("update With Filter", updateWithFilter)
update with array for AND/OR filter
graphql/e2e/common/mutation.go, line 4430 at r4 (raw file):
name: "Single Statement in Filter on Mutation", query: `mutation { addpost1(input: [{title: "Dgraph", numLikes: 100}]) {
Why test this? This should have been working even before the change.
graphql/e2e/common/mutation.go, line 4458 at r4 (raw file):
} }`, variables: map[string]interface{}{"filter": map[string]interface{}{"title": map[string]interface{}{"eq": "Dgraph"}}},
Why test this? This should have been working even before the change.
graphql/e2e/common/mutation.go, line 4492 at r4 (raw file):
}, { name: "Single Statement in Filter on Mutation using variables",
No idea about the test case from this name. Could you fix all names here?
This could be called Filter with only OR key at top level
graphql/e2e/common/mutation.go, line 4514 at r4 (raw file):
}, { name: "Single and Statement in Filter on Mutation",
Filter with only AND key at top level
Fix all other names as well
graphql/e2e/common/mutation.go, line 4587 at r4 (raw file):
} }`, variables: map[string]interface{}{"filter": map[string]interface{}{"and": []interface{}{map[string]interface {
Instead of all this, might have been better if you defined the JSON representation of variables here and unmarshalled into a map before calling execute. A JSON is easier to read than this Go map.
graphql/e2e/common/mutation.go, line 4640 at r4 (raw file):
name: "Single Statement in Filter on Update", query: `mutation updatepost1{ updatepost1(input:{filter:{title:{eq:"Dgraph"}},set:{numLikes:150}}){
What's the point of this test if it doesn't use AND or OR?
graphql/e2e/common/mutation.go, line 4659 at r4 (raw file):
}, { name: "Single Statement in Filter on Update with variable",
Again this test also doesn't use AND/OR so why add it?
graphql/e2e/common/query.go, line 2315 at r5 (raw file):
respData string }{{ name: "Single Statement in Filter",
Wasn't this working before? Why do we need another test for this?
graphql/e2e/common/query.go, line 2340 at r5 (raw file):
}, { name: "Single Statement in Filter using variables",
test not useful
graphql/e2e/common/query.go, line 2637 at r5 (raw file):
] }`, variables: map[string]interface{}{"filter": map[string]interface{}{"and": []interface{}{map[string]interface{}{"name": map[string]interface{}{"eq": "George"}}, map[string]interface{}{"reputation": map[string]interface{}{"eq": 4.5}}, map[string]interface{}{"qualification": map[string]interface{}{"eq": "Phd in CSE"}}}}},
Fix these variables to be a JSON and unamarshal it below.nn
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: 0 of 80 files reviewed, 14 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)
go.sum, line 7 at r4 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
can you try and delete this line and see if it still comes back after running
go mod tidy
?n
yeah, it comes back.
graphql/e2e/common/common.go, line 354 at r4 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
The cases above start with a small case so any reason to start this one with capital?
Also, this should be more descriptive, say
filters queries with array for AND/OR
done.
graphql/e2e/common/common.go, line 406 at r4 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
mutation with array for AND/OR filter
done.
graphql/e2e/common/common.go, line 407 at r4 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
update with array for AND/OR filter
done.
graphql/e2e/common/mutation.go, line 4430 at r4 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Why test this? This should have been working even before the change.
I wanted to make sure this is working after the change. Removed.
graphql/e2e/common/mutation.go, line 4458 at r4 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Why test this? This should have been working even before the change.
Removed.
graphql/e2e/common/mutation.go, line 4492 at r4 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
No idea about the test case from this name. Could you fix all names here?
This could be called
Filter with only OR key at top level
changed.
graphql/e2e/common/mutation.go, line 4514 at r4 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Filter with only AND key at top level
Fix all other names as well
changed.
graphql/e2e/common/mutation.go, line 4587 at r4 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Instead of all this, might have been better if you defined the JSON representation of variables here and unmarshalled into a map before calling execute. A JSON is easier to read than this Go map.
done.
graphql/e2e/common/mutation.go, line 4640 at r4 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
What's the point of this test if it doesn't use AND or OR?
removed.
graphql/e2e/common/mutation.go, line 4659 at r4 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Again this test also doesn't use AND/OR so why add it?
removed.
graphql/e2e/common/query.go, line 2315 at r5 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Wasn't this working before? Why do we need another test for this?
removed.
graphql/e2e/common/query.go, line 2340 at r5 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
test not useful
removed.
graphql/e2e/common/query.go, line 2637 at r5 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Fix these variables to be a JSON and unamarshal it below.nn
done.
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.
lgtm
Reviewed 1 of 49 files at r1, 11 of 70 files at r3, 65 of 68 files at r4, 4 of 4 files at r6.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)
Fixes GRAPHQL-743
addMutation also accepts an object along with an array. This is in accordance with https://spec.graphql.org/June2018/#sec-Type-System.List.
Seems like the input coercion fails when the value for the input is supplied through GraphQL variables because of vektah/gqlparser#137.
This change is
Docs Preview: