[fix] Preserved text -> jsonb migration for JSONField fields #798#799
Merged
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:
📝 WalkthroughWalkthroughThis PR reverts JSONField definitions back to TextField in historical Django migrations across Check and Metric models. The changes preserve all field options (blank, default, db_index, verbose_name) while reversing the field type in production squashed migrations and test sample migrations. A repair migration import is also updated to use a simpler Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
pandafy
force-pushed
the
issues/798-jsonb-migration
branch
from
cdc72fb to
cbbd6c2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/openwisp2/sample_check/migrations/0001_initial.py (1)
74-80: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMirror the production rationale inline in this fixture migration.
This fixture now depends on the same non-obvious historical
TextFielddefinition as production, so it needs a short comment here too; otherwise it is easy to “fix” the test migration back out of sync.As per coding guidelines, "Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/openwisp2/sample_check/migrations/0001_initial.py` around lines 74 - 80, The test migration's "params" TextField uses default=dict (a non-obvious choice) but lacks the explanatory rationale present in production; add a concise inline comment next to the "params" field definition (the TextField for "params") that explains why default=dict is used and that this mirrors the historical production field definition so the fixture stays in sync and is not accidentally "fixed" later.openwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.py (1)
67-73: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDocument why
paramsintentionally stays aTextFieldhere.This historical definition is easy to misread as an accidental downgrade, so please add a short inline note explaining that the text-backed field is intentional and should not be “cleaned up” back to
JSONField.As per coding guidelines, "Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.py` around lines 67 - 73, Add a short inline comment above the params = models.TextField(...) migration declaration explaining that this TextField is intentionally preserved in the migration (historical/frozen model state) and must not be converted to JSONField during migration cleanup—mention compatibility/serialization reasons and that changing it would alter the historical migration record; reference the params field and the TextField declaration so reviewers can find and understand why it's left as-is.tests/openwisp2/sample_monitoring/migrations/0001_initial.py (1)
104-110: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMirror the production rationale inline for these fixture tag fields.
These historical
TextFielddefinitions are intentionally counterintuitive, so a short comment here would help keep the fixture migration aligned with production and prevent a future cleanup from undoing the fix.As per coding guidelines, "Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable".
Also applies to: 114-118
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/openwisp2/sample_monitoring/migrations/0001_initial.py` around lines 104 - 110, Add a brief inline comment above the historical TextField fixture definitions (e.g., the "main_tags" models.TextField and the similar TextField at lines ~114-118) that explains the production rationale for using a TextField with db_index=True and default=dict (why this counterintuitive shape/choice is intentional and must be preserved in fixtures), so future maintainers know this mirrors production behavior and should not be simplified or converted without verifying compatibility.openwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.py (1)
15-20: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDocument why these historical tag fields intentionally stay
TextField.Both changes are subtle enough that a future migration cleanup can easily revert them. A concise comment above the first field explaining that these historical definitions must remain text-backed would make the intent durable.
As per coding guidelines, "Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable".
Also applies to: 25-29
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.py` around lines 15 - 20, Add a concise comment above the historical TextField definitions (the migrations' field=models.TextField(...) entries with verbose_name="main tags" and the analogous "extra tags" field) explaining that these fields are intentionally left as TextField for historical/migration compatibility (e.g., to preserve prior storage format and avoid altering DB-backed types in a historical migration), so future readers know not to change them; reference the TextField declarations in this migration (the models.TextField(...) lines) and add the same brief rationale above both occurrences.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@openwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.py`:
- Around line 67-73: Add a short inline comment above the params =
models.TextField(...) migration declaration explaining that this TextField is
intentionally preserved in the migration (historical/frozen model state) and
must not be converted to JSONField during migration cleanup—mention
compatibility/serialization reasons and that changing it would alter the
historical migration record; reference the params field and the TextField
declaration so reviewers can find and understand why it's left as-is.
In
`@openwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.py`:
- Around line 15-20: Add a concise comment above the historical TextField
definitions (the migrations' field=models.TextField(...) entries with
verbose_name="main tags" and the analogous "extra tags" field) explaining that
these fields are intentionally left as TextField for historical/migration
compatibility (e.g., to preserve prior storage format and avoid altering
DB-backed types in a historical migration), so future readers know not to change
them; reference the TextField declarations in this migration (the
models.TextField(...) lines) and add the same brief rationale above both
occurrences.
In `@tests/openwisp2/sample_check/migrations/0001_initial.py`:
- Around line 74-80: The test migration's "params" TextField uses default=dict
(a non-obvious choice) but lacks the explanatory rationale present in
production; add a concise inline comment next to the "params" field definition
(the TextField for "params") that explains why default=dict is used and that
this mirrors the historical production field definition so the fixture stays in
sync and is not accidentally "fixed" later.
In `@tests/openwisp2/sample_monitoring/migrations/0001_initial.py`:
- Around line 104-110: Add a brief inline comment above the historical TextField
fixture definitions (e.g., the "main_tags" models.TextField and the similar
TextField at lines ~114-118) that explains the production rationale for using a
TextField with db_index=True and default=dict (why this counterintuitive
shape/choice is intentional and must be preserved in fixtures), so future
maintainers know this mirrors production behavior and should not be simplified
or converted without verifying compatibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 57ee096c-3371-4a93-a53a-3d1e398dbc54
📒 Files selected for processing (5)
openwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.pyopenwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.pytests/openwisp2/sample_check/migrations/0001_initial.pytests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_monitoring/migrations/0001_initial.py
💤 Files with no reviewable changes (1)
- tests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.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). (11)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.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~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (1)
{**/*.sh,**/*.{js,ts,tsx,jsx},**/*.py,**/*.rb,**/*.go,**/*.java,**/*.cs,**/*.cpp,**/*.c}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
tests/openwisp2/sample_monitoring/migrations/0001_initial.pytests/openwisp2/sample_check/migrations/0001_initial.pyopenwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.pyopenwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.py
🧠 Learnings (4)
📚 Learning: 2026-02-25T18:42:08.825Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:309-317
Timestamp: 2026-02-25T18:42:08.825Z
Learning: In Selenium tests that use ChannelsLiveServerTestCase (e.g., openwisp_monitoring/tests/test_selenium.py), override settings to configure CHANNEL_LAYERS with channels_redis.core.RedisChannelLayer instead of InMemoryChannelLayer. This is required because the live server runs in a separate process from the test process, and InMemoryChannelLayer is per-process only and cannot handle cross-process WebSocket broadcasting needed for real-time location update tests.
Applied to files:
tests/openwisp2/sample_monitoring/migrations/0001_initial.pytests/openwisp2/sample_check/migrations/0001_initial.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In this repository (openwisp/openwisp-monitoring), the project targets Python 3.10–3.13 as defined by CI. Do not flag backports.zoneinfo as a missing dependency; zoneinfo is a built-in module in all supported Python versions. When reviewing Python code, assume zoneinfo is available and avoid suggesting installation of backports.zoneinfo. If a file imports zoneinfo or uses it for time zone handling, treat it as standard library usage compatible with the supported CI matrix.
Applied to files:
tests/openwisp2/sample_monitoring/migrations/0001_initial.pytests/openwisp2/sample_check/migrations/0001_initial.pyopenwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.pyopenwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In the openwisp-monitoring project, targets are Linux-based environments. Do not flag a Windows-specific tzdata package as a missing dependency in code reviews for Python files (e.g., openwisp_monitoring/utils.py). If a platform-specific dependency is truly required, document the exception in review guidelines and ensure CI/packaging checks enforce platform constraints rather than manual review.
Applied to files:
tests/openwisp2/sample_monitoring/migrations/0001_initial.pytests/openwisp2/sample_check/migrations/0001_initial.pyopenwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.pyopenwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.py
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.pyopenwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.py
pandafy
force-pushed
the
issues/798-jsonb-migration
branch
2 times, most recently
from
edc9b6b to
7dbc290
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@openwisp_monitoring/monitoring/migrations/0014_ensure_database_json_field.py`:
- Around line 23-49: The migration 0014_ensure_database_json_field.py currently
contains only migrations.AlterField for model "metric" fields main_tags and
extra_tags which keeps it a JSONField→JSONField no-op; instead change 0014 to
use migrations.SeparateDatabaseAndState: in the database operations execute
explicit SQL (e.g. ALTER TABLE ... ALTER COLUMN ... TYPE jsonb USING ... or
appropriate RunSQL) to convert the existing text column to jsonb for both
main_tags and extra_tags, and in the state operations set the field definitions
to models.JSONField (or alternately set the state first to models.TextField then
apply AlterField to JSONField) so Django’s migration state matches reality;
reference the migration class 0014_ensure_database_json_field.py, the
migrations.SeparateDatabaseAndState wrapper, migrations.RunSQL (or RunPython)
and the migrations.AlterField entries for model_name="metric"
name="main_tags"/"extra_tags" when making the change.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d878624b-b6e4-4d3c-85fd-13e1cd2799f6
📒 Files selected for processing (6)
openwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.pyopenwisp_monitoring/check/migrations/0013_ensure_database_json_field.pyopenwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.pyopenwisp_monitoring/monitoring/migrations/0014_ensure_database_json_field.pytests/openwisp2/sample_check/migrations/0001_initial.pytests/openwisp2/sample_monitoring/migrations/0001_initial.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). (12)
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | 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~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (1)
{**/*.sh,**/*.{js,ts,tsx,jsx},**/*.py,**/*.rb,**/*.go,**/*.java,**/*.cs,**/*.cpp,**/*.c}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_monitoring/check/migrations/0013_ensure_database_json_field.pyopenwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.pytests/openwisp2/sample_check/migrations/0001_initial.pytests/openwisp2/sample_monitoring/migrations/0001_initial.pyopenwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.pyopenwisp_monitoring/monitoring/migrations/0014_ensure_database_json_field.py
🧠 Learnings (4)
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/check/migrations/0013_ensure_database_json_field.pyopenwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.pyopenwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.pyopenwisp_monitoring/monitoring/migrations/0014_ensure_database_json_field.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In this repository (openwisp/openwisp-monitoring), the project targets Python 3.10–3.13 as defined by CI. Do not flag backports.zoneinfo as a missing dependency; zoneinfo is a built-in module in all supported Python versions. When reviewing Python code, assume zoneinfo is available and avoid suggesting installation of backports.zoneinfo. If a file imports zoneinfo or uses it for time zone handling, treat it as standard library usage compatible with the supported CI matrix.
Applied to files:
openwisp_monitoring/check/migrations/0013_ensure_database_json_field.pyopenwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.pytests/openwisp2/sample_check/migrations/0001_initial.pytests/openwisp2/sample_monitoring/migrations/0001_initial.pyopenwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.pyopenwisp_monitoring/monitoring/migrations/0014_ensure_database_json_field.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In the openwisp-monitoring project, targets are Linux-based environments. Do not flag a Windows-specific tzdata package as a missing dependency in code reviews for Python files (e.g., openwisp_monitoring/utils.py). If a platform-specific dependency is truly required, document the exception in review guidelines and ensure CI/packaging checks enforce platform constraints rather than manual review.
Applied to files:
openwisp_monitoring/check/migrations/0013_ensure_database_json_field.pyopenwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.pytests/openwisp2/sample_check/migrations/0001_initial.pytests/openwisp2/sample_monitoring/migrations/0001_initial.pyopenwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.pyopenwisp_monitoring/monitoring/migrations/0014_ensure_database_json_field.py
📚 Learning: 2026-02-25T18:42:08.825Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:309-317
Timestamp: 2026-02-25T18:42:08.825Z
Learning: In Selenium tests that use ChannelsLiveServerTestCase (e.g., openwisp_monitoring/tests/test_selenium.py), override settings to configure CHANNEL_LAYERS with channels_redis.core.RedisChannelLayer instead of InMemoryChannelLayer. This is required because the live server runs in a separate process from the test process, and InMemoryChannelLayer is per-process only and cannot handle cross-process WebSocket broadcasting needed for real-time location update tests.
Applied to files:
tests/openwisp2/sample_check/migrations/0001_initial.pytests/openwisp2/sample_monitoring/migrations/0001_initial.py
🔇 Additional comments (5)
openwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.py (1)
67-73: LGTM!tests/openwisp2/sample_check/migrations/0001_initial.py (1)
74-80: LGTM!openwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.py (1)
15-20: LGTM!Also applies to: 25-29
tests/openwisp2/sample_monitoring/migrations/0001_initial.py (1)
104-110: LGTM!Also applies to: 113-118
openwisp_monitoring/check/migrations/0013_ensure_database_json_field.py (1)
23-38: 🏗️ Heavy liftThe migration strategy is already documented and addresses your concern.
The docstring (lines 6–14) explicitly explains that historical migrations have been restored to
TextFieldsemantics. This restoration ensures that 0013'sAlterFieldrepresents aTextField → JSONFieldtransition at Django's migration state level, which correctly triggers the database schema conversion despite 0012 also targetingJSONField. This dual-step approach intentionally works around Django's no-op detection for identical field transitions.No additional comments or refactoring needed—the code follows the guideline of documenting non-obvious migration logic.
Comment on lines
+23
to
+49
| dependencies = [ | ||
| ("monitoring", "0013_replace_jsonfield_with_django_builtin"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AlterField( | ||
| model_name="metric", | ||
| name="main_tags", | ||
| field=models.JSONField( | ||
| blank=True, | ||
| db_index=True, | ||
| default=dict, | ||
| encoder=django.core.serializers.json.DjangoJSONEncoder, | ||
| verbose_name="main tags", | ||
| ), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name="metric", | ||
| name="extra_tags", | ||
| field=models.JSONField( | ||
| blank=True, | ||
| default=dict, | ||
| encoder=django.core.serializers.json.DjangoJSONEncoder, | ||
| verbose_name="extra tags", | ||
| ), | ||
| ), | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== previous replace migration ==="
fd '^0013_replace_jsonfield_with_django_builtin\.py$' | while read -r f; do
echo "--- $f ---"
rg -n -C2 'migrations\.AlterField|name="main_tags"|name="extra_tags"|models\.JSONField' "$f"
done
echo
echo "=== repair migration ==="
rg -n -C2 'migrations\.AlterField|name="main_tags"|name="extra_tags"|models\.JSONField' \
openwisp_monitoring/monitoring/migrations/0014_ensure_database_json_field.pyRepository: openwisp/openwisp-monitoring
Length of output: 1874
The migration's claimed fix doesn't match its implementation—it still risks being a JSONField→JSONField no-op.
The docstring correctly identifies the exact problem: migration 0013 already uses models.JSONField, so reapplying it in 0014 causes Django to skip the required database conversion. However, the docstring claims to address this by "restoring the historical migration state to TextField semantics," yet the actual 0014 migration contains only AlterField operations without any prior restoration step. Django will still interpret this as JSONField → JSONField and skip the PostgreSQL text → jsonb conversion.
The fix requires either:
- Manually editing 0013 back to
TextFieldbefore 0014 applies (but migrations should be immutable), or - Using
SeparateDatabaseAndState+ explicit SQL in 0014 to force the schema change, or - A different sequential strategy where 0014 first undoes 0013 (TextField), then reapplies JSONField
As written, 0014 will not repair existing PostgreSQL installations that already ran 0013.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openwisp_monitoring/monitoring/migrations/0014_ensure_database_json_field.py`
around lines 23 - 49, The migration 0014_ensure_database_json_field.py currently
contains only migrations.AlterField for model "metric" fields main_tags and
extra_tags which keeps it a JSONField→JSONField no-op; instead change 0014 to
use migrations.SeparateDatabaseAndState: in the database operations execute
explicit SQL (e.g. ALTER TABLE ... ALTER COLUMN ... TYPE jsonb USING ... or
appropriate RunSQL) to convert the existing text column to jsonb for both
main_tags and extra_tags, and in the state operations set the field definitions
to models.JSONField (or alternately set the state first to models.TextField then
apply AlterField to JSONField) so Django’s migration state matches reality;
reference the migration class 0014_ensure_database_json_field.py, the
migrations.SeparateDatabaseAndState wrapper, migrations.RunSQL (or RunPython)
and the migrations.AlterField entries for model_name="metric"
name="main_tags"/"extra_tags" when making the change.
7dbc290 to
9532aaf
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.py (1)
4-20:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
import djangodoes not exposedjango.coreas an attribute; accessingdjango.core.serializers.json.DjangoJSONEncoderat Line 20 will raiseAttributeErrorwhen the migration is imported.Python's import system does not automatically bind nested submodules as attributes on the parent module. Using only
import djangodoes not makedjango.coreavailable as an attribute, and the encoder reference will fail at migration import time.Proposed fix
-import django +from django.core.serializers.json import DjangoJSONEncoder from django.db import migrations, models @@ - encoder=django.core.serializers.json.DjangoJSONEncoder, + encoder=DjangoJSONEncoder,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.py` around lines 4 - 20, The migration currently does only "import django" but then references django.core.serializers.json.DjangoJSONEncoder which will raise AttributeError; update imports so the encoder symbol is imported directly (for example, use "from django.core.serializers.json import DjangoJSONEncoder") and then pass DjangoJSONEncoder to models.JSONField(encoder=...) in the AlterField for model_name="check" name="params" (or alternatively import the django.core.serializers.json module explicitly and reference its DjangoJSONEncoder), ensuring the encoder reference resolves at import time.openwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.py (1)
66-74: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding a comment explaining the historical TextField usage.
Using
models.TextFieldwithdefault=dictis non-obvious and could confuse future maintainers. A brief comment would clarify that this preserves the originaljsonfield.fields.JSONFieldsemantics (which inherited from TextField) to allow Django to detect the text→jsonb transition.📝 Suggested comment
( "params", + # TextField preserves original jsonfield.fields.JSONField semantics + # (inherited from TextField) so Django detects text→jsonb transition. models.TextField( blank=True, default=dict, help_text="parameters needed to perform the check", verbose_name="parameters", ), ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.py` around lines 66 - 74, Add a short inline comment next to the "params" field declaration explaining why models.TextField is used with default=dict (i.e., it preserves the historical jsonfield.fields.JSONField behavior that inherited from TextField so Django can detect the text→jsonb transition), so future maintainers understand this non-obvious choice; reference the "params" field, models.TextField and default=dict in the comment for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@openwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.py`:
- Around line 66-74: Add a short inline comment next to the "params" field
declaration explaining why models.TextField is used with default=dict (i.e., it
preserves the historical jsonfield.fields.JSONField behavior that inherited from
TextField so Django can detect the text→jsonb transition), so future maintainers
understand this non-obvious choice; reference the "params" field,
models.TextField and default=dict in the comment for clarity.
In
`@tests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.py`:
- Around line 4-20: The migration currently does only "import django" but then
references django.core.serializers.json.DjangoJSONEncoder which will raise
AttributeError; update imports so the encoder symbol is imported directly (for
example, use "from django.core.serializers.json import DjangoJSONEncoder") and
then pass DjangoJSONEncoder to models.JSONField(encoder=...) in the AlterField
for model_name="check" name="params" (or alternatively import the
django.core.serializers.json module explicitly and reference its
DjangoJSONEncoder), ensuring the encoder reference resolves at import time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fa942a26-e77f-4d24-9a57-38d7492de57a
📒 Files selected for processing (5)
openwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.pyopenwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.pytests/openwisp2/sample_check/migrations/0001_initial.pytests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_monitoring/migrations/0001_initial.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). (11)
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (1)
{**/*.sh,**/*.{js,ts,tsx,jsx},**/*.py,**/*.rb,**/*.go,**/*.java,**/*.cs,**/*.cpp,**/*.c}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.pytests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_check/migrations/0001_initial.pytests/openwisp2/sample_monitoring/migrations/0001_initial.pyopenwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.py
🧠 Learnings (4)
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.pyopenwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In this repository (openwisp/openwisp-monitoring), the project targets Python 3.10–3.13 as defined by CI. Do not flag backports.zoneinfo as a missing dependency; zoneinfo is a built-in module in all supported Python versions. When reviewing Python code, assume zoneinfo is available and avoid suggesting installation of backports.zoneinfo. If a file imports zoneinfo or uses it for time zone handling, treat it as standard library usage compatible with the supported CI matrix.
Applied to files:
openwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.pytests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_check/migrations/0001_initial.pytests/openwisp2/sample_monitoring/migrations/0001_initial.pyopenwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In the openwisp-monitoring project, targets are Linux-based environments. Do not flag a Windows-specific tzdata package as a missing dependency in code reviews for Python files (e.g., openwisp_monitoring/utils.py). If a platform-specific dependency is truly required, document the exception in review guidelines and ensure CI/packaging checks enforce platform constraints rather than manual review.
Applied to files:
openwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.pytests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_check/migrations/0001_initial.pytests/openwisp2/sample_monitoring/migrations/0001_initial.pyopenwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.py
📚 Learning: 2026-02-25T18:42:08.825Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:309-317
Timestamp: 2026-02-25T18:42:08.825Z
Learning: In Selenium tests that use ChannelsLiveServerTestCase (e.g., openwisp_monitoring/tests/test_selenium.py), override settings to configure CHANNEL_LAYERS with channels_redis.core.RedisChannelLayer instead of InMemoryChannelLayer. This is required because the live server runs in a separate process from the test process, and InMemoryChannelLayer is per-process only and cannot handle cross-process WebSocket broadcasting needed for real-time location update tests.
Applied to files:
tests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_check/migrations/0001_initial.pytests/openwisp2/sample_monitoring/migrations/0001_initial.py
🔇 Additional comments (3)
tests/openwisp2/sample_check/migrations/0001_initial.py (1)
73-81: LGTM!openwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.py (1)
11-37: LGTM!tests/openwisp2/sample_monitoring/migrations/0001_initial.py (1)
103-119: LGTM!
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
Member
Author
|
@coderabbitai full review! |
🧩 Analysis chain🏁 Script executed: Length of output: 6207 🏁 Script executed: Length of output: 3703 🏁 Script executed: Length of output: 3331
ReviewCore Logic ✅The fix is sound. The original third-party The migration chains are consistent across both apps:
Issue 1 — Broken import in
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@tests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.py`:
- Line 4: The import "import django" does not expose nested submodules and
causes AttributeError when referencing
django.core.serializers.json.DjangoJSONEncoder in the migration; update the
import to explicitly import the submodule or the class (e.g., import
django.core.serializers.json or from django.core.serializers.json import
DjangoJSONEncoder) so that references to
django.core.serializers.json.DjangoJSONEncoder succeed in the
0004_replace_jsonfield_with_django_builtin migration.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b7d754ea-d5fc-40d9-8c12-e1f1193a4cf9
📒 Files selected for processing (5)
openwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.pyopenwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.pytests/openwisp2/sample_check/migrations/0001_initial.pytests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.pytests/openwisp2/sample_monitoring/migrations/0001_initial.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
{**/*.sh,**/*.{js,ts,tsx,jsx},**/*.py,**/*.rb,**/*.go,**/*.java,**/*.cs,**/*.cpp,**/*.c}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.pytests/openwisp2/sample_check/migrations/0001_initial.pytests/openwisp2/sample_monitoring/migrations/0001_initial.pytests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.pyopenwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.py
🧠 Learnings (4)
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.pyopenwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In this repository (openwisp/openwisp-monitoring), the project targets Python 3.10–3.13 as defined by CI. Do not flag backports.zoneinfo as a missing dependency; zoneinfo is a built-in module in all supported Python versions. When reviewing Python code, assume zoneinfo is available and avoid suggesting installation of backports.zoneinfo. If a file imports zoneinfo or uses it for time zone handling, treat it as standard library usage compatible with the supported CI matrix.
Applied to files:
openwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.pytests/openwisp2/sample_check/migrations/0001_initial.pytests/openwisp2/sample_monitoring/migrations/0001_initial.pytests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.pyopenwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In the openwisp-monitoring project, targets are Linux-based environments. Do not flag a Windows-specific tzdata package as a missing dependency in code reviews for Python files (e.g., openwisp_monitoring/utils.py). If a platform-specific dependency is truly required, document the exception in review guidelines and ensure CI/packaging checks enforce platform constraints rather than manual review.
Applied to files:
openwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.pytests/openwisp2/sample_check/migrations/0001_initial.pytests/openwisp2/sample_monitoring/migrations/0001_initial.pytests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.pyopenwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.py
📚 Learning: 2026-02-25T18:42:08.825Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:309-317
Timestamp: 2026-02-25T18:42:08.825Z
Learning: In Selenium tests that use ChannelsLiveServerTestCase (e.g., openwisp_monitoring/tests/test_selenium.py), override settings to configure CHANNEL_LAYERS with channels_redis.core.RedisChannelLayer instead of InMemoryChannelLayer. This is required because the live server runs in a separate process from the test process, and InMemoryChannelLayer is per-process only and cannot handle cross-process WebSocket broadcasting needed for real-time location update tests.
Applied to files:
tests/openwisp2/sample_check/migrations/0001_initial.pytests/openwisp2/sample_monitoring/migrations/0001_initial.pytests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.py
🔇 Additional comments (4)
openwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.py (1)
67-74: LGTM!tests/openwisp2/sample_check/migrations/0001_initial.py (1)
73-81: LGTM!openwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.py (1)
11-30: LGTM!tests/openwisp2/sample_monitoring/migrations/0001_initial.py (1)
103-119: LGTM!
| # Replaces third-party jsonfield.fields.JSONField with Django's built-in models.JSONField | ||
|
|
||
| import django.core.serializers.json | ||
| import django |
There was a problem hiding this comment.
Import does not expose the required submodule.
import django only imports the top-level package; it does not make nested submodules like django.core.serializers.json accessible as attributes. Line 20's reference to django.core.serializers.json.DjangoJSONEncoder will raise AttributeError: module 'django' has no attribute 'core' when the migration runs.
Restore the explicit submodule import.
🐛 Proposed fix
-import django
+import django.core.serializers.json
from django.db import migrations, modelsAlso applies to: 17-23
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tests/openwisp2/sample_check/migrations/0004_replace_jsonfield_with_django_builtin.py`
at line 4, The import "import django" does not expose nested submodules and
causes AttributeError when referencing
django.core.serializers.json.DjangoJSONEncoder in the migration; update the
import to explicitly import the submodule or the class (e.g., import
django.core.serializers.json or from django.core.serializers.json import
DjangoJSONEncoder) so that references to
django.core.serializers.json.DjangoJSONEncoder succeed in the
0004_replace_jsonfield_with_django_builtin migration.
Historical migrations were updated to use models.TextField instead of models.JSONField for fields originally backed by the third-party jsonfield library. The original jsonfield.fields.JSONField inherited from TextField and stored values as PostgreSQL text. Replacing historical migration definitions with models.JSONField caused Django to interpret the migration transition as JSONField -> JSONField, resulting in a no-op migration and preventing PostgreSQL columns from being converted to jsonb. By preserving the historical field semantics as TextField, Django can correctly detect the transition from text -> jsonb and generate the required ALTER COLUMN ... TYPE jsonb migration SQL during upgrades. Closes #798
pandafy
force-pushed
the
issues/798-jsonb-migration
branch
from
9532aaf to
edfef92
Compare
nemesifier
approved these changes
May 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
Reference to Existing Issue
Closes #798
Description of Changes
Historical migrations were updated to use models.TextField instead of models.JSONField for fields originally backed by the third-party jsonfield library.
The original jsonfield.fields.JSONField inherited from TextField and stored values as PostgreSQL text. Replacing historical migration definitions with models.JSONField caused Django to interpret the migration transition as JSONField -> JSONField, resulting in a no-op migration and preventing PostgreSQL columns from being converted to jsonb.
By preserving the historical field semantics as TextField, Django can correctly detect the transition from text -> jsonb and generate the required ALTER COLUMN ... TYPE jsonb migration SQL during upgrades.