Re: [PATCH v3 2/3] Include the responsible actor in applicable events

2019-11-14 Thread Andrew Donnellan
On 15/11/19 9:37 am, Johan Herland wrote:>>> The remaining events 
(cover-created, series-created) are both triggered

by incoming emails, hence have no real actor as such, so we simply leave
the actor as None/NULL.


How is cover-created different from patch-created?


In practice, it turns out there is no difference, really:

The patch-created event is triggered by a signal from the Patch model
which potentially carries the ._edited_by attribute. However, AFAICS,
when patches are created, there is no preceding call to
Patch.is_editable(), hence Patch._edited_by is not set, and we end up
passing actor=None to Event.objects.create().

The cover-created event is triggered by a signal from CoverLetter
which has no ._edited_by, hence we pass no actor to
Events.objects.create() and it defaults to None.

I still figured I'd wire up the logic for patch-created, just in case
we at some point were to start setting ._edited_by on Patch objects
when they are first created.


OK. Personally I'd rather we not wire it up, as it's confusing to see 
something like that wired up and wonder why the field isn't being set.


It's also not clear how you should set the actor on email-triggered 
events as not every sender has a Patchwork account.



Andrew

--
Andrew Donnellan  OzLabs, ADL Canberra
a...@linux.ibm.com IBM Australia Limited

___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork


DB-murdering API query (index suggestions needed)

2019-11-14 Thread Konstantin Ryabitsev
Hi, all:

Today, the DB behind patchwork.kernel.org was in a semi-permanent state 
of suffering due to someone trying to suck down all patches in the 
linux-arm-kernel project. This is what the API request looked like:

GET 
/api/1.1/patches/?project=62&before=2019-11-01T00:00:00&per_page=100&page=6150 

The query behind this takes about 1 minute to run on a 20-core HT Xeon 
system and requires creating a huge temporary file (there are 18375 
patches in that project).

So, two questions, really:

1. Any indexes we can put in place to make this query perform better?
2. Is there a way to disable anonymous API access?

-K
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork


Re: [PATCH v3 2/3] Include the responsible actor in applicable events

2019-11-14 Thread Johan Herland
On Thu, Nov 14, 2019 at 6:31 AM Andrew Donnellan  wrote:
> On 17/10/19 9:44 am, Johan Herland wrote:
> > We want to use the events as an audit log. An important part of this is
> > recording _who_ made the changes that the events represent.
> >
> > To accomplish this, we need to know the current user (aka. request.user)
> > at the point where we create the Event instance. Event creation is
> > currently triggered by signals, but neither the signal handlers, nor the
> > model classes themselves have easy access to request.user.
> >
> > For Patch-based events (patch-created, patch-state-changed,
> > patch-delegated, patch-completed), we can do the following hack:
> > The relevant events are created in signal handlers that are all hooked
> > up to either the pre_save or post_save signals sent by Patch.save().
> > But before calling Patch.save(), Patchwork must naturally query
> > Patch.is_editable() to ascertain whether the patch can in fact be
> > changed by the current user. Thus, we only need a way to communicate
> > the current user from Patch.is_editable() to the signal handlers that
> > create the resulting Events. The Patch object itself is available in
> > both places, so we simply add an ._edited_by attribute to the instance
> > (which fortunately is not detected as a persistent db field by Django).
> >
> > The series-completed event is also triggered by Patch.save(), so uses
> > the same trick as above.
> >
> > For the check-created event the current user always happens to be the
> > same as the .user field recorded in the Check object itself.
> >
> > The remaining events (cover-created, series-created) are both triggered
> > by incoming emails, hence have no real actor as such, so we simply leave
> > the actor as None/NULL.
>
> How is cover-created different from patch-created?

In practice, it turns out there is no difference, really:

The patch-created event is triggered by a signal from the Patch model
which potentially carries the ._edited_by attribute. However, AFAICS,
when patches are created, there is no preceding call to
Patch.is_editable(), hence Patch._edited_by is not set, and we end up
passing actor=None to Event.objects.create().

The cover-created event is triggered by a signal from CoverLetter
which has no ._edited_by, hence we pass no actor to
Events.objects.create() and it defaults to None.

I still figured I'd wire up the logic for patch-created, just in case
we at some point were to start setting ._edited_by on Patch objects
when they are first created.

Hope this helps,

...Johan

-- 
Johan Herland, 
www.herland.net
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork