Pager rename and refactor

[analogrelay]

In working on pageable APIs, I found that we needed to return Response<T> values instead of just T values. Rather than explode the generic type (Result<Pager<Response<T>>>) it felt reasonable to have Pager<T> yield a stream of Result<Response<T>> itself. In addition, Pager<T> depends on T implementing a trait, Continuable, to extract the continuation value. If we want to return Response<T> (so that users can read response headers for each page), that gets very difficult and likely requires newtypes since both Response<T> and Continuable would be foreign types to service client crates and you can't implement a foreign trait on a foreign type.

So, here are a few refactors to Pageable that I think will make it easier to use:

1. Renamed to Pager, to align with the guidelines
2. The callback expects a Result<(Response<T>, Option<C>)> (where C is any Sendable owned value). The expectation here is that the callback will return Ok((response, Some(continuation))) to indicate a continuation and Ok((response, None)) to indicate the end of the pagination. This avoids the need for the Continuable trait entirely.
3. The Pager yields Result<Response<T>> values. This is really just shorthand. If we don't want to limit ourselves to HTTP here, Pager<T> could become Pager<Response<T>> without any issues, except that writing the type gets more verbose.

There's a full example in the doc comments to see how you would call this. Also azure_data_cosmos::clients::ContainerClient::query_items has been rewritten to use it.
 
I tried to see if we could get rid of the boxing by turning Pager into a trait and returning a gnarly (but largely inferred) generic type. That didn't work out, but we had boxing here before anyway and are network bound so I'm not too worried about it.

In addition, I was able to remove the declare! macro by using conditional compilation to include/exclude the Send bound. It makes for a slightly gnarly function definition (where clauses and individual trait bounds cannot be conditionally compiled, but <T: Bound> syntax can). I also removed the inner module trick since it doesn't seem like I'm getting the dead-code error on project_ref (which makes sense since it's private to the struct, so I'm not sure why it was happening at all)

[heaths]

Note: should see if this would close #1668

[analogrelay]

So, I believe #1668 is tied to #1796. Requiring that Pager<T> be Send + Sync also means requiring that the futures it captures be Send + Sync, which requires that any futures they capture and await be Send + Sync. Because of #1796, our trait async fns don't have clear bounds for that. At least that how I think things are playing out when I try to add Send + Sync bounds.

[heaths]

Renamed to Pager, to align with the guidelines

Cool. That's what most other languages use anyway, I believe. I know Go and JS do off hand. The "pageable" was certainly inspired by my time on the Azure SDK for .NET team.

The callback expects a Result<(Response, Option)> (where C is any Sendable owned value). The expectation here is that the callback will return Ok((response, Some(continuation))) to indicate a continuation and Ok((response, None)) to indicate the end of the pagination. This avoids the need for the Continuable trait entirely.

Could we use an opaque type if public instead? It's open to change if needed - why the REST API board doesn't allow top-level JSON array responses - and more obvious what the fields are.

The Pager yields Result<Response> values. This is really just shorthand. If we don't want to limit ourselves to HTTP here, Pager could become Pager<Response> without any issues, except that writing the type gets more verbose.

Our paging is already pretty HTTP-specific across most - if not all - languages, so I think this is fine.

[heaths]

Overall it looks great and the thoughts and design here are welcome! As noted, I really just have one outstanding question. We could discuss over Teams if it'd be easier or lengthy, or whatever.

[heaths]

Renamed to Pager, to align with the guidelines

Cool. That's what most other languages use anyway, I believe. I know Go and JS do off hand. The "pageable" was certainly inspired by my time on the Azure SDK for .NET team.

Actually, I remembered this morning they use Poller, but I think since we're doing the polling internally, Pager makes more sense. It's doing what a pager does as opposed to making the consumer poll the poller in a loop, which isn't idiomatic. I think if range-over-funcs existed earlier, the Go SDK would've been using that.

[analogrelay]

Actually, I remembered this morning they use Poller, but I think since we're doing the polling internally, Pager makes more sense.

I think Poller might be something else. The Rust guidelines refer to Pager and Poller separately. The Go SDK certainly has a Pager type

[heaths]

You're right. I'm getting my wires cross. Poller is for LROs. Too many balls in the air right now. 🤹

[heaths]

Just a nit about naming, but otherwise LG(reat)TM!

[analogrelay]

Just need a code review from some Cosmos owners to merge now, since part of this PR updates the Cosmos client to use the refactored Pager (ping @FabianMeiswinkel @Pilchie @kirankumarkolli @kundadebdatta @tvaron3 , let me know if you have any questions! Happy to go over anything)

[Pilchie]

I looked at it before, and it looked fine to me, I just didn't want to sign-off before the broader discussion happened.

[RickWinter]

[heart] Rick Winter reacted to your message:

…

________________________________ From: Ashley Stanton-Nurse ***@***.***> Sent: Thursday, October 10, 2024 4:36:40 PM To: Azure/azure-sdk-for-rust ***@***.***> Cc: Rick Winter ***@***.***>; Review requested ***@***.***> Subject: Re: [Azure/azure-sdk-for-rust] Pager rename and refactor (PR #1836) Merged #1836<#1836> into feature/track2. — Reply to this email directly, view it on GitHub<#1836 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABBZUAIRSEYEP77WJZKGW23Z22UJRAVCNFSM6AAAAABPQP3MVOVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJUGU4TINBRGEYDANA>. You are receiving this because your review was requested.Message ID: ***@***.***>