_RequestArgs.get should alias getfirst

[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​.

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t12684_alias_getfirst.

[Jun Omae]

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

trac/ticket/query.py

@@ -929 +929 @@
                 constraints = self._get_constraints(arg_list=arg_list)
             else:
                 query = Query.from_string(self.env, qstring)
-                arg_dict = {'order': query.order, 'group': query.group,
-                            'col': query.cols, 'max': query.max}
+                args = [('order', query.order), ('group', query.group),
+                        ('col', query.cols), ('max', query.max)]
                 if query.desc:
-                    arg_dict['desc'] = '1'
+                    args.append(('desc', '1'))
                 if query.groupdesc:
-                    arg_dict['groupdesc'] = '1'
-                args = arg_dict_to_args(arg_dict)
+                    args.append(('groupdesc', '1'))
+                args = arg_list_to_args(args)
                 constraints = query.constraints
 
             # 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

@@ -1019 +1019 @@
             # requested for clients without JavaScript
             add_num = None
             constraints = {}
-            for k, vals in req.args.iteritems():
+            for k in req.args:
+                vals = req.args.getlist(k)
                 match = self.add_re.match(k)
                 if match:
                     add_num = match.group(1)
@@ -1031 +1032 @@
                 clause_num = int(match.group('clause'))
                 if field not in fields:
                     continue
-                if not isinstance(vals, (list, tuple)):
-                    vals = [vals]
                 if vals:
                     mode = req.args.get(k + '_mode')
                     if mode:

[Jun Omae]

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

@@ -823 +823 @@
 
     def _populate(self, req, ticket, plain_fields=False):
         if not plain_fields:
-            fields = {k[6:]: v for k, v in req.args.iteritems()
+            fields = {k[6:]: req.args.get(k) for k in req.args
                       if k.startswith('field_') and
                          'revert_' + k[6:] not in req.args}
             # Handle revert of checkboxes (in particular, revert to 1)
@@ -833 +833 @@
                     if 'revert_' + k in req.args:
                         fields[k] = ticket[k]
         else:
-            fields = req.args.copy()
+            fields = {k: req.args.get(k) for k in req.args}
         # Prevent direct changes to protected fields (status and resolution are
         # set in the workflow, in get_ticket_changes())
         for each in Ticket.protected_fields:

[Ryan J Ollos]

Those changes look very good. Thanks!

Changes added to log:rjollos.git:t12684_alias_getfirst.

[Ryan J Ollos]

Committed to trunk in [15611:15613].