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): Introducing Nested Filtering for Dgraph GraphQL API with Optimized Query Support #9100

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

iyinoluwaayoola
Copy link

Description:

This merge request introduces Nested Filtering to the Dgraph GraphQL API. It allows for comprehensive filtering capabilities that span across queries, aggregations, and authorization functions.

How Does It Work?

Nested Filtering leverages Dgraph's var block to filter nested objects and selects parent objects through inverse predicates. To enable nested filtering, you need to add the @search directive to the nested field (edge) without any arguments.

Example:

type User {
    name: String @search(by: [hash])
    friends: [User] @hasInverse(field: friends) @search
}

Given the schema above, you can query users who have friends named "Ben" using the following query:

query {
    queryUser(filter: { friends: { name: { eq: "Ben" } } }) {
        name
    }
}

Schema Validation:

If the @search directive is added to an edge, the @hasInverse directive must also be set on one of the referenced fields. This validation process includes handling interface fields by checking the implemented types for reverse fields.

Testing:

Testing has been conducted locally, showing approximately 50% fewer scanned nodes

Docs
RFC -> https://discuss.dgraph.io/t/rfc-nested-filters-in-graphql/13560/37

…t filtering

♻️ (query_rewriter.go): Refactor to store field definition in a variable for reuse
✨ (wrappers.go): Add HasSearchDirective method to FieldDefinition interface and its implementation to check for search directive in a field
@iyinoluwaayoola iyinoluwaayoola requested a review from a team as a code owner June 9, 2024 15:09
@CLAassistant
Copy link

CLAassistant commented Jun 9, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@dgraph-bot dgraph-bot added area/graphql Issues related to GraphQL support on Dgraph. area/core internal mechanisms go Pull requests that update Go code labels Jun 9, 2024
@harshil-goel
Copy link
Contributor

Can you add some test cases in e2e folder, and one in query_rewriter?

✨ (query_rewriter.go): Remove unnecessary assignment of aggFilterQueries
💡 (query_rewriter.go): Add comments to explain nested object filtering
🐛 (query_rewriter.go): Fix nested object filtering to handle nested fields correctly
…tainability

✨ (query_test.yaml, schema.graphql, gqlschema_test.yml): Add tests and schema for nested filtering
🐛 (gqlschema.go, rules.go): Fix validation for @search directive to require @hasInverse directive when necessary

♻️ (wrappers.go): Rename function 'hasInverseReference' to 'hasInverse' for clarity
💡 (wrappers.go): Remove unnecessary semicolon and improve code formatting
✨ (wrappers.go): Extend 'isCustomType' function to include 'ast.Interface' kind
🐛 (wrappers.go): Add additional check for inverse type when querying from an interface, not the implemented type
…for readability

✨ (query_test.yaml): Add test cases for deeply nested object queries and AND/OR conditions
🐛 (gqlschema.go, rules.go, wrappers.go): Fix custom type detection by passing type instead of field definition
…unction" to increase test coverage and ensure the aggregate function works as expected in nested queries.
…port nested filtering

✨ (auth_query_test.yaml): Add test case for nested filter query
🐛 (mutation_rewriter.go): Update rewriteAsQueryByIds and addFilter functions to include mutation alias for better query identification

♻️ (query_rewriter.go): Refactor filterQueries to varQry for better semantics
✨ (query_rewriter.go): Add queryName parameter to rewriteAsQuery and addArgumentsToField functions to support query aliasing
🐛 (query_rewriter.go): Fix issue where only the first query was returned in rewriteRuleNode, now returns all queries including nested var queries
…ules for more test coverage

✨ (auth_delete_test.yaml): Add new test case for delete selection by nested filter
✨ (auth_query_test.yaml): Update existing test case and add new ones for positive, negative, and uncertain auth rules with nested filter
🐛 (mutation_rewriter.go): Fix addFilter function call to include authRw parameter

♻️ (query_rewriter.go): Refactor addFilter and buildFilter functions to include authRewriter parameter for better access control
💡 (query_rewriter.go): Update function calls to addFilter and buildFilter to pass authRewriter parameter for improved security

♻️ (query_rewriter.go): Refactor variable name from 'queries' to 'varQry' for better readability
✨ (query_rewriter.go): Add 'auth' parameter to 'buildFilter' and 'buildUnionFilter' functions to support authorization rewriting in queries
@iyinoluwaayoola
Copy link
Author

Hi @harshil-goel , added auth to the nested query filters along with a few test cases.

@iyinoluwaayoola
Copy link
Author

iyinoluwaayoola commented Jun 18, 2024

I see an opportunity to generally improve the auth queries by storing them per type and reusing them each time. This way, if a type is selected multiple times within the same request, we can reuse the auth variable and reduce the number of scans.

@iyinoluwaayoola
Copy link
Author

Hi @harshil-goel , can you please provide feedback or review in getting this merged? I see some CI failures, but unsure if they are related to the change.

Copy link
Contributor

@harshil-goel harshil-goel left a comment

Choose a reason for hiding this comment

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

Sorry for the time in review.
The diff looks great, I have minor changes here and there based on the examples.
Please take a look.

@@ -522,3 +522,19 @@ type ProjectDotProduct {
title: String
description_v: [Float!] @embedding @search(by: ["hnsw(metric: dotproduct, exponent: 4)"])
}

"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this required here? This makes it such that generated schema would contain this. From what I understand, you only need this in the tests?

@@ -2148,3 +2148,112 @@
Person.id : uid
}
}

-
name: "Query positive auth rules with nested filter"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you write one test around auth also? Where we have a auth filter on the query, so that we can make sure no uids are returned that shouldn't be

var(func: type(Nested_Y)) @filter(eq(Nested_Y.s, "y")) {
Nested_X_Auth4_y as Nested_Z.x
}
var(func: uid(Nested_XRoot)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This query seems unnecessary? We are just repeating the queryNested_X

query {
queryNested_X(func: uid(Nested_XRoot)) {
Nested_X.b : Nested_X.b
Nested_X.y : Nested_X.y @filter(uid(Nested_Y_1)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the filter appearing here? I don't think it's required? I am guessing that your intention is that If someone has a filter on root query, we should propagate that here as well? The problem is, what if I want to do a nested query where y.s has some value, but i want to see all the order values for that y as well. Lets just make it a filter at the root level for now, and then we can add a new api for y { s }. Something like y @filter(s: eq()) {s}

}
dgraph.uid : uid
}
var(func: type(Nested_Y)) @filter(eq(Nested_Y.s, "-")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am guessing you have made the change from the RFC(requirement for @search), so that you don't have to use @cascade?

var(func: type(Nested_Y)) @filter(eq(Nested_Y.s, "-")) {
queryNested_X_y as Nested_Z.x
}
Nested_XRoot as var(func: uid(Nested_X_3)) @filter(uid(Nested_X_Auth4))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we combine all of these queries? We have a lot of optimizations for multiple filters together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core internal mechanisms area/graphql Issues related to GraphQL support on Dgraph. go Pull requests that update Go code
Development

Successfully merging this pull request may close these issues.

4 participants