Skip to content

Conversation

oguzkocer
Copy link
Contributor

This PR is a step in organizing WPApiHelper. It introduces a RequestBuilder type that takes an Authentication and provides helpers for get, post & delete requests. Using this RequestBuilder, UsersRequestBuilder & PluginsRequestBuilder encapsulates the logic that was handled by WPApiHelper.

It removes the ApiEndpoint type as we no longer need an encapsulating type for all endpoints. Each request builder will only work with its own endpoint implementation. Furthermore, the api_base_url field for these endpoints have been changed to an Arc<ApiBaseUrl> so that we can have a single instance of it and not have to clone the whole thing everywhere.

Public API for WPApiHelper was not altered for this PR so that it's not a breaking change for our clients. I'd like to do the breaking changes in its own PR to make it easier to follow the changes.


To Test

  • make test-server && make dump-mysql && make backup-wp-content-plugins
  • cargo test --test '*' -- --nocapture --test-threads 1

Copy link
Contributor

@jkmassel jkmassel left a comment

Choose a reason for hiding this comment

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

Makes sense so far – one thing I wanted to leave in a public channel though for future reference.

Why these abstractions vs just having a very log lib.rs with 1000 repeated versions of this?

        WPNetworkRequest {
            method: RequestMethod::POST,
            url: self.api_endpoint.users.create().into(),
            header_map: self.header_map_for_post_request(),
            body: serde_json::to_vec(&params).ok(),
        }

api_endpoint: ApiEndpoint,
authentication: WPAuthentication,
request_builder: Arc<RequestBuilder>,
users_request: UsersRequestBuilder,
Copy link
Contributor

Choose a reason for hiding this comment

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

users_request is a bit of an awkward name, but I'm assume it's to make things nice for a caller like:

helper.users_request.create()

If not, we might want to consider just calling it users_request_builder, as lengthy as that is? WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to add the _builder suffix after fe13c62. I'll do that in the next iteration of WPApiHelper.

let url = Url::parse(site_url.as_str()).unwrap();
// TODO: Handle the url parse error
let api_endpoint = ApiEndpoint::new_from_str(site_url.as_str()).unwrap();
let api_base_url = Arc::new(ApiBaseUrl::new(site_url.as_str()).unwrap());
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about using Arc<T> for all of these vs just using .clone() (which is IMHO simpler because we pay all the costs upfront), but given the fact that all our FFI already uses Arc<T>, we can defer this and adjust if needed – callers shouldn't be affected.

@oguzkocer
Copy link
Contributor Author

Why these abstractions vs just having a very log lib.rs with 1000 repeated versions of this?

This is a bit difficult to handle at the moment, as I don't know what our final design will look like. I think RequestBuilder is a common abstraction to easily make get, post, delete requests. The endpoint generators are also common, although some libraries prefer to do this by generating it from a text file or similar.

The RequestBuilder variants such as UsersRequestBuilder & PluginsRequestBuilder are not strictly necessary, and for now, only abstracted for organizational reasons. What I'd like to do before we commit in either direction is to see if we can generate these request builders from the Params types with a proc macro. If we can't, then we can decide whether the abstraction or a longer implementation is better. I am fine with it either way.

@oguzkocer oguzkocer merged commit 08a551c into trunk May 24, 2024
@oguzkocer oguzkocer deleted the request-builder branch May 24, 2024 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants