Add support for custom domain name resolvers

[AquilaIrreale]

We needed this feature for a commercial product, maybe it can be of use to someone else too

Cheers
-- Simone

[spietika]

Thanks for sending the contribution!

Would you be able to provide an example how you are creating the client after these changes? I ran the tests, but most of them fail on missing type annotation for the client. When the type is annotated like this

let client: restson::blocking::RestClient<dns::GaiResolver> = RestClient::new_blocking("https://httpbin.org").unwrap();

There is still an error the trait Default is not implemented for GaiResolver.

I will look at this more closely later when I have more time, and also try to see if the explicit type annotation could be avoided altogether. This would keep the interface clean in the basic case where custom resolver is not needed.

If you can provide some further feedback it is much appreciated.

[AquilaIrreale]

Ok, the Default trait issue should now be resolved (through use of a zero-cost abstraction in the form of a newtype around GaiResolver to provide for the Default trait, we have no idea why it isn't already implemented by hyper, as constructing a GaiResolver is basically a no-op).

About the explicit type annotations, we made the error of assuming that rust would use the default type provided in the structs' definitions when none is specified by the user, but apparently it does not... for reasons that now escape me.

The latest commits contain a proposed solution: the new and new_blocking methods now only exist for RestClient<GaiResolver>. This way old and common-usecase users can keep building the most common type of client exactly like they did before:

let rest_client = RestClient::new_blocking("https://example.org").unwrap();

Users who need a special resolver can use the builder, either setting an appropriate hyper client instance explicitely

let mut http_connector = hyper::client::connect::HttpConnector::new_with_resolver(MyResolver::new());
http_connector.enforce_http(false);
let mut https_connector = hyper_tls::HttpsConnector::from((http_connector, tls_connector.into()));
https_connector.https_only(true);
let https_client = hyper::client::Client::builder()
    .build(https_connector);
let mut rest_client = RestClient::builder()
    .with_client(https_client)
    .build("https://example.org")
    .unwrap();

or by specifying the resolver type without setting an instance, in which case it will be default-constructed:

let rest_client: RestClient<MyResolver> = RestClient::builder()
    .build("https://example.org")
    .unwrap();

This should be 99% backwards compatible, bar cases in which people have over-annotated variable types, because RestClient now has a type parameter. Still I think it is easy and obvious enough to fix in client code that it should not be too big of an issue.

Let me know what you think of it
Cheers 👋

[spietika]

I do like the approach taken here, and it works almost perfectly. As you mentioned, new and new_blocking now work as expected without additional type annotations. However, when using the Builder with defaults the type is still needed.

Previously this was possible

let client = RestClient::builder()
    .timeout(Duration::from_secs(10))
    .build("https://httpbin.org")
    .unwrap();

which would now have to be changed either to annotate the type

let client = RestClient::<GaiResolver>::builder()

or use this hacky syntax to make the compiler actually infer the default type

let client = <RestClient>::builder()

The first approach forces the user to over-annotate, and the second one is quite unintuitive. Sidenote about why the default type does not work here: RestClient::builder() is basically same as RestClient::<_>::builder() and the compiler does not infer the default type for _ here. More discussion about this here .

How about also moving builder() inside impl RestClient<GaiResolver> with new? Then if the type needs to be changed the Builder can be created explicitly:

let builder = Builder::<MyResolver>::default();
let client = builder
    .timeout(Duration::from_secs(10))
    .build("https://httpbin.org")
    .unwrap();

[spietika]

I actually just noticed a regression. For some reason HTTPS requests with blocking clients do not work (cargo test get):

thread 'basic_get_https' panicked at 'called `Result::unwrap()` on an `Err` value: HyperError(hyper::Error(Connect, "invalid URL, scheme is not http"))', tests/get.rs:65:53

Seems to be same behaviour whether the blocking client was created with new_blocking or with Builder. HTTP requests seem to work, and also the async client works with both HTTP and HTTPS.

The same tests pass on master, but I cannot immediately spot what is causing the issue with these changes.

[AquilaIrreale]

How about also moving builder() inside impl RestClient<GaiResolver> with new? Then if the type needs to be changed the Builder can be created explicitly:

Yeah, of course... use is starting to get a bit obscure in the non-default case, but if backwards-compatibility is the main goal, I'd say it's still usable. Maybe the API can be cleaned up in a possible future (breaking) release, if the project ever ends up having one eventually.

Sidenote about why the default type does not work here: RestClient::builder() is basically same as RestClient::<_>::builder() and the compiler does not infer the default type for _ here. More discussion about this here .

Thank you for the link, it's been quite enlightening! (Although a bit disappointing for all the inconsistencies). The different behaviors the compiler has between type position and expression position always trip me up. Also:

let client = <RestClient>::builder()

that's a nice, obscure piece of syntax you got there... I think I have never seen it explicitly documented anywhere... although I guess it arises from normal path parsing rules? So no need to discuss it explicitly as its own thing in the docs, though it is quite surprising if you have never seen it before... basically it solves the issue by pushing RestClient in type position (nested inside of the expression) where the default parameter is correctly considered?

Anyways...

I actually just noticed a regression. For some reason HTTPS requests with blocking clients do not work (cargo test get):

That's weird, i was quite sure we had tested it. We should check again. I'll let you know when we have something new, although we are closed for summer holidays until just about the end of the month, I'll see if I can maybe take a look in my free time before then.

[spietika]

I, of course, try to minimize breaking changes and keep backward compatibility when it is feasible, but I'd say it is not the main "concern" here. While quite unlikely, the new type parameter can be breaking for some already, so I will likely bump the major number anyway.

My thinking is more about the fact that custom resolvers will probably be minority use case, so I would really like to keep the let client = RestClient::builder() //... working as is for the majority.

Unfortunately, the way the compiler handles type defaults is not very helpful here, and we need to jump through hoops to keep the syntax.

Alternative approach still could be to implelement two sets of new() and builder() with one set fixing the type to default resolver. This kind of approach is used e.g. in Vec with allocator type:

• Vec::new fixes the allocator type always to Global. This way Vec::new() works for most people without annotating the allocator type.
• Vec::new_in allows to change the allocator type

About the obscure syntax. I think this link probably explains it better than I can!

Again, thanks for the time and effort you have put in to iterating the code! And no sweat about not working on this for a while!