From b2d145f065626d89c00eb714fb89f2b2ed5245cf Mon Sep 17 00:00:00 2001
From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com>
Date: Tue, 30 Aug 2022 16:31:19 +0530
Subject: [PATCH] perf: short-circuit guest connection and basic perf tests
 (#17988) (#17991)

* perf: reorder condition to avoid redis call

* test: basic perf tests

(cherry picked from commit f5b8e5f015204ddb15e8507f4d169e93de4a30dd)

Co-authored-by: Ankush Menat <ankush@frappe.io>
---
 frappe/app.py             |  2 +-
 frappe/auth.py            |  6 ++--
 frappe/tests/test_perf.py | 64 +++++++++++++++++++++++++++++++++++++++
 frappe/tests/utils.py     | 18 +++++++++++
 4 files changed, 87 insertions(+), 3 deletions(-)
 create mode 100644 frappe/tests/test_perf.py

diff --git a/frappe/app.py b/frappe/app.py
index 298d94b06c..3cf1bf555a 100644
--- a/frappe/app.py
+++ b/frappe/app.py
@@ -46,7 +46,7 @@ class RequestContext:
 
 @local_manager.middleware
 @Request.application
-def application(request):
+def application(request: Request):
 	response = None
 
 	try:
diff --git a/frappe/auth.py b/frappe/auth.py
index fec7ade839..7b64ecc083 100644
--- a/frappe/auth.py
+++ b/frappe/auth.py
@@ -226,14 +226,16 @@ class LoginManager:
 
 	def clear_active_sessions(self):
 		"""Clear other sessions of the current user if `deny_multiple_sessions` is not set"""
+		if frappe.session.user == "Guest":
+			return
+
 		if not (
 			cint(frappe.conf.get("deny_multiple_sessions"))
 			or cint(frappe.db.get_system_setting("deny_multiple_sessions"))
 		):
 			return
 
-		if frappe.session.user != "Guest":
-			clear_sessions(frappe.session.user, keep_current=True)
+		clear_sessions(frappe.session.user, keep_current=True)
 
 	def authenticate(self, user: str = None, pwd: str = None):
 		from frappe.core.doctype.user.user import User
diff --git a/frappe/tests/test_perf.py b/frappe/tests/test_perf.py
new file mode 100644
index 0000000000..6e23fb9856
--- /dev/null
+++ b/frappe/tests/test_perf.py
@@ -0,0 +1,64 @@
+"""
+This file contains multiple primitive tests for avoiding performance regressions.
+
+- Time bound tests: Benchmarks are done on GHA before adding numbers
+- Query count tests: More than expected # of queries for any action is frequent source of
+  performance issues. This guards against such problems.
+
+
+E.g. We know get_controller is supposed to be cached and hence shouldn't make query post first
+query. This test can be written like this.
+
+>>> def test_controller_caching(self):
+>>>
+>>> 	get_controller("User")  # <- "warm up code"
+>>> 	with self.assertQueryCount(0):
+>>> 		get_controller("User")
+
+"""
+import unittest
+
+import frappe
+from frappe.model.base_document import get_controller
+from frappe.tests.utils import FrappeTestCase
+from frappe.website.path_resolver import PathResolver
+
+
+class TestPerformance(FrappeTestCase):
+	def reset_request_specific_caches(self):
+		# To simulate close to request level of handling
+		frappe.destroy()  # releases everything on frappe.local
+		frappe.init(site=self.TEST_SITE)
+		frappe.connect()
+		frappe.clear_cache()
+
+	def setUp(self) -> None:
+		self.reset_request_specific_caches()
+
+	def test_meta_caching(self):
+		frappe.get_meta("User")
+
+		with self.assertQueryCount(0):
+			frappe.get_meta("User")
+
+	def test_controller_caching(self):
+
+		get_controller("User")
+		with self.assertQueryCount(0):
+			get_controller("User")
+
+	def test_db_value_cache(self):
+		"""Link validation if repeated should just use db.value_cache, hence no extra queries"""
+		doc = frappe.get_last_doc("User")
+		doc.get_invalid_links()
+
+		with self.assertQueryCount(0):
+			doc.get_invalid_links()
+
+	@unittest.skip("Not implemented")
+	def test_homepage_resolver(self):
+		paths = ["/", "/app"]
+		for path in paths:
+			PathResolver(path).resolve()
+			with self.assertQueryCount(1):
+				PathResolver(path).resolve()
diff --git a/frappe/tests/utils.py b/frappe/tests/utils.py
index 79dfd76238..aa27a5eb01 100644
--- a/frappe/tests/utils.py
+++ b/frappe/tests/utils.py
@@ -19,10 +19,13 @@ class FrappeTestCase(unittest.TestCase):
 	otherwise this class will become ineffective.
 	"""
 
+	TEST_SITE = "test_site"
+
 	SHOW_TRANSACTION_COMMIT_WARNINGS = False
 
 	@classmethod
 	def setUpClass(cls) -> None:
+		cls.TEST_SITE = getattr(frappe.local, "site", None) or cls.TEST_SITE
 		# flush changes done so far to avoid flake
 		frappe.db.commit()
 		frappe.db.begin()
@@ -64,6 +67,21 @@ class FrappeTestCase(unittest.TestCase):
 		else:
 			self.assertEqual(expected, actual, msg=msg)
 
+	@contextmanager
+	def assertQueryCount(self, count):
+		def _sql_with_count(*args, **kwargs):
+			frappe.db.sql_query_count += 1
+			return orig_sql(*args, **kwargs)
+
+		try:
+			orig_sql = frappe.db.sql
+			frappe.db.sql_query_count = 0
+			frappe.db.sql = _sql_with_count
+			yield
+			self.assertLessEqual(frappe.db.sql_query_count, count)
+		finally:
+			frappe.db.sql = orig_sql
+
 
 def _commit_watcher():
 	import traceback