Skip to content

feat(orm): select_sub subquery-as-column + Laravel-style add_select + table()#170

Open
tmgbedu wants to merge 2 commits into
mainfrom
task/orm-select-sub-895b
Open

feat(orm): select_sub subquery-as-column + Laravel-style add_select + table()#170
tmgbedu wants to merge 2 commits into
mainfrom
task/orm-select-sub-895b

Conversation

@tmgbedu

@tmgbedu tmgbedu commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up B from splitting #168, updated per review (#909) to match Laravel's addSelect semantics.

  • select_sub(subquery, alias) — the subquery-as-column primitive. Renders (subquery) AS alias with bindings in SELECT position (before WHERE). Accepts a QueryBuilder or a callable that builds one. Mirrors Laravel selectSub($query, $as).
  • add_select(*columns) — Laravel's variadic column-adder (was previously a swapped-arg alias of select_sub; that shape is gone). Appends plain string columns (variadic or a single list) with a dedup guard. An associative {alias: subquery} entry (string key + queryable value) delegates to select_sub, seeding select(f"{table}.*") first when nothing is selected yet. A string value under a string key stays a plain column (Laravel parity).
  • table(name) — sets the target table on a bare builder; backs the callable form of select_sub.
  • Internal migration: the *Through / BelongsToMany with-count callers moved from the old add_select(alias, callable) shape to select_sub(callable, alias).
  • Reuse nit: process_columns now routes the subquery alias through subquery_alias_string() (single source of alias emission; SQL byte-identical — the helper returns AS {alias} in every dialect today).

Acceptance case (from review) works:

User.query().add_select({
    "last_login_at": Login.query().select("created_at").where_column("user_id", "users.id").latest().limit(1)
})
# SELECT "users".*, (SELECT "logins"."created_at" FROM "logins" WHERE user_id = users.id ORDER BY "created_at" DESC LIMIT 1) AS last_login_at FROM "users"

Tests

  • tests/masoniteorm/query/grammars/test_select_sub_grammar.pyselect_sub (builder + callable, non-builder TypeError, qmark binding order) and add_select (variadic, single list, dedup guard, assoc→select_sub with {table}.* seeding, assoc callable, string-value-under-string-key stays a column, mixed list, bare-subquery-without-alias TypeError, assoc qmark binding order) — all across 4 dialects.
  • tests/masoniteorm/sqlite/builder/test_sqlite_select_sub.py — real sqlite: select_sub builder/callable, add_select variadic, the Laravel {alias: subquery} acceptance case (real data), {table}.* seeding, table(), and a join-shaped select_sub (the migrated with-count call shape) compiling + executing.

Gates

  • uv run pytest --ignore=tests/masoniteorm/postgres --cov → 1820 passed / 7 skipped, coverage 78.96% (≥ fail_under 68).
  • ruff check + ruff format --check clean.
  • Zero regression to existing string-based select(). No remaining callers of the old add_select(alias, subquery) shape (verified by grep).

Not in scope

End-to-end relationship with-count is gated on the async count() coroutine issue — tracked as #886 (fix in progress on a branch stacked atop this one). This PR asserts the with-count construction path via select_sub.

@codecov

codecov Bot commented Jul 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.17647% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...tartkit/masoniteorm/relationships/BelongsToMany.py 0.00% 1 Missing ⚠️
...artkit/masoniteorm/relationships/HasManyThrough.py 0.00% 1 Missing ⚠️
...tartkit/masoniteorm/relationships/HasOneThrough.py 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@tmgbedu

tmgbedu commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

✅ Code Review Verdict: APPROVE (posted as comment — GH blocks self-approval)

Reviewed for correctness, binding order, alias emission, and regression. All gates reproduced locally on the PR branch.

Verified

  • SELECT-position binding order is correct. BaseGrammar._compile_select evaluates process_columns(...) before process_wheres(...) (kwargs eval left-to-right), so subquery bindings precede WHERE bindings. get_bindings() == [1, "active"] is genuinely exercised and correct. No accumulation across re-compiles: each to_qmark() builds a fresh grammar and the subquery's to_qmark() reassigns (not +=) its _bindings.
  • Alias emission (subquery) AS alias correct across all 4 dialects incl. MSSQL TOP n.
  • add_select callable form constructs QueryBuilder(connection, grammar, processor) matching the real __init__ signature; non-builder input raises TypeError as tested.
  • table() setter is genuinely new (no collision with any mixin/method on QueryBuilder) and correctly backs the previously-dangling chained .table() calls in HasManyThrough/HasOneThrough/BelongsToMany with-count paths. Full relationship suite passes.
  • Reuses existing SubGroupExpression rather than inventing a new mechanism — good altitude.
  • Zero regression / test quality: real sqlite-backed correlated reads + grammar-level assertions, no hollow mocks.

Gates reproduced (PR worktree)

  • uv run pytest --ignore=tests/masoniteorm/postgres1808 passed / 7 skipped (matches report)
  • New tests: 9 passed / 20 subtests
  • ruff check + ruff format --check → clean

Non-blocking notes

  • Alias in (subquery) AS {alias} is emitted unquoted — consistent with existing correlated where_column fragments; fine for normal identifiers. Not a blocker.
  • With-count paths now activated rely on the general suite for coverage; dedicated tests tracked in backlog #886.

No correctness bugs found. Recommend merge (per split order #168#169 → this).

@tmgbedu

tmgbedu commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

🔄 Revising verdict → Request changes on add_select API shape

Correctness/binding review stands, but flagging an API-convention mismatch with Laravel (which this ORM mirrors). This PR defines add_select(alias, subquery) as a 2-arg subquery-only method — that shadows Laravel's addSelect, which is a simple column-adder, not the subquery method.

Laravel's actual signatures:

public function addSelect($column)   // variadic / array of COLUMNS
{
    $columns = is_array($column) ? $column : func_get_args();
    foreach ($columns as $as => $column) {
        if (is_string($as) && $this->isQueryable($column)) {
            if (is_null($this->columns)) {
                $this->select($this->from.'.*');
            }
            $this->selectSub($column, $as);   // assoc [as => subquery] delegates here
        } else {
            if (is_array($this->columns) && in_array($column, $this->columns, true)) {
                continue;   // dedup guard
            }
            $this->columns[] = $column;       // plain column appended
        }
    }
    return $this;
}

public function selectSub($query, $as) { ... }   // the subquery-as-column method

Requested change:

  1. select_sub(subquery, alias) — keep as-is. It already matches Laravel selectSub($query, $as) (arg order correct). ✅
  2. add_select(*columns) — redefine to the Laravel semantics: a variadic column-adder that appends plain string columns (dedup-guarded, initializing select(f"{table}.*") if columns is empty), and delegates associative {alias: subquery} entries to select_sub. It should not be a thin alias of select_sub with swapped args.

Rationale: downstream users coming from Laravel/Masonite will expect add_select("name", "email") to add columns, not raise/misbehave. Current form breaks that expectation and the codebase's own convention of mirroring the Laravel query-builder surface.

Everything else in the prior review (binding order, table() setter, dialect parity, gates) is unaffected and still passes.

… table()

- select_sub(subquery, alias): the subquery-as-column primitive. Renders
  (subquery) AS alias with bindings in SELECT position; accepts a builder
  or a callable that builds one. Matches Laravel selectSub(query, as).
- add_select(*columns): Laravel's variadic column-adder. Appends plain
  string columns with a dedup guard; an associative {alias: subquery} entry
  (string key + queryable value) delegates to select_sub, seeding
  select({table}.*) first when nothing is selected yet. A string value
  under a string key stays a plain column.
- table(name): sets the target table on a bare builder; backs the callable
  form of select_sub.
- Migrate the *Through/BelongsToMany with-count callers from the old
  add_select(alias, callable) shape to select_sub(callable, alias).
- process_columns: route the subquery alias through subquery_alias_string()
  instead of a hardcoded AS (single source of alias emission; SQL unchanged).

Existing string-based select() is unchanged. Covered by grammar-level
to_sql()/to_qmark() with binding-order assertions across all four dialects,
plus a real sqlite suite incl. the Laravel addSelect({alias: subquery})
acceptance case. End-to-end relationship with-count remains gated on the
async count() fix (#886).
@tmgbedu tmgbedu force-pushed the task/orm-select-sub-895b branch from dd369f5 to 1107a49 Compare July 12, 2026 17:07
@tmgbedu tmgbedu changed the title feat(orm): select_sub / add_select subquery-as-column + table() setter feat(orm): select_sub subquery-as-column + Laravel-style add_select + table() Jul 12, 2026
@tmgbedu

tmgbedu commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

✅ Re-review (post force-push 1107a49a): APPROVE

The requested add_select API refactor is fully and faithfully implemented. Verified against Laravel addSelect/selectSub semantics.

Verified

  • add_select(*columns) = Laravel addSelect: variadic strings / single list, dedup guard, associative {alias: queryable} delegates to select_sub, seeds select(f"{table}.*") only when _columns is empty (does not clobber an explicit .select()), and a plain string value under a string key stays a column (matches Laravel's else branch). Bare queryable without a string alias raises TypeError.
  • select_sub(subquery, alias) is the primitive; add_select delegates to it (not the reverse). Signature/arg-order matches selectSub($query, $as). Unchanged binding-order behavior (SELECT-position bindings precede WHERE) still holds — re-verified via the assoc qmark binding-order test.
  • Caller migration complete: grep of src/ shows no old 2-arg add_select(alias, subquery) shape left; the 3 with-count callers (BelongsToMany, HasOneThrough, HasManyThrough) now use select_sub(callable, alias) with a select("*") guard.
  • Alias helper reuse: process_columns now emits the alias via subquery_alias_string() — confirmed it returns "AS {alias}" in all 4 grammars, so SQL is byte-identical today while centralizing alias emission.

Tests (real, not hollow)

  • Grammar-level across all 4 dialects: variadic, single-list, dedup, seed-table.*, keep-existing-columns, assoc callable, string-under-string-key, mixed list, bare-subquery TypeError, assoc qmark binding order.
  • Real sqlite: the exact Laravel acceptance case {last_login_at: Login.select('created_at').where_column('user_id','accounts.id').latest().limit(1)} with seeded data — asserts correct correlated results (Alice→2024-03-01, Bob→2024-02-01); seeds-table.* base-columns-preserved case; join-shaped callable (the with-count shape) compiles (JOIN + correlated column + alias) and executes.

Gates reproduced

  • pytest --ignore=tests/masoniteorm/postgres1820 passed / 7 skipped / 60 subtests
  • ruff check + ruff format --check → clean

Scope note

End-to-end relationship with-count (async count(*)) is intentionally deferred to #886 and asserted here at the construction level — agreed, not a blocker.

No outstanding issues. Recommend merge (per split order #168#169 → this).

Replace raw conn.execute CREATE TABLE / INSERT with the framework's Schema
builder and Model.insert() for the categories/posts and accounts/logins
fixtures. Same data and same assertions (incl. the Laravel addSelect
last_login_at acceptance case).
@tmgbedu

tmgbedu commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

✅ Light re-review (fixture change da46c311, #911): APPROVE — verdict unchanged

Scoped to the test-only fixture conversion (raw conn.executeSchema.create() + Model.insert()).

  • (a) Schema tables match test needscategories(id,name), posts(id,title,category_id), accounts(id,name), logins(id,user_id,created_at); columns line up with the models and every query under test.
  • (b) Seeded rows identical to prior data — categories Zebra/Apple/Mango; posts (1,z,1)(2,a,2)(3,m,3)(4,a2,2); accounts Alice/Bob; logins (1,1,2024-01-01)(2,1,2024-03-01)(3,2,2024-02-01). Byte-for-byte the same, including login dates.
  • (c) No raw conn.execute/DB.connection left in the test file — grep clean.
  • (d) Acceptance assertions unchanged & passinglast_login_at{Alice: 2024-03-01, Bob: 2024-02-01}; category-name correlated results unchanged. All 7 sqlite tests pass.
  • (e) Gates greenpytest --ignore=postgres --cov1820 passed / 7 skipped / 60 subtests, coverage 78.96% (≥68), ruff check + format clean.

No production code changed; approval stands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant