diff --git a/nautobot_design_builder/changes.py b/nautobot_design_builder/changes.py new file mode 100644 index 0000000..41c6eb3 --- /dev/null +++ b/nautobot_design_builder/changes.py @@ -0,0 +1,75 @@ +"""Logic used to track changes and change logging.""" + +from contextlib import contextmanager +from typing import TYPE_CHECKING + +from django.db import models as django_models + +if TYPE_CHECKING: + from nautobot_design_builder.design import ModelInstance + + +def _get_change_value(value): + if isinstance(value, django_models.Manager): + value = {item.pk for item in value.all()} + return value + + +@contextmanager +def change_log(model_instance: "ModelInstance", attr_name: str): + """Log changes for a field. + + This context manager will record the value of a field prior to a change + as well as the value after the change. If the values are different then + a change record is added to the underlying model instance. + + Args: + model_instance (ModelInstance): The model instance that is being updated. + attr_name (str): The attribute to be updated. + """ + old_value = _get_change_value(getattr(model_instance.design_instance, attr_name)) + yield + new_value = _get_change_value(getattr(model_instance.design_instance, attr_name)) + if old_value != new_value: + if isinstance(old_value, set): + model_instance.design_metadata.changes[attr_name] = { + "old_items": old_value, + "new_items": new_value, + } + # Many-to-Many changes need to be logged on the parent, + # and this won't happen implicitly so we log the changes + # explicitly here. + # + # TODO: This has been commented out because I *think* that it is + # no longer needed since their is now a journal log created in the + # create_child method. + # model_instance.design_metadata.environment.journal.log(model_instance) + else: + model_instance.design_metadata.changes[attr_name] = { + "old_value": old_value, + "new_value": new_value, + } + + +def revert_changed_dict(current_value: dict, original_value: dict, changed_value: dict) -> dict: + """Create a new dictionary that correctly returns a changed dictionary to its expected value. + + In many cases Nautobot model object have dictionary attributes (configuration contexts, secret + params, etc). The design builder will attempt to revert design related attributes within + these dictionaries. Any dictionary items changed by the design will be reverted to their original + value and any items added outside of the process will be left alone. Dictionary items added + by the design wil be removed. + + Args: + current_value (dict): The dictionary value as it is right now. + original_value (dict): The dictionary value before the change. + changed_value (dict): The dictionary value after the change. + + Returns: + dict: The dictionary as we expect it to be without the design changes. + """ + original_keys = set(original_value.keys()) + changed_keys = set(changed_value.keys()) + current_keys = set(current_value.keys()) + design_keys = changed_keys.union(original_keys) + return {**{key: current_value[key] for key in current_keys.difference(design_keys)}, **original_value} diff --git a/nautobot_design_builder/fields.py b/nautobot_design_builder/fields.py index 9e37f76..5236a77 100644 --- a/nautobot_design_builder/fields.py +++ b/nautobot_design_builder/fields.py @@ -39,7 +39,6 @@ """ from abc import ABC, abstractmethod -from contextlib import contextmanager from typing import Any, Mapping, Type, TYPE_CHECKING from django.db import models as django_models @@ -51,6 +50,7 @@ from nautobot.core.graphql.utils import str_to_var_name from nautobot.extras.models import Relationship, RelationshipAssociation +from nautobot_design_builder.changes import change_log from nautobot_design_builder.errors import DesignImplementationError from nautobot_design_builder.debug import debug_set @@ -58,48 +58,6 @@ from .design import ModelInstance -def _get_change_value(value): - if isinstance(value, django_models.Manager): - value = {item.pk for item in value.all()} - return value - - -@contextmanager -def change_log(model_instance: "ModelInstance", attr_name: str): - """Log changes for a field. - - This context manager will record the value of a field prior to a change - as well as the value after the change. If the values are different then - a change record is added to the underlying model instance. - - Args: - model_instance (ModelInstance): The model instance that is being updated. - attr_name (str): The attribute to be updated. - """ - old_value = _get_change_value(getattr(model_instance.design_instance, attr_name)) - yield - new_value = _get_change_value(getattr(model_instance.design_instance, attr_name)) - if old_value != new_value: - if isinstance(old_value, set): - model_instance.design_metadata.changes[attr_name] = { - "old_items": old_value, - "new_items": new_value, - } - # Many-to-Many changes need to be logged on the parent, - # and this won't happen implicitly so we log the changes - # explicitly here. - # - # TODO: This has been commented out because I *think* that it is - # no longer needed since their is now a journal log created in the - # create_child method. - # model_instance.design_metadata.environment.journal.log(model_instance) - else: - model_instance.design_metadata.changes[attr_name] = { - "old_value": old_value, - "new_value": new_value, - } - - class ModelField(ABC): """This represents any type of field (attribute or relationship) on a Nautobot model. diff --git a/nautobot_design_builder/models.py b/nautobot_design_builder/models.py index 483cbe6..39ef10e 100644 --- a/nautobot_design_builder/models.py +++ b/nautobot_design_builder/models.py @@ -13,6 +13,8 @@ from nautobot.extras.models import Job as JobModel, JobResult, Status, StatusField from nautobot.extras.utils import extras_features +from nautobot_design_builder.changes import revert_changed_dict + from .util import get_created_and_last_updated_usernames_for_model from . import choices from .errors import DesignValidationError @@ -555,31 +557,6 @@ class Meta: # noqa:D106 ("change_set", "_design_object_type", "_design_object_id"), ] - @staticmethod - def update_current_value_from_dict(current_value, added_value, removed_value): - """Update current value if it's a dictionary. - - The removed_value keys (the original one) are going to be recovered, the added_value ones - will be reverted, and the current_value ones that were not added by the design will be kept. - """ - keys_to_remove = [] - for key in current_value: - if key in added_value: - if key in removed_value: - # Reverting the value of keys that existed before and the design deployment modified - current_value[key] = removed_value[key] - else: - keys_to_remove.append(key) - - # Removing keys that were added by the design. - for key in keys_to_remove: - del current_value[key] - - # Recovering old keys that the ChangeRecord deleted. - for key in removed_value: - if key not in added_value: - current_value[key] = removed_value[key] - def revert(self, local_logger: logging.Logger = logger): # pylint: disable=too-many-branches """Revert the changes that are represented in this change record. @@ -627,7 +604,10 @@ def revert(self, local_logger: logging.Logger = logger): # pylint: disable=too- ) else: local_logger.info("Reverting change record", extra={"object": self.design_object}) - for attr_name, change in self.changes.items(): + changes = self.changes + if changes is None: + changes = {} + for attr_name, change in changes.items(): current_value = getattr(self.design_object, attr_name) if "old_items" in change: old_items = set(change["old_items"]) @@ -637,19 +617,16 @@ def revert(self, local_logger: logging.Logger = logger): # pylint: disable=too- current_items -= added_items current_value.set(current_value.filter(pk__in=current_items)) else: - old_value = change["old_value"] - new_value = change["new_value"] - - if isinstance(old_value, dict): + if isinstance(change["old_value"], dict): # config-context like thing, only change the keys # that were added/changed - self.update_current_value_from_dict( - current_value=current_value, - added_value=new_value, - removed_value=old_value if old_value else {}, + setattr( + self.design_object, + attr_name, + revert_changed_dict(current_value, change["old_value"], change["new_value"]), ) else: - setattr(self.design_object, attr_name, old_value) + setattr(self.design_object, attr_name, change["old_value"]) self.design_object.save() local_logger.info( diff --git a/nautobot_design_builder/tests/test_changes.py b/nautobot_design_builder/tests/test_changes.py new file mode 100644 index 0000000..8647d92 --- /dev/null +++ b/nautobot_design_builder/tests/test_changes.py @@ -0,0 +1,37 @@ +"""Tests for change utilities.""" + +from unittest import TestCase + +from nautobot_design_builder.changes import revert_changed_dict + + +class TestRevertChangedDict(TestCase): + """TestCase for the revert_changed_dict utility.""" + + def test_added_key(self): + original = {"key1": "initial-value"} + changed = {"key1": "initial-value", "key2": "new-value"} + current = {**changed} + got = revert_changed_dict(current, original, changed) + self.assertDictEqual(original, got) + + def test_extra_key(self): + original = {"key1": "initial-value"} + changed = {"key1": "initial-value", "key2": "new-value"} + current = {"key1": "initial-value", "key2": "new-value", "key3": "key3-value"} + got = revert_changed_dict(current, original, changed) + self.assertDictEqual({"key1": "initial-value", "key3": "key3-value"}, got) + + def test_missing_changed_value_without_current(self): + original = {"key1": "initial-value"} + changed = {"key2": "new-value"} + current = {"key2": "new-value", "key3": "key3-value"} + got = revert_changed_dict(current, original, changed) + self.assertDictEqual({"key1": "initial-value", "key3": "key3-value"}, got) + + def test_missing_changed_value_with_current(self): + original = {"key1": "initial-value"} + changed = {"key2": "new-value"} + current = {"key1": "changed-value", "key2": "new-value", "key3": "key3-value"} + got = revert_changed_dict(current, original, changed) + self.assertDictEqual({"key1": "initial-value", "key3": "key3-value"}, got) diff --git a/nautobot_design_builder/tests/test_model_change_record.py b/nautobot_design_builder/tests/test_model_change_record.py index b4d04b1..461e0b1 100644 --- a/nautobot_design_builder/tests/test_model_change_record.py +++ b/nautobot_design_builder/tests/test_model_change_record.py @@ -1,10 +1,10 @@ """Test ChangeRecord.""" -import unittest from unittest.mock import patch, Mock from nautobot.extras.models import Secret from nautobot.dcim.models import Manufacturer, DeviceType +from nautobot_design_builder.design import Environment, ModelInstance from nautobot_design_builder.errors import DesignValidationError from .test_model_deployment import BaseDeploymentTest @@ -155,34 +155,51 @@ def test_remove_dictionary_key(self): old_value, ) - @unittest.skip - def test_new_key_reverted_without_original_and_with_a_new_one(self): - # TODO: I don't understand this test - secret = Secret.objects.get(id=self.secret.id) - secret.parameters["key2"] = "changed-value" + def test_only_protected_field_attributes_are_reverted(self): + """This test confirms that non-design dictionary keys are unaffected. + + Any Nautobot JSON field stored as an object (dictionary) is protected + only for those object attributes (dictionary keys) that are part + of the design. This means that keys added outside of the design process + should remain when the design is reverted. This test confirms that + behavior. + """ + + original_params = {"key1": "initial-value"} + secret = Secret.objects.create( + name="test secret 1", + provider="environment-variable", + description="test description", + parameters=original_params, + ) secret.save() + updated_params = {"key1": "initial-value", "key2": "changed-value"} + design_secret = ModelInstance.factory(Secret)( + Environment(), + { + "!create_or_update:name": "test secret 1", + "parameters": updated_params, + }, + ) + design_secret.save() + change_record = self.create_change_record(secret, design_secret.design_metadata.changes) + print(design_secret.design_metadata.changes) secret.refresh_from_db() self.assertDictEqual( secret.parameters, - {"key1": "initial-value", "key2": "changed-value"}, + updated_params, ) - # Delete the initial value and add a new one - del secret.parameters["key1"] - secret.parameters["key3"] = "changed-value" + # The "final params" is the design parameters "key2" plus a "key3" that + # has been added outside of the design process. + final_params = {"key2": "changed-value", "key3": "changed-value"} + secret.parameters = final_params secret.save() - self.assertDictEqual( - secret.parameters, - { - "key2": "changed-value", - "key3": "changed-value", - }, - ) - - entry = self.create_change_record(secret, None) - entry.revert() + change_record.revert() secret.refresh_from_db() - self.assertDictEqual(self.secret.parameters, secret.parameters) + # reverting should bring back key1 and leave key3 alone (since it was added outside of the + # design process) + self.assertDictEqual(secret.parameters, {"key1": "initial-value", "key3": "changed-value"}) @patch("nautobot.extras.models.Secret.save") def test_reverting_without_old_value(self, save_mock: Mock): @@ -200,35 +217,6 @@ def test_reverting_without_old_value(self, save_mock: Mock): self.assertEqual(entry.design_object.parameters, {}) save_mock.assert_called() - @unittest.skip - @patch("nautobot.extras.models.Secret.save") - def test_reverting_without_new_value(self, save_mock: Mock): - # TODO: I don't understand this test - with patch("nautobot.extras.models.Secret.refresh_from_db"): - secret = Secret( - name="test secret 1", - provider="environment-variable", - description="Description", - parameters={"key1": "value1"}, - ) - secret.parameters = None - entry = self.create_change_record(secret, secret) - self.assertEqual(entry.design_object.parameters, None) - entry.revert() - self.assertEqual(entry.design_object.parameters, {"key1": "value1"}) - save_mock.assert_called() - - @unittest.skip - def test_change_property(self): - """This test checks that the 'display' property is properly managed.""" - updated_device_type = DeviceType.objects.get(id=self.device_type.id) - updated_device_type.model = "new name" - updated_device_type.save() - entry = self.create_change_record(updated_device_type, None) - entry.revert() - self.device_type.refresh_from_db() - self.assertEqual(self.device_type.model, "test device type") - def test_change_foreign_key(self): new_manufacturer = Manufacturer.objects.create(name="new manufacturer") new_manufacturer.save()