Fixed SubnetDivisionIndex foreign keys saved as NULL#1436
Fixed SubnetDivisionIndex foreign keys saved as NULL#1436ApurveKaranwal wants to merge 1 commit into
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:
📝 WalkthroughWalkthroughSubnet division provisioning now assigns Estimated code review effort: 2 (Simple) | ~10 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (6 passed)
✨ 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 |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The PR fixes issue #1435 (
The change includes a regression test (per AGENTS.md) and aligns with the referenced issue. Prior CodeRabbit nitpicks (line length, Files Reviewed (3 files)
Note: the incremental diff since the previously reviewed commit was polluted by an intervening rebase onto newer master; review was scoped to this PR's actual 3 changed files versus its merge-base. Previous Review Summaries (6 snapshots, latest commit 7e5e442)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 7e5e442)Status: No Issues Found | Recommendation: Merge The only change since the previous review is comment line-wrapping in Files Reviewed (1 file changed since last review)
Previous review (commit 13241c3)Status: No Issues Found | Recommendation: Merge The incremental change since the previous review is a single line in Files Reviewed (1 file changed since last review)
Previous review (commit a9d885c)Status: No Issues Found | Recommendation: Merge The incremental changes since the previous review are test-only refinements in
The underlying fix (passing model instances Files Reviewed (1 file changed since last review)
Previous review (commit d1282d7)Status: No Issues Found | Recommendation: Merge The change fixes the
A regression test ( No crash or security concerns on the changed lines. Remaining open comments are pre-existing style/lint nitpicks (E501/W293) already tracked by CodeRabbit and CI; they are out of scope for this review. Files Reviewed (3 files)
Previous review (commit f4d90fc)Status: No Issues Found | Recommendation: Merge The incremental change resolves the advisory from the previous review. At No crash or security concerns on the changed line. Files Reviewed (1 file in incremental diff)
Previous review (commit 389f144)Status: No Critical Issues Found | Recommendation: Merge Nice, focused fix. Passing the model instances ( Advisory (not blocking, outside changed lines)The same copy-before- Files Reviewed (3 files)
Reviewed by claude-opus-4.8 · Input: 26 · Output: 5.1K · Cached: 570.9K |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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_controller/subnet_division/tests/test_models.py`:
- Around line 1013-1036: Clean up test_subnet_division_index_integrity in the
regression test by removing the trailing whitespace on the blank line and
wrapping the long assertIsNotNone statements, including their f-string messages,
so every line stays within the repository’s 88-character limit while preserving
the assertions.
🪄 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: b0d5d1eb-4f6e-4be6-91ff-a306f9e2856a
📒 Files selected for processing (3)
openwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.py
📜 Review details
⏰ Context from checks skipped due to timeout. (13)
- GitHub Check: CodeRabbit
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | 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~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/subnet_division/tasks.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
openwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/subnet_division/tasks.py
🧠 Learnings (8)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/subnet_division/tasks.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/subnet_division/tasks.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/subnet_division/tasks.py
📚 Learning: 2026-06-07T12:07:08.468Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.py
📚 Learning: 2026-06-07T12:07:24.608Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.py
📚 Learning: 2026-06-25T12:20:18.414Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/base/models.py:571-572
Timestamp: 2026-06-25T12:20:18.414Z
Learning: When writing or reviewing tests that override pagination behavior via OpenWispPagination.paginate_queryset(), patch `view.pagination_page_size` (not `page_size`). The method uses `getattr(view, "pagination_page_size", self.page_size)`, so tests must set the attribute on the view to affect pagination. If the view class does not define `pagination_page_size`, using `unittest.mock.patch(..., create=True)` is intentional and correct because the attribute may not exist until patched.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.py
📚 Learning: 2026-06-07T12:07:25.164Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.py
📚 Learning: 2026-06-25T12:20:45.387Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/tests/test_api.py:916-932
Timestamp: 2026-06-25T12:20:45.387Z
Learning: When reviewing API pagination behavior in openwisp-controller, assume `OpenWispPagination.paginate_queryset()` allows a per-view page-size override via `getattr(view, "pagination_page_size", self.page_size)` (so `view.pagination_page_size`, if present, should affect pagination). In Python tests, it is valid to patch `pagination_page_size` on a view class even if the attribute isn’t declared on the class by default, by using `unittest.mock.patch.object(..., "pagination_page_size", ..., create=True)` so the override is available for the pagination logic during the test.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.py
🔇 Additional comments (3)
openwisp_controller/subnet_division/rule_types/base.py (2)
256-263: LGTM!
268-304: 🗄️ Data Integrity & IntegrationNo NULL-FK issue here
create_ips()usesip=ip_obj, and the IP rows are bulk-created beforeSubnetDivisionIndex.objects.bulk_create(), so this row doesn’t leaveip_idunset.> Likely an incorrect or invalid review comment.openwisp_controller/subnet_division/tasks.py (1)
99-116: LGTM! Theip=iprelation assignment at Line 112 correctly lets Django resolveip_idfrom the cachedipinstance onceIpAddress.objects.bulk_create(ips)runs (beforeSubnetDivisionIndex.objects.bulk_create(indexes)), matching the required save ordering for this FK-resolution mechanism.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_controller/subnet_division/rule_types/base.py`:
- Line 297: In create_ips, replace the subnet_id=subnet_obj.id assignment with
the subnet=subnet_obj object assignment, matching the existing FK assignment
style used by create_subnets while preserving the created relationship.
In `@openwisp_controller/subnet_division/tests/test_models.py`:
- Around line 1013-1036: Strengthen test_subnet_division_index_integrity by
recording the initial index count, asserting the post-update count increased by
the two added IPs, and replacing both `"ip" in index.keyword` checks with the
`"_ip"` pattern. Remove the trailing whitespace from the blank line after adding
the template.
🪄 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: ae976e39-dfa9-4c8d-b8ac-b41b77e415da
📒 Files selected for processing (3)
openwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.py
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
This pull request targets an issue that has not been validated by maintainers and seem to violate the OpenWISP Anti Spam Policy.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openwisp_controller/subnet_division/tests/test_models.py (1)
1013-1047: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLint failure and lingering test-robustness gaps from prior review rounds.
- Line 1017's comment is 91 characters, exceeding the project's 88-char flake8 limit (
E501), which will fail CI. Based on learnings, this repo enforcesE501via flake8'ssetup.cfg(max-line-length = 88), applicable to test files too.- Blank lines at 1016, 1029, 1034 split the method body; as per path instructions, avoid unnecessary blank lines inside function/method bodies.
"ip" in index.keywordis fragile — a rule label containing "ip" (e.g. "IPSEC") would make subnet-only keywords match too. Use"_ip" in index.keywordto match the actual keyword pattern.- No assertion that the index count actually increased after
number_of_ipsupdate, so the test would still pass even ifprovision_extra_ipscreated zero new indexes.🔧 Suggested fix
def test_subnet_division_index_integrity(self): rule = self._get_vpn_subdivision_rule() self.config.templates.add(self.template) - - # Verify initial indexes have non-NULL subnet_id and ip_id (if they are IP indexes) + # Verify initial indexes have non-NULL subnet_id and ip_id + # (if they are IP indexes) indexes = rule.subnetdivisionindex_set.all() self.assertTrue(indexes.exists()) + initial_index_count = indexes.count() for index in indexes: self.assertIsNotNone( index.subnet_id, f"SubnetDivisionIndex {index.keyword} has NULL subnet_id", ) - if "ip" in index.keyword: + if "_ip" in index.keyword: self.assertIsNotNone( index.ip_id, f"SubnetDivisionIndex {index.keyword} has NULL ip_id" ) - # Update number of IPs to trigger tasks.provision_extra_ips rule.number_of_ips = rule.number_of_ips + 2 rule.save() rule.refresh_from_db() - # Verify all indexes, including new ones, have non-NULL subnet_id and ip_id indexes = rule.subnetdivisionindex_set.all() + self.assertGreater(indexes.count(), initial_index_count) for index in indexes: self.assertIsNotNone( index.subnet_id, f"SubnetDivisionIndex {index.keyword} has NULL subnet_id after update", ) - if "ip" in index.keyword: + if "_ip" in index.keyword: self.assertIsNotNone( index.ip_id, f"SubnetDivisionIndex {index.keyword} has NULL ip_id after update", )🤖 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_controller/subnet_division/tests/test_models.py` around lines 1013 - 1047, Update test_subnet_division_index_integrity to keep comments within the 88-character limit and remove unnecessary blank lines inside the method. Replace each fragile "ip" keyword check with "_ip" matching, and record the initial index count before updating number_of_ips, then assert the post-update count increased before validating the new indexes.Sources: Path instructions, Learnings
🤖 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.
Duplicate comments:
In `@openwisp_controller/subnet_division/tests/test_models.py`:
- Around line 1013-1047: Update test_subnet_division_index_integrity to keep
comments within the 88-character limit and remove unnecessary blank lines inside
the method. Replace each fragile "ip" keyword check with "_ip" matching, and
record the initial index count before updating number_of_ips, then assert the
post-update count increased before validating the new indexes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 85380b9e-f3e7-4269-8e9e-9f565a1567dc
📒 Files selected for processing (3)
openwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.py
📜 Review details
⏰ Context from checks skipped due to timeout. (13)
- GitHub Check: CodeRabbit
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/subnet_division/rule_types/base.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
openwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/subnet_division/rule_types/base.py
🧠 Learnings (8)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/subnet_division/rule_types/base.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/subnet_division/rule_types/base.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/subnet_division/rule_types/base.py
📚 Learning: 2026-06-07T12:07:08.468Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.py
📚 Learning: 2026-06-07T12:07:24.608Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.py
📚 Learning: 2026-06-25T12:20:18.414Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/base/models.py:571-572
Timestamp: 2026-06-25T12:20:18.414Z
Learning: When writing or reviewing tests that override pagination behavior via OpenWispPagination.paginate_queryset(), patch `view.pagination_page_size` (not `page_size`). The method uses `getattr(view, "pagination_page_size", self.page_size)`, so tests must set the attribute on the view to affect pagination. If the view class does not define `pagination_page_size`, using `unittest.mock.patch(..., create=True)` is intentional and correct because the attribute may not exist until patched.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.py
📚 Learning: 2026-06-07T12:07:25.164Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.py
📚 Learning: 2026-06-25T12:20:45.387Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/tests/test_api.py:916-932
Timestamp: 2026-06-25T12:20:45.387Z
Learning: When reviewing API pagination behavior in openwisp-controller, assume `OpenWispPagination.paginate_queryset()` allows a per-view page-size override via `getattr(view, "pagination_page_size", self.page_size)` (so `view.pagination_page_size`, if present, should affect pagination). In Python tests, it is valid to patch `pagination_page_size` on a view class even if the attribute isn’t declared on the class by default, by using `unittest.mock.patch.object(..., "pagination_page_size", ..., create=True)` so the override is available for the pagination logic during the test.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.py
🔇 Additional comments (3)
openwisp_controller/subnet_division/rule_types/base.py (2)
220-266: LGTM!
268-304: 🎯 Functional CorrectnessCorrect fix:
ip=ip_objdefers FK resolution until afterIpAddress.objects.bulk_create().Assigning the unsaved
ip_objinstance lets Django resolveip_idfrom the object's real pk onceIpAddress.objects.bulk_create(generated_ips)(line 303) runs, before the caller'sSubnetDivisionIndex.objects.bulk_create()executes. This relies on Django's parent-before-child pk resolution inbulk_create()(fixed via Django's_prepare_related_fields_for_save), which requires the parent bulk_create to complete first — which it does here.openwisp_controller/subnet_division/tasks.py (1)
108-116: LGTM!
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_controller/subnet_division/tests/test_models.py`:
- Line 1037: Wrap the long comment near the count and index assertions onto
multiple comment lines so each remains within the repository’s 88-character
flake8 limit, preserving its meaning.
🪄 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: 8ed67f79-4cb3-41a5-b81f-39eee21627d5
📒 Files selected for processing (3)
openwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.py
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: CodeRabbit
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/subnet_division/tasks.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
openwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/subnet_division/tasks.py
🧠 Learnings (8)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/subnet_division/tasks.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/subnet_division/tasks.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/subnet_division/tests/test_models.pyopenwisp_controller/subnet_division/tasks.py
📚 Learning: 2026-06-07T12:07:08.468Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.py
📚 Learning: 2026-06-07T12:07:24.608Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.py
📚 Learning: 2026-06-25T12:20:18.414Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/base/models.py:571-572
Timestamp: 2026-06-25T12:20:18.414Z
Learning: When writing or reviewing tests that override pagination behavior via OpenWispPagination.paginate_queryset(), patch `view.pagination_page_size` (not `page_size`). The method uses `getattr(view, "pagination_page_size", self.page_size)`, so tests must set the attribute on the view to affect pagination. If the view class does not define `pagination_page_size`, using `unittest.mock.patch(..., create=True)` is intentional and correct because the attribute may not exist until patched.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.py
📚 Learning: 2026-06-07T12:07:25.164Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.py
📚 Learning: 2026-06-25T12:20:45.387Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/tests/test_api.py:916-932
Timestamp: 2026-06-25T12:20:45.387Z
Learning: When reviewing API pagination behavior in openwisp-controller, assume `OpenWispPagination.paginate_queryset()` allows a per-view page-size override via `getattr(view, "pagination_page_size", self.page_size)` (so `view.pagination_page_size`, if present, should affect pagination). In Python tests, it is valid to patch `pagination_page_size` on a view class even if the attribute isn’t declared on the class by default, by using `unittest.mock.patch.object(..., "pagination_page_size", ..., create=True)` so the override is available for the pagination logic during the test.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.py
🔇 Additional comments (3)
openwisp_controller/subnet_division/rule_types/base.py (1)
256-262: LGTM!Also applies to: 294-302
openwisp_controller/subnet_division/tasks.py (1)
108-116: LGTM!openwisp_controller/subnet_division/tests/test_models.py (1)
1013-1036: LGTM!Also applies to: 1038-1052
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openwisp_controller/subnet_division/tests/test_models.py (1)
1037-1037: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLine 1037 exceeds the 88-character flake8 limit (91 chars).
This was flagged in a previous review but remains unaddressed. Based on learnings, this repo enforces E501 via flake8's
setup.cfg(max-line-length = 88) on all Python files including tests.🔧 Proposed fix: wrap the comment
- # Verify count increased and all indexes, including new ones, have non-NULL subnet_id and ip_id + # Verify count increased and all indexes, including new ones, + # have non-NULL subnet_id and ip_id🤖 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_controller/subnet_division/tests/test_models.py` at line 1037, Wrap the long comment in the test around the verification of subnet_id and ip_id so each line is no longer than 88 characters, preserving its meaning.Source: Learnings
🤖 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.
Duplicate comments:
In `@openwisp_controller/subnet_division/tests/test_models.py`:
- Line 1037: Wrap the long comment in the test around the verification of
subnet_id and ip_id so each line is no longer than 88 characters, preserving its
meaning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c7515a87-2572-4e68-8fe8-6786c297f960
📒 Files selected for processing (3)
openwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/tests/test_models.py
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: CodeRabbit
- 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~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/subnet_division/tests/test_models.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
openwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/subnet_division/tests/test_models.py
🧠 Learnings (8)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/subnet_division/tests/test_models.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/subnet_division/tests/test_models.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/subnet_division/tasks.pyopenwisp_controller/subnet_division/rule_types/base.pyopenwisp_controller/subnet_division/tests/test_models.py
📚 Learning: 2026-06-07T12:07:08.468Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.py
📚 Learning: 2026-06-07T12:07:24.608Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.py
📚 Learning: 2026-06-25T12:20:18.414Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/base/models.py:571-572
Timestamp: 2026-06-25T12:20:18.414Z
Learning: When writing or reviewing tests that override pagination behavior via OpenWispPagination.paginate_queryset(), patch `view.pagination_page_size` (not `page_size`). The method uses `getattr(view, "pagination_page_size", self.page_size)`, so tests must set the attribute on the view to affect pagination. If the view class does not define `pagination_page_size`, using `unittest.mock.patch(..., create=True)` is intentional and correct because the attribute may not exist until patched.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.py
📚 Learning: 2026-06-07T12:07:25.164Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.py
📚 Learning: 2026-06-25T12:20:45.387Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/tests/test_api.py:916-932
Timestamp: 2026-06-25T12:20:45.387Z
Learning: When reviewing API pagination behavior in openwisp-controller, assume `OpenWispPagination.paginate_queryset()` allows a per-view page-size override via `getattr(view, "pagination_page_size", self.page_size)` (so `view.pagination_page_size`, if present, should affect pagination). In Python tests, it is valid to patch `pagination_page_size` on a view class even if the attribute isn’t declared on the class by default, by using `unittest.mock.patch.object(..., "pagination_page_size", ..., create=True)` so the override is available for the pagination logic during the test.
Applied to files:
openwisp_controller/subnet_division/tests/test_models.py
🔇 Additional comments (3)
openwisp_controller/subnet_division/rule_types/base.py (1)
256-263: LGTM!Also applies to: 294-302
openwisp_controller/subnet_division/tasks.py (1)
108-116: LGTM!openwisp_controller/subnet_division/tests/test_models.py (1)
1013-1036: LGTM!Also applies to: 1038-1052
|
Hi @nemesifier sir, I sincerely apologize for opening this PR before having the issue validated first. I have read the OpenWISP Contributing Guidelines and the Anti-Spam Policy, and I realize that skipping the triage process adds unnecessary overhead to your workflow. I got eager to submit the fix and rushed, but I will make sure to strictly follow the guidelines by waiting for maintainer validation in the future. Thank you for your patience and for routing this for review! |
Ensured that the subnet and ip foreign keys are populated correctly on SubnetDivisionIndex records during initial provisioning and when updating the number of IPs. Closes #1435
|
@ApurveKaranwal how did you find this issue? Are you using OpenWISP or are you hunting for bugs using AI tools? Please clarify. There's nothing wrong in using AI tools to hunt for bugs but you should not pretend it's not the case. |
|
Hey @nemesifier sir, Just to give some context: I was digging into the subnet division code and running tests locally to see how the admin index locks work. I noticed the automated indexes weren't actually locking the subnets/IPs in the admin, and when I checked the database, the Since tracing Django's bulk save behavior was a bit tricky, I used AI tools to help diagnose the ORM behavior (specifically why the So it was a mix of looking at the codebase, inspecting the database, and using AI to help speed up the debugging process. I definitely wasn't trying to hide anything, and I really appreciate your patience. I just wanted to locate a genuine bug and contribute a solid fix to the project! |
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
@ApurveKaranwal please see #1435 (comment)
You can re-open this PR once the bug is validated.
| SubnetDivisionIndex( | ||
| keyword=f"{division_rule.label}_subnet{subnet_id}", | ||
| subnet_id=subnet_obj.id, | ||
| subnet=subnet_obj, |
| subnet=subnet_obj, | ||
| ip=ip_obj, |
There was a problem hiding this comment.
Same here, how is this different?
| e.message_dict.get("master_subnet", []), | ||
| ) | ||
|
|
||
| def test_subnet_division_index_integrity(self): |
There was a problem hiding this comment.
Did you check that this test fails on the current master?
If yes, can you point out in the [issue comments[(https://github.com//issues/1435#issuecomment-4957428251) why the existing test cases are unable to detect the bug you reported?
3 participants
Checklist
Reference to Existing Issue
Closes #1435
Description of Changes
This PR fixes a bug where
SubnetDivisionIndexrecords were created withsubnet_id = NULLand/orip_id = NULLin the database.Why the bug occurred:
The code was instantiating
SubnetDivisionIndexinstances by copying the.idproperties directly (e.g.,subnet_id=subnet_obj.idandip_id=ip.id) before the subnets or IP addresses were bulk-saved. At the time of copy, these values wereNone. Althoughbulk_createsubsequently populated the primary keys on the model instances, the primitive values copied earlier remainedNoneand were saved asNULLin the database.The Fix:
We now pass the model instances themselves (
subnet=subnet_objandip=ip) instead of copying the.idproperties. Since the index instances hold object references, Django correctly resolves their primary keys once the parent objects are bulk-saved in the database.Test Coverage:
Added a new unit test method
test_subnet_division_index_integrity()inopenwisp_controller/subnet_division/tests/test_models.py. It asserts that:SubnetDivisionIndexhas a non-NULLsubnet_idand correctip_id(for IP indexes).number_of_ips(which triggers celery tasks) also have correct non-NULL foreign key fields.Screenshot
Logical proof of the bug and fix (Python simulation):