diff --git a/book/src/scoped-repositories.md b/book/src/scoped-repositories.md index 6fdae1c..b1411ee 100644 --- a/book/src/scoped-repositories.md +++ b/book/src/scoped-repositories.md @@ -113,6 +113,56 @@ dereferenced, so they cannot be used to probe for the existence of foreign ids. Replaying a cursor minted under a different scope yields well-defined (scoped) but position-shifted results. +## Filtering on the scope column + +By default the scope column generates no query surface of its own — every +read is already filtered by it, and per-scope listing *is* the ordinary +scoped `list_by_*(Only(value), ..)`. But some callers legitimately filter by +the scope column *through the normal query surface*: an all-access admin +listing that narrows to one tenant, for example. The scope value itself +(typically authz-derived) must never be touched by caller input — the +caller's choice belongs in the `Filters` struct like any other filter. + +For that, the scope column may **opt into** `find_by = true`, `list_by` or +`list_for`: + +```rust,ignore +partner_id(ty = "PartnerId", scope, find_by = true, list_for(by(created_at))), +``` + +This generates the usual fns (`find_by_partner_id`, `list_for_partner_id_by_*`) +and includes `partner_id: Option` in the generated `Filters` +struct. The caller value **composes** with the scope — it can narrow, never +widen: + +| Scope | Caller value | Result | +|-----------|--------------|---------------------------------------------------| +| `All` | none | unfiltered | +| `All` | `p` | `WHERE partner_id = p` | +| `Only(a)` | none | `WHERE partner_id = a` | +| `Only(a)` | `b` | `WHERE partner_id = b AND partner_id = a` — **empty unless `a == b`** | + +Under `Only`, the column is simply double-specified — once as the caller's +filter, once as the scope conjunct, exactly like any other filter column. A +mismatching caller value is a contradictory predicate that honestly returns +an empty result (`NotFound`/`None` for `find_by_*`) instead of being +silently ignored — a caller filter can narrow but never widen the scope. +Both predicates are plain equalities, so the query stays sargable against a +scope-led index. + +```rust,ignore +// admin listing: scope from authz, partner choice from the request +let scope = self.authz.enforce_permission(sub, obj, act).await?; // untouched +self.repo + .list_for_filters( + scope, + CustomerFilters { partner_id: request.partner_id, ..Default::default() }, + sort, + args, + ) + .await? +``` + ## Validation rules The macro rejects at compile time: @@ -121,14 +171,12 @@ The macro rejects at compile time: - an `Option` or `nullable`-annotated scope column (nullable scope columns are not supported — every row must belong to exactly one scope) - a `Forgettable` scope column -- `find_by`, `list_by` or `list_for` on the scope column itself — every read - is already filtered by it; per-scope listing *is* the ordinary scoped - `list_by_*(Only(value), ..)` - `scope` on nested repos — children are custody-guarded via their (scoped) parent -The scope column also generates no `find_by_partner_id` accessors: the scope -argument replaces them. +Without an explicit opt-in (see above) the scope column generates no +`find_by_partner_id` accessors: `scope` flips the column's `find_by` default +to `false`, and the scope argument replaces them. ## Index requirements diff --git a/es-entity-macros/src/repo/list_for_filters_fn.rs b/es-entity-macros/src/repo/list_for_filters_fn.rs index ef58e08..81f81d4 100644 --- a/es-entity-macros/src/repo/list_for_filters_fn.rs +++ b/es-entity-macros/src/repo/list_for_filters_fn.rs @@ -479,12 +479,6 @@ impl<'a> ListForFiltersFn<'a> { }; let cursor_ident = cursor_struct.ident(); - let n_filters: u32 = self - .for_columns - .iter() - .map(|c| if c.is_optional() { 2u32 } else { 1u32 }) - .sum(); - let destructure_tokens = cursor_struct.destructure_tokens(); let select_columns = cursor_struct.select_columns(None); let cursor_arg_tokens = cursor_struct.query_arg_tokens(); @@ -570,28 +564,28 @@ impl<'a> ListForFiltersFn<'a> { select_columns, self.table_name, filter_where, - cursor_struct.condition(n_filters + scope_offset, true), + cursor_struct.condition(param_idx - 1, true), if delete == DeleteOption::No { self.delete.not_deleted_condition() } else { "" }, cursor_struct.order_by(true), - n_filters + scope_offset + 1, + param_idx, ); let desc_query = format!( r#"SELECT {} FROM {} WHERE {}({}){} ORDER BY {} LIMIT ${}"#, select_columns, self.table_name, filter_where, - cursor_struct.condition(n_filters + scope_offset, false), + cursor_struct.condition(param_idx - 1, false), if delete == DeleteOption::No { self.delete.not_deleted_condition() } else { "" }, cursor_struct.order_by(false), - n_filters + scope_offset + 1, + param_idx, ); (asc_query, desc_query, fallback_arg_tokens) }; diff --git a/es-entity-macros/src/repo/mod.rs b/es-entity-macros/src/repo/mod.rs index 91a6639..d6c356b 100644 --- a/es-entity-macros/src/repo/mod.rs +++ b/es-entity-macros/src/repo/mod.rs @@ -600,7 +600,9 @@ mod tests { } #[test] - fn scope_column_with_query_flags_is_error() { + fn scope_column_composes_with_query_flags() { + // `scope` may coexist with `find_by = true`, `list_by` and `list_for` + // on the same column — the generated fns compose with the scope. for extra in ["find_by = true", "list_by = true", "list_for"] { let src = format!( r#" @@ -614,15 +616,62 @@ mod tests { "# ); let input: syn::DeriveInput = syn::parse_str(&src).unwrap(); - let err = derive(input).unwrap_err(); - assert!( - err.to_string() - .contains("cannot also be find_by, list_by or list_for"), - "unexpected error for `{extra}`: {err}" - ); + derive(input) + .unwrap_or_else(|err| panic!("scope column with `{extra}` should derive: {err}")); } } + #[test] + fn scope_column_find_by_composes_via_conjunct() { + let input: syn::DeriveInput = parse_quote! { + #[es_repo( + entity = "User", + columns(partner_id(ty = "PartnerId", scope, find_by = true)) + )] + struct Users { + pool: sqlx::PgPool, + } + }; + let tokens = derive(input) + .expect("scoped repo should derive") + .to_string(); + // explicit opt-in generates the find fns, scope argument included + assert!(tokens.contains("fn find_by_partner_id (& self , scope : impl Into < UserScope >")); + assert!( + tokens + .contains("fn maybe_find_by_partner_id (& self , scope : impl Into < UserScope >") + ); + assert!(tokens.contains("WHERE partner_id = $1 AND partner_id = $2")); + } + + #[test] + fn scope_column_list_for_composes_via_conjunct() { + let input: syn::DeriveInput = parse_quote! { + #[es_repo( + entity = "User", + columns( + partner_id(ty = "PartnerId", scope, list_for(by(created_at))), + status(ty = "String", list_for(by(created_at))) + ) + )] + struct Users { + pool: sqlx::PgPool, + } + }; + let tokens = derive(input) + .expect("scoped repo should derive") + .to_string(); + // the Filters struct carries the scope column like any other + // list_for column + assert!(tokens.contains("pub struct UserFilters")); + assert!(tokens.contains("pub partner_id : Option < PartnerId >")); + assert!(tokens.contains("pub status : Option < String >")); + assert!(tokens.contains( + "fn list_for_partner_id_by_created_at (& self , scope : impl Into < UserScope >" + )); + assert!(tokens.contains("(partner_id = $1) AND partner_id = $2")); + } + #[test] fn scope_on_nested_child_repo_is_error() { let input: syn::DeriveInput = parse_quote! { diff --git a/es-entity-macros/src/repo/options/columns.rs b/es-entity-macros/src/repo/options/columns.rs index af9328f..d04067b 100644 --- a/es-entity-macros/src/repo/options/columns.rs +++ b/es-entity-macros/src/repo/options/columns.rs @@ -25,9 +25,7 @@ impl Columns { } pub fn all_find_by(&self) -> impl Iterator { - self.all - .iter() - .filter(|c| c.opts.find_by() && !c.opts.scope) + self.all.iter().filter(|c| c.opts.find_by()) } pub fn all_list_by(&self) -> impl Iterator { @@ -51,9 +49,6 @@ impl Columns { /// `nullable`-annotated types are rejected — nullable scope columns are /// a future feature) /// - the scope column must not be `Forgettable` - /// - the scope column must not also be a query column (`find_by`, - /// `list_by`, `list_for`) — every generated read is already filtered by - /// it, so per-tenant queries are the ordinary scoped fns /// - the scope column must not be the `parent` column (nested repos /// cannot be scoped — children are custody-guarded via their parent) pub fn validate_scope(&self) -> darling::Result<()> { @@ -78,15 +73,6 @@ impl Columns { col.name(), ))); } - if col.opts.find_by == Some(true) - || col.opts.list_by == Some(true) - || col.opts.list_for_opts.is_some() - { - return Err(darling::Error::custom(format!( - "scope column '{}' cannot also be find_by, list_by or list_for — every read is already filtered by it", - col.name(), - ))); - } if col.opts.parent_opts.is_some() { return Err(darling::Error::custom(format!( "scope column '{}' cannot be the parent column — nested repos cannot be scoped", @@ -836,7 +822,9 @@ impl ColumnOpts { } fn find_by(&self) -> bool { - self.find_by.unwrap_or(true) + // `scope` flips the default to false — every read is already + // filtered by the scope column; explicit `find_by = true` opts in. + self.find_by.unwrap_or(!self.scope) } fn list_by(&self) -> bool { diff --git a/tests/scoped_repo.rs b/tests/scoped_repo.rs index eca2677..770ddfa 100644 --- a/tests/scoped_repo.rs +++ b/tests/scoped_repo.rs @@ -11,11 +11,17 @@ use es_entity::*; /// reads by the scope column; `All` reads across scopes. Writes (`create`, /// `update`, `delete`) keep their unscoped signatures — mutations operate on /// entities that could only have been obtained through a scoped read. +/// +/// The scope column additionally opts into `find_by`/`list_for`: callers +/// filter by `partner_id` through the normal query surface, composing with +/// the scope — under `Only(a)` the column is double-specified (filter AND +/// scope conjunct), so a mismatching caller value is a contradictory +/// predicate returning an empty result. #[derive(EsRepo, Debug)] #[es_repo( entity = "Contact", columns( - partner_id(ty = "PartnerId", scope), + partner_id(ty = "PartnerId", scope, find_by = true, list_for(by(created_at))), email(ty = "String"), status(ty = "String", list_for(by(created_at))), ) @@ -253,6 +259,7 @@ async fn scoped_list_for_and_filters_dispatch() -> anyhow::Result<()> { partner_a, ContactFilters { status: Some("inactive".to_string()), + ..Default::default() }, Sort { by: ContactSortBy::CreatedAt, @@ -435,6 +442,7 @@ async fn scoped_view_delegates() -> anyhow::Result<()> { .list_for_filters( ContactFilters { status: Some("inactive".to_string()), + ..Default::default() }, Sort { by: ContactSortBy::CreatedAt, @@ -466,3 +474,274 @@ async fn scoped_view_delegates() -> anyhow::Result<()> { Ok(()) } + +/// The scope column opted into `find_by = true`: the lookup composes with +/// the scope via the double-specified conjunct (`partner_id = $1 AND +/// partner_id = $2`). Under `Only(a)` a lookup of `b != a` is a +/// contradictory predicate — not-found, indistinguishable from missing. +#[tokio::test] +async fn scope_column_find_by_composes() -> anyhow::Result<()> { + let pool = helpers::init_pool().await?; + let contacts = Contacts::new(pool); + + let partner_a = PartnerId::new(); + let partner_b = PartnerId::new(); + seed_contacts(&contacts, partner_a, &[("fb-a", "active")]).await?; + seed_contacts(&contacts, partner_b, &[("fb-b", "active")]).await?; + + // All + value: plain lookup by the scope column + let found = contacts + .find_by_partner_id(ContactScope::All, partner_a) + .await?; + assert_eq!(found.partner_id, partner_a); + + // Only(a) + a: match — behaves like the scoped read + let found = contacts.find_by_partner_id(partner_a, partner_a).await?; + assert_eq!(found.partner_id, partner_a); + + // Only(a) + b: mismatch — not-found, indistinguishable from missing + let err = contacts.find_by_partner_id(partner_a, partner_b).await; + assert!(matches!(err, Err(ContactFindError::NotFound { .. }))); + assert!( + contacts + .maybe_find_by_partner_id(partner_a, partner_b) + .await? + .is_none() + ); + + Ok(()) +} + +/// The scope column opted into `list_for`: `ContactFilters` carries +/// `partner_id` and the predicates compose. The four scope × filter +/// combinations: +/// +/// - `All` + `None` → unfiltered +/// - `All` + `Some(p)` → rows of `p` +/// - `Only(a)` + `None` → rows of `a` +/// - `Only(a)` + `Some(b)` → `partner_id = b AND partner_id = a` — empty +/// unless `a == b` (a caller filter can narrow but never widen the scope) +#[tokio::test] +async fn scope_column_filter_combinations() -> anyhow::Result<()> { + let pool = helpers::init_pool().await?; + let contacts = Contacts::new(pool); + + let partner_a = PartnerId::new(); + let partner_b = PartnerId::new(); + let ids_a = seed_contacts( + &contacts, + partner_a, + &[("c1", "active"), ("c2", "inactive")], + ) + .await?; + seed_contacts(&contacts, partner_b, &[("c3", "active")]).await?; + + let query = || PaginatedQueryArgs { + first: 100, + after: None, + }; + let sort = || Sort { + by: ContactSortBy::CreatedAt, + direction: ListDirection::Descending, + }; + let partner_filter = |p: PartnerId| ContactFilters { + partner_id: Some(p), + ..Default::default() + }; + + // All + None: unfiltered (other tests seed the shared table — check + // containment, not count) + let ret = contacts + .list_for_filters( + ContactScope::All, + ContactFilters::default(), + sort(), + query(), + ) + .await?; + assert!( + ids_a + .iter() + .all(|id| ret.entities.iter().any(|c| c.id == *id)) + ); + + // All + Some(a): the caller's partner choice narrows the listing + let ret = contacts + .list_for_filters( + ContactScope::All, + partner_filter(partner_a), + sort(), + query(), + ) + .await?; + assert_eq!(ret.entities.len(), 2); + assert!(ret.entities.iter().all(|c| c.partner_id == partner_a)); + + // Only(a) + None: the scope alone filters + let ret = contacts + .list_for_filters(partner_a, ContactFilters::default(), sort(), query()) + .await?; + assert_eq!(ret.entities.len(), 2); + + // Only(a) + Some(a): match — same result as the scope alone + let ret = contacts + .list_for_filters(partner_a, partner_filter(partner_a), sort(), query()) + .await?; + assert_eq!(ret.entities.len(), 2); + + // Only(a) + Some(b): a caller filter can never widen the scope — the + // mismatch honestly returns nothing instead of being silently ignored + let ret = contacts + .list_for_filters(partner_a, partner_filter(partner_b), sort(), query()) + .await?; + assert!(ret.entities.is_empty()); + assert!(!ret.has_next_page); + assert!(ret.end_cursor.is_none()); + + // dedicated single-filter fn composes the same way + let ret = contacts + .list_for_partner_id_by_created_at( + ContactScope::All, + partner_a, + PaginatedQueryArgs { + first: 100, + after: None, + }, + ListDirection::Descending, + ) + .await?; + assert_eq!(ret.entities.len(), 2); + let ret = contacts + .list_for_partner_id_by_created_at( + partner_a, + partner_b, + PaginatedQueryArgs { + first: 100, + after: None, + }, + ListDirection::Descending, + ) + .await?; + assert!(ret.entities.is_empty()); + + // multi-filter (scope column + status) routes through the filters fn + let ret = contacts + .list_for_filters( + partner_a, + ContactFilters { + partner_id: Some(partner_a), + status: Some("active".to_string()), + }, + sort(), + query(), + ) + .await?; + assert_eq!(ret.entities.len(), 1); + assert_eq!(ret.entities[0].status, "active"); + let ret = contacts + .list_for_filters( + partner_a, + ContactFilters { + partner_id: Some(partner_b), + status: Some("active".to_string()), + }, + sort(), + query(), + ) + .await?; + assert!(ret.entities.is_empty()); + let ret = contacts + .list_for_filters( + ContactScope::All, + ContactFilters { + partner_id: Some(partner_a), + status: Some("active".to_string()), + }, + sort(), + query(), + ) + .await?; + assert_eq!(ret.entities.len(), 1); + assert_eq!(ret.entities[0].partner_id, partner_a); + + Ok(()) +} + +/// Cursor pagination respects the composed predicates: every page of an +/// `All` + partner-filtered listing stays within the filtered partner, and a +/// scope/filter mismatch with a cursor still yields an empty page. +#[tokio::test] +async fn scope_column_filter_cursor_pagination() -> anyhow::Result<()> { + let pool = helpers::init_pool().await?; + let contacts = Contacts::new(pool); + + let partner_a = PartnerId::new(); + let partner_b = PartnerId::new(); + let specs: Vec<(&str, &str)> = (0..5).map(|_| ("pg", "active")).collect(); + let ids_a = seed_contacts(&contacts, partner_a, &specs).await?; + seed_contacts(&contacts, partner_b, &specs).await?; + + let mut collected = Vec::new(); + let mut after = None; + let mut pages = 0; + loop { + let ret = contacts + .list_for_filters( + ContactScope::All, + ContactFilters { + partner_id: Some(partner_a), + ..Default::default() + }, + Sort { + by: ContactSortBy::CreatedAt, + direction: ListDirection::Descending, + }, + PaginatedQueryArgs { first: 2, after }, + ) + .await?; + pages += 1; + for entity in &ret.entities { + assert_eq!( + entity.partner_id, partner_a, + "partner filter leaked a foreign row" + ); + } + collected.extend(ret.entities.iter().map(|c| c.id)); + if !ret.has_next_page { + break; + } + after = ret.end_cursor; + } + assert!(pages >= 3, "expected pagination across pages, got {pages}"); + let expected: std::collections::HashSet<_> = ids_a.into_iter().collect(); + let collected: std::collections::HashSet<_> = collected.into_iter().collect(); + assert_eq!(collected, expected); + + // mismatch + cursor: the contradictory conjunct still yields an empty page + let cursor_page = contacts + .list_for_partner_id_by_created_at( + ContactScope::All, + partner_a, + PaginatedQueryArgs { + first: 1, + after: None, + }, + ListDirection::Descending, + ) + .await?; + let ret = contacts + .list_for_partner_id_by_created_at( + partner_b, + partner_a, + PaginatedQueryArgs { + first: 100, + after: cursor_page.end_cursor, + }, + ListDirection::Descending, + ) + .await?; + assert!(ret.entities.is_empty()); + assert!(!ret.has_next_page); + + Ok(()) +} diff --git a/tests/scoped_repo_sargable.rs b/tests/scoped_repo_sargable.rs new file mode 100644 index 0000000..429609b --- /dev/null +++ b/tests/scoped_repo_sargable.rs @@ -0,0 +1,216 @@ +mod entities; +mod helpers; + +use sqlx::PgPool; + +use entities::contact::*; +use es_entity::*; + +/// The scoped `Contacts` repo with `sargable_filters` opted in: the +/// scope × filter composition must hold through the specialized per-state +/// query matrix, not just the catch-all fallback (exercised by +/// `scoped_repo.rs`, which keeps the default). The scope column doubles as a +/// `list_for` filter column; under `Only` it is double-specified (filter AND +/// scope conjunct) — a mismatch is a contradictory predicate returning no +/// rows. +#[derive(EsRepo, Debug)] +#[es_repo( + entity = "Contact", + columns( + partner_id(ty = "PartnerId", scope, list_for(by(created_at))), + email(ty = "String"), + status(ty = "String", list_for(by(created_at))), + ), + sargable_filters +)] +pub struct Contacts { + pool: PgPool, +} + +impl Contacts { + pub fn new(pool: PgPool) -> Self { + Self { pool } + } +} + +async fn seed_contacts( + repo: &Contacts, + partner_id: PartnerId, + specs: &[(&str, &str)], +) -> anyhow::Result> { + let mut ids = Vec::new(); + for (email, status) in specs { + let id = ContactId::new(); + let new = NewContact::builder() + .id(id) + .partner_id(partner_id) + .email(format!("{email}-{id}@test.com")) + .status(*status) + .build() + .unwrap(); + repo.create(new).await?; + ids.push(id); + } + Ok(ids) +} + +#[tokio::test] +async fn sargable_scope_column_filter_combinations() -> anyhow::Result<()> { + let pool = helpers::init_pool().await?; + let contacts = Contacts::new(pool); + + let partner_a = PartnerId::new(); + let partner_b = PartnerId::new(); + let ids_a = seed_contacts( + &contacts, + partner_a, + &[("s1", "active"), ("s2", "inactive")], + ) + .await?; + seed_contacts(&contacts, partner_b, &[("s3", "active")]).await?; + + let query = || PaginatedQueryArgs { + first: 100, + after: None, + }; + let sort = || Sort { + by: ContactSortBy::CreatedAt, + direction: ListDirection::Descending, + }; + + // All + None: unfiltered (shared table — containment, not count) + let ret = contacts + .list_for_filters( + ContactScope::All, + ContactFilters::default(), + sort(), + query(), + ) + .await?; + assert!( + ids_a + .iter() + .all(|id| ret.entities.iter().any(|c| c.id == *id)) + ); + + // All + Some(a) + let ret = contacts + .list_for_filters( + ContactScope::All, + ContactFilters { + partner_id: Some(partner_a), + ..Default::default() + }, + sort(), + query(), + ) + .await?; + assert_eq!(ret.entities.len(), 2); + assert!(ret.entities.iter().all(|c| c.partner_id == partner_a)); + + // Only(a) + Some(a): match — the conjunct is satisfiable, same rows + let ret = contacts + .list_for_filters( + partner_a, + ContactFilters { + partner_id: Some(partner_a), + ..Default::default() + }, + sort(), + query(), + ) + .await?; + assert_eq!(ret.entities.len(), 2); + + // Only(a) + Some(b): mismatch — contradictory conjunct, empty + let ret = contacts + .list_for_filters( + partner_a, + ContactFilters { + partner_id: Some(partner_b), + ..Default::default() + }, + sort(), + query(), + ) + .await?; + assert!(ret.entities.is_empty()); + assert!(!ret.has_next_page); + + // multi-filter through the specialized scoped arms (partner_id filter, + // status filter and the scope conjunct all present) + let ret = contacts + .list_for_filters( + partner_a, + ContactFilters { + partner_id: Some(partner_a), + status: Some("active".to_string()), + }, + sort(), + query(), + ) + .await?; + assert_eq!(ret.entities.len(), 1); + assert_eq!(ret.entities[0].status, "active"); + let ret = contacts + .list_for_filters( + partner_a, + ContactFilters { + partner_id: Some(partner_b), + status: Some("active".to_string()), + }, + sort(), + query(), + ) + .await?; + assert!(ret.entities.is_empty()); + + Ok(()) +} + +#[tokio::test] +async fn sargable_scope_column_filter_cursor_pages() -> anyhow::Result<()> { + let pool = helpers::init_pool().await?; + let contacts = Contacts::new(pool); + + let partner_a = PartnerId::new(); + let partner_b = PartnerId::new(); + let specs: Vec<(&str, &str)> = (0..5).map(|_| ("sp", "active")).collect(); + let ids_a = seed_contacts(&contacts, partner_a, &specs).await?; + seed_contacts(&contacts, partner_b, &specs).await?; + + // All + Some(a), page size 2: both the page-1 and cursor-page + // specialized variants execute; every page stays within the partner + let mut collected = Vec::new(); + let mut after = None; + let mut pages = 0; + loop { + let ret = contacts + .list_for_filters( + ContactScope::All, + ContactFilters { + partner_id: Some(partner_a), + ..Default::default() + }, + Sort { + by: ContactSortBy::CreatedAt, + direction: ListDirection::Descending, + }, + PaginatedQueryArgs { first: 2, after }, + ) + .await?; + pages += 1; + assert!(ret.entities.iter().all(|c| c.partner_id == partner_a)); + collected.extend(ret.entities.iter().map(|c| c.id)); + if !ret.has_next_page { + break; + } + after = ret.end_cursor; + } + assert!(pages >= 3, "expected pagination across pages, got {pages}"); + let expected: std::collections::HashSet<_> = ids_a.into_iter().collect(); + let collected: std::collections::HashSet<_> = collected.into_iter().collect(); + assert_eq!(collected, expected); + + Ok(()) +}