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

Remove default host of localhost #105

Closed
jsha opened this issue Jul 1, 2020 · 2 comments · Fixed by #108
Closed

Remove default host of localhost #105

jsha opened this issue Jul 1, 2020 · 2 comments · Fixed by #108

Comments

@jsha
Copy link
Collaborator

jsha commented Jul 1, 2020

Request::new can be called with something that's not a URL, e.g. /path, and it will automatically prepend http://localhost/. I think that's the wrong thing in most cases. We should instead make it an error to construct a Request with a string that doesn't parse as a URL.

This will probably break a lot of doctests, but I think we should update the doctests to use real URLs. For now those can be http://localhost/; when #82 is fixed, those can be http://example.com/, with an override to point example.com to localhost so the tests run quickly and don't hit the network.

@algesten
Copy link
Owner

algesten commented Jul 1, 2020

This comes from that early ureq API was modelled on superagent, which is still my goto nodejs http client and somewhat of a hero for me in API design.

I think that's the wrong thing in most cases.

Is it? If we ask "what could the user possibly mean with this input?"

Philosophically both ureq (and now hreq), were and are driven by a feeling that in the rust community, sometimes correctness and strict adherence to spec overrides usability concerns in API design. The http crate is better now, but the correctness of hyper, where the http crate started, often shows in the API the user is exposed to.

A fast and correct HTTP implementation for Rust.

For example this method signature:

impl Request {
  fn header(&self, key: &str) -> Option<&str> {}
}

is no good if the overriding concern for the API is correctness. Not all correct header values can be represented as rust String, which forces the use of Result and/or an intermediary HeaderValue.

But how often do we really encounter such headers in the wild?

I want ureq (and hreq) to be correct and on spec, I want to be able to handle such headers if I find them, but without forcing the user of the API to jump through hoops. Simple, rememberable and User first API (how I encapsulated these thoughts in a design goal).

This is the bigger picture of the absence of Result, the synthetic http error, and why using a path as URL defaults to localhost.

In early ureq I did however lurch too far in the other direction, so in the same way I'm for reintroducing some Result instead of synthetic http error, also the localhost-by-default decision is up for challenge.

@jsha
Copy link
Collaborator Author

jsha commented Jul 2, 2020

Thanks for the additional background on the API design!

I think we can probably find a middle path, where we keep the API simple and also filter out some error cases, by deferring all errors until .do_call(). We would accept any String in the constructor of Request, and do validation later. If .do_call() finds that the URL is invalid, it would return a Response with the Error field already set.

@jsha jsha closed this as completed in #108 Jul 5, 2020
jsha added a commit that referenced this issue Jul 5, 2020
This doesn't change the API at all, since it delays any errors until do_call.
Since there was already an `Into<Response> for Error`, and do_call already
had a `.unwrap_or_else(|e| e.into())`, nothing in do_call needed to change.
The only visible effect of this is that code that was previously relying on
`get("/path")` to fetch something from localhost will now get an error. I think
most of those cases would probably actually be accidents, possibly caused
by typos, e.g. `get("example.com/path")` (forgetting the https: prefix).

Fixes #105
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.

2 participants