[Freeipa-devel] [PATCHES] 198-202 Refactor indirect membership processing

[Petr Viktorin]

I hid Send by mistake; continuing review:


On 11/08/2013 03:14 PM, Petr Viktorin wrote:
> On 10/31/2013 02:45 PM, Jan Cholasta wrote:
>> Hi,
>>
>> the attached patches fix <https://fedorahosted.org/freeipa/ticket/3971>.
>>
>> Tested with 25000 users.
>>
>> Honza
>
> Patch 198:
>
> Also update ipaldap's find_entries docstring, it no longer uses IPA
> defaults.
>
>
> While you're touching this part of code, I had some other improvements
> in mind -- you can consider them:
>
> In find_entries,
>      attrs_list = [a.lower() for a in attrs_list]
> to make sure 'memberindirect' is case insensitive

> In get_memberof, construct `indirect` as a set, for Ο(1) remove().
^ ignore that, it's nuked in 201 \o/

> Changing MEMBERS_ALL et.al. from numbers to descriptive strings, for
> easier debugging.
^ these can be removed entirely in 201


>
> Patch 199: Looks great
>
>
> Patch 200:
>
> objtype, res_list, red_id, res_ctrls = result
> Minor typo ----------^
>
>
> This construction won't work as you'd expect in Python 2:
>
> try:
>      (possibly raise interesting exception) (*)
> except:
>      try:
>          (possibly raise exception to ignore) (**)
>      except:
>          pass
>      raise  # (***)
>
> The problem is that the exception in (**) overwrites the "current active
> exception" raised in (*). In (***) the exception from the cleanup will be
> re-raised.
> The solution is to store the wanted exception info, including the
> traceback:
>      exc_type, exc_value, exc_traceback = sys.exc_info()
> and then re-raise explicitly:
>      raise exc_type, exc_value, exc_traceback
>
> Also, please log the ignored exception from cancelling the paged search.
>
>


Patch 201:
Great patch!
A nitpick, I'd rename _process_member{,of} to _process_member{,of}indirect


Patch 202: Looks good
While we're on the subject: Each Plugin has an "api" attribute. It would 
be nice if we started preferring `self.api` instead of the global 
singleton wherever possible, even though they're currently always the same.


-- 
Petr³