Preserve URI parse error info in error source chain#4025
Preserve URI parse error info in error source chain#4025veeceey wants to merge 1 commit intohyperium:masterfrom
Conversation
|
hey, just checking in — is there anything else needed from my side? |
When an invalid URI is parsed by the http crate, the InvalidUri error contains useful details about what part of the URI was invalid. Previously this information was discarded when converting to hyper::Error. Now the original InvalidUri/InvalidUriParts error is stored as the source of the hyper::Error, making it available through the standard Error::source() chain and downcastable to the concrete type. Closes hyperium#3043
9915783 to
51998bd
Compare
| #[cfg(all(any(feature = "client", feature = "server"), feature = "http1"))] | ||
| VersionH2, | ||
| Uri, | ||
| Uri(Option<Cause>), |
There was a problem hiding this comment.
I think this feels a bit unexpected. I'd expect all these variants to stay thin without allocations, and the Cause would only ever be used in with().
There was a problem hiding this comment.
Yeah that's a fair point — adding the Option to the Parse variant bloats the enum for all variants even though it's only ever Some during the brief From conversion. I'll rework it so Parse::Uri stays thin and instead thread the cause through a separate path into Error::with(). Give me a bit to push the updated approach.
There was a problem hiding this comment.
Dug into this more and it's trickier than I initially thought. The issue is that parse() returns ParseResult<T> = Result<_, Parse>, not Result<_, Error>. So the URI cause needs to survive through Parse before getting converted to Error at the boundary in io.rs.
Alternatives I looked at:
- Change
ParseResultto useErrorinstead ofParse— lets us skip the intermediary entirely, but it touches theHttp1Transactiontrait + every parse call site. Pretty invasive. - Handle URI parsing manually (replace
?with explicitmap_err) — still can't return anErrorfromparse(). - Current approach:
Option<Cause>onParse::Uri— adds ~16 bytes to theParseenum. TheSomeonly exists briefly betweenFrom<InvalidUri>andFrom<Parse> for Errorwhere it gets moved into.with().
If the enum size bump is a dealbreaker, option 1 is probably the right long-term fix but it's a bigger refactor. Happy to go whichever direction you prefer — or close this if you'd rather tackle it as part of a broader error rework.
Fixes #3043
When hyper parsed an HTTP request with an invalid URI, it would create a
Parse(Uri)error but discard the originalhttp::uri::InvalidUrierror. This meanterror.source()returnedNone, making it impossible to get the actual reason the URI was invalid.Changed
Parse::Urito carry anOption<Cause>so the originalInvalidUriorInvalidUriPartserror flows through toError::source(). The error message andDisplayoutput are unchanged - the improvement is only visible through theError::source()chain.Added a test that verifies
error.source()returns the originalhttp::uri::InvalidUri: