From 039be73af4bba125f1a70a47b689e0830f6bf7f6 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 14 Jun 2023 10:46:25 +0530 Subject: [PATCH] refactor: Consider singles for dynamic set_value usage (#21367) Found all usage using this semgrep rule: ```yaml - pattern: frappe.db.set_value($DOCTYPE, ...) - pattern-not: frappe.db.set_value("$STR", ...) ``` --- frappe/core/doctype/doctype/doctype.json | 2 +- frappe/core/doctype/file/file.py | 19 +++++++++----- frappe/desk/doctype/todo/todo.py | 22 ++++++++++------ frappe/desk/like.py | 5 +++- frappe/model/base_document.py | 5 +++- frappe/model/document.py | 32 ++++++++++++++++-------- frappe/tests/test_assign.py | 13 +++++++--- frappe/tests/test_document.py | 7 ++++++ 8 files changed, 75 insertions(+), 30 deletions(-) diff --git a/frappe/core/doctype/doctype/doctype.json b/frappe/core/doctype/doctype/doctype.json index 842898d064..5209a408eb 100644 --- a/frappe/core/doctype/doctype/doctype.json +++ b/frappe/core/doctype/doctype/doctype.json @@ -755,4 +755,4 @@ "states": [], "track_changes": 1, "translated_doctype": 1 -} \ No newline at end of file +} diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index c4cefc7271..7770226e79 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -236,12 +236,19 @@ class File(Document): ): return - frappe.db.set_value( - self.attached_to_doctype, - self.attached_to_name, - self.attached_to_field, - self.file_url, - ) + if frappe.get_meta(self.attached_to_doctype).issingle: + frappe.db.set_single_value( + self.attached_to_doctype, + self.attached_to_field, + self.file_url, + ) + else: + frappe.db.set_value( + self.attached_to_doctype, + self.attached_to_name, + self.attached_to_field, + self.file_url, + ) def fetch_attached_to_field(self, old_file_url): if self.attached_to_field: diff --git a/frappe/desk/doctype/todo/todo.py b/frappe/desk/doctype/todo/todo.py index e076f3384a..46a826688a 100644 --- a/frappe/desk/doctype/todo/todo.py +++ b/frappe/desk/doctype/todo/todo.py @@ -81,13 +81,21 @@ class ToDo(Document): ) assignments.reverse() - frappe.db.set_value( - self.reference_type, - self.reference_name, - "_assign", - json.dumps(assignments), - update_modified=False, - ) + if frappe.get_meta(self.reference_type).issingle: + frappe.db.set_single_value( + self.reference_type, + "_assign", + json.dumps(assignments), + update_modified=False, + ) + else: + frappe.db.set_value( + self.reference_type, + self.reference_name, + "_assign", + json.dumps(assignments), + update_modified=False, + ) except Exception as e: if frappe.db.is_table_missing(e) and frappe.flags.in_install: diff --git a/frappe/desk/like.py b/frappe/desk/like.py index 0f297455e7..60eea3c525 100644 --- a/frappe/desk/like.py +++ b/frappe/desk/like.py @@ -52,7 +52,10 @@ def _toggle_like(doctype, name, add, user=None): liked_by.remove(user) remove_like(doctype, name) - frappe.db.set_value(doctype, name, "_liked_by", json.dumps(liked_by), update_modified=False) + if frappe.get_meta(doctype).issingle: + frappe.db.set_single_value(doctype, "_liked_by", json.dumps(liked_by), update_modified=False) + else: + frappe.db.set_value(doctype, name, "_liked_by", json.dumps(liked_by), update_modified=False) except frappe.db.ProgrammingError as e: if frappe.db.is_column_missing(e): diff --git a/frappe/model/base_document.py b/frappe/model/base_document.py index 63188e749d..609bfa4b9e 100644 --- a/frappe/model/base_document.py +++ b/frappe/model/base_document.py @@ -646,7 +646,10 @@ class BaseDocument: def update_modified(self): """Update modified timestamp""" self.set("modified", now()) - frappe.db.set_value(self.doctype, self.name, "modified", self.modified, update_modified=False) + if getattr(self.meta, "issingle", False): + frappe.db.set_single_value(self.doctype, "modified", self.modified, update_modified=False) + else: + frappe.db.set_value(self.doctype, self.name, "modified", self.modified, update_modified=False) def _fix_numeric_types(self): for df in self.meta.get("fields"): diff --git a/frappe/model/document.py b/frappe/model/document.py index 9496c09077..e2a55065bb 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -1123,7 +1123,7 @@ class Document(BaseDocument): def reset_seen(self): """Clear _seen property and set current user as seen""" - if getattr(self.meta, "track_seen", False): + if getattr(self.meta, "track_seen", False) and not getattr(self.meta, "issingle", False): frappe.db.set_value( self.doctype, self.name, "_seen", json.dumps([frappe.session.user]), update_modified=False ) @@ -1182,15 +1182,25 @@ class Document(BaseDocument): if self.name is None: return - frappe.db.set_value( - self.doctype, - self.name, - fieldname, - value, - self.modified, - self.modified_by, - update_modified=update_modified, - ) + if self.meta.issingle: + frappe.db.set_single_value( + self.doctype, + fieldname, + value, + modified=self.modified, + modified_by=self.modified_by, + update_modified=update_modified, + ) + else: + frappe.db.set_value( + self.doctype, + self.name, + fieldname, + value, + self.modified, + self.modified_by, + update_modified=update_modified, + ) self.run_method("on_change") @@ -1375,7 +1385,7 @@ class Document(BaseDocument): if not user: user = frappe.session.user - if self.meta.track_seen and not frappe.flags.read_only: + if self.meta.track_seen and not frappe.flags.read_only and not self.meta.issingle: _seen = self.get("_seen") or [] _seen = frappe.parse_json(_seen) diff --git a/frappe/tests/test_assign.py b/frappe/tests/test_assign.py index 4243bd080a..c299d235dc 100644 --- a/frappe/tests/test_assign.py +++ b/frappe/tests/test_assign.py @@ -23,16 +23,23 @@ class TestAssign(FrappeTestCase): if not frappe.db.exists("User", "test@example.com"): frappe.get_doc({"doctype": "User", "email": "test@example.com", "first_name": "Test"}).insert() - added = assign(todo, "test@example.com") + self._test_basic_assign_on_document(todo) + + def _test_basic_assign_on_document(self, doc): + added = assign(doc, "test@example.com") self.assertTrue("test@example.com" in [d.owner for d in added]) - frappe.desk.form.assign_to.remove(todo.doctype, todo.name, "test@example.com") + frappe.desk.form.assign_to.remove(doc.doctype, doc.name, "test@example.com") # assignment is cleared - assignments = frappe.desk.form.assign_to.get(dict(doctype=todo.doctype, name=todo.name)) + assignments = frappe.desk.form.assign_to.get(dict(doctype=doc.doctype, name=doc.name)) self.assertEqual(len(assignments), 0) + def test_assign_single(self): + c = frappe.get_doc("Contact Us Settings") + self._test_basic_assign_on_document(c) + def test_assignment_count(self): frappe.db.delete("ToDo") diff --git a/frappe/tests/test_document.py b/frappe/tests/test_document.py index 4e575528ab..cdd1d08c62 100644 --- a/frappe/tests/test_document.py +++ b/frappe/tests/test_document.py @@ -452,6 +452,13 @@ class TestDocument(FrappeTestCase): frappe.exceptions.InvalidDates, doc.validate_from_to_dates, "start_date", "end_date" ) + def test_db_set_singles(self): + c = frappe.get_doc("Contact Us Settings") + key, val = "email_id", "admin1@example.com" + c.db_set(key, val) + changed_val = frappe.db.get_single_value(c.doctype, key) + self.assertEqual(val, changed_val) + class TestDocumentWebView(FrappeTestCase): def get(self, path, user="Guest"):