diff --git a/cypress/integration/control_link.js b/cypress/integration/control_link.js index 4c0587b009..d33261d198 100644 --- a/cypress/integration/control_link.js +++ b/cypress/integration/control_link.js @@ -79,7 +79,7 @@ context("Control Link", () => { it("should unset invalid value", () => { get_dialog_with_link().as("dialog"); - cy.intercept("/api/method/frappe.client.validate_link*").as("validate_link"); + cy.intercept("/api/method/frappe.client.validate_link_and_fetch*").as("validate_link"); cy.get(".frappe-control[data-fieldname=link] input").focus().as("input"); // Wait for dropdown to appear (request might be cached) cy.get("@input").parent().findByRole("listbox").should("be.visible"); @@ -92,7 +92,7 @@ context("Control Link", () => { it("should be possible set empty value explicitly", () => { get_dialog_with_link().as("dialog"); - cy.intercept("/api/method/frappe.client.validate_link*").as("validate_link"); + cy.intercept("/api/method/frappe.client.validate_link_and_fetch*").as("validate_link"); cy.get(".frappe-control[data-fieldname=link] input").focus().as("input"); // Wait for dropdown to appear (request might be cached) @@ -179,7 +179,7 @@ context("Control Link", () => { it("should update dependant fields (via fetch_from)", () => { cy.get("@todos").then((todos) => { cy.visit(`/desk/todo/${todos[0]}`); - cy.intercept("/api/method/frappe.client.validate_link*").as("validate_link"); + cy.intercept("/api/method/frappe.client.validate_link_and_fetch*").as("validate_link"); cy.fill_field("assigned_by", cy.config("testUser"), "Link"); cy.call("frappe.client.get_value", { @@ -203,7 +203,7 @@ context("Control Link", () => { "" ); - cy.window().its("cur_frm.doc.assigned_by").should("eq", null); + cy.window().its("cur_frm.doc.assigned_by").should("eq", undefined); // set valid value again cy.get("@input").clear().focus(); diff --git a/cypress/integration/dashboard_chart.js b/cypress/integration/dashboard_chart.js index 8a2fb91d08..4798ce98bc 100644 --- a/cypress/integration/dashboard_chart.js +++ b/cypress/integration/dashboard_chart.js @@ -12,6 +12,9 @@ context("Dashboard Chart", () => { cy.fill_field("chart_name", "Test Chart", "Data"); cy.fill_field("document_type", "Workspace Link", "Link"); + // wait for link field events to complete + cy.wait(1000); + cy.get('[data-fieldname="filters_json"]').click(); cy.get(".modal-dialog", { timeout: 500 }).should("be.visible"); diff --git a/frappe/client.py b/frappe/client.py index 4f03b458c0..a1ee54fa46 100644 --- a/frappe/client.py +++ b/frappe/client.py @@ -9,6 +9,7 @@ import frappe.model import frappe.utils from frappe import _ from frappe.desk.reportview import validate_args +from frappe.desk.search import PAGE_LENGTH_FOR_LINK_VALIDATION, search_widget from frappe.model.utils import is_virtual_doctype from frappe.utils import attach_expanded_links, get_safe_filters from frappe.utils.caching import http_cache @@ -406,52 +407,85 @@ def is_document_amended(doctype: str, docname: str): return False -@frappe.whitelist() -def validate_link(doctype: str, docname: str, fields=None): - if not isinstance(doctype, str): - frappe.throw(_("DocType must be a string")) +@frappe.whitelist(methods=["GET", "POST"]) +def validate_link_and_fetch( + doctype: str, + docname: str, + fields_to_fetch: list[str] | str | None = None, + # search_widget parameters + query: str | None = None, + filters: dict | list | str | None = None, + **search_args, +): + if not docname: + frappe.throw(_("Document Name must not be empty")) - if not isinstance(docname, str): - frappe.throw(_("Document Name must be a string")) + fields_to_fetch = frappe.parse_json(fields_to_fetch) - parent_doctype = None - if doctype != "DocType": - if frappe.get_meta(doctype).istable: # needed for links to child rows - parent_doctype = frappe.db.get_value(doctype, docname, "parenttype") - if not ( - frappe.has_permission(doctype, "select", parent_doctype=parent_doctype) - or frappe.has_permission(doctype, "read", parent_doctype=parent_doctype) - ): - frappe.throw( - _("You do not have Read or Select Permissions for {}").format(frappe.bold(doctype)), - frappe.PermissionError, - ) + # only cache is no fields to fetch and request is GET + can_cache = not fields_to_fetch and frappe.request.method == "GET" - values = frappe._dict() + # Use search_widget to validate - ensures filters/custom queries are respected + # in addition to standard permission checks + search_args["txt"] = docname + search_result = frappe.call( + search_widget, + doctype=doctype, + query=query, + filters=filters, + **search_args, + for_link_validation=True, + ) - if is_virtual_doctype(doctype): + if not search_result: + return {} # does not exist or filtered out + + # get value in the right case and type (str | int) + # for matching with search result + columns_to_fetch = ["name"] + if frappe.is_table(doctype): + columns_to_fetch.append("parenttype") # for child table permission check + + is_virtual_dt = is_virtual_doctype(doctype) + if is_virtual_dt: try: - frappe.get_doc(doctype, docname) - values.name = docname + doc = frappe.get_doc(doctype, docname) + doc.check_permission("select" if frappe.only_has_select_perm(doctype) else "read") + values = {"name": doc.name} + except frappe.DoesNotExistError: frappe.clear_last_message() - frappe.msgprint( - _("Document {0} {1} does not exist").format(frappe.bold(doctype), frappe.bold(docname)), + return {} + else: + values = frappe.db.get_value(doctype, docname, columns_to_fetch, as_dict=True) + + name_to_compare = values["name"] + # this will be used to fetch fields later + parent_doctype = values.pop("parenttype", None) + + if not name_to_compare: + return {} # does not exist + + # try to match name in search result + # if search_result is large, assume valid link (result may not appear in some custom queries) + if len(search_result) < PAGE_LENGTH_FOR_LINK_VALIDATION and not any( + item[0] == name_to_compare for item in search_result + ): + return {} # no permission or filtered out + + # don't cache or fetch for virtual doctypes + if is_virtual_dt: + return values + + if not fields_to_fetch: + if can_cache: + frappe.local.response_headers.set( + "Cache-Control", "private,max-age=1800,stale-while-revalidate=7200" ) return values - values.name = frappe.db.get_value(doctype, docname, cache=True) - - fields = frappe.parse_json(fields) - if not values.name: - return values - - if not fields: - frappe.local.response_headers.set("Cache-Control", "private,max-age=1800,stale-while-revalidate=7200") - return values - try: - values.update(get_value(doctype, fields, docname, parent=parent_doctype)) + values.update(get_value(doctype, fields_to_fetch, docname, parent=parent_doctype)) except frappe.PermissionError: frappe.clear_last_message() frappe.msgprint( diff --git a/frappe/desk/search.py b/frappe/desk/search.py index 43927b18ea..d18ce76748 100644 --- a/frappe/desk/search.py +++ b/frappe/desk/search.py @@ -16,6 +16,8 @@ from frappe.utils import cint, cstr, escape_html, unique from frappe.utils.caching import http_cache from frappe.utils.data import make_filter_tuple +PAGE_LENGTH_FOR_LINK_VALIDATION = 25_000 + def sanitize_searchfield(searchfield: str): if not searchfield: @@ -76,6 +78,7 @@ def search_widget( ignore_user_permissions: bool = False, *, link_fieldname: str | None = None, + for_link_validation: bool = False, ): if ignore_user_permissions: if reference_doctype and link_fieldname: @@ -103,6 +106,34 @@ def search_widget( if not query and doctype in standard_queries: query = standard_queries[doctype][-1] + if filters is None: + filters = {} + + are_filters_dict = isinstance(filters, dict) + include_disabled = False + if not query and are_filters_dict: + if "include_disabled" in filters: + if filters["include_disabled"] == 1: + include_disabled = True + filters.pop("include_disabled") + + filters = [make_filter_tuple(doctype, key, value) for key, value in filters.items()] + are_filters_dict = False + + if for_link_validation: + if are_filters_dict: + # we add filter if possible, otherwise rely on txt + if "name" not in filters: + filters["name"] = txt + else: + filters.append([doctype, "name", "=", txt]) + + as_dict = False + # for custom queries that don't respect filters but respect limit (rare) + # or for when we have to rely on txt + # we want to match "A" with "A" only and not "A1", "BA" etc. + page_length = PAGE_LENGTH_FOR_LINK_VALIDATION + if query: # Query = custom search query i.e. python function try: is_whitelisted(frappe.get_attr(query)) @@ -132,17 +163,6 @@ def search_widget( return [] meta = frappe.get_meta(doctype) - - include_disabled = False - if filters and "include_disabled" in filters: - if filters["include_disabled"] == 1: - include_disabled = True - filters.pop("include_disabled") - - if isinstance(filters, dict): - filters = [make_filter_tuple(doctype, key, value) for key, value in filters.items()] - elif filters is None: - filters = [] or_filters = [] # build from doctype @@ -189,7 +209,7 @@ def search_widget( # `idx` is number of times a document is referred, check link_count.py order_by = f"idx desc, {order_by_based_on_meta}" - if not meta.translated_doctype: + if not for_link_validation and not meta.translated_doctype: _txt = frappe.db.escape((txt or "").replace("%", "").replace("@", "")) # locate returns 0 if string is not found, convert 0 to null and then sort null to end in order by _relevance_expr = {"DIV": [1, {"NULLIF": [{"LOCATE": [_txt, "name"]}, 0]}]} @@ -214,29 +234,30 @@ def search_widget( strict=False, ) - if meta.translated_doctype: - # Filtering the values array so that query is included in very element - values = ( - result - for result in values - if any( - re.search(f"{re.escape(txt)}.*", _(cstr(value)) or "", re.IGNORECASE) - for value in (result.values() if as_dict else result) + if not for_link_validation: + if meta.translated_doctype: + # Filtering the values array so that query is included in very element + values = ( + result + for result in values + if any( + re.search(f"{re.escape(txt)}.*", _(cstr(value)) or "", re.IGNORECASE) + for value in (result.values() if as_dict else result) + ) ) - ) - # Sorting the values array so that relevant results always come first - # This will first bring elements on top in which query is a prefix of element - # Then it will bring the rest of the elements and sort them in lexicographical order - values = sorted(values, key=lambda x: relevance_sorter(x, txt, as_dict)) + # Sorting the values array so that relevant results always come first + # This will first bring elements on top in which query is a prefix of element + # Then it will bring the rest of the elements and sort them in lexicographical order + values = sorted(values, key=lambda x: relevance_sorter(x, txt, as_dict)) - # remove _relevance from results - if not meta.translated_doctype: - if as_dict: - for r in values: - r.pop("_relevance", None) - else: - values = [r[:-1] for r in values] + # remove _relevance from results + if not meta.translated_doctype: + if as_dict: + for r in values: + r.pop("_relevance", None) + else: + values = [r[:-1] for r in values] return values diff --git a/frappe/public/js/frappe/form/controls/link.js b/frappe/public/js/frappe/form/controls/link.js index a2504197d9..3439a290f7 100644 --- a/frappe/public/js/frappe/form/controls/link.js +++ b/frappe/public/js/frappe/form/controls/link.js @@ -27,13 +27,8 @@ frappe.ui.form.ControlLink = class ControlLink extends frappe.ui.form.ControlDat this.$input.on("focus", function () { if (!me.$input.val()) { me.$input.val(""); - - // Create a fake input event - const e = $.Event("input"); - e.target = me.$input[0]; - - // Pass it to on_input directly, bypassing debounce, so the dropdown opens immediately - me.on_input(e); + // trigger dropdown immediately + me.on_input(); } me.show_link_and_clear_buttons(); @@ -154,7 +149,7 @@ frappe.ui.form.ControlLink = class ControlLink extends frappe.ui.form.ControlDat return null; } get_label_value() { - return this.$input ? this.$input.val() : ""; + return this.$input?.val() || ""; } set_input_value(value) { this.$input && this.$input.val(value); @@ -272,7 +267,8 @@ frappe.ui.form.ControlLink = class ControlLink extends frappe.ui.form.ControlDat this.custom_awesomplete_filter && this.custom_awesomplete_filter(this.awesomplete); - this.$input.on("input", frappe.utils.debounce(this.on_input.bind(this), 500)); + this._debounced_input_handler = frappe.utils.debounce(this.on_input.bind(this), 500); + this.$input.on("input", this._debounced_input_handler); this.$input.on("blur", function () { if (me.selected) { @@ -379,49 +375,35 @@ frappe.ui.form.ControlLink = class ControlLink extends frappe.ui.form.ControlDat } /** - * Determine if we should use GET (enables HTTP caching) or POST. - * Use GET for empty searches with filters that fit in URL. - * Use POST for searches with text or large filters. + * Helps determine if we should use GET (enables HTTP caching) or POST. + * Use GET for filters that fit in URL. + * Use POST for large filters. */ - should_use_post_for_search(txt, filters, max_get_size = 2000) { - // Always use POST if there's search text - if (txt) return true; + are_filters_large(filters, max_get_size = 2000) { + if (!filters) return [false, filters]; - // If no filters, use GET - if (!filters) return false; - - // Check size of filters when stringified let filters_str = filters; if (typeof filters !== "string") { try { filters_str = JSON.stringify(filters); } catch (e) { // If stringification fails, use POST - return true; + return [true, filters]; } } // URL-encoded params add ~30% overhead on average const estimated_size = filters_str.length * 1.3; - return estimated_size > max_get_size; + return [estimated_size > max_get_size, filters_str]; } - on_input(e) { - var doctype = this.get_options(); + get_search_args(txt) { + const doctype = this.get_options(); if (!doctype) return; - if (!this.$input.cache[doctype]) { - this.$input.cache[doctype] = {}; - } - var term = e.target.value; - - if (this.$input.cache[doctype][term] != null) { - // immediately show from cache - this.awesomplete.list = this.$input.cache[doctype][term]; - } - var args = { - txt: term, - doctype: doctype, + const args = { + txt, + doctype, ignore_user_permissions: this.df.ignore_user_permissions, reference_doctype: this.get_reference_doctype() || "", page_length: cint(frappe.boot.sysdefaults?.link_field_results_limit) || 10, @@ -429,8 +411,34 @@ frappe.ui.form.ControlLink = class ControlLink extends frappe.ui.form.ControlDat }; this.set_custom_query(args); + return args; + } - const use_get = !this.should_use_post_for_search(term, args.filters); + on_input(e) { + const term = e ? e.target.value : this.$input.val(); + const args = this.get_search_args(term); + if (!args) return; + + const doctype = args.doctype; + const cache = this.$input.cache; + if (!cache[doctype]) { + cache[doctype] = {}; + } + + if (cache[doctype][term] != null) { + // immediately show from cache + this.awesomplete.list = cache[doctype][term]; + } + + const filters = args.filters; + let use_get = !term; + if (use_get) { + const [are_filters_large, filters_str] = this.are_filters_large(filters); + use_get = !are_filters_large; + + // perf: to prevent stringifying again in the call + args.filters = filters_str; + } frappe.call({ type: use_get ? "GET" : "POST", method: "frappe.desk.search.search_link", @@ -446,8 +454,8 @@ frappe.ui.form.ControlLink = class ControlLink extends frappe.ui.form.ControlDat // show filter description in awesomplete let filter_string = this.df.filter_description ? this.df.filter_description - : args.filters - ? await this.get_filter_description(args.filters) + : filters + ? await this.get_filter_description(filters) : null; if (filter_string) { r.message.push({ @@ -496,8 +504,8 @@ frappe.ui.form.ControlLink = class ControlLink extends frappe.ui.form.ControlDat }); } } - this.$input.cache[doctype][term] = r.message; - this.awesomplete.list = this.$input.cache[doctype][term]; + cache[doctype][term] = r.message; + this.awesomplete.list = cache[doctype][term]; this.toggle_href(doctype); r.message.forEach((item) => { frappe.utils.add_link_title(doctype, item.value, item.label); @@ -850,38 +858,31 @@ frappe.ui.form.ControlLink = class ControlLink extends frappe.ui.form.ControlDat return this.validate_link_and_fetch(value); } validate_link_and_fetch(value) { - const options = this.get_options(); - if (!options) { - return; - } + const args = this.get_search_args(value); + if (!args.doctype) return; const columns_to_fetch = Object.values(this.fetch_map); - const nothing_to_fetch = !columns_to_fetch.length; // if default and no fetch, no need to validate - if (nothing_to_fetch && this.df.__default_value === value) { - return value; - } - - if ( - nothing_to_fetch && - value && - this.awesomplete?._list?.find((item) => item.value === value && !item.action) - ) { - // if value is in the suggestion list, must be correct + if (!columns_to_fetch.length && this.df.__default_value === value) { return value; } const update_dependant_fields = (response) => { - let field_value = ""; - for (const [target_field, source_field] of Object.entries(this.fetch_map)) { - if (value) { - field_value = response[source_field]; - } + if (!columns_to_fetch.length) return; - if (this.layout?.set_value) { - this.layout.set_value(target_field, field_value); - } else if (this.frm) { + const layout_set_value = this.layout?.set_value; + if (!layout_set_value && (!this.frm || !this.docname)) { + return; + } + + const has_value = Boolean(response?.name); + for (const [target_field, source_field] of Object.entries(this.fetch_map)) { + const field_value = has_value ? response[source_field] : ""; + + if (layout_set_value) { + layout_set_value(target_field, field_value); + } else { frappe.model.set_value( this.df.parent, this.docname, @@ -894,32 +895,42 @@ frappe.ui.form.ControlLink = class ControlLink extends frappe.ui.form.ControlDat }; // to avoid unnecessary request - if (value) { - return frappe - .xcall( - "frappe.client.validate_link", - { - doctype: options, - docname: value, - fields: columns_to_fetch, - }, - "GET", - { cache: !columns_to_fetch.length } - ) - .then((response) => { - if (this.frm && !this.docname) { - return response.name; - } - if (!columns_to_fetch.length) { - return response.name; - } - update_dependant_fields(response); - return response.name; - }); - } else { - update_dependant_fields({}); + if (!value) { + update_dependant_fields(); return value; } + + // if there is a search_link call scheduled, cancel it + // validation will do it + this._debounced_input_handler?.cancel(); + + // filters may be too large to be sent as GET + let can_cache = !columns_to_fetch.length; + if (can_cache) { + const [are_filters_large, filters_str] = this.are_filters_large(args.filters); + can_cache = !are_filters_large; + + // perf: to prevent stringifying again in the call + args.filters = filters_str; + } + + return frappe + .xcall( + "frappe.client.validate_link_and_fetch", + { + ...args, + docname: value, + fields_to_fetch: columns_to_fetch, + }, + can_cache ? "GET" : "POST", + { cache: can_cache } + ) + .then((response) => { + if (!response) return; + + update_dependant_fields(response); + return response.name; + }); } fetch_map_for_quick_entry() { diff --git a/frappe/tests/test_client.py b/frappe/tests/test_client.py index 4b26900dfe..24bc03f9c3 100644 --- a/frappe/tests/test_client.py +++ b/frappe/tests/test_client.py @@ -155,30 +155,43 @@ class TestClient(IntegrationTestCase): self.assertEqual(get("ToDo", filters={}), get("ToDo", filters="{}")) todo.delete() - def test_client_validatate_link(self): - from frappe.client import validate_link + def test_client_validate_link_and_fetch(self): + from frappe.client import validate_link_and_fetch + # Use Role doctype (no custom query like User has) # Basic test - self.assertTrue(validate_link("User", "Guest")) + self.assertTrue(validate_link_and_fetch("Role", "System Manager")) # fixes capitalization if frappe.db.db_type == "mariadb": - self.assertEqual(validate_link("User", "GueSt"), {"name": "Guest"}) + self.assertEqual(validate_link_and_fetch("Role", "system manager"), {"name": "System Manager"}) # Fetch - self.assertEqual(validate_link("User", "Guest", fields=["enabled"]), {"name": "Guest", "enabled": 1}) + result = validate_link_and_fetch("Role", "System Manager", fields_to_fetch=["desk_access"]) + self.assertEqual(result.get("name"), "System Manager") + self.assertIn("desk_access", result) + + # Non-existent document returns empty + result = validate_link_and_fetch("Role", "Non Existent Role") + self.assertEqual(result, {}) + + # Filters - Role exists but filter excludes it + result = validate_link_and_fetch("Role", "System Manager", filters={"desk_access": 0}) + self.assertEqual(result, {}) + + # Filters - Role exists and filter matches + result = validate_link_and_fetch("Role", "System Manager", filters={"desk_access": 1}) + self.assertEqual(result.get("name"), "System Manager") # Permissions with self.set_user("Guest"), self.assertRaises(frappe.PermissionError): - self.assertEqual( - validate_link("User", "Guest", fields=["enabled"]), {"name": "Guest", "enabled": 1} - ) + validate_link_and_fetch("Role", "System Manager") - def test_validate_link_child_table(self): + def test_validate_link_and_fetch_for_child_table(self): """ - Test validate_link works for child table doctypes with field fetch. + Test validate_link_and_fetch works for child table doctypes with field fetch. """ - from frappe.client import validate_link + from frappe.client import validate_link_and_fetch self.addCleanup(frappe.db.rollback) @@ -188,7 +201,7 @@ class TestClient(IntegrationTestCase): child_row = user.block_modules[-1] - result = validate_link("Block Module", child_row.name, fields=["module"]) + result = validate_link_and_fetch("Block Module", child_row.name, fields_to_fetch=["module"]) self.assertEqual(result.get("name"), child_row.name) self.assertEqual(result.get("module"), "Setup")