Feat/schema validators session and scopes 4#58
Merged
Conversation
…test - PRD: §0.3 follow-on table (Phase C vs E); §7.7/§9.7 pointers to test and mapping docs - DESIGN_FIND_RELATED_SCOPES: mark chained-scope example done; link to tests - SEAORM_LIFEGUARD_MAPPING + COMPARISON: scope row + repository bullet for find_related + scope - tests/db_integration/related_trait: test_find_related_chains_scope_on_related_query; module docs - Memory bank: activeContext
- lifeguard_derive::scope_bundle + lifeguard::scope_bundle re-export - Tests scope_bundle_and_chains; PRD §0.3/§7.7, SEAORM, VISION, query::scope rustdoc - Memory bank activeContext
- Default method: find_related_scoped(related_scope) equals find_related()?.scope(…) - Integration test test_find_related_scoped_matches_chained_scope; related_trait module docs - DESIGN_FIND_RELATED_SCOPES, PRD §0.3/§7.7, SEAORM, COMPARISON, query::scope rustdoc - Memory bank activeContext
- CLI flags --watch and --watch-interval-secs (default 5, min 1); print when emitted Rust changes - infer_schema_once helper; stderr line on change after first snapshot - Unit tests infer_schema_watch_cli_parses / infer_schema_defaults_no_watch - PRD §5.7/§5.7a, lifeguard-migrate README, DEVELOPMENT.md, memory bank
Reconcile live pg_indexes simple key columns against merged migration column map; report IndexColumnDrift and fail CLI on mismatch. Add smoke tests, update PRD/README/DEVELOPMENT/design doc, refresh memory bank.
Add LIFEGUARD_BLESS_INFER_SCHEMA_GOLDENS opt-in to refresh tests/golden expected files; just bless-infer-schema-goldens. Mark PRD §5.7a golden snapshot item shipped; document in README, DEVELOPMENT, and DEV_RUSTDOC.
- IndexDefinition + #[index] INCLUDE (cols); sql_generator emits PostgreSQL INCLUDE - compare-schema: parse INCLUDE from pg_indexes.indexdef; drift on key + INCLUDE names - FindRelated::find_related_parent_scoped (INNER JOIN from_tbl + IntoCondition); export join_tbl_on_expr - Integration test related_trait; PRD/COMPARISON/SEAORM/DESIGN/migrate README; memory bank
…ose PRD §5.7a/§7.7/§9.7
…ire_index_coverage (T4) When merged migration and live pg_indexes share an index name, compare normalized CREATE INDEX text and report text-only-in-DB or only-in-migration names. Reuse column-level drift only when there is no migration line for that index. Add opt-in #[require_index_coverage] compile check. Update roadmap, migrate README, and Memory Bank active context.
…are-schema tracks
Add fetch_live_btree_index_key_opclasses using pg_index/pg_opclass/opcdefault; report index_btree_nondefault_opclass_drifts for shared tables. JSONB jsonb_path_ops smoke tests; docs and PRD updated.
…class docs Use btree text_pattern_ops on text for live opclass smoke tests; jsonb_path_ops is GIN-only and fails on btree (E42704). Update schema_migration_compare unit test and planning docs; refresh Memory Bank activeContext.
Add fetch_live_btree_expression_index_key_slots (pg_index indkey=0 + pg_get_indexdef). Emit IndexExpressionKeyVsSimpleMigrationDrift and suppress T1 for those indexes. Integration smoke tests; README, roadmap, and design docs updated.
…ng drift - Catalog fetch for all btree key slots + indcollation/indoption - T3 v2: normalize per-key fragments when expr on either side; mismatch drift; skip T1 on match - T2b: expected opclass from merged SQL or type default; migration_explicit_opclass on drift - T1 suppressed for opclass-only key differences when INCLUDE/WHERE tails match - Explicit COLLATE/ASC/DESC/NULLS vs live ordering/collation drift - Smoke + unit tests; README/roadmap/design updates
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds structured index parsing/emission and index-compare/reporting, derive-time index-coverage validation and a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Infer as lifeguard-migrate<br/>infer-schema
participant Pg as PostgreSQL<br/>Catalog
participant Emitter as Rust Code<br/>Emitter
participant Golden as Golden File
User->>Infer: run infer-schema --watch
loop every watch-interval-secs
Infer->>Pg: introspect tables, columns, indexes
Pg-->>Infer: table/column/index metadata
Infer->>Infer: infer_schema_once() → generated Rust
alt output changed
Infer->>Emitter: emit new Rust (with `#[index=...]`)
alt LIFEGUARD_BLESS_INFER_SCHEMA_GOLDENS=1
Emitter->>Golden: overwrite golden(s)
end
else no change
Infer->>User: (silent)
end
Infer->>Infer: sleep(watch_interval_secs)
end
sequenceDiagram
participant Derive as LifeModel<br/>proc-macro
participant Parser as ParsedIndexSpec<br/>parser
participant Validator as require_index_coverage<br/>validator
participant Generator as Code<br/>generator
Derive->>Parser: parse `#[index = "..."]` (UNIQUE/INCLUDE/WHERE/opclass/expr|cols)
Parser-->>Derive: ParsedIndexSpec + key_parts
Derive->>Validator: validate_require_index_coverage(fields, parsed)
alt validation passes
Validator-->>Derive: OK
Derive->>Generator: generate IndexDefinition (key_parts, include_columns, key_list_sql)
else validation fails
Validator-->>Derive: emit compile error
end
sequenceDiagram
participant App
participant FindRel as FindRelated API
participant RelQ as SelectQuery (related)
participant DB as Database
App->>FindRel: call find_related::<Post>()
FindRel->>RelQ: build join + relation WHERE (no parent scope)
alt find_related_scoped
App->>FindRel: call find_related_scoped(scope)
FindRel->>RelQ: apply related scope to related query
else find_related_parent_scoped
App->>FindRel: call find_related_parent_scoped(parent_scope)
FindRel->>RelQ: SELECT related JOIN parent ON ... AND apply parent_scope (reject through_tbl)
end
RelQ->>DB: execute SELECT
DB-->>RelQ: results
RelQ-->>App: return filtered related rows
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly enhances the ORM's schema tooling and relation capabilities, introducing structured index key definitions, support for INCLUDE and partial indexes, and advanced drift detection in compare-schema. It also adds a #[scope_bundle] macro, new scoped relation methods, and a --watch mode for schema inference. Technical feedback identifies a flaw in metrics initialization that could lead to disconnected registries, along with several parsing bugs in attribute and SQL normalization logic regarding multi-byte UTF-8 handling and escaped quote sequences in identifiers.
| @@ -84,13 +86,21 @@ impl LifeguardMetrics { | |||
| /// In production, this should be handled by the application's startup error handling. | |||
| #[must_use] | |||
| pub fn init() -> Self { | |||
There was a problem hiding this comment.
The init function is public and creates a new prometheus::Registry on every call. However, the global OpenTelemetry meter provider is only initialized once (via INIT_GLOBAL_METER_PROVIDER). This means that any LifeguardMetrics instance created by a subsequent call to init will hold a registry that is disconnected from the instruments created via global::meter, rendering its registry field useless for scraping. Consider making init private and ensuring all access goes through the METRICS singleton.
| let rest = tail[i + 1..].trim_start().to_string(); | ||
| return Ok((out, rest)); | ||
| } | ||
| out.push(char::from(bytes[i])); |
There was a problem hiding this comment.
The use of char::from(bytes[i]) incorrectly assumes that each byte in the collation name is a single character. This will corrupt multi-byte UTF-8 characters. The parser should iterate over chars() instead of bytes() to correctly handle non-ASCII collation names.
| r | ||
| } | ||
|
|
||
| fn collapse_ws_outside_quotes(input: &str) -> String { |
There was a problem hiding this comment.
The manual quote parsing logic in collapse_ws_outside_quotes does not correctly handle escaped quotes (e.g., '' in SQL strings or "" in identifiers). When it encounters a doubled quote, it will incorrectly toggle the in_single or in_double state, leading to incorrect whitespace normalization for the remainder of the statement. This may cause false positives in compare-schema for statements containing such escapes.
Comment on lines
+1198
to
+1212
| if let Some(pos) = lower.find(" collate ") { | ||
| return first_simple_index_column(&seg[..pos]); | ||
| } | ||
| let seg = seg | ||
| .split_whitespace() | ||
| .next() | ||
| .unwrap_or(seg) | ||
| .trim_end_matches(','); | ||
| if seg.starts_with('(') { | ||
| return None; | ||
| } | ||
| if seg.starts_with('"') { | ||
| let rest = &seg[1..]; | ||
| let end = rest.find('"')?; | ||
| return Some(rest[..end].to_string()); |
There was a problem hiding this comment.
first_simple_index_column has two issues with identifier parsing: 1) It uses split_whitespace().next() which will fail for quoted identifiers containing spaces (e.g., "My Column"). 2) The logic for finding the end of a quoted identifier does not account for escaped double quotes (""). The parser should be aware of quoted identifier boundaries and PostgreSQL escaping rules.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } | ||
| let columns = merge_columns_from_key_parts(&key_parts); | ||
| Ok((key_parts, columns)) | ||
| } |
There was a problem hiding this comment.
Multi-column expression coverage broken by comma splitting
Medium Severity
split_index_key_segments splits on commas at paren-depth 0 before classify_index_key_list processes the | coverage syntax. The documented grammar expr | col1, col2 for multi-column expression coverage is impossible to use — the comma between col1 and col2 is at depth 0 and gets split into separate segments. For example, idx((a + b) | a, b) produces an expression with coverage ["a"] plus a spurious simple-column key b, generating incorrect SQL and wrong key_parts.
Reviewed by Cursor Bugbot for commit d40331c. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (9)
scripts/setup_kind_cluster.sh (1)
49-49: Avoid hardcoded context string in status output.Line 49 hardcodes
kind-kind; derive it fromCLUSTER_NAME(orKUBE_CONTEXT) to avoid drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup_kind_cluster.sh` at line 49, The status echo currently hardcodes "kind-kind"; change it to derive the kubectl context dynamically instead of using the literal string by using the existing variables: prefer KUBE_CONTEXT if set, otherwise construct the kind context as "kind-${CLUSTER_NAME}" (or call `kubectl config current-context` to fetch the active context) and substitute that value into the echo line so the output uses the actual context rather than "kind-kind".lifeguard-migrate/tests/test_index_key_list_sql_generation.rs (1)
9-75: Add one negative test for invalid index-column references.These positive SQL-generation tests are solid; adding a failing case (unknown/non-local column in
#[index = "..."]) would harden the validation path and prevent regression in derive/migration checks.As per coding guidelines:
Every column referenced in #[index = "idx_name(col1, col2)"] must exist as a field on that struct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-migrate/tests/test_index_key_list_sql_generation.rs` around lines 9 - 75, Add a negative test that ensures the derive/SQL generator errors when an index references a non-existent column: create a test (e.g., generate_sql_errors_on_unknown_index_column) with a struct annotated #[derive(LifeModel)] and #[index = "idx_invalid(unknown_col)"] (and real columns like id, email), call sql_generator::generate_create_table_sql::<Entity>(Entity::table_definition()) and assert it returns an Err (or panics) and that the error message mentions the unknown column; this verifies the validation path in the derive/migration logic and prevents accepting indexes that reference fields not declared on the struct.lifeguard-migrate/src/generated_migration_diff.rs (1)
243-251: Table name comparison is case-sensitive but index parsing is case-insensitive.The
tbl == tablecomparison at line 247 is case-sensitive, but PostgreSQL treats unquoted identifiers as lowercase. If the migration usesCREATE INDEX ... ON Widgets(...)andtableis"widgets", this comparison will fail.Consider using case-insensitive comparison for unquoted table names to match PostgreSQL semantics.
♻️ Proposed fix
for line in combined.lines() { let Some((idx, tbl, stmt)) = try_parse_create_index_statement(line) else { continue; }; - if tbl == table { + if tbl.eq_ignore_ascii_case(table) { map.insert(idx, stmt); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-migrate/src/generated_migration_diff.rs` around lines 243 - 251, The comparison tbl == table in the loop is case-sensitive while try_parse_create_index_statement returns table identifiers parsed case-insensitively (Postgres treats unquoted identifiers as lowercase); update the comparison to normalize both sides (e.g., compare tbl.to_lowercase() == table.to_lowercase() or use a case-insensitive/equivalent_unquoted_ident comparison) before inserting into map so identifiers like "Widgets" and "widgets" match; locate this logic around the loop that calls try_parse_create_index_statement and adjust the comparison of tbl and table accordingly.lifeguard-derive/src/macros/scope_bundle.rs (1)
90-93: Function signature is completely rewritten.The macro replaces the original function's generics, return type, and body entirely. The original function body is discarded, which may surprise users who write logic in it. Consider adding a note to the macro's documentation that the annotated function's body is ignored.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-derive/src/macros/scope_bundle.rs` around lines 90 - 93, The macro currently overwrites the annotated function's signature and body (see func.sig.ident, func.sig.generics, func.sig.output, func.block in scope_bundle.rs), which discards any user-provided generics, return type, and body; update the macro's documentation to clearly state that the annotated function's body and signature (generics and return type) are ignored and replaced by the macro, and mention which symbols are replaced (func.sig.ident, func.sig.generics, func.sig.output, func.block) so users know to not rely on the original implementation when using the attribute.lifeguard-derive/src/macros/life_model.rs (1)
330-445: Consider extracting duplicated sort/nulls mapping logic.The
sortandnullsfield mapping logic (lines 355-372 and 413-430) is nearly identical between theColumnandExpressionbranches. While acceptable in macro code where explicitness aids debugging, extracting a helper could reduce maintenance burden if additional sort/null variants are added later.♻️ Optional helper extraction
fn sort_to_tokens(sort: &Option<ParsedBtreeSort>, span: proc_macro2::Span) -> proc_macro2::TokenStream { match sort { None => quote! { None }, Some(ParsedBtreeSort::Asc) => quote! { Some(lifeguard::IndexBtreeSort::Asc) }, Some(ParsedBtreeSort::Desc) => quote! { Some(lifeguard::IndexBtreeSort::Desc) }, } } fn nulls_to_tokens(nulls: &Option<ParsedBtreeNulls>, span: proc_macro2::Span) -> proc_macro2::TokenStream { match nulls { None => quote! { None }, Some(ParsedBtreeNulls::First) => quote! { Some(lifeguard::IndexBtreeNulls::First) }, Some(ParsedBtreeNulls::Last) => quote! { Some(lifeguard::IndexBtreeNulls::Last) }, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-derive/src/macros/life_model.rs` around lines 330 - 445, The duplicate mapping of sort and nulls in the Column and Expression arms (converting Option<ParsedBtreeSort> and Option<ParsedBtreeNulls> into quote! token streams) should be extracted into small helpers (e.g. sort_to_tokens and nulls_to_tokens) and called from both branches; update the closures that produce sort_ts and nulls_ts to call these helpers (pass the Option reference and struct_name.span() or a span) and return the resulting proc_macro2::TokenStream, keeping the rest of the quote! generation unchanged and referencing ParsedBtreeSort and ParsedBtreeNulls where needed.lifeguard-derive/src/attributes.rs (1)
567-585: Keep these new validation errors on named formatting.The added
format!calls use positional placeholders again. Please switch them to named captures to match the repo style rule.As per coding guidelines
Use named format string syntax instead of positional arguments in format! and write! macros.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-derive/src/attributes.rs` around lines 567 - 585, The new validation errors in the index column checks use positional format! placeholders; update both format! calls in the loops that validate index_def.columns and index_def.include_columns to use named format string syntax (e.g., "{col}", "{name}", "{available}") and pass the corresponding named arguments (col = col, name = index_def.name, available = valid_columns.iter().map(String::as_str).collect::<Vec<_>>().join(", ")); locate the code around the for loops referencing index_def, valid_columns, and attr to make the changes so the error messages follow the repo's named-format convention.lifeguard-migrate/src/sql_generator.rs (1)
305-316: Use named captures in the newwriteln!/format!calls.These additions reintroduce positional placeholders, which makes later edits easier to mis-wire.
As per coding guidelines `Use named format string syntax instead of positional arguments in format! and write! macros`.Possible cleanup
writeln!( sql, - "ALTER TABLE {} DROP CONSTRAINT IF EXISTS {};", - full_table_name, cname + "ALTER TABLE {full_table_name} DROP CONSTRAINT IF EXISTS {cname};" ) - .map_err(|e| format!("Failed to write SQL: {}", e))?; + .map_err(|e| format!("Failed to write SQL: {e}"))?; writeln!( sql, - "ALTER TABLE {} ADD CONSTRAINT {} CHECK ({});", - full_table_name, cname, check_expr + "ALTER TABLE {full_table_name} ADD CONSTRAINT {cname} CHECK ({check_expr});" ) - .map_err(|e| format!("Failed to write SQL: {}", e))?; + .map_err(|e| format!("Failed to write SQL: {e}"))?; @@ writeln!( sql, - "CREATE INDEX IF NOT EXISTS {} ON {}({});", - index_name, full_table_name, col_name + "CREATE INDEX IF NOT EXISTS {index_name} ON {full_table_name}({col_name});" ) - .map_err(|e| format!("Failed to write SQL: {}", e))?; + .map_err(|e| format!("Failed to write SQL: {e}"))?;Also applies to: 326-331
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-migrate/src/sql_generator.rs` around lines 305 - 316, The new writeln! / format! calls use positional placeholders; update them to use named captures so maintenance is safer: change the two writeln! invocations that reference full_table_name, cname, and check_expr (the ALTER TABLE ... DROP CONSTRAINT and ALTER TABLE ... ADD CONSTRAINT ... CHECK lines) to use named format arguments (e.g. "{table}", "{constraint}", "{expr}") and pass named parameters (table = full_table_name, constraint = cname, expr = check_expr) instead of relying on positional ordering; apply the same change to the similar block around the other occurrences noted (lines referencing full_table_name, cname in the later section).src/query/table/definition.rs (1)
188-196: Use named placeholders in this helper.
format!("{}({inner})", idx.name)is the remaining positional-format call in this path.format!("{name}({inner})", name = idx.name)keeps it aligned with the repo convention. As per coding guidelines, "Use named format string syntax instead of positional arguments in format! and write! macros"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/query/table/definition.rs` around lines 188 - 196, In the function index_definition_to_derive_index_value, replace the positional format call format!("{}({inner})", idx.name) with a named-placeholder format call to follow repo convention; specifically use format!("{name}({inner})", name = idx.name) so the index name uses a named parameter (refer to symbol idx.name and the function index_definition_to_derive_index_value to locate the change).lifeguard-migrate/src/schema_migration_compare.rs (1)
1008-1029: Prefer the standard string helpers in this parser block.Most of these branches pair a prefix check with manual byte slicing, and
strip_using_btree_after_on_tablestill carries a positionalformat!. Refactoring this block tostrip_prefix/strip_suffixand named placeholders would make it much easier to audit and align it with the repo’s Rust conventions. As per coding guidelines, "Use strip_prefix() and strip_suffix() methods instead of manual string slicing with starts_with() checks" and "Use named format string syntax instead of positional arguments in format! and write! macros"Also applies to: 1064-1086, 1112-1465
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-migrate/src/schema_migration_compare.rs` around lines 1008 - 1029, The parser functions (e.g., skip_using_method and strip_create_index_prefix_options, and the referenced strip_using_btree_after_on_table) use manual byte-slicing and positional format! which is error-prone; replace those manual checks/slices with str::strip_prefix and strip_suffix calls (trim_start only where needed) to obtain the remaining slice safely, and convert any positional format! / write! usages to named placeholders (e.g., format!("{name}", name=val)) so the code follows the project's string-handling conventions and avoids index-based slicing bugs; update all similar blocks in the specified ranges (including 1064-1086 and 1112-1465) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/planning/DESIGN_INDEX_COMPARE_ROADMAP.md`:
- Around line 3-44: The document contains broken relative links to
lifeguard-migrate (uses "../lifeguard-migrate/..." which resolves to
docs/lifeguard-migrate); update every occurrence to use
"../../lifeguard-migrate/..." so links to symbols like
fetch_live_btree_index_key_opclasses,
fetch_live_btree_expression_index_key_slots,
fetch_live_btree_index_key_catalog_slots, normalize_index_statement_for_compare,
index_statements_for_table_from_merged_baseline,
parse_pg_indexdef_access_method, normalize_index_key_slot_for_compare, and the
lifeguard-migrate README resolve correctly; search the file for
"../lifeguard-migrate/" and replace with "../../lifeguard-migrate/" for each
link.
In `@docs/planning/PRD_FOLLOWON_NEXT_THREE.md`:
- Around line 9-11: The three internal links use fragment anchors that don't
match the generated heading IDs due to special characters; update either the
target headings to include explicit anchor IDs that match the link fragments
(e.g., add {`#1-phase-e--g6-comparison--seaorm-mapping-hygiene`} to the "Phase E —
mapping / docs" heading) or change the link fragments to the renderer's
slugified forms (removing or replacing `/`, `—`, `+`, backticks) so they match
the actual generated IDs for the headings referenced in the diff.
In `@docs/UUID_AND_POSTGRES_TYPES.md`:
- Around line 85-86: Fix the malformed emphasis/backtick around TIMESTAMP WITH
TIME ZONE by removing the stray asterisks and using consistent inline
code/backtick markup (e.g., `TIMESTAMP WITH TIME ZONE`), and ensure the
references to postgres-types / tokio-postgres are consistently backticked (e.g.,
`postgres-types` / `tokio-postgres`) so the markdown lint no longer flags
mismatched emphasis in the paragraph mentioning TIMESTAMP WITH TIME ZONE and
Rust types like chrono::DateTime<Utc> or NaiveDateTime.
- Line 82: Update the broken relative markdown link that currently uses
"../planning/..." to use a document-local path "./planning/..." so the link
resolves correctly from this docs file; locate the markdown link string
containing "../planning/" and replace it with "./planning/" (ensure any
surrounding parentheses and link text remain unchanged).
In `@lifeguard-derive/README.md`:
- Around line 13-27: The README shows backticks around bold markers so asterisks
render literally; remove the inline backticks around the bolded items (e.g.
change "`**LifeModel**`" to "**LifeModel**", likewise for "**LifeRecord**",
"**DeriveEntity**", "**FromRow**", "**DeriveRelation**", "**DeriveLinked**") and
keep type/code spans for actual Rust types (use `uuid::Uuid`,
`Option<uuid::Uuid>`, and `String` in backticks) so emphasis and code formatting
render correctly; ensure the UUID guidance line uses bold for section text and
backticks for code examples consistently.
In `@lifeguard-derive/src/attributes.rs`:
- Around line 1123-1178: The parser currently accepts empty index names, empty
parenthesized key lists, and empty INCLUDE lists; update the parsing in the
block that computes (key_part, include_columns) and the subsequent parentheses
branch to fail early with syn::Error::new(Span::call_site(), ...) for these
cases: when the extracted name (variable name) is empty after trimming, when
key_part contains "()" or classify_index_key_list(columns_str)? returns an empty
columns/key_parts (reject and return an error), and when parsing INCLUDE yields
an empty inc Vec (reject instead of allowing include_columns = Vec::new()).
Reference symbols: main_part, key_part, include_idx, inner/inc parsing,
classify_index_key_list, and ParsedIndexSpec; add explicit checks immediately
after parsing those values and return a syn::Error describing the specific
problem (empty index name, empty key list, or empty INCLUDE list).
In `@lifeguard-migrate/src/generated_migration_diff.rs`:
- Around line 153-155: split_qualified_table_prefix miscalculates the advance
for a quoted identifier: when rest.starts_with('"') it uses i += 1 + end + 2
which over-advances and breaks parsing of schema-qualified names; change the
increment to i += end + 2 so the pointer advances by the exact quoted length
(both quotes plus inner chars). Update the increment in
split_qualified_table_prefix (and ensure any related logic in
try_parse_create_index_statement that relies on that prefix parsing still
behaves correctly).
In `@lifeguard-migrate/src/schema_infer.rs`:
- Around line 171-180: indexes_by_table is iterated to emit #[index = "..."]
attributes but those indexes are computed before unsupported columns are
dropped, so an emitted index may reference fields that were removed; before
calling index_definition_to_derive_index_value and writing the attribute, build
the set of actually emitted field names for this table (the same emitted-field
set used when pruning columns) and verify that every column referenced by
idx.columns and any INCLUDE columns exists in that set; if all referenced
columns exist, emit the attribute as now, otherwise skip emitting (or write it
commented out) so the generated struct only references fields that will actually
be present.
- Around line 334-370: The query is scanning all entries in ix.indkey including
INCLUDE columns, causing duplicates; restrict the lateral unnest by adding "AND
u.key_ord <= ix.indnkeyatts" to the WHERE clause so only true key columns are
returned (update the SQL WHERE that currently has "WHERE NOT ix.indisprimary AND
ix.indisvalid AND t.relkind = 'r'") and re-run parse_indexdef_include_where() to
ensure INCLUDE columns are only produced from indnkeyatts onward; reference
u.key_ord and ix.indnkeyatts when making the change.
In `@lifeguard-migrate/src/schema_migration_compare.rs`:
- Around line 658-690: fetch_live_pg_indexes currently filters primary keys by
index name suffix; change it to query pg_index (join pg_class/pg_namespace as
needed) and select index metadata using pg_get_indexdef, applying the filters
NOT xi.indisprimary and xi.indisvalid instead of the
index_name.ends_with("_pkey") check. Update the SQL in fetch_live_pg_indexes to
use a join on pg_index (alias xi) and schematic name parameter, remove the
post-row-name suffix check, and keep mapping to LiveIndexRow (table_name,
index_name, indexdef) while preserving existing error-wrapping on row.try_get
failures.
- Around line 602-610: The comparison is false-positive because qualified names
like `pg_catalog.text_pattern_ops` and quoted names differ from unqualified live
names; add a small normalizer (e.g., normalize_catalog_identifier(name: &str) ->
String) that strips a leading "schema." prefix if present and removes
surrounding double quotes (preserving case where appropriate), and use it before
comparing opclass and collation names: call it on `t.explicit_opclass`,
`o.default_opclass_name`, and `o.opclass_name` in the opclass comparison (where
`emit_opclass_drift` is set) and apply the same normalizer at the other reported
site (lines ~1619-1625) where collation comparisons occur. Ensure you normalize
both sides (expected and live) before the equality check rather than only one
side.
In `@lifeguard-migrate/src/sql_generator.rs`:
- Around line 359-369: The helper index_covers_only_column is incorrectly
treating expression/predicate or modified indexes as equivalent to a plain
single-column index; update index_covers_only_column (and its use of
index_key_parts_coverage_columns) to only return true when the index is an
unmodified plain single-column index: verify lifeguard::IndexDefinition has
exactly one coverage column equal to col_name AND that there are no key_parts
expressions, predicates, collations, or other modifiers present (i.e., require
index.key_parts to be empty and index.columns to contain a single plain column
with no modifiers), so expression-based or predicate-bearing indexes (e.g.,
lower(email) | email) do not suppress generation of a separate simple CREATE
INDEX on the column.
In `@scripts/setup_kind_cluster.sh`:
- Around line 29-35: The script may run subsequent kubectl commands against the
wrong cluster because it skips creation when a cluster exists but never sets the
kubectl context; immediately after the cluster-create check (the block using
CLUSTER_NAME and kind create cluster), explicitly set the kubectl context to the
Kind cluster by running kubectl config use-context "kind-${CLUSTER_NAME}" (or
detect the correct context name if different) and fail with a clear error if
that context doesn't exist, so all later kubectl commands target the intended
Kind cluster.
In `@src/query/table/definition.rs`:
- Around line 55-76: Change the positional format call format!("{}({inner})",
idx.name) to a named placeholder format!("{name}({inner})", name = idx.name) in
the index name rendering, and update format_index_key_list_sql and
push_index_key_suffixes to always quote SQL identifiers (column names and
operator classes) when they are not “simple” identifiers using the same logic
already used for COLLATE: detect whitespace/quotes or !is_simple_sql_ident(),
wrap in double quotes and escape internal quotes by doubling them; specifically
modify push_index_key_suffixes to quote opclass when needed (replace the
unconditional out.push_str(o)) and modify format_index_key_list_sql to emit
quoted column identifiers when necessary so reserved keywords and mixed-case
names are handled consistently.
In `@src/relation/traits.rs`:
- Around line 536-545: The code rebuilds the join using join_tbl_on_expr which
discards RelationDef::on_condition and condition_type and fails to handle
self-joins; change the join to use rel_def.join_on_expr() (preserving
on_condition and condition_type) when constructing the join for
find_related_parent_scoped, and if rel_def refers to the same table as
Self::Entity::default() either apply a proper aliasing strategy on the joined
entity or explicitly return an error/reject this path; ensure you still apply
the existing filters (build_where_condition(&rel_def, self) and
parent_scope.into_condition()) after reusing rel_def.join_on_expr().
---
Nitpick comments:
In `@lifeguard-derive/src/attributes.rs`:
- Around line 567-585: The new validation errors in the index column checks use
positional format! placeholders; update both format! calls in the loops that
validate index_def.columns and index_def.include_columns to use named format
string syntax (e.g., "{col}", "{name}", "{available}") and pass the
corresponding named arguments (col = col, name = index_def.name, available =
valid_columns.iter().map(String::as_str).collect::<Vec<_>>().join(", ")); locate
the code around the for loops referencing index_def, valid_columns, and attr to
make the changes so the error messages follow the repo's named-format
convention.
In `@lifeguard-derive/src/macros/life_model.rs`:
- Around line 330-445: The duplicate mapping of sort and nulls in the Column and
Expression arms (converting Option<ParsedBtreeSort> and Option<ParsedBtreeNulls>
into quote! token streams) should be extracted into small helpers (e.g.
sort_to_tokens and nulls_to_tokens) and called from both branches; update the
closures that produce sort_ts and nulls_ts to call these helpers (pass the
Option reference and struct_name.span() or a span) and return the resulting
proc_macro2::TokenStream, keeping the rest of the quote! generation unchanged
and referencing ParsedBtreeSort and ParsedBtreeNulls where needed.
In `@lifeguard-derive/src/macros/scope_bundle.rs`:
- Around line 90-93: The macro currently overwrites the annotated function's
signature and body (see func.sig.ident, func.sig.generics, func.sig.output,
func.block in scope_bundle.rs), which discards any user-provided generics,
return type, and body; update the macro's documentation to clearly state that
the annotated function's body and signature (generics and return type) are
ignored and replaced by the macro, and mention which symbols are replaced
(func.sig.ident, func.sig.generics, func.sig.output, func.block) so users know
to not rely on the original implementation when using the attribute.
In `@lifeguard-migrate/src/generated_migration_diff.rs`:
- Around line 243-251: The comparison tbl == table in the loop is case-sensitive
while try_parse_create_index_statement returns table identifiers parsed
case-insensitively (Postgres treats unquoted identifiers as lowercase); update
the comparison to normalize both sides (e.g., compare tbl.to_lowercase() ==
table.to_lowercase() or use a case-insensitive/equivalent_unquoted_ident
comparison) before inserting into map so identifiers like "Widgets" and
"widgets" match; locate this logic around the loop that calls
try_parse_create_index_statement and adjust the comparison of tbl and table
accordingly.
In `@lifeguard-migrate/src/schema_migration_compare.rs`:
- Around line 1008-1029: The parser functions (e.g., skip_using_method and
strip_create_index_prefix_options, and the referenced
strip_using_btree_after_on_table) use manual byte-slicing and positional format!
which is error-prone; replace those manual checks/slices with str::strip_prefix
and strip_suffix calls (trim_start only where needed) to obtain the remaining
slice safely, and convert any positional format! / write! usages to named
placeholders (e.g., format!("{name}", name=val)) so the code follows the
project's string-handling conventions and avoids index-based slicing bugs;
update all similar blocks in the specified ranges (including 1064-1086 and
1112-1465) accordingly.
In `@lifeguard-migrate/src/sql_generator.rs`:
- Around line 305-316: The new writeln! / format! calls use positional
placeholders; update them to use named captures so maintenance is safer: change
the two writeln! invocations that reference full_table_name, cname, and
check_expr (the ALTER TABLE ... DROP CONSTRAINT and ALTER TABLE ... ADD
CONSTRAINT ... CHECK lines) to use named format arguments (e.g. "{table}",
"{constraint}", "{expr}") and pass named parameters (table = full_table_name,
constraint = cname, expr = check_expr) instead of relying on positional
ordering; apply the same change to the similar block around the other
occurrences noted (lines referencing full_table_name, cname in the later
section).
In `@lifeguard-migrate/tests/test_index_key_list_sql_generation.rs`:
- Around line 9-75: Add a negative test that ensures the derive/SQL generator
errors when an index references a non-existent column: create a test (e.g.,
generate_sql_errors_on_unknown_index_column) with a struct annotated
#[derive(LifeModel)] and #[index = "idx_invalid(unknown_col)"] (and real columns
like id, email), call
sql_generator::generate_create_table_sql::<Entity>(Entity::table_definition())
and assert it returns an Err (or panics) and that the error message mentions the
unknown column; this verifies the validation path in the derive/migration logic
and prevents accepting indexes that reference fields not declared on the struct.
In `@scripts/setup_kind_cluster.sh`:
- Line 49: The status echo currently hardcodes "kind-kind"; change it to derive
the kubectl context dynamically instead of using the literal string by using the
existing variables: prefer KUBE_CONTEXT if set, otherwise construct the kind
context as "kind-${CLUSTER_NAME}" (or call `kubectl config current-context` to
fetch the active context) and substitute that value into the echo line so the
output uses the actual context rather than "kind-kind".
In `@src/query/table/definition.rs`:
- Around line 188-196: In the function index_definition_to_derive_index_value,
replace the positional format call format!("{}({inner})", idx.name) with a
named-placeholder format call to follow repo convention; specifically use
format!("{name}({inner})", name = idx.name) so the index name uses a named
parameter (refer to symbol idx.name and the function
index_definition_to_derive_index_value to locate the change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26c06e9b-bcf6-402a-913d-84efa99939eb
⛔ Files ignored due to path filters (2)
.agent/session.logis excluded by!**/*.logCargo.lockis excluded by!**/*.lock
📒 Files selected for processing (54)
.agent/memory-bank/activeContext.md.agent/memory-bank/progress.mdCOMPARISON.mdCargo.tomlDEVELOPMENT.mdREADME.mdROADMAP.mdTiltfileVISION.mddocs/UUID_AND_POSTGRES_TYPES.mddocs/planning/DESIGN_FIND_RELATED_SCOPES.mddocs/planning/DESIGN_INDEX_COMPARE_ROADMAP.mddocs/planning/DESIGN_INDEX_COMPARE_T2B_T3.mddocs/planning/DESIGN_INHERITED_PARENT_SCOPES_SPIKE.mddocs/planning/DESIGN_SCHEMA_INFERENCE_CLI_CODEGEN.mddocs/planning/DEV_RUSTDOC_AND_COVERAGE.mddocs/planning/PRD_FOLLOWON_NEXT_THREE.mddocs/planning/PRD_SCHEMA_VALIDATORS_SESSION_AND_SCOPES.mddocs/planning/README.mddocs/planning/lifeguard-derive/SEAORM_LIFEGUARD_MAPPING.mdexamples/find_related_scope_example.rsjustfilekind-config.yamlkind-config.yamllifeguard-derive/README.mdlifeguard-derive/src/attributes.rslifeguard-derive/src/lib.rslifeguard-derive/src/macros/life_model.rslifeguard-derive/src/macros/mod.rslifeguard-derive/src/macros/scope_bundle.rslifeguard-derive/tests/test_index_validation.rslifeguard-derive/tests/test_minimal.rslifeguard-derive/tests/ui.rslifeguard-derive/tests/ui/compile_error_index_expression_requires_coverage_columns.rslifeguard-derive/tests/ui/compile_error_require_index_coverage.rslifeguard-derive/tests/ui/compile_error_require_index_coverage.stderrlifeguard-migrate/README.mdlifeguard-migrate/src/generated_migration_diff.rslifeguard-migrate/src/main.rslifeguard-migrate/src/schema_infer.rslifeguard-migrate/src/schema_migration_compare.rslifeguard-migrate/src/sql_generator.rslifeguard-migrate/tests/migration_db_compare_smoke.rslifeguard-migrate/tests/test_index_key_list_sql_generation.rsscripts/setup_kind_cluster.shsrc/lib.rssrc/metrics.rssrc/query/mod.rssrc/query/scope.rssrc/query/table/definition.rssrc/query/table/mod.rssrc/relation/mod.rssrc/relation/traits.rstests/db_integration/related_trait.rs
Comment on lines
+3
to
+44
| **Status:** **T1**, **T2**, **T4** shipped; **T2b** **partial** (**live** opclass vs **merged explicit or type default**; [`fetch_live_btree_index_key_opclasses`](../lifeguard-migrate/src/schema_migration_compare.rs)). **T3** **partial:** v1 [`fetch_live_btree_expression_index_key_slots`](../lifeguard-migrate/src/schema_migration_compare.rs); v2 normalized slot compare + **T1** opclass-only dedupe + explicit **ordering/collation** vs `pg_index` ([`fetch_live_btree_index_key_catalog_slots`](../lifeguard-migrate/src/schema_migration_compare.rs)). **T3** derive / expression IR still optional; see [ROADMAP.md](../../ROADMAP.md). Complements [`lifeguard-migrate/README.md`](../lifeguard-migrate/README.md). | ||
|
|
||
| **PRD:** [§5.7a](./PRD_SCHEMA_VALIDATORS_SESSION_AND_SCOPES.md#57a-deferred-phase-a-stretch--end-of-backlog) — access-method drift, **T1**-style text compare, and **T4** derive coverage are implemented; **T2b** / **T3** extensions (expected opclass from migration, normalized expression-on-both-sides) remain backlog if product needs them. | ||
|
|
||
| --- | ||
|
|
||
| ## Shipped today (baseline) | ||
|
|
||
| - **Table / column** name reconciliation vs merged `*_generated_from_entities.sql`. | ||
| - **Index (name-level):** Parsed **btree key** + **`INCLUDE`** **column names** from `pg_indexes.indexdef` vs merged migration column map **when there is no `CREATE INDEX` line for that index name** in the merged baseline (expression indexes skipped when unparseable; PK indexes skipped per policy). | ||
| - **T1:** [`normalize_index_statement_for_compare`](../lifeguard-migrate/src/schema_migration_compare.rs) + [`index_statements_for_table_from_merged_baseline`](../lifeguard-migrate/src/generated_migration_diff.rs): for shared tables, when the same **index name** appears in merged migration SQL and in `pg_indexes`, normalized statements are compared; mismatch → [`IndexDefinitionTextDrift`](../lifeguard-migrate/src/schema_migration_compare.rs). Index names only in live DB or only in merged SQL → `IndexOnlyInDatabaseDrift` / `IndexOnlyInMigrationDrift` (with dedupe vs access-method / unknown-column drifts as implemented in `compare_generated_dir_to_live_db`). | ||
| - **T2 (partial):** [`parse_pg_indexdef_access_method`](../lifeguard-migrate/src/schema_migration_compare.rs) + [`IndexAccessMethodDrift`](../lifeguard-migrate/src/schema_migration_compare.rs): live indexes whose access method is not **`btree`** are reported. | ||
| - **T2b (partial):** [`fetch_live_btree_index_key_opclasses`](../lifeguard-migrate/src/schema_migration_compare.rs) + [`IndexBtreeNonDefaultOpclassDrift`](../lifeguard-migrate/src/schema_migration_compare.rs): btree key slots whose opclass is not the column type’s default (`pg_opclass.opcdefault`). Skips expression keys; does not yet assert **expected** opclass from merged migration text or entity types. | ||
| - **T3 (partial):** [`fetch_live_btree_expression_index_key_slots`](../lifeguard-migrate/src/schema_migration_compare.rs) + [`IndexExpressionKeyVsSimpleMigrationDrift`](../lifeguard-migrate/src/schema_migration_compare.rs): when merged migration `CREATE INDEX` parses as **simple** key columns only but live **`pg_index.indkey`** has an **expression** slot, structured drift; **T1** omitted for that index. | ||
| - **T4:** `#[require_index_coverage]` on the struct (see `lifeguard-derive` / `attributes.rs`): compile-time check that every DB column is covered by **`#[primary_key]`**, **`#[indexed]`**, a table **`#[index = "..."]`** key or INCLUDE list, or **`#[composite_unique = "..."]`**. | ||
|
|
||
| --- | ||
|
|
||
| ## Deferred tracks (independent increments) | ||
|
|
||
| | Track | Goal | Rough approach | Risk | | ||
| |-------|------|----------------|------| | ||
| | **~~T1 — Full `indexdef` text~~** | **Shipped (partial):** normalized string compare when index names align; optional `IF NOT EXISTS` / `CONCURRENTLY` / explicit **`USING btree`** stripped | [`normalize_index_statement_for_compare`](../lifeguard-migrate/src/schema_migration_compare.rs) | False positives on PG version formatting, quoting, or `WHERE` / opclass details still possible | | ||
| | **~~T2 — Access method (non-btree)~~** | **Shipped:** non-`btree` `USING` → drift | `parse_pg_indexdef_access_method` vs implicit btree | — | | ||
| | **T2b — Btree opclass tokens** | **Partial:** expected = merged explicit opclass token or type default; live mismatch → drift; **T1** suppressed for opclass-only key differences | [`fetch_live_btree_index_key_opclasses`](../lifeguard-migrate/src/schema_migration_compare.rs) + merged key parse | **Follow-on:** entity-type opclass without hand-written SQL | | ||
| | **T3 — Expression / functional keys** | **Partial:** v1 simple-vs-expression; v2 normalized slot compare when either side has expressions; explicit **COLLATE** / **ASC/DESC** / **NULLS** vs catalog | [`fetch_live_btree_index_key_catalog_slots`](../lifeguard-migrate/src/schema_migration_compare.rs), [`normalize_index_key_slot_for_compare`](../lifeguard-migrate/src/schema_migration_compare.rs) | **Follow-on:** `IndexDefinition` / derive; cast noise in expressions | | ||
| | **~~T4 — Derive-time field ↔ index~~** | **Shipped (opt-in):** `#[require_index_coverage]` | `validate_require_index_coverage` in `lifeguard-derive` | Ergonomics vs noise — attribute is optional | | ||
|
|
||
| --- | ||
|
|
||
| ## Suggested priority (product-neutral) | ||
|
|
||
| 1. **T2b** if **btree opclass** drift (same access method, different operator class) matters for your deployments. | ||
| 2. **T3** when expression indexes are common in target deployments (**v1** shipped for live-expression vs migration-simple; **v2**+ still optional). | ||
|
|
||
| --- | ||
|
|
||
| ## References | ||
|
|
||
| - **`T2b` / `T3` (detailed design):** [DESIGN_INDEX_COMPARE_T2B_T3.md](./DESIGN_INDEX_COMPARE_T2B_T3.md) — btree opclass parity and expression / functional index keys (catalog vs string parsing, drift taxonomy, phases, open questions). | ||
| - `lifeguard_migrate::schema_migration_compare` | ||
| - `lifeguard_migrate::sql_generator` / `IndexDefinition` |
There was a problem hiding this comment.
Fix broken relative links to lifeguard-migrate paths.
From docs/planning/, ../lifeguard-migrate/... points to docs/lifeguard-migrate/..., which is likely invalid. Use ../../lifeguard-migrate/... instead.
🔧 Suggested link-path fix
-... ([`fetch_live_btree_index_key_opclasses`](../lifeguard-migrate/src/schema_migration_compare.rs)) ...
+... ([`fetch_live_btree_index_key_opclasses`](../../lifeguard-migrate/src/schema_migration_compare.rs)) ...
-... ([`fetch_live_btree_index_key_catalog_slots`](../lifeguard-migrate/src/schema_migration_compare.rs)) ...
+... ([`fetch_live_btree_index_key_catalog_slots`](../../lifeguard-migrate/src/schema_migration_compare.rs)) ...
-... Complements [`lifeguard-migrate/README.md`](../lifeguard-migrate/README.md).
+... Complements [`lifeguard-migrate/README.md`](../../lifeguard-migrate/README.md).
-- **T1:** [`normalize_index_statement_for_compare`](../lifeguard-migrate/src/schema_migration_compare.rs) + [`index_statements_for_table_from_merged_baseline`](../lifeguard-migrate/src/generated_migration_diff.rs)...
+- **T1:** [`normalize_index_statement_for_compare`](../../lifeguard-migrate/src/schema_migration_compare.rs) + [`index_statements_for_table_from_merged_baseline`](../../lifeguard-migrate/src/generated_migration_diff.rs)...
-- **T2 (partial):** [`parse_pg_indexdef_access_method`](../lifeguard-migrate/src/schema_migration_compare.rs) + [`IndexAccessMethodDrift`](../lifeguard-migrate/src/schema_migration_compare.rs)...
+- **T2 (partial):** [`parse_pg_indexdef_access_method`](../../lifeguard-migrate/src/schema_migration_compare.rs) + [`IndexAccessMethodDrift`](../../lifeguard-migrate/src/schema_migration_compare.rs)...
-- **T2b (partial):** [`fetch_live_btree_index_key_opclasses`](../lifeguard-migrate/src/schema_migration_compare.rs) + [`IndexBtreeNonDefaultOpclassDrift`](../lifeguard-migrate/src/schema_migration_compare.rs)...
+- **T2b (partial):** [`fetch_live_btree_index_key_opclasses`](../../lifeguard-migrate/src/schema_migration_compare.rs) + [`IndexBtreeNonDefaultOpclassDrift`](../../lifeguard-migrate/src/schema_migration_compare.rs)...
-- **T3 (partial):** [`fetch_live_btree_expression_index_key_slots`](../lifeguard-migrate/src/schema_migration_compare.rs) + [`IndexExpressionKeyVsSimpleMigrationDrift`](../lifeguard-migrate/src/schema_migration_compare.rs)...
+- **T3 (partial):** [`fetch_live_btree_expression_index_key_slots`](../../lifeguard-migrate/src/schema_migration_compare.rs) + [`IndexExpressionKeyVsSimpleMigrationDrift`](../../lifeguard-migrate/src/schema_migration_compare.rs)...
-| **~~T1 — Full `indexdef` text~~** | ... | [`normalize_index_statement_for_compare`](../lifeguard-migrate/src/schema_migration_compare.rs) | ...
+| **~~T1 — Full `indexdef` text~~** | ... | [`normalize_index_statement_for_compare`](../../lifeguard-migrate/src/schema_migration_compare.rs) | ...
-| **T2b — Btree opclass tokens** | ... | [`fetch_live_btree_index_key_opclasses`](../lifeguard-migrate/src/schema_migration_compare.rs) + merged key parse | ...
+| **T2b — Btree opclass tokens** | ... | [`fetch_live_btree_index_key_opclasses`](../../lifeguard-migrate/src/schema_migration_compare.rs) + merged key parse | ...
-| **T3 — Expression / functional keys** | ... | [`fetch_live_btree_index_key_catalog_slots`](../lifeguard-migrate/src/schema_migration_compare.rs), [`normalize_index_key_slot_for_compare`](../lifeguard-migrate/src/schema_migration_compare.rs) | ...
+| **T3 — Expression / functional keys** | ... | [`fetch_live_btree_index_key_catalog_slots`](../../lifeguard-migrate/src/schema_migration_compare.rs), [`normalize_index_key_slot_for_compare`](../../lifeguard-migrate/src/schema_migration_compare.rs) | ...🧰 Tools
🪛 LanguageTool
[style] ~35-~35: To elevate your writing, try using an alternative expression here.
Context: ...ccess method, different operator class) matters for your deployments. 2. T3 when ex...
(MATTERS_RELEVANT)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/planning/DESIGN_INDEX_COMPARE_ROADMAP.md` around lines 3 - 44, The
document contains broken relative links to lifeguard-migrate (uses
"../lifeguard-migrate/..." which resolves to docs/lifeguard-migrate); update
every occurrence to use "../../lifeguard-migrate/..." so links to symbols like
fetch_live_btree_index_key_opclasses,
fetch_live_btree_expression_index_key_slots,
fetch_live_btree_index_key_catalog_slots, normalize_index_statement_for_compare,
index_statements_for_table_from_merged_baseline,
parse_pg_indexdef_access_method, normalize_index_key_slot_for_compare, and the
lifeguard-migrate README resolve correctly; search the file for
"../lifeguard-migrate/" and replace with "../../lifeguard-migrate/" for each
link.
Comment on lines
+9
to
+11
| | 1 | Phase E — mapping / docs | [§1](#1-phase-e--g6-comparison--seaorm-mapping-hygiene) | | ||
| | 2 | Phase C — examples (`find_related` + `.scope` / `.filter`) | [§2](#2-phase-c--find_related--related-side-scope-example-surface) | | ||
| | 4 | Phase C — inherited parent scopes + loaders (future) | [§3](#3-phase-c--inherited-parent-selectquery--loader-merge-future) | |
There was a problem hiding this comment.
Fix invalid link fragment anchors.
The static analysis tool flagged these internal links as having invalid fragments. The anchor slugs likely differ from the heading text due to special characters (/, —, +, backticks). Verify anchors match the generated heading IDs or use explicit anchor syntax.
🔧 Suggested fix
Consider adding explicit anchor IDs to the headings, or verify the correct slug format for your markdown renderer. For example:
-| 1 | Phase E — mapping / docs | [§1](`#1-phase-e--g6-comparison--seaorm-mapping-hygiene`) |
+| 1 | Phase E — mapping / docs | [§1](`#1-phase-e--g6--comparison--seaorm-mapping-hygiene`) |Or add explicit anchors to headings:
## 1. Phase E / G6 — COMPARISON + SEAORM mapping hygiene {`#1-phase-e--g6-comparison--seaorm-mapping-hygiene`}🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 9-9: Link fragments should be valid
(MD051, link-fragments)
[warning] 11-11: Link fragments should be valid
(MD051, link-fragments)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/planning/PRD_FOLLOWON_NEXT_THREE.md` around lines 9 - 11, The three
internal links use fragment anchors that don't match the generated heading IDs
due to special characters; update either the target headings to include explicit
anchor IDs that match the link fragments (e.g., add
{`#1-phase-e--g6-comparison--seaorm-mapping-hygiene`} to the "Phase E — mapping /
docs" heading) or change the link fragments to the renderer's slugified forms
(removing or replacing `/`, `—`, `+`, backticks) so they match the actual
generated IDs for the headings referenced in the diff.
|
|
||
| - Hauliage postmortem (real incident): [hauliage — postmortem consignments list_jobs](https://github.com/microscaler/hauliage/blob/main/docs/postmortem-consignments-list-jobs-empty-2026-04.md) (path may differ if the repo is vendored; clone: `hauliage/docs/postmortem-consignments-list-jobs-empty-2026-04.md`). | ||
| - Planning note (type inference): [ISSUE_UUID_NAIVEDATETIME_TYPE_INFERENCE.md](../planning/lifeguard-derive/ISSUE_UUID_NAIVEDATETIME_TYPE_INFERENCE.md) | ||
|
|
There was a problem hiding this comment.
Fix internal planning-doc link path.
The relative path on Line 82 is off for a file located under docs/; ../planning/... likely points outside docs. Use ./planning/....
🔧 Suggested fix
-- Planning note (type inference): [ISSUE_UUID_NAIVEDATETIME_TYPE_INFERENCE.md](../planning/lifeguard-derive/ISSUE_UUID_NAIVEDATETIME_TYPE_INFERENCE.md)
+- Planning note (type inference): [ISSUE_UUID_NAIVEDATETIME_TYPE_INFERENCE.md](./planning/lifeguard-derive/ISSUE_UUID_NAIVEDATETIME_TYPE_INFERENCE.md)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/UUID_AND_POSTGRES_TYPES.md` at line 82, Update the broken relative
markdown link that currently uses "../planning/..." to use a document-local path
"./planning/..." so the link resolves correctly from this docs file; locate the
markdown link string containing "../planning/" and replace it with "./planning/"
(ensure any surrounding parentheses and link text remain unchanged).
Comment on lines
+85
to
+86
| - `**TIMESTAMP WITH TIME ZONE`**: Prefer types that match your driver and Lifeguard bindings (often `chrono::DateTime<Utc>` or documented `NaiveDateTime` patterns). Mismatches also fail at **deserialize** time. | ||
| - When in doubt, check `**postgres-types`** / `**tokio-postgres`** compatibility for the Rust type you choose. |
There was a problem hiding this comment.
Clean up malformed emphasis/backtick markup.
The emphasis markers around TIMESTAMP WITH TIME ZONE are mismatched and trigger markdown lint noise.
🔧 Suggested fix
-- `**TIMESTAMP WITH TIME ZONE`**: Prefer types that match your driver and Lifeguard bindings (often `chrono::DateTime<Utc>` or documented `NaiveDateTime` patterns). Mismatches also fail at **deserialize** time.
+- **`TIMESTAMP WITH TIME ZONE`**: Prefer types that match your driver and Lifeguard bindings (often `chrono::DateTime<Utc>` or documented `NaiveDateTime` patterns). Mismatches also fail at **deserialize** time.🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 86-86: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/UUID_AND_POSTGRES_TYPES.md` around lines 85 - 86, Fix the malformed
emphasis/backtick around TIMESTAMP WITH TIME ZONE by removing the stray
asterisks and using consistent inline code/backtick markup (e.g., `TIMESTAMP
WITH TIME ZONE`), and ensure the references to postgres-types / tokio-postgres
are consistently backticked (e.g., `postgres-types` / `tokio-postgres`) so the
markdown lint no longer flags mismatched emphasis in the paragraph mentioning
TIMESTAMP WITH TIME ZONE and Rust types like chrono::DateTime<Utc> or
NaiveDateTime.
Comment on lines
+13
to
+27
| - `**LifeModel**` - Generates immutable database row representations | ||
| - `**LifeRecord**` - Generates mutable change-set objects for updates | ||
| - `**DeriveEntity**` - Generates entity unit structs and trait implementations | ||
| - `**FromRow**` - Generates `FromRow` trait implementations for converting database rows | ||
| - `**DeriveRelation**` - Generates `Related` trait implementations for entity relationships | ||
| - `**DeriveLinked**` - Generates `Linked` trait implementations for multi-hop relationships | ||
|
|
||
| ## Usage | ||
|
|
||
| ### UUID primary keys and foreign keys | ||
|
|
||
| If the PostgreSQL column is `**UUID**`, use `**uuid::Uuid**` in Rust (`Option<uuid::Uuid>` if nullable). `**String**` will fail at **runtime** when reading rows (driver decodes `uuid` as `Uuid`, not text). | ||
|
|
||
| See the Lifeguard book doc: **[UUID and Postgres scalar types](../docs/UUID_AND_POSTGRES_TYPES.md)**. | ||
|
|
There was a problem hiding this comment.
Fix markdown emphasis rendering in this section.
Using backticks around **...** renders literal asterisks instead of emphasis in Markdown output.
Suggested doc fix
-- `**LifeModel**` - Generates immutable database row representations
-- `**LifeRecord**` - Generates mutable change-set objects for updates
-- `**DeriveEntity**` - Generates entity unit structs and trait implementations
-- `**FromRow**` - Generates `FromRow` trait implementations for converting database rows
-- `**DeriveRelation**` - Generates `Related` trait implementations for entity relationships
-- `**DeriveLinked**` - Generates `Linked` trait implementations for multi-hop relationships
+- **LifeModel** - Generates immutable database row representations
+- **LifeRecord** - Generates mutable change-set objects for updates
+- **DeriveEntity** - Generates entity unit structs and trait implementations
+- **FromRow** - Generates `FromRow` trait implementations for converting database rows
+- **DeriveRelation** - Generates `Related` trait implementations for entity relationships
+- **DeriveLinked** - Generates `Linked` trait implementations for multi-hop relationships
-If the PostgreSQL column is `**UUID**`, use `**uuid::Uuid**` in Rust (`Option<uuid::Uuid>` if nullable). `**String**` will fail at **runtime** when reading rows (driver decodes `uuid` as `Uuid`, not text).
+If the PostgreSQL column is **UUID**, use `uuid::Uuid` in Rust (`Option<uuid::Uuid>` if nullable). `String` will fail at **runtime** when reading rows (driver decodes `uuid` as `Uuid`, not text).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lifeguard-derive/README.md` around lines 13 - 27, The README shows backticks
around bold markers so asterisks render literally; remove the inline backticks
around the bolded items (e.g. change "`**LifeModel**`" to "**LifeModel**",
likewise for "**LifeRecord**", "**DeriveEntity**", "**FromRow**",
"**DeriveRelation**", "**DeriveLinked**") and keep type/code spans for actual
Rust types (use `uuid::Uuid`, `Option<uuid::Uuid>`, and `String` in backticks)
so emphasis and code formatting render correctly; ensure the UUID guidance line
uses bold for section text and backticks for code examples consistently.
Comment on lines
+658
to
+690
| /// Non-primary indexes in `schema` from `pg_indexes` (includes unique indexes; expression indexes kept for parse skip). | ||
| pub fn fetch_live_pg_indexes( | ||
| executor: &dyn LifeExecutor, | ||
| schema: &str, | ||
| ) -> Result<Vec<LiveIndexRow>, LifeError> { | ||
| let sql = r" | ||
| SELECT tablename::text, indexname::text, indexdef::text | ||
| FROM pg_indexes | ||
| WHERE schemaname = $1 | ||
| ORDER BY tablename, indexname | ||
| "; | ||
| let rows = executor.query_all(sql, &[&schema])?; | ||
| let mut out = Vec::with_capacity(rows.len()); | ||
| for row in rows { | ||
| let table_name: String = row | ||
| .try_get(0) | ||
| .map_err(|e| LifeError::Other(format!("compare-schema pg_indexes tablename: {e}")))?; | ||
| let index_name: String = row | ||
| .try_get(1) | ||
| .map_err(|e| LifeError::Other(format!("compare-schema pg_indexes indexname: {e}")))?; | ||
| let indexdef: String = row | ||
| .try_get(2) | ||
| .map_err(|e| LifeError::Other(format!("compare-schema pg_indexes indexdef: {e}")))?; | ||
| if index_name.ends_with("_pkey") { | ||
| continue; | ||
| } | ||
| out.push(LiveIndexRow { | ||
| table_name, | ||
| index_name, | ||
| indexdef, | ||
| }); | ||
| } | ||
| Ok(out) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the full context of the file around the flagged lines
wc -l lifeguard-migrate/src/schema_migration_compare.rsRepository: microscaler/lifeguard
Length of output: 119
🏁 Script executed:
# Look at the function and surrounding context to understand the full picture
sed -n '650,700p' lifeguard-migrate/src/schema_migration_compare.rsRepository: microscaler/lifeguard
Length of output: 2161
🏁 Script executed:
# Check if there are other index-related queries in the file that handle PK filtering differently
rg -n "pg_index|indisprimary|_pkey" lifeguard-migrate/src/schema_migration_compare.rsRepository: microscaler/lifeguard
Length of output: 5166
🏁 Script executed:
# Search for other uses of pg_indexes to see if there's a pattern in this codebase
rg -n "pg_indexes" lifeguard-migrate/src/ -A 3 -B 3Repository: microscaler/lifeguard
Length of output: 10915
🌐 Web query:
PostgreSQL custom named primary key index indisprimary pg_index catalog
💡 Result:
To get the actual index relation that backs a table’s PRIMARY KEY (including when it has a custom name), query the catalogs like this:
1) Using pg_index.indisprimary (direct “this index is the PK” flag)
SELECT
t.oid::regclass AS table_name,
i.oid::regclass AS pk_index
FROM pg_catalog.pg_index x
JOIN pg_catalog.pg_class t ON t.oid = x.indrelid
JOIN pg_catalog.pg_class i ON i.oid = x.indexrelid
WHERE x.indrelid = 'public.my_table'::regclass
AND x.indisprimary;pg_index.indisprimary = true means that index “represents the primary key of the table.” [1]
2) Using pg_constraint (often the most semantically correct)
SELECT
conrelid::regclass AS table_name,
conname AS pk_constraint,
conindid::regclass AS pk_index
FROM pg_catalog.pg_constraint
WHERE conrelid = 'public.my_table'::regclass
AND contype = 'p';For a PRIMARY KEY constraint (contype = 'p'), pg_constraint.conindid is “the index supporting this constraint.” [2]
Notes on “custom named primary key index”
- PostgreSQL always enforces a PK via a unique index; the index can have a system-generated name or one you effectively control by controlling the constraint/index naming (e.g., creating a unique index first and attaching it with
PRIMARY KEY USING INDEX, or naming the constraint so the backing index name aligns). The queries above will return whatever the current backing index name is. [2]
Sources: PostgreSQL catalog docs for pg_index.indisprimary [1] and pg_constraint (contype, conindid) [2].
[1] https://www.postgresql.org/docs/current/catalog-pg-index.html
[2] https://www.postgresql.org/docs/current/catalog-pg-constraint.html
Filter primary-key indexes using pg_index.indisprimary, not _pkey suffix.
The _pkey suffix is not authoritative. A user-named primary-key constraint can produce an index with any name, and those indexes will be incorrectly included in index_rows and reported as drift. Other functions in this module (fetch_live_btree_index_key_opclasses, etc.) correctly use NOT xi.indisprimary—apply the same pattern here. Replace the pg_indexes view query with a direct pg_index join and filter by NOT xi.indisprimary and xi.indisvalid for consistency.
Suggested query shape
- SELECT tablename::text, indexname::text, indexdef::text
- FROM pg_indexes
- WHERE schemaname = $1
+ SELECT
+ t.relname::text AS tablename,
+ ic.relname::text AS indexname,
+ pg_get_indexdef(ic.oid)::text AS indexdef
+ FROM pg_index xi
+ JOIN pg_class ic ON ic.oid = xi.indexrelid
+ JOIN pg_class t ON t.oid = xi.indrelid
+ JOIN pg_namespace n ON n.oid = t.relnamespace
+ WHERE n.nspname = $1
+ AND t.relkind IN ('r', 'p')
+ AND ic.relkind = 'i'
+ AND xi.indisvalid
+ AND NOT xi.indisprimary
ORDER BY tablename, indexname
...
- if index_name.ends_with("_pkey") {
- continue;
- }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lifeguard-migrate/src/schema_migration_compare.rs` around lines 658 - 690,
fetch_live_pg_indexes currently filters primary keys by index name suffix;
change it to query pg_index (join pg_class/pg_namespace as needed) and select
index metadata using pg_get_indexdef, applying the filters NOT xi.indisprimary
and xi.indisvalid instead of the index_name.ends_with("_pkey") check. Update the
SQL in fetch_live_pg_indexes to use a join on pg_index (alias xi) and schematic
name parameter, remove the post-row-name suffix check, and keep mapping to
LiveIndexRow (table_name, index_name, indexdef) while preserving existing
error-wrapping on row.try_get failures.
Comment on lines
+359
to
+369
| fn index_covers_only_column( | ||
| index: &lifeguard::IndexDefinition, | ||
| col_name: &str, | ||
| ) -> bool { | ||
| let cov = if !index.key_parts.is_empty() { | ||
| index_key_parts_coverage_columns(&index.key_parts) | ||
| } else { | ||
| index.columns.clone() | ||
| }; | ||
| cov.len() == 1 && cov[0] == col_name | ||
| } |
There was a problem hiding this comment.
Don't treat expression or predicate indexes as a plain #[indexed] equivalent.
This helper now dedupes on coverage columns, so an explicit index like lower(email) | email (or any predicate/modifier-bearing index) suppresses the auto-generated plain CREATE INDEX ... (email). Those indexes are not interchangeable, so the generator can silently drop the simple index users asked for.
Possible fix
fn index_covers_only_column(
index: &lifeguard::IndexDefinition,
col_name: &str,
) -> bool {
- let cov = if !index.key_parts.is_empty() {
- index_key_parts_coverage_columns(&index.key_parts)
- } else {
- index.columns.clone()
- };
- cov.len() == 1 && cov[0] == col_name
+ if index.partial_where.is_some() {
+ return false;
+ }
+ match index.key_parts.as_slice() {
+ [lifeguard::IndexKeyPart::Column {
+ name,
+ opclass: None,
+ collate: None,
+ sort: None,
+ nulls: None,
+ }] => name == col_name,
+ [] => index.columns.len() == 1 && index.columns[0] == col_name,
+ _ => false,
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lifeguard-migrate/src/sql_generator.rs` around lines 359 - 369, The helper
index_covers_only_column is incorrectly treating expression/predicate or
modified indexes as equivalent to a plain single-column index; update
index_covers_only_column (and its use of index_key_parts_coverage_columns) to
only return true when the index is an unmodified plain single-column index:
verify lifeguard::IndexDefinition has exactly one coverage column equal to
col_name AND that there are no key_parts expressions, predicates, collations, or
other modifiers present (i.e., require index.key_parts to be empty and
index.columns to contain a single plain column with no modifiers), so
expression-based or predicate-bearing indexes (e.g., lower(email) | email) do
not suppress generation of a separate simple CREATE INDEX on the column.
Comment on lines
+29
to
35
| # Create shared cluster if missing (do not delete an existing cluster by default) | ||
| if ! kind get clusters 2>/dev/null | grep -q "^${CLUSTER_NAME}$"; then | ||
| echo "📦 Creating shared Kind cluster (${CLUSTER_NAME})..." | ||
| kind create cluster --config kind-config.yaml | ||
| else | ||
| echo "✅ Kind cluster '${CLUSTER_NAME}' already exists; skipping create." | ||
| fi |
There was a problem hiding this comment.
Set kubectl context explicitly before running kubectl commands.
With the new “skip create if exists” flow (Line 29), the script no longer guarantees current context is kind-kind. That can make Lines 39/43 run against the wrong cluster.
🔧 Proposed fix
CLUSTER_NAME="kind"
+KUBE_CONTEXT="kind-${CLUSTER_NAME}"
@@
if ! kind get clusters 2>/dev/null | grep -q "^${CLUSTER_NAME}$"; then
echo "📦 Creating shared Kind cluster (${CLUSTER_NAME})..."
- kind create cluster --config kind-config.yaml
+ kind create cluster --name "${CLUSTER_NAME}" --config kind-config.yaml
else
echo "✅ Kind cluster '${CLUSTER_NAME}' already exists; skipping create."
fi
+
+# Always target the intended Kind context, even when cluster already exists.
+kubectl config use-context "${KUBE_CONTEXT}" >/dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/setup_kind_cluster.sh` around lines 29 - 35, The script may run
subsequent kubectl commands against the wrong cluster because it skips creation
when a cluster exists but never sets the kubectl context; immediately after the
cluster-create check (the block using CLUSTER_NAME and kind create cluster),
explicitly set the kubectl context to the Kind cluster by running kubectl config
use-context "kind-${CLUSTER_NAME}" (or detect the correct context name if
different) and fail with a clear error if that context doesn't exist, so all
later kubectl commands target the intended Kind cluster.
Comment on lines
+55
to
+76
| /// Append optional ` COLLATE …`, operator class, `ASC`/`DESC`, `NULLS …` after a leading SQL fragment. | ||
| fn push_index_key_suffixes( | ||
| out: &mut String, | ||
| collate: &Option<String>, | ||
| opclass: &Option<String>, | ||
| sort: Option<IndexBtreeSort>, | ||
| nulls: Option<IndexBtreeNulls>, | ||
| ) { | ||
| if let Some(c) = collate { | ||
| out.push_str(" COLLATE "); | ||
| if c.chars().any(|ch| ch.is_whitespace() || ch == '"') || !is_simple_sql_ident(c) { | ||
| out.push('"'); | ||
| out.push_str(&c.replace('"', "\"\"")); | ||
| out.push('"'); | ||
| } else { | ||
| out.push_str(c); | ||
| } | ||
| } | ||
| if let Some(o) = opclass { | ||
| out.push(' '); | ||
| out.push_str(o); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l src/query/table/definition.rsRepository: microscaler/lifeguard
Length of output: 98
🏁 Script executed:
cat -n src/query/table/definition.rs | head -200Repository: microscaler/lifeguard
Length of output: 7796
🏁 Script executed:
cat -n src/query/table/definition.rs | sed -n '180,250p'Repository: microscaler/lifeguard
Length of output: 3309
🏁 Script executed:
# Check for all format! and write! calls in the file
rg 'format!\(|write!\(' src/query/table/definition.rs -A 1Repository: microscaler/lifeguard
Length of output: 155
Fix positional format string and properly quote SQL identifiers in index key rendering.
Line 196 uses positional syntax format!("{}({inner})", idx.name) instead of named placeholders. Change to format!("{name}({inner})", name = idx.name).
More critically, format_index_key_list_sql() emits unquoted column names (line 115) and operator classes (line 75), while COLLATE is conditionally quoted. This breaks SQL for reserved keywords and mixed-case identifiers. Column names and operator classes should be quoted when they are not simple SQL identifiers, matching the logic already applied to COLLATE on lines 65–71.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/query/table/definition.rs` around lines 55 - 76, Change the positional
format call format!("{}({inner})", idx.name) to a named placeholder
format!("{name}({inner})", name = idx.name) in the index name rendering, and
update format_index_key_list_sql and push_index_key_suffixes to always quote SQL
identifiers (column names and operator classes) when they are not “simple”
identifiers using the same logic already used for COLLATE: detect
whitespace/quotes or !is_simple_sql_ident(), wrap in double quotes and escape
internal quotes by doubling them; specifically modify push_index_key_suffixes to
quote opclass when needed (replace the unconditional out.push_str(o)) and modify
format_index_key_list_sql to emit quoted column identifiers when necessary so
reserved keywords and mixed-case names are handled consistently.
Comment on lines
+536
to
+545
| let join_on = join_tbl_on_expr( | ||
| &rel_def.from_tbl, | ||
| &rel_def.to_tbl, | ||
| &rel_def.from_col, | ||
| &rel_def.to_col, | ||
| ); | ||
| let mut q = SelectQuery::<R>::new(); | ||
| q = q.inner_join(Self::Entity::default(), join_on); | ||
| q = q.filter(build_where_condition(&rel_def, self)?); | ||
| q = q.filter(parent_scope.into_condition()); |
There was a problem hiding this comment.
Preserve the full RelationDef join semantics here.
Rebuilding the join with join_tbl_on_expr() only keeps the FK equality. Any RelationDef::on_condition / condition_type metadata is lost, and same-table relations are still joined without an alias, so find_related_parent_scoped() can diverge from the declared relation or emit ambiguous SQL. Reuse rel_def.join_on_expr() and either alias or explicitly reject self-joins on this path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/relation/traits.rs` around lines 536 - 545, The code rebuilds the join
using join_tbl_on_expr which discards RelationDef::on_condition and
condition_type and fails to handle self-joins; change the join to use
rel_def.join_on_expr() (preserving on_condition and condition_type) when
constructing the join for find_related_parent_scoped, and if rel_def refers to
the same table as Self::Entity::default() either apply a proper aliasing
strategy on the joined entity or explicitly return an error/reject this path;
ensure you still apply the existing filters (build_where_condition(&rel_def,
self) and parent_scope.into_condition()) after reusing rel_def.join_on_expr().
| return (sql.to_string(), None); | ||
| } | ||
| let last = words[words.len() - 1]; | ||
| if last.contains("_ops") || last.ends_with("_ops") { |
There was a problem hiding this comment.
Redundant contains check subsumes ends_with for opclass detection
Low Severity
In peel_trailing_opclass_from_expr, the condition last.contains("_ops") || last.ends_with("_ops") is redundant — contains is a superset of ends_with. More importantly, contains("_ops") is overly broad and would incorrectly match trailing words like my_ops_handler as an opclass, stripping them from the expression SQL. The check likely intended ends_with("_ops") only.
Reviewed by Cursor Bugbot for commit d6dc4f1. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lifeguard-derive/src/attributes.rs`:
- Around line 856-877: The quoted-collation parsing currently iterates bytes and
uses char::from(bytes[i]), which corrupts non-ASCII UTF-8; replace the
byte-level loop with a char/char_indices-based loop: iterate
tail.char_indices().skip(1) to accumulate characters into out (pushing the
char), detect doubled quote by peeking the next char or by checking the next
byte index from char_indices to see an immediate '"' and advance appropriately,
and compute the correct remainder slice using the byte index of the closing
quote (e.g., tail[closing_byte_index+1..].trim_start().to_string()). Update the
code that returns Ok((out, rest)) and the unterminated-quote error path
accordingly, removing the bytes/char::from usage and the manual usize byte index
logic.
In `@lifeguard-derive/src/macros/scope_bundle.rs`:
- Around line 55-63: The macro currently only rejects methods with a receiver
but silently rewrites functions that have typed parameters or generics; update
the validation in the scope_bundle macro to explicitly reject any typed
parameters or non-empty generics before the signature-rewriting code runs: check
func.sig.inputs for more than zero non-self arguments (or any FnArg::Typed) and
check func.sig.generics for non-empty params or where clauses, and if found
return Error::new with func.sig.fn_token.span() and a clear message like
"`#[scope_bundle]` must be on an associated function without typed parameters or
generics" instead of proceeding to the rewrite (the code that mutates the
signature around the current lines 91-93).
In `@README.md`:
- Line 13: The README contains malformed Markdown emphasis/code nesting like ``
`**may_postgres`** `` that breaks rendering and linter rules; locate every
similar occurrence (examples: the token may_postgres and other inline spans
flagged in the comment) and normalize them to valid Markdown by either using
code formatting only (e.g., `may_postgres`) or combining bold and code correctly
as **`may_postgres`**, ensuring backticks wrap the identifier and bold markers
surround the backtick-delimited span consistently for all affected occurrences.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91dac7af-fa74-469c-81be-91e5dd4e633c
📒 Files selected for processing (4)
README.mdlifeguard-derive/src/attributes.rslifeguard-derive/src/macros/life_model.rslifeguard-derive/src/macros/scope_bundle.rs
✅ Files skipped from review due to trivial changes (1)
- lifeguard-derive/src/macros/life_model.rs
Comment on lines
+856
to
+877
| if tail.starts_with('"') { | ||
| let bytes = tail.as_bytes(); | ||
| let mut i = 1usize; | ||
| let mut out = String::new(); | ||
| while i < bytes.len() { | ||
| if bytes[i] == b'"' { | ||
| if i + 1 < bytes.len() && bytes[i + 1] == b'"' { | ||
| out.push('"'); | ||
| i += 2; | ||
| continue; | ||
| } | ||
| let rest = tail[i + 1..].trim_start().to_string(); | ||
| return Ok((out, rest)); | ||
| } | ||
| out.push(char::from(bytes[i])); | ||
| i += 1; | ||
| } | ||
| return Err(syn::Error::new( | ||
| Span::call_site(), | ||
| "Invalid index definition: unterminated quoted collation", | ||
| )); | ||
| } |
There was a problem hiding this comment.
UTF-8 handling is incorrect for quoted collation names.
The byte-by-byte iteration with char::from(bytes[i]) only works correctly for ASCII characters. For collation names containing non-ASCII UTF-8 characters, this will produce corrupted strings because multi-byte UTF-8 sequences are split and misinterpreted.
🔧 Proposed fix using char iteration
if tail.starts_with('"') {
- let bytes = tail.as_bytes();
- let mut i = 1usize;
+ let mut chars = tail.chars();
+ chars.next(); // skip opening quote
let mut out = String::new();
- while i < bytes.len() {
- if bytes[i] == b'"' {
- if i + 1 < bytes.len() && bytes[i + 1] == b'"' {
+ let mut prev_was_quote = false;
+ let mut end_pos = 1usize; // track byte position for rest extraction
+ for c in chars {
+ end_pos += c.len_utf8();
+ if c == '"' {
+ if prev_was_quote {
+ // escaped quote ""
out.push('"');
- i += 2;
+ prev_was_quote = false;
continue;
}
- let rest = tail[i + 1..].trim_start().to_string();
+ prev_was_quote = true;
+ continue;
+ }
+ if prev_was_quote {
+ // previous quote was the closing one
+ let rest = tail[end_pos - c.len_utf8()..].trim_start().to_string();
return Ok((out, rest));
}
- out.push(char::from(bytes[i]));
- i += 1;
+ out.push(c);
}
+ if prev_was_quote {
+ // string ended with closing quote
+ let rest = tail[end_pos..].trim_start().to_string();
+ return Ok((out, rest));
+ }
return Err(syn::Error::new(
Span::call_site(),
"Invalid index definition: unterminated quoted collation",
));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lifeguard-derive/src/attributes.rs` around lines 856 - 877, The
quoted-collation parsing currently iterates bytes and uses char::from(bytes[i]),
which corrupts non-ASCII UTF-8; replace the byte-level loop with a
char/char_indices-based loop: iterate tail.char_indices().skip(1) to accumulate
characters into out (pushing the char), detect doubled quote by peeking the next
char or by checking the next byte index from char_indices to see an immediate
'"' and advance appropriately, and compute the correct remainder slice using the
byte index of the closing quote (e.g.,
tail[closing_byte_index+1..].trim_start().to_string()). Update the code that
returns Ok((out, rest)) and the unterminated-quote error path accordingly,
removing the bytes/char::from usage and the manual usize byte index logic.
Comment on lines
+55
to
+63
| if func.sig.receiver().is_some() { | ||
| return Error::new( | ||
| func.sig.fn_token.span(), | ||
| "`#[scope_bundle]` must be on an associated function without `self`", | ||
| ) | ||
| .to_compile_error() | ||
| .into(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Reject typed parameters/generics explicitly before rewriting the signature.
Line 55 only blocks self. Typed parameters and generics are currently accepted, then silently rewritten at Lines 91-93. Emit a clear compile error instead of mutating these signatures implicitly.
Suggested patch
@@
if func.sig.receiver().is_some() {
return Error::new(
func.sig.fn_token.span(),
"`#[scope_bundle]` must be on an associated function without `self`",
)
.to_compile_error()
.into();
}
+ if !func.sig.inputs.is_empty() {
+ return Error::new(
+ func.sig.inputs.span(),
+ "`#[scope_bundle]` requires a zero-argument function",
+ )
+ .to_compile_error()
+ .into();
+ }
+ if !func.sig.generics.params.is_empty() || func.sig.generics.where_clause.is_some() {
+ return Error::new(
+ func.sig.generics.span(),
+ "`#[scope_bundle]` does not support generic parameters",
+ )
+ .to_compile_error()
+ .into();
+ }
@@
func.sig.ident = new_ident;
func.sig.generics = syn::Generics::default();
func.sig.output = syn::parse_quote!(-> sea_query::Condition);Also applies to: 91-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lifeguard-derive/src/macros/scope_bundle.rs` around lines 55 - 63, The macro
currently only rejects methods with a receiver but silently rewrites functions
that have typed parameters or generics; update the validation in the
scope_bundle macro to explicitly reject any typed parameters or non-empty
generics before the signature-rewriting code runs: check func.sig.inputs for
more than zero non-self arguments (or any FnArg::Typed) and check
func.sig.generics for non-empty params or where clauses, and if found return
Error::new with func.sig.fn_token.span() and a clear message like
"`#[scope_bundle]` must be on an associated function without typed parameters or
generics" instead of proceeding to the rewrite (the code that mutates the
signature around the current lines 91-93).
| **The problem:** Existing Rust ORMs (SeaORM, Diesel, SQLx) target async/`Tokio`. The `may` coroutine runtime uses stackful coroutines, not async futures—**architectures do not bridge** without significant cost. For the narrative on that mismatch and the pain of forcing async ORMs onto a coroutine stack, see **[LIFEGUARD_BLOG_POST.md](./LIFEGUARD_BLOG_POST.md)**. | ||
|
|
||
| **The approach:** A full ORM on **`may_postgres`** (coroutine-native PostgreSQL). No async runtime on the database path. Pure coroutine I/O. | ||
| **The approach:** A full ORM on `**may_postgres`** (coroutine-native PostgreSQL). No async runtime on the database path. Pure coroutine I/O. |
There was a problem hiding this comment.
Fix malformed Markdown emphasis/code/link nesting.
Several inline spans mix ** and backticks in an invalid order (for example `**may_postgres`**), which breaks rendering and triggers MD037-style lint warnings. Please normalize to either **\...`**` or plain backticks/links.
Suggested Markdown cleanup
-**The approach:** A full ORM on `**may_postgres`** (coroutine-native PostgreSQL). No async runtime on the database path. Pure coroutine I/O.
+**The approach:** A full ORM on **`may_postgres`** (coroutine-native PostgreSQL). No async runtime on the database path. Pure coroutine I/O.
-**Who it is for:** Teams on `**may`** (for example **BRRTRouter**) ...
+**Who it is for:** Teams on **`may`** (for example **BRRTRouter**) ...
-High-level data flow: your app and ORM go through `**LifeguardPool`** ...; `**[lifeguard-reflector](./lifeguard-reflector/)`** refreshes ...
+High-level data flow: your app and ORM go through **`LifeguardPool`** ...; **[`lifeguard-reflector`](./lifeguard-reflector/)** refreshes ...
-Lifeguard is **OpenTelemetry-compatible**: optional `**tracing`** spans/events ...
+Lifeguard is **OpenTelemetry-compatible**: optional **`tracing`** spans/events ...
-| `**find_related` + related-side scope (compile-only example)**
+| **`find_related` + related-side scope (compile-only example)**
-Licensed under **MIT OR Apache-2.0** at your option (`[Cargo.toml](./Cargo.toml)`). The `[LICENSE](./LICENSE)` file ...
+Licensed under **MIT OR Apache-2.0** at your option ([`Cargo.toml`](./Cargo.toml)). The [`LICENSE`](./LICENSE) file ...Also applies to: 15-15, 23-23, 80-80, 112-112, 135-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 13, The README contains malformed Markdown emphasis/code
nesting like `` `**may_postgres`** `` that breaks rendering and linter rules;
locate every similar occurrence (examples: the token may_postgres and other
inline spans flagged in the comment) and normalize them to valid Markdown by
either using code formatting only (e.g., `may_postgres`) or combining bold and
code correctly as **`may_postgres`**, ensuring backticks wrap the identifier and
bold markers surround the backtick-delimited span consistently for all affected
occurrences.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lifeguard-derive/src/type_conversion.rs (2)
1053-1067:⚠️ Potential issue | 🟠 MajorSame dereference issue as
generate_value_to_field.Line 1054 uses
self.#field_name = Some(v);but should likely useSome(*v)to match theJsonpattern at line 1012.🛠️ Proposed fix (pending verification)
sea_query::Value::Decimal(Some(v)) => { - self.#field_name = Some(v); + self.#field_name = Some(*v); Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-derive/src/type_conversion.rs` around lines 1053 - 1067, The Decimal match arms in type_conversion.rs are assigning the Decimal wrapper value by moving the reference instead of the inner Decimal; in the sea_query::Value::Decimal(Some(v)) arm (and keep Decimal(None) unchanged) dereference v when assigning to the model field (i.e., set self.#field_name = Some(*v) instead of Some(v)) so the stored type matches the pattern used for Json at generate_value_to_field and avoid borrowing/move mismatches; ensure the same change is applied wherever the Decimal(Some(v)) pattern assigns to self.#field_name and that column: stringify!(`#column_variant`) stays unchanged in the error arm.
637-654:⚠️ Potential issue | 🟠 MajorMissing dereference for
Decimalvalue extraction.The reverse conversion from
Value::Decimal(Some(v))to field assignment does not dereference the boxed value, creating a type mismatch.This pattern is inconsistent with:
Value::Jsonextraction at line 221 (dereferences:Ok(*v))OwnedParamconversion at line 123 (dereferences:OwnedParam::Decimal(Some(*d)))Lines 637–638 and 1053–1054 both require dereferencing:
- sea_query::Value::Decimal(Some(v)) => { - self.#field_name = v; + sea_query::Value::Decimal(Some(v)) => { + self.#field_name = *v;and
- sea_query::Value::Decimal(Some(v)) => { - self.#field_name = Some(v); + sea_query::Value::Decimal(Some(v)) => { + self.#field_name = Some(*v);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-derive/src/type_conversion.rs` around lines 637 - 654, The Decimal match arms assign the boxed decimal directly (Value::Decimal(Some(v)) => self.#field_name = v;) causing a type mismatch; change the assignment to dereference the boxed value (e.g., use *v) in the match for Value::Decimal(Some(v)) and make the analogous fix in the other mirrored location (the second Decimal match around the OwnedParam/OwnedParam::Decimal pattern referenced), keeping the error arms using lifeguard::ActiveModelError::InvalidValueType and the same column/name symbols unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lifeguard-derive/src/type_conversion.rs`:
- Around line 272-277: The conversion currently returns
sea_query::Value::Decimal(Some(self.#field_name)) but if Value::Decimal expects
Option<Box<rust_decimal::Decimal>> you must wrap the field in Box::new(...) and
preserve the Option, e.g. construct
sea_query::Value::Decimal(Some(Box::new(self.#field_name))). Update the forward
conversion in the is_decimal_type branch (the block that uses field_type and
field_name), and apply the same change to the other similar conversion sites
mentioned (around the blocks corresponding to the ranges noted, e.g., the other
forward conversion functions at the locations handling decimal conversion) so
all Decimal conversions consistently use Option<Box<Decimal>>.
- Around line 273-277: The Decimal conversions use the wrong types:
Value::Decimal expects Option<Box<rust_decimal::Decimal>> and the reverse arms
receive Box<Decimal>, so update the three spots handling Value::Decimal: in the
forward conversion where you currently return
sea_query::Value::Decimal(Some(self.#field_name)) (in the is_decimal_type
branch) wrap the value with Box::new(self.#field_name); in the reverse
conversion match arms that handle sea_query::Value::Decimal(Some(v)) (the
assignments around the earlier 637–650 region) dereference v when assigning
(self.#field_name = *v); and in the Option-assignment reverse arm (around
1053–1063) dereference v into the Option (self.#field_name = Some(*v)).
In `@src/query/column/column_trait.rs`:
- Around line 110-112: The method is_contained_by_json lacks the same clippy
allowance used on other `is_*` builders, causing clippy::wrong_self_convention
warnings; add the attribute #[allow(clippy::wrong_self_convention)] directly
above the is_contained_by_json method (the function named is_contained_by_json
in column_trait.rs) so it matches the parity with
is_null/is_not_null/is_in/is_not_in and suppresses the lint for this builder.
- Around line 105-113: The JSON containment methods use
Expr::val(serde_json::Value) which yields untyped parameters; change both
json_contains and is_contained_by_json to explicitly cast the RHS to jsonb
(e.g., wrap the value expression with a CAST to jsonb or otherwise force
Expr::val(...).cast("jsonb") / an equivalent sea-query construct) so PostgreSQL
uses jsonb for @> and <@ operator resolution, and add the missing
#[allow(clippy::wrong_self_convention)] attribute to is_contained_by_json to
match the other is_* methods for lint consistency; update references in
json_contains, is_contained_by_json, Expr::val and BinOper::Custom("@>"/"<@")
accordingly.
---
Outside diff comments:
In `@lifeguard-derive/src/type_conversion.rs`:
- Around line 1053-1067: The Decimal match arms in type_conversion.rs are
assigning the Decimal wrapper value by moving the reference instead of the inner
Decimal; in the sea_query::Value::Decimal(Some(v)) arm (and keep Decimal(None)
unchanged) dereference v when assigning to the model field (i.e., set
self.#field_name = Some(*v) instead of Some(v)) so the stored type matches the
pattern used for Json at generate_value_to_field and avoid borrowing/move
mismatches; ensure the same change is applied wherever the Decimal(Some(v))
pattern assigns to self.#field_name and that column: stringify!(`#column_variant`)
stays unchanged in the error arm.
- Around line 637-654: The Decimal match arms assign the boxed decimal directly
(Value::Decimal(Some(v)) => self.#field_name = v;) causing a type mismatch;
change the assignment to dereference the boxed value (e.g., use *v) in the match
for Value::Decimal(Some(v)) and make the analogous fix in the other mirrored
location (the second Decimal match around the OwnedParam/OwnedParam::Decimal
pattern referenced), keeping the error arms using
lifeguard::ActiveModelError::InvalidValueType and the same column/name symbols
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3b757b9-cf3b-4013-a929-1af250d1e47a
📒 Files selected for processing (4)
AGENT.mdexamples/test_value.rslifeguard-derive/src/type_conversion.rssrc/query/column/column_trait.rs
✅ Files skipped from review due to trivial changes (2)
- AGENT.md
- examples/test_value.rs
Comment on lines
272
to
277
| // Check for rust_decimal::Decimal | ||
| if is_decimal_type(field_type) { | ||
| // Convert Decimal to String for SeaQuery Value | ||
| // SeaQuery doesn't have a Decimal variant, so we use String | ||
| return quote! { | ||
| sea_query::Value::String(Some(self.#field_name.to_string())) | ||
| sea_query::Value::Decimal(Some(self.#field_name)) | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Good: Switching to native Value::Decimal improves type fidelity.
Using the dedicated Value::Decimal variant instead of string serialization/parsing eliminates precision loss and parsing overhead. This aligns with how Uuid and NaiveDateTime are handled.
Pending the Box wrapping verification, if Value::Decimal is indeed Option<Box<Decimal>>, apply this fix consistently:
♻️ Proposed fix for all forward conversions
// Line 275
- sea_query::Value::Decimal(Some(self.#field_name))
+ sea_query::Value::Decimal(Some(Box::new(self.#field_name)))
// Line 366
- self.#field_name.as_ref().map(|v| sea_query::Value::Decimal(Some(*v))).unwrap_or(sea_query::Value::Decimal(None))
+ self.#field_name.as_ref().map(|v| sea_query::Value::Decimal(Some(Box::new(*v)))).unwrap_or(sea_query::Value::Decimal(None))
// Line 479
- .map(|v| sea_query::Value::Decimal(Some(*v)))
+ .map(|v| sea_query::Value::Decimal(Some(Box::new(*v))))Also applies to: 363-368, 476-480
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lifeguard-derive/src/type_conversion.rs` around lines 272 - 277, The
conversion currently returns sea_query::Value::Decimal(Some(self.#field_name))
but if Value::Decimal expects Option<Box<rust_decimal::Decimal>> you must wrap
the field in Box::new(...) and preserve the Option, e.g. construct
sea_query::Value::Decimal(Some(Box::new(self.#field_name))). Update the forward
conversion in the is_decimal_type branch (the block that uses field_type and
field_name), and apply the same change to the other similar conversion sites
mentioned (around the blocks corresponding to the ranges noted, e.g., the other
forward conversion functions at the locations handling decimal conversion) so
all Decimal conversions consistently use Option<Box<Decimal>>.
Comment on lines
273
to
277
| if is_decimal_type(field_type) { | ||
| // Convert Decimal to String for SeaQuery Value | ||
| // SeaQuery doesn't have a Decimal variant, so we use String | ||
| return quote! { | ||
| sea_query::Value::String(Some(self.#field_name.to_string())) | ||
| sea_query::Value::Decimal(Some(self.#field_name)) | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
sea-query Value::Decimal Box rust_decimal
💡 Result:
In sea-query, the Value enum supports Decimal(Option<Box<rust_decimal::Decimal>>) for precise decimal handling. This variant is available only when enabling the with-rust_decimal feature flag in your Cargo.toml: [dependencies] sea-query = { version = "0.32", features = ["with-rust_decimal"] } Usage example: use sea_query::{Value, Expr}; use rust_decimal::Decimal; let decimal_value = Decimal::new(12345, 2); // 123.45 let value = Value::Decimal(Some(Box::new(decimal_value))); Expr::value(value); The Decimal is boxed to keep the Value enum size to one pointer (for cache efficiency). Methods like is_decimal, as_ref_decimal, and decimal_to_f64 are available on Value with this feature. This integrates with SeaORM entities where Decimal maps to database DECIMAL/NUMERIC types.
Citations:
- 1: https://docs.rs/sea-query/latest/sea_query/value/enum.Value.html
- 2: https://docs.rs/sea-orm/latest/sea_orm/entity/enum.Value.html
- 3: https://docs.rs/sea-query/0.16.3/sea_query/value/trait.ValueType.html
- 4: https://www.sea-ql.org/SeaORM/docs/1.1.x/generate-entity/column-types/
- 5: Value: box BigDecimal and Array SeaQL/sea-query#931
- 6: https://docs.rs/sea-query
🏁 Script executed:
# First, let's look at the file to understand the context of each changed segment
head -300 lifeguard-derive/src/type_conversion.rs | tail -50Repository: microscaler/lifeguard
Length of output: 1586
🏁 Script executed:
# Check lines around 275 (generate_field_to_value Decimal)
sed -n '260,290p' lifeguard-derive/src/type_conversion.rsRepository: microscaler/lifeguard
Length of output: 986
🏁 Script executed:
# Check lines around 366
sed -n '355,375p' lifeguard-derive/src/type_conversion.rsRepository: microscaler/lifeguard
Length of output: 811
🏁 Script executed:
# Check lines around 479
sed -n '470,490p' lifeguard-derive/src/type_conversion.rsRepository: microscaler/lifeguard
Length of output: 637
🏁 Script executed:
# Check lines around 637-650
sed -n '630,660p' lifeguard-derive/src/type_conversion.rsRepository: microscaler/lifeguard
Length of output: 1282
🏁 Script executed:
# Check lines around 1053-1063
sed -n '1045,1070p' lifeguard-derive/src/type_conversion.rsRepository: microscaler/lifeguard
Length of output: 907
Add Box::new() wrapper to forward Decimal conversions; dereference v in reverse conversions.
Value::Decimal is Option<Box<rust_decimal::Decimal>>. Three locations need fixes:
-
Line 275: Wrap in
Box::new():sea_query::Value::Decimal(Some(Box::new(self.#field_name)))
-
Lines 637–650: Dereference when assigning:
sea_query::Value::Decimal(Some(v)) => { self.#field_name = *v;
-
Lines 1053–1063: Dereference when assigning to Option:
sea_query::Value::Decimal(Some(v)) => { self.#field_name = Some(*v);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lifeguard-derive/src/type_conversion.rs` around lines 273 - 277, The Decimal
conversions use the wrong types: Value::Decimal expects
Option<Box<rust_decimal::Decimal>> and the reverse arms receive Box<Decimal>, so
update the three spots handling Value::Decimal: in the forward conversion where
you currently return sea_query::Value::Decimal(Some(self.#field_name)) (in the
is_decimal_type branch) wrap the value with Box::new(self.#field_name); in the
reverse conversion match arms that handle sea_query::Value::Decimal(Some(v))
(the assignments around the earlier 637–650 region) dereference v when assigning
(self.#field_name = *v); and in the Option-assignment reverse arm (around
1053–1063) dereference v into the Option (self.#field_name = Some(*v)).
Comment on lines
+105
to
+113
| /// Check if target JSON element contains the supplied JSON value (`column @> value`) | ||
| fn json_contains<T: Into<serde_json::Value>>(self, value: T) -> Expr { | ||
| Expr::col(self).binary(sea_query::BinOper::Custom("@>"), Expr::val(value.into())) | ||
| } | ||
|
|
||
| /// Check if target JSON element is contained by the supplied JSON structure (`column <@ value`) | ||
| fn is_contained_by_json<T: Into<serde_json::Value>>(self, value: T) -> Expr { | ||
| Expr::col(self).binary(sea_query::BinOper::Custom("<@"), Expr::val(value.into())) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For sea-query v1.0.0-rc.29 on PostgreSQL, when using Expr::val(serde_json::Value) with BinOper::Custom("@>") or "<@", is the bound value treated as jsonb automatically, or is an explicit cast_as("jsonb") required for reliable operator resolution?
💡 Result:
For sea-query v1.0.0-rc.29 on PostgreSQL, when using Expr::val(serde_json::Value) with BinOper::Custom("@>") or "<@", an explicit cast_as("jsonb") is required for reliable operator resolution. The bound value is serialized to Value::Json, which is bound as a parameter by PostgresQueryBuilder. Postgres defaults unbound parameter types to text, and while jsonb @> and <@ accept text on the RHS (implicitly casting to jsonb), type inference can fail in ambiguous cases, especially with complex JSON or certain driver behaviors. Issue #695 highlights Postgres inferring text for placeholders without column context, causing failures. Official docs show Expr::contains (PgExpr) for jsonb @>, but for Custom BinOper, explicit casting ensures jsonb type: Expr::val(json_val).cast_as("jsonb").bin_oper(BinOper::Custom("@>"), Expr::col(jsonb_col)).
Citations:
- 1: https://docs.rs/sea-query/latest/sea_query/expr/struct.Expr.html
- 2: Explicitly annotate values in postgres when types cannot be inferred SeaQL/sea-query#695
- 3: https://www.postgresql.org/docs/current/functions-json.html
- 4: https://docs.rs/sea-query/latest/sea_query/value/enum.Value.html
- 5: https://doc.servo.org/src/sea_query/expr/trait.rs.html
Explicitly cast JSON values to jsonb for reliable PostgreSQL operator resolution.
The @> and <@ operators on PostgreSQL require jsonb-typed operands for correct type inference. When passing parameters via Expr::val(serde_json::Value), PostgreSQL defaults the unbound parameter type to text. While jsonb operators accept text and implicitly cast it, type inference can fail in ambiguous cases. Explicit casting is necessary for reliable operation across driver behaviors and complex JSON structures.
Additionally, is_contained_by_json is missing the #[allow(clippy::wrong_self_convention)] attribute that appears on the other is_* methods in this trait for consistency.
Suggested fix
-use sea_query::{Expr, ExprTrait, IntoColumnRef};
+use sea_query::{Alias, Expr, ExprTrait, IntoColumnRef};
@@
/// Check if target JSON element contains the supplied JSON value (`column @> value`)
fn json_contains<T: Into<serde_json::Value>>(self, value: T) -> Expr {
- Expr::col(self).binary(sea_query::BinOper::Custom("@>"), Expr::val(value.into()))
+ Expr::col(self).binary(
+ sea_query::BinOper::Custom("@>"),
+ Expr::val(value.into()).cast_as(Alias::new("jsonb")),
+ )
}
@@
/// Check if target JSON element is contained by the supplied JSON structure (`column <@ value`)
+ #[allow(clippy::wrong_self_convention)]
fn is_contained_by_json<T: Into<serde_json::Value>>(self, value: T) -> Expr {
- Expr::col(self).binary(sea_query::BinOper::Custom("<@"), Expr::val(value.into()))
+ Expr::col(self).binary(
+ sea_query::BinOper::Custom("<@"),
+ Expr::val(value.into()).cast_as(Alias::new("jsonb")),
+ )
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/query/column/column_trait.rs` around lines 105 - 113, The JSON
containment methods use Expr::val(serde_json::Value) which yields untyped
parameters; change both json_contains and is_contained_by_json to explicitly
cast the RHS to jsonb (e.g., wrap the value expression with a CAST to jsonb or
otherwise force Expr::val(...).cast("jsonb") / an equivalent sea-query
construct) so PostgreSQL uses jsonb for @> and <@ operator resolution, and add
the missing #[allow(clippy::wrong_self_convention)] attribute to
is_contained_by_json to match the other is_* methods for lint consistency;
update references in json_contains, is_contained_by_json, Expr::val and
BinOper::Custom("@>"/"<@") accordingly.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (8)
lifeguard-derive/src/attributes.rs (2)
1150-1183:⚠️ Potential issue | 🟠 MajorReject empty index names, key lists, and
INCLUDE ()at parse time.
#[index = "idx_name"],idx_name(), andINCLUDE ()still produce aParsedIndexSpecinstead of a hard error. That defers invalidCREATE INDEXSQL to a much later phase and makes broken annotations look accepted. Please explicitly reject emptyname, empty key parts, and empty include lists here.Based on learnings, "When adding or changing
#[index]or#[indexed]attributes, verify all columns exist as struct fields and run migration generation in a test database to ensure generatedCREATE INDEXstatements succeed."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-derive/src/attributes.rs` around lines 1150 - 1183, The parser currently accepts empty index names, empty key lists and empty INCLUDE lists; after extracting name (from let name = ...), columns (from classify_index_key_list -> columns) and include_columns (inc / include_columns), add explicit checks that name is not empty, columns is not empty, and if include_idx.is_some() then include_columns is not empty; for each violation return a syn::Error::new(Span::call_site(), "...") with a clear message (e.g. "Invalid index definition: empty index name", "Invalid index definition: empty key list", "Invalid index definition: INCLUDE must list at least one column") so ParsedIndexSpec is only created for valid inputs (refer to symbols include_idx, kp, inc, key_part, name, columns, include_columns, classify_index_key_list, ParsedIndexSpec).
879-894:⚠️ Potential issue | 🟡 MinorQuoted collation parsing still breaks UTF-8 names.
This loop constructs characters with
char::from(bytes[i]), so any non-ASCII quoted collation name is decoded byte-by-byte. The parser should iterate withchar_indices()and slice the remainder using the closing quote's byte offset instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-derive/src/attributes.rs` around lines 879 - 894, The quoted-collation parser is decoding non-ASCII names byte-by-byte via char::from(bytes[i]) which breaks UTF-8; update the loop that checks tail (currently using bytes, i, out) to iterate using tail.char_indices() (or .chars() with tracked byte offsets), accumulate full Rust chars into out, detect doubled quotes by comparing the next byte-offset to determine escaped quotes, and when you find the closing quote compute the remainder slice using the closing quote's byte index (e.g., tail[closing_byte_index+1..].trim_start()) to return Ok((out, rest)); replace the old byte-based indexing logic (bytes, i) accordingly so UTF-8 names are preserved.lifeguard-migrate/src/sql_generator.rs (1)
371-380:⚠️ Potential issue | 🟠 MajorModified indexes still suppress the plain
#[indexed]index.
index_covers_only_column()reduces structured keys to coverage columns, solower(email) | email, a collated/opclass key, or a partial index all look equivalent to plain(email). That can skip emitting the simple index the model asked for.Possible fix
fn index_covers_only_column( index: &lifeguard::IndexDefinition, col_name: &str, ) -> bool { - let cov = if !index.key_parts.is_empty() { - index_key_parts_coverage_columns(&index.key_parts) - } else { - index.columns.clone() - }; - cov.len() == 1 && cov[0] == col_name + if index.partial_where.is_some() { + return false; + } + match index.key_parts.as_slice() { + [lifeguard::IndexKeyPart::Column { + name, + opclass: None, + collate: None, + sort: None, + nulls: None, + }] => name == col_name, + [] => index.columns.len() == 1 && index.columns[0] == col_name, + _ => false, + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-migrate/src/sql_generator.rs` around lines 371 - 380, index_covers_only_column() currently reduces structured key_parts into coverage columns and treats complex keys (collated/opclass expressions or partial indexes) as equivalent to a plain single-column index, causing the plain #[indexed] index to be suppressed; change the logic to only return true when the index is truly the simple single-column index: require index.key_parts to be empty AND index.columns to be exactly [col_name], and also verify there are no collation/opclass/expression/partial properties present (check fields like any collation/opclass lists, expressions, or a where_clause on lifeguard::IndexDefinition) before returning true from index_covers_only_column so collated/opclass/partial or expression-based keys do not hide the explicit plain index.lifeguard-migrate/src/schema_migration_compare.rs (3)
685-687:⚠️ Potential issue | 🟠 MajorPrimary-key index filtering still uses unreliable
_pkeysuffix check.The
_pkeysuffix is not authoritative—a user-named primary-key constraint can produce an index with any name, and those indexes will be incorrectly included and reported as drift. Other functions in this file (fetch_live_btree_index_key_opclasses, etc.) correctly useNOT xi.indisprimary. Apply the same pattern here by replacing thepg_indexesview query with a directpg_indexjoin.Suggested query replacement
- let sql = r" - SELECT tablename::text, indexname::text, indexdef::text - FROM pg_indexes - WHERE schemaname = $1 - ORDER BY tablename, indexname - "; + let sql = r" + SELECT + t.relname::text AS tablename, + ic.relname::text AS indexname, + pg_get_indexdef(ic.oid)::text AS indexdef + FROM pg_index xi + JOIN pg_class ic ON ic.oid = xi.indexrelid + JOIN pg_class t ON t.oid = xi.indrelid + JOIN pg_namespace n ON n.oid = t.relnamespace + WHERE n.nspname = $1 + AND t.relkind IN ('r', 'p') + AND ic.relkind = 'i' + AND xi.indisvalid + AND NOT xi.indisprimary + ORDER BY t.relname, ic.relname + "; ... - if index_name.ends_with("_pkey") { - continue; - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-migrate/src/schema_migration_compare.rs` around lines 685 - 687, The current code filters primary-key indexes by checking index_name.ends_with("_pkey"), which is unreliable; remove that suffix check and change the query that reads pg_indexes to instead join pg_index (alias xi) and use NOT xi.indisprimary to exclude PK indexes—mirror the approach used in fetch_live_btree_index_key_opclasses/fetch_live_btree_index_columns. Update the SQL and any variables referencing index_name accordingly so primary-key indexes are excluded based on xi.indisprimary rather than the name.
605-616:⚠️ Potential issue | 🟠 MajorOpclass comparison still lacks qualifier normalization—causes false drift.
The migration parser preserves qualified names like
pg_catalog.text_pattern_ops, butfetch_live_btree_index_key_opclassesreturns unqualified names liketext_pattern_ops. The direct comparison at line 613 will produce false positives for semantically identical qualified names.Suggested normalizer and usage
+fn normalize_catalog_identifier(name: &str) -> String { + let stripped = name.trim(); + // Strip leading schema qualification (e.g., "pg_catalog.") + let base = stripped.rsplit('.').next().unwrap_or(stripped); + // Remove surrounding double quotes if present + base.trim_matches('"').to_string() +} + MigKeySlot::Simple(t) => { - let expected = t + let expected_raw = t .explicit_opclass .as_deref() .or(o.default_opclass_name.as_deref()); - let Some(exp) = expected else { + let Some(exp_raw) = expected_raw else { continue; }; - if o.opclass_name != exp { + let exp = normalize_catalog_identifier(exp_raw); + let live = normalize_catalog_identifier(&o.opclass_name); + if live != exp { emit_opclass_drift = true; migration_explicit_opclass = t.explicit_opclass.clone(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-migrate/src/schema_migration_compare.rs` around lines 605 - 616, The opclass comparison in the MigKeySlot::Simple branch compares o.opclass_name to exp directly, causing false drift when one side is schema-qualified (e.g. "pg_catalog.text_pattern_ops") and the other is unqualified ("text_pattern_ops"); update the comparison to normalize both names before comparing by stripping any schema qualifier (split on '.' and use the last segment) or otherwise canonicalizing names, use the normalized result for the if check that sets emit_opclass_drift, and still assign migration_explicit_opclass = t.explicit_opclass.clone() (store the original explicit value) so only the comparison uses the normalized forms; reference the symbols MigKeySlot::Simple, t.explicit_opclass, o.default_opclass_name, and the if o.opclass_name != exp comparison to locate the change.
1623-1637:⚠️ Potential issue | 🟠 MajorCollation comparison needs the same qualifier normalization.
Migration collation names may include schema qualification (e.g.,
pg_catalog."C"), but livepg_collation.collnamereturns unqualified names. Apply the same normalization as suggested for opclass.Apply normalizer
if let Some(ref mc) = tail.collation { let live_c = live .collation_name .as_deref() .unwrap_or("(database default)"); - if mc != live_c { + let mc_norm = normalize_catalog_identifier(mc); + let live_c_norm = if live_c == "(database default)" { + live_c.to_string() + } else { + normalize_catalog_identifier(live_c) + }; + if mc_norm != live_c_norm { out.push(IndexBtreeKeyOrderingCollationDrift {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-migrate/src/schema_migration_compare.rs` around lines 1623 - 1637, The collation comparison uses tail.collation (mc) and live.collation_name (live_c) but doesn't normalize schema-qualified migration names the way opclass normalization does, causing false positives; apply the same normalization routine used for opclass to both mc and live_c before comparing in the block that builds IndexBtreeKeyOrderingCollationDrift (i.e., normalize mc and live_c prior to the if mc != live_c check), and ensure the formatted detail and stored values use the normalized forms so schema-qualified names like pg_catalog."C" match unqualified collnames from live.collation_name.src/query/table/definition.rs (2)
73-76:⚠️ Potential issue | 🟠 MajorQuote opclass and column names for SQL safety.
COLLATEvalues are conditionally quoted (lines 65-71), but opclass (line 75) and column names (line 115) are emitted verbatim. Reserved keywords or mixed-case identifiers will produce invalid SQL.Apply consistent quoting
+fn quote_sql_ident_if_needed(s: &str) -> String { + if s.chars().any(|ch| ch.is_whitespace() || ch == '"') || !is_simple_sql_ident(s) { + format!("\"{}\"", s.replace('"', "\"\"")) + } else { + s.to_string() + } +} + fn push_index_key_suffixes( out: &mut String, collate: &Option<String>, opclass: &Option<String>, sort: Option<IndexBtreeSort>, nulls: Option<IndexBtreeNulls>, ) { if let Some(c) = collate { out.push_str(" COLLATE "); - if c.chars().any(|ch| ch.is_whitespace() || ch == '"') || !is_simple_sql_ident(c) { - out.push('"'); - out.push_str(&c.replace('"', "\"\"")); - out.push('"'); - } else { - out.push_str(c); - } + out.push_str("e_sql_ident_if_needed(c)); } if let Some(o) = opclass { out.push(' '); - out.push_str(o); + out.push_str("e_sql_ident_if_needed(o)); }And for column names in
format_index_key_list_sql:IndexKeyPart::Column { name, ... } => { - out.push_str(name); + out.push_str("e_sql_ident_if_needed(name)); push_index_key_suffixes(&mut out, collate, opclass, *sort, *nulls); }Also applies to: 115-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/query/table/definition.rs` around lines 73 - 76, The code emits opclass (variable opclass) and index column names verbatim which can break SQL for reserved or mixed-case identifiers; change the emission to use the same quoting helper used for COLLATE (the existing collate-quoting logic around lines 65-71) so opclass strings are wrapped with the identifier-quoting function and the functions that build key lists (format_index_key_list_sql / the method emitting columns around lines 115-116) also call that quoting helper for each column name; update places that push opclass and column identifiers to call the shared quote/escape function rather than push_str(o) or emitting raw names.
196-196:⚠️ Potential issue | 🟡 MinorUse named format string syntax per coding guidelines.
Line 196 uses positional syntax
format!("{}({inner})", idx.name). As per coding guidelines for**/*.rs, use named format string syntax.- let mut s = format!("{}({inner})", idx.name); + let mut s = format!("{name}({inner})", name = idx.name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/query/table/definition.rs` at line 196, Replace the positional format usage that builds s with a named-placeholder style: in the expression that currently calls format! with "{}({inner})" and idx.name, change it to use named placeholders for the index name and inner (e.g., use placeholder names like "name" and "inner") and pass name=idx.name and inner=inner to format! so the format string obeys the project's named-syntax guideline; update the variable s assignment accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lifeguard-derive/src/attributes.rs`:
- Around line 1007-1018: The current branch treats a malformed COLLATE as valid
because it only acts when parse_quoted_or_ident_collate(tail) is Ok and
otherwise falls through; change the handling so that when
parse_quoted_or_ident_collate returns Err you do NOT accept the COLLATE branch —
either propagate the parse error to the caller or simply skip/continue the
collate branch instead of returning a head/tail tuple. Concretely, replace the
if let Ok((coll, rest)) = parse_quoted_or_ident_collate(tail) { ... return ... }
pattern in the block that checks lower.find(" collate ") with an explicit match
on parse_quoted_or_ident_collate(tail) and handle Err by not returning (or by
returning an Err from the surrounding parsing function), ensuring invalid
COLLATE syntax is rejected immediately; keep references to lower.find, core,
parse_quoted_or_ident_collate, and the returned (head, Some(coll), opclass,
sort, nulls) tuple to locate the code.
- Around line 501-504: The doc comment for the fields is_view and view_query in
attributes.rs triggers Clippy/doc-markdown because terms like PostgreSQL VIEW
and BASE TABLE (and possibly view_query) need to be marked as code; update the
triple-slash doc comment to wrap those technical identifiers/terms in backticks
(e.g., `VIEW`, `BASE TABLE`, and `view_query` where appropriate) so the
documentation linter stops complaining and the crate can build.
- Around line 758-770: The validator for #[require_index_coverage] is currently
using valid_columns and only treating is_primary_key/is_indexed as covered,
which causes relation-only fields and unique-only columns to be miscounted;
update the logic in the coverage check (around the covered HashSet,
parse_column_attributes(field), extract_column_name(field), and valid_columns
computation) to build the required-column set from non-ignored struct fields
(skip fields with relation attributes like has_many/has_one/belongs_to or when
extract_column_name returns None) and mark a column as covered when
col_attrs.is_primary_key || col_attrs.is_indexed || col_attrs.is_unique; also
ensure valid_columns excludes relation-only fields so the diff compares only
real DB columns.
In `@lifeguard-migrate/src/schema_migration_compare.rs`:
- Around line 1454-1467: The NULLS clause checks use incorrect length constants
causing matches to fail; update the comparisons and slice/index offsets for the
"NULLS FIRST" and "NULLS LAST" branches so they use 11 and 10 (respectively)
instead of 12 and 11, and adjust the subsequent byte/index checks and slice
starts from 12->11 and 11->10 accordingly (the block that inspects r, sets
tail.explicit_nulls_first, and returns r[...] should be changed); ensure you
still verify following char is absent or whitespace before setting
explicit_nulls_first.
In `@lifeguard-migrate/src/sql_generator.rs`:
- Around line 69-78: The current branch for view handling in sql_generator.rs
writes a placeholder query ("SELECT 1; -- View Query Missing") when
table_def.is_view is true and view_query is absent; instead, detect missing view
SQL and return an Err so migrations fail fast. In the block that checks
table_def.is_view (and where view_query is obtained via
table_def.view_query.as_deref()), if view_query is None or empty, return Err
with a clear message referencing full_table_name and that the view query
metadata is missing; otherwise, use the actual view_query to write the CREATE OR
REPLACE VIEW statements as before.
In `@src/migration/state_table.rs`:
- Around line 21-24: The create path currently uses
Table::create().if_not_exists() for "lifeguard_migrations", which hides schema
drift between create_state_table() (defines success BOOLEAN NOT NULL DEFAULT
false) and initialize_state_table() (creates same table with DEFAULT true);
either remove .if_not_exists() from the Table::create() call in
create_state_table() so conflicting definitions surface as an error, or make
both create_state_table() and initialize_state_table() agree on the same default
and add an explicit reconciliation step in initialize_state_table() that
inspects the existing "success" column default and issues an ALTER TABLE to set
the expected default if it differs. Ensure you reference/create tests around
create_state_table(), initialize_state_table(), and the "lifeguard_migrations"
table to verify the chosen fix.
---
Duplicate comments:
In `@lifeguard-derive/src/attributes.rs`:
- Around line 1150-1183: The parser currently accepts empty index names, empty
key lists and empty INCLUDE lists; after extracting name (from let name = ...),
columns (from classify_index_key_list -> columns) and include_columns (inc /
include_columns), add explicit checks that name is not empty, columns is not
empty, and if include_idx.is_some() then include_columns is not empty; for each
violation return a syn::Error::new(Span::call_site(), "...") with a clear
message (e.g. "Invalid index definition: empty index name", "Invalid index
definition: empty key list", "Invalid index definition: INCLUDE must list at
least one column") so ParsedIndexSpec is only created for valid inputs (refer to
symbols include_idx, kp, inc, key_part, name, columns, include_columns,
classify_index_key_list, ParsedIndexSpec).
- Around line 879-894: The quoted-collation parser is decoding non-ASCII names
byte-by-byte via char::from(bytes[i]) which breaks UTF-8; update the loop that
checks tail (currently using bytes, i, out) to iterate using tail.char_indices()
(or .chars() with tracked byte offsets), accumulate full Rust chars into out,
detect doubled quotes by comparing the next byte-offset to determine escaped
quotes, and when you find the closing quote compute the remainder slice using
the closing quote's byte index (e.g., tail[closing_byte_index+1..].trim_start())
to return Ok((out, rest)); replace the old byte-based indexing logic (bytes, i)
accordingly so UTF-8 names are preserved.
In `@lifeguard-migrate/src/schema_migration_compare.rs`:
- Around line 685-687: The current code filters primary-key indexes by checking
index_name.ends_with("_pkey"), which is unreliable; remove that suffix check and
change the query that reads pg_indexes to instead join pg_index (alias xi) and
use NOT xi.indisprimary to exclude PK indexes—mirror the approach used in
fetch_live_btree_index_key_opclasses/fetch_live_btree_index_columns. Update the
SQL and any variables referencing index_name accordingly so primary-key indexes
are excluded based on xi.indisprimary rather than the name.
- Around line 605-616: The opclass comparison in the MigKeySlot::Simple branch
compares o.opclass_name to exp directly, causing false drift when one side is
schema-qualified (e.g. "pg_catalog.text_pattern_ops") and the other is
unqualified ("text_pattern_ops"); update the comparison to normalize both names
before comparing by stripping any schema qualifier (split on '.' and use the
last segment) or otherwise canonicalizing names, use the normalized result for
the if check that sets emit_opclass_drift, and still assign
migration_explicit_opclass = t.explicit_opclass.clone() (store the original
explicit value) so only the comparison uses the normalized forms; reference the
symbols MigKeySlot::Simple, t.explicit_opclass, o.default_opclass_name, and the
if o.opclass_name != exp comparison to locate the change.
- Around line 1623-1637: The collation comparison uses tail.collation (mc) and
live.collation_name (live_c) but doesn't normalize schema-qualified migration
names the way opclass normalization does, causing false positives; apply the
same normalization routine used for opclass to both mc and live_c before
comparing in the block that builds IndexBtreeKeyOrderingCollationDrift (i.e.,
normalize mc and live_c prior to the if mc != live_c check), and ensure the
formatted detail and stored values use the normalized forms so schema-qualified
names like pg_catalog."C" match unqualified collnames from live.collation_name.
In `@lifeguard-migrate/src/sql_generator.rs`:
- Around line 371-380: index_covers_only_column() currently reduces structured
key_parts into coverage columns and treats complex keys (collated/opclass
expressions or partial indexes) as equivalent to a plain single-column index,
causing the plain #[indexed] index to be suppressed; change the logic to only
return true when the index is truly the simple single-column index: require
index.key_parts to be empty AND index.columns to be exactly [col_name], and also
verify there are no collation/opclass/expression/partial properties present
(check fields like any collation/opclass lists, expressions, or a where_clause
on lifeguard::IndexDefinition) before returning true from
index_covers_only_column so collated/opclass/partial or expression-based keys do
not hide the explicit plain index.
In `@src/query/table/definition.rs`:
- Around line 73-76: The code emits opclass (variable opclass) and index column
names verbatim which can break SQL for reserved or mixed-case identifiers;
change the emission to use the same quoting helper used for COLLATE (the
existing collate-quoting logic around lines 65-71) so opclass strings are
wrapped with the identifier-quoting function and the functions that build key
lists (format_index_key_list_sql / the method emitting columns around lines
115-116) also call that quoting helper for each column name; update places that
push opclass and column identifiers to call the shared quote/escape function
rather than push_str(o) or emitting raw names.
- Line 196: Replace the positional format usage that builds s with a
named-placeholder style: in the expression that currently calls format! with
"{}({inner})" and idx.name, change it to use named placeholders for the index
name and inner (e.g., use placeholder names like "name" and "inner") and pass
name=idx.name and inner=inner to format! so the format string obeys the
project's named-syntax guideline; update the variable s assignment accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 385a7de1-27e0-47b3-b85f-7b7625e48ae4
📒 Files selected for processing (10)
lifeguard-derive/src/attributes.rslifeguard-derive/src/lib.rslifeguard-derive/src/macros/life_model.rslifeguard-migrate/src/generated_migration_diff.rslifeguard-migrate/src/schema_migration_compare.rslifeguard-migrate/src/sql_generator.rssrc/migration/schema_manager.rssrc/migration/state_table.rssrc/query/select.rssrc/query/table/definition.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/query/select.rs
- lifeguard-migrate/src/generated_migration_diff.rs
Comment on lines
+501
to
+504
| /// Whether the entity represents a PostgreSQL VIEW instead of a BASE TABLE | ||
| pub is_view: bool, | ||
| /// The select query backing the view (used for generation) | ||
| pub view_query: Option<String>, |
There was a problem hiding this comment.
Fix the Clippy/doc-markdown failure in this comment.
CI is already failing on this line, so this blocks the crate until the terms are backticked.
Minimal fix
- /// Whether the entity represents a PostgreSQL VIEW instead of a BASE TABLE
+ /// Whether the entity represents a `PostgreSQL` `VIEW` instead of a `BASE TABLE`🧰 Tools
🪛 GitHub Actions: Lifeguard CI
[error] 501-501: Clippy failed (doc_markdown / clippy::doc-markdown): documentation item in comment is missing backticks. Error at line 501: "Whether the entity represents a PostgreSQL VIEW instead of a BASE TABLE"; suggested "... represents a PostgreSQL VIEW ...". (The error is treated as fatal due to -D warnings.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lifeguard-derive/src/attributes.rs` around lines 501 - 504, The doc comment
for the fields is_view and view_query in attributes.rs triggers
Clippy/doc-markdown because terms like PostgreSQL VIEW and BASE TABLE (and
possibly view_query) need to be marked as code; update the triple-slash doc
comment to wrap those technical identifiers/terms in backticks (e.g., `VIEW`,
`BASE TABLE`, and `view_query` where appropriate) so the documentation linter
stops complaining and the crate can build.
Comment on lines
+758
to
+770
| let mut covered: HashSet<String> = HashSet::new(); | ||
| for field in fields { | ||
| let Some(field_name) = &field.ident else { | ||
| continue; | ||
| }; | ||
| if has_attribute(field, "skip") || has_attribute(field, "ignore") { | ||
| continue; | ||
| } | ||
| let col = extract_column_name(field) | ||
| .unwrap_or_else(|| utils::snake_case(&field_name.to_string())); | ||
| let col_attrs = parse_column_attributes(field)?; | ||
| if col_attrs.is_primary_key || col_attrs.is_indexed { | ||
| covered.insert(col); |
There was a problem hiding this comment.
#[require_index_coverage] is counting non-columns and missing #[unique].
This validator diffs against valid_columns, but that set can still include relation-only fields, and the field-level coverage check only accepts #[primary_key] / #[indexed]. Models with #[has_many] / #[belongs_to] / #[has_one] fields or unique-only columns will fail compilation even though no real database column is missing coverage. Please derive the required-column set from non-ignored fields and treat is_unique as coverage too.
As per coding guidelines, "The #[indexed] attribute on a field must only be used on real fields that exist on that struct."
Also applies to: 786-794
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lifeguard-derive/src/attributes.rs` around lines 758 - 770, The validator for
#[require_index_coverage] is currently using valid_columns and only treating
is_primary_key/is_indexed as covered, which causes relation-only fields and
unique-only columns to be miscounted; update the logic in the coverage check
(around the covered HashSet, parse_column_attributes(field),
extract_column_name(field), and valid_columns computation) to build the
required-column set from non-ignored struct fields (skip fields with relation
attributes like has_many/has_one/belongs_to or when extract_column_name returns
None) and mark a column as covered when col_attrs.is_primary_key ||
col_attrs.is_indexed || col_attrs.is_unique; also ensure valid_columns excludes
relation-only fields so the diff compares only real DB columns.
Comment on lines
+1007
to
+1018
| if let Some(pos) = lower.find(" collate ") { | ||
| let head = core[..pos].trim().to_string(); | ||
| let tail = core[pos + " COLLATE ".len()..].trim_start(); | ||
| if let Ok((coll, rest)) = parse_quoted_or_ident_collate(tail) { | ||
| let rest = rest.trim(); | ||
| let opclass = if rest.is_empty() { | ||
| None | ||
| } else { | ||
| Some(rest.to_string()) | ||
| }; | ||
| return (head, Some(coll), opclass, sort, nulls); | ||
| } |
There was a problem hiding this comment.
Invalid COLLATE syntax is being silently accepted.
When parse_quoted_or_ident_collate returns Err, this branch falls back to treating the whole tail as raw SQL. A malformed expression key therefore survives parsing and only fails much later when SQL is emitted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lifeguard-derive/src/attributes.rs` around lines 1007 - 1018, The current
branch treats a malformed COLLATE as valid because it only acts when
parse_quoted_or_ident_collate(tail) is Ok and otherwise falls through; change
the handling so that when parse_quoted_or_ident_collate returns Err you do NOT
accept the COLLATE branch — either propagate the parse error to the caller or
simply skip/continue the collate branch instead of returning a head/tail tuple.
Concretely, replace the if let Ok((coll, rest)) =
parse_quoted_or_ident_collate(tail) { ... return ... } pattern in the block that
checks lower.find(" collate ") with an explicit match on
parse_quoted_or_ident_collate(tail) and handle Err by not returning (or by
returning an Err from the surrounding parsing function), ensuring invalid
COLLATE syntax is rejected immediately; keep references to lower.find, core,
parse_quoted_or_ident_collate, and the returned (head, Some(coll), opclass,
sort, nulls) tuple to locate the code.
Comment on lines
+1454
to
+1467
| if r.len() >= 12 && r[..12].eq_ignore_ascii_case("nulls first") { | ||
| let ok = r.len() == 12 || r.as_bytes().get(12).is_some_and(|b| b.is_ascii_whitespace()); | ||
| if ok { | ||
| tail.explicit_nulls_first = Some(true); | ||
| return Some(r[12..].trim_start()); | ||
| } | ||
| } | ||
| if r.len() >= 11 && r[..11].eq_ignore_ascii_case("nulls last") { | ||
| let ok = r.len() == 11 || r.as_bytes().get(11).is_some_and(|b| b.is_ascii_whitespace()); | ||
| if ok { | ||
| tail.explicit_nulls_first = Some(false); | ||
| return Some(r[11..].trim_start()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Off-by-one error in NULLS clause length checks.
"NULLS FIRST" is 11 characters and "NULLS LAST" is 10 characters, but the code checks for lengths 12 and 11 respectively, causing these clauses to never match.
Fix length constants
fn strip_nulls_clause<'a>(rest: &'a str, tail: &mut ParsedSimpleKeyTail) -> Option<&'a str> {
let r = rest.trim_start();
- if r.len() >= 12 && r[..12].eq_ignore_ascii_case("nulls first") {
- let ok = r.len() == 12 || r.as_bytes().get(12).is_some_and(|b| b.is_ascii_whitespace());
+ if r.len() >= 11 && r[..11].eq_ignore_ascii_case("nulls first") {
+ let ok = r.len() == 11 || r.as_bytes().get(11).is_some_and(|b| b.is_ascii_whitespace());
if ok {
tail.explicit_nulls_first = Some(true);
- return Some(r[12..].trim_start());
+ return Some(r[11..].trim_start());
}
}
- if r.len() >= 11 && r[..11].eq_ignore_ascii_case("nulls last") {
- let ok = r.len() == 11 || r.as_bytes().get(11).is_some_and(|b| b.is_ascii_whitespace());
+ if r.len() >= 10 && r[..10].eq_ignore_ascii_case("nulls last") {
+ let ok = r.len() == 10 || r.as_bytes().get(10).is_some_and(|b| b.is_ascii_whitespace());
if ok {
tail.explicit_nulls_first = Some(false);
- return Some(r[11..].trim_start());
+ return Some(r[10..].trim_start());
}
}
Some(r)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lifeguard-migrate/src/schema_migration_compare.rs` around lines 1454 - 1467,
The NULLS clause checks use incorrect length constants causing matches to fail;
update the comparisons and slice/index offsets for the "NULLS FIRST" and "NULLS
LAST" branches so they use 11 and 10 (respectively) instead of 12 and 11, and
adjust the subsequent byte/index checks and slice starts from 12->11 and 11->10
accordingly (the block that inspects r, sets tail.explicit_nulls_first, and
returns r[...] should be changed); ensure you still verify following char is
absent or whitespace before setting explicit_nulls_first.
Comment on lines
+69
to
+78
| // Early exit for Views | ||
| if table_def.is_view { | ||
| let view_query = table_def.view_query.as_deref().unwrap_or("SELECT 1; -- View Query Missing"); | ||
| writeln!(sql, "CREATE OR REPLACE VIEW {} AS", full_table_name) | ||
| .map_err(|e| format!("Failed to write SQL: {}", e))?; | ||
| writeln!(sql, "{};", view_query) | ||
| .map_err(|e| format!("Failed to write SQL: {}", e))?; | ||
|
|
||
| // Return without columns, checks, index logic etc., since Views don't map to tables | ||
| return Ok(sql); |
There was a problem hiding this comment.
Don't generate a placeholder view definition.
Falling back to SELECT 1; -- View Query Missing turns missing metadata into a successful migration that creates the wrong view. This should return Err here so bad view definitions fail fast instead of mutating schema.
Possible fix
- let view_query = table_def.view_query.as_deref().unwrap_or("SELECT 1; -- View Query Missing");
+ let view_query = table_def.view_query.as_deref().ok_or_else(|| {
+ format!("view `{full_table_name}` is missing `view_query`")
+ })?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Early exit for Views | |
| if table_def.is_view { | |
| let view_query = table_def.view_query.as_deref().unwrap_or("SELECT 1; -- View Query Missing"); | |
| writeln!(sql, "CREATE OR REPLACE VIEW {} AS", full_table_name) | |
| .map_err(|e| format!("Failed to write SQL: {}", e))?; | |
| writeln!(sql, "{};", view_query) | |
| .map_err(|e| format!("Failed to write SQL: {}", e))?; | |
| // Return without columns, checks, index logic etc., since Views don't map to tables | |
| return Ok(sql); | |
| // Early exit for Views | |
| if table_def.is_view { | |
| let view_query = table_def.view_query.as_deref().ok_or_else(|| { | |
| format!("view `{full_table_name}` is missing `view_query`") | |
| })?; | |
| writeln!(sql, "CREATE OR REPLACE VIEW {} AS", full_table_name) | |
| .map_err(|e| format!("Failed to write SQL: {}", e))?; | |
| writeln!(sql, "{};", view_query) | |
| .map_err(|e| format!("Failed to write SQL: {}", e))?; | |
| // Return without columns, checks, index logic etc., since Views don't map to tables | |
| return Ok(sql); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lifeguard-migrate/src/sql_generator.rs` around lines 69 - 78, The current
branch for view handling in sql_generator.rs writes a placeholder query ("SELECT
1; -- View Query Missing") when table_def.is_view is true and view_query is
absent; instead, detect missing view SQL and return an Err so migrations fail
fast. In the block that checks table_def.is_view (and where view_query is
obtained via table_def.view_query.as_deref()), if view_query is None or empty,
return Err with a clear message referencing full_table_name and that the view
query metadata is missing; otherwise, use the actual view_query to write the
CREATE OR REPLACE VIEW statements as before.
Comment on lines
21
to
24
| Table::create() | ||
| .table("lifeguard_migrations") | ||
| .if_not_exists() | ||
| .col( |
There was a problem hiding this comment.
IF NOT EXISTS now hides the conflicting state-table definitions.
create_state_table() still encodes success BOOLEAN NOT NULL DEFAULT false, while initialize_state_table() creates the same table with DEFAULT true at Lines 77-85. After this change, whichever bootstrap path runs first silently wins and the other path reports success without reconciling the drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/migration/state_table.rs` around lines 21 - 24, The create path currently
uses Table::create().if_not_exists() for "lifeguard_migrations", which hides
schema drift between create_state_table() (defines success BOOLEAN NOT NULL
DEFAULT false) and initialize_state_table() (creates same table with DEFAULT
true); either remove .if_not_exists() from the Table::create() call in
create_state_table() so conflicting definitions surface as an error, or make
both create_state_table() and initialize_state_table() agree on the same default
and add an explicit reconciliation step in initialize_state_table() that
inspects the existing "success" column default and issues an ALTER TABLE to set
the expected default if it differs. Ensure you reference/create tests around
create_state_table(), initialize_state_table(), and the "lifeguard_migrations"
table to verify the chosen fix.
…ric type injection support
- Extend type_conversion for Option<NaiveDate> and Value round-trips - Add is_naive_date_type helper and unit test - Document incident in docs/postmortem-lifeguard-derive-naivedate-chronodate-2026-04.md - Link postmortem from AGENT.md
- Tilt/just: nextest coverage notes, replica URL helper, perf targets; examples use lifeguard schema - lifeguard-migrate: fix btree catalog SQL (indoption parens, unnest/key_ord); update compare-schema smoke - CI/compose/test infra: schema init, pool/replica/session integration tweaks - Derive/query/pool: chrono-related paths, validators, scopes (branch scope) - Docs: TEST_INFRASTRUCTURE, PERF_ORM, COMPLETE_CHRONO_* task/implementation guides
…Local Extend type_conversion for CHRONO Iteration A: detect DateTime<Utc> and DateTime<Local>, wire field/option Value emission and Value→field setters (including non-option uuid/naive paths on generate_value_to_field). Add uuid and typed chrono arms to generate_value_to_option_field. Add chrono + with-chrono dev-deps, unit tests for detectors, and integration tests for Model::get Value variants.
Use chrono FromSql for timestamptz rows instead of falling through generic try_get. Infer TIMESTAMPTZ when the Rust field is DateTime<Utc> or DateTime<Local> (type_conversion detectors).
…to_value Keeps pre-commit clippy (-D warnings) aligned with sibling conversion helpers.
- Add db_integration/chrono_timestamptz_from_row: FromRow SELECT, Record insert round-trip, nullable Option<DateTime<Utc>> (distinct tables per test for parallel safety). - Document FromRow strategy (direct try_get vs NaiveDateTime SystemTime bridge) in COMPLETE_CHRONO_IMPLEMENTATION.md. - Mark iterations A/B checklist rows and T1/T2; C1 audited via comment on insert path using type_conversion. - Note in life_record.rs that insert values follow the same get()/Value path.
- Add type_conversion::unwrap_option_inner_type and generate_expr_val_now_for_field_type for hook SQL Expr::val. - LifeRecord soft-delete UPDATE uses field types for deleted_at/updated_at (DateTime<Utc> → ChronoDateTimeUtc; NaiveDateTime → naive ChronoDateTime). - DB test: soft_delete_datetime_utc with TIMESTAMPTZ deleted_at. - Docs: PRD C2/C3/C4/T3 ticks; note legacy #[auto_timestamp] string hooks.
- converted_params: two-pass + typed NULL module docs; tests for mixed String/Json/ChronoDateTimeUtc NULLs and UUID/Int/ChronoDateTime NULLs. - Add docs/CHRONO_AND_POSTGRES_TYPES.md; link from UUID doc and PRD. - CHANGELOG [Unreleased] Chrono additive note; lifeguard-migrate README infer-schema vs Lifeguard version. - Mark PRD tasks D1–D3, E1–E5; §9 success criteria checkboxes.
- Document Chrono alignment on map_pg_to_rust (derive + CHRONO_AND_POSTGRES_TYPES) - Unit tests: timestamptz variants, USER-DEFINED + udt_name path, naive timestamp - Mark M1/M2 complete in COMPLETE_CHRONO_TASKS
- db_integration: NaiveDate DATE, DateTime<Local> TIMESTAMPTZ, mixed UUID+JSONB+Utc+i16 - lifeguard-migrate: map_pg_to_rust unit test for date → NaiveDate - converted_params: bind SmallInt/TinyInt/small unsigned as i16 (INT2); Json as serde_json::Value - OwnedParam: SmallInt(Option<i16>) mirroring INT2 path for pool dispatch - CHANGELOG + COMPLETE_CHRONO_TASKS (T4–T6 complete)
- converted_params: String(None)/Bytes(None) use Option<String>/Option<Vec<u8>> buckets (not generic INT null) - OwnedParam: String(None)/Bytes(None) map to typed variants; Json is Option<serde_json::Value> - Tests: postgres_types TEXT/BYTEA/JSONB null acceptance; pool mirror test - Docs + CHANGELOG; COMPLETE_CHRONO_TASKS D4 complete
Perf local_resources already used auto_init=False; add TRIGGER_MODE_MANUAL so build-lifeguard updates and file deps do not auto-trigger idam-perf runs. Document in TEST_INFRASTRUCTURE.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 22364281 | Triggered | Generic Password | 3524dad | scripts/get_replica_connection_string.sh | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
sea-query 1.0.0-rc.33+ can store Rc<str> in Value, so Values is not Send. StreamCursorValues uses unsafe Send for single-owner move into one coroutine.
Inner non-move closure held &StreamCursorValues; only Send was implemented, so &T was not Send. Move cursor body to run_stream_cursor_body().
…ilt manual trigger - nt-db-suite: add --config-file .config/nextest.toml (parity with Tilt/CI intent) - alias db-integration-suite := nt-db-suite; doc one-liner in header - test-db-suite: TRIGGER_MODE_MANUAL so deps do not auto-run the suite
…-trip CI compared nanosecond chrono instants to microsecond-precision values from Postgres.
This was referenced May 4, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Medium Risk
Touches core type conversion and SQL parameter binding paths plus pool parameter serialization, which can cause runtime DB errors if any
Valuevariant mapping is off. Also changes CI/DB schema wiring (search_path/init scripts) and observability deps, which may introduce environment-specific breakage.Overview
Adds end-to-end Chrono date/time support (including
DateTime<Utc>/Local) across derive codegen,FromRow, SQL type inference, and runtime parameter binding, with new docs/PRD/task trackers to lock the canonical Postgres↔Rust mappings.Fixes several Postgres bind correctness issues by preserving
INT2forSmallInt/TinyInt, bindingserde_json::Valueas JSON instead of strings, and ensuring typed SQL NULL handling forString/Bytes/Json(plus pool-sideOwnedParam::Json(Option<…>)).Updates CI/dev infrastructure to run against a dedicated
lifeguardschema viasearch_path(Compose init SQL + workflow env/PGOPTIONS), bumps Prometheus/OpenTelemetry dependencies to a consistent 0.29-era stack, and refactors theTiltfileto be cargo-only with manual DB-heavy targets; also adds afind_related_scope_exampleand expands docs/roadmaps around schema comparison and scopes.Reviewed by Cursor Bugbot for commit 226ffa0. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit