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

Merged
merged 22 commits into from
Jan 14, 2025

Conversation

jopemachine
Copy link
Member

@jopemachine jopemachine commented Nov 7, 2024

Fixes #3325 (BA-420).

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 requested a review from fregataa November 7, 2024 10:22
@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
@jopemachine jopemachine requested a review from fregataa November 13, 2024 06:03
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 jopemachine requested a review from fregataa November 13, 2024 09:15
@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.

@jopemachine jopemachine force-pushed the fix/broken-vfolder-gql branch from 82e5273 to 730212e Compare November 27, 2024 04:30
@HyeockJinKim
Copy link
Collaborator

please check CI. @jopemachine

Comment on lines +1392 to +1428
case VFolderRow():
return cls(
id=row.id,
host=row.host,
quota_scope_id=row.quota_scope_id,
name=row.name,
user=row.user,
user_email=row.user_row.email if row.user_row is not None else None,
group=row.group,
group_name=row.group_row.name if row.group_row is not None else None,
creator=row.creator,
domain_name=row.domain_name,
unmanaged_path=row.unmanaged_path,
usage_mode=row.usage_mode,
permission=row.permission,
ownership_type=row.ownership_type,
max_files=row.max_files,
max_size=row.max_size, # in MiB
created_at=row.created_at,
last_used=row.last_used,
cloneable=row.cloneable,
status=row.status,
cur_size=row.cur_size,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason why it was separated? The behavior seems identical to that of Row

Copy link
Member Author

Choose a reason for hiding this comment

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

The two codes are not the same although the meaning is almost identical.
For example, on the Row side, it is row["id"], while on the VFolderRow side, it is id=row.id.
But I agree it would be great if there were a way to simplify the code further.

@jopemachine jopemachine force-pushed the fix/broken-vfolder-gql branch from 8ce145f to 5fd6dd0 Compare December 23, 2024 02:36
@github-actions github-actions bot added size:L 100~500 LoC and removed size:S 10~30 LoC labels Dec 23, 2024
@jopemachine jopemachine force-pushed the fix/broken-vfolder-gql branch from 019e513 to 45cab2f Compare December 30, 2024 02:10
@jopemachine
Copy link
Member Author

@HyeockJinKim Could you review the PR again and confirm if it's ready to be merged?

@jopemachine jopemachine changed the title fix: Regression of outdated vfolder GQL resolver fix(BA-420): Regression of outdated vfolder GQL resolver Jan 6, 2025
@HyeockJinKim
Copy link
Collaborator

Could you review this once more? @fregataa

@jopemachine
Copy link
Member Author

@fregataa This PR is set to BA Sprint 1.
Are you able to review it? If not, it will need to be postponed to BA Sprint 2.

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.

👍

@fregataa fregataa added this pull request to the merge queue Jan 14, 2025
Merged via the queue into main with commit d90f801 Jan 14, 2025
23 checks passed
@fregataa fregataa deleted the fix/broken-vfolder-gql branch January 14, 2025 08:18
lablup-octodog pushed a commit that referenced this pull request Jan 14, 2025
Co-authored-by: Sanghun Lee <[email protected]>
Backported-from: main (24.12)
Backported-to: 24.03
Backport-of: 3047
lablup-octodog pushed a commit that referenced this pull request Jan 14, 2025
Co-authored-by: Sanghun Lee <[email protected]>
Backported-from: main (24.12)
Backported-to: 24.09
Backport-of: 3047
lablup-octodog pushed a commit that referenced this pull request Jan 14, 2025
Co-authored-by: Sanghun Lee <[email protected]>
Backported-from: main (24.12)
Backported-to: 23.09
Backport-of: 3047
github-merge-queue bot pushed a commit that referenced this pull request Jan 14, 2025
github-merge-queue bot pushed a commit that referenced this pull request Jan 14, 2025
github-merge-queue bot pushed a commit that referenced this pull request Jan 14, 2025
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:L 100~500 LoC type:bug Reports about that are not working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vfolder GQL resolver is outdated
3 participants