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

[Martin Kosek]

On Wed, 2012-03-28 at 10:39 +0200, Jan Cholasta wrote:
> 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).

Ok, it makes a sense as well.

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

ACK. Pushed to master, ipa-2-2.

Martin