fabi/EventSnap
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor(export): share the visibility filter between the row query and the estimate

Browse Source 
`query_uploads` selects the rows the archives are built from; `estimate_export_bytes`
sizes them for the disk preflight. They stated the same WHERE clause separately, and
the direction of drift matters: an estimate that MISSES rows the archive writes
under-reserves, which is precisely the ENOSPC the preflight exists to prevent.

The integration test claimed to guard this and cannot. Both sides of
`the_estimate_sums_exactly_the_rows_the_archive_will_contain` are `SRC:`-marked
hand-copies in tests/common/mod.rs -- neither is production code -- so drift means
production moved while both copies sat still, and the test goes on passing. The
convention is sound for pinning behaviour; it is structurally incapable of detecting
divergence from the thing it copies.

So fix it where it can be fixed. One `export_visibility_where!()` fragment,
`concat!`-ed into both queries at compile time (still `&'static str`, no allocation),
with the `u`/`usr` alias contract stated. Divergence is now impossible by
construction rather than watched for.

The tests keep their value and lose the overclaim: the docstrings now say they pin
WHICH uploads may be counted -- each excluded row in the fixture is excluded by a
different predicate, so weakening any one of them still fails here -- and say plainly
that they do not detect drift, with a pointer to what does.

No behaviour change. The filters were verified identical before the hoist
(`u.event_id = $1 AND u.deleted_at IS NULL AND usr.uploads_hidden = FALSE AND
usr.is_banned = FALSE`); 99 backend tests still pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
fabi
2026-07-29 20:50:04 +02:00
parent 5f702f2b40
commit 06bc9ddcb3
3 changed files with 57 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

43
backend/src/services/export.rs
View File

@@ -20,6 +20,29 @@ use crate::state::SseEvent;
static VIEWER_DIR: Dir<'_> = include_dir!("$CARGO_MANIFEST_DIR/static/export-viewer");
// ── Shared visibility filter ─────────────────────────────────────────────────
/// The predicate that decides what lands in a keepsake, as ONE definition.
///
/// Two queries have to agree on it: [`query_uploads`], which selects the rows the archives are
/// built from, and [`estimate_export_bytes`], which sizes them for the disk preflight. They used
/// to state it separately, and the direction of drift matters — an estimate that misses rows the
/// archive writes UNDER-reserves, which is the exact ENOSPC the preflight exists to prevent.
///
/// A `SRC:`-marked copy in the integration tests cannot catch that: drift means production moved
/// and the copy didn't, so both sides of such a test sit still and it keeps passing. Sharing the
/// fragment removes the failure by construction instead, and leaves the test doing what it is
/// actually good at — pinning the behaviour.
///
/// CONTRACT: callers must alias `upload` as `u` and join `"user"` as `usr`, and bind the event id
/// as `$1`.
macro_rules! export_visibility_where {
() => {
"WHERE u.event_id = $1 AND u.deleted_at IS NULL
AND usr.uploads_hidden = FALSE AND usr.is_banned = FALSE"
};
}
// ── DB query rows ────────────────────────────────────────────────────────────
#[derive(sqlx::FromRow)]
@@ -1051,7 +1074,7 @@ async fn run_html_export_inner(
// ── DB helpers ───────────────────────────────────────────────────────────────
async fn query_uploads(pool: &PgPool, event_id: Uuid) -> Result<Vec<ExportUploadRow>> {
Ok(sqlx::query_as::<_, ExportUploadRow>(
Ok(sqlx::query_as::<_, ExportUploadRow>(concat!(
"SELECT u.id, u.original_path, u.mime_type, u.caption,
usr.display_name AS uploader_name,
COUNT(DISTINCT l.user_id) AS like_count,
@@ -1059,11 +1082,12 @@ async fn query_uploads(pool: &PgPool, event_id: Uuid) -> Result<Vec<ExportUpload
FROM upload u
JOIN \"user\" usr ON usr.id = u.user_id
LEFT JOIN \"like\" l ON l.upload_id = u.id
WHERE u.event_id = $1 AND u.deleted_at IS NULL
AND usr.uploads_hidden = FALSE AND usr.is_banned = FALSE
",
export_visibility_where!(),
"
GROUP BY u.id, usr.display_name
ORDER BY u.created_at ASC",
)
))
.bind(event_id)
.fetch_all(pool)
.await?)
@@ -1267,15 +1291,16 @@ fn is_superseded_archive(
/// downscaled to 2000px first, which only makes this estimate more conservative — the direction we
/// want, since being wrong low means ENOSPC halfway through.
///
/// Matches [`query_uploads`]' visibility filter exactly, so hidden/banned uploads aren't counted.
/// Shares [`query_uploads`]' visibility filter via [`export_visibility_where`], so hidden/banned
/// uploads can't be counted here but skipped there (or the reverse, which under-reserves).
pub async fn estimate_export_bytes(pool: &PgPool, event_id: Uuid) -> Result<u64> {
let (bytes,): (i64,) = sqlx::query_as(
let (bytes,): (i64,) = sqlx::query_as(concat!(
"SELECT COALESCE(SUM(u.original_size_bytes), 0)::bigint
FROM upload u
JOIN \"user\" usr ON usr.id = u.user_id
WHERE u.event_id = $1 AND u.deleted_at IS NULL
AND usr.uploads_hidden = FALSE AND usr.is_banned = FALSE",
)
",
export_visibility_where!(),
))
.bind(event_id)
.fetch_one(pool)
.await

8
backend/tests/common/mod.rs
View File

@@ -293,6 +293,10 @@ pub async fn set_user_moderation(pool: &PgPool, user_id: Uuid, banned: bool, hid
/// SRC: `services/export.rs::query_uploads` — the visibility filter, verbatim, projected down to
/// `(id, original_size_bytes)`. This is the row set that ACTUALLY lands in the archives.
///
/// Production builds this WHERE from `export_visibility_where!()`, shared with
/// `estimate_export_bytes`. A copy here can pin the behaviour but CANNOT detect production moving
/// away from it — that is what sharing the fragment is for, not this.
pub async fn export_visible_uploads(pool: &PgPool, event_id: Uuid) -> Vec<(Uuid, i64)> {
sqlx::query_as(
"SELECT u.id, u.original_size_bytes
@@ -309,7 +313,9 @@ pub async fn export_visible_uploads(pool: &PgPool, event_id: Uuid) -> Vec<(Uuid,
.expect("export_visible_uploads")
}
/// SRC: `services/export.rs::estimate_export_bytes` — verbatim.
/// SRC: `services/export.rs::estimate_export_bytes` — verbatim. Same caveat as above: production
/// shares its WHERE with `query_uploads` via `export_visibility_where!()`, so these two copies
/// agreeing proves the behaviour, not the absence of drift.
pub async fn estimate_export_bytes(pool: &PgPool, event_id: Uuid) -> i64 {
let (bytes,): (i64,) = sqlx::query_as(
"SELECT COALESCE(SUM(u.original_size_bytes), 0)::bigint

23
backend/tests/export_preflight.rs
View File

@@ -16,10 +16,18 @@
//!
//! What these tests pin is the ESTIMATE — the part that decides. The arithmetic on top of it lives
//! in `services/export.rs`'s unit tests; the filesystem selection lives in `is_superseded_archive`.
//! The risk here is drift: if `query_uploads` ever gains or loses a visibility predicate and
//! `estimate_export_bytes` doesn't, the preflight silently sizes the wrong gallery. So rather than
//! restating the filter, these assert the estimate against the row set the archive actually
//! contains.
//!
//! ON DRIFT, precisely, because it is easy to overclaim here. The hazard is that `query_uploads`
//! (which selects the rows the archives are built from) and `estimate_export_bytes` (which sizes
//! them) could disagree — and an estimate missing rows the archive writes UNDER-reserves, the one
//! direction that reintroduces the ENOSPC. **These tests cannot catch that**, and neither can any
//! test in this harness: both sides here are `SRC:`-marked hand-copies in `tests/common/mod.rs`,
//! so if production moved and the copies didn't, they would sit still and keep passing.
//!
//! That is fixed where it can be — the two queries now share one `export_visibility_where!()`
//! fragment in `services/export.rs`, so they cannot diverge by construction. What is left for
//! these tests is what the convention is genuinely good at: pinning the BEHAVIOUR, so a change
//! that deliberately alters the filter has to come here and say so.
mod common;
@@ -29,9 +37,10 @@ use sqlx::PgPool;
/// The estimate must equal the sum over EXACTLY the rows `query_uploads` returns — computed from
/// that row set, not from a restatement of its WHERE clause.
///
/// PREVENTS: the two queries drifting apart. An estimate that counts rows the archive skips is
/// merely pessimistic; one that MISSES rows the archive writes under-reserves, which is the whole
/// failure being guarded against.
/// PINS: which uploads the preflight is allowed to count. Each excluded row below is excluded by a
/// DIFFERENT predicate, so a change that drops or weakens any one of them fails here and has to be
/// argued for. (It does not detect production drifting away from these copies — see the file
/// header; `export_visibility_where!()` is what makes that impossible.)
#[sqlx::test]
async fn the_estimate_sums_exactly_the_rows_the_archive_will_contain(pool: PgPool) {
let event_id = seed_event(&pool, "wedding").await;