From 6b5384608e19df3bd31944cfa4331eb2b01f0be0 Mon Sep 17 00:00:00 2001
From: Jiahao XU <Jiahao_XU@outlook.com>
Date: Wed, 7 Jun 2023 11:02:29 +1000
Subject: [PATCH] `GhApiClient` misc improvements (#1139)

- Increase `DEFAULT_RETRY_DURATION` to 5 minutes, since GitHub enforces
   rate limit on an hourly basis.
 - Refactor `check_for_status` & `fetch_release_artifacts_restful_api`
 - Optimize `percent_decode_http_url_path`: Avoid `percent_decode_str`
   if there is no `%` in the `input`.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
---
 .../binstalk-downloader/src/gh_api_client.rs  |  9 +++-
 .../src/gh_api_client/request.rs              | 48 ++++++++-----------
 2 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/crates/binstalk-downloader/src/gh_api_client.rs b/crates/binstalk-downloader/src/gh_api_client.rs
index a9e63f26..10bedf6f 100644
--- a/crates/binstalk-downloader/src/gh_api_client.rs
+++ b/crates/binstalk-downloader/src/gh_api_client.rs
@@ -21,7 +21,7 @@ mod request;
 pub use request::{GhApiContextError, GhApiError, GhGraphQLErrors};
 
 /// default retry duration if x-ratelimit-reset is not found in response header
-const DEFAULT_RETRY_DURATION: Duration = Duration::from_secs(3);
+const DEFAULT_RETRY_DURATION: Duration = Duration::from_secs(5 * 60);
 
 fn percent_encode_http_url_path(path: &str) -> PercentEncode<'_> {
     /// https://url.spec.whatwg.org/#fragment-percent-encode-set
@@ -42,7 +42,12 @@ fn percent_encode_http_url_path(path: &str) -> PercentEncode<'_> {
 }
 
 fn percent_decode_http_url_path(input: &str) -> CompactString {
-    percent_decode_str(input).decode_utf8_lossy().into()
+    if input.contains('%') {
+        percent_decode_str(input).decode_utf8_lossy().into()
+    } else {
+        // No '%', no need to decode.
+        CompactString::new(input)
+    }
 }
 
 /// The keys required to identify a github release.
diff --git a/crates/binstalk-downloader/src/gh_api_client/request.rs b/crates/binstalk-downloader/src/gh_api_client/request.rs
index 8b5e9291..2289566c 100644
--- a/crates/binstalk-downloader/src/gh_api_client/request.rs
+++ b/crates/binstalk-downloader/src/gh_api_client/request.rs
@@ -111,29 +111,26 @@ pub(super) enum FetchReleaseRet {
 }
 
 fn check_for_status(status: StatusCode, headers: &HeaderMap) -> Option<FetchReleaseRet> {
-    if status == remote::StatusCode::FORBIDDEN
-        && headers
-            .get("x-ratelimit-remaining")
-            .map(|val| val == "0")
-            .unwrap_or(false)
-    {
-        return Some(FetchReleaseRet::ReachedRateLimit {
-            retry_after: headers.get("x-ratelimit-reset").and_then(|value| {
-                let secs = value.to_str().ok()?.parse().ok()?;
-                Some(Duration::from_secs(secs))
-            }),
-        });
-    }
+    match status {
+        remote::StatusCode::FORBIDDEN
+            if headers
+                .get("x-ratelimit-remaining")
+                .map(|val| val == "0")
+                .unwrap_or(false) =>
+        {
+            Some(FetchReleaseRet::ReachedRateLimit {
+                retry_after: headers.get("x-ratelimit-reset").and_then(|value| {
+                    let secs = value.to_str().ok()?.parse().ok()?;
+                    Some(Duration::from_secs(secs))
+                }),
+            })
+        }
 
-    if status == remote::StatusCode::UNAUTHORIZED {
-        return Some(FetchReleaseRet::Unauthorized);
-    }
+        remote::StatusCode::UNAUTHORIZED => Some(FetchReleaseRet::Unauthorized),
+        remote::StatusCode::NOT_FOUND => Some(FetchReleaseRet::ReleaseNotFound),
 
-    if status == remote::StatusCode::NOT_FOUND {
-        return Some(FetchReleaseRet::ReleaseNotFound);
+        _ => None,
     }
-
-    None
 }
 
 async fn fetch_release_artifacts_restful_api(
@@ -157,14 +154,11 @@ async fn fetch_release_artifacts_restful_api(
 
     let response = request_builder.send(false).await?;
 
-    let status = response.status();
-    let headers = response.headers();
-
-    if let Some(ret) = check_for_status(status, headers) {
-        return Ok(ret);
+    if let Some(ret) = check_for_status(response.status(), response.headers()) {
+        Ok(ret)
+    } else {
+        Ok(FetchReleaseRet::Artifacts(response.json().await?))
     }
-
-    Ok(FetchReleaseRet::Artifacts(response.json().await?))
 }
 
 #[derive(Deserialize)]