Don't call save on devices, links, or LOSes unnecessarily

src/meshapi/tests/test_uisp_import.py

@@ -417,6 +417,9 @@ def test_update_link_many_changes(self, mock_get_last_seen):
         last_seen_date = datetime.datetime(2018, 11, 14, 15, 20, 32, 4000, tzinfo=tzutc())
         mock_get_last_seen.return_value = last_seen_date
 
+        # Link should have just one history entry from when we created it
+        self.assertEqual(1, len(self.link.history.all()))
+
         change_messages = update_link_from_uisp_data(
             self.link,
             uisp_link_id="fake-uisp-uuid2",
@@ -441,6 +444,22 @@ def test_update_link_many_changes(self, mock_get_last_seen):
             ],
         )
 
+        # Link should have a second entry, due to legitimate update
+        self.assertEqual(2, len(self.link.history.all()))
+
+        # Run the update again...
+        change_messages = update_link_from_uisp_data(
+            self.link,
+            uisp_link_id="fake-uisp-uuid2",
+            uisp_from_device=self.device1,
+            uisp_to_device=self.device3,
+            uisp_status=Link.LinkStatus.INACTIVE,
+        )
+
+        # And it SHOULD NOT make another history object
+        self.link.refresh_from_db()
+        self.assertEqual(2, len(self.link.history.all()))
+
     @patch("meshapi.util.uisp_import.update_objects.get_uisp_link_last_seen")
     def test_update_link_add_abandon_date(self, mock_get_last_seen):
         self.link.status = Link.LinkStatus.INACTIVE
@@ -749,6 +768,128 @@ def test_update_device_no_changes(self):
         self.assertEqual(change_messages, [])
 
 
+class TestUISPImportHandlersDontDuplicateHistory(TransactionTestCase):
+    def setUp(self) -> None:
+        self.node1 = Node(
+            network_number=1234,
+            status=Node.NodeStatus.ACTIVE,
+            type=Node.NodeType.STANDARD,
+            latitude=0,
+            longitude=0,
+        )
+        self.node1.save()
+
+        self.node2 = Node(
+            network_number=5678,
+            status=Node.NodeStatus.ACTIVE,
+            type=Node.NodeType.STANDARD,
+            latitude=0,
+            longitude=0,
+        )
+        self.node2.save()
+
+        self.building1 = Building(latitude=0, longitude=0, address_truth_sources=[])
+        self.building1.primary_node = self.node1
+        self.building1.save()
+
+        self.building2 = Building(latitude=0, longitude=0, address_truth_sources=[])
+        self.building2.primary_node = self.node2
+        self.building2.save()
+
+        self.device1 = Device(
+            node=self.node1,
+            status=Device.DeviceStatus.ACTIVE,
+            name="nycmesh-1234-dev1",
+            uisp_id="uisp-uuid1",
+        )
+        self.device1.save()
+
+        self.device2 = Device(
+            node=self.node2,
+            status=Device.DeviceStatus.ACTIVE,
+            name="nycmesh-5678-dev2",
+            uisp_id="uisp-uuid2",
+        )
+        self.device2.save()
+
+        self.link1 = Link(
+            from_device=self.device1,
+            to_device=self.device2,
+            status=Link.LinkStatus.ACTIVE,
+            type=Link.LinkType.FIVE_GHZ,
+            uisp_id="uisp-uuid1",
+        )
+        self.link1.save()
+
+    @patch("meshapi.util.uisp_import.sync_handlers.notify_admins_of_changes")
+    def test_import_and_sync_devices_and_ensure_history_does_not_duplicate(self, mock_notify_admins):
+
+        uisp_devices = [
+            {
+                "overview": {
+                    "status": "active",
+                    "createdAt": "2018-11-14T15:20:32.004Z",
+                    "lastSeen": "2024-08-12T02:04:35.335Z",
+                    "wirelessMode": "sta-ptmp",
+                },
+                "identification": {
+                    "id": "uisp-uuid1",
+                    "name": "nycmesh-1234-dev69",  # Gonna change dev1 to dev69
+                    "category": "wireless",
+                    "type": "airMax",
+                },
+            },
+            {
+                "overview": {
+                    "status": None,
+                    "createdAt": "2018-11-14T15:20:32.004Z",
+                    "lastSeen": "2024-08-12T02:04:35.335Z",
+                    "wirelessMode": "sta-ptmp",
+                },
+                "identification": {
+                    "id": "uisp-uuid2",
+                    "name": "nycmesh-5678-dev2",
+                    "category": "wireless",
+                    "type": "airMax",
+                },
+            },
+            {
+                "overview": {
+                    "status": "active",
+                    "createdAt": "2018-11-14T15:20:32.004Z",
+                    "lastSeen": "2024-08-12T02:04:35.335Z",
+                    "wirelessMode": "sta-ptmp",
+                },
+                "identification": {
+                    "id": "uisp-uuid100",
+                    "name": "nycmesh-1234-dev100",
+                    "category": "wireless",
+                    "type": "airMax",
+                },
+            },
+        ]
+
+        # Haven't updated yet
+        created_device = Device.objects.get(uisp_id="uisp-uuid1")
+        length_1 = len(created_device.history.all())
+        self.assertEqual(1, length_1)
+
+        import_and_sync_uisp_devices(uisp_devices)
+
+        # Legitimate update
+        created_device.refresh_from_db()
+        length_2 = len(created_device.history.all())
+        self.assertEqual(2, length_2)
+
+        # Run it again. Should be a noop
+        import_and_sync_uisp_devices(uisp_devices)
+
+        # Should not have made more history
+        created_device.refresh_from_db()
+        length_3 = len(created_device.history.all())
+        self.assertEqual(2, length_3)
+
+
 class TestUISPImportHandlers(TransactionTestCase):
     def setUp(self):
         self.node1 = Node(
@@ -1722,6 +1863,9 @@ def test_sync_links_with_los_update_existing(self):
         )
         los2.save()
 
+        # LOS should have one history entry from creation
+        self.assertEqual(1, len(los1.history.all()))
+
         sync_link_table_into_los_objects()
 
         los1.refresh_from_db()
@@ -1736,6 +1880,19 @@ def test_sync_links_with_los_update_existing(self):
         self.assertEqual(los2.source, LOS.LOSSource.EXISTING_LINK)
         self.assertEqual(los2.analysis_date, datetime.date.today())
 
+        # Get another entry from legitimite update
+        self.assertEqual(2, len(los1.history.all()))
+        self.assertEqual(2, len(los2.history.all()))
+
+        # Run the sync again...
+        sync_link_table_into_los_objects()
+
+        # SHOULD NOT have a third entry
+        los1.refresh_from_db()
+        los2.refresh_from_db()
+        self.assertEqual(2, len(los1.history.all()))
+        self.assertEqual(2, len(los2.history.all()))
+
     def test_sync_links_with_los_inactive_link(self):
         self.link1.status = Link.LinkStatus.INACTIVE
         self.link1.abandon_date = datetime.date(2024, 1, 2)

src/meshapi/util/uisp_import/sync_handlers.py

@@ -83,6 +83,9 @@ def import_and_sync_uisp_devices(uisp_devices: List[UISPDevice]) -> None:
             parse_uisp_datetime(uisp_device["overview"]["lastSeen"]) if uisp_device["overview"]["lastSeen"] else None
         )
 
+        # This block guards against most duplication by checking uisp-uuid against
+        # the uisp-uuids we already know about.
+        # Further avoidance of saving historical records is done in the update function
         with transaction.atomic():
             existing_devices: List[Device] = list(Device.objects.filter(uisp_id=uisp_uuid).select_for_update())
             if existing_devices:
@@ -129,6 +132,7 @@ def import_and_sync_uisp_devices(uisp_devices: List[UISPDevice]) -> None:
                 uisp_device["identification"]["model"], DEFAULT_SECTOR_WIDTH
             )
 
+            # Only when we're sure the sector doesn't exist do we save it
             sector = Sector(
                 **device_fields,
                 azimuth=guessed_compass_heading or DEFAULT_SECTOR_AZIMUTH,
@@ -286,6 +290,8 @@ def import_and_sync_uisp_links(uisp_links: List[UISPDataLink]) -> None:
                         notify_admins_of_changes(existing_link, change_list)
                 continue
 
+        # By now, we're reasonably sure the link doesn't exist, so go ahead and
+        # create it.
         link = Link(
             from_device=uisp_from_device,
             to_device=uisp_to_device,
@@ -355,18 +361,32 @@ def sync_link_table_into_los_objects() -> None:
 
             if len(existing_los_objects):
                 for existing_los in existing_los_objects:
+                    # Keep track of whether or not we actually changed anything,
+                    # so that we don't unnecessarily save later.
+                    changed_los = False
+
                     # Supersede manually annotated LOSes with their auto-generated counterparts,
                     # once they come online as links
                     if link.status == Link.LinkStatus.ACTIVE and existing_los.source == LOS.LOSSource.HUMAN_ANNOTATED:
                         existing_los.source = LOS.LOSSource.EXISTING_LINK
+                        changed_los = True
 
                     # Keep the LOS analysis date accurate for all that come from existing links
-                    if existing_los.source == LOS.LOSSource.EXISTING_LINK and link.last_functioning_date_estimate:
+                    if (
+                        existing_los.source == LOS.LOSSource.EXISTING_LINK
+                        and link.last_functioning_date_estimate
+                        and existing_los.analysis_date != link.last_functioning_date_estimate
+                    ):
                         existing_los.analysis_date = link.last_functioning_date_estimate
+                        changed_los = True
 
-                    existing_los.save()
+                    if changed_los:
+                        print("changed los")
+                        existing_los.save()
                 continue
 
+        # At this point, we're reasonably sure the LOS does not exist, so go ahead
+        # and create a new one.
         los = LOS(
             from_building=from_building,
             to_building=to_building,

src/meshapi/util/uisp_import/update_objects.py

@@ -80,7 +80,10 @@ def update_device_from_uisp_data(
         existing_device.abandon_date = uisp_last_seen.date()
         change_messages.append(f"Added missing abandon date of {existing_device.abandon_date} based on UISP last-seen")
 
-    existing_device.save()
+    # Only update the device if we actually changed anything to avoid making
+    # duplicate entries
+    if change_messages:
+        existing_device.save()
 
     if fire_device_deactivated_hook:
         hook_event.send(
@@ -102,12 +105,17 @@ def update_link_from_uisp_data(
 ) -> List[str]:
     change_messages = []
 
+    # We can't add change messages because they're super spammy,
+    # so use this to determine if we should save when changing the uisp_id.
+    uisp_link_id_changed = False
+
     if uisp_link_id != existing_link.uisp_id:
         existing_link.uisp_id = uisp_link_id
         logging.info(
             f"Changed UISP link ID to {uisp_link_id} for {existing_link} link (ID {existing_link.id}). "
             f"This is likely due to a UISP UUID rotation"
         )
+        uisp_link_id_changed = True
 
     uisp_device_pair = {uisp_to_device, uisp_from_device}
     db_device_pair = {existing_link.from_device, existing_link.to_device}
@@ -162,5 +170,8 @@ def update_link_from_uisp_data(
         existing_link.abandon_date = uisp_last_seen.date()
         change_messages.append(f"Added missing abandon date of {existing_link.abandon_date} based on UISP last-seen")
 
-    existing_link.save()
+    # Only save the object if there actually are change_messages, otherwise don't
+    # to avoid creating an unnecessary object.
+    if change_messages or uisp_link_id_changed:
+        existing_link.save()
     return change_messages