Name conflict if tracked model has field named "instance"#646
Name conflict if tracked model has field named "instance"#646shemigon wants to merge 5 commits intojazzband:masterfrom
Conversation
# Conflicts: # AUTHORS.rst # tests/test_fields/test_field_tracker.py
|
Hi, are there any plans to get this change into the upstream? We are being hit with it and wondering about prospective. |
|
@livenson I'm not a maintainer of the repository. I created this PR a while ago and it's been unnoticed ever since. |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #633 where models with a field named "instance" would fail with a TypeError when using FieldTracker or ModelTracker. The fix changes the patched __init__ wrapper to use *args, **kwargs instead of explicitly naming the first parameter instance, avoiding conflicts with model field names.
Key Changes:
- Modified
patch_initinmodel_utils/tracker.pyto use positional arguments instead of named parameters - Added test coverage for models with special field names that could conflict with internal parameter names
- Improved timezone handling in timestamp and timeframe model tests by replacing
datetime.today()withdjango.utils.timezone.now()
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| model_utils/tracker.py | Fixed the patch_init method to avoid naming conflicts by using *args and accessing the instance as args[0] |
| tests/models.py | Added TrackedModelWithSpecialNamedField test model with an "instance" field to verify the fix |
| tests/test_models/test_special_name_field_model.py | New test file verifying that models with "instance" fields can be created, updated, and deleted |
| tests/test_models/test_timestamped_model.py | Replaced datetime.today() with timezone-aware now() for proper timezone handling |
| tests/test_models/test_timeframed_model.py | Replaced datetime.now() with timezone-aware now() for consistency |
| AUTHORS.rst | Added contributor Boris Shemigon |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_model_with_instance_field(self) -> None: | ||
| t = TrackedModelWithSpecialNamedField.objects.create( | ||
| instance="45.55", | ||
| name="test", | ||
| ) | ||
| self.assertEqual(t.instance, "45.55") | ||
|
|
||
| t.instance = "56.78" | ||
| t.save() | ||
|
|
||
| t.delete() |
There was a problem hiding this comment.
The test should verify that the tracker functionality works correctly for models with an "instance" field. Consider adding assertions to check tracker methods like tracker.has_changed() and tracker.previous() to ensure the fix doesn't break tracker behavior. For example, after updating t.instance = "56.78", verify that t.tracker.has_changed('instance') returns True and t.tracker.previous('instance') returns the old value.
Problem
If a tracked model has a field called
instance, creating a instance of the model throwsSee issue #633
Solution
Removed
instanceargument from the new model init method and usedargs[0]instead.Commandments
CHANGES.rstfile to describe the changes, and quote according issue withGH-<issue_number>.