Query._get_constraints() should not modify a list in req.args

[Jun Omae]

I sometimes encounter IndexErrors from Query._get_constraints() on our production environment with private plugin which calls Query._get_constraints().

ERROR: Internal Server Error: <RequestWithSession "POST '/query'">, referrer 'https://example.org/query'
Traceback (most recent call last):
  File "/venv/lib/python2.7/site-packages/trac/web/main.py", line 554, in _dispatch_request
    dispatcher.dispatch(req)
  File "/venv/lib/python2.7/site-packages/trac/web/main.py", line 257, in dispatch
    self._post_process_request(req, *resp)
  File "/venv/lib/python2.7/site-packages/trac/web/main.py", line 388, in _post_process_request
    resp = f.post_process_request(req, *resp)
  File "/vol/releases/20190322024230/share/plugins/XxxPlugin.egg/xxx/project/ticket/custom_field.py", line 626, in post_process_request
    if not mod._get_constraints(req) and 'order' not in req.args:
  File "/venv/lib/python2.7/site-packages/trac/ticket/query.py", line 1066, in _get_constraints
    del vals[idx]
IndexError: list assignment index out of range

Investigating the issue, it is caused by that Query._get_constraints() modifies a list in req.args and is called twice when a condition is removed.

I don't consider the method should modify a list in req.args.

[9f904f1df/jomae.git]:

trac/ticket/query.py

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

trac/ticket/tests/query.py

@@ -1108 +1108 @@
         prop = req.chrome['script_data']['properties']['milestone']
         self.assertEqual({'label': 'Milestone', 'type': 'text'}, prop)
 
+    def test_get_constraints_keep_req_args(self):
+        arg_list = (('0_type', 'defect'), ('0_type', 'task'),
+                    ('0_type', 'enhancement'), ('rm_filter_0_type_1', '-'),
+                    ('1_type', 'task'))
+        req = MockRequest(self.env, method='POST', path_info='/query',
+                          arg_list=arg_list)
+        orig_args = arg_list_to_args(arg_list)
+        mod = QueryModule(self.env)
+        self.assertTrue(mod.match_request(req))
+        template, data, content_type = mod.process_request(req)
+        self.assertEqual([{'type': ['defect', 'enhancement']},
+                          {'type': ['task']}],
+                         data['query'].constraints)
+        self.assertEqual(orig_args, req.args)
+
 
 class QueryLinksTestCase(unittest.TestCase):
 

[Ryan J Ollos]

The change looks good to me.

[Jun Omae]

Thanks for the reviewing. I'll push the change.

[Jun Omae]

Committed in [16922] and merged in [16923,16924].