Fix post-merge review findings on has-one-of-many

docs/adr/050-has-one-of-many.md

@@ -1,7 +1,7 @@
-# 050 — Has-one-of-many derived from a HasMany (POC)
+# 050 — Has-one-of-many derived from a HasMany
 
 - Date: 2026-10-31
-- Status: Proposed (proof of concept, draft PR)
+- Status: Accepted (merged in #331; post-merge review fixes in a follow-up PR). The open questions below are pending a user decision.
 
 ## Problem
 
@@ -20,22 +20,46 @@ The ORM had no way to express this. `HasOne` eager-loads every child with `where
 1. **Pick in Python** — load every child and keep the max per parent. Rejected: loads N rows to keep one and breaks `where_has`.
 2. **Window function** — `id IN (SELECT id FROM (SELECT id, ROW_NUMBER() OVER (PARTITION BY fk ORDER BY col DESC, id DESC) rn FROM t) WHERE rn = 1)`. Works on SQLite ≥ 3.25, PostgreSQL and MySQL 8, but the builder has no derived-table source, so it needs hand-written SQL in `where_raw`, and without pushing the parent filter inside the window it ranks the whole table.
 3. **Laravel's join to an aggregate subquery** (`JOIN (SELECT MAX(col), fk … GROUP BY fk)`, with a second aggregate on the pk for ties). Needs `join_sub` (PR #330) and two nested aggregates for the tie-break.
-4. **Correlated subquery on the primary key** — `t.id = (SELECT o.id FROM t AS o WHERE o.fk = t.fk ORDER BY o.col DESC, o.id DESC LIMIT 1)`. Chosen.
+4. **Correlated subquery on the primary key** — `t.id = (SELECT o.id FROM t AS o WHERE o.fk = t.fk AND o.col IS NOT NULL ORDER BY o.col DESC, o.id DESC LIMIT 1)`. Chosen.
 
 ## Decision
 
-- `HasMany.latest_of_many(column="created_at")`, `oldest_of_many(column="created_at")` and `of_many(column="id", aggregate="max"|"min")` return a new `HasOneOfMany`, a `HasOne` subclass that reuses the parent's related model and keys. Any other aggregate raises `ValueError`.
-- `HasOneOfMany` overrides only `get_builder` and `get_exists_builder`. Both add `table.pk = (correlated subquery)`. The subquery orders by the column, then by the primary key in the same direction, so ties are deterministic (`max` → latest pk wins, `min` → earliest pk wins). The inherited `HasOne` eager (`where_in`), lazy (`load`), `where_has` and `register_related` code paths then work unchanged and always see at most one row per parent.
-- `relationship` (in `relationships/relationship.py`) is a decorator that returns a `RelationshipMethod` descriptor. On first access from an owner class it calls the decorated function with the **class** as its argument, so `self.statuses` is the class-level `HasMany` descriptor, not a loaded collection. The resulting relation is named after the method and cached per owner class. Instance access goes through the relation's normal `access` (loaded value or `LazyLoadingViolation`). `@relationship @classmethod` is also accepted.
+- `HasMany.latest_of_many(column="created_at")`, `oldest_of_many(column="created_at")` and `of_many(column=None, aggregate="max"|"min")` return a new `HasOneOfMany`, a `HasOne` subclass that reuses the parent's related model and keys. `aggregate` is typed `Literal["max", "min"]`, and any other value raises `ValueError` at runtime. `column=None` means the related model's `__primary_key__`, so models with a custom primary key work.
+- `HasOneOfMany` overrides only `get_builder` and `get_exists_builder`. Both add `table.pk = (correlated subquery)`. The subquery orders by the column and then by the primary key in the same direction, so ties are deterministic (`max` → latest pk wins, `min` → earliest pk wins). When the column is the primary key, it is ordered once. The inherited `HasOne` eager (`where_in`), lazy (`load`), `where_has` and `register_related` code paths then work unchanged and always see at most one row per parent.
+- NULLs: the subquery adds `column IS NOT NULL`, which gives `MAX`/`MIN` semantics on every database (NULLs are ignored), instead of each database's own NULL sort position. Without it, PostgreSQL's `DESC` and SQLite/MySQL's `ASC` would put NULL rows first. A parent whose children all have a NULL column gets `None`.
+- `HasOneOfMany` runs `HasOne.__init__`, so `_model_name` and `fn` are set the usual way, and `related_model()` resolves through the parent relation. It raises `TypeError` when the parent `HasMany` has neither an explicit `foreign_key` nor an attribute name to derive one from, instead of silently using `None_id`.
+- `relationship` (in `relationships/RelationshipMethod.py`, still exported as `fastapi_startkit.masoniteorm.relationship` and `fastapi_startkit.masoniteorm.relationships.relationship`) is a decorator that returns a `RelationshipMethod` descriptor. The implementation moved out of `relationship.py` because the exported function shadowed that module and its name was close to `models/relationship.py`. `relationships/relationship.py` stays as a re-export so the v0.71.0 import path `fastapi_startkit.masoniteorm.relationships.relationship` keeps working, and the package imports through it so the package attribute stays bound to the function. On first access from an owner class, the decorator calls the decorated function with the **class** as its argument, so `self.statuses` is the class-level `HasMany` descriptor, not a loaded collection. The result must be a new, unnamed `BaseRelationship`. Returning an existing descriptor (`return self.statuses`) or a builder raises `TypeError`, because naming it would rename the shared descriptor. The relation is named after the method and cached per owner class, so subclasses resolve their own. Instance access goes through the relation's normal `access` (loaded value or `LazyLoadingViolation`). Both the plain-function form and `@relationship @classmethod` are supported.
 - Typing: `relationship(fn: Callable[[Any], R]) -> R`, so the method's return annotation decides the type. `User.due_date` is `HasOne[Status]` and `user.due_date` is `Status | None`.
 
-## Known limitations / open questions
+## Caveats
 
-- basedpyright reports `self.statuses.latest_of_many()` as an error, because instance access types `statuses` as `Collection[Status]`. The `@relationship @classmethod def due_date(cls)` form type-checks cleanly. Choosing one canonical form is still open.
+- MySQL rejects writes through the relation, e.g. `User.due_date.where(...).update()` / `.delete()`, with error 1093 (the target table appears in a subquery). Reads are fine. Write through the `HasMany` or the related model instead.
+- A relationship method named `latest` or `oldest` overrides `Model.latest()`/`Model.oldest()` (basedpyright `reportIncompatibleMethodOverride`). Pick another name.
+- `where_column` emits unquoted identifiers (pre-existing behaviour), and now every of-many query uses it, so mixed-case table names on PostgreSQL would break.
+- Global scopes on the related model also run on the correlated subquery's builder. Scopes that hard-code the table name instead of using the builder's alias are not handled.
+- `@classmethod @relationship` (reversed order) is not supported.
+- Performance relies on an index on `(fk, column, pk)`.
+
+## Open questions (pending user decision)
+
+- Canonical form: basedpyright reports `self.statuses.latest_of_many()` as an error, because instance access types `statuses` as `Collection[Status]`. The `@relationship @classmethod def due_date(cls)` form type-checks cleanly. Both forms are kept until the user chooses.
+- Default column: `latest_of_many`/`oldest_of_many` keep `created_at`, matching `Model.latest()`/`oldest()`. Laravel defaults to the key.
 - Eager callbacks (`with_({"due_date": cb})`) and `where_has` callbacks filter the selected row ("the latest status, if it is done"), not the candidate set. Laravel's `ofMany(..., closure)` inner constraints are not implemented.
-- Global scopes on the related model run on the correlated subquery's builder too. Scopes that hard-code the table name instead of using the builder's alias are not covered by this POC.
-- Performance relies on an index on `(fk, column, pk)`. Only SQLite is tested.
 
 ## Validation
 
-`tests/masoniteorm/sqlite/relationships/test_has_one_of_many.py` covers: latest, oldest and `of_many` max/min with pk tie-breaks, parents without children (`None`), eager loading in one query (counted through `Connection.select`), lazy `load` on a model and on a collection, `where_has` with and without a callback, the classmethod form, unchanged `HasMany`, and rejection of unknown aggregates. The full suite (excluding Postgres) and basedpyright on `masoniteorm/relationships` pass.
+`tests/masoniteorm/sqlite/relationships/test_has_one_of_many.py` covers:
+- latest, oldest and `of_many` max/min with pk tie-breaks
+- parents without children (`None`)
+- eager loading in one query (counted through `Connection.select`)
+- lazy `load` on a model and on a collection
+- `where_has` with and without a callback
+- the classmethod form and an unchanged `HasMany`
+- rejection of unknown aggregates, existing descriptors, builders and unresolvable foreign keys
+- `related_model()` and the v0.71.0 import path
+- NULL columns ignored for latest and oldest
+- the `of_many()` default on a custom primary key, ordered once
+- a subclass of the owner
+- a self-referencing table, eager and `where_has`
+
+SQL snapshots are in `tests/masoniteorm/mysql/builder/test_mysql_has_one_of_many_sql.py` and `tests/masoniteorm/postgres/grammar/test_postgres_has_one_of_many_sql.py`. Neither needs a live server. The full suite (excluding Postgres), basedpyright and ruff all pass.

docs/adr/AGENTS.md

@@ -42,6 +42,6 @@ Read this index first, then open the records relevant to the change. Before impl
 | [042](042-fluent-validation-rules.md) | 2026-10-29 | Fluent validation rules with an async phase and a Laravel 422 shape | New `fastapi_startkit.validation`: `Field()`/`Email()` builders used as `Annotated` metadata on a `FormRequest` model (`required`, `nullable`, `string`, `integer`, `numeric`, `boolean`, `min`, `max`, `between`, `email`, `url`, `regex`, `in_`, `not_in`, `same_as`, `confirmed`, `required_if`, `password`, `unique`, `exists`, `add`, `msg`). A wildcard `field_validator` only records coerced values and the wrap `model_validator` fills missing `required` keys with `None`; it then runs sync rules per field in order with all values (so cross-field rules see later fields), skipping empty values for non-implicit rules and stopping at the first failure. `AsyncRule`s (`unique`/`exists` via the async `QueryBuilder`) run in an awaited second phase, `validate_async(context)`, triggered by the `Validated[Model]` FastAPI dependency with the route's typed path params as context (`ignore=lambda data: data["user_id"]`); an invalid path param skips the async phase so FastAPI returns its path 422 before any query; path params are collected by walking the public `Dependant` tree (no `get_flat_dependant`, removed in FastAPI 0.140) and cached per route. The dependency body key is the snake-cased model name (visible only when the endpoint has other body params). Rules on nested models raise `TypeError` at class definition. The existing unused `ValidationException` becomes a 422 `HttpException` carrying a `MessageBag`; `FastAPIProvider`'s existing `ValidationExceptionHandler` renders both phases as `{"message", "errors"}` (only the `message` key is new). |
 | [047](047-ai-bedrock-provider.md) | 2026-10-29 | AWS Bedrock provider for the AI module | Adds `provider="bedrock"` via LangChain's `bedrock_converse` (`ChatBedrockConverse`), the same `init_chat_model` path as other providers. `BedrockConfig` reads `AWS_REGION`/`AWS_DEFAULT_REGION`, IAM keys, session token, `AWS_PROFILE`, `AWS_BEARER_TOKEN_BEDROCK` and `AWS_BEDROCK_MODEL`. `Lab.get_client_options()` maps them to ChatBedrockConverse arguments and drops empty values so boto3's default credential chain applies; other providers return `{}`. The `bedrock` extra is `fastapi-startkit[ai]` plus `langchain-aws`, because `langchain-aws` does not pull in `langchain`. `langchain-aws` is imported only by `init_chat_model` when a Bedrock model is built. |
 | [048](048-async-first-validation.md) | 2026-10-30 | Async-first validation: one awaited rule pass, async hooks and closures | Supersedes ADR 042's two-phase design (sync rules in pydantic, `AsyncRule`s in `validate_async`). `await Model.validate_input(data, context=None, request=None)` is the only validation entry point: a per-call `Validator` runs `authorize` (false → 403 `AuthorizationException`), `prepare_for_validation`, a pydantic type-only parse (type errors added to the bag), then every field's rules in declaration and written order, awaiting a rule's result only when it is awaitable (sync rules unchanged, `unique`/`exists` in place via the async `QueryBuilder`), then `after`, and raises one `ValidationException`. Hooks may be sync or async classmethods; `Field().using(callback)` adds sync/async closure rules; `unique(ignore=...)` accepts async callables; `stop_on_first_failure` stops after the first error. `model_validate`/plain body params only parse types. `Validated[Model]` takes the raw body (`RawInput` keeps the OpenAPI `$ref` via `json_schema_input_schema`) and awaits `validate_input` with the typed path params, so type and rule errors share one 422. Several `Validated` params on one route are all validated: each records its errors in the request scope (prefixed with its embed key, e.g. `shipping.name`, when the body is embedded) and the route's last `Validated` dependency raises them together; `ValidationExceptionHandler` merges recorded errors into a `RequestValidationError` too. With `FastAPIProvider`, every 422 (also plain `RequestValidationError`) is `{"message", "errors"}`, a superset of `main`'s `{"errors"}`. `validate_async` is removed. |
-| [050](050-has-one-of-many.md) | 2026-10-31 | Has-one-of-many derived from a HasMany (POC) | `HasMany.latest_of_many(col="created_at")`, `oldest_of_many(col)` and `of_many(col, "max"\|"min")` return a `HasOneOfMany` (`HasOne` subclass reusing the parent keys) whose `get_builder`/`get_exists_builder` add `pk = (SELECT pk … WHERE fk = outer.fk ORDER BY col, pk LIMIT 1)`, so eager `with_`, lazy `load` and `where_has` pick one row per parent in SQL with a pk tie-break, portable without window functions or `join_sub`. A new `@relationship` decorator calls the method with the owner class (so `self.statuses` is the descriptor), names and caches the relation per class, and types it by the return annotation; `@relationship @classmethod` is the basedpyright-clean form. |
+| [050](050-has-one-of-many.md) | 2026-10-31 | Has-one-of-many derived from a HasMany | `HasMany.latest_of_many(col="created_at")`, `oldest_of_many(col)` and `of_many(col=None → related pk, Literal["max","min"])` return a `HasOneOfMany` (`HasOne` subclass reusing the parent keys; `TypeError` if the fk cannot be resolved). Its `get_builder`/`get_exists_builder` add `pk = (SELECT pk … WHERE fk = outer.fk AND col IS NOT NULL ORDER BY col, pk LIMIT 1)`, so eager `with_`, lazy `load` and `where_has` pick one row per parent in SQL with a pk tie-break, NULLs ignored like `MAX`/`MIN`, and no window functions or `join_sub`. `@relationship` (module `RelationshipMethod.py`, with `relationship.py` kept as a v0.71.0 re-export) calls the method with the owner class (so `self.statuses` is the descriptor), requires a new unnamed relation (`TypeError` otherwise), names and caches it per class and types it by the return annotation; `@relationship @classmethod` is the basedpyright-clean form. MySQL error 1093 blocks writes through the relation. Canonical form, default column and inner constraints are open. |
 | [051](051-nullable-column-casts.md) | 2026-10-31 | Nullable annotations infer the cast of their inner type | `Caster.normalize_type` only knew bare types, so `int \| None`, `Optional[int]` and `Union[int, None]` fell through to the `"str"` cast and nullable columns (e.g. a `HasMany` foreign key) were read back as strings, breaking eager-load key matching. `Caster.build` (replacing `build_casts`) removes `None` from a top-level union, also around or inside `Annotated`, maps the inner type to its usual cast and records the field as nullable. For nullable fields only, `Caster.get` turns `None` into `None` (or the `Field` default) and `Caster.set` writes `None`; bare annotations keep their behaviour. Unions of several non-`None` types get a pass-through `BaseCast`. Relationship key matching stays type-strict. |
 | [052](052-sqlite-savepoint-isolation.md) | 2026-10-31 | SQLite savepoints start a real transaction | pysqlite/aiosqlite defer `BEGIN` until the first DML, so a savepoint opened first in an outer transaction became a top-level transaction and `RELEASE` committed it, leaking rows past `DatabaseTransaction`/`RefreshDatabase` rollbacks. `SQliteConnection` listens to the engine `savepoint` event and runs `BEGIN` when the driver connection is not in a transaction. On the shared `StaticPool` connection, a lost `BEGIN` race (`OperationalError` while a transaction is already open) is ignored, so concurrent nested transactions still succeed as on `main`. SQLAlchemy's `isolation_level=None` + `BEGIN` on every `begin` recipe was rejected because concurrent transactions on `SQLiteConfig`'s shared `StaticPool` connection then fail with `cannot start a transaction within a transaction`. |

fastapi_startkit/src/fastapi_startkit/masoniteorm/relationships/HasMany.py

@@ -1,5 +1,5 @@
 from .BaseRelationship import BaseRelationship, ManyResult, TModel
-from .HasOneOfMany import HasOneOfMany
+from .HasOneOfMany import HasOneOfMany, OfManyAggregate
 from ..collection import Collection
 
 
@@ -32,7 +32,7 @@ def latest_of_many(self, column: str = "created_at") -> HasOneOfMany[TModel]:
     def oldest_of_many(self, column: str = "created_at") -> HasOneOfMany[TModel]:
         return self.of_many(column, "min")
 
-    def of_many(self, column: str = "id", aggregate: str = "max") -> HasOneOfMany[TModel]:
+    def of_many(self, column: str | None = None, aggregate: OfManyAggregate = "max") -> HasOneOfMany[TModel]:
         return HasOneOfMany(self, column, aggregate)
 
     def register_related(self, key, model, collection):

fastapi_startkit/src/fastapi_startkit/masoniteorm/relationships/HasOneOfMany.py

@@ -1,24 +1,26 @@
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, Literal
 
 from .BaseRelationship import TModel
 from .HasOne import HasOne
 
 if TYPE_CHECKING:
     from .HasMany import HasMany
 
-AGGREGATE_DIRECTIONS = {"max": "desc", "min": "asc"}
+type OfManyAggregate = Literal["max", "min"]
+
+AGGREGATE_DIRECTIONS: dict[str, str] = {"max": "desc", "min": "asc"}
 
 
 class HasOneOfMany(HasOne[TModel]):
-    def __init__(self, relation: "HasMany[TModel]", column: str, aggregate: str):
+    def __init__(self, relation: "HasMany[TModel]", column: str | None, aggregate: OfManyAggregate):
         if aggregate not in AGGREGATE_DIRECTIONS:
             raise ValueError(f"of_many() aggregate must be one of {', '.join(AGGREGATE_DIRECTIONS)}, got {aggregate!r}")
+        if relation.foreign_key is None and relation.attribute is None:
+            raise TypeError("of_many() needs a HasMany declared on a model or given an explicit foreign_key")
 
         relation.set_keys(None, relation.attribute)
+        super().__init__(foreign_key=relation.foreign_key, local_key=relation.local_key)
         self.relation = relation
-        self.fn = self.related_model
-        self.local_key = relation.local_key
-        self.foreign_key = relation.foreign_key
         self.column = column
         self.direction = AGGREGATE_DIRECTIONS[aggregate]
         self.attribute = None
@@ -44,11 +46,12 @@ def one_of_many(self, table):
         builder = self.relation.get_builder()
         alias = f"{builder.get_table_name()}_of_many"
         key = builder._model.__primary_key__
-        return (
+        column = self.column or key
+        query = (
             builder.table(builder.get_table_name(), alias)
             .select(f"{alias}.{key}")
             .where_column(f"{alias}.{self.foreign_key}", f"{table}.{self.foreign_key}")
-            .order_by(f"{alias}.{self.column}", self.direction)
-            .order_by(f"{alias}.{key}", self.direction)
-            .limit(1)
         )
+        if column != key:
+            query.where_not_null(f"{alias}.{column}").order_by(f"{alias}.{column}", self.direction)
+        return query.order_by(f"{alias}.{key}", self.direction).limit(1)