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

Fix(GraphQL): Add support for repeatitive fields in aggregate query #7094

Merged
merged 2 commits into from Dec 10, 2020

Conversation

vmrajas
Copy link
Contributor

@vmrajas vmrajas commented Dec 9, 2020

Motivation:
Recently, support for Aggregation Queries was added to GraphQL. It was discovered later on that there existed a bug in Aggregate Queries. The bug is that adding a repetitive aggregate field generates an incorrect DQL query.
Example:

aggregateCountry {
    count
    cnt : count
}

generated a wrong DQL query and gave a malformed DQL query error.
This PR fixes this.

Testing:

  1. Added tests to query_test.yaml .
  2. Added tests to graphql/e2e/common .
    Fixes GRAPHQL-882

This change is Reviewable

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Dec 9, 2020
@netlify
Copy link

netlify bot commented Dec 9, 2020

✔️ Deploy preview for dgraph-docs ready!

🔨 Explore the source changes: a87eff4

🔍 Inspect the deploy logs: https://app.netlify.com/sites/dgraph-docs/deploys/5fd0d2e32ceb35000764cecb

😎 Browse the preview: https://deploy-preview-7094--dgraph-docs.netlify.app

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 4 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @abhimanyusinghgaur, @MichaelJCompton, @minhaj-shakeel, and @vmrajas)


graphql/e2e/common/query.go, line 2929 at r1 (raw file):

				tmin : titleMin
				tmin_again : titleMin
				tmax: titleMax

Also, request this twice


graphql/resolve/query_rewriter.go, line 193 at r1 (raw file):

	// Add selection set to mainQuery and finalMainQuery.
	isAggregateFieldVisited := make(map[string]bool)
	// isAggregateFunctionVisited stores if the aggregate function has been added or not.

function for a field has been added or not. So the map entries would contain keys as nameMin, ageMin, nameName, etc.

Copy link
Contributor Author

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


graphql/e2e/common/query.go, line 2929 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Also, request this twice

Done.


graphql/resolve/query_rewriter.go, line 193 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

function for a field has been added or not. So the map entries would contain keys as nameMin, ageMin, nameName, etc.

Done.

@vmrajas vmrajas merged commit 7f12eee into master Dec 10, 2020
@vmrajas vmrajas deleted the GRAPHQL-882 branch December 10, 2020 17:22
vmrajas added a commit that referenced this pull request Dec 10, 2020
…7094)

* Fix repeatitive fields in aggregate query

* Address comments

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

None yet

2 participants