[Freeipa-devel] param type issues

[John Dennis]

I recently tried to add a new parameter class (i.e. derived from
Param), it did not go smoothly. I presume the problems I ran into were
coding oversights in our current base, not intentional limitations but
I like to confirm that.

1) Default values are not converted to the type of the parameter using
the stock default keyword. This seems like an omission. See the below
code snippets. get_default() is called and if there is a
default_from() function pointer it is invoked and it's return value is
passed to normalize() and convert(), this seems correct.

However if the default_from() function pointer is not provided the
default attribute is returned bypassing the normalization and
conversion steps, that seems incorrect. A default should be subject to
the same processing rules. One might argue that the default attribute
should already be normalized and converted because the programmer who
added the default should have done so. But that's not always true,
lets say you have a type which is most easily expressed as a string
for the purpose of specifying a default value, but is really converted
to another type when it's used. Here is a good example, DN's. A DN
parameter has the DN class as it's type, but you would like to specify
the default as a string, e.g. "ou=users". In the current scheme you're
forced to have this in your Param constructor:

DN_Param('binddn', default=DN('cn=directory manager'))

This is necessary because the type of a DN_Param is a DN
object. However if you do this then makeapi gets all confused, see
issue 2 below.

But if you allow the default value to be normalized and converted
everything just works nicely.

Here is another example, albeit contrived, but it serves to
illustrate.

Number('max', default="100")

This won't work either in the current code. Now you say "but you would
just write default=100". Yes that's true and why it's contrived, but
that only works because 100 happens to be a native Python type
(e.g. int). When defaults are either not native Python types or it's
easier to express the value differently (e.g. a duration specified as
the string "1day 2hours") the current scheme fails. It shouldn't be
necessary to supply a default_from() function pointer for these simple
cases just to get normalization and conversion, the default_from()
really makes most sense when you need to postpone generation of the
default value or it involves some computation.

The default value should be treated just as if the user had supplied
the value passing through all the same normization and conversion steps.

def __call__(self, value, **kw):
     """
     One stop shopping.
     """
     if value in NULLS:
         value = self.get_default(**kw)
     else:
         value = self.convert(self.normalize(value))
     if hasattr(self, 'env'):
         self.validate(value, self.env.context, supplied=self.name in kw)
     else:
         self.validate(value, supplied=self.name in kw)
     return value

def get_default(self, **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))
             except StandardError:
                 pass
     return self.default


2) makeapi cannot deal with defaults which are not Python primitives.

makeapi generates a string to describe a parameter, that includes the
default value. makeapi uses repr() to generate the string. That works
fine when defaults (or other items) are Python primitives but fails
for user defined types, the reason is because calling repr() on a user
defined type generates something like this:

ipalib.dn.DN object at 0x8c0128c

It's the type of the object followed by it's address in memory.

Thus makeapi adds this line to the API.txt file

option: DN_Param('binddn?', autofill=True, cli_name='bind_dn', 
default=<ipalib.dn.DN object at 0x8c0128c>)

Clearly the memory location of a default value is the wrong thing to
capture. (Recall from above you must specify the default as
DN('cn=directory manager') because conversions are not performed)

There are two solutions right off the top of my head

   a) Permit conversions from string values for defaults (issue 1),
      then the default remains a string.

   b) Use str() instead of repr() in makeapi (however we would lose the
      str vs. unicode marker, not sure if that's critical nor if that's
      actually how we shold be validating the type of the value
      anyway).

Perhaps both should be done.

FWIW the use of repr() in conjunction with a user defined type for a
default caused the makeapi --validate to fail (because it's trying to
compare memory locations). Tracking the cause of that failure was not
easy.

3) The class name cannot contain an underscore.

The find_name() function in makeapi uses a regexp that does not permit
an underscore in the name of the class. I presume this was an
oversight and not a requirement that class names do not contain
underscores.

4) makeapi is makeing assumptions about dict ordering.

makeapi when it generates a string calls repr() on the contents of a
dict. It uses that to compare to a previous run to see if they are
identical by doing a string comparision. That's not robust. There are
no guarantees concerning the ordering of keys in a dict, nor the
string values produced by repr(). If you want to compare dicts for
equality then you should compare dicts for equality. If you want to
use strings for comparison purposes you have to be a lot more careful
about how you generate that string representation.


-- 
John Dennis <jdennis at redhat.com>

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/