Merge pull request #24643 from akhilnarang/optimize-redis

refactor: try to optimize redis calls

frappe/boot.py

@@ -450,12 +450,12 @@ def get_marketplace_apps():
 		return request.json()["message"]
 
 	try:
-		apps = frappe.cache().get_value(cache_key, get_apps_from_fc, shared=True)
+		apps = frappe.cache.get_value(cache_key, get_apps_from_fc, shared=True)
 		installed_apps = set(frappe.get_installed_apps())
 		apps = [app for app in apps if app["name"] not in installed_apps]
 	except Exception:
 		# Don't retry for a day
-		frappe.cache().set_value(cache_key, apps, shared=True, expires_in_sec=24 * 60 * 60)
+		frappe.cache.set_value(cache_key, apps, shared=True, expires_in_sec=24 * 60 * 60)
 
 	return apps
 

frappe/cache_manager.py

@@ -82,13 +82,11 @@ def clear_user_cache(user=None):
 	clear_notifications(user)
 
 	if user:
-		for name in user_cache_keys:
-			frappe.cache.hdel(name, user)
+		frappe.cache.hdel_names(user_cache_keys, user)
 		frappe.cache.delete_keys("user:" + user)
 		clear_defaults_cache(user)
 	else:
-		for name in user_cache_keys:
-			frappe.cache.delete_key(name)
+		frappe.cache.delete_key(user_cache_keys)
 		clear_defaults_cache()
 		clear_global_cache()
 
@@ -103,17 +101,15 @@ def clear_global_cache():
 
 	clear_doctype_cache()
 	clear_website_cache()
-	frappe.cache.delete_value(global_cache_keys)
-	frappe.cache.delete_value(bench_cache_keys)
+	frappe.cache.delete_value(global_cache_keys + bench_cache_keys)
 	frappe.setup_module_map()
 
 
 def clear_defaults_cache(user=None):
 	if user:
-		for p in [user] + common_default_keys:
-			frappe.cache.hdel("defaults", p)
+		frappe.cache.hdel("defaults", [user] + common_default_keys)
 	elif frappe.flags.in_install != "frappe":
-		frappe.cache.delete_key("defaults")
+		frappe.cache.delete_value("defaults")
 
 
 def clear_doctype_cache(doctype=None):
@@ -128,15 +124,14 @@ def clear_doctype_cache(doctype=None):
 def _clear_doctype_cache_from_redis(doctype: str | None = None):
 	from frappe.desk.notifications import delete_notification_count_for
 
-	for key in ("is_table", "doctype_modules"):
-		frappe.cache.delete_value(key)
-
-	def clear_single(dt):
-		frappe.clear_document_cache(dt)
-		for name in doctype_cache_keys:
-			frappe.cache.hdel(name, dt)
+	to_del = ["is_table", "doctype_modules"]
 
 	if doctype:
+
+		def clear_single(dt):
+			frappe.clear_document_cache(dt)
+			frappe.cache.hdel_names(doctype_cache_keys, dt)
+
 		clear_single(doctype)
 
 		# clear all parent doctypes
@@ -157,9 +152,10 @@ def _clear_doctype_cache_from_redis(doctype: str | None = None):
 
 	else:
 		# clear all
-		for name in doctype_cache_keys:
-			frappe.cache.delete_value(name)
-		frappe.cache.delete_keys("document_cache::")
+		to_del += doctype_cache_keys
+		to_del += frappe.cache.get_keys("document_cache::")
+
+	frappe.cache.delete_value(to_del)
 
 
 def clear_controller_cache(doctype=None):

frappe/desk/notifications.py

@@ -72,7 +72,7 @@ def get_notifications_for_doctypes(config, notification_count):
 				except frappe.PermissionError:
 					frappe.clear_messages()
 					pass
-					# frappe.msgprint("Permission Error in notifications for {0}".format(d))
+				# frappe.msgprint("Permission Error in notifications for {0}".format(d))
 
 				except Exception as e:
 					# OperationalError: (1412, 'Table definition has changed, please retry transaction')
@@ -149,11 +149,10 @@ def clear_notifications(user=None):
 	for_module = list(config.get("for_module")) if config.get("for_module") else []
 	groups = for_doctype + for_module
 
-	for name in groups:
-		if user:
-			frappe.cache.hdel("notification_count:" + name, user)
-		else:
-			frappe.cache.delete_key("notification_count:" + name)
+	if user:
+		frappe.cache.hdel_names([f"notification_count:{name}" for name in groups], user)
+	else:
+		frappe.cache.delete_value([f"notification_count:{name}" for name in groups])
 
 
 def clear_notification_config(user):

frappe/tests/test_caching.py

@@ -2,6 +2,7 @@ import time
 from unittest.mock import MagicMock
 
 import frappe
+from frappe.core.doctype.doctype.test_doctype import new_doctype
 from frappe.tests.test_api import FrappeAPITestCase
 from frappe.tests.utils import FrappeTestCase
 from frappe.utils.caching import redis_cache, request_cache, site_cache
@@ -194,7 +195,7 @@ class TestDocumentCache(FrappeAPITestCase):
 		self.test_value = frappe.generate_hash()
 
 	def test_caching(self):
-		doc = frappe.get_cached_doc(self.TEST_DOCTYPE, self.TEST_DOCNAME)
+		frappe.get_cached_doc(self.TEST_DOCTYPE, self.TEST_DOCNAME)
 
 		with self.assertQueryCount(0):
 			doc = frappe.get_cached_doc(self.TEST_DOCTYPE, self.TEST_DOCNAME)
@@ -243,5 +244,79 @@ class TestRedisWrapper(FrappeAPITestCase):
 		frappe.cache.delete_keys(prefix)
 		self.assertEqual(len(frappe.cache.get_keys(prefix)), 0)
 
+	def test_hash(self):
+		key = "test_hash"
+
+		# Confirm that there's no data initially
+		exists = frappe.cache.exists(key)
+		self.assertFalse(exists)
+
+		# Insert 5 key-value pairs
+		for i in range(5):
+			frappe.cache.hset(key, f"key_{i}", f"value_{i}")
+
+		# Check that we have 5 values
+		values = frappe.cache.hgetall(key)
+		self.assertEqual(len(values), 5)
+
+		# Check that each value matches
+		for i in range(5):
+			value = frappe.cache.hget(key, f"key_{i}")
+			self.assertEqual(value, f"value_{i}")
+
+		# Check the keys themselves
+		keys = frappe.cache.hkeys(key)
+		for i in range(5):
+			self.assertIn(f"key_{i}".encode(), keys)
+
+		# Delete a single key and check that we still have the remaining 4
+		frappe.cache.hdel(key, "key_1")
+		values = frappe.cache.hgetall(key)
+		self.assertEqual(len(values), 4)
+
+		# Delete 2 keys and check that we still have the remaining 2
+		frappe.cache.hdel(key, ["key_2", "key_3"])
+		values = frappe.cache.hgetall(key)
+		self.assertEqual(len(values), 2)
+
+		# Delete the hash itself and confirm that there's no data
+		frappe.cache.delete_value(key)
+		exists = frappe.cache.exists(key)
+		self.assertFalse(exists)
+
+	def test_user_cache_clear(self):
+		from frappe.cache_manager import user_cache_keys
+
+		# Set some keys that a user's cache would usually have
+		user1 = frappe.utils.random_string(10)
+		user2 = frappe.utils.random_string(10)
+		for key in user_cache_keys:
+			frappe.cache.hset(key, user1, key)
+			frappe.cache.hset(key, user2, key)
+
+		frappe.clear_cache(user=user1)
+
+		# Check that the keys for user1 are gone
+		for key in user_cache_keys:
+			self.assertFalse(frappe.cache.hexists(key, user1))
+			self.assertTrue(frappe.cache.hexists(key, user2))
+
+	def test_doctype_cache_clear(self):
+		from frappe.cache_manager import doctype_cache_keys
+
+		# Set some keys that a user's cache would usually have
+		doctype1 = new_doctype(frappe.utils.random_string(10))
+		doctype2 = new_doctype(frappe.utils.random_string(10))
+		for key in doctype_cache_keys:
+			frappe.cache.hset(key, doctype1.name, key)
+			frappe.cache.hset(key, doctype2.name, key)
+
+		frappe.clear_cache(doctype=doctype1.name)
+
+		# Check that the keys for doctype1 are gone
+		for key in doctype_cache_keys:
+			self.assertFalse(frappe.cache.hexists(key, doctype1.name))
+			self.assertTrue(frappe.cache.hexists(key, doctype2.name))
+
 	def test_backward_compat_cache(self):
 		self.assertEqual(frappe.cache, frappe.cache())

frappe/twofactor.py

@@ -96,16 +96,17 @@ def cache_2fa_data(user, token, otp_secret, tmp_id):
 	pwd = frappe.form_dict.get("pwd")
 	verification_method = get_verification_method()
 
+	pipeline = frappe.cache.pipeline()
+
 	# set increased expiry time for SMS and Email
 	if verification_method in ["SMS", "Email"]:
 		expiry_time = frappe.flags.token_expiry or 300
-		frappe.cache.set(tmp_id + "_token", token)
-		frappe.cache.expire(tmp_id + "_token", expiry_time)
+		pipeline.set(tmp_id + "_token", token, expiry_time)
 	else:
 		expiry_time = frappe.flags.otp_expiry or 180
 	for k, v in {"_usr": user, "_pwd": pwd, "_otp_secret": otp_secret}.items():
-		frappe.cache.set(f"{tmp_id}{k}", v)
-		frappe.cache.expire(f"{tmp_id}{k}", expiry_time)
+		pipeline.set(f"{tmp_id}{k}", v, expiry_time)
+	pipeline.execute()
 
 
 def two_factor_is_enabled_for_(user):

frappe/utils/caching.py

@@ -154,11 +154,10 @@ def redis_cache(ttl: int | None = 3600, user: str | bool | None = None) -> Calla
 			func_call_key = func_key + "::" + str(__generate_request_cache_key(args, kwargs))
 			if frappe.cache.exists(func_call_key):
 				return frappe.cache.get_value(func_call_key, user=user)
-			else:
-				val = func(*args, **kwargs)
-				ttl = getattr(func, "ttl", 3600)
-				frappe.cache.set_value(func_call_key, val, expires_in_sec=ttl, user=user)
-				return val
+			val = func(*args, **kwargs)
+			ttl = getattr(func, "ttl", 3600)
+			frappe.cache.set_value(func_call_key, val, expires_in_sec=ttl, user=user)
+			return val
 
 		return redis_cache_wrapper
 

frappe/utils/password.py

@@ -12,7 +12,6 @@ from frappe.utils import cstr, encode
 
 Auth = Table("__Auth")
 
-
 passlibctx = CryptContext(
 	schemes=[
 		"pbkdf2_sha256",
@@ -104,9 +103,7 @@ def check_password(user, pwd, doctype="User", fieldname="password", delete_track
 
 
 def delete_login_failed_cache(user):
-	frappe.cache.hdel("last_login_tried", user)
 	frappe.cache.hdel("login_failed_count", user)
-	frappe.cache.hdel("locked_account_time", user)
 
 
 def update_password(user, pwd, doctype="User", fieldname="password", logout_all_sessions=False):

frappe/utils/redis_wrapper.py

@@ -144,7 +144,7 @@ class RedisWrapper(redis.Redis):
 			frappe.local.cache.pop(key, None)
 
 		try:
-			self.delete(*keys)
+			self.unlink(*keys)
 		except redis.exceptions.ConnectionError:
 			pass
 
@@ -238,22 +238,71 @@ class RedisWrapper(redis.Redis):
 			self.hset(name, key, value, shared=shared)
 		return value
 
-	def hdel(self, name, key, shared=False):
+	def hdel(
+		self,
+		name: str,
+		keys: str | list | tuple,
+		shared=False,
+		pipeline: redis.client.Pipeline | None = None,
+	):
+		"""
+		A wrapper around redis' HDEL command
+
+		:param name: The hash name
+		:param keys: the keys to delete
+		:param shared: shared frappe key or not
+		:param pipeline: A redis.client.Pipeline object, if this transaction is to be run in a pipeline
+		"""
 		_name = self.make_key(name, shared=shared)
 
-		if _name in frappe.local.cache:
-			if key in frappe.local.cache[_name]:
-				del frappe.local.cache[_name][key]
-		try:
-			super().hdel(_name, key)
-		except redis.exceptions.ConnectionError:
-			pass
+		name_in_local_cache = _name in frappe.local.cache
+
+		if not isinstance(keys, list | tuple):
+			if name_in_local_cache and keys in frappe.local.cache[_name]:
+				del frappe.local.cache[_name][keys]
+			if pipeline:
+				pipeline.hdel(_name, keys)
+			else:
+				super().hdel(_name, keys)
+			return
+
+		local_pipeline = False
+
+		if pipeline is None:
+			pipeline = self.pipeline()
+			local_pipeline = True
+
+		for key in keys:
+			if name_in_local_cache:
+				if key in frappe.local.cache[_name]:
+					del frappe.local.cache[_name][key]
+			try:
+				pipeline.hdel(_name, key)
+			except redis.exceptions.ConnectionError:
+				pass
+
+		if local_pipeline:
+			pipeline.execute()
+
+	def hdel_names(self, names: list | tuple, key: str):
+		"""
+		A function to call HDEL on multiple hash names with a common key, run in a single pipeline
+
+		:param names: The hash names
+		:param key: The common key
+		"""
+		pipeline = self.pipeline()
+		for name in names:
+			self.hdel(name, key, pipeline=pipeline)
+		pipeline.execute()
 
 	def hdel_keys(self, name_starts_with, key):
 		"""Delete hash names with wildcard `*` and key"""
+		pipeline = self.pipeline()
 		for name in self.get_keys(name_starts_with):
 			name = name.split("|", 1)[1]
-			self.hdel(name, key)
+			self.hdel(name, key, pipeline=pipeline)
+		pipeline.execute()
 
 	def hkeys(self, name):
 		try:

frappe/website/utils.py

@@ -10,7 +10,6 @@ import yaml
 from werkzeug.wrappers import Response
 
 import frappe
-from frappe import _
 from frappe.model.document import Document
 from frappe.utils import (
 	cint,
@@ -30,14 +29,13 @@ CLEANUP_PATTERN_3 = re.compile(r"(-)\1+")
 
 
 def delete_page_cache(path):
-	frappe.cache.delete_value("full_index")
-	groups = ("website_page", "page_context")
+	groups = ["website_page", "page_context"]
 	if path:
-		for name in groups:
-			frappe.cache.hdel(name, path)
+		frappe.cache.hdel_names(groups, path)
+		frappe.cache.delete_value("full_index")
 	else:
-		for name in groups:
-			frappe.cache.delete_key(name)
+		groups.append("full_index")
+		frappe.cache.delete_value(groups)
 
 
 def find_first_image(html):
@@ -363,25 +361,24 @@ def clear_cache(path=None):
 	:param path: (optional) for the given path"""
 	from frappe.website.router import clear_routing_cache
 
-	for key in (
+	clear_routing_cache()
+
+	keys = [
 		"website_generator_routes",
 		"website_pages",
 		"website_full_index",
 		"languages_with_name",
 		"languages",
-	):
-		frappe.cache.delete_value(key)
+		"website_404",
+	]
 
-	clear_routing_cache()
-
-	frappe.cache.delete_value("website_404")
 	if path:
 		frappe.cache.hdel("website_redirects", path)
 		delete_page_cache(path)
 	else:
 		clear_sitemap()
 		frappe.clear_cache("Guest")
-		for key in (
+		keys += [
 			"portal_menu_items",
 			"home_page",
 			"website_route_rules",
@@ -389,8 +386,9 @@ def clear_cache(path=None):
 			"website_redirects",
 			"page_context",
 			"website_page",
-		):
-			frappe.cache.delete_value(key)
+		]
+
+	frappe.cache.delete_value(keys)
 
 	for method in frappe.get_hooks("website_clear_cache"):
 		frappe.get_attr(method)(path)