Skip to content
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

Fix dropshot-pagination _stream client codegen #756

Merged
merged 3 commits into from
Apr 3, 2024
Merged

Fix dropshot-pagination _stream client codegen #756

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
38e7874
Fix dropshot-pagination `_stream` client codegen
jgallagher Apr 1, 2024
faf3f72
give builder_struct the same limit treatment
jgallagher Apr 2, 2024
79c6c9d
Merge branch 'main' into fix-paginated-stream
jgallagher Apr 2, 2024
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
8 changes: 6 additions & 2 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions progenitor-impl/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,13 @@ unicode-ident = "1.0.12"
[dev-dependencies]
dropshot = { git = "https://github.com/oxidecomputer/dropshot", default-features = false }
expectorate = "1.1"
futures = "0.3.30"
http = "0.2.9"
hyper = "0.14.27"
reqwest = "0.11.27"
rustfmt-wrapper = "0.2.1"
serde_yaml = "0.9"
serde_json = "1.0.113"
tokio = { version = "1.37.0", features = ["rt", "net"] }

progenitor-client.path = "../progenitor-client"
39 changes: 12 additions & 27 deletions progenitor-impl/src/method.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -710,9 +710,12 @@ impl Generator {
let step_params = method.params.iter().map(|param| {
if param.api_name.as_str() == "page_token" {
quote! { state.as_deref() }
} else if let OperationParameterKind::Query(_) = param.kind {
// Query parameters are None; having page_token as Some(_)
// is mutually exclusive with other query parameters.
} else if param.api_name.as_str() != "limit"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the change I'm least sure about, but I think it's fine? We should only be here if we have a non-None dropshot_pagination, and dropshot_pagination_data explicitly checks for both "page_token" and "limit" as query parameters.

Copy link
Collaborator

@ahl ahl Apr 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, this is the part I'm least sure about as well. My intention (unexpressed in any comments) is that the consumer might specify the size of the first page to optimize for latency and then rely on the server to give us chunky, throughput oriented pages subsequently. Imagine, for example, a CLI where we want to start printing output quickly.

Alternatively, would it make sense to have parameters for the first page and other pages?

In either case, if we don't include this change we might consider updating the function parameters docs which, currently, come from dropshot via the OpenAPI doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively, would it make sense to have parameters for the first page and other pages?

I thought about this. It doesn't seem unreasonable to me, although I suspect most clients ought to specify Some(small_value) for the first page and None (server choice) for the other pages, right? I wonder if it would help to put this in a new type specifically for these parameters for clarity? Totally spitballing, something like:

pub struct StreamPaginationLimits {
    // or just omit this one and recommend clients use `.take(_)`
    pub total: Option<NonZeroU32>,
    pub first_page: Option<NonZeroU32>,
    pub remaining_pages: Option<NonZeroU32>,
}

with helper methods for "let the server pick all the limits, I don't care" and "I just want a small first page, let the server pick for remaining pages".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another thought: I'm not sure the stream interface actually does the "small first page, larger subsequent pages" thing you want, although I guess it depends on the client. I think if you're using

while let Some(item) = stream.next() { .. }

with a first page limit of 10 and a subsequent page limit of 100, you'll incur the latency for the GET (10) on the first nex(), the next 9 will return immediately, then you'll incur the latency for GET (100) on the 11th call to next(), right?

&& matches!(param.kind, OperationParameterKind::Query(_))
{
// Query parameters (other than "page_token" and "limit")
// are None; having page_token as Some(_) is mutually
// exclusive with other query parameters.
quote! { None }
} else {
// Non-query parameters are passed in; this is necessary
Expand Down Expand Up @@ -744,15 +747,6 @@ impl Generator {
use futures::TryFutureExt;
use futures::TryStreamExt;

// Grab the limit. This is intended to be agnostic to the
// specific type for the limit input which is why it's a
// bit convoluted.
let final_stream_limit = limit
.clone()
.and_then(|x| std::num::NonZeroUsize::try_from(x).ok())
.map(std::num::NonZeroUsize::get)
.unwrap_or(usize::MAX);

// Execute the operation with the basic parameters
// (omitting page_token) to get the first page.
self.#operation_id( #(#first_params,)* )
Expand Down Expand Up @@ -800,7 +794,6 @@ impl Generator {
first.chain(rest)
})
.try_flatten_stream()
.take(final_stream_limit)
.boxed()
}
}
Expand Down Expand Up @@ -1767,7 +1760,12 @@ impl Generator {
self.uses_futures = true;

let step_params = method.params.iter().filter_map(|param| {
if let OperationParameterKind::Query(_) = param.kind {
if param.api_name.as_str() != "limit"
&& matches!(param.kind, OperationParameterKind::Query(_))
{
// Query parameters (other than "limit") are None; having
// page_token as Some(_), as we will during the loop below,
// is mutually exclusive with other query parameters.
let name = format_ident!("{}", param.name);
Some(quote! {
#name: Ok(None)
Expand Down Expand Up @@ -1799,18 +1797,6 @@ impl Generator {
use futures::TryFutureExt;
use futures::TryStreamExt;

// Grab the limit. This is intended to be agnostic to the
// specific type for the limit input which is why it's a
// bit convoluted.
let limit = self
.limit
.clone()
.ok()
.flatten()
.and_then(|x| std::num::NonZeroUsize::try_from(x).ok())
.map(std::num::NonZeroUsize::get)
.unwrap_or(usize::MAX);

// This is the builder template we'll use for iterative
// steps past the first; it has all query params set to
// None (the step will fill in page_token).
Expand Down Expand Up @@ -1865,7 +1851,6 @@ impl Generator {
first.chain(rest)
})
.try_flatten_stream()
.take(limit)
.boxed()
}
}
Expand Down
Loading
Loading