Fix 15 correctness bugs from the full-codebase review#18
Merged
Conversation
Engine (Rust): - Bind ZoneInfo-aware datetimes: variable-offset zones (use_tz with any IANA timezone) now normalize via astimezone(UTC) instead of raising TypeError on the FixedOffset extraction. - postgres execute_many: unify parameter types across all rows (widening INT8->FLOAT8->NUMERIC, clear error on incompatible mixes) instead of declaring from the first row and silently corrupting later rows; to_sql_checked hardening on the Int/Float fallback arms. - sync_fast_path: non-blocking pool checkout with fallback to the async path when the pool is busy, so a query issued while a transaction pins the only connection no longer deadlocks the event loop. - mssql: multi-row INSERT returns real generated ids via OUTPUT INSERTED.<pk> instead of the SCOPE_IDENTITY()-@@rowcount arithmetic, which is wrong under concurrent identity interleaving. ORM (Python): - bulk_create honours explicit auto-increment pks (IDENTITY_INSERT wrap where needed); mixed explicit/auto batches raise ValueError. - get_or_create/update_or_create create on the queryset's bound connection and inherit root-level equality filters, so related-manager get_or_create sets the FK and using_db() no longer writes to the default database. - Read paths snapshot the decode plan before awaiting the fetch, extending the 410ef33 invariant to _fetch/_fetch_annotated/ _fetch_select_related. - Grouped queries keep order_by-referenced annotations in the SELECT when the values()/values_list() projection omits them. - Model.delete and QuerySet.delete remove m2m join rows first on dialects whose join-table FKs do not cascade (SQL Server). - DatetimeField widens a bare date to a (tz-aware) midnight datetime at assignment; __date lookups still bind date-only values. - through_fields follows Django's (owner, target) column order; the forward_key/backward_key kwargs keep their documented meaning. - Lazy relation reads build through Meta.manager, matching prefetch. - Migrations: _field_source emits pk=True for FK/O2O primary keys (no more pk-less tables and non-converging AlterField loops); MySQL MODIFY COLUMN restates the column DEFAULT; CreateModelIfNotExists is re-run safe on MySQL (inline indexes) and MSSQL (sys.indexes guards). Tests are placed in the topical test files (test_review_fixes.py is redistributed the same way); full suite green on sqlite and postgres.
Oracle: the driver's 11-byte TIMESTAMP bind with a zero fraction compares unequal (byte-wise) to the server's canonical 7-byte storage, so col >= :midnight dropped exact-midnight rows. Zero-fraction datetimes now bind as DATE (full second precision, promoted to TIMESTAMP in mixed comparisons). Test: under use_tz every backend returns the aware UTC instant — the naive-DATETIME dialects re-attach UTC on read via _datetime_from_db — so the naive-expectation branch for mysql/mariadb/oracle/mssql was wrong. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
_from_db_row_fields/_from_db_rows_fields became dead code when the read paths started snapshotting the decode plan in queryset.py — removed. New tests cover the remaining missed lines: DatetimeField.to_db ISO-string parsing, _filter_create_kwargs pk aliasing / expression values / non-column names / AND-nested Q children, and _delete_m2m_rows deduping mirrored m2m declarations that share one join table. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Re-ran bench.py on all four backends (PostgreSQL 18.4, MySQL 8.4.10, MariaDB 11.8, SQLite; Apple Silicon, Python 3.12, N=5000, median of 5) and synced the tables in README.md, benchmarks/README.md, docs/performance.md, the BACKENDS dict in plot_benchmarks.py, and the regenerated chart PNGs. SQLite group_by is now a tie with SQLObject rather than a loss; the rest moved within noise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ORM (Python): - DatetimeField.to_db widens a bare date to a (tz-aware under use_tz) midnight datetime, closing the setattr+save()/update()/bulk_update() bypass that stored date-only text; the __date lookup narrows its comparison value to a date itself, so truncated comparisons still bind date-only values. - Related-manager .update()/.delete() delegate through an unscoped queryset: a soft-delete Meta.manager read scope no longer silently skips rows on relation-wide writes. - m2m join-row cleanup on non-cascading dialects (Model.delete and QuerySet.delete) now runs inside a transaction — a failed row delete rolls the join-row deletes back instead of orphaning them. New resolve_connection_name() helper resolves the name to open it on; an active transaction is reused, a raw connection object runs inline. - MSSQL bulk_create routes generated ids through OUTPUT ... INTO a table variable (works on tables with enabled triggers, where a bare OUTPUT clause raises error 334) and zips them in arrival order (sorting misassigned ids under a negative IDENTITY increment). - bulk_get_or_create/bulk_update_or_create partition a mix of explicit-pk and auto-pk creates instead of tripping bulk_create's mixed-batch ValueError. - through_fields docstring documents the 1.15 (owner, target) order change for upgrading declarations. Engine (Rust, postgres): - execute_many type unification: differing numeric mixes widen to NUMERIC (INT8->FLOAT8 silently rounded integers above 2^53); naive/aware datetime mixes unify to TIMESTAMPTZ and TEXT+UUID to UUID instead of erroring (both have exact encodings). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The base _write_qs default is reached via m2m manager bulk writes, and _bulk_create_partitioned's non-auto-pk path via a CharField-pk model in bulk_get_or_create. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A full review of the main branch surfaced 15 verified correctness bugs; this PR fixes all of them, each with regression tests placed in the topical test files.
Engine (Rust)
use_tz=Trueand any variable-offset IANA timezone (e.g.Europe/Berlin), every save/filter raisedTypeError: expected a datetime without tzinfo. Aware datetimes whose fixed-offset extraction fails now normalize viaastimezone(UTC).execute_manytype corruption (Postgres) — parameter types were declared from the first row only; a later row with a different type (e.g.[[1], [2.5]]intoDOUBLE PRECISION) wrote raw f64 bits into an INT8 parameter and silently stored a garbage value. Types are now unified across all rows (numeric widening INT8→FLOAT8→NUMERIC, clear error for incompatible mixes), withto_sql_checkedhardening as a backstop.sync_fast_pathevent-loop deadlock (SQLite) — a sync-path query calledblock_on(pool.get())on the loop thread; with a transaction pinning the pool's only connection the process hung forever. The sync path now uses a non-blocking checkout and falls back to the async path when the pool is busy.SCOPE_IDENTITY() - @@ROWCOUNT + 1arithmetic assumed contiguous identity allocation, which SQL Server does not guarantee under concurrency, so objects could silently receive other rows' pks. The multi-row INSERT now returns real ids viaOUTPUT INSERTED.<pk>.ORM (Python)
bulk_creatediscarded explicit pks on auto-increment models and overwroteobj.pkwith generated ids; explicit pks are now honoured (wrapped inIDENTITY_INSERTwhere the dialect needs it) and mixed explicit/auto batches raiseValueError.get_or_create/update_or_createdropped the queryset's connection binding and filters:author.books.get_or_create(...)created orphan rows without the FK, andusing_db(...)querysets selected on one database but inserted into the default. The create half now runs on the bound connection and inherits root-level equality filters (explicit kwargs/defaults win)._fetch/_fetch_annotated/_fetch_select_related, so a concurrent other-dialect compile can no longer swap decoders mid-hydration.annotate(n=...).order_by("-n").values_list(...)renderedORDER BY "n"with no"n"in the SELECT — an error on PG/MySQL/MSSQL and silently wrong ordering on SQLite. Ordering annotations are kept in the SELECT (never leaked into results).NO ACTIONthere):Model.deleteandQuerySet.deletenow remove join rows first on non-cascading dialects; the bulk path materializes affected pks up front so join-dependent filters (e.g.annotate(n=Count(...)).filter(n__gte=2)) stay correct.datein aDatetimeFieldwas stored as date-only text on SQLite (refetched asstr, breaking comparisons); it now widens to a tz-aware midnight datetime at assignment while__datelookups keep binding date-only values.through_fieldsorientation now follows Django's(owner_column, target_column)order — previously the columns were crossed, causing IntegrityErrors or silently swapped m2m rows. Theforward_key/backward_keykwargs keep their documented (Tortoise-compatible) meaning; only the docstrings were wrong and are fixed.Meta.managerwhile prefetch respected it, leaking soft-deleted rows throughauthor.books; both paths are now manager-scoped (add/remove/clearstay raw by design)._field_sourcenow emitspk=Truefor FK/O2O primary keys (previously a fresh DB got a table with no primary key plus a never-converging phantomAlterField); MySQLMODIFY COLUMNrestates the columnDEFAULTinstead of silently dropping it;CreateModelIfNotExistsis genuinely re-run safe on MySQL (indexes folded into the guardedCREATE TABLE) and MSSQL (sys.indexesexistence guards).Tests
Every fix has regression tests, placed in the topical test files; the old
test_review_fixes.pygrab-bag is redistributed the same way. Where feasible, tests were verified to fail against the pre-fix code.cargo test --lib: 53 passed; ruff + ty cleanNotes for reviewers
OUTPUT INSERTEDfails on tables with triggers (documented in a comment; matches common ORM behavior).AddIndexIfNotExistsremains unguarded (MySQL has no SQL-level guard), m2m prefetch still bypasses a custom target manager, andQuerySet.update(field=date(...))still binds a raw date.