Skip to content

Commit

Permalink
Make HTTP error more useful (serenity-rs#2277)
Browse files Browse the repository at this point in the history
While developing the automod regex PR, I noticed that HTTP errors include lots
of useless info about the URL, but don't even include the HTTP method
(POST/GET/PATCH/...). This commit changes that.

Old: `Http(UnsuccessfulRequest(ErrorResponse { status_code: 403, url: Url { scheme: "https", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("discord.com")), port: None, path: "/api/v10/guilds/703332075914264606/auto-moderation/rules", query: None, fragment: None }, error: DiscordJsonError { code: 50001, message: "Missing Access", errors: [] } }))`

New: `Http(UnsuccessfulRequest(ErrorResponse { status_code: 403, url: "https://discord.com/api/v10/guilds/703332075914264606/auto-moderation/rules", method: POST, error: DiscordJsonError { code: 50001, message: "Missing Access", errors: [] } }))`
  • Loading branch information
kangalio authored and mkrasnitski committed Feb 28, 2023
1 parent a774195 commit 50cb56d
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 15 deletions.
12 changes: 9 additions & 3 deletions src/http/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use super::ratelimiting::{RatelimitedRequest, Ratelimiter};
use super::request::Request;
use super::routing::RouteInfo;
use super::typing::Typing;
use super::{GuildPagination, HttpError, UserPagination};
use super::{ErrorResponse, GuildPagination, HttpError, UserPagination};
use crate::builder::CreateAttachment;
use crate::constants;
use crate::internal::prelude::*;
Expand Down Expand Up @@ -4046,6 +4046,7 @@ impl Http {
/// ```
#[instrument]
pub async fn request(&self, req: Request<'_>) -> Result<ReqwestResponse> {
let method = req.route.deconstruct().0;
let response = if self.ratelimiter_disabled {
let request = req.build(&self.client, &self.token, self.proxy.as_ref())?.build()?;
self.client.execute(request).await?
Expand All @@ -4057,7 +4058,9 @@ impl Http {
if response.status().is_success() {
Ok(response)
} else {
Err(Error::Http(HttpError::from_response(response).await))
Err(Error::Http(HttpError::UnsuccessfulRequest(
ErrorResponse::from_response(response, method.reqwest_method()).await,
)))
}
}

Expand All @@ -4067,6 +4070,7 @@ impl Http {
/// This is a function that performs a light amount of work and returns an
/// empty tuple, so it's called "self.wind" to denote that it's lightweight.
pub(super) async fn wind(&self, expected: u16, req: Request<'_>) -> Result<()> {
let method = req.route.deconstruct().0;
let response = self.request(req).await?;

if response.status().as_u16() == expected {
Expand All @@ -4076,7 +4080,9 @@ impl Http {
debug!("Expected {}, got {}", expected, response.status());
trace!("Unsuccessful response: {:?}", response);

Err(Error::Http(HttpError::from_response(response).await))
Err(Error::Http(HttpError::UnsuccessfulRequest(
ErrorResponse::from_response(response, method.reqwest_method()).await,
)))
}
}

Expand Down
22 changes: 10 additions & 12 deletions src/http/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::error::Error as StdError;
use std::fmt;

use reqwest::header::InvalidHeaderValue;
use reqwest::{Error as ReqwestError, Response, StatusCode, Url};
use reqwest::{Error as ReqwestError, Method, Response, StatusCode};
use serde::de::{Deserialize, Deserializer, Error as _};
use url::ParseError as UrlError;

Expand Down Expand Up @@ -33,19 +33,22 @@ pub struct DiscordJsonSingleError {
}

#[derive(Clone, Debug, Eq, PartialEq)]
#[non_exhaustive]
pub struct ErrorResponse {
pub status_code: StatusCode,
pub url: Url,
pub url: String,
pub method: Method,
pub error: DiscordJsonError,
}

impl ErrorResponse {
// We need a freestanding from-function since we cannot implement an async
// From-trait.
pub async fn from_response(r: Response) -> Self {
pub async fn from_response(r: Response, method: Method) -> Self {
ErrorResponse {
status_code: r.status(),
url: r.url().clone(),
url: r.url().to_string(),
method,
error: decode_resp(r).await.unwrap_or_else(|e| DiscordJsonError {
code: -1,
message: format!("[Serenity] Could not decode json when receiving error response from discord:, {e}"),
Expand Down Expand Up @@ -83,12 +86,6 @@ pub enum HttpError {
}

impl HttpError {
// We need a freestanding from-function since we cannot implement an async
// From-trait.
pub async fn from_response(r: Response) -> Self {
ErrorResponse::from_response(r).await.into()
}

/// Returns true when the error is caused by an unsuccessful request
#[must_use]
pub fn is_unsuccessful_request(&self) -> bool {
Expand Down Expand Up @@ -274,11 +271,12 @@ mod test {
let response = builder.body(body_string.into_bytes()).unwrap();

let reqwest_response: reqwest::Response = response.into();
let error_response = ErrorResponse::from_response(reqwest_response).await;
let error_response = ErrorResponse::from_response(reqwest_response, Method::POST).await;

let known = ErrorResponse {
status_code: reqwest::StatusCode::from_u16(403).unwrap(),
url: String::from("https://ferris.crab").parse().unwrap(),
url: String::from("https://ferris.crab/"),
method: Method::POST,
error,
};

Expand Down

0 comments on commit 50cb56d

Please sign in to comment.