diff --git a/api/features_api.py b/api/features_api.py index 7a4f9ec8b122..a0d34ccfd900 100644 --- a/api/features_api.py +++ b/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 = ( diff --git a/api/features_api_test.py b/api/features_api_test.py index 169ae4359b9e..b8525ea19645 100644 --- a/api/features_api_test.py +++ b/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.""" diff --git a/api/spec_mentors_api_test.py b/api/spec_mentors_api_test.py index 911ca50aa06f..dda0253d0887 100644 --- a/api/spec_mentors_api_test.py +++ b/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( diff --git a/framework/rediscache.py b/framework/rediscache.py index baf4adc1fb32..8fc544167ba9 100644 --- a/framework/rediscache.py +++ b/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 diff --git a/framework/rediscache_test.py b/framework/rediscache_test.py index 6573aee90d6c..6d448929b316 100644 --- a/framework/rediscache_test.py +++ b/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)) diff --git a/internals/core_models.py b/internals/core_models.py index 72997753a9da..43a667c395fd 100644 --- a/internals/core_models.py +++ b/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 diff --git a/internals/fetchmetrics.py b/internals/fetchmetrics.py index ce24eb5de9cf..311273e8bd45 100644 --- a/internals/fetchmetrics.py +++ b/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' diff --git a/internals/search.py b/internals/search.py index 7c8967735005..ee1df2405d4a 100644 --- a/internals/search.py +++ b/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: diff --git a/internals/search_test.py b/internals/search_test.py index d0ac746d0f2c..006dbc082c35 100644 --- a/internals/search_test.py +++ b/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.whencurrent_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') diff --git a/pages/guide.py b/pages/guide.py index 082964e09abc..8ddb5d9268d8 100644 --- a/pages/guide.py +++ b/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)