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

[Martin Kosek]

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.
> 
> 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

----------------------------------------------------------------------

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

Martin