Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #1587 (closed defect: fixed)

Opened 10 years ago

Last modified 7 years ago

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

Reported by: streawkceur Owned by: Chris Arndt
Priority: normal Milestone: 1.1.1
Component: Identity Version: 1.0.4b1
Severity: normal Keywords: needs tests, identity, nested variables filter
Cc: Felix.Schwarz

Description

As ToscaWidgets concatenates the named of nested form widgets by dots, the request looks like:

/login?login.user=user&login.pass=pass&login.submit=Login

Unfortunately (at least in this case), TurboGears seems to use formencode.variabledecode.variable_decode to create a nested dict out of the request params. The params therefore are:

{'login': {'user': 'user', 'pass': 'pass', 'submit': 'Login'}}

But Identity cannot handle nested request params. See turbogears.identity.identity_from_form:

submit= params.pop(self.submit_button_name, None)

That, of course, won't work when self.submit_button_name is "login.submit", because no such key exists. Instead the value is found in params['login']['submit'].

A simple fix would be to use this as the first line in identity_from_form:

params = formencode.variabledecode.variable_encode(cherrypy.request.params)

I don't think that it should have negative side effects. What do you think?

Felix Schwarz suggested to move the NestedVariablesFilter into the expose function, with NestedVariablesFilter=True by default to preserve the current behavior.

Attachments

naive_implementation.patch Download (3.6 KB) - added by Felix.Schwarz 10 years ago.

Change History

comment:1 Changed 10 years ago by Felix.Schwarz

  • Cc Felix.Schwarz added
  • Milestone changed from 1.0.4 to __unclassified__

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

Changed 10 years ago by Felix.Schwarz

comment:2 Changed 10 years ago by Felix.Schwarz

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.

comment:3 Changed 10 years ago by Felix.Schwarz

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.

comment:4 Changed 8 years ago by jorge.vargas

  • Milestone changed from __unclassified__ to 1.x

comment:5 Changed 8 years ago by Chris Arndt

  • Owner changed from anonymous to Chris Arndt
  • Keywords needs patch, added
  • Status changed from new to assigned
  • Milestone changed from 1.x to 1.1.x bugfix

comment:6 Changed 8 years ago by Chris Arndt

  • Keywords tests, added; patch, removed
  • Milestone changed from 1.1.x bugfix to 1.1.1

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.

comment:7 Changed 7 years ago by chrisz

  • Status changed from assigned to closed
  • Resolution set to fixed

Added unit test and ported to TG 1.5 in r7015.

comment:8 Changed 7 years ago by chrisz

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.

comment:9 Changed 7 years ago by Chris Arndt

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

comment:10 Changed 7 years ago by chrisz

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

comment:11 Changed 7 years ago by Chris Arndt

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.

comment:12 Changed 7 years ago by chrisz

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.

Note: See TracTickets for help on using tickets.