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 #2303 (closed defect: fixed)

Opened 10 years ago

Last modified 10 years ago

[PATCH] Problems when combining @paginate and @validate

Reported by: chrisz Owned by: 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 Download (609 bytes) - added by anthonyt 10 years ago.
fix off by one error
fix_r6566_t2303.diff Download (1.4 KB) - added by anthonyt 10 years ago.
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

comment:1 Changed 10 years ago by chrisz

  • Owner set to deets

comment:2 Changed 10 years ago by chrisz

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

comment:3 Changed 10 years ago 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}

comment:4 Changed 10 years ago 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.

comment:5 Changed 10 years ago by mramm

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

comment:6 Changed 10 years ago 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/

comment:7 follow-up: ↓ 8 Changed 10 years ago by chrisz

  • Status changed from closed to reopened
  • Resolution fixed 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?

comment:8 in reply to: ↑ 7 ; follow-up: ↓ 10 Changed 10 years ago 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

comment:9 Changed 10 years ago 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.

comment:10 in reply to: ↑ 8 Changed 10 years ago 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/.

comment:11 follow-up: ↓ 14 Changed 10 years ago by anthonyt

  • Status changed from closed to reopened
  • Resolution fixed 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.

Changed 10 years ago by anthonyt

fix off by one error

comment:12 in reply to: ↑ description Changed 10 years ago 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.

Changed 10 years ago by anthonyt

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

comment:13 Changed 10 years ago by anthonyt

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

comment:14 in reply to: ↑ 11 Changed 10 years ago 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.

Note: See TracTickets for help on using tickets.