[01:15] <lifeless> cjwatson: hi
[01:16] <lifeless> cjwatson: lazr.restful walks the declared attributes of its results
[01:16] <lifeless> cjwatson: catching permission errors and eliding those things, whatever they are.
[01:16] <cjwatson> OK
[01:16] <lifeless> cjwatson: so for a collection, it materializes the batch, then iterates
[01:17] <lifeless> if you have objects that have permission checks which require db lookups, you end up calling whatever logic is needed for each object -> very slow.
[01:18] <cjwatson> Got it, I think.  EntryResource.redacted_fields?
[01:18] <lifeless> Person._teamPrivacyQuery is likely what you want to use, however I'm stale on the detail; suggest you consult with purple (e.g. wgrant) about the current state of the art
[01:21] <wgrant> lifeless, cjwatson: Well, this is an... interesting... case
[01:21] <wgrant> Times you don't want to filter stuff out:
[01:21] <wgrant>  - auditing permissions
[01:21] <lifeless> cjwatson: So, I haven't read the plumbing in lazr.restful, but that sounds likely.
[01:22] <lifeless> wgrant: interacting w/public object -> discloses team.
[01:22] <wgrant> lifeless: Sure
[01:22] <wgrant> lifeless: this is an interaction with a public object
[01:22] <lifeless> wgrant: so if we add the rule, add it to the relevant core query, then join back on that.
[01:22] <wgrant> So it discloses the team
[01:22] <wgrant> So filtering based on privacy is wrong
[01:22] <wgrant> lifeless: no, that is impractical
[01:22] <wgrant> We can't just add all 90 FKs to the privacy check
[01:23] <wgrant> However
[01:23] <wgrant> I suspect that private team linkage checks might forbid ArchivePermissions
[01:24] <cjwatson> Is this a case where you should be forbidden from creating such APs in the first place?
[01:24] <wgrant> Right, I suspect it may already be
[01:24] <wgrant> Although the vocab is ValidPersonOrTeam, so maybe not
[01:25] <wgrant> lifeless: The private team security adapter is already very slow. We need to make it faster, not slower.
[01:25] <cjwatson> I rather suspect there are none right now and we could (a) check that quickly (b) forbid it henceforth.
[01:26] <wgrant> We eventually shouldn't forbid it, but it's the right thing to do now, until we have private team privacy sorted out
[01:27] <lifeless> so derived distros will want private teams
[01:27] <cjwatson> I'm not entirely convinced that a public archive would ever want to have upload permissions associated with it for a private team.
[01:27] <lifeless> do AP's apply to ppas ?
[01:27] <wgrant> lifeless: They do.
[01:28] <lifeless> and we have private teams with private ppas
[01:28] <wgrant> Yes.
[01:28] <wgrant> But they don't necessarily have APs
[01:28] <wgrant> For PPAs, the archive owner has implicit access
[01:28] <lifeless> so I believe this invalidates any assertion about public being the only valid choice in the system
[01:28] <cjwatson> I didn't assert that
[01:28] <cjwatson> Would the owner of a private PPA want to grant upload permissions to a team whose membership they can't see?
[01:28] <cjwatson> Surely not.
[01:29] <wgrant> cjwatson: They can see it
[01:29] <lifeless> cjwatson: no, but thats != not using private teams
[01:29] <wgrant> cjwatson: Because they're in the team
[01:29] <cjwatson> Indeed
[01:29] <lifeless> or they own it
[01:29] <cjwatson> Hence showing that team => not a problem
[01:29] <lifeless> and the implicit visibility rule says that they should be able to see it afterwards anyway.
[01:29] <cjwatson> I assert that if you can see the archive, you should be able to see the teams with permissions to upload to it
[01:29] <lifeless> so its fine, I think, to say that this API can force showing everything.
[01:29] <wgrant> cjwatson: The rule is that you can see the name, displayname and owner
[01:29] <wgrant> cjwatson: You can't see the membership
[01:29] <lifeless> but we need the reflexive side to work as well.
[01:30] <cjwatson> And that if that isn't true, we've made a mistake in allowing those permissions to be granted in the first place
[01:30] <lifeless> which is to say that if you see a team in the API you need to be able to see the team directly as well (limitedview)
[01:30] <cjwatson> (Note this API is only usable by people who can see the archive)
[01:31] <lifeless> sure
[01:31] <lifeless> I think wgrant and I are riffing about how to make sure the reflexive side works, more than anything.
[01:31] <wgrant> And we have strongly disagreed on the approach in the past :)
[01:31] <lifeless> that, and that the embedded team object in the API result needs to be appropriately neutered.
[01:32] <wgrant> lifeless: The team shouldn't be embedded, I don't think
[01:32] <lifeless> wgrant: think of it as a challenge to your ingenuity
[01:32] <wgrant> It'll include a link
[01:32] <wgrant> And should only check LimitedView for that
[01:32] <lifeless> wgrant: I'd like to be confident of that
[01:32] <wgrant> lifeless: Well
[01:32] <lifeless> wgrant: a test, even if thrown away afterwards, would do that.
[01:32] <wgrant> The way you become confident is to only grant LimitedView
[01:32] <wgrant> Not View
[01:32] <lifeless> wgrant: sure, +1
[01:34] <cjwatson> So, aside from the angle of avoiding unwanted disclosure or not, lifeless pointed out that not doing some kind of privacy check up front appears to be a performance issue
[01:35] <wgrant> Well
[01:35] <lifeless> history tells us :>
[01:35] <wgrant> As long as you preload the people themselves, it won't be a performance issue unless there are private teams.
[01:35] <cjwatson> If we're not filtering, then presumably we at least need to preload
[01:35] <cjwatson> Right
[01:35] <lifeless> there is a 'Person' eager loader which does the right thing for teams too
[01:35] <wgrant> (Since the privacy check on any person other than a private team is a trivial inline attribute access)
[01:35] <lifeless> so using that + injecting LimitedView onto everything should be sufficient
[01:37] <cjwatson> i.e. precache_permission_for_objects?
[01:38] <wgrant> Right
[01:49] <cjwatson> wgrant: Does my proposal in bug 745799 look awful?
[01:49] <_mup_> Bug #745799: DistroSeries:+queue Timeout accepting packages (bug structural subscriptions) <timeout> <Launchpad itself:Triaged> < https://launchpad.net/bugs/745799 >
[01:50] <cjwatson> It's kind of a hack around structsubs being insane, but ...
[01:58] <wgrant> cjwatson: "awful"? Yes. "necessary anyway"? Probably. I need to look at related structsub performance issues with StevenK on Monday, but I suspect the fix will be sufficiently uninvasive that it doesn't fix your issue.
[02:00] <cjwatson> The current implementation is sufficiently recursive that I couldn't see a way to improve it without ripping much of it to shreds
[02:01] <wgrant> Yeah
[02:01] <wgrant> It's reasonably likely that the refactoring that we require will make it substantially less bad.
[02:01] <wgrant> But it probably won't be reliably good enough for your uses.
[02:02] <cjwatson> Seeing as the list of bugs to close can be arbitrarily long, indeed
[02:02] <wgrant> Right
[02:03] <wgrant> We're only really interested in optimising for the single bug case.
[02:03] <wgrant> And that's all that's feasible to do at this stage without massive refactoring.
[02:03] <cjwatson> If you'd feel like allocating me a DB patch number, I can start with moving the review process for this into merge proposals and see what people think there
[02:04] <wgrant> cjwatson: Why's distroseries nullable/
[02:04] <wgrant> You could interpret that to mean upload_distroseries
[02:05] <wgrant> But that seems pointless.
[02:05] <cjwatson> cf. close_bugs_for_sourcepackagerelease
[02:05] <cjwatson> It's upload_distroseries in my current imp
[02:05] <cjwatson> Because otherwise I had to pass either a DSP or an SP, or whatever the objects are
[02:07] <wgrant> cjwatson: Sure, but I don't see much benefit in having the job use SPR.upload_distroseries, rather than forcing the job creators to always pass a series.
[02:08] <wgrant> SPR.upload_{distroseries,archive} are abominations. We should restrict their callsites as much as possible.
[02:08] <cjwatson> Ah, well that's possible yes
[02:08] <cjwatson> It's just close_bugs_for_queue_item, and that could use queue_item.distroseries
[02:08] <cjwatson> No idea why it doesn't
[02:09] <wgrant> Right
[02:09] <wgrant> It's only the initial non-copy PackageUpload stuff which should be using upload_* at all
[02:09] <wgrant> Everything else should use it from a publication
[02:09] <cjwatson> And "upload_distroseries" in processaccepted is actually SPPH.distroseries, not .upload_distroseries
[02:09] <cjwatson> The naming lies
[02:09] <wgrant> So keeping the upload_blah in queue.py wins for everyone
[02:09] <wgrant> Yeah, because when the function was originally written it was always the upload distroseries.
[02:10] <cjwatson> So yes, that can and should be non-nullable.
[02:11] <wgrant> Great
[02:11] <wgrant> Limiting the propagation of mistakes from 2005 is good :)
[02:11] <wgrant> I shall give you a number.