Merge pull request #451 from PingoLee/fix/432-441-parameter-alignment

fix(#432, #441): a nested render's values follow its markers, and two projections may not share an output name

Author: PingoLee

.github/skills/pormg-querybuilder-internals/SKILL.md

@@ -41,11 +41,13 @@ For positional backends, preserve bucket semantics and flatten order. The bucket
 
 `:cte → :select → :update → :join → :where → :having`
 
+**A nested render does not pick a bucket (#432).** An `Exists(...)`, a projected `Subquery(...)` or an `__@in` subquery renders inside the PARENT's clause, so its values are marked, lifted and re-emitted as one clause-ordered run at the parent's marker position (`nested_parameter_mark` / `detach_nested_run!`), with `own_contexts=true` so the inner build files its values under its own clauses first. Binding order is not text order: a build binds joins last and renders them first, which is what the buckets exist to reconcile.
+
 Parameter collector model:
 
 - `AbstractPormGParam`: base abstraction for all collectors
 - `PormGPostgresParam`: linear collector for `$1`, `$2`, ... placeholders
-- `PormGPositionalParam`: bucketed collector for positional `?` placeholders
+- `PormGSQLiteParam`: bucketed collector for positional `?` placeholders (concrete type `SQLiteParameterizedQuery`)
 
 When changing parameter behavior, verify:
 
@@ -54,7 +56,7 @@ When changing parameter behavior, verify:
 - parent and subquery inheritance behavior
 - HAVING alias promotion placement
 - custom join parameter routing into the join bucket
-- flattening through `get_final_parameters(::PormGPositionalParam)` in SQL-clause order
+- flattening through `get_final_parameters(::PormGSQLiteParam)` in SQL-clause order
 
 Query-building context rules:
 
@@ -235,7 +237,8 @@ When introducing a new parameterized SQL clause or changing clause order, update
 
 - bucket struct fields in `parameters.jl`
 - `set_context!` call sites in builder modules
-- `get_final_parameters` flatten order
+- `_BUCKET_ORDER` in `parameters.jl` — the single list both `get_final_parameters` and
+  `detach_nested_run!` (#432) read; there is no second copy to keep in sync
 - unit coverage in the canonical alignment tests
 - integration coverage if the behavior is user-visible
 

UPGRADING.md

@@ -45,6 +45,112 @@ _Changes merged but not yet cut into a release. A consumer dev'ing PormG at HEAD
 and `PormG.upgrade_guide` surfaces them by default. When the maintainer next rolls changes into a
 consuming app, `/pormg-cut-release` stamps every entry below with `0.5.0`, dates them, and tags it._
 
+## `values()` refuses two projections that would render the same output name (#441)
+
+- **Version**: Unreleased
+- **PormG ref**: #441; `src/querybuilder/object_manager.jl`, `src/querybuilder/build_query.jl`,
+  `docs/src/read/values_and_joins.md`
+- **Severity**: **behavior change (narrow)** — two shapes that render today stop building, and one
+  of them (`Value(...)` literals sharing a name) was working correctly rather than silently wrong.
+  Part of the `0.5.x` pre-publish wave.
+
+### What changed
+
+`get_select_query` memoises each resolved projection, keyed on `_as`. A second projection under a
+name already taken was replaced by the first and never resolved at all — so the caller silently got
+one column twice instead of the two they asked for. On SQLite it was worse than a lost column: the
+cached field's ALREADY-RENDERED text carries its `?`, so the statement emitted one placeholder more
+than the driver had values for and every parameter after it bound one slot early:
+
+```
+values("note", "h" => Exists(a), "h" => Exists(b))  +  filter("note" => "TAIL")
+    SQLite      3 placeholders, parameters ["AAA", "TAIL"]
+                -> "TAIL" bound to the SECOND Exists; the outer WHERE never bound at all
+    PostgreSQL  $1 twice, then $2 — legal SQL, still the wrong query
+```
+
+`values()` now refuses two projections that would render the same output name. It is refused at the
+call rather than repaired at render because two columns under one name are indistinguishable to
+everything downstream — a DataFrame column, an `order_by` alias — so no reading of the query keeps
+both.
+
+The rule is on the **output** name, so `"*"` counts as the physical columns the database expands it
+to, not the declared field names. With `note` declared `db_column = "obs"`, `values("*", "obs" => x)`
+collides while `values("*", "note" => x)` does not.
+
+**`Value(...)` duplicates are refused too**, and this is the part that removes working behavior:
+`values("lbl" => Value("a"), "lbl" => Value("b"))` renders both parameters correctly today, because
+literal projections bypass the memo entirely. It is refused anyway so the rule has no per-kind
+exception — a rule that holds only for some projection kinds is the sort of subtlety this defect
+family has repeatedly escaped through.
+
+**Unchanged, and in fact newly fixed:** naming the same expression under two *different* names.
+`values("a" => "points", "b" => "points")` used to render `as "a"` twice and drop `b`; it now returns
+both columns, over a single join where a path is involved. If your code worked around that by
+avoiding the shape, the workaround is no longer needed.
+
+Also retired: the #423 ambiguity guard in `order_by()`. It refused `order_by("x")` when `values()`
+projected `x` twice; `values()` now refuses that declaration, so the guard was unreachable.
+
+### How to find the calls to migrate
+
+Grep for `values(` calls with a repeated alias, then check `"*"` calls against the model's physical
+columns:
+
+```bash
+rg -n '\.values\(' .
+```
+
+Or programmatically — **run this against your CURRENT pin, before upgrading.** After upgrading it
+can never fire, because `values()` throws before you can hold an offending query. Note the `"*"`
+expansion: without it the check cannot see a star collision, which is the half most likely to bite:
+
+```julia
+function duplicate_projection_names(q)
+    m, names = q.object.model, String[]
+    for v in q.object.values
+        n = v.custom_as !== nothing ? v.custom_as : v._as
+        n === nothing && continue
+        n == "*" ? append!(names, unique(PormG.Models.field_db_column(m.fields[f], f)
+                                         for f in m.field_names)) : push!(names, n)
+    end
+    [n for n in unique(names) if count(==(n), names) > 1]
+end
+
+dups = duplicate_projection_names(query)
+isempty(dups) || @warn "values() projects a name twice" dups
+```
+
+### Migrate your app
+
+Both pairs below are real F1 columns and were executed against the `db_sl` fixture — the ✗ lines
+raise, the ✓ lines run.
+
+```julia
+# ✗ BEFORE — `n` twice; the Sum was silently discarded
+query = M.Result.objects
+query.values("driverid", "n" => Count("resultid"), "n" => Sum("points"))
+
+# ✓ AFTER — distinct names
+query = M.Result.objects
+query.values("driverid", "n_results" => Count("resultid"), "total_points" => Sum("points"))
+```
+
+```julia
+# ✗ BEFORE — `statusid` is already one of the columns the star emits
+query = M.Result.objects
+query.values("*", "statusid" => "points")
+
+# ✓ AFTER — a name the star does not already emit
+query = M.Result.objects
+query.values("*", "status_points" => "points")
+```
+
+If a field carries a `db_column`, the star's contribution is the **physical** name. The F1 fixture's
+`Db_column_scratch.sku` declares `db_column = "product_sku"`, so on that model it is
+`values("*", "product_sku" => …)` that collides — `values("*", "sku" => …)` does not, because the
+star never emits `sku`.
+
 ## A subquery consumed by `@in` / `Subquery` / `Exists` may no longer declare its own CTE (#433)
 
 - **Version**: Unreleased

docs/src/architecture.md

@@ -181,9 +181,23 @@ characterizations worth stating plainly.
 - **Positional-parameter buckets (deliberate, contained)** — for SQLite the positional `?` markers
   are collected into per-clause buckets and flattened in SQL-clause order
   (`:cte → :select → :update → :join → :where → :having`). The flatten order is single-sourced in
-  `get_final_parameters` and guarded by `test_alignment_sqlite.jl` / `test_parameters.jl`; the only
-  cost is that *adding a new bucket* is a documented multi-site edit (see the QueryBuilder skill's
-  maintenance checklist). Noted here so it is not mistaken for accidental coupling.
+  `_BUCKET_ORDER` (which `get_final_parameters` and the nested-run machinery both read) and guarded by `test_alignment_sqlite.jl` / `test_parameters.jl`. Noted here
+  so it is not mistaken for accidental coupling.
+
+  Two costs, not one. *Adding a new bucket* is a documented multi-site edit (see the QueryBuilder
+  skill's maintenance checklist). The subtler one: a bucket is chosen by the builder phase that binds
+  a value, while correctness depends on where that value's `?` is **emitted** — and those diverge
+  whenever a fragment renders somewhere other than its own clause. Three bugs came out of that gap —
+  #421 a relocated fragment and #432 a nested render, both SQLite-only, plus #441 a discarded
+  projection, whose *parameter* desync was SQLite-only but which dropped the column itself on **both**
+  backends. All three were silent. A nested render now re-emits its values as one clause-ordered run at its own marker
+  position (`nested_parameter_mark` / `detach_nested_run!`). All four such sites in the **read**
+  builder go through it — `Exists`, a projected `Subquery`, an `__@in` subquery, and a CTE body.
+
+  `deletion.jl` splices subqueries into hand-built `DELETE`/`UPDATE` clauses without it, and is
+  correct today only by coincidence: it builds a fresh collector per statement, and a lone
+  subquery's own text order *is* `_BUCKET_ORDER`, so the flatten happens to agree. That is a
+  narrower claim than "contained", and deliberately so.
 
 !!! note "\"Async-first\" is backend-specific"
     For **PostgreSQL** the async path is genuine: the pool lock is released before the round-trip and

docs/src/read/values_and_joins.md

@@ -313,11 +313,19 @@ query.order_by("driverid__surname")     # ORDER BY "Tb_1"."surname" ASC NULLS LA
     sort consistent with the column the query actually returns. If you meant the underlying column,
     name it explicitly (`order_by("-points")`) or choose an alias that shadows nothing.
 
-!!! note "Two projections may not share an alias in `order_by`"
-    `.values("x" => "grid", "x" => "points")` followed by `order_by("x")` raises `QueryBuildError`:
-    the sort key matches two different projections. PostgreSQL rejects an ambiguous `ORDER BY` alias
-    and SQLite would resolve it arbitrarily, so PormG refuses it rather than diverge. Give the two
-    projections distinct names.
+!!! note "Two projections may not share an output name"
+    `.values("x" => "grid", "x" => "points")` raises `QueryBuildError` at the `values()` call — two
+    columns under one name are indistinguishable to whatever reads the result, and on SQLite the
+    second one's placeholders desynchronize every parameter after it. Give the two projections
+    distinct names.
+
+    The rule is on the **output** name, so `"*"` counts as the physical columns the database expands
+    it to: with `note` declared `db_column = "obs"`, `.values("*", "obs" => …)` collides while
+    `.values("*", "note" => …)` does not.
+
+    Naming the same expression under two *different* names is fine and renders two columns —
+    `.values("a" => "points", "b" => "points")` gives you both, over a single join where a path is
+    involved.
 
 ---
 

src/querybuilder/build_helpers.jl

@@ -828,11 +828,16 @@ function _get_select_query(v::SubqueryObject, instruc::SQLInstruction; _as::Unio
   # Passing the shared `parameters` makes query() treat this as a subquery: it inherits the ambient
   # :select bucket (set by build()) and restores the context afterward, so the inner params flatten in
   # :select — textually correct (the SELECT list precedes FROM/JOIN/WHERE). Correlate via outer=instruc.
+  # #432: same nested-run reordering as `_build_exists_query` — this subquery's text sits in the
+  # SELECT list, so everything it binds must be one clause-ordered run in the ambient bucket.
+  nested_mark = nested_parameter_mark(instruc)
   inner_sql = query(handler,
                     table_alias=instruc.table_alias,
                     connection=instruc.connection,
                     parameters=instruc.parameters,
-                    outer=instruc)
+                    outer=instruc,
+                    own_contexts=true)
+  reattach_parameters!(instruc, detach_nested_run!(instruc, nested_mark))
   return string("(", inner_sql, ")")
 end
 function _get_select_query(q::SQLTypeF, instruc::SQLInstruction; _as::Union{Nothing,String}=nothing)
@@ -861,18 +866,25 @@ function _build_exists_query(subquery::SQLObjectHandler, instruc::SQLInstruction
   q.object.offset = 0
 
   old_context = instruc.parameters isa PormGSQLiteParam ? instruc.parameters.current_context : nothing
+  # #432: the inner build scatters its values across its own clause buckets while this EXISTS text is
+  # spliced into ONE of the parent's clauses. Mark every bucket, then re-emit what it bound as one
+  # contiguous run, clause-ordered, at this fragment's position. See `detach_nested_run!`.
+  nested_mark = nested_parameter_mark(instruc)
   instruction = try
     build(
       q.object,
       table_alias=instruc.table_alias,
       connection=instruc.connection,
       parameters=instruc.parameters,
-      set_contexts=false,
+      # #432: record this subquery's OWN clause roles so `detach_nested_run!` can sort its values
+      # into text order. The `finally` below restores the parent's ambient bucket.
+      set_contexts=true,
       outer=instruc,
     )
   finally
     old_context !== nothing && set_context!(instruc.parameters, old_context)
   end
+  reattach_parameters!(instruc, detach_nested_run!(instruc, nested_mark))
 
   safe_table_name = safe_table_identifier(Models.model_table_name(q.object.model), instruction.connection)
   safe_alias = quote_identifier(instruction.alias, instruction.connection)
@@ -1322,7 +1334,10 @@ function _get_filter_query(v::SQLTypeOper, instruc::SQLInstruction)
     _validate_membership_subquery(v)
     # #433: renders an inline WITH that binds into `:cte` while its text sits in the WHERE clause.
     _guard_no_nested_cte(v.values, "A membership filter (__@in / __@nin)")
-    placeholders = query(v.values, table_alias=instruc.table_alias, connection=instruc.connection, parameters=instruc.parameters, outer=instruc)
+    # #432: same nested-run reordering — the subquery renders inside this predicate's clause.
+    nested_mark = nested_parameter_mark(instruc)
+    placeholders = query(v.values, table_alias=instruc.table_alias, connection=instruc.connection, parameters=instruc.parameters, outer=instruc, own_contexts=true)
+    reattach_parameters!(instruc, detach_nested_run!(instruc, nested_mark))
     return string(_get_filter_query(v.column, instruc), " ", v.operator, " ($placeholders)")
   else
     @pormg_debug false