twitch_api icon indicating copy to clipboard operation
twitch_api copied to clipboard

Factor out client and request abstraction to external crate

Open simonsan opened this issue 4 years ago • 9 comments

Hey there, I love your really nice and clean work on the client abstraction with possible multiple clients to use and traits for Request, RequestGet etc.

I'm writing a backend that makes requests to multiple different APIs and I'm planning to use your client and request abstractions for this while only implementing the request traits for these APIs.

What came to my mind was, if it wouldn't be nice, to publish the client and request traits in an external crate as api-client, so people would just need to implement the request traits for the corresponding API and could use any client they want to pull the data.

Do you think this is a reasonable thing to do? I think this could be really valuable for the community, as that would make implementing crates for all kinds of APIs much easier. I could also imagine (long-term) porting the outdated kraken Twitch API crate to this foundation (and PR it here), while we could publish the whole thing then under https://crates.io/crates/twitch_api where I would like to add you as an owner as well. Would be lovely to have a wholesome crate of the Twitch API (as you are making your way to it).

simonsan avatar Mar 16 '21 18:03 simonsan

As an addition: I think what could be done as well then is to create a macro which is generating the request implementations for different endpoints or other tooling to generate the request implementation from an OpenAPI file.

simonsan avatar Mar 16 '21 18:03 simonsan

I'd be willing to do it, but I'm not sure I should. My inspiration for the abstraction came from the oauth2 crate, watching museun code multiple client abstractions for twitchchat, and the implementation in http-client. Currently there is to my knowledge one attempt at doing client abstractions for http, and that's http-client, but, that one doesn't exactly do what I'm doing. There is also a stale api crate which could be promising for the "request" parth, but it could do with some more attention.

http-client is nice, but it does not use http (for some good reasons), you could use that for client abstraction, but you'd need to use http-types.

There is one problem also. If I define these implementations in another crate for usage by other crates, making a default impl on create_request and others would be impossible for those crates. I guess I could make it work, not sure how.

Instead, I think it'd be better to expose the framework by usage of proc macros. I'm not sure I like the idea though.

So, I think, best course of action is a crate that enables you to do basically the following

use api_client::ApiClient;

#[derive(ApiClient)] /// Adds relevant methods
#[api(mod = "helix_request")]
struct<'client> HelixClient<'client, C> where C: api_client::ApiClient<'client> {
    #[api(client)]
    client: C,
    pub custom_field: (),
}

pub mod helix_request {
    pub trait RequestGet: api_client::Request {
        type Body: serde::Serialize;
        fn create_request(fields...) -> Result<api_client::Request, MyCreationError> {...}
        fn parse_response<'a>(response: api_client::Response<'a>) -> Result<<Self as Request>::Response<'a>, MyResponseError<'a>> {...} 
    }
    //... etc etc
}

pub mod foo_api_thing {
    pub struct FooRequest {
        pub foo: String,
    }
    impl api_client::Request for Foo {
        // something here
    }
    impl crate::helix_request::RequestGet for FooRequest {
        // something here
    }
}

I'm not sure it's possible, but it'd be neat I think.

Emilgardis avatar Mar 17 '21 16:03 Emilgardis

Good idea, yeah that looks nice. I already ported my code for the backend to yours and it works out nicely. Only thing I had to change is the parse_response part as I have a pretty unstable API and need to parse directly into serde_json::Value and not InnerResponse or alike. But yes, absolute lovely stuff, great work. :)

simonsan avatar Mar 18 '21 06:03 simonsan

Awesome! Could I perhaps see the source for that? No worries if its not publicly available

Emilgardis avatar Mar 18 '21 08:03 Emilgardis

Awesome! Could I perhaps see the source for that? No worries if its not publicly available

Sure, but beware of the code quality, I'm not a professional Rust programmer :D (and the crates are also not properly documented for the use case as I did the refactoring to the client/request abstraction just the last days) https://github.com/transparencies/transparencies-backend-rs/tree/main/crates

simonsan avatar Mar 18 '21 12:03 simonsan

Small cross-reference

Someone posted this article on reddit that reminded me of this issue: https://plume.benboeckel.net/~/JustAnotherBlog/designing-rust-bindings-for-rest-ap-is

The corresponding reddit thread with discussions is this one: https://www.reddit.com/r/rust/comments/r3txac/questions_on_ergonomically_wrapping_a_rest_api/

The corresponding discussion about factoring out the needed part of rust-gitlab into an API-agnostic crate is here: https://gitlab.kitware.com/utils/rust-gitlab/-/issues/56

simonsan avatar Nov 29 '21 23:11 simonsan

Yeah i saw that! Still havent given it a proper look yet. This issue is still on my todo list though 😅

Emilgardis avatar Nov 29 '21 23:11 Emilgardis

Yeah i saw that! Still havent given it a proper look yet. This issue is still on my todo list though 😅

No stress, but I do like your design really much, so I recommend it. (: rust-gitlab has a slightly different design that is also nice. I haven't looked so deep into it, that I could say much more about it. But I would love to bring this effort forward, that there might be some kind of well-designed api-agnostic REST-API crate for the community. And I think it's always good to work together. :)

simonsan avatar Nov 29 '21 23:11 simonsan

@Emilgardis I created a temporary private repository from the effort to outsource the implementation into an external crate and invited you to collaborate. Would you take a look?

simonsan avatar Nov 30 '21 18:11 simonsan