test(backend): DB-backed tests for the risky SQL; test isolation; clippy cleanup
All 40 backend tests were pure-function tests — not one line of SQL ran under `cargo test`,
even though the riskiest code in the repo is SQL. Add 12 DB-backed `#[sqlx::test]` tests
(fresh throwaway DB per test, real migrations) pinning the invariants that code is
load-bearing for, each mutation-verified to fail on broken code:
export_epoch.rs (8): epoch is post-increment on release (a worker born pre-increment is
inert); epoch strictly monotonic across reopen; a retired-epoch worker's writes are
no-ops; export_current is EXACTLY the invariant (8-case table); the ViewerOnly
carry-forward carries a done ZIP forward AND matches nothing when the ZIP is unfinished
(the af997a8 strand bug); boot recovery doesn't clobber a live ZIP.
upload_concurrency.rs (4): the atomic quota UPDATE stops two stale-snapshot uploads from
both committing (sequential AND concurrent-tx via EPQ); the FOR SHARE upload lock
serializes against release, blocking a photo from committing after the export snapshot.
Deleting FOR SHARE, or the carry-forward's `status='done'`, each makes a test fail.
Test isolation: TRUNCATE now also resets disk_cache and sse_tickets. The stale disk
reading was harmless only while quotas were globally off in e2e (they no longer are — see
the new quota spec), i.e. two holes were masking each other.
clippy: `cargo clippy --all-targets -- -D warnings` now passes (it did not). Deleted the
dead jobs.rs (an unused BackgroundJob sketch) and unused model methods; `#[allow(dead_code)]`
+ comment on the sqlx FromRow field-sets that ARE populated by the DB. No SQL or logic
changed. `cargo test` requires DATABASE_URL by design.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -75,10 +75,10 @@ impl IntoResponse for AppError {
|
||||
}
|
||||
|
||||
let mut resp = (status, axum::Json(body)).into_response();
|
||||
if let Some(secs) = retry_after_secs {
|
||||
if let Ok(val) = axum::http::HeaderValue::from_str(&secs.to_string()) {
|
||||
resp.headers_mut().insert(axum::http::header::RETRY_AFTER, val);
|
||||
}
|
||||
if let Some(secs) = retry_after_secs
|
||||
&& let Ok(val) = axum::http::HeaderValue::from_str(&secs.to_string())
|
||||
{
|
||||
resp.headers_mut().insert(axum::http::header::RETRY_AFTER, val);
|
||||
}
|
||||
resp
|
||||
}
|
||||
|
||||
@@ -98,6 +98,10 @@ pub async fn get_config(
|
||||
Ok(Json(rows.into_iter().collect()))
|
||||
}
|
||||
|
||||
/// Documents the wire shape of `PATCH /admin/config` (a flat `{key: value}` object).
|
||||
/// `patch_config` extracts the `HashMap` directly rather than going through this newtype, so it is
|
||||
/// never constructed in Rust — it stays as the serde-derived description of the request body.
|
||||
#[allow(dead_code)]
|
||||
#[derive(Deserialize)]
|
||||
pub struct PatchConfigRequest(pub HashMap<String, String>);
|
||||
|
||||
|
||||
@@ -85,6 +85,19 @@ pub async fn truncate_all(
|
||||
// could serve the previous test's toggles.
|
||||
state.config_cache.invalidate();
|
||||
|
||||
// The other two in-memory singletons that TRUNCATE used to leave standing.
|
||||
//
|
||||
// `disk_cache` holds a free-space reading for up to its TTL. TRUNCATE has just deleted every
|
||||
// uploaded file, which materially changes free space — so without this the next test can
|
||||
// compute a storage quota from the PREVIOUS test's disk. That was harmless only while quotas
|
||||
// were globally disabled in e2e (they no longer are: see specs/02-upload/quota.spec.ts, which
|
||||
// steers the per-user limit off `free_disk_bytes`), i.e. two holes were masking each other.
|
||||
state.disk_cache.invalidate();
|
||||
|
||||
// `sse_tickets` maps a ticket to a session token hash. TRUNCATE deletes the sessions, so every
|
||||
// surviving ticket is a dangling reference to a user that no longer exists.
|
||||
state.sse_tickets.clear();
|
||||
|
||||
Ok(StatusCode::NO_CONTENT)
|
||||
}
|
||||
|
||||
|
||||
@@ -168,14 +168,14 @@ pub async fn upload(
|
||||
// Validate caption length. Counted in chars (code points) to match the
|
||||
// "Zeichen" wording in the error message — `.len()` would be bytes and
|
||||
// reject perfectly valid German/emoji captions early.
|
||||
if let Some(ref cap) = caption {
|
||||
if cap.chars().count() > MAX_CAPTION_LENGTH {
|
||||
let _ = tokio::fs::remove_file(&temp_abs).await;
|
||||
return Err(AppError::BadRequest(format!(
|
||||
"Beschreibung ist zu lang. Maximum: {} Zeichen.",
|
||||
MAX_CAPTION_LENGTH
|
||||
)));
|
||||
}
|
||||
if let Some(ref cap) = caption
|
||||
&& cap.chars().count() > MAX_CAPTION_LENGTH
|
||||
{
|
||||
let _ = tokio::fs::remove_file(&temp_abs).await;
|
||||
return Err(AppError::BadRequest(format!(
|
||||
"Beschreibung ist zu lang. Maximum: {} Zeichen.",
|
||||
MAX_CAPTION_LENGTH
|
||||
)));
|
||||
}
|
||||
|
||||
// Determine the file type from its magic bytes and require it to be on the
|
||||
@@ -555,6 +555,9 @@ pub struct QuotaEstimate {
|
||||
pub limit_bytes: Option<i64>,
|
||||
pub active_uploaders: i64,
|
||||
pub free_disk_bytes: i64,
|
||||
/// The tolerance factor the limit above was computed with. Carried on the snapshot so the
|
||||
/// number is self-describing; no caller reads it back today.
|
||||
#[allow(dead_code)]
|
||||
pub tolerance: f64,
|
||||
}
|
||||
|
||||
|
||||
@@ -3,6 +3,10 @@ use serde::Serialize;
|
||||
use sqlx::PgPool;
|
||||
use uuid::Uuid;
|
||||
|
||||
// Row shape for `comment`: every field is populated by sqlx from `SELECT *` / `RETURNING *`.
|
||||
// `deleted_at` is not read in Rust today (the soft-delete filter lives in SQL), but it is part of
|
||||
// the row and stays here so the struct keeps mirroring the table.
|
||||
#[allow(dead_code)]
|
||||
#[derive(Debug, sqlx::FromRow)]
|
||||
pub struct Comment {
|
||||
pub id: Uuid,
|
||||
@@ -87,16 +91,8 @@ impl Comment {
|
||||
.await
|
||||
}
|
||||
|
||||
pub async fn soft_delete(pool: &PgPool, id: Uuid) -> Result<(), sqlx::Error> {
|
||||
sqlx::query("UPDATE comment SET deleted_at = NOW() WHERE id = $1")
|
||||
.bind(id)
|
||||
.execute(pool)
|
||||
.await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Event-scoped variant of [`Self::soft_delete`]. Returns `false` if the
|
||||
/// comment doesn't exist or belongs to a different event.
|
||||
/// Event-scoped soft delete. Returns `false` if the comment doesn't exist or belongs to a
|
||||
/// different event.
|
||||
/// Executor-generic so the delete and the keepsake regeneration can share one transaction
|
||||
/// (see `Upload::soft_delete_in_event` for why that must be atomic).
|
||||
pub async fn soft_delete_in_event(
|
||||
|
||||
@@ -2,6 +2,11 @@ use chrono::{DateTime, Utc};
|
||||
use sqlx::PgPool;
|
||||
use uuid::Uuid;
|
||||
|
||||
// Row shape for `event`: every field is populated by sqlx from `SELECT *` / `RETURNING *`. Several
|
||||
// (`slug`, `cover_image_path`, `export_epoch`, `created_at`) are not read through this struct today
|
||||
// — callers that need them query the column directly — but they are part of the row and stay here so
|
||||
// the struct keeps mirroring the table.
|
||||
#[allow(dead_code)]
|
||||
#[derive(Debug, sqlx::FromRow)]
|
||||
pub struct Event {
|
||||
pub id: Uuid,
|
||||
|
||||
@@ -1,6 +1,8 @@
|
||||
use sqlx::PgPool;
|
||||
use uuid::Uuid;
|
||||
|
||||
// Row shape for `hashtag`, populated by sqlx from `RETURNING *` in `upsert`. Callers only use
|
||||
// `id` today; `event_id`/`tag` are the rest of the row and stay part of the struct.
|
||||
#[allow(dead_code)]
|
||||
#[derive(Debug, sqlx::FromRow)]
|
||||
pub struct Hashtag {
|
||||
pub id: Uuid,
|
||||
@@ -61,22 +63,6 @@ impl Hashtag {
|
||||
.await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub async fn tags_for_upload(
|
||||
pool: &PgPool,
|
||||
upload_id: Uuid,
|
||||
) -> Result<Vec<String>, sqlx::Error> {
|
||||
let rows: Vec<(String,)> = sqlx::query_as(
|
||||
"SELECT h.tag FROM hashtag h
|
||||
JOIN upload_hashtag uh ON uh.hashtag_id = h.id
|
||||
WHERE uh.upload_id = $1
|
||||
ORDER BY h.tag",
|
||||
)
|
||||
.bind(upload_id)
|
||||
.fetch_all(pool)
|
||||
.await?;
|
||||
Ok(rows.into_iter().map(|r| r.0).collect())
|
||||
}
|
||||
}
|
||||
|
||||
/// Extract `#hashtags` from text (caption or body). Tags are restricted to
|
||||
|
||||
@@ -2,6 +2,9 @@ use chrono::{DateTime, Utc};
|
||||
use sqlx::PgPool;
|
||||
use uuid::Uuid;
|
||||
|
||||
// Row shape for `session`, populated by sqlx from `RETURNING *`. Session validation is done in SQL
|
||||
// (expiry/last-seen predicates), so no field is read in Rust — the struct is the row's shape.
|
||||
#[allow(dead_code)]
|
||||
#[derive(Debug, sqlx::FromRow)]
|
||||
pub struct Session {
|
||||
pub id: Uuid,
|
||||
|
||||
@@ -3,6 +3,10 @@ use serde::Serialize;
|
||||
use sqlx::PgPool;
|
||||
use uuid::Uuid;
|
||||
|
||||
// Row shape for `upload`: every field is populated by sqlx from `RETURNING *` in `create`. Callers
|
||||
// mostly use `id` and hand the rest to the compression/feed queries, so most fields are never read
|
||||
// through this struct — they stay here so it keeps mirroring the table.
|
||||
#[allow(dead_code)]
|
||||
#[derive(Debug, sqlx::FromRow)]
|
||||
pub struct Upload {
|
||||
pub id: Uuid,
|
||||
@@ -75,15 +79,6 @@ impl Upload {
|
||||
.await
|
||||
}
|
||||
|
||||
pub async fn find_by_id(pool: &PgPool, id: Uuid) -> Result<Option<Self>, sqlx::Error> {
|
||||
sqlx::query_as::<_, Self>(
|
||||
"SELECT * FROM upload WHERE id = $1 AND deleted_at IS NULL",
|
||||
)
|
||||
.bind(id)
|
||||
.fetch_optional(pool)
|
||||
.await
|
||||
}
|
||||
|
||||
/// Lean lookup for the public media aliases (`get_original`/`get_preview`/
|
||||
/// `get_thumbnail`): returns ONLY the file paths + mime for a visible upload —
|
||||
/// excluding soft-deleted rows, hidden owners (`uploads_hidden`), and banned owners
|
||||
|
||||
@@ -22,6 +22,10 @@ impl UserRole {
|
||||
}
|
||||
}
|
||||
|
||||
// Row shape for `user`: every field is populated by sqlx from `SELECT *` / `RETURNING *`.
|
||||
// `uploads_hidden`, `failed_pin_attempts` and `created_at` are enforced/updated in SQL rather than
|
||||
// read in Rust, but they are part of the row and stay here so the struct keeps mirroring the table.
|
||||
#[allow(dead_code)]
|
||||
#[derive(Debug, sqlx::FromRow)]
|
||||
pub struct User {
|
||||
pub id: Uuid,
|
||||
|
||||
@@ -34,6 +34,17 @@ impl DiskCache {
|
||||
}
|
||||
}
|
||||
|
||||
/// Drop the cached reading so the next `snapshot()` re-measures the filesystem.
|
||||
///
|
||||
/// Used by the e2e TRUNCATE endpoint. Truncating deletes every uploaded file, which materially
|
||||
/// changes free space — but the cached reading survives for up to the TTL, so the next test can
|
||||
/// compute a quota from the PREVIOUS test's disk. That matters now that the quota tests steer
|
||||
/// the per-user limit off `free_disk_bytes`: a stale reading makes the limit wrong and the test
|
||||
/// flaky, for reasons that have nothing to do with the code under test.
|
||||
pub fn invalidate(&self) {
|
||||
*self.inner.write().unwrap() = None;
|
||||
}
|
||||
|
||||
/// Cached `(total, free)` bytes for the filesystem that holds `media_path`.
|
||||
///
|
||||
/// Returns `None` when the mount can't be resolved — callers MUST treat that as
|
||||
@@ -47,10 +58,11 @@ impl DiskCache {
|
||||
/// caller to ask about a different volume (e.g. the exports volume, which is a separate mount)
|
||||
/// would have silently got the wrong filesystem's free space.
|
||||
pub fn snapshot(&self, path: &Path) -> Option<DiskInfo> {
|
||||
if let Some((cached_path, info, at)) = self.inner.read().unwrap().as_ref() {
|
||||
if cached_path == path && at.elapsed() < TTL {
|
||||
return Some(*info);
|
||||
}
|
||||
if let Some((cached_path, info, at)) = self.inner.read().unwrap().as_ref()
|
||||
&& cached_path == path
|
||||
&& at.elapsed() < TTL
|
||||
{
|
||||
return Some(*info);
|
||||
}
|
||||
let info = read_disk_for_path(path)?;
|
||||
*self.inner.write().unwrap() = Some((path.to_path_buf(), info, Instant::now()));
|
||||
|
||||
@@ -389,6 +389,10 @@ async fn invalidate_missing_files(
|
||||
/// start immediately.
|
||||
pub const REGEN_DEBOUNCE: Duration = Duration::from_secs(20);
|
||||
|
||||
// Export worker entry point: every argument is state the spawned worker is BORN with (notably
|
||||
// `epoch`). Bundling them into a struct would be a pure-refactor risk on the epoch logic for no
|
||||
// gain, so the arity stands.
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
pub fn spawn_export_jobs(
|
||||
event_id: Uuid,
|
||||
event_name: String,
|
||||
|
||||
@@ -2,7 +2,6 @@ pub mod compression;
|
||||
pub mod config;
|
||||
pub mod disk;
|
||||
pub mod export;
|
||||
pub mod jobs;
|
||||
pub mod maintenance;
|
||||
pub mod rate_limiter;
|
||||
pub mod sse_tickets;
|
||||
|
||||
@@ -123,12 +123,58 @@ mod tests {
|
||||
assert!(rl.check("k", 1, w), "the slot should expire once the window passes");
|
||||
}
|
||||
|
||||
/// `retry_after` is not a "some number in range" — it is the time until the oldest slot
|
||||
/// in the window frees up, and it is surfaced to clients as the backoff they sleep for
|
||||
/// (see `upload-queue.ts`). Asserting only `(1..=60)` spans the entire reachable domain
|
||||
/// of a 60s window, so a hardcoded `Err(1)` would satisfy it while telling every client
|
||||
/// to hammer the server a second later. Pin the actual value.
|
||||
#[test]
|
||||
fn retry_after_is_between_one_and_window() {
|
||||
fn retry_after_is_the_remaining_window() {
|
||||
let rl = RateLimiter::new();
|
||||
assert!(rl.check_with_retry("k", 1, MIN).is_ok());
|
||||
let retry = rl.check_with_retry("k", 1, MIN).unwrap_err();
|
||||
assert!((1..=60).contains(&retry), "retry_after {retry} out of range");
|
||||
|
||||
// The slot was consumed just now, so essentially the whole window remains.
|
||||
// `as_secs()` truncates the sub-second remainder, so a 30s window reports 29.
|
||||
let w30 = Duration::from_secs(30);
|
||||
assert!(rl.check_with_retry("a", 1, w30).is_ok());
|
||||
let a = rl.check_with_retry("a", 1, w30).unwrap_err();
|
||||
assert_eq!(a, 29, "retry_after must be the remaining window, got {a}");
|
||||
|
||||
// A different window must yield a different retry_after: no single constant can
|
||||
// satisfy both this and the assertion above.
|
||||
let w10 = Duration::from_secs(10);
|
||||
assert!(rl.check_with_retry("b", 1, w10).is_ok());
|
||||
let b = rl.check_with_retry("b", 1, w10).unwrap_err();
|
||||
assert_eq!(b, 9, "retry_after must scale with the window, got {b}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn retry_after_counts_down_as_the_window_elapses() {
|
||||
let rl = RateLimiter::new();
|
||||
let w = Duration::from_secs(30);
|
||||
assert!(rl.check_with_retry("k", 1, w).is_ok());
|
||||
let first = rl.check_with_retry("k", 1, w).unwrap_err();
|
||||
|
||||
std::thread::sleep(Duration::from_millis(1200));
|
||||
let second = rl.check_with_retry("k", 1, w).unwrap_err();
|
||||
|
||||
// A client that waits 1.2s must be told to wait ~1.2s less — otherwise the advertised
|
||||
// backoff is a constant, not a deadline.
|
||||
let shaved = first - second;
|
||||
assert!(
|
||||
(1..=2).contains(&shaved),
|
||||
"1.2s of waiting must shorten the advertised backoff by ~1s (got {first} then {second})"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn retry_after_floors_at_one_second() {
|
||||
let rl = RateLimiter::new();
|
||||
let w = Duration::from_millis(800);
|
||||
assert!(rl.check_with_retry("k", 1, w).is_ok());
|
||||
let retry = rl.check_with_retry("k", 1, w).unwrap_err();
|
||||
// The sub-second remainder truncates to 0; clients must never be told "retry in 0s"
|
||||
// (that's a busy-loop). The `.max(1)` floor is what prevents it.
|
||||
assert_eq!(retry, 1, "a sub-second remainder must floor to 1, got {retry}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -140,6 +186,59 @@ mod tests {
|
||||
assert!(rl.check("k", 1, MIN), "clear() must free the window");
|
||||
}
|
||||
|
||||
/// `prune()` is a memory-leak guard: without it a long-lived process keeps one HashMap
|
||||
/// entry per IP that ever connected. Nothing in the public API observes the map size, so
|
||||
/// the only way to catch a no-op body (`fn prune(&self) {}`) is to look at the map — the
|
||||
/// tests module can see the private field.
|
||||
#[test]
|
||||
fn prune_drops_keys_whose_windows_have_fully_expired() {
|
||||
let rl = RateLimiter::new();
|
||||
|
||||
// A key whose only timestamp is older than the 24h ceiling. We can't sleep for a day,
|
||||
// so backdate the Instant directly.
|
||||
let ancient = Instant::now()
|
||||
.checked_sub(Duration::from_secs(25 * 60 * 60))
|
||||
.expect("backdating an Instant by 25h");
|
||||
rl.windows
|
||||
.lock()
|
||||
.unwrap()
|
||||
.insert("stale".to_string(), vec![ancient]);
|
||||
|
||||
// ...alongside a key that is still inside its window.
|
||||
assert!(rl.check("live", 5, MIN));
|
||||
assert_eq!(rl.windows.lock().unwrap().len(), 2);
|
||||
|
||||
rl.prune();
|
||||
|
||||
let map = rl.windows.lock().unwrap();
|
||||
assert!(
|
||||
!map.contains_key("stale"),
|
||||
"prune() must drop keys whose timestamps have all expired"
|
||||
);
|
||||
assert!(
|
||||
map.contains_key("live"),
|
||||
"prune() must keep keys that still have live timestamps"
|
||||
);
|
||||
assert_eq!(map.len(), 1, "exactly one key should survive the prune");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn prune_does_not_reset_a_live_window() {
|
||||
// The counterpart to the test above: pruning must reclaim memory, never quota. If
|
||||
// prune() dropped live keys, every background sweep would hand attackers a fresh
|
||||
// budget.
|
||||
let rl = RateLimiter::new();
|
||||
assert!(rl.check("k", 1, MIN));
|
||||
assert!(!rl.check("k", 1, MIN));
|
||||
|
||||
rl.prune();
|
||||
|
||||
assert!(
|
||||
!rl.check("k", 1, MIN),
|
||||
"prune() must not clear a window that is still active"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn client_ip_takes_rightmost_forwarded_for_entry() {
|
||||
// The right-most entry is the hop our trusted proxy (Caddy) appended.
|
||||
|
||||
@@ -31,6 +31,13 @@ impl SseTicketStore {
|
||||
}
|
||||
}
|
||||
|
||||
/// Drop every outstanding ticket. Used by the e2e TRUNCATE endpoint: tickets are bound to a
|
||||
/// session token hash, and TRUNCATE deletes the sessions out from under them, so anything left
|
||||
/// here is a dangling reference to a user that no longer exists.
|
||||
pub fn clear(&self) {
|
||||
self.inner.lock().unwrap().clear();
|
||||
}
|
||||
|
||||
/// Mint a new ticket bound to the caller's session (identified by token hash).
|
||||
pub fn issue(&self, token_hash: String) -> String {
|
||||
let ticket = random_ticket();
|
||||
|
||||
Reference in New Issue
Block a user