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

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.

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

blueorangutan · 2026-06-25T13:00:05Z

@sureshanaparti 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.

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

Codecov Report

❌ Patch coverage is 49.50249% with 203 lines in your changes missing coverage. Please review.
✅ Project coverage is 19.53%. Comparing base (06aebb6) to head (b082934).
⚠️ Report is 124 commits behind head on main.

Files with missing lines	Patch %	Lines
...src/main/java/com/cloud/api/ApiResponseHelper.java	49.50%	121 Missing and 82 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #13490      +/-   ##
============================================
+ Coverage     18.94%   19.53%   +0.59%     
- Complexity    18366    19463    +1097     
============================================
  Files          6192     6303     +111     
  Lines        556361   569380   +13019     
  Branches      67908    69800    +1892     
============================================
+ Hits         105407   111254    +5847     
- Misses       439383   445905    +6522     
- Partials      11571    12221     +650

Flag	Coverage Δ
uitests	`3.41% <ø> (-0.11%)`	⬇️
unittests	`20.81% <49.50%> (+0.65%)`	⬆️

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.

blueorangutan · 2026-06-25T14:05:05Z

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

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.

blueorangutan · 2026-06-26T07:50:46Z

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

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

blueorangutan · 2026-07-03T14:32:03Z

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

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()));


blueorangutan · 2026-07-04T04:53:12Z

[SF] Trillian test result (tid-16476)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 48672 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13490-t16476-kvm-ol8.zip
Smoke tests completed. 151 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

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

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

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

…unused imports

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!

blueorangutan · 2026-07-13T17:08:04Z

@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-07-13T17:58:14Z

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

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

It should be good now!

blueorangutan · 2026-07-20T12:52:58Z

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

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

@blueorangutan test

blueorangutan · 2026-07-20T17:42:03Z

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

blueorangutan · 2026-07-21T15:18:52Z

[SF] Trillian test result (tid-16594)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 74092 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13490-t16594-kvm-ol8.zip
Smoke tests completed. 146 look OK, 10 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestNASBackupAndRecovery>:setup	`Error`	0.00	test_backup_recovery_nas.py
ContextSuite context=TestDeployVMsInParallel>:setup	`Error`	0.00	test_deploy_vms_in_parallel.py
ContextSuite context=TestConsoleEndpoint>:setup	`Error`	0.00	test_console_endpoint.py
test_03_verify_libvirt_attach_disk	`Error`	3.37	test_deploy_virtio_scsi_vm.py
test_04_verify_guest_lspci	`Failure`	40.49	test_deploy_virtio_scsi_vm.py
test_05_change_vm_ostype_restart	`Failure`	10.19	test_deploy_virtio_scsi_vm.py
test_06_verify_guest_lspci_again	`Failure`	40.53	test_deploy_virtio_scsi_vm.py
ContextSuite context=TestInternalLb>:setup	`Error`	0.00	test_internal_lb.py
test_02_deploy_vm_with_extraconfig_kvm	`Error`	1.42	test_deploy_vm_extra_config_data.py
test_03_update_vm_with_extraconfig_kvm	`Error`	1.40	test_deploy_vm_extra_config_data.py
ContextSuite context=TestDeployVMFromISO>:setup	`Error`	0.00	test_deploy_vm_iso.py
test_03_create_vpc_domain_vpc_offering	`Error`	13.14	test_domain_vpc_offerings.py
test_04_validate_vpc_offering_conserve_mode_disabled	`Error`	1518.29	test_domain_vpc_offerings.py
test_02_upgrade_kubernetes_cluster	`Failure`	10.92	test_kubernetes_clusters.py
test_03_deploy_and_scale_kubernetes_cluster	`Failure`	6.63	test_kubernetes_clusters.py
test_04_autoscale_kubernetes_cluster	`Failure`	9.74	test_kubernetes_clusters.py
test_05_basic_lifecycle_kubernetes_cluster	`Failure`	6.61	test_kubernetes_clusters.py
test_06_delete_kubernetes_cluster	`Failure`	7.67	test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster	`Failure`	8.11	test_kubernetes_clusters.py
test_10_vpc_tier_kubernetes_cluster	`Error`	7.95	test_kubernetes_clusters.py
test_11_test_unmanaged_cluster_lifecycle	`Error`	3.82	test_kubernetes_clusters.py
test_12_test_deploy_cluster_different_offerings_per_node_type	`Failure`	9.78	test_kubernetes_clusters.py
test_05_list_volumes_isrecursive	`Failure`	0.06	test_list_volumes.py
test_07_list_volumes_listall	`Failure`	0.05	test_list_volumes.py

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

    }

-    protected void handleCertificateResponse(String certStr, DirectDownloadCertificateResponse response) {
+protected void handleCertificateResponse(String certStr, DirectDownloadCertificateResponse response) {


Nit: this line lost its indentation.

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

-            Certificate cert = CertificateHelper.buildCertificate(certStr);
-            if (cert instanceof X509CertImpl) {
-                X509CertImpl certificate = (X509CertImpl) cert;
+            java.security.cert.Certificate cert = CertificateHelper.buildCertificate(certStr);


Nit: fully-qualified java.security.cert.* used inline here instead of importing, especially odd since the old imports for this package were just removed above.

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.

…ation

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.

test: Add unit tests for 18 extracted UsageType helper methods

b99f888

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

Merge branch 'main' into fix-11635-refactor-usage

9838eb7

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

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

c6225bd

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

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

c407a0c

…unused imports

PrashantBhanage added 2 commits July 5, 2026 19:47

test: Fix populateIpAddressUsageResponseTest after Copilot refactor

6c3b597

Fix: Add missing EOF newline to ApiResponseHelperTest

a54428b

apache deleted a comment from blueorangutan Jul 13, 2026

fix: pre-commit end-of-file fixes

b082934

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

Damans227 reviewed Jul 21, 2026

View reviewed changes

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

e784377

…ation

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!

blueorangutan 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!

blueorangutan commented Jun 25, 2026

Uh oh!

PrashantBhanage commented Jun 26, 2026

Uh oh!

blueorangutan 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!

blueorangutan 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!

blueorangutan commented Jul 4, 2026

Uh oh!

PrashantBhanage commented Jul 4, 2026

Uh oh!

PrashantBhanage commented Jul 5, 2026

Uh oh!

PrashantBhanage commented Jul 6, 2026

Uh oh!

blueorangutan commented Jul 13, 2026

Uh oh!

blueorangutan commented Jul 13, 2026

Uh oh!

PrashantBhanage commented Jul 16, 2026

Uh oh!

blueorangutan commented Jul 20, 2026

Uh oh!

DaanHoogland commented Jul 20, 2026

Uh oh!

blueorangutan commented Jul 20, 2026

Uh oh!

blueorangutan commented Jul 21, 2026

Uh oh!

Damans227 left a comment

Choose a reason for hiding this comment

Uh oh!

Damans227 Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

Damans227 Jul 21, 2026

Choose a reason for hiding this comment

PrashantBhanage commented Jun 25, 2026 •

edited

Loading

codecov Bot commented Jun 25, 2026 •

edited

Loading

PrashantBhanage left a comment •

edited

Loading