[Freeipa-devel] param type issues

[Rob Crittenden]

John Dennis wrote:
> 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

Params have always been rather primitive types so this has never come 
up. It seems like a lot of extra work to run every default through a 
validator but I don't think that's a show-stopper.

>
> 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.

We can probably just drop default from the things tracked by the API. It 
doesn't affect the data on the wire.

> 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.

Why would you want to? Param classes are by definition simple things: 
Int, Str, etc.

> 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.
>
>

makeapi was rather quick and dirty. dict ordering has always been the 
same (except when it isn't). In the year since we introduced makeapi I 
don't recall a case where dict ordering changed. I don't want to 
over-engineer things but since we already have dict comparison code in 
the test sutie we can probably leverage that. makeapi is just there to 
keep us honest. It doesn't have the same robustness requirements that 
other code has. In other words if it takes 15 minutes to properly 
compare the dicts then fine. If it is going to take a day then don't 
bother, the bang isn't worth the buck.

rob