resort to paginated requests for changed files

2bndy5 · 2bndy5 · commit 2aaf13d80558 · 2024-09-18T03:14:22.000-07:00
resolves #32
diff --git a/cpp-linter-lib/src/rest_api/github_api.rs b/cpp-linter-lib/src/rest_api/github_api.rs
@@ -192,20 +192,38 @@ impl RestApiClient for GithubApiClient {
                 .unwrap();
             let mut diff_header = HeaderMap::new();
             diff_header.insert("Accept", "application/vnd.github.diff".parse().unwrap());
-            let request =
-                Self::make_api_request(&self.client, url, Method::GET, None, Some(diff_header));
+            let request = Self::make_api_request(
+                &self.client,
+                url.as_str(),
+                Method::GET,
+                None,
+                Some(diff_header),
+            );
             let response = Self::send_api_request(
                 self.client.clone(),
                 request,
-                true,
+                false,
                 self.rate_limit_headers.to_owned(),
                 0,
             )
-            .await
-            .unwrap()
-            .text;
-
-            parse_diff_from_buf(response.as_bytes(), file_filter)
+            .await;
+            match response {
+                Some(response) => {
+                    if response.status.is_success() {
+                        return parse_diff_from_buf(response.text.as_bytes(), file_filter);
+                    } else {
+                        let endpoint = if is_pr {
+                            Url::parse(format!("{}/files", url.as_str()).as_str())
+                                .expect("failed to parse URL endpoint")
+                        } else {
+                            url
+                        };
+                        self.get_changed_files_paginated(endpoint, file_filter)
+                            .await
+                    }
+                }
+                None => panic!("Failed to get list of changed files."),
+            }
         } else {
             // get diff from libgit2 API
             let repo = open_repo(".")
@@ -215,6 +233,54 @@ impl RestApiClient for GithubApiClient {
         }
     }
 
+    async fn get_changed_files_paginated(
+        &self,
+        url: Url,
+        file_filter: &FileFilter,
+    ) -> Vec<FileObj> {
+        let mut url = Some(Url::parse_with_params(url.as_str(), &[("page", "1")]).unwrap());
+        let mut files = vec![];
+        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,
+                true,
+                self.rate_limit_headers.clone(),
+                0,
+            )
+            .await;
+            if let Some(response) = response {
+                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)
+                        .expect("Failed to deserialize list of changed files from json response");
+                    json_value.files
+                } else {
+                    serde_json::from_str::<Vec<GithubChangedFile>>(&response.text).expect(
+                        "Failed to deserialize list of file changes from Pull Request event.",
+                    )
+                };
+                for file in files_list {
+                    if let Some(patch) = file.patch {
+                        let diff = format!(
+                            "diff --git a/{old} b/{new}\n--- a/{old}\n+++ b/{new}\n{patch}",
+                            old = file.previous_filename.unwrap_or(file.filename.clone()),
+                            new = file.filename,
+                        );
+                        if let Some(file_obj) =
+                            parse_diff_from_buf(diff.as_bytes(), file_filter).first()
+                        {
+                            files.push(file_obj.to_owned());
+                        }
+                    }
+                }
+            }
+        }
+        files
+    }
+
     async fn post_feedback(
         &self,
         files: &[Arc<Mutex<FileObj>>],
@@ -671,6 +737,24 @@ impl From<Suggestion> for ReviewDiffComment {
     }
 }
 
+/// A structure for deserializing a single changed file in a CI event.
+#[derive(Debug, Deserialize, PartialEq, Clone)]
+struct GithubChangedFile {
+    /// The file's name (including relative path to repo root)
+    pub filename: String,
+    /// If renamed, this will be the file's old name as a [`Some`], otherwise [`None`].
+    pub previous_filename: Option<String>,
+    /// The individual patch that describes the file's changes.
+    pub patch: Option<String>,
+}
+
+/// A structure for deserializing a Push event's changed files.
+#[derive(Debug, Deserialize, PartialEq, Clone)]
+struct PushEventFiles {
+    /// The list of changed files.
+    pub files: Vec<GithubChangedFile>,
+}
+
 /// A structure for deserializing a comment from a response's json.
 #[derive(Debug, Deserialize, PartialEq, Clone)]
 struct PullRequestInfo {
@@ -840,7 +924,7 @@ mod test {
         }
         let request =
             GithubApiClient::make_api_request(&client.client, url, Method::GET, None, None);
-        let _response = GithubApiClient::send_api_request(
+        GithubApiClient::send_api_request(
             client.client.clone(),
             request,
             true,
diff --git a/cpp-linter-lib/src/rest_api/mod.rs b/cpp-linter-lib/src/rest_api/mod.rs
@@ -233,6 +233,16 @@ pub trait RestApiClient {
         file_filter: &FileFilter,
     ) -> impl Future<Output = Vec<FileObj>>;
 
+    /// A way to get the list of changed files using REST API calls that employ a paginated response.
+    ///
+    /// This is a helper to [`RestApiClient::get_list_of_changed_files()`] but takes a formulated URL
+    /// endpoint based on the context of the triggering CI event.
+    fn get_changed_files_paginated(
+        &self,
+        url: Url,
+        file_filter: &FileFilter,
+    ) -> impl Future<Output = Vec<FileObj>>;
+
     /// Makes a comment in MarkDown syntax based on the concerns in `format_advice` and
     /// `tidy_advice` about the given set of `files`.
     ///
diff --git a/cpp-linter-lib/tests/comments.rs b/cpp-linter-lib/tests/comments.rs
@@ -230,7 +230,7 @@ async fn setup(lib_root: &Path, test_params: &TestParams) {
 }
 
 async fn test_comment(test_params: &TestParams) {
-    let tmp_dir = create_test_space();
+    let tmp_dir = create_test_space(true);
     let lib_root = env::current_dir().unwrap();
     env::set_current_dir(tmp_dir.path()).unwrap();
     setup(&lib_root, test_params).await;
diff --git a/cpp-linter-lib/tests/common/mod.rs b/cpp-linter-lib/tests/common/mod.rs
@@ -20,7 +20,7 @@ use tempfile::TempDir;
 ///
 /// The returned directory object will automatically delete the
 /// temporary folder when it is dropped out of scope.
-pub fn create_test_space() -> TempDir {
+pub fn create_test_space(setup_meson: bool) -> TempDir {
     let tmp = TempDir::new().unwrap();
     fs::create_dir(tmp.path().join("src")).unwrap();
     let src = fs::read_dir("tests/demo").unwrap();
@@ -32,6 +32,10 @@ pub fn create_test_space() -> TempDir {
         }
     }
 
+    if !setup_meson {
+        return tmp;
+    }
+
     // generate compilation database with meson (& ninja)
     let test_dir = tmp.path().join("src");
     let mut cmd = Command::new("meson");
diff --git a/cpp-linter-lib/tests/paginated_changed_files.rs b/cpp-linter-lib/tests/paginated_changed_files.rs
@@ -0,0 +1,145 @@
+mod common;
+use chrono::Utc;
+use common::{create_test_space, mock_server};
+use mockito::Matcher;
+use tempfile::{NamedTempFile, TempDir};
+
+use cpp_linter_lib::{
+    common_fs::FileFilter,
+    rest_api::{github_api::GithubApiClient, RestApiClient},
+};
+use std::{env, io::Write, path::Path};
+
+#[derive(PartialEq)]
+enum EventType {
+    Push,
+    PullRequest(u64),
+}
+
+const REPO: &str = "cpp-linter/test-cpp-linter-action";
+const SHA: &str = "DEADBEEF";
+const TOKEN: &str = "123456";
+const RESET_RATE_LIMIT_HEADER: &str = "x-ratelimit-reset";
+const REMAINING_RATE_LIMIT_HEADER: &str = "x-ratelimit-remaining";
+
+async fn get_paginated_changes(lib_root: &Path, event_type: EventType) {
+    env::set_var("GITHUB_REPOSITORY", REPO);
+    env::set_var("GITHUB_SHA", SHA);
+    env::set_var("GITHUB_TOKEN", TOKEN);
+    env::set_var("CI", "true");
+    env::set_var(
+        "GITHUB_EVENT_NAME",
+        if event_type == EventType::Push {
+            "push"
+        } else {
+            "pull_request"
+        },
+    );
+    let tmp = TempDir::new().expect("Failed to create a temp dir for test");
+    let mut event_payload = NamedTempFile::new_in(tmp.path())
+        .expect("Failed to spawn a tmp file for test event payload");
+    env::set_var("GITHUB_EVENT_PATH", event_payload.path());
+    if let EventType::PullRequest(pr_number) = event_type {
+        event_payload
+            .write_all(
+                serde_json::json!({"number": pr_number})
+                    .to_string()
+                    .as_bytes(),
+            )
+            .expect("Failed to write data to test event payload file")
+    }
+
+    let reset_timestamp = (Utc::now().timestamp() + 60).to_string();
+    let asset_path = format!("{}/tests/paginated_changes", lib_root.to_str().unwrap());
+
+    let mut server = mock_server().await;
+    env::set_var("GITHUB_API_URL", server.url());
+    env::set_current_dir(tmp.path()).unwrap();
+    let gh_client = GithubApiClient::new();
+
+    let mut mocks = vec![];
+    let diff_end_point = format!(
+        "/repos/{REPO}/{}",
+        if let EventType::PullRequest(pr) = event_type {
+            format!("pulls/{pr}")
+        } else {
+            format!("commits/{SHA}")
+        }
+    );
+    mocks.push(
+        server
+            .mock("GET", diff_end_point.as_str())
+            .match_header("Accept", "application/vnd.github.diff")
+            .match_header("Authorization", TOKEN)
+            .with_header(REMAINING_RATE_LIMIT_HEADER, "50")
+            .with_header(RESET_RATE_LIMIT_HEADER, reset_timestamp.as_str())
+            .with_status(403)
+            .create(),
+    );
+    let pg_end_point = if event_type == EventType::Push {
+        diff_end_point.clone()
+    } else {
+        format!("{diff_end_point}/files")
+    };
+    for pg in 1..=2 {
+        let link = if pg == 1 {
+            format!("<{}{pg_end_point}?page=2>; rel=\"next\"", server.url())
+        } else {
+            "".to_string()
+        };
+        mocks.push(
+            server
+                .mock("GET", pg_end_point.as_str())
+                .match_header("Accept", "application/vnd.github.raw+json")
+                .match_header("Authorization", TOKEN)
+                .match_query(Matcher::UrlEncoded("page".to_string(), pg.to_string()))
+                .with_header(REMAINING_RATE_LIMIT_HEADER, "50")
+                .with_header(RESET_RATE_LIMIT_HEADER, reset_timestamp.as_str())
+                .with_body_from_file(format!(
+                    "{asset_path}/{}_files_pg{pg}.json",
+                    if event_type == EventType::Push {
+                        "push"
+                    } else {
+                        "pull_request"
+                    }
+                ))
+                .with_header("link", link.as_str())
+                .create(),
+        );
+    }
+
+    let file_filter = FileFilter::new(&[], vec!["cpp".to_string(), "hpp".to_string()]);
+    let files = gh_client.get_list_of_changed_files(&file_filter).await;
+    assert_eq!(files.len(), 2);
+    for file in files {
+        assert!(["src/demo.cpp", "src/demo.hpp"].contains(
+            &file
+                .name
+                .as_path()
+                .to_str()
+                .expect("Failed to get file name from path")
+        ));
+    }
+    for mock in mocks {
+        mock.assert();
+    }
+}
+
+async fn test_get_changes(event_type: EventType) {
+    let tmp_dir = create_test_space(false);
+    let lib_root = env::current_dir().unwrap();
+    env::set_current_dir(tmp_dir.path()).unwrap();
+    get_paginated_changes(&lib_root, event_type).await;
+    env::set_current_dir(lib_root.as_path()).unwrap();
+    drop(tmp_dir);
+}
+
+#[tokio::test]
+async fn get_push_files_paginated() {
+    test_get_changes(EventType::Push).await
+}
+
+#[tokio::test]
+async fn get_pr_files_paginated() {
+    test_get_changes(EventType::PullRequest(42)).await
+}
diff --git a/cpp-linter-lib/tests/paginated_changes/pull_request_files_pg1.json b/cpp-linter-lib/tests/paginated_changes/pull_request_files_pg1.json
@@ -0,0 +1,27 @@
+[
+  {
+    "sha": "52501fa1dc96d6bc6f8a155816df041b1de975d9",
+    "filename": ".github/workflows/cpp-lint-package.yml",
+    "status": "modified",
+    "additions": 9,
+    "deletions": 5,
+    "changes": 14,
+    "blob_url": "https://github.com/cpp-linter/test-cpp-linter-action/blob/635a9c57bdcca07b99ddef52c2640337c50280b1/.github%2Fworkflows%2Fcpp-lint-package.yml",
+    "raw_url": "https://github.com/cpp-linter/test-cpp-linter-action/raw/635a9c57bdcca07b99ddef52c2640337c50280b1/.github%2Fworkflows%2Fcpp-lint-package.yml",
+    "contents_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contents/.github%2Fworkflows%2Fcpp-lint-package.yml?ref=635a9c57bdcca07b99ddef52c2640337c50280b1",
+    "patch": "@@ -7,16 +7,17 @@ on:\n         description: 'which branch to test'\n         default: 'main'\n         required: true\n+  pull_request:\n \n jobs:\n   cpp-linter:\n     runs-on: windows-latest\n \n     strategy:\n       matrix:\n-        clang-version: ['7', '8', '9','10', '11', '12', '13', '14', '15', '16', '17']\n+        clang-version: ['10', '11', '12', '13', '14', '15', '16', '17']\n         repo: ['cpp-linter/cpp-linter']\n-        branch: ['${{ inputs.branch }}']\n+        branch: ['pr-review-suggestions']\n       fail-fast: false\n \n     steps:\n@@ -62,10 +63,13 @@ jobs:\n           -i=build \n           -p=build \n           -V=${{ runner.temp }}/llvm \n-          -f=false \n           --extra-arg=\"-std=c++14 -Wall\" \n-          --thread-comments=${{ matrix.clang-version == '12' }} \n-          -a=${{ matrix.clang-version == '12' }}\n+          --file-annotations=false\n+          --lines-changed-only=false\n+          --extension=h,c\n+          --thread-comments=${{ matrix.clang-version == '16' }} \n+          --tidy-review=${{ matrix.clang-version == '16' }}\n+          --format-review=${{ matrix.clang-version == '16' }}\n \n       - name: Fail fast?!\n         if: steps.linter.outputs.checks-failed > 0"
+  },
+  {
+    "sha": "1bf553e06e4b7c6c9a9be5da4845acbdeb04f6a5",
+    "filename": "src/demo.cpp",
+    "previous_filename": "src/demo.c",
+    "status": "modified",
+    "additions": 11,
+    "deletions": 10,
+    "changes": 21,
+    "blob_url": "https://github.com/cpp-linter/test-cpp-linter-action/blob/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.cpp",
+    "raw_url": "https://github.com/cpp-linter/test-cpp-linter-action/raw/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.cpp",
+    "contents_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contents/src%2Fdemo.cpp?ref=635a9c57bdcca07b99ddef52c2640337c50280b1",
+    "patch": "@@ -1,17 +1,18 @@\n /** This is a very ugly test code (doomed to fail linting) */\n #include \"demo.hpp\"\n-#include <cstdio>\n-#include <cstddef>\n+#include <stdio.h>\n \n-// using size_t from cstddef\n-size_t dummyFunc(size_t i) { return i; }\n \n-int main()\n-{\n-    for (;;)\n-        break;\n+\n+\n+int main(){\n+\n+    for (;;) break;\n+\n \n     printf(\"Hello world!\\n\");\n \n-    return 0;\n-}\n+\n+\n+\n+    return 0;}"
+  }
+]
diff --git a/cpp-linter-lib/tests/paginated_changes/pull_request_files_pg2.json b/cpp-linter-lib/tests/paginated_changes/pull_request_files_pg2.json
@@ -0,0 +1,14 @@
+[
+    {
+        "sha": "f93d0122ae2e3c1952c795837d71c432036b55eb",
+        "filename": "src/demo.hpp",
+        "status": "modified",
+        "additions": 3,
+        "deletions": 8,
+        "changes": 11,
+        "blob_url": "https://github.com/cpp-linter/test-cpp-linter-action/blob/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.hpp",
+        "raw_url": "https://github.com/cpp-linter/test-cpp-linter-action/raw/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.hpp",
+        "contents_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contents/src%2Fdemo.hpp?ref=635a9c57bdcca07b99ddef52c2640337c50280b1",
+        "patch": "@@ -5,12 +5,10 @@\n class Dummy {\n     char* useless;\n     int numb;\n+    Dummy() :numb(0), useless(\"\\0\"){}\n \n     public:\n-    void *not_usefull(char *str){\n-        useless = str;\n-        return 0;\n-    }\n+    void *not_useful(char *str){useless = str;}\n };\n \n \n@@ -28,14 +26,11 @@ class Dummy {\n \n \n \n-\n-\n-\n-\n \n \n struct LongDiff\n {\n+\n     long diff;\n \n };"
+    }
+]
diff --git a/cpp-linter-lib/tests/paginated_changes/push_files_pg1.json b/cpp-linter-lib/tests/paginated_changes/push_files_pg1.json
diff --git a/cpp-linter-lib/tests/paginated_changes/push_files_pg2.json b/cpp-linter-lib/tests/paginated_changes/push_files_pg2.json
diff --git a/cpp-linter-lib/tests/reviews.rs b/cpp-linter-lib/tests/reviews.rs

Original file line number	Diff line number	Diff line change
`@@ -230,7 +230,7 @@ async fn setup(lib_root: &Path, test_params: &TestParams) {`
`230`	`230`	`}`
`231`	`231`
`232`	`232`	`async fn test_comment(test_params: &TestParams) {`
`233`		`- let tmp_dir = create_test_space();`
	`233`	`+ let tmp_dir = create_test_space(true);`
`234`	`234`	`let lib_root = env::current_dir().unwrap();`
`235`	`235`	`env::set_current_dir(tmp_dir.path()).unwrap();`
`236`	`236`	`setup(&lib_root, test_params).await;`