Skip to content

Commit cbfed38

Browse files
JayDweeJayDwee
authored
Merge pull request #92 from KoalaBotUK/fix/issue-21-guild-user-save-swallow-errors
2 parents 3270d1d + f4272a7 commit cbfed38

9 files changed

Lines changed: 95 additions & 37 deletions

File tree

‎api/src/guilds/mod.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ async fn get_guilds_id(
123123
guild_id,
124124
..Default::default()
125125
};
126-
new_guild.save(&app_state.pg_pool).await;
126+
new_guild.save(&app_state.pg_pool).await?;
127127
new_guild
128128
}
129129
})))

‎api/src/guilds/models.rs‎

Lines changed: 39 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
1+
use crate::discord::ise;
12
use crate::guilds::verify::models::Verify;
23
use crate::guilds::votes::models::Vote;
4+
use http::StatusCode;
35
use lambda_http::tracing::{error};
46
use serde::{Deserialize, Serialize};
57
use std::collections::HashMap;
@@ -82,17 +84,44 @@ impl Guild {
8284
}
8385
}
8486

85-
pub async fn save(&self, pg_pool: &Pool<Postgres>) {
86-
match sqlx::query("INSERT INTO guilds (id, verify, vote) VALUES ($1, $2, $3) ON CONFLICT (id) DO UPDATE SET verify = $2, vote = $3, updated_at = CURRENT_TIMESTAMP")
87+
pub async fn save(&self, pg_pool: &Pool<Postgres>) -> Result<(), StatusCode> {
88+
sqlx::query("INSERT INTO guilds (id, verify, vote) VALUES ($1, $2, $3) ON CONFLICT (id) DO UPDATE SET verify = $2, vote = $3, updated_at = CURRENT_TIMESTAMP")
8789
.bind(BigDecimal::from(self.guild_id.into_nonzero().get()))
88-
.bind(serde_json::to_string(&self.verify).unwrap())
89-
.bind(serde_json::to_string(&self.vote).unwrap())
90+
.bind(serde_json::to_string(&self.verify).map_err(ise)?)
91+
.bind(serde_json::to_string(&self.vote).map_err(ise)?)
9092
.execute(pg_pool)
91-
.await {
92-
Ok(_) => (),
93-
Err(e) => {
94-
error!("Error saving user to DB: {}", e);
95-
}
96-
}
93+
.await
94+
.map_err(ise)?;
95+
Ok(())
96+
}
97+
}
98+
99+
#[cfg(test)]
100+
mod tests {
101+
use super::*;
102+
103+
/// Regression test for issue #21: `Guild::save` must return a `Result`
104+
/// that surfaces DB errors to the caller instead of swallowing them and
105+
/// reporting success. We point a lazily-connecting pool at a port that
106+
/// nothing is listening on, so the first query fails fast with a
107+
/// connection error, exercising the exact `sqlx::Error` -> `StatusCode`
108+
/// mapping path used in production. This does not require a live
109+
/// Postgres server, but a real DB integration test would additionally
110+
/// be valuable in CI to cover the success path end-to-end.
111+
#[tokio::test]
112+
async fn save_propagates_db_errors_instead_of_swallowing_them() {
113+
let pool = sqlx::postgres::PgPoolOptions::new()
114+
.connect_lazy("postgres://baduser:badpass@127.0.0.1:1/nonexistent_db")
115+
.expect("connect_lazy should not eagerly connect");
116+
117+
let guild = Guild::default();
118+
let result = guild.save(&pool).await;
119+
120+
assert_eq!(
121+
result,
122+
Err(StatusCode::INTERNAL_SERVER_ERROR),
123+
"save() must return an Err(StatusCode) when the underlying DB write fails, \
124+
not silently succeed"
125+
);
97126
}
98127
}

‎api/src/guilds/verify/controllers.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ async fn put_roles_id(
7979
// `members` is derived from `user_links`, not hand-incremented, so it
8080
// can never drift out of sync with the other role/link handlers.
8181
guild.verify.recompute_role_members();
82-
guild.save(&app_state.pg_pool).await;
82+
guild.save(&app_state.pg_pool).await?;
8383

8484
Ok(Json(json!(
8585
find_role(&guild.verify.roles, role_id).ok_or(StatusCode::NOT_FOUND)?
@@ -105,7 +105,7 @@ async fn delete_roles_id(
105105

106106
remove_existing_role(&mut guild, role_id, &app_state).await?;
107107

108-
guild.save(&app_state.pg_pool).await;
108+
guild.save(&app_state.pg_pool).await?;
109109

110110
Ok(StatusCode::NO_CONTENT)
111111
}
@@ -181,7 +181,7 @@ async fn post_recon(
181181
// recon uses exactly the same counting logic as put_roles_id/add/remove,
182182
// instead of a manual counter that can disagree with them.
183183
guild.verify.recompute_role_members();
184-
guild.save(&app_state.pg_pool).await;
184+
guild.save(&app_state.pg_pool).await?;
185185

186186
Ok(StatusCode::NO_CONTENT)
187187
}

‎api/src/guilds/votes/controllers.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ async fn post_votes(
106106
is_multi_select: vote_req.is_multi_select,
107107
};
108108
guild.vote.votes.push(new_vote.clone());
109-
guild.save(&app_state.pg_pool).await;
109+
guild.save(&app_state.pg_pool).await?;
110110

111111
if let Some(close_at) = vote_req.close_at {
112112
let mut headers = HeaderMap::new();
@@ -226,7 +226,7 @@ async fn close_vote(
226226
None => return Err(StatusCode::NOT_FOUND),
227227
};
228228
vote.open = false;
229-
guild.save(&app_state.pg_pool).await;
229+
guild.save(&app_state.pg_pool).await?;
230230

231231
let vote: &VoteVote = guild
232232
.vote

‎api/src/guilds/votes/interactions.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ pub(crate) async fn handle_component_interaction(
134134
responses.push(format!("You have added a vote for {o}."));
135135
}
136136
}
137-
guild.save(&app_state.pg_pool).await;
137+
guild.save(&app_state.pg_pool).await?;
138138

139139
Ok(Json(json!(InteractionResponse {
140140
kind: InteractionResponseType::ChannelMessageWithSource,

‎api/src/users/link_guilds.rs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ async fn put_link_guilds_id(
4545
user.link_guilds.retain(|g| g.guild_id != guild_id);
4646
user.link_guilds.push(new_link_guild.clone());
4747
let audit_new_data = user.link_guilds.clone();
48-
user.save(&app_state.pg_pool).await;
48+
user.save(&app_state.pg_pool).await?;
4949

5050
let mut guild = Guild::from_db(guild_id, &app_state.pg_pool).await.unwrap();
5151

@@ -71,8 +71,8 @@ async fn put_link_guilds_id(
7171
// mutation site (link add/remove, role add/remove, recon).
7272
guild.verify.recompute_role_members();
7373

74-
guild.save(&app_state.pg_pool).await;
75-
74+
guild.save(&app_state.pg_pool).await?;
75+
7676
// write audit
7777
audit(AuditMessage::new("update_link_guilds".to_string(), user_id, Some(guild_id),
7878
Some(audit_old_data), Some(audit_new_data)), &app_state.sqs).await;
@@ -129,8 +129,8 @@ async fn delete_link_guilds_id(
129129
// guard, which was only needed because the counter could otherwise drift.
130130
guild.verify.recompute_role_members();
131131

132-
user.save(&app_state.pg_pool).await;
133-
guild.save(&app_state.pg_pool).await;
132+
user.save(&app_state.pg_pool).await?;
133+
guild.save(&app_state.pg_pool).await?;
134134
Ok(StatusCode::NO_CONTENT)
135135
}
136136
async fn is_client_guild_member(

‎api/src/users/links.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -132,10 +132,10 @@ async fn post_link(
132132
// `retain` above) would otherwise inflate the count every time the
133133
// user re-links an address they already had.
134134
guild.verify.recompute_role_members();
135-
guild.save(&app_state.pg_pool).await;
135+
guild.save(&app_state.pg_pool).await?;
136136
}
137137
user_model.links.push(new_link.clone());
138-
user_model.save(&app_state.pg_pool).await;
138+
user_model.save(&app_state.pg_pool).await?;
139139
Ok(Json(json!(new_link)))
140140
}
141141

@@ -237,11 +237,11 @@ async fn delete_link(
237237
// instead of the old `if role.members > 0 { role.members -= 1 }`
238238
// guard, which only existed because the counter was untrustworthy.
239239
guild.verify.recompute_role_members();
240-
guild.save(&app_state.pg_pool).await;
240+
guild.save(&app_state.pg_pool).await?;
241241
}
242242
existing_link.active = false;
243243
user_model.links.push(existing_link);
244-
user_model.save(&app_state.pg_pool).await;
244+
user_model.save(&app_state.pg_pool).await?;
245245
Ok(StatusCode::NO_CONTENT)
246246
}
247247

‎api/src/users/mod.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ async fn get_users_id(
5353
user_id,
5454
..Default::default()
5555
};
56-
u.save(&app_state.pg_pool).await;
56+
u.save(&app_state.pg_pool).await?;
5757
u
5858
}
5959
};

‎api/src/users/models.rs‎

Lines changed: 39 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
use crate::discord::ise;
2+
use http::StatusCode;
13
use lambda_http::tracing::error;
24
use serde::{Deserialize, Serialize};
35
use sqlx::{Pool, Postgres, Row};
@@ -59,17 +61,44 @@ impl User {
5961
}
6062
}
6163

62-
pub async fn save(&self, pg_pool: &Pool<Postgres>) {
63-
match sqlx::query("INSERT INTO users (id, links, link_guilds) VALUES ($1, $2, $3) ON CONFLICT (id) DO UPDATE SET links = $2, link_guilds = $3, updated_at = CURRENT_TIMESTAMP")
64+
pub async fn save(&self, pg_pool: &Pool<Postgres>) -> Result<(), StatusCode> {
65+
sqlx::query("INSERT INTO users (id, links, link_guilds) VALUES ($1, $2, $3) ON CONFLICT (id) DO UPDATE SET links = $2, link_guilds = $3, updated_at = CURRENT_TIMESTAMP")
6466
.bind(BigDecimal::from(self.user_id.into_nonzero().get()))
65-
.bind(serde_json::to_string(&self.links).unwrap())
66-
.bind(serde_json::to_string(&self.link_guilds).unwrap())
67+
.bind(serde_json::to_string(&self.links).map_err(ise)?)
68+
.bind(serde_json::to_string(&self.link_guilds).map_err(ise)?)
6769
.execute(pg_pool)
68-
.await {
69-
Ok(_) => (),
70-
Err(e) => {
71-
error!("Error saving user to DB: {}", e);
72-
}
73-
}
70+
.await
71+
.map_err(ise)?;
72+
Ok(())
73+
}
74+
}
75+
76+
#[cfg(test)]
77+
mod tests {
78+
use super::*;
79+
80+
/// Regression test for issue #21: `User::save` must return a `Result`
81+
/// that surfaces DB errors to the caller instead of swallowing them and
82+
/// reporting success. We point a lazily-connecting pool at a port that
83+
/// nothing is listening on, so the first query fails fast with a
84+
/// connection error, exercising the exact `sqlx::Error` -> `StatusCode`
85+
/// mapping path used in production. This does not require a live
86+
/// Postgres server, but a real DB integration test would additionally
87+
/// be valuable in CI to cover the success path end-to-end.
88+
#[tokio::test]
89+
async fn save_propagates_db_errors_instead_of_swallowing_them() {
90+
let pool = sqlx::postgres::PgPoolOptions::new()
91+
.connect_lazy("postgres://baduser:badpass@127.0.0.1:1/nonexistent_db")
92+
.expect("connect_lazy should not eagerly connect");
93+
94+
let user = User::default();
95+
let result = user.save(&pool).await;
96+
97+
assert_eq!(
98+
result,
99+
Err(StatusCode::INTERNAL_SERVER_ERROR),
100+
"save() must return an Err(StatusCode) when the underlying DB write fails, \
101+
not silently succeed"
102+
);
74103
}
75104
}

0 commit comments

Comments
 (0)