diff --git a/crates/binstalk-fetchers/src/common.rs b/crates/binstalk-fetchers/src/common.rs index 28267390..6cc291d7 100644 --- a/crates/binstalk-fetchers/src/common.rs +++ b/crates/binstalk-fetchers/src/common.rs @@ -8,7 +8,7 @@ pub(super) use binstalk_downloader::{ remote::{Client, Url}, }; pub(super) use binstalk_git_repo_api::gh_api_client::GhApiClient; -use binstalk_git_repo_api::gh_api_client::{GhReleaseArtifact, HasReleaseArtifact}; +use binstalk_git_repo_api::gh_api_client::{GhApiError, GhReleaseArtifact}; pub(super) use binstalk_types::cargo_toml_binstall::{PkgFmt, PkgMeta}; pub(super) use compact_str::CompactString; pub(super) use tokio::task::JoinHandle; @@ -34,22 +34,24 @@ pub(super) async fn does_url_exist( debug!("Using GitHub API to check for existence of artifact, which will also cache the API response"); // The future returned has the same size as a pointer - match gh_api_client.has_release_artifact(artifact).await? { - HasReleaseArtifact::Yes { .. } => return Ok(true), - HasReleaseArtifact::No | HasReleaseArtifact::NoSuchRelease => return Ok(false), + match gh_api_client.has_release_artifact(artifact).await { + Ok(Some(_)) => return Ok(true), + Ok(None) | Err(GhApiError::NotFound) => return Ok(false), - HasReleaseArtifact::RateLimit { retry_after } => { + Err(GhApiError::RateLimit { retry_after }) => { WARN_RATE_LIMIT_ONCE.call_once(|| { warn!("Your GitHub API token (if any) has reached its rate limit and cannot be used again until {retry_after:?}, so we will fallback to HEAD/GET on the url."); warn!("If you did not supply a github token, consider doing so: GitHub limits unauthorized users to 60 requests per hour per origin IP address."); }); } - HasReleaseArtifact::Unauthorized => { + Err(GhApiError::Unauthorized) => { WARN_UNAUTHORIZED_ONCE.call_once(|| { warn!("GitHub API somehow requires a token for the API access, so we will fallback to HEAD/GET on the url."); warn!("Please consider supplying a token to cargo-binstall to speedup resolution."); }); } + + Err(err) => return Err(err.into()), } GH_API_CLIENT_FAILED.store(true, Relaxed); diff --git a/crates/binstalk-git-repo-api/src/gh_api_client.rs b/crates/binstalk-git-repo-api/src/gh_api_client.rs index 213ffb73..ee4bb386 100644 --- a/crates/binstalk-git-repo-api/src/gh_api_client.rs +++ b/crates/binstalk-git-repo-api/src/gh_api_client.rs @@ -130,40 +130,22 @@ impl GhApiClient { } } -enum FetchReleaseArtifactError { - Error(GhApiError), - RateLimit { retry_after: Instant }, - Unauthorized, -} - impl GhApiClient { async fn do_fetch_release_artifacts( &self, release: &GhRelease, auth_token: Option<&str>, - ) -> Result, FetchReleaseArtifactError> { - use common::GhApiRet::*; - use FetchReleaseArtifactError as Error; - + ) -> Result, GhApiError> { match release_artifacts::fetch_release_artifacts(&self.0.client, release, auth_token).await { - Ok(NotFound) => Ok(None), - Ok(Success(artifacts)) => Ok(Some(artifacts)), - Ok(ReachedRateLimit { retry_after }) => { - let retry_after = retry_after.unwrap_or(DEFAULT_RETRY_DURATION); - - let now = Instant::now(); - let retry_after = now - .checked_add(retry_after) - .unwrap_or_else(|| now + DEFAULT_RETRY_DURATION); - - Err(Error::RateLimit { retry_after }) - } - Ok(Unauthorized) => Err(Error::Unauthorized), - Err(err) => Err(Error::Error(err)), + Ok(artifacts) => Ok(Some(artifacts)), + Err(GhApiError::NotFound) => Ok(None), + Err(err) => Err(err), } } + /// Return `Ok(Some(api_artifact_url))` if exists. + /// /// The returned future is guaranteed to be pointer size. pub async fn has_release_artifact( &self, @@ -171,9 +153,7 @@ impl GhApiClient { release, artifact_name, }: GhReleaseArtifact, - ) -> Result { - use FetchReleaseArtifactError as Error; - + ) -> Result, GhApiError> { let once_cell = self.0.release_artifacts.get(release.clone()); let res = once_cell .get_or_try_init(|| { @@ -183,7 +163,9 @@ impl GhApiClient { if let Some(retry_after) = *guard { if retry_after.elapsed().is_zero() { - return Err(Error::RateLimit { retry_after }); + return Err(GhApiError::RateLimit { + retry_after: Some(retry_after - Instant::now()), + }); } else { // Instant retry_after is already reached. *guard = None; @@ -196,7 +178,7 @@ impl GhApiClient { .do_fetch_release_artifacts(&release, self.0.auth_token.as_deref()) .await { - Err(Error::Unauthorized) => { + Err(GhApiError::Unauthorized) => { self.0.is_auth_token_valid.store(false, Relaxed); } res => return res, @@ -209,52 +191,19 @@ impl GhApiClient { .await; match res { - Ok(Some(artifacts)) => Ok(artifacts - .get_artifact_url(&artifact_name) - .map(|url| HasReleaseArtifact::Yes { url }) - .unwrap_or(HasReleaseArtifact::No)), - Ok(None) => Ok(HasReleaseArtifact::NoSuchRelease), - Err(Error::Unauthorized) => Ok(HasReleaseArtifact::Unauthorized), - Err(Error::RateLimit { retry_after }) => { - *self.0.retry_after.lock().unwrap() = Some(retry_after); + Ok(Some(artifacts)) => Ok(artifacts.get_artifact_url(&artifact_name)), + Ok(None) => Ok(None), + Err(GhApiError::RateLimit { retry_after }) => { + *self.0.retry_after.lock().unwrap() = + Some(Instant::now() + retry_after.unwrap_or(DEFAULT_RETRY_DURATION)); - Ok(HasReleaseArtifact::RateLimit { retry_after }) + Err(GhApiError::RateLimit { retry_after }) } - Err(Error::Error(err)) => Err(err), + Err(err) => Err(err), } } } -#[derive(Eq, PartialEq, Clone, Debug)] -pub enum HasReleaseArtifact { - Yes { - /// get url for downloading the artifact using GitHub API (for private repository). - url: CompactString, - }, - No, - NoSuchRelease, - /// GitHub returns 401 requiring a token. - /// In this case, it makes sense to fallback to HEAD/GET. - Unauthorized, - - /// GitHub rate limit is applied per hour, so in case of reaching the rate - /// limit, [`GhApiClient`] will return this variant and let the user decide - /// what to do. - /// - /// Usually it is more sensible to fallback to directly HEAD/GET the - /// artifact url than waiting until `retry_after`. - /// - /// If you encounter this frequently, then you should consider getting an - /// authentication token (can be personal access or oath access token), - /// which should give you 5000 requests per hour per user. - /// - /// Rate limit for unauthorized user is 60 requests per hour per originating - /// IP address, so it is very easy to be rate limited. - RateLimit { - retry_after: Instant, - }, -} - #[cfg(test)] mod test { use super::*; @@ -373,39 +322,31 @@ mod test { eprintln!("In client {client:?}"); for artifact_name in artifacts { - let ret = client + let res = client .has_release_artifact(GhReleaseArtifact { release: release.clone(), artifact_name: artifact_name.to_compact_string(), }) - .await - .unwrap(); + .await; assert!( - matches!( - ret, - HasReleaseArtifact::Yes { .. } | HasReleaseArtifact::RateLimit { .. } - ), + matches!(res, Ok(Some(_)) | Err(GhApiError::RateLimit { .. })), "for '{artifact_name}': answer is {:#?}", - ret + res ); } - let ret = client + let res = client .has_release_artifact(GhReleaseArtifact { release: release.clone(), artifact_name: "123z".to_compact_string(), }) - .await - .unwrap(); + .await; assert!( - matches!( - ret, - HasReleaseArtifact::No | HasReleaseArtifact::RateLimit { .. } - ), - "ret = {:#?}", - ret + matches!(res, Ok(None) | Err(GhApiError::RateLimit { .. })), + "res = {:#?}", + res ); } } @@ -430,21 +371,18 @@ mod test { tag: "v0.18.2".to_compact_string(), }; - let ret = client + let err = client .has_release_artifact(GhReleaseArtifact { release, artifact_name: "1234".to_compact_string(), }) .await - .unwrap(); + .unwrap_err(); assert!( - matches!( - ret, - HasReleaseArtifact::NoSuchRelease | HasReleaseArtifact::RateLimit { .. } - ), - "ret = {:#?}", - ret + matches!(err, GhApiError::NotFound | GhApiError::RateLimit { .. }), + "err = {:#?}", + err ); } } diff --git a/crates/binstalk-git-repo-api/src/gh_api_client/common.rs b/crates/binstalk-git-repo-api/src/gh_api_client/common.rs index 60041488..82784f68 100644 --- a/crates/binstalk-git-repo-api/src/gh_api_client/common.rs +++ b/crates/binstalk-git-repo-api/src/gh_api_client/common.rs @@ -38,14 +38,7 @@ pub(super) fn percent_decode_http_url_path(input: &str) -> CompactString { } } -pub(super) enum GhApiRet { - ReachedRateLimit { retry_after: Option }, - NotFound, - Success(T), - Unauthorized, -} - -pub(super) fn check_for_status(status: StatusCode, headers: &HeaderMap) -> Option> { +fn check_http_status_and_header(status: StatusCode, headers: &HeaderMap) -> Result<(), GhApiError> { match status { remote::StatusCode::FORBIDDEN if headers @@ -53,7 +46,7 @@ pub(super) fn check_for_status(status: StatusCode, headers: &HeaderMap) -> Op .map(|val| val == "0") .unwrap_or(false) => { - Some(GhApiRet::ReachedRateLimit { + Err(GhApiError::RateLimit { retry_after: headers.get("x-ratelimit-reset").and_then(|value| { let secs = value.to_str().ok()?.parse().ok()?; Some(Duration::from_secs(secs)) @@ -61,13 +54,37 @@ pub(super) fn check_for_status(status: StatusCode, headers: &HeaderMap) -> Op }) } - remote::StatusCode::UNAUTHORIZED => Some(GhApiRet::Unauthorized), - remote::StatusCode::NOT_FOUND => Some(GhApiRet::NotFound), + remote::StatusCode::UNAUTHORIZED => Err(GhApiError::Unauthorized), + remote::StatusCode::NOT_FOUND => Err(GhApiError::NotFound), - _ => None, + _ => Ok(()), } } +pub(super) async fn issue_restful_api( + client: &remote::Client, + path: String, + auth_token: Option<&str>, +) -> Result +where + T: DeserializeOwned, +{ + let mut request_builder = client + .get(Url::parse(&format!("https://api.github.com/{path}"))?) + .header("Accept", "application/vnd.github+json") + .header("X-GitHub-Api-Version", "2022-11-28"); + + if let Some(auth_token) = auth_token { + request_builder = request_builder.bearer_auth(&auth_token); + } + + let response = request_builder.send(false).await?; + + check_http_status_and_header(response.status(), response.headers())?; + + Ok(response.json().await?) +} + #[derive(Deserialize)] enum GraphQLResponse { #[serde(rename = "data")] @@ -90,16 +107,11 @@ fn get_graphql_endpoint() -> &'static Url { }) } -pub(super) enum GraphQLResult { - Data(T), - Else(GhApiRet), -} - -pub(super) async fn issue_graphql_query( +pub(super) async fn issue_graphql_query( client: &remote::Client, query: String, auth_token: &str, -) -> Result, GhApiError> +) -> Result where T: DeserializeOwned, { @@ -115,19 +127,14 @@ where .bearer_auth(&auth_token); let response = request_builder.send(false).await?; - - if let Some(ret) = check_for_status(response.status(), response.headers()) { - return Ok(GraphQLResult::Else(ret)); - } + check_http_status_and_header(response.status(), response.headers())?; let response: GraphQLResponse = response.json().await?; match response { - GraphQLResponse::Data(data) => Ok(GraphQLResult::Data(data)), + GraphQLResponse::Data(data) => Ok(data), GraphQLResponse::Errors(errors) if errors.is_rate_limited() => { - Ok(GraphQLResult::Else(GhApiRet::ReachedRateLimit { - retry_after: None, - })) + Err(GhApiError::RateLimit { retry_after: None }) } GraphQLResponse::Errors(errors) => Err(errors.into()), } diff --git a/crates/binstalk-git-repo-api/src/gh_api_client/error.rs b/crates/binstalk-git-repo-api/src/gh_api_client/error.rs index 1c3b1b33..2612a968 100644 --- a/crates/binstalk-git-repo-api/src/gh_api_client/error.rs +++ b/crates/binstalk-git-repo-api/src/gh_api_client/error.rs @@ -1,4 +1,4 @@ -use std::{error, fmt, io}; +use std::{error, fmt, io, time::Duration}; use binstalk_downloader::remote; use compact_str::{CompactString, ToCompactString}; @@ -31,15 +31,30 @@ pub enum GhApiError { #[error("Remote failed to process GraphQL query: {0}")] GraphQLErrors(#[from] GhGraphQLErrors), + + #[error("Hit rate-limit, retry after {retry_after:?}")] + RateLimit { retry_after: Option }, + + #[error("Corresponding resource is not found")] + NotFound, + + #[error("Does not have permission to access the API")] + Unauthorized, } impl GhApiError { /// Attach context to [`GhApiError`] pub fn context(self, context: impl fmt::Display) -> Self { - Self::Context(Box::new(GhApiContextError { - context: context.to_compact_string(), - err: self, - })) + use GhApiError::*; + + if matches!(self, RateLimit { .. } | NotFound | Unauthorized) { + self + } else { + Self::Context(Box::new(GhApiContextError { + context: context.to_compact_string(), + err: self, + })) + } } } diff --git a/crates/binstalk-git-repo-api/src/gh_api_client/release_artifacts.rs b/crates/binstalk-git-repo-api/src/gh_api_client/release_artifacts.rs index 9c64775e..afa9780c 100644 --- a/crates/binstalk-git-repo-api/src/gh_api_client/release_artifacts.rs +++ b/crates/binstalk-git-repo-api/src/gh_api_client/release_artifacts.rs @@ -5,12 +5,12 @@ use std::{ hash::{Hash, Hasher}, }; -use binstalk_downloader::remote::{self, header::HeaderMap, StatusCode, Url}; +use binstalk_downloader::remote::{self}; use compact_str::CompactString; use serde::Deserialize; use super::{ - common::{self, issue_graphql_query, percent_encode_http_url_path, GraphQLResult}, + common::{issue_graphql_query, issue_restful_api, percent_encode_http_url_path}, GhApiError, GhRelease, }; @@ -65,38 +65,22 @@ impl Artifacts { } } -pub(super) type FetchReleaseRet = common::GhApiRet; - -fn check_for_status(status: StatusCode, headers: &HeaderMap) -> Option { - common::check_for_status(status, headers) -} - async fn fetch_release_artifacts_restful_api( client: &remote::Client, GhRelease { owner, repo, tag }: &GhRelease, auth_token: Option<&str>, -) -> Result { - let mut request_builder = client - .get(Url::parse(&format!( - "https://api.github.com/repos/{owner}/{repo}/releases/tags/{tag}", +) -> Result { + issue_restful_api( + client, + format!( + "repos/{owner}/{repo}/releases/tags/{tag}", owner = percent_encode_http_url_path(owner), repo = percent_encode_http_url_path(repo), tag = percent_encode_http_url_path(tag), - ))?) - .header("Accept", "application/vnd.github+json") - .header("X-GitHub-Api-Version", "2022-11-28"); - - if let Some(auth_token) = auth_token { - request_builder = request_builder.bearer_auth(&auth_token); - } - - let response = request_builder.send(false).await?; - - if let Some(ret) = check_for_status(response.status(), response.headers()) { - Ok(ret) - } else { - Ok(FetchReleaseRet::Success(response.json().await?)) - } + ), + auth_token, + ) + .await } #[derive(Deserialize)] @@ -149,7 +133,7 @@ async fn fetch_release_artifacts_graphql_api( client: &remote::Client, GhRelease { owner, repo, tag }: &GhRelease, auth_token: &str, -) -> Result { +) -> Result { let mut artifacts = Artifacts::default(); let mut cond = FilterCondition::Init; @@ -171,10 +155,7 @@ query {{ }}"# ); - let data: GraphQLData = match issue_graphql_query(client, query, auth_token).await? { - GraphQLResult::Data(data) => data, - GraphQLResult::Else(ret) => return Ok(ret), - }; + let data: GraphQLData = issue_graphql_query(client, query, auth_token).await?; let assets = data .repository @@ -191,10 +172,10 @@ query {{ } => { cond = FilterCondition::After(end_cursor); } - _ => break Ok(FetchReleaseRet::Success(artifacts)), + _ => break Ok(artifacts), } } else { - break Ok(FetchReleaseRet::NotFound); + break Err(GhApiError::NotFound); } } } @@ -203,7 +184,7 @@ pub(super) async fn fetch_release_artifacts( client: &remote::Client, release: &GhRelease, auth_token: Option<&str>, -) -> Result { +) -> Result { if let Some(auth_token) = auth_token { let res = fetch_release_artifacts_graphql_api(client, release, auth_token) .await @@ -211,7 +192,7 @@ pub(super) async fn fetch_release_artifacts( match res { // Fallback to Restful API - Ok(FetchReleaseRet::Unauthorized) => (), + Err(GhApiError::Unauthorized) => (), res => return res, } } diff --git a/crates/binstalk-git-repo-api/src/gh_api_client/repo_info.rs b/crates/binstalk-git-repo-api/src/gh_api_client/repo_info.rs index ec144398..2bb7e2ec 100644 --- a/crates/binstalk-git-repo-api/src/gh_api_client/repo_info.rs +++ b/crates/binstalk-git-repo-api/src/gh_api_client/repo_info.rs @@ -1,9 +1,8 @@ -use binstalk_downloader::remote::{header::HeaderMap, StatusCode, Url}; use compact_str::CompactString; use serde::Deserialize; use super::{ - common::{self, issue_graphql_query, percent_encode_http_url_path, GraphQLResult}, + common::{issue_graphql_query, issue_restful_api, percent_encode_http_url_path}, remote, GhApiError, GhRepo, }; @@ -32,37 +31,21 @@ impl RepoInfo { } } -pub(super) type FetchRepoInfoRet = common::GhApiRet; - -fn check_for_status(status: StatusCode, headers: &HeaderMap) -> Option { - common::check_for_status(status, headers) -} - async fn fetch_repo_info_restful_api( client: &remote::Client, GhRepo { owner, repo }: &GhRepo, auth_token: Option<&str>, -) -> Result { - let mut request_builder = client - .get(Url::parse(&format!( - "https://api.github.com/repos/{owner}/{repo}", +) -> Result { + issue_restful_api( + client, + format!( + "repos/{owner}/{repo}", owner = percent_encode_http_url_path(owner), repo = percent_encode_http_url_path(repo), - ))?) - .header("Accept", "application/vnd.github+json") - .header("X-GitHub-Api-Version", "2022-11-28"); - - if let Some(auth_token) = auth_token { - request_builder = request_builder.bearer_auth(&auth_token); - } - - let response = request_builder.send(false).await?; - - if let Some(ret) = check_for_status(response.status(), response.headers()) { - Ok(ret) - } else { - Ok(FetchRepoInfoRet::Success(response.json().await?)) - } + ), + auth_token, + ) + .await } #[derive(Deserialize)] @@ -74,7 +57,7 @@ async fn fetch_repo_info_graphql_api( client: &remote::Client, GhRepo { owner, repo }: &GhRepo, auth_token: &str, -) -> Result { +) -> Result { let query = format!( r#" query {{ @@ -88,17 +71,14 @@ query {{ }}"# ); - match issue_graphql_query(client, query, auth_token).await? { - GraphQLResult::Data(repo_info) => Ok(common::GhApiRet::Success(repo_info)), - GraphQLResult::Else(ret) => Ok(ret), - } + issue_graphql_query(client, query, auth_token).await } pub(super) async fn fetch_repo_info( client: &remote::Client, repo: &GhRepo, auth_token: Option<&str>, -) -> Result { +) -> Result { if let Some(auth_token) = auth_token { let res = fetch_repo_info_graphql_api(client, repo, auth_token) .await @@ -106,7 +86,7 @@ pub(super) async fn fetch_repo_info( match res { // Fallback to Restful API - Ok(FetchRepoInfoRet::Unauthorized) => (), + Err(GhApiError::Unauthorized) => (), res => return res, } } diff --git a/crates/binstalk/src/helpers.rs b/crates/binstalk/src/helpers.rs index 560fc3f3..4e5792e6 100644 --- a/crates/binstalk/src/helpers.rs +++ b/crates/binstalk/src/helpers.rs @@ -1,5 +1,8 @@ pub mod jobserver_client; -pub mod remote; +pub mod remote { + pub use binstalk_downloader::remote::*; + pub use url::ParseError as UrlParseError; +} pub(crate) mod target_triple; pub mod tasks; diff --git a/crates/binstalk/src/helpers/remote.rs b/crates/binstalk/src/helpers/remote.rs deleted file mode 100644 index 66dc5490..00000000 --- a/crates/binstalk/src/helpers/remote.rs +++ /dev/null @@ -1,54 +0,0 @@ -pub use binstalk_downloader::remote::*; -pub use url::ParseError as UrlParseError; - -use std::sync::{ - atomic::{AtomicBool, Ordering::Relaxed}, - Once, -}; -use tracing::{debug, warn}; - -use super::gh_api_client::{GhApiClient, GhReleaseArtifact, HasReleaseArtifact}; -use crate::errors::BinstallError; - -/// This function returns a future where its size should be at most size of -/// 2 pointers. -pub async fn does_url_exist( - client: Client, - gh_api_client: GhApiClient, - url: &Url, -) -> Result { - static GH_API_CLIENT_FAILED: AtomicBool = AtomicBool::new(false); - static WARN_RATE_LIMIT_ONCE: Once = Once::new(); - static WARN_UNAUTHORIZED_ONCE: Once = Once::new(); - - debug!("Checking for package at: '{url}'"); - - if !GH_API_CLIENT_FAILED.load(Relaxed) { - if let Some(artifact) = GhReleaseArtifact::try_extract_from_url(url) { - debug!("Using GitHub API to check for existence of artifact, which will also cache the API response"); - - // The future returned has the same size as a pointer - match gh_api_client.has_release_artifact(artifact).await? { - HasReleaseArtifact::Yes { .. } => return Ok(true), - HasReleaseArtifact::No | HasReleaseArtifact::NoSuchRelease => return Ok(false), - - HasReleaseArtifact::RateLimit { retry_after } => { - WARN_RATE_LIMIT_ONCE.call_once(|| { - warn!("Your GitHub API token (if any) has reached its rate limit and cannot be used again until {retry_after:?}, so we will fallback to HEAD/GET on the url."); - warn!("If you did not supply a github token, consider doing so: GitHub limits unauthorized users to 60 requests per hour per origin IP address."); - }); - } - HasReleaseArtifact::Unauthorized => { - WARN_UNAUTHORIZED_ONCE.call_once(|| { - warn!("GitHub API somehow requires a token for the API access, so we will fallback to HEAD/GET on the url."); - warn!("Please consider supplying a token to cargo-binstall to speedup resolution."); - }); - } - } - - GH_API_CLIENT_FAILED.store(true, Relaxed); - } - } - - Ok(Box::pin(client.remote_gettable(url.clone())).await?) -}