Skip to content

Fix 15 correctness bugs from the full-codebase review#18

Merged
vsdudakov merged 7 commits into
mainfrom
review-fixes-2026-07
Jul 10, 2026
Merged

Fix 15 correctness bugs from the full-codebase review#18
vsdudakov merged 7 commits into
mainfrom
review-fixes-2026-07

Conversation

@vsdudakov

Copy link
Copy Markdown
Owner

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)

  • ZoneInfo-aware datetime binding — with use_tz=True and any variable-offset IANA timezone (e.g. Europe/Berlin), every save/filter raised TypeError: expected a datetime without tzinfo. Aware datetimes whose fixed-offset extraction fails now normalize via astimezone(UTC).
  • execute_many type corruption (Postgres) — parameter types were declared from the first row only; a later row with a different type (e.g. [[1], [2.5]] into DOUBLE 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), with to_sql_checked hardening as a backstop.
  • sync_fast_path event-loop deadlock (SQLite) — a sync-path query called block_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.
  • MSSQL bulk insert pk backfillSCOPE_IDENTITY() - @@ROWCOUNT + 1 arithmetic 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 via OUTPUT INSERTED.<pk>.

ORM (Python)

  • bulk_create discarded explicit pks on auto-increment models and overwrote obj.pk with generated ids; explicit pks are now honoured (wrapped in IDENTITY_INSERT where the dialect needs it) and mixed explicit/auto batches raise ValueError.
  • get_or_create/update_or_create dropped the queryset's connection binding and filters: author.books.get_or_create(...) created orphan rows without the FK, and using_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).
  • Decode-plan race on reads — the snapshot-before-await invariant from 410ef33 (writes) now also covers _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(...) rendered ORDER 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).
  • MSSQL m2m deletes always failed (join-table FKs are NO ACTION there): Model.delete and QuerySet.delete now 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.
  • Bare date in a DatetimeField was stored as date-only text on SQLite (refetched as str, breaking comparisons); it now widens to a tz-aware midnight datetime at assignment while __date lookups keep binding date-only values.
  • through_fields orientation now follows Django's (owner_column, target_column) order — previously the columns were crossed, causing IntegrityErrors or silently swapped m2m rows. The forward_key/backward_key kwargs keep their documented (Tortoise-compatible) meaning; only the docstrings were wrong and are fixed.
  • Lazy relation reads bypassed custom Meta.manager while prefetch respected it, leaking soft-deleted rows through author.books; both paths are now manager-scoped (add/remove/clear stay raw by design).
  • Migrations: _field_source now emits pk=True for FK/O2O primary keys (previously a fresh DB got a table with no primary key plus a never-converging phantom AlterField); MySQL MODIFY COLUMN restates the column DEFAULT instead of silently dropping it; CreateModelIfNotExists is genuinely re-run safe on MySQL (indexes folded into the guarded CREATE TABLE) and MSSQL (sys.indexes existence guards).

Tests

Every fix has regression tests, placed in the topical test files; the old test_review_fixes.py grab-bag is redistributed the same way. Where feasible, tests were verified to fail against the pre-fix code.

  • sqlite suite: 1250 passed
  • postgres suite (live server): 1251 passed
  • cargo test --lib: 53 passed; ruff + ty clean

Notes for reviewers

  • MSSQL/MySQL-specific behavior is covered by rendered-SQL assertions and simulated capability flags (no live servers in this environment) — worth a pass against real servers before release.
  • OUTPUT INSERTED fails on tables with triggers (documented in a comment; matches common ORM behavior).
  • Known pre-existing gaps left out of scope, flagged in comments where relevant: MySQL standalone AddIndexIfNotExists remains unguarded (MySQL has no SQL-level guard), m2m prefetch still bypasses a custom target manager, and QuerySet.update(field=date(...)) still binds a raw date.

vsdudakov and others added 7 commits July 10, 2026 16:52
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>
@vsdudakov vsdudakov merged commit a2bee5c into main Jul 10, 2026
4 checks passed
@vsdudakov vsdudakov deleted the review-fixes-2026-07 branch July 10, 2026 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant