[PR #258] [MERGED] HTTP Error cleanup #352

New issue

Closed

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

kerem commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/ramsayleung/rspotify/pull/258
Author: @marioortizmanero
Created: 9/25/2021
Status: ✅ Merged
Merged: 10/8/2021
Merged by: @ramsayleung

Base: master ← Head: errors

📝 Commits (10+)

9bcd5b7 Improved error handling
80d9e3f Second approach
de330d6 Use full names for errors
f8b7fbc Small fixes
e97ec70 Even better thanks to associated types!!
ad74f81 Fix main crate as well
d58498a Fix clippy lints about map_err
467e039 Actually fix clippy lint
d3335d3 More idiomatic return
f7fa8cf Simplify map_err

📊 Changes

6 files changed (+151 additions, -95 deletions)

View changed files

📝 rspotify-http/Cargo.toml (+4 -2)
📝 rspotify-http/src/common.rs (+7 -29)
📝 rspotify-http/src/lib.rs (+3 -3)
📝 rspotify-http/src/reqwest.rs (+50 -44)
📝 rspotify-http/src/ureq.rs (+76 -16)
📝 src/lib.rs (+11 -1)

📄 Description

Description

This cleans up the errors for rspotify-http, according to #137.

I would say the rest of the error types are OK, so this closes #137.

Motivation and Context

The current error type for http tries to wrap up all the errors from the clients we support under the same interface. However, it's a mess because:

It mixes multiple kinds of errors for different clients; HttpError::Io is only needed for ureq
It serializes only specific kinds of errors. Why is there a variant for RateLimited or Unauthorized but it's StatusCode for the rest?
Aren't the Api and StatusCode variants the same?

So this PR attempts to fix it by simply having a variant of error for each client supported by rspotify-http. It's as easy as delegating error handling to the HTTP client. If the user has enabled ureq, then they should deal with ureq::Error instead of some wrapper of our own that might not have everything they need.

This way, error handling is complete. There is nothing of functionality we miss by wrapping all of them under the same interface. And we also don't have to maintain such a complex wrapper ourselves.

I remember that @hrkfdn pointed out that the RateLimited variant was a must for him in order to retry a request. Well, this design still makes it possible to do so. It's just dependent on what HTTP client he's using. If he uses reqwest then it should be something like:

match request(...) {
    ClientError::Http(HttpError::StatusCode(response)) => {
        // This is a reqwest::Response with full information about the error
        let timeout = response
            .headers()
            .get(reqwest::header::RETRY_AFTER)
            .and_then(|header| header.to_str().ok())
            .and_then(|duration| duration.parse().ok());
        time::sleep(timeout);
    }
    _ => { todo!() }
}

And if someone wanted to get the API error from Spotify:

match request(...) {
    ClientError::Http(HttpError::StatusCode(response)) => {
        let api_err = response
            .json::<rspotify::model::ApiError>()?;
    }
    _ => { todo!() }
}

Dependencies

None

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

The CI still passes

_{🔄 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/258 **Author:** [@marioortizmanero](https://github.com/marioortizmanero) **Created:** 9/25/2021 **Status:** ✅ Merged **Merged:** 10/8/2021 **Merged by:** [@ramsayleung](https://github.com/ramsayleung) **Base:** `master` ← **Head:** `errors` --- ### 📝 Commits (10+) - [`9bcd5b7`](https://github.com/ramsayleung/rspotify/commit/9bcd5b7e1035831466f14cfcfa7594e16b6f3dc8) Improved error handling - [`80d9e3f`](https://github.com/ramsayleung/rspotify/commit/80d9e3fde414455835e59152be72a7d5f86e8348) Second approach - [`de330d6`](https://github.com/ramsayleung/rspotify/commit/de330d642fbcc59ab617b8598567e576f8c175fd) Use full names for errors - [`f8b7fbc`](https://github.com/ramsayleung/rspotify/commit/f8b7fbcbaec4ed0f24092b7a469e11040068b228) Small fixes - [`e97ec70`](https://github.com/ramsayleung/rspotify/commit/e97ec70d378269ea8fe5f321b99b38ecdc78def6) Even better thanks to associated types!! - [`ad74f81`](https://github.com/ramsayleung/rspotify/commit/ad74f81bd08a5384143ff092c76d60dc9b89a5da) Fix main crate as well - [`d58498a`](https://github.com/ramsayleung/rspotify/commit/d58498a574f84887db46bebdb0590b18368092b2) Fix clippy lints about map_err - [`467e039`](https://github.com/ramsayleung/rspotify/commit/467e0392dcd1cbd24920c10c7adeed43aa0808b6) Actually fix clippy lint - [`d3335d3`](https://github.com/ramsayleung/rspotify/commit/d3335d3d2c28221c151f837d329c91064cbee736) More idiomatic return - [`f7fa8cf`](https://github.com/ramsayleung/rspotify/commit/f7fa8cf20a9bfe039742b60d04bad5a0cee7008c) Simplify map_err ### 📊 Changes **6 files changed** (+151 additions, -95 deletions) <details> <summary>View changed files</summary> 📝 `rspotify-http/Cargo.toml` (+4 -2) 📝 `rspotify-http/src/common.rs` (+7 -29) 📝 `rspotify-http/src/lib.rs` (+3 -3) 📝 `rspotify-http/src/reqwest.rs` (+50 -44) 📝 `rspotify-http/src/ureq.rs` (+76 -16) 📝 `src/lib.rs` (+11 -1) </details> ### 📄 Description ## Description This cleans up the errors for `rspotify-http`, according to #137. I would say the rest of the error types are OK, so this closes #137. ## Motivation and Context The [current error type for http](https://github.com/ramsayleung/rspotify/blob/bcfb7842aae7d2a0b8c82c53966efacc7e1f3966/rspotify-http/src/common.rs#L13) tries to wrap up all the errors from the clients we support under the same interface. However, it's a mess because: 1. It mixes multiple kinds of errors for different clients; `HttpError::Io` is only needed for ureq 2. It serializes only specific kinds of errors. Why is there a variant for `RateLimited` or `Unauthorized` but it's `StatusCode` for the rest? 3. Aren't the `Api` and `StatusCode` variants the same? So this PR attempts to fix it by simply having a variant of error for each client supported by `rspotify-http`. It's as easy as delegating error handling to the HTTP client. If the user has enabled `ureq`, then they should deal with `ureq::Error` instead of some wrapper of our own that might not have everything they need. This way, error handling is *complete*. There is nothing of functionality we miss by wrapping all of them under the same interface. And we also don't have to maintain such a complex wrapper ourselves. I remember that @hrkfdn pointed out that the `RateLimited` variant was a must for him in order to retry a request. Well, this design still makes it possible to do so. It's just dependent on what HTTP client he's using. If he uses reqwest then it should be something like: ```rust match request(...) { ClientError::Http(HttpError::StatusCode(response)) => { // This is a reqwest::Response with full information about the error let timeout = response .headers() .get(reqwest::header::RETRY_AFTER) .and_then(|header| header.to_str().ok()) .and_then(|duration| duration.parse().ok()); time::sleep(timeout); } _ => { todo!() } } ``` And if someone wanted to get the API error from Spotify: ```rust match request(...) { ClientError::Http(HttpError::StatusCode(response)) => { let api_err = response .json::<rspotify::model::ApiError>()?; } _ => { todo!() } } ``` ## Dependencies None ## Type of change Please delete options that are not relevant. - [X] Bug fix (non-breaking change which fixes an issue) - [X] New feature (non-breaking change which adds functionality) - [X] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [X] This change requires a documentation update ## How Has This Been Tested? The CI still passes --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>