Add with_callback that take a Fn taking request and returning responses

[izissise]

#458

This compile but I haven't successfully used it yet.

It takes a Fn but should probably take a FnMut or Stream for more convenience, but I couldn't make it work with RefCell or RWLock.

Any help is appreciated, especially if you known your way around Send + Sync + Async Stream :)

[fussybeaver]

Thanks for putting this up, it should be useful to check if this satisfies some of the many use-cases previously posted. I agree an FnMut would be more desirable... I'm also wondering about the additional sync_wrapper dependency, we may need to fold the implementation behind a feature flag if that's needed.

Appreciate it if someone in the community has any further insight.

[izissise]

I have a working version and an example use case which mod the request. I'll update this branch soon

[izissise]

Edit: It works and the doctest passes. Should be good to merge

[fussybeaver]

This looks great, I'm going to ask the others who wanted this to see if they need anything else...

[DDtKey]

I really appreciate the work done here! ❤️

I left only one subjective suggestion, if you find it valuable

But in terms of functionality, it seems to cover the needs. Thank you!

[DDtKey]

Sorry, looks like my suggestion got missed by rustfmt

[izissise]

My bad I didn't check before end, also I'm on my work laptop without my github ssh key so I won't be able to fix it soon.

Have you tested the doc example? Does it compile?

I'm wondering because I tried adding the 'static lifetime but the example in the doc wouldn't compile 2c5c765

[izissise]

Nevermind it just passed the test :) Thanks for the help :)

[izissise]

Also I wasn't careful and I commited the generated file

[izissise]

I'll be able to fix my change in 5hours or so, sorry for the inconveniance

[izissise]

I cleaned the MR and it passes the doc test now :)

[izissise]

One last thing, is that using Request<BodyType> forces the users to depends on hyper directly.
There is multiple solution if we think that is a problem:

• re-export hyper from bollard
• Hide the type (BollardRequest)
• Something else?

[fussybeaver]

One last thing, is that using Request<BodyType> forces the users to depends on hyper directly. There is multiple solution if we think that is a problem:

* re-export hyper from bollard

* Hide the type (BollardRequest)

* Something else?

I think hyper::Request and hyper::Response are just aliases of http::Request and http::Response, so we can use those imports instead as the http crate is a more general purpose crate.

[izissise]

Yes http crate is better. my concern is adding an extra dependency in users Cargo.toml.
For example if I want to write a docker client using bollard, I just want to have bollard dependency. Without any changes I would at least need to know about http and bump version in the Cargo.toml when needed.

Re exporting or hiding the type behind an alias solve this problem, the type to define the request will come from bollard crate

[fussybeaver]

Though that only applies if the user is using a custom request callback, at which point the user may wish to specify her own http library version...

I'm reading about some conflicting best practices about this when I read about it.. I'm wondering if someone more knowledgeable is available to briefly comment @jonhoo ? 🙏

[DDtKey]

I believe it's a good practice to re-export types (or even whole crate) if it's supposed to be a public interface for the library. In this case Request is part of public api for bollard.

Though that only applies if the user is using a custom request callback

I may want to have closure with explicit types definition (e.g: |req: http::Request|, or even write a function), so it's applicable not only for custom impl IMHO.

Another point here - versions should be consistent between client code and bollard. Let's say (unlikely), but at the end of the day http v2 is released, so users may want to upgrade but will have to use http1 as well (in their deps) until bollard is updated and etc.

For example, that's how hyper does this: https://github.com/hyperium/hyper/blob/67a4a498d8bbdce4e604bc578da4693fb048f83d/src/lib.rs#L96

[DDtKey]

Suggested change

	/// Callback transport trait
	pub trait TransportTrait: Send + Sync {
	/// Callback transport trait
	pub trait TransportTrait: Send + Sync {

a nit: I personally find such (pre/post)-fixes excessive, they don't have meaningful part - the type itself defines itself as a trait.
So it's subjective opinion, but CustomTransport or Transport should work well. Also callback in documentation looks a bit unrelated, it's just an interface to pass your custom transport implementation.
It's just implemented for callback too

[izissise]

I'll make the changes

[fussybeaver]

I may want to have closure with explicit types definition (e.g: |req: http::Request|, or even write a function), so it's applicable not only for custom impl IMHO.

It's not applicable for general usage of the library, outside of the use cases described here and associated tickets.

Another point here - versions should be consistent between client code and bollard. Let's say (unlikely), but at the end of the day http v2 is released, so users may want to upgrade but will have to use http1 as well (in their deps) until bollard is updated and etc.

Maybe I don't understand exactly what you mean, but if http v2 is released, by re-exporting the http crate, the user is forced to use http v1 for the callback handler and he can't actually use http v2 inside bollard.

[DDtKey]

Maybe I don't understand exactly what you mean, but if http v2 is released, by re-exporting the http crate, the user is forced to use http v1 for the callback handler and he can't actually use http v2 inside bollard.

No, what I mean is: bollard requires users to use particular version of dependency for this feature while their http dependency might be independent.

For example, I have some code in my application which depends on http crate, and in addition to that I have custom implementation of transport for bollard.
Let's say http2 is released and I've updated my code, but for bollard I'll need to have both http2 and http1 as part of my dependencies just because of custom implementation of the trait which requires me to use http1.

But when types are re-exported, it's responsibility of bollard to update the version, not mine as a client. It's ofc still uses http1 under the hood and you will see this within Cargo.lock, but I don't have to manage this, since it's part of bollard.

And what can be useful here for maintenance is that it's more obvious whether a crate update should be considered a breaking change if types are re-exported. Otherwise, it's easier to update a dependency without realizing that some users depend on it.

It's not applicable for general usage of the library, outside of the use cases described here and associated tickets.

It's still part of public api, it can be type alias or just re-export through some of modules - but it is, even for some particular use-case.

[fussybeaver]

OK, let's re-export these public types. I'm not entirely convinced ... but happy to defer to someone else's wisdom here and do some more reading.

[izissise]

LGTM

[fussybeaver]

Thanks for all the great work here!