[Freeipa-devel] [PATCH] 0004 permission-add gives confusing error when adding ACI to generated tree

[Martin Kosek]

On 10/16/2014 03:32 PM, thierry bordaz wrote:
> On 10/16/2014 12:45 PM, Martin Kosek wrote:
>> On 10/16/2014 12:08 PM, thierry bordaz wrote:
>>> On 10/15/2014 04:33 PM, Martin Kosek wrote:
>>>> On 10/15/2014 01:57 PM, thierry bordaz wrote:
>>>>> On 10/15/2014 01:26 PM, Martin Kosek wrote:
>>>>>> On 10/15/2014 01:08 PM, thierry bordaz wrote:
>>>>>>> https://fedorahosted.org/freeipa/ticket/4523
>>>>>> I see 2 issues with the patch:
>>>>>>
>>>>>> 1) Patch description should not contain "
>>>>>> Reviewed by:", this gets added later by a script (or human)
>>>>> ok
>>>>>> 2) The exception handling clause should be as focused as possible, i.e. not
>>>>>> including whole command, but rather just the failing call, i.e.:
>>>>>>
>>>>>>        def post_callback(self, ldap, dn, entry, *keys, **options):
>>>>>>            try:
>>>>>>                self.obj.add_aci(entry)
>>>>>>            except Exception:
>>>>>>
>>>>>> You can use
>>>>>>
>>>>>>        try:
>>>>>>            ...
>>>>>>        except errors.NotFound:
>>>>>>            self.obj.handle_not_found(*keys)
>>>>>>
>>>>>> to raise the right error.
>>>>>>
>>>>>> Martin
>>>>> Currently the exception is handled on the failure of
>>>>> baseldap.LDAPCreate.execute(). Do you recommend to make the fix inside
>>>>> baseldap.LDAPCreate.execute rather than at the 'permission_add.execute'
>>>>> level ?
>>>> No, not there. I thought that the exception happens in
>>>>
>>>>       def post_callback(self, ldap, dn, entry, *keys, **options):
>>>>           try:
>>>>               self.obj.add_aci(entry)
>>>>           except Exception:
>>>>              ...
>>>>
>>>>> Also using handle_not_found looks good, but it reports something like:
>>>>>
>>>>>      ipa permission-add user1 --right read --attrs cn --subtree
>>>>>      'cn=compat,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com'
>>>>>      ipa: ERROR: user1: permission not found
>>>>>
>>>>>
>>>>> If the entry 'user1' exists, it is not clear what was not found.
>>>>> Displaying the dn of the entry would help to know that we are updating an
>>>>> entry
>>>>> into the 'compat' tree.
>>>> Ah, sorry, I think I mislead you with this advise. You probably could use the
>>>> same except clause as already used:
>>>>
>>>>               except errors.NotFound:
>>>>                   raise errors.ValidationError(
>>>>                       name='ipapermlocation',
>>>>                       error=_('Entry %s does not exist') % location)
>>>>
>>>> Martin
>>> Thanks Martin for your review. Here is a second fix.
>> I am sorry, this is still not what I meant. You cannot wrap whole
>> permission-add command, wait for very general error and then report very
>> specific error. Try-except clauses need to be as close to the guarded problem
>> as possible.
>>
>> See PEP8 for example (http://legacy.python.org/dev/peps/pep-0008/):
>>
>> ..
>> Additionally, for all try/except clauses, limit the try clause to the absolute
>> minimum amount of code necessary. Again, this avoids masking bugs
>> ...
>>
>> To speed up the resolution of this patch, please see a proposed patch, I hope
>> it works for you.
>>
>> Martin
> Thanks Martin for  the patch and the explanations. I missed that the LDAPCreate
> triggered the exception when calling  permission_add.post_callback. Now I
> understand what you meant by catching the exception as close as possible.
> 
> Thierry
> 

I understand that as an ACK from you then :-)

Given the effort I had to develop the patch, I will push that original version
and add you as a reviewer.

Pushed to:
master: 061f7ff331531fa01801fb597feed924de6a2fd7
ipa-4-1: 0a54b1c948e5c25d971f5b0ef4bc079cbd605069

Thanks!
Martin