Use http::HeaderMap to store response headers #132

crazytonyli · 2024-05-31T00:26:59Z

Fixes #121.

This PR only changes headers in WpNetworkResponse, not WpNetworkRequest. It's fine for now to leave WpNetworkRequest as HashMap, because it's created internally in rust. But it'd be good to keep the implementation consistent between request and response types.

Changes

This PR uses http:HeaderMap in WpNetworkResponse. Thus, we have to expose WpNetworkResponse as a reference type instead of value type. I don't see a huge issue in this type change. One benefit it brings is that now we move all those standalong parse_xxx(response: WpNetworkResponse) functions into WpNetworkResponse.

oguzkocer · 2024-05-31T00:42:29Z

wp_api/src/request.rs

+    pub fn new(
+        body: Vec<u8>,
+        status_code: u16,
+        header_map: Option<HashMap<String, String>>,


If the constructor is taking a HashMap as an argument, how would that address the issue from #121:

HashMap is not suitable for storing HTTP headers, since header name can repeat.

In #121 (comment), I mentioned that using a HashMap to accept headers here won't be an issue on Apple platforms. I believe that's the case for OkHttp too. Because they merge repeated headers into one header. The issue here is storing headers in a HashMap. It should be okay to take a HashMap as an argument.

I am very sorry, but I don't really understand the difference between the two. Can you elaborate with an example?

How is storing it is different from taking it as an argument? If we are storing exactly the same thing as we take as an argument (in the uniffi::Record case before this change) how can that be different from taking it as an argument, converting it to HeaderMap and storing it? There is no mutation after it's stored, so I am a bit confused :(

It's quite late here, and I am only replying now because of our time zone difference, but I'll take another look at this tomorrow. Maybe it'll click then 🤞

One thing that would be really helpful - if it's easy to do - is to add a test to the existing implementation as a separate PR to showcase what can go wrong with it. Then, it'll be much easier to understand why this solution addresses it.

Do you mean like adding a failing test case: #133?

There are two potential issues here regarding how headers are stored in WpNetworkResponse.

A server may send multiple headers in response. i.e. the link header in curl -sI 'https://longreads.com'. WpNetworkResponse should be able to store those multiple link headers.

Headers need to be case insensitive. If we put a map "Server": "Nginx" into WpNetworkResponse, when we query the header like response.headers.get("server"), it should return Nginx.

Issue 1 won't occur in this library, because this library doesn't handle the raw HTTP response. It takes a HashMap which is created by iOS/Android's HTTP client. As long as those HTTP clients can process repeated HTTP headers, it'd be all good for this library to take those headers in. For example, in the case of curl -sI 'https://longreads.com', the HashMap passed from iOS to the Rust library won't contain multiple link header: it will be one entry for link whose value is all the link header value. See #121 (comment) for details.

Issue 2 does occur in this library, as demonstrated in #133.

In this PR, the WpNetworkResponse now stores an HeaderMap instance, not HashMap instance. The HashMap argument is converted into HeaderMap in the initializer.

Headers need to be case insensitive. If we put a map "Server": "Nginx" into WpNetworkResponse, when we query the header like response.headers.get("server"), it should return Nginx.

I was missing this bit, because I missed this comment when I was looking at the issue yesterday. I am really sorry to have wasted your time and I appreciate your patience in explaining the issue to me 🙇‍♂️

I'll dismiss my review and restart my review with this knowledge.

No worries! Thanks for the review.

oguzkocer

Thus, we have to expose WpNetworkResponse as a reference type instead of value type. I don't see a huge issue in this type change.

It's generally simpler to work with the Record types with uniffi, so whenever we can help it, we should prefer it over the Object type. With Object types, we are not able to expose the fields end always have to use functions - to opposite is true with Record types. The tricky bit with functions is that, we'll either have to expose things as a reference with Arc or reallocate them.

This is not necessarily a problem, but rather, a consideration to keep in mind when we are switching from Record to Object types.

One result of such a change would be that we'll expose limited functionality to the consumers, which - again - is not a problem. But, it's something to keep in mind. Not allowing consumers to use WpNetworkResponse outside of the provided functionality might even be a good thing.

I've left a line comment about constructor still taking a HashMap as an argument, so I believe the PR doesn't address the issue it intends to.

One option might be to accept Vec<(K, V)> in the constructor and build the HeaderMap that way. If we are ok with making WpNetworkObject a uniffi::Object, (which I am happy to give it a try) I don't see any reason that wouldn't work.

Also note that there is a TODO in WpNetworkResponse to address the issue by using a custom type for this. However, I think the custom type would only be necessary if we keep it as a uniffi::Record. So, if we intend to merge this PR after the issue is addressed, I think we can remove it.

As a final note, it'd be good to have a unit test that adds multiple values with the same key and validates that both values are there for the given key. Since this PR is addressing a specific issue, let's make sure that it does so through a unit test.

I was just about to submit my review, but I wanted to double check the documentation for HeaderMap. I see that there is a TryFrom<&'a HashMap<K, V>> but not from Vec<(K, V)> (although the extend function might work for that 🤔 ) which is giving me pause. I think it'd be good to double check that HeaderMap supports multiple values per key.

According to specification, multiple values per headers are supported as a comma separated list. So, I don't think libraries/clients have to support multiple value per key. And I feel like maybe we shouldn't either, if comma separated list is enough.

crazytonyli · 2024-05-31T03:54:42Z

I think it'd be good to double check that HeaderMap supports multiple values per key.

I don't think it does. But as I mentioned in my previous comment, it won't be an issue as long as the wrappers handle repeated headers properly.

I'm happy to change the initializer if you think that's the best way to go. But Vec<(String, String)> won't work, because it can't be exported as uniffi API. Maybe we'll need to use Vec<Vec<String>>.

oguzkocer · 2024-05-31T16:06:19Z

I'm happy to change the initializer if you think that's the best way to go.

Oh, I wasn't actually trying to suggest that. I was trying to better understand the issue and I thought looking into the implemented traits would be useful.

Let me check the TryFrom<&'a HashMap<K, V>> implementation to see if this would work for us. The possible issue I am seeing it with it is that we'd have to return a Result<Self, E> instead of Self.

Due to a misunderstanding of the goals of the PR

oguzkocer

@crazytonyli Thanks for addressing the issue and thank you again for your patience in explaining it to me.

I left some suggestions. Let me know what you think!

oguzkocer · 2024-05-31T16:29:10Z

wp_api/src/request.rs

    // It could be something similar to `reqwest`'s [`header`](https://docs.rs/reqwest/latest/reqwest/header/index.html)
    // module.
-    pub header_map: Option<HashMap<String, String>>,
+    headers: Arc<HeaderMap>,


We don't need to use Arc<T> for this since it's not supposed to be exposed to FFI through a function.

oguzkocer · 2024-05-31T16:33:50Z

wp_api/src/request.rs

    pub body: Vec<u8>,
    pub status_code: u16,


Now that we are "marking" this type as something that shouldn't be directly consumed, we can remove the pubs to clarify it further for Rust clients. This will prevent others from initializing it using the struct literal syntax and help compiler to point out the new associated function:

Although technically the headers being non-public is enough for this ^ bit, still it's a good idea to fully encapsulate all fields in this case.

While you're making this change, could you remove the TODO in this type? Thanks!

oguzkocer · 2024-05-31T16:45:54Z

wp_api/src/request.rs

+        body: Vec<u8>,
+        status_code: u16,
+        header_map: Option<HashMap<String, String>>,
+    ) -> Self {


I think we want to return a Result<Self, WpApiError> here with a new error variant ResponseHeaderParsingError. (or similar)

Following up on this comment, why creating a HeaderMap from HashMap could fail and I found a very simple case for it:

#[test] fn test_err_empty_header_name() { assert!(http::HeaderName::try_from("".to_string()).is_err()); }

The way this is handled right now with ok() is that it'll get silently swallowed and the whole header map will be discarded. The problem is, there is no trace of it anywhere, making it a very difficult bug to address later on.

Returning Result<Self, WpApiError> will force clients to address this possibility. They can do the exact same thing if they'd like - catch this specific error and try building the WpNetworkResponse again by passing None for the headers argument. Even though this practically results in exactly the same object after construction, because it's done in a client, it makes it clear how the possibility is handled. I think it's important for us to do the "right" thing from the Rust library and then if we want to keep it simple, we can do this workaround in the clients.

Do you think this error case warrant a panic? My thinking is that this method is designed to be only used by the native wrappers, which would always do the right thing to pass in valid headers (transforming headers in native HTTP clients to a HashMap is very simple and reliable). So in reality, it should never crash in production. If it crashes, I imagine it'd be very likely a programming error, instead of a WordPress site returns an invalid HTTP header in their response.

If we were to add an error here, I'd say declare it as a standalone error type instead of putting it into WpApiError. Adding to WpApiError means the apps/consumers would need to handle this error case, where they shouldn't need to.

We are hoping that we won't be the only consumers of this library, so I think it's important for us to properly handle this issue. However, I am not sure about the error type, can I think on it and get back to you?

@crazytonyli I've separated this into its own issue. #134

Since I am working on a major change at the moment, I'd like to merge this as soon as possible.

oguzkocer · 2024-05-31T16:49:26Z

wp_api/src/request.rs

+        Self {
+            body,
+            status_code,
+            headers: headers.into(),


Nitpick: I kind of like being consistent with the type names (HeaderMap in this case) when I can, so I'd have named this header_map in the WpNetworkResponse struct. (I am leaving the comment here because that line has a big discussion)

You can use variable shadowing to name the headers in line 80 as header_map to get this working cleanly.

oguzkocer · 2024-05-31T16:50:17Z

wp_api/src/request.rs

        );
    }
+
+    #[rstest]


Nitpick: We can mark this as #[test] since there is no rstest specific implementation involved.

oguzkocer · 2024-06-04T19:09:55Z

wp_api/src/request.rs

+        body: Vec<u8>,
+        status_code: u16,
+        header_map: Option<HashMap<String, String>>,
+    ) -> Self {


@crazytonyli I've separated this into its own issue. #134

Since I am working on a major change at the moment, I'd like to merge this as soon as possible.

This reverts commit bef8433.

Use HeaderMap to store response headers

f57370f

crazytonyli requested review from jkmassel and oguzkocer May 31, 2024 00:26

crazytonyli changed the title ~~Use HeaderMap to store response headers~~ Use http::HeaderMap to store response headers May 31, 2024

No need to re-wrap parsed result

463ec41

oguzkocer reviewed May 31, 2024

View reviewed changes

oguzkocer previously requested changes May 31, 2024

View reviewed changes

Use TryFrom<HashMap>

ae7819c

crazytonyli mentioned this pull request May 31, 2024

WpNetworkResponse header name is not case insensitive #133

Closed

oguzkocer self-requested a review May 31, 2024 15:59

oguzkocer requested changes May 31, 2024

View reviewed changes

crazytonyli added 4 commits June 4, 2024 09:24

Don't need to declare as Arc

b9195ce

Make WpNetworkResponse properties private

c9b9a14

Remove compelted TODO

d75b36a

Rename property back as header_map

a798e8d

oguzkocer approved these changes Jun 4, 2024

View reviewed changes

oguzkocer merged commit bef8433 into trunk Jun 4, 2024

oguzkocer deleted the refactor-network-response-header branch June 4, 2024 19:10

oguzkocer added a commit that referenced this pull request Jun 4, 2024

Revert "Use http::HeaderMap to store response headers (#132)"

7b77f64

This reverts commit bef8433.

oguzkocer mentioned this pull request Jun 4, 2024

Revert "Use http::HeaderMap to store response headers" #135

Merged

oguzkocer added a commit that referenced this pull request Jun 4, 2024

Revert "Use http::HeaderMap to store response headers (#132)" (#135)

beccf31

This reverts commit bef8433.

This was referenced Jul 1, 2024

Authorization Url Discovery #160

Merged

WpNetworkResponse constructor should return Result<T> #134

Closed

Use http::HeaderMap to store response headers #132

Use http::HeaderMap to store response headers #132

Uh oh!

Conversation

crazytonyli commented May 31, 2024

Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oguzkocer left a comment

Choose a reason for hiding this comment

Uh oh!

crazytonyli commented May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oguzkocer commented May 31, 2024

Uh oh!

oguzkocer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

crazytonyli commented May 31, 2024 •

edited

Loading