Skip to content

Commit 76d9954

Browse files
notheotherbennotheotherben
authored
Merge pull request #619 from SierraSoftworks/refactor/remove-unwrap-non-test
refactor: handle errors instead of panicking with unwrap in non-test code
2 parents 90c5373 + f3a497b commit 76d9954

6 files changed

Lines changed: 113 additions & 86 deletions

File tree

‎src/errors.rs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,18 @@ impl HumanizableError for reqwest::header::InvalidHeaderValue {
9595
}
9696
}
9797

98+
impl HumanizableError for tokio::task::JoinError {
99+
fn to_human_error(self) -> human_errors::Error {
100+
human_errors::wrap_system(
101+
self,
102+
"A backup task terminated unexpectedly before it could complete.",
103+
&[
104+
"This is likely a bug in github-backup, please report it to us on GitHub along with the error details above.",
105+
],
106+
)
107+
}
108+
}
109+
98110
#[derive(Debug)]
99111
pub struct ResponseError {
100112
pub status_code: StatusCode,

‎src/main.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ async fn run(args: Args, session: &Session) -> Result<(), Error> {
8282
.as_ref()
8383
.and_then(|s| s.find_next_occurrence(&chrono::Utc::now(), false).ok());
8484

85-
let handler = LoggingPairingHandler::new(&session);
85+
let handler = LoggingPairingHandler::new(session);
8686

8787
pinger.on_start().await;
8888

@@ -178,7 +178,7 @@ impl<E: BackupEntity> PairingHandler<E> for LoggingPairingHandler<'_> {
178178

179179
if error.is(human_errors::Kind::System) {
180180
let info = tracing_batteries::ErrorInfo::new(&error)
181-
.with_metadata("policy.kind", format!("{}", policy.kind));
181+
.with_metadata("policy.kind", policy.kind.to_string());
182182

183183
self.session.record_custom_error(info);
184184
} else {
@@ -197,7 +197,7 @@ impl<E: BackupEntity> PairingHandler<E> for LoggingPairingHandler<'_> {
197197
self.session.record_event(
198198
"policy::run",
199199
[
200-
("policy.kind", format!("{}", policy.kind)),
200+
("policy.kind", policy.kind.to_string()),
201201
("stats.new", summary.new.to_string()),
202202
("stats.unchanged", summary.unchanged.to_string()),
203203
("stats.updated", summary.updated.to_string()),

‎src/pairing.rs‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ use tracing_batteries::prelude::*;
88
use crate::{
99
BackupEntity, BackupPolicy, BackupSource,
1010
engines::{BackupEngine, BackupState},
11+
errors::HumanizableError as _,
1112
};
1213

1314
pub struct Pairing<E: BackupEntity, S: BackupSource<E>, T: BackupEngine<E>> {
@@ -132,7 +133,11 @@ impl<
132133
for to in policy.to.iter() {
133134
while join_set.len() >= self.concurrency_limit {
134135
debug!("Reached concurrency limit of {}, waiting for a task to complete", self.concurrency_limit);
135-
yield join_set.join_next().await.unwrap().unwrap();
136+
match join_set.join_next().await {
137+
Some(Ok(result)) => yield result,
138+
Some(Err(e)) => yield Err(e.to_human_error()),
139+
None => break,
140+
}
136141
}
137142

138143
let span = tracing_batteries::prelude::info_span!(parent: &span, "backup.step", item=%entity, target=%to);
@@ -147,7 +152,10 @@ impl<
147152
}
148153

149154
while let Some(fut) = join_set.join_next().await {
150-
yield fut.unwrap();
155+
match fut {
156+
Ok(result) => yield result,
157+
Err(e) => yield Err(e.to_human_error()),
158+
}
151159
}
152160
}
153161
}

‎src/sources/github_gist.rs‎

Lines changed: 27 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -38,35 +38,34 @@ impl BackupSource<GitRepo> for GitHubGistSource {
3838
policy: &'a BackupPolicy,
3939
cancel: &'a AtomicBool,
4040
) -> impl Stream<Item = Result<GitRepo, human_errors::Error>> + 'a {
41-
let target: GitHubRepoSourceKind = policy.from.as_str().parse().unwrap();
42-
43-
let url = format!(
44-
"{}/{}?{}",
45-
policy
46-
.properties
47-
.get("api_url")
48-
.unwrap_or(&"https://api.github.com".to_string())
49-
.trim_end_matches('/'),
50-
target.api_endpoint(GitHubArtifactKind::Gist),
51-
policy.properties.get("query").unwrap_or(&"".to_string())
52-
)
53-
.trim_end_matches('?')
54-
.to_string();
55-
56-
tracing_batteries::prelude::debug!("Calling {} to fetch gists", &url);
57-
58-
let refspecs = policy
59-
.properties
60-
.get("refspecs")
61-
.map(|r| r.split(',').map(|r| r.to_string()).collect::<Vec<String>>());
62-
63-
let recovery_mode: RecoveryMode = policy
64-
.properties
65-
.get("recovery")
66-
.map(|mode| mode.parse().unwrap())
67-
.unwrap_or_default();
68-
6941
async_stream::try_stream! {
42+
let target: GitHubRepoSourceKind = policy.from.as_str().parse()?;
43+
44+
let url = format!(
45+
"{}/{}?{}",
46+
policy
47+
.properties
48+
.get("api_url")
49+
.unwrap_or(&"https://api.github.com".to_string())
50+
.trim_end_matches('/'),
51+
target.api_endpoint(GitHubArtifactKind::Gist),
52+
policy.properties.get("query").unwrap_or(&"".to_string())
53+
)
54+
.trim_end_matches('?')
55+
.to_string();
56+
57+
tracing_batteries::prelude::debug!("Calling {} to fetch gists", &url);
58+
59+
let refspecs = policy
60+
.properties
61+
.get("refspecs")
62+
.map(|r| r.split(',').map(|r| r.to_string()).collect::<Vec<String>>());
63+
64+
let recovery_mode: RecoveryMode = match policy.properties.get("recovery") {
65+
Some(mode) => mode.parse()?,
66+
None => RecoveryMode::default(),
67+
};
68+
7069
if matches!(target, GitHubRepoSourceKind::Gist(_)) {
7170
let gist: GitHubGist = self.client.get(&url, &policy.credentials, cancel).await?;
7271
yield GitRepo::new(

‎src/sources/github_releases.rs‎

Lines changed: 34 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -39,12 +39,13 @@ impl GitHubReleasesSource {
3939
let releases_url = format!("{}/releases", repo.url);
4040

4141
for await release in self.client.get_paginated(&releases_url, &policy.credentials, cancel) {
42-
if let Err(e) = release {
43-
yield Err(e);
44-
continue;
45-
}
46-
47-
let release: GitHubRelease = release.unwrap();
42+
let release: GitHubRelease = match release {
43+
Ok(release) => release,
44+
Err(e) => {
45+
yield Err(e);
46+
continue;
47+
}
48+
};
4849

4950
let mut entity = Release::new(
5051
format!("{}/{}", &repo.full_name, &release.tag_name),
@@ -170,21 +171,27 @@ impl BackupSource<Release> for GitHubReleasesSource {
170171
policy: &'a BackupPolicy,
171172
cancel: &'a AtomicBool,
172173
) -> impl Stream<Item = Result<Release, human_errors::Error>> + 'a {
173-
let target: GitHubRepoSourceKind = policy.from.as_str().parse().unwrap();
174-
let url = format!(
175-
"{}/{}?{}",
176-
policy
177-
.properties
178-
.get("api_url")
179-
.unwrap_or(&"https://api.github.com".to_string())
180-
.trim_end_matches('/'),
181-
target.api_endpoint(GitHubArtifactKind::Release),
182-
policy.properties.get("query").unwrap_or(&"".to_string())
183-
)
184-
.trim_end_matches('?')
185-
.to_string();
186-
187174
async_stream::stream! {
175+
let target: GitHubRepoSourceKind = match policy.from.as_str().parse() {
176+
Ok(target) => target,
177+
Err(e) => {
178+
yield Err(e);
179+
return;
180+
}
181+
};
182+
let url = format!(
183+
"{}/{}?{}",
184+
policy
185+
.properties
186+
.get("api_url")
187+
.unwrap_or(&"https://api.github.com".to_string())
188+
.trim_end_matches('/'),
189+
target.api_endpoint(GitHubArtifactKind::Release),
190+
policy.properties.get("query").unwrap_or(&"".to_string())
191+
)
192+
.trim_end_matches('?')
193+
.to_string();
194+
188195
if matches!(target, GitHubRepoSourceKind::Repo(_)) {
189196
let repo: GitHubRepo = self.client.get(&url, &policy.credentials, cancel).await?;
190197

@@ -193,12 +200,13 @@ impl BackupSource<Release> for GitHubReleasesSource {
193200
}
194201
} else {
195202
for await repo in self.client.get_paginated(&url, &policy.credentials, cancel) {
196-
if let Err(e) = repo {
197-
yield Err(e);
198-
continue;
199-
}
200-
201-
let repo: GitHubRepo = repo.unwrap();
203+
let repo: GitHubRepo = match repo {
204+
Ok(repo) => repo,
205+
Err(e) => {
206+
yield Err(e);
207+
continue;
208+
}
209+
};
202210

203211
for await file in self.load_releases(policy, &repo, cancel) {
204212
yield file;

‎src/sources/github_repo.rs‎

Lines changed: 27 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -38,34 +38,34 @@ impl BackupSource<GitRepo> for GitHubRepoSource {
3838
policy: &'a BackupPolicy,
3939
cancel: &'a AtomicBool,
4040
) -> impl Stream<Item = Result<GitRepo, human_errors::Error>> + 'a {
41-
let target: GitHubRepoSourceKind = policy.from.as_str().parse().unwrap();
42-
let url = format!(
43-
"{}/{}?{}",
44-
policy
45-
.properties
46-
.get("api_url")
47-
.unwrap_or(&"https://api.github.com".to_string())
48-
.trim_end_matches('/'),
49-
target.api_endpoint(GitHubArtifactKind::Repo),
50-
policy.properties.get("query").unwrap_or(&"".to_string())
51-
)
52-
.trim_end_matches('?')
53-
.to_string();
54-
55-
tracing_batteries::prelude::debug!("Calling {} to fetch repos", &url);
56-
57-
let refspecs = policy
58-
.properties
59-
.get("refspecs")
60-
.map(|r| r.split(',').map(|r| r.to_string()).collect::<Vec<String>>());
61-
62-
let recovery_mode: RecoveryMode = policy
63-
.properties
64-
.get("recovery")
65-
.map(|mode| mode.parse().unwrap())
66-
.unwrap_or_default();
67-
6841
async_stream::try_stream! {
42+
let target: GitHubRepoSourceKind = policy.from.as_str().parse()?;
43+
44+
let url = format!(
45+
"{}/{}?{}",
46+
policy
47+
.properties
48+
.get("api_url")
49+
.unwrap_or(&"https://api.github.com".to_string())
50+
.trim_end_matches('/'),
51+
target.api_endpoint(GitHubArtifactKind::Repo),
52+
policy.properties.get("query").unwrap_or(&"".to_string())
53+
)
54+
.trim_end_matches('?')
55+
.to_string();
56+
57+
tracing_batteries::prelude::debug!("Calling {} to fetch repos", &url);
58+
59+
let refspecs = policy
60+
.properties
61+
.get("refspecs")
62+
.map(|r| r.split(',').map(|r| r.to_string()).collect::<Vec<String>>());
63+
64+
let recovery_mode: RecoveryMode = match policy.properties.get("recovery") {
65+
Some(mode) => mode.parse()?,
66+
None => RecoveryMode::default(),
67+
};
68+
6969
if matches!(target, GitHubRepoSourceKind::Repo(_)) {
7070
let repo: GitHubRepo = self.client.get(&url, &policy.credentials, cancel).await?;
7171
yield GitRepo::new(

0 commit comments

Comments
 (0)