Refactor: Modularize ApiResponseHelper#createUsageResponse by PrashantBhanage · Pull Request #13490 · apache/cloudstack

PrashantBhanage · 2026-06-25T08:33:25Z

Description

This PR addresses the technical debt in ApiResponseHelper#createUsageResponse(Usage) by modularizing the 530-line method.

Extracted the 500+ line switch/if-else block into a main dispatcher method populateUsageTypeSpecificDetails.
Created 18 individual private helper methods for each specific UsageType to improve maintainability and readability.
Introduced a UsageResourceDetails container class to safely manage and return resourceId and resourceType state for tag lookups.
Verified zero business logic changes; strictly structural refactoring.

Additional Changes

Added JUnit Jupiter, JUnit Vintage, and mockito-junit-jupiter test dependencies to server/pom.xml to support JUnit 5 tests.
Migrated ApiResponseHelperTest from JUnit 4 to JUnit 5 and added unit tests for the 18 extracted helper methods.
Fixed sonar-check.yml end-of-file formatting (pre-commit hook).
Fixed indentation and added proper java.security.cert imports in handleCertificateResponse.

Types of changes

Cleanup (Code refactoring and cleanup, that may add test cases)
Test (unit or integration test code)

How Has This Been Tested?

All 41 unit tests pass locally via mvn -pl server -Dtest=ApiResponseHelperTest test
Full server test suite passes: 3256 tests, 0 failures
Checkstyle passes with 0 violations
Packaging verified on el8, el9, el10, debian, suse15

boring-cyborg · 2026-06-25T08:33:29Z

Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
Here are some useful points:

In case of a new feature add useful documentation (raise doc PR at https://github.com/apache/cloudstack-documentation)
Be patient and persistent. It might take some time to get a review or get the final approval from the committers.
Pay attention to the quality of your code, ensure tests are passing and your PR doesn't have conflicts.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Issues, Mailing list and Slack.
Be sure to read the CloudStack Coding Conventions.
Apache CloudStack is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@cloudstack.apache.org (https://cloudstack.apache.org/mailing-lists.html)
Slack: https://apachecloudstack.slack.com/

sureshanaparti · 2026-06-25T12:57:46Z

@blueorangutan package

codecov · 2026-06-25T13:04:26Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 3.41%. Comparing base (77fbb0f) to head (d731c70).

❗ There is a different number of reports uploaded between BASE (77fbb0f) and HEAD (d731c70). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (77fbb0f) HEAD (d731c70)

unittests 1 0

Additional details and impacted files

@@              Coverage Diff              @@
##               main   #13490       +/-   ##
=============================================
- Coverage     19.73%    3.41%   -16.33%     
=============================================
  Files          6371      487     -5884     
  Lines        575784    41863   -533921     
  Branches      70478     7912    -62566     
=============================================
- Hits         113632     1429   -112203     
+ Misses       449805    40234   -409571     
+ Partials      12347      200    -12147

Flag	Coverage Δ
uitests	`3.41% <ø> (ø)`
unittests	`?`

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.

PrashantBhanage · 2026-06-26T04:39:55Z

Added unit tests for all 18 extracted UsageType helper methods to address the coverage concern. Also added the required JUnit Jupiter and mockito-junit-jupiter test dependencies to server/pom.xml. All 41 tests pass locally.

PrashantBhanage

I don't know where the problem is.

DaanHoogland · 2026-07-02T18:18:32Z

I don't know where the problem is.

what problem are you talking about, @PrashantBhanage ?

I only see an integration test failure which seems unrelated.

PrashantBhanage · 2026-07-03T04:18:20Z

I don't know where the problem is.

what problem are you talking about, @PrashantBhanage ?

I only see an integration test failure which seems unrelated.

Got it, thanks! I raised #13516 for the flaky test separately. The refactor itself should be good.

DaanHoogland · 2026-07-03T14:31:14Z

@blueorangutan test

Copilot

Pull request overview

This PR refactors ApiResponseHelper#createUsageResponse(Usage) by extracting usage-type-specific logic into a dispatcher (populateUsageTypeSpecificDetails) plus multiple private helper methods, introducing a small container (UsageResourceDetails) to carry resourceId/resourceType for tag lookups.

Changes:

Modularized usage-response population into dedicated helper methods keyed by UsageTypes.
Updated ApiResponseHelperTest to JUnit 5 + Mockito Jupiter and added focused tests for several usage-type helpers.
Added JUnit Jupiter / Vintage and Mockito Jupiter test dependencies to the server module POM.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 14 comments.

File	Description
`server/src/main/java/com/cloud/api/ApiResponseHelper.java`	Extracts usage-type logic into helper methods and centralizes tag lookup via `UsageResourceDetails`.
`server/src/test/java/com/cloud/api/ApiResponseHelperTest.java`	Migrates to JUnit 5/Mockito Jupiter and adds unit tests for the new helper methods via reflection.
`server/pom.xml`	Adds JUnit 5 (Jupiter + Vintage) and Mockito Jupiter dependencies to support the test migration.

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

            if (tmpl != null) {
-                usageRecResponse.setUsageId(tmpl.getUuid());
-                resourceId = tmpl.getId();
-            }
-            //Template/ISO Size
-            usageRecResponse.setSize(usageRecord.getSize());
-            if (usageRecord.getUsageType() == UsageTypes.ISO) {
-                usageRecResponse.setVirtualSize(usageRecord.getSize());
-                resourceType = ResourceObjectType.ISO;
-            } else {
-                usageRecResponse.setVirtualSize(usageRecord.getVirtualSize());
-                resourceType = ResourceObjectType.Template;
-            }
-            if (!oldFormat) {
-                final StringBuilder builder = new StringBuilder();
-                if (usageRecord.getUsageType() == UsageTypes.TEMPLATE) {
-                    builder.append("Template usage");
-                } else if (usageRecord.getUsageType() == UsageTypes.ISO) {
-                    builder.append("ISO usage");
-                }
-                if (tmpl != null) {
-                    builder.append(" for ").append(tmpl.getName()).append(" (").append(tmpl.getUuid()).append(") ")
-                            .append("with size ").append(toHumanReadableSize(usageRecord.getSize())).append(" and virtual size ").append(toHumanReadableSize(usageRecord.getVirtualSize()));
-                }
-                usageRecResponse.setDescription(builder.toString());
+                builder.append(" for ").append(tmpl.getName()).append(" (").append(tmpl.getUuid()).append(") ")
+                        .append("with size ").append(toHumanReadableSize(usageRecord.getSize())).append(" and virtual size ").append(toHumanReadableSize(usageRecord.getVirtualSize()));


            if (snap != null) {
-                usageRecResponse.setUsageId(snap.getUuid());
-                resourceId = snap.getId();
-            }
-            //Snapshot Size
-            usageRecResponse.setSize(usageRecord.getSize());
-            if (!oldFormat) {
-                final StringBuilder builder = new StringBuilder();
-                builder.append("Snapshot usage ");
-                if (snap != null) {
-                    builder.append("for ").append(snap.getName()).append(" (").append(snap.getUuid()).append(") ")
-                            .append("with size ").append(toHumanReadableSize(usageRecord.getSize()));
-                }
-                usageRecResponse.setDescription(builder.toString());
+                builder.append("for ").append(snap.getName()).append(" (").append(snap.getUuid()).append(") ")
+                        .append("with size ").append(toHumanReadableSize(usageRecord.getSize()));


PrashantBhanage · 2026-07-04T12:42:34Z

Applied all the Copilot suggestions, null checks for svcOffering, nullable Long values, and removed the unused variable.

PrashantBhanage · 2026-07-05T04:40:37Z

Fixed the build, replaced the internal X509CertImpl with standard Java API and removed the unused imports that Copilot introduced.

PrashantBhanage · 2026-07-06T14:44:03Z

I just pushed a quick fix to add the missing EOF newline in ApiResponseHelperTest.java, so the pre-commit check should be green now.

Also, just wanted to give a heads-up that the UI Build and Sonar JaCoCo failures look unrelated to my Java changes. The UI test seems to be timing out on a Vue router test (Status.spec.js), and Sonar is just throwing a 403 permission error right at the end when trying to drop a PR comment.

Let me know if there's anything else I need to tweak!

PrashantBhanage · 2026-07-16T11:55:04Z

It should be good now!

DaanHoogland · 2026-07-20T17:41:04Z

@blueorangutan test

Damans227

Nit: this "Refactor" PR also carries a .github/workflows/sonar-check.yml change and a server/pom.xml dependency bump (junit-jupiter/vintage/mockito-jupiter), plus a full JUnit4→JUnit5 migration of ApiResponseHelperTest. Worth calling out in the description since none of that is "modularize createUsageResponse".

Damans227 · 2026-07-21T18:44:08Z

+        return resourceDetails;
+    }
+
+    private UsageResourceDetails populateNetworkOfferingUsageResponse(Usage usageRecord, UsageRecordResponse usageRecResponse, boolean oldFormat, VMInstanceVO vmInstance) {


Nit: this method (and populateVpnUsersUsageResponse, populateVolumeSecondaryUsageResponse, populateBucketUsageResponse below) just returns new UsageResourceDetails() directly, while the other populate* methods declare a local resourceDetails, mutate it, and return that. Minor inconsistency in the extraction pattern.

PrashantBhanage · 2026-07-22T04:36:47Z

@Damans227 Fixed the indentation on handleCertificateResponse, added proper imports for java.security.cert classes instead of using fully-qualified names inline. Will also update the PR description to mention the pom.xml dependency additions and JUnit 5 migration.

DaanHoogland · 2026-07-24T09:06:04Z

@blueorangutan package

DaanHoogland · 2026-07-24T10:34:25Z

@blueorangutan test

DaanHoogland · 2026-07-24T10:35:53Z

thanks @PrashantBhanage , one more, can you rebase the PR to the 4.22 LTS branch?

PrashantBhanage · 2026-07-24T15:17:14Z

@DaanHoogland I have opened the 4.22 LTS backport PR here: [https://github.com//pull/13695]

Note: I intentionally excluded the JUnit 5 test migration from the backport to avoid framework conflicts with the stable 4.22 testing architecture, but the core backend refactor and the .toString() bug fix are included.

DaanHoogland · 2026-07-24T15:49:29Z

Note: I intentionally excluded the JUnit 5 test migration from the backport to avoid framework conflicts with the stable 4.22 testing architecture, but the core backend refactor and the .toString() bug fix are included.

ok, that would be an argument to go for this version and to abandon #13695

Fixes apache#11635 - Extracted the 500+ line switch/if-else block into a main dispatcher method `populateUsageTypeSpecificDetails`. - Created 18 individual private helper methods for each specific `UsageType` to improve maintainability and readability. - Introduced a `UsageResourceDetails` container class to safely manage and return `resourceId` and `resourceType` state for tag lookups. - Verified zero business logic changes; strictly structural refactoring.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

…unused imports

…ation

…port)

DaanHoogland · 2026-08-31T14:22:57Z

@blueorangutan shutup

DaanHoogland · 2026-08-31T14:34:37Z

@blueorangutan package

blueorangutan · 2026-08-31T14:36:05Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2026-08-31T14:50:06Z

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 19031

DaanHoogland · 2026-09-01T07:23:54Z

@PrashantBhanage can you look at the build errors, please?

boring-cyborg Bot added the component:build label Jun 26, 2026

weizhouapache added this to the 4.24.0 milestone Jun 29, 2026

PrashantBhanage mentioned this pull request Jun 30, 2026

test_mm_domain_limits fails intermittently due to unhandled memory resource limit error (errorCode 535) #13516

Open

PrashantBhanage commented Jul 2, 2026

View reviewed changes

DaanHoogland requested a review from Copilot July 3, 2026 14:28

Copilot started reviewing on behalf of DaanHoogland July 3, 2026 14:28 View session

Copilot AI reviewed Jul 3, 2026

View reviewed changes

apache deleted a comment from blueorangutan Jul 13, 2026

boring-cyborg Bot added component:integration component:UI labels Jul 14, 2026

Damans227 reviewed Jul 21, 2026

View reviewed changes

PrashantBhanage mentioned this pull request Jul 24, 2026

Refactor: Modularize ApiResponseHelper#createUsageResponse (4.22 Backport) #13695

Open

14 tasks

DaanHoogland added this to CloudStack Testing Aug 31, 2026

github-project-automation Bot moved this to Backlog in CloudStack Testing Aug 31, 2026

PrashantBhanage and others added 10 commits August 31, 2026 16:21

test: Add unit tests for 18 extracted UsageType helper methods

08f49f1

fix: Apply Copilot null-check suggestions and remove unused variable

8d2e19e

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Fix: Replace internal X509CertImpl with standard Java API and remove …

dffbb44

…unused imports

test: Fix populateIpAddressUsageResponseTest after Copilot refactor

1580fb2

Fix: Add missing EOF newline to ApiResponseHelperTest

fa1bbde

fix: pre-commit end-of-file fixes

7dd2263

fix: add proper imports for java.security.cert classes and fix indent…

23b612b

…ation

Fix: Remove invalid .toString() type mismatch in usage entity lookups

21a3662

Refactor: modularize ApiResponseHelper#createUsageResponse (4.22 back…

d731c70

…port)

DaanHoogland force-pushed the fix-11635-refactor-usage branch from da39c45 to d731c70 Compare August 31, 2026 14:21

DaanHoogland moved this from Backlog to Ready in CloudStack Testing Aug 31, 2026

DaanHoogland moved this from Ready to conflict/waiting in CloudStack Testing Sep 1, 2026

Uh oh!

Conversation

PrashantBhanage commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional Changes

Types of changes

How Has This Been Tested?

Uh oh!

boring-cyborg Bot commented Jun 25, 2026

Uh oh!

sureshanaparti commented Jun 25, 2026

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

PrashantBhanage commented Jun 26, 2026

Uh oh!

PrashantBhanage left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Jul 2, 2026

Uh oh!

PrashantBhanage commented Jul 3, 2026

Uh oh!

DaanHoogland commented Jul 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PrashantBhanage commented Jul 4, 2026

Uh oh!

PrashantBhanage commented Jul 5, 2026

Uh oh!

PrashantBhanage commented Jul 6, 2026

Uh oh!

PrashantBhanage commented Jul 16, 2026

Uh oh!

DaanHoogland commented Jul 20, 2026

Uh oh!

Damans227 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Damans227 Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

PrashantBhanage commented Jul 22, 2026

Uh oh!

DaanHoogland commented Jul 24, 2026

Uh oh!

DaanHoogland commented Jul 24, 2026

Uh oh!

DaanHoogland commented Jul 24, 2026

Uh oh!

PrashantBhanage commented Jul 24, 2026

Uh oh!

DaanHoogland commented Jul 24, 2026

Uh oh!

DaanHoogland commented Aug 31, 2026

Uh oh!

DaanHoogland commented Aug 31, 2026

Uh oh!

blueorangutan commented Aug 31, 2026

Uh oh!

blueorangutan commented Aug 31, 2026

Uh oh!

PrashantBhanage commented Jun 25, 2026 •

edited

Loading

codecov Bot commented Jun 25, 2026 •

edited

Loading

PrashantBhanage left a comment •

edited

Loading