From b948cae269f9e37689aee8ae5c3c3b3430ffe834 Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Tue, 2 Jun 2026 13:35:36 +0200 Subject: [PATCH] refactor(db): share update path so set_visibility avoids a redundant fetch; tie public-visibility const to the enum; test internal exclusion Co-Authored-By: Claude Sonnet 4.6 --- crates/db/src/catalog.rs | 59 +++++++++++++++++++++++------------ crates/db/tests/visibility.rs | 15 +++++++++ crates/domain/src/object.rs | 2 +- 3 files changed, 55 insertions(+), 21 deletions(-) diff --git a/crates/db/src/catalog.rs b/crates/db/src/catalog.rs index bd60672..f09d243 100644 --- a/crates/db/src/catalog.rs +++ b/crates/db/src/catalog.rs @@ -14,7 +14,7 @@ use crate::{audit, authority, fields, vocab}; const ENTITY_TYPE: &str = "object"; /// The visibility value eligible for the public surface. -const PUBLIC_VISIBILITY: &str = "public"; +const PUBLIC_VISIBILITY: &str = Visibility::Public.as_str(); const OBJECT_COLUMNS: &str = "id, object_number, object_name, number_of_objects, \ brief_description, current_location, current_owner, recorder, recording_date, \ @@ -117,6 +117,9 @@ where } /// List **public** objects ordered by object number, with `limit`/`offset` paging. +/// +/// `limit` and `offset` must be non-negative (Postgres rejects a negative `LIMIT`); +/// the public API layer clamps them before calling. pub async fn list_public_objects<'e, E>( executor: E, limit: i64, @@ -249,10 +252,25 @@ pub async fn update_object( return Ok(false); }; - let changes = update_changes(&old.to_input(), input); + apply_object_update(&mut *conn, actor, id, &old.to_input(), input).await?; + + Ok(true) +} + +/// Diff `old`→`new`, write the changed columns + an `updated` audit entry, both on +/// `conn`. A no-op (no field changed) touches neither the row's `updated_at` nor the +/// audit log. +async fn apply_object_update( + conn: &mut sqlx::PgConnection, + actor: AuditActor, + id: ObjectId, + old: &ObjectInput, + new: &ObjectInput, +) -> Result<(), sqlx::Error> { + let changes = update_changes(old, new); + if changes.is_empty() { - // No-op: don't touch updated_at or the audit log. - return Ok(true); + return Ok(()); } sqlx::query( @@ -263,15 +281,15 @@ pub async fn update_object( WHERE id = $1", ) .bind(id.to_uuid()) - .bind(&input.object_number) - .bind(&input.object_name) - .bind(input.number_of_objects) - .bind(input.brief_description.as_deref()) - .bind(input.current_location.as_deref()) - .bind(input.current_owner.as_deref()) - .bind(input.recorder.as_deref()) - .bind(input.recording_date) - .bind(input.visibility.as_str()) + .bind(&new.object_number) + .bind(&new.object_name) + .bind(new.number_of_objects) + .bind(new.brief_description.as_deref()) + .bind(new.current_location.as_deref()) + .bind(new.current_owner.as_deref()) + .bind(new.recorder.as_deref()) + .bind(new.recording_date) + .bind(new.visibility.as_str()) .execute(&mut *conn) .await?; @@ -287,7 +305,7 @@ pub async fn update_object( ) .await?; - Ok(true) + Ok(()) } /// Why changing an object's visibility failed. @@ -302,9 +320,9 @@ pub enum VisibilityError { } /// Move an object to `target` visibility, enforcing the stepwise state machine, and -/// audit the change. Reuses [`update_object`]'s diff/audit path, so only `visibility` -/// appears in the audit entry — and setting to the current value is an idempotent no-op -/// (no row touch, no audit). Pass a transaction connection. +/// audit the change. Uses the same diff/audit path as `update_object`, so only +/// `visibility` appears in the audit entry — and setting to the current value is an +/// idempotent no-op (no row touch, no audit). Pass a transaction connection. pub async fn set_visibility( conn: &mut sqlx::PgConnection, actor: AuditActor, @@ -317,10 +335,11 @@ pub async fn set_visibility( let new_visibility = object.visibility.transition_to(target)?; - let mut input = object.to_input(); + let old_input = object.to_input(); + let mut new_input = old_input.clone(); - input.visibility = new_visibility; - update_object(&mut *conn, actor, id, &input).await?; + new_input.visibility = new_visibility; + apply_object_update(&mut *conn, actor, id, &old_input, &new_input).await?; Ok(()) } diff --git a/crates/db/tests/visibility.rs b/crates/db/tests/visibility.rs index c020840..37a2d64 100644 --- a/crates/db/tests/visibility.rs +++ b/crates/db/tests/visibility.rs @@ -140,6 +140,13 @@ async fn public_reads_return_only_public_records(pool: PgPool) { ) .await .unwrap(); + let internal = catalog::create_object( + &mut tx, + AuditActor::System, + &object("I-1", Visibility::Internal), + ) + .await + .unwrap(); tx.commit().await.unwrap(); assert!( @@ -168,4 +175,12 @@ async fn public_reads_return_only_public_records(pool: PgPool) { .unwrap() .is_empty() ); + + // internal records are excluded from public reads too (not just draft) + assert!( + catalog::public_object_by_id(db.pool(), internal) + .await + .unwrap() + .is_none() + ); } diff --git a/crates/domain/src/object.rs b/crates/domain/src/object.rs index 474f787..eb3ded6 100644 --- a/crates/domain/src/object.rs +++ b/crates/domain/src/object.rs @@ -17,7 +17,7 @@ pub enum Visibility { } impl Visibility { - pub fn as_str(&self) -> &'static str { + pub const fn as_str(&self) -> &'static str { match self { Visibility::Draft => "draft", Visibility::Internal => "internal",