From 7fed55a1fdfe128ec7e7236fc1af95003758e9b1 Mon Sep 17 00:00:00 2001 From: Vilgot Fredenberg Date: Sun, 15 May 2022 22:11:18 +0200 Subject: [PATCH 1/5] refactor(http)!: only expose ratelimit info on response error The other variants were only useful prior to the API V8 error changes. --- http/src/api_error.rs | 255 ------------------------------------ http/src/error.rs | 56 +++++++- http/src/lib.rs | 1 - http/src/response/future.rs | 41 ++---- 4 files changed, 67 insertions(+), 286 deletions(-) delete mode 100644 http/src/api_error.rs diff --git a/http/src/api_error.rs b/http/src/api_error.rs deleted file mode 100644 index 9fe179c8eec..00000000000 --- a/http/src/api_error.rs +++ /dev/null @@ -1,255 +0,0 @@ -use serde::{Deserialize, Serialize}; -use std::fmt::{Display, Formatter, Result as FmtResult}; - -#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] -#[non_exhaustive] -#[serde(untagged)] -pub enum ApiError { - General(GeneralApiError), - /// Request has been ratelimited. - Ratelimited(RatelimitedApiError), - /// Something was wrong with the input when sending a message. - Message(MessageApiError), -} - -impl Display for ApiError { - fn fmt(&self, f: &mut Formatter<'_>) -> FmtResult { - match self { - Self::General(inner) => Display::fmt(inner, f), - Self::Message(inner) => Display::fmt(inner, f), - Self::Ratelimited(inner) => Display::fmt(inner, f), - } - } -} - -#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] -#[non_exhaustive] -pub struct GeneralApiError { - pub code: u64, - pub message: String, -} - -impl Display for GeneralApiError { - fn fmt(&self, f: &mut Formatter<'_>) -> FmtResult { - f.write_str("Error code ")?; - Display::fmt(&self.code, f)?; - f.write_str(": ")?; - - f.write_str(&self.message) - } -} - -/// Sending a message failed because the provided fields contained invalid -/// input. -#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] -#[non_exhaustive] -pub struct MessageApiError { - /// Fields within a provided embed were invalid. - pub embed: Option>, -} - -impl Display for MessageApiError { - fn fmt(&self, f: &mut Formatter<'_>) -> FmtResult { - f.write_str("message fields invalid: ")?; - - if let Some(embed) = &self.embed { - f.write_str("embed (")?; - - let field_count = embed.len().saturating_sub(1); - - for (idx, field) in embed.iter().enumerate() { - Display::fmt(field, f)?; - - if idx == field_count { - f.write_str(", ")?; - } - } - - f.write_str(")")?; - } - - Ok(()) - } -} - -/// Field within a [`MessageApiError`] [embed] list. -/// -/// [embed]: MessageApiError::embed -#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] -#[non_exhaustive] -#[serde(rename_all = "snake_case")] -pub enum MessageApiErrorEmbedField { - /// Something was wrong with the provided fields. - Fields, - /// The provided timestamp wasn't a valid RFC3339 string. - Timestamp, -} - -impl Display for MessageApiErrorEmbedField { - fn fmt(&self, f: &mut Formatter<'_>) -> FmtResult { - f.write_str(match self { - Self::Fields => "fields", - Self::Timestamp => "timestamp", - }) - } -} - -#[derive(Clone, Debug, Deserialize, Serialize)] -#[non_exhaustive] -pub struct RatelimitedApiError { - /// Whether the ratelimit is a global ratelimit. - pub global: bool, - /// Human readable message provided by the API. - pub message: String, - /// Amount of time to wait before retrying. - pub retry_after: f64, -} - -impl Display for RatelimitedApiError { - fn fmt(&self, f: &mut Formatter<'_>) -> FmtResult { - f.write_str("Got ")?; - - if self.global { - f.write_str("global ")?; - } - - f.write_str("ratelimited for ")?; - Display::fmt(&self.retry_after, f)?; - - f.write_str("s") - } -} - -impl Eq for RatelimitedApiError {} - -impl PartialEq for RatelimitedApiError { - fn eq(&self, other: &Self) -> bool { - self.global == other.global && self.message == other.message - } -} - -#[cfg(test)] -mod tests { - use super::{ - ApiError, GeneralApiError, MessageApiError, MessageApiErrorEmbedField, RatelimitedApiError, - }; - use serde_test::Token; - - #[test] - fn test_api_error_deser() { - let expected = GeneralApiError { - code: 10001, - message: "Unknown account".to_owned(), - }; - - serde_test::assert_tokens( - &expected, - &[ - Token::Struct { - name: "GeneralApiError", - len: 2, - }, - Token::Str("code"), - Token::U64(10001), - Token::Str("message"), - Token::Str("Unknown account"), - Token::StructEnd, - ], - ); - } - - #[test] - fn test_api_error_message() { - let expected = ApiError::Message(MessageApiError { - embed: Some( - [ - MessageApiErrorEmbedField::Fields, - MessageApiErrorEmbedField::Timestamp, - ] - .to_vec(), - ), - }); - - serde_test::assert_tokens( - &expected, - &[ - Token::Struct { - name: "MessageApiError", - len: 1, - }, - Token::Str("embed"), - Token::Some, - Token::Seq { len: Some(2) }, - Token::UnitVariant { - name: "MessageApiErrorEmbedField", - variant: "fields", - }, - Token::UnitVariant { - name: "MessageApiErrorEmbedField", - variant: "timestamp", - }, - Token::SeqEnd, - Token::StructEnd, - ], - ); - } - - #[test] - fn test_ratelimited_api_error() { - let expected = RatelimitedApiError { - global: true, - message: "You are being rate limited.".to_owned(), - retry_after: 6.457, - }; - - serde_test::assert_tokens( - &expected, - &[ - Token::Struct { - name: "RatelimitedApiError", - len: 3, - }, - Token::Str("global"), - Token::Bool(true), - Token::Str("message"), - Token::Str("You are being rate limited."), - Token::Str("retry_after"), - Token::F64(6.457), - Token::StructEnd, - ], - ); - } - - /// Assert that deserializing an [`ApiError::Ratelimited`] variant uses - /// the correct variant. - /// - /// Tests for [#1302], which was due to a previously ordered variant having - /// higher priority for untagged deserialization. - /// - /// [#1302]: https://github.com/twilight-rs/twilight/issues/1302 - #[test] - fn test_api_error_variant_ratelimited() { - let expected = ApiError::Ratelimited(RatelimitedApiError { - global: false, - message: "You are being rate limited.".to_owned(), - retry_after: 0.362, - }); - - serde_test::assert_tokens( - &expected, - &[ - Token::Struct { - name: "RatelimitedApiError", - len: 3, - }, - Token::Str("global"), - Token::Bool(false), - Token::Str("message"), - Token::Str("You are being rate limited."), - Token::Str("retry_after"), - Token::F64(0.362), - Token::StructEnd, - ], - ); - } -} diff --git a/http/src/error.rs b/http/src/error.rs index 71feb001ec2..e2afb611fa4 100644 --- a/http/src/error.rs +++ b/http/src/error.rs @@ -1,5 +1,6 @@ -use crate::{api_error::ApiError, json::JsonError, response::StatusCode}; +use crate::{json::JsonError, response::StatusCode}; use hyper::{Body, Response}; +use serde::{Deserialize, Serialize}; use std::{ error::Error as StdError, fmt::{Debug, Display, Formatter, Result as FmtResult}, @@ -110,7 +111,7 @@ pub enum ErrorType { RequestTimedOut, Response { body: Vec, - error: ApiError, + ratelimit: Option, status: StatusCode, }, /// API service is unavailable. Consider re-sending the request at a @@ -126,3 +127,54 @@ pub enum ErrorType { /// or is revoked. Recreate the client to configure a new token. Unauthorized, } + +#[derive(Clone, Debug, Deserialize, Serialize)] +#[non_exhaustive] +pub struct Ratelimit { + /// Whether the ratelimit is global. + pub global: bool, + /// Amount of time to wait before retrying. + pub retry_after: f64, +} + +impl Display for Ratelimit { + fn fmt(&self, f: &mut Formatter<'_>) -> FmtResult { + if self.global { + f.write_str("globally ")?; + } + + f.write_str("ratelimited for ")?; + Display::fmt(&self.retry_after, f)?; + + f.write_str("s") + } +} + +#[cfg(test)] +mod tests { + use super::Ratelimit; + use serde_test::Token; + + #[test] + fn test_ratelimited_api_error() { + let expected = Ratelimit { + global: true, + retry_after: 6.457, + }; + + serde_test::assert_ser_tokens( + &expected, + &[ + Token::Struct { + name: "Ratelimited", + len: 2, + }, + Token::Str("global"), + Token::Bool(true), + Token::Str("retry_after"), + Token::F64(6.457), + Token::StructEnd, + ], + ); + } +} diff --git a/http/src/lib.rs b/http/src/lib.rs index dc89f78708d..7714f66c850 100644 --- a/http/src/lib.rs +++ b/http/src/lib.rs @@ -128,7 +128,6 @@ clippy::unnecessary_wraps )] -pub mod api_error; pub mod client; pub mod error; pub mod request; diff --git a/http/src/response/future.rs b/http/src/response/future.rs index 039a46a499e..073aeda8880 100644 --- a/http/src/response/future.rs +++ b/http/src/response/future.rs @@ -1,8 +1,5 @@ use super::{Response, StatusCode}; -use crate::{ - api_error::ApiError, - error::{Error, ErrorType}, -}; +use crate::error::{Error, ErrorType}; use hyper::{client::ResponseFuture as HyperResponseFuture, StatusCode as HyperStatusCode}; use std::{ future::Future, @@ -35,30 +32,18 @@ struct Chunking { impl Chunking { fn poll(mut self, cx: &mut Context<'_>) -> InnerPoll { - let bytes = match Pin::new(&mut self.future).poll(cx) { - Poll::Ready(Ok(bytes)) => bytes, - Poll::Ready(Err(source)) => return InnerPoll::Ready(Err(source)), - Poll::Pending => return InnerPoll::Pending(ResponseFutureStage::Chunking(self)), - }; - - let error = match crate::json::from_bytes::(&bytes) { - Ok(error) => error, - Err(source) => { - return InnerPoll::Ready(Err(Error { - kind: ErrorType::Parsing { body: bytes }, - source: Some(Box::new(source)), - })); - } - }; - - InnerPoll::Ready(Err(Error { - kind: ErrorType::Response { - body: bytes, - error, - status: StatusCode::new(self.status.as_u16()), - }, - source: None, - })) + match Pin::new(&mut self.future).poll(cx) { + Poll::Ready(Ok(bytes)) => InnerPoll::Ready(Err(Error { + kind: ErrorType::Response { + ratelimit: crate::json::from_bytes(&bytes).ok(), + body: bytes, + status: StatusCode::new(self.status.as_u16()), + }, + source: None, + })), + Poll::Ready(Err(source)) => InnerPoll::Ready(Err(source)), + Poll::Pending => InnerPoll::Pending(ResponseFutureStage::Chunking(self)), + } } } From 83ee8d8ec3e825b1bea6479da2a7359c8ae12026 Mon Sep 17 00:00:00 2001 From: Vilgot Fredenberg Date: Sun, 15 May 2022 22:19:41 +0200 Subject: [PATCH 2/5] tests: rename serde token variable --- http/src/error.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http/src/error.rs b/http/src/error.rs index e2afb611fa4..09a04b2b557 100644 --- a/http/src/error.rs +++ b/http/src/error.rs @@ -166,7 +166,7 @@ mod tests { &expected, &[ Token::Struct { - name: "Ratelimited", + name: "Ratelimit", len: 2, }, Token::Str("global"), From c38edaf56eb21c81252a8532bb92c3a4e9e55fdb Mon Sep 17 00:00:00 2001 From: Vilgot Fredenberg Date: Mon, 30 May 2022 08:05:28 +0200 Subject: [PATCH 3/5] docs: add time unit and precision of `retry_after` --- http/src/error.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/http/src/error.rs b/http/src/error.rs index 09a04b2b557..04ad687017a 100644 --- a/http/src/error.rs +++ b/http/src/error.rs @@ -133,7 +133,9 @@ pub enum ErrorType { pub struct Ratelimit { /// Whether the ratelimit is global. pub global: bool, - /// Amount of time to wait before retrying. + /// Number of seconds to wait before retrying. + /// + /// Up to three decimals may be set to match millisecond precision. pub retry_after: f64, } From d79fafdeb44ff28477ba9df3c9b85753fd37a431 Mon Sep 17 00:00:00 2001 From: Vilgot Fredenberg Date: Mon, 20 Jun 2022 14:42:26 +0200 Subject: [PATCH 4/5] restore json error code and `ApiError` enum for response error --- http/src/error.rs | 59 +++++++++++++++++++++---------------- http/src/response/future.rs | 24 +++++++++------ 2 files changed, 49 insertions(+), 34 deletions(-) diff --git a/http/src/error.rs b/http/src/error.rs index 04ad687017a..8e5dbd725a7 100644 --- a/http/src/error.rs +++ b/http/src/error.rs @@ -111,7 +111,8 @@ pub enum ErrorType { RequestTimedOut, Response { body: Vec, - ratelimit: Option, + /// Deserialized context from [`body`]. + context: ApiError, status: StatusCode, }, /// API service is unavailable. Consider re-sending the request at a @@ -128,38 +129,39 @@ pub enum ErrorType { Unauthorized, } -#[derive(Clone, Debug, Deserialize, Serialize)] +/// API returned error context. +#[derive(Debug, Deserialize, Serialize)] #[non_exhaustive] -pub struct Ratelimit { - /// Whether the ratelimit is global. - pub global: bool, - /// Number of seconds to wait before retrying. +#[serde(untagged)] +pub enum ApiError { + /// [JSON error code]. /// - /// Up to three decimals may be set to match millisecond precision. - pub retry_after: f64, -} - -impl Display for Ratelimit { - fn fmt(&self, f: &mut Formatter<'_>) -> FmtResult { - if self.global { - f.write_str("globally ")?; - } - - f.write_str("ratelimited for ")?; - Display::fmt(&self.retry_after, f)?; - - f.write_str("s") - } + /// [JSON error code]: https://discord.com/developers/docs/topics/opcodes-and-status-codes#json-json-error-codes + Code(u64), + /// Request was ratelimited. + Ratelimit { + /// Whether the ratelimit is global. + global: bool, + /// Number of seconds to wait before retrying. + /// + /// Up to three decimals may be set to match millisecond precision. + retry_after: f64, + }, } #[cfg(test)] mod tests { - use super::Ratelimit; + use super::ApiError; + use serde::{Deserialize, Serialize}; use serde_test::Token; + use static_assertions::assert_impl_all; + use std::fmt::Debug; + + assert_impl_all!(ApiError: Debug, Deserialize<'static>, Send, Serialize, Sync); #[test] - fn test_ratelimited_api_error() { - let expected = Ratelimit { + fn api_error_ratelimited() { + let expected = ApiError::Ratelimit { global: true, retry_after: 6.457, }; @@ -168,7 +170,7 @@ mod tests { &expected, &[ Token::Struct { - name: "Ratelimit", + name: "ApiError", len: 2, }, Token::Str("global"), @@ -179,4 +181,11 @@ mod tests { ], ); } + + #[test] + fn api_error_code() { + let expected = ApiError::Code(0); + + serde_test::assert_ser_tokens(&expected, &[Token::U64(0)]); + } } diff --git a/http/src/response/future.rs b/http/src/response/future.rs index c6dcd1ce338..15946951ad6 100644 --- a/http/src/response/future.rs +++ b/http/src/response/future.rs @@ -33,15 +33,21 @@ struct Chunking { impl Chunking { fn poll(mut self, cx: &mut Context<'_>) -> InnerPoll { match Pin::new(&mut self.future).poll(cx) { - Poll::Ready(Ok(bytes)) => InnerPoll::Ready(Err(Error { - kind: ErrorType::Response { - ratelimit: crate::json::from_bytes(&bytes).ok(), - body: bytes, - status: StatusCode::new(self.status.as_u16()), - }, - source: None, - })), - Poll::Ready(Err(source)) => InnerPoll::Ready(Err(source)), + Poll::Ready(Ok(bytes)) => InnerPoll::Ready(match crate::json::from_bytes(&bytes) { + Ok(context) => Err(Error { + kind: ErrorType::Response { + body: bytes, + context, + status: StatusCode::new(self.status.as_u16()), + }, + source: None, + }), + Err(source) => Err(Error { + kind: ErrorType::Parsing { body: bytes }, + source: Some(Box::new(source)), + }), + }), + Poll::Ready(Err(e)) => InnerPoll::Ready(Err(e)), Poll::Pending => InnerPoll::Pending(ResponseFutureStage::Chunking(self)), } } From fb254d12b116977830bed8fb1e4ffcd658b2f5f8 Mon Sep 17 00:00:00 2001 From: Vilgot Fredenberg Date: Mon, 20 Jun 2022 14:57:42 +0200 Subject: [PATCH 5/5] intralink body on `ErrorType::response::context` --- http/src/error.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/http/src/error.rs b/http/src/error.rs index 8e5dbd725a7..dc2dc8cba05 100644 --- a/http/src/error.rs +++ b/http/src/error.rs @@ -112,6 +112,8 @@ pub enum ErrorType { Response { body: Vec, /// Deserialized context from [`body`]. + /// + /// [`body`]: Self::Response::body context: ApiError, status: StatusCode, },