[DPE-4335] Do not trigger secret revision change if the relation cont…

…ent is the same (#169)

* fix: Compare content before updating relation-based secret

* tests: Assert that relation-based secret revision changes with new content

* refactor: Move fix to CachedSecret.set_content

* tests: Extract secret revision test in its own test

* tests: Add non peer relation test for secret revision

* fix: Use correct var in integration charm

* fix: Use correct label name in test

* tests: Add unit test for data interface cached secret

* chore: Revert uneeded changes

* tests: Add interface-focused test for secret revision changes

* chore: Format

lib/charms/data_platform_libs/v0/data_interfaces.py

@@ -331,7 +331,7 @@ def _on_topic_requested(self, event: TopicRequestedEvent):
 
 # Increment this PATCH version before using `charmcraft publish-lib` or reset
 # to 0 if you are raising the major API version
-LIBPATCH = 36
+LIBPATCH = 37
 
 PYDEPS = ["ops>=2.0.0"]
 
@@ -658,6 +658,10 @@ def set_content(self, content: Dict[str, str]) -> None:
         if not self.meta:
             return
 
+        # DPE-4182: do not create new revision if the content stay the same
+        if content == self.get_content():
+            return
+
         if content:
             self._move_to_new_label_if_needed()
             self.meta.set_content(content)

tests/integration/database-charm/src/charm.py

@@ -151,7 +151,7 @@ def _on_change_admin_password(self, event: ActionEvent):
     def _on_set_secret_action(self, event: ActionEvent):
         """Change the admin password."""
         secret_field = event.params.get("field")
-        password = self._new_password()
+        password = event.params.get("value", self._new_password())
         for relation in self.database.relations:
             self.database.update_relation_data(relation.id, {secret_field: password})
 

tests/integration/helpers.py

@@ -303,3 +303,20 @@ async def get_secret_by_label(ops_test, label: str, owner: str = "") -> Dict[str
         if label == secret_data[secret_id].get("label"):
             if not owner or owner == secret_data[secret_id].get("owner"):
                 return secret_data[secret_id]["content"]["Data"]
+
+
+async def get_secret_revision_by_label(ops_test, label: str, owner: str = "") -> int:
+    secrets_raw = await ops_test.juju("list-secrets")
+    secret_ids = [
+        secret_line.split()[0] for secret_line in secrets_raw[1].split("\n")[1:] if secret_line
+    ]
+
+    for secret_id in secret_ids:
+        secret_data_raw = await ops_test.juju(
+            "show-secret", "--format", "json", "--reveal", secret_id
+        )
+        secret_data = json.loads(secret_data_raw[1])
+
+        if label == secret_data[secret_id].get("label"):
+            if not owner or owner == secret_data[secret_id].get("owner"):
+                return int(secret_data[secret_id]["revision"])

tests/integration/test_charm.py

@@ -20,6 +20,7 @@
     get_leader_id,
     get_non_leader_id,
     get_secret_by_label,
+    get_secret_revision_by_label,
     list_juju_secrets,
 )
 
@@ -293,6 +294,52 @@ async def test_peer_relation_secrets(component, ops_test: OpsTest):
     await action.wait()
 
 
+@pytest.mark.abort_on_fail
+@pytest.mark.usefixtures("only_with_juju_secrets")
+@pytest.mark.parametrize("component", ["app", "unit"])
+async def test_peer_relation_secret_revisions(component, ops_test: OpsTest):
+    """Check that only a content change triggers the emission of a new revision."""
+    # Given
+    leader_id = await get_leader_id(ops_test, DATABASE_APP_NAME)
+    unit_name = f"{DATABASE_APP_NAME}/{leader_id}"
+    owner = "database" if component == "app" else unit_name
+
+    # When
+    action = await ops_test.model.units.get(unit_name).run_action(
+        "set-peer-relation-field",
+        **{"component": component, "field": "secret-field", "value": "blablabla"},
+    )
+    await action.wait()
+
+    original_secret_revision = await get_secret_revision_by_label(
+        ops_test, f"database-peers.database.{component}", owner
+    )
+
+    action = await ops_test.model.units.get(unit_name).run_action(
+        "set-peer-relation-field",
+        **{"component": component, "field": "secret-field", "value": "blablabla2"},
+    )
+    await action.wait()
+
+    changed_secret_revision = await get_secret_revision_by_label(
+        ops_test, f"database-peers.database.{component}", owner
+    )
+
+    action = await ops_test.model.units.get(unit_name).run_action(
+        "set-peer-relation-field",
+        **{"component": component, "field": "secret-field", "value": "blablabla2"},
+    )
+    await action.wait()
+
+    unchanged_secret_revision = await get_secret_revision_by_label(
+        ops_test, f"database-peers.database.{component}", owner
+    )
+
+    # Then
+    assert original_secret_revision + 1 == changed_secret_revision
+    assert changed_secret_revision == unchanged_secret_revision
+
+
 @pytest.mark.abort_on_fail
 @pytest.mark.usefixtures("only_with_juju_secrets")
 @pytest.mark.parametrize("component", ["app", "unit"])
@@ -907,6 +954,60 @@ async def test_provider_with_additional_secrets(ops_test: OpsTest, database_char
     assert topsecret1 != topsecret2
 
 
+@pytest.mark.abort_on_fail
+@pytest.mark.usefixtures("only_with_juju_secrets")
+async def test_relation_secret_revisions(ops_test: OpsTest):
+    """Check that only a content change triggers the emission of a new revision."""
+    # Given
+    leader_id = await get_leader_id(ops_test, DATABASE_APP_NAME)
+    leader_name = f"{DATABASE_APP_NAME}/{leader_id}"
+    owner = "database"
+    rel_id = pytest.second_database_relation.id
+    group_mapping = "extra"
+
+    # When
+    action = await ops_test.model.units.get(leader_name).run_action(
+        "set-secret", **{"relation_id": rel_id, "field": "topsecret", "value": "initialvalue"}
+    )
+    await action.wait()
+
+    original_secret_revision = await get_secret_revision_by_label(
+        ops_test, f"{DATABASE_APP_NAME}.{rel_id}.{group_mapping}.secret", owner
+    )
+
+    action = await ops_test.model.units.get(leader_name).run_action(
+        "set-relation-field",
+        **{
+            "relation_id": pytest.second_database_relation.id,
+            "field": "topsecret",
+            "value": "changedvalue",
+        },
+    )
+    await action.wait()
+
+    changed_secret_revision = await get_secret_revision_by_label(
+        ops_test, f"{DATABASE_APP_NAME}.{rel_id}.{group_mapping}.secret", owner
+    )
+
+    action = await ops_test.model.units.get(leader_name).run_action(
+        "set-relation-field",
+        **{
+            "relation_id": pytest.second_database_relation.id,
+            "field": "topsecret",
+            "value": "changedvalue",
+        },
+    )
+    await action.wait()
+
+    unchanged_secret_revision = await get_secret_revision_by_label(
+        ops_test, f"{DATABASE_APP_NAME}.{rel_id}.{group_mapping}.secret", owner
+    )
+
+    # Then
+    assert original_secret_revision + 1 == changed_secret_revision
+    assert changed_secret_revision == unchanged_secret_revision
+
+
 @pytest.mark.parametrize("field,value", [("new_field", "blah"), ("tls", "True")])
 @pytest.mark.usefixtures("only_without_juju_secrets")
 async def test_provider_get_set_delete_fields(field, value, ops_test: OpsTest):
@@ -1072,6 +1173,7 @@ async def test_provider_get_set_delete_fields_secrets(
     assert action.results["return-code"] == 0
 
 
+@pytest.mark.abort_on_fail
 @pytest.mark.log_errors_allowed("Can't delete secret for relation")
 @pytest.mark.usefixtures("only_with_juju_secrets")
 async def test_provider_deleted_secret_is_removed(ops_test: OpsTest):

tests/unit/test_cached_secrets.py

@@ -0,0 +1,44 @@
+"""Component level tests."""
+
+from unittest.mock import Mock
+
+import pytest
+from ops.charm import CharmBase
+from ops.testing import Harness
+
+from charms.data_platform_libs.v0.data_interfaces import CachedSecret
+
+
+class EmptyCharm(CharmBase):
+    """Mock database charm to use in units tests."""
+
+    def __init__(self, *args):
+        super().__init__(*args)
+
+
+@pytest.fixture
+def harness() -> Harness:
+    harness = Harness(EmptyCharm)
+    harness.set_leader(True)
+    harness.begin_with_initial_hooks()
+    return harness
+
+
+@pytest.mark.usefixtures("only_with_juju_secrets")
+@pytest.mark.parametrize("scope", [("app"), ("unit")])
+def test_cached_secret_no_update(scope, harness: Harness[EmptyCharm], monkeypatch):
+    """Check that a cached secret do not trigger a revision if no update to content."""
+    # Given
+    content = {"value": "initialvalue"}
+    secret = CachedSecret(harness.model, getattr(harness.charm, scope), "my-label")
+    secret.add_secret(content)
+    patched = Mock()
+    # When
+    with monkeypatch.context() as m:
+        m.setattr(secret.meta, "set_content", patched)
+        secret.set_content(content)
+
+        secret.set_content({"value": "newvalue"})
+
+    # Then
+    patched.assert_called_once()

tests/unit/test_data_interfaces.py

@@ -28,6 +28,7 @@
     DatabaseRequires,
     DatabaseRequiresEvents,
     DataPeer,
+    DataPeerData,
     DataPeerOtherUnit,
     DataPeerUnit,
     Diff,
@@ -637,6 +638,47 @@ def test_peer_relation_interface_backwards_compatible_legacy_label(self, interfa
 
         assert secret2.peek_content()["secret-field"] == "blabla"
 
+    @parameterized.expand([("peer_relation_app",), ("peer_relation_unit",)])
+    @pytest.mark.usefixtures("only_with_juju_secrets")
+    def test_peer_relation_secret_secret_revision(self, interface_attr):
+        """Check the functionality of each public interface function."""
+        # Given
+        relation_id = self.harness.charm.peer_relation.id
+        interface: DataPeerData = getattr(self.harness.charm, interface_attr)
+
+        scope = interface_attr.split("_")[2]
+        scope_opj = getattr(self.harness.charm, scope)
+        secret = scope_opj.add_secret(
+            {"secret-field": "initialvalue"}, label=f"{PEER_RELATION_NAME}.database.{scope}"
+        )
+        cached_secret = interface.secrets.get(label=f"{PEER_RELATION_NAME}.database.{scope}")
+
+        initial_secret_revision = secret.get_info().revision
+        initial_cached_secret_revision = cached_secret.meta.get_info().revision
+
+        # When
+        interface.update_relation_data(relation_id, {"secret-field": "initialvalue"})
+        secret.get_content(refresh=True)
+
+        unchanged_secret_revision = secret.get_info().revision
+        unchanged_cached_secret_revision = cached_secret.meta.get_info().revision
+
+        interface.update_relation_data(relation_id, {"secret-field": "newvalue"})
+        secret.get_content(refresh=True)
+
+        changed_secret_revision = secret.get_info().revision
+        changed_cached_secret_revision = cached_secret.meta.get_info().revision
+
+        # Then
+        assert (
+            initial_secret_revision
+            == initial_cached_secret_revision
+            == unchanged_secret_revision
+            == unchanged_cached_secret_revision
+        )
+        assert changed_secret_revision == unchanged_secret_revision + 1
+        assert changed_cached_secret_revision == unchanged_cached_secret_revision + 1
+
     def test_peer_relation_other_unit(self):
         """Check the functionality of each public interface function on each "other" unit."""
         relation_id = self.harness.charm.peer_relation.id