Don't exit if multiple matches found for a filter

Previously, specifying a filter that returned multiple results would
result in an error. This made sense for API v1.0 because filters could
only be specified once. However, this is not necessarily the case for
v1.1, which does support multiple filters. In addition, it was
inconsistent for v1.0, as specifying multiple filters on the command
line would result in a warning, not an error.

Resolve this by only warning the user if multiple matches are found for
a given filter. This requires adding a check to ensure at least three
characters are provided for the filter so as not to make it too generic.

Signed-off-by: Stephen Finucane <[email protected]>

Author: stephenfin

git_pw/api.py

@@ -286,13 +286,51 @@ def new_func(ctx, *args, **kwargs):
 
             value = list(kwargs[param.name] or [])
             if value and len(value) > 1 and value != param.default:
-                msg = ('Filtering by multiple %ss is not supported with API '
+                msg = ('The `--%s` filter was specified multiple times. '
+                       'Filtering by multiple %ss is not supported with API '
                        'version 1.0. If the server supports it, use version '
                        '1.1 instead. Refer to https://git.io/vN3vX for more '
                        'information.')
 
-                LOG.warning(msg, param.name)
+                LOG.warning(msg, param.name, param.name)
 
         return ctx.invoke(f, *args, **kwargs)
 
     return update_wrapper(new_func, f)
+
+
+def retrieve_filter_ids(resource_type, filter_name, filter_value):
+    """Retrieve IDs for items passed through by filter.
+
+    Some filters require client-side filtering, e.g. filtering patches by
+    submitter names.
+
+    Arguments:
+        resource_type: The filter's resource endpoint name.
+        filter_name: The name of the filter.
+        filter_value: The value of the filter.
+
+    Returns:
+        A list of querystring key-value pairs to use in the actual request.
+    """
+    if len(filter_value) < 3:
+        # protect agaisnt really generic (and essentially meaningless) queries
+        LOG.error('Filters must be at least 3 characters long')
+        sys.exit(1)
+
+    # NOTE(stephenfin): This purposefully ignores the possiblity of a second
+    # page because it's unlikely and likely unnecessary
+    items = index(resource_type, [('q', filter_value)])
+    if len(items) == 0:
+        LOG.warning('No matching %s found: %s', filter_name, filter_value)
+    elif len(items) > 1 and version() < (1, 1):
+        # we don't support multiple filters in 1.0
+        msg = ('More than one match for found for `--%s=%s`. '
+               'Filtering by multiple %ss is not supported with '
+               'API version 1.0. If the server supports it, use '
+               'version 1.1 instead. Refer to https://git.io/vN3vX '
+               'for more information.')
+
+        LOG.warning(msg, filter_name, filter_value, filter_name)
+
+    return [(filter_name, item['id']) for item in items]

git_pw/bundle.py

@@ -141,15 +141,7 @@ def list_cmd(owner, limit, page, sort, name):
         if (api.version() >= (1, 1) and '@' not in own) or own.isdigit():
             params.append(('owner', own))
         else:
-            users = api.index('users', [('q', own)])
-            if len(users) == 0:
-                LOG.error('No matching owner found: %s', own)
-                sys.exit(1)
-            elif len(users) > 1:
-                LOG.error('More than one owner found: %s', own)
-                sys.exit(1)
-
-            params.append(('owner', users[0]['id']))
+            params.extend(api.retrieve_filter_ids('users', 'owner', own))
 
     params.extend([
         ('q', name),

git_pw/patch.py

@@ -220,30 +220,14 @@ def list_cmd(state, submitter, delegate, archived, limit, page, sort, name):
         if (api.version() >= (1, 1) and '@' in subm) or subm.isdigit():
             params.append(('submitter', subm))
         else:
-            people = api.index('people', [('q', subm)])
-            if len(people) == 0:
-                LOG.error('No matching submitter found: %s', subm)
-                sys.exit(1)
-            elif len(people) > 1:
-                LOG.error('More than one submitter found: %s', subm)
-                sys.exit(1)
-
-            params.append(('submitter', people[0]['id']))
+            params.extend(api.retrieve_filter_ids('people', 'submitter', subm))
 
     for delg in delegate:
         # we support server-side filtering by username (but not email) in 1.1
         if (api.version() >= (1, 1) and '@' not in delg) or delg.isdigit():
             params.append(('delegate', delg))
         else:
-            users = api.index('users', [('q', delg)])
-            if len(users) == 0:
-                LOG.error('No matching delegates found: %s', delg)
-                sys.exit(1)
-            elif len(users) > 1:
-                LOG.error('More than one delegate found: %s', delg)
-                sys.exit(1)
-
-            params.append(('delegate', users[0]['id']))
+            params.extend(api.retrieve_filter_ids('users', 'delegate', delg))
 
     params.extend([
         ('q', name),

git_pw/series.py

@@ -128,15 +128,7 @@ def list_cmd(submitter, limit, page, sort, name):
         if (api.version() >= (1, 1) and '@' in subm) or subm.isdigit():
             params.append(('submitter', subm))
         else:
-            people = api.index('people', [('q', subm)])
-            if len(people) == 0:
-                LOG.error('No matching submitter found: %s', subm)
-                sys.exit(1)
-            elif len(people) > 1:
-                LOG.error('More than one submitter found: %s', subm)
-                sys.exit(1)
-
-            params.append(('submitter', people[0]['id']))
+            params.extend(api.retrieve_filter_ids('people', 'submitter', subm))
 
     params.extend([
         ('q', name),

releasenotes/notes/filter-multiple-matches-197ff839f6b578da.yaml

@@ -0,0 +1,6 @@
+---
+features:
+  - |
+    Filtering patches, bundles and series by user will now only display a
+    warning if multiple matches are found for a given filter and you're using
+    API v1.0. Previously this would have been an unconditional error.

tests/test_api.py

@@ -71,3 +71,65 @@ def test_version(mock_server):
     mock_server.return_value = 'https://example.com/api/1.1'
 
     assert api.version() == (1, 1)
+
+
+@mock.patch.object(api, 'index')
+def test_retrieve_filter_ids_too_short(mock_index):
+    with pytest.raises(SystemExit):
+        api.retrieve_filter_ids('users', 'owner', 'f')
+
+    assert not mock_index.called
+
+
+@mock.patch.object(api, 'LOG')
+@mock.patch.object(api, 'index')
+def test_retrieve_filter_ids_no_matches(mock_index, mock_log):
+    mock_index.return_value = []
+
+    ids = api.retrieve_filter_ids('users', 'owner', 'foo')
+
+    assert mock_log.warning.called
+    assert ids == []
+
+
+@mock.patch.object(api, 'LOG')
+@mock.patch.object(api, 'version')
+@mock.patch.object(api, 'index')
+def test_retrieve_filter_ids_multiple_matches_1_0(mock_index, mock_version,
+                                                  mock_log):
+    mock_index.return_value = [
+        {'id': 1}, {'id': 2},  # incomplete but good enough
+    ]
+    mock_version.return_value = (1, 0)
+
+    ids = api.retrieve_filter_ids('users', 'owner', 'foo')
+
+    assert mock_log.warning.called
+    assert ids == [('owner', 1), ('owner', 2)]
+
+
+@mock.patch.object(api, 'LOG')
+@mock.patch.object(api, 'version')
+@mock.patch.object(api, 'index')
+def test_retrieve_filter_ids_multiple_matches_1_1(mock_index, mock_version,
+                                                  mock_log):
+    mock_index.return_value = [
+        {'id': 1}, {'id': 2},  # incomplete but good enough
+    ]
+    mock_version.return_value = (1, 1)
+
+    ids = api.retrieve_filter_ids('users', 'owner', 'foo')
+
+    assert not mock_log.warning.called
+    assert ids == [('owner', 1), ('owner', 2)]
+
+
+@mock.patch.object(api, 'LOG')
+@mock.patch.object(api, 'index')
+def test_retrieve_filter_ids(mock_index, mock_log):
+    mock_index.return_value = [{'id': 1}]
+
+    ids = api.retrieve_filter_ids('users', 'owner', 'foo')
+
+    assert not mock_log.warning.called
+    assert ids == [('owner', 1)]

tests/test_bundle.py

@@ -232,32 +232,31 @@ def test_list_with_filters(self, mock_index, mock_version):
 
         mock_index.assert_has_calls(calls)
 
-    @mock.patch('git_pw.bundle.LOG')
-    def test_list_with_invalid_filters(self, mock_log, mock_index,
-                                       mock_version):
-        """Validate behavior with filters applied.
+    @mock.patch('git_pw.api.LOG')
+    def test_list_with_wildcard_filters(self, mock_log, mock_index,
+                                        mock_version):
+        """Validate behavior with a "wildcard" filter.
 
-        Try to filter against a submitter filter that's too broad. This should
-        error out saying that too many possible submitters were found.
+        Patchwork API v1.0 did not support multiple filters correctly. Ensure
+        the user is warned as necessary if a filter has multiple matches.
         """
 
         people_rsp = [self._get_users(), self._get_users()]
         bundle_rsp = [self._get_bundle()]
         mock_index.side_effect = [people_rsp, bundle_rsp]
 
         runner = CLIRunner()
-        result = runner.invoke(bundle.list_cmd, ['--owner', 'john.doe'])
+        runner.invoke(bundle.list_cmd, ['--owner', 'john.doe'])
 
-        assert result.exit_code == 1, result
-        assert mock_log.error.called
+        assert mock_log.warning.called
 
     @mock.patch('git_pw.api.LOG')
     def test_list_with_multiple_filters(self, mock_log, mock_index,
                                         mock_version):
         """Validate behavior with use of multiple filters.
 
         Patchwork API v1.0 did not support multiple filters correctly. Ensure
-        the user is warned as necessary.
+        the user is warned as necessary if they specify multiple filters.
         """
 
         people_rsp = [self._get_users()]

tests/test_patch.py

@@ -337,35 +337,31 @@ def test_list_with_filters(self, mock_index, mock_version):
 
         mock_index.assert_has_calls(calls)
 
-    @mock.patch('git_pw.patch.LOG')
-    def test_list_with_invalid_filters(self, mock_log, mock_index,
-                                       mock_version):
-        """Validate behavior with filters applied.
+    @mock.patch('git_pw.api.LOG')
+    def test_list_with_wildcard_filters(self, mock_log, mock_index,
+                                        mock_version):
+        """Validate behavior with a "wildcard" filter.
 
-        Try to filter against a sumbmitter filter that's too broad. This should
-        error out saying that too many possible submitters were found.
+        Patchwork API v1.0 did not support multiple filters correctly. Ensure
+        the user is warned as necessary if a filter has multiple matches.
         """
 
         people_rsp = [self._get_person(), self._get_person()]
         patch_rsp = [self._get_patch()]
         mock_index.side_effect = [people_rsp, patch_rsp]
 
         runner = CLIRunner()
-        result = runner.invoke(patch.list_cmd, ['--submitter',
-                                                '[email protected]'])
+        runner.invoke(patch.list_cmd, ['--submitter', '[email protected]'])
 
-        assert result.exit_code == 1, result
-        assert mock_log.error.called
-
-        mock_index.side_effect = [people_rsp, patch_rsp]
+        assert mock_log.warning.called
 
     @mock.patch('git_pw.api.LOG')
     def test_list_with_multiple_filters(self, mock_log, mock_index,
                                         mock_version):
         """Validate behavior with use of multiple filters.
 
         Patchwork API v1.0 did not support multiple filters correctly. Ensure
-        the user is warned as necessary.
+        the user is warned as necessary if they specify multiple filters.
         """
 
         people_rsp = [self._get_person()]

tests/test_series.py

@@ -187,33 +187,31 @@ def test_list_with_filters(self, mock_index, mock_version):
 
         mock_index.assert_has_calls(calls)
 
-    @mock.patch('git_pw.series.LOG')
-    def test_list_with_invalid_filters(self, mock_log, mock_index,
-                                       mock_version):
-        """Validate behavior with filters applied.
+    @mock.patch('git_pw.api.LOG')
+    def test_list_with_wildcard_filters(self, mock_log, mock_index,
+                                        mock_version):
+        """Validate behavior with a "wildcard" filter.
 
-        Try to filter against a sumbmitter filter that's too broad. This should
-        error out saying that too many possible submitters were found.
+        Patchwork API v1.0 did not support multiple filters correctly. Ensure
+        the user is warned as necessary if a filter has multiple matches.
         """
 
         people_rsp = [self._get_people(), self._get_people()]
         series_rsp = [self._get_series()]
         mock_index.side_effect = [people_rsp, series_rsp]
 
         runner = CLIRunner()
-        result = runner.invoke(series.list_cmd, ['--submitter',
-                                                 '[email protected]'])
+        runner.invoke(series.list_cmd, ['--submitter', '[email protected]'])
 
-        assert result.exit_code == 1, result
-        assert mock_log.error.called
+        assert mock_log.warning.called
 
     @mock.patch('git_pw.api.LOG')
     def test_list_with_multiple_filters(self, mock_log, mock_index,
                                         mock_version):
         """Validate behavior with use of multiple filters.
 
         Patchwork API v1.0 did not support multiple filters correctly. Ensure
-        the user is warned as necessary.
+        the user is warned as necessary if they specify multiple filters.
         """
 
         people_rsp = [self._get_people()]