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 manage and return resourceId and resourceType for tag lookups.
The refactor is structural. The small behavior fixes are listed below.

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 extracted helper methods.
Replaced Boolean.getBoolean with Boolean.parseBoolean where a string value was being parsed.
Added null checks for the service offering and for nullable values passed to toHumanReadableSize.
Removed an unused variable.

Types of changes

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

How Has This Been Tested?

mvn -pl server -Dtest=ApiResponseHelperTest test on the branch rebased onto current main: 44 tests, 0 failures, 0 errors
Checkstyle passes

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

❌ Patch coverage is 64.12826% with 179 lines in your changes missing coverage. Please review.
✅ Project coverage is 19.97%. Comparing base (7ed0cec) to head (e64c939).
⚠️ Report is 71 commits behind head on main.

Files with missing lines	Patch %	Lines
...src/main/java/com/cloud/api/ApiResponseHelper.java	64.12%	123 Missing and 56 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #13490      +/-   ##
============================================
+ Coverage     19.91%   19.97%   +0.06%     
- Complexity    20198    20290      +92     
============================================
  Files          6373     6373              
  Lines        577230   577325      +95     
  Branches      70696    70704       +8     
============================================
+ Hits         114936   115309     +373     
+ Misses       449736   449418     -318     
- Partials      12558    12598      +40

Flag	Coverage Δ
uitests	`3.71% <ø> (ø)`
unittests	`21.24% <64.12%> (+0.06%)`	⬆️

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.

PrashantBhanage · 2026-10-06T12:12:46Z

@DaanHoogland @Damans227

I went through the Copilot findings against the current main branch after syncing the PR and restored the behavior that had been missing from the refactor, including:

key-pair response fields
domain resource tags
port-forwarding network name
system VM storage IP
keepmacaddressonpublicnic handling
VPC conserve mode
VPN gateway parameter indicators
secondary-IP descriptions
NIC enabled state
usage-response dispatcher coverage

The refactor itself remains focused on modularizing
ApiResponseHelper#createUsageResponse(Usage), without intentionally changing the existing behavior.

I also addressed the two maintainer concerns that were still actionable. The remaining maintainer point was style-only, as noted.

Validation completed:

ApiResponseHelperTest: 5/5 passing
createUsageResponse dispatcher test: 1/1 passing
clean working tree
latest fixes pushed in commit 0e07fe6

Could you please take another look at the updated PR?

Damans227 · 2026-10-06T12:32:11Z

Could you please take another look at the updated PR?

@PrashantBhanage my earlier points are covered and the missing response fields are back, outside the usage part it now matches main. one small difference left: network traffic records with no network attached lose the vm's tags, main still shows them. can we keep that like before?

PrashantBhanage · 2026-10-06T15:48:12Z

@Damans227 Fixed this too — network traffic records without an attached network now retain the VM tags like main.

Damans227 · 2026-10-06T16:10:07Z

network traffic records without an attached network now retain the VM tags like main

@PrashantBhanage that covers it, e64c939 sets the vm type back so those records keep the vm tags like main. thanks

PrashantBhanage · 2026-10-13T11:17:59Z

@DaanHoogland the Sonar check seems to fail only when posting the coverage comment due to a 403 error. Both targeted and full server tests pass locally. Should we handle this separately?

Damans227 · 2026-10-19T12:51:49Z

Should we handle this separately?

@PrashantBhanage yes, #14299 handles it. the 403 comes from the workflow, not this change

github-actions · 2026-10-26T07:13:00Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

Damans227 · 2026-10-29T12:16:10Z

                response.setSubject(certificate.getSubjectDN().toString());
                response.setIssuer(certificate.getIssuerDN().toString());
-                response.setSerialNum(certificate.getSerialNumberObject().toString());
+                response.setSerialNum(certificate.getSerialNumber().toString());


@PrashantBhanage any reason to change how the certificate serial number is shown here? main shows it in hex, this shows it as a plain number

@PrashantBhanage any reason to change how the certificate serial number is shown here? main shows it in hex, this shows it as a plain number

Okk So the serial number format changed because I swapped X509CertImpl for X509Certificate. I reverted that part, so handleCertificateResponse is the same as main now.

that covers it, thanks

Damans227

thanks @PrashantBhanage, CLGTM.

Damans227 · 2026-10-29T17:25:45Z

@PrashantBhanage this looks good on the code side.

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

2 of 14 tasks

PrashantBhanage force-pushed the fix-11635-refactor-usage branch from ded7ca3 to 59e1c0b Compare October 6, 2026 10:18

github-actions Bot added the status:has-conflicts label Oct 26, 2026

PrashantBhanage force-pushed the fix-11635-refactor-usage branch from e64c939 to 972b77d Compare October 29, 2026 11:39

github-actions Bot removed the status:has-conflicts label Oct 29, 2026

Damans227 reviewed Oct 29, 2026

View reviewed changes

Damans227 approved these changes Oct 29, 2026

View reviewed changes

Refactor: Modularize ApiResponseHelper#createUsageResponse

14fcce0

PrashantBhanage force-pushed the fix-11635-refactor-usage branch from 972b77d to 14fcce0 Compare October 29, 2026 16:22

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!

PrashantBhanage commented Oct 6, 2026

Uh oh!

Damans227 commented Oct 6, 2026

Uh oh!

PrashantBhanage commented Oct 6, 2026

Uh oh!

Damans227 commented Oct 6, 2026

Uh oh!

PrashantBhanage commented Oct 13, 2026

Uh oh!

Damans227 commented Oct 19, 2026

PrashantBhanage commented Jun 25, 2026 •

edited

Loading

codecov Bot commented Jun 25, 2026 •

edited

Loading

PrashantBhanage left a comment •

edited

Loading