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

3 files changed, 75 insertions, 34 deletions

diff --git a/ipalib/cli.py b/ipalib/cli.py
index ea320cf65..f72cca587 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 _
@@ -1152,7 +1152,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:
@@ -1160,7 +1160,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:
@@ -1172,7 +1172,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 47c72d1ab..f66596343 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(
@@ -648,17 +645,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)
 
@@ -753,6 +786,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:
@@ -762,9 +796,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 aea04b40d..24b8357a4 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -1694,13 +1694,6 @@ class dnszone_add(LDAPCreate):
         ),
     )
 
-    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_add, self).args_options_2_params(*args, **options)
-
     def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, **options):
         if not dns_container_exists(self.api.Backend.ldap2):
             raise errors.NotFound(reason=_('DNS is not configured'))
@@ -1747,13 +1740,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)
 
 
@@ -1761,8 +1747,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)