Skip to content

Conversation

oguzkocer
Copy link
Contributor

This PR proposes an initial design for the /posts endpoint of WordPress.org API: https://developer.wordpress.org/rest-api/reference/posts/. It's the result of investigating several approaches in a time-boxed manner and it is not a final design.

P.S: There is no PR review process for this repository yet, because it's all part of a prototype. So, I'll merge this PR without waiting for feedback. However, if you have any feedback, I'd love to hear about it!


The first design approach we have explored was to use a Builder pattern for the request parameters. It's a common design pattern for Rust and it's something developers from most programming languages are familiar with - which is important for a cross-platform project. It's also an ergonomic design when there are a lot of optional types.

The main issue we found with Builder pattern, in the context of using it in FFI layer, is that we can't have a mutable self type. For example, the following code doesn't work through FFI layer - because it's not safe:

impl Counter {
    // No mutable references to self allowed in UniFFI interfaces.
    fn increment(&mut self) {
        self.value = self.value + 1;
    }
}

Similarly, we can't use the following approach because we can't force the accessor to give up ownership of the memory through FFI layer - not without custom code anyway:

struct FooBuilder {
    bar: String,
}

impl FooBuilder {
    fn name(mut self, bar: String) -> Self {
        // Set the name on the builder itself, and return the builder by value.
        self.bar = bar;
        self
    }
}

We have the option to use interior mutability, so for example, we could do:

struct FooBuilder {
    bar: RwLock<String>,
}

impl FooBuilder {
    fn name(&self, bar: String) {
        *self.bar.write().unwrap() = bar;
    }
}

However, notice the method signature and specifically the return type. We are no longer returning Self because we only have a reference to the memory. That means we can't chain commands such as self.bar().baz() which arguably is one of the primary reasons to utilize a Builder pattern.


From what we have learned through the Builder pattern exploration, the design in this PR made sense to us as an initial approach.

The primary components that were supposed to utilize the Builder pattern are *Params objects such as PostsListParams. The reason we thought that this design made sense as an alternative is that we'd need these components even if we used a Builder pattern because when we call .build() on a Builder pattern object, it's supposed to return an object such as PostsListParams anyway. That's all to say that, if we really want to use the Builder pattern in each native layer, this design will accommodate that. And in general, it's a flexible design that's meant to be built on through higher layers.

One last note is that how we are going to make requests is not investigated at all in this PR. So, the PostsRequestBuilder methods are returning a PostsRequest is mostly a placeholder at this stage.

@oguzkocer oguzkocer merged commit 6362026 into trunk Nov 28, 2023
@jkmassel jkmassel mentioned this pull request Nov 29, 2023
@oguzkocer oguzkocer deleted the post_request_scaffolding branch November 29, 2023 12:22
//
// The reason we can use a `PostsRequest` type and not need a more specific type such as
// `PostsListRequest` is because the API documentation states that the return value for
// all `/posts` requests have the same schema: https://developer.wordpress.org/rest-api/reference/posts/#schema
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, this isn't quite true – the schema depends on the context, so view will return different fields than embed

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 think that's the case regardless of the action type (list, create etc), right? The documentation specifically says that:

The schema defines all the fields that exist within a post record. Any response from these endpoints can be expected to contain the fields below unless the _filter query parameter is used or the schema field only appears in a specific context.

That's where I was going from. However, the context bit is going to be a difficult problem for us to solve. I've actually left a TODO about that here.


I've been thinking about this after opening the PR, and regardless of the result of this discussion, we are probably going to end up with specific request types.

u32? per_page;
};

dictionary PostsRetrieveParams {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, but retrieve, update, and delete operate on a single object. Should we use non-plural nouns for these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had a discussion about this on Zoom today, but just for completeness sake, the reason I used Posts was to have a logical naming scheme which uses the endpoint as the first word of the type. However, I think we can always use the singular version of that and still have a logical naming scheme.

pub force: Option<bool>,
}

pub struct PostsRequest {
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 need an HTTP method member here too.

I assume we'll give this to a higher-level RestApi object that will hold the hostname, http protocol, etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeap, lots of fields are missing from this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants