From 14e6faa24efecf8ea2bd3b08e7f68aada1efc369 Mon Sep 17 00:00:00 2001 From: Mingwei Samuel Date: Thu, 4 Jun 2020 21:52:43 -0700 Subject: [PATCH] Restructure, add RiotApi.get_raw_response(...) method useful for proxies. New RiotApi.get_raw_response(...) method similar to existing RiotApi.get(...) and RiotApi.get_optional(...), but does not parse the result. Useful for proxies which just want to forward the content body. Also adds tests to JP of failure cases. --- src/lib.rs | 3 ++ src/req/regional_requester.rs | 72 ++++++++++++++--------------------- src/response_info.rs | 11 ++++++ src/riot_api.rs | 52 ++++++++++++++++++++++--- tests/tests_jp.rs | 28 ++++++++++++++ 5 files changed, 116 insertions(+), 50 deletions(-) create mode 100644 src/response_info.rs diff --git a/src/lib.rs b/src/lib.rs index fa3ea2b..c692254 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -24,6 +24,9 @@ pub mod models; mod req; +mod response_info; +pub use response_info::*; + mod riot_api; pub use riot_api::*; diff --git a/src/req/regional_requester.rs b/src/req/regional_requester.rs index 0d7f93a..ce55dda 100644 --- a/src/req/regional_requester.rs +++ b/src/req/regional_requester.rs @@ -6,6 +6,7 @@ use reqwest::{ Client, StatusCode, Url }; use tokio::time::delay_for; use crate::Result; +use crate::ResponseInfo; use crate::RiotApiError; use crate::RiotApiConfig; use crate::util::InsertOnlyCHashMap; @@ -14,9 +15,9 @@ use super::RateLimit; use super::RateLimitType; pub struct RegionalRequester { - /// Represents the app rate limit. + /// The app rate limit. app_rate_limit: RateLimit, - /// Represents method rate limits. + /// Method rate limits. method_rate_limits: InsertOnlyCHashMap<&'static str, RateLimit>, } @@ -37,29 +38,10 @@ impl RegionalRequester { } } - pub fn get_optional<'a, T: serde::de::DeserializeOwned>(self: Arc, + pub fn get<'a>(self: Arc, config: &'a RiotApiConfig, client: &'a Client, method_id: &'static str, region_platform: &'a str, path: String, query: Option) - -> impl Future>> + 'a - { - async move { - let response_result = self.get(config, client, - method_id, region_platform, path, query).await; - response_result.map(|value| Some(value)) - .or_else(|e| { - if let Some(status) = e.status_code() { - if Self::NONE_STATUS_CODES.contains(&status) { - return Ok(None); - }} - Err(e) - }) - } - } - - pub fn get<'a, T: serde::de::DeserializeOwned>(self: Arc, - config: &'a RiotApiConfig, client: &'a Client, - method_id: &'static str, region_platform: &'a str, path: String, query: Option) - -> impl Future> + 'a + -> impl Future> + 'a { async move { #[cfg(feature = "nightly")] let query = query.as_deref(); @@ -94,27 +76,29 @@ impl RegionalRequester { let status = response.status(); // Handle normal success / failure cases. - match response.error_for_status_ref() { - // Success. - Ok(_response) => { - log::trace!("Response {} (retried {} times), parsed result.", status, retries); - let value = response.json::().await; - break value.map_err(|e| RiotApiError::new(e, retries, None, Some(status))); - }, - // Failure, may or may not be retryable. - Err(err) => { - // Not-retryable: no more retries or 4xx or ? (3xx, redirects exceeded). - // Retryable: retries remaining, and 429 or 5xx. - if retries >= config.retries || - (StatusCode::TOO_MANY_REQUESTS != status - && !status.is_server_error()) - { - log::debug!("Response {} (retried {} times), returning.", status, retries); - break Err(RiotApiError::new(err, retries, Some(response), Some(status))); - } - log::debug!("Response {} (retried {} times), retrying.", status, retries); - }, - }; + let status_none = Self::NONE_STATUS_CODES.contains(&status); + // Success case. + if status.is_success() || status_none { + log::trace!("Response {} (retried {} times), success, returning result.", status, retries); + break Ok(ResponseInfo { + response: response, + retries: retries, + status_none: status_none, + }); + } + let err = response.error_for_status_ref().err().unwrap_or_else( + || panic!("Unhandlable response status code, neither success nor failure: {}.", status)); + // Failure, may or may not be retryable. + // Not-retryable: no more retries or 4xx or ? (3xx, redirects exceeded). + // Retryable: retries remaining, and 429 or 5xx. + if retries >= config.retries || + (StatusCode::TOO_MANY_REQUESTS != status + && !status.is_server_error()) + { + log::debug!("Response {} (retried {} times), failure, returning error.", status, retries); + break Err(RiotApiError::new(err, retries, Some(response), Some(status))); + } + log::debug!("Response {} (retried {} times), retrying.", status, retries); retries += 1; } diff --git a/src/response_info.rs b/src/response_info.rs new file mode 100644 index 0000000..546b8b9 --- /dev/null +++ b/src/response_info.rs @@ -0,0 +1,11 @@ +use reqwest::Response; + +/// A "raw" unparsed successful response from the Riot API, for internal or advanced use cases. +pub struct ResponseInfo { + /// The reqwest response. + pub response: Response, + /// The number of retries used, zero for first-try success. + pub retries: u8, + /// If the response has an HTTP status code indicating a `None` response (i.e. 204, 404). + pub status_none: bool, +} diff --git a/src/riot_api.rs b/src/riot_api.rs index 42d594c..7149278 100644 --- a/src/riot_api.rs +++ b/src/riot_api.rs @@ -5,7 +5,9 @@ use log; use reqwest::Client; use crate::Result; +use crate::ResponseInfo; use crate::RiotApiConfig; +use crate::RiotApiError; use crate::req::RegionalRequester; use crate::util::InsertOnlyCHashMap; @@ -74,12 +76,22 @@ impl RiotApi { /// * `region_platform` - The stringified platform, prepended to `.api.riotgames.com` to create the hostname. /// * `path` - The path relative to the hostname. /// * `query` - An optional query string. - pub fn get_optional<'a, T: serde::de::DeserializeOwned + 'a>(&'a self, + /// + /// # Returns + /// A future resolving to a `Result` containg either a `Option` (success) or a `RiotApiError` (failure). + pub async fn get_optional<'a, T: serde::de::DeserializeOwned + 'a>(&'a self, method_id: &'static str, region_platform: &'static str, path: String, query: Option) - -> impl Future>> + 'a + -> Result> { - self.regional_requester(region_platform) - .get_optional(&self.config, &self.client, method_id, region_platform, path, query) + let rinfo = self.get_raw_response(method_id, region_platform, path, query).await?; + if rinfo.status_none { + return Ok(None); + } + let retries = rinfo.retries; + let status = rinfo.response.status(); + let value = rinfo.response.json::().await; + value.map(|v| Some(v)) + .map_err(|e| RiotApiError::new(e, retries, None, Some(status))) } /// This method is not meant to be used directly. @@ -91,9 +103,37 @@ impl RiotApi { /// * `region_platform` - The stringified platform, prepended to `.api.riotgames.com` to create the hostname. /// * `path` - The path relative to the hostname. /// * `query` - An optional query string. - pub fn get<'a, T: serde::de::DeserializeOwned + 'a>(&'a self, + /// + /// # Returns + /// A future resolving to a `Result` containg either a `T` (success) or a `RiotApiError` (failure). + pub async fn get<'a, T: serde::de::DeserializeOwned + 'a>(&'a self, method_id: &'static str, region_platform: &'static str, path: String, query: Option) - -> impl Future> + 'a + -> Result + { + let rinfo = self.get_raw_response(method_id, region_platform, path, query).await?; + let retries = rinfo.retries; + let status = rinfo.response.status(); + let value = rinfo.response.json::().await; + value.map_err(|e| RiotApiError::new(e, retries, None, Some(status))) + } + + /// This method is not meant to be used directly. + /// + /// This sends a GET request based on the given parameters and returns a raw `ResponseInfo`. + /// + /// This can be used to implement a Riot API proxy without needing to deserialize and reserialize JSON responses. + /// + /// # Parameters + /// * `method_id` - A unique string id representing the endpoint method for per-method rate limiting. + /// * `region_platform` - The stringified platform, prepended to `.api.riotgames.com` to create the hostname. + /// * `path` - The path relative to the hostname. + /// * `query` - An optional query string. + /// + /// # Returns + /// A future resolving to a `Result` containg either a `ResponseInfo` (success) or a `RiotApiError` (failure). + pub fn get_raw_response<'a>(&'a self, + method_id: &'static str, region_platform: &'static str, path: String, query: Option) + -> impl Future> + 'a { self.regional_requester(region_platform) .get(&self.config, &self.client, method_id, region_platform, path, query) diff --git a/tests/tests_jp.rs b/tests/tests_jp.rs index 10ed8f1..7addcb0 100644 --- a/tests/tests_jp.rs +++ b/tests/tests_jp.rs @@ -9,6 +9,7 @@ use colored::*; use riven::consts::*; + async_tests!{ my_runner { // Summoner tests. @@ -18,5 +19,32 @@ async_tests!{ rassert_eq!("私の頭がかたい", s.name); Ok(()) }, + + // Failure cases. + // Make sure get_raw_response(...) with invalid path fails as expected. + raw_response_invalid: async { + let p = RIOT_API.get_raw_response("summoner-v4.getBySummonerName", Region::JP.into(), "INVALID/PATH".to_owned(), None); + let r = p.await; + rassert!(r.is_err()); + Ok(()) + }, + // summoner_v4().get_by_summoner_name(...) normally returns an option. + // If we use `get` (instead of `get_optional`) make sure it errors. + get_nonoptional_invalid: async { + let path_string = format!("/lol/summoner/v4/summoners/by-name/{}", "SUMMONER THAT DOES NOT EXIST"); + let p = RIOT_API.get::( + "summoner-v4.getBySummonerName", Region::JP.into(), path_string, None); + let r = p.await; + rassert!(r.is_err()); + Ok(()) + }, + // Make sure 403 is handled as expected. + tournament_forbidden: async { + let p = RIOT_API.tournament_v4().get_tournament_code(Region::JP, "INVALID_CODE"); + let r = p.await; + rassert!(r.is_err()); + rassert_eq!(Some(reqwest::StatusCode::FORBIDDEN), r.unwrap_err().status_code()); + Ok(()) + }, } }