Edgewall Software
Modify

Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#12426 closed enhancement (fixed)

Add as_int and as_bool methods to the Request class

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.2
Component: web frontend Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:
  • Added as_bool to _RequestArgs, which converts the value to a boolean or returns a default if an exception is raised.
  • Added as_int to _RequestArgs, which converts the value to an int or returns a default if an exception is raised.
  • Added getbool to _RequestArgs, which gets the value as a boolean or raises an HTTPBadRequest exception on invalid input.
  • Added getint to _RequestArgs, which gets the value as an integer or raises an HTTPBadRequest exception on invalid input.
  • Added require to _RequestArgs, which raises an HTTPBadRequest exception if the parameter is not in the dictionary.
Internal Changes:

Description (last modified by 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.

Attachments (0)

Change History (10)

comment:1 by Ryan J Ollos, 6 years ago

Description: modified (diff)

comment:2 by Ryan J Ollos, 6 years ago

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].

Last edited 6 years ago by Ryan J Ollos (previous) (diff)

comment:3 by Ryan J Ollos, 6 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Fixed in [14693].

comment:4 by Ryan J Ollos, 6 years ago

API Changes: modified (diff)
Release Notes: modified (diff)

comment:5 by Ryan J Ollos, 6 years ago

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

  • trac/ticket/report.py

    diff --git a/trac/ticket/report.py b/trac/ticket/report.py
    index 702328e..61b6026 100644
    a b class ReportModule(Component):  
    405405            if max:
    406406                params['max'] = max
    407407            params.update(kwargs)
    408             params['asc'] = 1 if asc else 0
     408            params['asc'] = 1 if params.get('asc', asc) else 0
    409409            return req.href.report(id, params)
    410410
    411411        data = {'action': 'view',

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

comment:6 by Ryan J Ollos, 6 years ago

comment:5 changes committed in [14782].

comment:7 by Jun Omae, 5 years ago

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].

comment:8 by Ryan J Ollos, 5 years ago

Thanks for spotting. I'll fix the issue.

comment:9 by Ryan J Ollos, 5 years ago

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.

comment:10 by Ryan J Ollos, 5 years ago

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.