Skip to content

fix(BA-2832): Seperate permission group DTOs based on whether relationship data load is needed#6415

Merged
HyeockJinKim merged 3 commits into
mainfrom
fix/BA-2832
Oct 27, 2025
Merged

fix(BA-2832): Seperate permission group DTOs based on whether relationship data load is needed#6415
HyeockJinKim merged 3 commits into
mainfrom
fix/BA-2832

Conversation

@seedspirit
Copy link
Copy Markdown
Contributor

resolves #6414 (BA-2832)

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version
  • Mention to the original issue
  • Installer updates including:
    • Fixtures for db schema changes
    • New mandatory config options
  • Update of end-to-end CLI integration tests in ai.backend.test
  • API server-client counterparts (e.g., manager API -> client SDK)
  • Test case(s) to:
    • Demonstrate the difference of before/after
    • Demonstrate the flow of abstract/conceptual models with a concrete implementation
  • Documentation
    • Contents in the docs directory
    • docstrings in public interfaces and type annotations

@seedspirit seedspirit self-assigned this Oct 27, 2025
@github-actions github-actions Bot added size:M 30~100 LoC comp:manager Related to Manager component labels Oct 27, 2025
@github-actions github-actions Bot added size:S 10~30 LoC and removed size:M 30~100 LoC labels Oct 27, 2025
@seedspirit seedspirit marked this pull request as ready for review October 27, 2025 06:00
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors permission group DTOs to separate concerns between basic data transfer and extended data that includes relationship data. The change introduces PermissionGroupExtendedData as a new DTO that includes nested permissions, while keeping PermissionGroupData as a lighter-weight alternative without relationships. This separation prevents lazy loading errors by making relationship data requirements explicit at the type level.

Key changes:

  • Introduced PermissionGroupExtendedData DTO to include nested permissions data
  • Updated RoleDataWithPermissions to use PermissionGroupExtendedData instead of PermissionGroupData
  • Added to_extended_data() method to PermissionGroupRow model for converting to the extended DTO

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/ai/backend/manager/data/permission/permission_group.py Adds new PermissionGroupExtendedData dataclass with permissions field
src/ai/backend/manager/models/rbac_models/permission/permission_group.py Implements to_extended_data() method and imports new DTO
src/ai/backend/manager/data/permission/role.py Updates import and type annotation to use PermissionGroupExtendedData
src/ai/backend/manager/models/rbac_models/role.py Changes method call from to_data() to to_extended_data()
changes/6451.enhance.md Documents the enhancement

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread changes/6451.enhance.md
Copy link
Copy Markdown
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.

Good. How about setting PR prefix as fix rather than refactor?

@seedspirit seedspirit changed the title refactor(BA-2832): Seperate permission group DTOs based on whether relationship data load is needed fix(BA-2832): Seperate permission group DTOs based on whether relationship data load is needed Oct 27, 2025
@HyeockJinKim HyeockJinKim added this pull request to the merge queue Oct 27, 2025
Merged via the queue into main with commit ac170fb Oct 27, 2025
32 checks passed
@HyeockJinKim HyeockJinKim deleted the fix/BA-2832 branch October 27, 2025 16:22
jopemachine pushed a commit that referenced this pull request Oct 30, 2025
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:S 10~30 LoC

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Creating User, Group fails with asyncpg error

4 participants