Merge lp:~mwhudson/launchpad/permit_timeout_from_features-on-participation-bug-861510 into lp:launchpad

I'm no kind of authority on what is tasteful in Zope or Launchpad, but this does sound good to me.

I think you should fix your 'XXX explain more' ;-) by adding something similar to the text in <https://bugs.launchpad.net/launchpad/+bug/861510/comments/3>, otherwise people will ask, as I did, why you need special handling of the hard_timeout feature flag, rather than just looking it up in the usual way.

Ah, I'd actually fixed that, but my ISP is having such a bad evening that my push had failed. It's pushed now (over 3G, sigh).

+1, but you definitely need review from someone else too

The permit_timeout_from_features flag may have been conceived to help in
checking the timeout feature flag, but it seems to me that it applies to
all feature flags. Perhaps its name should be
"permit_feature_flag_lookup" and the feature flag code should raise an
exception if asked for a flag before the time is right.

Shouldn't the set_permit_timeout_from_features method and
_permit_feature_timeout thread local be removed since the new flag is
the "right" way to look up and mutate the flag?

Having get_participation_extras return None if no appropriate
participation is found feels like a trap to me (i.e., when someone calls
it they are implicitly asserting that the extras are available). It
should raise an exception instead and there should be a
query_participation_extras function so we can be explicit about
tolerating missing extras.

If it survives, the XXX in get_participation_extras is required to have
a bug and person's name associated with it. For details see
https://dev.launchpad.net/PolicyandProcess/XXXPolicy.

It feels just a little weird that get_participation_extras returns a
participation. I can't think of a design that feels better that doesn't
add an inappropriate level of complexity. Here's the best I could do in
case it inspires something better:

    Add an IHasParticpationExtras interface that only has a get_extras
    method. Participations that support extras would also provide
    IHasParticpationExtras and get_extras would return an
    IParticipationExtras.

In fact, I think the data should really be attached to the interaction
instead of the participation. That would remove the need to search
through the participations to find one with extras and the need to
assert that no more than one have extras. Since the participation is
entirely about associating a principle with the interaction and the
extras aren't principal-dependent, the interaction seems like the right
place to put the extras.

Can we leave details of how this particular flag works to another merge proposal? This one is already confusing enough :-)

_permit_feature_timeout is not the thread local instance -- that instance is used for other things (that I'd also like to kill off, of course, but baby steps).

I agree in principle that stashing things on the interaction does seem cleaner in some ways. Maybe (sketching) something like this:

class IInteractionExtras(Interface):
    permit_timeout_from_features = Attribute(...)

class InteractionExtras(object):
    permit_timeout_from_features = False

class LaunchpadPermissiveSecurityPolicy(PermissiveSecurityPolicy):
    def __init__(self, *participations):
        PermissiveSecurityPolicy.__init__(self, *participations)
        self.extras = InteractionExtras()

(similar change to LaunchpadSecurityPolicy)

Then

def get_interaction_extras():
    # With some error checking, should fail if no interaction though
    return queryInteraction().extras

One part of this that feels, well, different to what I did is that it doesn't give a separate implementation for tests (LaunchpadTestRequest vs LaunchpadBrowserRequest). Maybe that's a good thing though.

What do you think?

Re "The permit_timeout_from_features flag may have been conceived to help in
checking the timeout feature flag, but it seems to me that it applies to
all feature flags. Perhaps its name should be
"permit_feature_flag_lookup" and the feature flag code should raise an
exception if asked for a flag before the time is right."

No - its truely just the bootstrap mechanism for the db timeout
feature - there is -no- intent that other feature lookups, if they
happen before the db timeout is found, should be blocked (and it would
be a bad, surprising, bug to block them).

-Rob

OK, I've made the changes I proposed and am kicking off a test in ec2. It's worked out to be a fairly small change, which is nice.

I probably should add a couple of docstrings but otherwise, please let me know what you think!

Robert: thanks for explaining that the flag really is just about the
timeout flag and not more general. I see now why that is.

> OK, I've made the changes I proposed and am kicking off a test in ec2.
> It's worked out to be a fairly small change, which is nice.
>
> I probably should add a couple of docstrings but otherwise, please let
> me know what you think!

I like it.

I'm still of the opinion that both get_ and query_ versions would be a
small win (the win being a more specific exception if you should have
handled the no-extras possibility but didn't), but you should feel free
to land as-is.