Separate HTTP client from the Spotify client

[marioortizmanero]

So I've come to the conclusion that the HTTP client should be separate from the Spotify client, and be kept as one of its members instead. This completely separates the logic from the HTTP abstraction with the Spotify client and makes the code more modular and cleaner.

This means that, to implement the get, post, and similar methods inside the http-specific modules (like http:reqwest, http::ureq), instead of doing impl Spotify directly, the implementation is for ReqwestClient or UreqClient. The latter is then exported as HTTPClient so that it can be used to perform requests. All these clients must implement the same trait as before, BaseClient, which is now renamed as BaseHTTPClient. This is just to make sure the interface exported by ureq and reqwest is the same.

What's now improved:

• No need for this part in the Spotify client, which was hardcoded to be used later on only on the http module, a quite bad practice:

  /// Spotify API object
  #[derive(Builder, Debug, Clone)]
  pub struct Spotify {
      /// reqwest needs an instance of its client to perform requests.
      #[cfg(feature = "client-reqwest")]
      #[builder(setter(skip))]
      pub(in crate) client: reqwest::Client,
      ...

  This is now a generic HTTP struct, which is always included, be it reqwest or ureq. For the latter it will be empty because it doesn't really require an instance of its client, but it avoids the conditional compilation of this field:

  /// Spotify API object
  #[derive(Builder, Debug, Clone)]
  pub struct Spotify {
      /// Internal member to perform requests to the Spotify API.
      #[builder(setter(skip))]
      pub(in crate) http: HTTPClient,

• Before, the reqwest and ureq implementations of the HTTP features were tied to what we needed to perform HTTP requests:

  let url = self.endpoint_url(url);
  
  // The default auth headers are used if none were specified.
  let mut auth;
  let headers = match headers {
      Some(h) => h,
      None => {
          auth = Headers::new();
          let (key, val) = headers::bearer_auth(self.get_token()?);
          auth.insert(key, val);
          &auth
      }
  };

  Now, the HTTP client doesn't know about the authentication process required for Spotify requests, and this has to be implemented in the Spotify client instead. Thus, this part is no longer repeated for each HTTP implementation (ureq, reqwest).

What's worse:

• This requires implementing wrappers over the HTTP clients. We currently need two different wrappers:

  • Basic ones like get, post, and similars, which only append the Spotify url to the relative one. These are used for the authentication process, to request the token and similars.
  • Endpoint wrappers, which also include the authentication headers over the basic wrappers.

  These wrappers aren't really necessary, but they guarantee we aren't forgetting to append the spotify url or the auth headers, while also reducing the endpoint boilerplate required (a goal in Meta-Issue #127).

[marioortizmanero]

This is ready for discussion and review now

[ramsayleung]

Great job! LGTM, there is a small nitpick that is there any way to reduce the two kinds of wrapper functions or combine them into single wrapper function internally? it seems a little bit redundancy.

[marioortizmanero]

Great job! LGTM, there is a small nitpick that is there any way to reduce the two kinds of wrapper functions or combine them into single wrapper function internally? it seems a little bit redundancy.

Unfortunately no. We need wrappers for two different reasons: to request auth tokens and similars, and to perform a request to the Spotify API with said tokens. So the former can't include the auth header and only needs its spotify prefix appended, and the latter needs both the prefix and the header.

aspotify solves this by always including a macro that appends this prefix when the request is performed, like so: get(endpoint!("/me"), ...) but our prefix is a variable so it can't fit in a macro, and we might as well reduce the repetitiveness and possibility of forgetting it.

[ramsayleung]

Could this function mark as inline function as the below functions?

[marioortizmanero]

Not sure if this one should be inline. I only added that for wrapper functions, but this is an actual function. It contains more code than the wrappers and would be repeated a lot of times by being inline. Just like how endpoint_url isn't inline either.

[ramsayleung]

ok, I get it.