[Freeipa-devel] [PATCHES] Improve framework parameter validation

[Jan Cholasta]

On 27.3.2012 17:41, Martin Kosek wrote:
> On Tue, 2012-03-27 at 16:42 +0200, Martin Kosek wrote:
>> On Tue, 2012-03-27 at 16:30 +0200, Jan Cholasta wrote:
>>> On 27.3.2012 16:00, Martin Kosek wrote:
>>>> On Thu, 2012-03-15 at 14:57 +0100, Jan Cholasta wrote:
>>>>> On 15.3.2012 14:20, Petr Viktorin wrote:
>>>>>> On 03/15/2012 12:05 PM, Jan Cholasta wrote:
>>>>>>> On 15.3.2012 11:36, Jan Cholasta wrote:
>>>>>>>> (this is a continuation of
>>>>>>>> <http://www.redhat.com/archives/freeipa-devel/2011-September/msg00327.html>)
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> the attached patches fix<https://fedorahosted.org/freeipa/ticket/1847>
>>>>>>>> and<https://fedorahosted.org/freeipa/ticket/2245>:
>>>>>>>>
>>>>>>>> [PATCH] Fix the procedure for getting default values of command
>>>>>>>> parameters.
>>>>>>>>
>>>>>>>> The parameters used in default_from of other parameters are now properly
>>>>>>>> validated before the default_from is called.
>>>>>>>>
>>>>>>>> [PATCH] Change parameters to use only default_from for dynamic default
>>>>>>>> values.
>>>>>>>>
>>>>>>>> Replace all occurences of create_default with equivalent default_from
>>>>>>>> and remove create_default from the framework. This is needed for proper
>>>>>>>> parameter validation, as there is no way to tell which parameters to
>>>>>>>> validate prior to calling create_default, because create_default does
>>>>>>>> not provide information about which parameters are used for generating
>>>>>>>> the default value.
>>>>>>>>
>>>>>>>> Honza
>>>>>>>>
>>>>>>>
>>>>>>> Forgot to remove one FIXME bit in dns.py. Update patch attached.
>>>>>>>
>>>>>>> Honza
>>>>>>
>>>>>>
>>>>>>    >   diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
>>>>>>    >   index a10960a..61c645d 100644
>>>>>>    >   --- a/ipalib/plugins/dns.py
>>>>>>    >   +++ b/ipalib/plugins/dns.py
>>>>>>    >   @@ -1528,7 +1528,7 @@ class dnszone(LDAPObject):
>>>>>>    >   label=_('SOA serial'),
>>>>>>    >   doc=_('SOA record serial number'),
>>>>>>    >   minvalue=1,
>>>>>>    >   - create_default=_create_zone_serial,
>>>>>>    >   + default_from=_create_zone_serial,
>>>>>>    >   autofill=True,
>>>>>>    >   ),
>>>>>>    >   Int('idnssoarefresh',
>>>>>>    >   diff --git a/ipalib/plugins/passwd.py b/ipalib/plugins/passwd.py
>>>>>>    >   index b26f7e9..9bee314 100644
>>>>>>    >   --- a/ipalib/plugins/passwd.py
>>>>>>    >   +++ b/ipalib/plugins/passwd.py
>>>>>>    >   @@ -69,7 +69,7 @@ class passwd(Command):
>>>>>>    >   label=_('User name'),
>>>>>>    >   primary_key=True,
>>>>>>    >   autofill=True,
>>>>>>    >   - create_default=lambda **kw: util.get_current_principal(),
>>>>>>    >   + default_from=lambda: util.get_current_principal(),
>>>>>>    >   normalizer=lambda value: normalize_principal(value),
>>>>>>    >   ),
>>>>>>    >   Password('password',
>>>>>>
>>>>>>
>>>>>> This is just a minor nitpick, but I'd like to know if there's a reason
>>>>>> behind it: why are you sometimes using lambda and sometimes not?
>>>>>
>>>>> I use lambda as a protective measure against accidents caused by adding
>>>>> optional arguments to the functions used. _create_zone_serial is an
>>>>> exception to that rule, because it is private to the dns plugin.
>>>>>
>>>>>>
>>>>>> The patch works well here, but I think I'm not the one to ack it.
>>>>>>
>>>>>
>>>>> Honza
>>>>>
>>>>
>>>> The patch looks OK, I found just minor issues.
>>>>
>>>> 1) We may want to add some check for wildcards (**kw) in default_from, I
>>>> guess it would mess with your dependency solver. Some nice error would
>>>> warn developers that they are doing something bad.
>>>
>>> Added the check.
>>>
>>>>
>>>> 2) Patch 47.4 needs minor rebasing
>>>
>>> Done.
>>>
>>>>
>>>> Martin
>>>>
>>>
>>> Updated patches attached.
>>>
>>> Honza
>>>
>>
>> I think you squashed the change with an incorrect commit. The check
>> should be rather included in patch 44.

I beg to differ, patch 44 changes the way parameter defaults are 
generated, which is not affected by this change at all. The check is 
there just to warn developers early if they use default_from in a wrong 
way (it wouldn't have worked even without the check). This could have 
happened in patch 47, if I had done a bad job replacing create_default 
with default_from, so the check belongs there (or into a separate patch).

>>
>> Martin
>
> I just noticed it breaks one unit test:
>
> ======================================================================
> ERROR: Test the `ipalib.parameters.DefaultFrom.__init__` method.
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>    File "/usr/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
>      self.test(*self.arg)
>    File "/home/mkosek/freeipa/tests/test_ipalib/test_parameters.py", line 52, in test_init
>      o = self.cls(callback, *keys)
>    File "/home/mkosek/freeipa/ipalib/parameters.py", line 201, in __init__
>      raise ValueError("callback: variable-length argument list not allowed")
> ValueError: callback: variable-length argument list not allowed
>
> ----------------------------------------------------------------------

Fixed.

>
> I also think it would be useful to have one test specifically for this
> new check.

Added.

>
> Martin
>

Updated patches attached.

Honza

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-44.6-parameter-validation.patch
Type: text/x-patch
Size: 10028 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120328/4f913533/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-47.6-parameter-remove-create-default.patch
Type: text/x-patch
Size: 14263 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120328/4f913533/attachment-0001.bin>