Refactor: Modularize ApiResponseHelper#createUsageResponse#13490
Refactor: Modularize ApiResponseHelper#createUsageResponse#13490PrashantBhanage wants to merge 11 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)
|
|
@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✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #13490 +/- ##
=============================================
- Coverage 18.94% 3.41% -15.54%
=============================================
Files 6192 487 -5705
Lines 556361 41867 -514494
Branches 67908 7912 -59996
=============================================
- Hits 105407 1429 -103978
+ Misses 439383 40238 -399145
+ Partials 11571 200 -11371
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 |
|
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 |
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. |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
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.
| 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())); |
|
[SF] Trillian test result (tid-16476)
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Applied all the Copilot suggestions, null checks for svcOffering, nullable Long values, and removed the unused variable. |
|
Fixed the build, replaced the internal X509CertImpl with standard Java API and removed the unused imports that Copilot introduced. |
|
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! |
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18553 |
|
It should be good now! |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18619 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-16594)
|
Damans227
left a comment
Damans227
left a comment
There was a problem hiding this comment.
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".
| return resourceDetails; | ||
| } | ||
|
|
||
| private UsageResourceDetails populateNetworkOfferingUsageResponse(Usage usageRecord, UsageRecordResponse usageRecResponse, boolean oldFormat, VMInstanceVO vmInstance) { |
There was a problem hiding this comment.
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.
|
@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. |
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18658 |
|
@blueorangutan test |
|
thanks @PrashantBhanage , one more, can you rebase the PR to the 4.22 LTS branch? |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
@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. |
ok, that would be an argument to go for this version and to abandon #13695 |
|
[SF] Trillian test result (tid-16605)
|
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.Additional Changes
server/pom.xmlto support JUnit 5 tests.ApiResponseHelperTestfrom JUnit 4 to JUnit 5 and added unit tests for the 18 extracted helper methods.sonar-check.ymlend-of-file formatting (pre-commit hook).java.security.certimports inhandleCertificateResponse.Types of changes
How Has This Been Tested?
mvn -pl server -Dtest=ApiResponseHelperTest test