Skip to content

Commit 29eabfa

Browse files
committed
fix(fixer): exclude Python class sentinels from save update_fields inference
The save_without_update_fields fixer was including Python class attributes (e.g. no_changes = False) in the inferred update_fields list, causing FieldDoesNotExist errors at runtime on Django 1.11+ since those attributes are not database columns. Add _collect_non_field_class_attrs() which scans every class definition in the module and returns attribute names that are NOT assigned to models.XYZField(...) instances. These sentinels are excluded from the update_fields suggestion. Root cause: celery/django-celery#643 CI failure (Python 3.7/Django 1.11 matrix) — our PR included no_changes in update_fields. Fixed in the django-celery fork (commit 01d5ea9) and now pact itself won't generate this invalid repair in future. Two regression tests: sentinel excluded, all-sentinel case → skipped. 305 tests passing.
1 parent 6b36d41 commit 29eabfa

2 files changed

Lines changed: 98 additions & 0 deletions

File tree

fixer.py

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -516,6 +516,49 @@ def _obj_name(node: ast.expr) -> str | None:
516516
return None
517517

518518

519+
def _collect_non_field_class_attrs(tree: ast.Module) -> frozenset[str]:
520+
"""
521+
Collect attribute names that are assigned simple Python values (not Django
522+
Field instances) at class-body level in any class in the module.
523+
524+
These are Python sentinels / class constants (e.g. `no_changes = False`,
525+
`objects = Manager()`, `Meta = ...`) — not real database columns. When
526+
pact infers `update_fields` from preceding attribute assignments it must
527+
exclude these, or Django raises FieldDoesNotExist at runtime.
528+
529+
Pattern detected:
530+
class SomeModel(models.Model):
531+
real_field = models.CharField(...) ← NOT collected (is a Field)
532+
no_changes = False ← collected (simple value)
533+
_state = ... ← collected (non-Field)
534+
"""
535+
non_fields: set[str] = set()
536+
for node in ast.walk(tree):
537+
if not isinstance(node, ast.ClassDef):
538+
continue
539+
for stmt in node.body:
540+
if not isinstance(stmt, ast.Assign):
541+
continue
542+
if len(stmt.targets) != 1:
543+
continue
544+
target = stmt.targets[0]
545+
if not isinstance(target, ast.Name):
546+
continue
547+
val = stmt.value
548+
# If the value is a call whose function name ends in "Field", it's a
549+
# real Django field definition — skip it.
550+
if isinstance(val, ast.Call):
551+
func = val.func
552+
is_field = (
553+
isinstance(func, ast.Attribute) and func.attr.endswith("Field")
554+
) or (isinstance(func, ast.Name) and func.id.endswith("Field"))
555+
if is_field:
556+
continue
557+
# Not a Field call — this is a Python class attribute / sentinel.
558+
non_fields.add(target.id)
559+
return frozenset(non_fields)
560+
561+
519562
def _collect_preceding_assignments(
520563
func_body: list[ast.stmt],
521564
save_lineno: int,
@@ -599,6 +642,10 @@ def _fix_save_without_update_fields(
599642
skipped.extend(violations)
600643
return list(lines), applied, skipped
601644

645+
# Attrs that are Python class constants / sentinels (not Django Fields).
646+
# These must never appear in update_fields — Django would raise FieldDoesNotExist.
647+
sentinel_attrs = _collect_non_field_class_attrs(tree)
648+
602649
# Build map: save_lineno → enclosing function body
603650
save_line_to_func: dict[int, list[ast.stmt]] = {}
604651
for node in ast.walk(tree):
@@ -647,6 +694,12 @@ def _fix_save_without_update_fields(
647694
skipped.append(ev)
648695
continue
649696

697+
# Filter out Python sentinels — keep only real DB columns.
698+
attrs = [a for a in attrs if a not in sentinel_attrs]
699+
if not attrs:
700+
skipped.append(ev)
701+
continue
702+
650703
# Build `update_fields=[...]` argument string
651704
fields_list = "[" + ", ".join(f'"{a}"' for a in reversed(attrs)) + "]"
652705

test_fixer.py

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -707,6 +707,51 @@ def update(obj):
707707
assert not result.changed or "update_fields" in result.patched
708708

709709

710+
def test_save_excludes_python_sentinel_attrs(tmp_path):
711+
"""Regression: class-level Python sentinels (not models.Field) excluded from update_fields.
712+
713+
Root cause of celery/django-celery#643 CI failure: pact inferred
714+
`update_fields=['no_changes', 'enabled']` but `no_changes = False` is a
715+
Python class attribute, not a DB column — Django raises FieldDoesNotExist.
716+
"""
717+
src = textwrap.dedent("""\
718+
class PeriodicTask(models.Model):
719+
enabled = models.BooleanField(default=True)
720+
no_changes = False
721+
722+
def _disable(model):
723+
model.no_changes = True
724+
model.enabled = False
725+
model.save()
726+
""")
727+
f = tmp_path / "s.py"
728+
f.write_text(src)
729+
ev = _ev("save_without_update_fields", 8, "model.save", str(f))
730+
result = fix_file(str(f), [ev])
731+
assert result.changed
732+
assert 'update_fields=["enabled"]' in result.patched
733+
assert "no_changes" not in result.patched.split("update_fields=")[1]
734+
ast.parse(result.patched)
735+
736+
737+
def test_save_all_sentinel_attrs_skipped(tmp_path):
738+
"""If all inferred attrs are sentinels, skip rather than emit empty update_fields."""
739+
src = textwrap.dedent("""\
740+
class Task(models.Model):
741+
_sentinel = None
742+
743+
def update(obj):
744+
obj._sentinel = True
745+
obj.save()
746+
""")
747+
f = tmp_path / "s.py"
748+
f.write_text(src)
749+
ev = _ev("save_without_update_fields", 5, "obj.save", str(f))
750+
result = fix_file(str(f), [ev])
751+
assert not result.changed
752+
assert len(result.skipped) == 1
753+
754+
710755
# ---------------------------------------------------------------------------
711756
# unvalidated_lookup_chain
712757
# ---------------------------------------------------------------------------

0 commit comments

Comments
 (0)