[PATCH] Problems when combining @paginate and @validate

[chrisz]

In TG 2.0, if you add a paginate decorator to a controller that has already a validate decorator, like this

    @validate(...)
    @paginate(...)

then the controller does not get the validated parameters any more, but gets only the raw parameters instead.

If you do it the other way around,

    @paginate(...)
    @validate(...)

then the validate will stumble over the additional page parameter.

The first problem can be solved with the proposed fix #2302 which makes the paginate decorator much cleaner and independent of the order of the decorators. However, this does not solve the second problem.

This second problem is caused by these two lines in tg.controller:

    if isinstance(controller.im_self, DecoratedController):
        params.update(pylons.request.params.mixed())

Even though the paginate decorator removes the page key from params, this update statement mixes it back in, and it is passed to the controller.

Do we really need these lines? What are they good for?

[chrisz]

This probably has been fixed in the  dispatch refactoring branch, but I haven't verified this yet.

[chrisz]

Just checked pagination with the dispatch refactoring branch (rev 831). I get the following error, not matter in which order I put the decorators:

GenerationException: url_for could not generate URL. Called with args: () {'page': 2}

[chrisz]

I noticed so far we did not have any pagination tests. I've now added at least a  basic test for paginate. If applied to the dispatch refactoring branch, it fails with the error mentioned above.

I'll later extend this test to check that pagination plays well with validation and the order does not matter.

[mramm]

Thanks chrisz. We've needed this test for some time now ;)

[percious]

This is now fixed in trunk and here:  http://bitbucket.org/percious/tg-21/

[chrisz]

Yes, it's working nice in  your branch (which also plays well with #2302), but I don't see where this has been fixed in the  trunk?

[percious]

Replying to chrisz:

Yes, it's working nice in  your branch (which also plays well with #2302), but I don't see where this has been fixed in the  trunk?

By trunk I mean:  http://svn.turbogears.org/trunk/ which is the 2.0 release. I have not yet merged my branch with the 2.1 trunk, but I will on Saturday at the latest.

cheers. -chris

[percious]

This is also now fixed in the 2.1 trunk, so I believe it is fixed everywhere... I'm closing the ticket.

[chrisz]

Replying to percious:

By trunk I mean:  http://svn.turbogears.org/trunk/ which is the 2.0 release. I have not yet merged my branch with the 2.1 trunk, but I will on Saturday at the latest.

Ok, now I found it: You actually meant  http://svn.turbogears.org/branches/2.0, not  http://svn.turbogears.org/trunk/.

[anthonyt]

There's a bug in the fix for 2.0.

Off by one error, where the keyword arguments are placed into the args array at the wrong indexes.

I've attached a patch that should fix the problem.

It's not as good a solution as the one in the 2.1 branch, but it fixed my problem.

[anthonyt]

fix off by one error

[anthonyt]

Oops! I just realized that the anthonyt.diff patch I uploaded should really have gone onto #2313.

There's another problem here, though.

Replying to chrisz:

This second problem is caused by these two lines in tg.controller:

if isinstance(controller.im_self, DecoratedController):
    params.update(pylons.request.params.mixed())

Even though the paginate decorator removes the page key from params, this update statement mixes it back in, and it is passed to the controller.

Do we really need these lines? What are they good for?

In response to the question "What are they good for?": Removing those lines prevents kwargs from GET/POST params from getting passed in to controller actions when using routes.

I refer to the comment that was right above the now deleted lines:

# this is here because the params were not getting passed in on controllers that
# were mapped with routes.  This is a fix, but it's in the wrong place.
# we need to add better tests to ensure decorated controllers with routings work
# properly.

In the current 2.0 code, the params are set once from pylons.request.params.mixed() in the ObjectDispatchController?. This means that any controller that avoids Object Dispatch would not have the parameters set. Those two lines were a hack, to re-set the params in the DecoratedController?, during validation.

Since, in tg2.0, all useful controllers extend DecoratedController?, I've moved the original params setting into the DecoratedController?'s _perform_call() method, avoiding the entire problem of setting it twice, and ensuring the params are set for Routed and for ObjectDispatched? requests.

This is less code and more straightforward. I'll post the patch right after this comment.

[anthonyt]

Simplify setting params in DecoratedController? and ObjectDispatch? controller. Avoids bug in re-setting params, and ensures params are set for routed & dispatched requests.

[chrisz]

Just for the record, this was fixed by percious in the 2.0 branch in r6567 by removing the two problematic lines, and this fix was ported to the TG 2.1 branch by me in r872:3efb. (I had claimed earlier that it already worked with TG 2.1 after the dispatch refactoring, but I can't reproduce that any more. I think the patch was still needed.)

As anthony pointed out, this fix may cause problems when using routes and it may be better to move the lines to the proper place (as in the suggested patch) instead of removing them completely. However, if this is the case, please provide a test for using TG2 with routes showing that the patch is needed and works correctly.