Skip to content

Fixing pagination on v3._ListSecurityGroupsRequest #1277

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
May 15, 2025
Merged

Fixing pagination on v3._ListSecurityGroupsRequest #1277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1446d37
issue-1242 fixing version of PaginatedRequest implemented by v3._List…
SaifuddinMerchant May 9, 2025
33fce00
issue-1242 running mvn spotless:apply to fix formatting for SecurityG…
SaifuddinMerchant May 14, 2025
563d381
issue-1242 Fixes issue that the subsequent listPage is not called bec…
SaifuddinMerchant May 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...t/src/main/java/org/cloudfoundry/client/v3/securitygroups/_ListSecurityGroupsRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

package org.cloudfoundry.client.v3.securitygroups;

import org.cloudfoundry.client.v2.PaginatedRequest;
import org.cloudfoundry.client.v3.PaginatedRequest;
import org.immutables.value.Value;
import org.cloudfoundry.client.v3.FilterParameter;
import org.cloudfoundry.Nullable;
Expand Down
28 changes: 28 additions & 0 deletions integration-test/src/test/java/org/cloudfoundry/client/v3/SecurityGroupsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import java.time.Duration;
import java.util.Arrays;
import java.util.Collections;
import org.cloudfoundry.AbstractIntegrationTest;
import org.cloudfoundry.client.CloudFoundryClient;
import org.cloudfoundry.client.v3.securitygroups.BindRunningSecurityGroupRequest;
Expand All @@ -34,6 +35,7 @@
import org.cloudfoundry.client.v3.securitygroups.UnbindRunningSecurityGroupRequest;
import org.cloudfoundry.client.v3.securitygroups.UnbindStagingSecurityGroupRequest;
import org.cloudfoundry.client.v3.securitygroups.UpdateSecurityGroupRequest;
import org.cloudfoundry.util.PaginationUtils;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
Expand Down Expand Up @@ -196,6 +198,32 @@ public void listStaging() {
.verify(Duration.ofMinutes(5));
}

@Test
public void listWithPagination() {
Copy link
Contributor

@Kehrlann Kehrlann May 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@Kehrlann Kehrlann May 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@SaifuddinMerchant SaifuddinMerchant May 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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
Contributor Author

Choose a reason for hiding this comment

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

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.

this.securityGroup
.flux()
.flatMap(
securityGroup ->
PaginationUtils.requestClientV3Resources(
page ->
cloudFoundryClient
.securityGroupsV3()
.list(
ListSecurityGroupsRequest.builder()
.page(page)
.perPage(1)
.names(
Collections
.singletonList(
securityGroup
.getName()))
.build())))
.as(StepVerifier::create)
.expectNextCount(1)
.expectComplete()
.verify(Duration.ofMinutes(5));
}

@Test
public void bindStagingSecurityGroup() {
Mono.zip(this.securityGroup, this.spaceId)
Expand Down