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

Opened 10 years ago

Last modified 10 years ago

@paginate API breakage in 1.0.8

Reported by: lmacken Owned by:
Priority: normal Milestone:
Component: TurboGears Version: 1.0.8
Severity: major Keywords: Patch
Cc: toshio

Description

With TG 1.0.4.4, my application runs great with this use of the paginate decorator:

    @paginate('updates', limit=20, allow_limit_override=True)

However, with TG 1.0.8, this behavior has changed, and the above example completely ignores the tg_paginate_limit argument. This caused some serious problems in our production environment, as *huge* datasets were getting returned without being properly paginated.

To get it to obey the tg_paginate_limit required adding a max_limit argument to the @paginate decorator, like so:

    @paginate('updates', limit=20, max_limit=50, allow_limit_override=True)

This sudden change in behaviour will probably cause problems for many. TG 1.0.8 throws the following deprecation warning

<string>:3: DeprecationWarning: allow_limit_override is deprecated. Use max_limit to specify an upper bound for limit.

Yet, specifying max_limit without allow_limit_override does not seem to allow for limit overrides...

Attachments

TurboGears-1.0.8-paginate.patch Download (1.5 KB) - added by toshio 10 years ago.
Better patch to handle allow_limit_override and max_limit

Change History

comment:1 Changed 10 years ago by toshio

I'm seeing a couple things in the code that look like they contribute to this problem. I've got a prelimininary patch. I'll paste it here before attaching so I can explain what I think is going on :-)

     @param max_limit: The maximum number to which the imposed limit
-    can be increased using the dynamic_limit keyword argument in the URL.
+    can be increased using the tp_paginate_limit keyword argument in the URL.
     If this is set to 0, no dynamic change at all will be allowed;
     if it is set to None, any change will be allowed.
     @type max_limit: int

This hunk is just documentation. AFAICS, there's no dynamic_limit keyword argument being parsed in the code. This is a separate issue from the other, code, changes.

@@ -150,9 +150,14 @@ def paginate(var_name, default_order='',
                         redirect(cherrypy.request.path_info, cherrypy.request.params)
 
             try:
-                limit_ = int(kwpop('limit'))
+                limit_ = int(kwpop('limit'), None)
+                if limit_ is None:
+                    # limit not specified by user, use default
+                    raise ValueError

I think this hunk is why limit without max_limit isn't working. Currently, if no limit is specified in the URL, we end up using min(None, max_limit) which is None. Checking for None and using the limit specified to the paginate decorator seems like the right thing to do.

                 if max_limit is not None:
-                    if max_limit <= 0 and not allow_limit_override:
+                    if max_limit <= 0 or not allow_limit_override:
+                        # Old and new way of specifying that user can't change
+                        # default, use default
                         raise ValueError
                     limit_ = min(limit_, max_limit)
             except (TypeError, ValueError):

This hunk fixes max_limit having no effect unless allow_limit_override is specified as well.

comment:2 Changed 10 years ago by toshio

Ugh. This problem is trickier than I thought at first. Since max_limit is a new variable and allow_limit_override is deprecated but we want to preserve behaviour for backwards compatibility, we need a little more information kept here. I'll have another patch soon.

comment:3 follow-up: ↓ 5 Changed 10 years ago by toshio

Okay, I've got some new test cases of which two fail:

        # can't override limit when server-new-style says no and
        # server-old-style says yes
        self.request("/conflict_no_change_limit?data_tgp_limit=9")
        assert '"data": [0, 1, 2, 3]' in self.body

        # can override server-old-style limit
        self.request("/old_limit?data_tgp_limit=5")
        assert '"data": [0, 1, 2, 3, 4]' in self.body
        Spy.assert_ok(self.body, 'page_count', 2)
        Spy.assert_ok(self.body, 'limit', 5)
        Spy.assert_ok(self.body, 'pages', xrange(1, 3))

The first one is when the new-style max_limit and old-style allow_limit_override are specified in a conflicting manner so it's not too worrisome. The second is a test case that mimics this bug report so it shows the problem.

Will attach a patch with both testcases and a fix.

Note that I wasn't able to replicate this:

Yet, specifying max_limit without allow_limit_override does not seem to allow for limit overrides...

Changed 10 years ago by toshio

Better patch to handle allow_limit_override and max_limit

comment:4 Changed 10 years ago by toshio

  • Keywords Patch added

comment:5 in reply to: ↑ 3 Changed 10 years ago by chrisz

Replying to toshio:

Note that I wasn't able to replicate this:

Yet, specifying max_limit without allow_limit_override does not seem to allow for limit overrides...

I think it was meant the other way round: Specifying allow_limit_override without max_limit does not seem to allow for limit overrides

comment:6 Changed 10 years ago by chrisz

I have now fixed this in r6575 and added a unit test to check the old behavior and the deprecation warning. Let me know if there is still a problem.

comment:7 Changed 10 years ago by toshio

Thanks chrisz! The applied patch has one thing that seems odd to me but it may be by design. If both allow_limit_override and max_limit are specified and they are in conflict with each other the user is able to override the default limit. My expectation is that max_limit would always take precedence as it's the new way of doing things and allow_limit_override is deprecated. Not a big deal as conflicts between the two are obviously a mistake in the application code.

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

Actually max_limit takes precedence, except for one corner case where max_limit=0 and allow_limit_override=True. This is because the default value for max_limit is 0, so we can't check whether this 0 was excplicitly passed or is the default value. We would need to use a NoDefault class or something to check that, but I think we don't really need to care, since we never defined what happens if you set conflicting parameters and you get a deprecation message anyway.

comment:9 in reply to: ↑ 8 Changed 10 years ago by toshio

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

Yeah, that's why my patch set the default arg to "unset". If we don't care about the corner case, that's fine.

I'll get the fix into the next Fedora build. Thanks!

Note: See TracTickets for help on using tickets.