From b2ff5e6125be07f8bd7634585aa3096d15b8b4a7 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 15 Jan 2025 10:30:02 +0530 Subject: [PATCH 1/5] perf: use cached navbar --- frappe/core/doctype/navbar_settings/navbar_settings.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/frappe/core/doctype/navbar_settings/navbar_settings.py b/frappe/core/doctype/navbar_settings/navbar_settings.py index 08fda8a8b6..dc3263a27b 100644 --- a/frappe/core/doctype/navbar_settings/navbar_settings.py +++ b/frappe/core/doctype/navbar_settings/navbar_settings.py @@ -24,8 +24,10 @@ class NavbarSettings(Document): def get_app_logo(): - app_logo = frappe.get_website_settings("app_logo") or frappe.db.get_single_value( - "Navbar Settings", "app_logo", cache=True + app_logo = frappe.get_website_settings("app_logo") or frappe.get_cached_value( + "Navbar Settings", + "Navbar Settings", + "app_logo", ) if not app_logo: From 2c2ec1387400fb04c10375cebfcaace7f8817c02 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 15 Jan 2025 10:47:18 +0530 Subject: [PATCH 2/5] perf: reuse jenv across requests --- frappe/utils/jinja.py | 42 +++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/frappe/utils/jinja.py b/frappe/utils/jinja.py index 4e5a2b6a1a..63f48a9b9f 100644 --- a/frappe/utils/jinja.py +++ b/frappe/utils/jinja.py @@ -6,35 +6,39 @@ from frappe.utils.caching import site_cache def get_jenv(): import frappe + from frappe.utils.safe_exec import get_safe_globals - if not getattr(frappe.local, "jenv", None): - from jinja2 import DebugUndefined - from jinja2.sandbox import SandboxedEnvironment + jenv = _get_jenv() + jenv.globals.update(get_safe_globals()) # TODO: This isn't thread safe. + frappe.local.jenv = jenv + return jenv - from frappe.utils.safe_exec import UNSAFE_ATTRIBUTES, get_safe_globals - UNSAFE_ATTRIBUTES = UNSAFE_ATTRIBUTES - {"format", "format_map"} +@site_cache(ttl=10 * 60, maxsize=4) +def _get_jenv(): + from jinja2 import DebugUndefined + from jinja2.sandbox import SandboxedEnvironment - class FrappeSandboxedEnvironment(SandboxedEnvironment): - def is_safe_attribute(self, obj, attr, *args, **kwargs): - if attr in UNSAFE_ATTRIBUTES: - return False + from frappe.utils.safe_exec import UNSAFE_ATTRIBUTES, get_safe_globals - return super().is_safe_attribute(obj, attr, *args, **kwargs) + UNSAFE_ATTRIBUTES = UNSAFE_ATTRIBUTES - {"format", "format_map"} - # frappe will be loaded last, so app templates will get precedence - jenv = FrappeSandboxedEnvironment(loader=get_jloader(), undefined=DebugUndefined) - set_filters(jenv) + class FrappeSandboxedEnvironment(SandboxedEnvironment): + def is_safe_attribute(self, obj, attr, *args, **kwargs): + if attr in UNSAFE_ATTRIBUTES: + return False - jenv.globals.update(get_safe_globals()) + return super().is_safe_attribute(obj, attr, *args, **kwargs) - methods, filters = get_jinja_hooks() - jenv.globals.update(methods or {}) - jenv.filters.update(filters or {}) + # frappe will be loaded last, so app templates will get precedence + jenv = FrappeSandboxedEnvironment(loader=get_jloader(), undefined=DebugUndefined) + set_filters(jenv) - frappe.local.jenv = jenv + methods, filters = get_jinja_hooks() + jenv.globals.update(methods or {}) + jenv.filters.update(filters or {}) - return frappe.local.jenv + return jenv def get_template(path): From 829062b1e36f6a822940821571d26d8c7061dbe1 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 15 Jan 2025 10:56:07 +0530 Subject: [PATCH 3/5] fix: make shared jenv thread-safe A new copy is created for each request, but cache is shared. --- frappe/tests/test_safe_exec.py | 15 +++++++++++++++ frappe/utils/jinja.py | 33 +++++++++++++++++++++++++-------- 2 files changed, 40 insertions(+), 8 deletions(-) diff --git a/frappe/tests/test_safe_exec.py b/frappe/tests/test_safe_exec.py index eb65e6beea..94b1c7422c 100644 --- a/frappe/tests/test_safe_exec.py +++ b/frappe/tests/test_safe_exec.py @@ -2,6 +2,7 @@ import types import frappe from frappe.tests import IntegrationTestCase +from frappe.utils.jinja import get_jenv from frappe.utils.safe_exec import ServerScriptNotEnabled, get_safe_globals, safe_exec @@ -127,3 +128,17 @@ class TestSafeExec(IntegrationTestCase): class TestNoSafeExec(IntegrationTestCase): def test_safe_exec_disabled_by_default(self): self.assertRaises(ServerScriptNotEnabled, safe_exec, "pass") + + +class TestJinjaGlobals(IntegrationTestCase): + def test_jenv_thread_safety(self): + first = get_jenv() + # reinit to create a new local ctx, this "simulates" two request running in two diff + # thread. + frappe.init(frappe.local.site, force=True) + second = get_jenv() + self.assertIsNot(first, second) + self.assertIsNot(first.globals, second.globals) + self.assertIsNot(first.filters, second.filters) + self.assertIsNot(first.globals["frappe"], second.globals["frappe"]) + self.assertIsNot(first.globals["frappe"]["form_dict"], second.globals["frappe"]["form_dict"]) diff --git a/frappe/utils/jinja.py b/frappe/utils/jinja.py index 63f48a9b9f..af4153e42e 100644 --- a/frappe/utils/jinja.py +++ b/frappe/utils/jinja.py @@ -8,18 +8,39 @@ def get_jenv(): import frappe from frappe.utils.safe_exec import get_safe_globals - jenv = _get_jenv() - jenv.globals.update(get_safe_globals()) # TODO: This isn't thread safe. + if jenv := getattr(frappe.local, "jenv", None): + return jenv + + default_jenv = _get_jenv() + jenv = default_jenv.overlay() + # XXX: This is safe to share between requests, the only reason why we are overlaying jenv is to + # reuse cache but still have request specific jenv object. + if not frappe._dev_server: + jenv.cache = default_jenv.cache + + jenv.globals = default_jenv.globals.copy() + jenv.filters = default_jenv.filters.copy() + + jenv.globals.update(get_safe_globals()) + methods, filters = get_jinja_hooks() + jenv.globals.update(methods or {}) + jenv.filters.update(filters or {}) + frappe.local.jenv = jenv + return jenv @site_cache(ttl=10 * 60, maxsize=4) def _get_jenv(): + # XXX: DO NOT use any thread/request specific data in this function! + # Some functionality like `get_safe_globals` appears safe but internally uses request local + # data. + from jinja2 import DebugUndefined from jinja2.sandbox import SandboxedEnvironment - from frappe.utils.safe_exec import UNSAFE_ATTRIBUTES, get_safe_globals + from frappe.utils.safe_exec import UNSAFE_ATTRIBUTES UNSAFE_ATTRIBUTES = UNSAFE_ATTRIBUTES - {"format", "format_map"} @@ -31,13 +52,9 @@ def _get_jenv(): return super().is_safe_attribute(obj, attr, *args, **kwargs) # frappe will be loaded last, so app templates will get precedence - jenv = FrappeSandboxedEnvironment(loader=get_jloader(), undefined=DebugUndefined) + jenv = FrappeSandboxedEnvironment(loader=get_jloader(), undefined=DebugUndefined, cache_size=32) set_filters(jenv) - methods, filters = get_jinja_hooks() - jenv.globals.update(methods or {}) - jenv.filters.update(filters or {}) - return jenv From 9ebfea1d08047e235ae15238f0186f24f53c9564 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 15 Jan 2025 09:59:08 +0530 Subject: [PATCH 4/5] perf: avoid duplicate template caching --- frappe/utils/jinja.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/frappe/utils/jinja.py b/frappe/utils/jinja.py index af4153e42e..7216479b9c 100644 --- a/frappe/utils/jinja.py +++ b/frappe/utils/jinja.py @@ -18,6 +18,7 @@ def get_jenv(): if not frappe._dev_server: jenv.cache = default_jenv.cache + # Note: Overlay by default is "linked", we need to copy everything we are updating. jenv.globals = default_jenv.globals.copy() jenv.filters = default_jenv.filters.copy() @@ -116,7 +117,7 @@ def render_template(template, context=None, is_path=None, safe_render=True): try: if is_path or guess_is_path(template): is_path = True - compiled_template = compile_template(template) + compiled_template = get_template(template) else: jenv: SandboxedEnvironment = get_jenv() if safe_render and ".__" in template: @@ -167,12 +168,6 @@ def get_jloader(): return jloader -@site_cache(ttl=10 * 60, maxsize=16) -def compile_template(path): - jenv = get_jenv() - return jenv.get_template(path) - - @site_cache(ttl=10 * 60, maxsize=8) def _get_jloader(): from jinja2 import ChoiceLoader, PackageLoader, PrefixLoader From ba6e3f6cd1cf2872e2837a62c93428f4db2db46b Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 15 Jan 2025 12:07:06 +0530 Subject: [PATCH 5/5] test: igore edge case of invalid strings We parse them as None instead of 1-1-1 etc --- frappe/tests/test_utils.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/frappe/tests/test_utils.py b/frappe/tests/test_utils.py index 36f622932b..105f0f3eed 100644 --- a/frappe/tests/test_utils.py +++ b/frappe/tests/test_utils.py @@ -68,6 +68,7 @@ from frappe.utils.data import ( get_url_to_form, get_year_ending, getdate, + is_invalid_date_string, now_datetime, nowtime, pretty_date, @@ -669,11 +670,15 @@ class TestDateUtils(IntegrationTestCase): @given(st.datetimes()) def test_get_datetime(self, original): + if is_invalid_date_string(str(original)): + return parsed = get_datetime(str(original)) self.assertEqual(parsed, original) @given(st.datetimes(timezones=st.timezones())) def test_get_datetime_tz_aware(self, original): + if is_invalid_date_string(str(original)): + return parsed = get_datetime(str(original)) self.assertEqual(parsed, original)