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: Regression of outdated vfolder GQL resolver #3047

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

Conversation

jopemachine
Copy link
Member

@jopemachine jopemachine commented Nov 7, 2024

When using vfolder GQL in the main branch, the following error occurs:

{
  "data": {
    "vfolder": null
  },
  "errors": [
    {
      "message": "VirtualFolder.batch_load_by_id() got an unexpected keyword argument 'user_uuid'",
      "locations": [
        {
          "line": 2,
          "column": 5
        }
      ],
      "path": [
        "vfolder"
      ]
    }
  ]
}

This PR resolves several issues causing the VFolder GQL to malfunction in the current main branch.

Request example

query ComputeContainer {
    vfolder (id: "VFOLDER_ID") {
        name
    }
}

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version

📚 Documentation preview 📚: https://sorna--3047.org.readthedocs.build/en/3047/


📚 Documentation preview 📚: https://sorna-ko--3047.org.readthedocs.build/ko/3047/

@github-actions github-actions bot added area:docs Documentations comp:manager Related to Manager component size:S 10~30 LoC labels Nov 7, 2024
@jopemachine jopemachine added the type:bug Reports about that are not working label Nov 7, 2024
@jopemachine jopemachine changed the title fix: Broken vfolders GQL fix: Broken Vfolder GQL Nov 7, 2024
@@ -1875,16 +1875,23 @@ async def resolve_vfolder(
user_id: Optional[uuid.UUID] = None,
) -> Optional[VirtualFolder]:
graph_ctx: GraphQueryContext = info.context
user_role = graph_ctx.user["role"]
loader = graph_ctx.dataloader_manager.get_loader(
graph_ctx,
"VirtualFolder.by_id",
Copy link
Member Author

@jopemachine jopemachine Nov 7, 2024

Choose a reason for hiding this comment

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

I want to use get_loader_by_func and VirtualFolder.batch_load_by_id here, but then I'm getting a type error since the return type of VirtualFolder.batch_load_by_id differs from what get_loader_by_func expects.

@jopemachine jopemachine added this to the 23.09 milestone Nov 7, 2024
@jopemachine jopemachine marked this pull request as ready for review November 7, 2024 10:22
@@ -1627,7 +1627,7 @@ async def batch_load_by_id(
query,
cls,
ids,
lambda row: row["user"],
Copy link
Member Author

@jopemachine jopemachine Nov 7, 2024

Choose a reason for hiding this comment

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

Since user_id is provided as a separate argument, the id here should be the vfolder's id.

Copy link
Member

@fregataa fregataa left a comment

Choose a reason for hiding this comment

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

I think the news fragment has been sabotaged by gitub action.. please resolve it

@jopemachine
Copy link
Member Author

I think the news fragment has been sabotaged by gitub action.. please resolve it

Resolved in #3059.

@fregataa fregataa self-requested a review November 12, 2024 23:44
Copy link
Member

@fregataa fregataa left a comment

Choose a reason for hiding this comment

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

Please update the PR title and the news fragment to "which GQL query resolver" is fixed. Broken Vfolder GQL is too vague

@jopemachine jopemachine changed the title fix: Broken Vfolder GQL fix: Regression of VFolder GQL outdated resolver Nov 13, 2024
@jopemachine jopemachine changed the title fix: Regression of VFolder GQL outdated resolver fix: Regression of VFolder GQL due to outdated resolver Nov 13, 2024
Copy link
Member

@fregataa fregataa left a comment

Choose a reason for hiding this comment

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

Please refer which GQL query resolver is fixed. e.g. Fix GraphQL vfolder query.
VFolder GQL could mean many other queries such as vfolder, vfolder_list, vfolder_node etc

@jopemachine jopemachine changed the title fix: Regression of VFolder GQL due to outdated resolver fix: Regression of outdated vfolder GQL resolver Nov 13, 2024
@jopemachine
Copy link
Member Author

jopemachine commented Nov 13, 2024

Please refer which GQL query resolver is fixed. e.g. Fix GraphQL vfolder query. VFolder GQL could mean many other queries such as vfolder, vfolder_list, vfolder_node etc

I used uppercase letters to maintain consistency with other PRs, but since it could cause misunderstandings, I changed it to lowercase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:docs Documentations comp:manager Related to Manager component size:S 10~30 LoC type:bug Reports about that are not working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants