Cache full search results. (#4382)

* Cache full search results.

* Addressed review comments.

api/features_api.py

@@ -96,7 +96,7 @@ def do_search(self):
     show_enterprise = (
         'feature_type' in user_query or self.get_bool_arg('showEnterprise'))
     try:
-      features_on_page, total_count = search.process_query(
+      features_on_page, total_count = search.process_query_using_cache(
           user_query, sort_spec=sort_spec, show_unlisted=show_unlisted_features,
           show_enterprise=show_enterprise, start=start, num=num, name_only=name_only)
     except ValueError as err:
@@ -306,7 +306,8 @@ def do_patch(self, **kwargs):
     notifier_helpers.notify_subscribers_and_save_amendments(
         feature, changed_fields, notify=True)
     # Remove all feature-related cache.
-    rediscache.delete_keys_with_prefix(FeatureEntry.feature_cache_prefix())
+    rediscache.delete_keys_with_prefix(FeatureEntry.DEFAULT_CACHE_KEY)
+    rediscache.delete_keys_with_prefix(FeatureEntry.SEARCH_CACHE_KEY)
     # Update full-text index.
     if feature:
       search_fulltext.index_feature(feature)
@@ -330,7 +331,8 @@ def do_delete(self, **kwargs) -> dict[str, str]:
       self.abort(403)
     feature.deleted = True
     feature.put()
-    rediscache.delete_keys_with_prefix(FeatureEntry.feature_cache_prefix())
+    rediscache.delete_keys_with_prefix(FeatureEntry.DEFAULT_CACHE_KEY)
+    rediscache.delete_keys_with_prefix(FeatureEntry.SEARCH_CACHE_KEY)
 
     # Write for new FeatureEntry entity.
     feature_entry: FeatureEntry | None = (

api/features_api_test.py

@@ -154,9 +154,9 @@ def tearDown(self):
         entity.key.delete()
 
     testing_config.sign_out()
-    rediscache.delete_keys_with_prefix('features|*')
-    rediscache.delete_keys_with_prefix('FeatureEntries|*')
-    rediscache.delete_keys_with_prefix('FeatureNames|*')
+    rediscache.delete_keys_with_prefix('features')
+    rediscache.delete_keys_with_prefix('FeatureEntries')
+    rediscache.delete_keys_with_prefix('FeatureNames')
 
   def test_get__all_listed(self):
     """Get all features that are listed."""

api/spec_mentors_api_test.py

@@ -62,8 +62,8 @@ def tearDown(self):
     self.app_admin.key.delete()
     testing_config.sign_out()
 
-    rediscache.delete_keys_with_prefix('features|*')
-    rediscache.delete_keys_with_prefix('FeatureEntries|*')
+    rediscache.delete_keys_with_prefix('features')
+    rediscache.delete_keys_with_prefix('FeatureEntries')
 
   def createFeature(self, params) -> FeatureEntry:
     with test_app.test_request_context(

framework/rediscache.py

@@ -121,8 +121,9 @@ def delete(key):
   redis_client.delete(cache_key)
 
 
-def delete_keys_with_prefix(pattern):
-  """Delete all keys matching a prefix pattern."""
+def delete_keys_with_prefix(prefix: str):
+  """Delete all keys matching a prefix."""
+  pattern = prefix + '|*'
   if redis_client is None:
     return
 

framework/rediscache_test.py

@@ -79,7 +79,7 @@ def test_delete_keys_with_prefix(self):
     rediscache.set('random_key1', '404')
     self.assertEqual('1', rediscache.get(KEY_1))
 
-    rediscache.delete_keys_with_prefix('cache_key|*')
+    rediscache.delete_keys_with_prefix('cache_key')
 
     self.assertEqual(None, rediscache.get(KEY_1))
     self.assertEqual(None, rediscache.get(KEY_2))

internals/core_models.py

@@ -199,6 +199,8 @@ def has_open_safari_review(self):
   # The prefix of rediscache keys for storing the feature name
   # of a feature.
   FEATURE_NAME_CACHE_KEY = 'FeatureNames'
+  # The prefix used when cacheing entire search results.
+  SEARCH_CACHE_KEY = 'FeatureSearch'
 
   def __init__(self, *args, **kwargs):
     # Initialise Feature.blink_components with a default value.  If
@@ -215,10 +217,6 @@ def __init__(self, *args, **kwargs):
   def feature_cache_key(cls, cache_key, feature_id):
     return '%s|%s' % (cache_key, feature_id)
 
-  @classmethod
-  def feature_cache_prefix(cls):
-    return '%s|*' % (cls.DEFAULT_CACHE_KEY)
-
   def put(self, **kwargs) -> Any:
     key = super(FeatureEntry, self).put(**kwargs)
     # Invalidate rediscache for the individual feature view.
@@ -230,6 +228,7 @@ def put(self, **kwargs) -> Any:
     cache_key = FeatureEntry.feature_cache_key(
         FeatureEntry.FEATURE_NAME_CACHE_KEY, self.key.integer_id())
     rediscache.delete(cache_key)
+    rediscache.delete_keys_with_prefix(FeatureEntry.SEARCH_CACHE_KEY)
 
     return key
 

internals/fetchmetrics.py

@@ -219,7 +219,7 @@ def get_template_data(self, **kwargs):
     # does a query on those datapoints and caches the result. If we don't invalidate when
     # we add datapoints, the cached query result will be lacking the new datapoints.
     # This is run once every 6 hours.
-    rediscache.delete_keys_with_prefix('metrics|*')
+    rediscache.delete_keys_with_prefix('metrics')
     return 'Success'
 
 

internals/search.py

@@ -22,6 +22,7 @@
 from google.cloud.ndb import Key
 from google.cloud.ndb.tasklets import Future  # for type checking only
 
+from framework import rediscache
 from framework import users
 from internals import (
   approval_defs,
@@ -37,7 +38,7 @@
 
 MAX_TERMS = 6
 DEFAULT_RESULTS_PER_PAGE = 100
-
+SEARCH_CACHE_TTL = 60 * 60  # One hour
 
 def process_exclude_deleted_unlisted_query() -> Future:
   """Return a future for all features, minus deleted and unlisted."""
@@ -311,6 +312,79 @@ def _sort_by_total_order(
   return sorted_id_list
 
 
+def make_cache_key(
+  user_query: str, sort_spec: str | None, show_unlisted: bool,
+  show_deleted: bool, show_enterprise: bool, start: int, num: int,
+  name_only: bool) -> str:
+  """Return a redis key string to store cached search results."""
+  return '|'.join([
+      FeatureEntry.SEARCH_CACHE_KEY,
+      user_query,
+      'sort_spec=' + str(sort_spec),
+      'show_unlisted=' + str(show_unlisted),
+      'show_deleted=' + str(show_deleted),
+      'show_enterprise=' + str(show_enterprise),
+      'start=' + str(start),
+      'num=' + str(num),
+      'name_only=' + str(name_only),
+  ])
+
+
+def is_cacheable(user_query: str, name_only: bool):
+  """Return True if this user query can be stored and viewed by other users."""
+  if not name_only:
+    logging.info('Search query not cached: could be large')
+    return False
+
+  if ':me' in user_query:
+    logging.info('Search query not cached: personalized')
+    return False
+
+  if ('is:recently-reviewed' in user_query or
+      'now' in user_query or
+      'current_stable' in user_query):
+    logging.info('Search query not cached: time-based')
+    return False
+
+  logging.info('Search query can be cached')
+  return True
+
+
+def process_query_using_cache(
+  user_query: str,
+  sort_spec: str | None = None,
+  show_unlisted=False,
+  show_deleted=False,
+  show_enterprise=False,
+  start=0,
+  num=DEFAULT_RESULTS_PER_PAGE,
+  context: Optional[QueryContext] = None,
+  name_only=False,
+) -> tuple[list[dict[str, Any]], int]:
+  """"""
+  cache_key = make_cache_key(
+      user_query, sort_spec, show_unlisted, show_deleted, show_enterprise,
+      start, num, name_only)
+  if is_cacheable(user_query, name_only):
+    logging.info('Checking cache at %r', cache_key)
+    cached_result = rediscache.get(cache_key)
+    if cached_result is not None:
+      logging.info('Found cached search result for %r', cache_key)
+      return cached_result
+
+  logging.info('Computing search result')
+  computed_result = process_query(
+      user_query, sort_spec=sort_spec, show_unlisted=show_unlisted,
+      show_deleted=show_deleted, show_enterprise=show_enterprise,
+      start=start, num=num, context=context, name_only=name_only)
+
+  if is_cacheable(user_query, name_only):
+    logging.info('Storing search result in cache: %r', cache_key)
+    rediscache.set(cache_key, computed_result, SEARCH_CACHE_TTL)
+
+  return computed_result
+
+
 def process_query(
   user_query: str,
   sort_spec: str | None = None,
@@ -341,6 +415,7 @@ def process_query(
     if not show_deleted:
       permission_terms.append(('', 'deleted', '=', 'false', None))
     # TODO(jrobbins): include unlisted features that the user is allowed to view.
+    # However, that would greatly complicate the search cache.
     if not show_unlisted:
       permission_terms.append(('', 'unlisted', '=', 'false', None))
     if not show_enterprise:

internals/search_test.py

@@ -450,6 +450,34 @@ def test_sort_by_total_order__multiple_items(self):
     actual = search._sort_by_total_order(feature_ids, total_order_ids)
     self.assertEqual([10, 9, 4, 1], actual)
 
+  def test_make_cache_key(self):
+    """We can make a search cache key."""
+    self.assertEqual(
+        ('FeatureSearch||sort_spec=None|show_unlisted=True|'
+         'show_deleted=False|show_enterprise=False|'
+         'start=0|num=100|name_only=True'),
+        search.make_cache_key('', None, True, False, False, 0, 100, True))
+    self.assertEqual(
+        ('FeatureSearch|canvas|sort_spec=created.when|show_unlisted=False|'
+         'show_deleted=True|show_enterprise=True|'
+         'start=1|num=20|name_only=False'),
+        search.make_cache_key(
+            'canvas', 'created.when', False, True, True, 1, 20, False))
+
+  def test_is_cacheable(self):
+    """We can make a search cache key."""
+    self.assertTrue(search.is_cacheable('', True))
+    self.assertTrue(search.is_cacheable('canvas', True))
+    self.assertTrue(search.is_cacheable('feature_type=4', True))
+
+    self.assertFalse(search.is_cacheable('starred-by:me', True))
+    self.assertFalse(search.is_cacheable('owner:me', True))
+    self.assertFalse(search.is_cacheable('pending-approval-by:me', True))
+    self.assertFalse(search.is_cacheable('is:recently-reviewed', True))
+    self.assertFalse(search.is_cacheable('created.when<now', True))
+    self.assertFalse(search.is_cacheable('shipping>current_stable', True))
+    self.assertFalse(search.is_cacheable('canvas', False))
+
   @mock.patch('internals.search.process_pending_approval_me_query')
   @mock.patch('internals.search.process_starred_me_query')
   @mock.patch('internals.search_queries.handle_me_query_async')

pages/guide.py

@@ -103,7 +103,8 @@ def process_post_data(self, **kwargs):
         feature_entry, [], is_update=False)
 
     # Remove all feature-related cache.
-    rediscache.delete_keys_with_prefix(FeatureEntry.feature_cache_prefix())
+    rediscache.delete_keys_with_prefix(FeatureEntry.DEFAULT_CACHE_KEY)
+    rediscache.delete_keys_with_prefix(FeatureEntry.SEARCH_CACHE_KEY)
 
     redirect_url = '/feature/' + str(key.integer_id())
     return self.redirect(redirect_url)
@@ -177,7 +178,7 @@ def process_post_data(self, **kwargs):
     self.write_gates_and_stages_for_feature(key.integer_id(), feature_type)
 
     # Remove all feature-related cache.
-    rediscache.delete_keys_with_prefix(FeatureEntry.feature_cache_prefix())
+    rediscache.delete_keys_with_prefix(FeatureEntry.DEFAULT_CACHE_KEY)
 
     redirect_url = '/guide/editall/' + str(key.integer_id()) + '#rollout1'
     return self.redirect(redirect_url)