From eecb3a311e7afd1d04d75114c7d8decf681b1b70 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Thu, 23 Jul 2026 10:42:16 -0500 Subject: [PATCH 01/41] Add returning() to queryset update() and delete() Chaining returning() before a queryset update() or delete() emits a Postgres RETURNING clause and hands back the affected rows instead of an int rowcount. No-arg returning() hydrates full model instances (RETURNING every concrete column); returning(*names) returns a list of dicts of just those columns. Without returning(), update()/delete() still return an int. returning() returns a ReturningQuerySet flavor whose update()/delete() are typed as returning a list, so static checkers see honest return types while the plain QuerySet keeps int. --- plain-postgres/plain/postgres/README.md | 23 +++ plain-postgres/plain/postgres/query.py | 134 ++++++++++++++++-- plain-postgres/plain/postgres/sql/compiler.py | 58 ++++++-- plain-postgres/plain/postgres/sql/query.py | 6 + 4 files changed, 201 insertions(+), 20 deletions(-) diff --git a/plain-postgres/plain/postgres/README.md b/plain-postgres/plain/postgres/README.md index f06bb0cb1d..3baee8cc69 100644 --- a/plain-postgres/plain/postgres/README.md +++ b/plain-postgres/plain/postgres/README.md @@ -7,6 +7,7 @@ - [Middleware](#middleware) - [Bypassing a connection pooler for management operations](#bypassing-a-connection-pooler-for-management-operations) - [Querying](#querying) +- [Returning affected rows](#returning-affected-rows) - [Schema management](#schema-management) - [Syncing](#syncing) - [Structural migrations](#structural-migrations) @@ -427,6 +428,28 @@ for row in HugeTable.query.iterator(chunk_size=2000): process(row) ``` +## Returning affected rows + +`QuerySet.update()` and `QuerySet.delete()` return an `int` rowcount. Chain `returning()` before the write to get the affected rows back instead — Postgres' `RETURNING` clause fetches them in the same statement, so there's no second query. + +```python +# No arguments: rows come back as model instances. +running = Job.query.filter(status="pending").returning().update(status="running") +for job in running: + print(job.id, job.status) # reflects the post-update values + +# Field names: rows come back as dicts of just those columns. +deleted = Event.query.filter(created_at__lt=cutoff).returning("id", "payload").delete() +for row in deleted: + print(row["id"], row["payload"]) # the rows as they were deleted +``` + +- **`returning()`** returns full model instances. For `update()` they hold the new values; for `delete()`, the rows as they were. +- **`returning("field", ...)`** returns a list of dicts with only those columns. Passing an unknown or non-concrete field name raises `FieldError` at the `returning()` call. +- Without `returning()`, `update()`/`delete()` return an `int` as before. + +`RETURNING` only reports rows of the statement's own target table. Rows removed by a cascading `ON DELETE` are never included — a `delete()` with `returning()` gives you the parent rows you deleted, not the children Postgres cascaded. + ## Transactions By default, each query runs in its own implicit transaction and is committed immediately (autocommit mode). When you need multiple queries to succeed or fail together — like creating a user and their profile — wrap them in an explicit transaction. diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index 1bc942bdd4..8f9cc72828 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -10,7 +10,7 @@ from collections.abc import Callable, Iterator, Sequence from functools import cached_property from itertools import islice -from typing import TYPE_CHECKING, Any, Never, Self, overload +from typing import TYPE_CHECKING, Any, Never, Self, cast, overload import psycopg @@ -48,7 +48,7 @@ from plain.utils.functional import partition # Re-exports for public API -__all__ = ["F", "Q", "QuerySet", "RawQuerySet", "Prefetch"] +__all__ = ["F", "Q", "QuerySet", "ReturningQuerySet", "RawQuerySet", "Prefetch"] if TYPE_CHECKING: from plain.postgres import Model @@ -286,6 +286,10 @@ class Task(Model): _fields: tuple[str, ...] | None _defer_next_filter: bool _deferred_filter: tuple[bool, tuple[Any, ...], dict[str, Any]] | None + # None => plain update()/delete() returning an int rowcount. + # () => RETURNING every concrete column, hydrated into model instances. + # (name, ...) => RETURNING those columns, returned as dicts. + _returning: tuple[str, ...] | None def __init__(self): """Minimal init for descriptor mode. Use from_model() to create instances.""" @@ -306,6 +310,7 @@ def from_model(cls, model: type[T], query: Query | None = None) -> Self: instance._fields = None instance._defer_next_filter = False instance._deferred_filter = None + instance._returning = None return instance @overload @@ -924,6 +929,60 @@ def last(self) -> T | None: return obj return None + @overload + def returning(self) -> ReturningQuerySet[T, list[T]]: ... + + @overload + def returning(self, *fields: str) -> ReturningQuerySet[T, list[dict[str, Any]]]: ... + + def returning(self, *fields: str) -> ReturningQuerySet[T, Any]: + """Capture the rows touched by the next update() or delete(). + + With no arguments, update()/delete() return the affected rows as + model instances (RETURNING every concrete column). Given field + names, they return a list of dicts holding just those columns. + Without returning(), update()/delete() return an int rowcount. + + The field names are validated here, so a bad name errors at the + returning() call rather than when the write runs. + """ + clone = self._chain() + clone.__class__ = ReturningQuerySet + clone._returning = fields + clone._resolve_returning_fields() + return cast("ReturningQuerySet[T, Any]", clone) + + def _resolve_returning_fields(self) -> list[Field]: + """Translate self._returning into the concrete fields to RETURN.""" + meta = self.model._model_meta + if not self._returning: + # No names given: RETURN every concrete column so the rows can be + # hydrated into full model instances. + return list(meta.concrete_fields) + fields = [] + for name in self._returning: + try: + field = meta.get_field(name) + except FieldDoesNotExist: + raise FieldError( + f"Cannot resolve '{name}' in returning() for " + f"{self.model.model_options.object_name}: no such field." + ) + if not isinstance(field, Field) or not field.concrete: + raise FieldError( + f"Cannot use '{name}' in returning(): only concrete " + "database columns can be returned." + ) + fields.append(field) + return fields + + def _hydrate_returning(self, fields: list[Field], rows: list[list]) -> list[Any]: + """Turn converted RETURNING rows into instances (no names) or dicts.""" + field_names = cast("list[str]", [field.name for field in fields]) + if not self._returning: + return [self.model.from_db(field_names, row) for row in rows] + return [dict(zip(field_names, row)) for row in rows] + def delete(self) -> int: """Delete the records in the current QuerySet. @@ -931,6 +990,9 @@ def delete(self) -> int: handled by Postgres via the declared `on_delete` clauses and are not included in the count. """ + return self._execute_delete() + + def _execute_delete(self) -> Any: if self.sql_query.is_sliced: raise TypeError("Cannot use 'limit' or 'offset' with delete().") if self.sql_query.distinct or self.sql_query.distinct_fields: @@ -938,6 +1000,10 @@ def delete(self) -> int: if self._fields is not None: raise TypeError("Cannot call delete() after .values() or .values_list()") + returning_fields = ( + self._resolve_returning_fields() if self._returning is not None else None + ) + del_query = self._chain() del_query.sql_query.select_for_update = False del_query.sql_query.select_related = False @@ -947,34 +1013,51 @@ def delete(self) -> int: # Mark the connection so outer atomic() blocks see the abort state # even if the caller catches IntegrityError themselves. with transaction.mark_for_rollback_on_error(): - count = del_query._raw_delete() + result = del_query._raw_delete(returning_fields) # Clear the result cache, in case this QuerySet gets reused. self._result_cache = None - return count + if returning_fields is None: + return result + return self._hydrate_returning(returning_fields, result) - def _raw_delete(self) -> int: + def _raw_delete(self, returning_fields: list[Field] | None = None) -> Any: """ Delete objects found from the given queryset in single direct SQL query. No signals are sent and there is no protection for cascades. + + Returns the rowcount, or the converted RETURNING rows when + returning_fields is given (only the target table's rows — cascade + deletes never appear in a RETURNING clause). """ - query = self.sql_query.clone() + from plain.postgres.sql.compiler import convert_returning_rows + + query = cast(DeleteQuery, self.sql_query.clone()) query.__class__ = DeleteQuery + query.returning_fields = returning_fields cursor = query.get_compiler().execute_sql(CURSOR) - if cursor: - with cursor: + if not cursor: + return [] if returning_fields is not None else 0 + with cursor: + if returning_fields is None: return cursor.rowcount - return 0 + rows = cursor.fetchall() + return convert_returning_rows(rows, returning_fields, get_connection()) def update(self, **kwargs: Any) -> int: """ Update all elements in the current QuerySet, setting all the given fields to the appropriate values. """ + return self._execute_update(kwargs) + + def _execute_update(self, values: dict[str, Any]) -> Any: if self.sql_query.is_sliced: raise TypeError("Cannot update a query once a slice has been taken.") + if self._fields is not None: + raise TypeError("Cannot call update() after .values() or .values_list()") query = self.sql_query.chain(UpdateQuery) - query.add_update_values(kwargs) + query.add_update_values(values) # Inline annotations in order_by(), if possible. new_order_by = [] @@ -998,10 +1081,19 @@ def update(self, **kwargs: Any) -> int: # Clear any annotations so that they won't be present in subqueries. query.annotations = {} + + returning_fields = ( + self._resolve_returning_fields() if self._returning is not None else None + ) + if returning_fields is not None: + query.returning_fields = returning_fields + with transaction.mark_for_rollback_on_error(): - rows = query.get_compiler().execute_sql(CURSOR) + result = query.get_compiler().execute_sql(CURSOR) self._result_cache = None - return rows + if returning_fields is None: + return result + return self._hydrate_returning(returning_fields, result) def _update(self, values: list[tuple[Field, Any]]) -> int: """ @@ -1435,6 +1527,7 @@ def _clone(self) -> Self: c._known_related_objects = self._known_related_objects c._iterable_class = self._iterable_class c._fields = self._fields + c._returning = self._returning return c def _attach_result_cache(self, obj: Self, cache: list[T]) -> None: @@ -1518,6 +1611,23 @@ def _validate_values_are_expressions( ) +class ReturningQuerySet[T: "Model", R](QuerySet[T]): + """A QuerySet whose update()/delete() return the affected rows. + + Produced by QuerySet.returning(); the second type parameter R is the + return type of update()/delete() (a list of instances or of dicts), + pinned by the returning() overloads. The runtime behavior is driven by + self._returning — this subclass only makes the return types honest for + static checkers. + """ + + def update(self, **kwargs: Any) -> R: # ty: ignore[invalid-method-override] + return self._execute_update(kwargs) + + def delete(self) -> R: # ty: ignore[invalid-method-override] + return self._execute_delete() + + class InstanceCheckMeta(type): def __instancecheck__(self, instance: object) -> bool: return isinstance(instance, QuerySet) and instance.sql_query.is_empty() diff --git a/plain-postgres/plain/postgres/sql/compiler.py b/plain-postgres/plain/postgres/sql/compiler.py index 8c0e73f142..cf095e27ce 100644 --- a/plain-postgres/plain/postgres/sql/compiler.py +++ b/plain-postgres/plain/postgres/sql/compiler.py @@ -50,7 +50,12 @@ if TYPE_CHECKING: from plain.postgres.connection import DatabaseConnection from plain.postgres.expressions import BaseExpression - from plain.postgres.sql.query import AggregateQuery, InsertQuery + from plain.postgres.sql.query import ( + AggregateQuery, + DeleteQuery, + InsertQuery, + UpdateQuery, + ) # Type aliases for SQL compilation results SqlParams = tuple[Any, ...] @@ -103,6 +108,21 @@ def apply_converters( yield row +def convert_returning_rows( + rows: Iterable, fields: list[Any], connection: DatabaseConnection +) -> list[list]: + """Apply each field's DB converters to the raw rows of a RETURNING clause. + + The fields are given in the same order as the emitted RETURNING columns, + so each field lines up with its value in every row. + """ + cols = [field.get_col(field.model.model_options.db_table) for field in fields] + converters = get_converters(cols, connection) + if converters: + return list(apply_converters(rows, converters, connection)) + return [list(row) for row in rows] + + class SQLCompiler: # Multiline ordering SQL clause may appear from RawSQL. ordering_parts = _lazy_re_compile( @@ -1509,6 +1529,8 @@ def execute_sql( # ty: ignore[invalid-method-override] class SQLDeleteCompiler(SQLCompiler): + query: DeleteQuery + @cached_property def single_alias(self) -> bool: # Ensure base table is in aliases. @@ -1537,11 +1559,16 @@ def contains_self_reference_subquery(self) -> bool: def _as_sql(self, query: Query) -> SqlWithParams: delete = f"DELETE FROM {self.quote_name_unless_alias(query.base_table)}" # ty: ignore[invalid-argument-type] + returning = "" + if self.query.returning_fields: + r_sql, _ = return_insert_columns(self.query.returning_fields) + if r_sql: + returning = f" {r_sql}" try: where, params = self.compile(query.where) except FullResultSet: - return delete, () - return f"{delete} WHERE {where}", tuple(params) + return f"{delete}{returning}", () + return f"{delete} WHERE {where}{returning}", tuple(params) def as_sql( self, with_limits: bool = True, with_col_aliases: bool = False @@ -1564,6 +1591,8 @@ def as_sql( class SQLUpdateCompiler(SQLCompiler): + query: UpdateQuery + def as_sql( self, with_limits: bool = True, with_col_aliases: bool = False ) -> SqlWithParams: @@ -1628,16 +1657,29 @@ def as_sql( params = [] else: result.append(f"WHERE {where}") + if self.query.returning_fields: + r_sql, _ = return_insert_columns(self.query.returning_fields) + if r_sql: + result.append(r_sql) return " ".join(result), tuple(update_params + list(params)) - def execute_sql(self, result_type: str) -> int: # ty: ignore[invalid-method-override] - """Execute the update and return the number of rows affected.""" + def execute_sql(self, result_type: str) -> Any: # ty: ignore[invalid-method-override] + """Execute the update. + + Return the number of rows affected, or — when the query carries + returning_fields — the converted RETURNING rows. + """ cursor = super().execute_sql(result_type) + if not cursor: + return [] if self.query.returning_fields else 0 try: - return cursor.rowcount if cursor else 0 + if self.query.returning_fields: + return convert_returning_rows( + cursor.fetchall(), self.query.returning_fields, self.connection + ) + return cursor.rowcount finally: - if cursor: - cursor.close() + cursor.close() def pre_sql_setup( self, with_col_aliases: bool = False diff --git a/plain-postgres/plain/postgres/sql/query.py b/plain-postgres/plain/postgres/sql/query.py index 0b4a4d0a19..e87e2afd96 100644 --- a/plain-postgres/plain/postgres/sql/query.py +++ b/plain-postgres/plain/postgres/sql/query.py @@ -2472,6 +2472,9 @@ def update_join_types(self, query: Query) -> set[str]: class DeleteQuery(Query): """A DELETE SQL query.""" + # Concrete fields to emit in a RETURNING clause, or None for a plain DELETE. + returning_fields: list[Field] | None = None + def get_compiler(self, *, elide_empty: bool = True) -> SQLDeleteCompiler: from plain.postgres.sql.compiler import SQLDeleteCompiler @@ -2515,6 +2518,9 @@ def delete_batch(self, id_list: list[Any]) -> int: class UpdateQuery(Query): """An UPDATE SQL query.""" + # Concrete fields to emit in a RETURNING clause, or None for a plain UPDATE. + returning_fields: list[Field] | None = None + def get_compiler(self, *, elide_empty: bool = True) -> SQLUpdateCompiler: from plain.postgres.sql.compiler import SQLUpdateCompiler From 2fa9c0ad909af558dae0291b271314f01871aac8 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Thu, 23 Jul 2026 10:46:44 -0500 Subject: [PATCH 02/41] Test returning() on queryset update() and delete() Cover instances vs. dicts, unchanged int behavior, JSON converter application, bad-field validation, the returning()-before-filter chain, and that a cascade delete's child rows never appear in RETURNING. --- ...ingevent_returningparent_returningchild.py | 41 +++++ .../tests/app/examples/models/__init__.py | 1 + .../tests/app/examples/models/returning.py | 35 ++++ plain-postgres/tests/public/test_returning.py | 151 ++++++++++++++++++ 4 files changed, 228 insertions(+) create mode 100644 plain-postgres/tests/app/examples/migrations/0019_returningevent_returningparent_returningchild.py create mode 100644 plain-postgres/tests/app/examples/models/returning.py create mode 100644 plain-postgres/tests/public/test_returning.py diff --git a/plain-postgres/tests/app/examples/migrations/0019_returningevent_returningparent_returningchild.py b/plain-postgres/tests/app/examples/migrations/0019_returningevent_returningparent_returningchild.py new file mode 100644 index 0000000000..0f1e81867a --- /dev/null +++ b/plain-postgres/tests/app/examples/migrations/0019_returningevent_returningparent_returningchild.py @@ -0,0 +1,41 @@ +# Generated by Plain 0.154.0 on 2026-07-23 15:32 + +from plain import postgres +from plain.postgres import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ("examples", "0018_storageparametersexample"), + ] + + operations = [ + migrations.CreateModel( + name="ReturningEvent", + fields=[ + ("id", postgres.PrimaryKeyField()), + ("count", postgres.IntegerField(default=0)), + ("label", postgres.TextField(max_length=100)), + ("payload", postgres.JSONField(allow_null=True, required=False)), + ], + ), + migrations.CreateModel( + name="ReturningParent", + fields=[ + ("id", postgres.PrimaryKeyField()), + ("name", postgres.TextField(max_length=100)), + ], + ), + migrations.CreateModel( + name="ReturningChild", + fields=[ + ("id", postgres.PrimaryKeyField()), + ( + "parent", + postgres.ForeignKeyField( + on_delete=postgres.CASCADE, to="examples.returningparent" + ), + ), + ], + ), + ] diff --git a/plain-postgres/tests/app/examples/models/__init__.py b/plain-postgres/tests/app/examples/models/__init__.py index 66e6b7039b..eff21dad43 100644 --- a/plain-postgres/tests/app/examples/models/__init__.py +++ b/plain-postgres/tests/app/examples/models/__init__.py @@ -13,6 +13,7 @@ nullability, querysets, relationships, + returning, storage_parameters, trees, unregistered, diff --git a/plain-postgres/tests/app/examples/models/returning.py b/plain-postgres/tests/app/examples/models/returning.py new file mode 100644 index 0000000000..1a1223172d --- /dev/null +++ b/plain-postgres/tests/app/examples/models/returning.py @@ -0,0 +1,35 @@ +"""Test fixtures for QuerySet.returning() on update()/delete().""" + +from __future__ import annotations + +from typing import Any + +from plain import postgres +from plain.postgres import types + + +@postgres.register_model +class ReturningEvent(postgres.Model): + label = types.TextField(max_length=100) + count = types.IntegerField(default=0) + payload: dict[str, Any] | None = types.JSONField(required=False, allow_null=True) + + query: postgres.QuerySet[ReturningEvent] = postgres.QuerySet() + + +@postgres.register_model +class ReturningParent(postgres.Model): + name = types.TextField(max_length=100) + + query: postgres.QuerySet[ReturningParent] = postgres.QuerySet() + + children: types.ReverseForeignKey[ReturningChild] = types.ReverseForeignKey( + to="ReturningChild", field="parent" + ) + + +@postgres.register_model +class ReturningChild(postgres.Model): + parent = types.ForeignKeyField(ReturningParent, on_delete=postgres.CASCADE) + + query: postgres.QuerySet[ReturningChild] = postgres.QuerySet() diff --git a/plain-postgres/tests/public/test_returning.py b/plain-postgres/tests/public/test_returning.py new file mode 100644 index 0000000000..783e54517d --- /dev/null +++ b/plain-postgres/tests/public/test_returning.py @@ -0,0 +1,151 @@ +"""QuerySet.returning() captures the rows touched by update() and delete(). + +Without returning(), update()/delete() return an int rowcount as always. +With it, no-arg returning() hydrates full model instances and returning(*names) +returns a list of dicts holding just those columns. +""" + +from __future__ import annotations + +import pytest +from app.examples.models.returning import ( + ReturningChild, + ReturningEvent, + ReturningParent, +) + +from plain.postgres.exceptions import FieldError +from plain.postgres.query import ReturningQuerySet + + +def _seed_events() -> None: + ReturningEvent(label="a", count=1, payload={"n": 1}).create() + ReturningEvent(label="a", count=1, payload={"n": 2}).create() + ReturningEvent(label="b", count=1, payload=None).create() + + +# =========================================================================== +# update() +# =========================================================================== + + +def test_update_without_returning_returns_int(db): + _seed_events() + result = ReturningEvent.query.filter(label="a").update(count=5) + assert result == 2 + + +def test_update_returning_instances_reflect_new_values(db): + _seed_events() + rows = ReturningEvent.query.filter(label="a").returning().update(count=9) + + assert len(rows) == 2 + assert all(isinstance(row, ReturningEvent) for row in rows) + # RETURNING on UPDATE reports the post-update values. + assert {row.count for row in rows} == {9} + # JSON converters are applied — payload comes back as a dict, not a string. + payloads = [row.payload for row in rows] + assert {p["n"] for p in payloads if p} == {1, 2} + + +def test_update_returning_named_fields_are_dicts(db): + _seed_events() + rows = ( + ReturningEvent.query.filter(label="a").returning("id", "count").update(count=7) + ) + + assert len(rows) == 2 + assert all(isinstance(row, dict) for row in rows) + assert all(set(row) == {"id", "count"} for row in rows) + assert {row["count"] for row in rows} == {7} + + +def test_update_returning_empty_result_is_empty_list(db): + _seed_events() + rows = ReturningEvent.query.filter(label="missing").returning().update(count=1) + assert rows == [] + + +# =========================================================================== +# delete() +# =========================================================================== + + +def test_delete_without_returning_returns_int(db): + _seed_events() + result = ReturningEvent.query.filter(label="a").delete() + assert result == 2 + + +def test_delete_returning_named_fields_gives_deleted_rows(db): + _seed_events() + rows = ReturningEvent.query.filter(label="a").returning("id", "payload").delete() + + assert len(rows) == 2 + assert all(isinstance(row, dict) and set(row) == {"id", "payload"} for row in rows) + # DELETE ... RETURNING reports the rows as they were. + assert {tuple(row["payload"].items()) for row in rows} == { + (("n", 1),), + (("n", 2),), + } + assert not ReturningEvent.query.filter(label="a").exists() + + +def test_delete_returning_instances(db): + _seed_events() + rows = ReturningEvent.query.filter(label="b").returning().delete() + + assert len(rows) == 1 + assert isinstance(rows[0], ReturningEvent) + assert rows[0].label == "b" + + +def test_delete_returning_empty_result_is_empty_list(db): + rows = ReturningEvent.query.filter(label="missing").returning("id").delete() + assert rows == [] + + +# =========================================================================== +# Validation and typing +# =========================================================================== + + +def test_returning_returns_a_returning_queryset(db): + assert isinstance(ReturningEvent.query.returning(), ReturningQuerySet) + assert isinstance(ReturningEvent.query.returning("id"), ReturningQuerySet) + + +def test_returning_bad_field_name_errors(db): + with pytest.raises(FieldError, match="no such field"): + ReturningEvent.query.returning("not_a_field") + + +def test_returning_before_filter_is_preserved(db): + _seed_events() + rows = ReturningEvent.query.returning().filter(label="a").update(count=3) + assert len(rows) == 2 + assert {row.count for row in rows} == {3} + + +def test_returning_then_values_update_errors(db): + # .values() + returning() is nonsensical; it must not silently misbehave. + with pytest.raises(TypeError, match="after .values"): + ReturningEvent.query.returning().values("id").update(count=1) + + +# =========================================================================== +# FK cascade — RETURNING only reports the target table's rows +# =========================================================================== + + +def test_delete_returning_excludes_cascade_deleted_children(db): + parent = ReturningParent(name="p").create() + ReturningChild(parent=parent).create() + ReturningChild(parent=parent).create() + + rows = ReturningParent.query.filter(id=parent.id).returning("id", "name").delete() + + # Only the parent row comes back, even though two children were cascaded. + assert len(rows) == 1 + assert rows[0] == {"id": parent.id, "name": "p"} + assert ReturningChild.query.count() == 0 From ce13350b0ce5c675098e9fc357d6df7a0c84dce2 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Thu, 23 Jul 2026 11:10:00 -0500 Subject: [PATCH 03/41] Consolidate RETURNING plumbing across insert/update/delete - Move DELETE RETURNING handling into SQLDeleteCompiler.execute_sql, symmetric to the UPDATE compiler; QuerySet._raw_delete and DeleteQuery.do_query now just delegate to it - Extract SQLCompiler._returning_sql(), shared by the UPDATE and DELETE compilers instead of duplicating the emission block - Rename return_insert_columns to returning_columns and return just the SQL string (the params tuple was always empty) - Reuse convert_returning_rows in the insert execute path instead of its own inline cols/converters copy - Resolve returning fields once at returning() time, stored on the clone as _returning_fields, instead of re-resolving at execute - Inline _execute_update/_execute_delete back into update()/delete(); ReturningQuerySet delegates through super() - Drop the duplicate cascade fixture; the cascade-exclusion test now reuses DeleteParent/ChildCascade --- plain-postgres/plain/postgres/dialect.py | 8 +-- plain-postgres/plain/postgres/query.py | 44 +++++-------- plain-postgres/plain/postgres/sql/compiler.py | 62 +++++++++++-------- plain-postgres/plain/postgres/sql/query.py | 8 +-- .../migrations/0019_returningevent.py | 22 +++++++ ...ingevent_returningparent_returningchild.py | 41 ------------ .../tests/app/examples/models/returning.py | 18 ------ plain-postgres/tests/public/test_returning.py | 17 +++-- 8 files changed, 88 insertions(+), 132 deletions(-) create mode 100644 plain-postgres/tests/app/examples/migrations/0019_returningevent.py delete mode 100644 plain-postgres/tests/app/examples/migrations/0019_returningevent_returningparent_returningchild.py diff --git a/plain-postgres/plain/postgres/dialect.py b/plain-postgres/plain/postgres/dialect.py index 98fcafe561..112cac6424 100644 --- a/plain-postgres/plain/postgres/dialect.py +++ b/plain-postgres/plain/postgres/dialect.py @@ -380,15 +380,15 @@ def lookup_cast(lookup_type: str, field: Field | None = None) -> str: return lookup -def return_insert_columns(fields: list[Field]) -> tuple[str, tuple[Any, ...]]: - """Return the RETURNING clause SQL and params to append to an INSERT query.""" +def returning_columns(fields: list[Field]) -> str: + """Return the RETURNING clause SQL for the given fields, or "" when empty.""" if not fields: - return "", () + return "" columns = [ f"{quote_name(field.model.model_options.db_table)}.{quote_name(field.column)}" for field in fields ] - return "RETURNING {}".format(", ".join(columns)), () + return "RETURNING {}".format(", ".join(columns)) def bulk_insert_sql(fields: list[Field], placeholder_rows: list[list[str]]) -> str: diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index 8f9cc72828..6e6c3ccfba 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -290,6 +290,9 @@ class Task(Model): # () => RETURNING every concrete column, hydrated into model instances. # (name, ...) => RETURNING those columns, returned as dicts. _returning: tuple[str, ...] | None + # The concrete fields resolved from _returning, or None for a plain write. + # Set once by returning() so update()/delete() don't re-resolve at execute. + _returning_fields: list[Field] | None def __init__(self): """Minimal init for descriptor mode. Use from_model() to create instances.""" @@ -311,6 +314,7 @@ def from_model(cls, model: type[T], query: Query | None = None) -> Self: instance._defer_next_filter = False instance._deferred_filter = None instance._returning = None + instance._returning_fields = None return instance @overload @@ -949,7 +953,7 @@ def returning(self, *fields: str) -> ReturningQuerySet[T, Any]: clone = self._chain() clone.__class__ = ReturningQuerySet clone._returning = fields - clone._resolve_returning_fields() + clone._returning_fields = clone._resolve_returning_fields() return cast("ReturningQuerySet[T, Any]", clone) def _resolve_returning_fields(self) -> list[Field]: @@ -990,9 +994,6 @@ def delete(self) -> int: handled by Postgres via the declared `on_delete` clauses and are not included in the count. """ - return self._execute_delete() - - def _execute_delete(self) -> Any: if self.sql_query.is_sliced: raise TypeError("Cannot use 'limit' or 'offset' with delete().") if self.sql_query.distinct or self.sql_query.distinct_fields: @@ -1000,9 +1001,7 @@ def _execute_delete(self) -> Any: if self._fields is not None: raise TypeError("Cannot call delete() after .values() or .values_list()") - returning_fields = ( - self._resolve_returning_fields() if self._returning is not None else None - ) + returning_fields = self._returning_fields del_query = self._chain() del_query.sql_query.select_for_update = False @@ -1019,7 +1018,8 @@ def _execute_delete(self) -> Any: self._result_cache = None if returning_fields is None: return result - return self._hydrate_returning(returning_fields, result) + # Reachable only via ReturningQuerySet.delete(), which returns R. + return self._hydrate_returning(returning_fields, result) # ty: ignore[invalid-return-type] def _raw_delete(self, returning_fields: list[Field] | None = None) -> Any: """ @@ -1030,34 +1030,22 @@ def _raw_delete(self, returning_fields: list[Field] | None = None) -> Any: returning_fields is given (only the target table's rows — cascade deletes never appear in a RETURNING clause). """ - from plain.postgres.sql.compiler import convert_returning_rows - query = cast(DeleteQuery, self.sql_query.clone()) query.__class__ = DeleteQuery query.returning_fields = returning_fields - cursor = query.get_compiler().execute_sql(CURSOR) - if not cursor: - return [] if returning_fields is not None else 0 - with cursor: - if returning_fields is None: - return cursor.rowcount - rows = cursor.fetchall() - return convert_returning_rows(rows, returning_fields, get_connection()) + return query.get_compiler().execute_sql(CURSOR) def update(self, **kwargs: Any) -> int: """ Update all elements in the current QuerySet, setting all the given fields to the appropriate values. """ - return self._execute_update(kwargs) - - def _execute_update(self, values: dict[str, Any]) -> Any: if self.sql_query.is_sliced: raise TypeError("Cannot update a query once a slice has been taken.") if self._fields is not None: raise TypeError("Cannot call update() after .values() or .values_list()") query = self.sql_query.chain(UpdateQuery) - query.add_update_values(values) + query.add_update_values(kwargs) # Inline annotations in order_by(), if possible. new_order_by = [] @@ -1082,9 +1070,7 @@ def _execute_update(self, values: dict[str, Any]) -> Any: # Clear any annotations so that they won't be present in subqueries. query.annotations = {} - returning_fields = ( - self._resolve_returning_fields() if self._returning is not None else None - ) + returning_fields = self._returning_fields if returning_fields is not None: query.returning_fields = returning_fields @@ -1093,7 +1079,8 @@ def _execute_update(self, values: dict[str, Any]) -> Any: self._result_cache = None if returning_fields is None: return result - return self._hydrate_returning(returning_fields, result) + # Reachable only via ReturningQuerySet.update(), which returns R. + return self._hydrate_returning(returning_fields, result) # ty: ignore[invalid-return-type] def _update(self, values: list[tuple[Field, Any]]) -> int: """ @@ -1528,6 +1515,7 @@ def _clone(self) -> Self: c._iterable_class = self._iterable_class c._fields = self._fields c._returning = self._returning + c._returning_fields = self._returning_fields return c def _attach_result_cache(self, obj: Self, cache: list[T]) -> None: @@ -1622,10 +1610,10 @@ class ReturningQuerySet[T: "Model", R](QuerySet[T]): """ def update(self, **kwargs: Any) -> R: # ty: ignore[invalid-method-override] - return self._execute_update(kwargs) + return cast("R", super().update(**kwargs)) def delete(self) -> R: # ty: ignore[invalid-method-override] - return self._execute_delete() + return cast("R", super().delete()) class InstanceCheckMeta(type): diff --git a/plain-postgres/plain/postgres/sql/compiler.py b/plain-postgres/plain/postgres/sql/compiler.py index cf095e27ce..f170700724 100644 --- a/plain-postgres/plain/postgres/sql/compiler.py +++ b/plain-postgres/plain/postgres/sql/compiler.py @@ -18,7 +18,7 @@ limit_offset_sql, on_conflict_suffix_sql, quote_name, - return_insert_columns, + returning_columns, ) from plain.postgres.exceptions import EmptyResultSet, FieldError, FullResultSet from plain.postgres.expressions import ( @@ -528,6 +528,14 @@ def compile(self, node: SQLCompilable) -> SqlWithParams: sql, params = node.as_sql(self, self.connection) return sql, tuple(params) + def _returning_sql(self) -> str: + """Return the RETURNING clause for this query's returning_fields, or "". + + Shared by the UPDATE and DELETE compilers, whose queries carry + returning_fields; the RETURNING clause never takes params. + """ + return returning_columns(self.query.returning_fields or []) # ty: ignore[unresolved-attribute] + def get_qualify_sql(self) -> tuple[list[str], list[Any]]: where_parts = [] if self.where: @@ -1336,7 +1344,6 @@ def explain_query(self) -> Generator[str]: class SQLInsertCompiler(SQLCompiler): query: InsertQuery returning_fields: list | None = None - returning_params: tuple = () def field_as_sql(self, field: Any, val: Any) -> tuple[str, list]: """ @@ -1487,17 +1494,11 @@ def as_sql( # ty: ignore[invalid-method-override] # Returns list for internal result.append( bulk_insert_sql(fields, placeholder_rows) # ty: ignore[invalid-argument-type] ) - params = param_rows if conflict_suffix_sql: result.append(conflict_suffix_sql) - # Skip empty r_sql in case returning_cols returns an empty string. - returning_cols = return_insert_columns(self.returning_fields) - if returning_cols: - r_sql, self.returning_params = returning_cols - if r_sql: - result.append(r_sql) - params += [list(self.returning_params)] - return [(" ".join(result), tuple(chain.from_iterable(params)))] + if returning := returning_columns(self.returning_fields): + result.append(returning) + return [(" ".join(result), tuple(chain.from_iterable(param_rows)))] # Bulk insert without returning fields result.append(bulk_insert_sql(fields, placeholder_rows)) # ty: ignore[invalid-argument-type] @@ -1509,7 +1510,6 @@ def execute_sql( # ty: ignore[invalid-method-override] self, returning_fields: list | None = None ) -> list: assert self.query.model is not None, "INSERT execution requires a model" - options = self.query.model.model_options self.returning_fields = returning_fields with self.connection.cursor() as cursor: for sql, params in self.as_sql(): @@ -1521,11 +1521,7 @@ def execute_sql( # ty: ignore[invalid-method-override] rows = cursor.fetchall() else: rows = [cursor.fetchone()] - cols = [field.get_col(options.db_table) for field in self.returning_fields] - converters = get_converters(cols, self.connection) - if converters: - rows = list(apply_converters(rows, converters, self.connection)) - return rows + return convert_returning_rows(rows, self.returning_fields, self.connection) class SQLDeleteCompiler(SQLCompiler): @@ -1559,11 +1555,9 @@ def contains_self_reference_subquery(self) -> bool: def _as_sql(self, query: Query) -> SqlWithParams: delete = f"DELETE FROM {self.quote_name_unless_alias(query.base_table)}" # ty: ignore[invalid-argument-type] - returning = "" - if self.query.returning_fields: - r_sql, _ = return_insert_columns(self.query.returning_fields) - if r_sql: - returning = f" {r_sql}" + returning = self._returning_sql() + if returning: + returning = f" {returning}" try: where, params = self.compile(query.where) except FullResultSet: @@ -1589,6 +1583,24 @@ def as_sql( outerq.add_filter("id__in", innerq) return self._as_sql(outerq) + def execute_sql(self, result_type: str) -> Any: # ty: ignore[invalid-method-override] + """Execute the delete. + + Return the number of rows deleted, or — when the query carries + returning_fields — the converted RETURNING rows. + """ + cursor = super().execute_sql(result_type) + if not cursor: + return [] if self.query.returning_fields else 0 + try: + if self.query.returning_fields: + return convert_returning_rows( + cursor.fetchall(), self.query.returning_fields, self.connection + ) + return cursor.rowcount + finally: + cursor.close() + class SQLUpdateCompiler(SQLCompiler): query: UpdateQuery @@ -1657,10 +1669,8 @@ def as_sql( params = [] else: result.append(f"WHERE {where}") - if self.query.returning_fields: - r_sql, _ = return_insert_columns(self.query.returning_fields) - if r_sql: - result.append(r_sql) + if returning := self._returning_sql(): + result.append(returning) return " ".join(result), tuple(update_params + list(params)) def execute_sql(self, result_type: str) -> Any: # ty: ignore[invalid-method-override] diff --git a/plain-postgres/plain/postgres/sql/query.py b/plain-postgres/plain/postgres/sql/query.py index e87e2afd96..ca9e641cc2 100644 --- a/plain-postgres/plain/postgres/sql/query.py +++ b/plain-postgres/plain/postgres/sql/query.py @@ -2485,11 +2485,9 @@ def do_query(self, table: str, where: Any) -> int: self.alias_map = {table: self.alias_map[table]} self.where = where - cursor = self.get_compiler().execute_sql(CURSOR) - if cursor: - with cursor: - return cursor.rowcount - return 0 + # The compiler returns the deleted row count directly (this query never + # carries returning_fields). + return self.get_compiler().execute_sql(CURSOR) def delete_batch(self, id_list: list[Any]) -> int: """ diff --git a/plain-postgres/tests/app/examples/migrations/0019_returningevent.py b/plain-postgres/tests/app/examples/migrations/0019_returningevent.py new file mode 100644 index 0000000000..c892fe015f --- /dev/null +++ b/plain-postgres/tests/app/examples/migrations/0019_returningevent.py @@ -0,0 +1,22 @@ +# Generated by Plain 0.154.0 on 2026-07-23 16:03 + +from plain import postgres +from plain.postgres import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ("examples", "0018_storageparametersexample"), + ] + + operations = [ + migrations.CreateModel( + name="ReturningEvent", + fields=[ + ("id", postgres.PrimaryKeyField()), + ("count", postgres.IntegerField(default=0)), + ("label", postgres.TextField(max_length=100)), + ("payload", postgres.JSONField(allow_null=True, required=False)), + ], + ), + ] diff --git a/plain-postgres/tests/app/examples/migrations/0019_returningevent_returningparent_returningchild.py b/plain-postgres/tests/app/examples/migrations/0019_returningevent_returningparent_returningchild.py deleted file mode 100644 index 0f1e81867a..0000000000 --- a/plain-postgres/tests/app/examples/migrations/0019_returningevent_returningparent_returningchild.py +++ /dev/null @@ -1,41 +0,0 @@ -# Generated by Plain 0.154.0 on 2026-07-23 15:32 - -from plain import postgres -from plain.postgres import migrations - - -class Migration(migrations.Migration): - dependencies = [ - ("examples", "0018_storageparametersexample"), - ] - - operations = [ - migrations.CreateModel( - name="ReturningEvent", - fields=[ - ("id", postgres.PrimaryKeyField()), - ("count", postgres.IntegerField(default=0)), - ("label", postgres.TextField(max_length=100)), - ("payload", postgres.JSONField(allow_null=True, required=False)), - ], - ), - migrations.CreateModel( - name="ReturningParent", - fields=[ - ("id", postgres.PrimaryKeyField()), - ("name", postgres.TextField(max_length=100)), - ], - ), - migrations.CreateModel( - name="ReturningChild", - fields=[ - ("id", postgres.PrimaryKeyField()), - ( - "parent", - postgres.ForeignKeyField( - on_delete=postgres.CASCADE, to="examples.returningparent" - ), - ), - ], - ), - ] diff --git a/plain-postgres/tests/app/examples/models/returning.py b/plain-postgres/tests/app/examples/models/returning.py index 1a1223172d..29267012d0 100644 --- a/plain-postgres/tests/app/examples/models/returning.py +++ b/plain-postgres/tests/app/examples/models/returning.py @@ -15,21 +15,3 @@ class ReturningEvent(postgres.Model): payload: dict[str, Any] | None = types.JSONField(required=False, allow_null=True) query: postgres.QuerySet[ReturningEvent] = postgres.QuerySet() - - -@postgres.register_model -class ReturningParent(postgres.Model): - name = types.TextField(max_length=100) - - query: postgres.QuerySet[ReturningParent] = postgres.QuerySet() - - children: types.ReverseForeignKey[ReturningChild] = types.ReverseForeignKey( - to="ReturningChild", field="parent" - ) - - -@postgres.register_model -class ReturningChild(postgres.Model): - parent = types.ForeignKeyField(ReturningParent, on_delete=postgres.CASCADE) - - query: postgres.QuerySet[ReturningChild] = postgres.QuerySet() diff --git a/plain-postgres/tests/public/test_returning.py b/plain-postgres/tests/public/test_returning.py index 783e54517d..f3306b15ca 100644 --- a/plain-postgres/tests/public/test_returning.py +++ b/plain-postgres/tests/public/test_returning.py @@ -8,11 +8,8 @@ from __future__ import annotations import pytest -from app.examples.models.returning import ( - ReturningChild, - ReturningEvent, - ReturningParent, -) +from app.examples.models.delete import ChildCascade, DeleteParent +from app.examples.models.returning import ReturningEvent from plain.postgres.exceptions import FieldError from plain.postgres.query import ReturningQuerySet @@ -139,13 +136,13 @@ def test_returning_then_values_update_errors(db): def test_delete_returning_excludes_cascade_deleted_children(db): - parent = ReturningParent(name="p").create() - ReturningChild(parent=parent).create() - ReturningChild(parent=parent).create() + parent = DeleteParent(name="p").create() + ChildCascade(parent=parent).create() + ChildCascade(parent=parent).create() - rows = ReturningParent.query.filter(id=parent.id).returning("id", "name").delete() + rows = DeleteParent.query.filter(id=parent.id).returning("id", "name").delete() # Only the parent row comes back, even though two children were cascaded. assert len(rows) == 1 assert rows[0] == {"id": parent.id, "name": "p"} - assert ReturningChild.query.count() == 0 + assert ChildCascade.query.count() == 0 From b43c3c2cdf811e9b26b1ba5d33f3a9a315541c08 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Thu, 23 Jul 2026 11:31:01 -0500 Subject: [PATCH 04/41] Add bulk_upsert() and make bulk_create insert-only --- plain-cache/plain/cache/core.py | 3 +- plain-postgres/plain/postgres/query.py | 255 ++++++++++++------ .../examples/migrations/0020_upsertitem.py | 22 ++ .../tests/app/examples/models/__init__.py | 1 + .../tests/app/examples/models/upsert.py | 21 ++ .../tests/public/test_bulk_upsert.py | 149 ++++++++++ 6 files changed, 365 insertions(+), 86 deletions(-) create mode 100644 plain-postgres/tests/app/examples/migrations/0020_upsertitem.py create mode 100644 plain-postgres/tests/app/examples/models/upsert.py create mode 100644 plain-postgres/tests/public/test_bulk_upsert.py diff --git a/plain-cache/plain/cache/core.py b/plain-cache/plain/cache/core.py index 8504c8ffcc..27c479e948 100644 --- a/plain-cache/plain/cache/core.py +++ b/plain-cache/plain/cache/core.py @@ -118,9 +118,8 @@ def set_many( self._model(key=key, value=value, expires_at=expires_at, created_at=now) for key, value in mapping.items() ] - self._model.query.bulk_create( + self._model.query.bulk_upsert( items, - update_conflicts=True, update_fields=["value", "expires_at", "updated_at"], unique_fields=["key"], ) diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index 6e6c3ccfba..0d8b7813a5 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -633,62 +633,18 @@ def _prepare_for_bulk_create(self, objs: list[T]) -> None: for obj in objs: obj._prepare_related_fields_for_save(operation_name="bulk_create") - def _check_bulk_create_options( - self, - update_conflicts: bool, - update_fields: list[Field] | None, - unique_fields: list[Field] | None, - ) -> OnConflict | None: - if update_conflicts: - if not update_fields: - raise ValueError( - "Fields that will be updated when a row insertion fails " - "on conflicts must be provided." - ) - if not unique_fields: - raise ValueError( - "Unique fields that can trigger the upsert must be provided." - ) - # Updating primary keys and non-concrete fields is forbidden. - from plain.postgres.fields.related import ManyToManyField - - if any( - not f.concrete or isinstance(f, ManyToManyField) for f in update_fields - ): - raise ValueError( - "bulk_create() can only be used with concrete fields in " - "update_fields." - ) - if any(f.primary_key for f in update_fields): - raise ValueError( - "bulk_create() cannot be used with primary keys in update_fields." - ) - if unique_fields: - from plain.postgres.fields.related import ManyToManyField - - if any( - not f.concrete or isinstance(f, ManyToManyField) - for f in unique_fields - ): - raise ValueError( - "bulk_create() can only be used with concrete fields " - "in unique_fields." - ) - return OnConflict.UPDATE - return None - def bulk_create( self, objs: Sequence[T], batch_size: int | None = None, - update_conflicts: bool = False, - update_fields: list[str] | None = None, - unique_fields: list[str] | None = None, ) -> list[T]: """ Insert each of the instances into the database. Do *not* call save() on each of the instances. Primary keys are set on the objects via the PostgreSQL RETURNING clause. Multi-table models are not supported. + + This is insert-only -- to insert-or-update on a conflict, use + bulk_upsert(). """ if batch_size is not None and batch_size <= 0: raise ValueError("Batch size must be a positive integer.") @@ -697,21 +653,6 @@ def bulk_create( if not objs: return objs meta = self.model._model_meta - unique_fields_objs: list[Field] | None = None - update_fields_objs: list[Field] | None = None - if unique_fields: - unique_fields_objs = [ - meta.get_forward_field(name) for name in unique_fields - ] - if update_fields: - update_fields_objs = [ - meta.get_forward_field(name) for name in update_fields - ] - on_conflict = self._check_bulk_create_options( - update_conflicts, - update_fields_objs, - unique_fields_objs, - ) fields = meta.concrete_fields self._prepare_for_bulk_create(objs) with transaction.atomic(savepoint=False): @@ -721,9 +662,6 @@ def bulk_create( objs_with_id, fields, batch_size, - on_conflict=on_conflict, - update_fields=update_fields_objs, - unique_fields=unique_fields_objs, ) id_field = meta.get_forward_field("id") for obj_with_id, results in zip(objs_with_id, returned_columns): @@ -739,12 +677,8 @@ def bulk_create( objs_without_id, fields, batch_size, - on_conflict=on_conflict, - update_fields=update_fields_objs, - unique_fields=unique_fields_objs, ) - if on_conflict is None: - assert len(returned_columns) == len(objs_without_id) + assert len(returned_columns) == len(objs_without_id) for obj_without_id, results in zip(objs_without_id, returned_columns): for result, field in zip(results, meta.db_returning_fields): assert field.name is not None @@ -753,6 +687,142 @@ def bulk_create( return objs + def _unique_fields_match_constraint(self, unique_fields: list[Field]) -> bool: + """True if unique_fields names the primary key, or a UniqueConstraint on + the model that has no condition and no expressions.""" + names = {f.name for f in unique_fields} + pk_field = self.model._model_meta.get_forward_field("id") + if names == {pk_field.name}: + return True + for constraint in self.model.model_options.total_unique_constraints: + if set(constraint.fields) == names: + return True + return False + + def _check_bulk_upsert_options( + self, + objs: list[T], + update_fields: list[Field], + unique_fields: list[Field], + ) -> None: + from plain.postgres.fields.related import ManyToManyField + + model_name = self.model.__name__ + + if not unique_fields: + raise ValueError("bulk_upsert() requires unique_fields.") + if not self._unique_fields_match_constraint(unique_fields): + names = [f.name for f in unique_fields] + raise ValueError( + f"bulk_upsert() unique_fields {names} on {model_name} must name " + "the primary key or a UniqueConstraint declared on the model " + "without a condition or expressions." + ) + + if not update_fields: + raise ValueError("bulk_upsert() requires update_fields.") + if any(not f.concrete or isinstance(f, ManyToManyField) for f in update_fields): + raise ValueError("bulk_upsert() update_fields must be concrete fields.") + if any(f.primary_key for f in update_fields): + raise ValueError("bulk_upsert() cannot update primary key fields.") + overlap = {f.name for f in update_fields} & {f.name for f in unique_fields} + if overlap: + raise ValueError( + "bulk_upsert() update_fields cannot overlap unique_fields: " + f"{sorted(overlap)}." + ) + + for field in unique_fields: + for obj in objs: + if field.value_from_object(obj) is None: + raise ValueError( + f"bulk_upsert() requires a non-null {field.name} on every " + "object; NULL never conflicts in Postgres, so it cannot " + "be upserted." + ) + + def bulk_upsert( + self, + objs: Sequence[T], + *, + update_fields: list[str], + unique_fields: list[str], + batch_size: int | None = None, + ) -> list[T]: + """ + Insert each instance, updating update_fields on any row that already + exists for the unique_fields key. Issues one + INSERT ... ON CONFLICT (unique_fields) DO UPDATE ... RETURNING per batch. + + Both inserted and updated objects come back with their DB-returned + fields (primary key, DB defaults) populated. unique_fields must name the + primary key or a UniqueConstraint declared on the model. + """ + if batch_size is not None and batch_size <= 0: + raise ValueError("Batch size must be a positive integer.") + + objs = list(objs) + if not objs: + return objs + + meta = self.model._model_meta + unique_fields_objs = [meta.get_forward_field(name) for name in unique_fields] + update_fields_objs = [meta.get_forward_field(name) for name in update_fields] + self._check_bulk_upsert_options(objs, update_fields_objs, unique_fields_objs) + + self._prepare_for_bulk_create(objs) + + # Include the PK column only when it is itself the conflict key; + # otherwise let Postgres generate the identity value. + pk_is_unique = any(f.primary_key for f in unique_fields_objs) + fields = meta.concrete_fields + if not pk_is_unique: + fields = [f for f in fields if not isinstance(f, PrimaryKeyField)] + + # RETURNING must carry the DB-returned fields (to populate the objects) + # plus the unique fields (to match each returned row to its object). + returning_fields = list(meta.db_returning_fields) + for field in unique_fields_objs: + if field not in returning_fields: + returning_fields.append(field) + unique_indices = [returning_fields.index(f) for f in unique_fields_objs] + db_returning_indices = [ + (returning_fields.index(f), f) for f in meta.db_returning_fields + ] + + # Sort by the conflict key so concurrent upserts touching overlapping + # keys lock rows in the same order and can't deadlock each other. + ordered = sorted( + objs, + key=lambda o: tuple(f.value_from_object(o) for f in unique_fields_objs), + ) + + with transaction.atomic(savepoint=False): + returned_rows = self._batched_upsert( + ordered, + fields, + returning_fields, + batch_size, + update_fields=update_fields_objs, + unique_fields=unique_fields_objs, + ) + + # RETURNING order isn't guaranteed to match VALUES order under ON + # CONFLICT, so match each returned row to its object by the unique key. + row_by_key = {} + for row in returned_rows: + key = tuple(row[i] for i in unique_indices) + row_by_key[key] = row + for obj in objs: + key = tuple(f.value_from_object(obj) for f in unique_fields_objs) + row = row_by_key[key] + for index, field in db_returning_indices: + assert field.name is not None + setattr(obj, field.name, row[index]) + obj._state.adding = False + + return objs + def bulk_update( self, objs: Sequence[T], fields: list[str], batch_size: int | None = None ) -> int: @@ -1460,9 +1530,6 @@ def _batched_insert( objs: list[T], fields: list[Field], batch_size: int | None, - on_conflict: OnConflict | None = None, - update_fields: list[Field] | None = None, - unique_fields: list[Field] | None = None, ) -> list[tuple[Any, ...]]: """ Helper method for bulk_create() to insert objs one batch at a time. @@ -1471,23 +1538,43 @@ def _batched_insert( batch_size = min(batch_size, max_batch_size) if batch_size else max_batch_size inserted_rows = [] for item in [objs[i : i + batch_size] for i in range(0, len(objs), batch_size)]: - if on_conflict is None: - inserted_rows.extend( - self._insert( # ty: ignore[invalid-argument-type] - item, - fields=fields, - returning_fields=self.model._model_meta.db_returning_fields, - ) + inserted_rows.extend( + self._insert( # ty: ignore[invalid-argument-type] + item, + fields=fields, + returning_fields=self.model._model_meta.db_returning_fields, ) - else: - self._insert( + ) + return inserted_rows + + def _batched_upsert( + self, + objs: list[T], + fields: list[Field], + returning_fields: list[Field], + batch_size: int | None, + update_fields: list[Field], + unique_fields: list[Field], + ) -> list[tuple[Any, ...]]: + """ + Helper method for bulk_upsert() to insert objs one batch at a time as + ON CONFLICT DO UPDATE, collecting the RETURNING rows from every batch. + """ + max_batch_size = max(len(objs), 1) + batch_size = min(batch_size, max_batch_size) if batch_size else max_batch_size + returned_rows = [] + for item in [objs[i : i + batch_size] for i in range(0, len(objs), batch_size)]: + returned_rows.extend( + self._insert( # ty: ignore[invalid-argument-type] item, fields=fields, - on_conflict=on_conflict, + returning_fields=returning_fields, + on_conflict=OnConflict.UPDATE, update_fields=update_fields, unique_fields=unique_fields, ) - return inserted_rows + ) + return returned_rows def _chain(self) -> Self: """ diff --git a/plain-postgres/tests/app/examples/migrations/0020_upsertitem.py b/plain-postgres/tests/app/examples/migrations/0020_upsertitem.py new file mode 100644 index 0000000000..c4af3c93fd --- /dev/null +++ b/plain-postgres/tests/app/examples/migrations/0020_upsertitem.py @@ -0,0 +1,22 @@ +# Generated by Plain 0.154.0 on 2026-07-23 16:24 + +from plain import postgres +from plain.postgres import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ("examples", "0019_returningevent"), + ] + + operations = [ + migrations.CreateModel( + name="UpsertItem", + fields=[ + ("id", postgres.PrimaryKeyField()), + ("key", postgres.TextField(max_length=100)), + ("label", postgres.TextField(default="")), + ("value", postgres.IntegerField(default=0)), + ], + ), + ] diff --git a/plain-postgres/tests/app/examples/models/__init__.py b/plain-postgres/tests/app/examples/models/__init__.py index eff21dad43..73dc91b489 100644 --- a/plain-postgres/tests/app/examples/models/__init__.py +++ b/plain-postgres/tests/app/examples/models/__init__.py @@ -17,4 +17,5 @@ storage_parameters, trees, unregistered, + upsert, ) diff --git a/plain-postgres/tests/app/examples/models/upsert.py b/plain-postgres/tests/app/examples/models/upsert.py new file mode 100644 index 0000000000..e33b74526d --- /dev/null +++ b/plain-postgres/tests/app/examples/models/upsert.py @@ -0,0 +1,21 @@ +"""Test fixtures for QuerySet.bulk_upsert().""" + +from __future__ import annotations + +from plain import postgres +from plain.postgres import types + + +@postgres.register_model +class UpsertItem(postgres.Model): + key = types.TextField(max_length=100) + value = types.IntegerField(default=0) + label = types.TextField(default="", required=False) + + query: postgres.QuerySet[UpsertItem] = postgres.QuerySet() + + model_options = postgres.Options( + constraints=[ + postgres.UniqueConstraint(fields=["key"], name="upsertitem_key_unique"), + ] + ) diff --git a/plain-postgres/tests/public/test_bulk_upsert.py b/plain-postgres/tests/public/test_bulk_upsert.py new file mode 100644 index 0000000000..eeff0303c4 --- /dev/null +++ b/plain-postgres/tests/public/test_bulk_upsert.py @@ -0,0 +1,149 @@ +"""QuerySet.bulk_upsert() inserts new rows and updates conflicting ones. + +One INSERT ... ON CONFLICT (unique_fields) DO UPDATE ... RETURNING per batch. +Every returned object -- inserted or updated -- comes back with its DB-returned +fields (primary key, DB defaults) populated, matched to its row by unique key. +""" + +from __future__ import annotations + +import pytest +from app.examples.models.upsert import UpsertItem + + +def test_bulk_upsert_inserts_new_rows_and_sets_pks(db): + items = [ + UpsertItem(key="a", value=1), + UpsertItem(key="b", value=2), + ] + returned = UpsertItem.query.bulk_upsert( + items, update_fields=["value"], unique_fields=["key"] + ) + + assert [r.id for r in returned] == [item.id for item in items] + assert all(item.id is not None for item in items) + stored = {row.key: row.value for row in UpsertItem.query.all()} + assert stored == {"a": 1, "b": 2} + + +def test_bulk_upsert_mixed_batch_inserts_and_updates(db): + UpsertItem(key="a", value=1).create() + existing_id = UpsertItem.query.get(key="a").id + + items = [ + UpsertItem(key="a", value=10), # conflicts -> update + UpsertItem(key="b", value=20), # new -> insert + ] + UpsertItem.query.bulk_upsert(items, update_fields=["value"], unique_fields=["key"]) + + by_key = {item.key: item for item in items} + # The updated row keeps its original primary key. + assert by_key["a"].id == existing_id + assert by_key["b"].id is not None + assert by_key["b"].id != existing_id + + stored = {row.key: row.value for row in UpsertItem.query.all()} + assert stored == {"a": 10, "b": 20} + + +def test_bulk_upsert_updates_only_named_fields(db): + UpsertItem(key="a", value=1, label="original").create() + + UpsertItem.query.bulk_upsert( + [UpsertItem(key="a", value=99, label="ignored")], + update_fields=["value"], + unique_fields=["key"], + ) + + row = UpsertItem.query.get(key="a") + assert row.value == 99 # named field updated + assert row.label == "original" # field not in update_fields preserved + + +def test_bulk_upsert_matches_returned_rows_by_key_not_order(db): + # Seed so every input row conflicts; RETURNING order under ON CONFLICT is + # not guaranteed to match VALUES order, so each object must be matched to + # its own row by unique key. + for key in ("a", "b", "c"): + UpsertItem(key=key, value=0).create() + seeded_ids = {row.key: row.id for row in UpsertItem.query.all()} + + items = [ + UpsertItem(key="c", value=3), + UpsertItem(key="a", value=1), + UpsertItem(key="b", value=2), + ] + UpsertItem.query.bulk_upsert(items, update_fields=["value"], unique_fields=["key"]) + + for item in items: + assert item.id == seeded_ids[item.key] + + stored = {row.key: row.value for row in UpsertItem.query.all()} + assert stored == {"a": 1, "b": 2, "c": 3} + + +def test_bulk_upsert_empty_returns_empty(db): + assert ( + UpsertItem.query.bulk_upsert([], update_fields=["value"], unique_fields=["key"]) + == [] + ) + + +def test_bulk_upsert_batches(db): + items = [UpsertItem(key=f"k{i}", value=i) for i in range(5)] + UpsertItem.query.bulk_upsert( + items, update_fields=["value"], unique_fields=["key"], batch_size=2 + ) + + assert all(item.id is not None for item in items) + assert UpsertItem.query.count() == 5 + + +def test_bulk_upsert_unique_fields_must_match_a_constraint(db): + with pytest.raises(ValueError, match="must name the primary key"): + UpsertItem.query.bulk_upsert( + [UpsertItem(key="a", value=1)], + update_fields=["value"], + unique_fields=["value"], # no unique constraint on value + ) + + +def test_bulk_upsert_null_unique_value_rejected(db): + with pytest.raises(ValueError, match="non-null key"): + UpsertItem.query.bulk_upsert( + [UpsertItem(key=None, value=1)], + update_fields=["value"], + unique_fields=["key"], + ) + + +def test_bulk_upsert_update_fields_cannot_overlap_unique_fields(db): + with pytest.raises(ValueError, match="cannot overlap unique_fields"): + UpsertItem.query.bulk_upsert( + [UpsertItem(key="a", value=1)], + update_fields=["key"], + unique_fields=["key"], + ) + + +def test_bulk_upsert_requires_update_fields(db): + with pytest.raises(ValueError, match="requires update_fields"): + UpsertItem.query.bulk_upsert( + [UpsertItem(key="a", value=1)], + update_fields=[], + unique_fields=["key"], + ) + + +def test_bulk_create_no_longer_accepts_update_conflicts(db): + # bulk_create is insert-only now; the conflict surface moved to bulk_upsert. + removed_conflict_kwargs: dict[str, object] = { + "update_conflicts": True, + "update_fields": ["value"], + "unique_fields": ["key"], + } + with pytest.raises(TypeError): + UpsertItem.query.bulk_create( + [UpsertItem(key="a", value=1)], + **removed_conflict_kwargs, # ty: ignore[invalid-argument-type] + ) From 081eda926d09de035b16a92eb833c88f6f9f577c Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Thu, 23 Jul 2026 11:33:03 -0500 Subject: [PATCH 05/41] Document bulk_upsert() in plain-postgres docs and rules --- .claude/rules/plain-postgres.md | 4 +-- plain-postgres/plain/postgres/README.md | 32 ++++++++++++++++++- .../agents/.claude/rules/plain-postgres.md | 4 +-- 3 files changed, 35 insertions(+), 5 deletions(-) diff --git a/.claude/rules/plain-postgres.md b/.claude/rules/plain-postgres.md index 98ce6e5ed1..ff79127867 100644 --- a/.claude/rules/plain-postgres.md +++ b/.claude/rules/plain-postgres.md @@ -63,11 +63,11 @@ Use `Model.query` to build querysets (e.g., `User.query.filter(is_active=True)`) - Use `.annotate(Count(...))` instead of calling `.count()` per row - Fetch all data in the view — templates should never trigger queries - Use `.exists()` not `.count() > 0`, `.count()` not `len(qs)` -- Use `bulk_create`/`bulk_update` for batch ops, `.update()`/`.delete()` for mass ops +- Use `bulk_create`/`bulk_update` for batch ops, `bulk_upsert` for atomic insert-or-update, `.update()`/`.delete()` for mass ops - Use `.values_list()` when you only need specific columns - Wrap multi-step writes in `transaction.atomic()` - Instance writes are `obj.create()` (always INSERT) and `obj.update()` (always UPDATE; `update(fields=[...])` limits the columns) — there is no `save()`, `force_insert`, or `force_update`. Constructing an instance then `create()`-ing it inserts; a hand-set `id` that collides raises `IntegrityError`. -- `create()`/`update()` raise `ValidationError` (not raw `psycopg.IntegrityError`) on a declared unique/check constraint violation, even a raced one — the DB enforces it, so inside an open `transaction.atomic()` the violation aborts the transaction (wrap the write in its own `atomic()` to catch and keep using the transaction). Set-based writes (`QuerySet.update()`/`bulk_create()`) raise raw `psycopg.IntegrityError`. Retrying on conflict? `except (psycopg.IntegrityError, ValidationError)`, or `bulk_create(..., update_conflicts=True)` +- `create()`/`update()` raise `ValidationError` (not raw `psycopg.IntegrityError`) on a declared unique/check constraint violation, even a raced one — the DB enforces it, so inside an open `transaction.atomic()` the violation aborts the transaction (wrap the write in its own `atomic()` to catch and keep using the transaction). Set-based writes (`QuerySet.update()`/`bulk_create()`) raise raw `psycopg.IntegrityError`. Retrying on conflict? `except (psycopg.IntegrityError, ValidationError)`, or `bulk_upsert(objs, update_fields=[...], unique_fields=[...])` for an atomic insert-or-update - Always paginate list queries — unbounded querysets get slower as data grows Run `uv run plain docs postgres` for full patterns with code examples. diff --git a/plain-postgres/plain/postgres/README.md b/plain-postgres/plain/postgres/README.md index 3baee8cc69..50dd20fdaf 100644 --- a/plain-postgres/plain/postgres/README.md +++ b/plain-postgres/plain/postgres/README.md @@ -388,6 +388,36 @@ for name in names: Tag.query.bulk_create([Tag(name=name) for name in names]) ``` +`bulk_create` is insert-only. To insert new rows and update the ones that +already exist in a single statement, use `bulk_upsert` (below). + +#### Use `bulk_upsert` to insert-or-update in one statement + +`bulk_upsert(objs, *, update_fields, unique_fields, batch_size=None)` issues one +`INSERT ... ON CONFLICT (unique_fields) DO UPDATE SET ... RETURNING` per batch. +Rows that don't exist yet are inserted; rows that collide on `unique_fields` have +their `update_fields` overwritten. Every object comes back — inserted or updated — +with its DB-generated fields (primary key, DB defaults) populated. + +```python +# Insert new items, refresh `value`/`expires_at` on any existing key. +CacheItem.query.bulk_upsert( + [CacheItem(key=k, value=v, expires_at=exp) for k, v in items], + update_fields=["value", "expires_at"], + unique_fields=["key"], +) +``` + +- `unique_fields` must name the **primary key** or a `UniqueConstraint` declared + on the model (no condition, no expressions) — this is the conflict target. +- `update_fields` must be concrete, non-primary-key, and must not overlap + `unique_fields`. +- Every object must have a non-null value for every unique field. `NULL` never + conflicts in Postgres, so it can't be upserted. +- Each batch is sorted by the conflict key and matches returned rows back to + objects by that key, so it's safe to run concurrently without deadlocking on + overlapping keys. + #### Use queryset `.update()` / `.delete()` for mass operations ```python @@ -1108,7 +1138,7 @@ except (psycopg.IntegrityError, ValidationError): ... # lost a race — reload and retry, or report it ``` -For a plain insert-or-update with no per-row logic, `bulk_create(..., update_conflicts=True, unique_fields=[...])` is an atomic upsert with no race to catch. +For a plain insert-or-update with no per-row logic, `bulk_upsert(objs, update_fields=[...], unique_fields=[...])` is an atomic upsert with no race to catch. Two caveats. The mapping covers **immediate** constraints — the default. An explicitly deferred constraint (`UniqueConstraint(deferrable=Deferrable.DEFERRED)`) is checked at commit, _after_ the write returns, so its violation still surfaces as a raw `psycopg.IntegrityError`. And when a row violates several constraints at once, a form's pre-check (or an explicit `validate_constraints()`) reports them all, while a direct `create()`/`update()` gets only the first one the database hits. diff --git a/plain-postgres/plain/postgres/agents/.claude/rules/plain-postgres.md b/plain-postgres/plain/postgres/agents/.claude/rules/plain-postgres.md index 98ce6e5ed1..ff79127867 100644 --- a/plain-postgres/plain/postgres/agents/.claude/rules/plain-postgres.md +++ b/plain-postgres/plain/postgres/agents/.claude/rules/plain-postgres.md @@ -63,11 +63,11 @@ Use `Model.query` to build querysets (e.g., `User.query.filter(is_active=True)`) - Use `.annotate(Count(...))` instead of calling `.count()` per row - Fetch all data in the view — templates should never trigger queries - Use `.exists()` not `.count() > 0`, `.count()` not `len(qs)` -- Use `bulk_create`/`bulk_update` for batch ops, `.update()`/`.delete()` for mass ops +- Use `bulk_create`/`bulk_update` for batch ops, `bulk_upsert` for atomic insert-or-update, `.update()`/`.delete()` for mass ops - Use `.values_list()` when you only need specific columns - Wrap multi-step writes in `transaction.atomic()` - Instance writes are `obj.create()` (always INSERT) and `obj.update()` (always UPDATE; `update(fields=[...])` limits the columns) — there is no `save()`, `force_insert`, or `force_update`. Constructing an instance then `create()`-ing it inserts; a hand-set `id` that collides raises `IntegrityError`. -- `create()`/`update()` raise `ValidationError` (not raw `psycopg.IntegrityError`) on a declared unique/check constraint violation, even a raced one — the DB enforces it, so inside an open `transaction.atomic()` the violation aborts the transaction (wrap the write in its own `atomic()` to catch and keep using the transaction). Set-based writes (`QuerySet.update()`/`bulk_create()`) raise raw `psycopg.IntegrityError`. Retrying on conflict? `except (psycopg.IntegrityError, ValidationError)`, or `bulk_create(..., update_conflicts=True)` +- `create()`/`update()` raise `ValidationError` (not raw `psycopg.IntegrityError`) on a declared unique/check constraint violation, even a raced one — the DB enforces it, so inside an open `transaction.atomic()` the violation aborts the transaction (wrap the write in its own `atomic()` to catch and keep using the transaction). Set-based writes (`QuerySet.update()`/`bulk_create()`) raise raw `psycopg.IntegrityError`. Retrying on conflict? `except (psycopg.IntegrityError, ValidationError)`, or `bulk_upsert(objs, update_fields=[...], unique_fields=[...])` for an atomic insert-or-update - Always paginate list queries — unbounded querysets get slower as data grows Run `uv run plain docs postgres` for full patterns with code examples. From 57d6092ae2abd8d581c2a8f0cf366e4b61eaff7b Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Thu, 23 Jul 2026 11:46:10 -0500 Subject: [PATCH 06/41] Simplify bulk_upsert internals - Fold _batched_upsert into _batched_insert via optional conflict kwargs - Compute each object's unique key once, reused for null-check, sort, and RETURNING row matching - Remove unreachable OnConflict.IGNORE / ON CONFLICT DO NOTHING branch - Drop redundant ManyToManyField check (non-concrete already covers it) - Use enumerate() for db_returning row indices - Move the unique-fields-match-constraint predicate onto Options --- plain-postgres/plain/postgres/constants.py | 1 - plain-postgres/plain/postgres/dialect.py | 2 - plain-postgres/plain/postgres/options.py | 11 +++ plain-postgres/plain/postgres/query.py | 107 ++++++++------------- 4 files changed, 51 insertions(+), 70 deletions(-) diff --git a/plain-postgres/plain/postgres/constants.py b/plain-postgres/plain/postgres/constants.py index cec1b9b90f..c1d7bd5356 100644 --- a/plain-postgres/plain/postgres/constants.py +++ b/plain-postgres/plain/postgres/constants.py @@ -9,5 +9,4 @@ class OnConflict(Enum): - IGNORE = "ignore" UPDATE = "update" diff --git a/plain-postgres/plain/postgres/dialect.py b/plain-postgres/plain/postgres/dialect.py index 112cac6424..b49c8f142a 100644 --- a/plain-postgres/plain/postgres/dialect.py +++ b/plain-postgres/plain/postgres/dialect.py @@ -584,8 +584,6 @@ def on_conflict_suffix_sql( update_fields: Iterable[str], unique_fields: Iterable[str], ) -> str: - if on_conflict == OnConflict.IGNORE: - return "ON CONFLICT DO NOTHING" if on_conflict == OnConflict.UPDATE: return "ON CONFLICT({}) DO UPDATE SET {}".format( ", ".join(map(quote_name, unique_fields)), diff --git a/plain-postgres/plain/postgres/options.py b/plain-postgres/plain/postgres/options.py index b94ac871a2..9d5736d78b 100644 --- a/plain-postgres/plain/postgres/options.py +++ b/plain-postgres/plain/postgres/options.py @@ -201,6 +201,17 @@ def total_unique_constraints(self) -> list[Any]: ) ] + def unique_fields_match_constraint(self, field_names: set[str | None]) -> bool: + """True if field_names names the primary key, or a UniqueConstraint on + the model that has no condition and no expressions.""" + pk_field = self.model._model_meta.get_forward_field("id") + if field_names == {pk_field.name}: + return True + for constraint in self.total_unique_constraints: + if set(constraint.fields) == field_names: + return True + return False + def __repr__(self) -> str: return f"" diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index 0d8b7813a5..fc287f45c0 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -687,31 +687,18 @@ def bulk_create( return objs - def _unique_fields_match_constraint(self, unique_fields: list[Field]) -> bool: - """True if unique_fields names the primary key, or a UniqueConstraint on - the model that has no condition and no expressions.""" - names = {f.name for f in unique_fields} - pk_field = self.model._model_meta.get_forward_field("id") - if names == {pk_field.name}: - return True - for constraint in self.model.model_options.total_unique_constraints: - if set(constraint.fields) == names: - return True - return False - def _check_bulk_upsert_options( self, - objs: list[T], update_fields: list[Field], unique_fields: list[Field], ) -> None: - from plain.postgres.fields.related import ManyToManyField - model_name = self.model.__name__ if not unique_fields: raise ValueError("bulk_upsert() requires unique_fields.") - if not self._unique_fields_match_constraint(unique_fields): + if not self.model.model_options.unique_fields_match_constraint( + {f.name for f in unique_fields} + ): names = [f.name for f in unique_fields] raise ValueError( f"bulk_upsert() unique_fields {names} on {model_name} must name " @@ -721,7 +708,7 @@ def _check_bulk_upsert_options( if not update_fields: raise ValueError("bulk_upsert() requires update_fields.") - if any(not f.concrete or isinstance(f, ManyToManyField) for f in update_fields): + if any(not f.concrete for f in update_fields): raise ValueError("bulk_upsert() update_fields must be concrete fields.") if any(f.primary_key for f in update_fields): raise ValueError("bulk_upsert() cannot update primary key fields.") @@ -732,15 +719,6 @@ def _check_bulk_upsert_options( f"{sorted(overlap)}." ) - for field in unique_fields: - for obj in objs: - if field.value_from_object(obj) is None: - raise ValueError( - f"bulk_upsert() requires a non-null {field.name} on every " - "object; NULL never conflicts in Postgres, so it cannot " - "be upserted." - ) - def bulk_upsert( self, objs: Sequence[T], @@ -768,10 +746,27 @@ def bulk_upsert( meta = self.model._model_meta unique_fields_objs = [meta.get_forward_field(name) for name in unique_fields] update_fields_objs = [meta.get_forward_field(name) for name in update_fields] - self._check_bulk_upsert_options(objs, update_fields_objs, unique_fields_objs) + self._check_bulk_upsert_options(update_fields_objs, unique_fields_objs) self._prepare_for_bulk_create(objs) + # Compute each object's conflict key exactly once, rejecting nulls as we + # go (NULL never conflicts in Postgres, so it can't be upserted). Reused + # below to sort the batch and to match RETURNING rows back to objects. + keyed: list[tuple[tuple[Any, ...], T]] = [] + for obj in objs: + key = [] + for field in unique_fields_objs: + value = field.value_from_object(obj) + if value is None: + raise ValueError( + f"bulk_upsert() requires a non-null {field.name} on every " + "object; NULL never conflicts in Postgres, so it cannot " + "be upserted." + ) + key.append(value) + keyed.append((tuple(key), obj)) + # Include the PK column only when it is itself the conflict key; # otherwise let Postgres generate the identity value. pk_is_unique = any(f.primary_key for f in unique_fields_objs) @@ -786,23 +781,19 @@ def bulk_upsert( if field not in returning_fields: returning_fields.append(field) unique_indices = [returning_fields.index(f) for f in unique_fields_objs] - db_returning_indices = [ - (returning_fields.index(f), f) for f in meta.db_returning_fields - ] + db_returning_indices = list(enumerate(meta.db_returning_fields)) # Sort by the conflict key so concurrent upserts touching overlapping # keys lock rows in the same order and can't deadlock each other. - ordered = sorted( - objs, - key=lambda o: tuple(f.value_from_object(o) for f in unique_fields_objs), - ) + keyed.sort(key=lambda pair: pair[0]) with transaction.atomic(savepoint=False): - returned_rows = self._batched_upsert( - ordered, + returned_rows = self._batched_insert( + [obj for _, obj in keyed], fields, - returning_fields, batch_size, + returning_fields=returning_fields, + on_conflict=OnConflict.UPDATE, update_fields=update_fields_objs, unique_fields=unique_fields_objs, ) @@ -813,8 +804,7 @@ def bulk_upsert( for row in returned_rows: key = tuple(row[i] for i in unique_indices) row_by_key[key] = row - for obj in objs: - key = tuple(f.value_from_object(obj) for f in unique_fields_objs) + for key, obj in keyed: row = row_by_key[key] for index, field in db_returning_indices: assert field.name is not None @@ -1530,36 +1520,19 @@ def _batched_insert( objs: list[T], fields: list[Field], batch_size: int | None, + *, + returning_fields: list[Field] | None = None, + on_conflict: OnConflict | None = None, + update_fields: list[Field] | None = None, + unique_fields: list[Field] | None = None, ) -> list[tuple[Any, ...]]: """ - Helper method for bulk_create() to insert objs one batch at a time. - """ - max_batch_size = max(len(objs), 1) - batch_size = min(batch_size, max_batch_size) if batch_size else max_batch_size - inserted_rows = [] - for item in [objs[i : i + batch_size] for i in range(0, len(objs), batch_size)]: - inserted_rows.extend( - self._insert( # ty: ignore[invalid-argument-type] - item, - fields=fields, - returning_fields=self.model._model_meta.db_returning_fields, - ) - ) - return inserted_rows - - def _batched_upsert( - self, - objs: list[T], - fields: list[Field], - returning_fields: list[Field], - batch_size: int | None, - update_fields: list[Field], - unique_fields: list[Field], - ) -> list[tuple[Any, ...]]: - """ - Helper method for bulk_upsert() to insert objs one batch at a time as - ON CONFLICT DO UPDATE, collecting the RETURNING rows from every batch. + Helper method for bulk_create()/bulk_upsert() to insert objs one batch + at a time, collecting the RETURNING rows from every batch. Pass the + on_conflict kwargs to run each batch as ON CONFLICT DO UPDATE. """ + if returning_fields is None: + returning_fields = self.model._model_meta.db_returning_fields max_batch_size = max(len(objs), 1) batch_size = min(batch_size, max_batch_size) if batch_size else max_batch_size returned_rows = [] @@ -1569,7 +1542,7 @@ def _batched_upsert( item, fields=fields, returning_fields=returning_fields, - on_conflict=OnConflict.UPDATE, + on_conflict=on_conflict, update_fields=update_fields, unique_fields=unique_fields, ) From 395f219b1b4a66851cf9e125c7f32036b47df844 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Thu, 23 Jul 2026 15:40:56 -0500 Subject: [PATCH 07/41] returning() takes field references, not strings QuerySet.returning(*fields) now accepts Field references (Model.field) instead of column-name strings. The with-fields overload is validated at the returning() call: each ref must be a concrete Field belonging to the queryset's model, and a string raises a clear TypeError pointing at Model.field. Return shape is unchanged (no-arg hydrates instances, with-fields returns dicts keyed by field name). --- plain-postgres/plain/postgres/README.md | 6 +- plain-postgres/plain/postgres/query.py | 62 +++++++++++-------- plain-postgres/tests/public/test_returning.py | 43 ++++++++++--- 3 files changed, 72 insertions(+), 39 deletions(-) diff --git a/plain-postgres/plain/postgres/README.md b/plain-postgres/plain/postgres/README.md index 3baee8cc69..bc0dc67612 100644 --- a/plain-postgres/plain/postgres/README.md +++ b/plain-postgres/plain/postgres/README.md @@ -438,14 +438,14 @@ running = Job.query.filter(status="pending").returning().update(status="running" for job in running: print(job.id, job.status) # reflects the post-update values -# Field names: rows come back as dicts of just those columns. -deleted = Event.query.filter(created_at__lt=cutoff).returning("id", "payload").delete() +# Field references: rows come back as dicts of just those columns. +deleted = Event.query.filter(created_at__lt=cutoff).returning(Event.id, Event.payload).delete() for row in deleted: print(row["id"], row["payload"]) # the rows as they were deleted ``` - **`returning()`** returns full model instances. For `update()` they hold the new values; for `delete()`, the rows as they were. -- **`returning("field", ...)`** returns a list of dicts with only those columns. Passing an unknown or non-concrete field name raises `FieldError` at the `returning()` call. +- **`returning(Model.field, ...)`** returns a list of dicts with only those columns. Pass field references (`Model.field`), not strings; a non-concrete field or one from another model raises an error at the `returning()` call. - Without `returning()`, `update()`/`delete()` return an `int` as before. `RETURNING` only reports rows of the statement's own target table. Rows removed by a cascading `ON DELETE` are never included — a `delete()` with `returning()` gives you the parent rows you deleted, not the children Postgres cascaded. diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index 6e6c3ccfba..aebaedf5bf 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -288,10 +288,10 @@ class Task(Model): _deferred_filter: tuple[bool, tuple[Any, ...], dict[str, Any]] | None # None => plain update()/delete() returning an int rowcount. # () => RETURNING every concrete column, hydrated into model instances. - # (name, ...) => RETURNING those columns, returned as dicts. - _returning: tuple[str, ...] | None - # The concrete fields resolved from _returning, or None for a plain write. - # Set once by returning() so update()/delete() don't re-resolve at execute. + # (field, ...) => RETURNING those columns, returned as dicts. + _returning: tuple[Field, ...] | None + # The concrete fields to RETURN, or None for a plain write. Set once by + # returning() so update()/delete() don't recompute them at execute. _returning_fields: list[Field] | None def __init__(self): @@ -937,17 +937,20 @@ def last(self) -> T | None: def returning(self) -> ReturningQuerySet[T, list[T]]: ... @overload - def returning(self, *fields: str) -> ReturningQuerySet[T, list[dict[str, Any]]]: ... + def returning( + self, *fields: Field[Any] + ) -> ReturningQuerySet[T, list[dict[str, Any]]]: ... - def returning(self, *fields: str) -> ReturningQuerySet[T, Any]: + def returning(self, *fields: Field[Any]) -> ReturningQuerySet[T, Any]: """Capture the rows touched by the next update() or delete(). With no arguments, update()/delete() return the affected rows as model instances (RETURNING every concrete column). Given field - names, they return a list of dicts holding just those columns. - Without returning(), update()/delete() return an int rowcount. + references (`Model.field`), they return a list of dicts holding just + those columns. Without returning(), update()/delete() return an int + rowcount. - The field names are validated here, so a bad name errors at the + The references are validated here, so a bad one errors at the returning() call rather than when the write runs. """ clone = self._chain() @@ -957,28 +960,35 @@ def returning(self, *fields: str) -> ReturningQuerySet[T, Any]: return cast("ReturningQuerySet[T, Any]", clone) def _resolve_returning_fields(self) -> list[Field]: - """Translate self._returning into the concrete fields to RETURN.""" - meta = self.model._model_meta + """Validate self._returning and produce the concrete fields to RETURN.""" + object_name = self.model.model_options.object_name if not self._returning: - # No names given: RETURN every concrete column so the rows can be - # hydrated into full model instances. - return list(meta.concrete_fields) - fields = [] - for name in self._returning: - try: - field = meta.get_field(name) - except FieldDoesNotExist: + # No references given: RETURN every concrete column so the rows can + # be hydrated into full model instances. + return list(self.model._model_meta.concrete_fields) + for field in self._returning: + if isinstance(field, str): + raise TypeError( + f"returning() takes field references, not strings. " + f"Pass {object_name}.{field} instead of {field!r}." + ) + if not isinstance(field, Field): + raise TypeError( + f"returning() takes field references like " + f"{object_name}., not {field!r}." + ) + if field.model is not self.model: raise FieldError( - f"Cannot resolve '{name}' in returning() for " - f"{self.model.model_options.object_name}: no such field." + f"Cannot use {field.model.model_options.object_name}." + f"{field.name} in returning() for {object_name}: it " + "belongs to a different model." ) - if not isinstance(field, Field) or not field.concrete: + if not field.concrete: raise FieldError( - f"Cannot use '{name}' in returning(): only concrete " - "database columns can be returned." + f"Cannot use {object_name}.{field.name} in returning(): " + "only concrete database columns can be returned." ) - fields.append(field) - return fields + return list(self._returning) def _hydrate_returning(self, fields: list[Field], rows: list[list]) -> list[Any]: """Turn converted RETURNING rows into instances (no names) or dicts.""" diff --git a/plain-postgres/tests/public/test_returning.py b/plain-postgres/tests/public/test_returning.py index f3306b15ca..d8f77fddf7 100644 --- a/plain-postgres/tests/public/test_returning.py +++ b/plain-postgres/tests/public/test_returning.py @@ -1,8 +1,8 @@ """QuerySet.returning() captures the rows touched by update() and delete(). Without returning(), update()/delete() return an int rowcount as always. -With it, no-arg returning() hydrates full model instances and returning(*names) -returns a list of dicts holding just those columns. +With it, no-arg returning() hydrates full model instances and +returning(*Model.field) returns a list of dicts holding just those columns. """ from __future__ import annotations @@ -48,7 +48,9 @@ def test_update_returning_instances_reflect_new_values(db): def test_update_returning_named_fields_are_dicts(db): _seed_events() rows = ( - ReturningEvent.query.filter(label="a").returning("id", "count").update(count=7) + ReturningEvent.query.filter(label="a") + .returning(ReturningEvent.id, ReturningEvent.count) + .update(count=7) ) assert len(rows) == 2 @@ -76,7 +78,13 @@ def test_delete_without_returning_returns_int(db): def test_delete_returning_named_fields_gives_deleted_rows(db): _seed_events() - rows = ReturningEvent.query.filter(label="a").returning("id", "payload").delete() + rows = ( + ReturningEvent.query.filter(label="a") + # payload is an explicitly annotated JSONField, so it types as dict|None + # at class access rather than Field; it is a Field at runtime. + .returning(ReturningEvent.id, ReturningEvent.payload) # ty: ignore[invalid-argument-type] + .delete() + ) assert len(rows) == 2 assert all(isinstance(row, dict) and set(row) == {"id", "payload"} for row in rows) @@ -98,7 +106,11 @@ def test_delete_returning_instances(db): def test_delete_returning_empty_result_is_empty_list(db): - rows = ReturningEvent.query.filter(label="missing").returning("id").delete() + rows = ( + ReturningEvent.query.filter(label="missing") + .returning(ReturningEvent.id) + .delete() + ) assert rows == [] @@ -109,12 +121,19 @@ def test_delete_returning_empty_result_is_empty_list(db): def test_returning_returns_a_returning_queryset(db): assert isinstance(ReturningEvent.query.returning(), ReturningQuerySet) - assert isinstance(ReturningEvent.query.returning("id"), ReturningQuerySet) + assert isinstance( + ReturningEvent.query.returning(ReturningEvent.id), ReturningQuerySet + ) -def test_returning_bad_field_name_errors(db): - with pytest.raises(FieldError, match="no such field"): - ReturningEvent.query.returning("not_a_field") +def test_returning_string_arg_errors(db): + with pytest.raises(TypeError, match="takes field references, not strings"): + ReturningEvent.query.returning("count") # ty: ignore[invalid-argument-type] + + +def test_returning_wrong_model_field_errors(db): + with pytest.raises(FieldError, match="belongs to a different model"): + ReturningEvent.query.returning(DeleteParent.name) def test_returning_before_filter_is_preserved(db): @@ -140,7 +159,11 @@ def test_delete_returning_excludes_cascade_deleted_children(db): ChildCascade(parent=parent).create() ChildCascade(parent=parent).create() - rows = DeleteParent.query.filter(id=parent.id).returning("id", "name").delete() + rows = ( + DeleteParent.query.filter(id=parent.id) + .returning(DeleteParent.id, DeleteParent.name) + .delete() + ) # Only the parent row comes back, even though two children were cascaded. assert len(rows) == 1 From de4ddb7eabf72b6c9c5ac8642720488f9fad3abb Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Thu, 23 Jul 2026 15:55:10 -0500 Subject: [PATCH 08/41] bulk_upsert() takes field references, not strings update_fields and unique_fields now accept Field references (Model.field) instead of column-name strings. A shared _validate_field_refs() helper enforces that each ref is a Field belonging to the queryset's model (also now backing returning()'s validation), so a string or a wrong-model ref raises a clear error pointing at Model.field. unique_fields_match_constraint still works on the derived name set. Migrates the plain-cache set_many() caller to CachedItem field references. --- plain-cache/plain/cache/core.py | 7 +- plain-postgres/plain/postgres/README.md | 6 +- plain-postgres/plain/postgres/query.py | 64 +++++++++++-------- .../tests/public/test_bulk_upsert.py | 58 +++++++++++++---- 4 files changed, 90 insertions(+), 45 deletions(-) diff --git a/plain-cache/plain/cache/core.py b/plain-cache/plain/cache/core.py index 27c479e948..156c4e3f9e 100644 --- a/plain-cache/plain/cache/core.py +++ b/plain-cache/plain/cache/core.py @@ -118,10 +118,11 @@ def set_many( self._model(key=key, value=value, expires_at=expires_at, created_at=now) for key, value in mapping.items() ] - self._model.query.bulk_upsert( + model = self._model + model.query.bulk_upsert( items, - update_fields=["value", "expires_at", "updated_at"], - unique_fields=["key"], + update_fields=[model.value, model.expires_at, model.updated_at], + unique_fields=[model.key], ) def get_or_set( diff --git a/plain-postgres/plain/postgres/README.md b/plain-postgres/plain/postgres/README.md index 7ddb50e028..42ee74c6a9 100644 --- a/plain-postgres/plain/postgres/README.md +++ b/plain-postgres/plain/postgres/README.md @@ -403,11 +403,13 @@ with its DB-generated fields (primary key, DB defaults) populated. # Insert new items, refresh `value`/`expires_at` on any existing key. CacheItem.query.bulk_upsert( [CacheItem(key=k, value=v, expires_at=exp) for k, v in items], - update_fields=["value", "expires_at"], - unique_fields=["key"], + update_fields=[CacheItem.value, CacheItem.expires_at], + unique_fields=[CacheItem.key], ) ``` +- `update_fields` and `unique_fields` take field references (`Model.field`), not + strings. - `unique_fields` must name the **primary key** or a `UniqueConstraint` declared on the model (no condition, no expressions) — this is the conflict target. - `update_fields` must be concrete, non-primary-key, and must not overlap diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index 6a1e9bd1b1..8d26c5bc53 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -723,8 +723,8 @@ def bulk_upsert( self, objs: Sequence[T], *, - update_fields: list[str], - unique_fields: list[str], + update_fields: list[Field], + unique_fields: list[Field], batch_size: int | None = None, ) -> list[T]: """ @@ -733,8 +733,9 @@ def bulk_upsert( INSERT ... ON CONFLICT (unique_fields) DO UPDATE ... RETURNING per batch. Both inserted and updated objects come back with their DB-returned - fields (primary key, DB defaults) populated. unique_fields must name the - primary key or a UniqueConstraint declared on the model. + fields (primary key, DB defaults) populated. update_fields and + unique_fields take field references (`Model.field`); unique_fields must + name the primary key or a UniqueConstraint declared on the model. """ if batch_size is not None and batch_size <= 0: raise ValueError("Batch size must be a positive integer.") @@ -743,10 +744,11 @@ def bulk_upsert( if not objs: return objs + self._validate_field_refs(unique_fields, where="bulk_upsert() unique_fields") + self._validate_field_refs(update_fields, where="bulk_upsert() update_fields") + meta = self.model._model_meta - unique_fields_objs = [meta.get_forward_field(name) for name in unique_fields] - update_fields_objs = [meta.get_forward_field(name) for name in update_fields] - self._check_bulk_upsert_options(update_fields_objs, unique_fields_objs) + self._check_bulk_upsert_options(update_fields, unique_fields) self._prepare_for_bulk_create(objs) @@ -756,7 +758,7 @@ def bulk_upsert( keyed: list[tuple[tuple[Any, ...], T]] = [] for obj in objs: key = [] - for field in unique_fields_objs: + for field in unique_fields: value = field.value_from_object(obj) if value is None: raise ValueError( @@ -769,7 +771,7 @@ def bulk_upsert( # Include the PK column only when it is itself the conflict key; # otherwise let Postgres generate the identity value. - pk_is_unique = any(f.primary_key for f in unique_fields_objs) + pk_is_unique = any(f.primary_key for f in unique_fields) fields = meta.concrete_fields if not pk_is_unique: fields = [f for f in fields if not isinstance(f, PrimaryKeyField)] @@ -777,10 +779,10 @@ def bulk_upsert( # RETURNING must carry the DB-returned fields (to populate the objects) # plus the unique fields (to match each returned row to its object). returning_fields = list(meta.db_returning_fields) - for field in unique_fields_objs: + for field in unique_fields: if field not in returning_fields: returning_fields.append(field) - unique_indices = [returning_fields.index(f) for f in unique_fields_objs] + unique_indices = [returning_fields.index(f) for f in unique_fields] db_returning_indices = list(enumerate(meta.db_returning_fields)) # Sort by the conflict key so concurrent upserts touching overlapping @@ -794,8 +796,8 @@ def bulk_upsert( batch_size, returning_fields=returning_fields, on_conflict=OnConflict.UPDATE, - update_fields=update_fields_objs, - unique_fields=unique_fields_objs, + update_fields=update_fields, + unique_fields=unique_fields, ) # RETURNING order isn't guaranteed to match VALUES order under ON @@ -1019,30 +1021,40 @@ def returning(self, *fields: Field[Any]) -> ReturningQuerySet[T, Any]: clone._returning_fields = clone._resolve_returning_fields() return cast("ReturningQuerySet[T, Any]", clone) - def _resolve_returning_fields(self) -> list[Field]: - """Validate self._returning and produce the concrete fields to RETURN.""" + def _validate_field_refs(self, fields: Sequence[Any], *, where: str) -> None: + """Require each item to be a Field reference on this queryset's model. + + `where` names the call in the error (e.g. "returning()", "bulk_upsert() + unique_fields") so a bad argument points the user at Model.field. + """ object_name = self.model.model_options.object_name - if not self._returning: - # No references given: RETURN every concrete column so the rows can - # be hydrated into full model instances. - return list(self.model._model_meta.concrete_fields) - for field in self._returning: + for field in fields: if isinstance(field, str): raise TypeError( - f"returning() takes field references, not strings. " + f"{where} takes field references, not strings. " f"Pass {object_name}.{field} instead of {field!r}." ) if not isinstance(field, Field): raise TypeError( - f"returning() takes field references like " - f"{object_name}., not {field!r}." + f"{where} takes field references like {object_name}., " + f"not {field!r}." ) if field.model is not self.model: raise FieldError( - f"Cannot use {field.model.model_options.object_name}." - f"{field.name} in returning() for {object_name}: it " - "belongs to a different model." + f"{where} cannot use {field.model.model_options.object_name}." + f"{field.name}: it belongs to a different model, not " + f"{object_name}." ) + + def _resolve_returning_fields(self) -> list[Field]: + """Validate self._returning and produce the concrete fields to RETURN.""" + if not self._returning: + # No references given: RETURN every concrete column so the rows can + # be hydrated into full model instances. + return list(self.model._model_meta.concrete_fields) + self._validate_field_refs(self._returning, where="returning()") + object_name = self.model.model_options.object_name + for field in self._returning: if not field.concrete: raise FieldError( f"Cannot use {object_name}.{field.name} in returning(): " diff --git a/plain-postgres/tests/public/test_bulk_upsert.py b/plain-postgres/tests/public/test_bulk_upsert.py index eeff0303c4..e3d8429fad 100644 --- a/plain-postgres/tests/public/test_bulk_upsert.py +++ b/plain-postgres/tests/public/test_bulk_upsert.py @@ -8,8 +8,11 @@ from __future__ import annotations import pytest +from app.examples.models.returning import ReturningEvent from app.examples.models.upsert import UpsertItem +from plain.postgres.exceptions import FieldError + def test_bulk_upsert_inserts_new_rows_and_sets_pks(db): items = [ @@ -17,7 +20,7 @@ def test_bulk_upsert_inserts_new_rows_and_sets_pks(db): UpsertItem(key="b", value=2), ] returned = UpsertItem.query.bulk_upsert( - items, update_fields=["value"], unique_fields=["key"] + items, update_fields=[UpsertItem.value], unique_fields=[UpsertItem.key] ) assert [r.id for r in returned] == [item.id for item in items] @@ -34,7 +37,9 @@ def test_bulk_upsert_mixed_batch_inserts_and_updates(db): UpsertItem(key="a", value=10), # conflicts -> update UpsertItem(key="b", value=20), # new -> insert ] - UpsertItem.query.bulk_upsert(items, update_fields=["value"], unique_fields=["key"]) + UpsertItem.query.bulk_upsert( + items, update_fields=[UpsertItem.value], unique_fields=[UpsertItem.key] + ) by_key = {item.key: item for item in items} # The updated row keeps its original primary key. @@ -51,8 +56,8 @@ def test_bulk_upsert_updates_only_named_fields(db): UpsertItem.query.bulk_upsert( [UpsertItem(key="a", value=99, label="ignored")], - update_fields=["value"], - unique_fields=["key"], + update_fields=[UpsertItem.value], + unique_fields=[UpsertItem.key], ) row = UpsertItem.query.get(key="a") @@ -73,7 +78,9 @@ def test_bulk_upsert_matches_returned_rows_by_key_not_order(db): UpsertItem(key="a", value=1), UpsertItem(key="b", value=2), ] - UpsertItem.query.bulk_upsert(items, update_fields=["value"], unique_fields=["key"]) + UpsertItem.query.bulk_upsert( + items, update_fields=[UpsertItem.value], unique_fields=[UpsertItem.key] + ) for item in items: assert item.id == seeded_ids[item.key] @@ -84,7 +91,9 @@ def test_bulk_upsert_matches_returned_rows_by_key_not_order(db): def test_bulk_upsert_empty_returns_empty(db): assert ( - UpsertItem.query.bulk_upsert([], update_fields=["value"], unique_fields=["key"]) + UpsertItem.query.bulk_upsert( + [], update_fields=[UpsertItem.value], unique_fields=[UpsertItem.key] + ) == [] ) @@ -92,7 +101,10 @@ def test_bulk_upsert_empty_returns_empty(db): def test_bulk_upsert_batches(db): items = [UpsertItem(key=f"k{i}", value=i) for i in range(5)] UpsertItem.query.bulk_upsert( - items, update_fields=["value"], unique_fields=["key"], batch_size=2 + items, + update_fields=[UpsertItem.value], + unique_fields=[UpsertItem.key], + batch_size=2, ) assert all(item.id is not None for item in items) @@ -103,8 +115,8 @@ def test_bulk_upsert_unique_fields_must_match_a_constraint(db): with pytest.raises(ValueError, match="must name the primary key"): UpsertItem.query.bulk_upsert( [UpsertItem(key="a", value=1)], - update_fields=["value"], - unique_fields=["value"], # no unique constraint on value + update_fields=[UpsertItem.value], + unique_fields=[UpsertItem.value], # no unique constraint on value ) @@ -112,8 +124,8 @@ def test_bulk_upsert_null_unique_value_rejected(db): with pytest.raises(ValueError, match="non-null key"): UpsertItem.query.bulk_upsert( [UpsertItem(key=None, value=1)], - update_fields=["value"], - unique_fields=["key"], + update_fields=[UpsertItem.value], + unique_fields=[UpsertItem.key], ) @@ -121,8 +133,8 @@ def test_bulk_upsert_update_fields_cannot_overlap_unique_fields(db): with pytest.raises(ValueError, match="cannot overlap unique_fields"): UpsertItem.query.bulk_upsert( [UpsertItem(key="a", value=1)], - update_fields=["key"], - unique_fields=["key"], + update_fields=[UpsertItem.key], + unique_fields=[UpsertItem.key], ) @@ -131,7 +143,25 @@ def test_bulk_upsert_requires_update_fields(db): UpsertItem.query.bulk_upsert( [UpsertItem(key="a", value=1)], update_fields=[], - unique_fields=["key"], + unique_fields=[UpsertItem.key], + ) + + +def test_bulk_upsert_string_field_rejected(db): + with pytest.raises(TypeError, match="takes field references, not strings"): + UpsertItem.query.bulk_upsert( + [UpsertItem(key="a", value=1)], + update_fields=["value"], # ty: ignore[invalid-argument-type] + unique_fields=[UpsertItem.key], + ) + + +def test_bulk_upsert_wrong_model_field_rejected(db): + with pytest.raises(FieldError, match="belongs to a different model"): + UpsertItem.query.bulk_upsert( + [UpsertItem(key="a", value=1)], + update_fields=[UpsertItem.value], + unique_fields=[ReturningEvent.label], ) From 38bfee85e3d39b905409e7780b9b1400842e8632 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Fri, 18 Sep 2026 14:40:27 -0500 Subject: [PATCH 09/41] Follow master's field APIs in returning() _model_meta.concrete_fields is now _model_meta.fields, and Field.concrete is gone, so the "is this a column?" check is an isinstance(ColumnField) test. The examples migration adopts master's tuple-literal migration format, and the reset tests read the real examples history instead of pinning its leaf name and length. --- plain-postgres/plain/postgres/README.md | 8 +++- plain-postgres/plain/postgres/query.py | 19 ++++----- .../migrations/0019_returningevent.py | 11 ++--- .../tests/app/examples/models/returning.py | 3 +- .../tests/internal/test_migrations_reset.py | 40 +++++++++++-------- plain-postgres/tests/public/test_returning.py | 1 - 6 files changed, 46 insertions(+), 36 deletions(-) diff --git a/plain-postgres/plain/postgres/README.md b/plain-postgres/plain/postgres/README.md index 3a8bcf89d0..23be3f2bf2 100644 --- a/plain-postgres/plain/postgres/README.md +++ b/plain-postgres/plain/postgres/README.md @@ -449,13 +449,17 @@ for job in running: print(job.id, job.status) # reflects the post-update values # Field references: rows come back as dicts of just those columns. -deleted = Event.query.filter(created_at__lt=cutoff).returning(Event.id, Event.payload).delete() +deleted = ( + Event.query.filter(created_at__lt=cutoff) + .returning(Event.id, Event.payload) + .delete() +) for row in deleted: print(row["id"], row["payload"]) # the rows as they were deleted ``` - **`returning()`** returns full model instances. For `update()` they hold the new values; for `delete()`, the rows as they were. -- **`returning(Model.field, ...)`** returns a list of dicts with only those columns. Pass field references (`Model.field`), not strings; a non-concrete field or one from another model raises an error at the `returning()` call. +- **`returning(Model.field, ...)`** returns a list of dicts with only those columns. Pass field references (`Model.field`), not strings; a many-to-many field or one from another model raises an error at the `returning()` call. - Without `returning()`, `update()`/`delete()` return an `int` as before. `RETURNING` only reports rows of the statement's own target table. Rows removed by a cascading `ON DELETE` are never included — a `delete()` with `returning()` gives you the parent rows you deleted, not the children Postgres cascaded. diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index df3eb159ab..5a5c260f34 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -31,6 +31,7 @@ Field, PrimaryKeyField, ) +from plain.postgres.fields.base import ColumnField from plain.postgres.functions import Cast from plain.postgres.query_utils import Q from plain.postgres.sql import ( @@ -286,10 +287,10 @@ class Task(Model): _defer_next_filter: bool _deferred_filter: tuple[bool, tuple[Any, ...], dict[str, Any]] | None # None => plain update()/delete() returning an int rowcount. - # () => RETURNING every concrete column, hydrated into model instances. + # () => RETURNING every column, hydrated into model instances. # (field, ...) => RETURNING those columns, returned as dicts. _returning: tuple[Field, ...] | None - # The concrete fields to RETURN, or None for a plain write. Set once by + # The fields to RETURN, or None for a plain write. Set once by # returning() so update()/delete() don't recompute them at execute. _returning_fields: list[Field] | None @@ -935,7 +936,7 @@ def returning(self, *fields: Field[Any]) -> ReturningQuerySet[T, Any]: """Capture the rows touched by the next update() or delete(). With no arguments, update()/delete() return the affected rows as - model instances (RETURNING every concrete column). Given field + model instances (RETURNING every column). Given field references (`Model.field`), they return a list of dicts holding just those columns. Without returning(), update()/delete() return an int rowcount. @@ -950,12 +951,12 @@ def returning(self, *fields: Field[Any]) -> ReturningQuerySet[T, Any]: return cast("ReturningQuerySet[T, Any]", clone) def _resolve_returning_fields(self) -> list[Field]: - """Validate self._returning and produce the concrete fields to RETURN.""" + """Validate self._returning and produce the fields to RETURN.""" object_name = self.model.model_options.object_name if not self._returning: - # No references given: RETURN every concrete column so the rows can + # No references given: RETURN every column so the rows can # be hydrated into full model instances. - return list(self.model._model_meta.concrete_fields) + return list(self.model._model_meta.fields) for field in self._returning: if isinstance(field, str): raise TypeError( @@ -973,16 +974,16 @@ def _resolve_returning_fields(self) -> list[Field]: f"{field.name} in returning() for {object_name}: it " "belongs to a different model." ) - if not field.concrete: + if not isinstance(field, ColumnField): raise FieldError( f"Cannot use {object_name}.{field.name} in returning(): " - "only concrete database columns can be returned." + "only database columns can be returned." ) return list(self._returning) def _hydrate_returning(self, fields: list[Field], rows: list[list]) -> list[Any]: """Turn converted RETURNING rows into instances (no names) or dicts.""" - field_names = cast("list[str]", [field.name for field in fields]) + field_names = [field.name for field in fields] if not self._returning: return [self.model.from_db(field_names, row) for row in rows] return [dict(zip(field_names, row)) for row in rows] diff --git a/plain-postgres/tests/app/examples/migrations/0019_returningevent.py b/plain-postgres/tests/app/examples/migrations/0019_returningevent.py index c892fe015f..90a33d8fb3 100644 --- a/plain-postgres/tests/app/examples/migrations/0019_returningevent.py +++ b/plain-postgres/tests/app/examples/migrations/0019_returningevent.py @@ -1,15 +1,12 @@ -# Generated by Plain 0.154.0 on 2026-07-23 16:03 +from plain.postgres import migrations from plain import postgres -from plain.postgres import migrations class Migration(migrations.Migration): - dependencies = [ - ("examples", "0018_storageparametersexample"), - ] + dependencies = (("examples", "0018_storageparametersexample"),) - operations = [ + operations = ( migrations.CreateModel( name="ReturningEvent", fields=[ @@ -19,4 +16,4 @@ class Migration(migrations.Migration): ("payload", postgres.JSONField(allow_null=True, required=False)), ], ), - ] + ) diff --git a/plain-postgres/tests/app/examples/models/returning.py b/plain-postgres/tests/app/examples/models/returning.py index 29267012d0..133b972f5b 100644 --- a/plain-postgres/tests/app/examples/models/returning.py +++ b/plain-postgres/tests/app/examples/models/returning.py @@ -4,9 +4,10 @@ from typing import Any -from plain import postgres from plain.postgres import types +from plain import postgres + @postgres.register_model class ReturningEvent(postgres.Model): diff --git a/plain-postgres/tests/internal/test_migrations_reset.py b/plain-postgres/tests/internal/test_migrations_reset.py index 96082af838..6cf053a08e 100644 --- a/plain-postgres/tests/internal/test_migrations_reset.py +++ b/plain-postgres/tests/internal/test_migrations_reset.py @@ -3,7 +3,7 @@ `plan_reset` builds it, `validate_reset` proves the post-reset graph loads and reproduces the models; the CLI writes and deletes. These tests copy the real `examples` history into the temporary migrations root so a reset of it -is the reset of an eighteen-migration package with a circular FK inside. +is the reset of the whole package, circular FK inside. """ from __future__ import annotations @@ -26,7 +26,12 @@ from plain.postgres.migrations.writer import MigrationWriter REAL_EXAMPLES = Path(__file__).parent.parent / "app" / "examples" / "migrations" -LEAF = "0018_storageparametersexample" +# Read the real history rather than pinning names, so adding a migration to the +# examples app doesn't break every assertion below. +EXAMPLES_NAMES = sorted(path.stem for path in REAL_EXAMPLES.glob("0*.py")) +EXAMPLES_COUNT = len(EXAMPLES_NAMES) +LEAF = EXAMPLES_NAMES[-1] +NEXT_NUMBER = f"{EXAMPLES_COUNT + 1:04d}" def migration_source( @@ -69,7 +74,7 @@ def test_reset_of_the_examples_history(migrations_dir: Path) -> None: current = loader() plan = plan_reset(current, "examples") - assert plan.baseline.name == "0019_baseline" + assert plan.baseline.name == f"{NEXT_NUMBER}_baseline" assert plan.baseline.supersedes == LEAF assert plan.baseline.since == "" assert plan.baseline.initial is None @@ -116,7 +121,7 @@ def test_written_baseline_loads_clean_with_the_history_gone( path.unlink() after = loader() - assert after.baselines["examples"].name == "0019_baseline" + assert after.baselines["examples"].name == f"{NEXT_NUMBER}_baseline" assert detect_model_changes(after, {"examples"}) == {} @@ -342,22 +347,25 @@ def test_reset_command_writes_and_deletes_then_the_runtime_adopts( result = CliRunner().invoke(reset, ["examples", "--since", "2.0"]) assert result.exit_code == 0, result.output - assert "Supersedes 0018_storageparametersexample" in result.output + assert f"Supersedes {LEAF}" in result.output assert "Recover from any failure with: git checkout --" in result.output assert "&& rm " in result.output assert "Commit the new file and the deletions together" in result.output assert "`since` is empty" not in result.output - assert "must have applied `examples.0018_storageparametersexample`" in result.output - assert examples_files(migrations_dir) == ["0019_baseline.py"] - source = (migrations_dir / "examples" / "0019_baseline.py").read_text() - assert "supersedes = '0018_storageparametersexample'" in source + assert f"must have applied `examples.{LEAF}`" in result.output + assert examples_files(migrations_dir) == [f"{NEXT_NUMBER}_baseline.py"] + source = (migrations_dir / "examples" / f"{NEXT_NUMBER}_baseline.py").read_text() + assert f"supersedes = '{LEAF}'" in source assert "since = '2.0'" in source assert "initial = True" not in source # The test database recorded the sentinel: the runtime adopts. applied = CliRunner().invoke(apply, ["--no-input"]) assert applied.exit_code == 0, applied.output - assert "examples.0019_baseline (baseline: recorded, not run)" in applied.output + assert ( + f"examples.{NEXT_NUMBER}_baseline (baseline: recorded, not run)" + in applied.output + ) def test_generated_baseline_runs_on_a_cleared_database( @@ -407,14 +415,14 @@ def test_uncommitted_history_refuses(repo: Path, migrations_dir: Path) -> None: assert f"{LEAF}.py" in result.output git(repo, "checkout", "--", ".") - (migrations_dir / "examples" / "0019_new.py").write_text( + (migrations_dir / "examples" / f"{NEXT_NUMBER}_new.py").write_text( migration_source(dependencies=(("examples", LEAF),)) ) result = CliRunner().invoke(reset, ["examples"]) assert result.exit_code != 0 assert "Not tracked by git" in result.output - assert "0019_new.py" in result.output - assert examples_files(migrations_dir)[-1] == "0019_new.py" + assert f"{NEXT_NUMBER}_new.py" in result.output + assert examples_files(migrations_dir)[-1] == f"{NEXT_NUMBER}_new.py" def test_outside_a_repository_refuses(migrations_dir: Path) -> None: @@ -422,7 +430,7 @@ def test_outside_a_repository_refuses(migrations_dir: Path) -> None: assert result.exit_code != 0 assert "git could not read" in result.output assert "not a git repository" in result.output - assert len(examples_files(migrations_dir)) == 18 + assert len(examples_files(migrations_dir)) == EXAMPLES_COUNT def test_pending_model_changes_refuse(migrations_dir: Path) -> None: @@ -436,8 +444,8 @@ def test_pending_model_changes_refuse(migrations_dir: Path) -> None: assert result.exit_code != 0 assert "model changes its migrations don't hold" in result.output - assert "Create model StorageParametersExample" in result.output - assert len(examples_files(migrations_dir)) == 17 + assert "Create model " in result.output + assert len(examples_files(migrations_dir)) == EXAMPLES_COUNT - 1 def test_code_defined_in_a_deleted_migration_is_flagged(migrations_dir: Path) -> None: diff --git a/plain-postgres/tests/public/test_returning.py b/plain-postgres/tests/public/test_returning.py index d8f77fddf7..57536d5b14 100644 --- a/plain-postgres/tests/public/test_returning.py +++ b/plain-postgres/tests/public/test_returning.py @@ -10,7 +10,6 @@ import pytest from app.examples.models.delete import ChildCascade, DeleteParent from app.examples.models.returning import ReturningEvent - from plain.postgres.exceptions import FieldError from plain.postgres.query import ReturningQuerySet From 05984f1f2fd07414c646cead787c7529b85d7e13 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Fri, 18 Sep 2026 14:50:49 -0500 Subject: [PATCH 10/41] Follow master's field APIs in bulk_upsert() _model_meta.concrete_fields is now _model_meta.fields and Field.concrete is gone, so update_fields is checked with isinstance(ColumnField). The examples migration adopts master's tuple-literal migration format. --- plain-postgres/plain/postgres/query.py | 6 +++--- .../tests/app/examples/migrations/0020_upsertitem.py | 10 +++------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index d107fe22b9..61d3c38edf 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -705,8 +705,8 @@ def _check_bulk_upsert_options( if not update_fields: raise ValueError("bulk_upsert() requires update_fields.") - if any(not f.concrete for f in update_fields): - raise ValueError("bulk_upsert() update_fields must be concrete fields.") + if any(not isinstance(f, ColumnField) for f in update_fields): + raise ValueError("bulk_upsert() update_fields must be database columns.") if any(f.primary_key for f in update_fields): raise ValueError("bulk_upsert() cannot update primary key fields.") overlap = {f.name for f in update_fields} & {f.name for f in unique_fields} @@ -769,7 +769,7 @@ def bulk_upsert( # Include the PK column only when it is itself the conflict key; # otherwise let Postgres generate the identity value. pk_is_unique = any(f.primary_key for f in unique_fields) - fields = meta.concrete_fields + fields = meta.fields if not pk_is_unique: fields = [f for f in fields if not isinstance(f, PrimaryKeyField)] diff --git a/plain-postgres/tests/app/examples/migrations/0020_upsertitem.py b/plain-postgres/tests/app/examples/migrations/0020_upsertitem.py index 26f1d9ae6f..67399beea9 100644 --- a/plain-postgres/tests/app/examples/migrations/0020_upsertitem.py +++ b/plain-postgres/tests/app/examples/migrations/0020_upsertitem.py @@ -1,16 +1,12 @@ -# Generated by Plain 0.154.0 on 2026-07-23 16:24 - from plain.postgres import migrations from plain import postgres class Migration(migrations.Migration): - dependencies = [ - ("examples", "0019_returningevent"), - ] + dependencies = (("examples", "0019_returningevent"),) - operations = [ + operations = ( migrations.CreateModel( name="UpsertItem", fields=[ @@ -20,4 +16,4 @@ class Migration(migrations.Migration): ("value", postgres.IntegerField(default=0)), ], ), - ] + ) From 7de6a0ea383172139a152cd8522911dbe9245670 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Fri, 18 Sep 2026 15:14:32 -0500 Subject: [PATCH 11/41] Anchor the reset test on the leaf migration's own models The pending-changes assertion read "Create model " with no name, which passes no matter what the refusal lists. Read the leaf migration's CreateModel operations instead, so deleting it has a named consequence and no branch has to re-pin the model by hand. --- .../tests/internal/test_migrations_reset.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/plain-postgres/tests/internal/test_migrations_reset.py b/plain-postgres/tests/internal/test_migrations_reset.py index 6cf053a08e..73ef8b6b7e 100644 --- a/plain-postgres/tests/internal/test_migrations_reset.py +++ b/plain-postgres/tests/internal/test_migrations_reset.py @@ -8,6 +8,7 @@ from __future__ import annotations +import importlib import shutil import subprocess from collections.abc import Callable @@ -32,6 +33,15 @@ EXAMPLES_COUNT = len(EXAMPLES_NAMES) LEAF = EXAMPLES_NAMES[-1] NEXT_NUMBER = f"{EXAMPLES_COUNT + 1:04d}" +# The models the leaf migration creates, so deleting it has a known consequence. +LEAF_CREATED_MODELS = [ + operation.name + for operation in importlib.import_module( + f"app.examples.migrations.{LEAF}" + ).Migration.operations + if isinstance(operation, operations.CreateModel) +] +assert LEAF_CREATED_MODELS, f"{LEAF} creates no models, so this file needs a new anchor" def migration_source( @@ -444,7 +454,8 @@ def test_pending_model_changes_refuse(migrations_dir: Path) -> None: assert result.exit_code != 0 assert "model changes its migrations don't hold" in result.output - assert "Create model " in result.output + for model_name in LEAF_CREATED_MODELS: + assert f"Create model {model_name}" in result.output assert len(examples_files(migrations_dir)) == EXAMPLES_COUNT - 1 From e3d794ab6d21a155aba2c1067ceb56cbd7cf12ee Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Fri, 18 Sep 2026 20:55:47 -0500 Subject: [PATCH 12/41] Annotate ReturningEvent for typed construction ReturningEvent predates the @dataclass_transform work, so it still used bare field assignments and re-declared the default `query`. Annotate each field with `Field[T]` and drop the redeclaration. Annotating `payload` as `Field[dict[str, Any] | None]` also makes `ReturningEvent.payload` a Field at class access, so the ty suppression on the delete-returning test is no longer needed. --- .../tests/app/examples/models/returning.py | 12 ++++++------ plain-postgres/tests/public/test_returning.py | 4 +--- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/plain-postgres/tests/app/examples/models/returning.py b/plain-postgres/tests/app/examples/models/returning.py index 133b972f5b..0ed99dcd75 100644 --- a/plain-postgres/tests/app/examples/models/returning.py +++ b/plain-postgres/tests/app/examples/models/returning.py @@ -4,15 +4,15 @@ from typing import Any -from plain.postgres import types +from plain.postgres import Field, types from plain import postgres @postgres.register_model class ReturningEvent(postgres.Model): - label = types.TextField(max_length=100) - count = types.IntegerField(default=0) - payload: dict[str, Any] | None = types.JSONField(required=False, allow_null=True) - - query: postgres.QuerySet[ReturningEvent] = postgres.QuerySet() + label: Field[str] = types.TextField(max_length=100) + count: Field[int] = types.IntegerField(default=0) + payload: Field[dict[str, Any] | None] = types.JSONField( + required=False, allow_null=True, default=None + ) diff --git a/plain-postgres/tests/public/test_returning.py b/plain-postgres/tests/public/test_returning.py index 57536d5b14..884e3c8157 100644 --- a/plain-postgres/tests/public/test_returning.py +++ b/plain-postgres/tests/public/test_returning.py @@ -79,9 +79,7 @@ def test_delete_returning_named_fields_gives_deleted_rows(db): _seed_events() rows = ( ReturningEvent.query.filter(label="a") - # payload is an explicitly annotated JSONField, so it types as dict|None - # at class access rather than Field; it is a Field at runtime. - .returning(ReturningEvent.id, ReturningEvent.payload) # ty: ignore[invalid-argument-type] + .returning(ReturningEvent.id, ReturningEvent.payload) .delete() ) From 3d8512fe0bbfe01fe30502d96f6dfa5fc97eff29 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Fri, 18 Sep 2026 21:00:09 -0500 Subject: [PATCH 13/41] Annotate UpsertItem for typed construction UpsertItem predates the @dataclass_transform work. Annotate its fields with `Field[T]` and drop the default `query` redeclaration. The null-key test constructs a deliberately invalid instance to reach bulk_upsert's runtime guard, which the typed constructor now flags -- suppress it the way the other intentional-violation tests do. Also point set_many's comment at bulk_upsert, the method it now calls. --- plain-postgres/tests/app/examples/models/upsert.py | 10 ++++------ plain-postgres/tests/public/test_bulk_upsert.py | 4 +++- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/plain-postgres/tests/app/examples/models/upsert.py b/plain-postgres/tests/app/examples/models/upsert.py index 74f51eab2d..4660682e03 100644 --- a/plain-postgres/tests/app/examples/models/upsert.py +++ b/plain-postgres/tests/app/examples/models/upsert.py @@ -2,18 +2,16 @@ from __future__ import annotations -from plain.postgres import types +from plain.postgres import Field, types from plain import postgres @postgres.register_model class UpsertItem(postgres.Model): - key = types.TextField(max_length=100) - value = types.IntegerField(default=0) - label = types.TextField(default="", required=False) - - query: postgres.QuerySet[UpsertItem] = postgres.QuerySet() + key: Field[str] = types.TextField(max_length=100) + value: Field[int] = types.IntegerField(default=0) + label: Field[str] = types.TextField(default="", required=False) model_options = postgres.Options( constraints=[ diff --git a/plain-postgres/tests/public/test_bulk_upsert.py b/plain-postgres/tests/public/test_bulk_upsert.py index c965514d0d..cee9f3a94f 100644 --- a/plain-postgres/tests/public/test_bulk_upsert.py +++ b/plain-postgres/tests/public/test_bulk_upsert.py @@ -122,7 +122,9 @@ def test_bulk_upsert_unique_fields_must_match_a_constraint(db): def test_bulk_upsert_null_unique_value_rejected(db): with pytest.raises(ValueError, match="non-null key"): UpsertItem.query.bulk_upsert( - [UpsertItem(key=None, value=1)], + # A null unique value is a type error the checker catches; the + # runtime guard is what protects callers that aren't type-checked. + [UpsertItem(key=None, value=1)], # ty: ignore[invalid-argument-type] update_fields=[UpsertItem.value], unique_fields=[UpsertItem.key], ) From be793d6d4be4e71adb5b80531f436090fe4222f4 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 12:55:15 -0500 Subject: [PATCH 14/41] Renumber the upsert migration past master's examples leaf --- .../migrations/{0020_upsertitem.py => 0022_upsertitem.py} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename plain-postgres/tests/app/examples/migrations/{0020_upsertitem.py => 0022_upsertitem.py} (89%) diff --git a/plain-postgres/tests/app/examples/migrations/0020_upsertitem.py b/plain-postgres/tests/app/examples/migrations/0022_upsertitem.py similarity index 89% rename from plain-postgres/tests/app/examples/migrations/0020_upsertitem.py rename to plain-postgres/tests/app/examples/migrations/0022_upsertitem.py index 67399beea9..8779d8e97e 100644 --- a/plain-postgres/tests/app/examples/migrations/0020_upsertitem.py +++ b/plain-postgres/tests/app/examples/migrations/0022_upsertitem.py @@ -4,7 +4,7 @@ class Migration(migrations.Migration): - dependencies = (("examples", "0019_returningevent"),) + dependencies = (("examples", "0021_returningevent"),) operations = ( migrations.CreateModel( From 7126881538323d4ab52ecb0cbb7c61c93bca73bf Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 13:20:15 -0500 Subject: [PATCH 15/41] plain-postgres: one write compiler for UPDATE and DELETE The two compilers had byte-identical execute_sql bodies, and the RETURNING clause was emitted through a _returning_sql() helper parked on the base SQLCompiler -- where `query.returning_fields` doesn't exist, hence the unresolved-attribute suppression. Give them a shared SQLWriteCompiler base that runs the statement once, and call returning_columns() directly now that both subclasses narrow `query`. returning_columns() takes None so the `or []` goes away too. The DELETE compiler builds its SQL the same way the UPDATE one does, which retires the leading-space fixup and makes it visible that the RETURNING clause is read off self.query -- load-bearing, since the multi-alias branch passes in a freshly built outer Query. Also drop the per-row list copy in convert_returning_rows when no converters apply: every consumer only zips the rows, and the INSERT path now shares this function, so bulk_create was paying for a copy it never used. --- plain-postgres/plain/postgres/dialect.py | 4 +- plain-postgres/plain/postgres/sql/compiler.py | 98 ++++++++----------- 2 files changed, 43 insertions(+), 59 deletions(-) diff --git a/plain-postgres/plain/postgres/dialect.py b/plain-postgres/plain/postgres/dialect.py index 96131ff907..aa790f7312 100644 --- a/plain-postgres/plain/postgres/dialect.py +++ b/plain-postgres/plain/postgres/dialect.py @@ -376,8 +376,8 @@ def lookup_cast(lookup_type: str, field: Field | None = None) -> str: return lookup -def returning_columns(fields: list[Field]) -> str: - """Return the RETURNING clause SQL for the given fields, or "" when empty.""" +def returning_columns(fields: list[Field] | None) -> str: + """Return the RETURNING clause SQL for the given fields, or "" when there are none.""" if not fields: return "" columns = [ diff --git a/plain-postgres/plain/postgres/sql/compiler.py b/plain-postgres/plain/postgres/sql/compiler.py index 7cea560529..340c4ea679 100644 --- a/plain-postgres/plain/postgres/sql/compiler.py +++ b/plain-postgres/plain/postgres/sql/compiler.py @@ -29,7 +29,7 @@ ResolvableExpression, Value, ) -from plain.postgres.fields import DATABASE_DEFAULT +from plain.postgres.fields import DATABASE_DEFAULT, Field from plain.postgres.fields.related import RelatedField from plain.postgres.functions import Cast, Random from plain.postgres.lookups import Lookup @@ -109,8 +109,8 @@ def apply_converters( def convert_returning_rows( - rows: Iterable, fields: list[Any], connection: DatabaseConnection -) -> list[list]: + rows: Iterable, fields: list[Field], connection: DatabaseConnection +) -> list[Sequence[Any]]: """Apply each field's DB converters to the raw rows of a RETURNING clause. The fields are given in the same order as the emitted RETURNING columns, @@ -120,7 +120,7 @@ def convert_returning_rows( converters = get_converters(cols, connection) if converters: return list(apply_converters(rows, converters, connection)) - return [list(row) for row in rows] + return list(rows) class SQLCompiler: @@ -527,14 +527,6 @@ def compile(self, node: SQLCompilable) -> SqlWithParams: sql, params = node.as_sql(self, self.connection) return sql, tuple(params) - def _returning_sql(self) -> str: - """Return the RETURNING clause for this query's returning_fields, or "". - - Shared by the UPDATE and DELETE compilers, whose queries carry - returning_fields; the RETURNING clause never takes params. - """ - return returning_columns(self.query.returning_fields or []) # ty: ignore[unresolved-attribute] - def get_qualify_sql(self) -> tuple[list[str], list[Any]]: where_parts = [] if self.where: @@ -1522,7 +1514,31 @@ def execute_sql( # ty: ignore[invalid-method-override] return convert_returning_rows(rows, self.returning_fields, self.connection) -class SQLDeleteCompiler(SQLCompiler): +class SQLWriteCompiler(SQLCompiler): + """Base for the UPDATE and DELETE compilers. + + Both of their queries can carry returning_fields, so both run the + statement the same way: a rowcount normally, the converted RETURNING + rows when fields were asked for. + """ + + query: UpdateQuery | DeleteQuery + + def execute_sql(self, result_type: str) -> Any: # ty: ignore[invalid-method-override] + cursor = super().execute_sql(result_type) + if not cursor: + return [] if self.query.returning_fields else 0 + try: + if self.query.returning_fields: + return convert_returning_rows( + cursor.fetchall(), self.query.returning_fields, self.connection + ) + return cursor.rowcount + finally: + cursor.close() + + +class SQLDeleteCompiler(SQLWriteCompiler): query: DeleteQuery @cached_property @@ -1552,15 +1568,19 @@ def contains_self_reference_subquery(self) -> bool: ) def _as_sql(self, query: Query) -> SqlWithParams: - delete = f"DELETE FROM {self.quote_name_unless_alias(query.base_table)}" # ty: ignore[invalid-argument-type] - returning = self._returning_sql() - if returning: - returning = f" {returning}" + result = [f"DELETE FROM {self.quote_name_unless_alias(query.base_table)}"] # ty: ignore[invalid-argument-type] try: where, params = self.compile(query.where) except FullResultSet: - return f"{delete}{returning}", () - return f"{delete} WHERE {where}{returning}", tuple(params) + params = () + else: + result.append(f"WHERE {where}") + # RETURNING comes off self.query, not the query argument: the + # multi-alias branch below passes in a freshly built outer Query that + # carries no returning_fields of its own. + if returning := returning_columns(self.query.returning_fields): + result.append(returning) + return " ".join(result), tuple(params) def as_sql( self, with_limits: bool = True, with_col_aliases: bool = False @@ -1581,26 +1601,8 @@ def as_sql( outerq.add_filter("id__in", innerq) return self._as_sql(outerq) - def execute_sql(self, result_type: str) -> Any: # ty: ignore[invalid-method-override] - """Execute the delete. - - Return the number of rows deleted, or — when the query carries - returning_fields — the converted RETURNING rows. - """ - cursor = super().execute_sql(result_type) - if not cursor: - return [] if self.query.returning_fields else 0 - try: - if self.query.returning_fields: - return convert_returning_rows( - cursor.fetchall(), self.query.returning_fields, self.connection - ) - return cursor.rowcount - finally: - cursor.close() - -class SQLUpdateCompiler(SQLCompiler): +class SQLUpdateCompiler(SQLWriteCompiler): query: UpdateQuery def as_sql( @@ -1667,28 +1669,10 @@ def as_sql( params = [] else: result.append(f"WHERE {where}") - if returning := self._returning_sql(): + if returning := returning_columns(self.query.returning_fields): result.append(returning) return " ".join(result), tuple(update_params + list(params)) - def execute_sql(self, result_type: str) -> Any: # ty: ignore[invalid-method-override] - """Execute the update. - - Return the number of rows affected, or — when the query carries - returning_fields — the converted RETURNING rows. - """ - cursor = super().execute_sql(result_type) - if not cursor: - return [] if self.query.returning_fields else 0 - try: - if self.query.returning_fields: - return convert_returning_rows( - cursor.fetchall(), self.query.returning_fields, self.connection - ) - return cursor.rowcount - finally: - cursor.close() - def pre_sql_setup( self, with_col_aliases: bool = False ) -> tuple[list[Any], list[Any], list[SqlWithParams]] | None: From 34918582144b810250b650e1e01ad52829c9af84 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 13:20:37 -0500 Subject: [PATCH 16/41] plain-postgres: make the returning() queryset state say what it means _returning held the raw returning() arguments purely so _hydrate_returning could ask one question of them -- "were any fields named?" -- which forced a three-state encoding (None / () / (field, ...)) the reader had to decode, and split the instances-vs-dicts decision across a parameter and an attribute. Keep the resolved columns and one explicit bool instead, and move hydration onto ReturningQuerySet, where the rows actually go. update()/delete() split into a plain method that returns the rowcount it advertises and an _execute_* that hands the raw result back, so the base QuerySet's `-> int` is true again and the two invalid-return-type suppressions go away. --- plain-postgres/plain/postgres/query.py | 110 ++++++++++++++----------- 1 file changed, 60 insertions(+), 50 deletions(-) diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index b51721c6ca..63df894a90 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -286,13 +286,13 @@ class Task(Model): _fields: tuple[str, ...] | None _defer_next_filter: bool _deferred_filter: tuple[bool, tuple[Any, ...], dict[str, Any]] | None - # None => plain update()/delete() returning an int rowcount. - # () => RETURNING every column, hydrated into model instances. - # (field, ...) => RETURNING those columns, returned as dicts. - _returning: tuple[Field, ...] | None - # The fields to RETURN, or None for a plain write. Set once by - # returning() so update()/delete() don't recompute them at execute. + # The columns to RETURN from the next update()/delete(), or None for a + # plain write that returns an int rowcount. Resolved once by returning() + # so the write doesn't recompute them at execute time. _returning_fields: list[Field] | None + # True when returning() was called with no arguments: hydrate the rows + # into model instances rather than dicts. + _returning_instances: bool def __init__(self): """Minimal init for descriptor mode. Use from_model() to create instances.""" @@ -312,8 +312,8 @@ def from_model(cls, model: type[T], query: Query | None = None) -> Self: instance._fields = None instance._defer_next_filter = False instance._deferred_filter = None - instance._returning = None instance._returning_fields = None + instance._returning_instances = False return instance @overload @@ -946,18 +946,23 @@ def returning(self, *fields: Field[Any]) -> ReturningQuerySet[T, Any]: """ clone = self._chain() clone.__class__ = ReturningQuerySet - clone._returning = fields - clone._returning_fields = clone._resolve_returning_fields() + if fields: + clone._returning_fields = self._validated_returning_fields(fields) + clone._returning_instances = False + else: + # No references given: RETURN every column so the rows can be + # hydrated into full model instances. + clone._returning_fields = list(self.model._model_meta.fields) + clone._returning_instances = True return cast("ReturningQuerySet[T, Any]", clone) - def _resolve_returning_fields(self) -> list[Field]: - """Validate self._returning and produce the fields to RETURN.""" + def _validated_returning_fields( + self, fields: tuple[Field[Any], ...] + ) -> list[Field]: + """Check each returning() reference and return the columns to RETURN.""" object_name = self.model.model_options.object_name - if not self._returning: - # No references given: RETURN every column so the rows can - # be hydrated into full model instances. - return list(self.model._model_meta.fields) - for field in self._returning: + columns = [] + for field in fields: if isinstance(field, str): raise TypeError( f"returning() takes field references, not strings. " @@ -979,14 +984,8 @@ def _resolve_returning_fields(self) -> list[Field]: f"Cannot use {object_name}.{field.name} in returning(): " "only database columns can be returned." ) - return list(self._returning) - - def _hydrate_returning(self, fields: list[Field], rows: list[list]) -> list[Any]: - """Turn converted RETURNING rows into instances (no names) or dicts.""" - field_names = [field.name for field in fields] - if not self._returning: - return [self.model.from_db(field_names, row) for row in rows] - return [dict(zip(field_names, row)) for row in rows] + columns.append(field) + return columns def delete(self) -> int: """Delete the records in the current QuerySet. @@ -995,6 +994,16 @@ def delete(self) -> int: handled by Postgres via the declared `on_delete` clauses and are not included in the count. """ + return self._execute_delete() + + def _execute_delete(self) -> Any: + """Run the DELETE. + + Returns the rowcount, or — when returning() set columns on this + queryset — the converted RETURNING rows for ReturningQuerySet.delete() + to hydrate. Only the target table's rows come back; cascade deletes + never appear in a RETURNING clause. + """ if self.sql_query.is_sliced: raise TypeError("Cannot use 'limit' or 'offset' with delete().") if self.sql_query.distinct or self.sql_query.distinct_fields: @@ -1002,8 +1011,6 @@ def delete(self) -> int: if self._fields is not None: raise TypeError("Cannot call delete() after .values() or .values_list()") - returning_fields = self._returning_fields - del_query = self._chain() del_query.sql_query.select_for_update = False del_query.sql_query.select_related = False @@ -1013,27 +1020,20 @@ def delete(self) -> int: # connection so outer atomic() blocks see the abort state even if the # caller catches IntegrityError themselves. with transaction.mark_for_rollback_on_error(): - result = del_query._raw_delete(returning_fields) + result = del_query._raw_delete() # Clear the result cache, in case this QuerySet gets reused. self._result_cache = None - if returning_fields is None: - return result - # Reachable only via ReturningQuerySet.delete(), which returns R. - return self._hydrate_returning(returning_fields, result) # ty: ignore[invalid-return-type] + return result - def _raw_delete(self, returning_fields: list[Field] | None = None) -> Any: + def _raw_delete(self) -> Any: """ Delete objects found from the given queryset in single direct SQL query. No signals are sent and there is no protection for cascades. - - Returns the rowcount, or the converted RETURNING rows when - returning_fields is given (only the target table's rows — cascade - deletes never appear in a RETURNING clause). """ query = cast(DeleteQuery, self.sql_query.clone()) query.__class__ = DeleteQuery - query.returning_fields = returning_fields + query.returning_fields = self._returning_fields return query.get_compiler().execute_sql(CURSOR) def update(self, **kwargs: Any) -> int: @@ -1041,6 +1041,15 @@ def update(self, **kwargs: Any) -> int: Update all elements in the current QuerySet, setting all the given fields to the appropriate values. """ + return self._execute_update(kwargs) + + def _execute_update(self, kwargs: dict[str, Any]) -> Any: + """Run the UPDATE. + + Returns the rowcount, or — when returning() set columns on this + queryset — the converted RETURNING rows for ReturningQuerySet.update() + to hydrate. + """ if self.sql_query.is_sliced: raise TypeError("Cannot update a query once a slice has been taken.") if self._fields is not None: @@ -1071,17 +1080,12 @@ def update(self, **kwargs: Any) -> int: # Clear any annotations so that they won't be present in subqueries. query.annotations = {} - returning_fields = self._returning_fields - if returning_fields is not None: - query.returning_fields = returning_fields + query.returning_fields = self._returning_fields with transaction.mark_for_rollback_on_error(): result = query.get_compiler().execute_sql(CURSOR) self._result_cache = None - if returning_fields is None: - return result - # Reachable only via ReturningQuerySet.update(), which returns R. - return self._hydrate_returning(returning_fields, result) # ty: ignore[invalid-return-type] + return result def _update(self, values: Sequence[tuple[Field, Any]]) -> int: """ @@ -1525,8 +1529,8 @@ def _clone(self) -> Self: c._known_related_objects = self._known_related_objects c._iterable_class = self._iterable_class c._fields = self._fields - c._returning = self._returning c._returning_fields = self._returning_fields + c._returning_instances = self._returning_instances return c def _attach_result_cache(self, obj: Self, cache: list[T]) -> None: @@ -1615,16 +1619,22 @@ class ReturningQuerySet[T: "Model", R](QuerySet[T]): Produced by QuerySet.returning(); the second type parameter R is the return type of update()/delete() (a list of instances or of dicts), - pinned by the returning() overloads. The runtime behavior is driven by - self._returning — this subclass only makes the return types honest for - static checkers. + pinned by the returning() overloads. """ def update(self, **kwargs: Any) -> R: # ty: ignore[invalid-method-override] - return cast("R", super().update(**kwargs)) + return cast("R", self._hydrate_returning(self._execute_update(kwargs))) def delete(self) -> R: # ty: ignore[invalid-method-override] - return cast("R", super().delete()) + return cast("R", self._hydrate_returning(self._execute_delete())) + + def _hydrate_returning(self, rows: list[Sequence[Any]]) -> list[Any]: + """Turn converted RETURNING rows into instances or dicts.""" + assert self._returning_fields is not None + field_names = [field.name for field in self._returning_fields] + if self._returning_instances: + return [self.model.from_db(field_names, row) for row in rows] + return [dict(zip(field_names, row)) for row in rows] class InstanceCheckMeta(type): From 26d27a892aeb75c3243d098ff89c0a8d2c217e8b Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 13:20:56 -0500 Subject: [PATCH 17/41] plain-postgres: refuse writes that returning() doesn't apply to returning() leaked into every other write on the same queryset. bulk_update() was the visible casualty: it chains internally and does `rows_updated += queryset.filter(id__in=ids).update(...)`, so the inner update handed back a list and the caller died on `unsupported operand type(s) for +=: 'int' and 'list'`. create(), bulk_create(), get_or_create() and update_or_create() quietly ignored it. Say so instead, the way .values() before a returning write already does. Also give Model.fk a real answer: at class level it is the relation, not its column -- that is what lets where() traverse it -- so returning(Child.parent) was dumping a descriptor repr into a TypeError. Point at no-arg returning(), which carries the foreign key on the instance. Tests cover the rejected writes, the relation reference, and the join rewrite on both update() and delete() -- RETURNING has to survive the `WHERE id IN (subquery)` rewrite in a single statement, which the PR claimed but never checked. --- plain-postgres/plain/postgres/README.md | 5 ++ plain-postgres/plain/postgres/query.py | 32 +++++++ plain-postgres/tests/public/test_returning.py | 87 +++++++++++++++++++ 3 files changed, 124 insertions(+) diff --git a/plain-postgres/plain/postgres/README.md b/plain-postgres/plain/postgres/README.md index c22fc0dddd..d8eb0f8416 100644 --- a/plain-postgres/plain/postgres/README.md +++ b/plain-postgres/plain/postgres/README.md @@ -525,9 +525,14 @@ for row in deleted: - **`returning()`** returns full model instances. For `update()` they hold the new values; for `delete()`, the rows as they were. - **`returning(Model.field, ...)`** returns a list of dicts with only those columns. Pass field references (`Model.field`), not strings; a many-to-many field or one from another model raises an error at the `returning()` call. - Without `returning()`, `update()`/`delete()` return an `int` as before. +- `returning()` only applies to `update()` and `delete()`. Any other write on the same queryset — `create()`, `bulk_create()`, `bulk_update()`, `get_or_create()`, `update_or_create()` — raises `TypeError` rather than quietly dropping it. + +The values you get back are whatever the statement wrote, exactly as Postgres holds them. A set-based `update()` doesn't run Python-side field hooks, so an `update_now=True` timestamp comes back unchanged unless the `update()` set it. `RETURNING` only reports rows of the statement's own target table. Rows removed by a cascading `ON DELETE` are never included — a `delete()` with `returning()` gives you the parent rows you deleted, not the children Postgres cascaded. +Every affected row is fetched and built into memory at once, so `returning()` belongs on writes you've already bounded by a filter. For a write that spans a whole table, take the rowcount and page through the rows separately. + ## Transactions By default, each query runs in its own implicit transaction and is committed immediately (autocommit mode). When you need multiple queries to succeed or fail together — like creating a user and their profile — wrap them in an explicit transaction. diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index 63df894a90..15a97a16e8 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -621,6 +621,7 @@ def create(self, **kwargs: Any) -> T: Create a new object with the given kwargs, saving it to the database and returning the created object. """ + self._reject_returning("create") obj = self.model(**kwargs) obj.create() return obj @@ -684,6 +685,7 @@ def bulk_create( save() on each of the instances. Primary keys are set on the objects via the PostgreSQL RETURNING clause. Multi-table models are not supported. """ + self._reject_returning("bulk_create") if batch_size is not None and batch_size <= 0: raise ValueError("Batch size must be a positive integer.") @@ -751,6 +753,7 @@ def bulk_update( """ Update the given fields in each of the given objects in the database. """ + self._reject_returning("bulk_update") if batch_size is not None and batch_size <= 0: raise ValueError("Batch size must be a positive integer.") if not fields: @@ -811,6 +814,7 @@ def get_or_create( Return a tuple of (object, created), where created is a boolean specifying whether an object was created. """ + self._reject_returning("get_or_create") # The get() needs to be targeted at the write database in order # to avoid potential transaction consistency problems. try: @@ -850,6 +854,7 @@ def update_or_create( Return a tuple (object, created), where created is a boolean specifying whether an object was created. """ + self._reject_returning("update_or_create") if create_defaults is None: update_defaults = create_defaults = defaults or {} else: @@ -960,9 +965,24 @@ def _validated_returning_fields( self, fields: tuple[Field[Any], ...] ) -> list[Field]: """Check each returning() reference and return the columns to RETURN.""" + # Local import: related_descriptors imports this module at load time. + from plain.postgres.fields.related_descriptors import ( + ForwardForeignKeyDescriptor, + ) + object_name = self.model.model_options.object_name columns = [] for field in fields: + if isinstance(field, ForwardForeignKeyDescriptor): + # Model.fk is the relation at class level, not its column -- + # that is what lets where() traverse it. So there is no + # reference to name the foreign key column with here. + raise FieldError( + f"Cannot use {object_name}.{field._field.name} in " + "returning(): it is a relation, not a column reference. " + "Use returning() with no arguments to get whole " + "instances, which carry the foreign key." + ) if isinstance(field, str): raise TypeError( f"returning() takes field references, not strings. " @@ -987,6 +1007,18 @@ def _validated_returning_fields( columns.append(field) return columns + def _reject_returning(self, method_name: str) -> None: + """Refuse a write that RETURNING doesn't apply to. + + returning() only changes what update() and delete() hand back. + Every other write would silently drop it, so say so instead. + """ + if self._returning_fields is not None: + raise TypeError( + f"Cannot call {method_name}() on a returning() queryset. " + "returning() only applies to update() and delete()." + ) + def delete(self) -> int: """Delete the records in the current QuerySet. diff --git a/plain-postgres/tests/public/test_returning.py b/plain-postgres/tests/public/test_returning.py index 884e3c8157..bea503fc84 100644 --- a/plain-postgres/tests/public/test_returning.py +++ b/plain-postgres/tests/public/test_returning.py @@ -166,3 +166,90 @@ def test_delete_returning_excludes_cascade_deleted_children(db): assert len(rows) == 1 assert rows[0] == {"id": parent.id, "name": "p"} assert ChildCascade.query.count() == 0 + + +# =========================================================================== +# Joins — the WHERE id IN (subquery) rewrite keeps RETURNING +# =========================================================================== + + +def test_update_returning_across_a_relation(db, capture_queries): + keep = DeleteParent(name="keep").create() + move = DeleteParent(name="move").create() + ChildCascade(parent=move).create() + ChildCascade(parent=move).create() + ChildCascade(parent=keep).create() + + with capture_queries() as queries: + rows = ( + ChildCascade.query.filter(parent__name="move") + .returning(ChildCascade.id) + .update(parent=keep) + ) + + # Filtering across the FK rewrites the UPDATE to `WHERE id IN (subquery)`; + # RETURNING has to survive that rewrite, in one statement. + assert len(queries) == 1 + assert "RETURNING" in queries[0]["sql"] + assert len(rows) == 2 + assert ChildCascade.query.filter(parent=keep).count() == 3 + + +def test_delete_returning_across_a_relation(db, capture_queries): + parent = DeleteParent(name="doomed").create() + ChildCascade(parent=parent).create() + ChildCascade(parent=parent).create() + + with capture_queries() as queries: + rows = ChildCascade.query.filter(parent__name="doomed").returning().delete() + + assert len(queries) == 1 + assert "RETURNING" in queries[0]["sql"] + assert len(rows) == 2 + assert all(row.parent.id == parent.id for row in rows) + + +# =========================================================================== +# Foreign key columns +# =========================================================================== + + +def test_returning_relation_reference_errors(db): + # Model.fk is the relation, not the column, so it has no spelling here -- + # say that instead of dumping the descriptor's repr. + with pytest.raises(FieldError, match="it is a relation, not a column"): + ChildCascade.query.returning(ChildCascade.parent) # ty: ignore[invalid-argument-type] + + +def test_returning_instances_carry_foreign_keys(db): + parent = DeleteParent(name="p").create() + ChildCascade(parent=parent).create() + + rows = ChildCascade.query.returning().delete() + + assert len(rows) == 1 + assert rows[0].parent.id == parent.id + + +# =========================================================================== +# Writes that RETURNING doesn't apply to say so +# =========================================================================== + + +@pytest.mark.parametrize( + "write", + [ + lambda qs: qs.create(label="x", count=1), + lambda qs: qs.bulk_create([ReturningEvent(label="x", count=1)]), + lambda qs: qs.bulk_update(list(ReturningEvent.query), ["count"]), + lambda qs: qs.get_or_create(label="x", count=1), + lambda qs: qs.update_or_create(label="x", defaults={"count": 1}), + ], + ids=["create", "bulk_create", "bulk_update", "get_or_create", "update_or_create"], +) +def test_returning_rejects_other_writes(db, write): + ReturningEvent(label="seed", count=1).create() + with pytest.raises( + TypeError, match="only applies to update\\(\\) and delete\\(\\)" + ): + write(ReturningEvent.query.returning()) From e999fa267227bf63f7c7d63c935a8991bcb7a975 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 13:22:48 -0500 Subject: [PATCH 18/41] plain-postgres: export ReturningQuerySet and pin the four return shapes ReturningQuerySet is the declared return type of returning(), so it has to be importable from plain.postgres like QuerySet is -- otherwise annotating a returning queryset means reaching into plain.postgres.query. The assert_type block is the part that actually proves the typing claim: update()/delete() stay int on a plain queryset, become list[Model] after no-arg returning(), and list[dict[str, Any]] after named fields. Checked by ./scripts/type-check, not at runtime. --- plain-postgres/plain/postgres/__init__.py | 3 ++- plain-postgres/tests/public/test_returning.py | 20 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/plain-postgres/plain/postgres/__init__.py b/plain-postgres/plain/postgres/__init__.py index 7b34ed355c..17686907be 100644 --- a/plain-postgres/plain/postgres/__init__.py +++ b/plain-postgres/plain/postgres/__init__.py @@ -49,7 +49,7 @@ ) from .indexes import Index from .options import Options -from .query import QuerySet +from .query import QuerySet, ReturningQuerySet from .query_utils import Q from . import types @@ -94,6 +94,7 @@ "PrimaryKeyField", "Q", "QuerySet", + "ReturningQuerySet", "RandomStringField", "ReverseForeignKey", "ReverseManyToMany", diff --git a/plain-postgres/tests/public/test_returning.py b/plain-postgres/tests/public/test_returning.py index bea503fc84..b57b91cf68 100644 --- a/plain-postgres/tests/public/test_returning.py +++ b/plain-postgres/tests/public/test_returning.py @@ -7,6 +7,8 @@ from __future__ import annotations +from typing import Any, assert_type + import pytest from app.examples.models.delete import ChildCascade, DeleteParent from app.examples.models.returning import ReturningEvent @@ -253,3 +255,21 @@ def test_returning_rejects_other_writes(db, write): TypeError, match="only applies to update\\(\\) and delete\\(\\)" ): write(ReturningEvent.query.returning()) + + +def test_returning_return_types_are_honest(db): + """The four write shapes resolve to what they actually return. + + These `assert_type` calls are checked by the type checker, not at + runtime -- `./scripts/type-check .` is what makes this test meaningful. + """ + qs = ReturningEvent.query.filter(label="a") + + assert_type(qs.update(count=1), int) + assert_type(qs.delete(), int) + assert_type(qs.returning().update(count=1), list[ReturningEvent]) + assert_type(qs.returning().delete(), list[ReturningEvent]) + assert_type( + qs.returning(ReturningEvent.id).update(count=1), list[dict[str, Any]] + ) + assert_type(qs.returning(ReturningEvent.id).delete(), list[dict[str, Any]]) From 5bf14d641835e967a40ff389e8b6baff8df1507e Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 13:32:09 -0500 Subject: [PATCH 19/41] plain-postgres: move the returning() type claims into the typing corpus test_returning_return_types_are_honest was an assert_type ladder with no runtime half, so it belongs in tests/typing/ where the checker is the test runner. The must-reject claims for string field names and relation references move there too, cross-referenced to the pytest.raises halves that stay in tests/public/. --- plain-postgres/plain/postgres/__init__.py | 2 +- plain-postgres/tests/public/test_returning.py | 22 +--------- .../tests/typing/returning_writes.py | 43 +++++++++++++++++++ 3 files changed, 45 insertions(+), 22 deletions(-) create mode 100644 plain-postgres/tests/typing/returning_writes.py diff --git a/plain-postgres/plain/postgres/__init__.py b/plain-postgres/plain/postgres/__init__.py index 17686907be..abe33f0b09 100644 --- a/plain-postgres/plain/postgres/__init__.py +++ b/plain-postgres/plain/postgres/__init__.py @@ -94,8 +94,8 @@ "PrimaryKeyField", "Q", "QuerySet", - "ReturningQuerySet", "RandomStringField", + "ReturningQuerySet", "ReverseForeignKey", "ReverseManyToMany", "SmallIntegerField", diff --git a/plain-postgres/tests/public/test_returning.py b/plain-postgres/tests/public/test_returning.py index b57b91cf68..9cc7f63703 100644 --- a/plain-postgres/tests/public/test_returning.py +++ b/plain-postgres/tests/public/test_returning.py @@ -7,13 +7,11 @@ from __future__ import annotations -from typing import Any, assert_type - import pytest from app.examples.models.delete import ChildCascade, DeleteParent from app.examples.models.returning import ReturningEvent +from plain.postgres import ReturningQuerySet from plain.postgres.exceptions import FieldError -from plain.postgres.query import ReturningQuerySet def _seed_events() -> None: @@ -255,21 +253,3 @@ def test_returning_rejects_other_writes(db, write): TypeError, match="only applies to update\\(\\) and delete\\(\\)" ): write(ReturningEvent.query.returning()) - - -def test_returning_return_types_are_honest(db): - """The four write shapes resolve to what they actually return. - - These `assert_type` calls are checked by the type checker, not at - runtime -- `./scripts/type-check .` is what makes this test meaningful. - """ - qs = ReturningEvent.query.filter(label="a") - - assert_type(qs.update(count=1), int) - assert_type(qs.delete(), int) - assert_type(qs.returning().update(count=1), list[ReturningEvent]) - assert_type(qs.returning().delete(), list[ReturningEvent]) - assert_type( - qs.returning(ReturningEvent.id).update(count=1), list[dict[str, Any]] - ) - assert_type(qs.returning(ReturningEvent.id).delete(), list[dict[str, Any]]) diff --git a/plain-postgres/tests/typing/returning_writes.py b/plain-postgres/tests/typing/returning_writes.py new file mode 100644 index 0000000000..04a65bc3d1 --- /dev/null +++ b/plain-postgres/tests/typing/returning_writes.py @@ -0,0 +1,43 @@ +"""`returning()` pins what update()/delete() hand back. + +Without it both writes are an int rowcount. The no-arg overload hydrates model +instances; the field-reference overload hands back dicts of just those columns. +The overload ladder is the whole promise, so it is asserted statically. +""" + +from __future__ import annotations + +from typing import Any, assert_type + +from app.examples.models.delete import ChildCascade +from app.examples.models.returning import ReturningEvent + + +def must_accept_the_plain_writes() -> None: + qs = ReturningEvent.query.filter(label="a") + assert_type(qs.update(count=1), int) + assert_type(qs.delete(), int) + + +def must_accept_no_arg_returning_as_instances() -> None: + qs = ReturningEvent.query.filter(label="a") + assert_type(qs.returning().update(count=1), list[ReturningEvent]) + assert_type(qs.returning().delete(), list[ReturningEvent]) + + +def must_accept_field_returning_as_dicts() -> None: + qs = ReturningEvent.query.filter(label="a") + assert_type(qs.returning(ReturningEvent.id).update(count=1), list[dict[str, Any]]) + assert_type(qs.returning(ReturningEvent.id).delete(), list[dict[str, Any]]) + + +def must_reject_string_field_names() -> None: + # Runtime half: tests/public/test_returning.py::test_returning_string_arg_errors. + ReturningEvent.query.returning("count") # ty: ignore[invalid-argument-type] + + +def must_reject_a_relation_reference() -> None: + # Model.fk is the relation descriptor, not a column. + # Runtime half: + # tests/public/test_returning.py::test_returning_relation_reference_errors. + ChildCascade.query.returning(ChildCascade.parent) # ty: ignore[invalid-argument-type] From f46e3539ade264e6374a935ec42abec08593203a Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 14:03:57 -0500 Subject: [PATCH 20/41] plain-postgres: make bulk_upsert refuse the keys it can't match back Three ways the row match-back could go wrong, all of them after the write had already gone out: - A db_returning unique field (create_now, generate=True, RandomStringField) leaves the objects holding a DatabaseDefault sentinel, which is not None, so it passed the null guard, became the lookup key, and never matched the real value Postgres returned -- a bare KeyError on an already-committed insert. Refused up front now. - Two objects sharing a conflict key hit a Postgres cardinality violation in one batch, but silently let the last one win when the batch boundary fell between them, hydrating both objects from the same row. Refused up front, whatever the batch size. - The arguments weren't validated at all when objs was empty. The match-back itself also moves inside the transaction: a row that can't be matched leaves the objects half-populated, so the write should roll back rather than commit. Collapsing keyed into a dict keyed by the conflict key does the duplicate check, the sort, and the lookup in one structure. --- plain-postgres/plain/postgres/query.py | 93 ++++++++++++++++---------- 1 file changed, 59 insertions(+), 34 deletions(-) diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index e664bf82dc..4cfb8bb60a 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -691,7 +691,10 @@ def _check_bulk_upsert_options( update_fields: list[Field], unique_fields: list[Field], ) -> None: - model_name = self.model.__name__ + object_name = self.model.model_options.object_name + + self._validate_field_refs(unique_fields, where="bulk_upsert() unique_fields") + self._validate_field_refs(update_fields, where="bulk_upsert() update_fields") if not unique_fields: raise ValueError("bulk_upsert() requires unique_fields.") @@ -700,10 +703,20 @@ def _check_bulk_upsert_options( ): names = [f.name for f in unique_fields] raise ValueError( - f"bulk_upsert() unique_fields {names} on {model_name} must name " + f"bulk_upsert() unique_fields {names} on {object_name} must name " "the primary key or a UniqueConstraint declared on the model " "without a condition or expressions." ) + for field in unique_fields: + # Postgres fills in every db_returning column but the primary key + # (create_now, generate=True, RandomStringField), so the objects + # never carry a value to conflict on or to match the row back by. + if field.db_returning and not field.primary_key: + raise ValueError( + f"bulk_upsert() cannot use {object_name}.{field.name} in " + "unique_fields: the database generates its value, so the " + "objects never carry one to conflict on." + ) if not update_fields: raise ValueError("bulk_upsert() requires update_fields.") @@ -732,9 +745,13 @@ def bulk_upsert( INSERT ... ON CONFLICT (unique_fields) DO UPDATE ... RETURNING per batch. Both inserted and updated objects come back with their DB-returned - fields (primary key, DB defaults) populated. update_fields and - unique_fields take field references (`Model.field`); unique_fields must - name the primary key or a UniqueConstraint declared on the model. + fields (primary key, DB defaults) populated, in the order they were + passed in. update_fields and unique_fields take field references + (`Model.field`); unique_fields must name the primary key or a + UniqueConstraint declared on the model. + + Only the named update_fields are written on a conflicting row -- an + update_now column left out of update_fields keeps its stored value. bulk_upsert() carries its own RETURNING to populate the objects, so a prior returning() has nothing to add and is refused. @@ -743,24 +760,23 @@ def bulk_upsert( if batch_size is not None and batch_size <= 0: raise ValueError("Batch size must be a positive integer.") + self._check_bulk_upsert_options(update_fields, unique_fields) + objs = list(objs) if not objs: return objs - self._validate_field_refs(unique_fields, where="bulk_upsert() unique_fields") - self._validate_field_refs(update_fields, where="bulk_upsert() update_fields") - meta = self.model._model_meta - self._check_bulk_upsert_options(update_fields, unique_fields) - + object_name = self.model.model_options.object_name self._prepare_for_bulk_create(objs) - # Compute each object's conflict key exactly once, rejecting nulls as we - # go (NULL never conflicts in Postgres, so it can't be upserted). Reused - # below to sort the batch and to match RETURNING rows back to objects. - keyed: list[tuple[tuple[Any, ...], T]] = [] + # Compute each object's conflict key exactly once. Reused below to sort + # the batches and to match RETURNING rows back to objects, so the keys + # have to be usable for both: no nulls (NULL never conflicts in + # Postgres) and no duplicates (one statement can only touch a row once). + obj_by_key: dict[tuple[Any, ...], T] = {} for obj in objs: - key = [] + key_values = [] for field in unique_fields: value = field.value_from_object(obj) if value is None: @@ -769,8 +785,16 @@ def bulk_upsert( "object; NULL never conflicts in Postgres, so it cannot " "be upserted." ) - key.append(value) - keyed.append((tuple(key), obj)) + key_values.append(value) + key = tuple(key_values) + if key in obj_by_key: + names = [f.name for f in unique_fields] + raise ValueError( + f"bulk_upsert() got more than one {object_name} with " + f"{names} = {list(key)}. Postgres can only touch a row once " + "per statement, so collapse the duplicates before calling." + ) + obj_by_key[key] = obj # Include the PK column only when it is itself the conflict key; # otherwise let Postgres generate the identity value. @@ -786,15 +810,15 @@ def bulk_upsert( if field not in returning_fields: returning_fields.append(field) unique_indices = [returning_fields.index(f) for f in unique_fields] - db_returning_indices = list(enumerate(meta.db_returning_fields)) - # Sort by the conflict key so concurrent upserts touching overlapping - # keys lock rows in the same order and can't deadlock each other. - keyed.sort(key=lambda pair: pair[0]) + # Issue the batches in conflict-key order so concurrent upserts touching + # overlapping keys lock rows in the same order and can't deadlock each + # other. objs itself is untouched, so the caller gets its input order. + sorted_keys = sorted(obj_by_key) with transaction.atomic(savepoint=False): returned_rows = self._batched_insert( - [obj for _, obj in keyed], + [obj_by_key[key] for key in sorted_keys], fields, batch_size, returning_fields=returning_fields, @@ -803,18 +827,19 @@ def bulk_upsert( unique_fields=unique_fields, ) - # RETURNING order isn't guaranteed to match VALUES order under ON - # CONFLICT, so match each returned row to its object by the unique key. - row_by_key = {} - for row in returned_rows: - key = tuple(row[i] for i in unique_indices) - row_by_key[key] = row - for key, obj in keyed: - row = row_by_key[key] - for index, field in db_returning_indices: - assert field.name is not None - setattr(obj, field.name, row[index]) - obj._state.adding = False + # RETURNING order isn't guaranteed to match VALUES order under ON + # CONFLICT, so match each returned row to its object by the unique + # key. Still inside the transaction: a row that can't be matched + # would leave the objects half-populated, so roll the write back. + assert len(returned_rows) == len(obj_by_key) + row_by_key = { + tuple(row[i] for i in unique_indices): row for row in returned_rows + } + for key, obj in obj_by_key.items(): + row = row_by_key[key] + for index, field in enumerate(meta.db_returning_fields): + setattr(obj, field.name, row[index]) + obj._state.adding = False return objs From 9cfa4cc8521c198cf1868b4c8e29ad15d974acb7 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 14:04:06 -0500 Subject: [PATCH 21/41] plain-postgres: cover bulk_upsert's key rules, composite keys, and types UpsertPair is unique on (bucket, slug) and on neither alone, so the composite conflict target is exercised for real: one conflicting row, one sharing a bucket, one sharing a slug, each matched back by the whole key. The rest pin what the previous commit fixed -- duplicate keys, a database-generated unique field, arguments validated on an empty objs list -- plus the returned list's order, which is the caller's order even though the batches are issued sorted. bulk_writes.py adds the static half: bulk_upsert hands back list[T], takes field references rather than strings, and bulk_create no longer has a conflict kwarg to pass. --- .../examples/migrations/0023_upsertpair.py | 21 ++++ .../tests/app/examples/models/upsert.py | 17 ++++ .../tests/public/test_bulk_upsert.py | 95 ++++++++++++++++++- plain-postgres/tests/typing/bulk_writes.py | 49 ++++++++++ 4 files changed, 180 insertions(+), 2 deletions(-) create mode 100644 plain-postgres/tests/app/examples/migrations/0023_upsertpair.py create mode 100644 plain-postgres/tests/typing/bulk_writes.py diff --git a/plain-postgres/tests/app/examples/migrations/0023_upsertpair.py b/plain-postgres/tests/app/examples/migrations/0023_upsertpair.py new file mode 100644 index 0000000000..55fbd897c9 --- /dev/null +++ b/plain-postgres/tests/app/examples/migrations/0023_upsertpair.py @@ -0,0 +1,21 @@ +# Generated by Plain 0.163.1 on 2026-09-19 18:54 + +from plain.postgres import migrations + +from plain import postgres + + +class Migration(migrations.Migration): + dependencies = (("examples", "0022_upsertitem"),) + + operations = ( + migrations.CreateModel( + name="UpsertPair", + fields=[ + ("id", postgres.PrimaryKeyField()), + ("bucket", postgres.TextField(max_length=100)), + ("slug", postgres.TextField(max_length=100)), + ("value", postgres.IntegerField(default=0)), + ], + ), + ) diff --git a/plain-postgres/tests/app/examples/models/upsert.py b/plain-postgres/tests/app/examples/models/upsert.py index 4660682e03..ddf319f279 100644 --- a/plain-postgres/tests/app/examples/models/upsert.py +++ b/plain-postgres/tests/app/examples/models/upsert.py @@ -18,3 +18,20 @@ class UpsertItem(postgres.Model): postgres.UniqueConstraint(fields=["key"], name="upsertitem_key_unique"), ] ) + + +@postgres.register_model +class UpsertPair(postgres.Model): + """Composite conflict key -- unique on (bucket, slug), not on either alone.""" + + bucket: Field[str] = types.TextField(max_length=100) + slug: Field[str] = types.TextField(max_length=100) + value: Field[int] = types.IntegerField(default=0) + + model_options = postgres.Options( + constraints=[ + postgres.UniqueConstraint( + fields=["bucket", "slug"], name="upsertpair_bucket_slug_unique" + ), + ] + ) diff --git a/plain-postgres/tests/public/test_bulk_upsert.py b/plain-postgres/tests/public/test_bulk_upsert.py index cee9f3a94f..7e66b1fb13 100644 --- a/plain-postgres/tests/public/test_bulk_upsert.py +++ b/plain-postgres/tests/public/test_bulk_upsert.py @@ -8,8 +8,10 @@ from __future__ import annotations import pytest +from app.examples.models.defaults import DBDefaultsExample +from app.examples.models.mixins import MixinTestModel from app.examples.models.returning import ReturningEvent -from app.examples.models.upsert import UpsertItem +from app.examples.models.upsert import UpsertItem, UpsertPair from plain.postgres.exceptions import FieldError @@ -77,10 +79,14 @@ def test_bulk_upsert_matches_returned_rows_by_key_not_order(db): UpsertItem(key="a", value=1), UpsertItem(key="b", value=2), ] - UpsertItem.query.bulk_upsert( + returned = UpsertItem.query.bulk_upsert( items, update_fields=[UpsertItem.value], unique_fields=[UpsertItem.key] ) + # Batches are issued in conflict-key order, but the caller gets its own + # order back. + assert [r.key for r in returned] == ["c", "a", "b"] + for item in items: assert item.id == seeded_ids[item.key] @@ -178,3 +184,88 @@ def test_bulk_create_no_longer_accepts_update_conflicts(db): [UpsertItem(key="a", value=1)], **removed_conflict_kwargs, # ty: ignore[invalid-argument-type] ) + + +def test_bulk_upsert_composite_unique_fields(db): + UpsertPair(bucket="b1", slug="s1", value=1).create() + seeded_id = UpsertPair.query.get(bucket="b1", slug="s1").id + + items = [ + UpsertPair(bucket="b1", slug="s1", value=10), # conflicts -> update + UpsertPair(bucket="b1", slug="s2", value=20), # same bucket, new slug + UpsertPair(bucket="b2", slug="s1", value=30), # same slug, new bucket + ] + UpsertPair.query.bulk_upsert( + items, + update_fields=[UpsertPair.value], + unique_fields=[UpsertPair.bucket, UpsertPair.slug], + ) + + # The conflicting row is matched back by the whole composite key. + assert items[0].id == seeded_id + assert len({item.id for item in items}) == 3 + + stored = {(row.bucket, row.slug): row.value for row in UpsertPair.query.all()} + assert stored == {("b1", "s1"): 10, ("b1", "s2"): 20, ("b2", "s1"): 30} + + +def test_bulk_upsert_duplicate_keys_rejected(db): + # Postgres raises a cardinality violation if one statement touches a row + # twice, and splitting duplicates across batches would silently let the + # last one win -- so they are refused up front, whatever the batch size. + with pytest.raises(ValueError, match="more than one UpsertItem"): + UpsertItem.query.bulk_upsert( + [UpsertItem(key="a", value=1), UpsertItem(key="a", value=2)], + update_fields=[UpsertItem.value], + unique_fields=[UpsertItem.key], + batch_size=1, + ) + + assert UpsertItem.query.count() == 0 + + +def test_bulk_upsert_database_generated_unique_field_rejected(db): + # db_uuid is generate=True, so the objects hold a DatabaseDefault sentinel + # rather than a value to conflict on. + with pytest.raises(ValueError, match="the database generates its value"): + DBDefaultsExample.query.bulk_upsert( + [DBDefaultsExample(name="a")], + update_fields=[DBDefaultsExample.name], + unique_fields=[DBDefaultsExample.db_uuid], + ) + + +def test_bulk_upsert_validates_arguments_even_when_empty(db): + # An empty objs list is still a bad call if the fields are wrong. + with pytest.raises(TypeError, match="takes field references, not strings"): + UpsertItem.query.bulk_upsert( + [], + update_fields=["value"], # ty: ignore[invalid-argument-type] + unique_fields=[UpsertItem.key], + ) + + +def test_bulk_upsert_leaves_update_now_alone_unless_named(db): + MixinTestModel(name="a").create() + seeded = MixinTestModel.query.get(name="a") + + renamed = MixinTestModel(name="b") + renamed.id = seeded.id + MixinTestModel.query.bulk_upsert( + [renamed], + update_fields=[MixinTestModel.name], + unique_fields=[MixinTestModel.id], + ) + + row = MixinTestModel.query.get(id=seeded.id) + assert row.name == "b" + # updated_at was not named, so the stored row keeps its old stamp. + assert row.updated_at == seeded.updated_at + + # Name it and the row is refreshed. + MixinTestModel.query.bulk_upsert( + [renamed], + update_fields=[MixinTestModel.name, MixinTestModel.updated_at], + unique_fields=[MixinTestModel.id], + ) + assert MixinTestModel.query.get(id=seeded.id).updated_at > seeded.updated_at diff --git a/plain-postgres/tests/typing/bulk_writes.py b/plain-postgres/tests/typing/bulk_writes.py new file mode 100644 index 0000000000..811f346ddf --- /dev/null +++ b/plain-postgres/tests/typing/bulk_writes.py @@ -0,0 +1,49 @@ +"""`bulk_create()` inserts and `bulk_upsert()` insert-or-updates. + +Both hand back the model instances they were given. `bulk_upsert()` takes field +references for its conflict target and its update columns, and `bulk_create()` +no longer takes a conflict surface at all -- both are static claims. +""" + +from __future__ import annotations + +from typing import assert_type + +from app.examples.models.upsert import UpsertItem + + +def must_accept_bulk_create_as_instances() -> None: + assert_type( + UpsertItem.query.bulk_create([UpsertItem(key="a", value=1)]), + list[UpsertItem], + ) + + +def must_accept_bulk_upsert_as_instances() -> None: + assert_type( + UpsertItem.query.bulk_upsert( + [UpsertItem(key="a", value=1)], + update_fields=[UpsertItem.value], + unique_fields=[UpsertItem.key], + ), + list[UpsertItem], + ) + + +def must_reject_string_field_names() -> None: + # Runtime half: + # tests/public/test_bulk_upsert.py::test_bulk_upsert_string_field_rejected. + UpsertItem.query.bulk_upsert( + [UpsertItem(key="a", value=1)], + update_fields=["value"], # ty: ignore[invalid-argument-type] + unique_fields=[UpsertItem.key], + ) + + +def must_reject_bulk_create_conflict_kwargs() -> None: + # bulk_create is insert-only. Runtime half: tests/public/test_bulk_upsert.py + # ::test_bulk_create_no_longer_accepts_update_conflicts. + UpsertItem.query.bulk_create( + [UpsertItem(key="a", value=1)], + update_conflicts=True, # ty: ignore[unknown-argument] + ) From 1d071f0724c811736eeea5fb33f3c970876173fb Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 14:04:12 -0500 Subject: [PATCH 22/41] plain-postgres: document what bulk_upsert does not refresh The subtlest thing about the API was undocumented: only the named update_fields are written on a conflict, so an update_now column left out keeps its stored value even though the in-memory object gets a fresh stamp. plain-cache had to learn that the hard way and encode it in a comment. The section now also states the returned list's order, that a unique Index is not a conflict target, and that duplicate keys raise. Two plain-cache comments still said bulk_create after set_many moved to bulk_upsert. --- plain-cache/plain/cache/core.py | 2 +- .../tests/internal/test_set_timestamps.py | 2 +- plain-postgres/plain/postgres/README.md | 30 ++++++++++++------- 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/plain-cache/plain/cache/core.py b/plain-cache/plain/cache/core.py index ecb491decc..628f4219e1 100644 --- a/plain-cache/plain/cache/core.py +++ b/plain-cache/plain/cache/core.py @@ -241,7 +241,7 @@ def touch(self, key: str, *, expiration: Expiration = None) -> bool: """ # QuerySet.update() issues a direct SQL UPDATE and does NOT fire pre_save, # so updated_at's update_now won't bump on its own -- stamp it by hand. - # (set_many() relies on pre_save instead, since bulk_create does fire it.) + # (set_many() relies on pre_save instead, since bulk_upsert does fire it.) now = timezone.now() updated = ( self._model.query.live() diff --git a/plain-cache/tests/internal/test_set_timestamps.py b/plain-cache/tests/internal/test_set_timestamps.py index 0ef86fad0a..c1adc54dce 100644 --- a/plain-cache/tests/internal/test_set_timestamps.py +++ b/plain-cache/tests/internal/test_set_timestamps.py @@ -1,6 +1,6 @@ """Timestamp invariants for the set-based write paths. -bulk_create fires pre_save (so updated_at's update_now bumps on its own) while +bulk_upsert fires pre_save (so updated_at's update_now bumps on its own) while QuerySet.update() does not -- see core.py for why set_many stamps created_at and touch stamps updated_at. These pin the observable invariants those choices buy. """ diff --git a/plain-postgres/plain/postgres/README.md b/plain-postgres/plain/postgres/README.md index 95ead9705c..5c2c65c656 100644 --- a/plain-postgres/plain/postgres/README.md +++ b/plain-postgres/plain/postgres/README.md @@ -471,28 +471,38 @@ already exist in a single statement, use `bulk_upsert` (below). `INSERT ... ON CONFLICT (unique_fields) DO UPDATE SET ... RETURNING` per batch. Rows that don't exist yet are inserted; rows that collide on `unique_fields` have their `update_fields` overwritten. Every object comes back — inserted or updated — -with its DB-generated fields (primary key, DB defaults) populated. +with its DB-generated fields (primary key, DB defaults) populated, in the order +you passed them in. ```python # Insert new items, refresh `value`/`expires_at` on any existing key. -CacheItem.query.bulk_upsert( - [CacheItem(key=k, value=v, expires_at=exp) for k, v in items], - update_fields=[CacheItem.value, CacheItem.expires_at], - unique_fields=[CacheItem.key], +CachedItem.query.bulk_upsert( + [CachedItem(key=k, value=v, expires_at=exp) for k, v in items], + update_fields=[CachedItem.value, CachedItem.expires_at], + unique_fields=[CachedItem.key], ) ``` - `update_fields` and `unique_fields` take field references (`Model.field`), not strings. - `unique_fields` must name the **primary key** or a `UniqueConstraint` declared - on the model (no condition, no expressions) — this is the conflict target. + on the model (no condition, no expressions) — this is the conflict target. A + unique `Index` is not enough; declare a `UniqueConstraint`. - `update_fields` must be concrete, non-primary-key, and must not overlap `unique_fields`. - Every object must have a non-null value for every unique field. `NULL` never - conflicts in Postgres, so it can't be upserted. -- Each batch is sorted by the conflict key and matches returned rows back to - objects by that key, so it's safe to run concurrently without deadlocking on - overlapping keys. + conflicts in Postgres, so it can't be upserted. A database-generated column + (`create_now`, `generate=True`, `RandomStringField`) can't be a unique field + either — your objects never hold its value. +- **Two objects with the same unique key raise `ValueError`.** Postgres can only + touch a row once per statement, so collapse duplicates before calling. +- **Only the named `update_fields` are written on a conflict.** An + `update_now=True` column left out of `update_fields` keeps its stored value, + even though the in-memory object gets a fresh stamp — name it in + `update_fields` if you want the row refreshed. +- Batches are issued in conflict-key order and returned rows are matched back to + objects by that key, so concurrent `bulk_upsert` calls over overlapping keys + can't deadlock each other. #### Use queryset `.update()` / `.delete()` for mass operations From 53b2b03d6dd6a3e9d3168325e9068506e3289573 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 14:16:20 -0500 Subject: [PATCH 23/41] plain-postgres: let bulk_upsert own the update_now columns An object whose pre_save stamp disagrees with the row it was just hydrated from is a trap, and "updated whenever the row is updated" is what update_now means. Every update_now column on the model now joins the caller's update_fields in the DO UPDATE SET list, deduplicated, so the stored row and the returned object carry the same stamp. The caller doesn't name them. Two rules fall out of that: - A create_now column can no longer be named in update_fields. EXCLUDED carries a freshly evaluated default, so naming one reset a creation timestamp on every update -- never what anyone meant. Same for generate=True and RandomStringField. A column that is also update_now is exempt, since rewriting it is the point. - An update_now column can no longer be a unique field. It is stamped again on the way in, so the key computed from the object would never match the value Postgres returned -- and auto-including it in the SET list would rewrite the conflict target underneath the match-back. The field-nature checks on unique_fields now run before the constraint check, so the more specific diagnosis wins. --- plain-postgres/plain/postgres/query.py | 54 +++++++++++++++++++------- 1 file changed, 41 insertions(+), 13 deletions(-) diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index 4cfb8bb60a..020e3e290f 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -698,6 +698,21 @@ def _check_bulk_upsert_options( if not unique_fields: raise ValueError("bulk_upsert() requires unique_fields.") + # The conflict key is also what matches each RETURNING row back to its + # object, so it has to be a value the caller holds and the row keeps. + for field in unique_fields: + if field.db_returning and not field.primary_key: + raise ValueError( + f"bulk_upsert() cannot use {object_name}.{field.name} in " + "unique_fields: the database generates its value, so the " + "objects never carry one to conflict on." + ) + if field.auto_fills_on_save: + raise ValueError( + f"bulk_upsert() cannot use {object_name}.{field.name} in " + "unique_fields: it is stamped again on every write, so it " + "can never be a stable conflict key." + ) if not self.model.model_options.unique_fields_match_constraint( {f.name for f in unique_fields} ): @@ -707,16 +722,6 @@ def _check_bulk_upsert_options( "the primary key or a UniqueConstraint declared on the model " "without a condition or expressions." ) - for field in unique_fields: - # Postgres fills in every db_returning column but the primary key - # (create_now, generate=True, RandomStringField), so the objects - # never carry a value to conflict on or to match the row back by. - if field.db_returning and not field.primary_key: - raise ValueError( - f"bulk_upsert() cannot use {object_name}.{field.name} in " - "unique_fields: the database generates its value, so the " - "objects never carry one to conflict on." - ) if not update_fields: raise ValueError("bulk_upsert() requires update_fields.") @@ -724,6 +729,18 @@ def _check_bulk_upsert_options( raise ValueError("bulk_upsert() update_fields must be database columns.") if any(f.primary_key for f in update_fields): raise ValueError("bulk_upsert() cannot update primary key fields.") + for field in update_fields: + # A database-owned value (create_now, generate=True, + # RandomStringField) isn't the caller's to overwrite: EXCLUDED + # carries a freshly evaluated default, so naming one here would + # reset a creation timestamp on every update. A column that is also + # update_now is exempt -- rewriting it is the whole point. + if field.db_returning and not field.auto_fills_on_save: + raise ValueError( + f"bulk_upsert() cannot update {object_name}.{field.name}: " + "the database generates its value, so the update would " + "overwrite the stored one with a fresh default." + ) overlap = {f.name for f in update_fields} & {f.name for f in unique_fields} if overlap: raise ValueError( @@ -750,8 +767,9 @@ def bulk_upsert( (`Model.field`); unique_fields must name the primary key or a UniqueConstraint declared on the model. - Only the named update_fields are written on a conflicting row -- an - update_now column left out of update_fields keeps its stored value. + A conflicting row is written with the named update_fields plus every + update_now column on the model, so the stored row and the returned + object agree on when it was last touched. bulk_upsert() carries its own RETURNING to populate the objects, so a prior returning() has nothing to add and is refused. @@ -796,6 +814,16 @@ def bulk_upsert( ) obj_by_key[key] = obj + # An update_now column is stamped by pre_save on the way in, so the + # object already holds a fresh value whether it inserts or updates. + # Setting it from EXCLUDED on the conflict path too is what keeps the + # stored row and the returned object agreeing -- and it's what + # update_now means. The caller doesn't have to name it. + conflict_update_fields = list(update_fields) + for field in meta.fields: + if field.auto_fills_on_save and field not in conflict_update_fields: + conflict_update_fields.append(field) + # Include the PK column only when it is itself the conflict key; # otherwise let Postgres generate the identity value. pk_is_unique = any(f.primary_key for f in unique_fields) @@ -823,7 +851,7 @@ def bulk_upsert( batch_size, returning_fields=returning_fields, on_conflict=OnConflict.UPDATE, - update_fields=update_fields, + update_fields=conflict_update_fields, unique_fields=unique_fields, ) From 4cf0b960ebf37f457564ab459dc3193370c6175d Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 14:16:27 -0500 Subject: [PATCH 24/41] plain-postgres: pin and document the update_now conflict path The test asserts both halves of the promise: the stored row's stamp advanced, and the object handed back carries that same stamp. Plus the two new refusals -- a create_now column in update_fields, an update_now column in unique_fields. plain.cache no longer names updated_at in update_fields; bulk_upsert refreshes it. Same statement, same columns, same timestamps -- its suite, including the created_at-preserved invariant, is unchanged. --- plain-cache/plain/cache/core.py | 14 +++---- plain-postgres/plain/postgres/README.md | 14 ++++--- .../tests/public/test_bulk_upsert.py | 37 +++++++++++++------ 3 files changed, 41 insertions(+), 24 deletions(-) diff --git a/plain-cache/plain/cache/core.py b/plain-cache/plain/cache/core.py index 628f4219e1..65c48cc048 100644 --- a/plain-cache/plain/cache/core.py +++ b/plain-cache/plain/cache/core.py @@ -106,12 +106,12 @@ def set_many( if not mapping: return - # bulk_upsert fires pre_save, so updated_at's update_now stamps a fresh - # now() at write time on its own. created_at (no update_now) would - # otherwise fall to its DB default, evaluated a hair later -- leaving a - # brand-new row with updated_at < created_at. Stamp created_at from an - # up-front `now` so created_at <= updated_at; it's omitted from - # update_fields, so it's preserved on conflict. + # bulk_upsert fires pre_save and refreshes update_now columns on the + # conflict path, so updated_at looks after itself. created_at (no + # update_now) would otherwise fall to its DB default, evaluated a hair + # later -- leaving a brand-new row with updated_at < created_at. Stamp + # created_at from an up-front `now` so created_at <= updated_at; being + # DB-owned it can't be named in update_fields, so it survives conflicts. now = timezone.now() expires_at = _coerce_expiration(expiration, now=now) items = [] @@ -125,7 +125,7 @@ def set_many( model = self._model model.query.bulk_upsert( items, - update_fields=[model.value, model.expires_at, model.updated_at], + update_fields=[model.value, model.expires_at], unique_fields=[model.key], ) diff --git a/plain-postgres/plain/postgres/README.md b/plain-postgres/plain/postgres/README.md index 5c2c65c656..b645ea2ef8 100644 --- a/plain-postgres/plain/postgres/README.md +++ b/plain-postgres/plain/postgres/README.md @@ -489,17 +489,19 @@ CachedItem.query.bulk_upsert( on the model (no condition, no expressions) — this is the conflict target. A unique `Index` is not enough; declare a `UniqueConstraint`. - `update_fields` must be concrete, non-primary-key, and must not overlap - `unique_fields`. + `unique_fields`. A column the database fills in (`create_now`, + `generate=True`, `RandomStringField`) can't be named either — the update would + overwrite the stored value with a freshly evaluated default. - Every object must have a non-null value for every unique field. `NULL` never conflicts in Postgres, so it can't be upserted. A database-generated column (`create_now`, `generate=True`, `RandomStringField`) can't be a unique field - either — your objects never hold its value. + either — your objects never hold its value. Nor can an `update_now=True` + column, which is stamped again on every write. - **Two objects with the same unique key raise `ValueError`.** Postgres can only touch a row once per statement, so collapse duplicates before calling. -- **Only the named `update_fields` are written on a conflict.** An - `update_now=True` column left out of `update_fields` keeps its stored value, - even though the in-memory object gets a fresh stamp — name it in - `update_fields` if you want the row refreshed. +- **`update_now=True` columns are refreshed on a conflict automatically.** You + don't name them in `update_fields`; a row that gets updated gets a fresh + stamp, and the object handed back carries the same one. - Batches are issued in conflict-key order and returned rows are matched back to objects by that key, so concurrent `bulk_upsert` calls over overlapping keys can't deadlock each other. diff --git a/plain-postgres/tests/public/test_bulk_upsert.py b/plain-postgres/tests/public/test_bulk_upsert.py index 7e66b1fb13..5e685abdde 100644 --- a/plain-postgres/tests/public/test_bulk_upsert.py +++ b/plain-postgres/tests/public/test_bulk_upsert.py @@ -245,7 +245,7 @@ def test_bulk_upsert_validates_arguments_even_when_empty(db): ) -def test_bulk_upsert_leaves_update_now_alone_unless_named(db): +def test_bulk_upsert_refreshes_update_now_columns_without_naming_them(db): MixinTestModel(name="a").create() seeded = MixinTestModel.query.get(name="a") @@ -253,19 +253,34 @@ def test_bulk_upsert_leaves_update_now_alone_unless_named(db): renamed.id = seeded.id MixinTestModel.query.bulk_upsert( [renamed], - update_fields=[MixinTestModel.name], + update_fields=[MixinTestModel.name], # updated_at deliberately absent unique_fields=[MixinTestModel.id], ) row = MixinTestModel.query.get(id=seeded.id) assert row.name == "b" - # updated_at was not named, so the stored row keeps its old stamp. - assert row.updated_at == seeded.updated_at + # The row was updated, so its update_now column was too. + assert row.updated_at > seeded.updated_at + # And the object handed back agrees with the row it was hydrated from -- + # pre_save stamps the object, EXCLUDED carries that same stamp to the row. + assert renamed.updated_at == row.updated_at - # Name it and the row is refreshed. - MixinTestModel.query.bulk_upsert( - [renamed], - update_fields=[MixinTestModel.name, MixinTestModel.updated_at], - unique_fields=[MixinTestModel.id], - ) - assert MixinTestModel.query.get(id=seeded.id).updated_at > seeded.updated_at + +def test_bulk_upsert_cannot_update_a_database_generated_column(db): + # created_at is create_now-only: EXCLUDED would carry a fresh now() and + # reset the creation timestamp on every update. + with pytest.raises(ValueError, match="the database generates its value"): + MixinTestModel.query.bulk_upsert( + [MixinTestModel(name="a")], + update_fields=[MixinTestModel.created_at], + unique_fields=[MixinTestModel.id], + ) + + +def test_bulk_upsert_update_now_unique_field_rejected(db): + with pytest.raises(ValueError, match="stamped again on every write"): + MixinTestModel.query.bulk_upsert( + [MixinTestModel(name="a")], + update_fields=[MixinTestModel.name], + unique_fields=[MixinTestModel.updated_at], + ) From e4eea990a746ad15399dc53c8b2d16987d15567b Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 14:32:08 -0500 Subject: [PATCH 25/41] plain-postgres: let bulk_upsert name a foreign key column Model.tenant is a ForwardForeignKeyDescriptor, not a Field, and to the checker it is type[Tenant] -- that is what makes where() traversal work, and .tenant.id traverses to the *related* model's column rather than the foreign key. So there was no way to write unique_fields=[Model.tenant, Model.slug] against UniqueConstraint(fields=["tenant", "slug"]): it raised TypeError at the call. bulk_upsert's two column lists now take the descriptor and unwrap it to the column it stands for. _validate_field_refs does the unwrapping and returns the resolved columns, so _check_bulk_upsert_options becomes _resolve_bulk_upsert_fields and hands both resolved lists back. The element type widens to Field[Any] | type[Model]. returning() is untouched: it refuses Model.fk before it ever reaches _validate_field_refs, because there the reference is ambiguous with asking for the whole related object. A column list can only mean the column, so here it isn't. The migration also carries the UpsertValueKey fixture the next two commits need -- one CreateModel batch rather than three. --- plain-postgres/plain/postgres/README.md | 6 +- plain-postgres/plain/postgres/query.py | 72 +++++++++++++------ ...pserttenant_upsertvaluekey_upsertscoped.py | 43 +++++++++++ .../tests/app/examples/models/upsert.py | 47 ++++++++++++ .../tests/public/test_bulk_upsert.py | 56 ++++++++++++++- plain-postgres/tests/typing/bulk_writes.py | 19 ++++- 6 files changed, 217 insertions(+), 26 deletions(-) create mode 100644 plain-postgres/tests/app/examples/migrations/0024_upserttenant_upsertvaluekey_upsertscoped.py diff --git a/plain-postgres/plain/postgres/README.md b/plain-postgres/plain/postgres/README.md index b645ea2ef8..02c14a12e1 100644 --- a/plain-postgres/plain/postgres/README.md +++ b/plain-postgres/plain/postgres/README.md @@ -484,7 +484,11 @@ CachedItem.query.bulk_upsert( ``` - `update_fields` and `unique_fields` take field references (`Model.field`), not - strings. + strings. A foreign key is named by the relation itself — `Model.tenant`, which + resolves to the `tenant_id` column. (This is the one write API that takes + `Model.fk`. `returning()` refuses it, because there it would be ambiguous with + asking for the whole related object; here a column list can only mean the + column.) - `unique_fields` must name the **primary key** or a `UniqueConstraint` declared on the model (no condition, no expressions) — this is the conflict target. A unique `Index` is not enough; declare a `UniqueConstraint`. diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index 020e3e290f..336c1fb514 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -53,6 +53,7 @@ if TYPE_CHECKING: from plain.postgres import Model + # The maximum number of results to fetch in a get() query. MAX_GET_RESULTS = 21 @@ -686,15 +687,22 @@ def bulk_create( return objs - def _check_bulk_upsert_options( + def _resolve_bulk_upsert_fields( self, - update_fields: list[Field], - unique_fields: list[Field], - ) -> None: + update_fields: Sequence[Any], + unique_fields: Sequence[Any], + ) -> tuple[list[Field], list[Field]]: + """Check both bulk_upsert() field lists and return the columns they + name, with any `Model.fk` reference resolved to its foreign key + column.""" object_name = self.model.model_options.object_name - self._validate_field_refs(unique_fields, where="bulk_upsert() unique_fields") - self._validate_field_refs(update_fields, where="bulk_upsert() update_fields") + unique_fields = self._validate_field_refs( + unique_fields, where="bulk_upsert() unique_fields" + ) + update_fields = self._validate_field_refs( + update_fields, where="bulk_upsert() update_fields" + ) if not unique_fields: raise ValueError("bulk_upsert() requires unique_fields.") @@ -748,12 +756,14 @@ def _check_bulk_upsert_options( f"{sorted(overlap)}." ) + return update_fields, unique_fields + def bulk_upsert( self, objs: Sequence[T], *, - update_fields: list[Field], - unique_fields: list[Field], + update_fields: list[Field[Any] | type[Model]], + unique_fields: list[Field[Any] | type[Model]], batch_size: int | None = None, ) -> list[T]: """ @@ -778,7 +788,9 @@ def bulk_upsert( if batch_size is not None and batch_size <= 0: raise ValueError("Batch size must be a positive integer.") - self._check_bulk_upsert_options(update_fields, unique_fields) + update_columns, unique_columns = self._resolve_bulk_upsert_fields( + update_fields, unique_fields + ) objs = list(objs) if not objs: @@ -795,7 +807,7 @@ def bulk_upsert( obj_by_key: dict[tuple[Any, ...], T] = {} for obj in objs: key_values = [] - for field in unique_fields: + for field in unique_columns: value = field.value_from_object(obj) if value is None: raise ValueError( @@ -806,7 +818,7 @@ def bulk_upsert( key_values.append(value) key = tuple(key_values) if key in obj_by_key: - names = [f.name for f in unique_fields] + names = [f.name for f in unique_columns] raise ValueError( f"bulk_upsert() got more than one {object_name} with " f"{names} = {list(key)}. Postgres can only touch a row once " @@ -819,14 +831,14 @@ def bulk_upsert( # Setting it from EXCLUDED on the conflict path too is what keeps the # stored row and the returned object agreeing -- and it's what # update_now means. The caller doesn't have to name it. - conflict_update_fields = list(update_fields) + conflict_update_columns = list(update_columns) for field in meta.fields: - if field.auto_fills_on_save and field not in conflict_update_fields: - conflict_update_fields.append(field) + if field.auto_fills_on_save and field not in conflict_update_columns: + conflict_update_columns.append(field) # Include the PK column only when it is itself the conflict key; # otherwise let Postgres generate the identity value. - pk_is_unique = any(f.primary_key for f in unique_fields) + pk_is_unique = any(f.primary_key for f in unique_columns) fields = meta.fields if not pk_is_unique: fields = [f for f in fields if not isinstance(f, PrimaryKeyField)] @@ -834,10 +846,10 @@ def bulk_upsert( # RETURNING must carry the DB-returned fields (to populate the objects) # plus the unique fields (to match each returned row to its object). returning_fields = list(meta.db_returning_fields) - for field in unique_fields: + for field in unique_columns: if field not in returning_fields: returning_fields.append(field) - unique_indices = [returning_fields.index(f) for f in unique_fields] + unique_indices = [returning_fields.index(f) for f in unique_columns] # Issue the batches in conflict-key order so concurrent upserts touching # overlapping keys lock rows in the same order and can't deadlock each @@ -851,8 +863,8 @@ def bulk_upsert( batch_size, returning_fields=returning_fields, on_conflict=OnConflict.UPDATE, - update_fields=conflict_update_fields, - unique_fields=unique_fields, + update_fields=conflict_update_columns, + unique_fields=unique_columns, ) # RETURNING order isn't guaranteed to match VALUES order under ON @@ -1085,14 +1097,30 @@ def returning(self, *fields: Field[Any]) -> ReturningQuerySet[T, Any]: clone._returning_instances = True return cast("ReturningQuerySet[T, Any]", clone) - def _validate_field_refs(self, fields: Sequence[Any], *, where: str) -> None: - """Require each item to be a Field reference on this queryset's model. + def _validate_field_refs(self, fields: Sequence[Any], *, where: str) -> list[Field]: + """Require each item to be a Field reference on this queryset's model, + and return the columns those references name. + + `Model.fk` is a ForwardForeignKeyDescriptor rather than a Field -- that + is what lets where() traverse to the related model -- so there is no + other way to name the foreign key column. The write APIs that take + column lists unwrap it here. returning() is the exception and refuses + it outright, which it does before calling this (see + _validated_returning_fields). `where` names the call in the error (e.g. "returning()", "bulk_upsert() unique_fields") so a bad argument points the user at Model.field. """ + # Local import: related_descriptors imports this module at load time. + from plain.postgres.fields.related_descriptors import ( + ForwardForeignKeyDescriptor, + ) + object_name = self.model.model_options.object_name + columns = [] for field in fields: + if isinstance(field, ForwardForeignKeyDescriptor): + field = field._field if isinstance(field, str): raise TypeError( f"{where} takes field references, not strings. " @@ -1109,6 +1137,8 @@ def _validate_field_refs(self, fields: Sequence[Any], *, where: str) -> None: f"{field.name}: it belongs to a different model, not " f"{object_name}." ) + columns.append(field) + return columns def _validated_returning_fields( self, fields: tuple[Field[Any], ...] diff --git a/plain-postgres/tests/app/examples/migrations/0024_upserttenant_upsertvaluekey_upsertscoped.py b/plain-postgres/tests/app/examples/migrations/0024_upserttenant_upsertvaluekey_upsertscoped.py new file mode 100644 index 0000000000..8ea4ec1377 --- /dev/null +++ b/plain-postgres/tests/app/examples/migrations/0024_upserttenant_upsertvaluekey_upsertscoped.py @@ -0,0 +1,43 @@ +# Generated by Plain 0.163.1 on 2026-09-19 19:26 + +from plain.postgres import migrations + +from plain import postgres + + +class Migration(migrations.Migration): + dependencies = (("examples", "0023_upsertpair"),) + + operations = ( + migrations.CreateModel( + name="UpsertTenant", + fields=[ + ("id", postgres.PrimaryKeyField()), + ("name", postgres.TextField(max_length=100)), + ], + ), + migrations.CreateModel( + name="UpsertValueKey", + fields=[ + ("id", postgres.PrimaryKeyField()), + ("blob", postgres.BinaryField()), + ("payload", postgres.JSONField()), + ("value", postgres.IntegerField(default=0)), + ("zone", postgres.TimeZoneField()), + ], + ), + migrations.CreateModel( + name="UpsertScoped", + fields=[ + ("id", postgres.PrimaryKeyField()), + ("slug", postgres.TextField(max_length=100)), + ("value", postgres.IntegerField(default=0)), + ( + "tenant", + postgres.ForeignKeyField( + on_delete=postgres.CASCADE, to="examples.upserttenant" + ), + ), + ], + ), + ) diff --git a/plain-postgres/tests/app/examples/models/upsert.py b/plain-postgres/tests/app/examples/models/upsert.py index ddf319f279..5345b7bb20 100644 --- a/plain-postgres/tests/app/examples/models/upsert.py +++ b/plain-postgres/tests/app/examples/models/upsert.py @@ -2,6 +2,8 @@ from __future__ import annotations +from zoneinfo import ZoneInfo + from plain.postgres import Field, types from plain import postgres @@ -35,3 +37,48 @@ class UpsertPair(postgres.Model): ), ] ) + + +@postgres.register_model +class UpsertTenant(postgres.Model): + name: Field[str] = types.TextField(max_length=100) + + +@postgres.register_model +class UpsertScoped(postgres.Model): + """A foreign key as part of the conflict key, and as an updated column.""" + + tenant: Field[UpsertTenant] = types.ForeignKeyField( + UpsertTenant, on_delete=postgres.CASCADE + ) + slug: Field[str] = types.TextField(max_length=100) + value: Field[int] = types.IntegerField(default=0) + + model_options = postgres.Options( + constraints=[ + postgres.UniqueConstraint( + fields=["tenant", "slug"], name="upsertscoped_tenant_slug_unique" + ), + ] + ) + + +@postgres.register_model +class UpsertValueKey(postgres.Model): + """A composite conflict key of column types whose Python values are + unhashable (jsonb dicts), unorderable (ZoneInfo), or neither (memoryview). + """ + + payload: Field[object] = types.JSONField() + blob: Field[bytes | memoryview] = types.BinaryField() + zone: Field[ZoneInfo] = types.TimeZoneField() + value: Field[int] = types.IntegerField(default=0) + + model_options = postgres.Options( + constraints=[ + postgres.UniqueConstraint( + fields=["payload", "blob", "zone"], + name="upsertvaluekey_payload_blob_zone_unique", + ), + ] + ) diff --git a/plain-postgres/tests/public/test_bulk_upsert.py b/plain-postgres/tests/public/test_bulk_upsert.py index 5e685abdde..87481a40a4 100644 --- a/plain-postgres/tests/public/test_bulk_upsert.py +++ b/plain-postgres/tests/public/test_bulk_upsert.py @@ -11,7 +11,12 @@ from app.examples.models.defaults import DBDefaultsExample from app.examples.models.mixins import MixinTestModel from app.examples.models.returning import ReturningEvent -from app.examples.models.upsert import UpsertItem, UpsertPair +from app.examples.models.upsert import ( + UpsertItem, + UpsertPair, + UpsertScoped, + UpsertTenant, +) from plain.postgres.exceptions import FieldError @@ -284,3 +289,52 @@ def test_bulk_upsert_update_now_unique_field_rejected(db): update_fields=[MixinTestModel.name], unique_fields=[MixinTestModel.updated_at], ) + + +def test_bulk_upsert_foreign_key_in_unique_fields(db): + # Model.fk is a relation descriptor, not a Field, but it is the only way to + # name the foreign key column -- so the write-API field lists accept it. + tenant = UpsertTenant(name="t1") + tenant.create() + tenant = UpsertTenant.query.get(name="t1") + UpsertScoped(tenant=tenant, slug="a", value=1).create() + seeded_id = UpsertScoped.query.get(slug="a").id + + items = [ + UpsertScoped(tenant=tenant, slug="a", value=10), # conflicts -> update + UpsertScoped(tenant=tenant, slug="b", value=20), # new -> insert + ] + UpsertScoped.query.bulk_upsert( + items, + update_fields=[UpsertScoped.value], + unique_fields=[UpsertScoped.tenant, UpsertScoped.slug], + ) + + assert items[0].id == seeded_id + assert items[1].id != seeded_id + stored = {row.slug: row.value for row in UpsertScoped.query.all()} + assert stored == {"a": 10, "b": 20} + + +def test_bulk_upsert_foreign_key_in_update_fields(db): + first = UpsertTenant(name="t1") + first.create() + second = UpsertTenant(name="t2") + second.create() + first = UpsertTenant.query.get(name="t1") + second = UpsertTenant.query.get(name="t2") + + UpsertScoped(tenant=first, slug="a", value=1).create() + seeded_id = UpsertScoped.query.get(slug="a").id + + moved = UpsertScoped(tenant=second, slug="a", value=2) + moved.id = seeded_id + UpsertScoped.query.bulk_upsert( + [moved], + update_fields=[UpsertScoped.tenant, UpsertScoped.value], + unique_fields=[UpsertScoped.id], + ) + + row = UpsertScoped.query.get(id=seeded_id) + assert row.tenant.id == second.id + assert row.value == 2 diff --git a/plain-postgres/tests/typing/bulk_writes.py b/plain-postgres/tests/typing/bulk_writes.py index 811f346ddf..d9a71b05ef 100644 --- a/plain-postgres/tests/typing/bulk_writes.py +++ b/plain-postgres/tests/typing/bulk_writes.py @@ -1,15 +1,16 @@ """`bulk_create()` inserts and `bulk_upsert()` insert-or-updates. Both hand back the model instances they were given. `bulk_upsert()` takes field -references for its conflict target and its update columns, and `bulk_create()` -no longer takes a conflict surface at all -- both are static claims. +references for its conflict target and its update columns -- including a +`Model.fk` reference, which types as the related model class rather than a +Field -- and `bulk_create()` no longer takes a conflict surface at all. """ from __future__ import annotations from typing import assert_type -from app.examples.models.upsert import UpsertItem +from app.examples.models.upsert import UpsertItem, UpsertScoped, UpsertTenant def must_accept_bulk_create_as_instances() -> None: @@ -47,3 +48,15 @@ def must_reject_bulk_create_conflict_kwargs() -> None: [UpsertItem(key="a", value=1)], update_conflicts=True, # ty: ignore[unknown-argument] ) + + +def must_accept_a_foreign_key_reference() -> None: + # Model.fk types as the related model class, not a Field, so the element + # type of these lists is the union. Runtime half: + # tests/public/test_bulk_upsert.py + # ::test_bulk_upsert_foreign_key_in_unique_fields. + UpsertScoped.query.bulk_upsert( + [UpsertScoped(tenant=UpsertTenant(name="t"), slug="s", value=1)], + update_fields=[UpsertScoped.value], + unique_fields=[UpsertScoped.tenant, UpsertScoped.slug], + ) From fd41ff1b8be46cec5bde0733b615da9282ded547 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 14:32:27 -0500 Subject: [PATCH 26/41] plain-postgres: build bulk_upsert conflict keys from prepared values The key was the raw Python attribute on one side and whatever psycopg handed back on the other, and it had to survive being a dict key. A jsonb column killed it outright -- 'cannot use tuple as a dict key (unhashable type: dict)' -- and where it didn't crash it could still mismatch, since a column's two sides need not be spelled the same: a bytea comes back as a memoryview, a TimeZoneField as a ZoneInfo, a naive datetime as an aware one. conflict_key_value() now runs over both sides. Preparing the value with the field does most of the work (ZoneInfo to its name, naive datetime to aware, UUID string to UUID); a memoryview then becomes bytes and a jsonb container becomes a canonical JSON string, which also makes two equal objects built with their keys in either order the same key. --- plain-postgres/plain/postgres/query.py | 35 ++++++++++++++++--- .../tests/public/test_bulk_upsert.py | 33 +++++++++++++++++ 2 files changed, 64 insertions(+), 4 deletions(-) diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index 336c1fb514..093d9f9fe6 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -5,6 +5,7 @@ from __future__ import annotations import copy +import json import operator import warnings from collections.abc import Callable, Iterator, Sequence @@ -54,6 +55,26 @@ from plain.postgres import Model +def conflict_key_value(field: Field, value: Any) -> Any: + """One component of a bulk_upsert() conflict key, in a form that hashes. + + The same function runs over both sides of the match -- the value taken off + the object and the value Postgres handed back -- so however a column spells + its Python value on each side, the two meet in the same place. Preparing + the value is most of that: a TimeZoneField's ZoneInfo becomes its name, a + naive datetime becomes aware, a UUID string becomes a UUID. + """ + value = field.get_prep_value(value) + if isinstance(value, memoryview | bytearray): + # psycopg hands a bytea column back as a memoryview. + return bytes(value) + if isinstance(value, dict | list): + # A jsonb container is unhashable, and two equal objects can be built + # with their keys in either order. + return json.dumps(value, sort_keys=True, default=str) + return value + + # The maximum number of results to fetch in a get() query. MAX_GET_RESULTS = 21 @@ -815,7 +836,7 @@ def bulk_upsert( "object; NULL never conflicts in Postgres, so it cannot " "be upserted." ) - key_values.append(value) + key_values.append(conflict_key_value(field, value)) key = tuple(key_values) if key in obj_by_key: names = [f.name for f in unique_columns] @@ -869,11 +890,17 @@ def bulk_upsert( # RETURNING order isn't guaranteed to match VALUES order under ON # CONFLICT, so match each returned row to its object by the unique - # key. Still inside the transaction: a row that can't be matched - # would leave the objects half-populated, so roll the write back. + # key -- run through conflict_key_value() again so both sides of the + # match are spelled the same way. Still inside the transaction: a + # row that can't be matched would leave the objects half-populated, + # so roll the write back. assert len(returned_rows) == len(obj_by_key) row_by_key = { - tuple(row[i] for i in unique_indices): row for row in returned_rows + tuple( + conflict_key_value(field, row[index]) + for field, index in zip(unique_columns, unique_indices) + ): row + for row in returned_rows } for key, obj in obj_by_key.items(): row = row_by_key[key] diff --git a/plain-postgres/tests/public/test_bulk_upsert.py b/plain-postgres/tests/public/test_bulk_upsert.py index 87481a40a4..62684b52c2 100644 --- a/plain-postgres/tests/public/test_bulk_upsert.py +++ b/plain-postgres/tests/public/test_bulk_upsert.py @@ -7,6 +7,8 @@ from __future__ import annotations +from zoneinfo import ZoneInfo + import pytest from app.examples.models.defaults import DBDefaultsExample from app.examples.models.mixins import MixinTestModel @@ -16,6 +18,7 @@ UpsertPair, UpsertScoped, UpsertTenant, + UpsertValueKey, ) from plain.postgres.exceptions import FieldError @@ -338,3 +341,33 @@ def test_bulk_upsert_foreign_key_in_update_fields(db): row = UpsertScoped.query.get(id=seeded_id) assert row.tenant.id == second.id assert row.value == 2 + + +def test_bulk_upsert_matches_keys_that_python_cannot_hash_or_sort(db): + # jsonb dicts are unhashable, ZoneInfo and memoryview are unorderable -- + # the conflict key has to survive being a dict key and being sorted. + chicago = ZoneInfo("America/Chicago") + utc = ZoneInfo("UTC") + UpsertValueKey(payload={"a": 1}, blob=b"x", zone=utc, value=1).create() + seeded_id = UpsertValueKey.query.get(value=1).id + + items = [ + # Conflicts: same key, written with its dict keys in the other order. + UpsertValueKey(payload={"a": 1}, blob=b"x", zone=utc, value=10), + UpsertValueKey(payload={"b": 2, "a": 1}, blob=b"y", zone=chicago, value=20), + UpsertValueKey(payload={"a": 1, "b": 2}, blob=b"z", zone=chicago, value=30), + ] + UpsertValueKey.query.bulk_upsert( + items, + update_fields=[UpsertValueKey.value], + unique_fields=[ + UpsertValueKey.payload, + UpsertValueKey.blob, + UpsertValueKey.zone, + ], + ) + + assert items[0].id == seeded_id + assert len({item.id for item in items}) == 3 + stored = {row.value for row in UpsertValueKey.query.all()} + assert stored == {10, 20, 30} From 419c49e5f82e9b9301781baedc931dd7a341b4e6 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 14:33:29 -0500 Subject: [PATCH 27/41] plain-postgres: order bulk_upsert batches by values that compare Sorting the conflict keys raw meant sorting whatever Python objects the columns held, and plenty of them have no ordering at all -- a ZoneInfo or a memoryview raises TypeError on '<'. Preparing the values (previous commit) fixes most of that, but one jsonb column can still hold an object in one row and a number in the next, which land as a string and an int and don't compare either. conflict_key_order() leads each component with its type's name, so every comparison happens inside a single type. The deadlock ordering only has to be the *same* order for every caller, not a meaningful one, so that is enough -- and it can't raise. Keys with a None component never reach it; those are refused as conflict keys before the sort, since NULL never conflicts in Postgres. --- plain-postgres/plain/postgres/query.py | 13 +++++++++- .../tests/public/test_bulk_upsert.py | 24 +++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index 093d9f9fe6..356f7703f9 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -75,6 +75,17 @@ def conflict_key_value(field: Field, value: Any) -> Any: return value +def conflict_key_order(key: tuple[Any, ...]) -> tuple[tuple[str, Any], ...]: + """The sort key for a bulk_upsert() conflict key. + + The deadlock ordering only has to be the *same* order for every caller, not + a meaningful one, and one column can hold values that don't compare to each + other -- a jsonb column with an object in one row and a number in the next. + Leading with the type's name keeps every comparison inside a single type. + """ + return tuple((type(value).__name__, value) for value in key) + + # The maximum number of results to fetch in a get() query. MAX_GET_RESULTS = 21 @@ -875,7 +886,7 @@ def bulk_upsert( # Issue the batches in conflict-key order so concurrent upserts touching # overlapping keys lock rows in the same order and can't deadlock each # other. objs itself is untouched, so the caller gets its input order. - sorted_keys = sorted(obj_by_key) + sorted_keys = sorted(obj_by_key, key=conflict_key_order) with transaction.atomic(savepoint=False): returned_rows = self._batched_insert( diff --git a/plain-postgres/tests/public/test_bulk_upsert.py b/plain-postgres/tests/public/test_bulk_upsert.py index 62684b52c2..a5c62bdd97 100644 --- a/plain-postgres/tests/public/test_bulk_upsert.py +++ b/plain-postgres/tests/public/test_bulk_upsert.py @@ -371,3 +371,27 @@ def test_bulk_upsert_matches_keys_that_python_cannot_hash_or_sort(db): assert len({item.id for item in items}) == 3 stored = {row.value for row in UpsertValueKey.query.all()} assert stored == {10, 20, 30} + + +def test_bulk_upsert_key_values_that_do_not_compare_to_each_other(db): + # A jsonb conflict key can hold an object in one row and a number in the + # next. Those don't compare, and the batches still have to be ordered. + utc = ZoneInfo("UTC") + items = [ + UpsertValueKey(payload={"a": 1}, blob=b"x", zone=utc, value=1), + UpsertValueKey(payload=7, blob=b"x", zone=utc, value=2), + UpsertValueKey(payload="s", blob=b"x", zone=utc, value=3), + UpsertValueKey(payload=[1, 2], blob=b"x", zone=utc, value=4), + ] + UpsertValueKey.query.bulk_upsert( + items, + update_fields=[UpsertValueKey.value], + unique_fields=[ + UpsertValueKey.payload, + UpsertValueKey.blob, + UpsertValueKey.zone, + ], + ) + + assert len({item.id for item in items}) == 4 + assert {row.value for row in UpsertValueKey.query.all()} == {1, 2, 3, 4} From cf3e8ee9a465b54d7e6c7db686d30ebb6af9e418 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 14:47:17 -0500 Subject: [PATCH 28/41] plain-postgres: canonicalize a JSON conflict key the way the column does jsonb object keys are always strings -- the encoder stringifies an int key on the way in, so {1: "a", "2": "b"} is a perfectly good value to store. Canonicalizing it with json.dumps(sort_keys=True) first, though, sorted the keys while one was still an int, and raised '<' not supported between instances of 'str' and 'int'. Encode with the field's own encoder (the same get_json_dumps() the write path uses), then re-parse and dump with the keys sorted -- by then every key is a string. That also drops the default=str fallback, which could quietly conflate two values the column would have kept apart, and the value-type check it replaces: a jsonb column now keys as a string whatever it holds, scalars included, so the branch belongs to the field type rather than to whatever Python object turned up. --- plain-postgres/plain/postgres/query.py | 16 ++++++--- .../tests/public/test_bulk_upsert.py | 36 +++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index 356f7703f9..17f97bec53 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -22,6 +22,7 @@ PLAIN_VERSION_PICKLE_KEY, get_connection, ) +from plain.postgres.dialect import get_json_dumps from plain.postgres.exceptions import ( FieldDoesNotExist, FieldError, @@ -33,6 +34,7 @@ PrimaryKeyField, ) from plain.postgres.fields.base import ColumnField +from plain.postgres.fields.json import JSONField from plain.postgres.functions import Cast from plain.postgres.query_utils import Q from plain.postgres.sql import ( @@ -65,13 +67,19 @@ def conflict_key_value(field: Field, value: Any) -> Any: naive datetime becomes aware, a UUID string becomes a UUID. """ value = field.get_prep_value(value) + if isinstance(field, JSONField): + # Canonicalize the value the way the column will store it: encode with + # the field's own encoder, which stringifies non-string object keys, + # and only then re-parse and dump with the keys sorted. Sorting before + # the encode would compare an int key against a str one and raise. A + # jsonb column keys as a string either way, scalars included, which is + # also what makes an otherwise unhashable object usable as a key. + return json.dumps( + json.loads(get_json_dumps(field.encoder)(value)), sort_keys=True + ) if isinstance(value, memoryview | bytearray): # psycopg hands a bytea column back as a memoryview. return bytes(value) - if isinstance(value, dict | list): - # A jsonb container is unhashable, and two equal objects can be built - # with their keys in either order. - return json.dumps(value, sort_keys=True, default=str) return value diff --git a/plain-postgres/tests/public/test_bulk_upsert.py b/plain-postgres/tests/public/test_bulk_upsert.py index a5c62bdd97..23fd40ecc2 100644 --- a/plain-postgres/tests/public/test_bulk_upsert.py +++ b/plain-postgres/tests/public/test_bulk_upsert.py @@ -395,3 +395,39 @@ def test_bulk_upsert_key_values_that_do_not_compare_to_each_other(db): assert len({item.id for item in items}) == 4 assert {row.value for row in UpsertValueKey.query.all()} == {1, 2, 3, 4} + + +def test_bulk_upsert_json_key_with_non_string_object_keys(db): + # jsonb object keys are always strings -- the encoder stringifies an int + # key on the way in. The conflict key has to be canonicalized the same + # way, or sorting the keys compares an int against a str and raises. + utc = ZoneInfo("UTC") + UpsertValueKey( + payload={1: "a", "2": "b", "nested": {"z": 1, "y": 2}}, + blob=b"x", + zone=utc, + value=1, + ).create() + seeded_id = UpsertValueKey.query.get(value=1).id + + # The same logical key, written with its keys in another order and the + # int key spelled as a string. + conflicting = UpsertValueKey( + payload={"nested": {"y": 2, "z": 1}, "2": "b", "1": "a"}, + blob=b"x", + zone=utc, + value=99, + ) + UpsertValueKey.query.bulk_upsert( + [conflicting], + update_fields=[UpsertValueKey.value], + unique_fields=[ + UpsertValueKey.payload, + UpsertValueKey.blob, + UpsertValueKey.zone, + ], + ) + + assert conflicting.id == seeded_id + assert UpsertValueKey.query.count() == 1 + assert UpsertValueKey.query.get(id=seeded_id).value == 99 From 086869bd2c8f075bc6a9c30bdc192fadc535b8c5 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 14:55:40 -0500 Subject: [PATCH 29/41] plain-postgres: give a NaN conflict key a value Python can match Postgres holds NaN equal to NaN for uniqueness, so a NaN is a legitimate conflict key. Python disagrees on both counts a key needs: NaN != NaN, and since 3.10 every NaN hashes by identity. So a NaN key raised KeyError: (nan,) matching its own row back, and two NaN keys in one call walked past the duplicate check and hit a Postgres cardinality violation instead. Decimal('NaN') is worse -- it also raises InvalidOperation when sorted. Both now canonicalize to one sentinel, on the input side and on the returned row, so a NaN key round-trips and two of them are duplicates. The docstring records the rest of a read-through for value classes that could break hashing, ordering or equality. Decimal("1.0") vs Decimal("1.00"), -0.0 vs 0.0, and two aware datetimes naming the same instant in different zones all already compare and hash equal in Python, and Postgres collapses each pair to one row too -- so the two agree with no help. bytes and str never meet, because a key component comes from one column. True and 1 are equal but land in different type buckets in conflict_key_order, which orders rather than raises. --- plain-postgres/plain/postgres/query.py | 21 ++++++++++ .../migrations/0025_upsertfloatkey.py | 20 ++++++++++ .../tests/app/examples/models/upsert.py | 16 ++++++++ .../tests/public/test_bulk_upsert.py | 39 +++++++++++++++++++ 4 files changed, 96 insertions(+) create mode 100644 plain-postgres/tests/app/examples/migrations/0025_upsertfloatkey.py diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index 17f97bec53..d6cb909885 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -6,9 +6,11 @@ import copy import json +import math import operator import warnings from collections.abc import Callable, Iterator, Sequence +from decimal import Decimal from functools import cached_property from itertools import islice from typing import TYPE_CHECKING, Any, Never, Self, cast, overload @@ -57,6 +59,11 @@ from plain.postgres import Model +# What a NaN conflict key normalizes to. Any value a float or numeric column +# can otherwise hold is a number, so a string can't collide with one. +NAN_CONFLICT_KEY = "" + + def conflict_key_value(field: Field, value: Any) -> Any: """One component of a bulk_upsert() conflict key, in a form that hashes. @@ -65,6 +72,13 @@ def conflict_key_value(field: Field, value: Any) -> Any: its Python value on each side, the two meet in the same place. Preparing the value is most of that: a TimeZoneField's ZoneInfo becomes its name, a naive datetime becomes aware, a UUID string becomes a UUID. + + Values that already compare and hash alike need no help here, and several + near-misses turn out to be in that group: Decimal("1.0") and + Decimal("1.00"), -0.0 and 0.0, and two aware datetimes naming the same + instant in different zones. Postgres collapses each of those pairs to one + row as well, so Python and the database agree without being told to. NaN is + the one place they disagree. """ value = field.get_prep_value(value) if isinstance(field, JSONField): @@ -80,6 +94,13 @@ def conflict_key_value(field: Field, value: Any) -> Any: if isinstance(value, memoryview | bytearray): # psycopg hands a bytea column back as a memoryview. return bytes(value) + if (isinstance(value, float) and math.isnan(value)) or ( + isinstance(value, Decimal) and value.is_nan() + ): + # Postgres holds NaN equal to NaN for uniqueness. Python doesn't, and + # every NaN hashes differently, so a NaN key would never match its own + # row back and two of them would walk past the duplicate check. + return NAN_CONFLICT_KEY return value diff --git a/plain-postgres/tests/app/examples/migrations/0025_upsertfloatkey.py b/plain-postgres/tests/app/examples/migrations/0025_upsertfloatkey.py new file mode 100644 index 0000000000..f7193c9a41 --- /dev/null +++ b/plain-postgres/tests/app/examples/migrations/0025_upsertfloatkey.py @@ -0,0 +1,20 @@ +# Generated by Plain 0.163.1 on 2026-09-19 19:44 + +from plain.postgres import migrations + +from plain import postgres + + +class Migration(migrations.Migration): + dependencies = (("examples", "0024_upserttenant_upsertvaluekey_upsertscoped"),) + + operations = ( + migrations.CreateModel( + name="UpsertFloatKey", + fields=[ + ("id", postgres.PrimaryKeyField()), + ("score", postgres.FloatField()), + ("value", postgres.IntegerField(default=0)), + ], + ), + ) diff --git a/plain-postgres/tests/app/examples/models/upsert.py b/plain-postgres/tests/app/examples/models/upsert.py index 5345b7bb20..cbc6882804 100644 --- a/plain-postgres/tests/app/examples/models/upsert.py +++ b/plain-postgres/tests/app/examples/models/upsert.py @@ -82,3 +82,19 @@ class UpsertValueKey(postgres.Model): ), ] ) + + +@postgres.register_model +class UpsertFloatKey(postgres.Model): + """A float conflict key -- the column type that can hold NaN.""" + + score: Field[float] = types.FloatField() + value: Field[int] = types.IntegerField(default=0) + + model_options = postgres.Options( + constraints=[ + postgres.UniqueConstraint( + fields=["score"], name="upsertfloatkey_score_unique" + ), + ] + ) diff --git a/plain-postgres/tests/public/test_bulk_upsert.py b/plain-postgres/tests/public/test_bulk_upsert.py index 23fd40ecc2..3ddcd6bfcd 100644 --- a/plain-postgres/tests/public/test_bulk_upsert.py +++ b/plain-postgres/tests/public/test_bulk_upsert.py @@ -14,6 +14,7 @@ from app.examples.models.mixins import MixinTestModel from app.examples.models.returning import ReturningEvent from app.examples.models.upsert import ( + UpsertFloatKey, UpsertItem, UpsertPair, UpsertScoped, @@ -431,3 +432,41 @@ def test_bulk_upsert_json_key_with_non_string_object_keys(db): assert conflicting.id == seeded_id assert UpsertValueKey.query.count() == 1 assert UpsertValueKey.query.get(id=seeded_id).value == 99 + + +def test_bulk_upsert_nan_key_round_trips(db): + # Postgres holds NaN equal to NaN for uniqueness; Python does not, so a + # NaN key has to be canonicalized or it can never match its own row back. + items = [UpsertFloatKey(score=float("nan"), value=1)] + UpsertFloatKey.query.bulk_upsert( + items, + update_fields=[UpsertFloatKey.value], + unique_fields=[UpsertFloatKey.score], + ) + seeded_id = items[0].id + assert seeded_id is not None + + updated = [UpsertFloatKey(score=float("nan"), value=2)] + UpsertFloatKey.query.bulk_upsert( + updated, + update_fields=[UpsertFloatKey.value], + unique_fields=[UpsertFloatKey.score], + ) + + assert updated[0].id == seeded_id + assert UpsertFloatKey.query.count() == 1 + assert UpsertFloatKey.query.get(id=seeded_id).value == 2 + + +def test_bulk_upsert_duplicate_nan_keys_rejected(db): + with pytest.raises(ValueError, match="more than one UpsertFloatKey"): + UpsertFloatKey.query.bulk_upsert( + [ + UpsertFloatKey(score=float("nan"), value=1), + UpsertFloatKey(score=float("nan"), value=2), + ], + update_fields=[UpsertFloatKey.value], + unique_fields=[UpsertFloatKey.score], + ) + + assert UpsertFloatKey.query.count() == 0 From 67073a576d31633925dcd1bab76a76cf36451137 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 15:16:45 -0500 Subject: [PATCH 30/41] plain-postgres: take any sequence of field references in bulk_upsert list is invariant, so hoisting a conflict target into a variable did not type-check even though the identical literal written inline did: fields = [M.fk, M.key] infers list[type[Scope] | Field[str]], which is not a list[Field[Any] | type[Model]]. Naming the thing you are keying on is the obvious way to write it, and a tuple was refused for the same reason. update_fields and unique_fields are Sequence now, on bulk_upsert and on _resolve_bulk_upsert_fields. The runtime already took any iterable -- _validate_field_refs walks it and builds the list the rest of the code wants -- so this is a signature change only. _validate_field_refs itself stays Sequence[Any]: it exists to reject whatever it is handed, so narrowing its input would defeat it. Inside _resolve_bulk_upsert_fields the resolved lists are now named update_columns/unique_columns rather than shadowing the parameters, matching what bulk_upsert already called them. The error messages still name the arguments the caller passed. --- plain-postgres/plain/postgres/query.py | 32 +++++++++---------- .../tests/public/test_bulk_upsert.py | 16 ++++++++++ plain-postgres/tests/typing/bulk_writes.py | 21 ++++++++++++ 3 files changed, 53 insertions(+), 16 deletions(-) diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index d6cb909885..9e1a2c3128 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -750,26 +750,26 @@ def bulk_create( def _resolve_bulk_upsert_fields( self, - update_fields: Sequence[Any], - unique_fields: Sequence[Any], + update_fields: Sequence[Field[Any] | type[Model]], + unique_fields: Sequence[Field[Any] | type[Model]], ) -> tuple[list[Field], list[Field]]: """Check both bulk_upsert() field lists and return the columns they name, with any `Model.fk` reference resolved to its foreign key column.""" object_name = self.model.model_options.object_name - unique_fields = self._validate_field_refs( + unique_columns = self._validate_field_refs( unique_fields, where="bulk_upsert() unique_fields" ) - update_fields = self._validate_field_refs( + update_columns = self._validate_field_refs( update_fields, where="bulk_upsert() update_fields" ) - if not unique_fields: + if not unique_columns: raise ValueError("bulk_upsert() requires unique_fields.") # The conflict key is also what matches each RETURNING row back to its # object, so it has to be a value the caller holds and the row keeps. - for field in unique_fields: + for field in unique_columns: if field.db_returning and not field.primary_key: raise ValueError( f"bulk_upsert() cannot use {object_name}.{field.name} in " @@ -783,22 +783,22 @@ def _resolve_bulk_upsert_fields( "can never be a stable conflict key." ) if not self.model.model_options.unique_fields_match_constraint( - {f.name for f in unique_fields} + {f.name for f in unique_columns} ): - names = [f.name for f in unique_fields] + names = [f.name for f in unique_columns] raise ValueError( f"bulk_upsert() unique_fields {names} on {object_name} must name " "the primary key or a UniqueConstraint declared on the model " "without a condition or expressions." ) - if not update_fields: + if not update_columns: raise ValueError("bulk_upsert() requires update_fields.") - if any(not isinstance(f, ColumnField) for f in update_fields): + if any(not isinstance(f, ColumnField) for f in update_columns): raise ValueError("bulk_upsert() update_fields must be database columns.") - if any(f.primary_key for f in update_fields): + if any(f.primary_key for f in update_columns): raise ValueError("bulk_upsert() cannot update primary key fields.") - for field in update_fields: + for field in update_columns: # A database-owned value (create_now, generate=True, # RandomStringField) isn't the caller's to overwrite: EXCLUDED # carries a freshly evaluated default, so naming one here would @@ -810,21 +810,21 @@ def _resolve_bulk_upsert_fields( "the database generates its value, so the update would " "overwrite the stored one with a fresh default." ) - overlap = {f.name for f in update_fields} & {f.name for f in unique_fields} + overlap = {f.name for f in update_columns} & {f.name for f in unique_columns} if overlap: raise ValueError( "bulk_upsert() update_fields cannot overlap unique_fields: " f"{sorted(overlap)}." ) - return update_fields, unique_fields + return update_columns, unique_columns def bulk_upsert( self, objs: Sequence[T], *, - update_fields: list[Field[Any] | type[Model]], - unique_fields: list[Field[Any] | type[Model]], + update_fields: Sequence[Field[Any] | type[Model]], + unique_fields: Sequence[Field[Any] | type[Model]], batch_size: int | None = None, ) -> list[T]: """ diff --git a/plain-postgres/tests/public/test_bulk_upsert.py b/plain-postgres/tests/public/test_bulk_upsert.py index 3ddcd6bfcd..e87b2edbcd 100644 --- a/plain-postgres/tests/public/test_bulk_upsert.py +++ b/plain-postgres/tests/public/test_bulk_upsert.py @@ -470,3 +470,19 @@ def test_bulk_upsert_duplicate_nan_keys_rejected(db): ) assert UpsertFloatKey.query.count() == 0 + + +def test_bulk_upsert_accepts_any_sequence_of_field_references(db): + # The parameters are Sequence, so a tuple -- or a conflict target hoisted + # into a variable, which a list parameter would reject as invariant -- + # works as well as an inline list. + conflict_target = (UpsertPair.bucket, UpsertPair.slug) + items = [UpsertPair(bucket="b", slug="s", value=1)] + UpsertPair.query.bulk_upsert( + items, + update_fields=(UpsertPair.value,), + unique_fields=conflict_target, + ) + + assert items[0].id is not None + assert UpsertPair.query.get(bucket="b", slug="s").value == 1 diff --git a/plain-postgres/tests/typing/bulk_writes.py b/plain-postgres/tests/typing/bulk_writes.py index d9a71b05ef..e742c8a683 100644 --- a/plain-postgres/tests/typing/bulk_writes.py +++ b/plain-postgres/tests/typing/bulk_writes.py @@ -60,3 +60,24 @@ def must_accept_a_foreign_key_reference() -> None: update_fields=[UpsertScoped.value], unique_fields=[UpsertScoped.tenant, UpsertScoped.slug], ) + + +def must_accept_a_hoisted_conflict_target() -> None: + # list is invariant, so a hoisted [M.fk, M.key] infers + # list[type[Scope] | Field[str]] and would not satisfy a list parameter + # even though the same literal written inline does. The parameters are + # Sequence so naming the target is as good as inlining it. + conflict_target = [UpsertScoped.tenant, UpsertScoped.slug] + UpsertScoped.query.bulk_upsert( + [UpsertScoped(tenant=UpsertTenant(name="t"), slug="s", value=1)], + update_fields=[UpsertScoped.value], + unique_fields=conflict_target, + ) + + +def must_accept_tuples() -> None: + UpsertScoped.query.bulk_upsert( + [UpsertScoped(tenant=UpsertTenant(name="t"), slug="s", value=1)], + update_fields=(UpsertScoped.value,), + unique_fields=(UpsertScoped.tenant, UpsertScoped.slug), + ) From 694147757bfb94eddaac22ee36327d54540fd0f8 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 20:31:06 -0500 Subject: [PATCH 31/41] plain-postgres: pin that RETURNING comes back in VALUES order bulk_create() has always mapped returned rows onto objects by position. Nothing in the SQL standard promises it will, and the next commit makes bulk_upsert() lean on the same thing on the ON CONFLICT path, so the assumption is worth a test rather than a comment. 400 rows, every other key seeded so the batch is a shuffled mix of inserts and conflicting updates, asserted to come back in the order they were sent. If this fails, both call sites are wrong together. --- .../tests/internal/test_returning_order.py | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 plain-postgres/tests/internal/test_returning_order.py diff --git a/plain-postgres/tests/internal/test_returning_order.py b/plain-postgres/tests/internal/test_returning_order.py new file mode 100644 index 0000000000..317aa06448 --- /dev/null +++ b/plain-postgres/tests/internal/test_returning_order.py @@ -0,0 +1,54 @@ +"""RETURNING comes back in VALUES order, including under ON CONFLICT. + +`bulk_create()` and `bulk_upsert()` both map returned rows onto the objects +they were given by position. That holds only because Postgres processes a +multi-row INSERT sequentially and emits one RETURNING row per VALUES row, in +order -- on the DO UPDATE path as much as the plain insert path. + +Nothing in the SQL standard promises this, so it is pinned here rather than +assumed. If this test ever fails, both call sites are wrong together. +""" + +from __future__ import annotations + +import random + +from app.examples.models.upsert import UpsertItem +from plain.postgres.db import get_connection + +ROWS = 400 + + +def _returned_keys(keys: list[str]) -> list[str]: + """Insert `keys` in one ON CONFLICT statement, return the RETURNING order.""" + table = UpsertItem.model_options.db_table + # Only the placeholder count is interpolated; every value is a parameter. + placeholders = ", ".join(["(%s, %s)"] * len(keys)) + sql = ( + f"INSERT INTO {table} (key, value) VALUES {placeholders} " + "ON CONFLICT (key) DO UPDATE SET value = EXCLUDED.value " + "RETURNING key" + ) + params = [param for key in keys for param in (key, 1)] + with get_connection().cursor() as cursor: + cursor.execute(sql, params) + return [row[0] for row in cursor.fetchall()] + + +def test_returning_order_matches_values_order_under_on_conflict(db): + # Seed every other key, so the batch is a shuffled mix of inserts and + # conflicting updates -- the case where the order could plausibly diverge. + for index in range(0, ROWS, 2): + UpsertItem(key=f"k{index}", value=0).create() + + keys = [f"k{index}" for index in range(ROWS)] + random.Random(0).shuffle(keys) + + assert _returned_keys(keys) == keys + + +def test_returning_order_matches_values_order_for_plain_inserts(db): + keys = [f"k{index}" for index in range(ROWS)] + random.Random(1).shuffle(keys) + + assert _returned_keys(keys) == keys From f7cd858af17b85a6cfe759f85f4054f0ecba45b4 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 20:31:17 -0500 Subject: [PATCH 32/41] plain-postgres: associate bulk_upsert rows by position, like bulk_create The by-key match-back was built on a claim in its own comment -- "RETURNING order isn't guaranteed to match VALUES order under ON CONFLICT" -- that does not hold. Postgres processes VALUES rows sequentially and emits one RETURNING row per row; bulk_create() has relied on exactly that all along. The previous commit pins it. So bulk_upsert() zips too, and a whole layer goes with it: the key -> object dict, the NaN sentinel, the memoryview and unhashable-json handling, the extra unique columns bolted onto RETURNING purely to match rows back, the returning_fields plumbing in _batched_insert that existed only to carry them, and the up-front cross-batch duplicate check. conflict_key_value/conflict_key_order collapse into one conflict_sort_value, which now only has to be deterministic and unraisable -- prepare the value, canonicalize jsonb through the field's encoder, reduce to (type name, str). No hashing, no equality. Duplicates change shape rather than disappearing. Two objects with the same key in one statement are a Postgres cardinality violation, now re-raised as a ValueError that says so. Across batches they are simply legal: the first inserts, the second updates, both objects hydrate from their own returned row, and the later write wins. Three guards stay, because none of them were about matching rows back. A NULL key, a database-generated key and an update_now key can none of them ever conflict, so in each case the upsert would silently be an insert every time -- the messages say that, and no longer mention the match. --- plain-postgres/plain/postgres/README.md | 15 +- plain-postgres/plain/postgres/query.py | 169 +++++++----------- .../tests/public/test_bulk_upsert.py | 60 ++++--- 3 files changed, 108 insertions(+), 136 deletions(-) diff --git a/plain-postgres/plain/postgres/README.md b/plain-postgres/plain/postgres/README.md index 02c14a12e1..4f7cebd767 100644 --- a/plain-postgres/plain/postgres/README.md +++ b/plain-postgres/plain/postgres/README.md @@ -499,16 +499,17 @@ CachedItem.query.bulk_upsert( - Every object must have a non-null value for every unique field. `NULL` never conflicts in Postgres, so it can't be upserted. A database-generated column (`create_now`, `generate=True`, `RandomStringField`) can't be a unique field - either — your objects never hold its value. Nor can an `update_now=True` - column, which is stamped again on every write. -- **Two objects with the same unique key raise `ValueError`.** Postgres can only - touch a row once per statement, so collapse duplicates before calling. + either — your objects never hold its value, so it could never conflict. Nor + can an `update_now=True` column, which is stamped again on every write. +- **Two objects with the same unique key in one batch raise `ValueError`.** + Postgres won't touch a row twice in one statement. Split across batches it's + allowed — the first inserts, the second updates, and the later write wins. - **`update_now=True` columns are refreshed on a conflict automatically.** You don't name them in `update_fields`; a row that gets updated gets a fresh stamp, and the object handed back carries the same one. -- Batches are issued in conflict-key order and returned rows are matched back to - objects by that key, so concurrent `bulk_upsert` calls over overlapping keys - can't deadlock each other. +- Batches are issued in conflict-key order, so concurrent `bulk_upsert` calls + over overlapping keys can't deadlock each other. Returned rows are mapped onto + the objects by position, exactly as `bulk_create` does. #### Use queryset `.update()` / `.delete()` for mass operations diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index 9e1a2c3128..afc1e9e22c 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -6,11 +6,9 @@ import copy import json -import math import operator import warnings from collections.abc import Callable, Iterator, Sequence -from decimal import Decimal from functools import cached_property from itertools import islice from typing import TYPE_CHECKING, Any, Never, Self, cast, overload @@ -59,60 +57,32 @@ from plain.postgres import Model -# What a NaN conflict key normalizes to. Any value a float or numeric column -# can otherwise hold is a number, so a string can't collide with one. -NAN_CONFLICT_KEY = "" +def conflict_sort_value(field: Field, value: Any) -> tuple[str, str]: + """One component of the order bulk_upsert() sends its batches in. + Concurrent callers only have to agree on an order, not on a meaningful + one, so this is built for two properties and no others: the same logical + key always produces the same result, and it can never raise. Preparing the + value the way the column will see it gets the first (a TimeZoneField's + ZoneInfo becomes its name, a naive datetime becomes aware), and reducing to + strings gets the second -- str() orders a NaN, a memoryview or a ZoneInfo + just as happily as an int, where `<` on any of them raises. -def conflict_key_value(field: Field, value: Any) -> Any: - """One component of a bulk_upsert() conflict key, in a form that hashes. - - The same function runs over both sides of the match -- the value taken off - the object and the value Postgres handed back -- so however a column spells - its Python value on each side, the two meet in the same place. Preparing - the value is most of that: a TimeZoneField's ZoneInfo becomes its name, a - naive datetime becomes aware, a UUID string becomes a UUID. - - Values that already compare and hash alike need no help here, and several - near-misses turn out to be in that group: Decimal("1.0") and - Decimal("1.00"), -0.0 and 0.0, and two aware datetimes naming the same - instant in different zones. Postgres collapses each of those pairs to one - row as well, so Python and the database agree without being told to. NaN is - the one place they disagree. + Leading with the type's name keeps a polymorphic column from interleaving: + a jsonb column holding an object in one row and a number in the next would + otherwise sort "7" next to "[7]". """ value = field.get_prep_value(value) if isinstance(field, JSONField): - # Canonicalize the value the way the column will store it: encode with - # the field's own encoder, which stringifies non-string object keys, - # and only then re-parse and dump with the keys sorted. Sorting before - # the encode would compare an int key against a str one and raise. A - # jsonb column keys as a string either way, scalars included, which is - # also what makes an otherwise unhashable object usable as a key. - return json.dumps( + # Encode with the field's own encoder, which stringifies non-string + # object keys, and only then re-parse and dump with the keys sorted -- + # sorting them first would compare an int key against a str one and + # raise. Two equal objects written with their keys in either order then + # sort to the same place. + value = json.dumps( json.loads(get_json_dumps(field.encoder)(value)), sort_keys=True ) - if isinstance(value, memoryview | bytearray): - # psycopg hands a bytea column back as a memoryview. - return bytes(value) - if (isinstance(value, float) and math.isnan(value)) or ( - isinstance(value, Decimal) and value.is_nan() - ): - # Postgres holds NaN equal to NaN for uniqueness. Python doesn't, and - # every NaN hashes differently, so a NaN key would never match its own - # row back and two of them would walk past the duplicate check. - return NAN_CONFLICT_KEY - return value - - -def conflict_key_order(key: tuple[Any, ...]) -> tuple[tuple[str, Any], ...]: - """The sort key for a bulk_upsert() conflict key. - - The deadlock ordering only has to be the *same* order for every caller, not - a meaningful one, and one column can hold values that don't compare to each - other -- a jsonb column with an object in one row and a number in the next. - Leading with the type's name keeps every comparison inside a single type. - """ - return tuple((type(value).__name__, value) for value in key) + return (type(value).__name__, str(value)) # The maximum number of results to fetch in a get() query. @@ -740,6 +710,11 @@ def bulk_create( fields, batch_size, ) + # Postgres emits one RETURNING row per VALUES row, in order, so + # the rows can be zipped straight onto the objects. bulk_upsert() + # relies on the same guarantee for its ON CONFLICT batches -- the + # two stand or fall together, and + # tests/internal/test_returning_order.py pins it. assert len(returned_columns) == len(objs_without_id) for obj_without_id, results in zip(objs_without_id, returned_columns): for result, field in zip(results, meta.db_returning_fields): @@ -767,8 +742,8 @@ def _resolve_bulk_upsert_fields( if not unique_columns: raise ValueError("bulk_upsert() requires unique_fields.") - # The conflict key is also what matches each RETURNING row back to its - # object, so it has to be a value the caller holds and the row keeps. + # A conflict key the caller doesn't control can never actually conflict, + # so the upsert would silently be an insert every time. for field in unique_columns: if field.db_returning and not field.primary_key: raise ValueError( @@ -861,13 +836,11 @@ def bulk_upsert( object_name = self.model.model_options.object_name self._prepare_for_bulk_create(objs) - # Compute each object's conflict key exactly once. Reused below to sort - # the batches and to match RETURNING rows back to objects, so the keys - # have to be usable for both: no nulls (NULL never conflicts in - # Postgres) and no duplicates (one statement can only touch a row once). - obj_by_key: dict[tuple[Any, ...], T] = {} + # A NULL conflict key never conflicts in Postgres, so the row would + # always insert and the upsert would quietly be an insert. + sort_keys = [] for obj in objs: - key_values = [] + key = [] for field in unique_columns: value = field.value_from_object(obj) if value is None: @@ -876,16 +849,8 @@ def bulk_upsert( "object; NULL never conflicts in Postgres, so it cannot " "be upserted." ) - key_values.append(conflict_key_value(field, value)) - key = tuple(key_values) - if key in obj_by_key: - names = [f.name for f in unique_columns] - raise ValueError( - f"bulk_upsert() got more than one {object_name} with " - f"{names} = {list(key)}. Postgres can only touch a row once " - "per statement, so collapse the duplicates before calling." - ) - obj_by_key[key] = obj + key.append(conflict_sort_value(field, value)) + sort_keys.append(tuple(key)) # An update_now column is stamped by pre_save on the way in, so the # object already holds a fresh value whether it inserts or updates. @@ -904,46 +869,40 @@ def bulk_upsert( if not pk_is_unique: fields = [f for f in fields if not isinstance(f, PrimaryKeyField)] - # RETURNING must carry the DB-returned fields (to populate the objects) - # plus the unique fields (to match each returned row to its object). - returning_fields = list(meta.db_returning_fields) - for field in unique_columns: - if field not in returning_fields: - returning_fields.append(field) - unique_indices = [returning_fields.index(f) for f in unique_columns] - # Issue the batches in conflict-key order so concurrent upserts touching # overlapping keys lock rows in the same order and can't deadlock each - # other. objs itself is untouched, so the caller gets its input order. - sorted_keys = sorted(obj_by_key, key=conflict_key_order) + # other. sorted() is stable, so equal keys keep their input order and + # the objects themselves are never compared. objs is left alone, so the + # caller gets its own order back. + order = sorted(range(len(objs)), key=lambda position: sort_keys[position]) + ordered_objs = [objs[position] for position in order] with transaction.atomic(savepoint=False): - returned_rows = self._batched_insert( - [obj_by_key[key] for key in sorted_keys], - fields, - batch_size, - returning_fields=returning_fields, - on_conflict=OnConflict.UPDATE, - update_fields=conflict_update_columns, - unique_fields=unique_columns, - ) - - # RETURNING order isn't guaranteed to match VALUES order under ON - # CONFLICT, so match each returned row to its object by the unique - # key -- run through conflict_key_value() again so both sides of the - # match are spelled the same way. Still inside the transaction: a - # row that can't be matched would leave the objects half-populated, - # so roll the write back. - assert len(returned_rows) == len(obj_by_key) - row_by_key = { - tuple( - conflict_key_value(field, row[index]) - for field, index in zip(unique_columns, unique_indices) - ): row - for row in returned_rows - } - for key, obj in obj_by_key.items(): - row = row_by_key[key] + try: + returned_rows = self._batched_insert( + ordered_objs, + fields, + batch_size, + on_conflict=OnConflict.UPDATE, + update_fields=conflict_update_columns, + unique_fields=unique_columns, + ) + except psycopg.errors.CardinalityViolation as exc: + names = [f.name for f in unique_columns] + raise ValueError( + f"bulk_upsert() sent two {object_name} objects with the same " + f"{names} in one statement, which Postgres refuses -- it can " + "only touch a row once per statement. Collapse the duplicates " + "before calling." + ) from exc + + # Postgres emits one RETURNING row per VALUES row, in order, on the + # DO UPDATE path as much as the insert path, so the rows come back + # in the order the objects were sent. bulk_create() maps its rows + # onto objects by position for the same reason -- the two stand or + # fall together, and tests/internal/test_returning_order.py pins it. + assert len(returned_rows) == len(ordered_objs) + for obj, row in zip(ordered_objs, returned_rows): for index, field in enumerate(meta.db_returning_fields): setattr(obj, field.name, row[index]) obj._state.adding = False @@ -1740,7 +1699,6 @@ def _batched_insert( fields: Sequence[Field], batch_size: int | None, *, - returning_fields: list[Field] | None = None, on_conflict: OnConflict | None = None, update_fields: list[Field] | None = None, unique_fields: list[Field] | None = None, @@ -1750,8 +1708,7 @@ def _batched_insert( at a time, collecting the RETURNING rows from every batch. Pass the on_conflict kwargs to run each batch as ON CONFLICT DO UPDATE. """ - if returning_fields is None: - returning_fields = self.model._model_meta.db_returning_fields + returning_fields = self.model._model_meta.db_returning_fields max_batch_size = max(len(objs), 1) batch_size = min(batch_size, max_batch_size) if batch_size else max_batch_size returned_rows = [] diff --git a/plain-postgres/tests/public/test_bulk_upsert.py b/plain-postgres/tests/public/test_bulk_upsert.py index e87b2edbcd..b216544247 100644 --- a/plain-postgres/tests/public/test_bulk_upsert.py +++ b/plain-postgres/tests/public/test_bulk_upsert.py @@ -2,7 +2,7 @@ One INSERT ... ON CONFLICT (unique_fields) DO UPDATE ... RETURNING per batch. Every returned object -- inserted or updated -- comes back with its DB-returned -fields (primary key, DB defaults) populated, matched to its row by unique key. +fields (primary key, DB defaults) populated from the row at its own position. """ from __future__ import annotations @@ -75,10 +75,9 @@ def test_bulk_upsert_updates_only_named_fields(db): assert row.label == "original" # field not in update_fields preserved -def test_bulk_upsert_matches_returned_rows_by_key_not_order(db): - # Seed so every input row conflicts; RETURNING order under ON CONFLICT is - # not guaranteed to match VALUES order, so each object must be matched to - # its own row by unique key. +def test_bulk_upsert_hydrates_every_object_when_all_of_them_conflict(db): + # Seed so every input row takes the DO UPDATE path, and pass them out of + # key order so the deadlock sort actually reorders the batch. for key in ("a", "b", "c"): UpsertItem(key=key, value=0).create() seeded_ids = {row.key: row.id for row in UpsertItem.query.all()} @@ -218,19 +217,35 @@ def test_bulk_upsert_composite_unique_fields(db): assert stored == {("b1", "s1"): 10, ("b1", "s2"): 20, ("b2", "s1"): 30} -def test_bulk_upsert_duplicate_keys_rejected(db): - # Postgres raises a cardinality violation if one statement touches a row - # twice, and splitting duplicates across batches would silently let the - # last one win -- so they are refused up front, whatever the batch size. - with pytest.raises(ValueError, match="more than one UpsertItem"): +def test_bulk_upsert_duplicate_keys_in_one_batch_rejected(db): + # Postgres refuses to touch a row twice in one statement. The raw + # CardinalityViolation is re-raised as something that names the problem. + # It aborts the surrounding transaction, so nothing is queried after it. + with pytest.raises(ValueError, match=r"same \['key'\] in one statement"): UpsertItem.query.bulk_upsert( [UpsertItem(key="a", value=1), UpsertItem(key="a", value=2)], update_fields=[UpsertItem.value], unique_fields=[UpsertItem.key], - batch_size=1, ) - assert UpsertItem.query.count() == 0 + +def test_bulk_upsert_duplicate_keys_in_separate_batches_are_legal(db): + # Split across statements there is no cardinality violation: the first + # inserts the row and the second updates it. + items = [UpsertItem(key="a", value=1), UpsertItem(key="a", value=2)] + UpsertItem.query.bulk_upsert( + items, + update_fields=[UpsertItem.value], + unique_fields=[UpsertItem.key], + batch_size=1, + ) + + assert UpsertItem.query.count() == 1 + row = UpsertItem.query.get(key="a") + # Both objects are hydrated, from their own returned row -- the same row. + assert [item.id for item in items] == [row.id, row.id] + # Equal keys keep their input order, so the later write is the one that lands. + assert row.value == 2 def test_bulk_upsert_database_generated_unique_field_rejected(db): @@ -344,9 +359,9 @@ def test_bulk_upsert_foreign_key_in_update_fields(db): assert row.value == 2 -def test_bulk_upsert_matches_keys_that_python_cannot_hash_or_sort(db): - # jsonb dicts are unhashable, ZoneInfo and memoryview are unorderable -- - # the conflict key has to survive being a dict key and being sorted. +def test_bulk_upsert_keys_that_python_cannot_sort(db): + # ZoneInfo and memoryview have no ordering at all and a jsonb dict has no + # useful one -- the batch still has to be put in a deterministic order. chicago = ZoneInfo("America/Chicago") utc = ZoneInfo("UTC") UpsertValueKey(payload={"a": 1}, blob=b"x", zone=utc, value=1).create() @@ -400,8 +415,8 @@ def test_bulk_upsert_key_values_that_do_not_compare_to_each_other(db): def test_bulk_upsert_json_key_with_non_string_object_keys(db): # jsonb object keys are always strings -- the encoder stringifies an int - # key on the way in. The conflict key has to be canonicalized the same - # way, or sorting the keys compares an int against a str and raises. + # key on the way in. The sort key has to be canonicalized the same way, or + # sorting the object's keys compares an int against a str and raises. utc = ZoneInfo("UTC") UpsertValueKey( payload={1: "a", "2": "b", "nested": {"z": 1, "y": 2}}, @@ -435,8 +450,8 @@ def test_bulk_upsert_json_key_with_non_string_object_keys(db): def test_bulk_upsert_nan_key_round_trips(db): - # Postgres holds NaN equal to NaN for uniqueness; Python does not, so a - # NaN key has to be canonicalized or it can never match its own row back. + # Postgres holds NaN equal to NaN for uniqueness, so a NaN key really does + # conflict -- and sorting it must not raise the way `<` on a NaN would. items = [UpsertFloatKey(score=float("nan"), value=1)] UpsertFloatKey.query.bulk_upsert( items, @@ -458,8 +473,9 @@ def test_bulk_upsert_nan_key_round_trips(db): assert UpsertFloatKey.query.get(id=seeded_id).value == 2 -def test_bulk_upsert_duplicate_nan_keys_rejected(db): - with pytest.raises(ValueError, match="more than one UpsertFloatKey"): +def test_bulk_upsert_duplicate_nan_keys_in_one_batch_rejected(db): + # Postgres holds the two NaNs equal, so this is the same row twice. + with pytest.raises(ValueError, match=r"same \['score'\] in one statement"): UpsertFloatKey.query.bulk_upsert( [ UpsertFloatKey(score=float("nan"), value=1), @@ -469,8 +485,6 @@ def test_bulk_upsert_duplicate_nan_keys_rejected(db): unique_fields=[UpsertFloatKey.score], ) - assert UpsertFloatKey.query.count() == 0 - def test_bulk_upsert_accepts_any_sequence_of_field_references(db): # The parameters are Sequence, so a tuple -- or a conflict target hoisted From 3f703324161e1b4637f47b10449a291a728a0841 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 20:39:00 -0500 Subject: [PATCH 33/41] plain-postgres: sort equal decimals to one place, whatever their scale Postgres numeric holds Decimal("1.0") and Decimal("1.00") equal; str() spells them apart, so two callers holding different scales of the same logical key would order their batches differently and the deadlock avoidance would have a hole in it. DecimalField.get_prep_value does not quantize -- it hands a Decimal straight back -- so the two spellings do reach the sort key. normalize() gives equal values one spelling, and abs() takes the sign off a negative zero, which normalize() keeps and Postgres does not distinguish. Both are guarded by is_finite(), which is load-bearing twice over: normalize() raises on a signaling NaN, and so does comparing one to zero. An exponent too large to normalize is left alone rather than taking the sort down -- Postgres rejects it on write with the better error. The unit test also records something worth knowing: a non-finite Decimal never reaches the sort key at all, because DecimalField.to_python refuses NaN and infinity first. The guard stays anyway -- conflict_sort_value takes any Field, not just that one. --- plain-postgres/plain/postgres/query.py | 16 ++++ .../internal/test_conflict_sort_value.py | 92 +++++++++++++++++++ 2 files changed, 108 insertions(+) create mode 100644 plain-postgres/tests/internal/test_conflict_sort_value.py diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index afc1e9e22c..ad5857457b 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -9,6 +9,7 @@ import operator import warnings from collections.abc import Callable, Iterator, Sequence +from decimal import Decimal from functools import cached_property from itertools import islice from typing import TYPE_CHECKING, Any, Never, Self, cast, overload @@ -82,6 +83,21 @@ def conflict_sort_value(field: Field, value: Any) -> tuple[str, str]: value = json.dumps( json.loads(get_json_dumps(field.encoder)(value)), sort_keys=True ) + if isinstance(value, Decimal) and value.is_finite(): + # Postgres numeric holds Decimal("1.0") and Decimal("1.00") equal but + # str() spells them differently, which would sort one logical key to + # two places. normalize() gives equal values one spelling. It overflows + # on an exponent no column could store anyway -- leave those to the + # write, which has the better error. NaN and infinity have no scale to + # strip, and normalizing a signaling NaN raises, so they skip it. + try: + value = value.normalize() + except ArithmeticError: + pass + if value == 0: + # normalize() keeps the sign on a negative zero, which Postgres + # and Python both hold equal to positive zero. + value = abs(value) return (type(value).__name__, str(value)) diff --git a/plain-postgres/tests/internal/test_conflict_sort_value.py b/plain-postgres/tests/internal/test_conflict_sort_value.py new file mode 100644 index 0000000000..d3f95b5fe8 --- /dev/null +++ b/plain-postgres/tests/internal/test_conflict_sort_value.py @@ -0,0 +1,92 @@ +"""The order bulk_upsert() sends its batches in. + +Concurrent callers avoid deadlocking each other by locking rows in the same +order, which only works if a given logical key always produces the same sort +value -- whichever way the caller happened to spell it. And since this runs +before any query, it must never raise. + +Unit-level because `conflict_sort_value` isn't public API; the behavior it buys +is covered end-to-end in tests/public/test_bulk_upsert.py. +""" + +from __future__ import annotations + +from decimal import Decimal +from zoneinfo import ZoneInfo + +import pytest +from app.examples.models.forms import FormsExample +from app.examples.models.upsert import UpsertValueKey +from plain.exceptions import ValidationError +from plain.postgres.query import conflict_sort_value + +AMOUNT = FormsExample.amount +RATIO = FormsExample.ratio + + +def test_equal_decimals_sort_together_whatever_their_scale(): + # Postgres numeric holds these equal, so they have to sort to one place -- + # str() alone spells them "1.0" and "1.00". + assert Decimal("1.0") == Decimal("1.00") + assert conflict_sort_value(AMOUNT, Decimal("1.0")) == conflict_sort_value( + AMOUNT, Decimal("1.00") + ) + # Including the forms an integer can take. + assert conflict_sort_value(AMOUNT, Decimal(100)) == conflict_sort_value( + AMOUNT, Decimal("1E+2") + ) + # And negative zero, which normalize() keeps the sign on. + assert conflict_sort_value(AMOUNT, Decimal("-0.00")) == conflict_sort_value( + AMOUNT, Decimal("0.0") + ) + + +def test_different_decimals_still_sort_apart(): + assert conflict_sort_value(AMOUNT, Decimal("1.0")) != conflict_sort_value( + AMOUNT, Decimal("1.5") + ) + + +def test_a_decimal_too_large_to_normalize_does_not_raise(): + # normalize() overflows on this, and the sort must not go down with it. + # Postgres rejects an exponent this large on write; that is the useful + # error, so the value passes through un-normalized. + assert isinstance(conflict_sort_value(AMOUNT, Decimal("1E+999999999")), tuple) + + +def test_a_non_finite_decimal_is_rejected_before_the_sort_key(): + # DecimalField.to_python refuses NaN and infinity, so they never reach the + # sort key at all. conflict_sort_value still guards them -- it takes any + # Field, and `Decimal("sNaN") == 0` raises -- but this is the real path. + for value in (Decimal("NaN"), Decimal("sNaN"), Decimal("Infinity")): + with pytest.raises(ValidationError): + conflict_sort_value(AMOUNT, value) + + +def test_values_with_no_ordering_of_their_own_do_not_raise(): + assert isinstance(conflict_sort_value(RATIO, float("nan")), tuple) + assert isinstance( + conflict_sort_value(UpsertValueKey.zone, ZoneInfo("America/Chicago")), tuple + ) + assert isinstance(conflict_sort_value(UpsertValueKey.blob, memoryview(b"x")), tuple) + + +def test_json_objects_sort_together_whatever_order_their_keys_were_written_in(): + payload = UpsertValueKey.payload + assert conflict_sort_value(payload, {"a": 1, "b": 2}) == conflict_sort_value( + payload, {"b": 2, "a": 1} + ) + # A non-string object key is valid -- the encoder stringifies it -- and + # sorting the keys before that encode would compare an int to a str. + assert isinstance(conflict_sort_value(payload, {1: "a", "2": "b"}), tuple) + + +def test_a_polymorphic_json_column_never_compares_across_types(): + # A jsonb column can hold an object in one row and a number in the next. + # Every value canonicalizes to a string first, so sorting a batch of them + # never puts an int up against a dict. + payload = UpsertValueKey.payload + keys = [conflict_sort_value(payload, value) for value in ({"a": 1}, 7, "s", [1, 2])] + assert {key[0] for key in keys} == {"str"} + assert len(set(keys)) == 4 + assert len(sorted(keys)) == 4 From e5b5de082f965ab8a0779e1ffc7012dca47ecf29 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 20:57:06 -0500 Subject: [PATCH 34/41] plain-postgres: render conflict keys the way Postgres compares them The sort key led with the value's type name, on the theory that a jsonb column holding an object in one row and a number in the next would otherwise interleave. It never did: the jsonb branch returns a string, so "7" and "[7]" were both already under 'str'. What the prefix did do was split keys Postgres holds equal -- a StrEnum member from the plain string, the same instant written at two offsets -- into different sort positions, which is the hole in the deadlock avoidance it was meant to close. Gone, and the spellings canonicalize instead: str.__str__ for a str subclass that renders itself some other way, bytes for a memoryview (whose str() was its address in memory, so a bytea key sorted nondeterministically), UTC for an aware datetime, a sign-folded repr for a float, normalize() for a decimal. Each one is a pair Postgres holds equal, so each one has to sort to one place. The result is a plain string -- one total order over everything, which cannot raise. The Decimal is_finite() guard goes too: DecimalField.get_prep_value rejects a non-finite value before the sort key ever sees it, so the branch was dead. The try/except stays, because an exponent too large to normalize is reachable and is Postgres's error to report. conflict_sort_value now takes a value already through get_prep_value rather than preparing it again, which also means a value that *prepares* to None is caught by the null check. --- plain-postgres/plain/postgres/query.py | 69 ++++++---- .../internal/test_conflict_sort_value.py | 126 ++++++++++++------ 2 files changed, 125 insertions(+), 70 deletions(-) diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index ad5857457b..6aebc8f5ed 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -5,6 +5,7 @@ from __future__ import annotations import copy +import datetime import json import operator import warnings @@ -58,47 +59,57 @@ from plain.postgres import Model -def conflict_sort_value(field: Field, value: Any) -> tuple[str, str]: +def conflict_sort_value(field: Field, value: Any) -> str: """One component of the order bulk_upsert() sends its batches in. Concurrent callers only have to agree on an order, not on a meaningful - one, so this is built for two properties and no others: the same logical - key always produces the same result, and it can never raise. Preparing the - value the way the column will see it gets the first (a TimeZoneField's - ZoneInfo becomes its name, a naive datetime becomes aware), and reducing to - strings gets the second -- str() orders a NaN, a memoryview or a ZoneInfo - just as happily as an int, where `<` on any of them raises. - - Leading with the type's name keeps a polymorphic column from interleaving: - a jsonb column holding an object in one row and a number in the next would - otherwise sort "7" next to "[7]". + one, so the requirement is narrow: two callers holding the same logical + key must render it the same way, and comparing the results must never + raise. Everything here serves that. + + The value arrives already through `get_prep_value`, which settles most of + it -- a TimeZoneField's ZoneInfo is its name by then, a UUID string is a + UUID, a naive datetime is aware. What is left is the spellings Postgres + holds equal that `str()` would not: a str subclass that renders itself + some other way, a bytea handed back as a memoryview, the same instant + written at two offsets, a signed zero, a decimal's scale. """ - value = field.get_prep_value(value) if isinstance(field, JSONField): # Encode with the field's own encoder, which stringifies non-string # object keys, and only then re-parse and dump with the keys sorted -- # sorting them first would compare an int key against a str one and - # raise. Two equal objects written with their keys in either order then - # sort to the same place. - value = json.dumps( + # raise. Two equal objects written with their keys in either order + # then render the same. + return json.dumps( json.loads(get_json_dumps(field.encoder)(value)), sort_keys=True ) - if isinstance(value, Decimal) and value.is_finite(): - # Postgres numeric holds Decimal("1.0") and Decimal("1.00") equal but - # str() spells them differently, which would sort one logical key to - # two places. normalize() gives equal values one spelling. It overflows - # on an exponent no column could store anyway -- leave those to the - # write, which has the better error. NaN and infinity have no scale to - # strip, and normalizing a signaling NaN raises, so they skip it. + if isinstance(value, str): + # A StrEnum member or a SafeString compares equal to the plain string, + # which is all the column holds, but renders itself differently. + # str.__str__ goes around the override. + return str.__str__(value) + if isinstance(value, memoryview | bytearray): + # psycopg hands a bytea column back as a memoryview, whose str() is + # where it happens to sit in memory. + return str(bytes(value)) + if isinstance(value, datetime.datetime) and value.tzinfo is not None: + # timestamptz stores the instant, not the offset it was written at. + return str(value.astimezone(datetime.UTC)) + if isinstance(value, float): + # Postgres holds -0.0 and 0.0 equal; adding zero folds the sign. + return repr(value + 0.0) + if isinstance(value, Decimal): + # numeric holds Decimal("1.0") and Decimal("1.00") equal. normalize() + # gives them one spelling, and abs() folds the negative zero it keeps. + # An exponent too large to normalize is left as it is -- Postgres + # rejects it on write, with the better error. try: value = value.normalize() + if value == 0: + value = abs(value) except ArithmeticError: pass - if value == 0: - # normalize() keeps the sign on a negative zero, which Postgres - # and Python both hold equal to positive zero. - value = abs(value) - return (type(value).__name__, str(value)) + return str(value) # The maximum number of results to fetch in a get() query. @@ -858,7 +869,9 @@ def bulk_upsert( for obj in objs: key = [] for field in unique_columns: - value = field.value_from_object(obj) + # Prepared once here and handed to the sort key, rather than + # prepared again inside it. + value = field.get_prep_value(field.value_from_object(obj)) if value is None: raise ValueError( f"bulk_upsert() requires a non-null {field.name} on every " diff --git a/plain-postgres/tests/internal/test_conflict_sort_value.py b/plain-postgres/tests/internal/test_conflict_sort_value.py index d3f95b5fe8..7e08c3b0c1 100644 --- a/plain-postgres/tests/internal/test_conflict_sort_value.py +++ b/plain-postgres/tests/internal/test_conflict_sort_value.py @@ -1,9 +1,9 @@ """The order bulk_upsert() sends its batches in. Concurrent callers avoid deadlocking each other by locking rows in the same -order, which only works if a given logical key always produces the same sort -value -- whichever way the caller happened to spell it. And since this runs -before any query, it must never raise. +order, which works only if two callers holding the same logical key render it +the same way -- however each of them happened to spell it. And since comparing +the results is the sort, comparing them must never raise. Unit-level because `conflict_sort_value` isn't public API; the behavior it buys is covered end-to-end in tests/public/test_bulk_upsert.py. @@ -11,82 +11,124 @@ from __future__ import annotations +import datetime from decimal import Decimal +from enum import Enum, StrEnum from zoneinfo import ZoneInfo import pytest from app.examples.models.forms import FormsExample -from app.examples.models.upsert import UpsertValueKey +from app.examples.models.upsert import UpsertItem, UpsertValueKey from plain.exceptions import ValidationError from plain.postgres.query import conflict_sort_value AMOUNT = FormsExample.amount RATIO = FormsExample.ratio +MOMENT = FormsExample.event_datetime +KEY = UpsertItem.key -def test_equal_decimals_sort_together_whatever_their_scale(): - # Postgres numeric holds these equal, so they have to sort to one place -- - # str() alone spells them "1.0" and "1.00". - assert Decimal("1.0") == Decimal("1.00") - assert conflict_sort_value(AMOUNT, Decimal("1.0")) == conflict_sort_value( - AMOUNT, Decimal("1.00") - ) - # Including the forms an integer can take. - assert conflict_sort_value(AMOUNT, Decimal(100)) == conflict_sort_value( - AMOUNT, Decimal("1E+2") +def sort_value(field, value): + """Prepare the value the way bulk_upsert() does, then render it.""" + return conflict_sort_value(field, field.get_prep_value(value)) + + +class Shade(StrEnum): + RED = "red" + + +class LegacyShade(str, Enum): + RED = "red" + + +class Shouty(str): + def __str__(self) -> str: # a SafeString-style override + return "SHOUTY" + + +def test_str_subclasses_render_as_the_string_the_column_holds(): + # All of these compare equal to "red" and store as "red", so they have to + # sort as "red" -- str() alone gives "LegacyShade.RED" and "SHOUTY". + assert Shade.RED == LegacyShade.RED == Shouty("red") == "red" + for value in (Shade.RED, LegacyShade.RED, Shouty("red"), "red"): + assert sort_value(KEY, value) == "red" + + +def test_bytes_and_memoryview_of_the_same_content_render_the_same(): + # str(memoryview) is an address, which differs run to run and between two + # callers holding the same bytes. + assert sort_value(UpsertValueKey.blob, memoryview(b"x")) == sort_value( + UpsertValueKey.blob, b"x" ) - # And negative zero, which normalize() keeps the sign on. - assert conflict_sort_value(AMOUNT, Decimal("-0.00")) == conflict_sort_value( - AMOUNT, Decimal("0.0") + assert sort_value(UpsertValueKey.blob, bytearray(b"x")) == sort_value( + UpsertValueKey.blob, b"x" ) + assert "memory at" not in sort_value(UpsertValueKey.blob, memoryview(b"x")) -def test_different_decimals_still_sort_apart(): - assert conflict_sort_value(AMOUNT, Decimal("1.0")) != conflict_sort_value( - AMOUNT, Decimal("1.5") - ) +def test_one_instant_written_at_two_offsets_renders_once(): + # timestamptz stores the instant, so these are one row. + utc = datetime.datetime(2024, 1, 1, 12, tzinfo=datetime.UTC) + new_york = datetime.datetime(2024, 1, 1, 7, tzinfo=ZoneInfo("America/New_York")) + assert utc == new_york + assert sort_value(MOMENT, utc) == sort_value(MOMENT, new_york) + + +def test_signed_zeros_render_the_same(): + assert -0.0 == 0.0 + assert sort_value(RATIO, -0.0) == sort_value(RATIO, 0.0) + assert sort_value(AMOUNT, Decimal("-0.00")) == sort_value(AMOUNT, Decimal("0.0")) + + +def test_equal_decimals_render_the_same_whatever_their_scale(): + assert Decimal("1.0") == Decimal("1.00") + assert sort_value(AMOUNT, Decimal("1.0")) == sort_value(AMOUNT, Decimal("1.00")) + assert sort_value(AMOUNT, Decimal(100)) == sort_value(AMOUNT, Decimal("1E+2")) + + +def test_different_values_still_render_apart(): + assert sort_value(AMOUNT, Decimal("1.0")) != sort_value(AMOUNT, Decimal("1.5")) + assert sort_value(KEY, "a") != sort_value(KEY, "b") def test_a_decimal_too_large_to_normalize_does_not_raise(): - # normalize() overflows on this, and the sort must not go down with it. - # Postgres rejects an exponent this large on write; that is the useful - # error, so the value passes through un-normalized. - assert isinstance(conflict_sort_value(AMOUNT, Decimal("1E+999999999")), tuple) + # normalize() overflows on this; Postgres rejects it on write, and that is + # the error worth surfacing, so the sort leaves it alone. + assert isinstance(sort_value(AMOUNT, Decimal("1E+999999999")), str) def test_a_non_finite_decimal_is_rejected_before_the_sort_key(): # DecimalField.to_python refuses NaN and infinity, so they never reach the - # sort key at all. conflict_sort_value still guards them -- it takes any - # Field, and `Decimal("sNaN") == 0` raises -- but this is the real path. + # sort key. Finding that out before any query is issued is the point. for value in (Decimal("NaN"), Decimal("sNaN"), Decimal("Infinity")): with pytest.raises(ValidationError): - conflict_sort_value(AMOUNT, value) + sort_value(AMOUNT, value) -def test_values_with_no_ordering_of_their_own_do_not_raise(): - assert isinstance(conflict_sort_value(RATIO, float("nan")), tuple) - assert isinstance( - conflict_sort_value(UpsertValueKey.zone, ZoneInfo("America/Chicago")), tuple - ) - assert isinstance(conflict_sort_value(UpsertValueKey.blob, memoryview(b"x")), tuple) +def test_values_with_no_ordering_of_their_own_render_and_sort(): + keys = [ + sort_value(RATIO, float("nan")), + sort_value(UpsertValueKey.zone, ZoneInfo("America/Chicago")), + sort_value(UpsertValueKey.blob, memoryview(b"x")), + ] + assert all(isinstance(key, str) for key in keys) + assert len(sorted(keys)) == 3 -def test_json_objects_sort_together_whatever_order_their_keys_were_written_in(): +def test_json_objects_render_the_same_whatever_order_their_keys_were_written_in(): payload = UpsertValueKey.payload - assert conflict_sort_value(payload, {"a": 1, "b": 2}) == conflict_sort_value( + assert sort_value(payload, {"a": 1, "b": 2}) == sort_value( payload, {"b": 2, "a": 1} ) # A non-string object key is valid -- the encoder stringifies it -- and # sorting the keys before that encode would compare an int to a str. - assert isinstance(conflict_sort_value(payload, {1: "a", "2": "b"}), tuple) + assert isinstance(sort_value(payload, {1: "a", "2": "b"}), str) -def test_a_polymorphic_json_column_never_compares_across_types(): +def test_a_polymorphic_json_column_sorts_without_comparing_across_types(): # A jsonb column can hold an object in one row and a number in the next. - # Every value canonicalizes to a string first, so sorting a batch of them - # never puts an int up against a dict. payload = UpsertValueKey.payload - keys = [conflict_sort_value(payload, value) for value in ({"a": 1}, 7, "s", [1, 2])] - assert {key[0] for key in keys} == {"str"} + keys = [sort_value(payload, value) for value in ({"a": 1}, 7, "s", [1, 2])] + assert all(isinstance(key, str) for key in keys) assert len(set(keys)) == 4 assert len(sorted(keys)) == 4 From b4f7f4d10af49a04757ea8c963340c0dd709d02f Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 20:58:53 -0500 Subject: [PATCH 35/41] plain-postgres: keep an id the caller set on a bulk_upsert object When the primary key wasn't the conflict target, the pk column was stripped from the INSERT for every object -- so an object handed over with id=5 was written with a generated identity instead, and then had its own id overwritten by the returned one. bulk_create() has always honored an explicit id, and bulk_create(update_conflicts=True) did too before this PR replaced it, so this was a regression in the swap. Objects now split the way bulk_create() splits them: those carrying an id insert with it, those without let Postgres generate one, each as its own statement with its own column list. Both halves keep the conflict-key ordering and the position mapping within themselves. When the primary key *is* the conflict target there is nothing to split -- every object has one, because a null conflict key is already refused. The multi-batch test is the case none of the others covered: batches smaller than the input, every other key seeded, input shuffled, so inserts and updates interleave across statements while the sort reorders them. --- plain-postgres/plain/postgres/query.py | 100 +++++++++++------- .../tests/public/test_bulk_upsert.py | 67 ++++++++++++ 2 files changed, 126 insertions(+), 41 deletions(-) diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index 6aebc8f5ed..ac2c920948 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -891,50 +891,68 @@ def bulk_upsert( if field.auto_fills_on_save and field not in conflict_update_columns: conflict_update_columns.append(field) - # Include the PK column only when it is itself the conflict key; - # otherwise let Postgres generate the identity value. - pk_is_unique = any(f.primary_key for f in unique_columns) + # An object that already carries an id inserts with it; one that + # doesn't lets Postgres generate the identity value. bulk_create() + # splits the same way -- an id the caller set is theirs, not ours to + # throw away. When the primary key *is* the conflict target every + # object has one, so there is nothing to split. fields = meta.fields - if not pk_is_unique: - fields = [f for f in fields if not isinstance(f, PrimaryKeyField)] - - # Issue the batches in conflict-key order so concurrent upserts touching - # overlapping keys lock rows in the same order and can't deadlock each - # other. sorted() is stable, so equal keys keep their input order and - # the objects themselves are never compared. objs is left alone, so the - # caller gets its own order back. - order = sorted(range(len(objs)), key=lambda position: sort_keys[position]) - ordered_objs = [objs[position] for position in order] + fields_without_pk = [f for f in fields if not isinstance(f, PrimaryKeyField)] + pk_is_unique = any(f.primary_key for f in unique_columns) + if pk_is_unique: + partitions = [(list(range(len(objs))), fields)] + else: + partitions = [ + ([p for p, obj in enumerate(objs) if obj.id is not None], fields), + ( + [p for p, obj in enumerate(objs) if obj.id is None], + fields_without_pk, + ), + ] with transaction.atomic(savepoint=False): - try: - returned_rows = self._batched_insert( - ordered_objs, - fields, - batch_size, - on_conflict=OnConflict.UPDATE, - update_fields=conflict_update_columns, - unique_fields=unique_columns, - ) - except psycopg.errors.CardinalityViolation as exc: - names = [f.name for f in unique_columns] - raise ValueError( - f"bulk_upsert() sent two {object_name} objects with the same " - f"{names} in one statement, which Postgres refuses -- it can " - "only touch a row once per statement. Collapse the duplicates " - "before calling." - ) from exc - - # Postgres emits one RETURNING row per VALUES row, in order, on the - # DO UPDATE path as much as the insert path, so the rows come back - # in the order the objects were sent. bulk_create() maps its rows - # onto objects by position for the same reason -- the two stand or - # fall together, and tests/internal/test_returning_order.py pins it. - assert len(returned_rows) == len(ordered_objs) - for obj, row in zip(ordered_objs, returned_rows): - for index, field in enumerate(meta.db_returning_fields): - setattr(obj, field.name, row[index]) - obj._state.adding = False + for positions, insert_fields in partitions: + if not positions: + continue + + # Issue the batches in conflict-key order so concurrent upserts + # touching overlapping keys lock rows in the same order and + # can't deadlock each other. sorted() is stable, so equal keys + # keep their input order and the objects themselves are never + # compared. objs is left alone, so the caller gets its own + # order back. + positions = sorted(positions, key=lambda p: sort_keys[p]) + sent_objs = [objs[position] for position in positions] + + try: + returned_rows = self._batched_insert( + sent_objs, + insert_fields, + batch_size, + on_conflict=OnConflict.UPDATE, + update_fields=conflict_update_columns, + unique_fields=unique_columns, + ) + except psycopg.errors.CardinalityViolation as exc: + names = [f.name for f in unique_columns] + raise ValueError( + f"bulk_upsert() sent two {object_name} objects with the " + f"same {names} in one statement, which Postgres refuses " + "-- it can only touch a row once per statement. Collapse " + "the duplicates before calling." + ) from exc + + # Postgres emits one RETURNING row per VALUES row, in order, on + # the DO UPDATE path as much as the insert path, so the rows + # come back in the order the objects were sent. bulk_create() + # maps its rows onto objects by position for the same reason -- + # the two stand or fall together, and + # tests/internal/test_returning_order.py pins it. + assert len(returned_rows) == len(sent_objs) + for obj, row in zip(sent_objs, returned_rows): + for index, field in enumerate(meta.db_returning_fields): + setattr(obj, field.name, row[index]) + obj._state.adding = False return objs diff --git a/plain-postgres/tests/public/test_bulk_upsert.py b/plain-postgres/tests/public/test_bulk_upsert.py index b216544247..1a78017a6c 100644 --- a/plain-postgres/tests/public/test_bulk_upsert.py +++ b/plain-postgres/tests/public/test_bulk_upsert.py @@ -7,6 +7,7 @@ from __future__ import annotations +import random from zoneinfo import ZoneInfo import pytest @@ -500,3 +501,69 @@ def test_bulk_upsert_accepts_any_sequence_of_field_references(db): assert items[0].id is not None assert UpsertPair.query.get(bucket="b", slug="s").value == 1 + + +def test_bulk_upsert_honors_an_id_the_caller_set(db): + # An explicitly set id is the caller's choice, not ours to discard for a + # generated one -- bulk_create() honors it, and so does this. + item = UpsertItem(key="a", value=1) + item.id = 5 + UpsertItem.query.bulk_upsert( + [item], update_fields=[UpsertItem.value], unique_fields=[UpsertItem.key] + ) + + assert item.id == 5 + assert UpsertItem.query.get(key="a").id == 5 + + +def test_bulk_upsert_mixes_objects_with_and_without_ids(db): + # The two go out as separate statements; both still come back hydrated + # from their own row. + with_id = UpsertItem(key="a", value=1) + with_id.id = 5 + without_id = UpsertItem(key="b", value=2) + UpsertItem.query.bulk_upsert( + [with_id, without_id], + update_fields=[UpsertItem.value], + unique_fields=[UpsertItem.key], + ) + + assert with_id.id == 5 + assert without_id.id is not None + assert without_id.id != 5 + assert {row.key: row.id for row in UpsertItem.query.all()} == { + "a": 5, + "b": without_id.id, + } + + +def test_bulk_upsert_across_several_batches_out_of_key_order(db): + # Every other key already exists, the input is shuffled, and the batches + # are smaller than the input -- so inserts and updates interleave across + # statements and the sort reorders them. Each object still has to come + # back hydrated from its own row. + for index in range(0, 8, 2): + UpsertItem(key=f"k{index}", value=0).create() + seeded = {row.key: row.id for row in UpsertItem.query.all()} + + keys = [f"k{index}" for index in range(8)] + random.Random(0).shuffle(keys) + items = [UpsertItem(key=key, value=int(key[1:])) for key in keys] + + UpsertItem.query.bulk_upsert( + items, + update_fields=[UpsertItem.value], + unique_fields=[UpsertItem.key], + batch_size=2, + ) + + assert [item.key for item in items] == keys # caller's order preserved + for item in items: + if item.key in seeded: + assert item.id == seeded[item.key] + assert item.id is not None + + assert UpsertItem.query.count() == 8 + assert {row.key: row.value for row in UpsertItem.query.all()} == { + f"k{index}": index for index in range(8) + } From 9b58e4fae555a4b6f59930b2879ac208895b9bf6 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 21:00:11 -0500 Subject: [PATCH 36/41] plain-postgres: catch a repeated update_field, and name the right call Naming the same column twice in update_fields reached Postgres as 'multiple assignments to same column', a syntax error raised mid-transaction after the batch had already gone out. It is a property of the argument, not of the data, so it belongs with the rest of the call-time validation -- which the empty-objs path runs too. _prepare_for_bulk_create's unsaved-related-object guard is shared with bulk_create(), and said so: a bulk_upsert() call with an unsaved foreign key was told 'bulk_create() prohibited to prevent data loss'. The operation name is threaded through now. --- plain-postgres/plain/postgres/query.py | 20 +++++++++--- .../tests/public/test_bulk_upsert.py | 32 +++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index ac2c920948..28171d637e 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -685,12 +685,12 @@ def create(self, **kwargs: Any) -> T: obj.create() return obj - def _prepare_for_bulk_create(self, objs: list[T]) -> None: + def _prepare_for_bulk_create(self, objs: list[T], *, operation_name: str) -> None: # The identity PK is the only PK type, so there's no literal Python # default to materialize -- obj.id stays None and the INSERT takes the # DB's DEFAULT path. for obj in objs: - obj._prepare_related_fields_for_save(operation_name="bulk_create") + obj._prepare_related_fields_for_save(operation_name=operation_name) def bulk_create( self, @@ -714,7 +714,7 @@ def bulk_create( return objs meta = self.model._model_meta fields = meta.fields - self._prepare_for_bulk_create(objs) + self._prepare_for_bulk_create(objs, operation_name="bulk_create") with transaction.atomic(savepoint=False): objs_with_id, objs_without_id = partition(lambda o: o.id is None, objs) if objs_with_id: @@ -812,6 +812,18 @@ def _resolve_bulk_upsert_fields( "the database generates its value, so the update would " "overwrite the stored one with a fresh default." ) + repeated = sorted( + { + field.name + for field in update_columns + if sum(other.name == field.name for other in update_columns) > 1 + } + ) + if repeated: + raise ValueError( + f"bulk_upsert() update_fields names {repeated} more than once; " + "Postgres assigns each column once per statement." + ) overlap = {f.name for f in update_columns} & {f.name for f in unique_columns} if overlap: raise ValueError( @@ -861,7 +873,7 @@ def bulk_upsert( meta = self.model._model_meta object_name = self.model.model_options.object_name - self._prepare_for_bulk_create(objs) + self._prepare_for_bulk_create(objs, operation_name="bulk_upsert") # A NULL conflict key never conflicts in Postgres, so the row would # always insert and the upsert would quietly be an insert. diff --git a/plain-postgres/tests/public/test_bulk_upsert.py b/plain-postgres/tests/public/test_bulk_upsert.py index 1a78017a6c..5157f0547f 100644 --- a/plain-postgres/tests/public/test_bulk_upsert.py +++ b/plain-postgres/tests/public/test_bulk_upsert.py @@ -567,3 +567,35 @@ def test_bulk_upsert_across_several_batches_out_of_key_order(db): assert {row.key: row.value for row in UpsertItem.query.all()} == { f"k{index}": index for index in range(8) } + + +def test_bulk_upsert_repeated_update_field_rejected(db): + # Postgres assigns each column once per statement; naming one twice is a + # syntax error mid-transaction, so it's caught at the call instead. + with pytest.raises(ValueError, match=r"names \['value'\] more than once"): + UpsertItem.query.bulk_upsert( + [UpsertItem(key="a", value=1)], + update_fields=[UpsertItem.value, UpsertItem.value], + unique_fields=[UpsertItem.key], + ) + + +def test_bulk_upsert_repeated_update_field_rejected_before_any_query(db): + # Validation happens at the call, so an empty objs list is checked too. + with pytest.raises(ValueError, match="more than once"): + UpsertItem.query.bulk_upsert( + [], + update_fields=[UpsertItem.value, UpsertItem.value], + unique_fields=[UpsertItem.key], + ) + + +def test_bulk_upsert_unsaved_related_object_names_bulk_upsert(db): + # The guard is shared with bulk_create(); the message has to name the call + # the user actually made. + with pytest.raises(ValueError, match=r"^bulk_upsert\(\) prohibited"): + UpsertScoped.query.bulk_upsert( + [UpsertScoped(tenant=UpsertTenant(name="unsaved"), slug="s", value=1)], + update_fields=[UpsertScoped.value], + unique_fields=[UpsertScoped.tenant, UpsertScoped.slug], + ) From 1a2a5c25731984aab3e8398c401b4ace5e324f87 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 21:01:08 -0500 Subject: [PATCH 37/41] plain-postgres: drive a numeric conflict key from a real column The decimal scale handling in conflict_sort_value had only a unit test behind it, using a DecimalField borrowed from an unrelated fixture. The end-to-end claim -- that upserting Decimal("1.00") finds the row Decimal("1.0") wrote -- was never actually run against Postgres. UpsertDecimalKey is numeric(12,4) with a unique constraint on it, which is the shape that makes the two spellings one row. Three tests: the scales conflict across calls, a signed zero conflicts with a plain one, and two scales of the same number in one statement are reported as the duplicate they are rather than as a raw cardinality violation. --- plain-postgres/plain/postgres/query.py | 5 +- .../migrations/0026_upsertdecimalkey.py | 20 +++++++ .../tests/app/examples/models/upsert.py | 18 ++++++ .../tests/public/test_bulk_upsert.py | 58 +++++++++++++++++++ 4 files changed, 99 insertions(+), 2 deletions(-) create mode 100644 plain-postgres/tests/app/examples/migrations/0026_upsertdecimalkey.py diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index 28171d637e..1daaea23f4 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -881,8 +881,9 @@ def bulk_upsert( for obj in objs: key = [] for field in unique_columns: - # Prepared once here and handed to the sort key, rather than - # prepared again inside it. + # Prepared once here and handed to the sort key, rather + # than prepared again inside it. A malformed value is rejected + # at this point, before any statement goes out. value = field.get_prep_value(field.value_from_object(obj)) if value is None: raise ValueError( diff --git a/plain-postgres/tests/app/examples/migrations/0026_upsertdecimalkey.py b/plain-postgres/tests/app/examples/migrations/0026_upsertdecimalkey.py new file mode 100644 index 0000000000..8f1f5d09ef --- /dev/null +++ b/plain-postgres/tests/app/examples/migrations/0026_upsertdecimalkey.py @@ -0,0 +1,20 @@ +# Generated by Plain 0.163.1 on 2026-09-20 02:00 + +from plain.postgres import migrations + +from plain import postgres + + +class Migration(migrations.Migration): + dependencies = (("examples", "0025_upsertfloatkey"),) + + operations = ( + migrations.CreateModel( + name="UpsertDecimalKey", + fields=[ + ("id", postgres.PrimaryKeyField()), + ("amount", postgres.DecimalField(decimal_places=4, max_digits=12)), + ("value", postgres.IntegerField(default=0)), + ], + ), + ) diff --git a/plain-postgres/tests/app/examples/models/upsert.py b/plain-postgres/tests/app/examples/models/upsert.py index cbc6882804..906152122c 100644 --- a/plain-postgres/tests/app/examples/models/upsert.py +++ b/plain-postgres/tests/app/examples/models/upsert.py @@ -2,6 +2,7 @@ from __future__ import annotations +from decimal import Decimal from zoneinfo import ZoneInfo from plain.postgres import Field, types @@ -98,3 +99,20 @@ class UpsertFloatKey(postgres.Model): ), ] ) + + +@postgres.register_model +class UpsertDecimalKey(postgres.Model): + """A numeric conflict key -- the column type whose scale Python keeps and + Postgres does not.""" + + amount: Field[Decimal] = types.DecimalField(max_digits=12, decimal_places=4) + value: Field[int] = types.IntegerField(default=0) + + model_options = postgres.Options( + constraints=[ + postgres.UniqueConstraint( + fields=["amount"], name="upsertdecimalkey_amount_unique" + ), + ] + ) diff --git a/plain-postgres/tests/public/test_bulk_upsert.py b/plain-postgres/tests/public/test_bulk_upsert.py index 5157f0547f..67f8f4dc80 100644 --- a/plain-postgres/tests/public/test_bulk_upsert.py +++ b/plain-postgres/tests/public/test_bulk_upsert.py @@ -8,6 +8,7 @@ from __future__ import annotations import random +from decimal import Decimal from zoneinfo import ZoneInfo import pytest @@ -15,6 +16,7 @@ from app.examples.models.mixins import MixinTestModel from app.examples.models.returning import ReturningEvent from app.examples.models.upsert import ( + UpsertDecimalKey, UpsertFloatKey, UpsertItem, UpsertPair, @@ -599,3 +601,59 @@ def test_bulk_upsert_unsaved_related_object_names_bulk_upsert(db): update_fields=[UpsertScoped.value], unique_fields=[UpsertScoped.tenant, UpsertScoped.slug], ) + + +def test_bulk_upsert_decimal_key_conflicts_across_scales(db): + # numeric(12,4) stores 1.0 and 1.00 as the same value, so the second call + # has to find the first one's row rather than insert beside it. + first = [UpsertDecimalKey(amount=Decimal("1.0"), value=1)] + UpsertDecimalKey.query.bulk_upsert( + first, + update_fields=[UpsertDecimalKey.value], + unique_fields=[UpsertDecimalKey.amount], + ) + + second = [UpsertDecimalKey(amount=Decimal("1.00"), value=2)] + UpsertDecimalKey.query.bulk_upsert( + second, + update_fields=[UpsertDecimalKey.value], + unique_fields=[UpsertDecimalKey.amount], + ) + + assert second[0].id == first[0].id + assert UpsertDecimalKey.query.count() == 1 + assert UpsertDecimalKey.query.get(id=first[0].id).value == 2 + + +def test_bulk_upsert_decimal_key_conflicts_across_signed_zero(db): + zero = [UpsertDecimalKey(amount=Decimal("0.0"), value=1)] + UpsertDecimalKey.query.bulk_upsert( + zero, + update_fields=[UpsertDecimalKey.value], + unique_fields=[UpsertDecimalKey.amount], + ) + + negative_zero = [UpsertDecimalKey(amount=Decimal("-0.00"), value=2)] + UpsertDecimalKey.query.bulk_upsert( + negative_zero, + update_fields=[UpsertDecimalKey.value], + unique_fields=[UpsertDecimalKey.amount], + ) + + assert negative_zero[0].id == zero[0].id + assert UpsertDecimalKey.query.count() == 1 + + +def test_bulk_upsert_decimal_keys_at_different_scales_in_one_batch(db): + # Postgres holds these equal, so they are the same row twice in one + # statement -- the sort renders them identically, and the cardinality + # violation is reported as the duplicate it is. + with pytest.raises(ValueError, match=r"same \['amount'\] in one statement"): + UpsertDecimalKey.query.bulk_upsert( + [ + UpsertDecimalKey(amount=Decimal("1.0"), value=1), + UpsertDecimalKey(amount=Decimal("1.000"), value=2), + ], + update_fields=[UpsertDecimalKey.value], + unique_fields=[UpsertDecimalKey.amount], + ) From c64eea05bdf3b40abf6525b0a28e9e568e7d9fc3 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 21:24:47 -0500 Subject: [PATCH 38/41] plain-postgres: sort every bulk_upsert object together, not each shape Splitting objects with ids from objects without them broke the thing the sort exists for. Each partition was sorted on its own and the with-id one always went first, so the lock order followed which objects happened to carry ids rather than the keys. Two callers over the same four keys sent a,b,c,d and c,d,a,b -- exactly the deadlock the sort is supposed to rule out. Reproduced by capturing the emitted parameters: caller A one statement ['a','b','c','d'], caller B two, ['c','d'] then ['a','b']. The sort now spans every object, and a new statement starts only where the row shape changes as the sorted list is walked. Same keys, same order, whoever is calling; an extra statement only where ids interleave, which is rare and never reorders anything. batch_size still caps the statement inside a run. Pinned by asserting the emitted key order rather than racing two sessions: the order is the entire guarantee, it is exactly what differed before, and asserting it directly cannot flake. --- plain-postgres/plain/postgres/query.py | 47 +++++----- .../internal/test_conflict_lock_order.py | 90 +++++++++++++++++++ 2 files changed, 113 insertions(+), 24 deletions(-) create mode 100644 plain-postgres/tests/internal/test_conflict_lock_order.py diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index 1daaea23f4..5159afe048 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -908,35 +908,34 @@ def bulk_upsert( # doesn't lets Postgres generate the identity value. bulk_create() # splits the same way -- an id the caller set is theirs, not ours to # throw away. When the primary key *is* the conflict target every - # object has one, so there is nothing to split. + # object has one, so every row has the same shape. fields = meta.fields fields_without_pk = [f for f in fields if not isinstance(f, PrimaryKeyField)] pk_is_unique = any(f.primary_key for f in unique_columns) - if pk_is_unique: - partitions = [(list(range(len(objs))), fields)] - else: - partitions = [ - ([p for p, obj in enumerate(objs) if obj.id is not None], fields), - ( - [p for p, obj in enumerate(objs) if obj.id is None], - fields_without_pk, - ), - ] - - with transaction.atomic(savepoint=False): - for positions, insert_fields in partitions: - if not positions: - continue - # Issue the batches in conflict-key order so concurrent upserts - # touching overlapping keys lock rows in the same order and - # can't deadlock each other. sorted() is stable, so equal keys - # keep their input order and the objects themselves are never - # compared. objs is left alone, so the caller gets its own - # order back. - positions = sorted(positions, key=lambda p: sort_keys[p]) - sent_objs = [objs[position] for position in positions] + # Lock rows in conflict-key order, so two callers touching overlapping + # keys can't deadlock each other. sorted() is stable, so equal keys keep + # their input order and the objects themselves are never compared. objs + # is left alone -- the caller gets its own order back. + # + # The sort has to span *every* object rather than each shape on its own: + # two callers holding the same keys but different ids would otherwise + # lock them in different orders, which is the deadlock this exists to + # avoid. So walk the sorted objects and start a new statement only where + # the shape changes -- an extra statement only where ids interleave. + runs: list[tuple[list[T], Sequence[Field]]] = [] + for position in sorted(range(len(objs)), key=lambda p: sort_keys[p]): + obj = objs[position] + insert_fields = ( + fields if pk_is_unique or obj.id is not None else fields_without_pk + ) + if runs and runs[-1][1] is insert_fields: + runs[-1][0].append(obj) + else: + runs.append(([obj], insert_fields)) + with transaction.atomic(savepoint=False): + for sent_objs, insert_fields in runs: try: returned_rows = self._batched_insert( sent_objs, diff --git a/plain-postgres/tests/internal/test_conflict_lock_order.py b/plain-postgres/tests/internal/test_conflict_lock_order.py new file mode 100644 index 0000000000..007ee8dbce --- /dev/null +++ b/plain-postgres/tests/internal/test_conflict_lock_order.py @@ -0,0 +1,90 @@ +"""bulk_upsert() locks rows in conflict-key order, whatever shape they are. + +Two callers touching overlapping keys deadlock unless they take the locks in +the same order. The order is the sorted conflict key -- and it has to span +every object, not each statement, because a caller whose objects carry ids +sends them in a separate statement from one whose objects don't. Sorting +within each statement would let two callers holding the same four keys lock +them as (a,b),(c,d) and (c,d),(a,b). + +This pins the emitted order rather than racing two sessions: the order is the +whole guarantee, and asserting it directly can't flake. +""" + +from __future__ import annotations + +from app.examples.models.upsert import UpsertItem +from plain.postgres.query import QuerySet + + +def sent_key_runs(monkeypatch, items) -> list[list[str]]: + """The keys bulk_upsert() sends, grouped by statement.""" + runs: list[list[str]] = [] + original = QuerySet._batched_insert + + def recording(self, objs, fields, batch_size, **kwargs): + runs.append([obj.key for obj in objs]) + return original(self, objs, fields, batch_size, **kwargs) + + monkeypatch.setattr(QuerySet, "_batched_insert", recording) + UpsertItem.query.bulk_upsert( + items, update_fields=[UpsertItem.value], unique_fields=[UpsertItem.key] + ) + return runs + + +def test_lock_order_is_the_sorted_key_when_no_object_carries_an_id(db, monkeypatch): + items = [UpsertItem(key=key, value=1) for key in ("d", "b", "c", "a")] + runs = sent_key_runs(monkeypatch, items) + + assert runs == [["a", "b", "c", "d"]] + + +def test_lock_order_is_the_same_when_some_objects_carry_ids(db, monkeypatch): + # The reviewer's scenario: the same four keys, but c and d arrive with ids, + # so they need their own statement. The keys still go out in sorted order. + items = [] + for key in ("d", "b", "c", "a"): + item = UpsertItem(key=key, value=1) + if key in ("c", "d"): + item.id = {"c": 101, "d": 102}[key] + items.append(item) + runs = sent_key_runs(monkeypatch, items) + + assert runs == [["a", "b"], ["c", "d"]] + assert [key for run in runs for key in run] == ["a", "b", "c", "d"] + + +def test_ids_interleaved_through_the_key_order_cost_a_statement_each(db, monkeypatch): + # A new statement starts only where the shape changes, so alternating ids + # is the worst case -- and the key order still holds across all of them. + items = [] + for index, key in enumerate(("a", "b", "c", "d")): + item = UpsertItem(key=key, value=1) + if index % 2 == 0: + item.id = 100 + index + items.append(item) + runs = sent_key_runs(monkeypatch, items) + + assert runs == [["a"], ["b"], ["c"], ["d"]] + + +def test_batch_size_splits_within_a_run_without_disturbing_the_order(db, monkeypatch): + items = [UpsertItem(key=key, value=1) for key in ("d", "b", "c", "a")] + runs: list[list[str]] = [] + original = QuerySet._batched_insert + + def recording(self, objs, fields, batch_size, **kwargs): + runs.append([obj.key for obj in objs]) + return original(self, objs, fields, batch_size, **kwargs) + + monkeypatch.setattr(QuerySet, "_batched_insert", recording) + UpsertItem.query.bulk_upsert( + items, + update_fields=[UpsertItem.value], + unique_fields=[UpsertItem.key], + batch_size=2, + ) + + # batch_size caps the statement inside a run; the run is still one call. + assert runs == [["a", "b", "c", "d"]] From d4c69e10324c602d73fa67a7318ad4d16fdf31d5 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 21:25:40 -0500 Subject: [PATCH 39/41] plain-postgres: say what bulk_upsert does with an id you set Four things the docs either said loosely or not at all, each now with a test behind it. An id you set is kept when the row is inserted, but a conflicting row already has one, and that is what gets hydrated back -- the stored row is the truth. The README said bulk_create's sentence about keeping the caller's id, which is only half of it here. An id colliding with a different row raises psycopg.errors.UniqueViolation, consistent with every other set-based write. 'The returned list is the caller's list' was wrong in a way that invites aliasing bugs: it is the caller's objects, in the caller's order, in a new list. Queryset filters are ignored, same as bulk_create -- undocumented until now. Plus the two pins the reviewer noticed were missing: a unique Index is not a conflict target, and update_fields cannot name the primary key. --- plain-postgres/plain/postgres/README.md | 25 ++++--- .../tests/public/test_bulk_upsert.py | 67 +++++++++++++++++++ 2 files changed, 84 insertions(+), 8 deletions(-) diff --git a/plain-postgres/plain/postgres/README.md b/plain-postgres/plain/postgres/README.md index 4f7cebd767..b62c54e090 100644 --- a/plain-postgres/plain/postgres/README.md +++ b/plain-postgres/plain/postgres/README.md @@ -470,9 +470,9 @@ already exist in a single statement, use `bulk_upsert` (below). `bulk_upsert(objs, *, update_fields, unique_fields, batch_size=None)` issues one `INSERT ... ON CONFLICT (unique_fields) DO UPDATE SET ... RETURNING` per batch. Rows that don't exist yet are inserted; rows that collide on `unique_fields` have -their `update_fields` overwritten. Every object comes back — inserted or updated — -with its DB-generated fields (primary key, DB defaults) populated, in the order -you passed them in. +their `update_fields` overwritten. You get back the objects you passed in, in the +order you passed them (a new list — `objs` itself is never reordered), each with +its DB-generated fields (primary key, DB defaults) populated. ```python # Insert new items, refresh `value`/`expires_at` on any existing key. @@ -492,8 +492,9 @@ CachedItem.query.bulk_upsert( - `unique_fields` must name the **primary key** or a `UniqueConstraint` declared on the model (no condition, no expressions) — this is the conflict target. A unique `Index` is not enough; declare a `UniqueConstraint`. -- `update_fields` must be concrete, non-primary-key, and must not overlap - `unique_fields`. A column the database fills in (`create_now`, +- `update_fields` must be concrete, non-primary-key, must not name the same + column twice (Postgres assigns each column once per statement), and must not + overlap `unique_fields`. A column the database fills in (`create_now`, `generate=True`, `RandomStringField`) can't be named either — the update would overwrite the stored value with a freshly evaluated default. - Every object must have a non-null value for every unique field. `NULL` never @@ -507,9 +508,17 @@ CachedItem.query.bulk_upsert( - **`update_now=True` columns are refreshed on a conflict automatically.** You don't name them in `update_fields`; a row that gets updated gets a fresh stamp, and the object handed back carries the same one. -- Batches are issued in conflict-key order, so concurrent `bulk_upsert` calls - over overlapping keys can't deadlock each other. Returned rows are mapped onto - the objects by position, exactly as `bulk_create` does. +- **An `id` you set is kept on the insert path; on a conflict the stored row + wins.** A new row is written with the `id` you gave it. A conflicting one + already has an `id`, and that is the one hydrated back onto your object — the + row in the table is the truth. An `id` that collides with a _different_ row + raises `psycopg.errors.UniqueViolation`, like any set-based write. +- Every object is sorted by its conflict key before anything is sent, so + concurrent `bulk_upsert` calls over overlapping keys lock rows in the same + order and can't deadlock each other. Returned rows are mapped onto the objects + by position, exactly as `bulk_create` does. +- Like `bulk_create`, the write is against the table: a filter on the queryset + you call it from doesn't narrow or exclude anything. #### Use queryset `.update()` / `.delete()` for mass operations diff --git a/plain-postgres/tests/public/test_bulk_upsert.py b/plain-postgres/tests/public/test_bulk_upsert.py index 67f8f4dc80..3b32594a41 100644 --- a/plain-postgres/tests/public/test_bulk_upsert.py +++ b/plain-postgres/tests/public/test_bulk_upsert.py @@ -11,8 +11,10 @@ from decimal import Decimal from zoneinfo import ZoneInfo +import psycopg import pytest from app.examples.models.defaults import DBDefaultsExample +from app.examples.models.indexes import IndexExample from app.examples.models.mixins import MixinTestModel from app.examples.models.returning import ReturningEvent from app.examples.models.upsert import ( @@ -657,3 +659,68 @@ def test_bulk_upsert_decimal_keys_at_different_scales_in_one_batch(db): update_fields=[UpsertDecimalKey.value], unique_fields=[UpsertDecimalKey.amount], ) + + +def test_bulk_upsert_conflict_hydrates_the_stored_id_over_the_caller_s(db): + # On the insert path a caller-set id is kept. On the conflict path the + # stored row is the truth: its id is what comes back, not the one passed. + UpsertItem(key="a", value=1).create() + stored_id = UpsertItem.query.get(key="a").id + + item = UpsertItem(key="a", value=2) + item.id = 99 + UpsertItem.query.bulk_upsert( + [item], update_fields=[UpsertItem.value], unique_fields=[UpsertItem.key] + ) + + assert item.id == stored_id + assert UpsertItem.query.count() == 1 + assert UpsertItem.query.get(key="a").value == 2 + + +def test_bulk_upsert_id_colliding_with_another_row_raises(db): + # The conflict target is `key`, so an id that collides with a different + # row is an ordinary primary key violation -- raised raw, like any + # set-based write. + UpsertItem(key="taken", value=1).create() + taken_id = UpsertItem.query.get(key="taken").id + + item = UpsertItem(key="new", value=1) + item.id = taken_id + with pytest.raises(psycopg.errors.UniqueViolation): + UpsertItem.query.bulk_upsert( + [item], update_fields=[UpsertItem.value], unique_fields=[UpsertItem.key] + ) + + +def test_bulk_upsert_unique_index_is_not_a_conflict_target(db): + # A unique Index would work as a Postgres arbiter, but bulk_upsert asks + # for a declared UniqueConstraint so the target is explicit in the model. + with pytest.raises(ValueError, match="must name the primary key"): + IndexExample.query.bulk_upsert( + [IndexExample(name="n", description="d")], + update_fields=[IndexExample.description], + unique_fields=[IndexExample.name], + ) + + +def test_bulk_upsert_cannot_update_the_primary_key(db): + with pytest.raises(ValueError, match="cannot update primary key fields"): + UpsertItem.query.bulk_upsert( + [UpsertItem(key="a", value=1)], + update_fields=[UpsertItem.id], + unique_fields=[UpsertItem.key], + ) + + +def test_bulk_upsert_ignores_queryset_filters(db): + # Like bulk_create, the write is against the table -- a filter on the + # queryset it is called from does not narrow or exclude anything. + UpsertItem(key="a", value=1).create() + + items = [UpsertItem(key="a", value=2), UpsertItem(key="b", value=3)] + UpsertItem.query.filter(key="nothing-matches-this").bulk_upsert( + items, update_fields=[UpsertItem.value], unique_fields=[UpsertItem.key] + ) + + assert {row.key: row.value for row in UpsertItem.query.all()} == {"a": 2, "b": 3} From a2e9683f81cff3551b9b61817341812446841d02 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 21:39:14 -0500 Subject: [PATCH 40/41] plain-postgres: finish the master merge, taking #85's final shapes Two leftovers from the pre-squash returning branch survived the merge as context rather than as conflicts, so they came through unnoticed. delete() and update() kept the _execute_delete/_execute_update split, which existed so the old runtime ReturningQuerySet subclass could call them. #85 landed with ReturningQuerySet as a TYPE_CHECKING-only type, so nothing calls them any more and master inlines both -- this was re-introducing a layer master had removed. __init__.py still exported ReturningQuerySet at runtime, from the same era. It is a static type now, and master doesn't export it. --- plain-postgres/plain/postgres/__init__.py | 3 +-- plain-postgres/plain/postgres/query.py | 19 ------------------- 2 files changed, 1 insertion(+), 21 deletions(-) diff --git a/plain-postgres/plain/postgres/__init__.py b/plain-postgres/plain/postgres/__init__.py index abe33f0b09..7b34ed355c 100644 --- a/plain-postgres/plain/postgres/__init__.py +++ b/plain-postgres/plain/postgres/__init__.py @@ -49,7 +49,7 @@ ) from .indexes import Index from .options import Options -from .query import QuerySet, ReturningQuerySet +from .query import QuerySet from .query_utils import Q from . import types @@ -95,7 +95,6 @@ "Q", "QuerySet", "RandomStringField", - "ReturningQuerySet", "ReverseForeignKey", "ReverseManyToMany", "SmallIntegerField", diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index afa666a380..3ff9a2ffb7 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -1452,16 +1452,6 @@ def delete(self) -> int: clause. The queryset is a ReturningQuerySet to a type checker by then, and its delete() is declared to return them. """ - return self._execute_delete() - - def _execute_delete(self) -> Any: - """Run the DELETE. - - Returns the rowcount, or — when returning() set columns on this - queryset — the converted RETURNING rows for ReturningQuerySet.delete() - to hydrate. Only the target table's rows come back; cascade deletes - never appear in a RETURNING clause. - """ if self.sql_query.is_sliced: raise TypeError("Cannot use 'limit' or 'offset' with delete().") if self.sql_query.distinct or self.sql_query.distinct_fields: @@ -1510,15 +1500,6 @@ def update(self, **kwargs: Any) -> int: The queryset is a ReturningQuerySet to a type checker by then, and its update() is declared to return them. """ - return self._execute_update(kwargs) - - def _execute_update(self, kwargs: dict[str, Any]) -> Any: - """Run the UPDATE. - - Returns the rowcount, or — when returning() set columns on this - queryset — the converted RETURNING rows for ReturningQuerySet.update() - to hydrate. - """ if self.sql_query.is_sliced: raise TypeError("Cannot update a query once a slice has been taken.") if self._fields is not None: From ac5278d3ce99e5a8c674977be89f1318766df014 Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Sat, 19 Sep 2026 21:53:11 -0500 Subject: [PATCH 41/41] plain-postgres: drop the merge-drifted ReturningQuerySet export from query.__all__ --- plain-postgres/plain/postgres/query.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plain-postgres/plain/postgres/query.py b/plain-postgres/plain/postgres/query.py index 3ff9a2ffb7..de02e820c4 100644 --- a/plain-postgres/plain/postgres/query.py +++ b/plain-postgres/plain/postgres/query.py @@ -54,7 +54,7 @@ from plain.utils.functional import partition # Re-exports for public API -__all__ = ["F", "Prefetch", "Q", "QuerySet", "RawQuerySet", "ReturningQuerySet"] +__all__ = ["F", "Prefetch", "Q", "QuerySet", "RawQuerySet"] if TYPE_CHECKING: from plain.postgres import Model