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 purging user due to fkey constraint of main_access_key #1775

Merged
merged 9 commits into from
Jan 11, 2024

Conversation

fregataa
Copy link
Member

@fregataa fregataa commented Dec 11, 2023

follow-up #1761

The main_access_key foreign key has ondelete=RESTRICT constraint, which disallow to delete any user record.
Replace the ondelete constraint on the main_access_key foreign key with SET NULL because the existing implementation deletes keypair record before deleting user.

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version
  • 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

@fregataa fregataa added the skip:changelog Make the action workflow to skip towncrier check label Dec 11, 2023
@fregataa fregataa self-assigned this Dec 11, 2023
@github-actions github-actions bot added comp:manager Related to Manager component require:db-migration Automatically set when alembic migrations are added or updated size:M 30~100 LoC labels Dec 11, 2023
@fregataa fregataa requested review from achimnol and kyujin-cho and removed request for achimnol December 11, 2023 12:19
@fregataa fregataa added this to the 24.03 milestone Dec 11, 2023
@fregataa fregataa requested review from achimnol and gee05053 and removed request for kyujin-cho December 11, 2023 12:28
@fregataa fregataa modified the milestones: 24.03, 23.09 Dec 11, 2023
@fregataa fregataa added the urgency:blocker IT SHOULD BE RESOLVED BEFORE NEXT RELEASE! label Dec 14, 2023
@achimnol achimnol changed the title fix: enable to delete users fix: Regression of purging user due to fkey constraint of main_access_key Dec 15, 2023
@fregataa fregataa requested a review from kyujin-cho January 11, 2024 05:27
Comment on lines 631 to 638
user_query = sa.select(UserRow.uuid).where(UserRow.main_access_key == access_key)
uuid_list: list[str] = [
str(raw_uuid) for raw_uuid in (await db_session.scalars(user_query)).all()
]
if uuid_list:
return DeleteKeyPair(
False, f"the keypair is used as main access key by user(uuid: {uuid_list})"
)
Copy link
Member

Choose a reason for hiding this comment

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

Seems like these lines can be simplified by just evaluating result of COUNT(*) query, as the purpose of this block is to check if the user is utilizing this keypair as its main keypair.

@fregataa fregataa added this pull request to the merge queue Jan 11, 2024
Merged via the queue into main with commit ec984c7 Jan 11, 2024
25 checks passed
@fregataa fregataa deleted the fix/main-access-key-ondelete-delete branch January 11, 2024 14:52
fregataa added a commit that referenced this pull request Jan 11, 2024
…s_key` (#1775)

Backported-from: main
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 require:db-migration Automatically set when alembic migrations are added or updated size:M 30~100 LoC skip:changelog Make the action workflow to skip towncrier check urgency:blocker IT SHOULD BE RESOLVED BEFORE NEXT RELEASE!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants