Merge lp:~james-w/python-oops/refactor-publishing into lp:python-oops

[James Westby]

Refactor publication to better support publish_only_new.

Hi,

Here's a stab at the approach we discussed on IRC, what do you think?

I think that maybe the publish method on Config should still do some looking
at the returned ids so that it can update the report, as the current docstring
says it is useful when there's only one publisher.

The approach taken was to leave things pretty much as they are for current
clients, but allow anyone that wants to allocate ids to have APIs they can use
to do it without losing fallback. Once clients have transitioned to the new
API then some cleanup can be done.

Thanks,

James

[James Westby (james-w)]

Hi,

I'm going to ask for a review from any of the reviewers so that I
can move forwards with this.

Thanks,

James

[Richard Harding (rharding)]

Couple of nitpicks.

Typo in the comment 'publihers' in two cases there.

The 'only_new' terminology kind of confused me as I was expected it to refer to newly published items, but it seems it's more of a flag to only pass along items not already published. So I wonder if s/only_new/only_unpublished might be more explicit to the check going on.

For the deprecated paths, should we toss out deprecation warnings and use the normal XXX: policy to file a bug that this needs to get cleaned out?

https://dev.launchpad.net/PolicyandProcess/XXXPolicy

review: Approve

[lp:~james-w/python-oops/refactor-publishing]

31. By James Westby on 2012-06-19

Small fixes from review. Thanks Rick.

32. By James Westby on 2012-06-19

Add a deprecation warning and XXX. Thanks Rich.

[Robert Collins (lifeless)]

The publish attribute needs more docs - what should it return, what is the protocol for a publisher with it. That will let the PublishToMany docs be smaller and more compact.

The results of .publish and .publishers should be compatible, so it doesn't need to be either or. Just call one then the other (eases migration).

I'm not sure about the 'and it picks one id' bit; that discards data. I'd be a lot happier without it. We can have a trivial publisher to choose a single result, to save folk reimplementing that.
def publish_one_id(publisher):
    def publish_one_id(report):
        ret = publisher(report)
        if not ret:
          return ret
        return ret[-1]
    return publish_one_id

You might like to extend list instead of composing a list; that would make introspection a little easier, give a default repr thats useful and so on.

Rather than a publish() method, define __call__, as the contract for publishers is that of simple callables, see what oops_amqp does, for instance.

Looking at this, I think we can make it simpler and remove the need for a bunch of tests; if PublishToMany were instead called 'publish_with_fallback', it could be:
def publish_with_fallback(*publishers):
    def result(report):
        ret = []
        for publisher in publishers:
            ret.extend(publisher(report)
            if ret:
                break
        return ret
    return result

publish_to_many then is just:
def publish_to_many(*publishers):
    def result(report):
        ret = []
        for publisher in publishers:
            ret.extend(publisher(report)
        return ret
    return result

What do you think?

[lp:~james-w/python-oops/refactor-publishing]

33. By James Westby on 2012-06-20

Improve the docstrings. Thanks Rob.

34. By James Westby on 2012-06-20

Call both `publisher` and `publishers`.

35. By James Westby on 2012-06-20

Subclass list rather than compose it. Thanks Rob.

36. By James Westby on 2012-06-20

Use `__call__` rather than `publish`. Thanks Rob.

37. By James Westby on 2012-06-20

Use a couple of functions to do publish_to_many and publish_with_fallback. Thanks Rob.

38. By James Westby on 2012-06-20

Add a converter for old-style publishers.

[James Westby (james-w)]

Hi,

I've made the suggested changes.

Thanks,

James

[Robert Collins (lifeless)]

112 + allocated-or-used for the report. The `publishers` should return any
would read better as 'should each return'

116 + non-True (that is None, 0, False, ''), it indicate s that the
excess ' '

What do you think about not warning in this release? Give folk a chance to update without nags, and then nag later?

+ to return a list of ids. The last id in the returned list
51 + will be assigned to the reports 'id' key. See the publish() method
52 + for more information.

This makes a false claim AFAICT: nothing ensures that that assignment takes place.

The handling of publishers could be delegated to publish_to_many:

def publish(self, report):
    publishers = publish_to_many(*map(convert_result_to_list, self.publishers))
    if self.publisher:
        publisher = publish_to_many(self.publisher, publishers)
    else:
        publisher = publishers
    return publisher(report)

convert_result_to_list seems overly smart to me: I see the convenience (just wrap any publisher and its ok), but it makes the code more complex; it makes it significantly harder to deprecate as it doesn't tell you when a publisher has been migrated - though we can obviously add that).
I think I'd rather see:
convert_result_to_list = lambda x:[x] if x else []
where we have a crisp function and as folk update their dependencies they migrate from publishers to publisher.

This is a little confused about plurals:
190 + When publishing oopses to this publisher they will be in turn published
191 + to all publisher registered to this instance until one of the publishers
192 + reports successful publication, at which point no further publishers
193 + will be tried.
Perhaps:
This publisher will attempt to publish to each of the supplied publishers in turn until one of them successfully publishes the report.

"accumulated list of all lists " is a bit ambiguous (does it mean [[], [], []] or [], for all empty responses. Uhm. Perhaps the problem is that the docs are duplicating the definition of publisher at each place. it might be simpler to say 'this is a publisher, see Config.publish for the calling and return conventions. This publisher delegates to the supplied publishers by calling them all, and aggregates the results.' (Similarly for the first-successful case). What do you think?

TestPublishToMany is missing a corner case (no publishers) for its return value assertions.
Also, test_adds_ids_from_publishers is sufficient to test the 1-item case (if 0 works and 2 works, one really can't not).

test_doesnt_publish_old might be better as test_publish_stops_when_a_publisher_succeeds (as age isn't really the issue); also, its easier to prove a positive :)

[lp:~james-w/python-oops/refactor-publishing]

39. By James Westby on 2012-06-26

More fixes from review. Thanks Rob.

[James Westby (james-w)]

Done.

> What do you think about not warning in this release? Give folk a chance to
> update without nags, and then nag later?

I'm going to leave the warning as instructed by #launchpad-dev. If you feel strongly
I'll take it out.

> The handling of publishers could be delegated to publish_to_many:
>
> def publish(self, report):
> publishers = publish_to_many(*map(convert_result_to_list,
> self.publishers))
> if self.publisher:
> publisher = publish_to_many(self.publisher, publishers)
> else:
> publisher = publishers
> return publisher(report)

Done, but this changes the behaviour, as nothing puts the id in the report
now, and so old style fallbacks won't work, and neither will any code that
expects to find the id in the report.

Thanks,

James

[Robert Collins (lifeless)]

Ah, we covered this on IRC, but I forgot to mention here.

I was proposing that:
 - publish(self, report): assign the last id to the report, for callers convenience.
 - publish_to_many(report) assign the id to the report as it goes, to allow multiple publishers to agree on the id.

[lp:~james-w/python-oops/refactor-publishing]

40. By James Westby on 2012-06-26

Revert to putting ids in the report.

41. By James Westby on 2012-06-26

Document the putting of the id in to the report.

=== modified file 'oops/__init__.py'
--- oops/__init__.py	2011-11-13 21:28:06 +0000
+++ oops/__init__.py	2012-06-26 20:59:19 +0000
@@ -29,8 +29,16 @@
 
 __all__ = [
     'Config',
+    'convert_result_to_list',
     'publish_new_only',
+    'publish_to_many',
+    'publish_with_fallback',
     ]
 
 from oops.config import Config
-from oops.publishers import publish_new_only
+from oops.publishers import (
+    convert_result_to_list,
+    publish_new_only,
+    publish_to_many,
+    publish_with_fallback,
+    )

=== modified file 'oops/config.py'
--- oops/config.py	2011-11-13 21:24:42 +0000
+++ oops/config.py	2012-06-26 20:59:19 +0000
@@ -24,7 +24,7 @@
   >>> config = Config()
   >>> def demo_publish(report):
   ...    return 'id 1'
-  >>> config.publishers.append(demo_publish)
+  >>> config.publisher = demo_publish
 
   This allows aggregation of oops reports from different programs into one
   oops-tools install.
@@ -100,33 +100,46 @@
 __metaclass__ = type
 
 from copy import deepcopy
+import warnings
 
 from createhooks import default_hooks
+from publishers import (
+    convert_result_to_list,
+    publish_to_many,
+    )
 
 
 class Config:
     """The configuration for the OOPS system.
-    
+
     :ivar on_create: A list of callables to call when making a new report. Each
         will be called in series with the new report and a creation context
         dict. The return value of the callbacks is ignored.
     :ivar filters: A list of callables to call when filtering a report. Each
         will be called in series with a report that is about to be published.
-        If the filter returns true (that is not None, 0, '' or False), then 
+        If the filter returns true (that is not None, 0, '' or False), then
         the report will not be published, and the call to publish will return
         None to the user.
+    :ivar publisher: A callable to call when publishing a report.
+        It will be called in series with the report to publish. It is expected
+        to return a list of ids. See the publish() method for more
+        information.
     :ivar publishers: A list of callables to call when publishing a report.
         Each will be called in series with the report to publish. Their return
         value will be assigned to the reports 'id' key : if a publisher
         allocates a different id than a prior publisher, only the last
         publisher in the list will have its id present in the report at the
-        end. See the publish() method for more information.
+        end. See the publish() method for more information. This attribute
+        is deprecated, Use the `publisher` attribute instead, and see
+        `oops.publishers.publish_to_many` if you want to publish to multiple
+        publishers.
     """
 
     def __init__(self):
         self.filters = []
         self.on_create = list(default_hooks)
         self.template = {}
+        self.publisher = None
         self.publishers = []
 
     def create(self, context=None):
@@ -153,43 +166,54 @@
     def publish(self, report):
         """Publish a report.
 
-        Each publisher is passed the report to publish. Publishers should
-        return the id they allocated-or-used for the report, which gets
-        automatically put into the report for them. All of the ids are also
-        returned to the caller, allowing them to handle the case when multiple
-        publishers allocated different ids. The id from the last publisher is
-        left in the report's id key. This is done for three reasons:
-        
-        * As a convenience for the common case when only one publisher is
-          present.
-          
-        * This allows the next publisher in the chain to reuse that id if it
-          wishes.
-          
-        * Showing users that a report was made only needs one id and this means
-          other code doesn't need to arbitrarily pick one - it is done by
-          publish().
-
-        If a publisher raises an exception, that will propagate to the caller.
-        If a publisher returns anything non-True (that is None, 0, False, ''),
-        it indicates that the publisher did not publish the report and no change
-        will be made to the report's 'id' key for that publisher.
-        This permits fallback chains. For instance the first publisher may
-        choose not to publish a report (e.g. the place it would publish to is
-        offline) and the second publisher can detect that by checking if there
-        is an id key.
+        The report will be passed through any filters, and then handed
+        off to the callable assigned to the `publisher` instance variable,
+        if any, and then passed through any publishers in the `publishers`
+        list instance variable. The return value will be the list returned
+        by `publisher` method, with the ids returned by the `publishers`
+        appended. The `publishers` list is deprecated, and the `publisher`
+        attribute should be used instead, with
+        `oops.publishers.publish_to_many` used if needed.
+
+        The `publisher` should return a list of ids that were
+        allocated-or-used for the report. The `publishers` should each return
+        any id that was allocated-or-used for the report. The return value of
+        the callables in the `publishers` list will be assigned to the `id`
+        key of the report. If a publisher in the `publishers` list returns
+        anything non-True (that is None, 0, False, ''), it indicates that the
+        publisher did not publish the report.
+
+        The last entry in the list of ids, if any, will be assigned to the 'id'
+        key of the report before returning, so that clients that only care to
+        deal with one id don't have to pick one themselves.
+
+        The whole list of ids will be returned to the caller to allow them
+        to handle the case where multiple ids were used for a report.
+
+        If any publisher raises an exception, that will propagate to the caller.
 
         :return: A list of the allocated ids.
         """
         for report_filter in self.filters:
             if report_filter(report):
                 return None
-        result = []
-        for publisher in self.publishers:
-            id = publisher(report)
-            if id:
-                report['id'] = id
-                result.append(id)
-            else:
-                result.append(None)
-        return result
+        # XXX: james_w 2012-06-19 bug=1015293: Deprecated code path,
+        #       this should be removed once users have had a chance
+        #       to migrate. The constructor and docstrings should
+        #       also be cleaned up at the same time.
+        if self.publishers:
+            warnings.warn(
+                "Using the oops.Config.publishers attribute is "
+                "deprecated. Use the oops.Config.publisher attribute "
+                "instead, with an oops.publishers.publish_to_many object "
+                "if multiple publishers are needed",
+                DeprecationWarning, stacklevel=2)
+        old_publishers = map(convert_result_to_list, self.publishers)
+        if self.publisher:
+            publisher = publish_to_many(self.publisher, *old_publishers)
+        else:
+            publisher = publish_to_many(*old_publishers)
+        ret = publisher(report)
+        if ret:
+            report['id'] = ret[-1]
+        return ret

=== modified file 'oops/publishers.py'
--- oops/publishers.py	2011-11-13 21:24:42 +0000
+++ oops/publishers.py	2012-06-26 20:59:19 +0000
@@ -21,9 +21,10 @@
     'publish_new_only',
     ]
 
+
 def publish_new_only(publisher):
     """Wraps a publisher with a check that the report has not had an id set.
-    
+
     This permits having fallback publishers that only publish if the earlier
     one failed.
 
@@ -31,9 +32,79 @@
 
       >>> config.publishers.append(amqp_publisher)
       >>> config.publishers.append(publish_new_only(datedir_repo.publish))
+
+    This function is deprecated. Instead please use publish_with_fallback.
     """
     def result(report):
         if report.get('id'):
             return None
         return publisher(report)
     return result
+
+
+def publish_with_fallback(*publishers):
+    """A publisher to fallback publishing through a list of publishers
+
+    This is a publisher, see Config.publish for the calling and return
+    conventions. This publisher delegates to the supplied publishers
+    by calling them all until one reports that it has published the
+    report, and aggregates the results.
+
+    :param *publishers: a list of callables to publish oopses to.
+    :return: a callable that will publish a report to each
+        of the publishers when called.
+    """
+    def result(report):
+        ret = []
+        for publisher in publishers:
+            ret.extend(publisher(report))
+            if ret:
+                break
+        return ret
+    return result
+
+
+def publish_to_many(*publishers):
+    """A fan-out publisher of oops reports.
+
+    This is a publisher, see Config.publish for the calling and return
+    conventions. This publisher delegates to the supplied publishers
+    by calling them all, and aggregates the results.
+
+    If a publisher returns a non-emtpy list (indicating that the report was
+    published) then the last item of this list will be set as the 'id' key
+    in the report before the report is passed to the next publisher. This
+    makes it possible for publishers later in the chain to re-use the id.
+
+    :param *publishers: a list of callables to publish oopses to.
+    :return: a callable that will publish a report to each
+        of the publishers when called.
+    """
+    def result(report):
+        ret = []
+        for publisher in publishers:
+            if ret:
+                report['id'] = ret[-1]
+            ret.extend(publisher(report))
+        return ret
+    return result
+
+
+def convert_result_to_list(publisher):
+    """Ensure that a publisher returns a list.
+
+    The old protocol for publisher callables was to return an id, or
+    a False value if the report was not published. The new protocol
+    is to return a list, which is empty if the report was not
+    published.
+
+    This function coverts a publisher using the old protocol in to one that
+    uses the  new protocol, translating values as needed.
+    """
+    def publish(report):
+        ret = publisher(report)
+        if ret:
+            return [ret]
+        else:
+            return []
+    return publish

=== modified file 'oops/tests/test_config.py'
--- oops/tests/test_config.py	2011-11-13 21:24:42 +0000
+++ oops/tests/test_config.py	2012-06-26 20:59:19 +0000
@@ -72,14 +72,12 @@
         def pub_1(report):
             return '1'
         def pub_2(report):
-            self.assertEqual('1', report['id'])
             return '2'
         config = Config()
         config.publishers.append(pub_1)
         config.publishers.append(pub_2)
         report = {}
         self.assertEqual(['1', '2'], config.publish(report))
-        self.assertEqual({'id': '2'}, report)
 
     def test_publish_filters_first(self):
         calls = []
@@ -101,7 +99,7 @@
         self.assertEqual(['ok', 'block'], calls)
 
     def test_publishers_returning_not_published_no_change_to_report(self):
-        # If a publisher returns non-zero (indicating it did not publish) no
+        # If a publisher returns False (indicating it did not publish) no
         # change is made to the report - this permits chaining publishers as a
         # primary and fallback: The fallback can choose to do nothing if there
         # is an id already assigned (and returns None to signal what it did);
@@ -114,5 +112,45 @@
         config.publishers.append(pub_succeed)
         config.publishers.append(pub_noop)
         report = {}
-        self.assertEqual(['1', None], config.publish(report))
-        self.assertEqual({'id': '1'}, report)
+        self.assertEqual(['1'], config.publish(report))
+
+    def test_publish_to_publisher(self):
+        calls = []
+        config = Config()
+        def succeed(report):
+            calls.append(report.copy())
+            return ['a']
+        config.publisher = succeed
+        report = dict(foo='bar')
+        config.publish(report)
+        self.assertEqual([dict(foo='bar')], calls)
+
+    def test_returns_return_value_of_publisher(self):
+        ids = ['a', 'b']
+        def succeed(report):
+            return ids
+        config = Config()
+        config.publisher = succeed
+        report = dict(foo='bar')
+        self.assertEqual(ids, config.publish(report))
+
+    def test_publisher_and_publishers_used_together(self):
+        calls = []
+        def nopublish(report):
+            calls.append(report)
+            return []
+        config = Config()
+        config.publisher = nopublish
+        config.publishers.append(nopublish)
+        config.publish({})
+        self.assertEqual(2, len(calls))
+
+    def test_puts_id_in_report(self):
+        the_id = 'b'
+        def succeed(report):
+            return ['a', the_id]
+        config = Config()
+        config.publisher = succeed
+        report = dict(foo='bar')
+        config.publish(report)
+        self.assertEqual(the_id, report['id'])

=== modified file 'oops/tests/test_publishers.py'
--- oops/tests/test_publishers.py	2011-11-13 21:24:42 +0000
+++ oops/tests/test_publishers.py	2012-06-26 20:59:19 +0000
@@ -18,11 +18,16 @@
 __metaclass__ = type
 
 import testtools
-from testtools.matchers import Is
-
-from oops import publish_new_only
-
-class TestPublishers(testtools.TestCase):
+
+from oops import (
+    convert_result_to_list,
+    publish_new_only,
+    publish_to_many,
+    publish_with_fallback,
+    )
+
+
+class TestPublisherNewOnly(testtools.TestCase):
 
     def test_publish_new_only_id_in_report(self):
         def pub_fail(report):
@@ -35,3 +40,131 @@
         publisher = publish_new_only(calls.append)
         publisher({'foo': 'bar'})
         self.assertEqual([{'foo': 'bar'}], calls)
+
+
+class TestPublishToMany(testtools.TestCase):
+
+    def test_publishes_to_one(self):
+        calls = []
+        def capture(report):
+            calls.append(report)
+            return []
+        publisher = publish_to_many(capture)
+        publisher(dict(foo='bar'))
+        self.assertEqual([dict(foo='bar')], calls)
+
+    def test_publishes_to_two(self):
+        calls1 = []
+        calls2 = []
+        def capture1(report):
+            calls1.append(report)
+            return []
+        def capture2(report):
+            calls2.append(report)
+            return []
+        publisher = publish_to_many(capture1, capture2)
+        publisher(dict(foo='bar'))
+        self.assertEqual([dict(foo='bar')], calls1)
+        self.assertEqual([dict(foo='bar')], calls2)
+
+    def test_returns_empty_list_with_no_publishers(self):
+        publisher = publish_to_many()
+        self.assertEqual([], publisher({}))
+
+    def test_adds_ids_from_publishers(self):
+        first_ids = ['a', 'b']
+        def first_success(report):
+            return first_ids
+        second_ids = ['c', 'd']
+        def second_success(report):
+            return second_ids
+        publisher = publish_to_many(first_success, second_success)
+        self.assertEqual(first_ids + second_ids, publisher({}))
+
+    def test_puts_id_in_report(self):
+        the_id = 'b'
+        def first(report):
+            return ['a', the_id]
+        def second(report):
+            self.assertEqual(the_id, report['id'])
+            return []
+        publisher = publish_to_many(first, second)
+        publisher({})
+
+    def test_puts_nothing_in_report_for_unpublished(self):
+        def first(report):
+            return []
+        def second(report):
+            if 'id' in report:
+                self.fail("id set to %s when previous publisher "
+                    "didn't publish" % report['id'])
+            return []
+        publisher = publish_to_many(first, second)
+        publisher({})
+
+
+class TestPublishWithFallback(testtools.TestCase):
+
+    def test_publishes_to_one(self):
+        calls = []
+        def capture(report):
+            calls.append(report)
+            return []
+        publisher = publish_with_fallback(capture)
+        publisher(dict(foo='bar'))
+        self.assertEqual([dict(foo='bar')], calls)
+
+    def test_publishes_to_two(self):
+        calls1 = []
+        calls2 = []
+        def capture1(report):
+            calls1.append(report)
+            return []
+        def capture2(report):
+            calls2.append(report)
+            return []
+        publisher = publish_with_fallback(capture1, capture2)
+        publisher(dict(foo='bar'))
+        self.assertEqual([dict(foo='bar')], calls1)
+        self.assertEqual([dict(foo='bar')], calls2)
+
+    def test_returns_ids_from_publisher(self):
+        ids = ['a', 'b']
+        def success(report):
+            return ids
+        publisher = publish_with_fallback(success)
+        self.assertEqual(ids, publisher({}))
+
+    def test_publishes_new(self):
+        def failure(report):
+            return []
+        calls = []
+        def capture(report):
+            calls.append(report)
+            return []
+        publisher = publish_with_fallback(failure, capture)
+        self.assertEqual([], publisher({}))
+        self.assertEqual(1, len(calls))
+
+    def test_publish_stops_when_a_publisher_succeeds(self):
+        def success(report):
+            return ['the id']
+        def fail(report):
+            self.fail("Called fallback when primary succeeded.")
+        publisher = publish_with_fallback(success, fail)
+        self.assertEqual(['the id'], publisher({}))
+
+
+class ConvertResultToListTests(testtools.TestCase):
+
+    def test_converts_False_to_empty_list(self):
+        # A false-ish value gets turned in to an empty list
+        def falseish(report):
+            return False
+        self.assertEqual([], convert_result_to_list(falseish)({}))
+
+    def test_converts_True_to_list(self):
+        # A true-ish value gets turned in to an empty list
+        def trueish(report):
+            return "aaa"
+        self.assertEqual(["aaa"], convert_result_to_list(trueish)({}))