Skip to content

Commit

Permalink
fix(GraphQL): fix exclusion of filters in Query generation in case of…
Browse files Browse the repository at this point in the history
… just Has Filters. (#6870)

Fixes GRAPHQL-787.

This PR fixes the Bug in query and mutations generation in case of types that have just `has` filter and not any other kind of filter. For example for the given type: 
```
type Student {
   name: String
   age: Int
}
```
the corresponding query generated was 
```
queryStudent(order: StudentOrder, first: Int, offset: Int): [Student]
```
and only `add` mutation was being generated.
Whereas, the correct query should be: 
```
queryStudent(filter: StudentFilter, order: StudentOrder, first: Int, offset: Int): [Student]
```
and corresponding update and delete mutation should be:
```
updateStudent(input: UpdateStudentInput!): UpdateStudentPayload
deleteStudent(filter: StudentFilter!): DeleteStudentPayload
```
  • Loading branch information
minhaj-shakeel authored Nov 19, 2020
1 parent 0c31195 commit 5e60ef1
Show file tree
Hide file tree
Showing 13 changed files with 527 additions and 110 deletions.
14 changes: 14 additions & 0 deletions graphql/resolve/query_test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -844,6 +844,20 @@
}
}
- name: "Query Has Filter on type which has neither ID field nor any search argument"
gqlquery: |
query {
queryNode(filter: {has: name}){
name
}
}
dgquery: |-
query {
queryNode(func: type(Node)) @filter(has(Node.name)) {
name : Node.name
dgraph.uid : uid
}
}
-
name: "Filters in same input object implies AND"
gqlquery: |
Expand Down
47 changes: 34 additions & 13 deletions graphql/schema/gqlschema.go
Original file line number Diff line number Diff line change
Expand Up @@ -892,39 +892,52 @@ func cleanupInput(sch *ast.Schema, def *ast.Definition, seen map[string]bool) {
}
def.Fields = def.Fields[:i]

if len(def.Fields) == 0 {
// In case of UpdateTypeInput, if TypePatch gets cleaned up then it becomes
// input UpdateTypeInput {
// filter: TypeFilter!
// }
// In this case, UpdateTypeInput should also be deleted.
if len(def.Fields) == 0 || (strings.HasPrefix(def.Name, "Update") && len(def.Fields) == 1) {
delete(sch.Types, def.Name)
}
}

func cleanSchema(sch *ast.Schema) {
// Let's go over inputs of the type TRef, TPatch and AddTInput and delete the ones which
// Let's go over inputs of the type TRef, TPatch AddTInput, UpdateTInput and delete the ones which
// don't have field inside them.
for k := range sch.Types {
if strings.HasSuffix(k, "Ref") || strings.HasSuffix(k, "Patch") ||
(strings.HasPrefix(k, "Add") && strings.HasSuffix(k, "Input")) {
((strings.HasPrefix(k, "Add") || strings.HasPrefix(k, "Update")) && strings.HasSuffix(k, "Input")) {
cleanupInput(sch, sch.Types[k], map[string]bool{})
}
}

// Let's go over mutations and cleanup those which don't have AddTInput defined in the schema
// Let's go over mutations and cleanup those which don't have AddTInput/UpdateTInput defined in the schema
// anymore.
i := 0 // helps us overwrite the array with valid entries.
for _, field := range sch.Mutation.Fields {
custom := field.Directives.ForName("custom")
// We would only modify add type queries.
if custom != nil || !strings.HasPrefix(field.Name, "add") {
// We would only modify add/update
if custom != nil || !(strings.HasPrefix(field.Name, "add") || strings.HasPrefix(field.Name, "update")) {
sch.Mutation.Fields[i] = field
i++
continue
}

// addT type mutations have an input which is AddTInput so if that doesn't exist anymore,
// we can delete the AddTPayload and also skip this mutation.
typ := field.Name[3:]
input := sch.Types["Add"+typ+"Input"]
if input == nil {
delete(sch.Types, "Add"+typ+"Payload")
// addT / updateT type mutations have an input which is AddTInput / UpdateTInput so if that doesn't exist anymore,
// we can delete the AddTPayload / UpdateTPayload and also skip this mutation.

var typeName, input string
if strings.HasPrefix(field.Name, "add") {
typeName = field.Name[3:]
input = "Add" + typeName + "Input"
} else if strings.HasPrefix(field.Name, "update") {
typeName = field.Name[6:]
input = "Update" + typeName + "Input"
}

if sch.Types[input] == nil {
delete(sch.Types, input)
continue
}
sch.Mutation.Fields[i] = field
Expand Down Expand Up @@ -1155,6 +1168,11 @@ func addFilterArgument(schema *ast.Schema, fld *ast.FieldDefinition) {
}

func addFilterArgumentForField(schema *ast.Schema, fld *ast.FieldDefinition, fldTypeName string) {
// Don't add filters for inbuilt types like String, Point, Polygon ...
if _, ok := inbuiltTypeToDgraph[fldTypeName]; ok {
return
}

fldType := schema.Types[fldTypeName]
if fldType.Kind == ast.Union || hasFilterable(fldType) {
fld.Arguments = append(fld.Arguments,
Expand Down Expand Up @@ -1351,10 +1369,13 @@ func addFilterType(schema *ast.Schema, defn *ast.Definition) {
schema.Types[filterName] = filter
}

// hasFilterable Returns whether TypeFilter for a defn will be generated or not.
// It returns true if any field have search arguments or it is an `ID` field or
// there is atleast one non-custom filter which would be the part of the has filter.
func hasFilterable(defn *ast.Definition) bool {
return fieldAny(defn.Fields,
func(fld *ast.FieldDefinition) bool {
return len(getSearchArgs(fld)) != 0 || isID(fld)
return len(getSearchArgs(fld)) != 0 || isID(fld) || !hasCustomOrLambda(fld)
})
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,12 @@ type AddTPayload {
numUids: Int
}

type DeleteIPayload {
i(filter: IFilter, order: IOrder, first: Int, offset: Int): [I]
msg: String
numUids: Int
}

type DeleteTPayload {
t(filter: TFilter, order: TOrder, first: Int, offset: Int): [T]
msg: String
Expand All @@ -311,6 +317,11 @@ type TAggregateResult {
count: Int
}

type UpdateIPayload {
i(filter: IFilter, order: IOrder, first: Int, offset: Int): [I]
numUids: Int
}

type UpdateTPayload {
t(filter: TFilter, order: TOrder, first: Int, offset: Int): [T]
numUids: Int
Expand Down Expand Up @@ -374,6 +385,11 @@ input IOrder {
then: IOrder
}

input IPatch {
"""Desc"""
s: String
}

input TFilter {
id: [ID!]
has: THasFilter
Expand Down Expand Up @@ -401,6 +417,12 @@ input TRef {
i: Int
}

input UpdateIInput {
filter: IFilter!
set: IPatch
remove: IPatch
}

input UpdateTInput {
filter: TFilter!
set: TPatch
Expand All @@ -412,8 +434,8 @@ input UpdateTInput {
#######################

type Query {
queryI(order: IOrder, first: Int, offset: Int): [I]
aggregateI: IAggregateResult
queryI(filter: IFilter, order: IOrder, first: Int, offset: Int): [I]
aggregateI(filter: IFilter): IAggregateResult
getT(id: ID!): T
queryT(filter: TFilter, order: TOrder, first: Int, offset: Int): [T]
aggregateT(filter: TFilter): TAggregateResult
Expand All @@ -424,6 +446,8 @@ type Query {
#######################

type Mutation {
updateI(input: UpdateIInput!): UpdateIPayload
deleteI(filter: IFilter!): DeleteIPayload
addT(input: [AddTInput!]!): AddTPayload
updateT(input: UpdateTInput!): UpdateTPayload
deleteT(filter: TFilter!): DeleteTPayload
Expand Down
30 changes: 27 additions & 3 deletions graphql/schema/testdata/schemagen/output/deprecated.graphql
Original file line number Diff line number Diff line change
Expand Up @@ -267,14 +267,25 @@ input StringHashFilter {
#######################

type AddAtypePayload {
atype(order: AtypeOrder, first: Int, offset: Int): [Atype]
atype(filter: AtypeFilter, order: AtypeOrder, first: Int, offset: Int): [Atype]
numUids: Int
}

type AtypeAggregateResult {
count: Int
}

type DeleteAtypePayload {
atype(filter: AtypeFilter, order: AtypeOrder, first: Int, offset: Int): [Atype]
msg: String
numUids: Int
}

type UpdateAtypePayload {
atype(filter: AtypeFilter, order: AtypeOrder, first: Int, offset: Int): [Atype]
numUids: Int
}

#######################
# Generated Enums
#######################
Expand Down Expand Up @@ -311,18 +322,29 @@ input AtypeOrder {
then: AtypeOrder
}

input AtypePatch {
iamDeprecated: String
soAmI: String
}

input AtypeRef {
iamDeprecated: String
soAmI: String
}

input UpdateAtypeInput {
filter: AtypeFilter!
set: AtypePatch
remove: AtypePatch
}

#######################
# Generated Query
#######################

type Query {
queryAtype(order: AtypeOrder, first: Int, offset: Int): [Atype]
aggregateAtype: AtypeAggregateResult
queryAtype(filter: AtypeFilter, order: AtypeOrder, first: Int, offset: Int): [Atype]
aggregateAtype(filter: AtypeFilter): AtypeAggregateResult
}

#######################
Expand All @@ -331,5 +353,7 @@ type Query {

type Mutation {
addAtype(input: [AddAtypeInput!]!): AddAtypePayload
updateAtype(input: UpdateAtypeInput!): UpdateAtypePayload
deleteAtype(filter: AtypeFilter!): DeleteAtypePayload
}

Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,20 @@
#######################

type X {
name(first: Int, offset: Int): [Y]
f1(first: Int, offset: Int): [Y] @dgraph(pred: "f1")
nameAggregate: YAggregateResult
f1Aggregate: YAggregateResult
name(filter: YFilter, first: Int, offset: Int): [Y]
f1(filter: YFilter, first: Int, offset: Int): [Y] @dgraph(pred: "f1")
nameAggregate(filter: YFilter): YAggregateResult
f1Aggregate(filter: YFilter): YAggregateResult
}

type Y {
f1(first: Int, offset: Int): [X] @dgraph(pred: "~f1")
f1Aggregate: XAggregateResult
f1(filter: XFilter, first: Int, offset: Int): [X] @dgraph(pred: "~f1")
f1Aggregate(filter: XFilter): XAggregateResult
}

type Z {
add(first: Int, offset: Int): [X]
addAggregate: XAggregateResult
add(filter: XFilter, first: Int, offset: Int): [X]
addAggregate(filter: XFilter): XAggregateResult
}

#######################
Expand Down Expand Up @@ -278,6 +278,44 @@ input StringHashFilter {
# Generated Types
#######################

type AddXPayload {
x(filter: XFilter, first: Int, offset: Int): [X]
numUids: Int
}

type AddZPayload {
z(filter: ZFilter, first: Int, offset: Int): [Z]
numUids: Int
}

type DeleteXPayload {
x(filter: XFilter, first: Int, offset: Int): [X]
msg: String
numUids: Int
}

type DeleteYPayload {
y(filter: YFilter, first: Int, offset: Int): [Y]
msg: String
numUids: Int
}

type DeleteZPayload {
z(filter: ZFilter, first: Int, offset: Int): [Z]
msg: String
numUids: Int
}

type UpdateXPayload {
x(filter: XFilter, first: Int, offset: Int): [X]
numUids: Int
}

type UpdateZPayload {
z(filter: ZFilter, first: Int, offset: Int): [Z]
numUids: Int
}

type XAggregateResult {
count: Int
}
Expand Down Expand Up @@ -334,11 +372,21 @@ input ZFilter {
#######################

type Query {
queryX(first: Int, offset: Int): [X]
aggregateX: XAggregateResult
queryY(first: Int, offset: Int): [Y]
aggregateY: YAggregateResult
queryZ(first: Int, offset: Int): [Z]
aggregateZ: ZAggregateResult
queryX(filter: XFilter, first: Int, offset: Int): [X]
aggregateX(filter: XFilter): XAggregateResult
queryY(filter: YFilter, first: Int, offset: Int): [Y]
aggregateY(filter: YFilter): YAggregateResult
queryZ(filter: ZFilter, first: Int, offset: Int): [Z]
aggregateZ(filter: ZFilter): ZAggregateResult
}

#######################
# Generated Mutations
#######################

type Mutation {
deleteX(filter: XFilter!): DeleteXPayload
deleteY(filter: YFilter!): DeleteYPayload
deleteZ(filter: ZFilter!): DeleteZPayload
}

Loading

0 comments on commit 5e60ef1

Please sign in to comment.