← 返回首页
Fixing pagination on v3._ListSecurityGroupsRequest by SaifuddinMerchant · Pull Request #1277 · cloudfoundry/cf-java-client · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Include my email address so I can be contacted

Saved searches

Use saved searches to filter your results more quickly

Appearance settings
Resetting focus

Fixing pagination on v3._ListSecurityGroupsRequest#1277

Merged
Kehrlann merged 3 commits into
cloudfoundry:mainfrom
SaifuddinMerchant:feature/issue-1242-bug-fix-v3-ListSecurityGroupsRequest-extends-wrong-pagination-group-fix-author-email
May 15, 2025
Merged

Fixing pagination on v3._ListSecurityGroupsRequest#1277
Kehrlann merged 3 commits into
cloudfoundry:mainfrom
SaifuddinMerchant:feature/issue-1242-bug-fix-v3-ListSecurityGroupsRequest-extends-wrong-pagination-group-fix-author-email

Conversation

SaifuddinMerchant commented May 9, 2025
edited
Loading

Copy link
Copy Markdown
Contributor

v3._ListSecurityGroupsRequest extends the v2.PaginatedRequest which is incorrect. It needs to extend the v3.PaginatedRequest. The code fixes the incorrect package referenced in v3._ListSecurityGroupsRequest and adds a integration test case

Issue Link: #1242

…SecurityGroupsRequest and adding an integration test case
SaifuddinMerchant changed the title issue-1242 fixing version of PaginatedRequest implemented by v3._List… issue-1242 fixing version of PaginatedRequest implemented by v3._ListSecurityGroupsRequest and adding an integration test case May 9, 2025
Kehrlann self-requested a review May 14, 2025 06:09
Kehrlann previously approved these changes May 14, 2025
Kehrlann self-requested a review May 14, 2025 15:01
}

@Test
public void listWithPagination() {

Kehrlann May 14, 2025
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

I am unsure whether this test really showcases #1242 .

When I revert the change to have a v2 paginated request, and change the test to use .resultsPerPage(1), it still passes on against CC API 3.191.0.

With .resultsPerPage(1), does it fail on your CF deployment?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

The failure I've seen is - results-per-page is a invalid parameter for V3 API.
This matches what I expect given the specs
https://v3-apidocs.cloudfoundry.org/version/3.191.0/index.html#list-security-groups

org.cloudfoundry.client.v3.ClientV3Exception: CF-UnprocessableEntity(10008): Unknown query parameter(s): 'results-per-page'. Valid parameters are: 'page', 'per_page', 'order_by', 'created_ats', 'updated_ats', 'guids', 'names', 'running_space_guids', 'staging_space_guids', 'globally_enabled_running', 'globally_enabled_staging'
at org.cloudfoundry.reactor.util.ErrorPayloadMappers.lambda$null$2(ErrorPayloadMappers.java:62)

I haven't been able to run the integration test stand alone, but I do get the issue above using resultsPerPage(x) in my application

Kehrlann May 14, 2025
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Got it.

The test is incorrect, and is not testing what's intended, because when you do

this.securityGroup .map(...)

This returns a Mono<Flux<SecurityGroupResource>>. By subscribing to this, through .verify(...), only the this.securityGroup request is ever called. The subsequent listPage is not called because nothing subscribes to the Flux it returns.

Instead, you should do:

this.securityGroup .flux() .flatMap(/* ... */) .as(StepVerifier::create) // ...

This gives you a Flux<SecurityGroupResource>, and all calls are made. By doing that, I'm getting the error with the v2 pagination:

[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.835 s <<< FAILURE! -- in org.cloudfoundry.client.v3.SecurityGroupsTest [ERROR] org.cloudfoundry.client.v3.SecurityGroupsTest.listWithPagination -- Time elapsed: 0.830 s <<< FAILURE! java.lang.AssertionError: expectation "expectNextCount(1)" failed (expected: count = 1; actual: counted = 0; signal: onError(org.cloudfoundry.client.v3.ClientV3Exception: CF-UnprocessableEntity(10008): Unknown query parameter(s): 'results-per-page'. Valid parameters are: 'page', 'per_page', 'order_by', 'created_ats', 'updated_ats', 'guids', 'names', 'running_space_guids', 'staging_space_guids', 'globally_enabled_running', 'globally_enabled_staging'))

Could you please update the test?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Greatly Appreciate the feedback and help, I will make the change

SaifuddinMerchant May 14, 2025
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

I'm also going to try and see if I can run the actual integration test for this method on my installation, so I might take some time to do that update

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

I have fixed the test based on your feedback. No change from what you suggested. I can't run the integration test module but I was able to run just the specific test case with some modifications from my own module.

P.S I had based my test case on public void list() that I think suffers from a similar issue aka the listSecurityGroup is never called.

Kehrlann self-requested a review May 14, 2025 15:06
…ause nothing subscribes to the Flux it returns.

Kehrlann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Thank you for your contribution!

I'll take another look at the list() tests separately, thanks for flagging them.

Kehrlann changed the title issue-1242 fixing version of PaginatedRequest implemented by v3._ListSecurityGroupsRequest and adding an integration test case Fixing pagination on v3._ListSecurityGroupsRequest May 15, 2025
Kehrlann merged commit cc7cd88 into cloudfoundry:main May 15, 2025

Copy link
Copy Markdown
Contributor

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Footer

© 2026 GitHub, Inc.