Edgewall Software
Modify

Opened 3 years ago

Closed 3 years ago

#12684 closed enhancement (fixed)

_RequestArgs.get should alias getfirst

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.3.2
Component: general Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:

Fixed exceptions due to req.args.get unexpectedly returning a list.

API Changes:

_RequestArgs.get aliases getfirst, so req.args.get will no longer return a list when there are duplicate keys in the parameter string. This change is not backward-compatible.

Description (last modified by Ryan J Ollos)

Discussed in #12682, to make the API more pythonic and to fix issues like the one seen in #12682, req.args.get should return a string rather than a string or list. If a list is expected req.args.getlist should be used.

_RequestArgs.getbinary will be added, as a shorthand for: req.args.getint('key', default, min=0, max=1).

The as_ methods of _RequestArgs should be modified/extended to be consistent with the changes.

Code in query.py can also be refactored to use _RequestArgs: trunk/trac/ticket/query.py@15573:949-971#L948.

Attachments (0)

Change History (7)

comment:1 by Ryan J Ollos, 3 years ago

Description: modified (diff)

comment:2 by Ryan J Ollos, 3 years ago

API Changes: modified (diff)
Milestone: next-dev-1.3.x1.3.2
Owner: set to Ryan J Ollos
Release Notes: modified (diff)
Status: newassigned

comment:3 by Jun Omae, 3 years ago

I think we could use arg_list_to_args rather than adding new arg_dict_to_args:

  • trac/ticket/query.py

    diff --git a/trac/ticket/query.py b/trac/ticket/query.py
    index 1b94a1f71..24d7e8cc0 100644
    a b class QueryModule(Component):  
    929929                constraints = self._get_constraints(arg_list=arg_list)
    930930            else:
    931931                query = Query.from_string(self.env, qstring)
    932                 arg_dict = {'order': query.order, 'group': query.group,
    933                             'col': query.cols, 'max': query.max}
     932                args = [('order', query.order), ('group', query.group),
     933                        ('col', query.cols), ('max', query.max)]
    934934                if query.desc:
    935                     arg_dict['desc'] = '1'
     935                    args.append(('desc', '1'))
    936936                if query.groupdesc:
    937                     arg_dict['groupdesc'] = '1'
    938                 args = arg_dict_to_args(arg_dict)
     937                    args.append(('groupdesc', '1'))
     938                args = arg_list_to_args(args)
    939939                constraints = query.constraints
    940940
    941941            # Substitute $USER, or ensure no field constraints that depend

Also, we could remove one of isinstance(vals, (list, tuple)) pattern in query.py:

  • trac/ticket/query.py

    diff --git a/trac/ticket/query.py b/trac/ticket/query.py
    index 1b94a1f71..24d7e8cc0 100644
    a b class QueryModule(Component):  
    10191019            # requested for clients without JavaScript
    10201020            add_num = None
    10211021            constraints = {}
    1022             for k, vals in req.args.iteritems():
     1022            for k in req.args:
     1023                vals = req.args.getlist(k)
    10231024                match = self.add_re.match(k)
    10241025                if match:
    10251026                    add_num = match.group(1)
    class QueryModule(Component):  
    10311032                clause_num = int(match.group('clause'))
    10321033                if field not in fields:
    10331034                    continue
    1034                 if not isinstance(vals, (list, tuple)):
    1035                     vals = [vals]
    10361035                if vals:
    10371036                    mode = req.args.get(k + '_mode')
    10381037                    if mode:

comment:4 by Jun Omae, 3 years ago

Another variant: values in req.args are used in trac/ticket/web_ui.py without checking type of the value. It would raise AttributeError: 'list' object has no attribute 'strip' if custom field which is time type added to query string of /newticket twice.

  • trac/ticket/web_ui.py

    diff --git a/trac/ticket/web_ui.py b/trac/ticket/web_ui.py
    index 5068c00c4..2676dc5fa 100644
    a b class TicketModule(Component):  
    823823
    824824    def _populate(self, req, ticket, plain_fields=False):
    825825        if not plain_fields:
    826             fields = {k[6:]: v for k, v in req.args.iteritems()
     826            fields = {k[6:]: req.args.get(k) for k in req.args
    827827                      if k.startswith('field_') and
    828828                         'revert_' + k[6:] not in req.args}
    829829            # Handle revert of checkboxes (in particular, revert to 1)
    class TicketModule(Component):  
    833833                    if 'revert_' + k in req.args:
    834834                        fields[k] = ticket[k]
    835835        else:
    836             fields = req.args.copy()
     836            fields = {k: req.args.get(k) for k in req.args}
    837837        # Prevent direct changes to protected fields (status and resolution are
    838838        # set in the workflow, in get_ticket_changes())
    839839        for each in Ticket.protected_fields:
Last edited 3 years ago by Jun Omae (previous) (diff)

comment:5 by Ryan J Ollos, 3 years ago

Those changes look very good. Thanks!

Changes added to log:rjollos.git:t12684_alias_getfirst.

comment:6 by Ryan J Ollos, 3 years ago

API Changes: modified (diff)

comment:7 by Ryan J Ollos, 3 years ago

Resolution: fixed
Status: assignedclosed

Committed to trunk in [15611:15613].

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 as closed 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.