Repository: canonical/operator
Pull request #439 (2020-10-28 15:30:52)

Make JujuStorage.notices respect the api :-)
Without this change, JujuStorage's notices method would always check
that the notices it yields matched the `event_path` parameter, despite
the docstring saying it was optional.

This fixes that, and adds test for the documented behaviour.

All hail @davigar15 for spotting the bug that resulted from this.

Fixes: #438

jameinel: If the docstring says we support '' then we can, but we should add a test
to that effect :)

John
=:->


On Wed, Oct 28, 2020 at 10:40 AM chipaca <notifications@github.com> wrote:

> *@chipaca* commented on this pull request.
> ------------------------------
>
> In ops/storage.py
> <https://github.com/canonical/operator/pull/439#discussion_r513547775>:
>
> > @@ -252,7 +252,7 @@ def notices(self, event_path: str):
>          """
>          notice_list = self._load_notice_list()
>          for row in notice_list:
> -            if row[0] != event_path:
> +            if event_path and row[0] != event_path:
>
> that was my first implementation, but the docstring does say If not
> supplied (or None/'').
>
> Happy to change it to just be None if that's OK. We obviously have no
> tests either way :-)
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/canonical/operator/pull/439#discussion_r513547775>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AABRQ7O2FRPINED6I3EPDNDSNA3NRANCNFSM4TCPQQXQ>
> .
>

chipaca: > we should add a test to that effect :)

it's all there, testing nothing, explicit `None`, and `''`.

Patch:
diff --git a/ops/storage.py b/ops/storage.py
index 6593d7c..562cde7 100755
--- a/ops/storage.py
+++ b/ops/storage.py
@@ -138,7 +138,7 @@ class SQLiteStorage:
                AND method_name=?
             ''', (event_path, observer_path, method_name))
 
-    def notices(self, event_path: typing.Optional[str]) ->\
+    def notices(self, event_path: str = None) ->\
             typing.Generator[typing.Tuple[str, str, str], None, None]:
         """Part of the Storage API, return all notices that begin with event_path.
 
@@ -240,7 +240,7 @@ class JujuStorage:
         notice_list.remove([event_path, observer_path, method_name])
         self._save_notice_list(notice_list)
 
-    def notices(self, event_path: str):
+    def notices(self, event_path: str = None):
         """Part of the Storage API, return all notices that begin with event_path.
 
         Args:
@@ -252,7 +252,7 @@ class JujuStorage:
         """
         notice_list = self._load_notice_list()
         for row in notice_list:
-            if row[0] != event_path:
+            if event_path and row[0] != event_path:
                 continue
             yield tuple(row)
 


Test patch:
diff --git a/test/test_storage.py b/test/test_storage.py
index dad5769..75e49ff 100755
--- a/test/test_storage.py
+++ b/test/test_storage.py
@@ -169,6 +169,25 @@ class StoragePermutations(abc.ABC):
             list(store.notices('event')),
             [('event', 'observer', 'method')])
 
+    def test_all_notices(self):
+        notices = [('e1', 'o1', 'm1'), ('e1', 'o2', 'm2'), ('e2', 'o3', 'm3')]
+        store = self.create_storage()
+        for notice in notices:
+            store.save_notice(*notice)
+
+        # passing in the arg, you get the ones that match
+        self.assertEqual(list(store.notices('e1')), notices[:2])
+        self.assertEqual(list(store.notices('e2')), notices[2:])
+        # the match is exact
+        self.assertEqual(list(store.notices('e%')), [])
+        self.assertEqual(list(store.notices('e*')), [])
+        self.assertEqual(list(store.notices('e.')), [])
+        self.assertEqual(list(store.notices('e')), [])
+        # no arg, or non-arg, means all
+        self.assertEqual(list(store.notices()), notices)
+        self.assertEqual(list(store.notices(None)), notices)
+        self.assertEqual(list(store.notices('')), notices)
+
     def test_load_notices(self):
         store = self.create_storage()
         self.assertEqual(list(store.notices('path')), [])