[PATCH] Allow ordering events by date

Fri Oct 18 02:03:03 AEDT 2019

On Thu, 2019-10-17 at 14:19 +0000, Jeremy Cline wrote:
> On Thu, Oct 17, 2019 at 03:09:16PM +0100, Stephen Finucane wrote:
> > 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 :)
> > 
> 
> Yeah, I mostly provided that so it was clear that it was just a
> nice-to-have for me.
> 
> > > > 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?
> > 
> 
> Ah, okay. Yeah, I don't think it'll be terribly hard, it just means more
> tests and so on. It seems to me that the "real" problem is that there's
> not an endpoint to retrieve the server patchwork version from? Or a
> header with the version included, or something.

It's less that an more that our API versions are separated from our
main version, so we need to do things to ensure behavior in the former
is consistent across multiple releases of the latter.

> Like I said, I don't mind doing it, it just seemed like an odd thing to
> do and I've never seen a REST API do v1.y versions. I'm still in the
> "it's an odd thing to do" camp, but there's a reason for it so that's
> okay.
> 
> If I get the itch to do some Patchwork work, would you welcome such an
> introspection API? And totally unrelated to this patch, would you accept
> a setup.py to make Patchwork installable with pip/setuptools?

For the former, sure. I've considered exposing an 'X-Patchwork-API-
Version' header in responses previously, so doing something similar but
for server version would probably make sense. For the latter, we can do
that but we will need to shuffle some things around before we do. To
the best of my knowledge, distributable Django applications shouldn't
include things like a 'manage.py' file. Mailman [1], with its
'example_project' folder, is probably a good template to follow. I also
think this should wait until 3.0 rather than 2.2 since it will require
a few changes to deployment tooling people have built up.

Stephen

[1] https://gitlab.com/mailman/hyperkitty/

> - Jeremy