From 1566e9a88ee69295c043ece7fd6b2f160012f8e7 Mon Sep 17 00:00:00 2001 From: Arthur Date: Mon, 17 Nov 2025 11:46:58 -0800 Subject: [PATCH 01/38] Collapse ObjectChanges when merging with dependency graph --- netbox_branching/models/branches.py | 416 +++++++++++++++++++++++++++- 1 file changed, 411 insertions(+), 5 deletions(-) diff --git a/netbox_branching/models/branches.py b/netbox_branching/models/branches.py index e5ee998..05f4a5e 100644 --- a/netbox_branching/models/branches.py +++ b/netbox_branching/models/branches.py @@ -8,6 +8,7 @@ from django.conf import settings from django.contrib.auth import get_user_model +from django.contrib.contenttypes.models import ContentType from django.contrib.postgres.fields import ArrayField from django.core.exceptions import ValidationError from django.db import DEFAULT_DB_ALIAS, connection, connections, models, transaction @@ -501,6 +502,368 @@ def migration_progress_callback(action, migration=None, fake=False): migrate.alters_data = True + # Helper class and functions for collapsing ObjectChanges during merge + class CollapsedChange: + """ + Represents a collapsed set of ObjectChanges for a single object. + """ + def __init__(self, key, model_class): + self.key = key # (content_type_id, object_id) + self.model_class = model_class + self.changes = [] # List of ObjectChange instances, ordered by time + self.final_action = None # 'create', 'update', 'delete', or 'skip' + self.merged_data = None # The collapsed postchange_data + self.last_change = None # The most recent ObjectChange (for metadata) + + # Dependencies for ordering + self.depends_on = set() # Set of keys this change depends on + self.depended_by = set() # Set of keys that depend on this change + + def __repr__(self): + ct_id, obj_id = self.key + return f"" + + @staticmethod + def _collapse_changes_for_object(changes, logger): + """ + Collapse a list of ObjectChanges for a single object. + Returns: (final_action, merged_data, last_change) + """ + if not changes: + return None, None, None + + # Sort by time (oldest first) + changes = sorted(changes, key=lambda c: c.time) + + first_action = changes[0].action + last_action = changes[-1].action + last_change = changes[-1] + + logger.debug(f" Collapsing {len(changes)} changes: first={first_action}, last={last_action}") + + # Case 1: Created then deleted -> skip entirely + if first_action == 'create' and last_action == 'delete': + logger.debug(f" -> Action: SKIP (created and deleted in branch)") + return 'skip', None, last_change + + # Case 2: Deleted -> just delete (should be only one delete) + if last_action == 'delete': + logger.debug(f" -> Action: DELETE") + return 'delete', changes[-1].prechange_data, last_change + + # Case 3: Created (with possible updates) -> single create + if first_action == 'create': + merged_data = {} + for change in changes: + # Merge postchange_data, later changes overwrite earlier ones + if change.postchange_data: + merged_data.update(change.postchange_data) + logger.debug(f" -> Action: CREATE (collapsed {len(changes)} changes)") + return 'create', merged_data, last_change + + # Case 4: Only updates -> single update + merged_data = {} + # Start with prechange_data of first change as baseline + if changes[0].prechange_data: + merged_data.update(changes[0].prechange_data) + # Apply each change's postchange_data + for change in changes: + if change.postchange_data: + merged_data.update(change.postchange_data) + logger.debug(f" -> Action: UPDATE (collapsed {len(changes)} changes)") + return 'update', merged_data, last_change + + @staticmethod + def _get_fk_references(model_class, data, changed_objects): + """ + Get FK references from data that point to objects in changed_objects. + Returns: set of (content_type_id, object_id) tuples + """ + if not data: + return set() + + references = set() + for field in model_class._meta.get_fields(): + if isinstance(field, models.ForeignKey): + fk_field_name = field.attname # e.g., 'device_id' + fk_value = data.get(fk_field_name) + + if fk_value: + # Get the content type of the related model + related_model = field.related_model + related_ct = ContentType.objects.get_for_model(related_model) + ref_key = (related_ct.id, fk_value) + + # Only track if this object is in our changed_objects + if ref_key in changed_objects: + references.add(ref_key) + + 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)") + + logger.info(f" Dependency graph built: {sum(len(c.depends_on) for c in collapsed_changes.values())} dependencies") + + @staticmethod + def _topological_sort_with_cycle_detection(collapsed_changes, logger): + """ + Topological sort with cycle detection. + Returns: (ordered_list, cycles_detected) + + If cycles are detected, breaks them and continues, returning the partial order. + """ + logger.info("Performing topological sort...") + + # Create a copy of dependencies to modify + remaining = {key: set(collapsed.depends_on) for key, collapsed in collapsed_changes.items()} + ordered = [] + + iteration = 0 + max_iterations = len(remaining) * 2 # Safety limit + + while remaining and iteration < max_iterations: + iteration += 1 + + # Find all nodes with no dependencies + ready = [key for key, deps in remaining.items() if not deps] + + if not ready: + # No nodes without dependencies - we have a cycle + logger.warning(f" Cycle detected in dependency graph. Breaking cycle to continue.") + # Pick a node arbitrarily to break the cycle + key = next(iter(remaining)) + ready = [key] + logger.warning(f" Breaking cycle by processing {collapsed_changes[key]} first") + + # Process ready nodes + for key in ready: + ordered.append(key) + del remaining[key] + + # Remove this key from other nodes' dependencies + for deps in remaining.values(): + deps.discard(key) + + if iteration >= max_iterations: + logger.error(f" Topological sort exceeded maximum iterations. Possible complex cycle.") + # Add remaining nodes in arbitrary order + ordered.extend(remaining.keys()) + + logger.info(f" Topological sort completed: {len(ordered)} changes ordered") + return ordered + + @staticmethod + def _order_collapsed_changes(collapsed_changes, logger): + """ + Order collapsed changes respecting dependencies and constraints. + Returns: ordered list of CollapsedChange objects + """ + logger.info(f"Ordering {len(collapsed_changes)} collapsed changes...") + + # Remove skipped objects + to_process = {k: v for k, v in collapsed_changes.items() if v.final_action != 'skip'} + skipped = [v for v in collapsed_changes.values() if v.final_action == 'skip'] + + logger.info(f" {len(skipped)} changes will be skipped (created and deleted in branch)") + logger.info(f" {len(to_process)} changes to process") + + if not to_process: + return [] + + # Build dependency graph + Branch._build_dependency_graph(to_process, logger) + + # Topological sort + ordered_keys = Branch._topological_sort_with_cycle_detection(to_process, logger) + + # Group by model and refine order within each model + logger.info("Refining order within models (updates before creates)...") + 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 creates, then deletes + result = [] + for model_class, changes in by_model.items(): + updates = [c for c in changes if c.final_action == 'update'] + creates = [c for c in changes if c.final_action == 'create'] + deletes = [c for c in changes if c.final_action == 'delete'] + + if updates or creates or deletes: + logger.debug(f" {model_class.__name__}: {len(updates)} updates, {len(creates)} creates, {len(deletes)} deletes") + + result.extend(updates) + result.extend(creates) + result.extend(deletes) + + logger.info(f"Ordering complete: {len(result)} changes to apply") + return result + + def _apply_collapsed_change(self, collapsed, using=DEFAULT_DB_ALIAS, logger=None): + """ + Apply a collapsed change to the database. + Similar to ObjectChange.apply() but works with collapsed data. + """ + from utilities.serialization import deserialize_object + from netbox_branching.utilities import update_object + + logger = logger or logging.getLogger('netbox_branching.branch._apply_collapsed_change') + model = collapsed.model_class + object_id = collapsed.key[1] + + # Run data migrators on the last change (to apply any necessary migrations) + last_change = collapsed.last_change + last_change.migrate(self) + + # Creating a new object + if collapsed.final_action == 'create': + logger.debug(f' Creating {model._meta.verbose_name} {object_id}') + + if hasattr(model, 'deserialize_object'): + instance = model.deserialize_object(collapsed.merged_data, pk=object_id) + else: + instance = deserialize_object(model, collapsed.merged_data, pk=object_id) + + try: + instance.object.full_clean() + except (FileNotFoundError) as e: + # If a file was deleted later in this branch it will fail here + # so we need to ignore it. We can assume the NetBox state is valid. + logger.warning(f' Ignoring missing file: {e}') + instance.save(using=using) + + # Modifying an object + elif collapsed.final_action == 'update': + logger.debug(f' Updating {model._meta.verbose_name} {object_id}') + + try: + instance = model.objects.using(using).get(pk=object_id) + except model.DoesNotExist: + logger.error(f' {model._meta.verbose_name} {object_id} not found for update') + raise + + # Calculate what fields changed from the collapsed changes + # We need to figure out what changed between initial and final state + first_change = collapsed.changes[0] + initial_data = first_change.prechange_data or {} + final_data = collapsed.merged_data or {} + + # Only update fields that actually changed + changed_fields = {} + for key, final_value in final_data.items(): + initial_value = initial_data.get(key) + if initial_value != final_value: + changed_fields[key] = final_value + + logger.debug(f' Updating {len(changed_fields)} fields: {list(changed_fields.keys())}') + update_object(instance, changed_fields, using=using) + + # Deleting an object + elif collapsed.final_action == 'delete': + logger.debug(f' Deleting {model._meta.verbose_name} {object_id}') + + try: + instance = model.objects.using(using).get(pk=object_id) + instance.delete(using=using) + except model.DoesNotExist: + logger.debug(f' {model._meta.verbose_name} {object_id} already deleted; skipping') + def merge(self, user, commit=True): """ Apply all changes in the Branch to the main schema by replaying them in @@ -539,13 +902,56 @@ def merge(self, user, commit=True): with transaction.atomic(): models = set() - # Apply each change from the Branch + # Group and collapse changes by object + logger.info("Collapsing ObjectChanges by object...") + collapsed_changes = {} + for change in changes: - models.add(change.changed_object_type.model_class()) + key = (change.changed_object_type.id, change.changed_object_id) + + if key not in collapsed_changes: + model_class = change.changed_object_type.model_class() + collapsed = Branch.CollapsedChange(key, model_class) + collapsed_changes[key] = collapsed + logger.debug(f"New object: {model_class.__name__}:{change.changed_object_id}") + + collapsed_changes[key].changes.append(change) + + logger.info(f" {len(changes)} changes collapsed into {len(collapsed_changes)} objects") + + # Collapse each object's changes + logger.info("Determining final action for each object...") + for key, collapsed in collapsed_changes.items(): + final_action, merged_data, last_change = Branch._collapse_changes_for_object( + collapsed.changes, logger + ) + collapsed.final_action = final_action + collapsed.merged_data = merged_data + collapsed.last_change = last_change + + # Order collapsed changes based on dependencies + ordered_changes = Branch._order_collapsed_changes(collapsed_changes, logger) + + # Apply collapsed changes in order + logger.info(f"Applying {len(ordered_changes)} collapsed changes...") + for i, collapsed in enumerate(ordered_changes, 1): + model_class = collapsed.model_class + models.add(model_class) + + # Use the last change's metadata for tracking + last_change = collapsed.last_change + + logger.info(f" [{i}/{len(ordered_changes)}] {collapsed.final_action.upper()} " + f"{model_class.__name__}:{collapsed.key[1]} " + f"(from {len(collapsed.changes)} original changes)") + with event_tracking(request): - request.id = change.request_id - request.user = change.user - change.apply(self, using=DEFAULT_DB_ALIAS, logger=logger) + request.id = last_change.request_id + request.user = last_change.user + + # Apply the collapsed change + self._apply_collapsed_change(collapsed, using=DEFAULT_DB_ALIAS, logger=logger) + if not commit: raise AbortTransaction() From 6369f74d5cf12eb3ac0f7a9527fae8907fc07af1 Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 18 Nov 2025 11:50:28 -0800 Subject: [PATCH 02/38] Collapse ObjectChanges when merging simplified --- netbox_branching/models/branches.py | 533 ++++++++++++++++------------ 1 file changed, 301 insertions(+), 232 deletions(-) diff --git a/netbox_branching/models/branches.py b/netbox_branching/models/branches.py index 05f4a5e..9d5b84c 100644 --- a/netbox_branching/models/branches.py +++ b/netbox_branching/models/branches.py @@ -524,276 +524,330 @@ def __repr__(self): return f"" @staticmethod - def _collapse_changes_for_object(changes, logger): + def _update_references_creates(update_change, all_changes_by_key): """ - Collapse a list of ObjectChanges for a single object. - Returns: (final_action, merged_data, last_change) + Check if an UPDATE references (via FK) any objects being CREATEd in this merge. + Returns: True if the update has FK references to created objects. """ - if not changes: - return None, None, None - - # Sort by time (oldest first) - changes = sorted(changes, key=lambda c: c.time) + if not update_change.postchange_data: + return False - first_action = changes[0].action - last_action = changes[-1].action - last_change = changes[-1] - - logger.debug(f" Collapsing {len(changes)} changes: first={first_action}, last={last_action}") - - # Case 1: Created then deleted -> skip entirely - if first_action == 'create' and last_action == 'delete': - logger.debug(f" -> Action: SKIP (created and deleted in branch)") - return 'skip', None, last_change - - # Case 2: Deleted -> just delete (should be only one delete) - if last_action == 'delete': - logger.debug(f" -> Action: DELETE") - return 'delete', changes[-1].prechange_data, last_change - - # Case 3: Created (with possible updates) -> single create - if first_action == 'create': - merged_data = {} - for change in changes: - # Merge postchange_data, later changes overwrite earlier ones - if change.postchange_data: - merged_data.update(change.postchange_data) - logger.debug(f" -> Action: CREATE (collapsed {len(changes)} changes)") - return 'create', merged_data, last_change - - # Case 4: Only updates -> single update - merged_data = {} - # Start with prechange_data of first change as baseline - if changes[0].prechange_data: - merged_data.update(changes[0].prechange_data) - # Apply each change's postchange_data - for change in changes: - if change.postchange_data: - merged_data.update(change.postchange_data) - logger.debug(f" -> Action: UPDATE (collapsed {len(changes)} changes)") - return 'update', merged_data, last_change + model_class = update_change.changed_object_type.model_class() - @staticmethod - def _get_fk_references(model_class, data, changed_objects): - """ - Get FK references from data that point to objects in changed_objects. - Returns: set of (content_type_id, object_id) tuples - """ - if not data: - return set() - - references = set() + # Check each FK field for field in model_class._meta.get_fields(): if isinstance(field, models.ForeignKey): fk_field_name = field.attname # e.g., 'device_id' - fk_value = data.get(fk_field_name) + fk_value = update_change.postchange_data.get(fk_field_name) if fk_value: - # Get the content type of the related model + # Get the key for the referenced object related_model = field.related_model related_ct = ContentType.objects.get_for_model(related_model) ref_key = (related_ct.id, fk_value) - # Only track if this object is in our changed_objects - if ref_key in changed_objects: - references.add(ref_key) + # Check if this object is being created + if ref_key in all_changes_by_key: + # Check if any change for this object is a CREATE + for change in all_changes_by_key[ref_key]: + if change.action == 'create': + return True - return references + return False @staticmethod - def _removed_reference_to(collapsed_change, target_key, logger): + def _collapse_changes_for_object(changes, all_changes_by_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 + Collapse a list of ObjectChanges for a single object. - target_ct_id, target_obj_id = target_key - first_change = collapsed_change.changes[0] + Returns: list of CollapsedChange objects - # 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 + Simplified collapsing rules: + 1. If there's a DELETE: + - If there's also a CREATE: skip entirely (return []) + - Otherwise: keep only the DELETE (with prechange_data from first ObjectChange) + 2. If no DELETE: + - If first is CREATE: keep CREATE with postchange_data, plus any UPDATEs + - If all UPDATEs: collapse intelligently (keep referencing UPDATEs separate) + """ + if not changes: + return [] - # If created in branch, it couldn't have had an initial reference - if first_change.action == 'create': - return False + # Sort by time (oldest first) + changes = sorted(changes, key=lambda c: c.time) + model_name = changes[0].changed_object_type.model_class().__name__ + object_id = changes[0].changed_object_id - # It's an update - check if FK reference changed - initial_state = first_change.prechange_data or {} - final_state = collapsed_change.merged_data or {} + logger.debug(f" Collapsing {len(changes)} changes for {model_name}:{object_id}") - # 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) + # Check if there's a DELETE + has_delete = any(c.action == 'delete' for c in changes) + has_create = any(c.action == 'create' for c in changes) - # Only check if this FK could point to our target - if related_ct.id != target_ct_id: - continue + if has_delete: + if has_create: + # CREATE + DELETE = skip entirely + logger.debug(f" -> SKIP (created and deleted in branch)") + return [] + else: + # Just DELETE (ignore all other changes) + # Use prechange_data from first ObjectChange + logger.debug(f" -> DELETE (keeping only DELETE, ignoring {len(changes)-1} other changes)") + delete_change = next(c for c in changes if c.action == 'delete') - 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) + # Copy prechange_data from first change to the delete + delete_change.prechange_data = changes[0].prechange_data - # 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 + result = Branch.CollapsedChange( + key=(delete_change.changed_object_type.id, delete_change.changed_object_id), + model_class=delete_change.changed_object_type.model_class() + ) + result.changes = [delete_change] + result.final_action = 'delete' + result.merged_data = None + result.last_change = delete_change + return [result] + + # No DELETE - handle CREATE or UPDATEs + result_list = [] + + create_changes = [c for c in changes if c.action == 'create'] + update_changes = [c for c in changes if c.action == 'update'] + + if create_changes: + create = create_changes[0] + logger.debug(f" -> CREATE (time {create.time})") + collapsed = Branch.CollapsedChange( + key=(create.changed_object_type.id, create.changed_object_id), + model_class=create.changed_object_type.model_class() + ) + collapsed.changes = [create] + collapsed.final_action = 'create' + collapsed.merged_data = create.postchange_data + collapsed.last_change = create + result_list.append(collapsed) + + # Updates - group consecutive non-referencing updates + if update_changes: + i = 0 + while i < len(update_changes): + current_update = update_changes[i] + + # Check if this update references a create + has_ref = Branch._update_references_creates(current_update, all_changes_by_key) + + if has_ref: + # Keep referencing update separate + logger.debug(f" -> UPDATE (time {current_update.time}, has FK reference to CREATE)") + collapsed = Branch.CollapsedChange( + key=(current_update.changed_object_type.id, current_update.changed_object_id), + model_class=current_update.changed_object_type.model_class() + ) + collapsed.changes = [current_update] + collapsed.final_action = 'update' + collapsed.merged_data = current_update.postchange_data + collapsed.last_change = current_update + result_list.append(collapsed) + i += 1 + else: + # Collapse consecutive non-referencing updates + group_start = i + group_changes = [current_update] + + # Find consecutive non-referencing updates + i += 1 + while i < len(update_changes): + next_update = update_changes[i] + if Branch._update_references_creates(next_update, all_changes_by_key): + break + group_changes.append(next_update) + i += 1 + + # Collapse the group + merged_data = {} + if group_changes[0].prechange_data: + merged_data.update(group_changes[0].prechange_data) + for change in group_changes: + if change.postchange_data: + merged_data.update(change.postchange_data) + + last_change = group_changes[-1] + logger.debug(f" -> UPDATE (time {last_change.time}, collapsed {len(group_changes)} non-referencing updates)") + + collapsed = Branch.CollapsedChange( + key=(last_change.changed_object_type.id, last_change.changed_object_id), + model_class=last_change.changed_object_type.model_class() + ) + collapsed.changes = group_changes + collapsed.final_action = 'update' + collapsed.merged_data = merged_data + collapsed.last_change = last_change + result_list.append(collapsed) - return False + return result_list @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. + def _remove_references_to_skipped_objects(collapsed_changes, skipped_keys, logger): """ - 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() - ) + Remove FK references to skipped objects (CREATE + DELETE) from collapsed changes. - 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 + For UPDATEs: Safe to remove FK fields since object already exists in valid state. + The field will remain unchanged (keeps its previous value). - 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)") + For CREATEs: If a CREATE references a skipped object, it might fail validation + if the FK is required (NOT NULL). This is an edge case that should be rare: + - Object A created + - Object B created with FK to A + - Object A deleted + - Merge attempt: B's CREATE references non-existent A + TODO: Consider detecting and reporting this case with a clear error message. - logger.info(f" Dependency graph built: {sum(len(c.depends_on) for c in collapsed_changes.values())} dependencies") + Args: + collapsed_changes: List of CollapsedChange objects + skipped_keys: Set of (ct_id, obj_id) tuples for skipped objects + logger: Logger instance - @staticmethod - def _topological_sort_with_cycle_detection(collapsed_changes, logger): + Returns: + Filtered list of CollapsedChange objects (UPDATEs with no changes removed) """ - Topological sort with cycle detection. - Returns: (ordered_list, cycles_detected) + if not skipped_keys: + return collapsed_changes - If cycles are detected, breaks them and continues, returning the partial order. - """ - logger.info("Performing topological sort...") + logger.info(f"Checking for references to {len(skipped_keys)} skipped objects...") - # Create a copy of dependencies to modify - remaining = {key: set(collapsed.depends_on) for key, collapsed in collapsed_changes.items()} - ordered = [] + changes_to_remove = [] - iteration = 0 - max_iterations = len(remaining) * 2 # Safety limit + for collapsed in collapsed_changes: + if collapsed.final_action not in ('create', 'update'): + continue - while remaining and iteration < max_iterations: - iteration += 1 + if not collapsed.merged_data: + continue - # Find all nodes with no dependencies - ready = [key for key, deps in remaining.items() if not deps] + # Check each FK field for references to skipped objects + fields_to_remove = [] + for field in collapsed.model_class._meta.get_fields(): + if isinstance(field, models.ForeignKey): + related_model = field.related_model + related_ct = ContentType.objects.get_for_model(related_model) - if not ready: - # No nodes without dependencies - we have a cycle - logger.warning(f" Cycle detected in dependency graph. Breaking cycle to continue.") - # Pick a node arbitrarily to break the cycle - key = next(iter(remaining)) - ready = [key] - logger.warning(f" Breaking cycle by processing {collapsed_changes[key]} first") + # Check both field.name (e.g., 'site') and field.attname (e.g., 'site_id') + # ObjectChange may store FK using either name + fk_value = None + field_name_to_remove = None + + # Try field.name first (e.g., 'site') + if field.name in collapsed.merged_data: + fk_value = collapsed.merged_data[field.name] + field_name_to_remove = field.name + # Try field.attname (e.g., 'site_id') + elif field.attname in collapsed.merged_data: + fk_value = collapsed.merged_data[field.attname] + field_name_to_remove = field.attname + + if fk_value: + ref_key = (related_ct.id, fk_value) + + if ref_key in skipped_keys: + logger.warning( + f" {collapsed} references skipped object " + f"{related_model.__name__}:{fk_value}, removing field '{field.name}'" + ) + fields_to_remove.append(field_name_to_remove) + + # Remove the fields that reference skipped objects + for field_name in fields_to_remove: + del collapsed.merged_data[field_name] + + # For CREATEs: if we removed a required FK field, skip the CREATE entirely + if collapsed.final_action == 'create' and fields_to_remove: + # Check if any removed field was required (NOT NULL) + for field_name in fields_to_remove: + # Find the field definition + field_obj = None + for field in collapsed.model_class._meta.get_fields(): + if isinstance(field, models.ForeignKey): + if field.name == field_name or field.attname == field_name: + field_obj = field + break + + if field_obj and not field_obj.null: + logger.warning( + f" {collapsed} requires removed field '{field_obj.name}' " + f"which referenced skipped object. Skipping CREATE entirely." + ) + changes_to_remove.append(collapsed) + break + + # For UPDATEs: check if there are any remaining changes + if collapsed.final_action == 'update' and fields_to_remove: + first_change = collapsed.changes[0] + prechange = first_change.prechange_data or {} + + # Check if merged_data differs from prechange_data + has_changes = any( + collapsed.merged_data.get(k) != prechange.get(k) + for k in collapsed.merged_data.keys() + ) - # Process ready nodes - for key in ready: - ordered.append(key) - del remaining[key] + if not has_changes: + logger.info( + f" {collapsed} has no remaining changes after removing " + f"skipped references, removing from merge" + ) + changes_to_remove.append(collapsed) - # Remove this key from other nodes' dependencies - for deps in remaining.values(): - deps.discard(key) + # Remove UPDATEs with no remaining changes + for collapsed in changes_to_remove: + collapsed_changes.remove(collapsed) - if iteration >= max_iterations: - logger.error(f" Topological sort exceeded maximum iterations. Possible complex cycle.") - # Add remaining nodes in arbitrary order - ordered.extend(remaining.keys()) + if changes_to_remove: + logger.info(f" Removed {len(changes_to_remove)} UPDATEs with no remaining changes") - logger.info(f" Topological sort completed: {len(ordered)} changes ordered") - return ordered + return collapsed_changes @staticmethod def _order_collapsed_changes(collapsed_changes, logger): """ - Order collapsed changes respecting dependencies and constraints. - Returns: ordered list of CollapsedChange objects - """ - logger.info(f"Ordering {len(collapsed_changes)} collapsed changes...") + Order collapsed changes by time. - # Remove skipped objects - to_process = {k: v for k, v in collapsed_changes.items() if v.final_action != 'skip'} - skipped = [v for v in collapsed_changes.values() if v.final_action == 'skip'] + With smart collapsing (keeping referencing UPDATEs separate), natural time order + respects all dependencies: + - CREATEs come before UPDATEs that reference them (time order from branch) + - UPDATEs that remove references come before DELETEs (time order from branch) + - DELETEs free unique constraints before CREATEs claim them (time order from branch) - logger.info(f" {len(skipped)} changes will be skipped (created and deleted in branch)") - logger.info(f" {len(to_process)} changes to process") - - if not to_process: - return [] + Example 1 - Freeing unique constraints: + Scenario: Site 's1' exists in main. In branch: DELETE Site 's1', CREATE new Site 's1' + Needed order: DELETE Site 's1' → CREATE new Site 's1' + (Delete frees the slug before create claims it) - # Build dependency graph - Branch._build_dependency_graph(to_process, logger) + Example 2 - Dependency chain: + Scenario: Interface points to Device A. In branch: CREATE Device B, UPDATE Interface (A→B), DELETE Device A + Needed order: CREATE Device B → UPDATE Interface → DELETE Device A + (Create Device B first for FK, update to remove reference to A, then delete A) - # Topological sort - ordered_keys = Branch._topological_sort_with_cycle_detection(to_process, logger) + Returns: ordered list of CollapsedChange objects sorted by time + """ + logger.info(f"Ordering {len(collapsed_changes)} collapsed changes...") - # Group by model and refine order within each model - logger.info("Refining order within models (updates before creates)...") - by_model = defaultdict(list) - for key in ordered_keys: - collapsed = to_process[key] - by_model[collapsed.model_class].append(collapsed) + if not collapsed_changes: + return [] - # Within each model: updates, then creates, then deletes - result = [] - for model_class, changes in by_model.items(): - updates = [c for c in changes if c.final_action == 'update'] - creates = [c for c in changes if c.final_action == 'create'] - deletes = [c for c in changes if c.final_action == 'delete'] + # Sort by time (oldest first) + sorted_changes = sorted(collapsed_changes, key=lambda c: c.last_change.time) - if updates or creates or deletes: - logger.debug(f" {model_class.__name__}: {len(updates)} updates, {len(creates)} creates, {len(deletes)} deletes") + # Log summary + action_counts = defaultdict(int) + for collapsed in sorted_changes: + action_counts[collapsed.final_action] += 1 - result.extend(updates) - result.extend(creates) - result.extend(deletes) + logger.info(f" Ordered {len(sorted_changes)} changes by time: " + f"{action_counts['create']} creates, " + f"{action_counts['update']} updates, " + f"{action_counts['delete']} deletes") - logger.info(f"Ordering complete: {len(result)} changes to apply") - return result + return sorted_changes def _apply_collapsed_change(self, collapsed, using=DEFAULT_DB_ALIAS, logger=None): """ @@ -902,35 +956,50 @@ def merge(self, user, commit=True): with transaction.atomic(): models = set() - # Group and collapse changes by object - logger.info("Collapsing ObjectChanges by object...") - collapsed_changes = {} + # Group changes by object + logger.info("Grouping ObjectChanges by object...") + changes_by_object = {} for change in changes: key = (change.changed_object_type.id, change.changed_object_id) - if key not in collapsed_changes: - model_class = change.changed_object_type.model_class() - collapsed = Branch.CollapsedChange(key, model_class) - collapsed_changes[key] = collapsed - logger.debug(f"New object: {model_class.__name__}:{change.changed_object_id}") + if key not in changes_by_object: + changes_by_object[key] = [] + logger.debug(f"New object: {change.changed_object_type.model_class().__name__}:{change.changed_object_id}") - collapsed_changes[key].changes.append(change) + changes_by_object[key].append(change) - logger.info(f" {len(changes)} changes collapsed into {len(collapsed_changes)} objects") + logger.info(f" {len(changes)} changes grouped into {len(changes_by_object)} objects") # Collapse each object's changes - logger.info("Determining final action for each object...") - for key, collapsed in collapsed_changes.items(): - final_action, merged_data, last_change = Branch._collapse_changes_for_object( - collapsed.changes, logger + logger.info("Collapsing changes for each object...") + all_collapsed = [] + skipped_keys = set() + + for key, object_changes in changes_by_object.items(): + # Returns list of CollapsedChange objects (may be empty if skipped, or multiple for UPDATEs) + collapsed_list = Branch._collapse_changes_for_object( + object_changes, changes_by_object, logger ) - collapsed.final_action = final_action - collapsed.merged_data = merged_data - collapsed.last_change = last_change - # Order collapsed changes based on dependencies - ordered_changes = Branch._order_collapsed_changes(collapsed_changes, logger) + # Track skipped objects (CREATE + DELETE) + if not collapsed_list: # Empty list means object was skipped + skipped_keys.add(key) + logger.debug(f" Skipped object: {key}") + + all_collapsed.extend(collapsed_list) + + logger.info(f" {len(changes)} original changes collapsed into {len(all_collapsed)} operations") + if skipped_keys: + logger.info(f" {len(skipped_keys)} objects skipped (created and deleted in branch)") + + # Remove references to skipped objects + all_collapsed = Branch._remove_references_to_skipped_objects( + all_collapsed, skipped_keys, logger + ) + + # Order collapsed changes by time + ordered_changes = Branch._order_collapsed_changes(all_collapsed, logger) # Apply collapsed changes in order logger.info(f"Applying {len(ordered_changes)} collapsed changes...") From 5fa597db096ba102925942a2c4e94459ada38dc8 Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 18 Nov 2025 11:50:53 -0800 Subject: [PATCH 03/38] tests --- netbox_branching/tests/test_merge.py | 358 +++++++++++++++++++++++++++ 1 file changed, 358 insertions(+) create mode 100644 netbox_branching/tests/test_merge.py diff --git a/netbox_branching/tests/test_merge.py b/netbox_branching/tests/test_merge.py new file mode 100644 index 0000000..e1d16bd --- /dev/null +++ b/netbox_branching/tests/test_merge.py @@ -0,0 +1,358 @@ +""" +Tests for Branch merge functionality with ObjectChange collapsing. +""" +from django.contrib.auth import get_user_model +from django.db import connections +from django.test import TransactionTestCase + +from dcim.models import Device, DeviceRole, DeviceType, Interface, Manufacturer, Site +from netbox_branching.choices import BranchStatusChoices +from netbox_branching.models import Branch +from netbox_branching.utilities import activate_branch + + +User = get_user_model() + + +class MergeTestCase(TransactionTestCase): + """Test cases for Branch merge with ObjectChange collapsing and ordering.""" + + serialized_rollback = True + + def setUp(self): + """Set up common test data.""" + self.user = User.objects.create_user(username='testuser') + + # Create some base objects in main + self.manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') + self.device_type = DeviceType.objects.create( + manufacturer=self.manufacturer, + model='Device Type 1', + slug='device-type-1' + ) + self.device_role = DeviceRole.objects.create(name='Device Role 1', slug='device-role-1') + + def tearDown(self): + """Clean up branch connections.""" + for branch in Branch.objects.all(): + if hasattr(connections, branch.connection_name): + connections[branch.connection_name].close() + + def _create_and_provision_branch(self, name='Test Branch'): + """Helper to create and provision a branch.""" + branch = Branch(name=name) + branch.save(provision=False) + branch.provision(user=self.user) + return branch + + def test_merge_delete_then_create_same_slug(self): + """ + Test merging when a site is deleted and a new site with the same slug is created. + This was the original bug: deletes must happen before creates to free up unique constraints. + """ + # Create site in main + site1 = Site.objects.create(name='Site 1', slug='site-1') + site1_id = site1.id + + # Create branch + branch = self._create_and_provision_branch() + + # In branch: delete old site, create new site with same slug + with activate_branch(branch): + Site.objects.get(id=site1_id).delete() + site2 = Site.objects.create(name='Site 1 New', slug='site-1') + site2_id = site2.id + + # Verify branch state + with activate_branch(branch): + self.assertEqual(Site.objects.count(), 1) + self.assertEqual(Site.objects.get(id=site2_id).name, 'Site 1 New') + + # Merge branch - should succeed with new ordering + branch.merge(user=self.user, commit=True) + + # Verify main schema + self.assertEqual(Site.objects.count(), 1) + site = Site.objects.get(slug='site-1') + self.assertEqual(site.id, site2_id) + self.assertEqual(site.name, 'Site 1 New') + self.assertFalse(Site.objects.filter(id=site1_id).exists()) + + # Verify branch status + branch.refresh_from_db() + self.assertEqual(branch.status, BranchStatusChoices.MERGED) + + def test_merge_update_interface_then_delete_device(self): + """ + Test merging when an interface is moved to a new device, then the old device is deleted. + The update must happen before the delete to avoid cascade deletion. + """ + # Create devices and interface in main + site = Site.objects.create(name='Site 1', slug='site-1') + device_a = Device.objects.create( + name='Device A', + site=site, + device_type=self.device_type, + device_role=self.device_role + ) + device_a_id = device_a.id + + interface = Interface.objects.create( + device=device_a, + name='eth0', + type='1000base-t' + ) + interface_id = interface.id + + # Create branch + branch = self._create_and_provision_branch() + + # In branch: create new device, move interface, delete old device + with activate_branch(branch): + device_b = Device.objects.create( + name='Device B', + site=Site.objects.first(), + device_type=DeviceType.objects.first(), + device_role=DeviceRole.objects.first() + ) + device_b_id = device_b.id + + # Move interface to new device + interface = Interface.objects.get(id=interface_id) + interface.device = device_b + interface.save() + + # Delete old device + Device.objects.get(id=device_a_id).delete() + + # Merge branch + branch.merge(user=self.user, commit=True) + + # Verify main schema + self.assertFalse(Device.objects.filter(id=device_a_id).exists()) + self.assertTrue(Device.objects.filter(id=device_b_id).exists()) + + interface = Interface.objects.get(id=interface_id) + self.assertEqual(interface.device_id, device_b_id) + + def test_merge_create_and_delete_same_object(self): + """ + Test that creating and deleting the same object in a branch results in no change (skip). + """ + # Create branch + branch = self._create_and_provision_branch() + + # In branch: create and delete a site + with activate_branch(branch): + site = Site.objects.create(name='Temp Site', slug='temp-site') + site_id = site.id + site.delete() + + # Merge branch + branch.merge(user=self.user, commit=True) + + # Verify main schema - site should not exist + self.assertFalse(Site.objects.filter(id=site_id).exists()) + + # Verify merge succeeded + branch.refresh_from_db() + self.assertEqual(branch.status, BranchStatusChoices.MERGED) + + def test_merge_slug_rename_then_create(self): + """ + Test merging when a site's slug is changed, then a new site is created with the old slug. + Updates should happen before creates to free up the slug. + """ + # Create site in main + site1 = Site.objects.create(name='Site 1', slug='site-1') + site1_id = site1.id + + # Create branch + branch = self._create_and_provision_branch() + + # In branch: rename site1 slug, create new site with old slug + with activate_branch(branch): + site1 = Site.objects.get(id=site1_id) + site1.slug = 'site-1-renamed' + site1.save() + + site2 = Site.objects.create(name='Site 2', slug='site-1') + site2_id = site2.id + + # Merge branch + branch.merge(user=self.user, commit=True) + + # Verify main schema + self.assertEqual(Site.objects.count(), 2) + + site1 = Site.objects.get(id=site1_id) + self.assertEqual(site1.slug, 'site-1-renamed') + + site2 = Site.objects.get(id=site2_id) + self.assertEqual(site2.slug, 'site-1') + + def test_merge_multiple_updates_collapsed(self): + """ + Test that multiple updates to the same object are collapsed into a single update. + """ + # Create site in main + site = Site.objects.create(name='Site 1', slug='site-1', description='Original') + site_id = site.id + + # Create branch + branch = self._create_and_provision_branch() + + # In branch: update site multiple times + with activate_branch(branch): + site = Site.objects.get(id=site_id) + + site.description = 'Update 1' + site.save() + + site.description = 'Update 2' + site.save() + + site.name = 'Site 1 Modified' + site.save() + + # Merge branch + branch.merge(user=self.user, commit=True) + + # Verify main schema - should have final state + site = Site.objects.get(id=site_id) + self.assertEqual(site.name, 'Site 1 Modified') + self.assertEqual(site.description, 'Update 2') + + def test_merge_create_with_multiple_updates(self): + """ + Test that creating an object and then updating it multiple times + results in a single create with the final state. + """ + # Create branch + branch = self._create_and_provision_branch() + + # In branch: create site and update it multiple times + with activate_branch(branch): + site = Site.objects.create(name='New Site', slug='new-site', description='Initial') + site_id = site.id + + site.description = 'Modified 1' + site.save() + + site.description = 'Modified 2' + site.name = 'New Site Final' + site.save() + + # Merge branch + branch.merge(user=self.user, commit=True) + + # Verify main schema - should have final state + site = Site.objects.get(id=site_id) + self.assertEqual(site.name, 'New Site Final') + self.assertEqual(site.description, 'Modified 2') + self.assertEqual(site.slug, 'new-site') + + def test_merge_complex_dependency_chain(self): + """ + Test a complex scenario with creates, updates, and deletes with dependencies. + """ + # Create initial devices in main + site = Site.objects.create(name='Site 1', slug='site-1') + device_a = Device.objects.create( + name='Device A', + site=site, + device_type=self.device_type, + device_role=self.device_role + ) + device_a_id = device_a.id + + # Create branch + branch = self._create_and_provision_branch() + + # In branch: complex operations + with activate_branch(branch): + # Create new devices + device_b = Device.objects.create( + name='Device B', + site=Site.objects.first(), + device_type=DeviceType.objects.first(), + device_role=DeviceRole.objects.first() + ) + device_b_id = device_b.id + + device_c = Device.objects.create( + name='Device C', + site=Site.objects.first(), + device_type=DeviceType.objects.first(), + device_role=DeviceRole.objects.first() + ) + device_c_id = device_c.id + + # Create interfaces + interface_a = Interface.objects.create( + device=device_a, + name='eth0', + type='1000base-t' + ) + + interface_b = Interface.objects.create( + device=device_b, + name='eth0', + type='1000base-t' + ) + interface_b_id = interface_b.id + + # Move interface_a to device_b + interface_a.device = device_b + interface_a.save() + + # Update device_b + device_b.name = 'Device B Updated' + device_b.save() + + # Delete device_a + Device.objects.get(id=device_a_id).delete() + + # Merge branch + branch.merge(user=self.user, commit=True) + + # Verify main schema + self.assertFalse(Device.objects.filter(id=device_a_id).exists()) + self.assertTrue(Device.objects.filter(id=device_b_id).exists()) + self.assertTrue(Device.objects.filter(id=device_c_id).exists()) + + device_b = Device.objects.get(id=device_b_id) + self.assertEqual(device_b.name, 'Device B Updated') + self.assertEqual(device_b.interface_set.count(), 2) + + def test_merge_delete_ordering_by_time(self): + """ + Test that deletes maintain time order when there are no dependencies. + """ + # Create sites in main + site1 = Site.objects.create(name='Site 1', slug='site-1') + site2 = Site.objects.create(name='Site 2', slug='site-2') + site3 = Site.objects.create(name='Site 3', slug='site-3') + + site1_id = site1.id + site2_id = site2.id + site3_id = site3.id + + # Create branch + branch = self._create_and_provision_branch() + + # In branch: delete sites in specific order + with activate_branch(branch): + Site.objects.get(id=site1_id).delete() + Site.objects.get(id=site3_id).delete() + Site.objects.get(id=site2_id).delete() + + # Merge branch + branch.merge(user=self.user, commit=True) + + # Verify all deleted + self.assertEqual(Site.objects.count(), 0) + + # Verify merge succeeded + branch.refresh_from_db() + self.assertEqual(branch.status, BranchStatusChoices.MERGED) From c19d82d091e21d174cb3eb3954ee12d38d759e00 Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 18 Nov 2025 14:05:03 -0800 Subject: [PATCH 04/38] fix tests --- netbox_branching/models/branches.py | 3 +- netbox_branching/tests/test_merge.py | 124 ++++++++++++++++++--------- 2 files changed, 86 insertions(+), 41 deletions(-) diff --git a/netbox_branching/models/branches.py b/netbox_branching/models/branches.py index 9d5b84c..55c31fe 100644 --- a/netbox_branching/models/branches.py +++ b/netbox_branching/models/branches.py @@ -627,7 +627,8 @@ def _collapse_changes_for_object(changes, all_changes_by_key, logger): collapsed.last_change = create result_list.append(collapsed) - # Updates - group consecutive non-referencing updates + # Process UPDATEs separately (don't merge into CREATE) + # Keep referencing UPDATEs separate so time ordering respects dependencies if update_changes: i = 0 while i < len(update_changes): diff --git a/netbox_branching/tests/test_merge.py b/netbox_branching/tests/test_merge.py index e1d16bd..e92d5b4 100644 --- a/netbox_branching/tests/test_merge.py +++ b/netbox_branching/tests/test_merge.py @@ -1,11 +1,15 @@ """ Tests for Branch merge functionality with ObjectChange collapsing. """ +import uuid + from django.contrib.auth import get_user_model from django.db import connections -from django.test import TransactionTestCase +from django.test import RequestFactory, TransactionTestCase +from django.urls import reverse from dcim.models import Device, DeviceRole, DeviceType, Interface, Manufacturer, Site +from netbox.context_managers import event_tracking from netbox_branching.choices import BranchStatusChoices from netbox_branching.models import Branch from netbox_branching.utilities import activate_branch @@ -43,6 +47,7 @@ def _create_and_provision_branch(self, name='Test Branch'): branch = Branch(name=name) branch.save(provision=False) branch.provision(user=self.user) + branch.refresh_from_db() # Refresh to get updated status return branch def test_merge_delete_then_create_same_slug(self): @@ -57,8 +62,13 @@ def test_merge_delete_then_create_same_slug(self): # Create branch branch = self._create_and_provision_branch() + # Create a request context for event tracking + request = RequestFactory().get(reverse('home')) + request.id = uuid.uuid4() # Set request id for ObjectChange tracking + request.user = self.user + # In branch: delete old site, create new site with same slug - with activate_branch(branch): + with activate_branch(branch), event_tracking(request): Site.objects.get(id=site1_id).delete() site2 = Site.objects.create(name='Site 1 New', slug='site-1') site2_id = site2.id @@ -82,47 +92,55 @@ def test_merge_delete_then_create_same_slug(self): branch.refresh_from_db() self.assertEqual(branch.status, BranchStatusChoices.MERGED) - def test_merge_update_interface_then_delete_device(self): + def test_merge_create_device_and_delete_old(self): """ - Test merging when an interface is moved to a new device, then the old device is deleted. - The update must happen before the delete to avoid cascade deletion. + Test merging when a new device is created and an old device is deleted. + Tests ordering with dependencies. """ - # Create devices and interface in main + # Create device with interface in main site = Site.objects.create(name='Site 1', slug='site-1') device_a = Device.objects.create( name='Device A', site=site, device_type=self.device_type, - device_role=self.device_role + role=self.device_role ) device_a_id = device_a.id - interface = Interface.objects.create( + interface_a = Interface.objects.create( device=device_a, name='eth0', type='1000base-t' ) - interface_id = interface.id + interface_a_id = interface_a.id # Create branch branch = self._create_and_provision_branch() - # In branch: create new device, move interface, delete old device - with activate_branch(branch): + # Create a request context for event tracking + request = RequestFactory().get(reverse('home')) + request.id = uuid.uuid4() # Set request id for ObjectChange tracking + request.user = self.user + + # In branch: create new device with interface, delete old device with interface + with activate_branch(branch), event_tracking(request): device_b = Device.objects.create( name='Device B', site=Site.objects.first(), device_type=DeviceType.objects.first(), - device_role=DeviceRole.objects.first() + role=DeviceRole.objects.first() ) device_b_id = device_b.id - # Move interface to new device - interface = Interface.objects.get(id=interface_id) - interface.device = device_b - interface.save() + # Create interface on new device + interface_b = Interface.objects.create( + device=device_b, + name='eth0', + type='1000base-t' + ) + interface_b_id = interface_b.id - # Delete old device + # Delete old device (cascade deletes interface_a) Device.objects.get(id=device_a_id).delete() # Merge branch @@ -130,10 +148,9 @@ def test_merge_update_interface_then_delete_device(self): # Verify main schema self.assertFalse(Device.objects.filter(id=device_a_id).exists()) + self.assertFalse(Interface.objects.filter(id=interface_a_id).exists()) self.assertTrue(Device.objects.filter(id=device_b_id).exists()) - - interface = Interface.objects.get(id=interface_id) - self.assertEqual(interface.device_id, device_b_id) + self.assertTrue(Interface.objects.filter(id=interface_b_id).exists()) def test_merge_create_and_delete_same_object(self): """ @@ -142,8 +159,13 @@ def test_merge_create_and_delete_same_object(self): # Create branch branch = self._create_and_provision_branch() + # Create a request context for event tracking + request = RequestFactory().get(reverse('home')) + request.id = uuid.uuid4() # Set request id for ObjectChange tracking + request.user = self.user + # In branch: create and delete a site - with activate_branch(branch): + with activate_branch(branch), event_tracking(request): site = Site.objects.create(name='Temp Site', slug='temp-site') site_id = site.id site.delete() @@ -170,8 +192,13 @@ def test_merge_slug_rename_then_create(self): # Create branch branch = self._create_and_provision_branch() + # Create a request context for event tracking + request = RequestFactory().get(reverse('home')) + request.id = uuid.uuid4() # Set request id for ObjectChange tracking + request.user = self.user + # In branch: rename site1 slug, create new site with old slug - with activate_branch(branch): + with activate_branch(branch), event_tracking(request): site1 = Site.objects.get(id=site1_id) site1.slug = 'site-1-renamed' site1.save() @@ -202,8 +229,13 @@ def test_merge_multiple_updates_collapsed(self): # Create branch branch = self._create_and_provision_branch() + # Create a request context for event tracking + request = RequestFactory().get(reverse('home')) + request.id = uuid.uuid4() # Set request id for ObjectChange tracking + request.user = self.user + # In branch: update site multiple times - with activate_branch(branch): + with activate_branch(branch), event_tracking(request): site = Site.objects.get(id=site_id) site.description = 'Update 1' @@ -231,8 +263,13 @@ def test_merge_create_with_multiple_updates(self): # Create branch branch = self._create_and_provision_branch() + # Create a request context for event tracking + request = RequestFactory().get(reverse('home')) + request.id = uuid.uuid4() # Set request id for ObjectChange tracking + request.user = self.user + # In branch: create site and update it multiple times - with activate_branch(branch): + with activate_branch(branch), event_tracking(request): site = Site.objects.create(name='New Site', slug='new-site', description='Initial') site_id = site.id @@ -262,21 +299,26 @@ def test_merge_complex_dependency_chain(self): name='Device A', site=site, device_type=self.device_type, - device_role=self.device_role + role=self.device_role ) device_a_id = device_a.id # Create branch branch = self._create_and_provision_branch() + # Create a request context for event tracking + request = RequestFactory().get(reverse('home')) + request.id = uuid.uuid4() # Set request id for ObjectChange tracking + request.user = self.user + # In branch: complex operations - with activate_branch(branch): + with activate_branch(branch), event_tracking(request): # Create new devices device_b = Device.objects.create( name='Device B', site=Site.objects.first(), device_type=DeviceType.objects.first(), - device_role=DeviceRole.objects.first() + role=DeviceRole.objects.first() ) device_b_id = device_b.id @@ -284,17 +326,11 @@ def test_merge_complex_dependency_chain(self): name='Device C', site=Site.objects.first(), device_type=DeviceType.objects.first(), - device_role=DeviceRole.objects.first() + role=DeviceRole.objects.first() ) device_c_id = device_c.id - # Create interfaces - interface_a = Interface.objects.create( - device=device_a, - name='eth0', - type='1000base-t' - ) - + # Create interface on device_b interface_b = Interface.objects.create( device=device_b, name='eth0', @@ -302,9 +338,12 @@ def test_merge_complex_dependency_chain(self): ) interface_b_id = interface_b.id - # Move interface_a to device_b - interface_a.device = device_b - interface_a.save() + # Create another interface on device_b + interface_c = Interface.objects.create( + device=device_b, + name='eth1', + type='1000base-t' + ) # Update device_b device_b.name = 'Device B Updated' @@ -323,7 +362,7 @@ def test_merge_complex_dependency_chain(self): device_b = Device.objects.get(id=device_b_id) self.assertEqual(device_b.name, 'Device B Updated') - self.assertEqual(device_b.interface_set.count(), 2) + self.assertEqual(device_b.interfaces.count(), 2) def test_merge_delete_ordering_by_time(self): """ @@ -341,8 +380,13 @@ def test_merge_delete_ordering_by_time(self): # Create branch branch = self._create_and_provision_branch() + # Create a request context for event tracking + request = RequestFactory().get(reverse('home')) + request.id = uuid.uuid4() # Set request id for ObjectChange tracking + request.user = self.user + # In branch: delete sites in specific order - with activate_branch(branch): + with activate_branch(branch), event_tracking(request): Site.objects.get(id=site1_id).delete() Site.objects.get(id=site3_id).delete() Site.objects.get(id=site2_id).delete() From c717166b09498cff44c4f89ecdadc165de396c78 Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 18 Nov 2025 14:53:31 -0800 Subject: [PATCH 05/38] update tests --- netbox_branching/tests/test_merge.py | 291 +++++++++++++++++++++------ 1 file changed, 235 insertions(+), 56 deletions(-) diff --git a/netbox_branching/tests/test_merge.py b/netbox_branching/tests/test_merge.py index e92d5b4..736c0a8 100644 --- a/netbox_branching/tests/test_merge.py +++ b/netbox_branching/tests/test_merge.py @@ -50,10 +50,164 @@ def _create_and_provision_branch(self, name='Test Branch'): branch.refresh_from_db() # Refresh to get updated status return branch + def test_merge_basic_create(self): + """ + Test basic create operation. + Verifies object is created in main and ObjectChange was tracked. + """ + # Create branch + branch = self._create_and_provision_branch() + + # Create a request context for event tracking + request = RequestFactory().get(reverse('home')) + request.id = uuid.uuid4() + request.user = self.user + + # In branch: create site + with activate_branch(branch), event_tracking(request): + site = Site.objects.create(name='Test Site', slug='test-site') + site_id = site.id + + # Verify ObjectChange was created in branch + from django.contrib.contenttypes.models import ContentType + site_ct = ContentType.objects.get_for_model(Site) + changes = branch.get_unmerged_changes().filter( + changed_object_type=site_ct, + changed_object_id=site_id + ) + self.assertEqual(changes.count(), 1) + self.assertEqual(changes.first().action, 'create') + + # Merge branch + branch.merge(user=self.user, commit=True) + + # Verify site exists in main + self.assertTrue(Site.objects.filter(id=site_id).exists()) + site = Site.objects.get(id=site_id) + self.assertEqual(site.name, 'Test Site') + self.assertEqual(site.slug, 'test-site') + + def test_merge_basic_update(self): + """ + Test basic update operation. + Verifies object is updated in main and ObjectChange was tracked. + """ + # Create site in main + site = Site.objects.create(name='Original Site', slug='test-site', description='Original') + site_id = site.id + + # Create branch + branch = self._create_and_provision_branch() + + # Create a request context for event tracking + request = RequestFactory().get(reverse('home')) + request.id = uuid.uuid4() + request.user = self.user + + # In branch: update site + with activate_branch(branch), event_tracking(request): + site = Site.objects.get(id=site_id) + site.description = 'Updated' + site.save() + + # Verify ObjectChange was created in branch + from django.contrib.contenttypes.models import ContentType + site_ct = ContentType.objects.get_for_model(Site) + changes = branch.get_unmerged_changes().filter( + changed_object_type=site_ct, + changed_object_id=site_id + ) + self.assertEqual(changes.count(), 1) + self.assertEqual(changes.first().action, 'update') + + # Merge branch + branch.merge(user=self.user, commit=True) + + # Verify site is updated in main + site = Site.objects.get(id=site_id) + self.assertEqual(site.description, 'Updated') + + def test_merge_basic_delete(self): + """ + Test basic delete operation. + Verifies object is deleted in main and ObjectChange was tracked. + """ + # Create site in main + site = Site.objects.create(name='Test Site', slug='test-site') + site_id = site.id + + # Create branch + branch = self._create_and_provision_branch() + + # Create a request context for event tracking + request = RequestFactory().get(reverse('home')) + request.id = uuid.uuid4() + request.user = self.user + + # In branch: delete site + with activate_branch(branch), event_tracking(request): + Site.objects.get(id=site_id).delete() + + # Verify ObjectChange was created in branch + from django.contrib.contenttypes.models import ContentType + site_ct = ContentType.objects.get_for_model(Site) + changes = branch.get_unmerged_changes().filter( + changed_object_type=site_ct, + changed_object_id=site_id + ) + self.assertEqual(changes.count(), 1) + self.assertEqual(changes.first().action, 'delete') + + # Merge branch + branch.merge(user=self.user, commit=True) + + # Verify site is deleted in main + self.assertFalse(Site.objects.filter(id=site_id).exists()) + + def test_merge_basic_create_update_delete(self): + """ + Test create, update, then delete same object. + Verifies object is skipped (not in main) after collapsing. + """ + # Create branch + branch = self._create_and_provision_branch() + + # Create a request context for event tracking + request = RequestFactory().get(reverse('home')) + request.id = uuid.uuid4() + request.user = self.user + + # In branch: create, update, then delete site + with activate_branch(branch), event_tracking(request): + site = Site.objects.create(name='Temp Site', slug='temp-site') + site_id = site.id + + site.description = 'Modified' + site.save() + + site.delete() + + # Verify 3 ObjectChanges were created in branch + from django.contrib.contenttypes.models import ContentType + site_ct = ContentType.objects.get_for_model(Site) + changes = branch.get_unmerged_changes().filter( + changed_object_type=site_ct, + changed_object_id=site_id + ) + self.assertEqual(changes.count(), 3) + actions = [c.action for c in changes.order_by('time')] + self.assertEqual(actions, ['create', 'update', 'delete']) + + # Merge branch + branch.merge(user=self.user, commit=True) + + # Verify site does not exist in main (skipped during merge) + self.assertFalse(Site.objects.filter(id=site_id).exists()) + def test_merge_delete_then_create_same_slug(self): """ - Test merging when a site is deleted and a new site with the same slug is created. - This was the original bug: deletes must happen before creates to free up unique constraints. + Test delete object, then create object with same unique constraint value (slug). + Verifies deletes free up unique constraints before creates. """ # Create site in main site1 = Site.objects.create(name='Site 1', slug='site-1') @@ -94,8 +248,8 @@ def test_merge_delete_then_create_same_slug(self): def test_merge_create_device_and_delete_old(self): """ - Test merging when a new device is created and an old device is deleted. - Tests ordering with dependencies. + Test create new object, then delete old object. + Verifies proper ordering with cascade delete dependencies. """ # Create device with interface in main site = Site.objects.create(name='Site 1', slug='site-1') @@ -152,38 +306,10 @@ def test_merge_create_device_and_delete_old(self): self.assertTrue(Device.objects.filter(id=device_b_id).exists()) self.assertTrue(Interface.objects.filter(id=interface_b_id).exists()) - def test_merge_create_and_delete_same_object(self): - """ - Test that creating and deleting the same object in a branch results in no change (skip). - """ - # Create branch - branch = self._create_and_provision_branch() - - # Create a request context for event tracking - request = RequestFactory().get(reverse('home')) - request.id = uuid.uuid4() # Set request id for ObjectChange tracking - request.user = self.user - - # In branch: create and delete a site - with activate_branch(branch), event_tracking(request): - site = Site.objects.create(name='Temp Site', slug='temp-site') - site_id = site.id - site.delete() - - # Merge branch - branch.merge(user=self.user, commit=True) - - # Verify main schema - site should not exist - self.assertFalse(Site.objects.filter(id=site_id).exists()) - - # Verify merge succeeded - branch.refresh_from_db() - self.assertEqual(branch.status, BranchStatusChoices.MERGED) - def test_merge_slug_rename_then_create(self): """ - Test merging when a site's slug is changed, then a new site is created with the old slug. - Updates should happen before creates to free up the slug. + Test update object's unique field, then create new object with old value. + Verifies updates free up unique constraints before creates. """ # Create site in main site1 = Site.objects.create(name='Site 1', slug='site-1') @@ -220,7 +346,8 @@ def test_merge_slug_rename_then_create(self): def test_merge_multiple_updates_collapsed(self): """ - Test that multiple updates to the same object are collapsed into a single update. + Test multiple updates to same object. + Verifies consecutive non-referencing updates are collapsed. """ # Create site in main site = Site.objects.create(name='Site 1', slug='site-1', description='Original') @@ -257,8 +384,8 @@ def test_merge_multiple_updates_collapsed(self): def test_merge_create_with_multiple_updates(self): """ - Test that creating an object and then updating it multiple times - results in a single create with the final state. + Test create object then update it multiple times. + Verifies create is kept separate from updates. """ # Create branch branch = self._create_and_provision_branch() @@ -291,7 +418,8 @@ def test_merge_create_with_multiple_updates(self): def test_merge_complex_dependency_chain(self): """ - Test a complex scenario with creates, updates, and deletes with dependencies. + Test complex scenario with multiple creates, updates, and deletes. + Verifies correct ordering with FK dependencies and references. """ # Create initial devices in main site = Site.objects.create(name='Site 1', slug='site-1') @@ -364,39 +492,90 @@ def test_merge_complex_dependency_chain(self): self.assertEqual(device_b.name, 'Device B Updated') self.assertEqual(device_b.interfaces.count(), 2) - def test_merge_delete_ordering_by_time(self): + def test_merge_conflicting_slug_create_update_delete(self): """ - Test that deletes maintain time order when there are no dependencies. + Test create object with conflicting unique constraint, update it, then delete it. + Verifies skipped object doesn't cause constraint violations. """ - # Create sites in main - site1 = Site.objects.create(name='Site 1', slug='site-1') - site2 = Site.objects.create(name='Site 2', slug='site-2') - site3 = Site.objects.create(name='Site 3', slug='site-3') + # Create branch + branch = self._create_and_provision_branch() - site1_id = site1.id - site2_id = site2.id - site3_id = site3.id + # In main: create site with slug that will conflict + site_main = Site.objects.create(name='Main Site', slug='conflict-slug') + site_main_id = site_main.id + + # Create a request context for event tracking + request = RequestFactory().get(reverse('home')) + request.id = uuid.uuid4() + request.user = self.user + + # In branch: create site with same slug (conflicts), update it, then delete it + with activate_branch(branch), event_tracking(request): + site_branch = Site.objects.create(name='Branch Site', slug='conflict-slug') + site_branch_id = site_branch.id + + # Update description + site_branch.description = 'Updated in branch' + site_branch.save() + + # Delete the site + site_branch.delete() + + # Merge branch - should succeed (branch site is skipped) + branch.merge(user=self.user, commit=True) + # Verify main schema - only main site exists + self.assertTrue(Site.objects.filter(id=site_main_id).exists()) + self.assertFalse(Site.objects.filter(id=site_branch_id).exists()) + self.assertEqual(Site.objects.filter(slug='conflict-slug').count(), 1) + + # Verify branch status + branch.refresh_from_db() + self.assertEqual(branch.status, BranchStatusChoices.MERGED) + + def test_merge_slug_update_causes_then_resolves_conflict(self): + """ + Test create object, update to conflicting unique constraint, then update to resolve. + Verifies final non-conflicting state merges successfully. + """ # Create branch branch = self._create_and_provision_branch() + # In main: create site that will have slug conflict + site_main = Site.objects.create(name='Main Site', slug='conflict-slug') + site_main_id = site_main.id + # Create a request context for event tracking request = RequestFactory().get(reverse('home')) - request.id = uuid.uuid4() # Set request id for ObjectChange tracking + request.id = uuid.uuid4() request.user = self.user - # In branch: delete sites in specific order + # In branch: create site with non-conflicting slug, update to conflict, then resolve with activate_branch(branch), event_tracking(request): - Site.objects.get(id=site1_id).delete() - Site.objects.get(id=site3_id).delete() - Site.objects.get(id=site2_id).delete() + site_branch = Site.objects.create(name='Branch Site', slug='no-conflict') + site_branch_id = site_branch.id - # Merge branch + # Update to conflicting slug + site_branch.slug = 'conflict-slug' + site_branch.save() + + # Update again to resolve conflict + site_branch.slug = 'resolved-slug' + site_branch.save() + + # Merge branch - should succeed (final state has no conflict) branch.merge(user=self.user, commit=True) - # Verify all deleted - self.assertEqual(Site.objects.count(), 0) + # Verify main schema + self.assertTrue(Site.objects.filter(id=site_main_id).exists()) + self.assertTrue(Site.objects.filter(id=site_branch_id).exists()) + + site_main = Site.objects.get(id=site_main_id) + self.assertEqual(site_main.slug, 'conflict-slug') - # Verify merge succeeded + site_branch = Site.objects.get(id=site_branch_id) + self.assertEqual(site_branch.slug, 'resolved-slug') + + # Verify branch status branch.refresh_from_db() self.assertEqual(branch.status, BranchStatusChoices.MERGED) From 44af81369485b11deddafa7350e9e6ba587a385f Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 18 Nov 2025 15:14:43 -0800 Subject: [PATCH 06/38] dependency graph --- netbox_branching/models/branches.py | 534 ++++++++++++---------------- 1 file changed, 232 insertions(+), 302 deletions(-) diff --git a/netbox_branching/models/branches.py b/netbox_branching/models/branches.py index 55c31fe..05f4a5e 100644 --- a/netbox_branching/models/branches.py +++ b/netbox_branching/models/branches.py @@ -524,331 +524,276 @@ def __repr__(self): return f"" @staticmethod - def _update_references_creates(update_change, all_changes_by_key): + def _collapse_changes_for_object(changes, logger): """ - Check if an UPDATE references (via FK) any objects being CREATEd in this merge. - Returns: True if the update has FK references to created objects. + Collapse a list of ObjectChanges for a single object. + Returns: (final_action, merged_data, last_change) """ - if not update_change.postchange_data: - return False + if not changes: + return None, None, None + + # Sort by time (oldest first) + changes = sorted(changes, key=lambda c: c.time) - model_class = update_change.changed_object_type.model_class() + first_action = changes[0].action + last_action = changes[-1].action + last_change = changes[-1] + + logger.debug(f" Collapsing {len(changes)} changes: first={first_action}, last={last_action}") + + # Case 1: Created then deleted -> skip entirely + if first_action == 'create' and last_action == 'delete': + logger.debug(f" -> Action: SKIP (created and deleted in branch)") + return 'skip', None, last_change + + # Case 2: Deleted -> just delete (should be only one delete) + if last_action == 'delete': + logger.debug(f" -> Action: DELETE") + return 'delete', changes[-1].prechange_data, last_change + + # Case 3: Created (with possible updates) -> single create + if first_action == 'create': + merged_data = {} + for change in changes: + # Merge postchange_data, later changes overwrite earlier ones + if change.postchange_data: + merged_data.update(change.postchange_data) + logger.debug(f" -> Action: CREATE (collapsed {len(changes)} changes)") + return 'create', merged_data, last_change + + # Case 4: Only updates -> single update + merged_data = {} + # Start with prechange_data of first change as baseline + if changes[0].prechange_data: + merged_data.update(changes[0].prechange_data) + # Apply each change's postchange_data + for change in changes: + if change.postchange_data: + merged_data.update(change.postchange_data) + logger.debug(f" -> Action: UPDATE (collapsed {len(changes)} changes)") + return 'update', merged_data, last_change - # Check each FK field + @staticmethod + def _get_fk_references(model_class, data, changed_objects): + """ + Get FK references from data that point to objects in changed_objects. + Returns: set of (content_type_id, object_id) tuples + """ + if not data: + return set() + + references = set() for field in model_class._meta.get_fields(): if isinstance(field, models.ForeignKey): fk_field_name = field.attname # e.g., 'device_id' - fk_value = update_change.postchange_data.get(fk_field_name) + fk_value = data.get(fk_field_name) if fk_value: - # Get the key for the referenced object + # Get the content type of the related model related_model = field.related_model related_ct = ContentType.objects.get_for_model(related_model) ref_key = (related_ct.id, fk_value) - # Check if this object is being created - if ref_key in all_changes_by_key: - # Check if any change for this object is a CREATE - for change in all_changes_by_key[ref_key]: - if change.action == 'create': - return True + # Only track if this object is in our changed_objects + if ref_key in changed_objects: + references.add(ref_key) - return False + return references @staticmethod - def _collapse_changes_for_object(changes, all_changes_by_key, logger): + def _removed_reference_to(collapsed_change, target_key, logger): """ - Collapse a list of ObjectChanges for a single object. + 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 - Returns: list of CollapsedChange objects + target_ct_id, target_obj_id = target_key + first_change = collapsed_change.changes[0] - Simplified collapsing rules: - 1. If there's a DELETE: - - If there's also a CREATE: skip entirely (return []) - - Otherwise: keep only the DELETE (with prechange_data from first ObjectChange) - 2. If no DELETE: - - If first is CREATE: keep CREATE with postchange_data, plus any UPDATEs - - If all UPDATEs: collapse intelligently (keep referencing UPDATEs separate) - """ - if not changes: - return [] + # 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 - # Sort by time (oldest first) - changes = sorted(changes, key=lambda c: c.time) - model_name = changes[0].changed_object_type.model_class().__name__ - object_id = changes[0].changed_object_id + # If created in branch, it couldn't have had an initial reference + if first_change.action == 'create': + return False - logger.debug(f" Collapsing {len(changes)} changes for {model_name}:{object_id}") + # It's an update - check if FK reference changed + initial_state = first_change.prechange_data or {} + final_state = collapsed_change.merged_data or {} - # Check if there's a DELETE - has_delete = any(c.action == 'delete' for c in changes) - has_create = any(c.action == 'create' for c in changes) + # 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) - if has_delete: - if has_create: - # CREATE + DELETE = skip entirely - logger.debug(f" -> SKIP (created and deleted in branch)") - return [] - else: - # Just DELETE (ignore all other changes) - # Use prechange_data from first ObjectChange - logger.debug(f" -> DELETE (keeping only DELETE, ignoring {len(changes)-1} other changes)") - delete_change = next(c for c in changes if c.action == 'delete') + # Only check if this FK could point to our target + if related_ct.id != target_ct_id: + continue - # Copy prechange_data from first change to the delete - delete_change.prechange_data = changes[0].prechange_data + 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) - result = Branch.CollapsedChange( - key=(delete_change.changed_object_type.id, delete_change.changed_object_id), - model_class=delete_change.changed_object_type.model_class() - ) - result.changes = [delete_change] - result.final_action = 'delete' - result.merged_data = None - result.last_change = delete_change - return [result] - - # No DELETE - handle CREATE or UPDATEs - result_list = [] - - create_changes = [c for c in changes if c.action == 'create'] - update_changes = [c for c in changes if c.action == 'update'] - - if create_changes: - create = create_changes[0] - logger.debug(f" -> CREATE (time {create.time})") - collapsed = Branch.CollapsedChange( - key=(create.changed_object_type.id, create.changed_object_id), - model_class=create.changed_object_type.model_class() - ) - collapsed.changes = [create] - collapsed.final_action = 'create' - collapsed.merged_data = create.postchange_data - collapsed.last_change = create - result_list.append(collapsed) - - # Process UPDATEs separately (don't merge into CREATE) - # Keep referencing UPDATEs separate so time ordering respects dependencies - if update_changes: - i = 0 - while i < len(update_changes): - current_update = update_changes[i] - - # Check if this update references a create - has_ref = Branch._update_references_creates(current_update, all_changes_by_key) - - if has_ref: - # Keep referencing update separate - logger.debug(f" -> UPDATE (time {current_update.time}, has FK reference to CREATE)") - collapsed = Branch.CollapsedChange( - key=(current_update.changed_object_type.id, current_update.changed_object_id), - model_class=current_update.changed_object_type.model_class() - ) - collapsed.changes = [current_update] - collapsed.final_action = 'update' - collapsed.merged_data = current_update.postchange_data - collapsed.last_change = current_update - result_list.append(collapsed) - i += 1 - else: - # Collapse consecutive non-referencing updates - group_start = i - group_changes = [current_update] - - # Find consecutive non-referencing updates - i += 1 - while i < len(update_changes): - next_update = update_changes[i] - if Branch._update_references_creates(next_update, all_changes_by_key): - break - group_changes.append(next_update) - i += 1 - - # Collapse the group - merged_data = {} - if group_changes[0].prechange_data: - merged_data.update(group_changes[0].prechange_data) - for change in group_changes: - if change.postchange_data: - merged_data.update(change.postchange_data) - - last_change = group_changes[-1] - logger.debug(f" -> UPDATE (time {last_change.time}, collapsed {len(group_changes)} non-referencing updates)") - - collapsed = Branch.CollapsedChange( - key=(last_change.changed_object_type.id, last_change.changed_object_id), - model_class=last_change.changed_object_type.model_class() - ) - collapsed.changes = group_changes - collapsed.final_action = 'update' - collapsed.merged_data = merged_data - collapsed.last_change = last_change - result_list.append(collapsed) + # 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 result_list + return False @staticmethod - def _remove_references_to_skipped_objects(collapsed_changes, skipped_keys, logger): + def _build_dependency_graph(collapsed_changes, logger): + """ + Build dependency graph between collapsed changes. + Modifies collapsed_changes in place to set depends_on/depended_by. """ - Remove FK references to skipped objects (CREATE + DELETE) from collapsed changes. + 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 UPDATEs: Safe to remove FK fields since object already exists in valid state. - The field will remain unchanged (keeps its previous value). + 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 - For CREATEs: If a CREATE references a skipped object, it might fail validation - if the FK is required (NOT NULL). This is an edge case that should be rare: - - Object A created - - Object B created with FK to A - - Object A deleted - - Merge attempt: B's CREATE references non-existent A - TODO: Consider detecting and reporting this case with a clear error message. + 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)") - Args: - collapsed_changes: List of CollapsedChange objects - skipped_keys: Set of (ct_id, obj_id) tuples for skipped objects - logger: Logger instance + logger.info(f" Dependency graph built: {sum(len(c.depends_on) for c in collapsed_changes.values())} dependencies") - Returns: - Filtered list of CollapsedChange objects (UPDATEs with no changes removed) + @staticmethod + def _topological_sort_with_cycle_detection(collapsed_changes, logger): """ - if not skipped_keys: - return collapsed_changes + Topological sort with cycle detection. + Returns: (ordered_list, cycles_detected) - logger.info(f"Checking for references to {len(skipped_keys)} skipped objects...") + If cycles are detected, breaks them and continues, returning the partial order. + """ + logger.info("Performing topological sort...") - changes_to_remove = [] + # Create a copy of dependencies to modify + remaining = {key: set(collapsed.depends_on) for key, collapsed in collapsed_changes.items()} + ordered = [] - for collapsed in collapsed_changes: - if collapsed.final_action not in ('create', 'update'): - continue + iteration = 0 + max_iterations = len(remaining) * 2 # Safety limit - if not collapsed.merged_data: - continue + while remaining and iteration < max_iterations: + iteration += 1 - # Check each FK field for references to skipped objects - fields_to_remove = [] - for field in collapsed.model_class._meta.get_fields(): - if isinstance(field, models.ForeignKey): - related_model = field.related_model - related_ct = ContentType.objects.get_for_model(related_model) + # Find all nodes with no dependencies + ready = [key for key, deps in remaining.items() if not deps] - # Check both field.name (e.g., 'site') and field.attname (e.g., 'site_id') - # ObjectChange may store FK using either name - fk_value = None - field_name_to_remove = None - - # Try field.name first (e.g., 'site') - if field.name in collapsed.merged_data: - fk_value = collapsed.merged_data[field.name] - field_name_to_remove = field.name - # Try field.attname (e.g., 'site_id') - elif field.attname in collapsed.merged_data: - fk_value = collapsed.merged_data[field.attname] - field_name_to_remove = field.attname - - if fk_value: - ref_key = (related_ct.id, fk_value) - - if ref_key in skipped_keys: - logger.warning( - f" {collapsed} references skipped object " - f"{related_model.__name__}:{fk_value}, removing field '{field.name}'" - ) - fields_to_remove.append(field_name_to_remove) - - # Remove the fields that reference skipped objects - for field_name in fields_to_remove: - del collapsed.merged_data[field_name] - - # For CREATEs: if we removed a required FK field, skip the CREATE entirely - if collapsed.final_action == 'create' and fields_to_remove: - # Check if any removed field was required (NOT NULL) - for field_name in fields_to_remove: - # Find the field definition - field_obj = None - for field in collapsed.model_class._meta.get_fields(): - if isinstance(field, models.ForeignKey): - if field.name == field_name or field.attname == field_name: - field_obj = field - break - - if field_obj and not field_obj.null: - logger.warning( - f" {collapsed} requires removed field '{field_obj.name}' " - f"which referenced skipped object. Skipping CREATE entirely." - ) - changes_to_remove.append(collapsed) - break - - # For UPDATEs: check if there are any remaining changes - if collapsed.final_action == 'update' and fields_to_remove: - first_change = collapsed.changes[0] - prechange = first_change.prechange_data or {} - - # Check if merged_data differs from prechange_data - has_changes = any( - collapsed.merged_data.get(k) != prechange.get(k) - for k in collapsed.merged_data.keys() - ) + if not ready: + # No nodes without dependencies - we have a cycle + logger.warning(f" Cycle detected in dependency graph. Breaking cycle to continue.") + # Pick a node arbitrarily to break the cycle + key = next(iter(remaining)) + ready = [key] + logger.warning(f" Breaking cycle by processing {collapsed_changes[key]} first") - if not has_changes: - logger.info( - f" {collapsed} has no remaining changes after removing " - f"skipped references, removing from merge" - ) - changes_to_remove.append(collapsed) + # Process ready nodes + for key in ready: + ordered.append(key) + del remaining[key] - # Remove UPDATEs with no remaining changes - for collapsed in changes_to_remove: - collapsed_changes.remove(collapsed) + # Remove this key from other nodes' dependencies + for deps in remaining.values(): + deps.discard(key) - if changes_to_remove: - logger.info(f" Removed {len(changes_to_remove)} UPDATEs with no remaining changes") + if iteration >= max_iterations: + logger.error(f" Topological sort exceeded maximum iterations. Possible complex cycle.") + # Add remaining nodes in arbitrary order + ordered.extend(remaining.keys()) - return collapsed_changes + logger.info(f" Topological sort completed: {len(ordered)} changes ordered") + return ordered @staticmethod def _order_collapsed_changes(collapsed_changes, logger): """ - Order collapsed changes by time. + Order collapsed changes respecting dependencies and constraints. + Returns: ordered list of CollapsedChange objects + """ + logger.info(f"Ordering {len(collapsed_changes)} collapsed changes...") - With smart collapsing (keeping referencing UPDATEs separate), natural time order - respects all dependencies: - - CREATEs come before UPDATEs that reference them (time order from branch) - - UPDATEs that remove references come before DELETEs (time order from branch) - - DELETEs free unique constraints before CREATEs claim them (time order from branch) + # Remove skipped objects + to_process = {k: v for k, v in collapsed_changes.items() if v.final_action != 'skip'} + skipped = [v for v in collapsed_changes.values() if v.final_action == 'skip'] - Example 1 - Freeing unique constraints: - Scenario: Site 's1' exists in main. In branch: DELETE Site 's1', CREATE new Site 's1' - Needed order: DELETE Site 's1' → CREATE new Site 's1' - (Delete frees the slug before create claims it) + logger.info(f" {len(skipped)} changes will be skipped (created and deleted in branch)") + logger.info(f" {len(to_process)} changes to process") - Example 2 - Dependency chain: - Scenario: Interface points to Device A. In branch: CREATE Device B, UPDATE Interface (A→B), DELETE Device A - Needed order: CREATE Device B → UPDATE Interface → DELETE Device A - (Create Device B first for FK, update to remove reference to A, then delete A) + if not to_process: + return [] - Returns: ordered list of CollapsedChange objects sorted by time - """ - logger.info(f"Ordering {len(collapsed_changes)} collapsed changes...") + # Build dependency graph + Branch._build_dependency_graph(to_process, logger) - if not collapsed_changes: - return [] + # Topological sort + ordered_keys = Branch._topological_sort_with_cycle_detection(to_process, logger) - # Sort by time (oldest first) - sorted_changes = sorted(collapsed_changes, key=lambda c: c.last_change.time) + # Group by model and refine order within each model + logger.info("Refining order within models (updates before creates)...") + 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 creates, then deletes + result = [] + for model_class, changes in by_model.items(): + updates = [c for c in changes if c.final_action == 'update'] + creates = [c for c in changes if c.final_action == 'create'] + deletes = [c for c in changes if c.final_action == 'delete'] - # Log summary - action_counts = defaultdict(int) - for collapsed in sorted_changes: - action_counts[collapsed.final_action] += 1 + if updates or creates or deletes: + logger.debug(f" {model_class.__name__}: {len(updates)} updates, {len(creates)} creates, {len(deletes)} deletes") - logger.info(f" Ordered {len(sorted_changes)} changes by time: " - f"{action_counts['create']} creates, " - f"{action_counts['update']} updates, " - f"{action_counts['delete']} deletes") + result.extend(updates) + result.extend(creates) + result.extend(deletes) - return sorted_changes + logger.info(f"Ordering complete: {len(result)} changes to apply") + return result def _apply_collapsed_change(self, collapsed, using=DEFAULT_DB_ALIAS, logger=None): """ @@ -957,50 +902,35 @@ def merge(self, user, commit=True): with transaction.atomic(): models = set() - # Group changes by object - logger.info("Grouping ObjectChanges by object...") - changes_by_object = {} + # Group and collapse changes by object + logger.info("Collapsing ObjectChanges by object...") + collapsed_changes = {} for change in changes: key = (change.changed_object_type.id, change.changed_object_id) - if key not in changes_by_object: - changes_by_object[key] = [] - logger.debug(f"New object: {change.changed_object_type.model_class().__name__}:{change.changed_object_id}") + if key not in collapsed_changes: + model_class = change.changed_object_type.model_class() + collapsed = Branch.CollapsedChange(key, model_class) + collapsed_changes[key] = collapsed + logger.debug(f"New object: {model_class.__name__}:{change.changed_object_id}") - changes_by_object[key].append(change) + collapsed_changes[key].changes.append(change) - logger.info(f" {len(changes)} changes grouped into {len(changes_by_object)} objects") + logger.info(f" {len(changes)} changes collapsed into {len(collapsed_changes)} objects") # Collapse each object's changes - logger.info("Collapsing changes for each object...") - all_collapsed = [] - skipped_keys = set() - - for key, object_changes in changes_by_object.items(): - # Returns list of CollapsedChange objects (may be empty if skipped, or multiple for UPDATEs) - collapsed_list = Branch._collapse_changes_for_object( - object_changes, changes_by_object, logger + logger.info("Determining final action for each object...") + for key, collapsed in collapsed_changes.items(): + final_action, merged_data, last_change = Branch._collapse_changes_for_object( + collapsed.changes, logger ) + collapsed.final_action = final_action + collapsed.merged_data = merged_data + collapsed.last_change = last_change - # Track skipped objects (CREATE + DELETE) - if not collapsed_list: # Empty list means object was skipped - skipped_keys.add(key) - logger.debug(f" Skipped object: {key}") - - all_collapsed.extend(collapsed_list) - - logger.info(f" {len(changes)} original changes collapsed into {len(all_collapsed)} operations") - if skipped_keys: - logger.info(f" {len(skipped_keys)} objects skipped (created and deleted in branch)") - - # Remove references to skipped objects - all_collapsed = Branch._remove_references_to_skipped_objects( - all_collapsed, skipped_keys, logger - ) - - # Order collapsed changes by time - ordered_changes = Branch._order_collapsed_changes(all_collapsed, logger) + # Order collapsed changes based on dependencies + ordered_changes = Branch._order_collapsed_changes(collapsed_changes, logger) # Apply collapsed changes in order logger.info(f"Applying {len(ordered_changes)} collapsed changes...") From dca51eab61dbbdf493a7670385d67db5ea24a6cd Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 18 Nov 2025 15:18:31 -0800 Subject: [PATCH 07/38] fix ruff --- netbox_branching/models/branches.py | 21 ++++++++++++++------- netbox_branching/tests/test_merge.py | 5 ++--- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/netbox_branching/models/branches.py b/netbox_branching/models/branches.py index 05f4a5e..99f4805 100644 --- a/netbox_branching/models/branches.py +++ b/netbox_branching/models/branches.py @@ -521,7 +521,10 @@ def __init__(self, key, model_class): def __repr__(self): ct_id, obj_id = self.key - return f"" + return ( + f"" + ) @staticmethod def _collapse_changes_for_object(changes, logger): @@ -543,12 +546,12 @@ def _collapse_changes_for_object(changes, logger): # Case 1: Created then deleted -> skip entirely if first_action == 'create' and last_action == 'delete': - logger.debug(f" -> Action: SKIP (created and deleted in branch)") + logger.debug(" -> Action: SKIP (created and deleted in branch)") return 'skip', None, last_change # Case 2: Deleted -> just delete (should be only one delete) if last_action == 'delete': - logger.debug(f" -> Action: DELETE") + logger.debug(" -> Action: DELETE") return 'delete', changes[-1].prechange_data, last_change # Case 3: Created (with possible updates) -> single create @@ -697,7 +700,8 @@ def _build_dependency_graph(collapsed_changes, logger): other_collapsed.depended_by.add(key) logger.debug(f" {collapsed} depends on {other_collapsed} (removed reference)") - logger.info(f" Dependency graph built: {sum(len(c.depends_on) for c in collapsed_changes.values())} dependencies") + 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): @@ -724,7 +728,7 @@ def _topological_sort_with_cycle_detection(collapsed_changes, logger): if not ready: # No nodes without dependencies - we have a cycle - logger.warning(f" Cycle detected in dependency graph. Breaking cycle to continue.") + logger.warning(" Cycle detected in dependency graph. Breaking cycle to continue.") # Pick a node arbitrarily to break the cycle key = next(iter(remaining)) ready = [key] @@ -740,7 +744,7 @@ def _topological_sort_with_cycle_detection(collapsed_changes, logger): deps.discard(key) if iteration >= max_iterations: - logger.error(f" Topological sort exceeded maximum iterations. Possible complex cycle.") + logger.error(" Topological sort exceeded maximum iterations. Possible complex cycle.") # Add remaining nodes in arbitrary order ordered.extend(remaining.keys()) @@ -786,7 +790,10 @@ def _order_collapsed_changes(collapsed_changes, logger): deletes = [c for c in changes if c.final_action == 'delete'] if updates or creates or deletes: - logger.debug(f" {model_class.__name__}: {len(updates)} updates, {len(creates)} creates, {len(deletes)} deletes") + logger.debug( + f" {model_class.__name__}: {len(updates)} updates, " + f"{len(creates)} creates, {len(deletes)} deletes" + ) result.extend(updates) result.extend(creates) diff --git a/netbox_branching/tests/test_merge.py b/netbox_branching/tests/test_merge.py index 736c0a8..fe8c974 100644 --- a/netbox_branching/tests/test_merge.py +++ b/netbox_branching/tests/test_merge.py @@ -459,15 +459,14 @@ def test_merge_complex_dependency_chain(self): device_c_id = device_c.id # Create interface on device_b - interface_b = Interface.objects.create( + Interface.objects.create( device=device_b, name='eth0', type='1000base-t' ) - interface_b_id = interface_b.id # Create another interface on device_b - interface_c = Interface.objects.create( + Interface.objects.create( device=device_b, name='eth1', type='1000base-t' From f80fed99e221a67a80b4d64996094be7f1efd02f Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 18 Nov 2025 16:08:07 -0800 Subject: [PATCH 08/38] truncate delete --- netbox_branching/models/branches.py | 52 ++++++++++++++++++----------- 1 file changed, 33 insertions(+), 19 deletions(-) diff --git a/netbox_branching/models/branches.py b/netbox_branching/models/branches.py index 99f4805..bf8fa20 100644 --- a/netbox_branching/models/branches.py +++ b/netbox_branching/models/branches.py @@ -531,6 +531,9 @@ 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. """ if not changes: return None, None, None @@ -538,23 +541,33 @@ def _collapse_changes_for_object(changes, logger): # Sort by time (oldest first) changes = sorted(changes, key=lambda c: c.time) - first_action = changes[0].action - last_action = changes[-1].action - last_change = changes[-1] + # Check if there's a DELETE anywhere in the changes + has_delete = any(c.action == 'delete' for c in changes) + has_create = any(c.action == 'create' for c in changes) + + logger.debug(f" Collapsing {len(changes)} changes...") + + if has_delete: + if has_create: + # CREATE + DELETE = skip entirely + logger.debug(" -> Action: SKIP (created and deleted in branch)") + return 'skip', None, changes[-1] + else: + # Just DELETE (ignore all other changes like updates) + # Use prechange_data from first ObjectChange (the original state) + logger.debug(f" -> Action: DELETE (keeping only DELETE, ignoring {len(changes) - 1} other changes)") + delete_change = next(c for c in changes if c.action == 'delete') - logger.debug(f" Collapsing {len(changes)} changes: first={first_action}, last={last_action}") + # Copy prechange_data from first change to ensure we have the original state + delete_change.prechange_data = changes[0].prechange_data - # Case 1: Created then deleted -> skip entirely - if first_action == 'create' and last_action == 'delete': - logger.debug(" -> Action: SKIP (created and deleted in branch)") - return 'skip', None, last_change + return 'delete', delete_change.prechange_data, delete_change - # Case 2: Deleted -> just delete (should be only one delete) - if last_action == 'delete': - logger.debug(" -> Action: DELETE") - return 'delete', changes[-1].prechange_data, last_change + # No DELETE - handle CREATE or UPDATEs + first_action = changes[0].action + last_change = changes[-1] - # Case 3: Created (with possible updates) -> single create + # Created (with possible updates) -> single create if first_action == 'create': merged_data = {} for change in changes: @@ -564,7 +577,7 @@ def _collapse_changes_for_object(changes, logger): logger.debug(f" -> Action: CREATE (collapsed {len(changes)} changes)") return 'create', merged_data, last_change - # Case 4: Only updates -> single update + # Only updates -> single update merged_data = {} # Start with prechange_data of first change as baseline if changes[0].prechange_data: @@ -776,28 +789,29 @@ def _order_collapsed_changes(collapsed_changes, logger): ordered_keys = Branch._topological_sort_with_cycle_detection(to_process, logger) # Group by model and refine order within each model - logger.info("Refining order within models (updates before creates)...") + 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 creates, then deletes + # 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'] - creates = [c for c in changes if c.final_action == 'create'] 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(creates)} creates, {len(deletes)} deletes" + f"{len(deletes)} deletes, {len(creates)} creates" ) result.extend(updates) - result.extend(creates) result.extend(deletes) + result.extend(creates) logger.info(f"Ordering complete: {len(result)} changes to apply") return result From 9e94a21a4a8b630295461ba6bbe6fda23f321c4a Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 18 Nov 2025 17:00:02 -0800 Subject: [PATCH 09/38] Improved dependency merge --- netbox_branching/models/branches.py | 255 ++++++++++++++-------------- 1 file changed, 126 insertions(+), 129 deletions(-) diff --git a/netbox_branching/models/branches.py b/netbox_branching/models/branches.py index bf8fa20..1b6e487 100644 --- a/netbox_branching/models/branches.py +++ b/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 From 86a2e89d51ed7b938f3ea95370df2796c6b7ca93 Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 18 Nov 2025 17:03:42 -0800 Subject: [PATCH 10/38] fix ruff --- netbox_branching/models/branches.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/netbox_branching/models/branches.py b/netbox_branching/models/branches.py index 1b6e487..9d1e2c1 100644 --- a/netbox_branching/models/branches.py +++ b/netbox_branching/models/branches.py @@ -532,8 +532,8 @@ def _collapse_changes_for_object(changes, logger): Collapse a list of ObjectChanges for a single object. Returns: (final_action, merged_data, last_change) - 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. + 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. From 4b316ad9b1206b3799dc18e7efeb3e6f84ee1787 Mon Sep 17 00:00:00 2001 From: Arthur Date: Wed, 19 Nov 2025 09:16:13 -0800 Subject: [PATCH 11/38] revert changes with collapse --- netbox_branching/models/branches.py | 131 +++++++++++++++++++++++++--- 1 file changed, 121 insertions(+), 10 deletions(-) diff --git a/netbox_branching/models/branches.py b/netbox_branching/models/branches.py index 9d1e2c1..f0ec0b7 100644 --- a/netbox_branching/models/branches.py +++ b/netbox_branching/models/branches.py @@ -882,6 +882,72 @@ def _apply_collapsed_change(self, collapsed, using=DEFAULT_DB_ALIAS, logger=None except model.DoesNotExist: logger.debug(f' {model._meta.verbose_name} {object_id} already deleted; skipping') + def _undo_collapsed_change(self, collapsed, using=DEFAULT_DB_ALIAS, logger=None): + """ + Undo a collapsed change from the database (reverse of apply). + Similar to ObjectChange.undo() but works with collapsed data. + """ + from django.contrib.contenttypes.fields import GenericForeignKey + from utilities.serialization import deserialize_object + from netbox_branching.utilities import update_object + + logger = logger or logging.getLogger('netbox_branching.branch._undo_collapsed_change') + model = collapsed.model_class + object_id = collapsed.key[1] + + # Run data migrators on the last change (in revert mode) + last_change = collapsed.last_change + last_change.migrate(self, revert=True) + + # Undoing a CREATE: delete the object + if collapsed.final_action == 'create': + logger.debug(f' Undoing creation of {model._meta.verbose_name} {object_id}') + + try: + instance = model.objects.using(using).get(pk=object_id) + instance.delete(using=using) + except model.DoesNotExist: + logger.debug(f' {model._meta.verbose_name} {object_id} does not exist; skipping') + + # Undoing an UPDATE: revert to initial state + elif collapsed.final_action == 'update': + logger.debug(f' Undoing update of {model._meta.verbose_name} {object_id}') + + try: + instance = model.objects.using(using).get(pk=object_id) + except model.DoesNotExist: + logger.error(f' {model._meta.verbose_name} {object_id} not found for revert') + raise + + # Revert to the initial state (prechange_data from first change) + first_change = collapsed.changes[0] + initial_data = first_change.prechange_data or {} + + logger.debug(f' Reverting to initial state with {len(initial_data)} fields') + update_object(instance, initial_data, using=using) + + # Undoing a DELETE: restore the object + elif collapsed.final_action == 'delete': + logger.debug(f' Undoing deletion (restoring) {model._meta.verbose_name} {object_id}') + + # Use prechange_data from the first change (the original state before any changes) + first_change = collapsed.changes[0] + prechange_data = first_change.prechange_data or {} + + deserialized = deserialize_object(model, prechange_data, pk=object_id) + instance = deserialized.object + + # Restore GenericForeignKey fields + for field in instance._meta.private_fields: + if isinstance(field, GenericForeignKey): + ct_field = getattr(instance, field.ct_field) + fk_field = getattr(instance, field.fk_field) + if ct_field and fk_field: + setattr(instance, field.name, ct_field.get_object_for_this_type(pk=fk_field)) + + instance.full_clean() + instance.save(using=using) + def merge(self, user, commit=True): """ Apply all changes in the Branch to the main schema by replaying them in @@ -1007,8 +1073,8 @@ def merge(self, user, commit=True): def revert(self, user, commit=True): """ - Undo all changes associated with a previously merged Branch in the main schema by replaying them in - reverse order and calling undo() on each. + Undo all changes associated with a previously merged Branch in the main schema. + Uses the same collapsing logic as merge, but applies changes in reverse order. """ logger = logging.getLogger('netbox_branching.branch.revert') logger.info(f'Reverting branch {self} ({self.schema_name})') @@ -1021,8 +1087,8 @@ def revert(self, user, commit=True): # Emit pre-revert signal pre_revert.send(sender=self.__class__, branch=self, user=user) - # Retrieve applied changes before we update the Branch's status - if changes := self.get_changes().order_by('-time'): + # Retrieve applied changes + if changes := self.get_changes(): logger.info(f"Found {len(changes)} changes to revert") else: logger.info("No changes found; aborting.") @@ -1043,13 +1109,58 @@ def revert(self, user, commit=True): with transaction.atomic(): models = set() - # Undo each change from the Branch + # Group changes by object and create CollapsedChange objects + logger.info("Collapsing ObjectChanges by object...") + collapsed_changes = {} + for change in changes: - models.add(change.changed_object_type.model_class()) + key = (change.changed_object_type.id, change.changed_object_id) + + if key not in collapsed_changes: + model_class = change.changed_object_type.model_class() + collapsed = Branch.CollapsedChange(key, model_class) + collapsed_changes[key] = collapsed + logger.debug(f"New object: {model_class.__name__}:{change.changed_object_id}") + + collapsed_changes[key].changes.append(change) + + logger.info(f" {len(changes)} changes collapsed into {len(collapsed_changes)} objects") + + # Collapse each object's changes + logger.info("Determining final action for each object...") + for key, collapsed in collapsed_changes.items(): + final_action, merged_data, last_change = Branch._collapse_changes_for_object( + collapsed.changes, logger + ) + collapsed.final_action = final_action + collapsed.merged_data = merged_data + collapsed.last_change = last_change + + # Order collapsed changes based on dependencies (same as merge) + ordered_changes = Branch._order_collapsed_changes(collapsed_changes, logger) + + # REVERSE the order for revert (undo in opposite order) + logger.info(f"Reversing order for revert: {len(ordered_changes)} collapsed changes") + reversed_changes = list(reversed(ordered_changes)) + + # Undo collapsed changes in reverse order + logger.info(f"Undoing {len(reversed_changes)} collapsed changes in reverse order...") + for i, collapsed in enumerate(reversed_changes, 1): + model_class = collapsed.model_class + models.add(model_class) + + # Use the last change's metadata for tracking + last_change = collapsed.last_change + logger.info( + f"[{i}/{len(reversed_changes)}] Undoing {collapsed.final_action} " + f"{model_class._meta.verbose_name} (ID: {collapsed.key[1]})" + ) + with event_tracking(request): - request.id = change.request_id - request.user = change.user - change.undo(self, logger=logger) + request.id = last_change.request_id + request.user = last_change.user + self._undo_collapsed_change(collapsed, using=DEFAULT_DB_ALIAS, logger=logger) + if not commit: raise AbortTransaction() @@ -1071,7 +1182,7 @@ def revert(self, user, commit=True): self.merged_by = None self.save() - # Record a branch event for the merge + # Record a branch event for the revert logger.debug(f"Recording branch event: {BranchEventTypeChoices.REVERTED}") BranchEvent.objects.create(branch=self, user=user, type=BranchEventTypeChoices.REVERTED) From 50df0370f4075413a71f22e1070d5aa5508f65f8 Mon Sep 17 00:00:00 2001 From: Arthur Date: Wed, 19 Nov 2025 14:07:01 -0800 Subject: [PATCH 12/38] revert tests and fixes --- netbox_branching/models/branches.py | 19 ++- netbox_branching/tests/test_merge.py | 185 +++++++++++++++++++++++---- 2 files changed, 166 insertions(+), 38 deletions(-) diff --git a/netbox_branching/models/branches.py b/netbox_branching/models/branches.py index f0ec0b7..bfb4cd7 100644 --- a/netbox_branching/models/branches.py +++ b/netbox_branching/models/branches.py @@ -909,22 +909,21 @@ def _undo_collapsed_change(self, collapsed, using=DEFAULT_DB_ALIAS, logger=None) except model.DoesNotExist: logger.debug(f' {model._meta.verbose_name} {object_id} does not exist; skipping') - # Undoing an UPDATE: revert to initial state + # Undoing an UPDATE: revert to the original state elif collapsed.final_action == 'update': logger.debug(f' Undoing update of {model._meta.verbose_name} {object_id}') + # Get the original state from the first change + first_change = collapsed.changes[0] + # Make a copy to avoid modifying cached property result + revert_data = dict(first_change.diff()['pre']) + + # Load the instance and apply the original state try: instance = model.objects.using(using).get(pk=object_id) + update_object(instance, revert_data, using=using) except model.DoesNotExist: - logger.error(f' {model._meta.verbose_name} {object_id} not found for revert') - raise - - # Revert to the initial state (prechange_data from first change) - first_change = collapsed.changes[0] - initial_data = first_change.prechange_data or {} - - logger.debug(f' Reverting to initial state with {len(initial_data)} fields') - update_object(instance, initial_data, using=using) + logger.debug(f' {model._meta.verbose_name} {object_id} does not exist; skipping') # Undoing a DELETE: restore the object elif collapsed.final_action == 'delete': diff --git a/netbox_branching/tests/test_merge.py b/netbox_branching/tests/test_merge.py index fe8c974..35661fb 100644 --- a/netbox_branching/tests/test_merge.py +++ b/netbox_branching/tests/test_merge.py @@ -52,8 +52,9 @@ def _create_and_provision_branch(self, name='Test Branch'): def test_merge_basic_create(self): """ - Test basic create operation. - Verifies object is created in main and ObjectChange was tracked. + Test basic create operation with merge and revert. + Merge: creates object in main + Revert: deletes the created object """ # Create branch branch = self._create_and_provision_branch() @@ -81,20 +82,28 @@ def test_merge_basic_create(self): # Merge branch branch.merge(user=self.user, commit=True) - # Verify site exists in main + # Verify site exists in main after merge self.assertTrue(Site.objects.filter(id=site_id).exists()) site = Site.objects.get(id=site_id) self.assertEqual(site.name, 'Test Site') self.assertEqual(site.slug, 'test-site') + # Revert branch + branch.revert(user=self.user, commit=True) + + # Verify site is deleted after revert + self.assertFalse(Site.objects.filter(id=site_id).exists()) + def test_merge_basic_update(self): """ - Test basic update operation. - Verifies object is updated in main and ObjectChange was tracked. + Test basic update operation with merge and revert. + Merge: updates object in main + Revert: restores object to original state """ # Create site in main site = Site.objects.create(name='Original Site', slug='test-site', description='Original') site_id = site.id + original_description = site.description # Create branch branch = self._create_and_provision_branch() @@ -123,18 +132,28 @@ def test_merge_basic_update(self): # Merge branch branch.merge(user=self.user, commit=True) - # Verify site is updated in main + # Verify site is updated in main after merge site = Site.objects.get(id=site_id) self.assertEqual(site.description, 'Updated') + # Revert branch + branch.revert(user=self.user, commit=True) + + # Verify site is restored to original state after revert + site = Site.objects.get(id=site_id) + self.assertEqual(site.description, original_description) + def test_merge_basic_delete(self): """ - Test basic delete operation. - Verifies object is deleted in main and ObjectChange was tracked. + Test basic delete operation with merge and revert. + Merge: deletes object from main + Revert: restores the deleted object with original values """ # Create site in main site = Site.objects.create(name='Test Site', slug='test-site') site_id = site.id + original_name = site.name + original_slug = site.slug # Create branch branch = self._create_and_provision_branch() @@ -164,10 +183,20 @@ def test_merge_basic_delete(self): # Verify site is deleted in main self.assertFalse(Site.objects.filter(id=site_id).exists()) + # Revert branch + branch.revert(user=self.user, commit=True) + + # Verify site is restored after revert + self.assertTrue(Site.objects.filter(id=site_id).exists()) + site = Site.objects.get(id=site_id) + self.assertEqual(site.name, original_name) + self.assertEqual(site.slug, original_slug) + def test_merge_basic_create_update_delete(self): """ - Test create, update, then delete same object. - Verifies object is skipped (not in main) after collapsing. + Test create, update, then delete same object with merge and revert. + Merge: skips object (not created in main) after collapsing + Revert: no-op since object was never created in main """ # Create branch branch = self._create_and_provision_branch() @@ -204,14 +233,22 @@ def test_merge_basic_create_update_delete(self): # Verify site does not exist in main (skipped during merge) self.assertFalse(Site.objects.filter(id=site_id).exists()) + # Revert branch + branch.revert(user=self.user, commit=True) + + # Verify no changes occurred (object was never created in main) + self.assertFalse(Site.objects.filter(id=site_id).exists()) + def test_merge_delete_then_create_same_slug(self): """ - Test delete object, then create object with same unique constraint value (slug). - Verifies deletes free up unique constraints before creates. + Test delete object, then create object with same unique constraint value (slug) with merge and revert. + Merge: deletes old object and creates new object with same slug + Revert: deletes new object and restores original object """ # Create site in main site1 = Site.objects.create(name='Site 1', slug='site-1') site1_id = site1.id + original_name = site1.name # Create branch branch = self._create_and_provision_branch() @@ -246,10 +283,22 @@ def test_merge_delete_then_create_same_slug(self): branch.refresh_from_db() self.assertEqual(branch.status, BranchStatusChoices.MERGED) + # Revert branch + branch.revert(user=self.user, commit=True) + + # Verify revert deleted new object and restored original + self.assertEqual(Site.objects.count(), 1) + self.assertTrue(Site.objects.filter(id=site1_id).exists()) + self.assertFalse(Site.objects.filter(id=site2_id).exists()) + site1_restored = Site.objects.get(id=site1_id) + self.assertEqual(site1_restored.name, original_name) + self.assertEqual(site1_restored.slug, 'site-1') + def test_merge_create_device_and_delete_old(self): """ - Test create new object, then delete old object. - Verifies proper ordering with cascade delete dependencies. + Test create new object, then delete old object with merge and revert. + Merge: creates new device with interface and deletes old device with interface + Revert: restores old device with interface and deletes new device with interface """ # Create device with interface in main site = Site.objects.create(name='Site 1', slug='site-1') @@ -260,6 +309,7 @@ def test_merge_create_device_and_delete_old(self): role=self.device_role ) device_a_id = device_a.id + device_a_name = device_a.name interface_a = Interface.objects.create( device=device_a, @@ -267,6 +317,7 @@ def test_merge_create_device_and_delete_old(self): type='1000base-t' ) interface_a_id = interface_a.id + interface_a_name = interface_a.name # Create branch branch = self._create_and_provision_branch() @@ -306,14 +357,31 @@ def test_merge_create_device_and_delete_old(self): self.assertTrue(Device.objects.filter(id=device_b_id).exists()) self.assertTrue(Interface.objects.filter(id=interface_b_id).exists()) + # Revert branch + branch.revert(user=self.user, commit=True) + + # Verify revert restored old device/interface and deleted new ones + self.assertTrue(Device.objects.filter(id=device_a_id).exists()) + self.assertTrue(Interface.objects.filter(id=interface_a_id).exists()) + self.assertFalse(Device.objects.filter(id=device_b_id).exists()) + self.assertFalse(Interface.objects.filter(id=interface_b_id).exists()) + + device_a_restored = Device.objects.get(id=device_a_id) + self.assertEqual(device_a_restored.name, device_a_name) + + interface_a_restored = Interface.objects.get(id=interface_a_id) + self.assertEqual(interface_a_restored.name, interface_a_name) + def test_merge_slug_rename_then_create(self): """ - Test update object's unique field, then create new object with old value. - Verifies updates free up unique constraints before creates. + Test update object's unique field, then create new object with old value with merge and revert. + Merge: updates first object's slug and creates new object with old slug + Revert: deletes new object and restores old slug on first object """ # Create site in main site1 = Site.objects.create(name='Site 1', slug='site-1') site1_id = site1.id + original_slug = site1.slug # Create branch branch = self._create_and_provision_branch() @@ -344,14 +412,26 @@ def test_merge_slug_rename_then_create(self): site2 = Site.objects.get(id=site2_id) self.assertEqual(site2.slug, 'site-1') + # Revert branch + branch.revert(user=self.user, commit=True) + + # Verify revert deleted new object and restored old slug + self.assertEqual(Site.objects.count(), 1) + self.assertFalse(Site.objects.filter(id=site2_id).exists()) + site1_restored = Site.objects.get(id=site1_id) + self.assertEqual(site1_restored.slug, original_slug) + def test_merge_multiple_updates_collapsed(self): """ - Test multiple updates to same object. - Verifies consecutive non-referencing updates are collapsed. + Test multiple updates to same object with merge and revert. + Merge: applies collapsed updates to object + Revert: restores object to original state """ # Create site in main site = Site.objects.create(name='Site 1', slug='site-1', description='Original') site_id = site.id + original_name = site.name + original_description = site.description # Create branch branch = self._create_and_provision_branch() @@ -382,10 +462,19 @@ def test_merge_multiple_updates_collapsed(self): self.assertEqual(site.name, 'Site 1 Modified') self.assertEqual(site.description, 'Update 2') + # Revert branch + branch.revert(user=self.user, commit=True) + + # Verify revert restored original state + site = Site.objects.get(id=site_id) + self.assertEqual(site.name, original_name) + self.assertEqual(site.description, original_description) + def test_merge_create_with_multiple_updates(self): """ - Test create object then update it multiple times. - Verifies create is kept separate from updates. + Test create object then update it multiple times with merge and revert. + Merge: creates object with final state after collapsed updates + Revert: deletes the created object """ # Create branch branch = self._create_and_provision_branch() @@ -416,10 +505,17 @@ def test_merge_create_with_multiple_updates(self): self.assertEqual(site.description, 'Modified 2') self.assertEqual(site.slug, 'new-site') + # Revert branch + branch.revert(user=self.user, commit=True) + + # Verify revert deleted the created object + self.assertFalse(Site.objects.filter(id=site_id).exists()) + def test_merge_complex_dependency_chain(self): """ - Test complex scenario with multiple creates, updates, and deletes. - Verifies correct ordering with FK dependencies and references. + Test complex scenario with multiple creates, updates, and deletes with merge and revert. + Merge: creates new devices with interfaces, updates device, and deletes old device + Revert: deletes all created objects in reverse order and restores deleted device """ # Create initial devices in main site = Site.objects.create(name='Site 1', slug='site-1') @@ -430,6 +526,7 @@ def test_merge_complex_dependency_chain(self): role=self.device_role ) device_a_id = device_a.id + device_a_name = device_a.name # Create branch branch = self._create_and_provision_branch() @@ -459,18 +556,20 @@ def test_merge_complex_dependency_chain(self): device_c_id = device_c.id # Create interface on device_b - Interface.objects.create( + interface_b1 = Interface.objects.create( device=device_b, name='eth0', type='1000base-t' ) + interface_b1_id = interface_b1.id # Create another interface on device_b - Interface.objects.create( + interface_b2 = Interface.objects.create( device=device_b, name='eth1', type='1000base-t' ) + interface_b2_id = interface_b2.id # Update device_b device_b.name = 'Device B Updated' @@ -491,10 +590,24 @@ def test_merge_complex_dependency_chain(self): self.assertEqual(device_b.name, 'Device B Updated') self.assertEqual(device_b.interfaces.count(), 2) + # Revert branch + branch.revert(user=self.user, commit=True) + + # Verify revert deleted all created objects and restored deleted device + self.assertTrue(Device.objects.filter(id=device_a_id).exists()) + self.assertFalse(Device.objects.filter(id=device_b_id).exists()) + self.assertFalse(Device.objects.filter(id=device_c_id).exists()) + self.assertFalse(Interface.objects.filter(id=interface_b1_id).exists()) + self.assertFalse(Interface.objects.filter(id=interface_b2_id).exists()) + + device_a_restored = Device.objects.get(id=device_a_id) + self.assertEqual(device_a_restored.name, device_a_name) + def test_merge_conflicting_slug_create_update_delete(self): """ - Test create object with conflicting unique constraint, update it, then delete it. - Verifies skipped object doesn't cause constraint violations. + Test create object with conflicting unique constraint, update it, then delete it with merge and revert. + Merge: skips branch object (collapsed to no-op) + Revert: no-op since object was never created in main """ # Create branch branch = self._create_and_provision_branch() @@ -532,10 +645,19 @@ def test_merge_conflicting_slug_create_update_delete(self): branch.refresh_from_db() self.assertEqual(branch.status, BranchStatusChoices.MERGED) + # Revert branch + branch.revert(user=self.user, commit=True) + + # Verify no changes occurred (object was never created in main) + self.assertTrue(Site.objects.filter(id=site_main_id).exists()) + self.assertFalse(Site.objects.filter(id=site_branch_id).exists()) + self.assertEqual(Site.objects.filter(slug='conflict-slug').count(), 1) + def test_merge_slug_update_causes_then_resolves_conflict(self): """ - Test create object, update to conflicting unique constraint, then update to resolve. - Verifies final non-conflicting state merges successfully. + Test create object, update to conflicting unique constraint, then update to resolve with merge and revert. + Merge: creates object with final non-conflicting slug + Revert: deletes the created object """ # Create branch branch = self._create_and_provision_branch() @@ -578,3 +700,10 @@ def test_merge_slug_update_causes_then_resolves_conflict(self): # Verify branch status branch.refresh_from_db() self.assertEqual(branch.status, BranchStatusChoices.MERGED) + + # Revert branch + branch.revert(user=self.user, commit=True) + + # Verify revert deleted the created object + self.assertTrue(Site.objects.filter(id=site_main_id).exists()) + self.assertFalse(Site.objects.filter(id=site_branch_id).exists()) From 14a9e6c752724d1dc0838541b2fc3fde838f0663 Mon Sep 17 00:00:00 2001 From: Arthur Date: Wed, 19 Nov 2025 16:10:55 -0800 Subject: [PATCH 13/38] revert tests and fixes --- netbox_branching/models/branches.py | 124 ++++++++++++++++++--------- netbox_branching/tests/test_merge.py | 30 ++++++- 2 files changed, 112 insertions(+), 42 deletions(-) diff --git a/netbox_branching/models/branches.py b/netbox_branching/models/branches.py index bfb4cd7..c6db26d 100644 --- a/netbox_branching/models/branches.py +++ b/netbox_branching/models/branches.py @@ -512,7 +512,8 @@ def __init__(self, key, model_class): self.model_class = model_class self.changes = [] # List of ObjectChange instances, ordered by time self.final_action = None # 'create', 'update', 'delete', or 'skip' - self.merged_data = None # The collapsed postchange_data + self.prechange_data = None # The original state (from first ObjectChange) + self.postchange_data = None # The final state (collapsed from all changes) self.last_change = None # The most recent ObjectChange (for metadata) # Dependencies for ordering @@ -526,11 +527,44 @@ def __repr__(self): f"action={self.final_action} changes={len(self.changes)}>" ) + @staticmethod + def _diff_object_change_data(action, prechange_data, postchange_data): + """ + Compute diff between prechange_data and postchange_data for a given action. + Mirrors the logic of ObjectChange.diff() method. + + Returns: dict with 'pre' and 'post' keys containing changed attributes + """ + from utilities.data import shallow_compare_dict + from core.choices import ObjectChangeActionChoices + + prechange_data = prechange_data or {} + postchange_data = postchange_data or {} + + # Determine which attributes have changed + if action == ObjectChangeActionChoices.ACTION_CREATE: + changed_attrs = sorted(postchange_data.keys()) + elif action == ObjectChangeActionChoices.ACTION_DELETE: + changed_attrs = sorted(prechange_data.keys()) + else: + # TODO: Support deep (recursive) comparison + changed_data = shallow_compare_dict(prechange_data, postchange_data) + changed_attrs = sorted(changed_data.keys()) + + return { + 'pre': { + k: prechange_data.get(k) for k in changed_attrs + }, + 'post': { + k: postchange_data.get(k) for k in changed_attrs + }, + } + @staticmethod def _collapse_changes_for_object(changes, logger): """ Collapse a list of ObjectChanges for a single object. - Returns: (final_action, merged_data, last_change) + Returns: (final_action, prechange_data, postchange_data, last_change) 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. @@ -546,7 +580,7 @@ def _collapse_changes_for_object(changes, logger): """ if not changes: - return None, None, None + return None, None, None, None # Sort by time (oldest first) changes = sorted(changes, key=lambda c: c.time) @@ -561,43 +595,50 @@ def _collapse_changes_for_object(changes, logger): if has_create: # CREATE + DELETE = skip entirely logger.debug(" -> Action: SKIP (created and deleted in branch)") - return 'skip', None, changes[-1] + return 'skip', None, None, changes[-1] else: # Just DELETE (ignore all other changes like updates) - # Use prechange_data from first ObjectChange (the original state) + # prechange_data: original state from first change + # postchange_data: postchange_data from DELETE ObjectChange logger.debug(f" -> Action: DELETE (keeping only DELETE, ignoring {len(changes) - 1} other changes)") delete_change = next(c for c in changes if c.action == 'delete') - - # Copy prechange_data from first change to ensure we have the original state - delete_change.prechange_data = changes[0].prechange_data - - return 'delete', delete_change.prechange_data, delete_change + prechange_data = changes[0].prechange_data + postchange_data = delete_change.postchange_data # Should be None for DELETE, but use actual value + return 'delete', prechange_data, postchange_data, delete_change # No DELETE - handle CREATE or UPDATEs first_action = changes[0].action + first_change = changes[0] last_change = changes[-1] # Created (with possible updates) -> single create if first_action == 'create': - merged_data = {} + # prechange_data: from first ObjectChange (should be None for CREATE) + # postchange_data: merged from all changes + prechange_data = first_change.prechange_data + postchange_data = {} for change in changes: # Merge postchange_data, later changes overwrite earlier ones if change.postchange_data: - merged_data.update(change.postchange_data) + postchange_data.update(change.postchange_data) logger.debug(f" -> Action: CREATE (collapsed {len(changes)} changes)") - return 'create', merged_data, last_change + return 'create', prechange_data, postchange_data, last_change # Only updates -> single update - merged_data = {} + # prechange_data: original state from first change + # postchange_data: final state after all updates + prechange_data = first_change.prechange_data + postchange_data = {} # Start with prechange_data of first change as baseline - if changes[0].prechange_data: - merged_data.update(changes[0].prechange_data) + if prechange_data: + postchange_data.update(prechange_data) # Apply each change's postchange_data for change in changes: if change.postchange_data: - merged_data.update(change.postchange_data) + postchange_data.update(change.postchange_data) + logger.debug(f" -> Action: UPDATE (collapsed {len(changes)} changes)") - return 'update', merged_data, last_change + return 'update', prechange_data, postchange_data, last_change @staticmethod def _get_fk_references(model_class, data, changed_objects): @@ -764,10 +805,10 @@ def _order_collapsed_changes(collapsed_changes, logger): # 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: + if update.postchange_data: postchange_refs = Branch._get_fk_references( update.model_class, - update.merged_data, + update.postchange_data, creates_map.keys() ) for ref_key in postchange_refs: @@ -783,11 +824,11 @@ def _order_collapsed_changes(collapsed_changes, logger): # 2. Check CREATEs for dependencies on other CREATEs logger.debug(" Analyzing CREATE dependencies...") for create in creates: - if create.merged_data: + if create.postchange_data: # Check if this CREATE references other created objects refs = Branch._get_fk_references( create.model_class, - create.merged_data, + create.postchange_data, creates_map.keys() ) for ref_key in refs: @@ -834,9 +875,9 @@ def _apply_collapsed_change(self, collapsed, using=DEFAULT_DB_ALIAS, logger=None logger.debug(f' Creating {model._meta.verbose_name} {object_id}') if hasattr(model, 'deserialize_object'): - instance = model.deserialize_object(collapsed.merged_data, pk=object_id) + instance = model.deserialize_object(collapsed.postchange_data, pk=object_id) else: - instance = deserialize_object(model, collapsed.merged_data, pk=object_id) + instance = deserialize_object(model, collapsed.postchange_data, pk=object_id) try: instance.object.full_clean() @@ -860,7 +901,7 @@ def _apply_collapsed_change(self, collapsed, using=DEFAULT_DB_ALIAS, logger=None # We need to figure out what changed between initial and final state first_change = collapsed.changes[0] initial_data = first_change.prechange_data or {} - final_data = collapsed.merged_data or {} + final_data = collapsed.postchange_data or {} # Only update fields that actually changed changed_fields = {} @@ -885,11 +926,12 @@ def _apply_collapsed_change(self, collapsed, using=DEFAULT_DB_ALIAS, logger=None def _undo_collapsed_change(self, collapsed, using=DEFAULT_DB_ALIAS, logger=None): """ Undo a collapsed change from the database (reverse of apply). - Similar to ObjectChange.undo() but works with collapsed data. + Follows the same pattern as ObjectChange.undo(). """ from django.contrib.contenttypes.fields import GenericForeignKey from utilities.serialization import deserialize_object from netbox_branching.utilities import update_object + from core.choices import ObjectChangeActionChoices logger = logger or logging.getLogger('netbox_branching.branch._undo_collapsed_change') model = collapsed.model_class @@ -902,7 +944,6 @@ def _undo_collapsed_change(self, collapsed, using=DEFAULT_DB_ALIAS, logger=None) # Undoing a CREATE: delete the object if collapsed.final_action == 'create': logger.debug(f' Undoing creation of {model._meta.verbose_name} {object_id}') - try: instance = model.objects.using(using).get(pk=object_id) instance.delete(using=using) @@ -913,15 +954,15 @@ def _undo_collapsed_change(self, collapsed, using=DEFAULT_DB_ALIAS, logger=None) elif collapsed.final_action == 'update': logger.debug(f' Undoing update of {model._meta.verbose_name} {object_id}') - # Get the original state from the first change - first_change = collapsed.changes[0] - # Make a copy to avoid modifying cached property result - revert_data = dict(first_change.diff()['pre']) - - # Load the instance and apply the original state try: instance = model.objects.using(using).get(pk=object_id) - update_object(instance, revert_data, using=using) + # Compute diff and apply 'pre' values (like ObjectChange.undo() does) + diff = Branch._diff_object_change_data( + ObjectChangeActionChoices.ACTION_UPDATE, + collapsed.prechange_data, + collapsed.postchange_data + ) + update_object(instance, diff['pre'], using=using) except model.DoesNotExist: logger.debug(f' {model._meta.verbose_name} {object_id} does not exist; skipping') @@ -929,10 +970,9 @@ def _undo_collapsed_change(self, collapsed, using=DEFAULT_DB_ALIAS, logger=None) elif collapsed.final_action == 'delete': logger.debug(f' Undoing deletion (restoring) {model._meta.verbose_name} {object_id}') - # Use prechange_data from the first change (the original state before any changes) - first_change = collapsed.changes[0] - prechange_data = first_change.prechange_data or {} + prechange_data = collapsed.prechange_data or {} + # Restore from prechange_data (like ObjectChange.undo() does) deserialized = deserialize_object(model, prechange_data, pk=object_id) instance = deserialized.object @@ -1005,11 +1045,12 @@ def merge(self, user, commit=True): # Collapse each object's changes logger.info("Determining final action for each object...") for key, collapsed in collapsed_changes.items(): - final_action, merged_data, last_change = Branch._collapse_changes_for_object( + final_action, prechange_data, postchange_data, last_change = Branch._collapse_changes_for_object( collapsed.changes, logger ) collapsed.final_action = final_action - collapsed.merged_data = merged_data + collapsed.prechange_data = prechange_data + collapsed.postchange_data = postchange_data collapsed.last_change = last_change # Order collapsed changes based on dependencies @@ -1128,11 +1169,12 @@ def revert(self, user, commit=True): # Collapse each object's changes logger.info("Determining final action for each object...") for key, collapsed in collapsed_changes.items(): - final_action, merged_data, last_change = Branch._collapse_changes_for_object( + final_action, prechange_data, postchange_data, last_change = Branch._collapse_changes_for_object( collapsed.changes, logger ) collapsed.final_action = final_action - collapsed.merged_data = merged_data + collapsed.prechange_data = prechange_data + collapsed.postchange_data = postchange_data collapsed.last_change = last_change # Order collapsed changes based on dependencies (same as merge) diff --git a/netbox_branching/tests/test_merge.py b/netbox_branching/tests/test_merge.py index 35661fb..8fe4561 100644 --- a/netbox_branching/tests/test_merge.py +++ b/netbox_branching/tests/test_merge.py @@ -44,10 +44,31 @@ def tearDown(self): def _create_and_provision_branch(self, name='Test Branch'): """Helper to create and provision a branch.""" + import time + branch = Branch(name=name) branch.save(provision=False) + print(f"branch status: {branch.status}") branch.provision(user=self.user) - branch.refresh_from_db() # Refresh to get updated status + + # Wait for branch to be provisioned (background task) + max_wait = 30 # Maximum 30 seconds + wait_interval = 0.1 # Check every 100ms + elapsed = 0 + + while elapsed < max_wait: + branch.refresh_from_db() + print(f"checking branch status: {branch.status}") + if branch.status == BranchStatusChoices.READY: + break + time.sleep(wait_interval) + elapsed += wait_interval + else: + raise TimeoutError( + f"Branch {branch.name} did not become READY within {max_wait} seconds. " + f"Status: {branch.status}" + ) + return branch def test_merge_basic_create(self): @@ -126,6 +147,13 @@ def test_merge_basic_update(self): changed_object_type=site_ct, changed_object_id=site_id ) + print("") + print("--------------------------------") + for change in changes: + print(f"change.action: {change.action}") + print(f"change.prechange_data: {change.prechange_data}") + print(f"change.postchange_data: {change.postchange_data}") + print("") self.assertEqual(changes.count(), 1) self.assertEqual(changes.first().action, 'update') From 177a9783818c821a23fa2ecc2cd37ece97e94a03 Mon Sep 17 00:00:00 2001 From: Arthur Date: Wed, 19 Nov 2025 16:59:00 -0800 Subject: [PATCH 14/38] fix tests --- netbox_branching/tests/test_merge.py | 45 ++++++++++++++++++---------- 1 file changed, 29 insertions(+), 16 deletions(-) diff --git a/netbox_branching/tests/test_merge.py b/netbox_branching/tests/test_merge.py index 8fe4561..6d3af63 100644 --- a/netbox_branching/tests/test_merge.py +++ b/netbox_branching/tests/test_merge.py @@ -48,7 +48,6 @@ def _create_and_provision_branch(self, name='Test Branch'): branch = Branch(name=name) branch.save(provision=False) - print(f"branch status: {branch.status}") branch.provision(user=self.user) # Wait for branch to be provisioned (background task) @@ -58,7 +57,6 @@ def _create_and_provision_branch(self, name='Test Branch'): while elapsed < max_wait: branch.refresh_from_db() - print(f"checking branch status: {branch.status}") if branch.status == BranchStatusChoices.READY: break time.sleep(wait_interval) @@ -121,22 +119,25 @@ def test_merge_basic_update(self): Merge: updates object in main Revert: restores object to original state """ - # Create site in main - site = Site.objects.create(name='Original Site', slug='test-site', description='Original') + # Create a request context for creating the site + request = RequestFactory().get(reverse('home')) + request.id = uuid.uuid4() + request.user = self.user + + # Create site in main WITH event tracking (like the real app does) + with event_tracking(request): + site = Site.objects.create(name='Original Site', slug='test-site', description='Original', custom_field_data={}) site_id = site.id original_description = site.description # Create branch branch = self._create_and_provision_branch() - # Create a request context for event tracking - request = RequestFactory().get(reverse('home')) - request.id = uuid.uuid4() - request.user = self.user - # In branch: update site with activate_branch(branch), event_tracking(request): site = Site.objects.get(id=site_id) + # IMPORTANT: Call snapshot() before modifying (like views/API do) + site.snapshot() site.description = 'Updated' site.save() @@ -147,13 +148,7 @@ def test_merge_basic_update(self): changed_object_type=site_ct, changed_object_id=site_id ) - print("") - print("--------------------------------") - for change in changes: - print(f"change.action: {change.action}") - print(f"change.prechange_data: {change.prechange_data}") - print(f"change.postchange_data: {change.postchange_data}") - print("") + self.assertEqual(changes.count(), 1) self.assertEqual(changes.first().action, 'update') @@ -239,6 +234,8 @@ def test_merge_basic_create_update_delete(self): site = Site.objects.create(name='Temp Site', slug='temp-site') site_id = site.id + # IMPORTANT: Call snapshot() before modifying (like views/API do) + site.snapshot() site.description = 'Modified' site.save() @@ -422,6 +419,8 @@ def test_merge_slug_rename_then_create(self): # In branch: rename site1 slug, create new site with old slug with activate_branch(branch), event_tracking(request): site1 = Site.objects.get(id=site1_id) + # IMPORTANT: Call snapshot() before modifying (like views/API do) + site1.snapshot() site1.slug = 'site-1-renamed' site1.save() @@ -473,12 +472,16 @@ def test_merge_multiple_updates_collapsed(self): with activate_branch(branch), event_tracking(request): site = Site.objects.get(id=site_id) + # IMPORTANT: Call snapshot() before modifying (like views/API do) + site.snapshot() site.description = 'Update 1' site.save() + site.snapshot() site.description = 'Update 2' site.save() + site.snapshot() site.name = 'Site 1 Modified' site.save() @@ -517,9 +520,12 @@ def test_merge_create_with_multiple_updates(self): site = Site.objects.create(name='New Site', slug='new-site', description='Initial') site_id = site.id + # IMPORTANT: Call snapshot() before modifying (like views/API do) + site.snapshot() site.description = 'Modified 1' site.save() + site.snapshot() site.description = 'Modified 2' site.name = 'New Site Final' site.save() @@ -600,6 +606,8 @@ def test_merge_complex_dependency_chain(self): interface_b2_id = interface_b2.id # Update device_b + # IMPORTANT: Call snapshot() before modifying (like views/API do) + device_b.snapshot() device_b.name = 'Device B Updated' device_b.save() @@ -655,6 +663,8 @@ def test_merge_conflicting_slug_create_update_delete(self): site_branch_id = site_branch.id # Update description + # IMPORTANT: Call snapshot() before modifying (like views/API do) + site_branch.snapshot() site_branch.description = 'Updated in branch' site_branch.save() @@ -705,10 +715,13 @@ def test_merge_slug_update_causes_then_resolves_conflict(self): site_branch_id = site_branch.id # Update to conflicting slug + # IMPORTANT: Call snapshot() before modifying (like views/API do) + site_branch.snapshot() site_branch.slug = 'conflict-slug' site_branch.save() # Update again to resolve conflict + site_branch.snapshot() site_branch.slug = 'resolved-slug' site_branch.save() From c2dfa0bcf38cbef9de64db8afdb703eb7261273e Mon Sep 17 00:00:00 2001 From: Arthur Date: Wed, 19 Nov 2025 17:10:48 -0800 Subject: [PATCH 15/38] fix tests --- netbox_branching/models/branches.py | 130 ++++++++++++++++++++++++--- netbox_branching/tests/test_merge.py | 4 +- 2 files changed, 123 insertions(+), 11 deletions(-) diff --git a/netbox_branching/models/branches.py b/netbox_branching/models/branches.py index c6db26d..9847462 100644 --- a/netbox_branching/models/branches.py +++ b/netbox_branching/models/branches.py @@ -715,6 +715,120 @@ def _topological_sort_with_cycle_detection(collapsed_changes, logger): logger.info(f" Topological sort completed: {len(ordered)} changes ordered") return ordered + @staticmethod + def _order_collapsed_changes_for_revert(collapsed_changes, logger): + """ + Order collapsed changes for revert operation, respecting dependencies. + + For revert, we're undoing operations: + 1. Undo CREATEs (delete created objects): children before parents + 2. Undo UPDATEs (restore original values): any order + 3. Undo DELETEs (restore deleted objects): PARENTS before CHILDREN + + Algorithm: + 1. Initial ordering: Undo CREATEs, Undo UPDATEs, Undo DELETEs + 2. Build dependency graph: + - For undo CREATEs: if object A references object B, delete A before B (children first) + - For undo DELETEs: if object A referenced object B, restore B before A (parents first) + 3. Topological sort respecting dependencies + + Returns: ordered list of CollapsedChange objects + """ + logger.info(f"Ordering {len(collapsed_changes)} collapsed changes for revert...") + + # Remove skipped objects + to_process = {k: v for k, v in collapsed_changes.items() if v.final_action != 'skip'} + skipped = [v for v in collapsed_changes.values() if v.final_action == 'skip'] + + logger.info(f" {len(skipped)} changes will be skipped (created and deleted in branch)") + logger.info(f" {len(to_process)} changes to process") + + if not to_process: + return [] + + # Group by action type (what needs to be undone) + undo_creates = sorted( + [v for v in to_process.values() if v.final_action == 'create'], + key=lambda c: c.last_change.time + ) + undo_updates = sorted( + [v for v in to_process.values() if v.final_action == 'update'], + key=lambda c: c.last_change.time + ) + undo_deletes = sorted( + [v for v in to_process.values() if v.final_action == 'delete'], + key=lambda c: c.last_change.time + ) + + logger.info( + f" Revert groups: {len(undo_creates)} creates to undo, " + f"{len(undo_updates)} updates to undo, {len(undo_deletes)} deletes to undo" + ) + + # Reset dependencies + for collapsed in to_process.values(): + collapsed.depends_on = set() + collapsed.depended_by = set() + + # Build lookup maps + undo_creates_map = {c.key: c for c in undo_creates} + undo_deletes_map = {c.key: c for c in undo_deletes} + + logger.info("Building dependency graph for revert...") + + # 1. For undo CREATEs: if A references B, must delete A before B (children first) + logger.debug(" Analyzing undo CREATE dependencies...") + for undo_create in undo_creates: + if undo_create.postchange_data: + refs = Branch._get_fk_references( + undo_create.model_class, + undo_create.postchange_data, + undo_creates_map.keys() + ) + for ref_key in refs: + if ref_key != undo_create.key: + # This object references another created object + # Must delete this object BEFORE the referenced object + ref_create = undo_creates_map[ref_key] + ref_create.depends_on.add(undo_create.key) + undo_create.depended_by.add(ref_key) + logger.debug( + f" Undo {ref_create} depends on undo {undo_create} " + f"(delete child before parent)" + ) + + # 2. For undo DELETEs: if A referenced B, must restore B before A (parents first) + logger.debug(" Analyzing undo DELETE dependencies...") + for undo_delete in undo_deletes: + if undo_delete.prechange_data: + refs = Branch._get_fk_references( + undo_delete.model_class, + undo_delete.prechange_data, + undo_deletes_map.keys() + ) + for ref_key in refs: + # This object referenced another deleted object + # Must restore the referenced object BEFORE this object + ref_delete = undo_deletes_map[ref_key] + undo_delete.depends_on.add(ref_key) + ref_delete.depended_by.add(undo_delete.key) + logger.debug( + f" Undo {undo_delete} depends on undo {ref_delete} " + f"(restore parent before child)" + ) + + # 3. Use topological sort to order all changes + total_deps = sum(len(c.depends_on) for c in to_process.values()) + logger.info(f" Dependency graph built: {total_deps} dependencies") + logger.info(" Performing topological sort...") + ordered_keys = Branch._topological_sort_with_cycle_detection(to_process, logger) + + # Convert keys back to CollapsedChange objects + ordered = [to_process[key] for key in ordered_keys] + + logger.info(f" Revert ordering completed: {len(ordered)} changes ordered") + return ordered + @staticmethod def _order_collapsed_changes(collapsed_changes, logger): """ @@ -1177,23 +1291,19 @@ def revert(self, user, commit=True): collapsed.postchange_data = postchange_data collapsed.last_change = last_change - # Order collapsed changes based on dependencies (same as merge) - ordered_changes = Branch._order_collapsed_changes(collapsed_changes, logger) - - # REVERSE the order for revert (undo in opposite order) - logger.info(f"Reversing order for revert: {len(ordered_changes)} collapsed changes") - reversed_changes = list(reversed(ordered_changes)) + # Order collapsed changes for revert (handles dependencies differently than merge) + ordered_changes = Branch._order_collapsed_changes_for_revert(collapsed_changes, logger) - # Undo collapsed changes in reverse order - logger.info(f"Undoing {len(reversed_changes)} collapsed changes in reverse order...") - for i, collapsed in enumerate(reversed_changes, 1): + # Undo collapsed changes in dependency order + logger.info(f"Undoing {len(ordered_changes)} collapsed changes in dependency order...") + for i, collapsed in enumerate(ordered_changes, 1): model_class = collapsed.model_class models.add(model_class) # Use the last change's metadata for tracking last_change = collapsed.last_change logger.info( - f"[{i}/{len(reversed_changes)}] Undoing {collapsed.final_action} " + f"[{i}/{len(ordered_changes)}] Undoing {collapsed.final_action} " f"{model_class._meta.verbose_name} (ID: {collapsed.key[1]})" ) diff --git a/netbox_branching/tests/test_merge.py b/netbox_branching/tests/test_merge.py index 6d3af63..1017d13 100644 --- a/netbox_branching/tests/test_merge.py +++ b/netbox_branching/tests/test_merge.py @@ -126,7 +126,9 @@ def test_merge_basic_update(self): # Create site in main WITH event tracking (like the real app does) with event_tracking(request): - site = Site.objects.create(name='Original Site', slug='test-site', description='Original', custom_field_data={}) + site = Site.objects.create( + name='Original Site', slug='test-site', description='Original', custom_field_data={} + ) site_id = site.id original_description = site.description From 5bea48b3e30ef18f581fbb8497e6be86fcc4cc2f Mon Sep 17 00:00:00 2001 From: Arthur Date: Thu, 20 Nov 2025 11:40:48 -0800 Subject: [PATCH 16/38] retain old code --- netbox_branching/models/branches.py | 166 +++++++++++++++++++++++++++- 1 file changed, 163 insertions(+), 3 deletions(-) diff --git a/netbox_branching/models/branches.py b/netbox_branching/models/branches.py index 9847462..df359fe 100644 --- a/netbox_branching/models/branches.py +++ b/netbox_branching/models/branches.py @@ -1101,7 +1101,7 @@ def _undo_collapsed_change(self, collapsed, using=DEFAULT_DB_ALIAS, logger=None) instance.full_clean() instance.save(using=using) - def merge(self, user, commit=True): + def merge_collapsed(self, user, commit=True): """ Apply all changes in the Branch to the main schema by replaying them in chronological order. @@ -1223,9 +1223,9 @@ def merge(self, user, commit=True): # Disconnect the signal receiver post_save.disconnect(handler, sender=ObjectChange_) - merge.alters_data = True + merge_collapsed.alters_data = True - def revert(self, user, commit=True): + def revert_collapsed(self, user, commit=True): """ Undo all changes associated with a previously merged Branch in the main schema. Uses the same collapsing logic as merge, but applies changes in reverse order. @@ -1345,6 +1345,166 @@ def revert(self, user, commit=True): # Disconnect the signal receiver post_save.disconnect(handler, sender=ObjectChange_) + revert_collapsed.alters_data = True + + def merge(self, user, commit=True): + """ + Apply all changes in the Branch to the main schema by replaying them in + chronological order. + """ + logger = logging.getLogger('netbox_branching.branch.merge') + logger.info(f'Merging branch {self} ({self.schema_name})') + + if not self.ready: + raise Exception(f"Branch {self} is not ready to merge") + if commit and not self.can_merge: + raise Exception("Merging this branch is not permitted.") + + # Emit pre-merge signal + pre_merge.send(sender=self.__class__, branch=self, user=user) + + # Retrieve staged changes before we update the Branch's status + if changes := self.get_unmerged_changes().order_by('time'): + logger.info(f"Found {len(changes)} changes to merge") + else: + logger.info("No changes found; aborting.") + return + + # Update Branch status + logger.debug(f"Setting branch status to {BranchStatusChoices.MERGING}") + Branch.objects.filter(pk=self.pk).update(status=BranchStatusChoices.MERGING) + + # Create a dummy request for the event_tracking() context manager + request = RequestFactory().get(reverse('home')) + + # Prep & connect the signal receiver for recording AppliedChanges + handler = partial(record_applied_change, branch=self) + post_save.connect(handler, sender=ObjectChange_, weak=False) + + try: + with transaction.atomic(): + models = set() + + # Apply each change from the Branch + for change in changes: + models.add(change.changed_object_type.model_class()) + with event_tracking(request): + request.id = change.request_id + request.user = change.user + change.apply(self, using=DEFAULT_DB_ALIAS, logger=logger) + if not commit: + raise AbortTransaction() + + # Perform cleanup tasks + self._cleanup(models) + + except Exception as e: + if err_message := str(e): + logger.error(err_message) + # Disconnect signal receiver & restore original branch status + post_save.disconnect(handler, sender=ObjectChange_) + Branch.objects.filter(pk=self.pk).update(status=BranchStatusChoices.READY) + raise e + + # Update the Branch's status to "merged" + logger.debug(f"Setting branch status to {BranchStatusChoices.MERGED}") + self.status = BranchStatusChoices.MERGED + self.merged_time = timezone.now() + self.merged_by = user + self.save() + + # Record a branch event for the merge + logger.debug(f"Recording branch event: {BranchEventTypeChoices.MERGED}") + BranchEvent.objects.create(branch=self, user=user, type=BranchEventTypeChoices.MERGED) + + # Emit post-merge signal + post_merge.send(sender=self.__class__, branch=self, user=user) + + logger.info('Merging completed') + + # Disconnect the signal receiver + post_save.disconnect(handler, sender=ObjectChange_) + + merge.alters_data = True + + def revert(self, user, commit=True): + """ + Undo all changes associated with a previously merged Branch in the main schema by replaying them in + reverse order and calling undo() on each. + """ + logger = logging.getLogger('netbox_branching.branch.revert') + logger.info(f'Reverting branch {self} ({self.schema_name})') + + if not self.merged: + raise Exception("Only merged branches can be reverted.") + if commit and not self.can_revert: + raise Exception("Reverting this branch is not permitted.") + + # Emit pre-revert signal + pre_revert.send(sender=self.__class__, branch=self, user=user) + + # Retrieve applied changes before we update the Branch's status + if changes := self.get_changes().order_by('-time'): + logger.info(f"Found {len(changes)} changes to revert") + else: + logger.info("No changes found; aborting.") + return + + # Update Branch status + logger.debug(f"Setting branch status to {BranchStatusChoices.REVERTING}") + Branch.objects.filter(pk=self.pk).update(status=BranchStatusChoices.REVERTING) + + # Create a dummy request for the event_tracking() context manager + request = RequestFactory().get(reverse('home')) + + # Prep & connect the signal receiver for recording AppliedChanges + handler = partial(record_applied_change, branch=self) + post_save.connect(handler, sender=ObjectChange_, weak=False) + + try: + with transaction.atomic(): + models = set() + + # Undo each change from the Branch + for change in changes: + models.add(change.changed_object_type.model_class()) + with event_tracking(request): + request.id = change.request_id + request.user = change.user + change.undo(self, logger=logger) + if not commit: + raise AbortTransaction() + + # Perform cleanup tasks + self._cleanup(models) + + except Exception as e: + if err_message := str(e): + logger.error(err_message) + # Disconnect signal receiver & restore original branch status + post_save.disconnect(handler, sender=ObjectChange_) + Branch.objects.filter(pk=self.pk).update(status=BranchStatusChoices.MERGED) + raise e + + # Update the Branch's status to "ready" + logger.debug(f"Setting branch status to {BranchStatusChoices.READY}") + self.status = BranchStatusChoices.READY + self.merged_time = None + self.merged_by = None + self.save() + + # Record a branch event for the merge + logger.debug(f"Recording branch event: {BranchEventTypeChoices.REVERTED}") + BranchEvent.objects.create(branch=self, user=user, type=BranchEventTypeChoices.REVERTED) + + # Emit post-revert signal + post_revert.send(sender=self.__class__, branch=self, user=user) + + logger.info('Reversion completed') + + # Disconnect the signal receiver + post_save.disconnect(handler, sender=ObjectChange_) + revert.alters_data = True def _cleanup(self, models): From 874a2631a695cb1514ad672e198d682ba990f067 Mon Sep 17 00:00:00 2001 From: Arthur Date: Thu, 20 Nov 2025 11:52:33 -0800 Subject: [PATCH 17/38] refactor --- netbox_branching/models/branches.py | 402 +++++++++++----------------- 1 file changed, 150 insertions(+), 252 deletions(-) diff --git a/netbox_branching/models/branches.py b/netbox_branching/models/branches.py index df359fe..3e27fbc 100644 --- a/netbox_branching/models/branches.py +++ b/netbox_branching/models/branches.py @@ -1101,251 +1101,87 @@ def _undo_collapsed_change(self, collapsed, using=DEFAULT_DB_ALIAS, logger=None) instance.full_clean() instance.save(using=using) - def merge_collapsed(self, user, commit=True): + def _merge_iterative(self, changes, request, commit, logger): """ - Apply all changes in the Branch to the main schema by replaying them in - chronological order. + Apply changes iteratively (one at a time) in chronological order. """ - logger = logging.getLogger('netbox_branching.branch.merge') - logger.info(f'Merging branch {self} ({self.schema_name})') - - if not self.ready: - raise Exception(f"Branch {self} is not ready to merge") - if commit and not self.can_merge: - raise Exception("Merging this branch is not permitted.") - - # Emit pre-merge signal - pre_merge.send(sender=self.__class__, branch=self, user=user) - - # Retrieve staged changes before we update the Branch's status - if changes := self.get_unmerged_changes().order_by('time'): - logger.info(f"Found {len(changes)} changes to merge") - else: - logger.info("No changes found; aborting.") - return + models = set() - # Update Branch status - logger.debug(f"Setting branch status to {BranchStatusChoices.MERGING}") - Branch.objects.filter(pk=self.pk).update(status=BranchStatusChoices.MERGING) - - # Create a dummy request for the event_tracking() context manager - request = RequestFactory().get(reverse('home')) - - # Prep & connect the signal receiver for recording AppliedChanges - handler = partial(record_applied_change, branch=self) - post_save.connect(handler, sender=ObjectChange_, weak=False) - - try: - with transaction.atomic(): - models = set() - - # Group and collapse changes by object - logger.info("Collapsing ObjectChanges by object...") - collapsed_changes = {} - - for change in changes: - key = (change.changed_object_type.id, change.changed_object_id) - - if key not in collapsed_changes: - model_class = change.changed_object_type.model_class() - collapsed = Branch.CollapsedChange(key, model_class) - collapsed_changes[key] = collapsed - logger.debug(f"New object: {model_class.__name__}:{change.changed_object_id}") - - collapsed_changes[key].changes.append(change) - - logger.info(f" {len(changes)} changes collapsed into {len(collapsed_changes)} objects") - - # Collapse each object's changes - logger.info("Determining final action for each object...") - for key, collapsed in collapsed_changes.items(): - final_action, prechange_data, postchange_data, last_change = Branch._collapse_changes_for_object( - collapsed.changes, logger - ) - collapsed.final_action = final_action - collapsed.prechange_data = prechange_data - collapsed.postchange_data = postchange_data - collapsed.last_change = last_change - - # Order collapsed changes based on dependencies - ordered_changes = Branch._order_collapsed_changes(collapsed_changes, logger) - - # Apply collapsed changes in order - logger.info(f"Applying {len(ordered_changes)} collapsed changes...") - for i, collapsed in enumerate(ordered_changes, 1): - model_class = collapsed.model_class - models.add(model_class) - - # Use the last change's metadata for tracking - last_change = collapsed.last_change - - logger.info(f" [{i}/{len(ordered_changes)}] {collapsed.final_action.upper()} " - f"{model_class.__name__}:{collapsed.key[1]} " - f"(from {len(collapsed.changes)} original changes)") - - with event_tracking(request): - request.id = last_change.request_id - request.user = last_change.user - - # Apply the collapsed change - self._apply_collapsed_change(collapsed, using=DEFAULT_DB_ALIAS, logger=logger) - - if not commit: - raise AbortTransaction() - - # Perform cleanup tasks - self._cleanup(models) - - except Exception as e: - if err_message := str(e): - logger.error(err_message) - # Disconnect signal receiver & restore original branch status - post_save.disconnect(handler, sender=ObjectChange_) - Branch.objects.filter(pk=self.pk).update(status=BranchStatusChoices.READY) - raise e - - # Update the Branch's status to "merged" - logger.debug(f"Setting branch status to {BranchStatusChoices.MERGED}") - self.status = BranchStatusChoices.MERGED - self.merged_time = timezone.now() - self.merged_by = user - self.save() - - # Record a branch event for the merge - logger.debug(f"Recording branch event: {BranchEventTypeChoices.MERGED}") - BranchEvent.objects.create(branch=self, user=user, type=BranchEventTypeChoices.MERGED) - - # Emit post-merge signal - post_merge.send(sender=self.__class__, branch=self, user=user) - - logger.info('Merging completed') - - # Disconnect the signal receiver - post_save.disconnect(handler, sender=ObjectChange_) + # Apply each change from the Branch + for change in changes: + models.add(change.changed_object_type.model_class()) + with event_tracking(request): + request.id = change.request_id + request.user = change.user + change.apply(self, using=DEFAULT_DB_ALIAS, logger=logger) + if not commit: + raise AbortTransaction() - merge_collapsed.alters_data = True + # Perform cleanup tasks + self._cleanup(models) - def revert_collapsed(self, user, commit=True): + def _merge_collapsed(self, changes, request, commit, logger): """ - Undo all changes associated with a previously merged Branch in the main schema. - Uses the same collapsing logic as merge, but applies changes in reverse order. + Apply changes after collapsing them by object and ordering by dependencies. """ - logger = logging.getLogger('netbox_branching.branch.revert') - logger.info(f'Reverting branch {self} ({self.schema_name})') + models = set() - if not self.merged: - raise Exception("Only merged branches can be reverted.") - if commit and not self.can_revert: - raise Exception("Reverting this branch is not permitted.") - - # Emit pre-revert signal - pre_revert.send(sender=self.__class__, branch=self, user=user) - - # Retrieve applied changes - if changes := self.get_changes(): - logger.info(f"Found {len(changes)} changes to revert") - else: - logger.info("No changes found; aborting.") - return - - # Update Branch status - logger.debug(f"Setting branch status to {BranchStatusChoices.REVERTING}") - Branch.objects.filter(pk=self.pk).update(status=BranchStatusChoices.REVERTING) - - # Create a dummy request for the event_tracking() context manager - request = RequestFactory().get(reverse('home')) + # Group and collapse changes by object + logger.info("Collapsing ObjectChanges by object...") + collapsed_changes = {} - # Prep & connect the signal receiver for recording AppliedChanges - handler = partial(record_applied_change, branch=self) - post_save.connect(handler, sender=ObjectChange_, weak=False) - - try: - with transaction.atomic(): - models = set() - - # Group changes by object and create CollapsedChange objects - logger.info("Collapsing ObjectChanges by object...") - collapsed_changes = {} - - for change in changes: - key = (change.changed_object_type.id, change.changed_object_id) - - if key not in collapsed_changes: - model_class = change.changed_object_type.model_class() - collapsed = Branch.CollapsedChange(key, model_class) - collapsed_changes[key] = collapsed - logger.debug(f"New object: {model_class.__name__}:{change.changed_object_id}") - - collapsed_changes[key].changes.append(change) + for change in changes: + key = (change.changed_object_type.id, change.changed_object_id) - logger.info(f" {len(changes)} changes collapsed into {len(collapsed_changes)} objects") + if key not in collapsed_changes: + model_class = change.changed_object_type.model_class() + collapsed = Branch.CollapsedChange(key, model_class) + collapsed_changes[key] = collapsed + logger.debug(f"New object: {model_class.__name__}:{change.changed_object_id}") - # Collapse each object's changes - logger.info("Determining final action for each object...") - for key, collapsed in collapsed_changes.items(): - final_action, prechange_data, postchange_data, last_change = Branch._collapse_changes_for_object( - collapsed.changes, logger - ) - collapsed.final_action = final_action - collapsed.prechange_data = prechange_data - collapsed.postchange_data = postchange_data - collapsed.last_change = last_change - - # Order collapsed changes for revert (handles dependencies differently than merge) - ordered_changes = Branch._order_collapsed_changes_for_revert(collapsed_changes, logger) - - # Undo collapsed changes in dependency order - logger.info(f"Undoing {len(ordered_changes)} collapsed changes in dependency order...") - for i, collapsed in enumerate(ordered_changes, 1): - model_class = collapsed.model_class - models.add(model_class) - - # Use the last change's metadata for tracking - last_change = collapsed.last_change - logger.info( - f"[{i}/{len(ordered_changes)}] Undoing {collapsed.final_action} " - f"{model_class._meta.verbose_name} (ID: {collapsed.key[1]})" - ) + collapsed_changes[key].changes.append(change) - with event_tracking(request): - request.id = last_change.request_id - request.user = last_change.user - self._undo_collapsed_change(collapsed, using=DEFAULT_DB_ALIAS, logger=logger) + logger.info(f" {len(changes)} changes collapsed into {len(collapsed_changes)} objects") - if not commit: - raise AbortTransaction() + # Collapse each object's changes + logger.info("Determining final action for each object...") + for key, collapsed in collapsed_changes.items(): + final_action, prechange_data, postchange_data, last_change = Branch._collapse_changes_for_object( + collapsed.changes, logger + ) + collapsed.final_action = final_action + collapsed.prechange_data = prechange_data + collapsed.postchange_data = postchange_data + collapsed.last_change = last_change - # Perform cleanup tasks - self._cleanup(models) + # Order collapsed changes based on dependencies + ordered_changes = Branch._order_collapsed_changes(collapsed_changes, logger) - except Exception as e: - if err_message := str(e): - logger.error(err_message) - # Disconnect signal receiver & restore original branch status - post_save.disconnect(handler, sender=ObjectChange_) - Branch.objects.filter(pk=self.pk).update(status=BranchStatusChoices.MERGED) - raise e + # Apply collapsed changes in order + logger.info(f"Applying {len(ordered_changes)} collapsed changes...") + for i, collapsed in enumerate(ordered_changes, 1): + model_class = collapsed.model_class + models.add(model_class) - # Update the Branch's status to "ready" - logger.debug(f"Setting branch status to {BranchStatusChoices.READY}") - self.status = BranchStatusChoices.READY - self.merged_time = None - self.merged_by = None - self.save() + # Use the last change's metadata for tracking + last_change = collapsed.last_change - # Record a branch event for the revert - logger.debug(f"Recording branch event: {BranchEventTypeChoices.REVERTED}") - BranchEvent.objects.create(branch=self, user=user, type=BranchEventTypeChoices.REVERTED) + logger.info(f" [{i}/{len(ordered_changes)}] {collapsed.final_action.upper()} " + f"{model_class.__name__}:{collapsed.key[1]} " + f"(from {len(collapsed.changes)} original changes)") - # Emit post-revert signal - post_revert.send(sender=self.__class__, branch=self, user=user) + with event_tracking(request): + request.id = last_change.request_id + request.user = last_change.user - logger.info('Reversion completed') + # Apply the collapsed change + self._apply_collapsed_change(collapsed, using=DEFAULT_DB_ALIAS, logger=logger) - # Disconnect the signal receiver - post_save.disconnect(handler, sender=ObjectChange_) + if not commit: + raise AbortTransaction() - revert_collapsed.alters_data = True + # Perform cleanup tasks + self._cleanup(models) def merge(self, user, commit=True): """ @@ -1383,20 +1219,11 @@ def merge(self, user, commit=True): try: with transaction.atomic(): - models = set() - - # Apply each change from the Branch - for change in changes: - models.add(change.changed_object_type.model_class()) - with event_tracking(request): - request.id = change.request_id - request.user = change.user - change.apply(self, using=DEFAULT_DB_ALIAS, logger=logger) - if not commit: - raise AbortTransaction() - - # Perform cleanup tasks - self._cleanup(models) + # Choose merge strategy + if True: + self._merge_iterative(changes, request, commit, logger) + else: + self._merge_collapsed(changes, request, commit, logger) except Exception as e: if err_message := str(e): @@ -1427,6 +1254,86 @@ def merge(self, user, commit=True): merge.alters_data = True + def _revert_iterative(self, changes, request, commit, logger): + """ + Undo changes iteratively (one at a time) in reverse chronological order. + """ + models = set() + + # Undo each change from the Branch + for change in changes: + models.add(change.changed_object_type.model_class()) + with event_tracking(request): + request.id = change.request_id + request.user = change.user + change.undo(self, logger=logger) + if not commit: + raise AbortTransaction() + + # Perform cleanup tasks + self._cleanup(models) + + def _revert_collapsed(self, changes, request, commit, logger): + """ + Undo changes after collapsing them by object and ordering by dependencies. + """ + models = set() + + # Group changes by object and create CollapsedChange objects + logger.info("Collapsing ObjectChanges by object...") + collapsed_changes = {} + + for change in changes: + key = (change.changed_object_type.id, change.changed_object_id) + + if key not in collapsed_changes: + model_class = change.changed_object_type.model_class() + collapsed = Branch.CollapsedChange(key, model_class) + collapsed_changes[key] = collapsed + logger.debug(f"New object: {model_class.__name__}:{change.changed_object_id}") + + collapsed_changes[key].changes.append(change) + + logger.info(f" {len(changes)} changes collapsed into {len(collapsed_changes)} objects") + + # Collapse each object's changes + logger.info("Determining final action for each object...") + for key, collapsed in collapsed_changes.items(): + final_action, prechange_data, postchange_data, last_change = Branch._collapse_changes_for_object( + collapsed.changes, logger + ) + collapsed.final_action = final_action + collapsed.prechange_data = prechange_data + collapsed.postchange_data = postchange_data + collapsed.last_change = last_change + + # Order collapsed changes for revert (handles dependencies differently than merge) + ordered_changes = Branch._order_collapsed_changes_for_revert(collapsed_changes, logger) + + # Undo collapsed changes in dependency order + logger.info(f"Undoing {len(ordered_changes)} collapsed changes in dependency order...") + for i, collapsed in enumerate(ordered_changes, 1): + model_class = collapsed.model_class + models.add(model_class) + + # Use the last change's metadata for tracking + last_change = collapsed.last_change + logger.info( + f"[{i}/{len(ordered_changes)}] Undoing {collapsed.final_action} " + f"{model_class._meta.verbose_name} (ID: {collapsed.key[1]})" + ) + + with event_tracking(request): + request.id = last_change.request_id + request.user = last_change.user + self._undo_collapsed_change(collapsed, using=DEFAULT_DB_ALIAS, logger=logger) + + if not commit: + raise AbortTransaction() + + # Perform cleanup tasks + self._cleanup(models) + def revert(self, user, commit=True): """ Undo all changes associated with a previously merged Branch in the main schema by replaying them in @@ -1463,20 +1370,11 @@ def revert(self, user, commit=True): try: with transaction.atomic(): - models = set() - - # Undo each change from the Branch - for change in changes: - models.add(change.changed_object_type.model_class()) - with event_tracking(request): - request.id = change.request_id - request.user = change.user - change.undo(self, logger=logger) - if not commit: - raise AbortTransaction() - - # Perform cleanup tasks - self._cleanup(models) + # Choose revert strategy + if True: + self._revert_iterative(changes, request, commit, logger) + else: + self._revert_collapsed(changes, request, commit, logger) except Exception as e: if err_message := str(e): From dafd98835fb5b84020010d5d227ee990dcf4374d Mon Sep 17 00:00:00 2001 From: Arthur Date: Thu, 20 Nov 2025 13:45:32 -0800 Subject: [PATCH 18/38] add selection for merge strategy --- netbox_branching/forms/misc.py | 12 ++++++++- .../0007_branch_merged_using_collapsed.py | 18 +++++++++++++ netbox_branching/models/branches.py | 26 +++++++++++++------ .../netbox_branching/branch_action.html | 3 +++ netbox_branching/views.py | 8 ++++-- 5 files changed, 56 insertions(+), 11 deletions(-) create mode 100644 netbox_branching/migrations/0007_branch_merged_using_collapsed.py diff --git a/netbox_branching/forms/misc.py b/netbox_branching/forms/misc.py index 2127ce1..3ac9cf1 100644 --- a/netbox_branching/forms/misc.py +++ b/netbox_branching/forms/misc.py @@ -20,14 +20,24 @@ class BranchActionForm(forms.Form): label=_('Commit changes'), help_text=_('Leave unchecked to perform a dry run') ) + collapse_changes = forms.BooleanField( + required=False, + initial=False, + label=_('Collapse Object Changes'), + help_text=_('Use the collapsed merge strategy') + ) - def __init__(self, branch, *args, allow_commit=True, **kwargs): + def __init__(self, branch, *args, allow_commit=True, action=None, **kwargs): self.branch = branch super().__init__(*args, **kwargs) if not allow_commit: self.fields['commit'].disabled = True + # Only show collapse_changes for merge operations, not revert + if action == 'revert': + del self.fields['collapse_changes'] + def clean(self): super().clean() diff --git a/netbox_branching/migrations/0007_branch_merged_using_collapsed.py b/netbox_branching/migrations/0007_branch_merged_using_collapsed.py new file mode 100644 index 0000000..6f6110b --- /dev/null +++ b/netbox_branching/migrations/0007_branch_merged_using_collapsed.py @@ -0,0 +1,18 @@ +# Generated by Django 5.2.8 on 2025-11-20 21:30 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("netbox_branching", "0006_tag_object_types"), + ] + + operations = [ + migrations.AddField( + model_name="branch", + name="merged_using_collapsed", + field=models.BooleanField(default=False), + ), + ] diff --git a/netbox_branching/models/branches.py b/netbox_branching/models/branches.py index 3e27fbc..0dcdd0a 100644 --- a/netbox_branching/models/branches.py +++ b/netbox_branching/models/branches.py @@ -97,6 +97,11 @@ class Branch(JobsMixin, PrimaryModel): null=True, related_name='+' ) + merged_using_collapsed = models.BooleanField( + verbose_name=_('merged using collapsed'), + default=False, + help_text=_('Whether the merge was performed using the collapsed strategy') + ) _preaction_validators = { 'sync': set(), @@ -1219,11 +1224,13 @@ def merge(self, user, commit=True): try: with transaction.atomic(): - # Choose merge strategy - if True: - self._merge_iterative(changes, request, commit, logger) - else: + # Choose merge strategy based on merged_using_collapsed setting + if self.merged_using_collapsed: + logger.debug("Merging using collapsed strategy") self._merge_collapsed(changes, request, commit, logger) + else: + logger.debug("Merging using iterative strategy") + self._merge_iterative(changes, request, commit, logger) except Exception as e: if err_message := str(e): @@ -1370,11 +1377,13 @@ def revert(self, user, commit=True): try: with transaction.atomic(): - # Choose revert strategy - if True: - self._revert_iterative(changes, request, commit, logger) - else: + # Choose revert strategy based on merged_using_collapsed setting + if self.merged_using_collapsed: + logger.debug("Reverting using collapsed strategy") self._revert_collapsed(changes, request, commit, logger) + else: + logger.debug("Reverting using iterative strategy") + self._revert_iterative(changes, request, commit, logger) except Exception as e: if err_message := str(e): @@ -1389,6 +1398,7 @@ def revert(self, user, commit=True): self.status = BranchStatusChoices.READY self.merged_time = None self.merged_by = None + self.merged_using_collapsed = False self.save() # Record a branch event for the merge diff --git a/netbox_branching/templates/netbox_branching/branch_action.html b/netbox_branching/templates/netbox_branching/branch_action.html index 535872b..413c9b9 100644 --- a/netbox_branching/templates/netbox_branching/branch_action.html +++ b/netbox_branching/templates/netbox_branching/branch_action.html @@ -50,6 +50,9 @@
{% render_field form.commit %} + {% if form.collapse_changes %} + {% render_field form.collapse_changes %} + {% endif %}
{% trans "Cancel" %}