[PR #186] [MERGED] Separate HTTP client from the Spotify client #296

New issue

Closed

opened 2026-02-27 20:24:09 +03:00 by kerem · 0 comments

kerem commented

2026-02-27 20:24:09 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/ramsayleung/rspotify/pull/186
Author: @marioortizmanero
Created: 2/16/2021
Status: ✅ Merged
Merged: 2/20/2021
Merged by: @ramsayleung

Base: master ← Head: separate-http

📝 Commits (8)

5c9a000 Some initial work
8195f8d documentation
824e3d3 fix doc
d5f4c93 finish reqwest
20d47d1 compiling correctly now
284398d ureq working
5e3fa6e inline endpoint_get
ddc9a19 Merge branch 'master' into separate-http

📊 Changes

5 files changed (+260 additions, -139 deletions)

View changed files

📝 src/client.rs (+84 -81)
📝 src/http/mod.rs (+129 -11)
📝 src/http/reqwest.rs (+28 -25)
📝 src/http/ureq.rs (+18 -21)
📝 src/oauth2.rs (+1 -1)

📄 Description

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 #127).

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/ramsayleung/rspotify/pull/186 **Author:** [@marioortizmanero](https://github.com/marioortizmanero) **Created:** 2/16/2021 **Status:** ✅ Merged **Merged:** 2/20/2021 **Merged by:** [@ramsayleung](https://github.com/ramsayleung) **Base:** `master` ← **Head:** `separate-http` --- ### 📝 Commits (8) - [`5c9a000`](https://github.com/ramsayleung/rspotify/commit/5c9a000cda550284840660ec18ce85c2aa62da65) Some initial work - [`8195f8d`](https://github.com/ramsayleung/rspotify/commit/8195f8d60f862cd06b47ac808b7c8ea7f3f8268c) documentation - [`824e3d3`](https://github.com/ramsayleung/rspotify/commit/824e3d3c8cd0820c78063c0d7393f4f7b7983cdd) fix doc - [`d5f4c93`](https://github.com/ramsayleung/rspotify/commit/d5f4c9382d1b211cc4ca868bbdbc283d4473b9f0) finish reqwest - [`20d47d1`](https://github.com/ramsayleung/rspotify/commit/20d47d11b208262cb15e882816a2968f276a0d36) compiling correctly now - [`284398d`](https://github.com/ramsayleung/rspotify/commit/284398d7607a5991b4c90f65bc1184d63b326f3a) ureq working - [`5e3fa6e`](https://github.com/ramsayleung/rspotify/commit/5e3fa6e22068332406fea8755b8c852fd0f0c909) inline endpoint_get - [`ddc9a19`](https://github.com/ramsayleung/rspotify/commit/ddc9a1917089059cb31c6f6de8708f51cefc223c) Merge branch 'master' into separate-http ### 📊 Changes **5 files changed** (+260 additions, -139 deletions) <details> <summary>View changed files</summary> 📝 `src/client.rs` (+84 -81) 📝 `src/http/mod.rs` (+129 -11) 📝 `src/http/reqwest.rs` (+28 -25) 📝 `src/http/ureq.rs` (+18 -21) 📝 `src/oauth2.rs` (+1 -1) </details> ### 📄 Description 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: ```rust /// 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: ```rust /// 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: ```rust 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 #127). --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>