Skip to content

http_interop: Remove requirement for header values to be UTF-8#672

Merged
algesten merged 2 commits intoalgesten:mainfrom
MarijnS95:http-interop-remove-utf8-header-value-requirement
Oct 23, 2023
Merged

http_interop: Remove requirement for header values to be UTF-8#672
algesten merged 2 commits intoalgesten:mainfrom
MarijnS95:http-interop-remove-utf8-header-value-requirement

Conversation

@MarijnS95
Copy link
Copy Markdown
Contributor

CC @kade-robertson

The current interop implementation forces fallibility around header values when they are not UTF-8, both in conversions from http to ureq as well as the other way around.

This should not be necessary as both http and ureq treat these as opaque byte arrays internally, but unfortunately the current open API for ureq and http make it a bit nasty to deal with.

@MarijnS95 MarijnS95 force-pushed the http-interop-remove-utf8-header-value-requirement branch from 11189ad to b46a442 Compare October 10, 2023 23:08
@MarijnS95 MarijnS95 force-pushed the http-interop-remove-utf8-header-value-requirement branch from b46a442 to 1d1d3a8 Compare October 11, 2023 08:53
Copy link
Copy Markdown
Owner

@algesten algesten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@algesten
Copy link
Copy Markdown
Owner

Can we construct a test for this?

@MarijnS95 MarijnS95 force-pushed the http-interop-remove-utf8-header-value-requirement branch from 1d1d3a8 to 485656a Compare October 11, 2023 14:05
@MarijnS95
Copy link
Copy Markdown
Contributor Author

Sure! I added them for requests but probably also have to add them for responses... And found that the current str-based convenience API makes it a bit tedious to test these :)

@MarijnS95 MarijnS95 force-pushed the http-interop-remove-utf8-header-value-requirement branch 3 times, most recently from bcb160a to c354e58 Compare October 11, 2023 19:09
@algesten
Copy link
Copy Markdown
Owner

Looks great! Thanks!

Maybe that test helper function need to be allowed unused for some test feature flag combo?

@MarijnS95 MarijnS95 force-pushed the http-interop-remove-utf8-header-value-requirement branch from c354e58 to 9171db5 Compare October 17, 2023 15:37
@MarijnS95
Copy link
Copy Markdown
Contributor Author

Yeah. It's a bit nasty though as it's a useful function, just not something to expose publicly and the conditional is quite large now...

@MarijnS95 MarijnS95 force-pushed the http-interop-remove-utf8-header-value-requirement branch from 9171db5 to 458d80e Compare October 17, 2023 15:39
@MarijnS95 MarijnS95 requested a review from algesten October 17, 2023 15:39
The current interop implementation forces fallibility around header
values when they are not UTF-8, both in conversions from `http` to
`ureq` as well as the other way around.

This should not be necessary as both `http` and `ureq` treat these as
opaque byte arrays internally, but unfortunately the current open API
for `ureq` and `http` make it a bit nasty to deal with.
@MarijnS95 MarijnS95 force-pushed the http-interop-remove-utf8-header-value-requirement branch from 458d80e to a857d88 Compare October 17, 2023 15:41
@algesten algesten merged commit 776e887 into algesten:main Oct 23, 2023
@algesten
Copy link
Copy Markdown
Owner

Thank you! This looks great!

@MarijnS95 MarijnS95 deleted the http-interop-remove-utf8-header-value-requirement branch October 23, 2023 13:34
@MarijnS95
Copy link
Copy Markdown
Contributor Author

@algesten thanks! This should be the last of my PRs to http_interop.

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 this pull request may close these issues.

2 participants