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
--
Jan Cholasta
>From a34a87d1fd2f506e762eb7301d2c09e70677067f 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 ea320cf..f72cca5 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 47c72d1..f665963 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 2126f60..8cc4302 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)
--
1.7.7.6
>From cb27e925dc460a57428e9bdbf43a7e89f890a660 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 | 81 +++-------------------------------
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, 21 insertions(+), 116 deletions(-)
diff --git a/ipalib/parameters.py b/ipalib/parameters.py
index ec6020e..805f024 100644
--- a/ipalib/parameters.py
+++ b/ipalib/parameters.py
@@ -196,8 +196,10 @@ class DefaultFrom(ReadOnly):
CALLABLE_ERROR % ('callback', callback, type(callback))
)
self.callback = callback
+ fc = callback.func_code
+ if fc.co_flags & 0x0c:
+ raise ValueError("callback: variable-length argument list not allowed")
if len(keys) == 0:
- fc = callback.func_code
self.keys = fc.co_varnames[:fc.co_argcount]
else:
self.keys = keys
@@ -308,13 +310,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
@@ -379,7 +377,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),
@@ -481,20 +478,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(
@@ -990,59 +973,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 e69686c..2126f60 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -234,7 +234,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. The format follows RFC 1912 """
return int('%s01' % time.strftime('%Y%m%d'))
@@ -1554,7 +1554,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 83c33dd..fa39247 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_split_csv(self):
"""
Test the `ipalib.parameters.Param.split_csv` method with csv.
--
1.7.7.6
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
