v0.9.7: fix diff bug, Django 5.2/6.0 + Python 3.13/3.14 compat, CI/packaging modernization#22
Conversation
- fixture.py:129 fix always-False `not diff` → `not different` (temp file leak + TypeError in assertNoDiff on exact match); add regression test - CI: checkout@v4, setup-python@v5, codecov-action@v4; drop py3.8/3.9, add py3.13/3.14; add release.yml for automated PyPI publish on tag push - tox: add django52/django60, drop EOL django40/41/32; migrate qa env from flake8 to ruff; fix [gh-actions] mapping to include qa on py3.12 - Migrate setup.py → pyproject.toml (setuptools>=61); bump version to 0.9.7 - README: fix wrong import in usage example, document ignore_pk, update supported version matrix, replace Travis/old Codecov badges Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Rename django.yml → tests.yml (test matrix only, no qa) - Add lint.yml: runs tox -e qa (ruff) standalone - Add docs.yml: runs tox -e docs (sphinx-build) standalone - Add [testenv:docs] to tox.ini with sphinx + Django 4.2 - Add docs/conf.py and docs/index.rst with autodoc for public API - Remove qa from [gh-actions] mapping (has its own workflow now) - Update README badge to point to tests.yml Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- pyproject.toml: ignore N818 (DbDiffException rename is breaking API), blanket-ignore D/E501/I on generated migration files - fixture.py: wrap long __init__ signature - test.py: shorten docstring summary line - utils.py: extract spec variable to shorten long line; noqa C901 on diff() and _diff_by_content() (naturally complex algorithms) - test_utils.py: wrap long assertion dicts, shorten docstring/comment Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- fixture.py: assertNoDiff now handles diff() returning None (exact match case exposed by the not-diff bug fix); open fixture in binary mode for ijson to silence DeprecationWarning - utils.py: patch_transaction_test_case detects whether _reset_sequences is a classmethod (Django 5.x) or instance method (Django 4.x) and defines the replacement accordingly — fixes TypeError on Django 5.2+ Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
checked the wrong thing. The right signal is whether _fixture_setup is a classmethod (meaning it calls cls._reset_sequences), not whether _reset_sequences itself is one.
… AUTO_INCREMENT reset Django 5.2 changed _reset_sequences to a @staticmethod (not @classmethod), so isinstance(_raw, classmethod) was False and the else-branch called _raw(cls, db_name) passing two args to a one-arg function. Fix: check isinstance(_raw, (classmethod, staticmethod)) to distinguish both descriptor types from a plain function; for either, call _original(db_name) since the descriptor protocol already resolves the callable correctly. Only plain functions (Django 4.x) need _original(cls, db_name). Also fix MySQL sequence_reset: `or 0` on empty tables sets AUTO_INCREMENT=0 which MySQL 8 ignores (counter stays at previous value). Use `or 1` instead to force a proper reset to 1. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Django 6.0 requires Python 3.12+; testing py311 against it was invalid. Confirmed matrix: django42: py310-py312 django52: py310-py314 django60: py312-py314 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
psycopg2-binary==2.9.9 has no Python 3.13/3.14 wheels; its .so crashes with undefined symbol: _PyInterpreterState_Get on those interpreters. Use unpinned psycopg2-binary for django42/52 (latest wheel supports 3.13), and switch to psycopg[binary] (psycopg3) for django60, which Django 6.0 prefers and which has full 3.13/3.14 support. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Django 6.0 creates content types in a different PK order than earlier versions (migration execution order changed). Content type PKs are meaningless — what matters is app_label+model content — so ignore_pk=True matches records by field values and makes the test stable across versions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
All tracked work is now reflected in the code, CI, and CHANGELOG. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRelease 0.9.7: packaging moved to pyproject.toml; CI and tox matrices updated (drop Python 3.8/3.9; add 3.13/3.14; add Django 5.2/6.0); Fixture.diff and TransactionTestCase patching logic fixed/refactored; Sphinx docs added; assorted tests, docstrings, imports, and DB backend normalization updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Test Runner
participant Fixture as Fixture.assertNoDiff()
participant Diff as Fixture.diff()
participant FS as Tempfile (dump)
participant Parser as ijson Parser
participant DB as Database
participant Comparator as _diff_by_content
Runner->>Fixture: call assertNoDiff()
Fixture->>Diff: call diff()
Diff->>FS: write DB dump to tempfile
Diff->>Parser: stream-parse fixture (binary mode)
Parser->>Comparator: yield fixture items
Comparator->>DB: query DB state/models
Comparator-->>Diff: return (different, missing, unexpected) or None
Diff-->>Fixture: return result (None or tuple)
alt result is None
Fixture-->>Runner: return (no assertion)
else diffs found
Fixture-->>Runner: raise DiffFound with details
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dbdiff/fixture.py (1)
126-130:⚠️ Potential issue | 🟡 MinorMinor: temp dump file is leaked when a diff is found.
dump_pathis created viatempfile.mkstemp(...)at the top of the method and only unlinked on the no-diff branch (Line 127). Whendiff()returns the(unexpected, missing, different)tuple at Line 130, the temp file stays on disk for the lifetime of the process. Over a test run with many failing fixtures this can accumulate.If the file is intentionally kept for debugging when a diff is found, consider documenting that in the docstring; otherwise unlink it before returning the tuple (or use
try/finallyaround the comparison).♻️ One way to clean up unconditionally
- if not unexpected and not missing and not different: - os.unlink(dump_path) - return None - - return unexpected, missing, different + os.unlink(dump_path) + if not unexpected and not missing and not different: + return None + return unexpected, missing, different🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dbdiff/fixture.py` around lines 126 - 130, The temp file created via tempfile.mkstemp (dump_path) is only unlinked in the "no-diff" branch causing leaks when diff() returns (unexpected, missing, different); modify the diff routine (refer to dump_path, unexpected, missing, different) to always remove the temporary file before returning — either wrap the comparison logic in a try/finally that unlinks dump_path in the finally block or explicitly call os.unlink(dump_path) in both the no-diff and diff-return branches (or document keeping the file in the function docstring if you intend to preserve it for debugging).
🧹 Nitpick comments (7)
CHANGELOG (1)
1-1: Consider breaking the 0.9.7 entry into multiple lines for readability.The new entry packs many distinct changes (bug fix, version support, packaging, CI, release workflow) into one long line, while previous entries are concise single-topic summaries. A bulleted/multi-line format would improve scanability.
📝 Suggested format
-0.9.7 Fix always-False diff condition (temp file leak + assertNoDiff TypeError on exact match); add Python 3.10-3.14 and Django 4.2/5.2/6.0 support; drop Python 3.8/3.9 and Django EOL versions; migrate to pyproject.toml; modernize CI (checkout@v4, setup-python@v5, codecov-action); add automated PyPI release workflow +0.9.7 + - Fix always-False diff condition causing temp file leak and assertNoDiff + TypeError on exact match + - Add Python 3.10-3.14 and Django 4.2/5.2/6.0 support + - Drop Python 3.8/3.9 and Django EOL versions + - Migrate to pyproject.toml + - Modernize CI (checkout@v4, setup-python@v5, codecov-action) + - Add automated PyPI release workflow🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG` at line 1, Split the single-line CHANGELOG entry for version "0.9.7" into multiple lines (one item per line or a short bulleted list) so each distinct change is its own readable entry: separate the bugfix about the always-False diff condition (temp file leak + assertNoDiff TypeError), the added Python 3.10–3.14 and Django 4.2/5.2/6.0 support, the dropped Python 3.8/3.9 and EOL Django versions, the migration to pyproject.toml, CI modernizations (checkout@v4, setup-python@v5, codecov-action), and the automated PyPI release workflow into distinct lines that match the concise style of prior entries under "0.9.7".docs/index.rst (1)
1-16: Optional: consider documenting additional public modules.
dbdiff.serializers.jsonanddbdiff.appsare part of the public surface (referenced fromAppConfig.ready()and used bydumpdata) but aren't included in the API reference. Adding them would make the Sphinx output match the package's actual documented surface. Not blocking.Also, since
..include:: ../README.rstinjects whatever heading hierarchy README defines, double-check the rendered TOC depth is what you want — otherwise wrap the API section with an explicit higher-level heading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/index.rst` around lines 1 - 16, The API docs omit two public modules; add automodule directives for dbdiff.serializers.json and dbdiff.apps (e.g., add ".. automodule:: dbdiff.serializers.json\n :members:" and ".. automodule:: dbdiff.apps\n :members:") alongside the existing automodule entries so Sphinx includes those public symbols, and if the README include changes heading depth wrap the API Reference block with an explicit heading level (e.g., a top-level "API Reference" underline) to ensure the rendered TOC depth is correct.dbdiff/tests/test_fixture.py (1)
27-32: Good regression coverage for thenot diffshadowing fix.Test correctly exercises the exact-match path that previously leaked temp files and broke
assertNoDiff(). One small thought: it implicitly relies ondbdiff/fixtures/dbdiff_test_group.jsoncontaining exactly{id: 1, name: 'initial_name'}— if that fixture is ever edited, this test will silently start exercising a non-exact-match path instead. A brief inline comment naming the expected fixture content (or asserting it) would make the regression intent more durable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dbdiff/tests/test_fixture.py` around lines 27 - 32, Update test_diff_exact_match_returns_none to make the expected fixture content explicit: either add a short inline comment stating that the test relies on the fixture file containing {"id": 1, "name": "initial_name"}, or add an assertion that verifies the fixture's loaded data equals that dict before calling self.fixture.diff(); reference the test method name (test_diff_exact_match_returns_none) and the call to self.fixture.diff() so future edits to the fixture won't silently change the test intent.dbdiff/utils.py (2)
93-97: Nit: redundantlist(...)wrap on Line 97.
expected_listis already materialized as alist(...)at Line 93, sofor exp_pk, exp_fields in list(expected_list):is wrapping a list in another list. Iterateexpected_listdirectly.♻️ Suggested cleanup
- for exp_pk, exp_fields in list(expected_list): + for exp_pk, exp_fields in expected_list:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dbdiff/utils.py` around lines 93 - 97, The for-loop redundantly wraps expected_list in list(...) even though expected_list was already constructed as a list; update the loop that iterates over expected_list (the line using "for exp_pk, exp_fields in list(expected_list):") to iterate directly over expected_list ("for exp_pk, exp_fields in expected_list:") to remove the unnecessary list() allocation and clarify intent.
72-73: Nit: redundantsetdefaulton Line 73.Line 72 already runs
different.setdefault(model, {}), so the chained.setdefault(model, {}).setdefault(pk, {})on Line 73 re-creates the per-model dict unnecessarily. You can either drop Line 72 (the chained call covers both) or simplify Line 73 to only set the inner pk key.♻️ Suggested cleanup
- different.setdefault(model, {}) - different.setdefault(model, {}).setdefault(pk, {}) + different.setdefault(model, {}).setdefault(pk, {})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dbdiff/utils.py` around lines 72 - 73, The two setdefault calls on the same dict are redundant; remove the duplicate by keeping a single initialization and then set the inner pk key. For example, either delete the first different.setdefault(model, {}) and keep the chained different.setdefault(model, {}).setdefault(pk, {}), or change the second line to use the already-initialized mapping (different[model].setdefault(pk, {})) so you only initialize the per-model dict once; update the code around the different variable where model and pk are used.pyproject.toml (1)
45-46: Restrict package discovery to avoid shipping the test suite in the wheel.
[tool.setuptools.packages.find]with onlywhere = ["."]will pick up every importable package under the repo root, which meansdbdiff.testsanddbdiff.tests.project.*(settings, migrations, fixtures) get included in the built distribution. That bloats the wheel and lets downstream usersimport dbdiff.tests.*, which they shouldn't rely on.♻️ Proposed fix
[tool.setuptools.packages.find] where = ["."] +include = ["dbdiff*"] +exclude = ["dbdiff.tests*"]Verify the resulting layout once applied (e.g.
python -m buildand inspect the.whl's top-level namespaces).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 45 - 46, The package discovery config [tool.setuptools.packages.find] currently uses where = ["."] which pulls in test packages; update that section to restrict discovery by adding an exclude pattern (e.g. exclude = ["dbdiff.tests*", "tests*", "tests.*"]) or explicitly list packages to include so dbdiff.tests and related test namespaces are omitted; modify the same [tool.setuptools.packages.find] block (where = ["."]) to add the exclude key or replace discovery with an explicit package list, then rebuild (python -m build) and inspect the produced .whl to verify test packages are no longer present..github/workflows/ci.yml (1)
73-76: Optional: bumpcodecov/codecov-actionto v6.
codecov/codecov-action@v6is the current major version and brings improved tokenless OIDC behavior on public repos, better retry logic, and other enhancements. Not blocking —@v4still works — but worth a one‑line bump while you're modernizing the rest of the actions.♻️ Suggested bump
- name: Upload coverage - uses: codecov/codecov-action@v4 + uses: codecov/codecov-action@v6 with: fail_ci_if_error: false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 73 - 76, Update the GitHub Actions step that uses the Codecov action: locate the step with name "Upload coverage" that currently references uses: codecov/codecov-action@v4 and change it to codecov/codecov-action@v6; ensure the existing inputs (e.g., fail_ci_if_error: false) remain compatible and adjust any input names if CI lints complain after bumping the action.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dbdiff/serializers/base.py`:
- Line 52: Fix the typo in the docstring in serializers.base: change "Strip
trailing zeros for constitency." to "Strip trailing zeros for consistency." —
update the docstring string literal in the serializers/base.py location where
that sentence appears so the module/function/class docstring reads
"consistency".
In `@dbdiff/utils.py`:
- Around line 162-195: Update the patch_transaction_test_case code and comments
to reflect actual Django behavior and remove small redundancies: clarify in the
comment above new_reset_sequences/_needs_explicit_cls that Django 4.x uses a
plain function/staticmethod variations and that passing cls to the original
_reset_sequences is safe across supported Django versions (mention
TransactionTestCase._reset_sequences and new_reset_sequences), remove the
duplicate .setdefault() chaining (the earlier setdefault already populates the
dict) and eliminate the unnecessary list() wrapping of expected_list (it's
already a list), and either add a simple idempotency guard in
patch_transaction_test_case to avoid reapplying the patch on repeated calls or
add a concise comment documenting the recursion/multiple-patch risk and why
current use (apps.ready()) is safe.
In `@tox.ini`:
- Around line 66-75: The docs tox env (testenv:docs) pins Django to
"Django>=4.2,<5.0" which contradicts the repo's supported matrix (4.2 / 5.2 /
6.0) and can hide 5.2+ compatibility or deprecation issues when running
"sphinx-build -W" from docs/conf.py
(DJANGO_SETTINGS_MODULE=dbdiff.tests.project.settings_sqlite); update the
testenv:docs deps to match the primary supported Django version used across the
matrix (e.g., use Django>=5.2,<6.0 or the same range as your envlist/pyproject
classifiers) so docs builds exercise the same Django surface as CI.
- Around line 37-39: Update dbdiff/tests/project/settings_postgresql.py to
replace the legacy ENGINE value 'django.db.backends.postgresql_psycopg2' with
the modern 'django.db.backends.postgresql' so the ENGINE setting in that file
uses django.db.backends.postgresql (compatible with Django 6.0 and
psycopg[binary]); locate the ENGINE assignment in settings_postgresql.py and
change its string value accordingly.
---
Outside diff comments:
In `@dbdiff/fixture.py`:
- Around line 126-130: The temp file created via tempfile.mkstemp (dump_path) is
only unlinked in the "no-diff" branch causing leaks when diff() returns
(unexpected, missing, different); modify the diff routine (refer to dump_path,
unexpected, missing, different) to always remove the temporary file before
returning — either wrap the comparison logic in a try/finally that unlinks
dump_path in the finally block or explicitly call os.unlink(dump_path) in both
the no-diff and diff-return branches (or document keeping the file in the
function docstring if you intend to preserve it for debugging).
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 73-76: Update the GitHub Actions step that uses the Codecov
action: locate the step with name "Upload coverage" that currently references
uses: codecov/codecov-action@v4 and change it to codecov/codecov-action@v6;
ensure the existing inputs (e.g., fail_ci_if_error: false) remain compatible and
adjust any input names if CI lints complain after bumping the action.
In `@CHANGELOG`:
- Line 1: Split the single-line CHANGELOG entry for version "0.9.7" into
multiple lines (one item per line or a short bulleted list) so each distinct
change is its own readable entry: separate the bugfix about the always-False
diff condition (temp file leak + assertNoDiff TypeError), the added Python
3.10–3.14 and Django 4.2/5.2/6.0 support, the dropped Python 3.8/3.9 and EOL
Django versions, the migration to pyproject.toml, CI modernizations
(checkout@v4, setup-python@v5, codecov-action), and the automated PyPI release
workflow into distinct lines that match the concise style of prior entries under
"0.9.7".
In `@dbdiff/tests/test_fixture.py`:
- Around line 27-32: Update test_diff_exact_match_returns_none to make the
expected fixture content explicit: either add a short inline comment stating
that the test relies on the fixture file containing {"id": 1, "name":
"initial_name"}, or add an assertion that verifies the fixture's loaded data
equals that dict before calling self.fixture.diff(); reference the test method
name (test_diff_exact_match_returns_none) and the call to self.fixture.diff() so
future edits to the fixture won't silently change the test intent.
In `@dbdiff/utils.py`:
- Around line 93-97: The for-loop redundantly wraps expected_list in list(...)
even though expected_list was already constructed as a list; update the loop
that iterates over expected_list (the line using "for exp_pk, exp_fields in
list(expected_list):") to iterate directly over expected_list ("for exp_pk,
exp_fields in expected_list:") to remove the unnecessary list() allocation and
clarify intent.
- Around line 72-73: The two setdefault calls on the same dict are redundant;
remove the duplicate by keeping a single initialization and then set the inner
pk key. For example, either delete the first different.setdefault(model, {}) and
keep the chained different.setdefault(model, {}).setdefault(pk, {}), or change
the second line to use the already-initialized mapping
(different[model].setdefault(pk, {})) so you only initialize the per-model dict
once; update the code around the different variable where model and pk are used.
In `@docs/index.rst`:
- Around line 1-16: The API docs omit two public modules; add automodule
directives for dbdiff.serializers.json and dbdiff.apps (e.g., add "..
automodule:: dbdiff.serializers.json\n :members:" and ".. automodule::
dbdiff.apps\n :members:") alongside the existing automodule entries so Sphinx
includes those public symbols, and if the README include changes heading depth
wrap the API Reference block with an explicit heading level (e.g., a top-level
"API Reference" underline) to ensure the rendered TOC depth is correct.
In `@pyproject.toml`:
- Around line 45-46: The package discovery config
[tool.setuptools.packages.find] currently uses where = ["."] which pulls in test
packages; update that section to restrict discovery by adding an exclude pattern
(e.g. exclude = ["dbdiff.tests*", "tests*", "tests.*"]) or explicitly list
packages to include so dbdiff.tests and related test namespaces are omitted;
modify the same [tool.setuptools.packages.find] block (where = ["."]) to add the
exclude key or replace discovery with an explicit package list, then rebuild
(python -m build) and inspect the produced .whl to verify test packages are no
longer present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 96e534a1-1803-4beb-9405-244a34b86718
📒 Files selected for processing (25)
.github/workflows/ci.ymlCHANGELOGREADME.rstdbdiff/apps.pydbdiff/exceptions.pydbdiff/fixture.pydbdiff/sequence.pydbdiff/serializers/base.pydbdiff/serializers/json.pydbdiff/test.pydbdiff/tests/decimal_test/migrations/0001_initial.pydbdiff/tests/decimal_test/migrations/0002_auto_20160102_0914.pydbdiff/tests/project/settings.pydbdiff/tests/project/settings_postgresql.pydbdiff/tests/test_decimal.pydbdiff/tests/test_fixture.pydbdiff/tests/test_mixin.pydbdiff/tests/test_plugin.pydbdiff/tests/test_utils.pydbdiff/utils.pydocs/conf.pydocs/index.rstpyproject.tomlsetup.pytox.ini
💤 Files with no reviewable changes (2)
- dbdiff/serializers/json.py
- setup.py
- serializers/base.py: fix "constitency" typo → "consistency" - utils.py: remove redundant setdefault() on line 72; remove unnecessary list() wrapping in _diff_by_content(); add idempotency note to patch_transaction_test_case docstring - fixture.py: wrap diff() body in try/finally so dump_path is always unlinked, even when a diff is found - tox.ini: bump docs env Django from >=4.2,<5.0 to >=5.2,<6.0 - settings_postgresql.py: update ENGINE to django.db.backends.postgresql - ci.yml: bump codecov/codecov-action@v4 → @v6 - CHANGELOG: split 0.9.7 entry into one item per line - test_fixture.py: add inline comment noting expected fixture content - docs/index.rst: add automodule directives for serializers.json and apps - pyproject.toml: exclude test packages from wheel discovery Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dbdiff/utils.py (1)
68-80:⚠️ Potential issue | 🟠 MajorBuild the field diff lazily and compare key sets too.
This still blows up when
result_fieldsis missing a key (Line 75raisesKeyError), and it can also emit a truthy-but-emptydifferent[model][pk] = {}when the only mismatch is an extra key on the result side. Compute a localfield_diffover the union of keys first, then persist it only when non-empty.Proposed fix
+ missing_value = object() - different.setdefault(model, {}).setdefault(pk, {}) - - for expected_field, expected_value in expected_fields.items(): - result_value = result_fields[expected_field] - - if expected_value == result_value: - continue - - different[model][pk][expected_field] = ( - expected_value, - result_value - ) + field_diff = {} + for field_name in expected_fields.keys() | result_fields.keys(): + expected_value = expected_fields.get(field_name, missing_value) + result_value = result_fields.get(field_name, missing_value) + if expected_value == result_value: + continue + field_diff[field_name] = (expected_value, result_value) + + if field_diff: + different.setdefault(model, {})[pk] = field_diff🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dbdiff/utils.py` around lines 68 - 80, The code currently indexes result_fields[expected_field] which can KeyError and also writes an empty different[model][pk] when only extra keys exist; fix by building a local field_diff dict for the union of keys: compute keys = set(expected_fields.keys()) | set(result_fields.keys()), iterate those keys, use expected_fields.get(k, MISSING) and result_fields.get(k, MISSING) (or check membership) to compare safely, add mismatches to field_diff, and only assign different.setdefault(model, {}).setdefault(pk, {}) = field_diff (or set it) if field_diff is non-empty; update references to result_fields, expected_fields, different, result, model, pk accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dbdiff/utils.py`:
- Around line 181-187: The code builds SQL by interpolating table names into
statements (variables: tables, statements, cursor, connection) which can break
on names with quotes and triggers S608; change it to execute a parameterized
query per table instead of formatting strings—iterate over tables and call
cursor.execute("UPDATE SQLITE_SEQUENCE SET SEQ=0 WHERE NAME=%s", (t,)) (Django
will convert %s to ? for SQLite) so you avoid direct string interpolation and
satisfy the linter.
---
Outside diff comments:
In `@dbdiff/utils.py`:
- Around line 68-80: The code currently indexes result_fields[expected_field]
which can KeyError and also writes an empty different[model][pk] when only extra
keys exist; fix by building a local field_diff dict for the union of keys:
compute keys = set(expected_fields.keys()) | set(result_fields.keys()), iterate
those keys, use expected_fields.get(k, MISSING) and result_fields.get(k,
MISSING) (or check membership) to compare safely, add mismatches to field_diff,
and only assign different.setdefault(model, {}).setdefault(pk, {}) = field_diff
(or set it) if field_diff is non-empty; update references to result_fields,
expected_fields, different, result, model, pk accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d72c52d9-18c1-4983-a22c-a099937b83be
📒 Files selected for processing (10)
.github/workflows/ci.ymlCHANGELOGdbdiff/fixture.pydbdiff/serializers/base.pydbdiff/tests/project/settings_postgresql.pydbdiff/tests/test_fixture.pydbdiff/utils.pydocs/index.rstpyproject.tomltox.ini
✅ Files skipped from review due to trivial changes (3)
- dbdiff/serializers/base.py
- pyproject.toml
- CHANGELOG
🚧 Files skipped from review as they are similar to previous changes (6)
- dbdiff/tests/project/settings_postgresql.py
- docs/index.rst
- dbdiff/tests/test_fixture.py
- dbdiff/fixture.py
- .github/workflows/ci.yml
- tox.ini
| statements = [ | ||
| "UPDATE SQLITE_SEQUENCE SET SEQ=0 WHERE NAME='%s';" % t | ||
| for t in tables | ||
| ] | ||
|
|
||
| cursor = connection.cursor() | ||
|
|
||
| for statement in statements: | ||
| cursor.execute(statement) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Django 4.2, 5.2, and 6.0, does cursor.execute("UPDATE sqlite_sequence SET seq=0 WHERE name=%s", [table]) support bound parameters correctly on SQLite via Django's database cursor wrapper?
💡 Result:
Yes, in Django 4.2, 5.2, and 6.0, cursor.execute("UPDATE sqlite_sequence SET seq=0 WHERE name=%s", [table]) supports bound parameters correctly on SQLite via Django's database cursor wrapper. Django's SQLite backend uses SQLiteCursorWrapper, which transparently converts the "%s" format-style placeholder to SQLite's "?" qmark style using regex replacement: FORMAT_QMARK_REGEX.sub("?", query).replace("%%", "%"). This handles the single positional parameter correctly, passing it to the underlying sqlite3.Cursor.execute. This conversion has been stable since early Django versions and remains unchanged in current master (post-6.0). Official docs confirm using "%s" placeholders with cursor.execute(sql, [params]) for SQLite raw queries. Django's sequence reset support (ticket #31479, merged 2020 in Django 3.1+) generates exactly this SQL with bound parameters for ops.sequence_reset_sql and executes it successfully in tests.
Citations:
- 1: https://github.com/django/django/blob/master/django/db/backends/sqlite3/base.py
- 2: https://docs.djangoproject.com/en/4.2/topics/db/sql/
- 3: https://code.djangoproject.com/ticket/31479
- 4: Fixed #31479 -- Added support to reset sequences on SQLite. django/django#12748
- 5: https://code.djangoproject.com/ticket/31730
Parameterize the sqlite_sequence update instead of formatting table names into SQL.
Although db_table values are project-controlled, string interpolation still breaks on valid names containing ' and triggers Ruff's S608 warning. Django's SQLite backend correctly supports parameter binding with %s placeholders, converting them to ? markers automatically. Use parameterized queries to match Django's own pattern for sequence reset operations.
Proposed fix
- statements = [
- "UPDATE SQLITE_SEQUENCE SET SEQ=0 WHERE NAME='%s';" % t
- for t in tables
- ]
- cursor = connection.cursor()
- for statement in statements:
- cursor.execute(statement)
+ with connection.cursor() as cursor:
+ for table in tables:
+ cursor.execute(
+ "UPDATE SQLITE_SEQUENCE SET SEQ=0 WHERE NAME = %s",
+ [table],
+ )🧰 Tools
🪛 Ruff (0.15.12)
[error] 182-182: Possible SQL injection vector through string-based query construction
(S608)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dbdiff/utils.py` around lines 181 - 187, The code builds SQL by interpolating
table names into statements (variables: tables, statements, cursor, connection)
which can break on names with quotes and triggers S608; change it to execute a
parameterized query per table instead of formatting strings—iterate over tables
and call cursor.execute("UPDATE SQLITE_SEQUENCE SET SEQ=0 WHERE NAME=%s", (t,))
(Django will convert %s to ? for SQLite) so you avoid direct string
interpolation and satisfy the linter.
|
Hey @PurpleToti — thanks for the thorough work (the not diff shadowing fix especially is a real find 🙏) |
Updated comment formatting for clarity in test case. (line was too long)
|
|
||
| with open(self.path, 'r') as e, open(dump_path, 'r') as r: | ||
| expected, result = json.load(e), json.load(r) | ||
| try: |
There was a problem hiding this comment.
What's the new try/finally for here exactly?
There was a problem hiding this comment.
To ensure tmp file (dump_path) is always deleted
|
Generally I agréé with your comment @pfouque but I am not sure we can really split, the commits are splitted and reviewable indepently, but we get the final ci outcome here directly If you have a project using dbdiff, could you try out this branch and report any problem here? Otherwise i suppose we're Moving forward with this, we can still add changes if we need to later on |
|
TLDR: You're the boss here! ;) Unfortunately I don't have project using this directly, I was using it working on django-cities-light (and discovered it was barely used in fact: yourlabs/django-cities-light#288). |
4 participants
Summary
This PR brings
django-dbdiffup to date for current Python and Django versions, fixes a criticalsilent bug, and modernizes CI and packaging.
Bug Fixes
Critical —
fixture.py:129: always-Falsenot diffconditionThe variable name
diffinFixture.diff()shadowed the importeddifffunction fromutils.not diffevaluated against a function object, which is always truthy, so the guard was neverentered. Consequence: (1) the temp dump file was never deleted on a clean match (file leak),
(2)
assertNoDiffreceivedNoneback and raisedTypeErrorinstead of passing.Fixed to
not different.Fix
assertNoDiffNone handling — handlediff()returningNoneon exact match; openfixture in binary mode to silence ijson
DeprecationWarning.Fix
patch_transaction_test_casefor Django 5.2 + MySQLDjango 5.2 changed
_reset_sequencesfrom a@classmethodto a@staticmethod; the olddetection logic broke and called the replacement with the wrong number of arguments. Also fixes
MySQL AUTO_INCREMENT reset:
or 0left the counter at its previous value in MySQL 8; changedto
or 1.Fix psycopg version for Python 3.13/3.14 and Django 6.0
psycopg2-binary==2.9.9has no wheels for Python 3.13/3.14 (crashes withundefined symbol: _PyInterpreterState_Get). Now uses unpinnedpsycopg2-binaryforDjango 4.2/5.2 and
psycopg[binary](psycopg3) for Django 6.0.Fix tox matrix: drop
py311fromdjango60envsDjango 6.0 requires Python 3.12+;
py311-django60was an invalid combination.New / Extended Features
ignore_pkinContentTypeTestCase— Django 6.0 creates content types with a differentPK ordering (migration execution order changed). Using
ignore_pk=Truematches records byfield values, making the built-in test stable across all supported Django versions.
Builds on the
ignore_pkoption added toFixtureanddiff()by@marianoramirez353 (commit
1bef061).
Compatibility
@pfouque) to Python 3.13 and 3.14 and Django 5.2 LTS / 6.0
CI / Packaging
actions/checkout@v1→v4,actions/setup-python@v4→v5codecovCLI step withcodecov/codecov-action@v4django.ymlworkflow withlint,docs, andtestsjobssetup.py→pyproject.toml(PEP 517/518,setuptools>=61)flake8→ruff; fix all lint errorsdocs/conf.py,docs/index.rstwith autodoc)MAINTENANCE.md(planning artifact, no longer needed)Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Tests
Chores