[feature] Invalidate VPN cache when organization config variables change #1177
Conversation
Srinath0916
force-pushed
the
feature/vpn-cache-invalidation-1098
branch
10 times, most recently
from
fb709ba to
954e35c
Compare
Srinath0916
force-pushed
the
feature/vpn-cache-invalidation-1098
branch
3 times, most recently
from
497fd5a to
3342a52
Compare
…nge openwisp#1098 Implemented automatic VPN cache invalidation when OrganizationConfigSettings context changes to prevent stale VPN configurations and connectivity issues. Changes: - Added signal handler to detect OrganizationConfigSettings context changes - Added Celery task to invalidate VPN cache for all VPNs in organization - Connected post_save signal to trigger cache invalidation automatically - Uses string import to avoid circular import issues - Ensures VPNs always use latest configuration variables This resolves the issue where VPN cache was not automatically invalidated when organization configuration variables changed, causing inconsistency and connectivity problems. Fixes openwisp#1098
Srinath0916
force-pushed
the
feature/vpn-cache-invalidation-1098
branch
from
3342a52 to
7e365af
Compare
|
Hi @pandafy, I've been working on implementing the VPN cache invalidation for issue #1098 and spent most of the night trying to get it working, but I'm running into a really strange CI problem. I have the implementation ready - signal handler to detect OrganizationConfigSettings context changes, Celery task to invalidate VPN cache, and proper signal connections. The logic is straightforward and follows the same patterns as existing handlers in the codebase. But I keep hitting this geckodriver error (visible in the checks above) that seems completely unrelated to my changes. The weird thing is that it happens with ANY modification I make - even just adding TODO comments or copying the exact pattern from working handlers like I tried many different approaches throughout the night to get past this CI error. You can see all my attempts in the issue page - I know it looks messy with all those commits, but I was just trying everything I could think of. Every single approach fails with the same geckodriver error, even when the code changes are minimal. My other PR (#1173) passes all CI checks fine, so I don't think it's an account or general setup issue. This seems specific to this particular feature. Could you help me understand what might be causing this? Is there something about VPN-related code that triggers different test suites? Or maybe there's a specific approach you'd recommend for this type of cache invalidation? I really want to get this working and contribute to OpenWISP properly. The implementation is ready, just need to figure out this CI puzzle. Sorry for all the iteration - I was just determined to get past this error! |
WalkthroughAdds app config attribute Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant DjangoORM as Django ORM
participant OrgConfig as OrganizationConfigSettings
participant Txn as transaction.on_commit
participant Celery as Celery Queue
participant DB as Database
participant GetVpnView
participant VPN
Client->>DjangoORM: save OrganizationConfigSettings
DjangoORM->>OrgConfig: __init__ (snapshot _initial_context)
DjangoORM->>OrgConfig: save()
OrgConfig->>OrgConfig: compare context vs _initial_context
alt context changed AND organization is active
OrgConfig->>Txn: register on_commit callbacks
Txn->>Celery: enqueue invalidate_organization_vpn_cache(org_id) on commit
Celery->>DB: query VPNs by organization_id
loop each VPN
Celery->>GetVpnView: invalidate_get_vpn_cache(vpn)
Celery->>VPN: invalidate_checksum_cache()
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2026-01-15T15:05:49.557ZApplied to files:
📚 Learning: 2026-01-12T22:27:48.342ZApplied to files:
📚 Learning: 2026-01-15T15:07:17.354ZApplied to files:
🧬 Code graph analysis (2)openwisp_controller/config/base/multitenancy.py (3)
openwisp_controller/config/tests/test_handlers.py (3)
⏰ 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)
🔇 Additional comments (9)
✏️ Tip: You can disable this entire section by setting 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
openwisp_controller/config/tasks.py (1)
111-115: Move import outside the loop for efficiency.The import statement is executed on every iteration, which is unnecessary overhead.
🔎 Proposed fix
Vpn = load_model("config", "Vpn") + from .controller.views import GetVpnView for vpn in Vpn.objects.filter(organization_id=organization_id).iterator(): - from .controller.views import GetVpnView - GetVpnView.invalidate_get_vpn_cache(vpn) vpn.invalidate_checksum_cache()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
openwisp_controller/config/apps.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_handlers.py
💤 Files with no reviewable changes (1)
- openwisp_controller/config/tests/test_handlers.py
🧰 Additional context used
🧬 Code graph analysis (3)
openwisp_controller/config/apps.py (2)
openwisp_controller/config/handlers.py (1)
organization_config_settings_change_handler(157-171)openwisp_controller/config/base/vpn.py (1)
post_save(905-916)
openwisp_controller/config/handlers.py (2)
openwisp_controller/config/tasks.py (1)
invalidate_organization_vpn_cache(104-115)openwisp_controller/geo/base/models.py (1)
organization_id(64-65)
openwisp_controller/config/tasks.py (3)
openwisp_controller/geo/base/models.py (1)
organization_id(64-65)openwisp_controller/config/controller/views.py (2)
GetVpnView(485-516)invalidate_get_vpn_cache(508-516)openwisp_controller/config/base/base.py (2)
invalidate_checksum_cache(57-64)invalidate_checksum_cache(92-94)
🪛 GitHub Actions: OpenWISP Controller CI Build
openwisp_controller/config/apps.py
[error] 342-342: AttributeError: 'ConfigConfig' object has no attribute 'organization_config_settings_model' during Django app ready initialization.
openwisp_controller/config/handlers.py
[error] 168-168: Black check failed. E501 line too long (100 > 88 characters).
🪛 Ruff (0.14.10)
openwisp_controller/config/handlers.py
157-157: Unused function argument: kwargs
(ARG001)
🔇 Additional comments (3)
openwisp_controller/config/handlers.py (1)
157-171: LGTM on the handler logic.The implementation correctly:
- Skips newly-added instances via
_state.adding- Fetches only the
contextfield from the DB for comparison- Uses
transaction.on_committo ensure the task runs after the transaction commits- Handles
DoesNotExistgracefullyThe
**kwargsparameter is correctly included for signal handler forward compatibility (the Ruff warning is a false positive for signal handlers).openwisp_controller/config/tasks.py (1)
103-116: LGTM on the task implementation.The task correctly:
- Uses
soft_time_limit=7200consistent with other cache invalidation tasks- Filters VPNs by organization using
.iterator()for memory efficiency- Calls both
invalidate_get_vpn_cacheandinvalidate_checksum_cacheto fully refresh VPN cachesThis aligns with the pattern used in
invalidate_controller_views_cache(lines 197-215), with the additionalinvalidate_checksum_cache()call needed when organization config variables change.openwisp_controller/config/apps.py (1)
269-275: LGTM on the import addition.The handler import is correctly placed alongside the other cache invalidation handlers.
- Added missing model definition that was causing AttributeError - Fixed line length for black formatting - Actually implemented the VPN cache invalidation logic - Moved import out of loop Should fix the CI failures now.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openwisp_controller/config/tasks.py (1)
103-115: Consider adding.only("id")andSoftTimeLimitExceededhandling for consistency.The task works correctly but could be improved for consistency with similar tasks in this file:
- The existing
invalidate_controller_views_cache(line 212) uses.only("id").iterator()to reduce memory overhead.- Other tasks with
soft_time_limit=7200(e.g.,update_template_related_config_status) handleSoftTimeLimitExceededto log meaningful errors.🔎 Proposed improvements
@shared_task(soft_time_limit=7200) def invalidate_organization_vpn_cache(organization_id): """ Invalidates VPN cache for all VPNs in an organization when organization configuration variables change. """ Vpn = load_model("config", "Vpn") from .controller.views import GetVpnView - - for vpn in Vpn.objects.filter(organization_id=organization_id).iterator(): - GetVpnView.invalidate_get_vpn_cache(vpn) - vpn.invalidate_checksum_cache() + + try: + for vpn in ( + Vpn.objects.filter(organization_id=organization_id).only("id").iterator() + ): + GetVpnView.invalidate_get_vpn_cache(vpn) + vpn.invalidate_checksum_cache() + except SoftTimeLimitExceeded: + logger.error( + "soft time limit hit while executing " + f"invalidate_organization_vpn_cache for organization {organization_id}" + )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
openwisp_controller/config/apps.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tasks.py
🧰 Additional context used
🧬 Code graph analysis (3)
openwisp_controller/config/tasks.py (3)
openwisp_controller/geo/base/models.py (1)
organization_id(64-65)openwisp_controller/config/controller/views.py (2)
GetVpnView(485-516)invalidate_get_vpn_cache(508-516)openwisp_controller/config/base/base.py (2)
invalidate_checksum_cache(57-64)invalidate_checksum_cache(92-94)
openwisp_controller/config/apps.py (2)
openwisp_controller/config/handlers.py (1)
organization_config_settings_change_handler(157-174)openwisp_controller/config/base/vpn.py (1)
post_save(905-916)
openwisp_controller/config/handlers.py (2)
openwisp_controller/config/tasks.py (1)
invalidate_organization_vpn_cache(104-114)openwisp_controller/geo/base/models.py (1)
organization_id(64-65)
🪛 Ruff (0.14.10)
openwisp_controller/config/handlers.py
157-157: Unused function argument: kwargs
(ARG001)
⏰ 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.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | 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: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🔇 Additional comments (3)
openwisp_controller/config/apps.py (3)
64-66: LGTM!The model loading is correctly added in
__setmodels__, following the existing pattern.
276-277: LGTM!The handler import is properly added alongside related handlers.
343-347: Signal connection looks correct, but depends on handler fix.The signal connection follows the established pattern in this file. However, this will need to be updated if the handler is split into
pre_saveandpost_savehandlers as suggested in the handlers.py review.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
openwisp_controller/config/tasks.py (1)
118-122: Consider usinglogging.exceptionfor better debugging.When catching exceptions,
logging.exceptionautomatically includes the traceback, which provides more context for diagnosing soft time limit issues.🔎 Proposed improvement
except SoftTimeLimitExceeded: - logger.error( + logger.exception( "soft time limit hit while executing " f"invalidate_organization_vpn_cache for organization {organization_id}" )Based on static analysis hints.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
openwisp_controller/config/apps.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tasks.py
🧰 Additional context used
🧬 Code graph analysis (3)
openwisp_controller/config/handlers.py (2)
openwisp_controller/config/tasks.py (1)
invalidate_organization_vpn_cache(104-122)openwisp_controller/geo/base/models.py (1)
organization_id(64-65)
openwisp_controller/config/apps.py (2)
openwisp_controller/config/handlers.py (2)
organization_config_settings_pre_save_handler(157-167)organization_config_settings_post_save_handler(170-180)openwisp_controller/config/base/vpn.py (1)
post_save(905-916)
openwisp_controller/config/tasks.py (3)
openwisp_controller/geo/base/models.py (1)
organization_id(64-65)openwisp_controller/config/controller/views.py (2)
GetVpnView(485-516)invalidate_get_vpn_cache(508-516)openwisp_controller/config/base/base.py (2)
invalidate_checksum_cache(57-64)invalidate_checksum_cache(92-94)
🪛 Ruff (0.14.10)
openwisp_controller/config/handlers.py
157-157: Unused function argument: kwargs
(ARG001)
170-170: Unused function argument: kwargs
(ARG001)
openwisp_controller/config/tasks.py
119-122: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ 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.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
🔇 Additional comments (4)
openwisp_controller/config/tasks.py (1)
103-117: LGTM! Efficient implementation following codebase patterns.The task correctly:
- Uses
.only("id").iterator()for memory-efficient iteration- Calls both
GetVpnView.invalidate_get_vpn_cache()andvpn.invalidate_checksum_cache()to ensure complete cache invalidation- Follows the same patterns as other tasks in this file
openwisp_controller/config/handlers.py (2)
157-167: Excellent implementation of the pre-save pattern.The handler correctly:
- Returns early for new objects (no original context to compare)
- Uses
.only("context")to efficiently load just the needed field- Stores the original context on the instance for comparison in post-save
- Handles the edge case where the DB record doesn't exist
170-180: Correct post-save implementation with proper change detection.The handler correctly:
- Retrieves the original context stored by the pre-save handler
- Only triggers invalidation when context actually changed (not None and differs)
- Uses
transaction.on_committo ensure the task is only scheduled after a successful database commit- Converts
organization_idto string for Celery serializationThis addresses the previous critical issue where the comparison logic was inverted.
openwisp_controller/config/apps.py (1)
64-66: Proper model loading and signal wiring.The changes correctly:
- Load
OrganizationConfigSettingsmodel in__setmodels__()(lines 64-66), addressing the previousAttributeError- Import both handlers in the appropriate location (lines 276-278)
- Wire pre_save and post_save signals with descriptive
dispatch_uidvalues (lines 344-353)The signal connections follow the established patterns in this file and correctly link the handlers to the model.
Also applies to: 276-278, 344-353
Srinath0916
force-pushed
the
feature/vpn-cache-invalidation-1098
branch
from
b48cac9 to
f6a46c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
openwisp_controller/config/tasks.py (1)
118-122: Consider usinglogging.exceptionfor better debugging context.The static analysis tool suggests using
logging.exceptioninstead oflogging.errorwhen logging from an exception handler. This automatically includes the stack trace, which aids debugging.🔎 Proposed fix
except SoftTimeLimitExceeded: - logger.error( + logger.exception( "soft time limit hit while executing " f"invalidate_organization_vpn_cache for organization {organization_id}" )Based on static analysis hints.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
openwisp_controller/config/apps.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tasks.py
🧰 Additional context used
🧬 Code graph analysis (2)
openwisp_controller/config/handlers.py (2)
openwisp_controller/config/tasks.py (1)
invalidate_organization_vpn_cache(104-122)openwisp_controller/geo/base/models.py (1)
organization_id(64-65)
openwisp_controller/config/tasks.py (3)
openwisp_controller/geo/base/models.py (1)
organization_id(64-65)openwisp_controller/config/controller/views.py (2)
GetVpnView(485-516)invalidate_get_vpn_cache(508-516)openwisp_controller/config/base/base.py (2)
invalidate_checksum_cache(57-64)invalidate_checksum_cache(92-94)
🪛 Ruff (0.14.10)
openwisp_controller/config/handlers.py
157-157: Unused function argument: kwargs
(ARG001)
170-170: Unused function argument: kwargs
(ARG001)
openwisp_controller/config/tasks.py
119-122: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | 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.12 | 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.11 | django~=5.1.0
🔇 Additional comments (6)
openwisp_controller/config/handlers.py (2)
157-167: LGTM! Pre-save handler correctly captures original context.The handler properly:
- Skips new instances using
_state.adding- Efficiently fetches only the
contextfield from the database- Stores the original value for comparison in post_save
- Handles the edge case where the instance might not exist in the database
The
kwargsparameter flagged by static analysis is required for Django signal handlers and can be safely ignored.
170-180: LGTM! Post-save handler correctly detects changes and schedules invalidation.The handler properly:
- Retrieves the original context stored by the pre_save handler
- Only triggers invalidation when context actually changes
- Uses
transaction.on_committo ensure the task runs after successful database commit- Passes the organization ID as a string for Celery serialization
The logic correctly avoids false triggers on new instances and unchanged updates. The
kwargsparameter flagged by static analysis is required for Django signal handlers.openwisp_controller/config/apps.py (3)
64-66: LGTM! Model loading follows established patterns.The
organization_config_settings_modelis loaded correctly usingload_modeland assigned to the app config instance, consistent with other models in__setmodels__. This resolves the AttributeError mentioned in past reviews.
276-277: LGTM! Handler imports are correct.Both
organization_config_settings_pre_save_handlerandorganization_config_settings_post_save_handlerare imported in the appropriate location withinenable_cache_invalidation, following the existing pattern for other cache invalidation handlers.
344-353: LGTM! Signal connections properly wired.The pre_save and post_save signals are correctly connected to their respective handlers with:
- Proper sender model (
self.organization_config_settings_model)- Unique, descriptive
dispatch_uidvalues following the naming convention- Correct placement within
enable_cache_invalidationalongside other cache invalidation signalsThis completes the signal wiring required for VPN cache invalidation when organization config settings change.
openwisp_controller/config/tasks.py (1)
103-117: No changes needed. The.only("id")approach is correct and sufficient for cache invalidation.The cache invalidation methods call
get_cached_checksum.invalidate(instance)andget_cached_configuration.invalidate(instance), which only require the instance'spkto compute the cache key (viaget_cached_args_rewrite(instance)that returnsinstance.pk.hex). No additional fields are accessed during invalidation. This pattern is already used elsewhere in the codebase—bulk_invalidate_get_cached_checksum()uses the same.only("id").iterator()approach, confirming it is the intended pattern.Likely an incorrect or invalid review comment.
The post_save handler was comparing identical values so changes were never detected.
Split into pre_save (store original) and post_save (compare & invalidate) handlers.
Also added .only('id') optimization and SoftTimeLimitExceeded handling as suggested.
Srinath0916
force-pushed
the
feature/vpn-cache-invalidation-1098
branch
from
f6a46c7 to
cf5582b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
openwisp_controller/config/tasks.py (1)
103-123: Code duplication: Consider extracting VPN cache invalidation logic.This task duplicates the VPN iteration and cache invalidation pattern from
invalidate_controller_views_cache(lines 219-222). Both iterate over VPNs filtered byorganization_id, use.only("id").iterator(), and callGetVpnView.invalidate_get_vpn_cache(vpn).The duplication is minimal (~4 lines), but extracting a helper function would improve maintainability if these patterns are used elsewhere in the future.
Note: The checksum cache invalidation in this task is intentional and appropriate. This task is called when organization config settings change (actual data modification), whereas
invalidate_controller_views_cacheis called when an organization is disabled (only cache visibility concerns), so they have different invalidation requirements.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
openwisp_controller/config/apps.pyopenwisp_controller/config/handlers.pyopenwisp_controller/config/tasks.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openwisp_controller/config/apps.py
🧰 Additional context used
🧬 Code graph analysis (2)
openwisp_controller/config/tasks.py (3)
openwisp_controller/geo/base/models.py (1)
organization_id(64-65)openwisp_controller/config/controller/views.py (2)
GetVpnView(485-516)invalidate_get_vpn_cache(508-516)openwisp_controller/config/base/base.py (2)
invalidate_checksum_cache(57-64)invalidate_checksum_cache(92-94)
openwisp_controller/config/handlers.py (2)
openwisp_controller/config/tasks.py (1)
invalidate_organization_vpn_cache(104-122)openwisp_controller/geo/base/models.py (1)
organization_id(64-65)
🪛 Ruff (0.14.10)
openwisp_controller/config/handlers.py
157-157: Unused function argument: kwargs
(ARG001)
170-170: Unused function argument: kwargs
(ARG001)
⏰ 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.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | 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.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🔇 Additional comments (2)
openwisp_controller/config/handlers.py (2)
157-167: LGTM! Pre-save handler correctly captures original context.The handler properly:
- Returns early for new instances
- Efficiently fetches only the
contextfield from the database- Stores the original value for comparison in post-save
- Handles the DoesNotExist exception
170-180: LGTM! Post-save handler correctly triggers cache invalidation.The handler properly:
- Retrieves the stored original context with a safe fallback
- Only triggers invalidation when context actually changed
- Uses
transaction.on_committo ensure the task runs only after successful database commit- Converts
organization_idto string as expected by the Celery taskThe previous critical issue regarding comparing identical values has been successfully resolved using the pre-save/post-save pattern.
Srinath0916
force-pushed
the
feature/vpn-cache-invalidation-1098
branch
from
cb03988 to
d1c2da8
Compare
…nwisp#1098 - Use getattr for version-safe _initial_context access - Add behavioral tests covering all invalidation scenarios Fixes openwisp#1098
Srinath0916
force-pushed
the
feature/vpn-cache-invalidation-1098
branch
from
d1c2da8 to
87d7730
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/config/base/multitenancy.py (1)
60-75: Guard change detection whenupdate_fieldsomitscontext.If
save(update_fields=...)excludescontext,_initial_contextis still unconditionally updated, creating a desync with the database that can suppress cache invalidation on subsequent saves. Only compare and update_initial_contextwhencontextis actually being persisted.Proposed fix
def save( self, force_insert=False, force_update=False, using=None, update_fields=None ): context_changed = False - if not self._state.adding: + context_will_be_saved = update_fields is None or "context" in update_fields + if not self._state.adding and context_will_be_saved: initial_context = getattr(self, "_initial_context", None) if initial_context is not None: context_changed = initial_context != self.context super().save(force_insert, force_update, using, update_fields) if context_changed and self.organization.is_active: transaction.on_commit( lambda: tasks.invalidate_organization_vpn_cache.delay( str(self.organization_id) ) ) - self._initial_context = deepcopy(self.context) + if context_will_be_saved: + self._initial_context = deepcopy(self.context)
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/tests/test_handlers.py`:
- Around line 37-69: Wrap the DB operations that call config_settings.save() in
each test inside a captureOnCommitCallbacks context so transaction.on_commit
callbacks execute; for the four tests
(test_vpn_cache_invalidated_on_context_change,
test_no_cache_invalidation_on_create,
test_no_cache_invalidation_for_inactive_org,
test_no_cache_invalidation_if_context_unchanged) change the code to use with
self.captureOnCommitCallbacks(execute=True): around the code that mutates and
saves config_settings (and object creation where needed) so
tasks.invalidate_organization_vpn_cache.delay (the on_commit queued task)
actually runs during the test.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_controller/config/base/multitenancy.pyopenwisp_controller/config/tests/test_handlers.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/base/multitenancy.py
📚 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/config/base/multitenancy.pyopenwisp_controller/config/tests/test_handlers.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/config/base/multitenancy.pyopenwisp_controller/config/tests/test_handlers.py
🧬 Code graph analysis (1)
openwisp_controller/config/base/multitenancy.py (3)
openwisp_controller/config/exceptions.py (1)
OrganizationDeviceLimitExceeded(5-20)openwisp_controller/config/tasks.py (1)
invalidate_organization_vpn_cache(104-122)openwisp_controller/geo/base/models.py (1)
organization_id(64-65)
⏰ 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). (8)
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | 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~=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~=5.1.0
🔇 Additional comments (3)
openwisp_controller/config/base/multitenancy.py (2)
4-12: Import updates look good.The additions align with the new task wiring.
50-53: Snapshotting_initial_contexton load is solid.This sets up reliable change detection.
openwisp_controller/config/tests/test_handlers.py (1)
21-27: Mock reset before the subtest is a good tweak.Keeps call assertions isolated between subtests.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/base/multitenancy.py`:
- Around line 65-75: The save() logic must ignore context change detection and
avoid overwriting _initial_context when the caller used update_fields that does
not include "context": modify the code in multitenancy.save() to first detect if
update_fields is not None and "context" not in update_fields, and in that case
skip computing context_changed, skip scheduling
tasks.invalidate_organization_vpn_cache.delay, and do not update
self._initial_context; otherwise (when context is being saved or update_fields
is None) compute context_changed against self._initial_context and, if changed
and self.organization.is_active, call transaction.on_commit(lambda:
tasks.invalidate_organization_vpn_cache.delay(str(self.organization_id)),
using=using) so the callback uses the same DB connection, and finally
deepcopy(self.context) into self._initial_context only in the branch where
context was part of the save.
🧹 Nitpick comments (1)
openwisp_controller/config/base/multitenancy.py (1)
50-53: No evidence of deferredcontextfield in queries.I scanned the codebase for
.only()and.defer()patterns onOrganizationConfigSettings. The only explicit optimization found is.only("context")atconfig/views.py:194, which includescontextrather than deferring it. The main query incontroller/views.py:367uses.select_related("organization")without any field filtering. No.defer("context")patterns exist in the codebase.While the concern is theoretically sound—accessing a deferred field in
__init__would trigger an unnecessary query—there is no practical evidence that this model is instantiated withcontextdeferred. If performance optimization patterns emerge in the future, this would become relevant.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_controller/config/base/multitenancy.pyopenwisp_controller/config/tests/test_handlers.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openwisp_controller/config/tests/test_handlers.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/base/multitenancy.py
📚 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/config/base/multitenancy.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/config/base/multitenancy.py
🧬 Code graph analysis (1)
openwisp_controller/config/base/multitenancy.py (3)
openwisp_controller/config/base/device_group.py (1)
save(79-90)openwisp_controller/config/tasks.py (1)
invalidate_organization_vpn_cache(104-122)openwisp_controller/geo/base/models.py (1)
organization_id(64-65)
⏰ 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~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.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.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~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
🔇 Additional comments (1)
openwisp_controller/config/base/multitenancy.py (1)
5-12: Imports updated appropriately for the new task usage.No concerns here.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/tests/test_handlers.py`:
- Around line 33-41: The local variable named created returned from
OrganizationConfigSettings.objects.get_or_create in the _get_org_config_settings
function is unused and triggers a Ruff RUF059 warning; rename it to _created or
_ (e.g., change "config_settings, created =
OrganizationConfigSettings.objects.get_or_create(...)" to "config_settings,
_created = ..." or "config_settings, _ = ...") so the unused value is explicitly
ignored while leaving the rest of _get_org_config_settings unchanged.
- Around line 44-81: The test method signatures exceed the project's 88-char
line length limit (E501); wrap the long decorated function definitions (e.g.,
test_vpn_cache_invalidated_on_context_change,
test_no_cache_invalidation_on_create,
test_no_cache_invalidation_for_inactive_org,
test_no_cache_invalidation_if_context_unchanged) so each decorator stays on its
own line and the def line is broken across multiple lines for its parameters (or
put one decorator per line and break the parameter list), ensuring the full
definition lines are <=88 chars and no noqa is used.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_controller/config/base/multitenancy.pyopenwisp_controller/config/tests/test_handlers.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/base/multitenancy.pyopenwisp_controller/config/tests/test_handlers.py
📚 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/config/base/multitenancy.pyopenwisp_controller/config/tests/test_handlers.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/config/base/multitenancy.pyopenwisp_controller/config/tests/test_handlers.py
🧬 Code graph analysis (2)
openwisp_controller/config/base/multitenancy.py (3)
openwisp_controller/config/base/vpn.py (2)
save(244-293)save(870-878)openwisp_controller/config/base/device_group.py (1)
save(79-90)openwisp_controller/config/tasks.py (2)
invalidate_organization_vpn_cache(104-122)bulk_invalidate_config_get_cached_checksum(199-201)
openwisp_controller/config/tests/test_handlers.py (3)
openwisp_controller/config/tests/test_controller.py (1)
_create_org(44-49)openwisp_controller/config/base/multitenancy.py (1)
save(60-82)openwisp_controller/config/tasks.py (2)
invalidate_organization_vpn_cache(104-122)bulk_invalidate_config_get_cached_checksum(199-201)
🪛 Ruff (0.14.14)
openwisp_controller/config/tests/test_handlers.py
38-38: Unpacked variable created is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ 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~=4.2.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.13 | 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.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
🔇 Additional comments (5)
openwisp_controller/config/base/multitenancy.py (3)
5-11: Imports for transaction + tasks are appropriate.No concerns with these additions.
50-53: Baseline context snapshot is clear and safe.Deepcopying the initial context provides a stable baseline for change detection.
60-82: Change detection + on_commit invalidation flow looks solid.The update_fields guard and on_commit callback are well-placed, and updating
_initial_contextonly when context is saved avoids false positives.openwisp_controller/config/tests/test_handlers.py (2)
3-5: Test utility import looks fine.No concerns here.
22-27: Subtest isolation is clearer now.Resetting the mock before the inactive→active case keeps assertions independent.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Srinath0916
force-pushed
the
feature/vpn-cache-invalidation-1098
branch
from
c4c5f14 to
5bca3cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/base/multitenancy.py`:
- Around line 50-55: AbstractOrganizationConfigSettings.__init__ calls
self._is_deferred("context") but _is_deferred is only defined on AbstractDevice,
causing AttributeError; fix by either implementing _is_deferred on
AbstractOrganizationConfigSettings (copy the logic from
AbstractDevice._is_deferred) or refactor the init to check deferred state via
self.get_deferred_fields() (e.g., treat "context" as deferred if "context" in
self.get_deferred_fields()); update both occurrences (the checks at
initialization and the later use at line ~84) to use the new _is_deferred
implementation or the get_deferred_fields() approach so the deferred-field
detection works without relying on AbstractDevice.
🧹 Nitpick comments (2)
openwisp_controller/config/base/multitenancy.py (1)
73-82: Inconsistent transaction semantics between the two cache invalidation calls.
bulk_invalidate_config_get_cached_checksum.delay()is called immediately aftersuper().save(), whileinvalidate_organization_vpn_cacheis wrapped intransaction.on_commit. If an outer transaction wraps this save and later rolls back, the config cache will be invalidated but the VPN cache won't, leading to inconsistent state.Consider wrapping both in
on_commitfor atomicity, or document why the asymmetry is intentional.Suggested fix for consistent transaction handling
if context_changed and self.organization.is_active: - tasks.bulk_invalidate_config_get_cached_checksum.delay( - {"device__organization_id": str(self.organization_id)} - ) - transaction.on_commit( - lambda: tasks.invalidate_organization_vpn_cache.delay( - str(self.organization_id) - ), - using=using, - ) + org_id = str(self.organization_id) + transaction.on_commit( + lambda: tasks.bulk_invalidate_config_get_cached_checksum.delay( + {"device__organization_id": org_id} + ), + using=using, + ) + transaction.on_commit( + lambda: tasks.invalidate_organization_vpn_cache.delay(org_id), + using=using, + )openwisp_controller/config/tests/test_handlers.py (1)
80-94: Consider adding a test forupdate_fieldsexcludingcontext.The implementation in
multitenancy.pyhas special handling for whensave(update_fields=...)is called without"context". Adding a test case would ensure this edge case is covered:`@patch.object`(tasks.invalidate_organization_vpn_cache, "delay") `@patch.object`(tasks.bulk_invalidate_config_get_cached_checksum, "delay") def test_no_cache_invalidation_when_context_not_in_update_fields( self, config_cache_mock, vpn_cache_mock ): """Test no invalidation when save uses update_fields without context""" config_settings = self._get_org_config_settings() config_settings.context = {"new": "context"} with self.captureOnCommitCallbacks(execute=True): config_settings.save(update_fields=["registration_enabled"]) vpn_cache_mock.assert_not_called() config_cache_mock.assert_not_called()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_controller/config/base/multitenancy.pyopenwisp_controller/config/tests/test_handlers.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/base/multitenancy.py
📚 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/config/base/multitenancy.pyopenwisp_controller/config/tests/test_handlers.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/config/base/multitenancy.pyopenwisp_controller/config/tests/test_handlers.py
🧬 Code graph analysis (2)
openwisp_controller/config/base/multitenancy.py (3)
openwisp_controller/config/base/device.py (2)
_is_deferred(304-308)save(270-288)openwisp_controller/config/base/device_group.py (1)
save(79-90)openwisp_controller/config/tasks.py (2)
bulk_invalidate_config_get_cached_checksum(199-201)invalidate_organization_vpn_cache(104-122)
openwisp_controller/config/tests/test_handlers.py (3)
openwisp_controller/config/tests/test_controller.py (1)
_create_org(44-49)openwisp_controller/config/base/multitenancy.py (1)
save(63-87)openwisp_controller/config/tasks.py (2)
invalidate_organization_vpn_cache(104-122)bulk_invalidate_config_get_cached_checksum(199-201)
🪛 Ruff (0.14.14)
openwisp_controller/config/tests/test_handlers.py
37-37: Unpacked variable created is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🔇 Additional comments (5)
openwisp_controller/config/base/multitenancy.py (2)
50-55: Good use of__init__for initial state tracking.Storing
_initial_contextat initialization avoids the extra database query thatDeviceGroup.save()uses (seebase/device_group.py:78-89). The handling ofmodels.DEFERREDstate is correct for cases where.only()or.defer()excludes thecontextfield.
66-71: Context change detection logic looks correct.The guard for
context_in_updateproperly handlesupdate_fieldsscenarios, and themodels.DEFERREDcheck prevents false positives when the field was never loaded. This addresses the concerns raised in previous reviews.openwisp_controller/config/tests/test_handlers.py (3)
42-55: Good test coverage for context change invalidation.The test correctly patches both task delays, uses
captureOnCommitCallbacks(execute=True)to trigger theon_commitcallback, and verifies both caches are invalidated with the correct organization ID.
57-64: Comprehensive negative test cases.The tests properly cover:
- No invalidation on creation (new objects shouldn't trigger cache invalidation)
- No invalidation for inactive organizations
- No invalidation when context remains unchanged
All use
captureOnCommitCallbackscorrectly to ensure proper transaction handling during tests.Also applies to: 66-78, 80-94
21-26: Good improvement to test isolation.Adding
reset_mock()and the intermediate assertions for creating an inactive org ensures cleaner test state and more precise verification of when the task should/shouldn't be called.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Srinath0916
force-pushed
the
feature/vpn-cache-invalidation-1098
branch
from
5bca3cd to
95a4c8d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/base/multitenancy.py`:
- Around line 66-71: The current logic skips cache invalidation when
_initial_context == models.DEFERRED, which treats a deferred/unknown initial
state as "not changed"; update the check in the save/update path (the variables
context_changed, context_in_update, _initial_context, models.DEFERRED, and
self.context) so that if the instance is not adding and context_in_update is
True and _initial_context is models.DEFERRED you conservatively treat the
context as changed (i.e., set context_changed = True), otherwise keep the
existing comparison behavior (initial_context != self.context) for non-deferred
initial values; this ensures deferred fields trigger invalidation when updated.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_controller/config/base/multitenancy.pyopenwisp_controller/config/tests/test_handlers.py
🧰 Additional context used
🧠 Learnings (3)
📚 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/config/base/multitenancy.pyopenwisp_controller/config/tests/test_handlers.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/base/multitenancy.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/config/base/multitenancy.pyopenwisp_controller/config/tests/test_handlers.py
🧬 Code graph analysis (1)
openwisp_controller/config/tests/test_handlers.py (3)
openwisp_controller/config/tests/test_controller.py (1)
_create_org(44-49)openwisp_controller/config/base/multitenancy.py (1)
save(63-87)openwisp_controller/config/tasks.py (2)
invalidate_organization_vpn_cache(104-122)bulk_invalidate_config_get_cached_checksum(199-201)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
🔇 Additional comments (3)
openwisp_controller/config/base/multitenancy.py (1)
50-56: Baseline tracking looks good.
The deferred-field guard keeps _initial_context consistent.openwisp_controller/config/tests/test_handlers.py (2)
21-27: Nice improvement to inactive-org coverage.
Resetting the mock before toggling activation makes the assertions clearer.
29-95: Solid test matrix for cache invalidation.
Covers change, create, inactive org, and unchanged context paths with clear mock assertions.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Srinath0916
force-pushed
the
feature/vpn-cache-invalidation-1098
branch
from
95a4c8d to
82cfe2a
Compare
Srinath0916
force-pushed
the
feature/vpn-cache-invalidation-1098
branch
from
82cfe2a to
240026a
Compare
Srinath0916
force-pushed
the
feature/vpn-cache-invalidation-1098
branch
from
240026a to
81ccddf
Compare
|
@nemesifier @stktyagi Feedback addressed, please have a look and review it. |
Srinath0916
force-pushed
the
feature/vpn-cache-invalidation-1098
branch
from
81ccddf to
dfc4ee7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/base/multitenancy.py`:
- Around line 77-86: The bulk_invalidate_config_get_cached_checksum task is
dispatched immediately while invalidate_organization_vpn_cache is deferred with
transaction.on_commit, causing inconsistent behavior on rollback; change the
call to tasks.bulk_invalidate_config_get_cached_checksum.delay(...) so it is
executed inside transaction.on_commit (like the
invalidate_organization_vpn_cache call), using the same transaction.on_commit
wrapper and preserving the filter payload {"device__organization_id":
str(self.organization_id)} and the using parameter, conditioned on
context_changed and self.organization.is_active.
🧹 Nitpick comments (1)
openwisp_controller/config/base/multitenancy.py (1)
72-75: Redundantcontext_in_updatecheck.The condition
and context_in_updateon line 72 is redundant since the outerifon line 68 already guaranteescontext_in_updateisTrue. This doesn't affect correctness but reduces readability.Proposed simplification
if not self._state.adding and context_in_update: initial_context = getattr(self, "_initial_context", None) if initial_context is not None and initial_context != models.DEFERRED: context_changed = initial_context != self.context - elif initial_context == models.DEFERRED and context_in_update: + elif initial_context == models.DEFERRED: # Conservative: if we don't know initial state and context is # being updated, assume it changed to avoid stale cache context_changed = True
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
openwisp_controller/config/base/multitenancy.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_handlers.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openwisp_controller/config/tasks.py
🧰 Additional context used
🧠 Learnings (3)
📚 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/config/base/multitenancy.pyopenwisp_controller/config/tests/test_handlers.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/base/multitenancy.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/config/base/multitenancy.pyopenwisp_controller/config/tests/test_handlers.py
🧬 Code graph analysis (2)
openwisp_controller/config/base/multitenancy.py (2)
openwisp_controller/config/tasks.py (2)
bulk_invalidate_config_get_cached_checksum(198-200)invalidate_organization_vpn_cache(104-121)openwisp_controller/geo/base/models.py (1)
organization_id(64-65)
openwisp_controller/config/tests/test_handlers.py (3)
openwisp_controller/config/tests/test_controller.py (1)
_create_org(44-49)openwisp_controller/config/base/multitenancy.py (1)
save(63-91)openwisp_controller/config/tasks.py (2)
invalidate_organization_vpn_cache(104-121)bulk_invalidate_config_get_cached_checksum(198-200)
⏰ 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.10 | 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.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.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~=5.1.0
🔇 Additional comments (3)
openwisp_controller/config/base/multitenancy.py (1)
50-55: LGTM - Clean initialization with proper deferred field handling.The
__init__method correctly snapshots the initial context, handling deferred fields viaget_deferred_fields(). This avoids the previous_is_deferredmethod issue.openwisp_controller/config/tests/test_handlers.py (2)
21-26: LGTM - Proper mock isolation for subtest.Adding
reset_mock()before creatinginactive_orgensures clean state for the "inactive to active" test scenario. Good practice for accurate assertions.
29-95: Comprehensive test coverage for VPN cache invalidation.The new test class correctly covers:
- Context change triggers both cache invalidations
- Creation does not trigger invalidation
- Inactive organizations are skipped
- Unchanged context does not trigger invalidation
The proper use of
captureOnCommitCallbacks(execute=True)ensurestransaction.on_commitcallbacks execute during tests. Bothconfig_cache_mockandvpn_cache_mockare verified in each test, aligning with the dual invalidation logic inmultitenancy.py.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Srinath0916
force-pushed
the
feature/vpn-cache-invalidation-1098
branch
from
dfc4ee7 to
6542c80
Compare
- Fixed transaction.on_commit callback structure - Added proper config cache invalidation alongside VPN cache - Updated tests to use captureOnCommitCallbacks for transaction testing - Fixed update_fields handling to prevent unnecessary invalidations Fixes openwisp#1098
Srinath0916
force-pushed
the
feature/vpn-cache-invalidation-1098
branch
from
6542c80 to
ed6beb7
Compare
5 participants
When organization configuration variables are updated, VPN cache wasn't being invalidated automatically. This caused VPNs to use stale config data leading to connectivity issues.
Added signal handler to detect context changes in OrganizationConfigSettings and trigger cache invalidation for all VPNs in that organization. Also added comprehensive tests to ensure the functionality works correctly.
Fixes #1098
Checklist
Reference to Existing Issue
Closes #1098.
Description of Changes
organization_config_settings_change_handlerinhandlers.pyto detect when OrganizationConfigSettings context field changesinvalidate_organization_vpn_cacheCelery task intasks.pyto invalidate VPN cache for all VPNs in the affected organizationapps.pytest_organization_vpn_cache.pycovering signal handling, task execution, and edge casesThe implementation ensures VPN cache is automatically invalidated whenever organization configuration variables change, preventing stale configuration issues.
Screenshot
Not applicable - this is a backend functionality change with no UI components.