server: remove user-project mappings when delete a project by weizhouapache · Pull Request #14150 · apache/cloudstack

weizhouapache · 2026-09-11T10:09:29Z

Description

This PR fixes #12601

When an account is added to a project with a project role (e.g. via addUserToProject), the resulting project_account row has a non-null user_id. Project cleanup (deleteProject ... cleanup=true) unassigns every account from the project via ProjectAccountDaoImpl#findByProjectIdUserId(projectId, accountId, null), expecting a null userId to mean "match this account regardless of which user_id the row has".

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
Build/CI
Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Verified on a live 2-node management server deployment:

Built and deployed the patched jar to both management nodes.
Reproduced the original issue with the pre-fix code: created an account, created a project, added the account's user to the project with addUserToProject roletype=Admin (producing a project_account row with a non-null user_id), deleted the project with cleanup=true (reported success), then attempted deleteAccount — reproduced the exact errorcode 431 failure from Unable to delete account if the project is removed #12601.
Repeated the same steps against the fixed jar: deleteProject cleanup=true followed by deleteAccount both succeeded, and the account no longer appeared in listAccounts.
Confirmed both management nodes ended up running the identical fixed build.

How did you try to break this feature and the system with this change?

codecov · 2026-09-11T10:18:14Z

Codecov Report

❌ Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.42%. Comparing base (a4d3c66) to head (f4aa2c4).
⚠️ Report is 9 commits behind head on 4.20.

Files with missing lines	Patch %	Lines
.../com/cloud/projects/dao/ProjectAccountDaoImpl.java	0.00%	7 Missing ⚠️
...in/java/com/cloud/projects/ProjectManagerImpl.java	0.00%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.20   #14150      +/-   ##
============================================
+ Coverage     16.37%   16.42%   +0.04%     
- Complexity    13614    13675      +61     
============================================
  Files          5669     5669              
  Lines        501527   501629     +102     
  Branches      60922    60938      +16     
============================================
+ Hits          82148    82397     +249     
+ Misses       410171   409994     -177     
- Partials       9208     9238      +30

Flag	Coverage Δ
uitests	`4.16% <ø> (+0.02%)`	⬆️
unittests	`17.29% <0.00%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

🟡 Changes recommended

Add regression tests covering removal of user-specific mappings during project cleanup.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes project cleanup so user-specific project mappings are removed when deleting a project, allowing associated accounts to be deleted.

Changes:

Allows nullable userId lookups in the project-account DAO.
Uses account-wide lookup during project cleanup.
Updates the DAO contract.

File summaries

File	Summary
`server/src/main/java/com/cloud/projects/ProjectManagerImpl.java`	Removes mappings regardless of user association during cleanup.
`engine/schema/src/main/java/com/cloud/projects/dao/ProjectAccountDaoImpl.java`	Implements optional user filtering.
`engine/schema/src/main/java/com/cloud/projects/dao/ProjectAccountDao.java`	Updates the DAO method signature.

Review details

Suppressed comments (1)

engine/schema/src/main/java/com/cloud/projects/dao/ProjectAccountDaoImpl.java:116

Please add a regression test for the new nullable path. The current tests exercise user-specific lookups but do not verify that a user-specific project_account row is found and removed when userId is null; without that coverage, the fix for #12601 can regress to the old account-level-only lookup unnoticed.

        if (userId != null) {
            sc.setParameters("userId", userId);
        }

Files reviewed: 3/3 changed files
Comments generated: 1
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

    @Override
    public boolean unassignAccountFromProject(long projectId, long accountId) {
-        ProjectAccountVO projectAccount = _projectAccountDao.findByProjectIdAccountId(projectId, accountId);
+        ProjectAccountVO projectAccount = _projectAccountDao.findByProjectIdUserId(projectId, accountId, null);


DaanHoogland

clgtm, can you generate some tests @weizhouapache ?

weizhouapache · 2026-10-05T08:02:01Z

clgtm, can you generate some tests @weizhouapache ?

I tried to add some unit tests, it worked with 4.23
but when backported to this PR, it did not work.
it seems to have some conflicts with #13817

Damans227 · 2026-10-14T15:25:08Z

    @Override
    public boolean unassignAccountFromProject(long projectId, long accountId) {
-        ProjectAccountVO projectAccount = _projectAccountDao.findByProjectIdAccountId(projectId, accountId);
+        ProjectAccountVO projectAccount = _projectAccountDao.findByProjectIdUserId(projectId, accountId, null);


what about projects deleted before this fix? their leftover user rows still block deleting the account, should the upgrade clean those up too?

yes, but I am hesitated to do it, as it needs to be added to all upgrade files (4.20, 4.22, 24.0)

Damans227 · 2026-10-14T15:25:09Z

        SearchCriteria<ProjectAccountVO> sc = AllFieldsSearch.create();
        sc.setParameters("projectId", projectId);
-        sc.setParameters("userId", userId);
+        if (userId != null) {


can we keep this lookup as is and remove each row by its id in the cleanup loop instead? the loop already has the row

added a new method removeByProjectIdAccountId to remove all records by projectid and accountid

This reverts commit 506fbc0.

unassignAccountFromProject looked up a single project_account row with userId IS NULL and removed at most that one. Mappings created for users, and extra users of the same account, were left behind when a project was cleaned up. unassignAccountFromProject now removes every project_account row matching the project and account through ProjectAccountDao.removeByProjectIdAccountId. cleanupProject keeps its per-row loop and therefore covers user mappings too.

weizhouapache · 2026-10-26T11:03:24Z

@blueorangutan package

blueorangutan · 2026-10-26T11:04:05Z

@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2026-10-26T12:29:08Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19749

weizhouapache · 2026-10-26T13:38:03Z

@blueorangutan test

blueorangutan · 2026-10-26T13:40:05Z

@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

server: remove user-project mappings when delete a project

506fbc0

boring-cyborg Bot added the component:projects label Sep 11, 2026

weizhouapache added this to the 4.20.4 milestone Sep 11, 2026

weizhouapache mentioned this pull request Sep 11, 2026

Delete user-project mappings on project deletion #13456

Closed

2 of 12 tasks

weizhouapache requested a lite review from Copilot September 14, 2026 07:52

weizhouapache marked this pull request as ready for review September 14, 2026 07:52

Copilot started reviewing on behalf of weizhouapache September 14, 2026 07:57

View session

Copilot AI reviewed Sep 14, 2026

View reviewed changes

winterhazel self-requested a review September 22, 2026 03:09

DaanHoogland reviewed Sep 30, 2026

View reviewed changes

weizhouapache force-pushed the fix-12601-project-account-cleanup branch from 9099c91 to 506fbc0 Compare October 5, 2026 08:00

Damans227 reviewed Oct 14, 2026

View reviewed changes

weizhouapache added 2 commits October 26, 2026 11:46

Revert "server: remove user-project mappings when delete a project"

a5a8aee

This reverts commit 506fbc0.

Conversation

weizhouapache commented Sep 11, 2026

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Uh oh!

codecov Bot commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Oct 5, 2026

Uh oh!

Damans227 Oct 14, 2026

Choose a reason for hiding this comment

Uh oh!

weizhouapache Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

Damans227 Oct 14, 2026

Choose a reason for hiding this comment

Uh oh!

weizhouapache Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Oct 26, 2026

Uh oh!

blueorangutan commented Oct 26, 2026

Uh oh!

blueorangutan commented Oct 26, 2026

Uh oh!

weizhouapache commented Oct 26, 2026

Uh oh!

blueorangutan commented Oct 26, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Sep 11, 2026 •

edited

Loading