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.

Sign up for GitHub

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

Jump to bottom

URI parse errors should preserve information from the HTTP crate #3043

Open
hawkw opened this issue Nov 3, 2022 · 3 comments · May be fixed by #3044
Open

URI parse errors should preserve information from the HTTP crate #3043

hawkw opened this issue Nov 3, 2022 · 3 comments · May be fixed by #3044
Assignees
@hawkw
Labels
C-feature Category: feature. This is adding a new feature. E-easy Effort: easy. A task that would be a great starting point for a new contributor.

Comments

@hawkw
Copy link
Contributor

hawkw commented Nov 3, 2022

Is your feature request related to a problem? Please describe.

When an invalid URI is parsed by the http crate, it returns an InvalidUri error. When hyper encounters one of these errors, it converts it into a hyper::Error type with an error kind that indicates that an invalid URI was encountered. The http::uri::InvalidUri type contains additional information describing what part of the URI was invalid, which is included in the error's fmt::Display output: https://github.com/hyperium/http/blob/c1f309e0c3695f8e14e1b54c738681783d4c7b9e/src/uri/mod.rs#L1063-L1079

Unfrortunately, when hyper converts these errors into its hyper::Error type, the additional information is discarded:

hyper/src/error.rs

Lines 489 to 499 in 75aac9f

impl From<http::uri::InvalidUri> for Parse {
fn from(_: http::uri::InvalidUri) -> Parse {
Parse::Uri
}
}
impl From<http::uri::InvalidUriParts> for Parse {
fn from(_: http::uri::InvalidUriParts) -> Parse {
Parse::Uri
}
}

The error that's returned to the application will format as just "invalid URI", which is not particularly descriptive. Being able to surface more information about why a URI was invalid would be useful, particularly in applications where URIs are input by the user.

Describe the solution you'd like

It would be nice if hyper preserved the description of what part of the URI was invalid. I think we should probably change the Parse::Uri variant in hyper::error to contain an http::uri::InvalidUri, so that we can format the additional error information in the fmt::Display impl for hyper::Error.

Since we don't want to expose types from other crates in hyper's public API, for stability reasons, we probably shouldn't allow the user to access the http::uri::InvalidUri error in the hyper::Error source chain, or allow downcasting the error to it.

Describe alternatives you've considered

  • We could include the http::uri::InvalidUri as a source of hyper::Error. This might be conceptually the "right thing", but it would expose a type from an external crate in hyper's public API, which is undesirable as http is not yet 1.0.
  • We could leave things the way they are now. Unfortunately, that would mean that hyper would completely swallow information describing what part of the URI was invalid, and this would never be exposed to the user.
@hawkw hawkw added E-easy Effort: easy. A task that would be a great starting point for a new contributor. C-feature Category: feature. This is adding a new feature. labels Nov 3, 2022
@hawkw hawkw self-assigned this Nov 3, 2022
@hawkw
Copy link
Contributor Author

hawkw commented Nov 3, 2022

I'm happy to put together a quick PR for this, but I figured it was worth opening an issue first.

@hawkw hawkw mentioned this issue Nov 3, 2022
Closed
@LucioFranco
Copy link
Member

Why not let users downcast it? If http will also 1.0 I don't see the issue in that? Otherwise, I agree with what you said.

@hawkw
Copy link
Contributor Author

hawkw commented Nov 4, 2022

Why not let users downcast it? If http will also 1.0 I don't see the issue in that? Otherwise, I agree with what you said.

When http 1.0 is released, we should probably make it available as a source/downcast target. In the meantime, it would still be better to at least display it in fmt::Display.

hawkw added a commit that referenced this issue Nov 4, 2022
@hawkw
feat(error): preserve InvalidUri details
08622d5 
This branch changes the representation of the `Parse::Uri` error kind to
preserve information provided by the `http` crate's `uri::InvalidUri`
and `uri::InvalidUriParts` errors. A new `InvalidUri` enum is added that
holds one of those two error types, or a custom message (since `hyper`
currently returns `Parse::Uri` errors that didn't come from an inner
`http` error in some cases). The new enum has a custom `Display` and
`Debug` implementation to reduce repetition of the string "invalid URI"
in its formatted output.

This is _not_ stored as the error's `cause` currently in order to avoid
exposing the `http` crate's error types in the public API. However, when
`http` 1.0 is released, we can simplify this code significantly by
storing the error as a cause and exposing it in the source chain.

Closes #3043
@hawkw hawkw linked a pull request Nov 4, 2022 that will close this issue
Open
hawkw added a commit that referenced this issue Nov 4, 2022
@hawkw
feat(error): preserve InvalidUri details
c3e4bb3 
This branch changes the representation of the `Parse::Uri` error kind to
preserve information provided by the `http` crate's `uri::InvalidUri`
and `uri::InvalidUriParts` errors. A new `InvalidUri` enum is added that
holds one of those two error types, or a custom message (since `hyper`
currently returns `Parse::Uri` errors that didn't come from an inner
`http` error in some cases). The new enum has a custom `Display` and
`Debug` implementation to reduce repetition of the string "invalid URI"
in its formatted output.

This is _not_ stored as the error's `cause` currently in order to avoid
exposing the `http` crate's error types in the public API. However, when
`http` 1.0 is released, we can simplify this code significantly by
storing the error as a cause and exposing it in the source chain.

Closes #3043
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hawkw hawkw

Labels
C-feature Category: feature. This is adding a new feature. E-easy Effort: easy. A task that would be a great starting point for a new contributor.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(error): preserve InvalidUri details hyperium/hyper
2 participants
@hawkw @LucioFranco