From 0e9243db48f1f05a627cf17f84e0db4611ec148e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ben=20P=C3=BCschel?= Date: Fri, 30 Aug 2024 14:59:52 +0200 Subject: [PATCH 1/5] fix(installation)!: Return Result instead of panicking Refs: #641 BREAKING CHANGE: `Octocrab::installation` now returns a Result, changing the public API. --- src/lib.rs | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 41c81969..157133c3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1008,20 +1008,27 @@ impl Octocrab { /// then obtain an installation ID, and then pass that here to /// obtain a new `Octocrab` with which you can make API calls /// with the permissions of that installation. - pub fn installation(&self, id: InstallationId) -> Octocrab { + pub fn installation(&self, id: InstallationId) -> Result { let app_auth = if let AuthState::App(ref app_auth) = self.auth_state { app_auth.clone() } else { - panic!("Github App authorization is required to target an installation"); + let source = std::io::Error::new( + std::io::ErrorKind::Other, + "Github App authorization is required to target an installation", + ); + return Err(Error::Other { + backtrace: Backtrace::generate_with_source(&source), + source: Box::new(source), + }); }; - Octocrab { + Ok(Octocrab { client: self.client.clone(), auth_state: AuthState::Installation { app: app_auth, installation: id, token: CachedToken::default(), }, - } + }) } /// Similar to `installation`, but also eagerly caches the installation @@ -1034,7 +1041,7 @@ impl Octocrab { &self, id: InstallationId, ) -> Result<(Octocrab, SecretString)> { - let crab = self.installation(id); + let crab = self.installation(id)?; let token = crab.request_installation_auth_token().await?; Ok((crab, token)) } @@ -1480,7 +1487,12 @@ impl Octocrab { { (app, installation, token) } else { - panic!("Installation not configured"); + let source = + std::io::Error::new(std::io::ErrorKind::Other, "Installation not configured"); + return Err(Error::Other { + backtrace: Backtrace::generate_with_source(&source), + source: Box::new(source), + }); }; let mut request = Builder::new(); let mut sensitive_value = From 2319522bd648b24719652d6554d3e70747619ea3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ben=20P=C3=BCschel?= Date: Fri, 20 Sep 2024 17:47:34 +0200 Subject: [PATCH 2/5] refactor(error)!: add `installation` variant Adds a new variant `Installation` on the Error enum and also makes the enum non-exhaustive to prevent further breaking changes when adding new variants in the future. --- src/error.rs | 28 +++++++++++++++++++++++++++- src/lib.rs | 22 +++++++++------------- 2 files changed, 36 insertions(+), 14 deletions(-) diff --git a/src/error.rs b/src/error.rs index a987fdc8..93573203 100644 --- a/src/error.rs +++ b/src/error.rs @@ -22,6 +22,7 @@ impl std::error::Error for UriParseError {} /// An error that could have occurred while using [`crate::Octocrab`]. #[derive(Snafu, Debug)] #[snafu(visibility(pub))] +#[non_exhaustive] pub enum Error { GitHub { source: GitHubError, @@ -35,7 +36,10 @@ pub enum Error { source: InvalidUri, backtrace: Backtrace, }, - + Installation { + source: InstallationError, + backtrace: Backtrace, + }, InvalidHeaderValue { source: http::header::InvalidHeaderValue, backtrace: Backtrace, @@ -126,3 +130,25 @@ impl fmt::Display for GitHubError { } impl std::error::Error for GitHubError {} + +/// An error that could have occurred while trying to target an installation. +#[derive(Debug, Clone)] +pub struct InstallationError { + pub message: String, +} + +impl InstallationError { + pub fn new(message: impl Into) -> Self { + Self { + message: message.into(), + } + } +} + +impl fmt::Display for InstallationError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "{}", self.message) + } +} + +impl std::error::Error for InstallationError {} diff --git a/src/lib.rs b/src/lib.rs index 157133c3..004302ae 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -249,7 +249,7 @@ use crate::service::middleware::retry::RetryConfig; use crate::api::{code_scannings, users}; use auth::{AppAuth, Auth}; -use models::{AppId, InstallationId, InstallationToken}; +use models::{AppId, Installation, InstallationId, InstallationToken}; pub use self::{ api::{ @@ -1012,13 +1012,11 @@ impl Octocrab { let app_auth = if let AuthState::App(ref app_auth) = self.auth_state { app_auth.clone() } else { - let source = std::io::Error::new( - std::io::ErrorKind::Other, - "Github App authorization is required to target an installation", - ); - return Err(Error::Other { - backtrace: Backtrace::generate_with_source(&source), - source: Box::new(source), + let source = return Err(Error::Installation { + backtrace: Backtrace::generate(), + source: error::InstallationError::new( + "Github App authorization is required to target an installation", + ), }); }; Ok(Octocrab { @@ -1487,11 +1485,9 @@ impl Octocrab { { (app, installation, token) } else { - let source = - std::io::Error::new(std::io::ErrorKind::Other, "Installation not configured"); - return Err(Error::Other { - backtrace: Backtrace::generate_with_source(&source), - source: Box::new(source), + return Err(Error::Installation { + backtrace: Backtrace::generate(), + source: error::InstallationError::new("Installation not configured"), }); }; let mut request = Builder::new(); From b99934aa953d6f0930932f02d7cc881eddaac867 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ben=20P=C3=BCschel?= Date: Sat, 21 Sep 2024 20:25:13 +0200 Subject: [PATCH 3/5] style: remove unused import --- src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index 004302ae..234ab740 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -249,7 +249,7 @@ use crate::service::middleware::retry::RetryConfig; use crate::api::{code_scannings, users}; use auth::{AppAuth, Auth}; -use models::{AppId, Installation, InstallationId, InstallationToken}; +use models::{AppId, InstallationId, InstallationToken}; pub use self::{ api::{ From 8bd4e3b6e4ded40ca9e855664909fcfa9afd6ac8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ben=20P=C3=BCschel?= Date: Sat, 21 Sep 2024 20:31:34 +0200 Subject: [PATCH 4/5] style: remove unnecessary error let binding --- src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index 234ab740..7177576a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1012,7 +1012,7 @@ impl Octocrab { let app_auth = if let AuthState::App(ref app_auth) = self.auth_state { app_auth.clone() } else { - let source = return Err(Error::Installation { + return Err(Error::Installation { backtrace: Backtrace::generate(), source: error::InstallationError::new( "Github App authorization is required to target an installation", From cf86160e8160723538c51300eb265c9475d7bfc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ben=20P=C3=BCschel?= Date: Mon, 30 Sep 2024 15:54:17 +0200 Subject: [PATCH 5/5] refactor: remove installation error message --- src/error.rs | 28 ++-------------------------- src/lib.rs | 4 ---- 2 files changed, 2 insertions(+), 30 deletions(-) diff --git a/src/error.rs b/src/error.rs index 93573203..c4ac9a65 100644 --- a/src/error.rs +++ b/src/error.rs @@ -36,10 +36,8 @@ pub enum Error { source: InvalidUri, backtrace: Backtrace, }, - Installation { - source: InstallationError, - backtrace: Backtrace, - }, + #[snafu(display("Installation Error: Github App authorization is required to target an installation.\n\nFound at {}", backtrace))] + Installation { backtrace: Backtrace }, InvalidHeaderValue { source: http::header::InvalidHeaderValue, backtrace: Backtrace, @@ -130,25 +128,3 @@ impl fmt::Display for GitHubError { } impl std::error::Error for GitHubError {} - -/// An error that could have occurred while trying to target an installation. -#[derive(Debug, Clone)] -pub struct InstallationError { - pub message: String, -} - -impl InstallationError { - pub fn new(message: impl Into) -> Self { - Self { - message: message.into(), - } - } -} - -impl fmt::Display for InstallationError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "{}", self.message) - } -} - -impl std::error::Error for InstallationError {} diff --git a/src/lib.rs b/src/lib.rs index 7177576a..dd658f04 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1014,9 +1014,6 @@ impl Octocrab { } else { return Err(Error::Installation { backtrace: Backtrace::generate(), - source: error::InstallationError::new( - "Github App authorization is required to target an installation", - ), }); }; Ok(Octocrab { @@ -1487,7 +1484,6 @@ impl Octocrab { } else { return Err(Error::Installation { backtrace: Backtrace::generate(), - source: error::InstallationError::new("Installation not configured"), }); }; let mut request = Builder::new();