Skip to content

fix(GraphQL): handle filters for enum properly #6887

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

Merged
merged 10 commits into from
Nov 17, 2020

Conversation

minhaj-shakeel
Copy link
Contributor

@minhaj-shakeel minhaj-shakeel commented Nov 12, 2020

Fixes GRAPHQL-817.

This PR is a breaking change for the eq filter on the List of enum Types.

This PR fixes bugs in the filter generated for enum types. For the given schema:-

enum Status {
   ACTIVE
   DEACTIVATED
}

type VerificationFilter {
    status: [Status]! @search
}

the hash filter generated for status was:

input Status_hash {
   eq: [Status]!
   in: [Status]
}

whereas, it should be:

input Status_hash {
    eq: Status
    in: [Status]
}

This PR also fixes incorrect filter generated for exact index in List of enum types.
Earlier it was being generated like:

input Status_exact {
	eq: [Status]
	in: [Status]
	le: [Status]
	lt: [Status]
	ge: [Status]
	gt: [Status]
	between: Status
}

but it should be like:

input Status_exact {
	eq: Status
	in: [Status]
	le: Status
	lt: Status
	ge: Status
	gt: Status
	between: Status
}

Breaking change lies in the generation of eq filters. Earlier it was eq: [Status]! but now it is eq: Status. Both are syntactically fine but eq: [Status]! conflicts with the expected usage of eq filter.


This change is Reviewable

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Nov 12, 2020
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.

We need some query rewriting tests in which

  1. You pass IN as null
  2. You pass an item in the IN array as null
  3. You pass eq as null
  4. You pass an item in eq array as null
  5. You pass both IN and eq as null.

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @MichaelJCompton and @minhaj-shakeel)


graphql/schema/gqlschema.go, line 1237 at r1 (raw file):

					typ = &ast.Type{
						NamedType: enumTypeName,
						NonNull:   false,

this is the default value so no need to mention it anywhere


graphql/schema/testdata/schemagen/input/searchables.graphql, line 23 at r1 (raw file):

	postType: PostType @search
		postTypeNonNull: PostType! @search

something messed up about the indentation here


graphql/schema/testdata/schemagen/output/interfaces-with-types.graphql, line 510 at r1 (raw file):

input Episode_hash {
	eq: [Episode!]!

It was an array earlier for enums and now is just one value? This would be a breaking change. I do realize that we must remove the outer ! but we can still keep it as eq: [Episode!]


graphql/schema/testdata/schemagen/output/interfaces-with-types.graphql, line 511 at r1 (raw file):

input Episode_hash {
	eq: Episode
	in: [Episode]

Should this not be [Episode!] if the field was of type Episode!because [null] is not a valid value in that case?

Copy link
Contributor Author

@minhaj-shakeel minhaj-shakeel 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: 3 of 9 files reviewed, 4 unresolved discussions (waiting on @MichaelJCompton, @minhaj-shakeel, and @pawanrawal)


graphql/schema/gqlschema.go, line 1237 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

this is the default value so no need to mention it anywhere

Done.


graphql/schema/testdata/schemagen/input/searchables.graphql, line 23 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

something messed up about the indentation here

Done.


graphql/schema/testdata/schemagen/output/interfaces-with-types.graphql, line 511 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Should this not be [Episode!] if the field was of type Episode!because [null] is not a valid value in that case?

Keeping this as it is as dist over the call.

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.

Reviewed 1 of 6 files at r2.
Reviewable status: 4 of 9 files reviewed, 1 unresolved discussion (waiting on @MichaelJCompton, @minhaj-shakeel, and @pawanrawal)


graphql/schema/testdata/schemagen/output/searchables.graphql, line 27 at r2 (raw file):

	postTypeTrigram: PostType @search(by: [trigram])
	postTypeRegexp: PostType @search(by: [regexp])
	postTypeExact(first: Int, offset: Int): [PostType] @search(by: [exact])

Can we do pagination over enums, that shouldn't be allowed.

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.

Reviewed 4 of 6 files at r2, 5 of 5 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MichaelJCompton and @minhaj-shakeel)


graphql/schema/gqlschema.go, line 1117 at r3 (raw file):

		// Ordering and pagination, however, only makes sense for fields of
		// list types (not scalar lists or enum lists).
		if isTypeList(fld) && !isEnumList(fld, schema) {

nice change

@minhaj-shakeel minhaj-shakeel merged commit 40b20c7 into master Nov 17, 2020
@minhaj-shakeel minhaj-shakeel deleted the minhaj/fix-filters-for-enum branch November 17, 2020 10:43
minhaj-shakeel added a commit that referenced this pull request Nov 19, 2020
This PR is a breaking change for the `eq` filter on the List of `enum` Types.

This PR fixes bugs in the filter generated for enum types. For the given schema:-
```
enum Status {
   ACTIVE
   DEACTIVATED
}

type VerificationFilter {
    status: [Status]! @search
}
```

the `hash` filter generated for `status` was:
```
input Status_hash {
   eq: [Status]!
   in: [Status]
}
```
whereas, it should be:
```
input Status_hash {
    eq: Status
    in: [Status]
}
```

This PR also fixes incorrect filter generated for `exact` index in List of enum types.
Earlier it was being generated like:
```
input Status_exact {
	eq: [Status]
	in: [Status]
	le: [Status]
	lt: [Status]
	ge: [Status]
	gt: [Status]
	between: Status
}
```
but it should be like:
```
input Status_exact {
	eq: Status
	in: [Status]
	le: Status
	lt: Status
	ge: Status
	gt: Status
	between: Status
}
```

Breaking change lies in the generation of `eq` filters. Earlier it was `eq: [Status]!` but now it is `eq: Status`. Both are syntactically fine but `eq: [Status]!` conflicts with the expected usage of `eq` filter.

(cherry picked from commit 40b20c7)
minhaj-shakeel added a commit that referenced this pull request Nov 19, 2020
This PR is a breaking change for the `eq` filter on the List of `enum` Types.

This PR fixes bugs in the filter generated for enum types. For the given schema:-
```
enum Status {
   ACTIVE
   DEACTIVATED
}

type VerificationFilter {
    status: [Status]! @search
}
```

the `hash` filter generated for `status` was:
```
input Status_hash {
   eq: [Status]!
   in: [Status]
}
```
whereas, it should be:
```
input Status_hash {
    eq: Status
    in: [Status]
}
```

This PR also fixes incorrect filter generated for `exact` index in List of enum types.
Earlier it was being generated like:
```
input Status_exact {
	eq: [Status]
	in: [Status]
	le: [Status]
	lt: [Status]
	ge: [Status]
	gt: [Status]
	between: Status
}
```
but it should be like:
```
input Status_exact {
	eq: Status
	in: [Status]
	le: Status
	lt: Status
	ge: Status
	gt: Status
	between: Status
}
```

Breaking change lies in the generation of `eq` filters. Earlier it was `eq: [Status]!` but now it is `eq: Status`. Both are syntactically fine but `eq: [Status]!` conflicts with the expected usage of `eq` filter.

(cherry picked from commit 40b20c7)
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.

2 participants