From bf46aa1c3968da5483a04d73188d33e1e48dfeed Mon Sep 17 00:00:00 2001 From: Bunsy <268838774+bunnykong@users.noreply.github.com> Date: Sun, 4 Oct 2026 09:13:58 +0800 Subject: [PATCH 1/4] Preserve nullable equality plans with runtime SQL shapes Select col = ? plus a reserved running bind position for non-nil values, or col IS NULL with no bind slot. The same branch selects both fragment and position, and captures each RHS once; subsequent binds consume the reserved positions. Inline reads use = value / IS NULL. Ruby-only value normalization leaves strict targets on their original lowering. SQLite excludes IS from its non-null partial-index implication rule and cannot perform the same LEFT JOIN reduction. Avoid IS ? even though its row results can match. Generate one branch per nullable predicate, not 2^n methods. Bound queries with more than seven nullable predicates use transient preparation, limiting cached shapes to 128 per query. Provide the shared uncached primitive as this shape budget's prerequisite. Tests: param_binds_planner executes emitted reads in both bind modes, checks single/composite partial indexes and LEFT JOIN plans, two/four shapes and all 256 eight-predicate null masks with no bound cache growth. param_binds covers all eight int/text/bool masks with trailing fixed binds, nil versus real zero/empty/false values, association/key guards, and CRuby plus compiled Spinel execution. Existing writes stay inline. Co-Authored-By: Claude Opus 5.5 --- docs/pipeline/runtime.md | 14 ++ runtime/ruby/active_record/connection.rb | 14 ++ runtime/ruby/active_record/connection.rbs | 3 + runtime/ruby/db.rbs | 7 +- runtime/spinel/db.rb | 76 ++++++- runtime/spinel/db_cruby.rb | 37 +++- runtime/spinel/db_jruby.rb | 59 ++++++ scripts/ci-plan.py | 2 + src/emit/ruby.rs | 4 +- src/emit/ruby/library.rs | 29 ++- src/lower/arel/ir.rs | 8 +- src/lower/arel/mod.rs | 33 ++- src/lower/arel/ruby_values.rs | 50 +++++ src/lower/arel/visitor.rs | 162 +++++++++++---- src/lower/controller_to_library/mod.rs | 7 +- src/lower/model_to_library/mod.rs | 18 +- tests/ci_plan_test.py | 2 + tests/db_shim_conformance.rs | 11 + tests/param_binds.rs | 241 +++++++++++++++++++++- tests/param_binds_associations.rb | 35 ++++ tests/param_binds_emit.rb | 67 +++++- tests/param_binds_nil.rb | 20 ++ tests/param_binds_planner.rb | 124 +++++++++++ tests/param_binds_planner.rs | 117 +++++++++++ 24 files changed, 1064 insertions(+), 76 deletions(-) create mode 100644 src/lower/arel/ruby_values.rs create mode 100644 tests/param_binds_associations.rb create mode 100644 tests/param_binds_nil.rb create mode 100644 tests/param_binds_planner.rb create mode 100644 tests/param_binds_planner.rs diff --git a/docs/pipeline/runtime.md b/docs/pipeline/runtime.md index b62221ffc..3007e555f 100644 --- a/docs/pipeline/runtime.md +++ b/docs/pipeline/runtime.md @@ -170,6 +170,20 @@ it. Snapshot tests + toolchain tests catch drift. ## Emitter ↔ runtime contract +Ruby-family lowered equality reads select SQL from the runtime value: a +non-nil value uses `col = ?` and a bind; nil uses `col IS NULL` without a slot. +The same branch selects the fragment and reserves its running bind position, +so later predicates cannot shift out of alignment. Inline emission uses +`col = ` or `col IS NULL`. `IS ?` is deliberately avoided: +SQLite excludes `IS` from its [partial-index non-null implication rule](https://www.sqlite.org/partialindex.html#queries_using_partial_indexes). + +Each nullable predicate contributes two possible fragments. Code size stays +linear: no query variants are enumerated in the compiler. A bound query with +up to seven nullable predicates has at most 128 shapes; queries above that +budget use `prepare_uncached`, avoiding exponential growth within a long +lease. Existing per-connection cache limits still apply across query sites. +Strict-target lowering retains its existing predicates and lifecycle. + For each target: - **Emitter assumes** specific function names, signatures, and diff --git a/runtime/ruby/active_record/connection.rb b/runtime/ruby/active_record/connection.rb index ddfac5cfe..5a15dbf3d 100644 --- a/runtime/ruby/active_record/connection.rb +++ b/runtime/ruby/active_record/connection.rb @@ -148,6 +148,20 @@ def _await_preload nil end + # Ruby accepts nil at these public entry points. Reject it before a + # key-typed adapter can coerce it to a real zero/empty-string key. + def self.find(id) + raise RecordNotFound, "Couldn't find #{name} without an ID" if id.nil? + result = _adapter_find_by_id(id) + raise RecordNotFound, "Couldn't find #{name} with id=#{id}" if result.nil? + result + end + + def self.exists?(id) + return false if id.nil? + _adapter_exists_by_id?(id) + end + # Stateless facade — every member delegates straight to `Db`, so a # fresh instance per call is cheap and dodges class-ivar state. def self.connection diff --git a/runtime/ruby/active_record/connection.rbs b/runtime/ruby/active_record/connection.rbs index f60841eff..6dab91dd2 100644 --- a/runtime/ruby/active_record/connection.rbs +++ b/runtime/ruby/active_record/connection.rbs @@ -41,6 +41,9 @@ module ActiveRecord def self.record_timestamps: () -> bool def update_column: (String | Symbol name, untyped value) -> bool def _as_json_only: (Array[Symbol] only) -> Hash[String, untyped] + # Ruby-family key guards also accept nil at public entry points. + def self.find: (Integer | String | nil id) -> Base + def self.exists?: (Integer | String | nil id) -> bool # Ruby-family overrides of base.rb's Array / COUNT fallbacks. def self.where: (Hash[Symbol, untyped] conditions) -> Relation def self.all: () -> Relation diff --git a/runtime/ruby/db.rbs b/runtime/ruby/db.rbs index 4c06788fe..6cba3c5ae 100644 --- a/runtime/ruby/db.rbs +++ b/runtime/ruby/db.rbs @@ -14,6 +14,8 @@ module Db def self.close: () -> void def self.exec: (String) -> void def self.prepare: (String) -> Integer + # Reads with too many SQL shapes bypass the cache; finalize releases them. + def self.prepare_uncached: (String) -> Integer def self.step?: (Integer) -> bool def self.column_int: (Integer, Integer) -> Integer def self.column_float: (Integer, Integer) -> Float @@ -32,9 +34,12 @@ module Db # gate emits parameterized queries. Keys the prepared-statement cache # on the static query shape instead of per-value. Every Db shim # (cruby-gem + spinel-FFI) implements these. - def self.bind_int: (Integer, Integer, Integer) -> void + def self.bind_int: (Integer, Integer, Integer?) -> void def self.bind_text: (Integer, Integer, String) -> void def self.bind_bool: (Integer, Integer, bool) -> void + def self.bind_int_opt: (Integer, Integer, Integer?) -> void + def self.bind_text_opt: (Integer, Integer, String?) -> void + def self.bind_bool_opt: (Integer, Integer, bool?) -> void def self.last_insert_rowid: () -> Integer def self.changes: () -> Integer # Query-log capture (issue #27) — records the SQL each prepare/exec diff --git a/runtime/spinel/db.rb b/runtime/spinel/db.rb index ef0c2af67..19550a8f0 100644 --- a/runtime/spinel/db.rb +++ b/runtime/spinel/db.rb @@ -108,6 +108,7 @@ module SQL # coercion for the -1 destructor is the spinel primitive validated in # spinel's test/ffi_ptr_int_literal.rb. ffi_func :sqlite3_bind_int64, [:ptr, :int, :long], :int + ffi_func :sqlite3_bind_null, [:ptr, :int], :int ffi_func :sqlite3_bind_text, [:ptr, :int, :str, :int, :ptr], :int # `sqlite3_column_type` reports the storage class of a column in the # current row; 5 is SQLITE_NULL. The nullable-column reads below need @@ -341,13 +342,18 @@ def cell_text(row, i) # before the first step), and the real stmt it promoted to when the # recorded prefix ran out (0 = still replaying). class QcCursor - def initialize(entry) + def initialize(entry, preparable) @entry = entry + @preparable = preparable @pos = -1 @promoted = false @real_ptr = nil end + def preparable + @preparable + end + def promoted @promoted end @@ -498,6 +504,16 @@ def prepare_cached(sql) end i -= 1 end + prepare_owned(sql, cached) + end + + # Reads with many SQL shapes share checkout and lease cleanup with + # busy-hit transient statements. + def prepare_uncached(sql) + prepare_owned(sql, false) + end + + def prepare_owned(sql, cached) # `SQL.stmt_out` is ONE 8-byte out-buffer for the whole process (an # `ffi_buffer`, static C storage). Under parallel OS workers two # connections preparing at the same moment wrote their statement @@ -652,11 +668,11 @@ def qc_clear # The handle for `sql`: a replay handle on a hit, else the real stmt # (recording its rows as they are stepped, when the cache is on). # Returns 0 as "no replay" so `Db.prepare` can tell the two apart. - def qc_lookup(sql) + def qc_lookup(sql, preparable = true) return 0 if !@qc_on || sql.include?("?") e = @qc_by_sql[sql] return 0 if e.nil? - @qc_cursors.push(QcCursor.new(e)) + @qc_cursors.push(QcCursor.new(e, preparable)) @qc_cursors.length end @@ -766,7 +782,7 @@ def qc_step?(handle) return false if e.eof # The first consumer stopped before the end and this one wants more: # re-run the real statement and fast-forward past what was replayed. - ptr = prepare_cached(e.qc_sql) + ptr = c.preparable ? prepare_cached(e.qc_sql) : prepare_uncached(e.qc_sql) c.promote_to(ptr) n = 0 while n < e.nrows @@ -1531,6 +1547,27 @@ def self.prepare(sql) ptr end + # Bypass statement reuse for nullable shape overflow, retaining the + # separate request result cache. A partial replay must promote uncached. + # Finalize releases the transient immediately; abandoned handles close + # at lease release. Outside with_connection they remain owned until + # Db.close, so scripts must finalize explicitly after use. + def self.prepare_uncached(sql) + # Keep shared cache keys string-typed even when the compiled program + # has no transient call site from which Spinel can infer this argument. + sql = sql.to_s + conn = current_conn + handle = conn.qc_lookup(sql, false) + if handle != 0 + $stderr.puts " CACHE " + sql if @sql_trace + return handle + end + record_query(sql) + ptr = conn.prepare_uncached(sql) + conn.qc_record(sql, ptr) + ptr + end + # Query-log capture — see db_cruby.rb for the full rationale (the # test-side analog of Rails' `sql.active_record` SQLCounter; the one # instrument that can see the includes(:assoc) N+1 `compare` is blind @@ -1756,7 +1793,7 @@ def self.column_name(stmt, i) # roundhouse#12 Path A.1: with caching on, "finalize" means rewind the # cached stmt (reset cursor + clear any bound params) so the next call - # reuses it. Busy-hit transients are really finalized on release. + # reuses it. Busy-hit and explicit uncached reads are finalized on release. def self.finalize(stmt) conn = current_conn if stmt.is_a?(Integer) @@ -1777,8 +1814,35 @@ def self.finalize(stmt) # reset + clear_bindings'd at its previous `finalize`, so re-binding # here starts clean. def self.bind_int(stmt, idx, value) + # A nullable caller must not lose nil at the typed FFI integer boundary. + bind_int_opt(stmt, idx, value) + end + + def self.bind_int_opt(stmt, idx, value) + raise "Db.bind failed (21): cannot bind a replay cursor" if stmt.is_a?(Integer) + if value.nil? + current_conn.bind_checked(stmt, idx, SQL.sqlite3_bind_null(stmt, idx)) + else + current_conn.bind_checked(stmt, idx, SQL.sqlite3_bind_int64(stmt, idx, value)) + end + end + + def self.bind_text_opt(stmt, idx, value) raise "Db.bind failed (21): cannot bind a replay cursor" if stmt.is_a?(Integer) - current_conn.bind_checked(stmt, idx, SQL.sqlite3_bind_int64(stmt, idx, value)) + if value.nil? + current_conn.bind_checked(stmt, idx, SQL.sqlite3_bind_null(stmt, idx)) + else + bind_text(stmt, idx, value) + end + end + + def self.bind_bool_opt(stmt, idx, value) + raise "Db.bind failed (21): cannot bind a replay cursor" if stmt.is_a?(Integer) + if value.nil? + current_conn.bind_checked(stmt, idx, SQL.sqlite3_bind_null(stmt, idx)) + else + current_conn.bind_checked(stmt, idx, SQL.sqlite3_bind_int64(stmt, idx, value ? 1 : 0)) + end end def self.bind_text(stmt, idx, value) diff --git a/runtime/spinel/db_cruby.rb b/runtime/spinel/db_cruby.rb index 5e82206f1..2fe37808a 100644 --- a/runtime/spinel/db_cruby.rb +++ b/runtime/spinel/db_cruby.rb @@ -738,10 +738,31 @@ def self.prepare(sql) end handle = { stmt: stmt, row: nil, cached: cached, capture: nil, open: open } open[stmt] = handle + handle[:capture] = { rows: [], names: stmt.columns, eof: false, sql: sql } if !qcache.nil? && !parameterized + handle + end + + # Explicit uncached reads skip statement reuse, but the separate + # request result cache still applies. Partial replays already promote + # to a transient statement, which finalize closes. + def self.prepare_uncached(sql) qcache = Fiber[:rh_qcache] - unless qcache.nil? || parameterized - handle[:capture] = { rows: [], names: stmt.columns, eof: false, sql: sql } + parameterized = sql.include?("?") + if !qcache.nil? && !parameterized && (hit = qcache[sql]) + return { stmt: nil, row: nil, cached: false, replay: hit, pos: 0, sql: sql } end + record_query(sql) + conn = current_dbh + stmt = conn.prepare(sql) + statement_handle(open_statements(conn), stmt, sql, false, !qcache.nil? && !parameterized) + end + + # Transient handles use the same ownership and bounded capture contract. + # The cached path constructs its handle inline on the query hot path. + def self.statement_handle(open, stmt, sql, cached, capture_rows) + handle = { stmt: stmt, row: nil, cached: cached, capture: nil, open: open } + open[stmt] = handle + handle[:capture] = { rows: [], names: stmt.columns, eof: false, sql: sql } if capture_rows handle end @@ -932,6 +953,18 @@ def self.bind_int(handle, idx, value) bind_value(handle, idx, value) end + def self.bind_int_opt(handle, idx, value) + bind_int(handle, idx, value) + end + + def self.bind_text_opt(handle, idx, value) + value.nil? ? bind_value(handle, idx, nil) : bind_text(handle, idx, value) + end + + def self.bind_bool_opt(handle, idx, value) + bind_value(handle, idx, value.nil? ? nil : (value ? 1 : 0)) + end + def self.bind_text(handle, idx, value) bind_value(handle, idx, value) end diff --git a/runtime/spinel/db_jruby.rb b/runtime/spinel/db_jruby.rb index ef1481191..567295080 100644 --- a/runtime/spinel/db_jruby.rb +++ b/runtime/spinel/db_jruby.rb @@ -358,6 +358,28 @@ def self.prepare(sql) st end + # Explicit uncached reads skip statement reuse, retaining result replay. + # finalize closes real transient statements, including replay promotion. + def self.prepare_uncached(sql) + qcache = Fiber[:rh_qcache] + parameterized = sql.include?("?") + if !qcache.nil? && !parameterized && (hit = qcache[sql]) + st = Stmt.new(nil, false) + st.sql = sql + st.replay = hit + return st + end + record_query(sql) + conn = current_dbh + pstmt = conn.raw.prepare_statement(sql) + st = Stmt.new(pstmt, false) + st.open = conn.open_statements + st.open[pstmt] = st + st.sql = sql + st.capture = { rows: [], names: nil, eof: false } if !qcache.nil? && !parameterized + st + end + # Run the query exactly once, lazily. `sqlite_adapter.rb`'s `select_rows` # calls `column_count` before the first `step?`, so either entry point # may be first to need a live ResultSet — execute on whichever wins and @@ -615,6 +637,43 @@ def self.statement_failed(stmt, _operation, error) raise error end + # Optional read predicates occupy one slot whether nil or present. + def self.bind_int_opt(handle, idx, value) + ps = handle.pstmt + raise "statement is not bindable" if ps.nil? || handle.executed + if value.nil? + ps.set_null(idx, Java::JavaSql::Types::INTEGER) + else + ps.set_long(idx, value) + end + rescue StandardError => error + statement_failed(handle, "bind", error) + end + + def self.bind_text_opt(handle, idx, value) + ps = handle.pstmt + raise "statement is not bindable" if ps.nil? || handle.executed + if value.nil? + ps.set_null(idx, Java::JavaSql::Types::VARCHAR) + else + ps.set_string(idx, value) + end + rescue StandardError => error + statement_failed(handle, "bind", error) + end + + def self.bind_bool_opt(handle, idx, value) + ps = handle.pstmt + raise "statement is not bindable" if ps.nil? || handle.executed + if value.nil? + ps.set_null(idx, Java::JavaSql::Types::INTEGER) + else + ps.set_long(idx, value ? 1 : 0) + end + rescue StandardError => error + statement_failed(handle, "bind", error) + end + def self.last_insert_rowid st = current_dbh.raw.create_statement rs = st.execute_query("SELECT last_insert_rowid()") diff --git a/scripts/ci-plan.py b/scripts/ci-plan.py index a8628194a..92f31aab9 100755 --- a/scripts/ci-plan.py +++ b/scripts/ci-plan.py @@ -112,6 +112,8 @@ def native_coverage(path): "tests/param_binds_runtime.rb", "tests/param_binds_cruby_cache.rb", "tests/param_binds_spinel_cache.rb", + "tests/param_binds_associations.rb", + "tests/param_binds_nil.rb", "runtime/spinel/test/statement_cache_cases.rb", "tests/support/emit_and_run.rs", "src/lower/model_to_library/adapter_emit.rs", diff --git a/src/emit/ruby.rs b/src/emit/ruby.rs index df90efda2..6f354951b 100644 --- a/src/emit/ruby.rs +++ b/src/emit/ruby.rs @@ -218,13 +218,14 @@ pub(crate) fn materialize_models( ) .0, ); - let lcs = crate::lower::model_to_library::lower_models_inner( + let lcs = crate::lower::model_to_library::lower_models_inner_with_ruby_values( &app.models, &app.schema, Vec::new(), ¶ms_specs, &assoc_scopes, materialization, + true, ).0; (lcs, params_specs) } @@ -576,6 +577,7 @@ fn lower_controllers_for_spinel(app: &App, format_breadth: FormatBreadth) -> Vec &app.controllers, model_extras, crate::lower::controller_to_library::LowerControllerOptions { + ruby_read_values: true, schema: Some(&app.schema), views: &app.views, library_classes: &app.library_classes, diff --git a/src/emit/ruby/library.rs b/src/emit/ruby/library.rs index dd79550df..b39748f6c 100644 --- a/src/emit/ruby/library.rs +++ b/src/emit/ruby/library.rs @@ -5198,6 +5198,29 @@ pub(crate) fn apply_hydration_nil_lowering(lcs: &mut [LibraryClass], app: &App) if let Some(lc) = lcs.iter_mut().find(|lc| lc.name == model.name) { for m in &mut lc.methods { widen_fk_zero_guards(&mut m.body, &nullable_fks); + // Polymorphic readers dispatch on the type discriminator, + // so they have no zero-sentinel guard to widen. Guard their + // nullable FK here too, before any key-typed adapter call. + if m.name_span.is_synthetic() { + for assoc in model.associations() { + if let crate::dialect::Association::BelongsTo { + name, foreign_key, polymorphic: true, .. + } = assoc { + if m.name == *name && nullable.contains(foreign_key) { + let cond = Expr::new(Span::synthetic(), ExprNode::Send { + recv: Some(Expr::new(Span::synthetic(), ExprNode::Ivar { name: foreign_key.clone() })), + method: Symbol::from("nil?"), args: vec![], block: None, + parenthesized: false, + }); + m.body = Expr::new(m.body.span, ExprNode::If { + cond, + then_branch: Expr::new(Span::synthetic(), ExprNode::Lit { value: Literal::Nil }), + else_branch: m.body.clone(), + }); + } + } + } + } } } } @@ -5276,10 +5299,8 @@ fn widen_fk_zero_guards(expr: &mut Expr, fks: &BTreeSet) { if method.as_str() == "==" && args.len() == 1 => { matches!(&*r.node, ExprNode::Ivar { name } if fks.contains(name)) - && matches!( - &*args[0].node, - ExprNode::Lit { value: Literal::Int { value: 0 } } - ) + && (matches!(&*args[0].node, ExprNode::Lit { value: Literal::Int { value: 0 } }) + || matches!(&*args[0].node, ExprNode::Lit { value: Literal::Str { value } } if value.is_empty())) } _ => false, }; diff --git a/src/lower/arel/ir.rs b/src/lower/arel/ir.rs index c7515c124..8341dc2d9 100644 --- a/src/lower/arel/ir.rs +++ b/src/lower/arel/ir.rs @@ -212,6 +212,9 @@ pub enum ColumnSpec { #[derive(Clone, Debug)] pub enum Predicate { Eq(ColRef, Value), + /// Ruby-family runtime equality: select `= value` or `IS NULL` from + /// the value, preserving SQLite partial-index and join optimizations. + NullableEq(ColRef, Value), And(Box, Box), Or(Box, Box), } @@ -268,9 +271,8 @@ pub enum ValueType { } impl ValueType { - /// True for the nullable variants. Callers that cannot express - /// NULL (the placeholder-bind path has no `bind_null`) route these - /// through inline escaping instead. + /// True for the nullable variants. Optional binds and inline escapes + /// both preserve nil as SQL NULL. pub fn is_nullable(self) -> bool { matches!(self, ValueType::IntOpt | ValueType::StrOpt | ValueType::FloatOpt | ValueType::BoolOpt) } diff --git a/src/lower/arel/mod.rs b/src/lower/arel/mod.rs index e2599b29c..6be264bde 100644 --- a/src/lower/arel/mod.rs +++ b/src/lower/arel/mod.rs @@ -17,6 +17,7 @@ pub mod build; pub mod ir; pub mod visitor; +mod ruby_values; pub use build::{try_build_arel, try_build_arel_with_assocs}; pub use ir::{ @@ -61,6 +62,17 @@ pub fn rewrite_arel_in_expr_with_assocs( schema: &Schema, registry: &HashMap, assocs: &[crate::lower::model_associations::AssociationEdge], +) -> bool { + rewrite_arel_in_expr_with_ruby_values(expr, schema, registry, assocs, false) +} + +/// Ruby-family values preserve SQL NULL without changing strict-target emit. +pub(crate) fn rewrite_arel_in_expr_with_ruby_values( + expr: &mut Expr, + schema: &Schema, + registry: &HashMap, + assocs: &[crate::lower::model_associations::AssociationEdge], + ruby_read_values: bool, ) -> bool { // Names (ivars/locals) the body later refines with relation-chain // methods (`@moderations.where(...)` after `@moderations = @@ -69,7 +81,7 @@ pub fn rewrite_arel_in_expr_with_assocs( // runtime Relation path. let mut refined = std::collections::HashSet::new(); collect_relation_refined_names(expr, &mut refined); - let mut changed = rewrite_arel_inner(expr, schema, registry, assocs, &refined); + let mut changed = rewrite_arel_inner(expr, schema, registry, assocs, &refined, ruby_read_values); // Both call sites hand us a METHOD BODY, and a body that is a // single statement is not a `Seq` — so the hoist post-pass inside // `rewrite_arel_inner`, which walks a Seq's statement list, had no @@ -192,6 +204,7 @@ fn rewrite_arel_inner( registry: &HashMap, assocs: &[crate::lower::model_associations::AssociationEdge], refined: &std::collections::HashSet, + ruby_read_values: bool, ) -> bool { if let ExprNode::Assign { target, .. } = expr.node.as_ref() { let name = match target { @@ -204,9 +217,12 @@ fn rewrite_arel_inner( } } if let ExprNode::Send { .. } = expr.node.as_ref() { - if let Some((op, owner)) = + if let Some((mut op, owner)) = build::try_build_arel_with_assocs(expr, schema, registry, assocs) { + if ruby_read_values { + ruby_values::normalize(&mut op, schema); + } let mut replacement = SqliteVisitor.visit(&op, schema, &owner); // The expansion replaces the recognized chain wholesale; // its provenance is the chain call site. Subtrees the @@ -241,15 +257,15 @@ fn rewrite_arel_inner( let ExprNode::Send { recv: Some(recv), args, .. } = &mut *expr.node else { unreachable!("matched Send with recv above"); }; - let mut changed = rewrite_arel_spine_args(recv, schema, registry, assocs, refined); + let mut changed = rewrite_arel_spine_args(recv, schema, registry, assocs, refined, ruby_read_values); for a in args { - changed |= rewrite_arel_inner(a, schema, registry, assocs, refined); + changed |= rewrite_arel_inner(a, schema, registry, assocs, refined, ruby_read_values); } return changed; } let mut changed = false; walk_subexprs_mut(expr, &mut |e| { - changed |= rewrite_arel_inner(e, schema, registry, assocs, refined) + changed |= rewrite_arel_inner(e, schema, registry, assocs, refined, ruby_read_values) }); // Post-pass: when an Arel rewrite landed a multi-stmt hydrate Seq // in a *value* position — directly as an Assign value @@ -281,17 +297,18 @@ fn rewrite_arel_spine_args( registry: &HashMap, assocs: &[crate::lower::model_associations::AssociationEdge], refined: &std::collections::HashSet, + ruby_read_values: bool, ) -> bool { let mut changed = false; if let ExprNode::Send { recv, args, block, .. } = &mut *expr.node { if let Some(r) = recv { - changed |= rewrite_arel_spine_args(r, schema, registry, assocs, refined); + changed |= rewrite_arel_spine_args(r, schema, registry, assocs, refined, ruby_read_values); } for a in args { - changed |= rewrite_arel_inner(a, schema, registry, assocs, refined); + changed |= rewrite_arel_inner(a, schema, registry, assocs, refined, ruby_read_values); } if let Some(b) = block { - changed |= rewrite_arel_inner(b, schema, registry, assocs, refined); + changed |= rewrite_arel_inner(b, schema, registry, assocs, refined, ruby_read_values); } } changed diff --git a/src/lower/arel/ruby_values.rs b/src/lower/arel/ruby_values.rs new file mode 100644 index 000000000..5591e081a --- /dev/null +++ b/src/lower/arel/ruby_values.rs @@ -0,0 +1,50 @@ +//! Ruby-family read values. Strict targets retain their existing value +//! representation until their nullable runtime and bind capabilities land. + +use crate::schema::Schema; +use crate::ty::Ty; + +use super::ir::{ArelOp, Predicate, Value, ValueType}; + +pub(super) fn normalize(op: &mut ArelOp, schema: &Schema) { + if let ArelOp::Select(select) = op { + if let Some(predicate) = &mut select.conditions { + normalize_predicate(predicate, schema); + } + } +} + +fn normalize_predicate(predicate: &mut Predicate, schema: &Schema) { + match predicate { + Predicate::And(left, right) | Predicate::Or(left, right) => { + normalize_predicate(left, schema); + normalize_predicate(right, schema); + } + Predicate::Eq(col, Value::Runtime { expr, ty }) => { + let nullable_column = schema.tables.get(&col.table.0) + .and_then(|table| table.columns.iter().find(|c| c.name == col.column)) + .is_some_and(|column| column.nullable && !column.primary_key); + let nullable = nullable_column || expr.ty.as_ref().is_some_and(is_nullable); + if nullable { + *ty = match ty { + ValueType::Int | ValueType::IntOpt => ValueType::IntOpt, + ValueType::Str | ValueType::StrOpt => ValueType::StrOpt, + ValueType::Bool | ValueType::BoolOpt => ValueType::BoolOpt, + ValueType::FloatOpt => ValueType::FloatOpt, + }; + if nullable_column { + *predicate = Predicate::NullableEq(col.clone(), Value::Runtime { expr: expr.clone(), ty: *ty }); + } + } + } + Predicate::Eq(_, _) | Predicate::NullableEq(_, _) => {} + } +} + +fn is_nullable(ty: &Ty) -> bool { + match ty { + Ty::Nil => true, + Ty::Union { variants } => variants.iter().any(is_nullable), + _ => false, + } +} diff --git a/src/lower/arel/visitor.rs b/src/lower/arel/visitor.rs index c2d8f9494..512fd5947 100644 --- a/src/lower/arel/visitor.rs +++ b/src/lower/arel/visitor.rs @@ -53,42 +53,106 @@ pub(crate) fn param_binds_enabled() -> bool { .unwrap_or(false) } -/// One deferred bind: the unrendered value expr plus the `ValueType` -/// that picks `bind_int` / `bind_text` / `bind_bool`. Accumulated by the -/// SQL composers alongside the `?` they emit, then drained into -/// `Db.bind_*(stmt, i, expr)` calls right after the `Db.prepare`. +/// A typed pending bind. Nullable predicates also select an SQL fragment. +/// Their runtime branch reserves a position together with the `= ?` fragment; +/// position zero means `IS NULL` and no bind. The RHS is evaluated just once. struct Bind { expr: Expr, ty: ValueType, + nullable_column: Option, + parameterized: bool, } -/// `Db.bind_(stmt, <1-based idx>, )` for each accumulated -/// bind, in placeholder order (sqlite bind indices are 1-based). The -/// per-value type is known at composition time, so each bind is -/// monomorphic — no heterogeneous bind bag on the hot path. -fn emit_bind_calls(stmt: &Symbol, binds: &[Bind]) -> Vec { +fn bind_local(kind: &str, i: usize) -> Symbol { + Symbol::from(format!("__rh_{}_{}", kind, i)) +} + +fn has_shaped_predicates(binds: &[Bind]) -> bool { + binds.iter().any(|b| b.nullable_column.is_some()) +} + +/// No exponential code generation: one branch per nullable predicate. Up to +/// seven predicates have at most 128 SQL shapes; larger combinations bypass +/// the cache, including Spinel's cache which only trims at lease boundaries. +const MAX_CACHED_NULLABLE_PREDICATES: usize = 7; + +fn emit_prepare(stmt: &Symbol, sql: Expr, binds: &[Bind]) -> Vec { let db = ClassId(Symbol::from(DB_MOD)); - binds - .iter() - .enumerate() - .map(|(i, b)| { - let method = match b.ty { - ValueType::Int => "bind_int", - ValueType::Str => "bind_text", - ValueType::Bool => "bind_bool", - // Nullable values never reach here: `push_value_segment` - // routes them through inline escaping, since the bind - // surface has no `bind_null`. - ty if ty.is_nullable() => unreachable!("nullable value in bind path: {ty:?}"), - _ => unreachable!(), + let mut out = Vec::new(); + let shaped = has_shaped_predicates(binds); + let index = Symbol::from("__rh_bind_index"); + if shaped && binds.iter().any(|b| b.parameterized) { + out.push(assign_var(&index, lit_int(0))); + } + for (i, b) in binds.iter().enumerate() { + if !shaped { break; } + // Reserving the slot is the bind decision. The later typed bind + // consumes this position, never re-evaluating a nullable RHS. + let reserve = || vec![ + assign_var(&index, send_to(var_ref(&index), "+", vec![lit_int(1)], false)), + assign_var(&bind_local("position", i), var_ref(&index)), + ]; + if let Some(column) = &b.nullable_column { + let value = bind_local("value", i); + let fragment = bind_local("predicate", i); + out.push(assign_var(&value, b.expr.clone())); + if b.parameterized { + out.push(assign_var(&bind_local("position", i), lit_int(0))); + } + let non_nil_sql = if b.parameterized { + lit_str(format!("{} = ?", column)) + } else { + concat_chain(vec![lit_str(format!("{} = ", column)), escape_value(&db, + &Value::Runtime { expr: var_ref(&value), ty: b.ty })]) }; - db_call( - &db, - method, - vec![var_ref(stmt), lit_int((i + 1) as i64), b.expr.clone()], - ) - }) - .collect() + let mut non_nil = vec![assign_var(&fragment, non_nil_sql)]; + if b.parameterized { non_nil.extend(reserve()); } + out.push(Expr::new(Span::synthetic(), ExprNode::If { + cond: send_to(var_ref(&value), "nil?", vec![], false), + then_branch: assign_var(&fragment, lit_str(format!("{} IS NULL", column))), + else_branch: seq(non_nil), + })); + } else { + out.push(assign_var(&bind_local("value", i), b.expr.clone())); + out.extend(reserve()); + } + } + let nullable_count = binds.iter().filter(|b| b.nullable_column.is_some()).count(); + let prepare = if nullable_count > MAX_CACHED_NULLABLE_PREDICATES && binds.iter().any(|b| b.parameterized) { + "prepare_uncached" + } else { + "prepare" + }; + out.push(assign_var(stmt, db_call(&db, prepare, vec![sql]))); + out +} + +/// Fixed-shape queries keep literal positions. Shaped queries consume the +/// positions reserved during SQL construction, skipping nil predicates. +fn emit_bind_calls(stmt: &Symbol, binds: &[Bind]) -> Vec { + let db = ClassId(Symbol::from(DB_MOD)); + let shaped = has_shaped_predicates(binds); + binds.iter().enumerate().filter(|(_, b)| b.parameterized).map(|(i, b)| { + let method = match b.ty { + ValueType::Int => "bind_int", + ValueType::Str => "bind_text", + ValueType::Bool => "bind_bool", + ValueType::IntOpt => "bind_int_opt", + ValueType::StrOpt => "bind_text_opt", + ValueType::BoolOpt => "bind_bool_opt", + ValueType::FloatOpt => unreachable!("unserialized float in bind path"), + }; + let position = if shaped { var_ref(&bind_local("position", i)) } else { lit_int((i + 1) as i64) }; + let value = if shaped { var_ref(&bind_local("value", i)) } else { b.expr.clone() }; + let call = db_call(&db, method, vec![var_ref(stmt), position.clone(), value]); + if b.nullable_column.is_some() { + Expr::new(Span::synthetic(), ExprNode::If { + cond: send_to(position, ">", vec![lit_int(0)], false), + then_branch: call, + else_branch: nil_lit(), + }) + } else { call } + }).collect() } /// Render an Arel tree into an `Expr` that calls into the per-target @@ -173,7 +237,7 @@ fn emit_single_hydrate(sel: &Select, table: &Table, owner: &ClassId, param: bool let mut binds = Vec::new(); let sql = compose_sql_select(sel, table, param, &mut binds); - let stmt_assign = assign_var(&stmt, db_call(&db, "prepare", vec![sql])); + let prepare = emit_prepare(&stmt, sql, &binds); let result_init = assign_var(&result, nil_lit()); // if Db.step?(stmt) ; result = .from_stmt(stmt) ; end @@ -193,7 +257,7 @@ fn emit_single_hydrate(sel: &Select, table: &Table, owner: &ClassId, param: bool let finalize = db_call(&db, "finalize", vec![var_ref(&stmt)]); // stmt = prepare ; [bind …] ; result = nil ; if step? {…} ; finalize ; result - let mut stmts = vec![stmt_assign]; + let mut stmts = prepare; stmts.extend(emit_bind_calls(&stmt, &binds)); stmts.push(result_init); stmts.push(if_expr); @@ -217,7 +281,7 @@ fn emit_multi_hydrate( let mut binds = Vec::new(); let sql = compose_sql_select(sel, table, param, &mut binds); - let stmt_assign = assign_var(&stmt, db_call(&db, "prepare", vec![sql])); + let prepare = emit_prepare(&stmt, sql, &binds); // Empty Array literal carries an explicit `Array` type // annotation so strict-target emit (Crystal `[] of Owner`) // matches the subsequent `results << instance` push semantics. @@ -271,7 +335,7 @@ fn emit_multi_hydrate( // hydrate loop. Preload sub-queries build their own statements and // keep the inline `IN (…)` list (variable arity — not parameterized // here; see the note in `push_preload_stmts`). - let mut stmts = vec![stmt_assign]; + let mut stmts = prepare; stmts.extend(emit_bind_calls(&stmt, &binds)); stmts.push(results_init); stmts.push(while_loop); @@ -580,7 +644,7 @@ fn emit_pluck(sel: &Select, table: &Table, col: &super::ir::ColRef, param: bool) let mut binds = Vec::new(); let sql = compose_sql_select(sel, table, param, &mut binds); - let stmt_assign = assign_var(&stmt, db_call(&db, "prepare", vec![sql])); + let prepare = emit_prepare(&stmt, sql, &binds); // The element type is carried explicitly for the same reason the // hydrate loop carries `Array`: an untyped `[]` types as @@ -613,7 +677,7 @@ fn emit_pluck(sel: &Select, table: &Table, col: &super::ir::ColRef, param: bool) }, ); - let mut stmts = vec![stmt_assign]; + let mut stmts = prepare; stmts.extend(emit_bind_calls(&stmt, &binds)); stmts.push(results_init); stmts.push(while_loop); @@ -678,7 +742,7 @@ fn emit_group_count( let mut binds = Vec::new(); let sql = compose_sql_select(sel, table, param, &mut binds); - let stmt_assign = assign_var(&stmt, db_call(&db, "prepare", vec![sql])); + let prepare = emit_prepare(&stmt, sql, &binds); let results_init = assign_var( &results, @@ -716,7 +780,7 @@ fn emit_group_count( }, ); - let mut stmts = vec![stmt_assign]; + let mut stmts = prepare; stmts.extend(emit_bind_calls(&stmt, &binds)); stmts.push(results_init); stmts.push(while_loop); @@ -738,14 +802,14 @@ fn emit_count(sel: &Select, table: &Table, param: bool) -> Expr { let mut binds = Vec::new(); push_where_segments(&mut segments, sel.conditions.as_ref(), table, param, &mut binds); - let stmt_assign = assign_var(&stmt, db_call(&db, "prepare", vec![concat_chain(segments)])); + let prepare = emit_prepare(&stmt, concat_chain(segments), &binds); let step = db_call(&db, "step?", vec![var_ref(&stmt)]); let read = db_call(&db, "column_int", vec![var_ref(&stmt), lit_int(0)]); let result_assign = assign_var(&result, read); let finalize = db_call(&db, "finalize", vec![var_ref(&stmt)]); // stmt = prepare ; [bind …] ; step? ; result = column_int ; finalize ; result - let mut stmts = vec![stmt_assign]; + let mut stmts = prepare; stmts.extend(emit_bind_calls(&stmt, &binds)); stmts.push(step); stmts.push(result_assign); @@ -768,12 +832,12 @@ fn emit_exists(sel: &Select, table: &Table, param: bool) -> Expr { segments.push(lit_str(format!(" LIMIT {}", n))); } - let stmt_assign = assign_var(&stmt, db_call(&db, "prepare", vec![concat_chain(segments)])); + let prepare = emit_prepare(&stmt, concat_chain(segments), &binds); let result_assign = assign_var(&result, db_call(&db, "step?", vec![var_ref(&stmt)])); let finalize = db_call(&db, "finalize", vec![var_ref(&stmt)]); // stmt = prepare ; [bind …] ; result = step? ; finalize ; result - let mut stmts = vec![stmt_assign]; + let mut stmts = prepare; stmts.extend(emit_bind_calls(&stmt, &binds)); stmts.push(result_assign); stmts.push(finalize); @@ -973,6 +1037,15 @@ fn push_predicate_segments( Predicate::Eq(col, Value::LiteralNull) => { segments.push(lit_str(format!("{} IS NULL", crate::naming::sql_ident(col.column.as_str())))); } + Predicate::NullableEq(col, Value::Runtime { expr, ty }) => { + segments.push(var_ref(&bind_local("predicate", binds.len()))); + binds.push(Bind { + expr: expr.clone(), ty: *ty, + nullable_column: Some(crate::naming::sql_ident(col.column.as_str())), + parameterized: param && *ty != ValueType::FloatOpt, + }); + } + Predicate::NullableEq(_, _) => unreachable!("only runtime predicates select a shape"), Predicate::Eq(col, val) => { segments.push(lit_str(format!("{} = ", crate::naming::sql_ident(col.column.as_str())))); push_value_segment(segments, val, param, binds); @@ -1004,11 +1077,10 @@ fn push_value_segment(segments: &mut Vec, val: &Value, param: bool, binds: Value::LiteralStr(s) => segments.push(lit_str(format!("'{}'", s.replace('\'', "''")))), Value::LiteralBool(b) => segments.push(lit_str(if *b { "1".into() } else { "0".into() })), Value::LiteralNull => segments.push(lit_str("NULL".into())), - // A nullable value can't ride the placeholder path — there is - // no `bind_null` — so it always renders inline. - Value::Runtime { expr, ty } if param && !ty.is_nullable() => { + // One optional bind still occupies exactly one placeholder. + Value::Runtime { expr, ty } if param && *ty != ValueType::FloatOpt => { segments.push(lit_str("?".to_string())); - binds.push(Bind { expr: expr.clone(), ty: *ty }); + binds.push(Bind { expr: expr.clone(), ty: *ty, nullable_column: None, parameterized: true }); } Value::Runtime { .. } => segments.push(escape_value(&db, val)), } diff --git a/src/lower/controller_to_library/mod.rs b/src/lower/controller_to_library/mod.rs index 7e86eb4ce..ba2233816 100644 --- a/src/lower/controller_to_library/mod.rs +++ b/src/lower/controller_to_library/mod.rs @@ -247,6 +247,8 @@ fn returns_relation(ty: &Ty) -> bool { /// positional args. #[derive(Default)] pub struct LowerControllerOptions<'a> { + /// Ruby-family nullable read values; strict-target defaults stay unchanged. + pub ruby_read_values: bool, /// App `Schema` — enables the Arel SQL-chain lowering pass. pub schema: Option<&'a crate::schema::Schema>, /// App views — scanned for `*.json.jbuilder` format dispatch and the @@ -291,6 +293,7 @@ pub fn lower_controllers_with_arel_views_assocs_and_routes( opts: LowerControllerOptions, ) -> Vec { let LowerControllerOptions { + ruby_read_values, schema, views, library_classes, @@ -532,8 +535,8 @@ pub fn lower_controllers_with_arel_views_assocs_and_routes( let refined_across_methods = refined_result_methods.contains(&method.name); if let Some(schema) = schema { if !refined_across_methods { - rewritten |= crate::lower::arel::rewrite_arel_in_expr_with_assocs( - &mut method.body, schema, &classes, assocs, + rewritten |= crate::lower::arel::rewrite_arel_in_expr_with_ruby_values( + &mut method.body, schema, &classes, assocs, ruby_read_values, ); } } diff --git a/src/lower/model_to_library/mod.rs b/src/lower/model_to_library/mod.rs index db42da18b..504efd2f5 100644 --- a/src/lower/model_to_library/mod.rs +++ b/src/lower/model_to_library/mod.rs @@ -210,6 +210,20 @@ pub(crate) fn lower_models_inner( params_specs: &crate::lower::controller_to_library::params::ParamsSpecs, unfolded: &std::collections::HashSet<(ClassId, Symbol)>, materialization: Materialization<'_>, +) -> (Vec, HashMap) { + lower_models_inner_with_ruby_values( + models, schema, extra_class_infos, params_specs, unfolded, materialization, false, + ) +} + +pub(crate) fn lower_models_inner_with_ruby_values( + models: &[Model], + schema: &Schema, + extra_class_infos: Vec<(ClassId, crate::analyze::ClassInfo)>, + params_specs: &crate::lower::controller_to_library::params::ParamsSpecs, + unfolded: &std::collections::HashSet<(ClassId, Symbol)>, + materialization: Materialization<'_>, + ruby_read_values: bool, ) -> (Vec, HashMap) { let mut all_methods: Vec<(Vec, ClassId, Option<&Table>, &Model)> = Vec::new(); let mut classes: HashMap = HashMap::new(); @@ -347,7 +361,9 @@ pub(crate) fn lower_models_inner( let unfold = method.receiver == crate::dialect::MethodReceiver::Class && unfolded.contains(&(model.name.clone(), method.name.clone())); if !unfold { - crate::lower::arel::rewrite_arel_in_expr(&mut method.body, schema, &classes); + crate::lower::arel::rewrite_arel_in_expr_with_ruby_values( + &mut method.body, schema, &classes, &[], ruby_read_values, + ); } type_method_body(method, &classes, table, Some(model)); } diff --git a/tests/ci_plan_test.py b/tests/ci_plan_test.py index 4005228a1..d17d97853 100644 --- a/tests/ci_plan_test.py +++ b/tests/ci_plan_test.py @@ -402,6 +402,8 @@ def test_param_binds_owns_lowering_drivers_and_database_runtime(self): "tests/param_binds_runtime.rb", "tests/param_binds_cruby_cache.rb", "tests/param_binds_spinel_cache.rb", + "tests/param_binds_associations.rb", + "tests/param_binds_nil.rb", "tests/support/emit_and_run.rs", ]: with self.subTest(path=path): diff --git a/tests/db_shim_conformance.rs b/tests/db_shim_conformance.rs index 676ad2417..a2898dd1d 100644 --- a/tests/db_shim_conformance.rs +++ b/tests/db_shim_conformance.rs @@ -403,6 +403,17 @@ const RUBY_FAMILY_LIFECYCLE: &[&str] = &[ "checkpoint_in_background!", ]; +#[test] +fn ruby_family_shims_declare_optional_binds() { + let rbs = read_shim("runtime/ruby/db.rbs"); + for method in ["bind_int_opt", "bind_text_opt", "bind_bool_opt"] { + assert!(defines_in_rbs(&rbs, method), "missing RBS: {method}"); + for path in ["runtime/spinel/db.rb", "runtime/spinel/db_cruby.rb", "runtime/spinel/db_jruby.rb"] { + assert!(defines_with(&read_shim(path), method, &["def self."], false, false), "{path}: {method}"); + } + } +} + #[test] fn every_ruby_family_shim_defines_the_request_lifecycle_hooks() { let mut missing: Vec = Vec::new(); diff --git a/tests/param_binds.rs b/tests/param_binds.rs index 5fa71c81c..9b9bf1686 100644 --- a/tests/param_binds.rs +++ b/tests/param_binds.rs @@ -41,10 +41,17 @@ ActiveRecord::Schema[8.1].define(version: 1) do create_table "parents", force: :cascade do |t| t.string "name", null: false t.integer "other_id", null: false + t.integer "number" + t.string "optional_name" + t.boolean "flag" end create_table "items", force: :cascade do |t| t.integer "parent_id", null: false t.string "name", null: false + t.integer "number" + t.string "optional_name" + t.boolean "flag" + t.boolean "required_flag", null: false end end "#, @@ -97,6 +104,36 @@ class Parent < ApplicationRecord row = Item.find_by(name: @name) row.nil? ? -1 : row.id end + def nullable_pair(id, number, name, flag, parent) + @id = id.to_i + @number = number.nil? ? nil : number.to_i + @optional_name = name.nil? ? nil : name.to_s + @flag = flag.nil? ? nil : flag == true + @other_id = parent.to_i + Item.where(id: @id, number: @number, optional_name: @optional_name, flag: @flag, parent_id: @other_id).count + end + def nullable_key(number) + @number = number.nil? ? nil : number.to_i + Item.where(id: @number).count + end + def not_null_number(number) + @number = number.nil? ? nil : number.to_i + Item.where(parent_id: @number).count + end + def not_null_name(name) + @optional_name = name.nil? ? nil : name.to_s + Item.where(name: @optional_name).count + end + def not_null_flag(flag) + @flag = flag.nil? ? nil : flag == true + Item.where(required_flag: @flag).count + end + def nil_key_exists + Item.exists?(nil) + end + def nil_key_find + Item.find(nil) + end end "#, ) @@ -143,11 +180,35 @@ fn emitted(test: &str, target: BuildTarget) { "rows(value)", "count_id(value)", "pair(value, parent)", + "nullable_pair(id, number, name, flag, parent)", "items", ] { assert_bound(&probe, method, "bind_int", binds_on); } assert_bound(&probe, "named(value)", "bind_text", binds_on); + let nullable = probe.split(" def nullable_pair(").nth(1).unwrap(); + let nullable = nullable.split("\n end").next().unwrap(); + for (column, escape) in [ + ("number", "escape_int_opt"), + ("optional_name", "escape_string_opt"), + ("flag", "escape_bool_opt"), + ] { + assert!(nullable.contains(&format!("{column} IS NULL")), "{nullable}"); + assert!(!nullable.contains(&format!("{column} IS ?")), "{nullable}"); + if !binds_on { + assert!(nullable.contains(&format!("Db.{escape}(")), "{nullable}"); + } + } + if binds_on { + assert_eq!(nullable.matches("Db.bind_").count(), 5, "{nullable}"); + assert!(nullable.contains("Db.bind_int("), "{nullable}"); + assert!(nullable.contains("Db.bind_int_opt("), "{nullable}"); + assert!(nullable.contains("Db.bind_text_opt("), "{nullable}"); + assert!(nullable.contains("Db.bind_bool_opt("), "{nullable}"); + let key = probe.split(" def nullable_key(").nth(1).unwrap().split("\n end").next().unwrap(); + assert!(key.contains("WHERE id = ?"), "{key}"); + assert!(key.contains("Db.bind_int_opt(stmt, 1, @number)"), "{key}"); + } let item = std::fs::read_to_string(dir.join("app/models/item.rb")).unwrap(); for method in [ "self._adapter_find_by_id(id)", @@ -246,6 +307,13 @@ fn run_script(dir: &std::path::Path, script: &str, native: bool) { } } +struct ScratchDir(PathBuf); +impl Drop for ScratchDir { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + fn runtime(native: bool) { let root = PathBuf::from(env!("CARGO_MANIFEST_DIR")); let base = option_env!("CARGO_TARGET_TMPDIR") @@ -256,6 +324,7 @@ fn runtime(native: bool) { std::process::id() )); std::fs::create_dir_all(&dir).unwrap(); + let _cleanup = ScratchDir(dir.clone()); let prelude = if native { for name in ["db.rb", "active_support_time_parsing.rb"] { std::fs::copy(root.join("runtime/spinel").join(name), dir.join(name)).unwrap(); @@ -342,7 +411,8 @@ puts "runtime: CRuby interrupted reader keeps ownership across a different id" 1, "missing or ambiguous lifecycle probe marker" ); - let body = body.replace(marker, &format!("{lifecycle}\n{marker}")); + let nil_contract = include_str!("param_binds_nil.rb"); + let body = body.replace(marker, &format!("{nil_contract}\n{lifecycle}\n{marker}")); let body = body.replace( "# Observe bytes as a BLOB", &format!("{clear}\n# Observe bytes as a BLOB"), @@ -387,6 +457,175 @@ fn bind_runtime_spinel() { runtime(true); } +fn nullable_associations(test: &str, target: BuildTarget) { + if std::env::var_os("ROUNDHOUSE_BINDS_CHILD").is_none() { + for mode in ["0", "1"] { + println!("{test}: ROUNDHOUSE_PARAM_BINDS={mode}"); + success( + Command::new(std::env::current_exe().unwrap()) + .args(["--exact", test, "--include-ignored", "--nocapture"]) + .env("ROUNDHOUSE_BINDS_CHILD", "1") + .env("ROUNDHOUSE_PARAM_BINDS", mode), + ); + } + return; + } + let app = emit_and_run::empty_app() + .write( + "app/models/application_record.rb", + "class ApplicationRecord < ActiveRecord::Base\n self.abstract_class = true\nend\n", + ) + .write( + "app/controllers/application_controller.rb", + "class ApplicationController < ActionController::Base\nend\n", + ) + .write( + "config/routes.rb", + "Rails.application.routes.draw do\nend\n", + ) + .write( + "app/models/account.rb", + "class Account < ApplicationRecord\n has_many :links\n has_many :taggings, as: :taggable\nend\n", + ) + .write( + "app/models/article.rb", + "class Article < ApplicationRecord\n has_many :links\nend\n", + ) + .write( + "app/models/link.rb", + "class Link < ApplicationRecord\n belongs_to :account, optional: true\n belongs_to :article, optional: true\nend\n", + ) + .write( + "app/models/tagging.rb", + "class Tagging < ApplicationRecord\n belongs_to :taggable, polymorphic: true, optional: true\nend\n", + ) + .write( + "db/schema.rb", + r#" +ActiveRecord::Schema[8.1].define(version: 1) do + create_table "accounts", force: :cascade do |t| + t.string "name", null: false + end + create_table "articles", id: :uuid, force: :cascade do |t| + t.string "title", null: false + end + create_table "links", force: :cascade do |t| + t.integer "account_id" + t.uuid "article_id" + end + create_table "taggings", force: :cascade do |t| + t.integer "taggable_id" + t.string "taggable_type" + end +end +"#, + ); + let (dir, errors) = app.emit(target); + assert!(errors.is_empty(), "{}", errors.join("\n")); + let binds_on = std::env::var("ROUNDHOUSE_PARAM_BINDS").unwrap() == "1"; + let link = std::fs::read_to_string(dir.join("app/models/link.rb")).unwrap(); + assert_bound(&link, "account", "bind_int", binds_on); + assert_bound(&link, "article", "bind_text", binds_on); + let tagging = std::fs::read_to_string(dir.join("app/models/tagging.rb")).unwrap(); + assert_bound(&tagging, "taggable", "bind_int", binds_on); + // Spinel currently boxes column_int_opt's nil sentinel as an Integer + // when passing it to a generated setter. That separate hydration gap + // makes .nil? false before this reader runs. Exercise native integer + // nil inputs through the post-emission assignments in the shared + // script, and retain native SQL-NULL hydration coverage for UUIDs. + let integer_hydration = if target == BuildTarget::Spinel { + "" + } else { + r#" +raise "nil integer FK from SQL resolved an association" unless from_sql.account.nil? +raise "nil polymorphic FK from SQL resolved an association" unless Tagging.find(1).taggable.nil? +raise "nil polymorphic type from SQL resolved an association" unless Tagging.find(3).taggable.nil? +puts "emit: SQL-NULL integer and polymorphic association readers passed" +"# + }; + let script = format!( + r#"require_relative "boot" +require_relative "app/models/account" +require_relative "app/models/article" +require_relative "app/models/link" +require_relative "app/models/tagging" +SqliteAdapter.configure("file:nil_associations?mode=memory&cache=shared") +ActiveRecord.adapter = SqliteAdapter +Schema.statements.each {{ |sql| Db.exec(sql) }} +{} +{} +from_sql = Link.find(1) +raise "nil UUID FK from SQL resolved an association" unless from_sql.article.nil? +puts "emit: SQL-NULL UUID association reader passed" +{integer_hydration} +Db.close +"#, + cache_probe(target == BuildTarget::Spinel), + include_str!("param_binds_associations.rb") + ); + run_script(&dir, &script, target == BuildTarget::Spinel); +} + +#[test] +fn nullable_associations_ruby() { + nullable_associations("nullable_associations_ruby", BuildTarget::Ruby); +} + +#[test] +#[ignore = "requires Spinel (SPINEL=/path/to/spinel)"] +fn nullable_associations_spinel() { + nullable_associations("nullable_associations_spinel", BuildTarget::Spinel); +} + +fn cache_probe(native: bool) -> &'static str { + if native { + r#" +module SQL + ffi_func :sqlite3_next_stmt, [:ptr, :ptr], :ptr +end +class DbConn + def gate_cache_size + @entries.length + end + def gate_live_statements + n = 0 + ptr = SQL.sqlite3_next_stmt(dbh, nil) + while !ptr.nil? + n += 1 + ptr = SQL.sqlite3_next_stmt(dbh, ptr) + end + n + end +end +module Db + def self.gate_cache_size + current_conn.gate_cache_size + end + def self.gate_live_statements + current_conn.gate_live_statements + end + def self.gate_released(stmt) + nil + end +end +"# + } else { + r#" +module Db + def self.gate_cache_size + (current_dbh.instance_variable_get(:@rh_stmt_cache) || {}).size + end + def self.gate_live_statements + 0 + end + def self.gate_released(stmt) + raise "transient statement was not closed" if stmt[:stmt] && !stmt[:stmt].closed? + end +end +"# + } +} + fn raw_where_substitution(target: BuildTarget) { let (dir, errors) = overlay().emit(target); assert!(errors.is_empty(), "{}", errors.join("\n")); diff --git a/tests/param_binds_associations.rb b/tests/param_binds_associations.rb new file mode 100644 index 000000000..9caa4f225 --- /dev/null +++ b/tests/param_binds_associations.rb @@ -0,0 +1,35 @@ +# Real sentinel-key rows distinguish nil from accidental 0/"" coercion. +# The Rails source has no explicit nil assignment to supply inferred +# nilability to a synthesized association's Arel RHS. These assignments +# happen after emission, so only the reader's own guard can protect it. +Db.exec("INSERT INTO accounts (id, name) VALUES (0, 'zero trap'), (7, 'present')") +Db.exec("INSERT INTO articles (id, title) VALUES ('', 'empty trap'), ('00000000-0000-0000-0000-000000000007', 'present')") +Db.exec("INSERT INTO links (id, account_id, article_id) VALUES (1, NULL, NULL), (2, 7, '00000000-0000-0000-0000-000000000007')") +Db.exec("INSERT INTO taggings (id, taggable_id, taggable_type) VALUES (1, NULL, 'Account'), (2, 7, 'Account'), (3, NULL, NULL)") + +8.times do + absent = Link.new + absent.account_id = nil + absent.article_id = nil + untagged = Tagging.new + untagged.taggable_id = nil + untagged.taggable_type = "Account" + cache_before = Db.gate_cache_size + raise "nil integer FK resolved an association" unless absent.account.nil? + raise "nil UUID FK resolved an association" unless absent.article.nil? + raise "nil polymorphic FK resolved an association" unless untagged.taggable.nil? + raise "nil FK prepared a query" unless Db.gate_cache_size == cache_before + present = Link.find(2) + account = present.account + article = present.article + raise "present integer FK lost" if account.nil? + raise "present integer FK changed" unless account.id == 7 + raise "present UUID FK lost" if article.nil? + raise "present UUID FK changed" unless article.id == "00000000-0000-0000-0000-000000000007" + tagged = Tagging.find(2).taggable + raise "present polymorphic FK lost" if tagged.nil? + raise "present polymorphic FK changed" unless tagged.id == 7 + untagged.taggable_type = nil + raise "nil polymorphic type resolved an association" unless untagged.taggable.nil? +end +puts "emit: explicit nil integer/UUID/polymorphic association readers and nonnil controls passed" diff --git a/tests/param_binds_emit.rb b/tests/param_binds_emit.rb index f64535da8..81e220db5 100644 --- a/tests/param_binds_emit.rb +++ b/tests/param_binds_emit.rb @@ -6,12 +6,12 @@ Db.exec("INSERT INTO parents (id, other_id, name) VALUES (" + i.to_s + ", 0, 'parent-" + i.to_s + "')") # Deliberately asymmetric: swapping (id, parent_id) must not find a # different row with the same COUNT(*). A reversal permutation hides it! - Db.exec("INSERT INTO items (id, parent_id, name) VALUES (" + i.to_s + ", " + ((i % 32) + 1).to_s + ", 'row-" + i.to_s + "')") + Db.exec("INSERT INTO items (id, parent_id, name, required_flag) VALUES (" + i.to_s + ", " + ((i % 32) + 1).to_s + ", 'row-" + i.to_s + "', 0)") # Distinct group sizes make a count expose stale *existing* ids too. j = 0 while j < i % 4 extra_id = 1000 + i * 10 + j - Db.exec("INSERT INTO items (id, parent_id, name) VALUES (" + extra_id.to_s + ", " + i.to_s + ", 'extra-" + extra_id.to_s + "')") + Db.exec("INSERT INTO items (id, parent_id, name, required_flag) VALUES (" + extra_id.to_s + ", " + i.to_s + ", 'extra-" + extra_id.to_s + "', 0)") j += 1 end i += 1 @@ -48,3 +48,66 @@ Db.query_cache_end end puts "emit: 32 ids, 96 serial interleaved rounds, nine read methods passed" + +# Nullable values must select NULL rows in either emit mode. The integer +# predicates on either side pin the order of the three optional bind slots. +Db.exec("INSERT INTO items (id, parent_id, name, number, optional_name, flag, required_flag) VALUES (501, 500, 'nullable', NULL, NULL, NULL, 0)") +Db.exec("INSERT INTO items (id, parent_id, name, number, optional_name, flag, required_flag) VALUES (502, 500, 'nullable', 0, '', 0, 0)") +Db.exec("INSERT INTO items (id, parent_id, name, number, optional_name, flag, required_flag) VALUES (503, 500, 'nullable', 73, '雪', 1, 0)") +Db.exec("INSERT INTO items (id, parent_id, name, required_flag) VALUES (0, 500, 'zero-key', 0)") +Db.exec("INSERT INTO items (id, parent_id, name, required_flag) VALUES (504, 0, 'zero-parent', 0)") +Db.exec("INSERT INTO items (id, parent_id, name, required_flag) VALUES (505, 500, '', 1)") +i = 0 +while i < 12 + raise "nullable nil row" unless probe.nullable_pair(501, nil, nil, nil, 500) == 1 + raise "nullable zero row" unless probe.nullable_pair(502, 0, "", false, 500) == 1 + raise "nullable value row" unless probe.nullable_pair(503, 73, "雪", true, 500) == 1 + raise "nil must not match false/zero/empty" unless probe.nullable_pair(502, nil, nil, nil, 500) == 0 + raise "nullable final bind position" unless probe.nullable_pair(501, nil, nil, nil, 501) == 0 + raise "nullable RHS against nonnullable key" unless probe.nullable_key(nil) == 0 + raise "nil matched a NOT NULL zero value" unless probe.not_null_number(nil) == 0 + raise "NOT NULL zero remains a real value" unless probe.not_null_number(0) == 1 + raise "zero key is a real value" unless probe.nullable_key(0) == 1 + raise "nil matched a NOT NULL empty string" unless probe.not_null_name(nil) == 0 + raise "NOT NULL empty string remains a real value" unless probe.not_null_name("") == 1 + raise "nil matched a NOT NULL boolean" unless probe.not_null_flag(nil) == 0 + raise "NOT NULL false remains a real value" unless probe.not_null_flag(false) == 85 + raise "NOT NULL true remains a real value" unless probe.not_null_flag(true) == 1 + i += 1 +end +puts "emit: nullable int/text/bool and surrounding bind positions passed" + +# Three independently nullable predicates have eight SQL shapes. Visit +# every combination repeatedly with a different id and a fixed trailing +# predicate; a skipped NULL slot must not shift any later bind incorrectly. +mask = 0 +while mask < 8 + number_sql = (mask & 1) == 0 ? "NULL" : "73" + name_sql = (mask & 2) == 0 ? "NULL" : "'snow'" + flag_sql = (mask & 4) == 0 ? "NULL" : "1" + Db.exec("INSERT INTO items (id, parent_id, name, number, optional_name, flag, required_flag) VALUES (" + + (600 + mask).to_s + ", 900, 'combination', " + number_sql + ", " + name_sql + ", " + flag_sql + ", 0)") + mask += 1 +end +12.times do + mask = 0 + while mask < 8 + number = (mask & 1) == 0 ? nil : 73 + name = (mask & 2) == 0 ? nil : "snow" + flag = (mask & 4) == 0 ? nil : true + raise "nullable combination #{mask}" unless probe.nullable_pair(600 + mask, number, name, flag, 900) == 1 + raise "nullable combination trailing bind #{mask}" unless probe.nullable_pair(600 + mask, number, name, flag, 901) == 0 + mask += 1 + end +end +puts "emit: all eight nullable SQL shapes, 192 interleaved hit/miss reads passed" + +# Public key methods receive nil even though their synthesized primitive is +# key-typed. Rails rejects it before touching SQLite; id=0 must not match. +raise "exists?(nil) matched zero key" if probe.nil_key_exists +begin + probe.nil_key_find + raise "find(nil) matched zero key" +rescue ActiveRecord::RecordNotFound +end +puts "emit: find(nil) and exists?(nil) reject a real zero key passed" diff --git a/tests/param_binds_nil.rb b/tests/param_binds_nil.rb new file mode 100644 index 000000000..7a03e3c31 --- /dev/null +++ b/tests/param_binds_nil.rb @@ -0,0 +1,20 @@ +# Contract item 8: nil against a NOT NULL column must never select zero. +# Use the typed primitive directly, as well as the emitted optional path. +Db.exec("CREATE TABLE bind_not_null (value INTEGER NOT NULL)") +Db.exec("INSERT INTO bind_not_null VALUES (0), (73)") +def check_not_null_integer(value) + stmt = Db.prepare("SELECT COUNT(*) FROM bind_not_null WHERE value = ?") + Db.bind_int(stmt, 1, value) + raise "missing NOT NULL count" unless Db.step?(stmt) + expected = value.nil? ? 0 : 1 + actual = Db.column_int(stmt, 0) + Db.finalize(stmt) + raise "typed bind_int(nil) matched zero" unless actual == expected +end +12.times do + check_not_null_integer(73) + check_not_null_integer(nil) + check_not_null_integer(0) + check_not_null_integer(nil) +end +puts "runtime: 48 typed integer/NULL comparisons against NOT NULL passed" diff --git a/tests/param_binds_planner.rb b/tests/param_binds_planner.rb new file mode 100644 index 000000000..b863dcf6e --- /dev/null +++ b/tests/param_binds_planner.rb @@ -0,0 +1,124 @@ +require_relative "boot" +require_relative "app/models/indexed_row" +require_relative "app/models/paired_row" +require_relative "app/models/joined_row" +require_relative "app/models/wide_row" +SqliteAdapter.configure(":memory:") +ActiveRecord.adapter = SqliteAdapter +Schema.statements.each { |sql| Db.exec(sql) } + +# The Arel fast path does not yet fold joins. A view lets the emitted +# nullable WHERE predicate participate in SQLite's real LEFT JOIN planner +# without adding unsupported joins to the lowerer or rewriting emitted SQL. +Db.exec("DROP TABLE joined_rows") +Db.exec("CREATE VIEW joined_rows AS SELECT host_rows.id, indexed_rows.a FROM host_rows LEFT JOIN indexed_rows ON indexed_rows.id = host_rows.id") +Db.exec("WITH RECURSIVE n(id) AS (SELECT 1 UNION ALL SELECT id + 1 FROM n WHERE id < 1000) INSERT INTO indexed_rows SELECT id, CASE WHEN id = 7 THEN 7 END FROM n") +Db.exec("WITH RECURSIVE n(id) AS (SELECT 1 UNION ALL SELECT id + 1 FROM n WHERE id < 1000) INSERT INTO paired_rows SELECT id, CASE WHEN id = 7 THEN 7 END, CASE WHEN id = 7 THEN 11 END FROM n") +Db.exec("INSERT INTO host_rows SELECT id FROM indexed_rows") +Db.exec("INSERT INTO host_rows VALUES (1001)") +Db.exec("INSERT INTO wide_rows DEFAULT VALUES") +Db.exec("CREATE INDEX single_present ON indexed_rows(a) WHERE a IS NOT NULL") +Db.exec("CREATE INDEX pair_present ON paired_rows(a, b) WHERE a IS NOT NULL AND b IS NOT NULL") +Db.exec("ANALYZE") + +module Db + class << self + attr_reader :planner_sql, :planner_binds + alias planner_original_prepare prepare + alias planner_original_prepare_uncached prepare_uncached + alias planner_original_bind_value bind_value + def prepare(sql) + @planner_sql = sql + @planner_binds = [] + planner_original_prepare(sql) + end + def prepare_uncached(sql) + @planner_sql = sql + @planner_binds = [] + planner_original_prepare_uncached(sql) + end + def bind_value(handle, index, value) + @planner_binds[index - 1] = value + planner_original_bind_value(handle, index, value) + end + def planner_details + current_dbh.execute("EXPLAIN QUERY PLAN " + @planner_sql, @planner_binds).map { |row| row[3] }.join("; ") + end + end +end + +def expect_result(label, expected, actual) + raise "#{label}: expected #{expected}, got #{actual}" unless expected == actual +end + +bound = ENV.fetch("PLANNER_BINDS") == "1" +single = IndexedRow.new +pair = PairedRow.new +joined = JoinedRow.new +Db.with_connection do + single_shapes = [] + pair_shapes = [] + 3.times do + expect_result("single non-nil", 1, single.matching(7)) + single_shapes << Db.planner_sql + plan = Db.planner_details + raise "single partial index lost: #{Db.planner_sql}: #{plan}" unless plan.include?("USING COVERING INDEX single_present (a=?)") + expect_result("single placeholder values", bound ? [7] : [], Db.planner_binds) + puts "planner binds=#{bound}: single partial index: #{plan}" + + expect_result("single nil", 999, single.matching(nil)) + single_shapes << Db.planner_sql + raise "nil must remove bind slot: #{Db.planner_sql}" unless Db.planner_sql.include?("a IS NULL") && !Db.planner_sql.include?("?") + expect_result("single nil bind count", [], Db.planner_binds) + + expect_result("pair non-nil", 1, pair.matching(7, 11)) + pair_shapes << Db.planner_sql + plan = Db.planner_details + raise "composite partial index lost: #{Db.planner_sql}: #{plan}" unless plan.include?("USING COVERING INDEX pair_present (a=? AND b=?)") + expect_result("pair placeholder order", bound ? [7, 11] : [], Db.planner_binds) + puts "planner binds=#{bound}: composite partial index: #{plan}" + + expect_result("pair nil/non-nil", 0, pair.matching(nil, 11)) + pair_shapes << Db.planner_sql + expect_result("pair removed first slot", bound ? [11] : [], Db.planner_binds) + expect_result("pair non-nil/nil", 0, pair.matching(7, nil)) + pair_shapes << Db.planner_sql + expect_result("pair removed last slot", bound ? [7] : [], Db.planner_binds) + expect_result("pair nil/nil", 999, pair.matching(nil, nil)) + pair_shapes << Db.planner_sql + expect_result("pair no slots", [], Db.planner_binds) + + expect_result("joined non-nil", 1, joined.matching(7)) + plan = Db.planner_details + raise "LEFT JOIN was not strength-reduced: #{Db.planner_sql}: #{plan}" if plan.include?("LEFT-JOIN") + raise "reduced join lost partial index: #{plan}" unless plan.include?("single_present") + puts "planner binds=#{bound}: LEFT JOIN strength reduction: #{plan}" + + expect_result("joined nil including unmatched row", 1000, joined.matching(nil)) + plan = Db.planner_details + raise "nil must retain LEFT JOIN: #{plan}" unless plan.include?("LEFT-JOIN") + expect_result("join nil bind count", [], Db.planner_binds) + end + expect_result("single shape count", 2, single_shapes.uniq.length) + expect_result("pair shape count", 4, pair_shapes.uniq.length) + # Changing the non-nil values must reuse the already observed bound + # shape. Inline mode intentionally retains value-bearing main SQL. + [8, 19, 1023].each do |value| + expect_result("single changed bind #{value}", 0, single.matching(value)) + expect_result("single non-nil values share one shape", single_shapes[0], Db.planner_sql) if bound + expect_result("pair changed bind #{value}", 0, pair.matching(value, value + 1)) + expect_result("pair non-nil values share one shape", pair_shapes[0], Db.planner_sql) if bound + end + + wide = WideRow.new + cache = Db.current_dbh.instance_variable_get(:@rh_stmt_cache) + cached_before = cache.length + 256.times do |mask| + values = 8.times.map { |bit| (mask & (1 << bit)) == 0 ? nil : bit + 1 } + expect_result("eight-nullable shape #{mask}", mask == 0 ? 1 : 0, wide.matching(*values)) + expect_result("eight-nullable bind order #{mask}", bound ? values.compact : [], Db.planner_binds) + end + expect_result("eight nullable predicates bypass statement cache", cached_before, cache.length) if bound +end +puts "planner: binds=#{bound}, 3 repeated single/composite/join plan groups, 2/4 null patterns, varying non-nil values, 256 eight-nullable combinations passed" +Db.close diff --git a/tests/param_binds_planner.rs b/tests/param_binds_planner.rs new file mode 100644 index 000000000..3f74048c4 --- /dev/null +++ b/tests/param_binds_planner.rs @@ -0,0 +1,117 @@ +//! Runtime nullable shapes must preserve SQLite's partial-index eligibility +//! and outer-join strength reduction. Plans are captured from emitted reads, +//! not independently reconstructed predicates. Each flag gets a subprocess. + +#[path = "support/emit_and_run.rs"] +mod emit_and_run; + +use roundhouse::project::BuildTarget; +use std::process::Command; + +#[test] +fn nullable_read_plans_ruby() { + if std::env::var_os("ROUNDHOUSE_BINDS_CHILD").is_none() { + for mode in ["0", "1"] { + let output = Command::new(std::env::current_exe().unwrap()) + .args(["--exact", "nullable_read_plans_ruby", "--nocapture"]) + .env("ROUNDHOUSE_BINDS_CHILD", "1") + .env("ROUNDHOUSE_PARAM_BINDS", mode) + .output().unwrap(); + assert!(output.status.success(), "binds={mode}\n{}\n{}", + String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr)); + print!("{}", String::from_utf8_lossy(&output.stdout)); + } + return; + } + let (dir, errors) = emit_and_run::empty_app() + .write("app/models/application_record.rb", "class ApplicationRecord < ActiveRecord::Base\n self.abstract_class = true\nend\n") + .write("app/controllers/application_controller.rb", "class ApplicationController < ActionController::Base\nend\n") + .write("config/routes.rb", "Rails.application.routes.draw do\nend\n") + .write("db/schema.rb", r#" +ActiveRecord::Schema[8.1].define(version: 1) do + create_table :indexed_rows do |t| + t.integer :a + end + create_table :paired_rows do |t| + t.integer :a + t.integer :b + end + create_table :host_rows do |t| + end + create_table :joined_rows do |t| + t.integer :a + end + create_table :wide_rows do |t| + t.integer :a + t.integer :b + t.integer :c + t.integer :d + t.integer :e + t.integer :f + t.integer :g + t.integer :h + end +end +"#) + .write("app/models/indexed_row.rb", r#" +class IndexedRow < ApplicationRecord + def matching(value) + @a = value.nil? ? nil : value.to_i + IndexedRow.where(a: @a).count + end +end +"#) + .write("app/models/paired_row.rb", r#" +class PairedRow < ApplicationRecord + def matching(first, second) + @a = first.nil? ? nil : first.to_i + @b = second.nil? ? nil : second.to_i + PairedRow.where(a: @a, b: @b).count + end +end +"#) + .write("app/models/joined_row.rb", r#" +class JoinedRow < ApplicationRecord + def matching(value) + @a = value.nil? ? nil : value.to_i + JoinedRow.where(a: @a).count + end +end +"#) + .write("app/models/wide_row.rb", r#" +class WideRow < ApplicationRecord + def matching(a, b, c, d, e, f, g, h) + @a = a.nil? ? nil : a.to_i + @b = b.nil? ? nil : b.to_i + @c = c.nil? ? nil : c.to_i + @d = d.nil? ? nil : d.to_i + @e = e.nil? ? nil : e.to_i + @f = f.nil? ? nil : f.to_i + @g = g.nil? ? nil : g.to_i + @h = h.nil? ? nil : h.to_i + WideRow.where(a: @a, b: @b, c: @c, d: @d, e: @e, f: @f, g: @g, h: @h).count + end +end +"#) + .emit(BuildTarget::Ruby); + assert!(errors.is_empty(), "{}", errors.join("\n")); + let bound = std::env::var("ROUNDHOUSE_PARAM_BINDS").unwrap() == "1"; + for model in ["indexed_row", "paired_row", "joined_row", "wide_row"] { + let source = std::fs::read_to_string(dir.join(format!("app/models/{model}.rb"))).unwrap(); + let method = source.split_once(" def matching(").unwrap().1.split_once("\n end").unwrap().0; + assert!(!method.contains(".where("), "read was not lowered: {method}"); + assert_eq!(method.contains("Db.bind_"), bound, "{method}"); + if model == "wide_row" && bound { + assert!(method.contains("Db.prepare_uncached("), "eight nullable predicates must bypass the 128-shape cache: {method}"); + } else { + assert!(method.contains("Db.prepare("), "{method}"); + } + } + std::fs::write(dir.join("planner_gate.rb"), include_str!("param_binds_planner.rb")).unwrap(); + let output = emit_and_run::ruby().arg("planner_gate.rb").current_dir(&dir) + .env("PLANNER_BINDS", if bound { "1" } else { "0" }).output().unwrap(); + assert!(output.status.success(), "{}\n{}\n{}", dir.display(), + String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr)); + print!("{}", String::from_utf8_lossy(&output.stdout)); + std::fs::remove_dir_all(dir.parent().unwrap()).unwrap(); +} From 281df2809e4dc3098119922534df2c1fb26ef628 Mon Sep 17 00:00:00 2001 From: Bunsy <268838774+bunnykong@users.noreply.github.com> Date: Sun, 4 Oct 2026 09:13:59 +0800 Subject: [PATCH 2/4] Match bound storage values and release failed text conversion Keep text/BLOB and nullable boolean binding consistent with inline writes: preserve NUL/non-ASCII binary bytes, UTF-8 text, SQL NULL and false. Include to_s, encoding checks and conversion in the text binder failure handler so exceptions release the checkout before a caller rescues inside the connection lease. CRuby and JRuby reuse their existing failure paths. Tests: binary escaping, bool binding and the shared CRuby/Spinel runtime gate exercise storage classes, bytes and nil/false alternation. param_binds_cleanup adds six CRuby encoding/to_s failures and asserts zero owned statements within the same lease. The old preprocessing path fails that assertion on the first UTF-16LE value. Co-Authored-By: Claude Opus 5.5 --- runtime/ruby/db.rbs | 3 +- runtime/spinel/db.rb | 8 ++++- runtime/spinel/db_cruby.rb | 17 ++++++++-- runtime/spinel/db_jruby.rb | 29 +++++++++++++++- tests/db_bind_bool.rs | 40 ++++++++++++++++++++++ tests/db_escape_binary.rs | 51 ++++++++++++++++++++++++++++ tests/param_binds.rs | 16 ++++++++- tests/param_binds_cleanup.rs | 24 +++++++++++++ tests/param_binds_runtime.rb | 56 +++++++++++++++++++++++++++++++ tests/param_binds_text_cleanup.rb | 35 +++++++++++++++++++ 10 files changed, 273 insertions(+), 6 deletions(-) create mode 100644 tests/db_bind_bool.rs create mode 100644 tests/param_binds_cleanup.rs create mode 100644 tests/param_binds_text_cleanup.rb diff --git a/runtime/ruby/db.rbs b/runtime/ruby/db.rbs index 6cba3c5ae..da75046e4 100644 --- a/runtime/ruby/db.rbs +++ b/runtime/ruby/db.rbs @@ -36,7 +36,8 @@ module Db # (cruby-gem + spinel-FFI) implements these. def self.bind_int: (Integer, Integer, Integer?) -> void def self.bind_text: (Integer, Integer, String) -> void - def self.bind_bool: (Integer, Integer, bool) -> void + # A nullable boolean binds SQL NULL, never the false/0 representation. + def self.bind_bool: (Integer, Integer, bool?) -> void def self.bind_int_opt: (Integer, Integer, Integer?) -> void def self.bind_text_opt: (Integer, Integer, String?) -> void def self.bind_bool_opt: (Integer, Integer, bool?) -> void diff --git a/runtime/spinel/db.rb b/runtime/spinel/db.rb index 19550a8f0..b0dff658e 100644 --- a/runtime/spinel/db.rb +++ b/runtime/spinel/db.rb @@ -1847,6 +1847,8 @@ def self.bind_bool_opt(stmt, idx, value) def self.bind_text(stmt, idx, value) raise "Db.bind failed (21): cannot bind a replay cursor" if stmt.is_a?(Integer) + # This shim's inline writer uses TEXT regardless of Ruby encoding. + # Preserve all bytes at the FFI boundary, including embedded NULs. current_conn.bind_checked(stmt, idx, SQL.sqlite3_bind_text(stmt, idx, value, value.bytesize, -1)) end @@ -1854,7 +1856,11 @@ def self.bind_text(stmt, idx, value) # form and the INTEGER affinity `t.boolean` columns get. def self.bind_bool(stmt, idx, value) raise "Db.bind failed (21): cannot bind a replay cursor" if stmt.is_a?(Integer) - current_conn.bind_checked(stmt, idx, SQL.sqlite3_bind_int64(stmt, idx, value ? 1 : 0)) + if value.nil? + current_conn.bind_checked(stmt, idx, SQL.sqlite3_bind_null(stmt, idx)) + else + current_conn.bind_checked(stmt, idx, SQL.sqlite3_bind_int64(stmt, idx, value ? 1 : 0)) + end end def self.last_insert_rowid diff --git a/runtime/spinel/db_cruby.rb b/runtime/spinel/db_cruby.rb index 2fe37808a..cd0e7a232 100644 --- a/runtime/spinel/db_cruby.rb +++ b/runtime/spinel/db_cruby.rb @@ -966,11 +966,24 @@ def self.bind_bool_opt(handle, idx, value) end def self.bind_text(handle, idx, value) - bind_value(handle, idx, value) + value = value.to_s + # Match escape_string's storage class exactly. The gem otherwise binds + # every BINARY string as BLOB and every UTF-8 string (even NUL) as TEXT; + # SQLite equality does not equate the same bytes across those classes. + if value.include?("\0") || (value.encoding == Encoding::BINARY && !value.ascii_only?) + bind_value(handle, idx, value.b) + elsif value.encoding == Encoding::BINARY + bind_value(handle, idx, value.encode(Encoding::UTF_8)) + else + bind_value(handle, idx, value) + end + rescue StandardError => error + # Encoding checks/conversion can raise before bind_value is entered. + statement_failed(handle, "bind", error) end def self.bind_bool(handle, idx, value) - bind_value(handle, idx, value ? 1 : 0) + bind_value(handle, idx, value.nil? ? nil : (value ? 1 : 0)) end def self.last_insert_rowid diff --git a/runtime/spinel/db_jruby.rb b/runtime/spinel/db_jruby.rb index 567295080..6aa778b69 100644 --- a/runtime/spinel/db_jruby.rb +++ b/runtime/spinel/db_jruby.rb @@ -656,7 +656,7 @@ def self.bind_text_opt(handle, idx, value) if value.nil? ps.set_null(idx, Java::JavaSql::Types::VARCHAR) else - ps.set_string(idx, value) + bind_text(handle, idx, value) end rescue StandardError => error statement_failed(handle, "bind", error) @@ -674,6 +674,33 @@ def self.bind_bool_opt(handle, idx, value) statement_failed(handle, "bind", error) end + # Match this shim's inline writer, including ASCII-only BINARY strings + # remaining TEXT. set_bytes keeps NUL and non-ASCII binary data out of + # Java String decoding; both JDBC setters copy the Ruby value at bind time. + def self.bind_text(stmt, idx, value) + pstmt = stmt.pstmt + return nil if pstmt.nil? + value = value.to_s + if value.include?("\0") || (value.encoding == Encoding::BINARY && !value.ascii_only?) + pstmt.set_bytes(idx, value.to_java_bytes) + else + pstmt.set_string(idx, value) + end + rescue StandardError => error + statement_failed(stmt, "bind", error) + end + + # SQLite boolean values are integers, with NULL distinct from false/0. + def self.bind_bool(stmt, idx, value) + pstmt = stmt.pstmt + return nil if pstmt.nil? + if value.nil? + pstmt.set_null(idx, Java::JavaSql::Types::INTEGER) + else + pstmt.set_int(idx, value ? 1 : 0) + end + end + def self.last_insert_rowid st = current_dbh.raw.create_statement rs = st.execute_query("SELECT last_insert_rowid()") diff --git a/tests/db_bind_bool.rs b/tests/db_bind_bool.rs new file mode 100644 index 000000000..b1232d376 --- /dev/null +++ b/tests/db_bind_bool.rs @@ -0,0 +1,40 @@ +//! JDBC setter contract; the CRuby/Spinel runtime proof lives in param_binds. + +use std::process::Command; + +#[test] +fn jruby_boolean_binding_distinguishes_null_from_false() { + let script = r#" +src = File.read("runtime/spinel/db_jruby.rb") +defn = src[/^ def self\.bind_bool\(stmt, idx, value\)\n.*?^ end\n/m] or abort "no bind_bool" +module Db; end +module Java + module JavaSql + module Types + INTEGER = 4 + end + end +end +Db.module_eval(defn) +class JdbcBoolRecorder + attr_reader :call + def set_null(index, type); @call = [:null, index, type]; end + def set_int(index, value); @call = [:int, index, value]; end +end +handle = Struct.new(:pstmt).new(JdbcBoolRecorder.new) +[[false, [:int, 3, 0]], [nil, [:null, 3, 4]], [true, [:int, 3, 1]], + [nil, [:null, 3, 4]], [false, [:int, 3, 0]]].each do |value, expected| + Db.bind_bool(handle, 3, value) + abort handle.pstmt.call.inspect unless handle.pstmt.call == expected +end +Db.bind_bool(Struct.new(:pstmt).new(nil), 1, nil) +puts "JRuby nullable boolean setter contract passed (no JDBC runtime)" +"#; + let output = Command::new("ruby") + .args(["-e", script]) + .current_dir(env!("CARGO_MANIFEST_DIR")) + .output() + .expect("ruby"); + assert!(output.status.success(), "{}", String::from_utf8_lossy(&output.stderr)); + print!("{}", String::from_utf8_lossy(&output.stdout)); +} diff --git a/tests/db_escape_binary.rs b/tests/db_escape_binary.rs index 29007df45..c2e63bc3f 100644 --- a/tests/db_escape_binary.rs +++ b/tests/db_escape_binary.rs @@ -59,3 +59,54 @@ fn the_cruby_shim_writes_bytes_as_a_blob_literal() { fn the_jruby_shim_writes_bytes_as_a_blob_literal() { check("runtime/spinel/db_jruby.rb"); } + +#[test] +fn the_jruby_text_binder_selects_the_writers_storage_class() { + // Exercise the JDBC call boundary without requiring a JDK in the unit + // job. Actual JDBC execution belongs to the JRuby runtime lane. + let script = r##" +src = File.read("runtime/spinel/db_jruby.rb") +defn = src[/^ def self\.bind_text\(stmt, idx, value\)\n.*?^ end\n/m] or abort "no bind_text" +module Db; end +Db.module_eval(defn) +class String + def to_java_bytes; bytes; end +end +class JdbcBindRecorder + attr_reader :call + def set_bytes(index, value); @call = [:blob, index, value]; end + def set_string(index, value); @call = [:text, index, value.bytes]; end +end +Handle = Struct.new(:pstmt) +cases = [ + ["a\0b", :blob], + ["a\0b".b, :blob], + [[255, 254, 39].pack("C*"), :blob], + ["café".b, :blob], + ["abc".b, :text], + ["".b, :text], + ["it's", :text], + ["雪 café", :text], +] +recorder = JdbcBindRecorder.new +handle = Handle.new(recorder) +cases.each do |value, storage| + Db.bind_text(handle, 3, value) + expected = [storage, 3, value.bytes] + abort "#{value.inspect}: #{recorder.call.inspect} != #{expected.inspect}" if recorder.call != expected +end +Db.bind_text(Handle.new(nil), 1, "unused") +print "OK" +"##; + let out = Command::new("ruby") + .args(["-e", script]) + .current_dir(env!("CARGO_MANIFEST_DIR")) + .output() + .expect("ruby"); + assert!( + out.status.success(), + "{}", + String::from_utf8_lossy(&out.stderr) + ); + assert_eq!(String::from_utf8_lossy(&out.stdout), "OK"); +} diff --git a/tests/param_binds.rs b/tests/param_binds.rs index 9b9bf1686..ed0cfbef2 100644 --- a/tests/param_binds.rs +++ b/tests/param_binds.rs @@ -423,13 +423,27 @@ puts "runtime: CRuby interrupted reader keeps ownership across a different id" } else { include_str!("param_binds_cruby_cache.rb") }; + // Spinel's existing inline writer cannot put NUL in SQL text. Its + // bind-byte contract above still covers NUL; the gem/JDBC writers use + // BLOB literals, so the gem path also checks writer/reader parity. + let nul_writer = if native { + "" + } else { + r#" +inline_bound_string("NUL UTF-8", "a\0b") +inline_bound_string("NUL binary", "a\0b".b) +inline_bound_string("invalid UTF-8 tag", "\xFFa".force_encoding(Encoding::UTF_8)) +inline_bound_string("gem BLOB wrapper", SQLite3::Blob.new("plain-ascii")) +puts "runtime: NUL inline writes and bound reads agree" +"# + }; let script = format!( "{prelude}\nENV[\"DATABASE_POOL_SIZE\"] = \"1\"\n\ Db.configure(\"file:cache_cases?mode=memory&cache=shared\", pool_size: 1)\n\ {cache_cases}\nStatementCacheTest.new.run\n\ puts \"runtime: 12 statement cache ownership and error tests passed\"\nDb.close\n\ ENV[\"DATABASE_POOL_SIZE\"] = \"4\"\n\ - Db.configure(\"file:bind_runtime?mode=memory&cache=shared\", pool_size: 4)\n{body}\n{ownership}\nDb.close\n" + Db.configure(\"file:bind_runtime?mode=memory&cache=shared\", pool_size: 4)\n{body}\n{nul_writer}\n{ownership}\nDb.close\n" ); run_script(&dir, &script, native); std::fs::remove_dir_all(dir).expect("remove successful runtime probe"); diff --git a/tests/param_binds_cleanup.rs b/tests/param_binds_cleanup.rs new file mode 100644 index 000000000..13495701e --- /dev/null +++ b/tests/param_binds_cleanup.rs @@ -0,0 +1,24 @@ +//! Generated reads must release statements before a rescued failure returns +//! to an ongoing lease, including failures before the driver's bind method. + +#[path = "support/emit_and_run.rs"] +mod emit_and_run; + +use std::path::Path; +use std::process::Command; + +fn success(command: &mut Command) { + let output = command.output().unwrap_or_else(|e| panic!("{command:?}: {e}")); + assert!(output.status.success(), "{command:?}: {}\n{}\n{}", output.status, + String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr)); + print!("{}", String::from_utf8_lossy(&output.stdout)); +} + +#[test] +fn text_preprocessing_cleanup_ruby() { + success(emit_and_run::ruby().arg("-r") + .arg(Path::new(env!("CARGO_MANIFEST_DIR")).join("runtime/ruby/active_record/connection_pool.rb")) + .arg("-r") + .arg(Path::new(env!("CARGO_MANIFEST_DIR")).join("runtime/spinel/db_cruby.rb")) + .arg(Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/param_binds_text_cleanup.rb"))); +} diff --git a/tests/param_binds_runtime.rb b/tests/param_binds_runtime.rb index d04f1c7f3..be6caeaeb 100644 --- a/tests/param_binds_runtime.rb +++ b/tests/param_binds_runtime.rb @@ -130,6 +130,45 @@ def bind_ephemeral(stmt) Db.finalize(stmt) puts "runtime: quotes, UTF-8, long text, copy ownership and GC passed" +# Reads must match the values Db.exec writes inline, not merely round-trip +# through the bind API. In particular SQLite TEXT and BLOB with identical +# bytes are not equal, and ASCII-only BINARY strings are written as TEXT. +Db.exec("CREATE TABLE bind_string_rows (value TEXT NOT NULL)") +def inline_bound_string(label, value) + Db.exec("DELETE FROM bind_string_rows") + Db.exec("INSERT INTO bind_string_rows VALUES (" + Db.escape_string(value) + ")") + stored = Db.prepare("SELECT typeof(value), hex(value) FROM bind_string_rows") + raise "missing inline string" if !Db.step?(stored) + storage_type = Db.column_text(stored, 0) + storage_bytes = Db.column_text(stored, 1) + Db.finalize(stored) + + inline = Db.prepare("SELECT COUNT(*) FROM bind_string_rows WHERE value = " + Db.escape_string(value)) + raise "missing inline count" if !Db.step?(inline) + expect_int(label + " inline lookup", 1, Db.column_int(inline, 0)) + Db.finalize(inline) + bound = Db.prepare("SELECT COUNT(*) FROM bind_string_rows WHERE value = ?") + Db.bind_text(bound, 1, value) + raise "missing bound count" if !Db.step?(bound) + expect_int(label + " bound lookup", 1, Db.column_int(bound, 0)) + Db.finalize(bound) + + bytes = Db.prepare("SELECT typeof(?), hex(?)") + Db.bind_text(bytes, 1, value) + Db.bind_text(bytes, 2, value) + raise "missing bound bytes" if !Db.step?(bytes) + expect_text(label + " storage class", storage_type, Db.column_text(bytes, 0)) + expect_text(label + " stored bytes", storage_bytes, Db.column_text(bytes, 1)) + Db.finalize(bytes) +end +inline_bound_string("quotes", "quote's \"double\" ? -- SQL") +inline_bound_string("UTF-8", "雪 café 🦀") +inline_bound_string("ASCII binary", "plain-ascii".b) +inline_bound_string("empty binary", "".b) +inline_bound_string("UTF-8 binary", "café".b) +inline_bound_string("invalid UTF-8 binary", "\xFF\xFE'".b) +puts "runtime: inline writes and bound reads agree on text/binary storage and bytes" + # Observe bytes as a BLOB, not SQLite length(TEXT) or column_text's C-string # conversion: SQLite string expressions on embedded NUL are not specified. # The bound value must preserve all three bytes (61 00 62). @@ -139,3 +178,20 @@ def bind_ephemeral(stmt) expect_text("embedded NUL bytes", "610062", Db.column_text(stmt, 0)) Db.finalize(stmt) puts "runtime: embedded NUL bytes passed" + +# The primitive must keep nil distinct from false even if a caller bypasses +# the lowerer's nullable inline path. Alternate on one cached shape as well. +def expect_bound_bool(value, expected) + stmt = Db.prepare("SELECT COALESCE(?, -7)") + Db.bind_bool(stmt, 1, value) + raise "missing bool row" if !Db.step?(stmt) + expect_int("nullable bool", expected, Db.column_int(stmt, 0)) + Db.finalize(stmt) +end + +expect_bound_bool(false, 0) +expect_bound_bool(nil, -7) +expect_bound_bool(true, 1) +expect_bound_bool(nil, -7) +expect_bound_bool(false, 0) +puts "runtime: nullable boolean preserves SQL NULL passed" diff --git a/tests/param_binds_text_cleanup.rb b/tests/param_binds_text_cleanup.rb new file mode 100644 index 000000000..8e0f86a49 --- /dev/null +++ b/tests/param_binds_text_cleanup.rb @@ -0,0 +1,35 @@ +# Run with either CRuby's or JRuby's Db shim preloaded via -r. +def cleanup_owned_count + conn = Db.current_dbh + if RUBY_ENGINE == "jruby" + conn.open_statements.size + else + Db.open_statements(conn).size + end +end + +class CleanupBadText + def to_s + raise "text conversion failed" + end +end + +Db.configure(":memory:") +Db.with_connection do + ["x".encode("UTF-16LE"), CleanupBadText.new].each do |value| + 3.times do + stmt = Db.prepare("SELECT ? AS preprocessing_failure") + begin + Db.bind_text(stmt, 1, value) + raise "expected text preprocessing failure" + rescue StandardError => error + raise error unless error.message.include?("incompatible character encodings") || error.message.include?("text conversion failed") + end + count = cleanup_owned_count + raise "text preprocessing left #{count} owned statements inside the lease" unless count == 0 + Db.finalize(stmt) # Idempotent after the binder's own failure cleanup. + end + end +end +puts "cleanup: 6 text preprocessing failures; every owned count was zero before lease end" +Db.close From 9be9f033197e4985d1c6ac2a0e18b56ae357e204 Mon Sep 17 00:00:00 2001 From: Bunsy <268838774+bunnykong@users.noreply.github.com> Date: Sun, 4 Oct 2026 09:13:59 +0800 Subject: [PATCH 3/4] Serialize read arguments explicitly and finalize exceptional reads Select numeric conversion from the RHS type, not column nullability: scalar floats call to_s even against nullable columns; optional floats use an explicit nil-preserving conversion. Do not use a narrowing Cast for serialization, since later typing can erase it. Native Time/Date values use writer-compatible formatting while String filters remain text. A Ruby-family IR pass wraps each generated prepare/finalize lifetime in begin/ensure. Serialization after prepare, binds, stepping and hydration all finalize before an exception reaches the caller, including reloads, preloads and dynamic hydration. Strict-target emit remains untouched. Tests: param_binds_values runs scalar-to-nullable and optional-to-required Float, nil/non-nil Time, timezone/microsecond and string/date controls on CRuby and compiled Spinel. param_binds_cleanup asserts zero owned handles inside the lease after 21 inline and 24 bound generated failures on each runtime. Removing serialization or ensure fails the retained controls. Register value, plan and cleanup suites with the native CI planner. Co-Authored-By: Claude Opus 5.5 --- docs/pipeline/runtime.md | 5 + scripts/ci-plan.py | 29 +-- src/emit/ruby/library.rs | 63 +++++++ src/lower/arel/ruby_values.rs | 68 +++++++- tests/ci_plan_test.py | 25 ++- tests/param_binds_cleanup.rb | 54 ++++++ tests/param_binds_cleanup.rs | 101 +++++++++++ tests/param_binds_planner.rs | 2 +- tests/param_binds_raw_where.rb | 4 +- tests/param_binds_values.rb | 53 ++++++ tests/param_binds_values.rs | 310 +++++++++++++++++++++++++++++++++ 11 files changed, 687 insertions(+), 27 deletions(-) create mode 100644 tests/param_binds_cleanup.rb create mode 100644 tests/param_binds_values.rb create mode 100644 tests/param_binds_values.rs diff --git a/docs/pipeline/runtime.md b/docs/pipeline/runtime.md index 3007e555f..262b84d96 100644 --- a/docs/pipeline/runtime.md +++ b/docs/pipeline/runtime.md @@ -184,6 +184,11 @@ budget use `prepare_uncached`, avoiding exponential growth within a long lease. Existing per-connection cache limits still apply across query sites. Strict-target lowering retains its existing predicates and lifecycle. +Generated Ruby-family reads use `ensure Db.finalize(stmt)` around binding, +serialization after prepare, stepping and hydration, including reload and +preloads. Text preprocessing failures also release the binder's checkout. +Cleanup therefore completes before a caller rescues within an ongoing lease. + For each target: - **Emitter assumes** specific function names, signatures, and diff --git a/scripts/ci-plan.py b/scripts/ci-plan.py index 92f31aab9..c10788588 100755 --- a/scripts/ci-plan.py +++ b/scripts/ci-plan.py @@ -41,12 +41,13 @@ # must not fail the Ruby PR floor. PUBLICATION = [*BASE, "compare", "browser-smoke-typescript"] CORE = ["build-spinel", "toolchain-spinel", "compare-spinel"] +PARAM_BIND_TESTS = ["param_binds", "param_binds_values", "param_binds_planner", "param_binds_cleanup"] SPINEL_TESTS = [ "date_columns_spinel", "framework_tests_spinel", "spinel_web_push_crypto", "spinel_db_lease", - "param_binds", + *PARAM_BIND_TESTS, "spinel_stmt_cache_lru", "db_sqlite_concurrency", "spinel_param_builder", @@ -106,19 +107,23 @@ def native_coverage(path): suites.add(focused[1]) if path == "tests/support/db_concurrency_spinel.rb": suites.add("db_sqlite_concurrency") + # Gate drivers stay flat beside their Rust harness. Match the most + # specific suite first (e.g. param_binds_values before param_binds). + if path == "tests/param_binds_text_cleanup.rb": + suites.add("param_binds_cleanup") + elif path.startswith("tests/") and path.endswith(".rb"): + stem = path[len("tests/"):-len(".rb")] + for suite in reversed(PARAM_BIND_TESTS): + if stem == suite or stem.startswith(suite + "_"): + suites.add(suite) + break + if path == "runtime/spinel/test/statement_cache_cases.rb": + suites.add("param_binds") if path in { - "tests/param_binds_emit.rb", - "tests/param_binds_raw_where.rb", - "tests/param_binds_runtime.rb", - "tests/param_binds_cruby_cache.rb", - "tests/param_binds_spinel_cache.rb", - "tests/param_binds_associations.rb", - "tests/param_binds_nil.rb", - "runtime/spinel/test/statement_cache_cases.rb", "tests/support/emit_and_run.rs", "src/lower/model_to_library/adapter_emit.rs", - } or path.startswith("src/lower/arel/"): - suites.add("param_binds") + } or path.startswith(("src/lower/arel/", "src/lower/model_to_library/adapter_emit/")): + suites.update(PARAM_BIND_TESTS) if path.startswith(("runtime/spinel/", "runtime/ruby/")) and not interpreter_only: name = path.rsplit("/", 1)[-1] owned_tests = set() @@ -145,7 +150,7 @@ def native_coverage(path): owned_tests.update( ( "spinel_db_lease", - "param_binds", + *PARAM_BIND_TESTS, "spinel_stmt_cache_lru", "db_sqlite_concurrency", ) diff --git a/src/emit/ruby/library.rs b/src/emit/ruby/library.rs index b39748f6c..401003a9d 100644 --- a/src/emit/ruby/library.rs +++ b/src/emit/ruby/library.rs @@ -5856,6 +5856,67 @@ pub(super) fn emit_library_class_pair_with_synthesized( vec![rb, rbs] } +/// A lowered read owns its statement from prepare until finalize, including +/// argument serialization and row hydration. Release within that scope even +/// when the caller rescues inside a longer connection lease. This Ruby-only +/// pass leaves strict-target IR unchanged and covers reloads and preloads too. +fn read_statement_cleanup(lc: &LibraryClass) -> std::borrow::Cow<'_, LibraryClass> { + fn db_send(expr: &Expr, name: &str) -> bool { + matches!(&*expr.node, ExprNode::Send { recv: Some(recv), method, .. } + if method.as_str() == name && matches!(&*recv.node, + ExprNode::Const { path } if path.len() == 1 && path[0].as_str() == "Db")) + } + fn prepare(expr: &Expr) -> bool { + db_send(expr, "prepare") || db_send(expr, "prepare_uncached") + } + fn prepared_name(expr: &Expr) -> Option<&Symbol> { + match &*expr.node { + ExprNode::Assign { target: LValue::Var { name, .. }, value } if prepare(value) => Some(name), + _ => None, + } + } + fn finalize(expr: &Expr, name: &Symbol) -> bool { + db_send(expr, "finalize") && matches!(&*expr.node, + ExprNode::Send { args, .. } if args.len() == 1 + && matches!(&*args[0].node, ExprNode::Var { name: actual, .. } if actual == name)) + } + fn needs_cleanup(expr: &Expr) -> bool { + let mut found = prepared_name(expr).is_some(); + expr.node.for_each_child(&mut |child| found |= needs_cleanup(child)); + found + } + fn rewrite(expr: &mut Expr) { + expr.node.for_each_child_mut(&mut rewrite); + let ExprNode::Seq { exprs } = expr.node.as_mut() else { return }; + let mut start = 0; + while start < exprs.len() { + if let Some(name) = prepared_name(&exprs[start]) { + if let Some(end) = (start + 1..exprs.len()).find(|&i| finalize(&exprs[i], name)) { + let mut work: Vec<_> = exprs.drain(start + 1..=end).collect(); + let cleanup = work.pop().unwrap(); + let body = Expr::new(Span::synthetic(), ExprNode::Seq { exprs: work }); + exprs.insert(start + 1, Expr::new(Span::synthetic(), ExprNode::BeginRescue { + body, + rescues: vec![], + else_branch: None, + ensure: Some(cleanup), + implicit: false, + })); + } + } + start += 1; + } + } + if !lc.methods.iter().any(|m| needs_cleanup(&m.body)) { + return std::borrow::Cow::Borrowed(lc); + } + let mut adapted = lc.clone(); + for method in &mut adapted.methods { + rewrite(&mut method.body); + } + std::borrow::Cow::Owned(adapted) +} + /// Emit a group of LibraryFunctions sharing a `module_path` as a /// single Ruby file. Mirrors `typescript::library::emit_module_file` /// — converts the function group into a synthetic @@ -6006,6 +6067,8 @@ pub(super) fn emit_library_class_decl_with_synthesized( out_path: PathBuf, synthesized_siblings: &[(String, String)], ) -> EmittedFile { + let guarded = read_statement_cleanup(lc); + let lc = guarded.as_ref(); // The one chokepoint every library-shape file goes through, and the // only place that knows which class is being emitted — the send // emitter is a free function reached from a dozen callers. A reopen diff --git a/src/lower/arel/ruby_values.rs b/src/lower/arel/ruby_values.rs index 5591e081a..e3b51670f 100644 --- a/src/lower/arel/ruby_values.rs +++ b/src/lower/arel/ruby_values.rs @@ -1,7 +1,9 @@ //! Ruby-family read values. Strict targets retain their existing value //! representation until their nullable runtime and bind capabilities land. -use crate::schema::Schema; +use crate::expr::{Expr, ExprNode, Literal}; +use crate::ident::Symbol; +use crate::schema::{ColumnType, Schema}; use crate::ty::Ty; use super::ir::{ArelOp, Predicate, Value, ValueType}; @@ -21,10 +23,16 @@ fn normalize_predicate(predicate: &mut Predicate, schema: &Schema) { normalize_predicate(right, schema); } Predicate::Eq(col, Value::Runtime { expr, ty }) => { - let nullable_column = schema.tables.get(&col.table.0) - .and_then(|table| table.columns.iter().find(|c| c.name == col.column)) - .is_some_and(|column| column.nullable && !column.primary_key); - let nullable = nullable_column || expr.ty.as_ref().is_some_and(is_nullable); + let column = schema.tables.get(&col.table.0) + .and_then(|table| table.columns.iter().find(|c| c.name == col.column)); + let nullable_column = column.is_some_and(|column| column.nullable && !column.primary_key); + let nullable_value = expr.ty.as_ref().is_some_and(is_nullable); + let nullable = nullable_column || nullable_value; + if let Some(column) = column { + // A nullable column does not make a scalar RHS optional: + // Float still needs to_s; only an optional RHS preserves nil. + *expr = serialized_value_expr(expr, &column.col_type, nullable_value); + } if nullable { *ty = match ty { ValueType::Int | ValueType::IntOpt => ValueType::IntOpt, @@ -41,6 +49,56 @@ fn normalize_predicate(predicate: &mut Predicate, schema: &Schema) { } } +/// Only native temporal values need the writer's formatter. A String ivar +/// (for example a date filter from params) already contains SQL text and +/// must stay a String. Explicit numeric conversion preserves nil; an IR +/// narrowing Cast can be elided after later typing and is not serialization. +fn serialized_value_expr(expr: &Expr, column: &ColumnType, nullable: bool) -> Expr { + let text_ty = if nullable { Ty::Union { variants: vec![Ty::Str, Ty::Nil] } } else { Ty::Str }; + let converted = match column { + ColumnType::Float | ColumnType::Decimal { .. } => { + let text = Expr::new(expr.span, ExprNode::Send { + recv: Some(expr.clone()), method: Symbol::from("to_s"), args: vec![], + block: None, parenthesized: false, + }); + if nullable { + // The builder admits only pure ivar reads here. Keep this + // semantic conversion explicit through subsequent typing. + Expr::new(expr.span, ExprNode::If { + cond: Expr::new(expr.span, ExprNode::Send { + recv: Some(expr.clone()), method: Symbol::from("nil?"), args: vec![], + block: None, parenthesized: false, + }), + then_branch: Expr::new(expr.span, ExprNode::Lit { value: Literal::Nil }), + else_branch: text, + }) + } else { + text + } + } + ColumnType::Date | ColumnType::DateTime | ColumnType::Time => { + let method = match expr.ty.as_ref().map(Ty::peel_nilable) { + Some(Ty::Time) => "format_db_time", + Some(Ty::Date) => "format_db_date", + // App RBS signatures may still spell the native type as + // a root class; nominal namespaced classes are not Time. + Some(Ty::Class { id, args }) if args.is_empty() && id.0.as_str() == "Time" => "format_db_time", + Some(Ty::Class { id, args }) if args.is_empty() && id.0.as_str() == "Date" => "format_db_date", + _ => return expr.clone(), + }; + Expr::new(expr.span, ExprNode::Send { + recv: Some(Expr::new(expr.span, ExprNode::Const { path: vec![Symbol::from("ActiveSupport")] })), + method: Symbol::from(method), args: vec![expr.clone()], + block: None, parenthesized: true, + }) + } + ColumnType::Integer | ColumnType::BigInt | ColumnType::Reference { .. } + | ColumnType::Boolean | ColumnType::String { .. } | ColumnType::Text + | ColumnType::Binary | ColumnType::Json | ColumnType::Uuid => return expr.clone(), + }; + crate::lower::typing::with_ty(converted, text_ty) +} + fn is_nullable(ty: &Ty) -> bool { match ty { Ty::Nil => true, diff --git a/tests/ci_plan_test.py b/tests/ci_plan_test.py index d17d97853..6dad17943 100644 --- a/tests/ci_plan_test.py +++ b/tests/ci_plan_test.py @@ -408,7 +408,10 @@ def test_param_binds_owns_lowering_drivers_and_database_runtime(self): ]: with self.subTest(path=path): plan = ci.select([path]) - self.assertEqual(plan["spinel_tests"], ["param_binds"]) + self.assertEqual( + plan["spinel_tests"], + ci.PARAM_BIND_TESTS if path.startswith("src/") or path == "tests/support/emit_and_run.rs" else ["param_binds"], + ) self.assertEqual( self.extras(plan), set(ci.CORE) | {"framework-tests-spinel"} ) @@ -416,7 +419,7 @@ def test_param_binds_owns_lowering_drivers_and_database_runtime(self): ci.select(["runtime/spinel/db.rb"])["spinel_tests"], [ "spinel_db_lease", - "param_binds", + *ci.PARAM_BIND_TESTS, "spinel_stmt_cache_lru", "db_sqlite_concurrency", ], @@ -426,7 +429,7 @@ def test_param_binds_owns_lowering_drivers_and_database_runtime(self): [ "date_columns_spinel", "spinel_db_lease", - "param_binds", + *ci.PARAM_BIND_TESTS, "spinel_stmt_cache_lru", "db_sqlite_concurrency", ], @@ -457,6 +460,14 @@ def test_jdbc_probes_select_the_existing_comparison_without_archives(self): self.assertEqual(plan["smoke"], []) self.assertEqual(plan["archives"], []) + def test_param_bind_suite_drivers_select_their_native_harness(self): + for suite in ci.PARAM_BIND_TESTS: + for suffix in (".rs", ".rb", "_runtime.rb"): + with self.subTest(suite=suite, suffix=suffix): + plan = ci.select(["tests/" + suite + suffix]) + self.assertEqual(plan["spinel_tests"], [suite]) + self.assertIn("framework-tests-spinel", plan["jobs"]) + def test_runtime_owners_choose_asymmetric_focused_binaries(self): cases = { "runtime/spinel/web_push_crypto.rb": "spinel_web_push_crypto", @@ -504,18 +515,18 @@ def test_shared_runtime_and_driver_inputs_select_real_harnesses(self): "tests/spinel_stmt_cache_lru.rb": ["spinel_stmt_cache_lru"], "tests/support/db_concurrency_spinel.rb": ["db_sqlite_concurrency"], "runtime/spinel/db.rb": [ - "spinel_db_lease", "param_binds", "spinel_stmt_cache_lru", + "spinel_db_lease", *ci.PARAM_BIND_TESTS, "spinel_stmt_cache_lru", "db_sqlite_concurrency", ], "runtime/spinel/sqlite_adapter.rb": [ "date_columns_spinel", "spinel_db_lease", - "param_binds", + *ci.PARAM_BIND_TESTS, "spinel_stmt_cache_lru", "db_sqlite_concurrency", ], "runtime/spinel/active_support_time_parsing.rb": [ - "spinel_db_lease", "param_binds", "spinel_stmt_cache_lru", + "spinel_db_lease", *ci.PARAM_BIND_TESTS, "spinel_stmt_cache_lru", "db_sqlite_concurrency", ], "runtime/spinel/date.rb": ["date_columns_spinel"], @@ -562,7 +573,7 @@ def test_union_and_deletion_paths_keep_each_owner(self): "date_columns_spinel", "spinel_web_push_crypto", "spinel_db_lease", - "param_binds", + *ci.PARAM_BIND_TESTS, "spinel_stmt_cache_lru", "db_sqlite_concurrency", ], diff --git a/tests/param_binds_cleanup.rb b/tests/param_binds_cleanup.rb new file mode 100644 index 000000000..956ce005d --- /dev/null +++ b/tests/param_binds_cleanup.rb @@ -0,0 +1,54 @@ +# Faults run inside the actual emitted serialization/bind/hydration methods. +# Every ownership assertion is inside the same lease as the rescued failure. +module CleanupFaults + @current = "" + def self.current=(value) + @current = value + end + def self.trip(value) + raise "injected cleanup " + value if value == @current + nil + end +end + +def expect_cleanup_failure(fault) + CleanupFaults.current = fault + begin + yield + raise "missing cleanup failure " + fault + rescue RuntimeError => error + raise error unless error.message.include?("injected cleanup " + fault) + ensure + CleanupFaults.current = "" + end + count = Db.cleanup_owned_count + raise fault + " left " + count.to_s + " owned statements inside the lease" unless count == 0 + nil +end + +SqliteAdapter.configure("file:cleanup_gate?mode=memory&cache=shared") +ActiveRecord.adapter = SqliteAdapter +Schema.statements.each { |sql| Db.exec(sql) } +Db.exec("INSERT INTO parents (id, name) VALUES (1, 'one')") +Db.exec("INSERT INTO readings (id, parent_id, recorded_at, label) VALUES (1, 1, '2023-11-14 22:13:20.000000', 'one')") +needle = Time.at(1700000000).utc +row = Reading.find(1) +controller = ParentsController.new +binds = ENV["ROUNDHOUSE_PARAM_BINDS"] == "1" +Db.with_connection do + 3.times do + expect_cleanup_failure("serialize") { row.time_count(needle) } + expect_cleanup_failure("bind") { row.time_count(needle) } if binds + expect_cleanup_failure("step") { row.time_count(needle) } + expect_cleanup_failure("hydrate") { row.single(1) } + expect_cleanup_failure("hydrate") { row.many(1) } + expect_cleanup_failure("reload") { row._adapter_reload } + expect_cleanup_failure("hydrate") { controller.preloaded } + expect_cleanup_failure("hydrate") { Reading._hydrate_all("SELECT id, parent_id, recorded_at, label FROM readings") } + raise "lease no longer usable" unless row.time_count(needle) == 1 + raise "successful recovery retained a statement" unless Db.cleanup_owned_count == 0 + end + true +end +puts "cleanup: " + (binds ? "24" : "21") + " rescued generated failures; every owned count was zero before lease end" +Db.close diff --git a/tests/param_binds_cleanup.rs b/tests/param_binds_cleanup.rs index 13495701e..eb5f74365 100644 --- a/tests/param_binds_cleanup.rs +++ b/tests/param_binds_cleanup.rs @@ -4,6 +4,7 @@ #[path = "support/emit_and_run.rs"] mod emit_and_run; +use roundhouse::project::BuildTarget; use std::path::Path; use std::process::Command; @@ -14,6 +15,106 @@ fn success(command: &mut Command) { print!("{}", String::from_utf8_lossy(&output.stdout)); } +fn inject(path: &Path, header: &str, fault: &str) { + let source = std::fs::read_to_string(path).unwrap(); + assert_eq!(source.matches(header).count(), 1, "{}: {header}", path.display()); + std::fs::write(path, source.replace(header, + &format!("{header}\n CleanupFaults.trip(\"{fault}\")"))).unwrap(); +} + +fn emitted(test: &str, target: BuildTarget) { + if std::env::var_os("ROUNDHOUSE_BINDS_CHILD").is_none() { + for mode in ["0", "1"] { + success(Command::new(std::env::current_exe().unwrap()) + .args(["--exact", test, "--include-ignored", "--nocapture"]) + .env("ROUNDHOUSE_BINDS_CHILD", "1").env("ROUNDHOUSE_PARAM_BINDS", mode)); + } + return; + } + let (dir, errors) = emit_and_run::empty_app() + .write("app/models/application_record.rb", "class ApplicationRecord < ActiveRecord::Base\n self.abstract_class = true\nend\n") + .write("app/controllers/application_controller.rb", "class ApplicationController < ActionController::Base\nend\n") + .write("app/controllers/parents_controller.rb", "class ParentsController < ApplicationController\n def index\n render plain: preloaded.length.to_s\n end\n def preloaded\n Parent.includes(:readings).to_a\n end\nend\n") + .write("config/routes.rb", "Rails.application.routes.draw do\n get '/parents', to: 'parents#index'\nend\n") + .write("sig/reading.rbs", "class Reading\n def time_count: (Time value) -> Integer\nend\n") + .write("db/schema.rb", r#" +ActiveRecord::Schema[8.1].define(version: 1) do + create_table :parents do |t| + t.string :name, null: false + end + create_table :readings do |t| + t.integer :parent_id, null: false + t.datetime :recorded_at, null: false + t.string :label, null: false + end +end +"#) + .write("app/models/parent.rb", r#" +class Parent < ApplicationRecord + has_many :readings +end +"#) + .write("app/models/reading.rb", r#" +class Reading < ApplicationRecord + belongs_to :parent + def time_count(value) + @needle = value + Reading.where(recorded_at: @needle).count + end + def single(value) + @id = value.to_i + Reading.find_by(id: @id) + end + def many(value) + @id = value.to_i + Reading.where(id: @id).to_a + end +end +"#).emit(target); + assert!(errors.is_empty(), "{}", errors.join("\n")); + let path = dir.join("app/models/reading.rb"); + let source = std::fs::read_to_string(&path).unwrap(); + for method in ["time_count(value)", "single(value)", "many(value)", "_adapter_reload"] { + let body = source.split_once(&format!(" def {method}\n")).unwrap().1 + .split_once("\n end").unwrap().0; + assert!(body.contains("\n ensure\n Db.finalize(stmt)"), "{method}: {body}"); + } + let parent = std::fs::read_to_string(dir.join("app/controllers/parents_controller.rb")).unwrap(); + assert!(parent.contains("ensure\n Db.finalize(__readings_stmt)"), "{parent}"); + inject(&path, " def self.from_stmt(stmt)", "hydrate"); + inject(&dir.join("runtime/active_support_time_parsing.rb"), " def self.format_db_time(value)", "serialize"); + let native = target == BuildTarget::Spinel; + inject(&dir.join("runtime/db.rb"), if native { " def self.bind_text(stmt, idx, value)" } else { " def self.bind_text(handle, idx, value)" }, "bind"); + inject(&dir.join("runtime/db.rb"), if native { " def self.step?(stmt)" } else { " def self.step?(handle)\n entry = handle" }, "step"); + inject(&dir.join("runtime/db.rb"), if native { " def self.column_int(stmt, i)" } else { " def self.column_int(handle, i)" }, "reload"); + let probe = if native { + "class DbConn\n def cleanup_owned_count\n @open.length\n end\nend\nmodule Db\n def self.cleanup_owned_count\n current_conn.cleanup_owned_count\n end\nend\n" + } else { + "module Db\n def self.cleanup_owned_count\n open_statements(current_dbh).size\n end\nend\n" + }; + let script = format!("require_relative \"boot\"\nrequire_relative \"app/models/parent\"\nrequire_relative \"app/models/reading\"\nrequire_relative \"app/controllers/parents_controller\"\n{probe}\n{}", include_str!("param_binds_cleanup.rb")); + std::fs::write(dir.join("cleanup_gate.rb"), script).unwrap(); + println!("cleanup tree: {} binds={}", dir.display(), std::env::var("ROUNDHOUSE_PARAM_BINDS").unwrap()); + if native { + success(Command::new(std::env::var("SPINEL").unwrap_or_else(|_| "spinel".into())) + .args(["cleanup_gate.rb", "-o", "cleanup_gate"]).current_dir(&*dir)); + success(Command::new(dir.join("cleanup_gate")).current_dir(&*dir)); + } else { + success(emit_and_run::ruby().arg("cleanup_gate.rb").current_dir(&*dir)); + } +} + +#[test] +fn generated_cleanup_ruby() { + emitted("generated_cleanup_ruby", BuildTarget::Ruby); +} + +#[test] +#[ignore = "requires Spinel (SPINEL=/path/to/spinel)"] +fn generated_cleanup_spinel() { + emitted("generated_cleanup_spinel", BuildTarget::Spinel); +} + #[test] fn text_preprocessing_cleanup_ruby() { success(emit_and_run::ruby().arg("-r") diff --git a/tests/param_binds_planner.rs b/tests/param_binds_planner.rs index 3f74048c4..3d63e0126 100644 --- a/tests/param_binds_planner.rs +++ b/tests/param_binds_planner.rs @@ -108,7 +108,7 @@ end } } std::fs::write(dir.join("planner_gate.rb"), include_str!("param_binds_planner.rb")).unwrap(); - let output = emit_and_run::ruby().arg("planner_gate.rb").current_dir(&dir) + let output = emit_and_run::ruby().arg("planner_gate.rb").current_dir(&*dir) .env("PLANNER_BINDS", if bound { "1" } else { "0" }).output().unwrap(); assert!(output.status.success(), "{}\n{}\n{}", dir.display(), String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr)); diff --git a/tests/param_binds_raw_where.rb b/tests/param_binds_raw_where.rb index 547de9168..9289d4c18 100644 --- a/tests/param_binds_raw_where.rb +++ b/tests/param_binds_raw_where.rb @@ -2,8 +2,8 @@ # never become the input to a later placeholder or replacement expansion. Db.with_connection do backslashes = %q{path\1\&\`\'} + "雪" - Db.exec("INSERT INTO items (id, parent_id, name) VALUES (1, 1, 'a'), (2, 1, 'what?')") - Db.exec("INSERT INTO items (id, parent_id, name) VALUES (3, 2, " + Db.escape_string(backslashes) + ")") + Db.exec("INSERT INTO items (id, parent_id, name, required_flag) VALUES (1, 1, 'a', 0), (2, 1, 'what?', 0)") + Db.exec("INSERT INTO items (id, parent_id, name, required_flag) VALUES (3, 2, " + Db.escape_string(backslashes) + ", 0)") scalar = ActiveRecord::Relation.new(Item).where("items.name = ? OR items.id = ?", "what?", 1) raise "scalar value consumed later placeholder" unless scalar.count == 2 diff --git a/tests/param_binds_values.rb b/tests/param_binds_values.rb new file mode 100644 index 000000000..d729b8d64 --- /dev/null +++ b/tests/param_binds_values.rb @@ -0,0 +1,53 @@ +def expect_count(label, expected, actual) + raise "#{label}: expected #{expected}, got #{actual}" if actual != expected +end + +# Exercise the emitted inline writers, not an independently formatted seed. +# The two nearby timestamps distinguish preserved microseconds from to_s. +t1 = Time.at(1_700_000_000, 123456).getlocal("-04:00") +t2 = Time.at(1_700_000_000, 123457).getlocal("+09:00") +values = [0.0, -1.25, 1.23456789012345, 1.25e-20, 1.25e20, -1.25e20, + 1.7976931348623157e308, -1.7976931348623157e308, + 2.2250738585072014e-308, 4.9406564584124654e-324] +values.each_with_index do |value, index| + row = Reading.new + row.ratio = value + row.optional_ratio = value + row.recorded_at = t1 + row.optional_at = t2 + row.save! + expect_count("float #{index}", 1, row.ratio_matches) + expect_count("optional float #{index}", 1, row.optional_ratio_matches) + expect_count("scalar float against nullable column #{index}", 1, row.scalar_optional_ratio_matches) + expect_count("nullable float against required column #{index}", 1, row.required_ratio_matches_value(value)) + expect_count("timestamp #{index}", index + 1, row.time_matches) + expect_count("optional timestamp #{index}", index + 1, row.optional_time_matches) + expect_count("mixed predicate #{index}", 1, row.pair_matches) +end + +row = Reading.new +row.ratio = 42.5 +row.recorded_at = t2 +row.save! +expect_count("nearby timestamp", 1, row.time_matches) +expect_count("nil float", 1, row.optional_ratio_matches) +expect_count("nil timestamp", 1, row.optional_time_matches) +expect_count("nil against required float", 0, row.required_ratio_matches_value(nil)) +expect_count("nil against required timestamp", 0, row.required_time_matches_value(nil)) +expect_count("value against required timestamp", 1, row.required_time_matches_value(t2)) + +# The same instant expressed in another timezone matches the stored value; +# the adjoining microsecond remains a separate row. +row.recorded_at = t1.getutc +expect_count("same instant UTC", values.length, row.time_matches) +row.recorded_at = t2.getutc +expect_count("microsecond retained", 1, row.time_matches) +expect_count("direct offset Time", values.length, + row.time_matches_value(Time.at(1_700_000_000, 123456).getlocal("-04:00"))) +puts "typed values: 10 scalar-to-nullable floats, Float/Time nil and non-nil against nullable/required columns, UTC/offset/microseconds, mixed predicates passed" + +expect_count("String timestamp stays text", values.length, row.string_time_matches) +expect_count("optional String timestamp", values.length, + row.string_time_matches_value("2023-11-14 22:13:20.123457")) +expect_count("nil String timestamp", 1, row.string_time_matches_value(nil)) +puts "typed values: String timestamp filters stay text; optional String binds NULL" diff --git a/tests/param_binds_values.rs b/tests/param_binds_values.rs new file mode 100644 index 000000000..df9a68034 --- /dev/null +++ b/tests/param_binds_values.rs @@ -0,0 +1,310 @@ +//! Float/temporal read contract for roundhouse#12. Values serialize the same +//! way before inline escaping or binding. Run each compiler flag in its own +//! process, like param_binds.rs. + +#[path = "support/emit_and_run.rs"] +mod emit_and_run; + +use roundhouse::project::BuildTarget; +use std::process::Command; + +fn overlay() -> emit_and_run::Overlay { + emit_and_run::empty_app() + .write( + "sig/reading.rbs", + "class Reading\n def time_matches_value: (Time value) -> Integer\n def required_time_matches_value: (Time? value) -> Integer\n def required_ratio_matches_value: (Float? value) -> Integer\n def string_time_matches_value: (String? value) -> Integer\nend\n", + ) + .write( + "app/models/application_record.rb", + "class ApplicationRecord < ActiveRecord::Base\n self.abstract_class = true\nend\n", + ) + .write( + "app/controllers/application_controller.rb", + "class ApplicationController < ActionController::Base\nend\n", + ) + .write( + "config/routes.rb", + "Rails.application.routes.draw do\nend\n", + ) + .write( + "db/schema.rb", + r#" +ActiveRecord::Schema[8.1].define(version: 1) do + create_table "readings", force: :cascade do |t| + t.float "ratio", null: false + t.float "optional_ratio" + t.datetime "recorded_at", null: false + t.datetime "optional_at" + end +end +"#, + ) + .write( + "app/models/reading.rb", + r#" +class Reading < ApplicationRecord + def ratio_matches + @number = ratio.to_f + Reading.where(ratio: @number).count + end + def optional_ratio_matches + Reading.where(optional_ratio: @optional_ratio).count + end + def scalar_optional_ratio_matches + @number = ratio.to_f + Reading.where(optional_ratio: @number).count + end + def required_ratio_matches_value(value) + @nullable_number = value + Reading.where(ratio: @nullable_number).count + end + def time_matches + @needle = recorded_at + Reading.where(recorded_at: @needle).count + end + def optional_time_matches + @optional_needle = optional_at + Reading.where(optional_at: @optional_needle).count + end + def time_matches_value(value) + @bound_time = value + Reading.where(recorded_at: @bound_time).count + end + def required_time_matches_value(value) + @nullable_time = value + Reading.where(recorded_at: @nullable_time).count + end + def string_time_matches + @string_needle = "2023-11-14 22:13:20.123456" + Reading.where(recorded_at: @string_needle).count + end + def string_time_matches_value(value) + @optional_string_needle = value + Reading.where(optional_at: @optional_string_needle).count + end + def pair_matches + @key = id.to_i + @number = ratio.to_f + @needle = recorded_at + Reading.where(id: @key, ratio: @number, recorded_at: @needle).count + end +end +"#, + ) +} + +fn success(command: &mut Command) { + let output = command + .output() + .unwrap_or_else(|e| panic!("{command:?}: {e}")); + assert!( + output.status.success(), + "{command:?}: {}\n{}\n{}", + output.status, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + print!("{}", String::from_utf8_lossy(&output.stdout)); +} + +fn emitted(test: &str, target: BuildTarget) { + if std::env::var_os("ROUNDHOUSE_BINDS_CHILD").is_none() { + for mode in ["0", "1"] { + println!("{test}: ROUNDHOUSE_PARAM_BINDS={mode}"); + success( + Command::new(std::env::current_exe().unwrap()) + .args(["--exact", test, "--include-ignored", "--nocapture"]) + .env("ROUNDHOUSE_BINDS_CHILD", "1") + .env("ROUNDHOUSE_PARAM_BINDS", mode), + ); + } + return; + } + let (dir, errors) = overlay().emit(target); + assert!(errors.is_empty(), "{}", errors.join("\n")); + let source = std::fs::read_to_string(dir.join("app/models/reading.rb")).unwrap(); + for method in [ + "ratio_matches", + "optional_ratio_matches", + "scalar_optional_ratio_matches", + "required_ratio_matches_value(value)", + "time_matches", + "optional_time_matches", + "time_matches_value(value)", + "required_time_matches_value(value)", + "string_time_matches", + "string_time_matches_value(value)", + "pair_matches", + ] { + let header = format!(" def {method}\n"); + let body = source + .split_once(&header) + .unwrap() + .1 + .split_once("\n end") + .unwrap() + .0; + assert!(body.contains("Db.prepare("), "{header}{body}"); + assert!(!body.contains("Reading.where("), "{header}{body}"); + if method.starts_with("optional_") && std::env::var("ROUNDHOUSE_PARAM_BINDS").unwrap() == "1" { + assert!(body.contains(" IS NULL"), "{header}{body}"); + assert!(body.contains(" = ?"), "{header}{body}"); + assert!(!body.contains(" IS ?"), "{header}{body}"); + assert!(body.contains("Db.bind_text_opt("), "{header}{body}"); + } + if method == "ratio_matches" || method == "scalar_optional_ratio_matches" { + assert!(body.contains("@number.to_s"), "{header}{body}"); + let bind = if method == "ratio_matches" { "Db.bind_text(" } else { "Db.bind_text_opt(" }; + assert_eq!( + body.contains(bind), + std::env::var("ROUNDHOUSE_PARAM_BINDS").unwrap() == "1", + "{header}{body}" + ); + } + if method == "required_ratio_matches_value(value)" { + assert!(body.contains(".to_s"), "nullable Float arguments must serialize before binding: {header}{body}"); + } + if method.starts_with("string_time") { + assert!(!body.contains("ActiveSupport.format_db_time("), "{header}{body}"); + let bind = if method == "string_time_matches" { "Db.bind_text(" } else { "Db.bind_text_opt(" }; + assert_eq!(body.contains(bind), std::env::var("ROUNDHOUSE_PARAM_BINDS").unwrap() == "1", "{header}{body}"); + } else if method.contains("time") || method == "pair_matches" { + assert!( + body.contains("ActiveSupport.format_db_time("), + "{header}{body}" + ); + } + if method == "time_matches_value(value)" { + assert_eq!( + body.contains("Db.bind_text("), + std::env::var("ROUNDHOUSE_PARAM_BINDS").unwrap() == "1", + "{header}{body}" + ); + } + if method == "pair_matches" && std::env::var("ROUNDHOUSE_PARAM_BINDS").unwrap() == "1" { + assert!( + body.contains("Db.bind_int(stmt, 1, @key)"), + "{header}{body}" + ); + assert!( + body.contains("Db.bind_text(stmt, 2, @number.to_s)"), + "{header}{body}" + ); + assert!( + body.contains("Db.bind_text(stmt, 3, ActiveSupport.format_db_time(@needle))"), + "{header}{body}" + ); + } + } + let script = format!( + r#"require_relative "boot" +require_relative "app/models/reading" +SqliteAdapter.configure("file:bind_values?mode=memory&cache=shared") +ActiveRecord.adapter = SqliteAdapter +Schema.statements.each {{ |sql| Db.exec(sql) }} +{} +Db.close +"#, + include_str!("param_binds_values.rb") + ); + std::fs::write(dir.join("bind_values.rb"), script).unwrap(); + println!("value gate tree: {}", dir.display()); + if target == BuildTarget::Spinel { + success( + Command::new(std::env::var("SPINEL").unwrap_or_else(|_| "spinel".into())) + .args(["bind_values.rb", "-o", "bind_values"]) + .current_dir(&*dir), + ); + success(Command::new(dir.join("bind_values")).current_dir(&*dir)); + } else { + success(Command::new("ruby").arg("bind_values.rb").current_dir(&*dir)); + } +} + +#[test] +fn typed_values_ruby() { + emitted("typed_values_ruby", BuildTarget::Ruby); +} + +#[test] +#[ignore = "requires Spinel (SPINEL=/path/to/spinel)"] +fn typed_values_spinel() { + emitted("typed_values_spinel", BuildTarget::Spinel); +} + +#[test] +fn string_temporal_ivar_stays_a_string_on_strict_targets() { + for (target, path, method, forbidden) in [ + (BuildTarget::Go, "app/v2/article.go", "SinceStringCount", "Rh_format_db_time(self.Since)"), + (BuildTarget::Rust, "src/models/article.rs", "since_string_count", "self.since.map(crate::rh_datetime::format_db_time)"), + ] { + let (dir, errors) = emit_and_run::real_blog().edit( + "app/models/article.rb", + "class Article < ApplicationRecord", + "class Article < ApplicationRecord\n def since_string_count\n @since = \"2020-01-01 00:00:00.000000\"\n Article.where(created_at: @since).count\n end", + ).emit(target); + assert!(errors.is_empty(), "{}", errors.join("\n")); + let source = std::fs::read_to_string(dir.join(path)).unwrap(); + assert!(source.contains(method), "{target:?}: missing method"); + assert!(!source.contains(forbidden), "{target:?}: a String was passed to the native time formatter"); + } +} + +#[test] +fn date_predicates_ruby() { + if std::env::var_os("ROUNDHOUSE_BINDS_CHILD").is_none() { + for mode in ["0", "1"] { + success(Command::new(std::env::current_exe().unwrap()) + .args(["--exact", "date_predicates_ruby", "--nocapture"]) + .env("ROUNDHOUSE_BINDS_CHILD", "1").env("ROUNDHOUSE_PARAM_BINDS", mode)); + } + return; + } + let (dir, errors) = emit_and_run::empty_app() + .write("app/models/application_record.rb", "class ApplicationRecord < ActiveRecord::Base\n self.abstract_class = true\nend\n") + .write("app/controllers/application_controller.rb", "class ApplicationController < ActionController::Base\nend\n") + .write("config/routes.rb", "Rails.application.routes.draw do\nend\n") + .write("db/schema.rb", "ActiveRecord::Schema[8.1].define(version: 1) do\n create_table :calendar_entries do |t|\n t.date :due_on\n end\nend\n") + .write("sig/calendar_entry.rbs", "class CalendarEntry\n def date_matches: (Date? value) -> Integer\n def text_matches: (String? value) -> Integer\nend\n") + .write("app/models/calendar_entry.rb", r#" +class CalendarEntry < ApplicationRecord + def date_matches(value) + @date_needle = value + CalendarEntry.where(due_on: @date_needle).count + end + def text_matches(value) + @text_needle = value + CalendarEntry.where(due_on: @text_needle).count + end +end +"#).emit(BuildTarget::Ruby); + assert!(errors.is_empty(), "{}", errors.join("\n")); + let source = std::fs::read_to_string(dir.join("app/models/calendar_entry.rb")).unwrap(); + assert!(source.contains("ActiveSupport.format_db_date(@date_needle)"), "{source}"); + assert!(!source.contains("ActiveSupport.format_db_date(@text_needle)"), "{source}"); + if std::env::var("ROUNDHOUSE_PARAM_BINDS").unwrap() == "1" { + assert!(source.contains("Db.bind_text_opt("), "{source}"); + assert!(source.contains(" IS NULL"), "{source}"); + assert!(source.contains(" = ?"), "{source}"); + } + std::fs::write(dir.join("date_gate.rb"), r#" +require_relative "boot" +require_relative "app/models/calendar_entry" +SqliteAdapter.configure(":memory:") +ActiveRecord.adapter = SqliteAdapter +Schema.statements.each { |sql| Db.exec(sql) } +date = Date.new(2024, 2, 29) +row = CalendarEntry.new +row.due_on = date +row.save! +CalendarEntry.new.save! +raise "Date value" unless row.date_matches(date) == 1 +raise "Date NULL" unless row.date_matches(nil) == 1 +raise "String date" unless row.text_matches("2024-02-29") == 1 +raise "String NULL" unless row.text_matches(nil) == 1 +raise "different date" unless row.date_matches(Date.new(2024, 3, 1)) == 0 +puts "typed values: native Ruby Date/String and NULL predicates passed" +Db.close +"#).unwrap(); + success(Command::new("ruby").arg("date_gate.rb").current_dir(&*dir)); +} From 09a93169b2856a37f8fe2b97bcff442e490d651a Mon Sep 17 00:00:00 2001 From: Bunsy <268838774+bunnykong@users.noreply.github.com> Date: Wed, 7 Oct 2026 13:25:07 +0800 Subject: [PATCH 4/4] Preserve String-key adapter dispatch in seeded Spinel builds Match inherited read-adapter inputs to the app's key contract in Spinel output, retaining Integer-only sidecars and each model's schema-specific scalar signature. Keep the public nil guards. Compile and execute String-only finder and exists adapters with emitted RBS in both bind modes, with a CRuby control. Co-Authored-By: Claude Opus 5.5 --- src/project.rs | 28 ++++++++++++++ tests/param_binds.rs | 92 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+) diff --git a/src/project.rs b/src/project.rs index 163c84b27..7c89fa0d8 100644 --- a/src/project.rs +++ b/src/project.rs @@ -789,6 +789,34 @@ pub fn spinel_relation_model_handle(files: &mut [(String, String)]) -> Result<() *conn = conn.replace(narrow, wide); } *conn = widen_spinel_base_on_sigs(conn); + + // The public find/exists? nil guards live in the connection reopen. + // Spinel seeds their inherited adapter calls from Base's declaration, + // even when a String-keyed model overrides the adapter with a String + // argument. Follow the app-wide id contract established by + // widen_key_contract: Integer-only apps keep their original seeds, + // while apps with String keys need both kinds at shared dispatch. + // Each generated model still declares its schema's scalar. + let base = files + .iter_mut() + .find(|(p, _)| { + p == "sig/runtime/active_record/base.rbs" || p == "runtime/active_record/base.rbs" + }) + .ok_or_else(|| { + "spinel_relation_model_handle: active_record/base.rbs not in the tree".to_string() + })?; + if base.1.contains(" def id: () -> (Integer | String)\n") { + for method in ["self._adapter_find_by_id", "self._adapter_exists_by_id?"] { + let narrow = format!(" def {method}: (Integer id)"); + let wide = format!(" def {method}: (Integer | String id)"); + if !base.1.contains(&narrow) { + return Err(format!( + "spinel_relation_model_handle: base.rbs no longer declares {narrow:?}" + )); + } + base.1 = base.1.replace(&narrow, &wide); + } + } resolve_runtime_sig_conflicts(files) } diff --git a/tests/param_binds.rs b/tests/param_binds.rs index ed0cfbef2..313950f79 100644 --- a/tests/param_binds.rs +++ b/tests/param_binds.rs @@ -471,6 +471,98 @@ fn bind_runtime_spinel() { runtime(true); } +// A String-only override exposes the inherited Integer adapter seed that +// SQLite's escape helpers otherwise hide. Compile the actual emitted RBS; +// an unseeded native compile does not exercise this boundary. +fn string_key_adapter(test: &str, target: BuildTarget) { + if std::env::var_os("ROUNDHOUSE_BINDS_CHILD").is_none() { + for mode in ["0", "1"] { + println!("{test}: ROUNDHOUSE_PARAM_BINDS={mode}"); + success( + Command::new(std::env::current_exe().unwrap()) + .args(["--exact", test, "--include-ignored", "--nocapture"]) + .env("ROUNDHOUSE_BINDS_CHILD", "1") + .env("ROUNDHOUSE_PARAM_BINDS", mode), + ); + } + return; + } + let (dir, errors) = emit_and_run::empty_app() + .write( + "app/models/application_record.rb", + "class ApplicationRecord < ActiveRecord::Base\n self.abstract_class = true\nend\n", + ) + .write( + "app/models/widget.rb", + "class Widget < ApplicationRecord\n self.primary_key = \"identifier\"\nend\n", + ) + .write( + "app/controllers/application_controller.rb", + "class ApplicationController < ActionController::Base\nend\n", + ) + .write("config/routes.rb", "Rails.application.routes.draw do\nend\n") + .write( + "db/schema.rb", + "ActiveRecord::Schema[8.1].define(version: 1) do\n create_table \"widgets\", primary_key: \"identifier\", id: :string do |t|\n t.string \"name\"\n end\nend\n", + ) + .emit(target); + assert!(errors.is_empty(), "{}", errors.join("\n")); + let _cleanup = ScratchDir(dir.parent().unwrap().to_path_buf()); + let script = r#"require_relative "boot" +class Widget + def self._adapter_find_by_id(id) + record = Widget.new + record.name = id.upcase + record + end + def self._adapter_exists_by_id?(id) + id.upcase == "LITERAL_KEY" + end +end +raise "String adapter input" unless Widget.find("literal_key").name == "LITERAL_KEY" +raise "empty String key" unless Widget.find("").name == "" +raise "zero String key" unless Widget.find("0").name == "0" +raise "String exists adapter" unless Widget.exists?("literal_key") +raise "missing String exists adapter" if Widget.exists?("other") +raise "nil exists guard" if Widget.exists?(nil) +begin + Widget.find(nil) + raise "nil reached adapter" +rescue ActiveRecord::RecordNotFound +end +puts "PASS seeded String-only find/exists adapter boundary and nil guards" +"#; + if target == BuildTarget::Spinel { + std::fs::write(dir.join("strict_adapter.rb"), script).unwrap(); + let compiler = std::env::var("SPINEL").unwrap_or_else(|_| "spinel".into()); + let mut command = Command::new(compiler); + command + .args(["--rbs", ".", "strict_adapter.rb", "-o", "strict_adapter"]) + .current_dir(&dir); + let output = command.output().expect("compile seeded String adapter"); + assert!( + !String::from_utf8_lossy(&output.stderr).contains("type seeds are unavailable"), + "RBS extractor is required: {}", + String::from_utf8_lossy(&output.stderr) + ); + check_success(&command, &output); + success(Command::new(dir.join("strict_adapter")).current_dir(&dir)); + } else { + run_script(&dir, script, false); + } +} + +#[test] +fn string_key_adapter_ruby() { + string_key_adapter("string_key_adapter_ruby", BuildTarget::Ruby); +} + +#[test] +#[ignore = "requires Spinel and its RBS extractor"] +fn string_key_adapter_spinel() { + string_key_adapter("string_key_adapter_spinel", BuildTarget::Spinel); +} + fn nullable_associations(test: &str, target: BuildTarget) { if std::env::var_os("ROUNDHOUSE_BINDS_CHILD").is_none() { for mode in ["0", "1"] {