[change] Replace third-party JSONField with native models.JSONField a…#743
[change] Replace third-party JSONField with native models.JSONField a…#743HMTAhir327 wants to merge 4 commits intoopenwisp:masterfrom
Conversation
…nd optimize indexing
|
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:
📝 WalkthroughWalkthroughReplaces external jsonfield JSONField usages with Django's built-in models.JSONField across models and migrations (fields: params, main_tags, extra_tags), removes custom dump_kwargs/load_kwargs, adds GinIndex declarations for main_tags and extra_tags, adds migration 0013_migrate_to_jsonfield which on PostgreSQL casts text columns to jsonb, updates/drops/creates indexes and uses SeparateDatabaseAndState, adds a CI step that waits for InfluxDB readiness, and adds test/dev requirements (commitizen, prompt-toolkit exclusion, django-filter constraint). Sequence Diagram(s)sequenceDiagram
participant Runner as Migration Runner
participant Migration as 0013_migrate_to_jsonfield
participant DB as PostgreSQL
participant State as Django Model State
Runner->>Migration: start migration
Migration->>DB: DROP existing text indexes (if present)
Migration->>DB: ALTER COLUMN main_tags TYPE jsonb USING main_tags::jsonb
Migration->>DB: ALTER COLUMN extra_tags TYPE jsonb USING extra_tags::jsonb
Migration->>DB: CREATE GIN INDEX on main_tags
Migration->>DB: CREATE GIN INDEX on extra_tags
Migration->>State: apply state_operations (fields -> models.JSONField)
Runner->>Migration: finish
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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. 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: 7
🤖 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_monitoring/monitoring/base/models.py`:
- Around line 94-99: Remove the redundant B-tree index by deleting db_index=True
from the main_tags JSONField declaration (symbol: main_tags) so only the
GinIndex declared in Meta.indexes is used; then update the corresponding
migration 0004_metric_main_and_extra_tags.py to drop/remove the db_index on
main_tags (or regenerate the migration) and also update
tests/openwisp2/sample_monitoring/migrations/0001_initial.py to reflect the same
field definition (no db_index) so test DB schema and migrations remain
consistent. Ensure the GinIndex referenced in Meta.indexes remains unchanged.
- Around line 119-122: The model's Meta.indexes lists two GinIndex entries
(main_tags_gin_idx and extra_tags_gin_idx) but migration 0013 created those GIN
indexes via RunSQL without updating Django's migration state, so the
autodetector keeps emitting AddIndex migrations; update the third RunSQL in
migration 0013 to use SeparateDatabaseAndState (or add state_operations) that
include AddIndex operations for the same GinIndex instances (reflecting
Meta.indexes) so the migration state records the indexes and makemigrations
stops regenerating them.
In
`@openwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.py`:
- Around line 13-31: Remove the unnecessary B-tree index creation by deleting
the db_index=True argument from the migrations.AddField that adds the
"main_tags" JSONField in migration 0004_metric_main_and_extra_tags.py (ensure
the AddField for model_name="metric", name="main_tags" keeps blank=True,
default=dict, verbose_name, etc. but no db_index). This prevents creating the
B-tree on fresh installs; migration 0013 already drops any existing B-tree and
creates the GIN index for existing databases.
In `@openwisp_monitoring/monitoring/migrations/0013_auto_20260224_0730.py`:
- Around line 14-19: The reverse_sql for the RunSQL that converts
monitoring_metric.main_tags and extra_tags back to text currently does not
recreate the original B-tree indexes (e.g.,
monitoring_metric_main_tags_2d8550ae) and the earlier DROP INDEX (RunSQL.noop)
prevents automatic restoration; update the migration by either adding reverse
SQL that recreates the dropped B-tree indexes after converting columns back to
text (recreate monitoring_metric_main_tags_* and monitoring_metric_extra_tags_*
with the original index definitions) or, if recreating is not possible, add a
clear inline comment in the migration near migrations.RunSQL explaining that
rollback will not restore those indexes and document the operational impact and
acceptance (including whether a table-scan/check on main_tags after rollback is
acceptable for SLA).
- Around line 20-24: The RunSQL call creates GIN indexes at DB level but doesn't
update Django's migration state, causing makemigrations to later emit AddIndex
for the same indexes; fix by importing GinIndex from
django.contrib.postgres.indexes and adding a state_operations parameter to that
migrations.RunSQL containing migrations.AddIndex entries that mirror the DB
indexes (e.g. migrations.AddIndex(model_name='metric',
index=GinIndex(fields=['main_tags'], name='main_tags_gin_idx')) and
migrations.AddIndex(model_name='metric', index=GinIndex(fields=['extra_tags'],
name='extra_tags_gin_idx'))), leaving the sql and reverse_sql as-is so DB and
Django state stay synchronized.
- Around line 9-24: The migration uses PostgreSQL-only SQL in migrations.RunSQL
blocks (ALTER ... TYPE jsonb, :: cast, GIN indexes) which will fail on SQLite;
modify the migration so the PostgreSQL-specific SQL is only executed when
schema_editor.connection.vendor == 'postgresql' — for example replace each
RunSQL with a RunPython that checks schema_editor.connection.vendor and executes
the SQL only for 'postgresql', or use SeparateDatabaseAndState to include the
Postgres RunSQL operations in the database-only part while providing a noop
state change for other backends; update the reverse_sql similarly or provide
noop reversals for non-Postgres.
In `@tests/openwisp2/sample_monitoring/migrations/0001_initial.py`:
- Around line 104-119: The migration sets db_index=True on the JSONField
"main_tags", which creates a B-tree index in tests and diverges from the
intended production GIN-only schema; remove db_index=True from the "main_tags"
field definition and, if you need GIN behavior in tests, add a GinIndex for the
JSON key on the migration's options["indexes"] (use
django.contrib.postgres.indexes.GinIndex targeting "main_tags" and/or
"extra_tags"); update the migration 0001_initial definition accordingly and
ensure the GinIndex import is added to the migration file.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
openwisp_monitoring/check/base/models.pyopenwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.pyopenwisp_monitoring/monitoring/base/models.pyopenwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.pyopenwisp_monitoring/monitoring/migrations/0013_auto_20260224_0730.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). (11)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.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.1.0
- GitHub Check: Python==3.13 | 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.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
🧰 Additional context used
🧠 Learnings (1)
📚 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/0013_auto_20260224_0730.pyopenwisp_monitoring/monitoring/base/models.pyopenwisp_monitoring/check/base/models.pyopenwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.py
🧬 Code graph analysis (1)
openwisp_monitoring/monitoring/migrations/0013_auto_20260224_0730.py (2)
openwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.py (1)
Migration(6-38)tests/openwisp2/sample_monitoring/migrations/0001_initial.py (1)
Migration(21-269)
🔇 Additional comments (3)
openwisp_monitoring/check/base/models.py (1)
37-42: LGTM — clean migration tomodels.JSONField.Removing
dump_kwargs/load_kwargsis correct; the built-in field handles serialisation internally and the public field contract (blank=True,default=dict) is preserved.tests/openwisp2/sample_check/migrations/0001_initial.py (1)
73-81: LGTM.Historical migration correctly updated to
models.JSONField, removing the now-unnecessary third-party import.openwisp_monitoring/check/migrations/0001_initial_squashed_0002_check_unique_together.py (1)
67-74: LGTM.Squashed migration updated cleanly to
models.JSONField, necessary to allow removal of thejsonfieldpackage dependency.
| main_tags = models.JSONField( | ||
| _("main tags"), | ||
| default=dict, | ||
| blank=True, | ||
| load_kwargs={"object_pairs_hook": OrderedDict}, | ||
| dump_kwargs={"indent": 4}, | ||
| db_index=True, | ||
| ) |
There was a problem hiding this comment.
Remove db_index=True from main_tags — redundant alongside GinIndex.
Django's own documentation states: "Index and Field.db_index both create a B-tree index, which isn't particularly helpful when querying JSONField. On PostgreSQL only, you can use GinIndex that is better suited." Keeping db_index=True here creates a redundant, less-useful B-tree index on top of the GIN index declared in Meta.indexes, wasting both index storage and write overhead on every row mutation.
Remove db_index=True from main_tags; the GIN index in Meta.indexes (lines 119–122) is sufficient. Also update migration 0004_metric_main_and_extra_tags.py and tests/openwisp2/sample_monitoring/migrations/0001_initial.py consistently.
🔧 Proposed fix
- main_tags = models.JSONField(
- _("main tags"),
- default=dict,
- blank=True,
- db_index=True,
- )
+ main_tags = models.JSONField(
+ _("main tags"),
+ default=dict,
+ blank=True,
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_monitoring/monitoring/base/models.py` around lines 94 - 99, Remove
the redundant B-tree index by deleting db_index=True from the main_tags
JSONField declaration (symbol: main_tags) so only the GinIndex declared in
Meta.indexes is used; then update the corresponding migration
0004_metric_main_and_extra_tags.py to drop/remove the db_index on main_tags (or
regenerate the migration) and also update
tests/openwisp2/sample_monitoring/migrations/0001_initial.py to reflect the same
field definition (no db_index) so test DB schema and migrations remain
consistent. Ensure the GinIndex referenced in Meta.indexes remains unchanged.
| indexes = [ | ||
| GinIndex(fields=["main_tags"], name="main_tags_gin_idx"), | ||
| GinIndex(fields=["extra_tags"], name="extra_tags_gin_idx"), | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "GinIndex\|AddIndex" openwisp_monitoring/monitoring/migrations/Repository: openwisp/openwisp-monitoring
Length of output: 54
🏁 Script executed:
# Find all migrations in the monitoring app
fd -t f "\.py$" openwisp_monitoring/monitoring/migrations/ | head -20
# Also check if the model file exists and examine its Meta.indexes
fd -t f "models\.py" openwisp_monitoring/monitoring/ | head -10
# Check for RunSQL in migration files more broadly
rg -l "RunSQL" openwisp_monitoring/monitoring/migrations/Repository: openwisp/openwisp-monitoring
Length of output: 1495
🏁 Script executed:
# Examine migration 0013
cat -n openwisp_monitoring/monitoring/migrations/0013_auto_20260224_0730.py
# Examine the model file for Meta.indexes
sed -n '100,150p' openwisp_monitoring/monitoring/base/models.py | cat -nRepository: openwisp/openwisp-monitoring
Length of output: 3585
Meta.indexes GIN entries are not reflected in migration state → makemigrations will generate spurious AddIndex migrations.
Migration 0013 creates the GIN indexes via raw RunSQL without state_operations, so Django's migration state after applying 0013 has no record of main_tags_gin_idx or extra_tags_gin_idx. The migration autodetector compares the model's Meta.indexes (which lists both GinIndex entries) against this state and will always detect a "pending change", regenerating AddIndex migrations on every makemigrations run. Applying those generated migrations against a database that already has the indexes from migration 0013 will fail with a "relation already exists" error.
Per Django documentation: "Much like RunSQL, ensure that if you change schema inside here you're either doing it outside the scope of the Django model system (e.g. triggers) or that you use SeparateDatabaseAndState to add in operations that will reflect your changes to the model state – otherwise, the versioned ORM and the autodetector will stop working correctly."
Fix: add state_operations to the third RunSQL in migration 0013 to include the corresponding AddIndex operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_monitoring/monitoring/base/models.py` around lines 119 - 122, The
model's Meta.indexes lists two GinIndex entries (main_tags_gin_idx and
extra_tags_gin_idx) but migration 0013 created those GIN indexes via RunSQL
without updating Django's migration state, so the autodetector keeps emitting
AddIndex migrations; update the third RunSQL in migration 0013 to use
SeparateDatabaseAndState (or add state_operations) that include AddIndex
operations for the same GinIndex instances (reflecting Meta.indexes) so the
migration state records the indexes and makemigrations stops regenerating them.
| migrations.AddField( | ||
| model_name="metric", | ||
| name="main_tags", | ||
| field=jsonfield.fields.JSONField( | ||
| field=models.JSONField( | ||
| blank=True, | ||
| db_index=True, | ||
| default=dict, | ||
| dump_kwargs={"indent": 4}, | ||
| load_kwargs={"object_pairs_hook": collections.OrderedDict}, | ||
| verbose_name="main tags", | ||
| ), | ||
| ), | ||
| migrations.AddField( | ||
| model_name="metric", | ||
| name="extra_tags", | ||
| field=jsonfield.fields.JSONField( | ||
| field=models.JSONField( | ||
| blank=True, | ||
| default=dict, | ||
| dump_kwargs={"indent": 4}, | ||
| load_kwargs={"object_pairs_hook": collections.OrderedDict}, | ||
| verbose_name="extra tags", | ||
| ), | ||
| ), |
There was a problem hiding this comment.
main_tags's db_index=True creates a wasteful B-tree that migration 0013 immediately drops on fresh installs.
On a fresh PostgreSQL installation, this migration creates a B-tree index on main_tags, then migration 0013 drops it and creates a GIN index instead. Django docs note that db_index creates a B-tree index which "isn't particularly helpful when querying JSONField." The B-tree creation and drop is unnecessary churn for fresh deployments.
Remove db_index=True from both main_tags definitions here (consistent with removing it from the model) so migration 0004 no longer creates the B-tree at all; migration 0013's DROP INDEX IF EXISTS already handles the case where the old B-tree exists on existing installations.
🔧 Proposed fix
migrations.AddField(
model_name="metric",
name="main_tags",
field=models.JSONField(
blank=True,
- db_index=True,
default=dict,
verbose_name="main tags",
),
),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_monitoring/monitoring/migrations/0004_metric_main_and_extra_tags.py`
around lines 13 - 31, Remove the unnecessary B-tree index creation by deleting
the db_index=True argument from the migrations.AddField that adds the
"main_tags" JSONField in migration 0004_metric_main_and_extra_tags.py (ensure
the AddField for model_name="metric", name="main_tags" keeps blank=True,
default=dict, verbose_name, etc. but no db_index). This prevents creating the
B-tree on fresh installs; migration 0013 already drops any existing B-tree and
creates the GIN index for existing databases.
| migrations.RunSQL( | ||
| sql='ALTER TABLE monitoring_metric ALTER COLUMN main_tags TYPE jsonb USING main_tags::jsonb; ' | ||
| 'ALTER TABLE monitoring_metric ALTER COLUMN extra_tags TYPE jsonb USING extra_tags::jsonb;', | ||
| reverse_sql='ALTER TABLE monitoring_metric ALTER COLUMN main_tags TYPE text; ' | ||
| 'ALTER TABLE monitoring_metric ALTER COLUMN extra_tags TYPE text;' | ||
| ), |
There was a problem hiding this comment.
reverse_sql for column type conversion does not restore the dropped B-tree indexes — incomplete rollback.
When unapplying this migration (rolling back to 0012), the columns revert to text but the previously-existing B-tree indexes (monitoring_metric_main_tags_2d8550ae etc.) are not recreated, leaving the table without any index on main_tags. The reverse_sql=migrations.RunSQL.noop on the first operation (the DROP INDEX) makes this impossible to automatically restore.
While perfect rollback isn't always achievable, at minimum add a comment in the migration acknowledging this, and consider whether a check=False table-scan on main_tags after rollback is acceptable for your SLA.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_monitoring/monitoring/migrations/0013_auto_20260224_0730.py` around
lines 14 - 19, The reverse_sql for the RunSQL that converts
monitoring_metric.main_tags and extra_tags back to text currently does not
recreate the original B-tree indexes (e.g.,
monitoring_metric_main_tags_2d8550ae) and the earlier DROP INDEX (RunSQL.noop)
prevents automatic restoration; update the migration by either adding reverse
SQL that recreates the dropped B-tree indexes after converting columns back to
text (recreate monitoring_metric_main_tags_* and monitoring_metric_extra_tags_*
with the original index definitions) or, if recreating is not possible, add a
clear inline comment in the migration near migrations.RunSQL explaining that
rollback will not restore those indexes and document the operational impact and
acceptance (including whether a table-scan/check on main_tags after rollback is
acceptable for SLA).
| migrations.RunSQL( | ||
| sql='CREATE INDEX IF NOT EXISTS main_tags_gin_idx ON monitoring_metric USING gin (main_tags); ' | ||
| 'CREATE INDEX IF NOT EXISTS extra_tags_gin_idx ON monitoring_metric USING gin (extra_tags);', | ||
| reverse_sql='DROP INDEX IF EXISTS main_tags_gin_idx; DROP INDEX IF EXISTS extra_tags_gin_idx;' | ||
| ), |
There was a problem hiding this comment.
Missing state_operations on the GIN index RunSQL — will generate spurious AddIndex migrations every time makemigrations runs.
The raw SQL creates main_tags_gin_idx and extra_tags_gin_idx in the database, but nothing in this RunSQL updates Django's migration state. Because AbstractMetric.Meta.indexes declares both GinIndex entries and the migration state has no record of them, the Django documentation warns: "If the actual state of the database and Django's view of the state get out of sync, this can break the migration framework, even leading to data loss. It's worth exercising caution and checking your database and state operations carefully. You can use sqlmigrate and dbshell to check your database operations. You can use makemigrations, especially with --dry-run, to check your state operations."
Every subsequent makemigrations run will detect the unrecorded GIN indexes and generate a new migration with migrations.AddIndex. Applying that migration against the live database (which already has the indexes from this raw SQL) will fail with "relation already exists".
Fix: Add state_operations to the third RunSQL (note you also need to import GinIndex in this migration file):
🔧 Proposed fix
+from django.contrib.postgres.indexes import GinIndex
from django.db import migrations
class Migration(migrations.Migration):
dependencies = [
('monitoring', '0012_migrate_signal_metrics'),
]
operations = [
migrations.RunSQL(
sql='DROP INDEX IF EXISTS monitoring_metric_main_tags_2d8550ae_like; '
'DROP INDEX IF EXISTS monitoring_metric_main_tags_2d8550ae;',
reverse_sql=migrations.RunSQL.noop
),
migrations.RunSQL(
sql='ALTER TABLE monitoring_metric ALTER COLUMN main_tags TYPE jsonb USING main_tags::jsonb; '
'ALTER TABLE monitoring_metric ALTER COLUMN extra_tags TYPE jsonb USING extra_tags::jsonb;',
reverse_sql='ALTER TABLE monitoring_metric ALTER COLUMN main_tags TYPE text; '
'ALTER TABLE monitoring_metric ALTER COLUMN extra_tags TYPE text;'
),
migrations.RunSQL(
sql='CREATE INDEX IF NOT EXISTS main_tags_gin_idx ON monitoring_metric USING gin (main_tags); '
'CREATE INDEX IF NOT EXISTS extra_tags_gin_idx ON monitoring_metric USING gin (extra_tags);',
reverse_sql='DROP INDEX IF EXISTS main_tags_gin_idx; DROP INDEX IF EXISTS extra_tags_gin_idx;',
+ state_operations=[
+ migrations.AddIndex(
+ model_name='metric',
+ index=GinIndex(fields=['main_tags'], name='main_tags_gin_idx'),
+ ),
+ migrations.AddIndex(
+ model_name='metric',
+ index=GinIndex(fields=['extra_tags'], name='extra_tags_gin_idx'),
+ ),
+ ],
),
]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_monitoring/monitoring/migrations/0013_auto_20260224_0730.py` around
lines 20 - 24, The RunSQL call creates GIN indexes at DB level but doesn't
update Django's migration state, causing makemigrations to later emit AddIndex
for the same indexes; fix by importing GinIndex from
django.contrib.postgres.indexes and adding a state_operations parameter to that
migrations.RunSQL containing migrations.AddIndex entries that mirror the DB
indexes (e.g. migrations.AddIndex(model_name='metric',
index=GinIndex(fields=['main_tags'], name='main_tags_gin_idx')) and
migrations.AddIndex(model_name='metric', index=GinIndex(fields=['extra_tags'],
name='extra_tags_gin_idx'))), leaving the sql and reverse_sql as-is so DB and
Django state stay synchronized.
| "main_tags", | ||
| jsonfield.fields.JSONField( | ||
| models.JSONField( | ||
| blank=True, | ||
| db_index=True, | ||
| default=dict, | ||
| dump_kwargs={"indent": 4}, | ||
| load_kwargs={"object_pairs_hook": collections.OrderedDict}, | ||
| verbose_name="main tags", | ||
| ), | ||
| ), | ||
| ( | ||
| "extra_tags", | ||
| jsonfield.fields.JSONField( | ||
| models.JSONField( | ||
| blank=True, | ||
| default=dict, | ||
| dump_kwargs={"indent": 4}, | ||
| load_kwargs={"object_pairs_hook": collections.OrderedDict}, | ||
| verbose_name="extra tags", | ||
| ), | ||
| ), |
There was a problem hiding this comment.
db_index=True inconsistency with the GIN-only intent.
main_tags keeps db_index=True here, which creates a B-tree index in the test database but no GIN index (there is no migration 0013 equivalent for the test app). The test schema diverges from the production schema target (GIN, no B-tree). Consider removing db_index=True and, if GIN index behaviour needs to be covered, adding it to the test migration's options["indexes"].
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/openwisp2/sample_monitoring/migrations/0001_initial.py` around lines
104 - 119, The migration sets db_index=True on the JSONField "main_tags", which
creates a B-tree index in tests and diverges from the intended production
GIN-only schema; remove db_index=True from the "main_tags" field definition and,
if you need GIN behavior in tests, add a GinIndex for the JSON key on the
migration's options["indexes"] (use django.contrib.postgres.indexes.GinIndex
targeting "main_tags" and/or "extra_tags"); update the migration 0001_initial
definition accordingly and ensure the GinIndex import is added to the migration
file.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.py`:
- Around line 9-26: The migration currently runs PostgreSQL-specific migrations
via migrations.RunSQL (the operations list containing migrations.RunSQL that
ALTER TABLE to jsonb, create GIN indexes, and DROP INDEX) which will fail on
SpatiaLite; update the migration to only execute those RunSQL statements when
connection.vendor == 'postgresql' by wrapping them in a migrations.RunPython
that checks connection.vendor and runs the SQL via schema_editor.execute (or use
migrations.SeparateDatabaseAndState to keep state changes while skipping SQL on
non-postgres). Specifically, locate the migrations.RunSQL entries for ALTER
TABLE ... TYPE jsonb, CREATE INDEX ... USING gin and the DROP INDEX IF EXISTS
statements and replace them with a Postgres-only guard using connection.vendor
(or SeparateDatabaseAndState) so tests using SpatiaLite/SQLite skip these
Postgres-specific statements.
- Around line 21-25: The migration 0013_migrate_to_jsonfield.py currently
creates main_tags_gin_idx and extra_tags_gin_idx via a migrations.RunSQL block
while the Metric model's Meta.indexes also declares GinIndex entries for those
same names (main_tags_gin_idx, extra_tags_gin_idx), causing dual management;
pick one approach and make the code match it: either remove the RunSQL block
from 0013_migrate_to_jsonfield.py if you want
Django/Metric.Meta.indexes/GinIndex to be the single source of truth (then
re-run makemigrations to let Django manage the indexes), or remove the GinIndex
entries from Metric.Meta.indexes (and keep the RunSQL) so the raw SQL migration
is the source of truth — update the code accordingly and ensure the names
(main_tags_gin_idx, extra_tags_gin_idx) no longer appear in both places.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.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.12 | django~=5.1.0
- GitHub Check: Python==3.12 | 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.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
🧰 Additional context used
🧠 Learnings (1)
📚 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/monitoring/migrations/0013_migrate_to_jsonfield.py
🔇 Additional comments (2)
openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.py (2)
10-14: The original review's primary claim is factually incorrect. Migration 0004 definitively shows thatextra_tagswas created withoutdb_index=True, so it never had B-tree indexes. Onlymain_tagshaddb_index=True(line 18 of migration 0004), making Operation 1's removal of onlymain_tagsindexes correct.The GIN indexes created in Operation 3 have names (
main_tags_gin_idxandextra_tags_gin_idx) that match the model'sMeta.indexesdeclarations, which is the correct approach and prevents duplicate index creation by Django.The migration is properly structured and requires no changes.
Likely an incorrect or invalid review comment.
15-20: The bare::jsonbcast is safe—all existing data in these columns contains valid JSON objects (specifically'{}'from thedefault=dictsetting), not empty strings or malformed data. Since migration 0004 defined these fields withdefault=dictandblank=True, the database stores'{}'(valid JSON), not empty strings. There is no evidence of empty strings in the data.The reverse SQL could optionally include an explicit
USING main_tags::textclause for consistency, but this is minor and not critical—PostgreSQL handles the implicit jsonb-to-text cast reliably.Likely an incorrect or invalid review comment.
| migrations.RunSQL( | ||
| sql="CREATE INDEX IF NOT EXISTS main_tags_gin_idx ON monitoring_metric USING gin (main_tags); " | ||
| "CREATE INDEX IF NOT EXISTS extra_tags_gin_idx ON monitoring_metric USING gin (extra_tags);", | ||
| reverse_sql="DROP INDEX IF EXISTS main_tags_gin_idx; DROP INDEX IF EXISTS extra_tags_gin_idx;", | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'GinIndex' --type=py -A5 -B5Repository: openwisp/openwisp-monitoring
Length of output: 2113
🏁 Script executed:
rg -n 'main_tags|extra_tags' --type=py -g '**/monitoring/migrations/*.py' -A3 -B3Repository: openwisp/openwisp-monitoring
Length of output: 5475
🏁 Script executed:
fd -e py -p '*/models*' | head -20Repository: openwisp/openwisp-monitoring
Length of output: 360
🏁 Script executed:
# Check the migration file more thoroughly to understand the complete context
cat -n openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.pyRepository: openwisp/openwisp-monitoring
Length of output: 1459
🏁 Script executed:
# Check if there are any subsequent migrations that might interact with these indexes
fd -type f -name '*.py' -path '*/monitoring/migrations/*' | sort | tail -10Repository: openwisp/openwisp-monitoring
Length of output: 242
🏁 Script executed:
# Check the model definition to confirm Meta.indexes is the source
sed -n '110,130p' openwisp_monitoring/monitoring/base/models.pyRepository: openwisp/openwisp-monitoring
Length of output: 663
Remove the RunSQL index creation or remove the GinIndex declarations from Meta.indexes—both cannot coexist.
The migration creates main_tags_gin_idx and extra_tags_gin_idx via RunSQL, but the Metric model's Meta.indexes also declares identical GinIndex objects with the same names. This creates dual management of the same indexes: the migration manages them explicitly via SQL, while Django's ORM expects to manage indexes declared in Meta.indexes through its own migration system.
Although the IF NOT EXISTS clause prevents immediate errors, this inconsistency will cause problems:
- When future migrations try to drop or modify these indexes, Django may not recognize them as ORM-managed indexes, leading to unexpected behavior.
- It's unclear which source (migration or ORM) should be the source of truth.
Choose one approach:
- Keep the
RunSQLand remove theGinIndexdeclarations fromMeta.indexes, or - Keep the
Meta.indexesdeclarations and remove thisRunSQLblock, letting Django auto-generate the migration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.py`
around lines 21 - 25, The migration 0013_migrate_to_jsonfield.py currently
creates main_tags_gin_idx and extra_tags_gin_idx via a migrations.RunSQL block
while the Metric model's Meta.indexes also declares GinIndex entries for those
same names (main_tags_gin_idx, extra_tags_gin_idx), causing dual management;
pick one approach and make the code match it: either remove the RunSQL block
from 0013_migrate_to_jsonfield.py if you want
Django/Metric.Meta.indexes/GinIndex to be the single source of truth (then
re-run makemigrations to let Django manage the indexes), or remove the GinIndex
entries from Metric.Meta.indexes (and keep the RunSQL) so the raw SQL migration
is the source of truth — update the code accordingly and ensure the names
(main_tags_gin_idx, extra_tags_gin_idx) no longer appear in both places.
HMTAhir327
force-pushed
the
change/replace-jsonfield-with-builtin
branch
from
4617aea to
dd5bf29
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.py (2)
38-59: Good use ofSeparateDatabaseAndStatewith vendor-guardedRunPython.The pattern correctly keeps Django's model state in sync via
state_operationswhile performing the actual PostgreSQL-specific DDL throughdatabase_operations. The vendor check ensures non-PostgreSQL backends (e.g., SpatiaLite in tests) are not affected.However, the dual management concern for GIN indexes (created here via raw SQL and declared in
Meta.indexeson the model) is still outstanding — see prior review comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.py` around lines 38 - 59, The migration uses SeparateDatabaseAndState with database operations that create GIN indexes via raw SQL in migrate_to_jsonb/rollback_jsonb while the Metric model also declares those GIN indexes in its Meta.indexes, causing duplicate-index creation/conflicts; remove the raw SQL GIN creation/drop from the database_operations (or conversely remove the index entries from Metric.Meta.indexes) so only one mechanism manages the GIN indexes, and update migrate_to_jsonb and rollback_jsonb to no longer create/drop the same GIN indexes (or to only create/drop them when Meta.indexes is not declaring them) to avoid double-management.
16-19: Dual GIN index management: raw SQLCREATE INDEXhere conflicts withGinIndexinMeta.indexes.The
CREATE INDEX IF NOT EXISTS main_tags_gin_idx/extra_tags_gin_idxhere will coexist with the identicalGinIndexdeclarations on the model'sMeta.indexes. WhileIF NOT EXISTSprevents an immediate error, Django's migration framework will consider itself the owner of theMeta.indexes-declared indexes, leading to confusion in future auto-generated migrations (e.g.,RemoveIndexorRenameIndexmay target an index it didn't create).Pick one owner: either keep raw SQL here and remove the
GinIndexfromMeta, or remove thisCREATE INDEXblock and letmakemigrationsgenerate the index migration fromMeta.indexes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.py` around lines 16 - 19, Remove the raw SQL index creation: delete the cursor.execute(...) block that runs "CREATE INDEX IF NOT EXISTS main_tags_gin_idx ..." in the migration and let the model-level GinIndex declarations in Meta.indexes own the indexes; this ensures Django's migration framework (via the GinIndex entries) will create/manage the GIN indexes instead of having duplicate raw-SQL-created indexes.
🤖 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_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.py`:
- Around line 22-30: The rollback_jsonb function currently drops the GIN indexes
and converts columns back to text but does not recreate the original B-tree
indexes removed by migrate_to_jsonb; update rollback_jsonb to, after altering
main_tags and extra_tags back to text, execute CREATE INDEX statements to
re-create the original B-tree indexes (specifically
monitoring_metric_main_tags_2d8550ae_like and
monitoring_metric_main_tags_2d8550ae and any equivalent extra_tags indexes that
migrate_to_jsonb dropped), ensuring index names and types match the originals
and only running on PostgreSQL (same vendor check) so the DB returns to its
pre-migration state.
- Around line 12-15: The migration's SQL in cursor.execute alters
monitoring_metric columns main_tags and extra_tags using a direct text→jsonb
cast which will fail for empty strings; change the USING expressions to guard
empty strings (e.g. use NULLIF(main_tags, '')::jsonb and NULLIF(extra_tags,
'')::jsonb) so empty strings become NULL before casting (update the SQL string
inside the cursor.execute call that performs ALTER TABLE monitoring_metric ALTER
COLUMN main_tags and extra_tags).
---
Duplicate comments:
In `@openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.py`:
- Around line 38-59: The migration uses SeparateDatabaseAndState with database
operations that create GIN indexes via raw SQL in
migrate_to_jsonb/rollback_jsonb while the Metric model also declares those GIN
indexes in its Meta.indexes, causing duplicate-index creation/conflicts; remove
the raw SQL GIN creation/drop from the database_operations (or conversely remove
the index entries from Metric.Meta.indexes) so only one mechanism manages the
GIN indexes, and update migrate_to_jsonb and rollback_jsonb to no longer
create/drop the same GIN indexes (or to only create/drop them when Meta.indexes
is not declaring them) to avoid double-management.
- Around line 16-19: Remove the raw SQL index creation: delete the
cursor.execute(...) block that runs "CREATE INDEX IF NOT EXISTS
main_tags_gin_idx ..." in the migration and let the model-level GinIndex
declarations in Meta.indexes own the indexes; this ensures Django's migration
framework (via the GinIndex entries) will create/manage the GIN indexes instead
of having duplicate raw-SQL-created indexes.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.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). (9)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
🧰 Additional context used
🧠 Learnings (1)
📚 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/monitoring/migrations/0013_migrate_to_jsonfield.py
🔇 Additional comments (1)
openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.py (1)
8-14: The migration correctly handles index drops —extra_tagsnever had B-tree indexes.Migration 0004 added
main_tagswithdb_index=Truebutextra_tagswithout it. Onlymain_tagshas auto-generated B-tree indexes (the ones with hash2d8550ae_likeand2d8550ae), which the migration correctly drops. Sinceextra_tagsnever haddb_index=True, no B-tree indexes exist for it, so theALTER COLUMNtojsonbis safe. The migration logic is sound.Likely an incorrect or invalid review comment.
| cursor.execute( | ||
| "ALTER TABLE monitoring_metric ALTER COLUMN main_tags TYPE jsonb USING main_tags::jsonb; " | ||
| "ALTER TABLE monitoring_metric ALTER COLUMN extra_tags TYPE jsonb USING extra_tags::jsonb;" | ||
| ) |
There was a problem hiding this comment.
Unsafe cast: empty-string values will crash the text → jsonb conversion.
If any row has main_tags = '' or extra_tags = '' (possible since blank=True), ''::jsonb raises invalid input syntax for type json. Use a guarded cast:
Proposed fix
cursor.execute(
- "ALTER TABLE monitoring_metric ALTER COLUMN main_tags TYPE jsonb USING main_tags::jsonb; "
- "ALTER TABLE monitoring_metric ALTER COLUMN extra_tags TYPE jsonb USING extra_tags::jsonb;"
+ "ALTER TABLE monitoring_metric ALTER COLUMN main_tags TYPE jsonb "
+ "USING CASE WHEN main_tags IS NULL THEN NULL "
+ "WHEN main_tags = '' THEN '{}'::jsonb "
+ "ELSE main_tags::jsonb END; "
+ "ALTER TABLE monitoring_metric ALTER COLUMN extra_tags TYPE jsonb "
+ "USING CASE WHEN extra_tags IS NULL THEN NULL "
+ "WHEN extra_tags = '' THEN '{}'::jsonb "
+ "ELSE extra_tags::jsonb END;"
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.py`
around lines 12 - 15, The migration's SQL in cursor.execute alters
monitoring_metric columns main_tags and extra_tags using a direct text→jsonb
cast which will fail for empty strings; change the USING expressions to guard
empty strings (e.g. use NULLIF(main_tags, '')::jsonb and NULLIF(extra_tags,
'')::jsonb) so empty strings become NULL before casting (update the SQL string
inside the cursor.execute call that performs ALTER TABLE monitoring_metric ALTER
COLUMN main_tags and extra_tags).
| def rollback_jsonb(apps, schema_editor): | ||
| if schema_editor.connection.vendor != "postgresql": | ||
| return | ||
| with schema_editor.connection.cursor() as cursor: | ||
| cursor.execute("DROP INDEX IF EXISTS main_tags_gin_idx; DROP INDEX IF EXISTS extra_tags_gin_idx;") | ||
| cursor.execute( | ||
| "ALTER TABLE monitoring_metric ALTER COLUMN main_tags TYPE text; " | ||
| "ALTER TABLE monitoring_metric ALTER COLUMN extra_tags TYPE text;" | ||
| ) |
There was a problem hiding this comment.
Rollback does not recreate the original B-tree indexes dropped by the forward migration.
migrate_to_jsonb drops monitoring_metric_main_tags_2d8550ae_like and monitoring_metric_main_tags_2d8550ae, but rollback_jsonb only drops the GIN indexes and converts back to text — it never restores the B-tree indexes. After a rollback the main_tags column (and extra_tags, if applicable) would lack the indexes that existed before this migration ran.
Consider adding CREATE INDEX statements in the rollback to restore the original state, or document that a full re-migration of 0004 onward is needed after rollback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.py`
around lines 22 - 30, The rollback_jsonb function currently drops the GIN
indexes and converts columns back to text but does not recreate the original
B-tree indexes removed by migrate_to_jsonb; update rollback_jsonb to, after
altering main_tags and extra_tags back to text, execute CREATE INDEX statements
to re-create the original B-tree indexes (specifically
monitoring_metric_main_tags_2d8550ae_like and
monitoring_metric_main_tags_2d8550ae and any equivalent extra_tags indexes that
migrate_to_jsonb dropped), ensuring index names and types match the originals
and only running on PostgreSQL (same vendor check) so the DB returns to its
pre-migration state.
dd5bf29 to
2012a75
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.py (3)
12-15:⚠️ Potential issue | 🔴 CriticalUnsafe
text → jsonbcast on empty strings will crash the migration.
blank=Trueon the legacy text field means rows can havemain_tags = ''orextra_tags = ''.''::jsonbraisesinvalid input syntax for type jsonon PostgreSQL, aborting the entire migration. Apply a guarded CASE expression as previously proposed:🐛 Proposed fix
cursor.execute( - "ALTER TABLE monitoring_metric ALTER COLUMN main_tags TYPE jsonb USING main_tags::jsonb; " - "ALTER TABLE monitoring_metric ALTER COLUMN extra_tags TYPE jsonb USING extra_tags::jsonb;" + "ALTER TABLE monitoring_metric ALTER COLUMN main_tags TYPE jsonb " + "USING CASE WHEN main_tags IS NULL THEN NULL " + "WHEN main_tags = '' THEN '{}'::jsonb " + "ELSE main_tags::jsonb END; " + "ALTER TABLE monitoring_metric ALTER COLUMN extra_tags TYPE jsonb " + "USING CASE WHEN extra_tags IS NULL THEN NULL " + "WHEN extra_tags = '' THEN '{}'::jsonb " + "ELSE extra_tags::jsonb END;" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.py` around lines 12 - 15, The ALTER TABLE SQL in migrations/0013_migrate_to_jsonfield.py uses unsafe casts like main_tags::jsonb and extra_tags::jsonb which will fail on empty strings; update the string in cursor.execute to use a guarded CASE/trim expression for both columns (e.g., in the USING clause, replace main_tags::jsonb with a CASE that treats empty/blank main_tags as an empty JSON array/object and otherwise casts main_tags to jsonb, and do the same for extra_tags) so the migration skips or converts blank values instead of crashing.
16-19:⚠️ Potential issue | 🟠 MajorVerify no dual management of
main_tags_gin_idx/extra_tags_gin_idx.If
Metric.Meta.indexesstill declaresGinIndexobjects with the same names (main_tags_gin_idx,extra_tags_gin_idx), two mechanisms manage the same physical indexes: thisRunPythoncursor block and Django's ORM migration machinery. TheIF NOT EXISTSguard prevents an immediate crash, but future ORM-generated migrations may attempt to create, drop, or rename indexes that were already created (or not) by this SQL block, causing silent divergence or errors.Run the following to confirm whether the duplicate declaration still exists:
#!/bin/bash # Check Metric model's Meta.indexes for GinIndex declarations rg -n 'GinIndex|main_tags_gin_idx|extra_tags_gin_idx' --type=py -A3 -B3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.py` around lines 16 - 19, The migration's RunPython SQL creates physical indexes named main_tags_gin_idx and extra_tags_gin_idx which may be duplicated by GinIndex entries in Metric.Meta.indexes; search for Metric.Meta.indexes (look for GinIndex, main_tags_gin_idx, extra_tags_gin_idx) and if those GinIndex declarations exist either remove or rename them so Django's ORM migration machinery doesn't try to manage the same physical indexes, or conversely remove the raw SQL index creation from the RunPython block and let Django create the indexes via model Meta.indexes; update the migration comment to document which mechanism (raw SQL vs Django index) is authoritative to avoid future drift.
22-30:⚠️ Potential issue | 🟡 Minor
rollback_jsonbleaves the schema without the original B-tree indexes.The forward migration drops
monitoring_metric_main_tags_2d8550ae_likeandmonitoring_metric_main_tags_2d8550ae, but the reverse never recreates them. After a rollback,main_tagsis back totextbut the previously existing B-tree indexes are permanently gone, leaving queries that relied on them unindexed.Add
CREATE INDEXstatements inrollback_jsonbto restore the originals:🔄 Proposed fix
def rollback_jsonb(apps, schema_editor): if schema_editor.connection.vendor != "postgresql": return with schema_editor.connection.cursor() as cursor: cursor.execute("DROP INDEX IF EXISTS main_tags_gin_idx; DROP INDEX IF EXISTS extra_tags_gin_idx;") cursor.execute( "ALTER TABLE monitoring_metric ALTER COLUMN main_tags TYPE text; " "ALTER TABLE monitoring_metric ALTER COLUMN extra_tags TYPE text;" ) + cursor.execute( + "CREATE INDEX IF NOT EXISTS monitoring_metric_main_tags_2d8550ae " + "ON monitoring_metric (main_tags); " + "CREATE INDEX IF NOT EXISTS monitoring_metric_main_tags_2d8550ae_like " + "ON monitoring_metric (main_tags varchar_pattern_ops);" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.py` around lines 22 - 30, rollback_jsonb currently drops the GIN indexes but fails to recreate the original B-tree indexes; update rollback_jsonb to execute CREATE INDEX statements to restore the two B-tree indexes that were removed in the forward migration (monitoring_metric_main_tags_2d8550ae and monitoring_metric_main_tags_2d8550ae_like) by running SQL like CREATE INDEX monitoring_metric_main_tags_2d8550ae ON monitoring_metric (main_tags) and CREATE INDEX monitoring_metric_main_tags_2d8550ae_like ON monitoring_metric (main_tags text_pattern_ops) (and similarly recreate any equivalent index for extra_tags if one existed) immediately after altering the column types back to text so that queries relying on those B-tree indexes are restored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Line 99: The current health-check condition using the line `if curl -sI
http://localhost:8086/ping | grep "204" > /dev/null; then` is fragile because it
can match "204" anywhere in headers; update that check to explicitly assert the
HTTP status line equals 204 by either anchoring the grep/regex to the start of
the status line (e.g., match lines beginning with "HTTP/" followed by the 204
status) or by using curl’s status-code output (write-out / -w) to capture the
numeric HTTP status and compare it to 204; apply this change to the existing
if-condition so only the actual HTTP status determines success.
- Around line 98-105: The loop currently always runs sleep 3 on every iteration
including the final failed attempt; update the for-loop block so that after the
curl check (curl -sI http://localhost:8086/ping | grep "204") you immediately
exit on success, and only call sleep 3 when you will retry (i.e., when the
current iteration is not the last). Concretely, keep the curl check and echo
"InfluxDB is ready!" / exit 0 on success, and move or gate the sleep 3 so it
only runs when i < 20 (or otherwise before the next iteration), avoiding the
trailing 3s stall after the 20th failed check.
In `@requirements-test.txt`:
- Line 8: The requirements line "django-filter<25.0" lacks a minimum version and
an explanation; update that requirement to include a sensible lower bound (for
example >=23.2) to prevent resolving very old releases and add an inline comment
on the same line explaining the <25.0 ceiling (e.g., note that django-filter
25.x removed the in‑built API schema generation methods deprecated since v23.2)
so future maintainers understand the constraint.
---
Duplicate comments:
In `@openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.py`:
- Around line 12-15: The ALTER TABLE SQL in
migrations/0013_migrate_to_jsonfield.py uses unsafe casts like main_tags::jsonb
and extra_tags::jsonb which will fail on empty strings; update the string in
cursor.execute to use a guarded CASE/trim expression for both columns (e.g., in
the USING clause, replace main_tags::jsonb with a CASE that treats empty/blank
main_tags as an empty JSON array/object and otherwise casts main_tags to jsonb,
and do the same for extra_tags) so the migration skips or converts blank values
instead of crashing.
- Around line 16-19: The migration's RunPython SQL creates physical indexes
named main_tags_gin_idx and extra_tags_gin_idx which may be duplicated by
GinIndex entries in Metric.Meta.indexes; search for Metric.Meta.indexes (look
for GinIndex, main_tags_gin_idx, extra_tags_gin_idx) and if those GinIndex
declarations exist either remove or rename them so Django's ORM migration
machinery doesn't try to manage the same physical indexes, or conversely remove
the raw SQL index creation from the RunPython block and let Django create the
indexes via model Meta.indexes; update the migration comment to document which
mechanism (raw SQL vs Django index) is authoritative to avoid future drift.
- Around line 22-30: rollback_jsonb currently drops the GIN indexes but fails to
recreate the original B-tree indexes; update rollback_jsonb to execute CREATE
INDEX statements to restore the two B-tree indexes that were removed in the
forward migration (monitoring_metric_main_tags_2d8550ae and
monitoring_metric_main_tags_2d8550ae_like) by running SQL like CREATE INDEX
monitoring_metric_main_tags_2d8550ae ON monitoring_metric (main_tags) and CREATE
INDEX monitoring_metric_main_tags_2d8550ae_like ON monitoring_metric (main_tags
text_pattern_ops) (and similarly recreate any equivalent index for extra_tags if
one existed) immediately after altering the column types back to text so that
queries relying on those B-tree indexes are restored.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.ymlopenwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.pyrequirements-test.txt
📜 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). (7)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
🧰 Additional context used
🧠 Learnings (1)
📚 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/monitoring/migrations/0013_migrate_to_jsonfield.py
| freezegun~=1.5.5 | ||
| commitizen~=4.13.0 | ||
| prompt-toolkit!=3.0.52 | ||
| django-filter<25.0 |
There was a problem hiding this comment.
django-filter<25.0 is missing a lower bound and the rationale for the upper cap is undocumented.
django-filter 25.1 removed the in-built API schema generation methods that had been deprecated since v23.2, which explains the <25.0 ceiling. However:
- No lower bound is specified —
pipcould technically resolve a very old version ofdjango-filterif nothing else narrows it down. - There is no inline comment explaining why 25.x is excluded, which makes the intent opaque for future maintainers.
🔧 Suggested fix
-django-filter<25.0
+django-filter>=24.0,<25.0 # cap at <25.0 avoids breaking removal of deprecated DRF schema methods in 25.1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| django-filter<25.0 | |
| django-filter>=24.0,<25.0 # cap at <25.0 avoids breaking removal of deprecated DRF schema methods in 25.1 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@requirements-test.txt` at line 8, The requirements line "django-filter<25.0"
lacks a minimum version and an explanation; update that requirement to include a
sensible lower bound (for example >=23.2) to prevent resolving very old releases
and add an inline comment on the same line explaining the <25.0 ceiling (e.g.,
note that django-filter 25.x removed the in‑built API schema generation methods
deprecated since v23.2) so future maintainers understand the constraint.
2012a75 to
c767b07
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
.github/workflows/ci.yml (2)
98-105: Trailingsleep 3on the final failed iteration — duplicate of existing review comment.On the 20th iteration, when the curl check fails,
sleep 3still executes before the loop falls through to the timeout error, adding a needless 3-second stall. Movingsleep 3to the top of the loop (before the curl check) avoids this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 98 - 105, The loop that waits for InfluxDB currently runs curl then sleeps, which causes an unnecessary final sleep after the 20th failed attempt; modify the for loop in the CI workflow (the block starting with for i in {1..20}; do and referencing the curl check of http://localhost:8086/ping and the sleep 3) to move the sleep 3 to the top of the loop (before the curl check) so the script sleeps between attempts but does not perform the trailing 3-second pause after the final failed iteration.
99-99:grep "204"is fragile — duplicate of existing review comment.The bare substring match can spuriously succeed on any header containing "204" (e.g.,
Content-Length: 2048). The previously suggested fix (grep -q "^HTTP/.* 204") still applies and has not been addressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml at line 99, The current health-check line using the pipeline "curl -sI http://localhost:8086/ping | grep \"204\"" is fragile because it can match any header containing "204"; update the conditional that invokes curl piped to grep to use an anchored HTTP-status match instead (e.g., change the grep invocation to use a pattern like ^HTTP/.* 204 or use curl's numeric status output) so only a true HTTP 204 response succeeds; locate the line containing the curl -sI ... | grep invocation and replace the grep pattern with the anchored match (or switch to curl -s -o /dev/null -w '%{http_code}' and compare to 204) to make the check robust.openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.py (3)
12-15:⚠️ Potential issue | 🔴 CriticalGuard empty-string values before casting to jsonb.
''::jsonbfails, andblank=Trueallows empty strings, so this migration can crash on existing data. UseNULLIFor a CASE guard.🛠️ Suggested fix
- "ALTER TABLE monitoring_metric ALTER COLUMN main_tags TYPE jsonb USING main_tags::jsonb; " - "ALTER TABLE monitoring_metric ALTER COLUMN extra_tags TYPE jsonb USING extra_tags::jsonb;" + "ALTER TABLE monitoring_metric ALTER COLUMN main_tags TYPE jsonb " + "USING NULLIF(main_tags, '')::jsonb; " + "ALTER TABLE monitoring_metric ALTER COLUMN extra_tags TYPE jsonb " + "USING NULLIF(extra_tags, '')::jsonb;"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.py` around lines 12 - 15, The ALTER TABLE statements in cursor.execute cast main_tags and extra_tags directly to jsonb which will fail on empty strings; modify the SQL in the cursor.execute call so each cast guards empty-string values (e.g., use NULLIF(main_tags, '')::jsonb or a CASE expression to convert '' to NULL before casting) for both main_tags and extra_tags to avoid migration crashes on blank values.
26-30:⚠️ Potential issue | 🟡 MinorRollback should recreate the dropped B-tree indexes.
Forward migration drops
monitoring_metric_main_tags_2d8550ae_likeandmonitoring_metric_main_tags_2d8550ae, but rollback never restores them, leaving post-rollback schema under-indexed.🛠️ Suggested fix (ensure opclass matches original)
cursor.execute( "ALTER TABLE monitoring_metric ALTER COLUMN main_tags TYPE text; " "ALTER TABLE monitoring_metric ALTER COLUMN extra_tags TYPE text;" ) + cursor.execute( + "CREATE INDEX IF NOT EXISTS monitoring_metric_main_tags_2d8550ae " + "ON monitoring_metric (main_tags); " + "CREATE INDEX IF NOT EXISTS monitoring_metric_main_tags_2d8550ae_like " + "ON monitoring_metric (main_tags varchar_pattern_ops);" + )Verification script (find original index definitions in earlier migrations):
#!/bin/bash rg -n --type=py 'monitoring_metric_main_tags_2d8550ae' -C3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.py` around lines 26 - 30, The rollback currently doesn't recreate the two B-tree indexes dropped in the forward migration; update the reverse migration to recreate monitoring_metric_main_tags_2d8550ae_like and monitoring_metric_main_tags_2d8550ae using cursor.execute statements so the schema isn't left under-indexed, ensuring the index definitions (columns/expressions and opclass) match the original definitions exactly (e.g., same operator class for text/array ops) when you add the CREATE INDEX IF NOT EXISTS statements in the migration's reverse logic.
16-18:⚠️ Potential issue | 🟠 MajorAvoid dual ownership of GIN indexes.
These indexes are created via raw SQL here. If
Metric.Meta.indexesalso declaresGinIndexwith the same names, Django migrations and the manual SQL will conflict. Pick one source of truth.Verification script (expect only one definition source):
#!/bin/bash # Find any ORM-managed GinIndex or explicit index names. rg -n --type=py 'GinIndex|main_tags_gin_idx|extra_tags_gin_idx' -C3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.py` around lines 16 - 18, The migration currently creates GIN indexes "main_tags_gin_idx" and "extra_tags_gin_idx" via raw SQL which will conflict if the ORM also defines GinIndex entries for Metric (check Metric.Meta.indexes); pick a single source of truth: either remove these raw SQL statements from 0013_migrate_to_jsonfield.py and let Django manage the GinIndex declarations on the Metric model, or remove the GinIndex entries from Metric.Meta.indexes and keep the raw SQL (and, if keeping raw SQL, ensure the migration documents/creates them via RunSQL with reversible SQL). Locate the raw SQL in 0013_migrate_to_jsonfield.py (the cursor.execute call) and the GinIndex definitions in the Metric model's Meta.indexes to make the change.requirements-test.txt (1)
8-8:django-filter<25.0still lacks a lower bound and an explanatory comment.This was raised in a previous review — the constraint still has no minimum version and no inline documentation of why
>=25.0is excluded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@requirements-test.txt` at line 8, The requirement line "django-filter<25.0" lacks a minimum version and an explanatory comment; update that token in requirements-test.txt to include an explicit lower bound (e.g., change to "django-filter>=<min_version>,<25.0>") and add an inline comment explaining why versions >=25.0 are excluded (compatibility/breaking change reference or ticket/PR number) so future reviewers know the rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 95-96: The "Start InfluxDB" step currently has no id so later
steps can't see its outcome; add an id (e.g., id: start_influxdb) to the "Start
InfluxDB" step and then change the "Wait for InfluxDB" step condition to also
require that steps.start_influxdb.conclusion == 'success' in addition to the
existing !cancelled() and steps.deps.conclusion == 'success' checks so the wait
loop is skipped if starting InfluxDB failed.
In `@requirements-test.txt`:
- Line 6: Remove the unused test dependency "commitizen" from
requirements-test.txt: delete the line containing commitizen~=4.13.0 (symbol:
commitizen) to avoid inflating test environment installs; if you believe it is
actually required, replace the line instead with the same entry plus an inline
comment explaining the hidden dependency and where it's used so reviewers/CI can
understand why it must remain.
- Line 7: Add an inline comment next to the prompt-toolkit!=3.0.52 entry
explaining that prompt-toolkit 3.0.52 introduced internal layout changes that
broke questionary (which commitizen depends on), reference questionary issue
`#454`, and note that the break was fixed in questionary 2.1.1 but the project
uses an older questionary via commitizen so the exclusion is required to avoid
the regression; keep the comment succinct and place it on the same line or
immediately above the prompt-toolkit!=3.0.52 requirement in
requirements-test.txt.
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 98-105: The loop that waits for InfluxDB currently runs curl then
sleeps, which causes an unnecessary final sleep after the 20th failed attempt;
modify the for loop in the CI workflow (the block starting with for i in
{1..20}; do and referencing the curl check of http://localhost:8086/ping and the
sleep 3) to move the sleep 3 to the top of the loop (before the curl check) so
the script sleeps between attempts but does not perform the trailing 3-second
pause after the final failed iteration.
- Line 99: The current health-check line using the pipeline "curl -sI
http://localhost:8086/ping | grep \"204\"" is fragile because it can match any
header containing "204"; update the conditional that invokes curl piped to grep
to use an anchored HTTP-status match instead (e.g., change the grep invocation
to use a pattern like ^HTTP/.* 204 or use curl's numeric status output) so only
a true HTTP 204 response succeeds; locate the line containing the curl -sI ... |
grep invocation and replace the grep pattern with the anchored match (or switch
to curl -s -o /dev/null -w '%{http_code}' and compare to 204) to make the check
robust.
In `@openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.py`:
- Around line 12-15: The ALTER TABLE statements in cursor.execute cast main_tags
and extra_tags directly to jsonb which will fail on empty strings; modify the
SQL in the cursor.execute call so each cast guards empty-string values (e.g.,
use NULLIF(main_tags, '')::jsonb or a CASE expression to convert '' to NULL
before casting) for both main_tags and extra_tags to avoid migration crashes on
blank values.
- Around line 26-30: The rollback currently doesn't recreate the two B-tree
indexes dropped in the forward migration; update the reverse migration to
recreate monitoring_metric_main_tags_2d8550ae_like and
monitoring_metric_main_tags_2d8550ae using cursor.execute statements so the
schema isn't left under-indexed, ensuring the index definitions
(columns/expressions and opclass) match the original definitions exactly (e.g.,
same operator class for text/array ops) when you add the CREATE INDEX IF NOT
EXISTS statements in the migration's reverse logic.
- Around line 16-18: The migration currently creates GIN indexes
"main_tags_gin_idx" and "extra_tags_gin_idx" via raw SQL which will conflict if
the ORM also defines GinIndex entries for Metric (check Metric.Meta.indexes);
pick a single source of truth: either remove these raw SQL statements from
0013_migrate_to_jsonfield.py and let Django manage the GinIndex declarations on
the Metric model, or remove the GinIndex entries from Metric.Meta.indexes and
keep the raw SQL (and, if keeping raw SQL, ensure the migration
documents/creates them via RunSQL with reversible SQL). Locate the raw SQL in
0013_migrate_to_jsonfield.py (the cursor.execute call) and the GinIndex
definitions in the Metric model's Meta.indexes to make the change.
In `@requirements-test.txt`:
- Line 8: The requirement line "django-filter<25.0" lacks a minimum version and
an explanatory comment; update that token in requirements-test.txt to include an
explicit lower bound (e.g., change to "django-filter>=<min_version>,<25.0>") and
add an inline comment explaining why versions >=25.0 are excluded
(compatibility/breaking change reference or ticket/PR number) so future
reviewers know the rationale.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.ymlopenwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.pyrequirements-test.txt
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 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/monitoring/migrations/0013_migrate_to_jsonfield.py
🔇 Additional comments (1)
openwisp_monitoring/monitoring/migrations/0013_migrate_to_jsonfield.py (1)
33-59: LGTM: state vs DB operations are cleanly separated.SeparateDatabaseAndState with RunPython keeps ORM state aligned while controlling DB-side conversion explicitly.
| - name: Wait for InfluxDB | ||
| if: ${{ !cancelled() && steps.deps.conclusion == 'success' }} |
There was a problem hiding this comment.
The wait step should also guard on "Start InfluxDB" succeeding.
The "Start InfluxDB" step (lines 91–93) has no id, so its outcome is invisible to subsequent steps. If docker compose up -d influxdb fails while deps succeeded, the wait step's condition (!cancelled() && steps.deps.conclusion == 'success') still evaluates to true, causing the loop to spin for the full 60 seconds before timing out — wasting CI time across all matrix combinations.
🔧 Proposed fix
Add an id to the "Start InfluxDB" step (existing context, not shown in diff above):
- name: Start InfluxDB
+ id: start_influxdb
if: ${{ !cancelled() && steps.deps.conclusion == 'success' }}
run: docker compose up -d influxdbThen tighten the wait step's condition:
- if: ${{ !cancelled() && steps.deps.conclusion == 'success' }}
+ if: ${{ !cancelled() && steps.deps.conclusion == 'success' && steps.start_influxdb.conclusion == 'success' }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 95 - 96, The "Start InfluxDB" step
currently has no id so later steps can't see its outcome; add an id (e.g., id:
start_influxdb) to the "Start InfluxDB" step and then change the "Wait for
InfluxDB" step condition to also require that steps.start_influxdb.conclusion ==
'success' in addition to the existing !cancelled() and steps.deps.conclusion ==
'success' checks so the wait loop is skipped if starting InfluxDB failed.
| mock-ssh-server~=0.9.1 | ||
| channels_redis~=4.3.0 | ||
| freezegun~=1.5.5 | ||
| commitizen~=4.13.0 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all requirements files to understand the structure
fd -type f -name 'requirements*.txt' -o -name 'requirements*.txt' | head -20
fd -type f -name '*.txt' | grep -E 'req|depend|setup'Repository: openwisp/openwisp-monitoring
Length of output: 413
🏁 Script executed:
#!/bin/bash
# Check what's in the current requirements-test.txt file
cat -n requirements-test.txtRepository: openwisp/openwisp-monitoring
Length of output: 373
🏁 Script executed:
#!/bin/bash
# Search for commitizen references across the codebase to understand its usage
rg 'commitizen' -i --type txt --type py --type md -B 2 -A 2Repository: openwisp/openwisp-monitoring
Length of output: 281
🏁 Script executed:
#!/bin/bash
# Check if there's any CI/CD configuration that uses commitizen
fd -type f \( -name '.github' -o -name '.gitlab-ci.yml' -o -name 'tox.ini' -o -name 'setup.cfg' -o -name 'pyproject.toml' \)
rg 'commitizen' -i --type yaml --type ini --type tomlRepository: openwisp/openwisp-monitoring
Length of output: 270
🌐 Web query:
commitizen 4.13.8 latest version pypi release date 2026
💡 Result:
Commitizen’s latest version on PyPI is 4.13.8, and its PyPI release date is February 18, 2026. [1]
Sources
[1] https://pypi.org/project/commitizen/
🏁 Script executed:
#!/bin/bash
# Find all requirements files more carefully
find . -type f -name 'requirements*.txt' 2>/dev/null
find . -type f \( -name '*.txt' -o -name '*.cfg' -o -name '*.toml' -o -name '*.yml' -o -name '*.yaml' \) 2>/dev/null | head -30Repository: openwisp/openwisp-monitoring
Length of output: 349
🏁 Script executed:
#!/bin/bash
# Check for CI/CD configuration files
find . -type f \( -name '.github' -o -path '*/.github/*' -o -name 'tox.ini' -o -name 'setup.cfg' -o -name 'pyproject.toml' -o -name '.gitlab-ci.yml' -o -name '.travis.yml' \) 2>/dev/null | head -20Repository: openwisp/openwisp-monitoring
Length of output: 397
🏁 Script executed:
#!/bin/bash
# Search for commitizen in CI/CD and config files using rg
rg 'commitizen' --max-count=20 2>/dev/nullRepository: openwisp/openwisp-monitoring
Length of output: 112
🏁 Script executed:
#!/bin/bash
# Check setup.py or pyproject.toml for package definition
find . -maxdepth 2 -type f \( -name 'setup.py' -o -name 'pyproject.toml' \) -exec cat {} \; 2>/dev/null | head -100Repository: openwisp/openwisp-monitoring
Length of output: 2320
🏁 Script executed:
#!/bin/bash
# Look at git history for this requirements file
git log --oneline requirements-test.txt 2>/dev/null | head -10
git show HEAD:requirements-test.txt 2>/dev/nullRepository: openwisp/openwisp-monitoring
Length of output: 394
🏁 Script executed:
#!/bin/bash
# Check requirements.txt (production)
cat requirements.txtRepository: openwisp/openwisp-monitoring
Length of output: 244
🏁 Script executed:
#!/bin/bash
# Check the CI workflow to see how tests are run and if commitizen is used
cat .github/workflows/ci.ymlRepository: openwisp/openwisp-monitoring
Length of output: 4891
🏁 Script executed:
#!/bin/bash
# Check setup.cfg for test configuration
cat setup.cfgRepository: openwisp/openwisp-monitoring
Length of output: 362
🌐 Web query:
commitizen tool purpose what does it do
💡 Result:
Commitizen is a CLI tool that helps teams standardize Git commit messages (by default using the Conventional Commits format) by prompting you interactively when you commit, so messages are consistent and machine-parseable. [1]
Because commits follow a predictable structure, Commitizen can then automate parts of the release workflow—notably bumping semantic versions from commit history and generating/updating changelogs (e.g., via cz bump). [1]
🏁 Script executed:
#!/bin/bash
# Find and examine the run-qa-checks script
find . -name 'run-qa-checks' -o -name 'run-qa-checks.sh' 2>/dev/null
ls -la run-qa-checks 2>/dev/null
cat run-qa-checksRepository: openwisp/openwisp-monitoring
Length of output: 1020
🏁 Script executed:
#!/bin/bash
# Check if commitizen is referenced in any Python or bash scripts
rg 'commitizen|cz ' --max-count=10 2>/dev/nullRepository: openwisp/openwisp-monitoring
Length of output: 112
Remove commitizen from requirements-test.txt—it is unused in tests, QA checks, and CI workflows.
commitizen is a commit-standardization and release-management tool (for enforcing conventional commits and automating version bumps), not a test dependency. It does not appear in the QA check script, CI workflows, or any test execution path. Keeping it in test requirements unnecessarily inflates every test environment install without providing value. Either remove it or document its necessity with an inline comment if there's a hidden dependency we've missed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@requirements-test.txt` at line 6, Remove the unused test dependency
"commitizen" from requirements-test.txt: delete the line containing
commitizen~=4.13.0 (symbol: commitizen) to avoid inflating test environment
installs; if you believe it is actually required, replace the line instead with
the same entry plus an inline comment explaining the hidden dependency and where
it's used so reviewers/CI can understand why it must remain.
| channels_redis~=4.3.0 | ||
| freezegun~=1.5.5 | ||
| commitizen~=4.13.0 | ||
| prompt-toolkit!=3.0.52 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "requirements-test.txt" -type f | head -5Repository: openwisp/openwisp-monitoring
Length of output: 95
🌐 Web query:
prompt-toolkit 3.0.52 questionary commitizen crash issue
💡 Result:
prompt-toolkit==3.0.52 (released Aug 27, 2025) introduced an internal layout/container structure change that broke questionary (and therefore commitizen, which depends on questionary). The typical crash is:
AttributeError: 'VSplit' object has no attribute 'content'coming fromquestionary/prompts/common.pywhile traversing the prompt-toolkit layout tree. [1]
Upstream status / fixes
- The breakage was reported as questionary issue
#454(“prompt_toolkit==3.0.52 break questionary”). [1] - It was fixed by questionary PR
#456, switching toPromptSession.layout.find_all_windows()instead of relying on a fixed.layout.containerpath. [2] - A
questionaryrelease 2.1.1 (Aug 28, 2025) exists (i.e., after the fix). [3]
What to do
- Prefer: upgrade
questionaryto >= 2.1.1 (so it’s compatible with prompt-toolkit 3.0.52). [2][3] - Workaround: pin
prompt_toolkitaway from 3.0.52 (e.g., 3.0.51).commitizenitself addedprompt_toolkit!=3.0.52with an explicit note pointing to questionary#454. [4]
Sources
- questionary issue
#454(crash + traceback) [1] - questionary PR
#456(fix) [2] - PyPI: questionary release history (shows 2.1.1 on Aug 28, 2025) [3]
- commitizen
pyproject.toml(excludesprompt_toolkit!=3.0.52due to questionary#454) [4]
🏁 Script executed:
cat -n requirements-test.txtRepository: openwisp/openwisp-monitoring
Length of output: 373
🏁 Script executed:
# Check git history to understand the context better
git log --oneline requirements-test.txt | head -10Repository: openwisp/openwisp-monitoring
Length of output: 148
🏁 Script executed:
rg "questionary" --type txt --type py . | head -20Repository: openwisp/openwisp-monitoring
Length of output: 54
🏁 Script executed:
# Check if there are other requirements files that might pin questionary
fd "requirements" -type f | xargs grep -l "questionary"Repository: openwisp/openwisp-monitoring
Length of output: 244
🏁 Script executed:
fd "requirements" -type f | xargs catRepository: openwisp/openwisp-monitoring
Length of output: 242
🏁 Script executed:
# Also check pyproject.toml or setup.py for dependencies
git ls-files | grep -E "(setup\.py|pyproject\.toml|setup\.cfg)"Repository: openwisp/openwisp-monitoring
Length of output: 105
🏁 Script executed:
cat setup.pyRepository: openwisp/openwisp-monitoring
Length of output: 1798
🏁 Script executed:
cat pyproject.tomlRepository: openwisp/openwisp-monitoring
Length of output: 593
🏁 Script executed:
cat requirements.txtRepository: openwisp/openwisp-monitoring
Length of output: 244
🏁 Script executed:
# Check git blame to see when prompt-toolkit!=3.0.52 was added
git log -p --all -- requirements-test.txt | grep -A 5 -B 5 "prompt-toolkit" | head -30Repository: openwisp/openwisp-monitoring
Length of output: 54
Add inline comment documenting the questionary compatibility issue.
prompt-toolkit!=3.0.52 excludes a version that broke questionary (and therefore commitizen, which depends on questionary). Add an inline comment explaining the root cause so future maintainers understand why this exclusion exists.
The breakage stems from prompt-toolkit 3.0.52's internal layout changes, which questionary relied on. This was fixed in questionary 2.1.1, but the project uses commitizen with an older questionary version. See questionary issue #454.
🔧 Suggested fix
-prompt-toolkit!=3.0.52
+prompt-toolkit!=3.0.52 # Breaks questionary (see questionary#454); fixed in questionary 2.1.1+📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| prompt-toolkit!=3.0.52 | |
| prompt-toolkit!=3.0.52 # Breaks questionary (see questionary#454); fixed in questionary 2.1.1+ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@requirements-test.txt` at line 7, Add an inline comment next to the
prompt-toolkit!=3.0.52 entry explaining that prompt-toolkit 3.0.52 introduced
internal layout changes that broke questionary (which commitizen depends on),
reference questionary issue `#454`, and note that the break was fixed in
questionary 2.1.1 but the project uses an older questionary via commitizen so
the exclusion is required to avoid the regression; keep the comment succinct and
place it on the same line or immediately above the prompt-toolkit!=3.0.52
requirement in requirements-test.txt.
25e94eb to
1fa05e4
Compare
…p#673 Optimized PostgreSQL with jsonb and GIN indexes. Synchronized migration state with models and verified locally. Closes openwisp#673
HMTAhir327
force-pushed
the
change/replace-jsonfield-with-builtin
branch
from
1fa05e4 to
95e4653
Compare
Reverted the previous migration adjustments while investigating CI failures.
HMTAhir327
force-pushed
the
change/replace-jsonfield-with-builtin
branch
from
22a8daf to
e6370fa
Compare
…nd optimize indexing openwisp#673 Modernized JSON storage using native Django fields and PostgreSQL GIN indexes. Includes SeparateDatabaseAndState for safe schema migration, SpatiaLite compatibility guards, and enhanced CI health-checks. Closes openwisp#673
HMTAhir327
force-pushed
the
change/replace-jsonfield-with-builtin
branch
from
e6370fa to
912f18c
Compare
|
I have finalized the implementation by:
|
|
Please resolve the comments posted by code rabbit until it PR is approved. You can do this by:
Ive checked the changes and I didn't find anything that hasnt been flagged by code rabbit already |
2 participants
…nd optimize indexing
Checklist
Reference to Existing Issue
Closes ##673
Please open a new issue if there isn't an existing issue yet.
Description of Changes
This PR modernizes how we store and query telemetry data by moving from a deprecated third-party library to Django's native JSONField. This version ensures the database is actually optimized for production performance.
Key Improvements:
Faster Queries with GIN Indexes: Switched from standard B-tree indexes to GIN indexes for main_tags and extra_tags. This allows PostgreSQL to search inside JSON data significantly faster, which is essential for scaling telemetry.
Reliable Database Migration: Added a specific migration (0013) that uses custom SQL to safely convert old text columns into jsonb. This prevents the deployment crashes that happen when trying to change data types while old indexes are still attached.
Cleaner Codebase: Removed all remaining imports of the old jsonfield package from historical migrations to ensure the project doesn't break once the dependency is fully deleted.
Screenshot
Please include any relevant screenshots.