Change WpNetworkHeaderMap to be a newtype for HashMap<String, Vec<String>>
#500
Conversation
| [ | ||
| self.0.get(header_name), | ||
| self.0.get(&header_name.to_lowercase()), | ||
| ] |
There was a problem hiding this comment.
We should make an insensitive case comparison here, right? But this is not quite that? For example, header_value_as_u32("x-wp-total") returns none if the server uses HTTP1.
There was a problem hiding this comment.
I prefer the current implementation of using HeaderMap, since we no longer need to think about header names being case insensitive whenever reading from the "header map" type.
My thinking is we should stick with the HeaderMap internally in the library, but export the "header map" type as HashMap in the uniffi interface.
There was a problem hiding this comment.
Using a separate type just to be able to handle things in a case insensitive way doesn't make sense to me. When we use two different types, we have to consider them both and just its extra mental load is not really worth it. Furthermore, converting http::HeaderMap is not a free operation and we don't get many benefits out of that cost anymore as we have to keep converting back and forth between requests and responses.
I'd have preferred if we could use http::HeaderMap everywhere, but if we can't do that, I'd prefer we use HashMap<String, Vec<String>> everywhere.
There was a problem hiding this comment.
For example, header_value_as_u32("x-wp-total") returns none if the server uses HTTP1.
Would you mind adding a failing test for this? I am probably missing something here because when I test it, it does seem to work with lowercase.
There was a problem hiding this comment.
I meant a new unit test. The new unit test passes for me locally and it seems like it passed in CI as well. Am I missing something?
There was a problem hiding this comment.
Ah.. the commit is not in this branch. I thought it might be the case after I commented, sorry about that. Let me take a look.
There was a problem hiding this comment.
Right.. so, this doesn't work because we don't know the case of the original key. I just borrowed this from the link header lookup logic during the refactor and didn't pay attention to it afterwards.
I think there are some cases this is an issue even in current trunk, because Rust is not the only one who might do a lookup, so using http::HeaderMap doesn't completely fix the issue. For example, if we expose the keys such as X-WP-Total and then Kotlin or Swift does the lookup, there is no guarantee that they'll do it in a case-insensitive way.
The solution to this would be to use a key such as LowercaseString which is what http::HeaderName does under the hood, but the problem is that'd also require using uniffi::Object because otherwise we can't control its construction.
There is also unicase, but it has the same problem of requiring uniffi::Object.
There was a problem hiding this comment.
For example, if we expose the keys such as X-WP-Total and then Kotlin or Swift does the lookup
IMHO, this issue is with Kotlin/Swift code. They should parse a string map using a proper HTTP header type, just as Rust should parse a string map using a proper HTTP header type.
There was a problem hiding this comment.
this issue is with Kotlin/Swift code
This stance is against what we are trying to do with the wordpress-rs project. Whenever possible, Rust layer is responsible for making sure that Kotlin/Swift code can't go wrong.
I have an idea but I am not sure if it'll work through the FFI layer. I'll test it in a few hours.
|
I have fixed CI issues in #489. Rebasing this PR should resolve the CI failures. |
This is a follow up to #490. As described in that PR, we are changing the exposed header map type to be
HashMap<String, Vec<String>>. However, #490 doesn't change the inner type that we work with, leaving it ashttp::HeaderMap. Although there is some merit in usinghttp::HeaderMapas our inner type, I don't think it outweighs the extra complexity of using 2 different types for the same thing. That's why this PR proposes convertingWpNetworkHeaderMapto a new type forHashMap<String, Vec<String>>.We still expose a function that converts this new type to
http::HeaderMap, ensuring that Rust clients, including our integration test suite, can easily work with it.The PR removes some unit tests that no longer make sense and adds a new one to validate that we are correctly parsing the number of pages. It also exposes additional fields for
WpNetworkRequestso that Rust clients can use them without cloning the value - which wasn't an issue when usingArc<T>.I believe there's still room for improvement in the request module, but as far as I can tell, this PR addresses our immediate needs. Any further improvements can be made in subsequent PRs.