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
--
Jan Cholasta
>From 0191725580268c280f02b3428f7de2effd7a4b02 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 | 22 ++++----------
3 files changed, 75 insertions(+), 34 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..69e3019 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -1667,13 +1667,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'))
@@ -1720,13 +1713,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 +1720,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
