On 01.08.2011 17:44, Rob Crittenden wrote:
> Alexander Bokovoy wrote:
>> Hi,
>>
>> as result of discussion on Param and environment/context, here is patch
>> to fix
>>      https://fedorahosted.org/freeipa/ticket/1549
>>      https://fedorahosted.org/freeipa/ticket/1550
>>
>> CLI and Web UI work.
> 
> nack.
> 
> When using ipalib outside of the cli the wrong attribute is in the error
> message. See the test program in
> https://fedorahosted.org/freeipa/attachment/ticket/187
> 
> In this sample the error when context='cli' should be 'desc' otherwise
> it should be 'description'. With your patch it is always 'desc'.
Following yesterday's discussion on IRC with Rob, I further investigated
the issue and came up with a following fix (attached).

The patch extends arguments supported by Param class to accept
environment and set it if it is not None before locking down the class.

It further extends create_param() helper function to re-create params
from a passed spec if provided environment is not None.

Command and Object now use create_param() to inject their environment
into specified arguments, parameters, and options.

There are might be more cases where using create_param(spec,
env=self.env) would be needed as I have found that, for example, running

ipa user-add

and not passing first name (--first) would give you

$ ./ipa user-add
First name:
ipa: ERROR: 'givenname' is required

while actual error should be

ipa: ERROR: 'first' is required

This is solved by the attached patch by introducing create_param() call
into Object while fixes in Command will give you protection for
client-side commands as well.

If approach is fine, I can find and fix other places.
-- 
/ Alexander Bokovoy
diff --git a/ipalib/frontend.py b/ipalib/frontend.py
index 3534310..eb5f2b3 100644
--- a/ipalib/frontend.py
+++ b/ipalib/frontend.py
@@ -807,7 +807,7 @@ class Command(HasParam):
         `ipalib.crud.Create` subclass.
         """
         for arg in self._get_param_iterable('args'):
-            yield arg
+            yield create_param(arg, self.env)
 
     def check_args(self, args):
         """
@@ -846,7 +846,7 @@ class Command(HasParam):
         `ipalib.crud.Create` subclass.
         """
         for option in self._get_param_iterable('options'):
-            yield option
+            yield create_param(option, self.env)
         for o in self.has_output:
             if isinstance(o, (Entry, ListOfEntries)):
                 yield Flag('all',
@@ -1098,10 +1098,10 @@ class Object(HasParam):
             else:
                 assert isinstance(spec, Param)
                 key = spec.name
+            p = spec
             if key in props:
-                yield props.pop(key).param
-            else:
-                yield create_param(spec)
+                p = props.pop(key).param
+            yield create_param(p, self.env)
         def get_key(p):
             if p.param.required:
                 if p.param.default_from is None:
diff --git a/ipalib/parameters.py b/ipalib/parameters.py
index e1c0b09..0e89677 100644
--- a/ipalib/parameters.py
+++ b/ipalib/parameters.py
@@ -110,7 +110,7 @@ from constants import NULLS, TYPE_ERROR, CALLABLE_ERROR
 from text import Gettext, FixMe
 import csv
 from xmlrpclib import MAXINT, MININT
-
+from config import Env
 
 class DefaultFrom(ReadOnly):
     """
@@ -317,6 +317,7 @@ class Param(ReadOnly):
         ('flags', frozenset, frozenset()),
         ('hint', (str, Gettext), None),
         ('alwaysask', bool, False),
+        ('env', (NoneType, Env), None),
 
         # The 'default' kwarg gets appended in Param.__init__():
         # ('default', self.type, None),
@@ -399,7 +400,10 @@ class Param(ReadOnly):
                 raise ValueError('kwarg %r conflicts with attribute on %s' % (
                     key, self.__class__.__name__)
                 )
-            setattr(self, key, value)
+            # Special handling of env: if environment is not specified, 
+            # omit creating the corresponding attribute.
+            if not (key == 'env' and value is None):
+                setattr(self, key, value)
             rule_name = '_rule_%s' % key
             if value is not None and hasattr(self, rule_name):
                 class_rules.append(getattr(self, rule_name))
@@ -1680,7 +1684,7 @@ class AccessTime(Str):
         return None
 
 
-def create_param(spec):
+def create_param(spec, env=None):
     """
     Create an `Str` instance from the shorthand ``spec``.
 
@@ -1698,13 +1702,16 @@ def create_param(spec):
     >>> (s.name, s.required, s.multivalue)
     ('hometown', False, False)
 
-    On the other hand, if ``spec`` is already a `Param` instance, it is
+    On the other hand, if ``spec`` is already a `Param` instance and ``env`` 
is None, it is
     returned unchanged.  For example:
 
     >>> b = Bytes('cert')
     >>> create_param(b) is b
     True
 
+    If ``env`` is specified and not None, this function will return a copy
+    of ``spec`` with specified environment.
+
     As a plugin author, you will not call this function directly (which would
     be no more convenient than simply creating the `Str` instance).  Instead,
     `frontend.Command` will call it for you when it evaluates the
@@ -1712,11 +1719,20 @@ def create_param(spec):
     will call it for you when it evaluates the ``takes_params`` attribute.
 
     :param spec: A spec string or a `Param` instance.
+    :param env:  An environment to re-create `Param` instance in.
     """
     if isinstance(spec, Param):
-        return spec
+        if env is None:
+            return spec
+        # otherwise clone spec and add env
+        kw = {}
+        for (key, kind, default) in spec.kwargs:
+            if hasattr(spec, key):
+                kw[key] = getattr(spec, key)
+        kw['env'] = env
+        return spec.__class__(spec.name,**kw)
     if type(spec) is not str:
         raise TypeError(
             TYPE_ERROR % ('spec', (str, Param), spec, type(spec))
         )
-    return Str(spec)
+    return Str(spec, env=env)
diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index 1ff7a2a..20f4b39 100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.py
@@ -385,8 +385,8 @@ class LDAPObject(Object):
             if parent_obj.primary_key:
                 pkey = parent_obj.primary_key
                 yield pkey.__class__(
-                    parent_obj.name + pkey.name, required=True, query=True,
-                    cli_name=parent_obj.name, label=pkey.label
+                    "%s_%s" % (parent_obj.name, pkey.name), required=True, 
query=True,
+                    cli_name=parent_obj.name, label=pkey.label, env=self.env
                 )
 
     def has_objectclass(self, classes, objectclass):
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to