search

dscottboggs / mastodon-async

Mastodon Client for Rust
Other
34 stars 12 forks source link

Comb-through A.K.A. v2 #54

Open dscottboggs opened 1 year ago

dscottboggs commented 1 year ago

It has come to my attention that the Mastodon API has been updated a lot since the original work this was based on, and we need to comb through the API documentation ensuring that each type is implemented according to the spec.

When this is released, we will cut a new v2.0 release. Breaking changes will be a part of this.

Tasks for this PR

Comb through entities

Entities, second pass

Comb through methods

Finishing up/other tasks

baummarten commented 1 year ago

What do you prefer to do do with URLs that accept multiple optional parameters (like /api/v1/timelines/public), just add all of them with an Option type, or maybe some kind of a builder pattern?

dscottboggs commented 1 year ago

The previous authors seemed to be fans of using the builder pattern to generate structures which use serde of serialization into a urlencoded string, and I think it fits nicely. There are a couple changes I'm making to the way they did that as a part of v2:

  1. I'd like to use the derive_builder crate
  2. (and I wouldn't mind some criticism of this point) I'd like to include default values rather than use option types where it makes sense. For example, the public timelines type would be like 
    #[derive(Builder, Default, Serialize, Deserialize, Clone)]
pub struct TimelineQuery {
   #[builder(default)]
   pub local: bool,
   #[builder(default)]
   pub remote: bool,
   #[builder(default)]
   pub only_media: bool,
   #[builder(setter(into, strip_option))]
   pub max_id: Option<String>,
   #[builder(setter(into, strip_option))]
   pub since_id: Option<String>,
   #[builder(setter(into, strip_option))]
   pub min_id: Option<String>,
   #[builder(setter(into, strip_option))]
   pub limit: u8,
}

    rather than

    #[derive(Builder, Default, Serialize, Deserialize, Clone)]
pub struct TimelineQuery {
   #[builder(setter(strip_option))]
   pub local: Option<bool>,
   #[builder(setter(strip_option))]
   pub remote: Option<bool>,
   #[builder(setter(strip_option))]
   pub only_media: Option<bool>,
   #[builder(setter(into, strip_option))]
   pub max_id: Option<String>,
   #[builder(setter(into, strip_option))]
   pub since_id: Option<String>,
   #[builder(setter(into, strip_option))]
   pub min_id: Option<String>,
   #[builder(setter(into, strip_option))]
   pub limit: Option<u8>,
}

    The idea is basically that unnecessary options are un-ergonomic, so anywhere where a default is specified and unlikely to change, make it the default here.

  3. All fields should be public. These types may be useful to consumers of the library in other ways, and this also allows constructing the types without using the builder pattern if someone finds it inconvenient.

Another thought I have on this is that perhaps it would be a good idea to take a look at this section of the paged_routes! macro definition and its corresponding invokation to see if we can draw inspiration for making the way we do this more DRY.

Any thoughts?

baummarten commented 1 year ago

(1) Sure. (2) If there's an obvious default then yeah, no need for an Option. The only disadvantage I see that there's no way to leave it unset, but since the default for booleans are given in the API description (and Eugen has gone on record that existing API won't be messed with), then the logic could be "if value=default then don't set anything", if that ever matters. (3) That also makes sense, I don't think types for API queries could possibly have any invariants that need to be protected.

VyrCossont commented 1 year ago

Any way I can help out here? Is the comb branch where all of this work exists currently?

Would love to have this; the admin API will be critical for building automod tools for Mastodon, and I ended up implementing most of it in https://github.com/VyrCossont/mastodon-async/tree/admin-api before noticing this issue.

dscottboggs commented 1 year ago

Hello, yes, I am coming back to this soon but a couple of more urgent projects have come up, I should be wrapping them up soon (at least the urgent part). I really appreciate your work on the admin API, however, as you noted there may be some things which are already done. By all means, create a pull request (targeting the comb branch) and let's take a look at what needs done to get that merged.

VyrCossont commented 1 year ago

Right on. I'll take a look at the comb branch and see if anything from my admin-api branch is reusable or if there's other work I can easily pick up.

VyrCossont commented 1 year ago

Also, my take on builders and defaults is that, while unnecessary options are un-ergonomic:

  1. #[builder(setter(strip_option))] makes this a non-issue from the builder's caller's perspective.
  2. I'd rather not send parameters that aren't explicitly set. This makes the requests smaller and easier to read when debugging.
  3. We're going to be dealing with at least 4 families of implementation of the Mastodon API: Mastodon/Glitch/Hometown, Pleroma/Akkoma, GotoSocial, and now Calckey (a Misskey fork), plus there's a pretty broad version spread of deployed instances of all of these. I think we should be conservative in what we send, in case one of them chokes on a parameter that it hasn't implemented yet.
VyrCossont commented 1 year ago

I tried implementing a builder with some mandatory and some optional parameters using derive_builder and serde_with:

use derive_builder::Builder;
use mastodon_async_entities::{report::Category, AccountId, RuleId, StatusId};
use serde_with::skip_serializing_none;

/// Form used to create a report
///
/// // Example
///
/// ```
/// use mastodon_async::{entities::{AccountId, report::Category}, requests::AddReportRequest};
/// let request = AddReportRequest::builder(AccountId::new("666")).category(Category::Spam).build();
/// ```
#[skip_serializing_none]
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Builder)]
#[builder(
    custom_constructor,
    build_fn(private, name = "try_build"),
    setter(into, strip_option)
)]
pub struct AddReportRequest {
    /// The account being reported.
    #[builder(private)]
    pub account_id: AccountId,
    /// Attach statuses to the report to provide additional context.
    #[builder(default)]
    pub status_ids: Option<Vec<StatusId>>,
    /// The reason for the report.
    #[builder(default)]
    pub comment: Option<String>,
    /// If the account is remote, should the report be forwarded to the remote admin?
    #[builder(default)]
    pub forward: Option<bool>,
    /// Machine-readable category of the report.
    #[builder(default)]
    pub category: Option<Category>,
    /// Rules broken by a [`Category::Violation`] report.
    #[builder(default)]
    pub rule_ids: Option<Vec<RuleId>>,
}

impl AddReportRequest {
    /// Start building a form for creating a report.
    pub fn builder(account_id: AccountId) -> AddReportRequestBuilder {
        let mut builder = AddReportRequestBuilder::create_empty();
        builder.account_id(account_id);
        builder
    }
}

impl AddReportRequestBuilder {
    /// Build the form for creating a report.
    pub fn build(&self) -> AddReportRequest {
        self.try_build()
            .expect("One or more required fields are missing!")
    }
}

#[cfg(test)]
mod tests {
    use super::*;
    use serde_json;

    #[test]
    fn test_serialize_request() {
        let request = AddReportRequest::builder(AccountId::new("666"))
            .category(Category::Spam)
            .build();
        let ser = serde_json::to_string(&request).expect("Couldn't serialize");
        assert_eq!(ser, r#"{"account_id":"666","category":"spam"}"#);
    }
}

This gives the caller two options, both safe: they can create an AddReportRequest directly and the type system forces them to provide mandatory params (as well as None or Default::default() for each optional param), or they can call AddReportRequest::builder(account_id), which takes the mandatory params, call builder methods for any optional params, and then .build().

The boilerplate cost of this is a #[builder(default)] annotation on every optional param and a #[builder(private)] annotation on every mandatory param (assuming we want to hide methods that are redundant with the ::builder(…) method that takes mandatory params), plus the struct-level #[skip_serializing_none] and #[builder(…)] annotations and the ::builder(…) and .build() methods. We can probably improve on this, but it doesn't seem horrible as is, especially compared to how many lines we can drop that are redundant handwritten builder methods and tests for errors that won't be able to happen any more.

dscottboggs commented 1 year ago

Looks good to me.

dscottboggs commented 1 year ago

@VyrCossont you may want to review the changes in this commit https://github.com/dscottboggs/mastodon-async/pull/91/commits/035637f21f714e4cd84027c05b4c2eedb94cfaef

VyrCossont commented 1 year ago

Filled in the reporting and admin APIs here: https://github.com/VyrCossont/mastodon-async/tree/comb

The above builder pattern is pretty much fine, I think. I'm going to convert the older builders to that style where it makes sense.

dscottboggs commented 1 year ago

This is great! Would you mind creating a PR for this?

VyrCossont commented 1 year ago

Sure, I'll open a draft and let you know once it's ready for review.

VyrCossont commented 1 year ago

Draft in #93. I'm concurrently working on an automod bot that uses mastodon-async, so the admin APIs I've implemented should get a workout, as well as the status creation API.