Add as_int and as_bool methods to the Request class

[Ryan J Ollos]

comment:5:ticket:12325 suggested adding as_int and as_bool methods to the Request class. This was discussed further a bit later in the ticket.

as_int and as_bool in trac.util do not raise exceptions. Perhaps we should be somewhat consistent with the interface of those functions. We could add two pairs of methods with the following behaviors:

• as_int(self, name, default=None, min=None, max=None)
  • Return default if name not in dict
  • Return default if exception raised converting self[name] to an int
• getint(self, name, default=None, min=None, max=None)
  • Return default if name not in dict
  • Raise HTTPBadRequest if exception is raised converting self[name] to an int

get_int would be a better name, but getint is more consistent with getfirst and getlist. Also it seems reasonable to expect that a get method might raise an exception.

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t12426_request_methods. I'm just posting for initial feedback on the methods added to the _RequestArgs class. There is still work to be done in utilizing the new methods throughout the codebase.

I tended towards using getint and getbool, thus always raising HTTPBadRequest on bad input. However, we may want to handle differently the two common scenarios: values input by the user and values not directly input by the user.

An example of values input by the user would be going back x days on the Timeline page. For the case that the input value is not an integer I initially chose an HTTPBadRequest, but reconsidered: [b5f40dee/rjollos.git].

In cases for which the user does not directly enter a value, such as the page number on search or query results, I'm more convinced we should raise an HTTPBadRequest on bad input: [13a4e04a/rjollos.git#file0]. In these cases the bad input would only occur due to bots or the user editing the URL.

Also at issue is how to require that a request argument is provided. Raising HTTPBadRequest when default is not provided to getint or getbool seems like an API inconsistency. Therefore I added a require method to _RequestArgs instead: [ab37fe54/rjollos.git].

[Ryan J Ollos]

Fixed in [14693].

[Ryan J Ollos]

[14693] broke sorting on the report page. The fix is to restore the previous logic:

trac/ticket/report.py

@@ -405 +405 @@
             if max:
                 params['max'] = max
             params.update(kwargs)
-            params['asc'] = 1 if asc else 0
+            params['asc'] = 1 if params.get('asc', asc) else 0
             return req.href.report(id, params)
 
         data = {'action': 'view',

I don't remember why I thought to change that. I'll push the fix shortly.

[Ryan J Ollos]

comment:5 changes committed in [14782].

[Jun Omae]

I noticed a minor issue. After [14782]. , the last query would be saved with /report/1?asc=True&page=1. Prior to [14782], the asc parameter was 1.

This is caused by passing boolean to asc parameter of as_int(params.get('asc'), asc, min=0, max=1) in [14782].

[Ryan J Ollos]

Thanks for spotting. I'll fix the issue.

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t12426_bool_asc_regression. I used bool(asc) in one or two places for clarify when a boolean argument was expected, even if not strictly necessary and an integer would technically suffice.

[Ryan J Ollos]

Committed to 1.2-stable in r15490, merged to trunk in r15491:r15492.