Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion native/swift/Sources/wordpress-api/Login/API+Login.swift
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,6 @@ public extension WordPressAPI {

func getRestAPICapabilities(forApiRoot url: URL, using session: URLSession) async throws -> WpApiDetails {
let wpResponse = try await self.perform(request: WpNetworkRequest(method: .get, url: url, headerMap: [:]))
return try parseApiDetailsResponse(response: wpResponse)
return try wpResponse.parseApiDetailsResponse()
}
}
11 changes: 0 additions & 11 deletions wp_api/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,17 +178,6 @@ impl WpApiParamOrder {
}
}

#[uniffi::export]
pub fn parse_api_details_response(response: WpNetworkResponse) -> Result<WpApiDetails, WpApiError> {
let api_details =
serde_json::from_slice(&response.body).map_err(|err| WpApiError::ParsingError {
reason: err.to_string(),
response: response.body_as_string(),
})?;

Ok(api_details)
}

#[uniffi::export]
pub fn get_link_header(response: &WpNetworkResponse, name: &str) -> Option<WpRestApiUrl> {
if let Some(url) = response.get_link_header(name) {
Expand Down
101 changes: 85 additions & 16 deletions wp_api/src/request.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
use std::{collections::HashMap, fmt::Debug};

use http::HeaderMap;
use serde::Deserialize;
use url::Url;

use crate::login::WpApiDetails;
use crate::WpApiError;

use self::endpoint::WpEndpointUrl;
Expand Down Expand Up @@ -55,23 +57,45 @@ impl Debug for WpNetworkRequest {
}

// Has custom `Debug` trait implementation
#[derive(uniffi::Record)]
#[derive(uniffi::Object)]
pub struct WpNetworkResponse {
pub body: Vec<u8>,
pub status_code: u16,
// TODO: We probably want to implement a specific type for these headers instead of using a
// regular HashMap.
//
// 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>>,
body: Vec<u8>,
status_code: u16,
header_map: HeaderMap,
}

#[uniffi::export]
impl WpNetworkResponse {
#[uniffi::constructor]
pub fn new(
body: Vec<u8>,
status_code: u16,
header_map: Option<HashMap<String, String>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 🤞

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

  1. 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.
  2. 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries! Thanks for the review.

) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

let header_map: HeaderMap = header_map
.and_then(|m| (&m).try_into().ok())
.unwrap_or_default();

Self {
body,
status_code,
header_map,
}
}

pub fn parse_api_details_response(&self) -> Result<WpApiDetails, WpApiError> {
serde_json::from_slice(&self.body).map_err(|err| WpApiError::ParsingError {
reason: err.to_string(),
response: self.body_as_string(),
})
}
}

impl WpNetworkResponse {
pub fn get_link_header(&self, name: &str) -> Option<Url> {
self.header_map
.as_ref()
.map(|h_map| h_map.get(LINK_HEADER_KEY))?
.get(LINK_HEADER_KEY)
.and_then(|v| v.to_str().ok())
.and_then(|link_header| parse_link_header::parse_with_rel(link_header).ok())
.and_then(|link_map| {
link_map
Expand Down Expand Up @@ -187,11 +211,11 @@ mod tests {
#[case] expected_prev_link_header: Option<&str>,
#[case] expected_next_link_header: Option<&str>,
) {
let response = WpNetworkResponse {
body: Vec::with_capacity(0),
status_code: 200,
header_map: Some([("Link".to_string(), link.to_string())].into()),
};
let response = WpNetworkResponse::new(
Vec::with_capacity(0),
200,
Some([("Link".to_string(), link.to_string())].into()),
);

assert_eq!(
expected_prev_link_header.and_then(|s| Url::parse(s).ok()),
Expand All @@ -202,4 +226,49 @@ mod tests {
response.get_link_header("next")
);
}

#[test]
fn test_headers_case_insentive() {
let headers: HashMap<String, String> = [
("server".to_string(), "nginx".to_string()),
("x-nananana".to_string(), "Batcache-Hit".to_string()),
(
"date".to_string(),
"Thu, 30 May 2024 23:52:17 GMT".to_string(),
),
(
"content-type".to_string(),
"text/html; charset=UTF-8".to_string(),
),
(
"strict-transport-security".to_string(),
"max-age=31536000".to_string(),
),
("vary".to_string(), "Accept-Encoding".to_string()),
(
"Link".to_string(),
"<http://localhost/wp-json/wp/v2/posts?page=2>; rel=\"next\"".to_string(),
),
]
.into();
let response = WpNetworkResponse::new(Vec::with_capacity(0), 200, Some(headers));

assert_eq!(response.header_map.get("Server").unwrap(), "nginx");
assert_eq!(
response.header_map.get("X-Nananana").unwrap(),
"Batcache-Hit"
);
assert_eq!(
response.header_map.get("Date").unwrap(),
"Thu, 30 May 2024 23:52:17 GMT"
);
assert_eq!(
response.header_map.get("Content-Type").unwrap(),
"text/html; charset=UTF-8"
);
assert_eq!(
response.header_map.get("link").unwrap(),
"<http://localhost/wp-json/wp/v2/posts?page=2>; rel=\"next\""
);
}
}
12 changes: 6 additions & 6 deletions wp_api/tests/integration_test_common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,12 +194,12 @@ impl AsyncWpNetworking {
request = request.body(body);
}
let response = request.send().await?;

Ok(WpNetworkResponse {
status_code: response.status().as_u16(),
body: response.bytes().await.unwrap().to_vec(),
header_map: None, // TODO: Properly read the headers
})
let status_code = response.status().as_u16();
Ok(WpNetworkResponse::new(
response.bytes().await.unwrap().to_vec(),
status_code,
None, // TODO: Properly read the headers
))
}

fn request_method(method: RequestMethod) -> http::Method {
Expand Down