[PATCH] Allow ordering events by date

Fri Oct 18 01:09:16 AEDT 2019

On Thu, 2019-10-17 at 09:35 -0400, Jeremy Cline wrote:
> On Thu, Oct 17, 2019 at 02:07:28PM +0100, Stephen Finucane wrote:
> > On Tue, 2019-10-15 at 17:30 -0400, Jeremy Cline wrote:
> > > By default, the events API orders events by date in descending order
> > > (newest first). However, it's useful to be able to order the events by
> > > oldest events first. For example, when a client is polling the events
> > > API for new events since a given date and wishes to process them in
> > > chronological order.
> > > 
> > > Signed-off-by: Jeremy Cline <jcline at redhat.com>
> > 
> > I'd purposefully avoided doing this initially because I wanted
> > '/events' to be thought of as a firehose that should be just consumed
> > as things were generated. We could have started deleting old events
> > after e.g. 4 weeks and kill pagination entirely. In hindsight though,
> > mistakes I made during implementation, such as the use of date-based
> > rather than cursor-based pagination, and the lack of webhooks or
> > another non-polling mechanism meant things couldn't _really_ work like
> > this. In addition, there's the series that aims to add an "actor" for
> > auditing purposes, meaning we probably should kill the idea of ever
> > deleting old events. So, overall, perhaps my original goal no longer
> > makes sense and we should just do this? Daniel - what are your
> > thoughts?
> > 
> 
> Interesting. To expand a little bit on why I want this, I'm writing a
> mailing list <-> Git{Lab,Hub,Whatever} bridge. I'm just adding a Django
> application that can run along side Patchwork to handle web hooks coming
> from Git{Lab,Hub}, and toyed with the idea of just using a Django signal
> to catch when incoming patch series are done, but opted to use this API
> since that seemed like prone to breakage.
> 
> I ran into this particular chronological issue, but if this endpoint
> isn't really intended to be used this way (or rather, folks don't want
> this API to turn into that) I don't *need* this to do what I want.

To be clear, I'm very much sitting on the fence about this rn and am
looking for input from others so I can get off said fence. It's
certainly not a definite no yet :)

> > In any case, this unfortunately needs to be a little more complicated
> > than it is at the moment. Notes below.
> > 
> > > ---
> > >  patchwork/api/event.py                         |  2 +-
> > >  patchwork/tests/api/test_event.py              | 18 ++++++++++++++++++
> > >  ...-order-events-by-date-7484164761c5231b.yaml |  5 +++++
> > >  3 files changed, 24 insertions(+), 1 deletion(-)
> > >  create mode 100644 releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml
> > > 
> > > diff --git a/patchwork/api/event.py b/patchwork/api/event.py
> > > index c0d973d..e6d467d 100644
> > > --- a/patchwork/api/event.py
> > > +++ b/patchwork/api/event.py
> > > @@ -77,7 +77,7 @@ class EventList(ListAPIView):
> > >      serializer_class = EventSerializer
> > >      filter_class = filterset_class = EventFilterSet
> > >      page_size_query_param = None  # fixed page size
> > > -    ordering_fields = ()
> > > +    ordering_fields = ('date',)
> > 
> > This is going to apply to all API versions, from v1.0 to v1.2. However,
> > we actually want it to only apply to v1.2, just so API v1.0 behaves the
> > exact same on a Patchwork v2.0 instance as it does on a v2.2 instance.
> > I don't know if we've done versioning on fields before, but it should
> > be easy to override whatever method in 'ListAPIView' is responsible for
> > consuming 'ordering_field' from the querystring to ignore 'date' if API
> > version < 1.2. Let me know if you need help here.
> > 
> 
> So, I'm happy to do this if that's what is required, but I must say I
> don't see the value of it. This adds a completely optional query
> parameter that defaults to the exact same thing it did before so the API
> doesn't change unless the client is passing a bunch of nonsense
> parameters that did nothing, but happened to include the ``order=date``
> parameter. Since that's undocumented behavior I don't see this as
> breaking anything.

It's not so much that behavior will suddenly change behind people's
back, but rather avoiding confusion where this feature worked on one
instance but does nothing on another despite using the same API
version. I want to say "if you use API 1.2, this works and, if not, it
doesn't", rather than "this works on API 1.2 and also on other versions
but only if you use this PATCH version of Patchwork, which oh by the
way isn't discoverable via the API itself".

With that said, I haven't actually checked to see just how much effort
is involved here so it could be stupid big. If so, we can think about
ignoring it. I do think I'd like to try though, if that's okay?

Stephen

> Regardless, if that's what folks really want, that's what I'll do.
> 
> > >      ordering = '-date'
> > >  
> > >      def get_queryset(self):
> > > diff --git a/patchwork/tests/api/test_event.py b/patchwork/tests/api/test_event.py
> > > index 8816538..bff8f40 100644
> > > --- a/patchwork/tests/api/test_event.py
> > > +++ b/patchwork/tests/api/test_event.py
> > > @@ -149,6 +149,24 @@ class TestEventAPI(utils.APITestCase):
> > >          resp = self.client.get(self.api_url(), {'series': 999999})
> > >          self.assertEqual(0, len(resp.data))
> > >  
> > > +    def test_order_by_date_default(self):
> > > +        """Assert the default ordering is by date descending."""
> > > +        self._create_events()
> > > +
> > > +        resp = self.client.get(self.api_url())
> > > +        events = Event.objects.order_by("-date").all()
> > > +        for api_event, event in zip(resp.data, events):
> > > +            self.assertEqual(api_event["id"], event.id)
> > > +
> > > +    def test_order_by_date_ascending(self):
> > > +        """Assert the default ordering is by date descending."""
> > > +        self._create_events()
> > > +
> > > +        resp = self.client.get(self.api_url(), {'order': 'date'})
> > > +        events = Event.objects.order_by("date").all()
> > > +        for api_event, event in zip(resp.data, events):
> > > +            self.assertEqual(api_event["id"], event.id)
> > > +
> > >      def test_create(self):
> > >          """Ensure creates aren't allowed"""
> > >          user = create_maintainer()
> > > diff --git a/releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml b/releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml
> > > new file mode 100644
> > > index 0000000..5d5328d
> > > --- /dev/null
> > > +++ b/releasenotes/notes/api-order-events-by-date-7484164761c5231b.yaml
> > > @@ -0,0 +1,5 @@
> > > +---
> > > +features:
> > 
> > We have an 'api' section for this stuff which should be used here.
> > 
> 
> Ah, right.
> 
> > > +  - |
> > > +    Allow ordering events from the events API by date. This can be done by
> > > +    adding ``order=date`` or ``order=-date`` (the default) parameters.
> 
> Thanks for the review!
> 
> - Jeremy
> 