diff --git a/cpp-linter/src/common_fs/file_filter.rs b/cpp-linter/src/common_fs/file_filter.rs index 82697e0..fc848cd 100644 --- a/cpp-linter/src/common_fs/file_filter.rs +++ b/cpp-linter/src/common_fs/file_filter.rs @@ -61,7 +61,8 @@ impl FileFilter { for line in read_buf.split('\n') { if line.trim_start().starts_with("path") { assert!(line.find('=').unwrap() > 0); - let submodule = String::from("./") + line.split('=').last().unwrap().trim(); + let submodule = + String::from("./") + line.split('=').next_back().unwrap().trim(); log::debug!("Found submodule: {submodule}"); let mut is_ignored = true; for pat in &self.not_ignored { @@ -159,7 +160,7 @@ impl FileFilter { let mut is_hidden = false; let parent = path .components() - .last() + .next_back() .ok_or(anyhow!("parent directory not known for {path:?}"))?; if parent.as_os_str().to_str().unwrap().starts_with('.') { is_hidden = true; diff --git a/cpp-linter/src/logger.rs b/cpp-linter/src/logger.rs index 1c1290e..49a5c7d 100644 --- a/cpp-linter/src/logger.rs +++ b/cpp-linter/src/logger.rs @@ -1,9 +1,12 @@ //! A module to initialize and customize the logger object used in (most) stdout. -use std::env; +use std::{ + env, + io::{stdout, Write}, +}; use colored::{control::set_override, Colorize}; -use log::{Level, LevelFilter, Metadata, Record}; +use log::{Level, LevelFilter, Log, Metadata, Record}; #[derive(Default)] struct SimpleLogger; @@ -21,21 +24,43 @@ impl SimpleLogger { } } -impl log::Log for SimpleLogger { +impl Log for SimpleLogger { fn enabled(&self, metadata: &Metadata) -> bool { metadata.level() <= log::max_level() } fn log(&self, record: &Record) { + let mut stdout = stdout().lock(); if record.target() == "CI_LOG_GROUPING" { // this log is meant to manipulate a CI workflow's log grouping - println!("{}", record.args()); + writeln!(stdout, "{}", record.args()).expect("Failed to write log command to stdout"); + stdout + .flush() + .expect("Failed to flush log command to stdout"); } else if self.enabled(record.metadata()) { - println!( - "[{}]: {}", - Self::level_color(&record.level()), - record.args() - ); + let module = record.module_path(); + if module.is_none_or(|v| v.starts_with("cpp_linter")) { + writeln!( + stdout, + "[{}]: {}", + Self::level_color(&record.level()), + record.args() + ) + .expect("Failed to write log message to stdout"); + } else { + writeln!( + stdout, + "[{}]{{{}:{}}}: {}", + Self::level_color(&record.level()), + module.unwrap(), // safe to unwrap here because the None case is caught above + record.line().unwrap_or_default(), + record.args() + ) + .expect("Failed to write detailed log message to stdout"); + } + stdout + .flush() + .expect("Failed to flush log message to stdout"); } } diff --git a/cpp-linter/src/rest_api/github/mod.rs b/cpp-linter/src/rest_api/github/mod.rs index f44a588..756c81a 100644 --- a/cpp-linter/src/rest_api/github/mod.rs +++ b/cpp-linter/src/rest_api/github/mod.rs @@ -16,7 +16,7 @@ use reqwest::{ }; // project specific modules/crates -use super::{RestApiClient, RestApiRateLimitHeaders}; +use super::{send_api_request, RestApiClient, RestApiRateLimitHeaders}; use crate::clang_tools::clang_format::tally_format_advice; use crate::clang_tools::clang_tidy::tally_tidy_advice; use crate::clang_tools::ClangVersions; @@ -148,14 +148,9 @@ impl RestApiClient for GithubApiClient { None, Some(diff_header), )?; - let response = Self::send_api_request( - self.client.clone(), - request, - self.rate_limit_headers.to_owned(), - 0, - ) - .await - .with_context(|| "Failed to get list of changed files.")?; + let response = send_api_request(&self.client, request, &self.rate_limit_headers) + .await + .with_context(|| "Failed to get list of changed files.")?; if response.status().is_success() { Ok(parse_diff_from_buf( &response.bytes().await?, diff --git a/cpp-linter/src/rest_api/github/specific_api.rs b/cpp-linter/src/rest_api/github/specific_api.rs index a9f789e..67b0747 100644 --- a/cpp-linter/src/rest_api/github/specific_api.rs +++ b/cpp-linter/src/rest_api/github/specific_api.rs @@ -17,7 +17,7 @@ use crate::{ cli::{FeedbackInput, LinesChangedOnly}, common_fs::{FileFilter, FileObj}, git::parse_diff_from_buf, - rest_api::{RestApiRateLimitHeaders, COMMENT_MARKER, USER_AGENT}, + rest_api::{send_api_request, RestApiRateLimitHeaders, COMMENT_MARKER, USER_AGENT}, }; use super::{ @@ -94,14 +94,9 @@ impl GithubApiClient { while let Some(ref endpoint) = url { let request = Self::make_api_request(&self.client, endpoint.as_str(), Method::GET, None, None)?; - let response = Self::send_api_request( - self.client.clone(), - request, - self.rate_limit_headers.clone(), - 0, - ) - .await - .with_context(|| "Failed to get paginated list of changed files")?; + let response = send_api_request(&self.client, request, &self.rate_limit_headers) + .await + .with_context(|| "Failed to get paginated list of changed files")?; url = Self::try_next_page(response.headers()); let files_list = if self.event_name != "pull_request" { let json_value: PushEventFiles = serde_json::from_str(&response.text().await?) @@ -233,14 +228,7 @@ impl GithubApiClient { Some(serde_json::json!(&payload).to_string()), None, )?; - match Self::send_api_request( - self.client.clone(), - request, - self.rate_limit_headers.to_owned(), - 0, - ) - .await - { + match send_api_request(&self.client, request, &self.rate_limit_headers).await { Ok(response) => { Self::log_response(response, "Failed to post thread comment").await; } @@ -270,13 +258,7 @@ impl GithubApiClient { while let Some(ref endpoint) = comments_url { let request = Self::make_api_request(&self.client, endpoint.as_str(), Method::GET, None, None)?; - let result = Self::send_api_request( - self.client.clone(), - request, - self.rate_limit_headers.to_owned(), - 0, - ) - .await; + let result = send_api_request(&self.client, request, &self.rate_limit_headers).await; match result { Err(e) => { log::error!("Failed to get list of existing thread comments: {e:?}"); @@ -330,11 +312,10 @@ impl GithubApiClient { None, None, )?; - match Self::send_api_request( - self.client.clone(), + match send_api_request( + &self.client, req, - self.rate_limit_headers.to_owned(), - 0, + &self.rate_limit_headers, ) .await { @@ -391,12 +372,7 @@ impl GithubApiClient { // if we got here, then we know that it is a self.pull_request is a valid value .join(self.pull_request.to_string().as_str())?; let request = Self::make_api_request(&self.client, url.as_str(), Method::GET, None, None)?; - let response = Self::send_api_request( - self.client.clone(), - request, - self.rate_limit_headers.clone(), - 0, - ); + let response = send_api_request(&self.client, request, &self.rate_limit_headers); let url = Url::parse(format!("{}/", url).as_str())?.join("reviews")?; let dismissal = self.dismiss_outdated_reviews(&url); @@ -462,7 +438,7 @@ impl GithubApiClient { dismissal.await?; // free up the `url` variable let request = Self::make_api_request( &self.client, - url.clone(), + url, Method::POST, Some( serde_json::to_string(&payload) @@ -470,14 +446,7 @@ impl GithubApiClient { ), None, )?; - match Self::send_api_request( - self.client.clone(), - request, - self.rate_limit_headers.clone(), - 0, - ) - .await - { + match send_api_request(&self.client, request, &self.rate_limit_headers).await { Ok(response) => { if !response.status().is_success() { Self::log_response(response, "Failed to post a new PR review").await; @@ -496,13 +465,7 @@ impl GithubApiClient { while let Some(ref endpoint) = url_ { let request = Self::make_api_request(&self.client, endpoint.as_str(), Method::GET, None, None)?; - let result = Self::send_api_request( - self.client.clone(), - request, - self.rate_limit_headers.clone(), - 0, - ) - .await; + let result = send_api_request(&self.client, request, &self.rate_limit_headers).await; match result { Err(e) => { log::error!("Failed to get a list of existing PR reviews: {e:?}"); @@ -538,11 +501,10 @@ impl GithubApiClient { Some(REVIEW_DISMISSAL.to_string()), None, ) { - match Self::send_api_request( - self.client.clone(), + match send_api_request( + &self.client, req, - self.rate_limit_headers.clone(), - 0, + &self.rate_limit_headers, ) .await { diff --git a/cpp-linter/src/rest_api/mod.rs b/cpp-linter/src/rest_api/mod.rs index 059ea9b..0e35ea9 100644 --- a/cpp-linter/src/rest_api/mod.rs +++ b/cpp-linter/src/rest_api/mod.rs @@ -64,7 +64,7 @@ pub trait RestApiClient { /// Construct a HTTP request to be sent. /// - /// The idea here is that this method is called before [`RestApiClient::send_api_request()`]. + /// The idea here is that this method is called before [`send_api_request()`]. /// ```ignore /// let request = Self::make_api_request( /// &self.client, @@ -73,15 +73,10 @@ pub trait RestApiClient { /// None, /// None /// ); - /// let response = Self::send_api_request( - /// self.client.clone(), - /// request, - /// self.rest_api_headers.clone(), - /// 0, // start recursion count at 0 (max iterations is 4) - /// ); + /// let response = send_api_request(&self.client, request, &self.rest_api_headers); /// match response.await { - /// Some(res) => {/* handle response */} - /// None => {/* handle failure */} + /// Ok(res) => {/* handle response */} + /// Err(e) => {/* handle failure */} /// } /// ``` fn make_api_request( @@ -103,104 +98,6 @@ pub trait RestApiClient { req.build().map_err(Error::from) } - /// A convenience function to send HTTP requests and respect a REST API rate limits. - /// - /// This method must own all the data passed to it because asynchronous recursion is used. - /// Recursion is needed when a secondary rate limit is hit. The server tells the client that - /// it should back off and retry after a specified time interval. - /// - /// Note, pass `0` to the `retries` parameter, which is used to count recursive iterations. - /// This function will recur a maximum of 4 times, and this only happens when the response's - /// headers includes a retry interval. - fn send_api_request( - client: Client, - request: Request, - rate_limit_headers: RestApiRateLimitHeaders, - retries: u8, - ) -> impl Future> + Send { - async move { - static MAX_RETRIES: u8 = 5; - for i in retries..MAX_RETRIES { - let result = client - .execute(request.try_clone().ok_or(anyhow!( - "Failed to clone request object for recursive behavior" - ))?) - .await; - if let Ok(response) = &result { - if [403u16, 429u16].contains(&response.status().as_u16()) { - // rate limit may have been exceeded - - // check if primary rate limit was violated; panic if so. - let mut requests_remaining = None; - if let Some(remaining) = - response.headers().get(&rate_limit_headers.remaining) - { - if let Ok(count) = remaining.to_str() { - if let Ok(value) = count.parse::() { - requests_remaining = Some(value); - } else { - log::debug!( - "Failed to parse i64 from remaining attempts about rate limit: {count}" - ); - } - } - } else { - // NOTE: I guess it is sometimes valid for a request to - // not include remaining rate limit attempts - log::debug!( - "Response headers do not include remaining API usage count" - ); - } - if requests_remaining.is_some_and(|v| v <= 0) { - if let Some(reset_value) = - response.headers().get(&rate_limit_headers.reset) - { - if let Ok(epoch) = reset_value.to_str() { - if let Ok(value) = epoch.parse::() { - if let Some(reset) = DateTime::from_timestamp(value, 0) { - return Err(anyhow!( - "REST API rate limit exceeded! Resets at {}", - reset - )); - } - } else { - log::debug!( - "Failed to parse i64 from reset time about rate limit: {epoch}" - ); - } - } - } else { - log::debug!("Response headers does not include a reset timestamp"); - } - return Err(anyhow!("REST API rate limit exceeded!")); - } - - // check if secondary rate limit is violated; backoff and try again. - if let Some(retry_value) = response.headers().get(&rate_limit_headers.retry) - { - if let Ok(retry_str) = retry_value.to_str() { - if let Ok(retry) = retry_str.parse::() { - let interval = Duration::from_secs(retry + (i as u64).pow(2)); - tokio::time::sleep(interval).await; - } else { - log::debug!( - "Failed to parse u64 from retry interval about rate limit: {retry_str}" - ); - } - } - continue; - } - } - return result.map_err(Error::from); - } - return result.map_err(Error::from); - } - Err(anyhow!( - "REST API secondary rate limit exceeded after {MAX_RETRIES} retries." - )) - } - } - /// A way to get the list of changed files using REST API calls. It is this method's /// job to parse diff blobs and return a list of changed files. /// @@ -317,6 +214,91 @@ pub trait RestApiClient { } } +const MAX_RETRIES: u8 = 5; + +/// A convenience function to send HTTP requests and respect a REST API rate limits. +/// +/// This method respects both primary and secondary rate limits. +/// In the event where the secondary rate limits is reached, +/// this function will wait for a time interval specified the server and retry afterward. +async fn send_api_request( + client: &Client, + request: Request, + rate_limit_headers: &RestApiRateLimitHeaders, +) -> Result { + for i in 0..MAX_RETRIES { + let result = client + .execute(request.try_clone().ok_or(anyhow!( + "Failed to clone request object for recursive behavior" + ))?) + .await; + if let Ok(response) = &result { + if [403u16, 429u16].contains(&response.status().as_u16()) { + // rate limit may have been exceeded + + // check if primary rate limit was violated; panic if so. + let mut requests_remaining = None; + if let Some(remaining) = response.headers().get(&rate_limit_headers.remaining) { + if let Ok(count) = remaining.to_str() { + if let Ok(value) = count.parse::() { + requests_remaining = Some(value); + } else { + log::debug!( + "Failed to parse i64 from remaining attempts about rate limit: {count}" + ); + } + } + } else { + // NOTE: I guess it is sometimes valid for a request to + // not include remaining rate limit attempts + log::debug!("Response headers do not include remaining API usage count"); + } + if requests_remaining.is_some_and(|v| v <= 0) { + if let Some(reset_value) = response.headers().get(&rate_limit_headers.reset) { + if let Ok(epoch) = reset_value.to_str() { + if let Ok(value) = epoch.parse::() { + if let Some(reset) = DateTime::from_timestamp(value, 0) { + return Err(anyhow!( + "REST API rate limit exceeded! Resets at {}", + reset + )); + } + } else { + log::debug!( + "Failed to parse i64 from reset time about rate limit: {epoch}" + ); + } + } + } else { + log::debug!("Response headers does not include a reset timestamp"); + } + return Err(anyhow!("REST API rate limit exceeded!")); + } + + // check if secondary rate limit is violated; backoff and try again. + if let Some(retry_value) = response.headers().get(&rate_limit_headers.retry) { + if let Ok(retry_str) = retry_value.to_str() { + if let Ok(retry) = retry_str.parse::() { + let interval = Duration::from_secs(retry + (i as u64).pow(2)); + tokio::time::sleep(interval).await; + } else { + log::debug!( + "Failed to parse u64 from retry interval about rate limit: {retry_str}" + ); + } + } + continue; + } + } + return result.map_err(Error::from); + } + return result.map_err(Error::from); + } + Err(anyhow!( + "REST API secondary rate limit exceeded after {MAX_RETRIES} retries." + )) +} + fn make_format_comment( files: &[Arc>], comment: &mut String, @@ -419,7 +401,7 @@ mod test { logger, }; - use super::{RestApiClient, RestApiRateLimitHeaders}; + use super::{send_api_request, RestApiClient, RestApiRateLimitHeaders}; /// A dummy struct to impl RestApiClient #[derive(Default)] @@ -579,7 +561,7 @@ mod test { } let request = TestClient::make_api_request(&client, server.url(), Method::GET, None, None).unwrap(); - TestClient::send_api_request(client.clone(), request, rate_limit_headers.clone(), 0) + send_api_request(&client, request, &rate_limit_headers) .await .unwrap(); }