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.

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

rust: document that HttpFgbReader won't work with https urls by default #231

Closed
msalib opened this issue Aug 13, 2022 · 2 comments · Fixed by pka/http-range-client#4
Closed
Assignees

Comments

@msalib
Copy link
Contributor

msalib commented Aug 13, 2022

I stumbled on this -- if you make a simple app using flatgeobuf that doesn't depend directly on reqwest@0.11.0, it will fail to handle https urls with this error message:

Error: http error `error sending request for url (https://blah/blah/blah.fgb): error trying to connect: invalid URL, scheme is not http`

That error originates in hyper. I think the issue is that http-range-client has an optional dependency on reqwest and so without that, you'll get a client that has no support for HTTPS. That's...really odd. I'd suggest either:

  • document clearly that HttpFgbReader will not read HTTPS urls unless you add an explicit dependency on the same version of request that flatgeobuf's http-range-client depends on, or
  • add a tls or http feature to flatgeobuf and document that in the README.
@pka
Copy link
Member

pka commented Aug 15, 2022

That's not a good behaviour. IMO we should add the TLS feature per default when using the http feature.

@michaelkirk
Copy link
Collaborator

michaelkirk commented Aug 21, 2023

I agree. Rather than add more documentation, I think it's preferable to just do what people expect to be done anyway. For the same reason reqwest enables https by default, so should http-range-client. People fully expect an http client to be able to speak tls by default without needing to read documentation and pass special options.

I've opened a PR that does this here pka/http-range-client#4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants