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

feature: impl graphql relay resolver for user and group #1719

Merged
merged 35 commits into from
Nov 29, 2023

Conversation

fregataa
Copy link
Member

@fregataa fregataa commented Nov 15, 2023

Implement async compatible graphql relay Node and Connection types.
Implement user/group relay Node and Connection.

Why re-invent GraphQL relay?

Current Graphene(v 3.3.0) has a relay implementation but it does not support asynchronous resolver functions. Additionally, Graphene Connection resolver is implemented to only accept one complete array(Iterable value) without considering pagination, which is a huge performance issue.
Here, the newly "invented" Connection took pagination into account. But, pagination order of this Connection can only be either forward or backward in a single query, meaning that queries such as first and last together or after and before together are limited.

Query all groups

# query all groups
query GroupsQuery {
  group_nodes {
    edges {
      node {
        id
        name
      }
    }
  }
}

Query group with relay id

query GroupsQuery($id: String!) {
    group_node(id: $id) {
        id
        name
        user_nodes(order: "created_at", offset: 2, first: 3) {
            edges {
                node {
                    id
                    username
                    created_at
                }
            }
            count
            pageInfo {
                hasNextPage
                hasPreviousPage
            }
        }
    }
}

# variables
# {
#    "id": "R3JvdXBOb2RlOjJkZTJiOTY5LTFkMDQtNDhhNi1hZjE2LTBiYzhhZGIzYzgzMQ=="
# }

Query node with node's global id

{
    node(id: "R3JvdXBOb2RlOjJkZTJiOTY5LTFkMDQtNDhhNi1hZjE2LTBiYzhhZGIzYzgzMQ==") {
        ... on GroupNode {
            name
            description
        }
    }
}

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version
  • Documentation
  • API server-client counterparts (e.g., manager API -> client SDK)
  • Test case(s) to demonstrate the difference of before/after

Sorry, something went wrong.

@fregataa fregataa added this to the 24.03 milestone Nov 15, 2023
@fregataa fregataa requested a review from yomybaby November 15, 2023 16:35
@fregataa fregataa self-assigned this Nov 15, 2023
@fregataa fregataa marked this pull request as draft November 15, 2023 16:35
@github-actions github-actions bot added comp:manager Related to Manager component size:L 100~500 LoC labels Nov 15, 2023
@fregataa fregataa marked this pull request as ready for review November 17, 2023 15:45
@fregataa fregataa marked this pull request as draft November 20, 2023 01:51
@github-actions github-actions bot added size:XL 500~ LoC and removed size:L 100~500 LoC labels Nov 20, 2023
@fregataa fregataa marked this pull request as ready for review November 28, 2023 08:34
@yomybaby
Copy link
Member

It works properly.

  group_nodes(order:"-name") {
    edges {
      node {
        id
        name
        user_nodes(first:1) {
          edges {
            node {
              username
            }
          }
          count
        }
      }
      cursor
    }
    pageInfo {
      hasNextPage
      hasPreviousPage
      startCursor
      endCursor
    }
  }

Copy link
Member

@yomybaby yomybaby left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@achimnol achimnol left a comment

Choose a reason for hiding this comment

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

Just a minor refactoring to make it more readable:
SQLInfoForGQLConnGraphQLConnectionSQLInfo

@fregataa fregataa requested a review from achimnol November 28, 2023 13:24
Copy link
Member

@rapsealk rapsealk left a comment

Choose a reason for hiding this comment

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

Minor fixes and suggestions. Thanks for the job!

src/ai/backend/manager/models/base.py Outdated Show resolved Hide resolved
src/ai/backend/manager/models/base.py Outdated Show resolved Hide resolved
src/ai/backend/manager/models/base.py Outdated Show resolved Hide resolved
Comment on lines +306 to +308
group_node = graphene.Field(
GroupNode, id=graphene.String(required=True), description="Added in 24.03.0."
)
Copy link
Member

Choose a reason for hiding this comment

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

If AsyncNode is to handle Relay Global ID, how about setting id field to AsyncNode itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have tried it, but it needs so much work. so I deprioritized this task

Copy link
Member

Choose a reason for hiding this comment

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

Let's try this in next PR!

@fregataa fregataa requested a review from rapsealk November 29, 2023 05:20
Copy link
Member

@rapsealk rapsealk left a comment

Choose a reason for hiding this comment

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

LGTM!
I'm wondering if there can be a better name than OrderingItem to emphasize the action "ordering".

@fregataa fregataa added this pull request to the merge queue Nov 29, 2023
Merged via the queue into main with commit 59b6481 Nov 29, 2023
22 checks passed
@fregataa fregataa deleted the feature/graphene-async-relay branch November 29, 2023 11:28
fregataa added a commit that referenced this pull request Feb 19, 2024
Backported-from: main (24.03)
Backported-to: 23.09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:manager Related to Manager component size:XL 500~ LoC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants