Ticket #2303 (closed defect: fixed)

Opened 1 year ago

Last modified 9 months ago

[PATCH] Problems when combining @paginate and @validate

Reported by: chrisz Assigned to: deets
Priority: normal Milestone: 2.0
Component: TurboGears Version: 2.0b7
Severity: normal Keywords: paginate validate
Cc:

Description

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?

Attachments

anthonyt.diff (0.6 kB) - added by anthonyt on 06/12/09 19:40:40.
fix off by one error
fix_r6566_t2303.diff (1.4 kB) - added by anthonyt on 06/15/09 22:08:36.
Simplify setting params in DecoratedController? and ObjectDispatch? controller. Avoids bug in re-setting params, and ensures params are set for routed & dispatched requests.

Change History

04/08/09 14:50:28 changed by chrisz

  • owner set to deets.

04/13/09 14:13:21 changed by chrisz

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

04/14/09 07:05:08 changed by 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}

04/14/09 15:10:56 changed by 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.

04/20/09 18:14:58 changed by mramm

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

04/23/09 17:16:10 changed by percious

  • status changed from new to closed.
  • resolution set to fixed.

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

(follow-up: ↓ 8 ) 04/24/09 02:36:48 changed by chrisz

  • status changed from closed to reopened.
  • resolution deleted.

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?

(in reply to: ↑ 7 ; follow-up: ↓ 10 ) 04/24/09 10:11:35 changed by 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

04/24/09 14:24:27 changed by percious

  • status changed from reopened to closed.
  • resolution set to fixed.

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

(in reply to: ↑ 8 ) 04/24/09 15:36:12 changed by 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/.

(follow-up: ↓ 14 ) 06/12/09 19:39:57 changed by anthonyt

  • status changed from closed to reopened.
  • resolution deleted.

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.

06/12/09 19:40:40 changed by anthonyt

  • attachment anthonyt.diff added.

fix off by one error

(in reply to: ↑ description ) 06/15/09 22:07:08 changed by 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.

06/15/09 22:08:36 changed by anthonyt

  • attachment fix_r6566_t2303.diff added.

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

06/15/09 22:17:17 changed by anthonyt

  • summary changed from Problems when combining @paginate and @validate to [PATCH] Problems when combining @paginate and @validate.

(in reply to: ↑ 11 ) 06/22/09 13:07:46 changed by chrisz

  • status changed from reopened to closed.
  • resolution set to fixed.

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.