[fix] Prevent FallbackMixin from generating spurious migrations #1231#604
[fix] Prevent FallbackMixin from generating spurious migrations #1231#604shivsubh wants to merge 1 commit intoopenwisp:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFallbackMixin was modified to make the Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment Tip CodeRabbit can use your project's `ruff` configuration to improve the quality of Python code reviews.Add a Ruff configuration file to your project to customize how CodeRabbit runs |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@nemesifier can you please review this PR? It is fixing openwisp/openwisp-controller#1231 |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
The main problem here is that there's no regression test, can you please add one? It should fail without the patch.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge OverviewThis PR fixes a legitimate issue where Changes Reviewed
VerificationThe fix correctly addresses the root cause described in openwisp/openwisp-controller#1231 - fallback values were being serialized into migration files, causing unnecessary migrations when settings changed. Files Reviewed (2 files)
|
shivsubh
force-pushed
the
issues/1231-fallback-fileds-db-migrations
branch
from
dd534d4 to
05ad77a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_project/tests/test_model.py`:
- Around line 184-187: Expand test_fallback_field_deconstruct to cover other
field types that use FallbackMixin: fetch additional fields from
OrganizationRadiusSettings via OrganizationRadiusSettings._meta.get_field for
examples such as a CharField, DecimalField, and PositiveIntegerField (use the
actual field names present on the model), call field.deconstruct() for each, and
assert that "fallback" is not present in the returned kwargs; ensure you
reference the existing test name test_fallback_field_deconstruct and the model
field retrieval pattern so the new assertions mirror the existing is_active
check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f9760f4d-14c7-42b9-84e3-e5c46df67197
📒 Files selected for processing (2)
openwisp_utils/fields.pytests/test_project/tests/test_model.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
🔇 Additional comments (2)
openwisp_utils/fields.py (2)
50-54: LGTM: Makingfallbackoptional is correct for backward compatibility.Existing migrations that omit
fallback(after being regenerated) will still work sincekwargs.pop("fallback", None)gracefully handles the absent key.
56-58: The deconstruct() change will not cause migration divergence.The
fallbackparameter is popped from kwargs in__init__(line 51) before being passed to the parent class, so the parent field never stores it. Whendeconstruct()returns the parent's deconstruction, it correctly excludesfallbackbecause the parent never possessed it. When existing migrations are deserialized, the same__init__logic popsfallbackagain, and both the current field and deserialized migration field produce identicaldeconstruct()outputs. Django's autodetector will detect no changes.No migration updates or breaking changes to document.
Black and Flake8 FailuresHello @shivsubh, There are multiple Explanation: The Remediation: To fix these issues, please run the following command in your local environment: openwisp-qa-formatThis command will automatically format your code according to |
shivsubh
force-pushed
the
issues/1231-fallback-fileds-db-migrations
branch
from
05ad77a to
461ea73
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/test_project/tests/test_model.py (1)
184-196: 🧹 Nitpick | 🔵 TrivialExpand this to the remaining
FallbackMixinfield classes.This now covers four subclasses, but it still skips
is_first_name_required,password_reset_url, andextra_config. Since each parent field type can influencedeconstruct()behavior, I'd extend the matrix to all fallback-backed fields here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_project/tests/test_model.py` around lines 184 - 196, The test test_fallback_field_deconstruct only checks four fields; extend it to also check the other FallbackMixin-backed fields by retrieving each field via model._meta.get_field(...) for is_first_name_required, password_reset_url, and extra_config, calling field.deconstruct() and asserting "fallback" is not present in the returned kwargs for each; update the test function (test_fallback_field_deconstruct) to include these additional assertions alongside the existing checks for OrganizationRadiusSettings, Book, and Shelf.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_utils/fields.py`:
- Around line 50-58: The field's deconstruct() omits the instance's fallback
value so clones lose it; update the class to include "fallback" in
Field.non_db_attrs and ensure deconstruct() copies the fallback into the
returned kwargs (when fallback is not None) so that __init__ receives it on
reconstruction; modify the class-level non_db_attrs to include "fallback" and
adjust deconstruct() (alongside __init__ which already pops fallback) so
get_default() and from_db_value() retain the original fallback.
---
Duplicate comments:
In `@tests/test_project/tests/test_model.py`:
- Around line 184-196: The test test_fallback_field_deconstruct only checks four
fields; extend it to also check the other FallbackMixin-backed fields by
retrieving each field via model._meta.get_field(...) for is_first_name_required,
password_reset_url, and extra_config, calling field.deconstruct() and asserting
"fallback" is not present in the returned kwargs for each; update the test
function (test_fallback_field_deconstruct) to include these additional
assertions alongside the existing checks for OrganizationRadiusSettings, Book,
and Shelf.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7908496-bab7-42bd-9a22-057a64c554e4
📒 Files selected for processing (2)
openwisp_utils/fields.pytests/test_project/tests/test_model.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
shivsubh
force-pushed
the
issues/1231-fallback-fileds-db-migrations
branch
from
461ea73 to
79190e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openwisp_utils/fields.py (1)
50-51: 🧹 Nitpick | 🔵 TrivialConsider using a property to extend
non_db_attrsinstead of overriding it.Setting
non_db_attrsas a class attribute replaces the parent class'snon_db_attrsentirely, rather than extending it. While current Django base fields may not have significant entries, this pattern is fragile if parent classes addnon_db_attrsin the future.♻️ Proposed fix to safely extend parent's non_db_attrs
- non_db_attrs = ("fallback",) + `@property` + def non_db_attrs(self): + return super().non_db_attrs + ("fallback",)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_utils/fields.py` around lines 50 - 51, Replace the class attribute non_db_attrs = ("fallback",) with an instance property that merges the parent's non_db_attrs with ("fallback",) to avoid clobbering future parent values; e.g., add a `@property` def non_db_attrs(self): parent = getattr(super(YourFieldClass, self), "non_db_attrs", ()) and return tuple(parent) + ("fallback",) — this references the non_db_attrs symbol and uses super() safely to extend instead of overriding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_project/tests/test_model.py`:
- Around line 184-196: Update test_fallback_field_deconstruct to wrap each field
check in subTest(...) for consistency and add a negative assertion for a field
that does not define a fallback (or defines fallback=None) so that deconstruct()
does NOT include "fallback" in kwargs; specifically, in
test_fallback_field_deconstruct iterate or individually subTest for
OrganizationRadiusSettings._meta.get_field("is_active"),
OrganizationRadiusSettings._meta.get_field("greeting_text"),
Book._meta.get_field("price"), Shelf._meta.get_field("books_count") and assert
"fallback" in kwargs for those, and add another subTest using a model field that
has no fallback (e.g. a title/name field) to assert "fallback" not in kwargs.
Ensure changes are made inside the test_fallback_field_deconstruct function
only.
---
Duplicate comments:
In `@openwisp_utils/fields.py`:
- Around line 50-51: Replace the class attribute non_db_attrs = ("fallback",)
with an instance property that merges the parent's non_db_attrs with
("fallback",) to avoid clobbering future parent values; e.g., add a `@property`
def non_db_attrs(self): parent = getattr(super(YourFieldClass, self),
"non_db_attrs", ()) and return tuple(parent) + ("fallback",) — this references
the non_db_attrs symbol and uses super() safely to extend instead of overriding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6d8d44e8-643b-45a7-997b-f8c62b26bb26
📒 Files selected for processing (2)
openwisp_utils/fields.pytests/test_project/tests/test_model.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.0.0
🔇 Additional comments (1)
openwisp_utils/fields.py (1)
58-62: LGTM!The conditional inclusion of
fallbackindeconstruct()correctly preserves the fallback value for field reconstruction viaField.clone(), whilenon_db_attrsensures Django's migration framework ignores changes to fallback values. This approach addresses the spurious migration issue while maintaining field functionality.
shivsubh
force-pushed
the
issues/1231-fallback-fileds-db-migrations
branch
from
79190e2 to
f1a3845
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openwisp_utils/fields.py (1)
50-50:⚠️ Potential issue | 🟠 MajorExtend Django’s
non_db_attrs; don’t replace it.Line 50 shadows Django’s built-in
Field.non_db_attrstuple instead of extending it. Django documents custom fields assuper().non_db_attrs + ("your_attr",), and the schema editor strips every name in that tuple before deciding whether anAlterFieldis a no-op. With("fallback",)here, fallback fields stop inheriting Django’s default non-schema attrs likechoices,help_text, andvalidators, so edits to those can start generating migrations again. (docs.djangoproject.com)Suggested fix
class FallbackMixin(object): @@ - non_db_attrs = ("fallback",) + `@property` + def non_db_attrs(self): + return super().non_db_attrs + ("fallback",)In Django custom model fields, should `Field.non_db_attrs` be extended with `super().non_db_attrs + ("fallback",)` instead of replaced with `("fallback",)`? Also confirm that migration diffing removes every attribute listed in `non_db_attrs` before deciding whether an `AlterField` is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_utils/fields.py` at line 50, The custom field currently replaces Django's Field.non_db_attrs with ("fallback",) which loses default non-schema attributes; change the field class to extend the base tuple by setting non_db_attrs = super().non_db_attrs + ("fallback",) (ensuring it's a tuple) so built-in names like choices, help_text, and validators are preserved; this uses the field class's non_db_attrs attribute (in openwisp_utils/fields.py) and ensures Django's migration diffing still strips every name in non_db_attrs before deciding if an AlterField is a no-op.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_project/tests/test_model.py`:
- Around line 184-204: Expand testing beyond deconstruct() by adding a
regression test that uses Django's MigrationAutodetector: create two
ProjectState instances (before/after) where the model fields (e.g.,
OrganizationRadiusSettings.is_active, Book.price, Shelf.books_count) differ only
by a non-db attribute like "fallback" (i.e., set Field.non_db_attrs to include
"fallback" or include fallback in the field's deconstruction but not in
DB-affecting attrs), run MigrationAutodetector.detect_changes between the
states, and assert that no AlterField operations are produced; place this
alongside test_fallback_field_deconstruct and reference MigrationAutodetector,
ProjectState, and the affected field names to locate the code to change.
---
Duplicate comments:
In `@openwisp_utils/fields.py`:
- Line 50: The custom field currently replaces Django's Field.non_db_attrs with
("fallback",) which loses default non-schema attributes; change the field class
to extend the base tuple by setting non_db_attrs = super().non_db_attrs +
("fallback",) (ensuring it's a tuple) so built-in names like choices, help_text,
and validators are preserved; this uses the field class's non_db_attrs attribute
(in openwisp_utils/fields.py) and ensures Django's migration diffing still
strips every name in non_db_attrs before deciding if an AlterField is a no-op.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 51028e77-db6c-4c53-ae08-11a702ac3d41
📒 Files selected for processing (2)
openwisp_utils/fields.pytests/test_project/tests/test_model.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
shivsubh
force-pushed
the
issues/1231-fallback-fileds-db-migrations
branch
from
f1a3845 to
dcaacef
Compare
Flake8 Fails on Unused ImportHello @shivsubh, There is one distinct failure observed in the CI logs:
|
shivsubh
force-pushed
the
issues/1231-fallback-fileds-db-migrations
branch
2 times, most recently
from
ad400a9 to
b6504a6
Compare
I have added the regression test for the patch and also i have attached the failing regression test screenshots without the patch: 
|
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Have you verified the fix fails without the patch? See my comment below.
|
|
||
| from django.db.migrations.autodetector import MigrationAutodetector | ||
| from django.db.migrations.loader import MigrationLoader | ||
| from django.db.migrations.questioner import NonInteractiveMigrationQuestioner |
There was a problem hiding this comment.
move imports at the top, please don't send us this AI slop without manual review
There was a problem hiding this comment.
sorry, my bad. I forgot to do a manual review of tests in the end. I will keep this in mind from next time.
shivsubh
force-pushed
the
issues/1231-fallback-fileds-db-migrations
branch
from
b6504a6 to
5411b8a
Compare
Yes, I have verified that it is failing without the patch: |
shivsubh
force-pushed
the
issues/1231-fallback-fileds-db-migrations
branch
from
5411b8a to
4c6d7bb
Compare
nemesifier
changed the title
|
Hi @shivsubh 👋, This is a friendly reminder that this pull request has had no activity for 7 days since changes were requested. We'd love to see this contribution merged! Please take a moment to:
If you're busy or need more time, no worries! Just leave a comment to let us know you're still working on it. Note: within 7 more days, the linked issue will be unassigned to allow other contributors to work on it. Thank you for your contribution! 🙏 |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Ignore the comment from the bot, I just haven't had time to test this yet.
shivsubh
force-pushed
the
issues/1231-fallback-fileds-db-migrations
branch
from
4c6d7bb to
c440597
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
The code looks good. Manual testing is needed to validate and confirm it can be merged. Thanks!🙏
There was a problem hiding this comment.
Two things:
-
deconstruct()is now dead code. It callssuper().deconstruct()and returns the result untouched. The parent classes never seefallback(it'spop()'d beforesuper().__init__()), sosuper().deconstruct()will never include it. You can delete the method entirely, same behavior, less confusion. -
No test for
clone(). Thatclone()override is the sneaky-important part of this fix. Django'sField.clone()round-trips throughdeconstruct(), so without your override, cloned fields silently lose their fallback. This matters for model inheritance, FK resolution, and other Django internals. A test like this would catch it:
def test_fallback_field_clone_preserves_fallback(self):
field = OrganizationRadiusSettings._meta.get_field("is_active")
cloned = field.clone()
self.assertEqual(cloned.fallback, field.fallback)If someone later removes that clone() override thinking "eh, it's simple, probably unnecessary", this test is the thing that screams at them.
The `deconstruct()` method was serializing the fallback kwarg into Django migration files. This caused new migrations to be generated whenever the fallback default value changed in settings, even though no actual database schema change had occurred. The fix removes fallback from deconstruct() so Django no longer tracks it as part of the field migration state. fallback is also made optional in `__init__` (defaulting to None) so existing migrations that omit the kwarg remain valid. Fixes: #1231
shivsubh
force-pushed
the
issues/1231-fallback-fileds-db-migrations
branch
from
c440597 to
3fbfc41
Compare
3 participants
The deconstruct() method was serializing the fallback kwarg into Django migration files. This caused new migrations to be generated whenever the fallback default value changed in settings, even though no actual database schema change had occurred.
The fix removes fallback from deconstruct() so Django no longer tracks it as part of the field migration state. fallback is also made optional in init (defaulting to None) so existing migrations that omit the kwarg remain valid.
Fixes: openwisp/openwisp-controller#1231
Checklist