Identity bug: Form field names with dots in the name don't work

I don't think this feature is ready for 1.0.4.

I tried to fix this ticket by moving the variable_decode functionality into the expose decorator. It turned out that this task is not so easy to fix. While the patch above will work with exposed functions that use keyword parameters, it will break apps using positional parameters due to code that is generated by decorate() in turbogears.decorator.

An example:

def checkform(foo, *args, **kwargs):
     print foo

args = []
params = {'foo-1': 1, 'foo-2': 2, 'foo-3': 3}
# checkform(*args, **params) will fail because there is no key 'foo' in params
converted_params = {'foo': [1, 2, 3]}
# checkform(*args, **converted_params) works

decorate() tries to retain the original signature. It generates a function on the fly that looks like this:

def checkform(self, foo, *_decorator__varargs, **_decorator__kwargs):
  return caller(func, self, foo, *_decorator__varargs, **_decorator__kwargs)

This function wrapper is called by CherryPy in cherrypy._cphttptools():

body = page_handler(*virtual_path, **self.params)

If positional parameters should work with variabledecode (as they do now), the parameters must be decoded before CherryPy calls the generated wrapper function. Another alternative would be to use other wrapper methods which do not reproduce the signature of the decorated function but just use *args and **kwargs but this seems to be a quite radical change.

I don't see how I can fix this before 2.0 when switching to Pylons.

Furthermore, the variable_decode functionality must be integrated into the validate() decorator because validation will fail if variable_decode was not run on the arguments before. But decorators are executed in the order of there placement in the user's code. Putting variable_decode in another decorator (e.g. expose() to minimize backward incompatibility - expose() is the only decorator which is guaranteed to be present on the called function) adds requirements on the decorators order and is therefore error prone. As the change impact should be quite small (only custom decorators may be affected), I think that validate() is the right place to put the variable_decode() thing.

Fixed in r6979 by encoding and decoding the request params in identity.visitor.IdentityVisitPlugin.identity_from_form.

Needs unit tests before this ticket can be closed.

Added unit test and ported to TG 1.5 in r7015.

Just noticed that the patch is actually not needed under TG 1.5 any more because there the params are not nested in the identity plugin any more. Also improved the patch for TG 1.1 by operating on the nested variables instead of flattening and renesting the params. Committed in r7016.

@chrisz: Did you actually measure the performance of r6979 vs. r7016? E.g. with http://chrisarndt.de/projects/wf-shootout/

I've run that benchmark now, using a simple controller and an url with a bunch of request params, but the margin of deviation between different runs are so high on my machine (up to 10%) that it's impossible to reliably measure any difference. In practice it probably won't matter anyway because templating and database acess takes so much more time. So maybe we should use the simple method again?

(Note that this affects only 1.1, it will be obsolete in TG 1.5 anyway.)

I find the pop_nested_value static method a bit complex and hard to understand (especially since it has no docstring), so, yes, I would vote for using the method from r6979 again (maybe you can measure before and after that revision too?). Less code, less complexity, better maintainable. But I'll leave the decision to you.

You're right, that pop_from_nested_dict method really adds too much complexity to the identity plugin. But on the other hand accessing only the necessary values without touching the rest is much more effective and elegant. When I measured identity_from_form in isolation with a small bunch of request params it was 7x faster using pop_from_nested_dict than using formencode to decode/encode the whole params dict. As I said, this is probably not measurable in practice for the whole stack, but anyway, I feel better with the more effective method.

So I have now moved the pop_from_nested_dict method into the util module, documented it and added a solid unit test. It might be handy in other situations as well.