feat(gax)!: stable paginator by dbolduc · Pull Request #1466 · googleapis/google-cloud-rust

dbolduc · 2025-03-11T15:57:31Z

Part of the work for #720

Makes the Paginator class part of gax. It is no longer gated by the unstable-stream feature. The Paginator picks up a to_stream() function which is gated by the unstable-stream feature. (Same goes for ItemPaginator).

We renamed the stream() function to paginator() on the paginated builders.

`unstable-stream` feature in client crates

All downstream crates still have an unstable-stream feature. I think we should get rid of them, and let applications opt in via the dependency on gax. If we want to do this, I will send another PR.

We would need to update some of our docs, because we could no longer one-line things like:

   cargo add google-cloud-secretmanager-v1 --features unstable-stream

On testing feature configurations...

We now testing gax with and without unstable-stream. (By removing both dev.dependencies on this feature).

codecov · 2025-03-11T16:08:08Z

Codecov Report

Attention: Patch coverage is 36.36364% with 14 lines in your changes missing coverage. Please review.

Project coverage is 95.18%. Comparing base (e5d7b72) to head (fee79b2).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
src/gax/src/paginator.rs	36.36%	14 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1466      +/-   ##
==========================================
- Coverage   95.95%   95.18%   -0.77%     
==========================================
  Files          38       38              
  Lines        1531     1538       +7     
==========================================
- Hits         1469     1464       -5     
- Misses         62       74      +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

coryan

I think you need a new integration test that does not enable unstable-stream and still runs the iterations.

coryan · 2025-03-11T16:08:29Z

src/gax/src/paginator.rs

+    /// Enable the `unstable-stream` feature to interact with a [`futures::stream::Stream`].
+    ///
+    /// [`futures::stream::Stream`]: https://docs.rs/futures/latest/futures/stream/trait.Stream.html
+    pub async fn next_stable(&mut self) -> Option<Result<T, E>> {


Can we arrange things so this function is called next()? If not, can we call it next_page()?

Calling this function next() seems to work.

generator/internal/rust/templates/crate/src/builders.rs.mustache

src/auth/integration-tests/Cargo.toml

dbolduc · 2025-03-12T15:54:55Z

I think you need a new integration test that does not enable unstable-stream and still runs the iterations.

I went with gating the unit test for the feature and testing the crates with all features or no features:

google-cloud-rust/.github/workflows/sdk.yaml

Lines 45 to 48 in 18ab348

    
           - name: Test with features disabled 
        
             run: cargo test --workspace --no-default-features 
        
           - name: Test with features enabled 
        
             run: cargo test --workspace --all-features

coryan

I think this works, but we need to decide if we want to depend on futures (which is at 0.3) in our implementation.

src/integration-tests/src/secret_manager/openapi.rs

src/integration-tests/src/secret_manager/protobuf.rs

generator/internal/rust/templates/crate/src/builders.rs.mustache

src/gax/Cargo.toml

src/gax/src/paginator.rs

dbolduc · 2025-03-13T15:52:16Z

but we need to decide if we want to depend on futures (which is at 0.3) in our implementation.

Ack. I did not make the dep on futures optional in this PR. I want to get something in first, then we can think about refactoring the implementation.

If you think it is cleaner to not use futures in the implementation, I can look into that.

I went with gating the unit test for the feature and testing the crates with all features or no features:

modification: I reverted the changes to sdk.yaml. But I did remove unstable-stream from being always on in our gax tests. (It was formerly enabled both by gax's dev.dependencies and via the echo-server.)

I have a meeting item to discuss the build cache anyway.

coryan

The code looks fine to me, but the test coverage results look weird. I worry that maybe something in the features setting is making us miss testing?

coryan · 2025-03-13T16:03:24Z

.github/workflows/sdk.yaml

      - run: cargo doc --workspace
        env:
          RUSTDOCFLAGS: "-D warnings"
-      - run: cargo doc --package google-cloud-gax


As I recall, cargo doc can fail to build if you have some features enabled vs. disabled (e.g. if you link an element that is not present because the feature is disabled). I believe we should keep this test.

I reverted all changes to .github/workflows/sdk.yaml, but that is not obvious from the 9000 commits associated with this PR.

coryan · 2025-03-13T16:37:23Z

src/gax/src/paginator.rs

+        let mut stream = client
+            .list_rpc_stream(TestRequest::default())
+            .items()
+            .to_stream();


codecov.io seems confused about the coverage for to_stream(). is this code running?

Hm, it should be exercised here:

google-cloud-rust/.github/workflows/sdk.yaml

Line 47 in 6682457

- run: cargo test --workspace

Locally:

$ cargo test --workspace | grep to_stream test paginator::tests::test_item_paginator_to_stream ... ok test paginator::tests::test_paginator_to_stream ... ok

FWIW, I believe the coverage build uses a different set of options:

google-cloud-rust/.github/workflows/sdk.yaml

Lines 69 to 74 in 6682457

- run: |

cargo tarpaulin --out xml \

--package google-cloud-auth \

--package google-cloud-gax \

--package google-cloud-lro \

--package google-cloud-wkt

oh, duh. I misread the yaml.

coryan · 2025-03-13T16:44:30Z

src/integration-tests/tests/pagination.rs

Should we have a test for to_stream() in this file?

Added. The crate depends on clients with the unstable-stream feature enabled. I will clean up the features/dependencies in a follow up PR.

coryan · 2025-03-13T16:47:36Z

Congrats on the code removal btw.

coryan reviewed Mar 11, 2025

View reviewed changes

dbolduc marked this pull request as ready for review March 11, 2025 19:23

dbolduc changed the title ~~feat(gax): stable paginator~~ feat(gax)!: stable paginator Mar 12, 2025

coryan reviewed Mar 12, 2025

View reviewed changes

src/gax/src/paginator.rs Outdated Show resolved Hide resolved

dbolduc added 9 commits March 13, 2025 12:10

feat(gax): stable paginator

1d1c35b

generator changes

e0b9372

generated changes

800a333

golden files

9013ac9

ci fixes

8fcf04b

address review comments

597c690

generated + golden changes

29e6805

rename variables; to_stream function.

9932557

revert all changes to sdk.yaml

82ca8d4

dbolduc force-pushed the pagination-gated-stream-dev branch from 6593c68 to 82ca8d4 Compare March 13, 2025 16:10

coryan approved these changes Mar 13, 2025

View reviewed changes

address review comments

fee79b2

coryan approved these changes Mar 13, 2025

View reviewed changes

dbolduc merged commit f8d2b7e into googleapis:main Mar 13, 2025
20 checks passed

dbolduc deleted the pagination-gated-stream-dev branch March 13, 2025 20:02

dbolduc mentioned this pull request Mar 25, 2025

cleanup(gax): rename paginator to stream #1611

Merged

	- run: \|
	cargo tarpaulin --out xml \
	--package google-cloud-auth \
	--package google-cloud-gax \
	--package google-cloud-lro \
	--package google-cloud-wkt

Conversation

dbolduc commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

unstable-stream feature in client crates

On testing feature configurations...

Uh oh!

codecov bot commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

coryan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dbolduc commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coryan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dbolduc commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coryan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbolduc Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coryan commented Mar 13, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dbolduc commented Mar 11, 2025 •

edited

Loading

`unstable-stream` feature in client crates

codecov bot commented Mar 11, 2025 •

edited

Loading

dbolduc commented Mar 12, 2025 •

edited

Loading

dbolduc commented Mar 13, 2025 •

edited

Loading

dbolduc Mar 13, 2025 •

edited

Loading