[feature] Updating stale WHOIS records #1116

DragnEmperor · 2025-08-26T17:57:14Z

Checklist

I have read the OpenWISP Contributing Guidelines.
I have manually tested the changes proposed in this pull request.
I have written new test cases for new code and/or updated existing tests for changes to existing code.
I have updated the documentation.

Reference to Existing Issue

Description of Changes

Added functionality to update WHOIS records which are older than a threshold which is a configurable setting.
Since the main conditions are different from existing conditions of whois lookup, have defined new method for handling update.
Updated documentation and test cases.

devkapilbansal · 2025-08-29T16:24:52Z

docs/user/estimated-location.rst

+Overview
+--------
+
+The Estimated Location feature automatically creates or updates a device’s


I believe we need not to repeat the topic name. Same for other cases as well

Suggested change

The Estimated Location feature automatically creates or updates a device’s

This feature automatically creates or updates a device’s

devkapilbansal · 2025-08-29T16:27:48Z

docs/user/whois.rst

+This will be triggered for the same scenarios defined in `trigger
+conditions <whois_trigger_conditions_>`_ but among the conditions **only
+WHOIS enablement is required**.


This statement is not very clear

devkapilbansal · 2025-08-29T16:39:46Z

openwisp_controller/geo/estimated_location/tasks.py

+            )
+            return
+
+    def _handle_attach_existing_location(


Looks like this function has a lot of responsibilities, should we refactor this?

devkapilbansal · 2025-08-29T16:43:06Z

openwisp_controller/config/management/commands/clear_last_ip.py

+
+
+class Command(BaseCommand):
+    help = "Clear the last IP address, if set, of active devices of all organizations."


Suggested change

help = "Clear the last IP address, if set, of active devices of all organizations."

help = "Clears the last IP address (if set) for every active devices of all organizations."

devkapilbansal · 2025-08-29T16:44:42Z

openwisp_controller/config/management/commands/clear_last_ip.py

+                "Are you sure you want to do this?\n\n"
+                "Type 'yes' to continue, or 'no' to cancel: "
+            )
+            if input("".join(message)) != "yes":


Should we make it case-insensitive?

We can but since we have explicitly provided the choices, it shouldn't be confusing

devkapilbansal · 2025-08-29T16:45:03Z

openwisp_controller/config/management/commands/clear_last_ip.py

+                raise CommandError("Operation cancelled by user.")
+
+        devices = Device.objects.filter(_is_deactivated=False).only("last_ip")
+        # Filter devices that have no WHOIS information for their last IP


Suggested change

# Filter devices that have no WHOIS information for their last IP

# Filter out devices that have no WHOIS information for their last IP

It excludes the devices "having" WHOIS information. Will rephrase this for clarity

devkapilbansal · 2025-08-29T16:48:58Z

openwisp_controller/config/whois/service.py

+        if not self.is_valid_public_ip_address(new_ip):
+            return False
+
+        if not self.is_whois_enabled:
+            return False
+
+        return self.is_estimated_location_enabled


Suggested change

if not self.is_valid_public_ip_address(new_ip):

return False

if not self.is_whois_enabled:

return False

return self.is_estimated_location_enabled

return (

self.is_valid_public_ip_address(new_ip) and

self.is_whois_enabled and

self.is_estimated_location_enabled

)

I am following the same structure that we have done for other functions dealing with conditions within this class.
I think we decided to split the conditions in other functions to make them more readable.

devkapilbansal · 2025-08-29T16:56:01Z

openwisp_controller/config/whois/tasks.py

+        # Host is based on the db that is used to fetch the details.
+        # As we are using GeoLite2, 'geolite.info' host is used.
+        # Refer: https://geoip2.readthedocs.io/en/latest/#sync-web-service-example
+        ip_client = geoip2_webservice.Client(
+            account_id=app_settings.WHOIS_GEOIP_ACCOUNT,
+            license_key=app_settings.WHOIS_GEOIP_KEY,
+            host="geolite.info",


suggestion: This should be abstracted out for extensibility

devkapilbansal · 2025-08-29T16:58:10Z

openwisp_controller/config/whois/tasks.py

    **app_settings.API_TASK_RETRY_OPTIONS,
 )
-def fetch_whois_details(self, device_pk, initial_ip_address, new_ip_address):
+def fetch_whois_details(self, device_pk, initial_ip_address):


This function is quite big to follow, we should look to break it in smaller functions, if possible

codesankalp

I notice there are quite a few else conditions now just to handle updates. If the task is only about refreshing stale information, why not run it as a scheduled task in Celery? Would that cause any issues?

This way, we can avoid calling the update method in the WhoIs service for every checksum call or other device update operation.

codesankalp · 2025-08-26T18:42:33Z

docs/user/whois.rst

 A WHOIS lookup is triggered automatically when:

- A new device is registered.
+- A new device is registered or its last IP address is changed.


we can add it as a new pointer below

codesankalp · 2025-08-26T18:43:40Z

openwisp_controller/config/whois/service.py

+            - The WHOIS information of new ip is present and is not older than
+              14 days.


let's use the configured variable name

DragnEmperor · 2025-08-31T13:51:24Z

I notice there are quite a few else conditions now just to handle updates. If the task is only about refreshing stale information, why not run it as a scheduled task in Celery? Would that cause any issues?

This way, we can avoid calling the update method in the WhoIs service for every checksum call or other device update operation.

Hi Sankalp, initially the plan was to create a task but as discussed on the weekly call, it was decided that it should run only when required to avoid too many requests.

nemesifier · 2025-09-23T13:07:17Z

docs/user/estimated-location.rst

+Managing Older Estimated Locations
+----------------------------------
+
+Whenever location related fields in WHOIS record are updated as per


Suggested change

Whenever location related fields in WHOIS record are updated as per

Whenever location related fields in WHOIS records are updated as per

nemesifier · 2025-09-23T13:18:34Z

openwisp_controller/config/whois/service.py

        self.device = device

+    @staticmethod
+    def geoip_client():


What about?

Suggested change

def geoip_client():

def get_geoip_client():

For consistency.

nemesifier · 2025-09-23T13:19:59Z

openwisp_controller/config/whois/service.py

+            raise exc_type(message)
+        except requests.RequestException as e:
+            raise e
+


Let's remove this blank line

Suggested change

nemesifier · 2025-09-23T13:20:10Z

openwisp_controller/config/whois/service.py

+
+        try:
+            data = ip_client.city(ip_address=ip_address)
+


Let's remove this blank line

Suggested change

nemesifier · 2025-09-23T13:20:17Z

openwisp_controller/config/whois/service.py

+        the relevant information.
+        """
+        ip_client = self.geoip_client()
+


Let's remove this blank line

Suggested change

nemesifier · 2025-09-23T13:20:25Z

openwisp_controller/config/whois/service.py

+        ip_address = self.device.last_ip
+        if not self.is_valid_public_ip_address(ip_address):
+            return
+


Let's remove this blank line

Suggested change

nemesifier · 2025-09-23T13:35:12Z

openwisp_controller/geo/estimated_location/tasks.py

-    Existing location here means a location of another device whose last_ip matches
-    the given ip_address.
-    Does not alters the existing location if it is not estimated.
+def _create_estimated_location(device_location, location_defaults, ip_address):


I think we should make this function create or update the location, this would make the code more readable.

We have similar constructs in our code base, please see:

https://github.com/search?q=org%3Aopenwisp+get_or_create&type=code

Moreover, another pattern that we're striving to follow is to have most logic concentrated in either models or classes like the one you did for the whois service, and then have admin views, API views and celery tasks use the methods provided by in models.

nemesifier · 2025-09-23T13:37:06Z

openwisp_controller/geo/estimated_location/tasks.py

+                device_pk=device.pk,
+                notify_type="estimated_location_updated",
+                actor=current_location,
+            )


Ideally this logic would go in the create or update function/method.

codesankalp

I tested it and it is working as expected.

codesankalp · 2025-10-05T07:05:35Z

openwisp_controller/geo/estimated_location/tasks.py

+    # If there are multiple devices with same last_ip then we need to inform
+    # the user to resolve the conflict manually.


do we need this comment? logger error message seems to be clear.

codesankalp · 2025-10-05T07:11:32Z

openwisp_controller/geo/estimated_location/tasks.py

+    if existing_device_location:
+        existing_location = existing_device_location.location
+        # We need to remove existing estimated location of the device
+        # if it is not shared
+        if attached_devices_exists is False:
+            current_location.delete()
+        device_location.location = existing_location
+        device_location.full_clean()
+        device_location.save()
+        logger.info(
+            f"Estimated location saved successfully for {device.pk}"
+            f" for IP: {ip_address}"
+        )


Do we have a test asserting the number of queries?

Have added this in the latest commit for creation/updating related tests for estimate location

nemesifier

Thanks for adding assertNumQueries, it's always good to keep the query count in check because new contributors can inadvertently change ORM calls and cause query count to go banana.

I have a few comments below.

nemesifier · 2025-10-09T22:58:04Z

openwisp_controller/config/whois/tasks.py

        whois_instance = WHOISInfo(**whois_details)
        whois_instance.full_clean()
-        whois_instance.save()
+        whois_instance.save(force_insert=True)


one curiosity, why is this necessary?

Django hits an UPDATE query using the primary key ip_address to update any existing records, if any. Since we have checks to ensure we update existing record, if it exists, I think we can proceed with force_insert.

nemesifier · 2025-10-09T22:59:08Z

openwisp_controller/geo/estimated_location/tasks.py

        # We need to remove existing estimated location of the device
        # if it is not shared
-        if attached_devices_exists is False:
-            current_location.delete()


Looks like the comment above needs to be moved too.

nemesifier

@DragnEmperor the test failing due to email verification not being sent can be fixed with this: 260289a.

Added required checks for ensuring WHOIS records older than a certain Threshold (configurable) will be updated along with the coordinates. Formatted existing code to be consistent with current coding standards. Extend django-loci's location change form template Closes #1058 Signed-off-by: DragnEmperor <[email protected]>

DragnEmperor added this to [GSoC25] WHOIS Information and IP Address-Based Geolocation Aug 26, 2025

DragnEmperor moved this to In Progress in [GSoC25] WHOIS Information and IP Address-Based Geolocation Aug 26, 2025

DragnEmperor self-assigned this Aug 26, 2025

DragnEmperor added enhancement gsoc Part of a Google Summer of Code project labels Aug 26, 2025

DragnEmperor requested review from codesankalp and devkapilbansal August 27, 2025 12:32

DragnEmperor changed the title ~~[enhancement] Added updating older WHOIS records~~ [enhancement] Updating stale WHOIS records Aug 27, 2025

DragnEmperor force-pushed the issues/1034-fuzzy-location-creation branch from 0a1c1cc to 2c1dda6 Compare August 29, 2025 04:57

devkapilbansal requested changes Aug 29, 2025

View reviewed changes

DragnEmperor force-pushed the issues/1058-update-whois branch 2 times, most recently from ddcf823 to 55a77c2 Compare August 29, 2025 23:09

codesankalp reviewed Aug 31, 2025

View reviewed changes

DragnEmperor moved this from In Progress to Needs Review in [GSoC25] WHOIS Information and IP Address-Based Geolocation Sep 2, 2025

DragnEmperor force-pushed the issues/1034-fuzzy-location-creation branch 2 times, most recently from 7c909e4 to f934ae3 Compare September 9, 2025 16:12

Base automatically changed from issues/1034-fuzzy-location-creation to gsoc25-whois September 20, 2025 16:21

nemesifier changed the title ~~[enhancement] Updating stale WHOIS records~~ [feature] Updating stale WHOIS records Sep 20, 2025

DragnEmperor force-pushed the issues/1058-update-whois branch from df469a6 to 155ea0b Compare September 21, 2025 10:41

nemesifier reviewed Sep 23, 2025

View reviewed changes

codesankalp reviewed Oct 5, 2025

View reviewed changes

nemesifier reviewed Oct 9, 2025

View reviewed changes

DragnEmperor force-pushed the gsoc25-whois branch from dbb7885 to 141a1b1 Compare October 14, 2025 18:38

DragnEmperor force-pushed the issues/1058-update-whois branch 2 times, most recently from cf35f57 to 5eca0e0 Compare October 15, 2025 02:57

nemesifier reviewed Oct 16, 2025

View reviewed changes

DragnEmperor force-pushed the gsoc25-whois branch from 141a1b1 to 3f6ec02 Compare October 17, 2025 03:02

DragnEmperor force-pushed the issues/1058-update-whois branch from 005500f to 3346bfc Compare October 17, 2025 03:06

	The Estimated Location feature automatically creates or updates a device’s
	This feature automatically creates or updates a device’s



		class Command(BaseCommand):
		help = "Clear the last IP address, if set, of active devices of all organizations."

	help = "Clear the last IP address, if set, of active devices of all organizations."
	help = "Clears the last IP address (if set) for every active devices of all organizations."

	# Filter devices that have no WHOIS information for their last IP
	# Filter out devices that have no WHOIS information for their last IP

		- The WHOIS information of new ip is present and is not older than
		14 days.

	Whenever location related fields in WHOIS record are updated as per
	Whenever location related fields in WHOIS records are updated as per

		# If there are multiple devices with same last_ip then we need to inform
		# the user to resolve the conflict manually.

Uh oh!

[feature] Updating stale WHOIS records #1116

Are you sure you want to change the base?

[feature] Updating stale WHOIS records #1116

Uh oh!

Conversation

DragnEmperor commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Reference to Existing Issue

Description of Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codesankalp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DragnEmperor commented Aug 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codesankalp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DragnEmperor Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

DragnEmperor commented Aug 26, 2025 •

edited

Loading

DragnEmperor Oct 6, 2025 •

edited

Loading