Refactor: Modularize ApiResponseHelper#createUsageResponse - #13490
PrashantBhanage wants to merge 12 commits into
Conversation
|
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13490 +/- ##
============================================
+ Coverage 19.73% 19.82% +0.08%
- Complexity 19955 20018 +63
============================================
Files 6371 6371
Lines 575784 575963 +179
Branches 70478 70499 +21
============================================
+ Hits 113632 114176 +544
+ Misses 449805 449290 -515
- Partials 12347 12497 +150
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:
|
|
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. |
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 |
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())); |
|
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! |
|
It should be good now! |
|
@blueorangutan test |
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 |
|
@blueorangutan test |
|
thanks @PrashantBhanage , one more, can you rebase the PR to the 4.22 LTS branch? |
|
@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. |
da39c45 to
d731c70
Compare
|
@blueorangutan shutup |
|
@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 ✖️ debian ✖️ suse15. SL-JID 19031 |
|
@PrashantBhanage can you look at the build errors, please? |
|
@DaanHoogland Fixed the build issues caused by the missing response helper methods. Added them back and verified the changes with 3,723 server tests passing. |
|
@PrashantBhanage , I am a bit worried. I see sonarcube workflow changes in the same PR as production code changes. Can you create a separate PR for those? |
@DaanHoogland I split the Sonar workflow changes out of this PR as requested. The workflow fix is now in separate PR #14299: I also removed |
|
@Damans227 , are all your concerns met? |
|
@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. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Multiple established API response fields are dropped, and the usage refactor has an undocumented behavior change with incomplete public-path coverage.
Review effort: Balanced
Findings: 3
Open (15)
Resolved since last review (12)
| // populate account | ||
| try { | ||
| Account account = ApiDBUtils.findAccountById(keyPair.getAccountId()); | ||
| if (account != null && account.getType() != Account.Type.PROJECT) { | ||
| response.setAccountName(account.getAccountName()); |
| domainResponse.setHasChild(true); | ||
| } | ||
| populateDomainTags(domain.getUuid(), domainResponse); | ||
| domainResponse.setObjectName("domain"); |
| Network guestNtwk = ApiDBUtils.findNetworkById(fwRule.getNetworkId()); | ||
| response.setNetworkId(guestNtwk.getUuid()); | ||
| response.setNetworkName(guestNtwk.getName()); | ||
|
|
| } | ||
| } else if (network.getTrafficType() == TrafficType.Storage) { | ||
| vmResponse.setStorageIp(singleNicProfile.getIPv4Address()); | ||
| } |
| network.getVpcId() == null && network.getGuestType() == Network.GuestType.Isolated) { | ||
| response.setKeepMacAddressOnPublicNic(network.getKeepMacAddressOnPublicNic()); | ||
| } | ||
|
|
| } else { | ||
| usageRecResponse.setNetworkId(network.getUuid()); | ||
| resourceDetails.resourceId = network.getId(); | ||
| } | ||
| usageRecResponse.setResourceName(network.getName()); |
| response.setNicId(nic.getUuid()); | ||
| response.setNwId(network.getUuid()); | ||
| response.setDescription(result.getDescription()); | ||
|
|
| NicSecondaryIpResponse ipRes = new NicSecondaryIpResponse(); | ||
| ipRes.setId(ip.getUuid()); | ||
| ipRes.setDescription(ip.getDescription()); | ||
|
|
| @@ -4903,7 +5029,6 @@ public NicResponse createNicResponse(Nic result) { | |||
| response.setVpcName(vpc.getName()); | |||
| } | |||
|
|
|||
| private Object invokeUsageDetailsHelper(String methodName, Class<?>[] parameterTypes, Object... args) throws Exception { | ||
| Method method = ApiResponseHelper.class.getDeclaredMethod(methodName, parameterTypes); | ||
| method.setAccessible(true); | ||
| return method.invoke(helper, args); |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19400 |
|
@PrashantBhanage can you argue/code away those co-pilot comments? |
@DaanHoogland two of my three points are fixed, the last one was only about style and can stay. the bigger problem is the new copilot comments look right, this branch removes newer code from main like the network name on port forwarding rules. it needs a fresh rebase before testing. |
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