Refactor: Modularize ApiResponseHelper#createUsageResponse#13490
Open
PrashantBhanage wants to merge 3 commits into
Open
Refactor: Modularize ApiResponseHelper#createUsageResponse#13490PrashantBhanage wants to merge 3 commits into
PrashantBhanage wants to merge 3 commits into
Conversation
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.
|
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)
|
Contributor
|
@blueorangutan package |
|
@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 Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13490 +/- ##
============================================
+ Coverage 18.94% 18.98% +0.03%
- Complexity 18366 18392 +26
============================================
Files 6192 6192
Lines 556361 556444 +83
Branches 67908 67907 -1
============================================
+ Hits 105407 105642 +235
+ Misses 439383 439166 -217
- Partials 11571 11636 +65
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18377 |
Author
|
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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18390 |
PrashantBhanage
commented
Jul 2, 2026
Contributor
what problem are you talking about, @PrashantBhanage ? I only see an integration test failure which seems unrelated. |
Author
Got it, thanks! I raised #13516 for the flaky test separately. The refactor itself should be good. |
Contributor
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
Contributor
There was a problem hiding this comment.
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
ApiResponseHelperTestto JUnit 5 + Mockito Jupiter and added focused tests for several usage-type helpers. - Added JUnit Jupiter / Vintage and Mockito Jupiter test dependencies to the
servermodule 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.
Comment on lines
+4187
to
+4190
| UsageResourceDetails resourceDetails = populateUsageTypeSpecificDetails(usageRecord, usageRecResponse, oldFormat, vmInstance, template); | ||
| if(resourceTagResponseMap != null && resourceTagResponseMap.get(resourceDetails.resourceId + ":" + resourceDetails.resourceType) != null) { | ||
| usageRecResponse.setTags(resourceTagResponseMap.get(resourceDetails.resourceId + ":" + resourceDetails.resourceType)); | ||
| } |
Comment on lines
+4282
to
+4286
| if(usageRecord.getCpuCores() != null) { | ||
| usageRecResponse.setCpuNumber(usageRecord.getCpuCores()); | ||
| } else if (svcOffering.getCpu() != null){ | ||
| usageRecResponse.setCpuNumber(svcOffering.getCpu().longValue()); | ||
| } |
Comment on lines
+4287
to
+4291
| if(usageRecord.getCpuSpeed() != null) { | ||
| usageRecResponse.setCpuSpeed(usageRecord.getCpuSpeed()); | ||
| } else if(svcOffering.getSpeed() != null){ | ||
| usageRecResponse.setCpuSpeed(svcOffering.getSpeed().longValue()); | ||
| } |
Comment on lines
+4292
to
+4296
| if(usageRecord.getMemory() != null) { | ||
| usageRecResponse.setMemory(usageRecord.getMemory()); | ||
| } else if(svcOffering.getRamSize() != null) { | ||
| usageRecResponse.setMemory(svcOffering.getRamSize().longValue()); | ||
| } |
Comment on lines
+4323
to
+4326
| Long networkId = ip.getAssociatedWithNetworkId(); | ||
| if (networkId == null) { | ||
| networkId = ip.getSourceNetworkId(); | ||
| } |
Comment on lines
4504
to
+4506
| 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())); |
Comment on lines
4527
to
+4529
| 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())); |
Comment on lines
+4654
to
+4655
| builder.append(" with size ").append(toHumanReadableSize(usageRecord.getSize())); | ||
| builder.append(" and with virtual size ").append(toHumanReadableSize(usageRecord.getVirtualSize())); |
Comment on lines
+4718
to
+4721
| if (volume != null) { | ||
| builder.append(" for ").append(volume.getName()).append(" (").append(volume.getUuid()).append(") ") | ||
| .append("with size ").append(toHumanReadableSize(usageRecord.getSize())); | ||
| } |
Comment on lines
+4746
to
+4749
| if (vmInstance != null) { | ||
| builder.append(" for VM ").append(vmInstance.getHostName()).append(" (").append(vmInstance.getUuid()).append(") ") | ||
| .append("with size ").append(toHumanReadableSize(usageRecord.getVirtualSize())); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #11635
Description
This PR addresses the technical debt in
ApiResponseHelper#createUsageResponse(Usage)by modularizing the 530-line method.populateUsageTypeSpecificDetails.UsageTypeto improve maintainability and readability.UsageResourceDetailscontainer class to safely manage and returnresourceIdandresourceTypestate for tag lookups.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
N/A (Pure backend refactoring)
How Has This Been Tested?
Ran a successful clean compile locally to verify no structural or syntax breaks:
mvn -pl server -DskipTests clean compilePassed all local Checkstyle validations.
How did you try to break this feature and the system with this change?
This is a pure structural refactor. Carefully verified that no business logic, string formatting, or calculations were altered. I am relying on the existing automated CI test suites to ensure data integrity and backward compatibility remain strictly intact.