Merge pull request #63 from canonical/remove_remote_unit_ids

removed remote unit ids var from Relation

.gitignore

@@ -9,3 +9,4 @@ __pycache__/
 *.egg-info
 dist/
 *.pytest_cache
+htmlcov/

README.md

@@ -370,7 +370,6 @@ To declare a peer relation, you should use `scenario.state.PeerRelation`. The co
 that peer relations do not have a "remote app" (it's this app, in fact). So unlike `Relation`, a `PeerRelation` does not
 have `remote_app_name` or `remote_app_data` arguments. Also, it talks in terms of `peers`:
 
-- `Relation.remote_unit_ids` maps to `PeerRelation.peers_ids`
 - `Relation.remote_units_data` maps to `PeerRelation.peers_data`
 
 ```python
@@ -488,7 +487,7 @@ remote unit that the event is about. The reason that this parameter is not suppl
 events, is that the relation already ties 'this app' to some 'remote app' (cfr. the `Relation.remote_app_name` attr),
 but not to a specific unit. What remote unit this event is about is not a `State` concern but an `Event` one.
 
-The `remote_unit_id` will default to the first ID found in the relation's `remote_unit_ids`, but if the test you are
+The `remote_unit_id` will default to the first ID found in the relation's `remote_units_data`, but if the test you are
 writing is close to that domain, you should probably override it and pass it manually.
 
 ```python

pyproject.toml

@@ -8,7 +8,7 @@ build-backend = "setuptools.build_meta"
 [project]
 name = "ops-scenario"
 
-version = "5.2.2"
+version = "5.3"
 
 authors = [
     { name = "Pietro Pasotti", email = "[email protected]" }

scenario/__init__.py

@@ -14,7 +14,6 @@
     Model,
     Mount,
     Network,
-    ParametrizedEvent,
     PeerRelation,
     Port,
     Relation,
@@ -34,7 +33,6 @@
     "deferred",
     "StateValidationError",
     "Secret",
-    "ParametrizedEvent",
     "RelationBase",
     "Relation",
     "SubordinateRelation",

scenario/consistency_checker.py

@@ -185,6 +185,7 @@ def _check_action_event(
             f"action event {event.name} refers to action {action.name} "
             f"which is not declared in the charm metadata (actions.yaml).",
         )
+        return
 
     _check_action_param_types(charm_spec, action, errors, warnings)
 

scenario/mocking.py

@@ -177,7 +177,9 @@ def relation_list(self, relation_id: int) -> Tuple[str]:
         relation = self._get_relation_by_id(relation_id)
 
         if isinstance(relation, PeerRelation):
-            return tuple(f"{self.app_name}/{unit_id}" for unit_id in relation.peers_ids)
+            return tuple(
+                f"{self.app_name}/{unit_id}" for unit_id in relation.peers_data
+            )
         return tuple(
             f"{relation.remote_app_name}/{unit_id}"
             for unit_id in relation._remote_unit_ids

scenario/state.py

@@ -35,6 +35,8 @@
     PathLike = Union[str, Path]
     AnyRelation = Union["Relation", "PeerRelation", "SubordinateRelation"]
     AnyJson = Union[str, bool, dict, int, float, list]
+    RawSecretRevisionContents = RawDataBagContents = Dict[str, str]
+    UnitID = int
 
 logger = scenario_logger.getChild("state")
 
@@ -103,7 +105,7 @@ class Secret(_DCBase):
     id: str
 
     # mapping from revision IDs to each revision's contents
-    contents: Dict[int, Dict[str, str]]
+    contents: Dict[int, "RawSecretRevisionContents"]
 
     # indicates if the secret is owned by THIS unit, THIS app or some other app/unit.
     owner: Literal["unit", "application", None] = None
@@ -168,7 +170,7 @@ def _set_revision(self, revision: int):
 
     def _update_metadata(
         self,
-        content: Optional[Dict[str, str]] = None,
+        content: Optional["RawSecretRevisionContents"] = None,
         label: Optional[str] = None,
         description: Optional[str] = None,
         expire: Optional[datetime.datetime] = None,
@@ -195,27 +197,6 @@ def normalize_name(s: str):
     return s.replace("-", "_")
 
 
-class ParametrizedEvent:
-    def __init__(self, accept_params: Tuple[str], category: str, *args, **kwargs):
-        self._accept_params = accept_params
-        self._category = category
-        self._args = args
-        self._kwargs = kwargs
-
-    def __call__(self, remote_unit: Optional[str] = None) -> "Event":
-        """Construct an Event object using the arguments provided at init and any extra params."""
-        if remote_unit and "remote_unit" not in self._accept_params:
-            raise ValueError(
-                f"cannot pass param `remote_unit` to a "
-                f"{self._category} event constructor.",
-            )
-
-        return Event(*self._args, *self._kwargs, relation_remote_unit_id=remote_unit)
-
-    def deferred(self, handler: Callable, event_id: int = 1) -> "DeferredEvent":
-        return self().deferred(handler=handler, event_id=event_id)
-
-
 _next_relation_id_counter = 1
 
 
@@ -237,8 +218,8 @@ class RelationBase(_DCBase):
     # Every new Relation instance gets a new one, if there's trouble, override.
     relation_id: int = dataclasses.field(default_factory=next_relation_id)
 
-    local_app_data: Dict[str, str] = dataclasses.field(default_factory=dict)
-    local_unit_data: Dict[str, str] = dataclasses.field(default_factory=dict)
+    local_app_data: "RawDataBagContents" = dataclasses.field(default_factory=dict)
+    local_unit_data: "RawDataBagContents" = dataclasses.field(default_factory=dict)
 
     @property
     def _databags(self):
@@ -251,7 +232,10 @@ def _remote_unit_ids(self) -> Tuple[int]:
         """Ids of the units on the other end of this relation."""
         raise NotImplementedError()
 
-    def _get_databag_for_remote(self, unit_id: int) -> Dict[str, str]:  # noqa: U100
+    def _get_databag_for_remote(
+        self,
+        unit_id: int,  # noqa: U100
+    ) -> "RawDataBagContents":
         """Return the databag for some remote unit ID."""
         raise NotImplementedError()
 
@@ -318,74 +302,18 @@ def broken_event(self) -> "Event":
         )
 
 
-def unify_ids_and_remote_units_data(ids: List[int], data: Dict[int, Any]):
-    """Unify and validate a list of unit IDs and a mapping from said ids to databag contents.
-
-    This allows the user to pass equivalently:
-    ids = []
-    data = {1: {}}
-
-    or
-
-    ids = [1]
-    data = {}
-
-    or
-
-    ids = [1]
-    data = {1: {}}
-
-    but catch the inconsistent:
-
-    ids = [1]
-    data = {2: {}}
-
-    or
-
-    ids = [2]
-    data = {1: {}}
-    """
-    if ids and data:
-        if not set(ids) == set(data):
-            raise StateValidationError(
-                f"{ids} should include any and all IDs from {data}",
-            )
-    elif ids:
-        data = {x: {} for x in ids}
-    elif data:
-        ids = list(data)
-    else:
-        ids = [0]
-        data = {0: {}}
-    return ids, data
-
-
 @dataclasses.dataclass(frozen=True)
 class Relation(RelationBase):
     remote_app_name: str = "remote"
 
-    # fixme: simplify API by deriving remote_unit_ids from remote_units_data.
-    remote_unit_ids: List[int] = dataclasses.field(default_factory=list)
-
     # local limit
     limit: int = 1
 
-    remote_app_data: Dict[str, str] = dataclasses.field(default_factory=dict)
-    remote_units_data: Dict[int, Dict[str, str]] = dataclasses.field(
-        default_factory=dict,
+    remote_app_data: "RawDataBagContents" = dataclasses.field(default_factory=dict)
+    remote_units_data: Dict["UnitID", "RawDataBagContents"] = dataclasses.field(
+        default_factory=lambda: {0: {}},
     )
 
-    def __post_init__(self):
-        super().__post_init__()
-
-        remote_unit_ids, remote_units_data = unify_ids_and_remote_units_data(
-            self.remote_unit_ids,
-            self.remote_units_data,
-        )
-        # bypass frozen dataclass
-        object.__setattr__(self, "remote_unit_ids", remote_unit_ids)
-        object.__setattr__(self, "remote_units_data", remote_units_data)
-
     @property
     def _remote_app_name(self) -> str:
         """Who is on the other end of this relation?"""
@@ -394,9 +322,9 @@ def _remote_app_name(self) -> str:
     @property
     def _remote_unit_ids(self) -> Tuple[int]:
         """Ids of the units on the other end of this relation."""
-        return tuple(self.remote_unit_ids)
+        return tuple(self.remote_units_data)
 
-    def _get_databag_for_remote(self, unit_id: int) -> Dict[str, str]:
+    def _get_databag_for_remote(self, unit_id: int) -> "RawDataBagContents":
         """Return the databag for some remote unit ID."""
         return self.remote_units_data[unit_id]
 
@@ -411,8 +339,8 @@ def _databags(self):
 
 @dataclasses.dataclass(frozen=True)
 class SubordinateRelation(RelationBase):
-    remote_app_data: Dict[str, str] = dataclasses.field(default_factory=dict)
-    remote_unit_data: Dict[str, str] = dataclasses.field(default_factory=dict)
+    remote_app_data: "RawDataBagContents" = dataclasses.field(default_factory=dict)
+    remote_unit_data: "RawDataBagContents" = dataclasses.field(default_factory=dict)
 
     # app name and ID of the remote unit that *this unit* is attached to.
     remote_app_name: str = "remote"
@@ -423,7 +351,7 @@ def _remote_unit_ids(self) -> Tuple[int]:
         """Ids of the units on the other end of this relation."""
         return (self.remote_unit_id,)
 
-    def _get_databag_for_remote(self, unit_id: int) -> Dict[str, str]:
+    def _get_databag_for_remote(self, unit_id: int) -> "RawDataBagContents":
         """Return the databag for some remote unit ID."""
         if unit_id is not self.remote_unit_id:
             raise ValueError(
@@ -447,10 +375,11 @@ def remote_unit_name(self) -> str:
 
 @dataclasses.dataclass(frozen=True)
 class PeerRelation(RelationBase):
-    peers_data: Dict[int, Dict[str, str]] = dataclasses.field(default_factory=dict)
-
-    # IDs of the peers. Consistency checks will validate that *this unit*'s ID is not in here.
-    peers_ids: List[int] = dataclasses.field(default_factory=list)
+    peers_data: Dict["UnitID", "RawDataBagContents"] = dataclasses.field(
+        default_factory=lambda: {0: {}},
+    )
+    # mapping from peer unit IDs to their databag contents.
+    # Consistency checks will validate that *this unit*'s ID is not in here.
 
     @property
     def _databags(self):
@@ -462,21 +391,12 @@ def _databags(self):
     @property
     def _remote_unit_ids(self) -> Tuple[int]:
         """Ids of the units on the other end of this relation."""
-        return tuple(self.peers_ids)
+        return tuple(self.peers_data)
 
-    def _get_databag_for_remote(self, unit_id: int) -> Dict[str, str]:
+    def _get_databag_for_remote(self, unit_id: int) -> "RawDataBagContents":
         """Return the databag for some remote unit ID."""
         return self.peers_data[unit_id]
 
-    def __post_init__(self):
-        peers_ids, peers_data = unify_ids_and_remote_units_data(
-            self.peers_ids,
-            self.peers_data,
-        )
-        # bypass frozen dataclass guards
-        object.__setattr__(self, "peers_ids", peers_ids)
-        object.__setattr__(self, "peers_data", peers_data)
-
 
 def _random_model_name():
     import random

tests/test_consistency_checker.py

@@ -142,6 +142,11 @@ def test_bad_config_option_type():
         Event("bar"),
         _CharmSpec(MyCharm, {}, config={"options": {"foo": {"type": "string"}}}),
     )
+    assert_inconsistent(
+        State(config={"foo": True}),
+        Event("bar"),
+        _CharmSpec(MyCharm, {}, config={"options": {"foo": {}}}),
+    )
     assert_consistent(
         State(config={"foo": True}),
         Event("bar"),
@@ -151,12 +156,26 @@ def test_bad_config_option_type():
 
 @pytest.mark.parametrize("bad_v", ("1.0", "0", "1.2", "2.35.42", "2.99.99", "2.99"))
 def test_secrets_jujuv_bad(bad_v):
+    secret = Secret("secret:foo", {0: {"a": "b"}})
     assert_inconsistent(
-        State(secrets=[Secret("secret:foo", {0: {"a": "b"}})]),
+        State(secrets=[secret]),
         Event("bar"),
         _CharmSpec(MyCharm, {}),
         bad_v,
     )
+    assert_inconsistent(
+        State(secrets=[secret]),
+        secret.changed_event,
+        _CharmSpec(MyCharm, {}),
+        bad_v,
+    )
+
+    assert_inconsistent(
+        State(),
+        secret.changed_event,
+        _CharmSpec(MyCharm, {}),
+        bad_v,
+    )
 
 
 @pytest.mark.parametrize("good_v", ("3.0", "3.1", "3", "3.33", "4", "100"))
@@ -182,6 +201,20 @@ def test_peer_relation_consistency():
     )
 
 
+def test_duplicate_endpoints_inconsistent():
+    assert_inconsistent(
+        State(),
+        Event("bar"),
+        _CharmSpec(
+            MyCharm,
+            {
+                "requires": {"foo": {"interface": "bar"}},
+                "provides": {"foo": {"interface": "baz"}},
+            },
+        ),
+    )
+
+
 def test_sub_relation_consistency():
     assert_inconsistent(
         State(relations=[Relation("foo")]),
@@ -191,6 +224,7 @@ def test_sub_relation_consistency():
             {"requires": {"foo": {"interface": "bar", "scope": "container"}}},
         ),
     )
+
     assert_consistent(
         State(relations=[SubordinateRelation("foo")]),
         Event("bar"),
@@ -226,6 +260,32 @@ def test_container_pebble_evt_consistent():
     )
 
 
+def test_action_not_in_meta_inconsistent():
+    action = Action("foo", params={"bar": "baz"})
+    assert_inconsistent(
+        State(),
+        action.event,
+        _CharmSpec(MyCharm, meta={}, actions={}),
+    )
+
+
+def test_action_meta_type_inconsistent():
+    action = Action("foo", params={"bar": "baz"})
+    assert_inconsistent(
+        State(),
+        action.event,
+        _CharmSpec(
+            MyCharm, meta={}, actions={"foo": {"params": {"bar": {"type": "zabazaba"}}}}
+        ),
+    )
+
+    assert_inconsistent(
+        State(),
+        action.event,
+        _CharmSpec(MyCharm, meta={}, actions={"foo": {"params": {"bar": {}}}}),
+    )
+
+
 def test_action_name():
     action = Action("foo", params={"bar": "baz"})