Users endpoint #58

oguzkocer · 2024-04-05T17:48:45Z

Addresses #17, #18, #19 & #20. Implements the users endpoint types and requests. It doesn't include filtering which has its own issue #29 and it doesn't implement the meta field which again has its own issue #57.

Introduces derive_builder Rust dependency. I've added this just to make it easier to test the Rust code, but I am not particularly happy with its API and generated code. I decided to keep it for this PR because otherwise we'd have to add a lot of code just to set some of the parameter fields to None, but at some point I'll look into alternatives.
Extracts a generic parse_response_for_generic_errors function so we don't keep duplicating the same code. I don't think this will remain as is, but just the fact that it's extracted in some form will make it easier to refactor later.
Splits the url building from the request building. I think this makes it easier to follow what's happening in each implementation and makes it easier to refactor later.
Introduces WPContext and makes it a required argument for endpoints that accept it. In the API, this parameter is optional and defaults to view. However, in order to parse the result correctly, the context needs to be known. Requiring the context to be set should help avoid mistakes in parsing and make the code self documenting. Native wrappers can work around this requirement by adding default values, but the difference there would be that the default value will still be visible in the native code, as opposed to being hidden at a lower level.
Adds body to WPNetworkRequest which is used to make POST requests. We could consider making WPNetworkRequest an enum, but I don't think that design consideration belongs to this PR, so I've made it optional for now.
Implements basic /users requests in wp_cli. My intention is to follow this PR up with a PR that adds a test suite for /users, as also discussed on Slack, but I wanted to verify at least the basics of the requests are working as expected. The subsequent PR will likely remove all this code as they would be covered by the test suite.
There are still bits an pieces that I am not very happy with in this PR, but in the interest of shipping things quickly, I am leaving those annoyances to a later PR. I am also not sure how we should address them yet. We can discuss the details if they show up in the PR review, but otherwise, I don't think it's too important to document them here.

crazytonyli · 2024-04-09T00:41:41Z

wp_api/src/lib.rs

-            }
-            WPAuthentication::None => (),
-        }
-
        WPNetworkRequest {
            method: RequestMethod::GET,
            url: Url::parse(url.as_str()).unwrap().into(),


Just a random question. unwrap seems very dangerous to me, but also appears to be used fairly often in rust code (maybe I have a wrong impression...), should we ban it and return Result instead?

I saw your subsequent comment as well, but I agree with your original sentiment. Within the library code - and for now that's only the wp_api crate - I think we should avoid directly using unwrap unless it's accompanied by a prior check to make sure the unwrap is safe. If you take a look at #53, this is how I approached that implementation. The only unwrap there is after a validation that it is safe.

Having said all that, unfortunately we are not there with the wp_api yet. We have unwraps all over the place because this project started as a prototype and we haven't had a chance to address them yet. Furthermore, the way to avoid unwraps is to have good error handling setup, which is what we are missing at the moment. This is in my todo for users once I add the integration tests.

In general, I like the following process:

Implement happy path, with some shortcuts - including using unwrap

Add testing

Refactor the code to add error handling and remove any unwraps

I'd normally do this in a feature branch, but since we are still in a transitionary stage, it's faster to directly target trunk.

crazytonyli · 2024-04-09T00:51:49Z

wp_api/src/users.rs

+pub struct UsersEndpoint {}
+
+impl UsersEndpoint {
+    pub fn list_users(site_url: &Url, context: WPContext, params: Option<&UserListParams>) -> Url {


Out of curiosity, what's the difference between Option<&T> and &Option<T>? It looks like they can be used in the same way (see this code).

Option<&T> means we either have a reference to data or we have nothing. &Option<T> means we have a reference to an Option which may or may not have data.

I'd highly suggest watching this youtube video on this exact topic. They do a much better job of explaining it then I could 😅

Thanks for the link! (I did not expect a 18 minutes long video to explain their difference though 😅 )

crazytonyli · 2024-04-09T00:55:05Z

wp_api/src/lib.rs

+            method: RequestMethod::POST,
+            url: UsersEndpoint::create_user(&self.site_url).into(),
+            header_map: self.header_map(),
+            body: serde_json::to_vec(&params).ok(),


Do you think we should add a Content-Type: application/json header here?

For some reason, I did this in wp_networking's request, instead of adding it to the header map that we build here 🤦

Addressed in 49ffd28.

crazytonyli · 2024-04-09T00:59:32Z

wp_api/src/lib.rs

+        }
+    }
+
+    pub fn create_user_request(&self, params: UserCreateParams) -> WPNetworkRequest {


It probably doesn't make any difference for now, but what do you think declaring params as &UserCreateParams, because this function doesn't need to take its ownership?

Although I agree that this is the "correct" thing to do, I am a bit reluctant. Mainly because we can't take Option<&UserListParams> in the list_users_request as UniFFI doesn't currently support Option<&T>.

If we use Option<UserListParams> for list_users_request and we use &T everywhere else, that'd make our API inconsistent. So, if we want to use &T, I think we should consider making UserListParams non optional as well.

Neither feels that good to me, because either way we are going to have a non-ideal implementation somewhere, but I think the overall consistency is very important. So, we should decide which one we like better.

I guess one argument for using Copy trait as opposed to references in our API might be that it's a safer implementation to be used through FFI. 🤷‍♂️ However, I am mostly leaning towards making UserListParams non-optional. What do you think?

I missed that these are exposed as uniffi API, which of course comes with many constraints.

If we use Option for list_users_request and we use &T everywhere else, that'd make our API inconsistent.

I agree. Having watched the video linked, I think we should use Option<&T> consistently in our functions.

I guess one argument for using Copy trait as opposed to references in our API might be that it's a safer implementation to be used through FFI.

If I understand it correctly, using Copy means they will be copied in all function calls, right? Not just where uniffi operates (between native and Rust)? If that's the case, that feels like a big compromise to me, because I don't think we want to copy them in the internal function calls.

Plus, I believe uniffi already copies function arguments when passing them back and forth between native and Rust.

Not something needs to be address in this PR, but I'm wondering if we should clearly separate "pure" Rust code and Rust code that is exposed as uniffi API. That way we can write sensible Rust code (for example, users::list_user_request(Option<&Params>)), and only makes undesired API choices (such as list_user_request(&Option<Params>)) on the uniffi API layer which calls the "pure" Rust code. That means the uniffi API layer becomes very thin with minimal meaningful code.

I agree. Having watched the video linked, I think we should use Option<&T> consistently in our functions.

I actually didn't even consider using &Option<T> in this case, because the fact that it works feels like it's by chance rather than by design. However, in this case, since UniFFI doesn't support Option<&T>, I think using &Option<T> is an OK compromise. It also doesn't have any of the other issues I mentioned before. Hopefully someday soon Option<&T> will also be supported which will make it a non-breaking change for the native, so I think this is a decent way to go for now. I've made the change in 9656d8c.

If I understand it correctly, using Copy means they will be copied in all function calls, right? Not just where uniffi operates (between native and Rust)? If that's the case, that feels like a big compromise to me, because I don't think we want to copy them in the internal function calls.

Unless I am mistaken, compiler will optimize most of that away and will only actually copy if the values are mutated down the line.

Not something needs to be address in this PR, but I'm wondering if we should clearly separate "pure" Rust code and Rust code that is exposed as uniffi API. That way we can write sensible Rust code (for example, users::list_user_request(Option<&Params>)), and only makes undesired API choices (such as list_user_request(&Option)) on the uniffi API layer which calls the "pure" Rust code. That means the uniffi API layer becomes very thin with minimal meaningful code.

I think we should - but not as part of this PR 😅

crazytonyli · 2024-04-09T01:09:14Z

wp_api/src/users.rs

+    parse_response_for_generic_errors(response)?;
+    serde_json::from_slice(&response.body).map_err(|err| WPApiError::ParsingError {
+        reason: err.to_string(),
+        response: std::str::from_utf8(&response.body).unwrap().to_string(),


Using String::from_utf8?

It's probably okay to unwrap here, but maybe using from_utf8_loosy just in case?

Addressed in c46d3d8.

crazytonyli · 2024-04-09T01:11:49Z

wp_api/src/users.rs

+
+impl UsersEndpoint {
+    pub fn list_users(site_url: &Url, context: WPContext, params: Option<&UserListParams>) -> Url {
+        let mut url = site_url.join("/wp-json/wp/v2/users").unwrap();


Regarding my previous comment about banning unwrap, this is a legitimate use case of unwrap, because we know for sure that this join call produces a url. So, maybe we can't simply ban unwrap. Maybe warn about unwrap and require us manually add a #[allow(unwrap)] or something to suppress the warning?

Actually, I am not so sure about that. We are taking a site_url as an argument, it's not something we have built - not in this part of the implementation anyway and we can't assume implementation details outside of this context, so I think it's possible to get parse errors.

I think we need to implement a UrlParseError error type regardless of this implementation, so I'd say it's safer and nicer to return that type in this case as well. If it never returns this error, that's fine. With ? shortcut it shouldn't add any extra complications to our code.

Correctly me if I'm wrong, considering site_url is already a legal URL, calling "join" using a hard-coded string which we know is also a legal URL path should always produces a legal URL result. Does that mean we'll get a Some(Url) in return here?

Not sure if this is actionable, but something like this join call would be a great thing to put under test because we could pretty trivially mess it up

I created a separate issue for this and will look into the necessity of unwrap - or proper error handling - as part of that: #63

crazytonyli · 2024-04-09T01:14:00Z

wp_api/src/users.rs

+            .unwrap();
+        url.query_pairs_mut()
+            .append_pair("context", context.as_str());
+        url


Nitpick: chaining these method calls to make this function a one statement implementation?

Could you share what you have in mind? As far as I know this is not chainable, but I could be missing something.

Specifically, query_pairs_mut() will erase the type as it returns Serializer<'_, UrlQuery<'_>> which I don't think can be transformed back to Url, but again, maybe I am missing something?

Nope. I was wrong. I though you can treat url like a temporary variable in other languages. It appears we must hold a mut reference to it.

This can work, but I don't think cloning the already built URL is worth it.

pub fn retrieve_user(site_url: &Url, user_id: UserId, context: WPContext) -> Url { site_url .join(format!("/wp-json/wp/v2/users/{}", user_id).as_str()) .unwrap() .query_pairs_mut() .append_pair("context", context.as_str()) .finish() .to_owned() }

Yeah, that doesn't look right to me at all.

crazytonyli · 2024-04-09T01:17:15Z

wp_api/src/users.rs

+}
+
+impl Display for WPApiParamUsersOrderBy {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {


Random question: There are also as_str functions in other places to convert a type to a URL query value. How do you choose to implement Display or an as_str function?

Good catch! I initially implemented the Display trait for all of them, but then I realized that's not as good as a separate as_str function - and it seemed that's the more idiomatic way to do it when I looked at other libraries. It seems I've missed updating this one which I addressed in 66508da.

crazytonyli · 2024-04-09T01:42:33Z

wp_api/src/users.rs

+                .map(|x| ("has_published_posts", x.to_string())),
+        ]
+        .into_iter()
+        .flatten()


What do you think slightly changing this implementation to something like this: 1a46a50?

I am not 100% sure, but I think the original implementation will require less memory, although it's probably not that big of a deal.

I don't have a strong preference, but I opted for this implementation because I think it conveys the right information at each step. In one, we are only adding arguments if we have them, in other we add them and them strip them away. If you'd prefer your suggested approach I wouldn't mind changing it.

I didn't think about memory usage at all, but I agree with you it's probably not a big deal in this particular case here.

Maybe it's jut me, the suggest code is easier to read on first glance. Because it lists out all the possible keys on the "outside layer", instead of the inner maps. 😄

I've included your suggested change in 6c9fe91 with a slight improvement to the filter_map.

oguzkocer · 2024-04-09T15:56:25Z

Thanks for your review @crazytonyli 🙇‍♂️ I think I replied to all your comments. Let me know what you think!

crazytonyli

Added a couple of final comments. The building url one probably should be addressed.

I see you are working on integration test using real test sites. Have you thought about adding unit tests?

crazytonyli · 2024-04-09T23:08:06Z

wp_api/src/users.rs

+    response: &WPNetworkResponse,
+) -> Result<Option<UserWithViewContext>, WPApiError> {
+    parse_users_response(response)
+}


Is it okay to remove the Option from the returned results: Result<UserWithViewContext, WPApiError>?

Addressed in 24fcd6b.

crazytonyli · 2024-04-09T23:13:26Z

wp_api/src/users.rs

+    }
+
+    pub fn create_user(site_url: &Url) -> Url {
+        site_url.join("/wp-json/wp/v2/users").unwrap()


I don't think we should use join here, because site_url may not always be "domain root". See this code.

Maybe path_segments_mut is what we need here?

This is correct and another good candidate for a test – for instance:

How does this work for a bare domain (example.com)

How does this work for a subdomain (wp.example.com)

How does this work for a subdirectory (example.com/wp)

I thought join was a basic string join with error handling, however that doesn't seem to be the case. I don't think Url is matching our needs as something we use everywhere in the code - as in, taking Url as an argument or returning Url type.

I'd like to introduce a new type that has an easier to understand public facing API and use Url internally instead. I am not just talking about a wrapper around Url, but instead something that internally handles site_url.join("/wp-json/wp/v2/users") and similar as well.

I'll get started working on that immediately, but I'd prefer to open a separate PR for it as it'll be a lot more than just using path_segments_mut.

Addressed in #66.

crazytonyli · 2024-04-10T01:11:48Z

I think we need a function to parse DELETE responses. At least, checking if there is any error in the response.

crazytonyli · 2024-04-10T21:19:15Z

wp_api/src/lib.rs

+        WPNetworkRequest {
+            method: RequestMethod::POST,
+            url: UsersEndpoint::update_current_user(&self.site_url).into(),
+            header_map: self.header_map(),


Content-Type header is missing in this request.

Addressed in 618e099.

jkmassel · 2024-04-10T21:49:45Z

wp_api/src/lib.rs

+        WPNetworkRequest {
+            method: RequestMethod::GET,
+            url: UsersEndpoint::list_users(&self.site_url, context, params.as_ref()).into(),
+            header_map: self.header_map(),


We should probably also always send Accept: application/json to indicate that's what we want per https://www.rfc-editor.org/rfc/rfc7231#section-5.3.2. This applies to all requests to the server that expect to receive data back.

This will likely matter more for future pluggable implementations (where we might say something like Accept: wordpress/stats-json)

Addressed in b8f0b8d. As part of that I've made WPNetworkRequest.header_map to be non-optional. I think this Accept header should be included for all requests, since we always want a json response, but let me know if I am missing anything.

jkmassel · 2024-04-10T23:02:57Z

wp_cli/src/main.rs

+        .create_user_request(&user_create_params);
+    let user_create_response = wp_networking.request(user_create_request).unwrap();
+    let created_user =
+        wp_api::parse_retrieve_user_response_with_edit_context(&user_create_response);


I've had a chance to play around with this and I'm not sure that using parse_retrieve_user_response_with_edit_context makes sense for this – if I create a user and the HTTP request succeeds, I'd expect that I'd receive a UserWithEditContext back, not an Option< UserWithEditContext>.

If the HTTP request doesn't succeed, I'd expect an error type to be emitted.

WDYT?

I think this was addressed as part of this in 24fcd6b, but let me know if I am misunderstanding your comment and this requires further action.

jkmassel · 2024-04-10T23:03:43Z

wp_api/src/users.rs

+#[uniffi::export]
+pub fn parse_retrieve_user_response_with_edit_context(
+    response: &WPNetworkResponse,
+) -> Result<Option<UserWithEditContext>, WPApiError> {


Under which circumstances would this method return Ok(None)?

I was thinking when the returned array is empty. However, it makes sense to map that to an error type, so I've updated it in 24fcd6b.

oguzkocer · 2024-04-11T16:02:33Z

I think we need a function to parse DELETE responses. At least, checking if there is any error in the response.

@crazytonyli Although I agree with that, I am not sure what that looks like yet, so I'd like to tackle it in a future PR.

oguzkocer · 2024-04-11T16:04:09Z

I see you are working on integration test using real test sites. Have you thought about adding unit tests?

@crazytonyli I'd like to add unit tests, but for which parts, I am not sure yet. I have a few places in mind, but others will mostly come out of working on the integration tests.

Once this PR is merged in my focus is on testing and error handling. So, happy to discuss details within those PRs.

oguzkocer · 2024-04-11T16:22:11Z

@crazytonyli @jkmassel Thanks a lot for the reviews! 🙇‍♂️ I've addressed or replied to all your comments. Let me know what you think!

oguzkocer added 24 commits April 3, 2024 13:06

An initial and somewhat naive implementation of list users

fe9b8b9

Add the remaining fields to UserListParams

98b30bc

Implements user_retrieve_request

a2c4940

Change UserListParams.slug to be Vec<String>

498fe32

Merge remote-tracking branch 'origin/trunk' into users_types

8765f53

Add UserCreateParams

5b73f8e

Implement create user request

f9bdd0b

Implement user_update_request

4c9046c

Implement user_delete_request

884faea

Add a header_map helper to WPApiHelper

61f962c

Implements UsersEndpoint

3e50d00

Rename user requests to start with verb

f3d2f80

Implements retrieve current user

40559a7

Adds UserId new type

8061a5b

Use UserId for retrieve, update & delete user requests

62cf189

Implements update & delete current user

c604808

Update capabilities & avatar_urls types for SparseUser

798770e

Move parsing code for users to users module

9f6da9c

Extract generic error parsing to a helper

f71b212

Move WPApiParamOrder to wp_api lib.rs

a8aa1b7

Replace Display trait with as_str for WPContext

d3fbcd1

Rename retrieve_current_user as retrieve_current_user_request

d9f5535

Replace Display trait with as_str for WPApiParamOrder

301014e

Remove unused list_posts from wp_networking

858e10f

oguzkocer added Rust Endpoint labels Apr 5, 2024

oguzkocer added this to the 0.1 milestone Apr 5, 2024

oguzkocer requested review from crazytonyli and jkmassel April 5, 2024 17:48

Add body argument to WPNetworkRequest in WordPressAPI.swift

151ffec

oguzkocer mentioned this pull request Apr 8, 2024

Update test_credentials file format to use site_url, username & token #60

Merged

crazytonyli reviewed Apr 9, 2024

View reviewed changes

oguzkocer added 3 commits April 9, 2024 11:13

Add application/json header to WPNetworkRequest for post requests

49ffd28

Prefer from_utf8_lossy over from_utf8

c46d3d8

Replace Display trait with as_str for WPApiParamUsersOrderBy

66508da

crazytonyli requested changes Apr 9, 2024

View reviewed changes

Use references for user request parameters

9656d8c

crazytonyli reviewed Apr 10, 2024

View reviewed changes

jkmassel reviewed Apr 10, 2024

View reviewed changes

crazytonyli mentioned this pull request Apr 11, 2024

Implement users endpoints in Swift #62

Closed

oguzkocer added 5 commits April 11, 2024 11:16

Refactor UserListParams.query_pairs() to be more readable

6c9fe91

Remove the Option from parse retrieve user results

24fcd6b

update_current_user_request should use post request header map

618e099

Add 'Accept' header to all WPRequests

b8f0b8d

Update breaking changes in Swift for headerMap becoming non-optional

2023d8d

crazytonyli approved these changes Apr 11, 2024

View reviewed changes

oguzkocer merged commit d875f56 into trunk Apr 12, 2024
19 checks passed

oguzkocer deleted the users_types branch April 12, 2024 17:56

This was referenced Apr 16, 2024

List Users #17

Closed

Update a User #19

Closed

Delete a User #20

Closed

Retrieve a User #18

Closed

Users endpoint #58

Users endpoint #58

Conversation

oguzkocer commented Apr 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oguzkocer commented Apr 9, 2024

crazytonyli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazytonyli commented Apr 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkmassel Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oguzkocer commented Apr 11, 2024

oguzkocer commented Apr 11, 2024

oguzkocer commented Apr 11, 2024

jkmassel Apr 10, 2024 •

edited

Loading