[fix] Fixes to whois/estimated locations for bugs found during testing#1197
[fix] Fixes to whois/estimated locations for bugs found during testing#1197nemesifier merged 13 commits intogsoc25-whoisfrom
Conversation
📝 WalkthroughWalkthroughThe changes adjust WHOIS and estimated-location behavior: Device initialization now avoids adding duplicate "last_ip" entries to _changed_checked_fields; WHOISService.is_older enforces timezone-aware datetimes; WHOIS processing handles missing location/timezone and maps specific GeoIP2 exceptions to user-facing messages; WHOIS task execution uses Django cache to track per-task and per-device failure/success states to limit duplicate notifications; estimated-location creation avoids recreating identical locations when WHOIS data is unchanged. Tests updated/added accordingly. Sequence Diagram(s)sequenceDiagram
participant Task as fetch_whois_details Task
participant Cache as Django Cache
participant GeoIP as GeoIP2 Service
participant WHOIS as WHOISService
participant DB as Device / Location DB
participant Notify as Notification system
Task->>Cache: check per-task/per-device last_operation
alt cache indicates already-notified permanent failure
Task-->>Notify: (skip duplicate notification)
end
Task->>GeoIP: lookup IP
GeoIP-->>Task: result or exception
alt exception
Task->>Cache: set failure key (per-task or per-device)
Task->>Notify: send notification (only if not cached)
else success
Task->>WHOIS: process_whois_details(result)
WHOIS-->>WHOIS: derive coordinates/timezone (allow None)
WHOIS->>DB: create/update WHOISInfo
WHOIS->>DB: attach or reuse existing Location (no-op if unchanged)
WHOIS->>Cache: set last_operation = "success" (task & device)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Signed-off-by: DragnEmperor <dragnemperor@gmail.com>
| ) | ||
| self._changed_checked_fields = self._changed_checked_fields + ["last_ip"] | ||
| super().__init__(*args, **kwargs) | ||
| self._set_initial_values_for_changed_checked_fields() |
There was a problem hiding this comment.
When I see this code now, I think it is incorrect to add last_ip in the _changed_checked_fields here.
_changed_checked_fields is a class variable, so we should only update it in the class definition.
There was a problem hiding this comment.
This is done to allow test in isolation, as it is evaluated at import time. We encountered the same problem elsewhere as well:
There was a problem hiding this comment.
Okay! If it creates a problem in the future, we will update the tests and mock this private attribute for the tests to simulate that the feature is turned on.
Signed-off-by: DragnEmperor <dragnemperor@gmail.com>
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
Thanks for following up @DragnEmperor! Overall the PR looks good.
I want to understand how general last status is different from device level status. What kind of decisions are we making based on the different last operation.
| device = self._create_device() | ||
| self._create_config(device=device) |
There was a problem hiding this comment.
Why is this code moved to the last test case?
| @override_settings(CELERY_TASK_EAGER_PROPAGATES=True) | ||
| @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True) | ||
| @mock.patch("openwisp_controller.config.whois.tasks.fetch_whois_details.retry") | ||
| def test_whois_task_retry_mechanism(self, mock_retry): |
| with self.subTest("Test Status Cached per device for other exceptions"): | ||
| cache.clear() | ||
| device = trigger_error_and_assert_cached(Exception, True, 1) | ||
| with self.subTest(f"Cache reused by {Exception.__name__}"), mock.patch( | ||
| self._WHOIS_GEOIP_CLIENT, side_effect=Exception("test") | ||
| ): | ||
| device.last_ip = "172.217.22.15" | ||
| device.save() | ||
| cache_key = f"{self._WHOIS_TASK_NAME}_{device.pk}_last_operation" | ||
| self.assertEqual(cache.get(cache_key), "error") | ||
| self.assertEqual(notification_qs.count(), 0) |
There was a problem hiding this comment.
I understand this problem as following:
- We clear the cache status to obtain a clean state
- First exception will set the cache and send the notification
- Second exception will check that the last operation failed and will not send the notification
In the end, we should have one notification, right?
There was a problem hiding this comment.
Let's say a new system was configured which does not have any WHOIS record. Then, the user pasted the wrong credentials.
In this case, the first task executed will run into an error and it should send a notification to the user.
If we don't do this, then this will remain unnoticed. And, all future errors will be silenced because of the cache key.
Please let me know if my assumptions are wrong.
There was a problem hiding this comment.
I understand this problem as following:
- We clear the cache status to obtain a clean state
- First exception will set the cache and send the notification
- Second exception will check that the last operation failed and will not send the notification
In the end, we should have one notification, right?
yes the notification will be 1 and 0 for any subsequent errors
There was a problem hiding this comment.
Let's say a new system was configured which does not have any WHOIS record. Then, the user pasted the wrong credentials.
In this case, the first task executed will run into an error and it should send a notification to the user.
If we don't do this, then this will remain unnoticed. And, all future errors will be silenced because of the cache key.
Please let me know if my assumptions are wrong.
Yes this is the correct flow. The current implementation sends only one notification if error is of non_recoverable type i.e. one of the permanent errors (OutOfQueries, Authentication, Permission and AddressNotFound).
If we are talking about a scenario where two errors of different types (lets say Authentication and Permission) are raised simultaneously then yes the latter one will go unnoticed.
We can store the exception type in the cache and restrict notifications if same exception is raised again.
There was a problem hiding this comment.
Yes this is the correct flow. The current implementation sends only one notification if error is of non_recoverable type i.e. one of the permanent errors (OutOfQueries, Authentication, Permission and AddressNotFound).
This is correct.
We can store the exception type in the cache and restrict notifications if same exception is raised again.
We don't need to implement this. The current flow is sufficient.
| else: | ||
| task_key = f"{self.name}_{device_pk}_last_operation" | ||
| last_operation = cache.get(task_key) | ||
| if last_operation != "error": | ||
| cache.set(task_key, "error", None) | ||
| send_whois_task_notification( | ||
| device=device_pk, notify_type="whois_device_error" | ||
| ) |
There was a problem hiding this comment.
According to this logic, if I have 100 devices, and the all 100 of them encounters HTTPError, then I will get 100 notifications. I don't think this is ideal.
There was a problem hiding this comment.
Following is an evaluation by GitHub Copilot using Claude Sonnet 4.5:
TLDR
HTTPError currently sends a notification per device, causing spam during service outages
(100 devices = 100 notifications). Recommendation: Use global caching like permanent errors
to send one notification regardless of device count. This prevents spam while appropriately
treating HTTPError as a service-wide issue rather than device-specific failures.
HTTPError Notification Strategy Evaluation
Current Implementation
- HTTPError uses device-specific caching
- Each device that fails after retries sends a separate notification
- Problem: During GeoIP2 service outages, 100 devices = 100 notifications
Option A: Global Cache (Like Permanent Errors)
Pros:
- Simple to implement (consistent with existing permanent error handling)
- Prevents notification spam during service outages
- Clear signal that GeoIP2 service is having issues
- One notification regardless of device count
Cons:
- Lose visibility into which specific devices failed
- Can't distinguish between service-wide outage vs specific device issues
- No per-device recovery tracking
Best for:
Small/medium deployments, rare service outages
Option B: Hybrid with Time-Based Throttling
Approach:
- First HTTPError within 1 hour window sends notification with affected device count
- Subsequent HTTPErrors within same window are tracked but don't send notifications
- After 1 hour, if still failing, send new summary notification
- Maintain device-level cache for recovery tracking
Pros:
- Prevents notification spam (maximum 1 notification per hour)
- Preserves device-level tracking for troubleshooting
- Notifications include helpful context like "15 devices affected"
- Enables future recovery notifications per device
- Distinguishes between isolated failures vs service-wide outages
Cons:
- More complex implementation
- Requires additional cache key management
- Slightly more difficult to understand and maintain
- Delayed visibility (up to 1 hour) if issues start gradually
Best for:
Large deployments with many devices
Option C: Keep Current Behavior + Document
Pros:
- No code changes needed
- Preserves full device-level visibility
- Simple to understand
Cons:
- Notification spam during outages
- Poor admin experience at scale
- Could lead to notification fatigue and missed alerts
Best for:
Very small deployments (fewer than 10 devices)
Recommendation: Option A (Global Cache)
Why Option A:
- HTTPError after exhausting retries typically indicates service-wide issues such as API downtime or network problems, not device-specific issues
- Consistent with how other service-wide errors (OutOfQueriesError, AuthenticationError) are already handled
- Simple, maintainable solution that can be implemented quickly
- Significantly improves admin experience without over-engineering
Implementation Strategy:
- Treat HTTPError the same as permanent errors in the on_failure method
- Use global cache key instead of device-specific key
- Send single notification for service-wide HTTPError failures
- Can still log individual device failures for troubleshooting via application logs
Future Enhancement Path:
- If device-level tracking becomes a requirement, can upgrade to Option B
- Add admin dashboard showing recent failed devices by querying application logs
- Monitor whether HTTPError patterns suggest need for more granular approach
Why Not Option B:
- Adds significant complexity without clear immediate need
- Start simple and enhance later if production usage reveals necessity
- Overhead of time-based throttling may not be justified initially
Why Not Option C:
- Notification spam is a real UX problem that will negatively affect production deployments
- Admin fatigue leads to missed important alerts
- Does not scale beyond small installations
There was a problem hiding this comment.
According to this logic, if I have 100 devices, and the all 100 of them encounters HTTPError, then I will get 100 notifications. I don't think this is ideal.
Yes, I was a bit confused if we should have global level cache or device level so I implemented both (global for permanent/non-recoverable and others for device level).
Do we need device level errors to send notification?
There was a problem hiding this comment.
Let's handle this problem with a global cache.
| general_status_key = f"{self.name}_last_operation" | ||
| device_status_key = f"{self.name}_{device_pk}_last_operation" | ||
| if cache.get(general_status_key) != "success": | ||
| cache.set(general_status_key, "success", None) | ||
| if cache.get(device_status_key) != "success": | ||
| cache.set(device_status_key, "success", None) |
There was a problem hiding this comment.
Can this be moved to OpenwispCeleryTask.on_success ?
There was a problem hiding this comment.
While reviewing this PR, I also noticed the following issue
Bug: Estimated Location Recreated Despite Identical WHOIS Data
Steps to Reproduce:
-
Setup: Two devices with same public IP (e.g., 160.202.37.XXX)
- Device A has estimated location L1
- Device B has estimated location L1 (same shared location)
-
Trigger: Clear
last_ipfield for both devices (set to empty string) -
Action: Both devices fetch their checksum sequentially
- Both devices get the same IP: 160.202.37.XXX
- WHOIS lookup returns SAME coordinates as location L1
-
Expected Result:
- Both devices keep using location L1
- No new location created (data unchanged)
- No notifications sent
-
Actual Result:
- Device A: Creates NEW location L2 with IDENTICAL coordinates
- Device B: Reuses L2
- Location L1 is DELETED
- Two notifications sent ("created" and "updated")
Root Cause:
Code condition checks if (other_devices_exist AND location_is_estimated) but does NOT check if WHOIS data actually changed before creating new location.
Impact:
- Unnecessary location churn on every IP refresh
- Old locations deleted (data loss)
- Database bloat
- Misleading notifications
I have implemented a test for this, but I don't understand where to put this fix.
diff --git a/openwisp_controller/geo/estimated_location/tests/tests.py b/openwisp_controller/geo/estimated_location/tests/tests.py
index 14222f23..8eb07abf 100644
--- a/openwisp_controller/geo/estimated_location/tests/tests.py
+++ b/openwisp_controller/geo/estimated_location/tests/tests.py
@@ -560,6 +560,58 @@ class TestEstimatedLocationTransaction(
)
mock_info.reset_mock()
+ @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True)
+ @mock.patch(_ESTIMATED_LOCATION_INFO_LOGGER)
+ @mock.patch(_WHOIS_GEOIP_CLIENT)
+ def test_unchanged_whois_data_no_location_recreation(
+ self,
+ mock_client,
+ mock_info,
+ ):
+ """
+ Two devices share same IP/location. Clear last_ip, then both fetch
+ checksum with same IP. WHOIS returns identical coords. Expected: keep
+ existing location. Bug: creates new location, deletes old one.
+ """
+ connect_whois_handlers()
+ mocked_response = self._mocked_client_response()
+ mock_client.return_value.city.return_value = mocked_response
+ shared_ip = "20.49.19.19"
+ device1 = self._create_device(
+ name="device-a",
+ mac_address="00:11:22:33:44:55",
+ last_ip=shared_ip,
+ )
+ device2 = self._create_device(
+ name="device-b",
+ mac_address="00:11:22:33:44:66",
+ last_ip=shared_ip,
+ )
+ original_location = device1.devicelocation.location
+ self.assertEqual(original_location.pk, device2.devicelocation.location.pk)
+ location_count = Location.objects.count()
+ notification_count = notification_qs.count()
+
+ # Clear the last ip for both devices, so setting them again
+ # will trigger the WHOIS lookup flow.
+ for device in (device1, device2):
+ device.last_ip = ""
+ device.save(update_fields=["last_ip"])
+ device.refresh_from_db()
+
+ # We set the same shared IP again. This simulates device fetching checksum.
+ for device in (device1, device2):
+ device.last_ip = shared_ip
+ device.save(update_fields=["last_ip"])
+ device.refresh_from_db()
+
+ # The location object should remain unchanged since the WHOIS data is the same.
+ self.assertEqual(original_location.pk, device1.devicelocation.location.pk)
+ self.assertEqual(device1.devicelocation.location.pk, device2.devicelocation.location.pk)
+ self.assertEqual(Location.objects.count(), location_count)
+ self.assertTrue(Location.objects.filter(pk=original_location.pk).exists())
+ self.assertEqual(notification_qs.count(), notification_count)
+
@mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True)
@mock.patch(_ESTIMATED_LOCATION_INFO_LOGGER)
@mock.patch(_ESTIMATED_LOCATION_ERROR_LOGGER)
I fixed this issue in 37448f2 |
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
@DragnEmperor can you help me verify this scenario?
Steps to Replicate
-
Create a device
-
Configure features (initial state)
- Enable the WHOIS feature for the device.
- Ensure the Estimated Location feature is disabled.
-
Adjust WHOIS triggering behavior
- Edit WHOIsService.is_older such that it always return True
- This will allow to test an expired WHOIS record.
- Alternatively, you can update the WHOIS record from the Django shell
-
Trigger WHOIS
- Trigger the WHOIS lookup for the device.
- Confirm that the WHOIS record is created as expected.
- Ideally, we should do it by fetching checksum of the device
-
Enable Estimated Location
- Enable the Estimated Location feature for the same device.
-
Trigger WHOIS again
- Trigger the WHOIS lookup once more for the device.
-
Verify result
- Check whether an Estimated Location entry is created for the device after the WHOIS lookup.
| and current_location.geometry == location_defaults.get("geometry") | ||
| and current_location.name == location_defaults.get("name") | ||
| ): | ||
| logger.info( |
There was a problem hiding this comment.
| logger.info( | |
| logger.debug( |
Otherwise we'll flood the logs with no benefit.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
| if exc.__class__ in EXCEPTION_MESSAGES: | ||
| task_key = f"{self.name}_last_operation" | ||
| last_operation = cache.get(task_key) | ||
| if last_operation != "non_recoverable": | ||
| cache.set(task_key, "non_recoverable", None) | ||
| send_whois_task_notification( | ||
| device=device_pk, notify_type="whois_device_error" | ||
| ) | ||
| else: | ||
| task_key = f"{self.name}_{device_pk}_last_operation" | ||
| last_operation = cache.get(task_key) | ||
| if last_operation != "error": | ||
| cache.set(task_key, "error", None) | ||
| send_whois_task_notification( | ||
| device=device_pk, notify_type="whois_device_error" | ||
| ) |
There was a problem hiding this comment.
Sure, I think we can do that. A follow up issue would be good.
Also you can le tme know about any changes in #1147 if there are any whenever you are free to, so that we can move with completeness towards the follow up later.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_controller/geo/estimated_location/tests/tests.py`:
- Around line 563-612: The test function
test_unchanged_whois_data_no_location_recreation has an unused mocked logger
parameter mock_info introduced by `@mock.patch`(_ESTIMATED_LOCATION_INFO_LOGGER);
remove the unused mock to satisfy static analysis by deleting the
`@mock.patch`(_ESTIMATED_LOCATION_INFO_LOGGER) decorator and removing mock_info
from the test signature, ensuring the remaining `@mock.patch`(_WHOIS_GEOIP_CLIENT)
and its parameter (mock_client) remain in the correct order.
🧹 Nitpick comments (2)
openwisp_controller/config/whois/tasks.py (1)
27-52: Well-designed notification deduplication logic.The cache-based approach effectively prevents notification spam by distinguishing between:
- Per-task permanent failures (e.g.,
OutOfQueriesError,AuthenticationError) - one notification until manually cleared- Per-device transient failures - one notification per device until success
One concern: using
cache.set(..., None)sets an indefinite timeout. While this may be intentional for permanent errors, it means the "error" state for per-device failures will persist forever unless explicitly cleared or the cache is flushed. Consider if a TTL would be more appropriate for device-level errors to allow retry notifications after some period.openwisp_controller/config/whois/tests/tests.py (1)
421-433: Avoid brittleside_effectlist for cache.get.
Using a single-itemside_effectmakes the test fail withStopIterationif the implementation callscache.getmore than once. Preferreturn_value=Noneand, if desired, assert call count explicitly.♻️ Proposed tweak
- with mock.patch( - "django.core.cache.cache.get", side_effect=[None] - ) as mocked_get, mock.patch("django.core.cache.cache.set") as mocked_set: + with mock.patch( + "django.core.cache.cache.get", return_value=None + ) as mocked_get, mock.patch("django.core.cache.cache.set") as mocked_set:Also applies to: 438-447
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
openwisp_controller/config/base/device.pyopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tasks.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/config/whois/tests/utils.pyopenwisp_controller/config/whois/utils.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openwisp_controller/config/base/device.py
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
📚 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/whois/service.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/whois/tests/utils.pyopenwisp_controller/config/whois/utils.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/whois/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/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/config/whois/tests/utils.pyopenwisp_controller/config/whois/utils.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/whois/tasks.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/whois/tests/tests.pyopenwisp_controller/config/whois/tests/utils.pyopenwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-01-15T14:06:53.460Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Applied to files:
openwisp_controller/geo/estimated_location/tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.py
🧬 Code graph analysis (6)
openwisp_controller/config/whois/service.py (1)
openwisp_controller/config/whois/utils.py (1)
send_whois_task_notification(62-73)
openwisp_controller/config/whois/tests/tests.py (1)
openwisp_controller/config/whois/service.py (2)
is_older(69-81)process_whois_details(145-197)
openwisp_controller/geo/estimated_location/tasks.py (2)
openwisp_controller/config/base/config.py (1)
name(132-139)openwisp_controller/geo/base/models.py (1)
device(152-153)
openwisp_controller/config/whois/tests/utils.py (1)
openwisp_controller/geo/base/models.py (1)
device(152-153)
openwisp_controller/geo/estimated_location/tests/tests.py (5)
openwisp_controller/config/base/device.py (1)
save(279-299)openwisp_controller/geo/base/models.py (2)
save(68-106)device(152-153)openwisp_controller/config/base/multitenancy.py (1)
save(90-101)openwisp_controller/config/whois/handlers.py (1)
connect_whois_handlers(7-29)openwisp_controller/config/whois/tests/utils.py (1)
_mocked_client_response(46-58)
openwisp_controller/config/whois/tasks.py (1)
openwisp_controller/config/whois/utils.py (1)
send_whois_task_notification(62-73)
🪛 Ruff (0.14.14)
openwisp_controller/config/whois/service.py
75-78: Avoid specifying long messages outside the exception class
(TRY003)
openwisp_controller/geo/estimated_location/tests/tests.py
569-569: Unused method argument: mock_info
(ARG002)
🔇 Additional comments (17)
openwisp_controller/config/whois/utils.py (1)
44-59: LGTM! Well-structured exception messages mapping.The
EXCEPTION_MESSAGESdictionary cleanly centralizes GeoIP2 error messages with proper i18n support. TheAddressNotFoundErrormessage correctly includes a placeholder for dynamic IP address formatting, while other messages are appropriately static.openwisp_controller/config/whois/tests/utils.py (1)
60-70: LGTM! Simplified test setup.The direct device creation with public IP and subsequent mock reset streamlines the test flow. The cache interaction tests have been appropriately moved to the main test file where they can be tested with more granular assertions.
openwisp_controller/geo/estimated_location/tasks.py (2)
31-34: LGTM! Correct guard for existing location attachment.The updated condition properly checks that
existing_device_locationexists AND its location differs from the currentdevice_location.locationbefore proceeding with the attachment logic. This prevents unnecessary operations when locations are already identical.
67-79: Good fix for preventing unnecessary location recreation.This guard correctly addresses the reported bug where shared locations were being recreated despite identical WHOIS data. The check compares
geometryandnamewhich are the key identifying attributes derived from WHOIS coordinates and address data. The comparison logic handles all cases correctly, including whengeometryisNone(in cases where WHOIS coordinates are unavailable), since the equality comparisons work as expected.openwisp_controller/config/whois/tasks.py (1)
80-85: Cache success status updates look correct.Setting both the general and device-specific cache keys to "success" after a successful fetch allows future failures to trigger notifications again. This complements the on_failure logic well.
However, there's a subtle issue: on Lines 82-83 and 84-85, you're checking if the value isn't already "success" before setting it. But after a failure, the value would be "non_recoverable" or "error", not "success". The condition
!= "success"will be True in those cases, which is correct. But if you want to explicitly clear the error state and allow future error notifications, this approach works.openwisp_controller/geo/estimated_location/tests/tests.py (2)
155-155: Good addition of task name constant.Centralizing
_WHOIS_TASK_NAMEas a class attribute improves maintainability and ensures consistent cache key construction in tests.
189-212: Comprehensive cache interaction assertions.The test properly validates that:
- Cache is queried for existing state
- Organization config is cached with correct timeout
- Task-level and device-level success statuses are set
The use of
assert_has_callsallows verifying the expected calls without being overly strict about call order.openwisp_controller/config/whois/service.py (2)
68-81: Good defensive programming for timezone awareness.The explicit check for timezone-aware datetimes using
timezone.is_aware(dt)prevents subtle bugs that could occur with naive datetime comparisons. This is especially important when themodifiedfield comes from the database and could potentially be naive in misconfigured environments.The static analysis hint (TRY003) about long exception messages is a style preference. The inline message here is acceptable since it provides clear guidance for fixing the issue.
178-197: Robust handling of missing location data.The changes properly handle edge cases where WHOIS response lacks location information:
- Default
timezoneto empty string (matching non-nullable CharField requirement)coordinatesset toNonewhen lat/long are missing- Clean conditional with walrus operator for location check
The comment on Lines 178-180 clearly documents the rationale for the empty string default, which is helpful for future maintainers.
openwisp_controller/config/whois/tests/tests.py (8)
8-8: Imports and model loading look fine.Also applies to: 25-28
66-66: Comment-only change.
143-147: Good coverage for naive datetime guard.
391-391: Nice: shared task name constant avoids duplication.
397-408: Missing-coordinates scenario is well covered.
739-778: Notification assertions aligned with new caching behavior.
779-849: Cache status coverage is solid (per-task/per-device/success).
851-874: Retry mechanism test additions look good.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| @mock.patch.object(config_app_settings, "WHOIS_CONFIGURED", True) | ||
| @mock.patch(_ESTIMATED_LOCATION_INFO_LOGGER) | ||
| @mock.patch(_WHOIS_GEOIP_CLIENT) | ||
| def test_unchanged_whois_data_no_location_recreation( | ||
| self, | ||
| mock_client, | ||
| mock_info, | ||
| ): | ||
| """Ensure identical WHOIS results do not recreate a shared Location when | ||
| devices reuse the same IP.""" | ||
| connect_whois_handlers() | ||
| mocked_response = self._mocked_client_response() | ||
| mock_client.return_value.city.return_value = mocked_response | ||
| shared_ip = "20.49.19.19" | ||
| device1 = self._create_device( | ||
| name="device-a", | ||
| mac_address="00:11:22:33:44:55", | ||
| last_ip=shared_ip, | ||
| ) | ||
| device2 = self._create_device( | ||
| name="device-b", | ||
| mac_address="00:11:22:33:44:66", | ||
| last_ip=shared_ip, | ||
| ) | ||
| original_location = device1.devicelocation.location | ||
| self.assertEqual(original_location.pk, device2.devicelocation.location.pk) | ||
| location_count = Location.objects.count() | ||
| notification_count = notification_qs.count() | ||
|
|
||
| # Clear the last ip for both devices, so setting them again | ||
| # will trigger the WHOIS lookup flow. | ||
| for device in (device1, device2): | ||
| device.last_ip = "" | ||
| device.save(update_fields=["last_ip"]) | ||
| device.refresh_from_db() | ||
|
|
||
| # We set the same shared IP again. This simulates device fetching checksum. | ||
| for device in (device1, device2): | ||
| device.last_ip = shared_ip | ||
| device.save(update_fields=["last_ip"]) | ||
| device.refresh_from_db() | ||
|
|
||
| # The location object should remain unchanged since the WHOIS data is the same. | ||
| self.assertEqual(original_location.pk, device1.devicelocation.location.pk) | ||
| self.assertEqual( | ||
| device1.devicelocation.location.pk, device2.devicelocation.location.pk | ||
| ) | ||
| self.assertEqual(Location.objects.count(), location_count) | ||
| self.assertTrue(Location.objects.filter(pk=original_location.pk).exists()) | ||
| self.assertEqual(notification_qs.count(), notification_count) |
There was a problem hiding this comment.
Good regression test for the location recreation bug.
This test validates the fix reported by pandafy: identical WHOIS data should not trigger location recreation. The test:
- Creates two devices sharing the same IP and location
- Clears and re-sets
last_ipto trigger WHOIS flow - Verifies the original location object is preserved
However, static analysis correctly flags mock_info as unused. If the logger mock isn't being asserted against, consider removing it to clean up the test signature.
Proposed fix: Remove unused mock_info parameter
`@mock.patch.object`(config_app_settings, "WHOIS_CONFIGURED", True)
- `@mock.patch`(_ESTIMATED_LOCATION_INFO_LOGGER)
`@mock.patch`(_WHOIS_GEOIP_CLIENT)
def test_unchanged_whois_data_no_location_recreation(
self,
mock_client,
- mock_info,
):🧰 Tools
🪛 Ruff (0.14.14)
569-569: Unused method argument: mock_info
(ARG002)
🤖 Prompt for AI Agents
In `@openwisp_controller/geo/estimated_location/tests/tests.py` around lines 563 -
612, The test function test_unchanged_whois_data_no_location_recreation has an
unused mocked logger parameter mock_info introduced by
`@mock.patch`(_ESTIMATED_LOCATION_INFO_LOGGER); remove the unused mock to satisfy
static analysis by deleting the `@mock.patch`(_ESTIMATED_LOCATION_INFO_LOGGER)
decorator and removing mock_info from the test signature, ensuring the remaining
`@mock.patch`(_WHOIS_GEOIP_CLIENT) and its parameter (mock_client) remain in the
correct order.
|
I have tested this scenario #1197 (review), and my suspicion was correct. When the WHOIS record is stale and the device does not already have an estimated location, the logic does not create a Estimated location for the device. EDIT: The potential fix for this scenario will re-create the DeviceLocation object if user have deleted it manually whenever a WHOIS Lookup is performed. I don't think this is ideal. |
Signed-off-by: DragnEmperor <dragnemperor@gmail.com>
| with self.subTest(f"Cache populated by {first_error.__name__}"): | ||
| cache.clear() | ||
| trigger_error_and_assert_cached(first_error, 1) | ||
|
|
| for subsequent_error in permanent_errors: | ||
| if subsequent_error is first_error: | ||
| continue | ||
|
|
| self.assertEqual(notification_qs.count(), notification_count) | ||
| notification_qs.delete() | ||
| return device | ||
|
|
| errors.AuthenticationError, | ||
| errors.PermissionRequiredError, | ||
| ] | ||
|
|
| assert isinstance(mock_retry.call_args.kwargs["exc"], exception) | ||
| else: | ||
| self.assertEqual(mock_retry.call_count, 0) | ||
|
|
| location.longitude, location.latitude, srid=4326 | ||
| ) | ||
| timezone = location.time_zone or "" | ||
|
|
| self.assertEqual(original_location.pk, device2.devicelocation.location.pk) | ||
| location_count = Location.objects.count() | ||
| notification_count = notification_qs.count() | ||
|
|
| device.last_ip = "" | ||
| device.save(update_fields=["last_ip"]) | ||
| device.refresh_from_db() | ||
|
|
| device.last_ip = shared_ip | ||
| device.save(update_fields=["last_ip"]) | ||
| device.refresh_from_db() | ||
|
|
| **whois_obj._get_defaults_for_estimated_location(), | ||
| "organization_id": device.organization_id, | ||
| } | ||
|
|
nemesifier
changed the title
4 participants
Related to #1196, contains other changes suggested by different AI models, @pandafy @DragnEmperor feel free to pick what you think works.