Use RFC-compliant HeaderValue type. #287
Conversation
When "header-value" feature is enabled, a RFC-compliant HeaderValue type is used instead of the previously used UTF-8 String type. Since HTTP field values aren't UTF-8 encoded, using the String type meant that a plugin would crash when valid, but obsolete, non-UTF-8 characters were present in HTTP headers and/or trailers. This feature is currently optional to avoid breaking changes and to help with migration, but it will become a default feature in v0.3. The HeaderValue type is re-exported from the http crate. Signed-off-by: Piotr Sikora <[email protected]>
Signed-off-by: Piotr Sikora <[email protected]>
|
|
||
| - name: Clippy (header-value) | ||
| run: cargo clippy --release --all-targets --target=wasm32-wasip1 --features header-value | ||
|
|
There was a problem hiding this comment.
Note to myself: add cargo test and cargo bench with --features header-value if merged after #283.
| @@ -1159,46 +1193,59 @@ mod utils { | |||
| bytes | |||
| } | |||
|
|
|||
| pub(super) fn serialize_map(map: Vec<(&str, &str)>) -> Bytes { | |||
| pub(super) fn serialize_map<V>(map: Vec<(&str, V)>) -> Bytes | |||
There was a problem hiding this comment.
Note to myself: drop Bytes if merged after #286.
| This crate supports the following optional features: | ||
|
|
||
| - `header-value` - uses RFC-compliant `HeaderValue` instead of UTF-8 `String` for HTTP header and trailer values. | ||
| This will become the default in future releases. |
There was a problem hiding this comment.
Note that this PR doesn't introduce any changes or restrictions on header names in the SDK, since all valid HTTP field names are also valid UTF-8 strings, and proxies are very strict about this, so we never panic deserializing header names.
Having said that, I'm tempted to use HeaderName type while we're here, and turn this into a full typed variant.
Unfortunately, because we're passing HTTP/2 pseudo-headers (:authority, :status, etc.) in the header map, those names are non-compliant with RFC HTTP field names, so we'd need to either:
- create wrapper around
http::HeaderNametype, - create our own more forgiving type,
- return a pair of maps, one with pseudo-headers and one with HTTP headers.
| log = "0.4" | ||
|
|
||
| [features] | ||
| default = [] | ||
| header-value = ["dep:bytes", "dep:http"] |
There was a problem hiding this comment.
This feature could be renamed to valid-header, strict-header or rfc-header to make this reusable in case we want to add restrictions on header name in the future.
Alternatively, we could drop this as a feature and instead add those functions as variants with _typed() suffix.
Thoughts?
| } | ||
|
|
||
| #[cfg(feature = "header-value")] | ||
| pub(super) fn deserialize_map(mut bytes: bytes::Bytes) -> Vec<(String, HeaderValue)> { |
There was a problem hiding this comment.
Note to self: drop bytes:: prefix if merged after #286.
When "header-value" feature is enabled, a RFC-compliant HeaderValue
type is used instead of the previously used UTF-8 String type.
Since HTTP field values aren't UTF-8 encoded, using the String type
meant that a plugin would crash when valid, but obsolete, non-UTF-8
characters were present in HTTP headers and/or trailers.
This feature is currently optional to avoid breaking changes and to
help with migration, but it will become a default feature in v0.3.
The HeaderValue type is re-exported from the http crate.