Improved dependency merge

Author: arthanson

netbox_branching/models/branches.py

@@ -532,8 +532,18 @@ def _collapse_changes_for_object(changes, logger):
         Collapse a list of ObjectChanges for a single object.
         Returns: (final_action, merged_data, last_change)
 
-        Simplified DELETE logic: If DELETE appears anywhere in the changes,
-        ignore all other changes and keep only the DELETE.
+        A key point is that we only care about the final state of the object. Also 
+        each ChangeObject needs to be correct so the final state is correct, i.e. 
+        if we delete an object, there aren't going to be other objects still referencing it.
+
+        ChangeObject can have CREATE, UPDATE, and DELETE actions.
+        We need to collapse these changes into a single action.
+        We can have the following cases:
+           - CREATE + (any updates) + DELETE = skip entirely
+           - (anything other than CREATE) + DELETE = DELETE
+           - CREATE + UPDATEs = CREATE
+           - multiple UPDATEs = UPDATE
+
         """
         if not changes:
             return None, None, None
@@ -616,106 +626,6 @@ def _get_fk_references(model_class, data, changed_objects):
 
         return references
 
-    @staticmethod
-    def _removed_reference_to(collapsed_change, target_key, logger):
-        """
-        Check if this collapsed change removed an FK reference to target_key.
-        Returns True if:
-        - The object previously referenced target_key (in its initial state)
-        - The final state does NOT reference target_key (or object is deleted)
-        """
-        if not collapsed_change.changes:
-            return False
-
-        target_ct_id, target_obj_id = target_key
-        first_change = collapsed_change.changes[0]
-
-        # If this object is being deleted, check if it referenced target initially
-        if collapsed_change.final_action == 'delete':
-            initial_refs = Branch._get_fk_references(
-                collapsed_change.model_class,
-                first_change.prechange_data or {},
-                {target_key}
-            )
-            return target_key in initial_refs
-
-        # If created in branch, it couldn't have had an initial reference
-        if first_change.action == 'create':
-            return False
-
-        # It's an update - check if FK reference changed
-        initial_state = first_change.prechange_data or {}
-        final_state = collapsed_change.merged_data or {}
-
-        # Check each FK field
-        for field in collapsed_change.model_class._meta.get_fields():
-            if isinstance(field, models.ForeignKey):
-                related_model = field.related_model
-                related_ct = ContentType.objects.get_for_model(related_model)
-
-                # Only check if this FK could point to our target
-                if related_ct.id != target_ct_id:
-                    continue
-
-                fk_field_name = field.attname  # e.g., 'device_id'
-                initial_value = initial_state.get(fk_field_name)
-                final_value = final_state.get(fk_field_name)
-
-                # Reference was removed or changed from target
-                if initial_value == target_obj_id and initial_value != final_value:
-                    logger.debug(f"    Found removed reference: {field.name} was {initial_value}, now {final_value}")
-                    return True
-
-        return False
-
-    @staticmethod
-    def _build_dependency_graph(collapsed_changes, logger):
-        """
-        Build dependency graph between collapsed changes.
-        Modifies collapsed_changes in place to set depends_on/depended_by.
-        """
-        logger.info("Building dependency graph...")
-
-        # 1. FK dependencies for creates/updates
-        #    If we CREATE/UPDATE object A with FK to object B,
-        #    and B is being created, then B must be created first
-        logger.debug("  Analyzing FK dependencies for creates/updates...")
-        for key, collapsed in collapsed_changes.items():
-            if collapsed.final_action in ('create', 'update'):
-                fk_refs = Branch._get_fk_references(
-                    collapsed.model_class,
-                    collapsed.merged_data,
-                    collapsed_changes.keys()
-                )
-
-                for ref_key in fk_refs:
-                    ref_collapsed = collapsed_changes[ref_key]
-                    # Only add dependency if the referenced object is being created
-                    if ref_collapsed.final_action == 'create':
-                        collapsed.depends_on.add(ref_key)
-                        ref_collapsed.depended_by.add(key)
-                        logger.debug(f"    {collapsed} depends on {ref_collapsed} (FK reference)")
-
-        # 2. Delete dependencies
-        #    If we DELETE object A, and object B removes its reference to A,
-        #    then B's change must happen before A's delete
-        logger.debug("  Analyzing dependencies for deletes...")
-        for key, collapsed in collapsed_changes.items():
-            if collapsed.final_action == 'delete':
-                # Find all changes that removed references to this object
-                for other_key, other_collapsed in collapsed_changes.items():
-                    if other_key == key:
-                        continue
-
-                    if Branch._removed_reference_to(other_collapsed, key, logger):
-                        # other_collapsed must happen before collapsed (the delete)
-                        collapsed.depends_on.add(other_key)
-                        other_collapsed.depended_by.add(key)
-                        logger.debug(f"    {collapsed} depends on {other_collapsed} (removed reference)")
-
-        total_deps = sum(len(c.depends_on) for c in collapsed_changes.values())
-        logger.info(f"  Dependency graph built: {total_deps} dependencies")
-
     @staticmethod
     def _topological_sort_with_cycle_detection(collapsed_changes, logger):
         """
@@ -767,7 +677,24 @@ def _topological_sort_with_cycle_detection(collapsed_changes, logger):
     @staticmethod
     def _order_collapsed_changes(collapsed_changes, logger):
         """
-        Order collapsed changes respecting dependencies and constraints.
+        Order collapsed changes respecting dependencies and time.
+
+        Algorithm:
+        1. Initial ordering by time: DELETEs, UPDATEs, CREATEs (each group sorted by time)
+        2. Build dependency graph:
+           - If UPDATE references deleted object in prechange_data → UPDATE must come before DELETE
+             (UPDATE removes the FK reference, allowing the DELETE to proceed)
+           - If UPDATE references created object in postchange_data → CREATE must come before UPDATE
+             (CREATE must exist before UPDATE can reference it)
+           - If CREATE references another created object in postchange_data → referenced CREATE must come first
+             (Referenced object must exist before referencing object is created)
+        3. Topological sort respecting dependencies
+
+        This ensures:
+        - DELETEs generally happen first to free unique constraints (time order within group)
+        - UPDATEs that remove FK references happen before their associated DELETEs
+        - CREATEs happen before UPDATEs/CREATEs that reference them
+
         Returns: ordered list of CollapsedChange objects
         """
         logger.info(f"Ordering {len(collapsed_changes)} collapsed changes...")
@@ -782,36 +709,106 @@ def _order_collapsed_changes(collapsed_changes, logger):
         if not to_process:
             return []
 
-        # Build dependency graph
-        Branch._build_dependency_graph(to_process, logger)
+        # Group by action and sort each group by time
+        deletes = sorted(
+            [v for v in to_process.values() if v.final_action == 'delete'],
+            key=lambda c: c.last_change.time
+        )
+        updates = sorted(
+            [v for v in to_process.values() if v.final_action == 'update'],
+            key=lambda c: c.last_change.time
+        )
+        creates = sorted(
+            [v for v in to_process.values() if v.final_action == 'create'],
+            key=lambda c: c.last_change.time
+        )
 
-        # Topological sort
-        ordered_keys = Branch._topological_sort_with_cycle_detection(to_process, logger)
+        logger.info(
+            f"  Initial time-based groups: {len(deletes)} deletes, "
+            f"{len(updates)} updates, {len(creates)} creates"
+        )
+
+        # Reset dependencies
+        for collapsed in to_process.values():
+            collapsed.depends_on = set()
+            collapsed.depended_by = set()
+
+        # Build lookup maps for efficient dependency checking
+        deletes_map = {c.key: c for c in deletes}
+        creates_map = {c.key: c for c in creates}
 
-        # Group by model and refine order within each model
-        logger.info("Refining order within models (deletes before creates to free unique constraints)...")
-        by_model = defaultdict(list)
-        for key in ordered_keys:
-            collapsed = to_process[key]
-            by_model[collapsed.model_class].append(collapsed)
-
-        # Within each model: updates, then deletes, then creates
-        # This ensures deletes free up unique constraints (like slugs) before creates claim them
-        result = []
-        for model_class, changes in by_model.items():
-            updates = [c for c in changes if c.final_action == 'update']
-            deletes = [c for c in changes if c.final_action == 'delete']
-            creates = [c for c in changes if c.final_action == 'create']
-
-            if updates or creates or deletes:
-                logger.debug(
-                    f"  {model_class.__name__}: {len(updates)} updates, "
-                    f"{len(deletes)} deletes, {len(creates)} creates"
+        logger.info("Building dependency graph...")
+
+        # 1. Check UPDATEs for dependencies
+        logger.debug("  Analyzing UPDATE dependencies...")
+        for update in updates:
+            # Check if UPDATE references deleted object in prechange_data
+            # This means the UPDATE had a reference that it's removing
+            # The UPDATE must happen BEFORE the DELETE so the FK reference is removed first
+            if update.changes[0].prechange_data:
+                prechange_refs = Branch._get_fk_references(
+                    update.model_class,
+                    update.changes[0].prechange_data,
+                    deletes_map.keys()
                 )
+                for ref_key in prechange_refs:
+                    # DELETE depends on UPDATE (UPDATE removes reference, then DELETE can proceed)
+                    delete_collapsed = deletes_map[ref_key]
+                    delete_collapsed.depends_on.add(update.key)
+                    update.depended_by.add(ref_key)
+                    logger.debug(
+                        f"    {delete_collapsed} depends on {update} "
+                        f"(UPDATE removes FK reference before DELETE)"
+                    )
+
+            # Check if UPDATE references created object in postchange_data
+            # This means the UPDATE needs the CREATE to exist first
+            # The CREATE must happen BEFORE the UPDATE
+            if update.merged_data:
+                postchange_refs = Branch._get_fk_references(
+                    update.model_class,
+                    update.merged_data,
+                    creates_map.keys()
+                )
+                for ref_key in postchange_refs:
+                    # UPDATE depends on CREATE
+                    create_collapsed = creates_map[ref_key]
+                    update.depends_on.add(ref_key)
+                    create_collapsed.depended_by.add(update.key)
+                    logger.debug(
+                        f"    {update} depends on {create_collapsed} "
+                        f"(UPDATE references created object)"
+                    )
+
+        # 2. Check CREATEs for dependencies on other CREATEs
+        logger.debug("  Analyzing CREATE dependencies...")
+        for create in creates:
+            if create.merged_data:
+                # Check if this CREATE references other created objects
+                refs = Branch._get_fk_references(
+                    create.model_class,
+                    create.merged_data,
+                    creates_map.keys()
+                )
+                for ref_key in refs:
+                    if ref_key != create.key:  # Don't self-reference
+                        # CREATE depends on another CREATE
+                        ref_create = creates_map[ref_key]
+                        create.depends_on.add(ref_key)
+                        ref_create.depended_by.add(create.key)
+                        logger.debug(
+                            f"    {create} depends on {ref_create} "
+                            f"(CREATE references another created object)"
+                        )
+
+        total_deps = sum(len(c.depends_on) for c in to_process.values())
+        logger.info(f"  Dependency graph built: {total_deps} dependencies")
+
+        # Topological sort to respect dependencies
+        ordered_keys = Branch._topological_sort_with_cycle_detection(to_process, logger)
 
-            result.extend(updates)
-            result.extend(deletes)
-            result.extend(creates)
+        # Convert keys back to collapsed changes
+        result = [to_process[key] for key in ordered_keys]
 
         logger.info(f"Ordering complete: {len(result)} changes to apply")
         return result