Merge "Don't query db if criteria longer than col length"

keystone/common/driver_hints.py

@@ -30,6 +30,10 @@ class Hints(object):
     accessed publicly. Also it contains a dict called limit, which will
     indicate the amount of data we want to limit our listing to.
 
+    If the filter is discovered to never match, then `cannot_match` can be set
+    to indicate that there will not be any matches and the backend work can be
+    short-circuited.
+
     Each filter term consists of:
 
     * ``name``: the name of the attribute being matched
@@ -44,6 +48,7 @@ class Hints(object):
     def __init__(self):
         self.limit = None
         self.filters = list()
+        self.cannot_match = False
 
     def add_filter(self, name, value, comparator='equals',
                    case_sensitive=False):

keystone/common/sql/core.py

@@ -239,6 +239,39 @@ def truncated(f):
     return wrapper
 
 
+class _WontMatch(Exception):
+    """Raised to indicate that the filter won't match.
+
+    This is raised to short-circuit the computation of the filter as soon as
+    it's discovered that the filter requested isn't going to match anything.
+
+    A filter isn't going to match anything if the value is too long for the
+    field, for example.
+
+    """
+
+    @classmethod
+    def check(cls, value, col_attr):
+        """Check if the value can match given the column attributes.
+
+        Raises this class if the value provided can't match any value in the
+        column in the table given the column's attributes. For example, if the
+        column is a string and the value is longer than the column then it
+        won't match any value in the column in the table.
+
+        """
+        col = col_attr.property.columns[0]
+        if isinstance(col.type, sql.types.Boolean):
+            # The column is a Boolean, we should have already validated input.
+            return
+        if not col.type.length:
+            # The column doesn't have a length so can't validate anymore.
+            return
+        if len(value) > col.type.length:
+            raise cls()
+        # Otherwise the value could match a value in the column.
+
+
 def _filter(model, query, hints):
     """Applies filtering to a query.
 
@@ -276,10 +309,13 @@ def _filter(model, query, hints):
             return query
 
         if filter_['comparator'] == 'contains':
+            _WontMatch.check(filter_['value'], column_attr)
             query_term = column_attr.ilike('%%%s%%' % filter_['value'])
         elif filter_['comparator'] == 'startswith':
+            _WontMatch.check(filter_['value'], column_attr)
             query_term = column_attr.ilike('%s%%' % filter_['value'])
         elif filter_['comparator'] == 'endswith':
+            _WontMatch.check(filter_['value'], column_attr)
             query_term = column_attr.ilike('%%%s' % filter_['value'])
         else:
             # It's a filter we don't understand, so let the caller
@@ -299,33 +335,40 @@ def _filter(model, query, hints):
 
         """
         key = filter_['name']
-        if isinstance(getattr(model, key).property.columns[0].type,
-                      sql.types.Boolean):
+
+        col = getattr(model, key)
+        if isinstance(col.property.columns[0].type, sql.types.Boolean):
             cumulative_filter_dict[key] = (
                 utils.attr_as_boolean(filter_['value']))
         else:
+            _WontMatch.check(filter_['value'], col)
             cumulative_filter_dict[key] = filter_['value']
 
-    filter_dict = {}
-    satisfied_filters = []
-    for filter_ in hints.filters:
-        if filter_['name'] not in model.attributes:
-            continue
-        if filter_['comparator'] == 'equals':
-            exact_filter(model, filter_, filter_dict)
-            satisfied_filters.append(filter_)
-        else:
-            query = inexact_filter(model, query, filter_, satisfied_filters)
+    try:
+        filter_dict = {}
+        satisfied_filters = []
+        for filter_ in hints.filters:
+            if filter_['name'] not in model.attributes:
+                continue
+            if filter_['comparator'] == 'equals':
+                exact_filter(model, filter_, filter_dict)
+                satisfied_filters.append(filter_)
+            else:
+                query = inexact_filter(model, query, filter_,
+                                       satisfied_filters)
 
-    # Apply any exact filters we built up
-    if filter_dict:
-        query = query.filter_by(**filter_dict)
+        # Apply any exact filters we built up
+        if filter_dict:
+            query = query.filter_by(**filter_dict)
 
-    # Remove satisfied filters, then the caller will know remaining filters
-    for filter_ in satisfied_filters:
-        hints.filters.remove(filter_)
+        # Remove satisfied filters, then the caller will know remaining filters
+        for filter_ in satisfied_filters:
+            hints.filters.remove(filter_)
 
-    return query
+        return query
+    except _WontMatch:
+        hints.cannot_match = True
+        return
 
 
 def _limit(query, hints):
@@ -366,6 +409,10 @@ def filter_limit_query(model, query, hints):
     # First try and satisfy any filters
     query = _filter(model, query, hints)
 
+    if hints.cannot_match:
+        # Nothing's going to match, so don't bother with the query.
+        return []
+
     # NOTE(henry-nash): Any unsatisfied filters will have been left in
     # the hints list for the controller to handle. We can only try and
     # limit here if all the filters are already satisfied since, if not,

keystone/tests/unit/test_backend.py

@@ -5804,6 +5804,40 @@ class FilterTests(filtering.FilterTests):
         self._delete_test_data('user', user_list)
         self._delete_test_data('group', group_list)
 
+    def _get_user_name_field_size(self):
+        """Return the size of the user name field for the backend.
+
+        Subclasses can override this method to indicate that the user name
+        field is limited in length. The user name is the field used in the test
+        that validates that a filter value works even if it's longer than a
+        field.
+
+        If the backend doesn't limit the value length then return None.
+
+        """
+        return None
+
+    def test_filter_value_wider_than_field(self):
+        # If a filter value is given that's larger than the field in the
+        # backend then no values are returned.
+
+        user_name_field_size = self._get_user_name_field_size()
+
+        if user_name_field_size is None:
+            # The backend doesn't limit the size of the user name, so pass this
+            # test.
+            return
+
+        # Create some users just to make sure would return something if the
+        # filter was ignored.
+        self._create_test_data('user', 2)
+
+        hints = driver_hints.Hints()
+        value = 'A' * (user_name_field_size + 1)
+        hints.add_filter('name', value)
+        users = self.identity_api.list_users(hints=hints)
+        self.assertEqual([], users)
+
 
 class LimitTests(filtering.FilterTests):
     ENTITIES = ['user', 'group', 'project']

keystone/tests/unit/test_backend_sql.py

@@ -743,6 +743,9 @@ class SqlTokenCacheInvalidation(SqlTests, test_backend.TokenCacheInvalidation):
 
 class SqlFilterTests(SqlTests, test_backend.FilterTests):
 
+    def _get_user_name_field_size(self):
+        return identity_sql.User.name.type.length
+
     def clean_up_entities(self):
         """Clean up entity test data from Filter Test Cases."""