Skip to content
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): Add HasFilter in the Schema #6168

Merged
merged 9 commits into from
Aug 17, 2020

Conversation

minhaj-shakeel
Copy link
Contributor

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


This change is Reviewable

Docs Preview: Dgraph Preview

@CLAassistant
Copy link

CLAassistant commented Aug 12, 2020

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Aug 12, 2020
@pawanrawal pawanrawal changed the title add HasFilter In the Schema feat(GraphQL): Add HasFilter in the Schema Aug 12, 2020
@minhaj-shakeel minhaj-shakeel changed the base branch from master to minhaj/addHasFilterToGraphQL August 12, 2020 12:24
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 3 of 41 files at r3, 2 of 2 files at r4, 38 of 38 files at r5.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @abhimanyusinghgaur, @danielmai, @manishrjain, @martinmr, @MichaelJCompton, @minhaj-shakeel, and @vvbalaji-dgraph)


dgraph/cmd/zero/run.go, line 189 at r5 (raw file):

	glog.Infof("Setting Config to: %+v", opts)

	if opts.nodeId == 0 {

Don't make this change here, make it in a separate PR.


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

			addAddPayloadType(sch, defn)
			addMutations(sch, defn)

no need to add new line here


graphql/schema/gqlschema.go, line 717 at r5 (raw file):

	}

	// Interfaces could have just ID field but Classes cannot for eg:

its not Classes, it should be but Types cannot


graphql/schema/gqlschema.go, line 720 at r5 (raw file):

	// interface I {
	//   id: ID!
	//	}

indentation needs fixing on this line


graphql/schema/gqlschema.go, line 809 at r5 (raw file):

// }
func addFilterType(schema *ast.Schema, defn *ast.Definition) {
	// if !hasFilterable(defn) {

why comment this out?


graphql/schema/gqlschema.go, line 847 at r5 (raw file):

	}

	//Has filter makes sense only if there is atleast one non ID field in the defn

Space after //, like // Has Filter


graphql/schema/gqlschema.go, line 848 at r5 (raw file):

	//Has filter makes sense only if there is atleast one non ID field in the defn
	if (len(defn.Fields) == 1 && !isID(defn.Fields[0])) || len(defn.Fields) > 1 {

there is a function to getNonIDFields, you could use that and check the length of fields returned from it for this


graphql/schema/testdata/schemagen/input/comments-and-descriptions.graphql, line 10 at r5 (raw file):

    Desc
    """
    s: String! #@search

What is this?


graphql/schema/testdata/schemagen/input/interface-with-id-directive.graphql, line 14 at r5 (raw file):

#interface LibraryItem {

Why have commented out bits and why do we need to change this test?


posting/list.go, line 532 at r5 (raw file):

	defer l.RUnlock()
	if pl, ok := l.mutationMap[startTs]; ok {
		for _, p := range pl.GetPostings() {

why is this change here?


wiki/content/deploy/fast-data-loading.md, line 48 at r5 (raw file):

### Encrypted imports via Live Loader (Enterprise Feature)

A new flag `--encryption_key_file` is added to the Live Loader. This option is required to decrypt the encrypted export data and schema files. Once the export files are decrypted, the Live Loader streams the data to a live Alpha instance.

Not sure why this change is there for the PR?


wiki/content/enterprise-features/binary-backups.md, line 217 at r5 (raw file):

Encrypted backups are a Enterprise feature that are available from v20.03.1 and v1.2.3 and allow you to encrypt your backups and restore them. This documentation describes how to implement encryption into your binary backups.
Starting with v20.07.0, we also added support for Encrypted Backups using encryption keys sitting on Vault. 

unrelated change

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: 39 of 41 files reviewed, 12 unresolved discussions (waiting on @abhimanyusinghgaur, @danielmai, @manishrjain, @martinmr, @MichaelJCompton, @minhaj-shakeel, @pawanrawal, and @vvbalaji-dgraph)


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

Previously, pawanrawal (Pawan Rawal) wrote…

no need to add new line here

Done.


graphql/schema/gqlschema.go, line 717 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

its not Classes, it should be but Types cannot

Done.


graphql/schema/gqlschema.go, line 720 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

indentation needs fixing on this line

Done.


graphql/schema/gqlschema.go, line 809 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

why comment this out?

It is not required now because there could be has filter even if defn has no filterable field.


graphql/schema/gqlschema.go, line 847 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Space after //, like // Has Filter

Done.


graphql/schema/gqlschema.go, line 848 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

there is a function to getNonIDFields, you could use that and check the length of fields returned from it for this

Done.

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: 39 of 41 files reviewed, 12 unresolved discussions (waiting on @abhimanyusinghgaur, @danielmai, @manishrjain, @martinmr, @MichaelJCompton, @minhaj-shakeel, @pawanrawal, and @vvbalaji-dgraph)


graphql/schema/testdata/schemagen/input/interface-with-id-directive.graphql, line 14 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Why have commented out bits and why do we need to change this test?

Was playing around with schema, forgot to revert the changes. fixed now.

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.

LGTM

}
filter.EnumValues = append(filter.EnumValues,
&ast.EnumValueDefinition{Name: fld.Name})

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this newline isn't required

@@ -812,6 +839,13 @@ func addFilterType(schema *ast.Schema, defn *ast.Definition) {
}
}

// Has filter makes sense only if there is atleast one non ID field in the defn
if len(getNonIDFields(schema, defn)) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually sorry we should use getFieldsWithoutIDType here because that gives all fields except field having type ID. So it also gives fields with @id directive which is what we want because user can give the Has filter on a field with @id directive as well.

@minhaj-shakeel minhaj-shakeel merged commit c66ecce into minhaj/addHasFilterToGraphQL Aug 17, 2020
@minhaj-shakeel minhaj-shakeel deleted the minhaj/addHasFilter branch August 17, 2020 10:04
minhaj-shakeel added a commit that referenced this pull request Aug 24, 2020
* feat(GraphQL): Add HasFilter in the Schema (#6168)

* add HadFilter In the Schema

* fix deepsource failure

* fix Teamcity failure

* fix TestCases

* address Pawan's comments

* address Pawan's Comments

* feat(GraphQL): Add has filter in Query Rewriter (#6205)

* add has filter in query_rewriter and add test case

* remove space

* change Predicate Format and add test cases

* minor changes

* remove spaces

* change in testcases

* address Pawan's comments

* add more test cases

* fix CI failure

* add more test cases

* feat(GraphQL): add e2e tests for has filter (#6247)

* add test data

* add e2e tests

* remove spacing

* change format of has query

* change addPost mutation

* fix test failures

* fix has filter test failure
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.

3 participants