From f05936cbd1ddc651ed9974f47a3b0af0cdc30145 Mon Sep 17 00:00:00 2001 From: Cyborus Date: Thu, 30 Nov 2023 15:14:51 -0500 Subject: [PATCH 01/20] use `wrap_err` instead of `map_err` --- tests/ci_test.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/ci_test.rs b/tests/ci_test.rs index 08ee904..18e270f 100644 --- a/tests/ci_test.rs +++ b/tests/ci_test.rs @@ -1,5 +1,5 @@ use forgejo_api::Forgejo; -use eyre::{eyre, ensure}; +use eyre::{eyre, ensure, WrapErr}; #[tokio::test] async fn ci() -> eyre::Result<()> { @@ -9,8 +9,8 @@ async fn ci() -> eyre::Result<()> { let mut results = Vec::new(); - results.push(user(&api).await.map_err(|e| eyre!("user error: {e}"))); - results.push(repo(&api).await.map_err(|e| eyre!("repo error: {e}"))); + results.push(user(&api).await.wrap_err("user error")); + results.push(repo(&api).await.wrap_err("repo error")); let mut errors = 0; for res in results.into_iter().filter_map(Result::err) { @@ -102,7 +102,7 @@ async fn repo(api: &forgejo_api::Forgejo) -> eyre::Result<()> { }); let mut push_options = git2::PushOptions::new(); push_options.remote_callbacks(callbacks); - origin.push(&[branch_ref_name], Some(&mut push_options)).map_err(|e| eyre!("failed to push branch, {e}"))?; + origin.push(&[branch_ref_name], Some(&mut push_options)).wrap_err("failed to push branch")?; let pr_opt = forgejo_api::CreatePullRequestOption { assignee: None, @@ -115,8 +115,8 @@ async fn repo(api: &forgejo_api::Forgejo) -> eyre::Result<()> { milestone: None, title: "test pr".into(), }; - let pr = api.create_pr("TestingAdmin", "test", pr_opt).await.map_err(|e| eyre!("couldn't create pr, {e}"))?; - let is_merged = api.is_merged("TestingAdmin", "test", pr.number).await.map_err(|e| eyre!("couldn't find unmerged pr {}, {}", pr.number, e))?; + let pr = api.create_pr("TestingAdmin", "test", pr_opt).await.wrap_err("couldn't create pr")?; + let is_merged = api.is_merged("TestingAdmin", "test", pr.number).await.wrap_err_with(|| eyre!("couldn't find unmerged pr {}", pr.number))?; ensure!(!is_merged, "pr should not yet be merged"); let merge_opt = forgejo_api::MergePullRequestOption { act: forgejo_api::MergePrAction::Merge, @@ -128,8 +128,8 @@ async fn repo(api: &forgejo_api::Forgejo) -> eyre::Result<()> { head_commit_id: None, merge_when_checks_succeed: None, }; - api.merge_pr("TestingAdmin", "test", pr.number, merge_opt).await.map_err(|e| eyre!("couldn't merge pr {}, {}", pr.number, e))?; - let is_merged = api.is_merged("TestingAdmin", "test", pr.number).await.map_err(|e| eyre!("couldn't find merged pr {}, {}", pr.number, e))?; + api.merge_pr("TestingAdmin", "test", pr.number, merge_opt).await.wrap_err_with(|| eyre!("couldn't merge pr {}", pr.number))?; + let is_merged = api.is_merged("TestingAdmin", "test", pr.number).await.wrap_err_with(|| eyre!("couldn't find merged pr {}", pr.number))?; ensure!(is_merged, "pr should be merged"); Ok(()) From 19effc86d077090db99ca83bf9f98a14f9d4a9c6 Mon Sep 17 00:00:00 2001 From: Cyborus Date: Thu, 30 Nov 2023 15:21:28 -0500 Subject: [PATCH 02/20] print whole error chain --- tests/ci_test.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/ci_test.rs b/tests/ci_test.rs index 18e270f..1017765 100644 --- a/tests/ci_test.rs +++ b/tests/ci_test.rs @@ -13,9 +13,11 @@ async fn ci() -> eyre::Result<()> { results.push(repo(&api).await.wrap_err("repo error")); let mut errors = 0; - for res in results.into_iter().filter_map(Result::err) { + for report in results.into_iter().filter_map(Result::err) { errors += 1; - println!("{res}"); + for (i, err) in report.chain().enumerate() { + println!("{i}. {err}"); + } } if errors > 0 { eyre::bail!("test failed"); @@ -83,7 +85,7 @@ async fn repo(api: &forgejo_api::Forgejo) -> eyre::Result<()> { let mut push_options = git2::PushOptions::new(); push_options.remote_callbacks(callbacks); let mut origin = local_repo.remote("origin", remote_repo.clone_url.as_str())?; - origin.push(&[branch_ref_name], Some(&mut push_options))?; + origin.push(&[dbg!(branch_ref_name)], Some(&mut push_options))?; tokio::fs::write("/test_repo/example.rs", "fn add_one(x: u32) -> u32 { x + 1 }").await?; let mut index = local_repo.index()?; @@ -102,7 +104,7 @@ async fn repo(api: &forgejo_api::Forgejo) -> eyre::Result<()> { }); let mut push_options = git2::PushOptions::new(); push_options.remote_callbacks(callbacks); - origin.push(&[branch_ref_name], Some(&mut push_options)).wrap_err("failed to push branch")?; + origin.push(&[dbg!(branch_ref_name)], Some(&mut push_options)).wrap_err("failed to push branch")?; let pr_opt = forgejo_api::CreatePullRequestOption { assignee: None, From 31a53eb88a7f13e834e69352bdbc972668d7eff2 Mon Sep 17 00:00:00 2001 From: Cyborus Date: Fri, 1 Dec 2023 19:23:01 -0500 Subject: [PATCH 03/20] make `PullRequest`'s `milestone` field optional --- src/repository.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/repository.rs b/src/repository.rs index 0564f5d..79e9f83 100644 --- a/src/repository.rs +++ b/src/repository.rs @@ -358,7 +358,7 @@ pub struct PullRequest { #[serde(with = "time::serde::rfc3339::option")] pub merged_at: Option, pub merged_by: User, - pub milestone: Milestone, + pub milestone: Option, pub number: u64, pub patch_url: Url, pub pin_order: u64, From b952e1108a2b713c2ab5ae40938b54cdfe0c4faf Mon Sep 17 00:00:00 2001 From: Cyborus Date: Sun, 10 Dec 2023 00:53:52 -0500 Subject: [PATCH 04/20] include body in `ForgejoError::BadStructure` --- Cargo.lock | 1 + Cargo.toml | 1 + src/lib.rs | 28 +++++++++++++--------------- tests/ci_test.rs | 5 +++++ 4 files changed, 20 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 436e2be..c3f06ad 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -174,6 +174,7 @@ dependencies = [ "git2", "reqwest", "serde", + "serde_json", "soft_assert", "thiserror", "time", diff --git a/Cargo.toml b/Cargo.toml index 6989702..ad84340 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -13,6 +13,7 @@ tokio = { version = "1.29.1", features = ["net"] } url = { version = "2.4.0", features = ["serde"] } serde = { version = "1.0.168", features = ["derive"] } time = { version = "0.3.22", features = ["parsing", "serde", "formatting"] } +serde_json = "1.0.108" [dev-dependencies] eyre = "0.6.9" diff --git a/src/lib.rs b/src/lib.rs index a6a7daf..6d0aa89 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -30,28 +30,18 @@ pub enum ForgejoError { HostRequired, #[error("scheme must be http or https")] HttpRequired, - #[error("{0}")] // for some reason, you can't use `source` and `transparent` together - ReqwestError(#[source] reqwest::Error), + #[error(transparent)] + ReqwestError(#[from] reqwest::Error), #[error("API key should be ascii")] KeyNotAscii, #[error("the response from forgejo was not properly structured")] - BadStructure(#[source] reqwest::Error), + BadStructure(#[source] serde_json::Error, String), #[error("unexpected status code {} {}", .0.as_u16(), .0.canonical_reason().unwrap_or(""))] UnexpectedStatusCode(StatusCode), #[error("{} {}: {}", .0.as_u16(), .0.canonical_reason().unwrap_or(""), .1)] ApiError(StatusCode, String), } -impl From for ForgejoError { - fn from(e: reqwest::Error) -> Self { - if e.is_decode() { - ForgejoError::BadStructure(e) - } else { - ForgejoError::ReqwestError(e) - } - } -} - impl Forgejo { pub fn new(api_key: &str, url: Url) -> Result { Self::with_user_agent(api_key, url, "forgejo-api-rs") @@ -163,7 +153,11 @@ impl Forgejo { async fn execute(&self, request: Request) -> Result { let response = self.client.execute(request).await?; match response.status() { - status if status.is_success() => Ok(response.json::().await?), + status if status.is_success() => { + let body = response.text().await?; + let out = serde_json::from_str(&body).map_err(|e| ForgejoError::BadStructure(e, body))?; + Ok(out) + }, status if status.is_client_error() => Err(ForgejoError::ApiError( status, response.json::().await?.message, @@ -192,7 +186,11 @@ impl Forgejo { ) -> Result, ForgejoError> { let response = self.client.execute(request).await?; match response.status() { - status if status.is_success() => Ok(Some(response.json::().await?)), + status if status.is_success() => { + let body = response.text().await?; + let out = serde_json::from_str(&body).map_err(|e| ForgejoError::BadStructure(e, body))?; + Ok(out) + }, StatusCode::NOT_FOUND => Ok(None), status if status.is_client_error() => Err(ForgejoError::ApiError( status, diff --git a/tests/ci_test.rs b/tests/ci_test.rs index 1017765..9c5d8b5 100644 --- a/tests/ci_test.rs +++ b/tests/ci_test.rs @@ -17,6 +17,11 @@ async fn ci() -> eyre::Result<()> { errors += 1; for (i, err) in report.chain().enumerate() { println!("{i}. {err}"); + if let Some(err) = err.downcast_ref::() { + if let forgejo_api::ForgejoError::BadStructure(_, body) = err { + println!("BODY: {body}"); + } + } } } if errors > 0 { From 0a07502dbc81c5d34e3980b17750c1e01746ecfe Mon Sep 17 00:00:00 2001 From: Cyborus Date: Sun, 10 Dec 2023 01:08:41 -0500 Subject: [PATCH 05/20] `PullRequest.requested_reviewers` can be null --- src/repository.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/repository.rs b/src/repository.rs index 79e9f83..8e26bd9 100644 --- a/src/repository.rs +++ b/src/repository.rs @@ -362,7 +362,7 @@ pub struct PullRequest { pub number: u64, pub patch_url: Url, pub pin_order: u64, - pub requested_reviewers: Vec, + pub requested_reviewers: Option>, pub state: State, pub title: String, #[serde(with = "time::serde::rfc3339")] From 3c1675976cccd8f5c79a46c47111069ac4892a0f Mon Sep 17 00:00:00 2001 From: Cyborus Date: Sun, 10 Dec 2023 01:14:13 -0500 Subject: [PATCH 06/20] `PullRequest`'s `merge_commit_sha` and `merged_by` can be null --- src/repository.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/repository.rs b/src/repository.rs index 8e26bd9..8624530 100644 --- a/src/repository.rs +++ b/src/repository.rs @@ -352,12 +352,12 @@ pub struct PullRequest { pub is_locked: bool, pub labels: Vec