Merge lp://staging/~adeuring/launchpad/api-query-permissions-on-object into lp://staging/launchpad

I have a couple of concerns here.

Firstly, if these are properties they will be calculated on -every- render of the object. Permission checks are not always cheap and are sometimes very expensive. We will be shooting our performance in the foot.

Secondly, will it scale performance wise ;)

I think that rather than calculating every possible permission on every API generation we should instead permit specifying the precise things needed for a page and only calculate those.

I'm very confident that the overhead of this will mount up, so I'm going to needsfixing this and put the branch into in-progress. Sorry :(.

I'm happy to talk/IRC/email about implementation strategies if needed; offhand it looks to me like any given page knows the permissions that will be needed a-priori so can inject them into the json cache at runtime. This is probably simpler and may remove the need to expose the userCanWrite logic etc entirely.

On 18.04.2011 21:25, Robert Collins wrote:
> Review: Needs Fixing
> I have a couple of concerns here.
>
> Firstly, if these are properties they will be calculated on -every- render of the object. Permission checks are not always cheap and are sometimes very expensive. We will be shooting our performance in the foot.
>
> Secondly, will it scale performance wise ;)
>
> I think that rather than calculating every possible permission on every API generation we should instead permit specifying the precise things needed for a page and only calculate those.
>
> I'm very confident that the overhead of this will mount up, so I'm going to needsfixing this and put the branch into in-progress. Sorry :(.
>
> I'm happy to talk/IRC/email about implementation strategies if needed; offhand it looks to me like any given page knows the permissions that will be needed a-priori so can inject them into the json cache at runtime. This is probably simpler and may remove the need to expose the userCanWrite logic etc entirely.

Well, the goal of the branch is to provide access information for a page
like
https://translations.launchpad.dev/ubuntu/hoary/+source/alsa-utils/+sharing-details

whoops. Most of my reply (sent via mail) was eaten somewhere. Copied from it and pasted into the web form:

Well, the goal of the branch is to provide access information for a page
like
https://translations.launchpad.dev/ubuntu/hoary/+source/alsa-utils/+sharing-details
.

We don't need it for the "state" of the page when it is rendered. But if
a user can changes for example the packaging link, we should present the
button to set the branch linked to the product series, the button to
configure translations for the product and the button to configure
translation synchronisation only if the the user has the permission to
do that -- and we don't know this when the page is rendered.

Aaron had exactly the opposite view: We should use properties because we
get the permission details in the same webservice request that delivers
the object itself, so no need for additional requests when the packaging
is changed, just to let the JS code know which icon to show which not to
show. (Note that we need indeed two requests: one for the new product,
and one for the product series.)

Anyway, what would you suggest? something like a method

Product.userCanWrite('property_1', 'property_2', ...)?

Aaron and I had a brief discussion on IRC which may help:
07:28 < abentley> lifeless: injecting the permissions into the json
cache only helps at page load time.
07:28 < abentley> lifeless: We also need to evaluate the permissions
when we retrieve the objects over the web service.
07:28 < lifeless> abentley: that surprises me
07:28 < lifeless> abentley: can you help me understand why thats needed ?
07:30 < abentley> lifeless: A +sharing-details page may start with no
productseries associated with the sourcepackage. When the user adds a
productseries, we need to know whether the user can change the branch
associated with the
                  productseries.
07:30 < lifeless> ok
07:30 < lifeless> adding a productseries is a named method right ?
07:30 < abentley> lifeless: yes.
07:31 < lifeless> so, I can suggest alternative implementations here
07:31 < lifeless> such as return the permissions with the result of the add
07:31 < lifeless> or do a separate lookup (though latency is a pain)
07:32 < abentley> lifeless: Yes, the reason I asked for them to be
properties was to avoid latency.
07:33 < lifeless> I understand - and sympathise - the problem though
is simply that lazr evaluates -all- properties every time
07:33 < lifeless> and permission checks are often very expensive
07:34 < abentley> lifeless: another option would be to use the json
cache and then provide a way to get an updated version of the json
cache.
07:34 < lifeless> abentley: that sounds like an interesting approach
07:34 < lifeless> because the json cache is tailored to the page AIUI
07:35 < abentley> lifeless: it would slay a lot of other latency
concerns I have, too.
07:36 < abentley> lifeless: The json cache is tailored per-view.
07:38 < lifeless> abentley: excellent!
07:38 < abentley> lifeless: well, except that it's scope-creepy.
07:40 < lifeless> in terms of feature delivery? I guess yes :(
07:41 < abentley> lifeless: Originally, we were just going to expose
Person.canWrite etc. But that doesn't work because lazr.restful
doesn't support dynamic typing.
07:44 < lifeless> yeah, I saw the comment
07:44 < lifeless> its a shame, that would have been fairly pithy
07:44 < lifeless> would still have incurred a round trip for you
07:49 < lifeless> abentley: I don't want to leave you & your team hanging
07:50 < lifeless> abentley: would doing a separate round trip be
tolerable for the page? If so that seems like low development overhead
(change the property to a method)
07:52 < abentley> lifeless: We could certainly start there.
07:52 < abentley> lifeless: I have no idea what the performance of the
page is like on production.

07:53 < lifeless> only one way to find out
07:57 < abentley> lifeless: about 2 seconds.
07:57 < abentley> lifeless: for natty/bzrtools on qastaging
07:58 < lifeless> abentley: thats from choosing the productseries till
it shows up in the form, or the initial page load?
07:58 < abentley> lifeless: the former.
07:58 < lifeless> if you unset it
07:58 < lifeless> and set it again
07:58 < lifeless> it may be faster
07:59 < lifeless> qastaging only holds about 5% of the prod DB in RAM
07:59 < abentley> lifeless: I did actually.
07:59 < abentley> lifeless: that time...

On 19.04.2011 00:48, Robert Collins wrote:
[...]
>>From this I suggest the following options:
> - either returning the needed information from the set packaging call
> (perhaps by using a custom call to do what you need). More dev time
> needed, but no additional round trips and no overhead on API GET calls
> for other code paths.

That would indeed be the most efficient option. But it feels wrong to me
to return such data from Sourcepackage.setPackaging(): The corresponding
method ProductSeries.setPackaging() returns the packaging itself. While
we already have an inconsistency (Sourcepackage.setPackaging() returns
None at present), I think we should not let the method return something
that is tailored to be useful as the result of a webservice request for
a special web page. This return value would not make any sense if for
example the "+edit-packaging" view for source package would be AJAXified.

> - look into being able to get a fresh jsoncache from an API call
> (variant of the first option) - specific to a view, will include all
> the right data, easy to keep consistent with the scripts on a page

We could add something like
SourcePackage.setPackagingInTranslationSettingPage(), which just calls
setPackaging() and returns the permission information needed on the
"+sharing-details" page. This looks still a bit odd in model code, but
we don't export anything else than model classes in the webservice.

> - have a dedicated api call to check what you need - one extra
> roundtrip, but nicely isolated and reusable

Considering that the view we are talking about is not intended to be
called very frequently (once you got the translation settings right you
should no longer change them, otherwise your translators might go
ballistic...), this seems the most reasonable suggestion to me.

So, what about a mixin class with just one method, like so:

class PermissionCheck:

    def userHasPermissionsFor(
        self, access_attributes, write_attributes):

access_attributes and write_attributes would be sequences of attribute
names; the method returns True if the current user has access/write
permissions for all given attributes.

In our case, this will still require _two_ request, because we need to
check permissions for one Product attribute and for two ProductSeries
Attributes, but as written above, we are not talking about a page that
is supposed to be used 100 times per day by a single user.

That last option sounds ok. SourcePackage.setPackagingInTranslationSettingPage would be faster, if you want the page to feel snappy. I'm easy either way.

So, here is a new version of the branch. The property
ProductSeries.user_can_set_branch is gone; instead we have a
new method SourcePackage.setPackagingReturnSharingDetailPermissions(),
which does the same as setPackaging(), but returns a dictionary with
the permission information we need on the +sharing-details page.

./bin/test registry -vvt TestSourcePackage.test_setPackaging
./bin/test registry -vvt TestPerson.test_can