-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fix NSX SDK list handling to fetch all paginated results #12241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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)
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12241 +/- ##
============================================
+ Coverage 3.58% 17.46% +13.88%
- Complexity 0 15525 +15525
============================================
Files 445 5914 +5469
Lines 37552 529493 +491941
Branches 6914 64685 +57771
============================================
+ Hits 1347 92488 +91141
- Misses 36039 426584 +390545
- Partials 166 10421 +10255
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes NSX SDK list operations to properly handle pagination by following cursor chains until all pages are fetched. Previously, only the first page of results was retrieved, which could lead to incomplete data when the result set exceeded a single page.
Key Changes:
- Introduced a new
PagedFetcherutility class that implements cursor-based pagination logic - Updated multiple list operations in
NsxApiClientto usePagedFetcherfor complete result retrieval - Added comprehensive unit tests for the pagination functionality
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
PagedFetcher.java |
New utility class implementing cursor-based pagination with a fluent builder pattern for fetching all pages of results |
PagedFetcherTest.java |
Comprehensive unit tests covering single-page, multi-page, and edge cases for the pagination logic |
NsxApiClient.java |
Updated list operations (NAT rules, transport zones, segment ports, LB monitors, groups, services, etc.) to use PagedFetcher; refactored methods to return Optional for cleaner null handling; improved logging consistency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxApiClient.java
Outdated
Show resolved
Hide resolved
plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxApiClient.java
Outdated
Show resolved
Hide resolved
plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/service/PagedFetcherTest.java
Outdated
Show resolved
Hide resolved
plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/service/PagedFetcherTest.java
Outdated
Show resolved
Hide resolved
… and non-empty `cursor` field when more pages are available. The previous implementation only fetched the first page and ignored pagination. This change updates the list retrieval flow to: - follow the `cursor` chain until no further pages exist - accumulate items from all pages - return a single merged result to the caller This ensures that list operations return the complete dataset rather than just the first page.
de4cf14 to
070b956
Compare
Pearl1594
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code lgtm.
|
@blueorangutan package |
|
@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
This fix is currently undergoing testing. When the test results are ready, I will update the description accordingly. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16006 |
Fix for NSX plugin.
Description
NSX SDK list operations are pageable: the API returns a non-null and non-empty
cursorfield when more pages are available. The previous implementation only fetched the first page and ignored pagination.This change updates the list retrieval flow to:
cursorchain until no further pages existThis ensures that list operations return the complete dataset rather than just the first page.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?