(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

--
Jan Cholasta
>From c5e1f63ea3bcb15fce5c90aafe700a23565b2213 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Mon, 16 Jan 2012 09:21:50 -0500
Subject: [PATCH 2/2] 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.

ticket 1847
---
 ipalib/cli.py         |    8 ++--
 ipalib/frontend.py    |   79 ++++++++++++++++++++++++++++++++++++++++---------
 ipalib/plugins/dns.py |   15 ++++-----
 3 files changed, 75 insertions(+), 27 deletions(-)

diff --git a/ipalib/cli.py b/ipalib/cli.py
index 7af6376..b955082 100644
--- a/ipalib/cli.py
+++ b/ipalib/cli.py
@@ -48,7 +48,7 @@ import plugable
 import util
 from errors import (PublicError, CommandError, HelpError, InternalError,
         NoSuchNamespaceError, ValidationError, NotFound, NotConfiguredError,
-        PromptFailed)
+        PromptFailed, ConversionError)
 from constants import CLI_TAB
 from parameters import Password, Bytes, File, Str, StrEnum
 from text import _
@@ -1151,7 +1151,7 @@ class cli(backend.Executioner):
             if (param.required and param.name not in kw) or \
                 (param.alwaysask and honor_alwaysask) or self.env.prompt_all:
                 if param.autofill:
-                    kw[param.name] = param.get_default(**kw)
+                    kw[param.name] = cmd.get_default_of(param.name, **kw)
                 if param.name in kw and kw[param.name] is not None:
                     continue
                 if param.password:
@@ -1159,7 +1159,7 @@ class cli(backend.Executioner):
                         param.label, param.confirm
                     )
                 else:
-                    default = param.get_default(**kw)
+                    default = cmd.get_default_of(param.name, **kw)
                     error = None
                     while True:
                         if error is not None:
@@ -1171,7 +1171,7 @@ class cli(backend.Executioner):
                             if value is not None:
                                 kw[param.name] = value
                             break
-                        except ValidationError, e:
+                        except (ValidationError, ConversionError), e:
                             error = e.error
             elif param.password and kw.get(param.name, False) is True:
                 kw[param.name] = self.Backend.textui.prompt_password(
diff --git a/ipalib/frontend.py b/ipalib/frontend.py
index da25b4c..6d7a696 100644
--- a/ipalib/frontend.py
+++ b/ipalib/frontend.py
@@ -396,6 +396,7 @@ class Command(HasParam):
     args = Plugin.finalize_attr('args')
     options = Plugin.finalize_attr('options')
     params = Plugin.finalize_attr('params')
+    params_by_default = Plugin.finalize_attr('params_by_default')
     obj = None
 
     use_output_validation = True
@@ -419,11 +420,7 @@ class Command(HasParam):
         self.debug(
             'raw: %s(%s)', self.name, ', '.join(self._repr_iter(**params))
         )
-        while True:
-            default = self.get_default(**params)
-            if len(default) == 0:
-                break
-            params.update(default)
+        params.update(self.get_default(**params))
         params = self.normalize(**params)
         params = self.convert(**params)
         self.debug(
@@ -628,17 +625,53 @@ class Command(HasParam):
         >>> c.get_default(color=u'Yellow')
         {}
         """
-        return dict(self.__get_default_iter(kw))
+        params = [p.name for p in self.params() if p.name not in kw and (p.required or p.autofill)]
+        return dict(self.__get_default_iter(params, kw))
 
-    def __get_default_iter(self, kw):
+    def get_default_of(self, name, **kw):
         """
-        Generator method used by `Command.get_default`.
+        Return default value for parameter `name`.
         """
-        for param in self.params():
-            if param.name in kw:
-                continue
-            if param.required or param.autofill:
-                default = param.get_default(**kw)
+        default = dict(self.__get_default_iter([name], kw))
+        return default.get(name)
+
+    def __get_default_iter(self, params, kw):
+        """
+        Generator method used by `Command.get_default` and `Command.get_default_of`.
+        """
+        # Find out what additional parameters are needed to dynamically create
+        # the default values with default_from.
+        dep = set()
+        for param in reversed(self.params_by_default):
+            if param.name in params or param.name in dep:
+                if param.default_from is None:
+                    continue
+                for name in param.default_from.keys:
+                    dep.add(name)
+
+        for param in self.params_by_default():
+            default = None
+            hasdefault = False
+            if param.name in dep:
+                if param.name in kw:
+                    # Parameter is specified, convert and validate the value.
+                    kw[param.name] = param(kw[param.name], **kw)
+                else:
+                    # Parameter is not specified, use default value. Convert
+                    # and validate the value, it might not be returned so
+                    # there's no guarantee it will be converted and validated
+                    # later.
+                    default = param(None, **kw)
+                    if default is not None:
+                        kw[param.name] = default
+                    hasdefault = True
+            if param.name in params:
+                if not hasdefault:
+                    # Default value is not available from the previous step,
+                    # get it now. At this point it is certain that the value
+                    # will be returned, so let the caller care about conversion
+                    # and validation.
+                    default = param.get_default(**kw)
                 if default is not None:
                     yield (param.name, default)
 
@@ -733,6 +766,7 @@ class Command(HasParam):
         else:
             self.max_args = None
         self._create_param_namespace('options')
+        params_nosort = tuple(self.args()) + tuple(self.options()) #pylint: disable=E1102
         def get_key(p):
             if p.required:
                 if p.sortorder < 0:
@@ -742,9 +776,26 @@ class Command(HasParam):
                 return 1
             return 2
         self.params = NameSpace(
-            sorted(tuple(self.args()) + tuple(self.options()), key=get_key),    #pylint: disable=E1102
+            sorted(params_nosort, key=get_key),
             sort=False
         )
+        # Sort params so that the ones with default_from come after the ones
+        # that the default_from might depend on and save the result in
+        # params_by_default namespace.
+        params = []
+        for i in params_nosort:
+            pos = len(params)
+            for j in params_nosort:
+                if j.default_from is None:
+                    continue
+                if i.name not in j.default_from.keys:
+                    continue
+                try:
+                    pos = min(pos, params.index(j))
+                except ValueError:
+                    pass
+            params.insert(pos, i)
+        self.params_by_default = NameSpace(params, sort=False)
         self.output = NameSpace(self._iter_output(), sort=False)
         self._create_param_namespace('output_params')
         super(Command, self)._on_finalize()
diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index 61c645d..61ce56e 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -1720,13 +1720,6 @@ api.register(dnszone_del)
 class dnszone_mod(LDAPUpdate):
     __doc__ = _('Modify DNS zone (SOA record).')
 
-    def args_options_2_params(self, *args, **options):
-        # FIXME: Check if name_from_ip is valid. The framework should do that,
-        #        but it does not. Before it's fixed, this should suffice.
-        if 'name_from_ip' in options:
-            self.obj.params['name_from_ip'](unicode(options['name_from_ip']))
-        return super(dnszone_mod, self).args_options_2_params(*args, **options)
-
 api.register(dnszone_mod)
 
 
@@ -1734,8 +1727,12 @@ class dnszone_find(LDAPSearch):
     __doc__ = _('Search for DNS zones (SOA records).')
 
     def args_options_2_params(self, *args, **options):
-        # FIXME: Check if name_from_ip is valid. The framework should do that,
-        #        but it does not. Before it's fixed, this should suffice.
+        # FIXME: Check that name_from_ip is valid. This is necessary because
+        #        custom validation rules, including _validate_ipnet, are not
+        #        used when doing a search. Once we have a parameter type for
+        #        IP network objects, this will no longer be necessary, as the
+        #        parameter type will handle the validation itself (see
+        #        <https://fedorahosted.org/freeipa/ticket/2266>).
         if 'name_from_ip' in options:
             self.obj.params['name_from_ip'](unicode(options['name_from_ip']))
         return super(dnszone_find, self).args_options_2_params(*args, **options)
-- 
1.7.7.6

>From 31e397a862b319874e5b52935d177a07fc08f9a3 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Thu, 15 Mar 2012 04:32:37 -0400
Subject: [PATCH 1/2] 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.
---
 ipalib/parameters.py                 |   77 ++--------------------------------
 ipalib/plugins/dns.py                |    4 +-
 ipalib/plugins/passwd.py             |    2 +-
 ipaserver/plugins/join.py            |    4 +-
 make-lint                            |    6 +-
 makeapi                              |    1 -
 tests/test_ipalib/test_parameters.py |   39 +++--------------
 7 files changed, 18 insertions(+), 115 deletions(-)

diff --git a/ipalib/parameters.py b/ipalib/parameters.py
index 48155da..f988438 100644
--- a/ipalib/parameters.py
+++ b/ipalib/parameters.py
@@ -308,13 +308,9 @@ class Param(ReadOnly):
         encoder
       - default_from: a custom function for generating default values of
         parameter instance
-      - create_default: a custom function for generating default values of
-        parameter instance. Unlike default_from attribute, this function
-        is not wrapped. `Param.get_default()` documentation provides further
-        details
       - autofill: by default, only `required` parameters get a default value
-        from default_from or create_default functions. When autofill is
-        enabled, optional attributes get the default value filled too
+        from the default_from function. When autofill is enabled, optional
+        attributes get the default value filled too
       - query: this attribute is controlled by framework. When the `query`
         is enabled, framework assumes that the value is only queried and not
         inserted in the LDAP. Validation is then relaxed - custom
@@ -374,7 +370,6 @@ class Param(ReadOnly):
         ('normalizer', callable, None),
         ('encoder', callable, None),
         ('default_from', DefaultFrom, None),
-        ('create_default', callable, None),
         ('autofill', bool, False),
         ('query', bool, False),
         ('attribute', bool, False),
@@ -476,20 +471,6 @@ class Param(ReadOnly):
                 class_rules.append(getattr(self, rule_name))
         check_name(self.cli_name)
 
-        # Check that only default_from or create_default was provided:
-        assert not hasattr(self, '_get_default'), self.nice
-        if callable(self.default_from):
-            if callable(self.create_default):
-                raise ValueError(
-                    '%s: cannot have both %r and %r' % (
-                        self.nice, 'default_from', 'create_default')
-                )
-            self._get_default = self.default_from
-        elif callable(self.create_default):
-            self._get_default = self.create_default
-        else:
-            self._get_default = None
-
         # Check that only 'include' or 'exclude' was provided:
         if None not in (self.include, self.exclude):
             raise ValueError(
@@ -951,59 +932,9 @@ class Param(ReadOnly):
         >>> kw = dict(first=u'John', department=u'Engineering')
         >>> login.get_default(**kw)
         u'my-static-login-default'
-
-        The second, less common way to construct a dynamic default is to provide
-        a callback via the ``create_default`` keyword argument.  Unlike a
-        ``default_from`` callback, your ``create_default`` callback will not get
-        wrapped in any dispatcher.  Instead, it will be called directly, which
-        means your callback must accept arbitrary keyword arguments, although
-        whether your callback utilises these values is up to your
-        implementation.  For example:
-
-        >>> def make_csr(**kw):
-        ...     print '  make_csr(%r)' % (kw,)  # Note output below
-        ...     return 'Certificate Signing Request'
-        ...
-        >>> csr = Bytes('csr', create_default=make_csr)
-
-        Your ``create_default`` callback will be called with whatever keyword
-        arguments are passed to `Param.get_default()`.  For example:
-
-        >>> kw = dict(arbitrary='Keyword', arguments='Here')
-        >>> csr.get_default(**kw)
-          make_csr({'arguments': 'Here', 'arbitrary': 'Keyword'})
-        'Certificate Signing Request'
-
-        And your ``create_default`` callback is called even if
-        `Param.get_default()` is called with *zero* keyword arguments.
-        For example:
-
-        >>> csr.get_default()
-          make_csr({})
-        'Certificate Signing Request'
-
-        The ``create_default`` callback will most likely be used as a
-        pre-execute hook to perform some special client-side operation.  For
-        example, the ``csr`` parameter above might make a call to
-        ``/usr/bin/openssl``.  However, often a ``create_default`` callback
-        could also be implemented as a ``default_from`` callback.  When this is
-        the case, a ``default_from`` callback should be used as they are more
-        structured and therefore less error-prone.
-
-        The ``default_from`` and ``create_default`` keyword arguments are
-        mutually exclusive.  If you provide both, a ``ValueError`` will be
-        raised.  For example:
-
-        >>> homedir = Str('home',
-        ...     default_from=lambda login: '/home/%s' % login,
-        ...     create_default=lambda **kw: '/lets/use/this',
-        ... )
-        Traceback (most recent call last):
-          ...
-        ValueError: Str('home'): cannot have both 'default_from' and 'create_default'
         """
-        if self._get_default is not None:
-            default = self._get_default(**kw)
+        if self.default_from is not None:
+            default = self.default_from(**kw)
             if default is not None:
                 try:
                     return self.convert(self.normalize(default))
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
@@ -226,7 +226,7 @@ def _rname_validator(ugettext, zonemgr):
         return unicode(e)
     return None
 
-def _create_zone_serial(**kwargs):
+def _create_zone_serial():
     """Generate serial number for zones."""
     return int('%s01' % time.strftime('%Y%d%m'))
 
@@ -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',
diff --git a/ipaserver/plugins/join.py b/ipaserver/plugins/join.py
index 81c336b..0073363 100644
--- a/ipaserver/plugins/join.py
+++ b/ipaserver/plugins/join.py
@@ -52,7 +52,7 @@ class join(Command):
             validate_host,
             cli_name='hostname',
             doc=_("The hostname to register as"),
-            create_default=lambda **kw: unicode(util.get_fqdn()),
+            default_from=lambda: unicode(util.get_fqdn()),
             autofill=True,
             #normalizer=lamda value: value.lower(),
         ),
@@ -60,7 +60,7 @@ class join(Command):
     takes_options= (
         Str('realm',
             doc=_("The IPA realm"),
-            create_default=lambda **kw: get_realm(),
+            default_from=lambda: get_realm(),
             autofill=True,
         ),
         Str('nshardwareplatform?',
diff --git a/make-lint b/make-lint
index 6b54312..7ecd59d 100755
--- a/make-lint
+++ b/make-lint
@@ -56,9 +56,9 @@ class IPATypeChecker(TypeChecker):
         'ipalib.plugins.misc.env': ['env'],
         'ipalib.parameters.Param': ['cli_name', 'cli_short_name', 'label',
             'doc', 'required', 'multivalue', 'primary_key', 'normalizer',
-            'default', 'default_from', 'create_default', 'autofill', 'query',
-            'attribute', 'include', 'exclude', 'flags', 'hint', 'alwaysask',
-            'sortorder', 'csv', 'csv_separator', 'csv_skipspace'],
+            'default', 'default_from', 'autofill', 'query', 'attribute',
+            'include', 'exclude', 'flags', 'hint', 'alwaysask', 'sortorder',
+            'csv', 'csv_separator', 'csv_skipspace'],
         'ipalib.parameters.Bool': ['truths', 'falsehoods'],
         'ipalib.parameters.Int': ['minvalue', 'maxvalue'],
         'ipalib.parameters.Decimal': ['minvalue', 'maxvalue', 'precision'],
diff --git a/makeapi b/makeapi
index 007531a..b9c149e 100755
--- a/makeapi
+++ b/makeapi
@@ -45,7 +45,6 @@ PARAM_IGNORED_KW_ATTRIBUTES = ('label',
                                'normalizer',
                                'encoder',
                                'default_from',
-                               'create_default',
                                'hint',
                                'flags',
                                'sortorder',)
diff --git a/tests/test_ipalib/test_parameters.py b/tests/test_ipalib/test_parameters.py
index ad8d840..3580588 100644
--- a/tests/test_ipalib/test_parameters.py
+++ b/tests/test_ipalib/test_parameters.py
@@ -185,8 +185,6 @@ class test_Param(ClassChecker):
         assert o.normalizer is None
         assert o.default is None
         assert o.default_from is None
-        assert o.create_default is None
-        assert o._get_default is None
         assert o.autofill is False
         assert o.query is False
         assert o.attribute is False
@@ -250,16 +248,6 @@ class test_Param(ClassChecker):
         assert str(e) == \
             "Param('my_param'): takes no such kwargs: 'ape', 'great'"
 
-        # Test that ValueError is raised if you provide both default_from and
-        # create_default:
-        e = raises(ValueError, self.cls, 'my_param',
-            default_from=lambda first, last: first[0] + last,
-            create_default=lambda **kw: 'The Default'
-        )
-        assert str(e) == '%s: cannot have both %r and %r' % (
-            "Param('my_param')", 'default_from', 'create_default',
-        )
-
         # Test that ValueError is raised if you provide both include and
         # exclude:
         e = raises(ValueError, self.cls, 'my_param',
@@ -276,15 +264,11 @@ class test_Param(ClassChecker):
         e = raises(ValueError, self.cls, 'my_param', csv=True)
         assert str(e) == '%s: cannot have csv without multivalue' % "Param('my_param')"
 
-        # Test that _get_default gets set:
-        call1 = lambda first, last: first[0] + last
-        call2 = lambda **kw: 'The Default'
-        o = self.cls('my_param', default_from=call1)
-        assert o.default_from.callback is call1
-        assert o._get_default is o.default_from
-        o = self.cls('my_param', create_default=call2)
-        assert o.create_default is call2
-        assert o._get_default is call2
+        # Test that default_from gets set:
+        call = lambda first, last: first[0] + last
+        o = self.cls('my_param', default_from=call)
+        assert type(o.default_from) is parameters.DefaultFrom
+        assert o.default_from.callback is call
 
     def test_repr(self):
         """
@@ -579,7 +563,7 @@ class test_Param(ClassChecker):
 
     def test_get_default(self):
         """
-        Test the `ipalib.parameters.Param._get_default` method.
+        Test the `ipalib.parameters.Param.get_default` method.
         """
         class PassThrough(object):
             value = None
@@ -624,17 +608,6 @@ class test_Param(ClassChecker):
         assert o._convert_scalar.value is default
         assert o.normalizer.value is default
 
-        # Test with create_default:
-        o = Str('my_str',
-            normalizer=PassThrough(),
-            default=u'Static Default',
-            create_default=lambda **kw: u'The created default',
-        )
-        default = o.get_default(first=u'john', last='doe')
-        assert_equal(default, u'The created default')
-        assert o._convert_scalar.value is default
-        assert o.normalizer.value is default
-
     def test_csv_normalize(self):
         """
         Test the `ipalib.parameters.Param.normalize` method with csv.
-- 
1.7.7.6

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to