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