From 44f603fc6e1be5495f76eb777042e09c1ae9d822 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 27 Aug 2021 20:32:23 +0530 Subject: [PATCH 01/36] refactor: frappe.db.set_value * Simplified logic * Perf enhancements (removed unnecessary conditional computations) * Use query builder and ORM to build queries instead of juggling --- frappe/database/database.py | 62 ++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 32 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 65242e0419..fc8bb8321d 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -677,46 +677,44 @@ class Database(object): :param debug: Print the query in the developer / js console. :param for_update: Will add a row-level lock to the value that is being set so that it can be released on commit. """ - if not modified: - modified = now() - if not modified_by: - modified_by = frappe.session.user + is_single_doctype = not (dn and dt != dn) + to_update = field if isinstance(field, dict) else {field: val} - to_update = {} if update_modified: - to_update = {"modified": modified, "modified_by": modified_by} + modified = modified or now() + modified_by = modified_by or frappe.session.user + to_update.update({"modified": modified, "modified_by": modified_by}) - if isinstance(field, dict): - to_update.update(field) - else: - to_update.update({field: val}) + if not is_single_doctype: + docnames = tuple(x[0] for x in self.get_values(dt, dn, 'name', debug=debug, for_update=for_update)) + if not docnames: + if debug: + print("Matched with no rows...exitting") + return - if dn and dt!=dn: - # with table - set_values = [] - for key in to_update: - set_values.append('`{0}`=%({0})s'.format(key)) + table = frappe.qb.DocType(dt) - for name in self.get_values(dt, dn, 'name', for_update=for_update, debug=debug): - values = dict(name=name[0]) - values.update(to_update) + query = frappe.qb.update(table) + for column, value in to_update.items(): + query = query.set(column, value) + query = query.where(table.name.isin(docnames)) + query.run(debug=debug) - self.sql("""update `tab{0}` - set {1} where name=%(name)s""".format(dt, ', '.join(set_values)), - values, debug=debug) + for d in docnames: + frappe.clear_document_cache(dt, d) - frappe.clear_document_cache(dt, values['name']) else: - # for singles - keys = list(to_update) - self.sql(''' - delete from `tabSingles` - where field in ({0}) and - doctype=%s'''.format(', '.join(['%s']*len(keys))), - list(keys) + [dt], debug=debug) - for key, value in to_update.items(): - self.sql('''insert into `tabSingles` (doctype, field, value) values (%s, %s, %s)''', - (dt, key, value), debug=debug) + frappe.db.delete( + "Singles", + filters={"field": ("in", tuple(to_update)), "doctype": dt}, debug=debug + ) + + singles_data = tuple((dt, key, value) for key, value in to_update.items()) + query = ( + frappe.qb.into("Singles") + .columns("doctype", "field", "value") + .insert(singles_data) + ).run(debug=debug) frappe.clear_document_cache(dt, dn) From aefb8e3b06019edf5e9bef9d9ff1dc38624117be Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 8 Oct 2021 11:00:55 +0530 Subject: [PATCH 02/36] fix: Expand iterable to get rid of extra brackets Although valid SQL, MariaDB didn't like double brackets on VALUES. It raised ERROR 1241: Operand should contain 1 column(s) --- frappe/database/database.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index fc8bb8321d..35ee503ace 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -709,11 +709,11 @@ class Database(object): filters={"field": ("in", tuple(to_update)), "doctype": dt}, debug=debug ) - singles_data = tuple((dt, key, value) for key, value in to_update.items()) + singles_data = ((dt, key, value) for key, value in to_update.items()) query = ( frappe.qb.into("Singles") .columns("doctype", "field", "value") - .insert(singles_data) + .insert(*singles_data) ).run(debug=debug) frappe.clear_document_cache(dt, dn) From 697d6f2d7aeb75663e5c9487273f50c1d3d9fb86 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 22 Oct 2021 19:19:54 +0530 Subject: [PATCH 03/36] fix: Run query even if no documents found for_update If no docnames are found, then use NULL as fallback. I tested this on PostgreSQL & MariaDB and it seems to work as expected In [1]: from frappe.query_builder import Field In [2]: frappe.db.set_value('ToDo', Field("creation") > Field("modified"), 'description', 'change 2', for_update=True, debug=True) SELECT "name" FROM "tabToDo" WHERE "creation">"modified" Execution time: 0.0 sec UPDATE "tabToDo" SET "description"='change 2',"modified"='2021-10-22 09:38:40.110481',"modified_by"='Administrator' WHERE "name" IN (NULL) Execution time: 0.0 sec --- frappe/database/database.py | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 35ee503ace..3de6c8b2ae 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -20,9 +20,9 @@ from frappe.utils import now, getdate, cast, get_datetime from frappe.model.utils.link_count import flush_local_link_count from frappe.query_builder.functions import Count from frappe.query_builder.functions import Min, Max, Avg, Sum -from frappe.query_builder.utils import Column +from frappe.query_builder.utils import Column, DocType from .query import Query -from pypika.terms import Criterion, PseudoColumn +from pypika.terms import Criterion, PseudoColumn, NullValue class Database(object): @@ -686,18 +686,20 @@ class Database(object): to_update.update({"modified": modified, "modified_by": modified_by}) if not is_single_doctype: - docnames = tuple(x[0] for x in self.get_values(dt, dn, 'name', debug=debug, for_update=for_update)) - if not docnames: - if debug: - print("Matched with no rows...exitting") - return + table = DocType(dt) - table = frappe.qb.DocType(dt) + if for_update: + docnames = tuple( + x[0] for x in self.get_values(dt, dn, "name", debug=debug, for_update=for_update) + ) or (NullValue(),) + query = frappe.qb.update(table).where(table.name.isin(docnames)) + + else: + query = self.query.build_conditions(table=dt, filters=dn, update=True) - query = frappe.qb.update(table) for column, value in to_update.items(): query = query.set(column, value) - query = query.where(table.name.isin(docnames)) + query.run(debug=debug) for d in docnames: From b0c30363cf4bc0e73711e29a6f676dc70b925147 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 22 Oct 2021 20:18:17 +0530 Subject: [PATCH 04/36] fix: Cast values as str for all single doctypes Re-arranged block for simplicity. Type casting doesn't change anything it seems. On get_value, values are casted properly. --- frappe/database/database.py | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 3de6c8b2ae..ee3ad175b1 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -685,7 +685,21 @@ class Database(object): modified_by = modified_by or frappe.session.user to_update.update({"modified": modified, "modified_by": modified_by}) - if not is_single_doctype: + if is_single_doctype: + frappe.db.delete( + "Singles", + filters={"field": ("in", tuple(to_update)), "doctype": dt}, debug=debug + ) + + singles_data = ((dt, key, str(value)) for key, value in to_update.items()) + query = ( + frappe.qb.into("Singles") + .columns("doctype", "field", "value") + .insert(*singles_data) + ).run(debug=debug) + frappe.clear_document_cache(dt, dn) + + else: table = DocType(dt) if for_update: @@ -702,24 +716,6 @@ class Database(object): query.run(debug=debug) - for d in docnames: - frappe.clear_document_cache(dt, d) - - else: - frappe.db.delete( - "Singles", - filters={"field": ("in", tuple(to_update)), "doctype": dt}, debug=debug - ) - - singles_data = ((dt, key, value) for key, value in to_update.items()) - query = ( - frappe.qb.into("Singles") - .columns("doctype", "field", "value") - .insert(*singles_data) - ).run(debug=debug) - - frappe.clear_document_cache(dt, dn) - if dt in self.value_cache: del self.value_cache[dt] From 123bcc95098612af1b013cccdd5eb70c2c9ee929 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 7 Jan 2022 14:16:45 +0530 Subject: [PATCH 05/36] fix: Clear cache on set_value --- frappe/database/database.py | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index ee3ad175b1..05169364f0 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -10,19 +10,20 @@ import re import string from contextlib import contextmanager from time import time -from typing import Dict, List, Union, Tuple +from typing import Dict, List, Tuple, Union + +from pypika.terms import Criterion, NullValue, PseudoColumn import frappe import frappe.defaults import frappe.model.meta from frappe import _ -from frappe.utils import now, getdate, cast, get_datetime from frappe.model.utils.link_count import flush_local_link_count from frappe.query_builder.functions import Count -from frappe.query_builder.functions import Min, Max, Avg, Sum -from frappe.query_builder.utils import Column, DocType +from frappe.query_builder.utils import DocType +from frappe.utils import cast, get_datetime, getdate, now + from .query import Query -from pypika.terms import Criterion, PseudoColumn, NullValue class Database(object): @@ -697,19 +698,26 @@ class Database(object): .columns("doctype", "field", "value") .insert(*singles_data) ).run(debug=debug) - frappe.clear_document_cache(dt, dn) + frappe.clear_document_cache(dt, dt) else: table = DocType(dt) if for_update: docnames = tuple( - x[0] for x in self.get_values(dt, dn, "name", debug=debug, for_update=for_update) + self.get_values(dt, dn, "name", debug=debug, for_update=for_update, pluck=True) ) or (NullValue(),) query = frappe.qb.update(table).where(table.name.isin(docnames)) + for docname in docnames: + frappe.clear_document_cache(dt, docname) + else: query = self.query.build_conditions(table=dt, filters=dn, update=True) + # TODO: Fix this; doesn't work rn - gavin@frappe.io + # frappe.cache().hdel_keys(dt, "document_cache") + # Workaround: clear all document caches + frappe.cache().delete_value('document_cache') for column, value in to_update.items(): query = query.set(column, value) @@ -719,7 +727,6 @@ class Database(object): if dt in self.value_cache: del self.value_cache[dt] - @staticmethod def set(doc, field, val): """Set value in document. **Avoid**""" From 31cc658e6d9d8febd06be34979198ea2cfe604ff Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 7 Jan 2022 14:17:28 +0530 Subject: [PATCH 06/36] fix(typing): Add type hints for frappe.cache --- frappe/__init__.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index 08c0f794b3..d07223a170 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -140,6 +140,8 @@ lang = local("lang") # This if block is never executed when running the code. It is only used for # telling static code analyzer where to find dynamically defined attributes. if typing.TYPE_CHECKING: + from frappe.utils.redis_wrapper import RedisWrapper + from frappe.database.mariadb.database import MariaDBDatabase from frappe.database.postgres.database import PostgresDatabase from frappe.query_builder.builder import MariaDB, Postgres @@ -147,6 +149,7 @@ if typing.TYPE_CHECKING: db: typing.Union[MariaDBDatabase, PostgresDatabase] qb: typing.Union[MariaDB, Postgres] + # end: static analysis hack def init(site, sites_path=None, new_site=False): @@ -310,7 +313,7 @@ def destroy(): # memcache redis_server = None -def cache(): +def cache() -> "RedisWrapper": """Returns redis connection.""" global redis_server if not redis_server: From 34a6f7adb5019a906d57ff44e1e604383d619bf9 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 7 Jan 2022 20:43:50 +0530 Subject: [PATCH 07/36] fix(qb): Patch ParameterizedValueWrapper as wrapper_cls --- frappe/query_builder/builder.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/frappe/query_builder/builder.py b/frappe/query_builder/builder.py index a65d50fdeb..8ae8324de4 100644 --- a/frappe/query_builder/builder.py +++ b/frappe/query_builder/builder.py @@ -1,8 +1,12 @@ from pypika import MySQLQuery, Order, PostgreSQLQuery, terms +from pypika.dialects import MySQLQueryBuilder, PostgreSQLQueryBuilder from pypika.queries import Schema, Table -from frappe.utils import get_table_name from pypika.terms import Function +from frappe.query_builder.terms import ParameterizedValueWrapper +from frappe.utils import get_table_name + + class Base: terms = terms desc = Order.desc @@ -34,6 +38,10 @@ class Base: class MariaDB(Base, MySQLQuery): Field = terms.Field + @classmethod + def _builder(cls, *args, **kwargs) -> "MySQLQueryBuilder": + return super()._builder(*args, wrapper_cls=ParameterizedValueWrapper, **kwargs) + @classmethod def from_(cls, table, *args, **kwargs): if isinstance(table, str): @@ -53,6 +61,10 @@ class Postgres(Base, PostgreSQLQuery): # they are two different objects. The quick fix used here is to replace the # Field names in the "Field" function. + @classmethod + def _builder(cls, *args, **kwargs) -> "PostgreSQLQueryBuilder": + return super()._builder(*args, wrapper_cls=ParameterizedValueWrapper, **kwargs) + @classmethod def Field(cls, field_name, *args, **kwargs): if field_name in cls.field_translation: From 6aa9a0bef5b6003df8de4729a41cc0c3d90c9786 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 7 Jan 2022 20:44:51 +0530 Subject: [PATCH 08/36] style: Sorted imports & whitespace consistency --- frappe/query_builder/utils.py | 10 +++++----- frappe/utils/__init__.py | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/frappe/query_builder/utils.py b/frappe/query_builder/utils.py index 2767e90242..7797ce856c 100644 --- a/frappe/query_builder/utils.py +++ b/frappe/query_builder/utils.py @@ -1,16 +1,16 @@ from enum import Enum -from typing import Any, Callable, Dict, Union, get_type_hints from importlib import import_module +from typing import Any, Callable, Dict, Union, get_type_hints from pypika import Query from pypika.queries import Column +from pypika.terms import PseudoColumn import frappe +from frappe.query_builder.terms import NamedParameterWrapper from .builder import MariaDB, Postgres -from pypika.terms import PseudoColumn -from frappe.query_builder.terms import NamedParameterWrapper class db_type_is(Enum): MARIADB = "mariadb" @@ -60,7 +60,7 @@ def patch_query_execute(): def prepare_query(query): params = {} - query = query.get_sql(param_wrapper = NamedParameterWrapper(params)) + query = query.get_sql(param_wrapper=NamedParameterWrapper(params)) if frappe.flags.in_safe_exec and not query.lower().strip().startswith("select"): raise frappe.PermissionError('Only SELECT SQL allowed in scripting') return query, params @@ -78,7 +78,7 @@ def patch_query_execute(): def patch_query_aggregation(): """Patch aggregation functions to frappe.qb """ - from frappe.query_builder.functions import _max, _min, _avg, _sum + from frappe.query_builder.functions import _avg, _max, _min, _sum frappe.qb.max = _max frappe.qb.min = _min diff --git a/frappe/utils/__init__.py b/frappe/utils/__init__.py index ffff7032aa..e3ddd5ae06 100644 --- a/frappe/utils/__init__.py +++ b/frappe/utils/__init__.py @@ -56,8 +56,8 @@ def get_email_address(user=None): def get_formatted_email(user, mail=None): """get Email Address of user formatted as: `John Doe `""" fullname = get_fullname(user) - method = get_hook_method('get_sender_details') + if method: sender_name, mail = method() # if method exists but sender_name is "" From 5de0b5cd352831700b449113ba80c677dd3896ce Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 10 Jan 2022 13:23:49 +0530 Subject: [PATCH 09/36] style: remove commented code, whitespaces test_file, added typing hint --- frappe/core/doctype/file/test_file.py | 27 ++------------------------- 1 file changed, 2 insertions(+), 25 deletions(-) diff --git a/frappe/core/doctype/file/test_file.py b/frappe/core/doctype/file/test_file.py index 2c1042e104..3ae5b87128 100644 --- a/frappe/core/doctype/file/test_file.py +++ b/frappe/core/doctype/file/test_file.py @@ -7,9 +7,8 @@ import frappe import os import unittest from frappe import _ -from frappe.core.doctype.file.file import get_attached_images, move_file, get_files_in_folder, unzip_file +from frappe.core.doctype.file.file import File, get_attached_images, move_file, get_files_in_folder, unzip_file from frappe.utils import get_files_path -# test_records = frappe.get_test_records('File') test_content1 = 'Hello' test_content2 = 'Hello World' @@ -24,8 +23,6 @@ def make_test_doc(): class TestSimpleFile(unittest.TestCase): - - def setUp(self): self.attached_to_doctype, self.attached_to_docname = make_test_doc() self.test_content = test_content1 @@ -38,21 +35,13 @@ class TestSimpleFile(unittest.TestCase): _file.save() self.saved_file_url = _file.file_url - def test_save(self): _file = frappe.get_doc("File", {"file_url": self.saved_file_url}) content = _file.get_content() self.assertEqual(content, self.test_content) - def tearDown(self): - # File gets deleted on rollback, so blank - pass - - class TestBase64File(unittest.TestCase): - - def setUp(self): self.attached_to_doctype, self.attached_to_docname = make_test_doc() self.test_content = base64.b64encode(test_content1.encode('utf-8')) @@ -66,18 +55,12 @@ class TestBase64File(unittest.TestCase): _file.save() self.saved_file_url = _file.file_url - def test_saved_content(self): _file = frappe.get_doc("File", {"file_url": self.saved_file_url}) content = _file.get_content() self.assertEqual(content, test_content1) - def tearDown(self): - # File gets deleted on rollback, so blank - pass - - class TestSameFileName(unittest.TestCase): def test_saved_content(self): self.attached_to_doctype, self.attached_to_docname = make_test_doc() @@ -130,8 +113,6 @@ class TestSameFileName(unittest.TestCase): class TestSameContent(unittest.TestCase): - - def setUp(self): self.attached_to_doctype1, self.attached_to_docname1 = make_test_doc() self.attached_to_doctype2, self.attached_to_docname2 = make_test_doc() @@ -186,10 +167,6 @@ class TestSameContent(unittest.TestCase): limit_property.delete() frappe.clear_cache(doctype='ToDo') - def tearDown(self): - # File gets deleted on rollback, so blank - pass - class TestFile(unittest.TestCase): def setUp(self): @@ -398,7 +375,7 @@ class TestFile(unittest.TestCase): def test_make_thumbnail(self): # test web image - test_file = frappe.get_doc({ + test_file: File = frappe.get_doc({ "doctype": "File", "file_name": 'logo', "file_url": frappe.utils.get_url('/_test/assets/image.jpg'), From 04e79eb0750bfb465391a4b102f0dd8f37c902f0 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 10 Jan 2022 13:25:47 +0530 Subject: [PATCH 10/36] fix(qb:ValueWrapper): Use get_value_sql only for str values --- frappe/query_builder/terms.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/frappe/query_builder/terms.py b/frappe/query_builder/terms.py index 2032cd8497..0e0e2c4800 100644 --- a/frappe/query_builder/terms.py +++ b/frappe/query_builder/terms.py @@ -21,7 +21,10 @@ class ParameterizedValueWrapper(ValueWrapper): sql = self.get_value_sql(quote_char=quote_char, secondary_quote_char=secondary_quote_char, **kwargs) return format_alias_sql(sql, self.alias, quote_char=quote_char, **kwargs) else: - value_sql = self.get_value_sql(quote_char=quote_char, **kwargs) if not isinstance(self.value,int) else self.value + if isinstance(self.value, str): + value_sql = self.get_value_sql(quote_char=quote_char, **kwargs) + else: + value_sql = self.value param_sql = param_wrapper.get_sql(**kwargs) param_wrapper.update_parameters(param_key=param_sql, param_value=value_sql, **kwargs) return format_alias_sql(param_sql, self.alias, quote_char=quote_char, **kwargs) From f6fba91fd203c33218f0f2fe64b4aaee3772f49f Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 10 Jan 2022 13:44:23 +0530 Subject: [PATCH 11/36] chore(typing): Add type hints in qb builder classes --- frappe/query_builder/builder.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frappe/query_builder/builder.py b/frappe/query_builder/builder.py index 8ae8324de4..d2fdeab324 100644 --- a/frappe/query_builder/builder.py +++ b/frappe/query_builder/builder.py @@ -1,6 +1,6 @@ from pypika import MySQLQuery, Order, PostgreSQLQuery, terms from pypika.dialects import MySQLQueryBuilder, PostgreSQLQueryBuilder -from pypika.queries import Schema, Table +from pypika.queries import QueryBuilder, Schema, Table from pypika.terms import Function from frappe.query_builder.terms import ParameterizedValueWrapper @@ -23,13 +23,13 @@ class Base: return Table(table_name, *args, **kwargs) @classmethod - def into(cls, table, *args, **kwargs): + def into(cls, table, *args, **kwargs) -> QueryBuilder: if isinstance(table, str): table = cls.DocType(table) return super().into(table, *args, **kwargs) @classmethod - def update(cls, table, *args, **kwargs): + def update(cls, table, *args, **kwargs) -> QueryBuilder: if isinstance(table, str): table = cls.DocType(table) return super().update(table, *args, **kwargs) From 30528080480fde90a78eadf1fc9f2b136e1f26b7 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 10 Jan 2022 15:05:57 +0530 Subject: [PATCH 12/36] fix: Don't cast to str if None or Falsy * refactor: use get_single_value instead of set_value(blah, None, blah1) * User.reload in test_home_page to combat what happens locally --- frappe/database/database.py | 2 +- frappe/tests/test_website.py | 1 + frappe/website/utils.py | 4 ++-- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 05169364f0..28ae177885 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -692,7 +692,7 @@ class Database(object): filters={"field": ("in", tuple(to_update)), "doctype": dt}, debug=debug ) - singles_data = ((dt, key, str(value)) for key, value in to_update.items()) + singles_data = ((dt, key, str(value) if value else value) for key, value in to_update.items()) query = ( frappe.qb.into("Singles") .columns("doctype", "field", "value") diff --git a/frappe/tests/test_website.py b/frappe/tests/test_website.py index 992d876243..cfadb09a04 100644 --- a/frappe/tests/test_website.py +++ b/frappe/tests/test_website.py @@ -20,6 +20,7 @@ class TestWebsite(unittest.TestCase): doctype='User', email='test-user-for-home-page@example.com', first_name='test')).insert(ignore_if_duplicate=True) + user.reload() role = frappe.get_doc(dict( doctype = 'Role', diff --git a/frappe/website/utils.py b/frappe/website/utils.py index cb8008277c..7beefc8164 100644 --- a/frappe/website/utils.py +++ b/frappe/website/utils.py @@ -88,7 +88,7 @@ def get_home_page(): # portal default if not home_page: - home_page = frappe.db.get_value("Portal Settings", None, "default_portal_home") + home_page = frappe.db.get_single_value("Portal Settings", "default_portal_home") # by hooks if not home_page: @@ -96,7 +96,7 @@ def get_home_page(): # global if not home_page: - home_page = frappe.db.get_value("Website Settings", None, "home_page") + home_page = frappe.db.get_single_value("Website Settings", "home_page") if not home_page: home_page = "login" if frappe.session.user == 'Guest' else "me" From f7cb090c5a763b1da05276d217ffda947ffa1e55 Mon Sep 17 00:00:00 2001 From: saxenabhishek Date: Mon, 10 Jan 2022 16:09:53 +0530 Subject: [PATCH 13/36] style: test_query_builder --- frappe/tests/test_query_builder.py | 33 +++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/frappe/tests/test_query_builder.py b/frappe/tests/test_query_builder.py index d2242cc6f7..ab1d8292b8 100644 --- a/frappe/tests/test_query_builder.py +++ b/frappe/tests/test_query_builder.py @@ -25,8 +25,14 @@ class TestCustomFunctionsMariaDB(unittest.TestCase): ) def test_constant_column(self): - query = frappe.qb.from_("DocType").select("name", ConstantColumn("John").as_("User")) - self.assertEqual(query.get_sql(), "SELECT `name`,'John' `User` FROM `tabDocType`") + query = frappe.qb.from_("DocType").select( + "name", ConstantColumn("John").as_("User") + ) + self.assertEqual( + query.get_sql(), "SELECT `name`,'John' `User` FROM `tabDocType`" + ) + + @run_only_if(db_type_is.POSTGRES) class TestCustomFunctionsPostgres(unittest.TestCase): def test_concat(self): @@ -39,8 +45,13 @@ class TestCustomFunctionsPostgres(unittest.TestCase): ) def test_constant_column(self): - query = frappe.qb.from_("DocType").select("name", ConstantColumn("John").as_("User")) - self.assertEqual(query.get_sql(), 'SELECT "name",\'John\' "User" FROM "tabDocType"') + query = frappe.qb.from_("DocType").select( + "name", ConstantColumn("John").as_("User") + ) + self.assertEqual( + query.get_sql(), 'SELECT "name",\'John\' "User" FROM "tabDocType"' + ) + class TestBuilderBase(object): def test_adding_tabs(self): @@ -56,12 +67,13 @@ class TestBuilderBase(object): self.assertIsInstance(data, list) def test_walk(self): - DocType = frappe.qb.DocType('DocType') + DocType = frappe.qb.DocType("DocType") query = ( frappe.qb.from_(DocType) .select(DocType.name) - .where((DocType.owner == "Administrator' --") - & (Coalesce(DocType.search_fields == "subject")) + .where( + (DocType.owner == "Administrator' --") + & (Coalesce(DocType.search_fields == "subject")) ) ) self.assertTrue("walk" in dir(query)) @@ -69,9 +81,9 @@ class TestBuilderBase(object): self.assertIn("%(param1)s", query) self.assertIn("%(param2)s", query) - self.assertIn("param1",params) - self.assertEqual(params["param1"],"Administrator' --") - self.assertEqual(params["param2"],"subject") + self.assertIn("param1", params) + self.assertEqual(params["param1"], "Administrator' --") + self.assertEqual(params["param2"], "subject") @run_only_if(db_type_is.MARIADB) @@ -84,6 +96,7 @@ class TestBuilderMaria(unittest.TestCase, TestBuilderBase): "SELECT * FROM `__Auth`", frappe.qb.from_("__Auth").select("*").get_sql() ) + @run_only_if(db_type_is.POSTGRES) class TestBuilderPostgres(unittest.TestCase, TestBuilderBase): def test_adding_tabs_in_from(self): From 479f637f7695e361ea1632114bbd71bf698bf6bd Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 10 Jan 2022 20:52:28 +0530 Subject: [PATCH 14/36] fix(single:set_value): Cast using sbool instead of str for all --- frappe/database/database.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 28ae177885..866814c094 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -21,7 +21,7 @@ from frappe import _ from frappe.model.utils.link_count import flush_local_link_count from frappe.query_builder.functions import Count from frappe.query_builder.utils import DocType -from frappe.utils import cast, get_datetime, getdate, now +from frappe.utils import cast, get_datetime, getdate, now, sbool from .query import Query @@ -692,7 +692,7 @@ class Database(object): filters={"field": ("in", tuple(to_update)), "doctype": dt}, debug=debug ) - singles_data = ((dt, key, str(value) if value else value) for key, value in to_update.items()) + singles_data = ((dt, key, sbool(value)) for key, value in to_update.items()) query = ( frappe.qb.into("Singles") .columns("doctype", "field", "value") From 202145f0ec558a06b64c31959237809a09929b38 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 10 Jan 2022 20:53:25 +0530 Subject: [PATCH 15/36] test: Add tests for frappe.db.set_value --- frappe/tests/test_db.py | 165 ++++++++++++++++++++++++++++++++++------ 1 file changed, 141 insertions(+), 24 deletions(-) diff --git a/frappe/tests/test_db.py b/frappe/tests/test_db.py index cdef4354ed..679ff73728 100644 --- a/frappe/tests/test_db.py +++ b/frappe/tests/test_db.py @@ -1,21 +1,21 @@ -# -*- coding: utf-8 -*- - -# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors +# Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE +import datetime +import inspect import unittest from random import choice -import datetime +from unittest.mock import patch import frappe from frappe.custom.doctype.custom_field.custom_field import create_custom_field -from frappe.utils import random_string -from frappe.utils.testutils import clear_custom_fields -from frappe.query_builder import Field from frappe.database import savepoint - -from .test_query_builder import run_only_if, db_type_is +from frappe.database.database import Database +from frappe.query_builder import Field from frappe.query_builder.functions import Concat_ws +from frappe.tests.test_query_builder import db_type_is, run_only_if +from frappe.utils import add_days, now, random_string +from frappe.utils.testutils import clear_custom_fields class TestDB(unittest.TestCase): @@ -84,20 +84,6 @@ class TestDB(unittest.TestCase): ), ) - def test_set_value(self): - todo1 = frappe.get_doc(dict(doctype='ToDo', description = 'test_set_value 1')).insert() - todo2 = frappe.get_doc(dict(doctype='ToDo', description = 'test_set_value 2')).insert() - - frappe.db.set_value('ToDo', todo1.name, 'description', 'test_set_value change 1') - self.assertEqual(frappe.db.get_value('ToDo', todo1.name, 'description'), 'test_set_value change 1') - - # multiple set-value - frappe.db.set_value('ToDo', dict(description=('like', '%test_set_value%')), - 'description', 'change 2') - - self.assertEqual(frappe.db.get_value('ToDo', todo1.name, 'description'), 'change 2') - self.assertEqual(frappe.db.get_value('ToDo', todo2.name, 'description'), 'change 2') - def test_escape(self): frappe.db.escape("香港濟生堂製藥有限公司 - IT".encode("utf-8")) @@ -246,7 +232,6 @@ class TestDB(unittest.TestCase): frappe.delete_doc(test_doctype, doc) clear_custom_fields(test_doctype) - def test_savepoints(self): frappe.db.rollback() save_point = "todonope" @@ -353,6 +338,138 @@ class TestDDLCommandsMaria(unittest.TestCase): self.assertEquals(len(indexs_in_table), 2) +class TestDBSetValue(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.todo1 = frappe.get_doc(doctype="ToDo", description="test_set_value 1").insert() + cls.todo2 = frappe.get_doc(doctype="ToDo", description="test_set_value 2").insert() + + def test_update_single_doctype_field(self): + value = frappe.db.get_single_value("System Settings", "deny_multiple_sessions") + changed_value = not value + + frappe.db.set_value("System Settings", "System Settings", "deny_multiple_sessions", changed_value) + current_value = frappe.db.get_single_value("System Settings", "deny_multiple_sessions") + self.assertEqual(current_value, changed_value) + + changed_value = not current_value + frappe.db.set_value("System Settings", None, "deny_multiple_sessions", changed_value) + current_value = frappe.db.get_single_value("System Settings", "deny_multiple_sessions") + self.assertEqual(current_value, changed_value) + + changed_value = not current_value + frappe.db.set_single_value("System Settings", "deny_multiple_sessions", changed_value) + current_value = frappe.db.get_single_value("System Settings", "deny_multiple_sessions") + self.assertEqual(current_value, changed_value) + + def test_update_single_row_single_column(self): + frappe.db.set_value("ToDo", self.todo1.name, "description", "test_set_value change 1") + updated_value = frappe.db.get_value("ToDo", self.todo1.name, "description") + self.assertEqual(updated_value, "test_set_value change 1") + + def test_update_single_row_multiple_columns(self): + description, status = "Upated by test_update_single_row_multiple_columns", "Closed" + + frappe.db.set_value("ToDo", self.todo1.name, { + "description": description, + "status": status, + }, update_modified=False) + + updated_desciption, updated_status = frappe.db.get_value("ToDo", + filters={"name": self.todo1.name}, + fieldname=["description", "status"] + ) + + self.assertEqual(description, updated_desciption) + self.assertEqual(status, updated_status) + + def test_update_multiple_rows_single_column(self): + frappe.db.set_value("ToDo", {"description": ("like", "%test_set_value%")}, "description", "change 2") + + self.assertEqual(frappe.db.get_value("ToDo", self.todo1.name, "description"), "change 2") + self.assertEqual(frappe.db.get_value("ToDo", self.todo2.name, "description"), "change 2") + + def test_update_multiple_rows_multiple_columns(self): + todos_to_update = frappe.get_all("ToDo", filters={ + "description": ("like", "%test_set_value%"), + "status": ("!=", "Closed") + }, pluck="name") + + frappe.db.set_value("ToDo", { + "description": ("like", "%test_set_value%"), + "status": ("!=", "Closed") + }, { + "status": "Closed", + "priority": "High" + }) + + test_result = frappe.get_all("ToDo", filters={"name": ("in", todos_to_update)}, fields=["status", "priority"]) + + self.assertTrue(all(x for x in test_result if x["status"] == "Closed")) + self.assertTrue(all(x for x in test_result if x["priority"] == "High")) + + def test_update_modified_options(self): + self.todo2.reload() + + todo = self.todo2 + updated_description = f"{todo.description} - by `test_update_modified_options`" + custom_modified = datetime.datetime.fromisoformat(add_days(now(), 10)) + custom_modified_by = "user_that_doesnt_exist@example.com" + + frappe.db.set_value("ToDo", todo.name, "description", updated_description, update_modified=False) + self.assertEqual(updated_description, frappe.db.get_value("ToDo", todo.name, "description")) + self.assertEqual(todo.modified, frappe.db.get_value("ToDo", todo.name, "modified")) + + frappe.db.set_value("ToDo", todo.name, "description", "test_set_value change 1", modified=custom_modified, modified_by=custom_modified_by) + self.assertTupleEqual( + (custom_modified, custom_modified_by), + frappe.db.get_value("ToDo", todo.name, ["modified", "modified_by"]) + ) + + def test_for_update(self): + self.todo1.reload() + + with patch.object(Database, "sql") as sql_called: + frappe.db.set_value( + self.todo1.doctype, + self.todo1.name, + "description", + f"{self.todo1.description}-edit by `test_for_update`" + ) + first_query = sql_called.call_args_list[0].args[0] + second_query = sql_called.call_args_list[1].args[0] + self.assertTrue(sql_called.call_count == 2) + self.assertTrue("FOR UPDATE" in first_query) + self.assertTrue("UPDATE `tabToDo` SET" in second_query) + + def test_cleared_cache(self): + self.todo2.reload() + + with patch.object(frappe, "clear_document_cache") as clear_cache: + frappe.db.set_value( + self.todo2.doctype, + self.todo2.name, + "description", + f"{self.todo2.description}-edit by `test_cleared_cache`" + ) + clear_cache.assert_called() + + def test_update_alias(self): + args = (self.todo1.doctype, self.todo1.name, "description", "Updated by `test_update_alias`") + kwargs = {"for_update": False, "modified": None, "modified_by": None, "update_modified": True, "debug": False} + + self.assertTrue("return self.set_value(" in inspect.getsource(frappe.db.update)) + + with patch.object(Database, "set_value") as set_value: + frappe.db.update(*args, **kwargs) + set_value.assert_called_once() + set_value.assert_called_with(*args, **kwargs) + + @classmethod + def tearDownClass(cls): + frappe.db.rollback() + + @run_only_if(db_type_is.POSTGRES) class TestDDLCommandsPost(unittest.TestCase): test_table_name = "TestNotes" From d81fe8699c4adfc06864dc3231c50af8b074058f Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 10 Jan 2022 20:53:54 +0530 Subject: [PATCH 16/36] feat: frappe.db.set_single_value Alias to frappe.db.set_value without having to pass the doctype name twice or teh second one as None Other changes: * set/get db APIs for single DocTypes * Make use of redundant cache key in frappe.db.get_single_value --- frappe/client.py | 1 - frappe/database/database.py | 16 +++++++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/frappe/client.py b/frappe/client.py index e835e7fee7..7280c29ba4 100644 --- a/frappe/client.py +++ b/frappe/client.py @@ -99,7 +99,6 @@ def get_value(doctype, fieldname, filters=None, as_dict=True, debug=False, paren if not filters: filters = None - if frappe.get_meta(doctype).issingle: value = frappe.db.get_values_from_single(fields, filters, doctype, as_dict=as_dict, debug=debug) else: diff --git a/frappe/database/database.py b/frappe/database/database.py index 866814c094..0a02380e05 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -556,6 +556,20 @@ class Database(object): def get_list(*args, **kwargs): return frappe.get_list(*args, **kwargs) + def set_single_value(self, doctype, fieldname, value, *args, **kwargs): + """Set field value of Single DocType. + + :param doctype: DocType of the single object + :param fieldname: `fieldname` of the property + :param value: `value` of the property + + Example: + + # Update the `deny_multiple_sessions` field in System Settings DocType. + company = frappe.db.set_single_value("System Settings", "deny_multiple_sessions", True) + """ + return self.set_value(doctype, doctype, fieldname, value, *args, **kwargs) + def get_single_value(self, doctype, fieldname, cache=False): """Get property of Single DocType. Cache locally by default @@ -571,7 +585,7 @@ class Database(object): if not doctype in self.value_cache: self.value_cache[doctype] = {} - if fieldname in self.value_cache[doctype]: + if cache and fieldname in self.value_cache[doctype]: return self.value_cache[doctype][fieldname] val = self.query.get_sql( From 15f3523c242200e4a4a914db4c68c82ea9602a7f Mon Sep 17 00:00:00 2001 From: saxenabhishek Date: Tue, 11 Jan 2022 00:04:14 +0530 Subject: [PATCH 17/36] test: qb parameterization where conditions set update conditions where with a function --- frappe/tests/test_query_builder.py | 37 ++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 7 deletions(-) diff --git a/frappe/tests/test_query_builder.py b/frappe/tests/test_query_builder.py index ab1d8292b8..bc98e166ea 100644 --- a/frappe/tests/test_query_builder.py +++ b/frappe/tests/test_query_builder.py @@ -66,24 +66,47 @@ class TestBuilderBase(object): self.assertIsInstance(query.run, Callable) self.assertIsInstance(data, list) - def test_walk(self): + +class TestParameterization(unittest.TestCase): + def test_where_conditions(self): DocType = frappe.qb.DocType("DocType") query = ( frappe.qb.from_(DocType) .select(DocType.name) - .where( - (DocType.owner == "Administrator' --") - & (Coalesce(DocType.search_fields == "subject")) - ) + .where((DocType.owner == "Administrator' --")) ) self.assertTrue("walk" in dir(query)) query, params = query.walk() self.assertIn("%(param1)s", query) - self.assertIn("%(param2)s", query) self.assertIn("param1", params) self.assertEqual(params["param1"], "Administrator' --") - self.assertEqual(params["param2"], "subject") + + def test_set_cnoditions(self): + DocType = frappe.qb.DocType("DocType") + query = frappe.qb.update(DocType).set(DocType.value, "some_value") + + self.assertTrue("walk" in dir(query)) + query, params = query.walk() + + self.assertIn("%(param1)s", query) + self.assertIn("param1", params) + self.assertEqual(params["param1"], "some_value") + + def test_where_conditions_functions(self): + DocType = frappe.qb.DocType("DocType") + query = ( + frappe.qb.from_(DocType) + .select(DocType.name) + .where(Coalesce(DocType.search_fields == "subject")) + ) + + self.assertTrue("walk" in dir(query)) + query, params = query.walk() + + self.assertIn("%(param1)s", query) + self.assertIn("param1", params) + self.assertEqual(params["param1"], "subject") @run_only_if(db_type_is.MARIADB) From 0f7539472045f6f0111411592d6fdbcfe5bd94de Mon Sep 17 00:00:00 2001 From: saxenabhishek Date: Tue, 11 Jan 2022 00:05:38 +0530 Subject: [PATCH 18/36] refactor: pythonic NamedParameterWrapper --- frappe/query_builder/terms.py | 13 ++++++++----- frappe/query_builder/utils.py | 6 +++--- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/frappe/query_builder/terms.py b/frappe/query_builder/terms.py index 0e0e2c4800..21cc557730 100644 --- a/frappe/query_builder/terms.py +++ b/frappe/query_builder/terms.py @@ -5,18 +5,21 @@ from pypika.utils import format_alias_sql class NamedParameterWrapper(): - def __init__(self, parameters: Dict[str, Any]): - self.parameters = parameters + def __init__(self) -> None: + self.parameters={} - def update_parameters(self, param_key: Any, param_value: Any, **kwargs): + def update_parameters(self, param_key: Any, param_value: Any, **kwargs)->None: self.parameters[param_key[2:-2]] = param_value - def get_sql(self, **kwargs): + def get_sql(self, **kwargs)->str: return f'%(param{len(self.parameters) + 1})s' + def get_parameters(self)->Dict[str, Any]: + return self.parameters + class ParameterizedValueWrapper(ValueWrapper): - def get_sql(self, quote_char: Optional[str] = None, secondary_quote_char: str = "'", param_wrapper= None, **kwargs: Any) -> str: + def get_sql(self, quote_char: Optional[str] = None, secondary_quote_char: str = "'", param_wrapper:NamedParameterWrapper = None, **kwargs: Any) -> str: if param_wrapper is None: sql = self.get_value_sql(quote_char=quote_char, secondary_quote_char=secondary_quote_char, **kwargs) return format_alias_sql(sql, self.alias, quote_char=quote_char, **kwargs) diff --git a/frappe/query_builder/utils.py b/frappe/query_builder/utils.py index 7797ce856c..cbd6147e01 100644 --- a/frappe/query_builder/utils.py +++ b/frappe/query_builder/utils.py @@ -59,11 +59,11 @@ def patch_query_execute(): return frappe.db.sql(query, params, *args, **kwargs) # nosemgrep def prepare_query(query): - params = {} - query = query.get_sql(param_wrapper=NamedParameterWrapper(params)) + param_collector = NamedParameterWrapper() + query = query.get_sql(param_wrapper=param_collector) if frappe.flags.in_safe_exec and not query.lower().strip().startswith("select"): raise frappe.PermissionError('Only SELECT SQL allowed in scripting') - return query, params + return query, param_collector.get_parameters() query_class = get_attr(str(frappe.qb).split("'")[1]) builder_class = get_type_hints(query_class._builder).get('return') From e2d49c17fffde5330f98a96ce11132d7f81a56b5 Mon Sep 17 00:00:00 2001 From: saxenabhishek Date: Tue, 11 Jan 2022 00:43:59 +0530 Subject: [PATCH 19/36] refactor: remove reduandant func call --- frappe/query_builder/terms.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/frappe/query_builder/terms.py b/frappe/query_builder/terms.py index 21cc557730..9cf2b08cf0 100644 --- a/frappe/query_builder/terms.py +++ b/frappe/query_builder/terms.py @@ -8,11 +8,10 @@ class NamedParameterWrapper(): def __init__(self) -> None: self.parameters={} - def update_parameters(self, param_key: Any, param_value: Any, **kwargs)->None: + def get_sql(self, param_value , **kwargs)->str: + param_key = f'%(param{len(self.parameters) + 1})s' self.parameters[param_key[2:-2]] = param_value - - def get_sql(self, **kwargs)->str: - return f'%(param{len(self.parameters) + 1})s' + return param_key def get_parameters(self)->Dict[str, Any]: return self.parameters @@ -24,12 +23,10 @@ class ParameterizedValueWrapper(ValueWrapper): sql = self.get_value_sql(quote_char=quote_char, secondary_quote_char=secondary_quote_char, **kwargs) return format_alias_sql(sql, self.alias, quote_char=quote_char, **kwargs) else: + value_sql = self.value if isinstance(self.value, str): value_sql = self.get_value_sql(quote_char=quote_char, **kwargs) - else: - value_sql = self.value - param_sql = param_wrapper.get_sql(**kwargs) - param_wrapper.update_parameters(param_key=param_sql, param_value=value_sql, **kwargs) + param_sql = param_wrapper.get_sql(param_value=value_sql, **kwargs) return format_alias_sql(param_sql, self.alias, quote_char=quote_char, **kwargs) From 471c0bd697404080e4c6df841ce2db02b88880d1 Mon Sep 17 00:00:00 2001 From: saxenabhishek Date: Tue, 11 Jan 2022 01:00:02 +0530 Subject: [PATCH 20/36] docs: Working of NamedParameters in qb --- frappe/query_builder/terms.py | 60 +++++++++++++++++++++++++++++------ 1 file changed, 50 insertions(+), 10 deletions(-) diff --git a/frappe/query_builder/terms.py b/frappe/query_builder/terms.py index 9cf2b08cf0..b0be40e0d2 100644 --- a/frappe/query_builder/terms.py +++ b/frappe/query_builder/terms.py @@ -4,33 +4,64 @@ from pypika.terms import Function, ValueWrapper from pypika.utils import format_alias_sql -class NamedParameterWrapper(): +class NamedParameterWrapper: + """Utility class to hold parameter values and keys""" + def __init__(self) -> None: - self.parameters={} + self.parameters = {} + + def get_sql(self, param_value: Any, **kwargs) -> str: + """returns SQL for a parameter, while adding the real value in a dict + + Args: + param_value (Any): Value of the parameter - def get_sql(self, param_value , **kwargs)->str: - param_key = f'%(param{len(self.parameters) + 1})s' + Returns: + str: parameter used in the SQL query + """ + param_key = f"%(param{len(self.parameters) + 1})s" self.parameters[param_key[2:-2]] = param_value return param_key - def get_parameters(self)->Dict[str, Any]: + def get_parameters(self) -> Dict[str, Any]: + """get dict with parameters and values + + Returns: + Dict[str, Any]: parameter dict + """ return self.parameters class ParameterizedValueWrapper(ValueWrapper): - def get_sql(self, quote_char: Optional[str] = None, secondary_quote_char: str = "'", param_wrapper:NamedParameterWrapper = None, **kwargs: Any) -> str: + """ + Class to monkey patch ValueWrapper + + Adds functionality to parameterize queries when a `param wrapper` is passed in get_sql() + """ + + def get_sql(self, quote_char: Optional[str] = None, secondary_quote_char: str = "'", param_wrapper: Optional[NamedParameterWrapper] = None, **kwargs: Any) -> str: if param_wrapper is None: - sql = self.get_value_sql(quote_char=quote_char, secondary_quote_char=secondary_quote_char, **kwargs) + sql = self.get_value_sql( + quote_char=quote_char, + secondary_quote_char=secondary_quote_char, + **kwargs, + ) return format_alias_sql(sql, self.alias, quote_char=quote_char, **kwargs) else: value_sql = self.value if isinstance(self.value, str): + # add quotes if it's a string value value_sql = self.get_value_sql(quote_char=quote_char, **kwargs) param_sql = param_wrapper.get_sql(param_value=value_sql, **kwargs) return format_alias_sql(param_sql, self.alias, quote_char=quote_char, **kwargs) class ParameterizedFunction(Function): + """ + Class to monkey patch pypika.terms.Functions + + Only to pass `param_wrapper` in `get_function_sql`. + """ def get_sql(self, **kwargs: Any) -> str: with_alias = kwargs.pop("with_alias", False) with_namespace = kwargs.pop("with_namespace", False) @@ -38,15 +69,24 @@ class ParameterizedFunction(Function): dialect = kwargs.pop("dialect", None) param_wrapper = kwargs.pop("param_wrapper", None) - function_sql = self.get_function_sql(with_namespace=with_namespace, quote_char=quote_char, param_wrapper=param_wrapper, dialect=dialect) + function_sql = self.get_function_sql( + with_namespace=with_namespace, + quote_char=quote_char, + param_wrapper=param_wrapper, + dialect=dialect, + ) if self.schema is not None: function_sql = "{schema}.{function}".format( - schema=self.schema.get_sql(quote_char=quote_char, dialect=dialect, **kwargs), + schema=self.schema.get_sql( + quote_char=quote_char, dialect=dialect, **kwargs + ), function=function_sql, ) if with_alias: - return format_alias_sql(function_sql, self.alias, quote_char=quote_char, **kwargs) + return format_alias_sql( + function_sql, self.alias, quote_char=quote_char, **kwargs + ) return function_sql From 9f1503c5821c0601e1845248c6357c6682cedb9e Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 11 Jan 2022 12:54:18 +0530 Subject: [PATCH 21/36] test: Postgres compatible db tests --- frappe/tests/test_db.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/frappe/tests/test_db.py b/frappe/tests/test_db.py index 679ff73728..6998468bfe 100644 --- a/frappe/tests/test_db.py +++ b/frappe/tests/test_db.py @@ -438,9 +438,14 @@ class TestDBSetValue(unittest.TestCase): ) first_query = sql_called.call_args_list[0].args[0] second_query = sql_called.call_args_list[1].args[0] + self.assertTrue(sql_called.call_count == 2) self.assertTrue("FOR UPDATE" in first_query) - self.assertTrue("UPDATE `tabToDo` SET" in second_query) + if frappe.conf.db_type == "postgres": + from frappe.database.postgres.database import modify_query + self.assertTrue(modify_query("UPDATE `tabToDo` SET") in second_query) + if frappe.conf.db_type == "mariadb": + self.assertTrue("UPDATE `tabToDo` SET" in second_query) def test_cleared_cache(self): self.todo2.reload() From f5072af00da03c9fe7aca71e0c98b282fff54c8f Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 11 Jan 2022 12:54:48 +0530 Subject: [PATCH 22/36] fix: sbool before cint in cast for Check types Noticed an issue when get_single_value wasn't returning the correct values; by converting true to 0. Tested it out. Here's the examples: In [2]: sbool("2") Out[2]: '2' In [3]: cint(sbool("2")) Out[3]: 2 In [4]: cint(sbool("-1")) Out[4]: -1 In [5]: cint(sbool("0")) Out[5]: 0 In [6]: cint(sbool("1000")) Out[6]: 1000 In [7]: cint(sbool("10_000")) Out[7]: 10000 In [8]: cint(sbool("true")) Out[8]: 1 --- frappe/utils/data.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/utils/data.py b/frappe/utils/data.py index 545d49054a..313816557d 100644 --- a/frappe/utils/data.py +++ b/frappe/utils/data.py @@ -607,7 +607,7 @@ def cast(fieldtype, value=None): value = flt(value) elif fieldtype in ("Int", "Check"): - value = cint(value) + value = cint(sbool(value)) elif fieldtype in ("Data", "Text", "Small Text", "Long Text", "Text Editor", "Select", "Link", "Dynamic Link"): From ece1564e4d05ebc225bdc039513e449085780fc9 Mon Sep 17 00:00:00 2001 From: saxenabhishek Date: Tue, 11 Jan 2022 15:32:07 +0530 Subject: [PATCH 23/36] fix: replace the field method with a column --- frappe/query_builder/__init__.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/frappe/query_builder/__init__.py b/frappe/query_builder/__init__.py index bf7be84c51..a49c617ecf 100644 --- a/frappe/query_builder/__init__.py +++ b/frappe/query_builder/__init__.py @@ -1,8 +1,18 @@ -from frappe.query_builder.terms import ParameterizedValueWrapper, ParameterizedFunction -import pypika +import pypika.terms +from pypika import * + +from frappe.query_builder.terms import ParameterizedFunction, ParameterizedValueWrapper +from frappe.query_builder.utils import ( + Column, + DocType, + get_query_builder, + patch_query_aggregation, + patch_query_execute, +) pypika.terms.ValueWrapper = ParameterizedValueWrapper pypika.terms.Function = ParameterizedFunction -from pypika import * -from frappe.query_builder.utils import Column, DocType, get_query_builder, patch_query_execute, patch_query_aggregation +# * Overrides the field() method and replaces it with the a `PseudoColumn` 'field' for consistency +pypika.queries.Selectable.__getattr__ = lambda table, x: Field(x, table=table) +pypika.queries.Selectable.field = pypika.terms.PseudoColumn("field") From b64dc6508801aea8bb40f02190807de59c35b7fc Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 11 Jan 2022 16:25:10 +0530 Subject: [PATCH 24/36] fix: Set cache=True to maintain old behaviour consistency --- frappe/database/database.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 0a02380e05..3b6d2d47b5 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -570,7 +570,7 @@ class Database(object): """ return self.set_value(doctype, doctype, fieldname, value, *args, **kwargs) - def get_single_value(self, doctype, fieldname, cache=False): + def get_single_value(self, doctype, fieldname, cache=True): """Get property of Single DocType. Cache locally by default :param doctype: DocType of the single object whose value is requested From 9121014f488aa82a0fd38222ad2f85ea80ba73c9 Mon Sep 17 00:00:00 2001 From: saxenabhishek Date: Tue, 11 Jan 2022 16:53:42 +0530 Subject: [PATCH 25/36] fix: ignore copy of getattr methods --- frappe/query_builder/__init__.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/frappe/query_builder/__init__.py b/frappe/query_builder/__init__.py index a49c617ecf..5b58e70c4e 100644 --- a/frappe/query_builder/__init__.py +++ b/frappe/query_builder/__init__.py @@ -1,5 +1,7 @@ import pypika.terms from pypika import * +from pypika import Field +from pypika.utils import ignore_copy from frappe.query_builder.terms import ParameterizedFunction, ParameterizedValueWrapper from frappe.query_builder.utils import ( @@ -14,5 +16,6 @@ pypika.terms.ValueWrapper = ParameterizedValueWrapper pypika.terms.Function = ParameterizedFunction # * Overrides the field() method and replaces it with the a `PseudoColumn` 'field' for consistency -pypika.queries.Selectable.__getattr__ = lambda table, x: Field(x, table=table) +pypika.queries.Selectable.__getattr__ = ignore_copy(lambda table, x: Field(x, table=table)) +pypika.queries.Selectable.__getitem__ = ignore_copy(lambda table, x: Field(x, table=table)) pypika.queries.Selectable.field = pypika.terms.PseudoColumn("field") From 02bdc35490fff1e2b0c31c1ac08bcf0151873807 Mon Sep 17 00:00:00 2001 From: saxenabhishek Date: Sat, 15 Jan 2022 14:55:28 +0530 Subject: [PATCH 26/36] fix: make parameters for strings only --- frappe/query_builder/terms.py | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/frappe/query_builder/terms.py b/frappe/query_builder/terms.py index b0be40e0d2..71f60af656 100644 --- a/frappe/query_builder/terms.py +++ b/frappe/query_builder/terms.py @@ -39,21 +39,24 @@ class ParameterizedValueWrapper(ValueWrapper): Adds functionality to parameterize queries when a `param wrapper` is passed in get_sql() """ - def get_sql(self, quote_char: Optional[str] = None, secondary_quote_char: str = "'", param_wrapper: Optional[NamedParameterWrapper] = None, **kwargs: Any) -> str: - if param_wrapper is None: + def get_sql( + self, + quote_char: Optional[str] = None, + secondary_quote_char: str = "'", + param_wrapper: Optional[NamedParameterWrapper] = None, + **kwargs: Any, + ) -> str: + if param_wrapper and isinstance(self.value, str): + # add quotes if it's a string value + value_sql = self.get_value_sql(quote_char=quote_char, **kwargs) + sql = param_wrapper.get_sql(param_value=value_sql, **kwargs) + else: sql = self.get_value_sql( quote_char=quote_char, secondary_quote_char=secondary_quote_char, **kwargs, ) - return format_alias_sql(sql, self.alias, quote_char=quote_char, **kwargs) - else: - value_sql = self.value - if isinstance(self.value, str): - # add quotes if it's a string value - value_sql = self.get_value_sql(quote_char=quote_char, **kwargs) - param_sql = param_wrapper.get_sql(param_value=value_sql, **kwargs) - return format_alias_sql(param_sql, self.alias, quote_char=quote_char, **kwargs) + return format_alias_sql(sql, self.alias, quote_char=quote_char, **kwargs) class ParameterizedFunction(Function): @@ -62,6 +65,7 @@ class ParameterizedFunction(Function): Only to pass `param_wrapper` in `get_function_sql`. """ + def get_sql(self, **kwargs: Any) -> str: with_alias = kwargs.pop("with_alias", False) with_namespace = kwargs.pop("with_namespace", False) From b403d67845770c187d8b6772441e1903d6e04677 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 17 Jan 2022 14:53:35 +0530 Subject: [PATCH 27/36] fix!: Don't use strtobool in sbool util strtobool would convert 't', 'yes' to True too. Which shouldn't be a problem generally but could be one possibly. --- frappe/utils/data.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/frappe/utils/data.py b/frappe/utils/data.py index 59ce135b90..764df27187 100644 --- a/frappe/utils/data.py +++ b/frappe/utils/data.py @@ -737,12 +737,15 @@ def sbool(x): x (str): String to be converted to Bool Returns: - object: Returns Boolean or type(x) + object: Returns Boolean or x """ - from distutils.util import strtobool - try: - return bool(strtobool(x)) + val = x.lower() + if val in ('true', '1'): + return True + elif val in ('false', '0'): + return False + return bool(x) except Exception: return x From 429f839ea3a79b846ce17a85c7ab4a3c798f5181 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 17 Jan 2022 15:07:44 +0530 Subject: [PATCH 28/36] style: Add typing, sorted imports# --- frappe/__init__.py | 1 - frappe/utils/data.py | 59 ++++++++++++++++++++++++++------------------ 2 files changed, 35 insertions(+), 25 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index f93fa538bb..86e5a03359 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -314,7 +314,6 @@ def destroy(): release_local(local) -# memcache redis_server = None def cache() -> "RedisWrapper": """Returns redis connection.""" diff --git a/frappe/utils/data.py b/frappe/utils/data.py index 764df27187..8148d194c6 100644 --- a/frappe/utils/data.py +++ b/frappe/utils/data.py @@ -1,17 +1,22 @@ -# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors +# Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE -from typing import Optional -import frappe -import operator -import json import base64 -import re, datetime, math, time +import datetime +import json +import math +import operator +import re +import time from code import compile_command +from enum import Enum +from typing import Any, Dict, List, Optional, Tuple, Union from urllib.parse import quote, urljoin -from frappe.desk.utils import slug + from click import secho -from enum import Enum + +import frappe +from frappe.desk.utils import slug DATE_FORMAT = "%Y-%m-%d" TIME_FORMAT = "%H:%M:%S.%f" @@ -201,7 +206,7 @@ def get_time_zone(): return frappe.cache().get_value("time_zone", _get_time_zone) def convert_utc_to_timezone(utc_timestamp, time_zone): - from pytz import timezone, UnknownTimeZoneError + from pytz import UnknownTimeZoneError, timezone utcnow = timezone('UTC').localize(utc_timestamp) try: return utcnow.astimezone(timezone(time_zone)) @@ -726,7 +731,7 @@ def ceil(s): def cstr(s, encoding='utf-8'): return frappe.as_unicode(s, encoding) -def sbool(x): +def sbool(x: str) -> Union[bool, Any]: """Converts str object to Boolean if possible. Example: "true" becomes True @@ -920,13 +925,13 @@ number_format_info = { "#.########": (".", "", 8) } -def get_number_format_info(format): +def get_number_format_info(format: str) -> Tuple[str, str, int]: return number_format_info.get(format) or (".", ",", 2) # # convert currency to words # -def money_in_words(number, main_currency = None, fraction_currency=None): +def money_in_words(number: str, main_currency: Optional[str] = None, fraction_currency: Optional[str] = None): """ Returns string in words with currency and fraction currency. """ @@ -1012,9 +1017,11 @@ def is_image(filepath): def get_thumbnail_base64_for_image(src): from os.path import exists as file_exists + from PIL import Image + + from frappe import cache, safe_decode from frappe.core.doctype.file.file import get_local_image - from frappe import safe_decode, cache if not src: frappe.throw('Invalid source for image: {0}'.format(src)) @@ -1305,7 +1312,7 @@ operator_map = { "None": lambda a, b: (not a) and True or False } -def evaluate_filters(doc, filters): +def evaluate_filters(doc, filters: Union[Dict, List, Tuple]): '''Returns true if doc matches filters''' if isinstance(filters, dict): for key, value in filters.items(): @@ -1322,7 +1329,7 @@ def evaluate_filters(doc, filters): return True -def compare(val1, condition, val2, fieldtype=None): +def compare(val1: Any, condition: str, val2: Any, fieldtype: Optional[str] = None): ret = False if fieldtype: val2 = cast(fieldtype, val2) @@ -1331,7 +1338,7 @@ def compare(val1, condition, val2, fieldtype=None): return ret -def get_filter(doctype, f, filters_config=None): +def get_filter(doctype: str, f: Union[Dict, List, Tuple], filters_config=None) -> "frappe._dict": """Returns a _dict like { @@ -1418,8 +1425,10 @@ def make_filter_dict(filters): return _filter def sanitize_column(column_name): - from frappe import _ import sqlparse + + from frappe import _ + regex = re.compile("^.*[,'();].*") column_name = sqlparse.format(column_name, strip_comments=True, keyword_case="lower") blacklisted_keywords = ['select', 'create', 'insert', 'delete', 'drop', 'update', 'case', 'and', 'or'] @@ -1495,9 +1504,10 @@ def strip(val, chars=None): return (val or "").replace("\ufeff", "").replace("\u200b", "").strip(chars) def to_markdown(html): - from html2text import html2text from html.parser import HTMLParser + from html2text import html2text + text = None try: text = html2text(html or '') @@ -1507,7 +1517,8 @@ def to_markdown(html): return text def md_to_html(markdown_text): - from markdown2 import markdown as _markdown, MarkdownError + from markdown2 import MarkdownError + from markdown2 import markdown as _markdown extras = { 'fenced-code-blocks': None, @@ -1532,14 +1543,14 @@ def md_to_html(markdown_text): def markdown(markdown_text): return md_to_html(markdown_text) -def is_subset(list_a, list_b): +def is_subset(list_a: List, list_b: List) -> bool: '''Returns whether list_a is a subset of list_b''' return len(list(set(list_a) & set(list_b))) == len(list_a) -def generate_hash(*args, **kwargs): +def generate_hash(*args, **kwargs) -> str: return frappe.generate_hash(*args, **kwargs) -def guess_date_format(date_string): +def guess_date_format(date_string: str) -> str: DATE_FORMATS = [ r"%d/%b/%y", r"%d-%m-%Y", @@ -1614,13 +1625,13 @@ def guess_date_format(date_string): if date_format and time_format: return (date_format + ' ' + time_format).strip() -def validate_json_string(string): +def validate_json_string(string: str) -> None: try: json.loads(string) except (TypeError, ValueError): raise frappe.ValidationError -def get_user_info_for_avatar(user_id): +def get_user_info_for_avatar(user_id: str) -> Dict: user_info = { "email": user_id, "image": "", From efd5c197cbec0a483eccf2e2def7f33e4876ea08 Mon Sep 17 00:00:00 2001 From: saxenabhishek Date: Tue, 25 Jan 2022 03:07:34 +0530 Subject: [PATCH 29/36] fix: sbool converting int stored as string --- frappe/utils/data.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/utils/data.py b/frappe/utils/data.py index 8148d194c6..b13044b2e6 100644 --- a/frappe/utils/data.py +++ b/frappe/utils/data.py @@ -750,7 +750,7 @@ def sbool(x: str) -> Union[bool, Any]: return True elif val in ('false', '0'): return False - return bool(x) + return x except Exception: return x From 97d6a9641911de8e895ea7e02c8e41edec4a83ae Mon Sep 17 00:00:00 2001 From: saxenabhishek Date: Tue, 25 Jan 2022 13:28:11 +0530 Subject: [PATCH 30/36] fix: timedelta parsing in pypika --- frappe/query_builder/terms.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/frappe/query_builder/terms.py b/frappe/query_builder/terms.py index 71f60af656..f9751612b6 100644 --- a/frappe/query_builder/terms.py +++ b/frappe/query_builder/terms.py @@ -1,3 +1,4 @@ +from datetime import timedelta from typing import Any, Dict, Optional from pypika.terms import Function, ValueWrapper @@ -51,6 +52,9 @@ class ParameterizedValueWrapper(ValueWrapper): value_sql = self.get_value_sql(quote_char=quote_char, **kwargs) sql = param_wrapper.get_sql(param_value=value_sql, **kwargs) else: + # * BUG: pypika doesen't parse timedeltas + if isinstance(self.value, timedelta): + self.value = str(self.value) sql = self.get_value_sql( quote_char=quote_char, secondary_quote_char=secondary_quote_char, From 1741c478261f6ecc39d8b7adb976c35e243fe147 Mon Sep 17 00:00:00 2001 From: saxenabhishek Date: Tue, 25 Jan 2022 15:26:16 +0530 Subject: [PATCH 31/36] test: avoid parameterization of qb objects --- frappe/tests/test_query_builder.py | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/frappe/tests/test_query_builder.py b/frappe/tests/test_query_builder.py index bc98e166ea..bb7a883b5b 100644 --- a/frappe/tests/test_query_builder.py +++ b/frappe/tests/test_query_builder.py @@ -5,7 +5,7 @@ import frappe from frappe.query_builder.custom import ConstantColumn from frappe.query_builder.functions import Coalesce, GroupConcat, Match from frappe.query_builder.utils import db_type_is - +from frappe.query_builder import Case def run_only_if(dbtype: db_type_is) -> Callable: return unittest.skipIf( @@ -108,6 +108,27 @@ class TestParameterization(unittest.TestCase): self.assertIn("param1", params) self.assertEqual(params["param1"], "subject") + def test_case(self): + DocType = frappe.qb.DocType("DocType") + query = ( + frappe.qb.from_(DocType) + .select( + Case() + .when(DocType.search_fields == "value", "other_value") + .when(Coalesce(DocType.search_fields == "subject_in_function"), "true_value") + ) + ) + + self.assertTrue("walk" in dir(query)) + query, params = query.walk() + + self.assertIn("%(param1)s", query) + self.assertIn("param1", params) + self.assertEqual(params["param1"], "value") + self.assertEqual(params["param2"], "other_value") + self.assertEqual(params["param3"], "subject_in_function") + self.assertEqual(params["param4"], "true_value") + @run_only_if(db_type_is.MARIADB) class TestBuilderMaria(unittest.TestCase, TestBuilderBase): From 9091b2a037635015bbe16ca205047cd2aaa3a8bc Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 28 Jan 2022 18:31:35 +0530 Subject: [PATCH 32/36] feat(minor): Case option in run-tests for specifying TestCase --- frappe/commands/utils.py | 5 +++-- frappe/test_runner.py | 17 ++++++++++------- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/frappe/commands/utils.py b/frappe/commands/utils.py index 41b607b192..e3379a43aa 100644 --- a/frappe/commands/utils.py +++ b/frappe/commands/utils.py @@ -623,6 +623,7 @@ def transform_database(context, table, engine, row_format, failfast): @click.command('run-tests') @click.option('--app', help="For App") @click.option('--doctype', help="For DocType") +@click.option('--case', help="Select particular TestCase") @click.option('--doctype-list-path', help="Path to .txt file for list of doctypes. Example erpnext/tests/server/agriculture.txt") @click.option('--test', multiple=True, help="Specific test") @click.option('--ui-tests', is_flag=True, default=False, help="Run UI Tests") @@ -636,7 +637,7 @@ def transform_database(context, table, engine, row_format, failfast): @pass_context def run_tests(context, app=None, module=None, doctype=None, test=(), profile=False, coverage=False, junit_xml_output=False, ui_tests = False, doctype_list_path=None, - skip_test_records=False, skip_before_tests=False, failfast=False): + skip_test_records=False, skip_before_tests=False, failfast=False, case=None): with CodeCoverage(coverage, app): import frappe.test_runner @@ -658,7 +659,7 @@ def run_tests(context, app=None, module=None, doctype=None, test=(), profile=Fal ret = frappe.test_runner.main(app, module, doctype, context.verbose, tests=tests, force=context.force, profile=profile, junit_xml_output=junit_xml_output, - ui_tests=ui_tests, doctype_list_path=doctype_list_path, failfast=failfast) + ui_tests=ui_tests, doctype_list_path=doctype_list_path, failfast=failfast, case=case) if len(ret.failures) == 0 and len(ret.errors) == 0: ret = 0 diff --git a/frappe/test_runner.py b/frappe/test_runner.py index 1839f15ae8..05f1ce1cd7 100644 --- a/frappe/test_runner.py +++ b/frappe/test_runner.py @@ -30,7 +30,7 @@ def xmlrunner_wrapper(output): def main(app=None, module=None, doctype=None, verbose=False, tests=(), force=False, profile=False, junit_xml_output=None, ui_tests=False, - doctype_list_path=None, skip_test_records=False, failfast=False): + doctype_list_path=None, skip_test_records=False, failfast=False, case=None): global unittest_runner if doctype_list_path: @@ -76,7 +76,7 @@ def main(app=None, module=None, doctype=None, verbose=False, tests=(), if doctype: ret = run_tests_for_doctype(doctype, verbose, tests, force, profile, failfast=failfast, junit_xml_output=junit_xml_output) elif module: - ret = run_tests_for_module(module, verbose, tests, profile, failfast=failfast, junit_xml_output=junit_xml_output) + ret = run_tests_for_module(module, verbose, tests, profile, failfast=failfast, junit_xml_output=junit_xml_output, case=case) else: ret = run_all_tests(app, verbose, profile, ui_tests, failfast=failfast, junit_xml_output=junit_xml_output) @@ -182,16 +182,16 @@ def run_tests_for_doctype(doctypes, verbose=False, tests=(), force=False, profil return _run_unittest(modules, verbose=verbose, tests=tests, profile=profile, failfast=failfast, junit_xml_output=junit_xml_output) -def run_tests_for_module(module, verbose=False, tests=(), profile=False, failfast=False, junit_xml_output=False): +def run_tests_for_module(module, verbose=False, tests=(), profile=False, failfast=False, junit_xml_output=False, case=None): module = importlib.import_module(module) if hasattr(module, "test_dependencies"): for doctype in module.test_dependencies: make_test_records(doctype, verbose=verbose) frappe.db.commit() - return _run_unittest(module, verbose=verbose, tests=tests, profile=profile, failfast=failfast, junit_xml_output=junit_xml_output) + return _run_unittest(module, verbose=verbose, tests=tests, profile=profile, failfast=failfast, junit_xml_output=junit_xml_output, case=case) -def _run_unittest(modules, verbose=False, tests=(), profile=False, failfast=False, junit_xml_output=False): +def _run_unittest(modules, verbose=False, tests=(), profile=False, failfast=False, junit_xml_output=False, case=None): frappe.db.begin() test_suite = unittest.TestSuite() @@ -200,7 +200,10 @@ def _run_unittest(modules, verbose=False, tests=(), profile=False, failfast=Fals modules = [modules] for module in modules: - module_test_cases = unittest.TestLoader().loadTestsFromModule(module) + if case: + module_test_cases = unittest.TestLoader().loadTestsFromTestCase(getattr(module, case)) + else: + module_test_cases = unittest.TestLoader().loadTestsFromModule(module) if tests: for each in module_test_cases: for test_case in each.__dict__["_tests"]: @@ -337,7 +340,7 @@ def make_test_records_for_doctype(doctype, verbose=0, force=False): elif hasattr(test_module, "test_records"): if doctype in frappe.local.test_objects: frappe.local.test_objects[doctype] += make_test_objects(doctype, test_module.test_records, verbose, force) - else: + else: frappe.local.test_objects[doctype] = make_test_objects(doctype, test_module.test_records, verbose, force) else: From 8037866dc1d79476e1dd6607a85f3eb79edfbdd0 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 28 Jan 2022 18:33:03 +0530 Subject: [PATCH 33/36] fix: Handle parsing and formatting timedeltas * Added utils parse_timedelta, format_timedelta * Added to json_handler for de-serializing timedelta objects --- frappe/utils/__init__.py | 2 +- frappe/utils/data.py | 37 +++++++++++++++++++++++++++++++++++-- frappe/utils/formatters.py | 9 +++++++-- frappe/utils/response.py | 14 ++++++++------ 4 files changed, 51 insertions(+), 11 deletions(-) diff --git a/frappe/utils/__init__.py b/frappe/utils/__init__.py index a275c4f64b..fb2e91a262 100644 --- a/frappe/utils/__init__.py +++ b/frappe/utils/__init__.py @@ -1,4 +1,4 @@ -# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors +# Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE import functools diff --git a/frappe/utils/data.py b/frappe/utils/data.py index 8148d194c6..6cd0c45995 100644 --- a/frappe/utils/data.py +++ b/frappe/utils/data.py @@ -104,11 +104,17 @@ def get_timedelta(time: Optional[str] = None) -> Optional[datetime.timedelta]: datetime.timedelta: Timedelta object equivalent of the passed `time` string """ from dateutil import parser + from dateutil.parser import ParserError time = time or "0:0:0" try: - t = parser.parse(time) + try: + t = parser.parse(time) + except ParserError as e: + if "day" in e.args[1]: + from frappe.utils import parse_timedelta + return parse_timedelta(time) return datetime.timedelta( hours=t.hour, minutes=t.minute, seconds=t.second, microseconds=t.microsecond ) @@ -332,7 +338,7 @@ def get_time(time_str): return time_str else: if isinstance(time_str, datetime.timedelta): - time_str = str(time_str) + return format_timedelta(time_str) return parser.parse(time_str).time() def get_datetime_str(datetime_obj): @@ -1678,3 +1684,30 @@ class UnicodeWithAttrs(str): def __init__(self, text): self.toc_html = text.toc_html self.metadata = text.metadata + + +def format_timedelta(o: datetime.timedelta) -> str: + # mariadb allows a wide diff range - https://mariadb.com/kb/en/time/ + # but frappe doesnt - i think via babel : only allows 0..23 range for hour + total_seconds = o.total_seconds() + hours, remainder = divmod(total_seconds, 3600) + minutes, seconds = divmod(remainder, 60) + rounded_seconds = round(seconds, 6) + int_seconds = int(seconds) + + if rounded_seconds == int_seconds: + seconds = int_seconds + else: + seconds = rounded_seconds + + return "{:01}:{:02}:{:02}".format(int(hours), int(minutes), seconds) + + +def parse_timedelta(s: str) -> datetime.timedelta: + # ref: https://stackoverflow.com/a/21074460/10309266 + if 'day' in s: + m = re.match(r"(?P[-\d]+) day[s]*, (?P\d+):(?P\d+):(?P\d[\.\d+]*)", s) + else: + m = re.match(r"(?P\d+):(?P\d+):(?P\d[\.\d+]*)", s) + + return datetime.timedelta(**{key: float(val) for key, val in m.groupdict().items()}) diff --git a/frappe/utils/formatters.py b/frappe/utils/formatters.py index 9436dea2c2..9916853caf 100644 --- a/frappe/utils/formatters.py +++ b/frappe/utils/formatters.py @@ -3,9 +3,11 @@ import frappe import datetime -from frappe.utils import formatdate, fmt_money, flt, cstr, cint, format_datetime, format_time, format_duration +from frappe.utils import formatdate, fmt_money, flt, cstr, cint, format_datetime, format_time, format_duration, format_timedelta from frappe.model.meta import get_field_currency, get_field_precision import re +from dateutil.parser import ParserError + def format_value(value, df=None, doc=None, currency=None, translated=False, format=None): '''Format value based on given fieldtype, document reference, currency reference. @@ -47,7 +49,10 @@ def format_value(value, df=None, doc=None, currency=None, translated=False, form return format_datetime(value) elif df.get("fieldtype")=="Time": - return format_time(value) + try: + return format_time(value) + except ParserError: + return format_timedelta(value) elif value==0 and df.get("fieldtype") in ("Int", "Float", "Currency", "Percent") and df.get("print_hide_if_no_value"): # this is required to show 0 as blank in table columns diff --git a/frappe/utils/response.py b/frappe/utils/response.py index f6ad91dbd2..a852c584c6 100644 --- a/frappe/utils/response.py +++ b/frappe/utils/response.py @@ -1,4 +1,4 @@ -# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors +# Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE import json @@ -16,7 +16,7 @@ from werkzeug.local import LocalProxy from werkzeug.wsgi import wrap_file from werkzeug.wrappers import Response from werkzeug.exceptions import NotFound, Forbidden -from frappe.utils import cint +from frappe.utils import cint, format_timedelta from urllib.parse import quote from frappe.core.doctype.access_log.access_log import make_access_log @@ -122,12 +122,14 @@ def make_logs(response = None): def json_handler(obj): """serialize non-serializable data for json""" - # serialize date - import collections.abc + from collections.abc import Iterable - if isinstance(obj, (datetime.date, datetime.timedelta, datetime.datetime, datetime.time)): + if isinstance(obj, (datetime.date, datetime.datetime, datetime.time)): return str(obj) + elif isinstance(obj, datetime.timedelta): + return format_timedelta(obj) + elif isinstance(obj, decimal.Decimal): return float(obj) @@ -138,7 +140,7 @@ def json_handler(obj): doc = obj.as_dict(no_nulls=True) return doc - elif isinstance(obj, collections.abc.Iterable): + elif isinstance(obj, Iterable): return list(obj) elif type(obj)==type or isinstance(obj, Exception): From 4990a59c48ba7948489e262e02a377d788bcc8ce Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 28 Jan 2022 18:35:27 +0530 Subject: [PATCH 34/36] test: Added unit tests for format_timedelta, parse_timedelta, json_handler --- frappe/tests/test_utils.py | 68 +++++++++++++++++++++++++++++++++++--- 1 file changed, 64 insertions(+), 4 deletions(-) diff --git a/frappe/tests/test_utils.py b/frappe/tests/test_utils.py index 5c1541e0de..83ae6df191 100644 --- a/frappe/tests/test_utils.py +++ b/frappe/tests/test_utils.py @@ -1,11 +1,14 @@ -# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors +# Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE +from decimal import Decimal +from enum import Enum import unittest import frappe from frappe.utils import evaluate_filters, money_in_words, scrub_urls, get_url from frappe.utils import validate_url, validate_email_address from frappe.utils import ceil, floor +from frappe.utils import format_timedelta, parse_timedelta from frappe.utils.data import cast, validate_python_code from frappe.utils.diff import get_version_diff, version_query, _get_value_from_version @@ -13,10 +16,12 @@ from PIL import Image from frappe.utils.image import strip_exif_data, optimize_image import io from mimetypes import guess_type -from datetime import datetime, timedelta, date +from datetime import datetime, time, timedelta, date from unittest.mock import patch +import pytz + class TestFilters(unittest.TestCase): def test_simple_dict(self): self.assertTrue(evaluate_filters({'doctype': 'User', 'status': 'Open'}, {'status': 'Open'})) @@ -275,7 +280,6 @@ class TestPythonExpressions(unittest.TestCase): class TestDiffUtils(unittest.TestCase): - @classmethod def setUpClass(cls): cls.doc = frappe.get_doc(doctype="Client Script", dt="Client Script") @@ -328,4 +332,60 @@ class TestDateUtils(unittest.TestCase): self.assertEqual(frappe.utils.get_last_day_of_week("2020-12-24"), frappe.utils.getdate("2020-12-26")) self.assertEqual(frappe.utils.get_last_day_of_week("2020-12-28"), - frappe.utils.getdate("2021-01-02")) \ No newline at end of file + frappe.utils.getdate("2021-01-02")) + + +class TestResponse(unittest.TestCase): + def test_json_handler(self): + import json + from frappe.utils.response import json_handler + + class TEST(Enum): + ABC = "!@)@)!" + BCE = "ENJD" + + GOOD_OBJECT = { + "time_types": [ + date(year=2020, month=12, day=2), + datetime(year=2020, month=12, day=2, hour=23, minute=23, second=23, microsecond=23, tzinfo=pytz.utc), + time(hour=23, minute=23, second=23, microsecond=23, tzinfo=pytz.utc), + timedelta(days=10, hours=12, minutes=120, seconds=10), + ], + "float": [ + Decimal(29.21), + ], + "doc": [ + frappe.get_doc("System Settings"), + ], + "iter": [ + {1, 2, 3}, + (1, 2, 3), + "abcdef", + ], + "string": "abcdef" + } + + BAD_OBJECT = {"Enum": TEST} + + processed_object = json.loads(json.dumps(GOOD_OBJECT, default=json_handler)) + + self.assertTrue(all([isinstance(x, str) for x in processed_object["time_types"]])) + self.assertTrue(all([isinstance(x, float) for x in processed_object["float"]])) + self.assertTrue(all([isinstance(x, (list, str)) for x in processed_object["iter"]])) + self.assertIsInstance(processed_object["string"], str) + with self.assertRaises(TypeError): + json.dumps(BAD_OBJECT, default=json_handler) + +class TestTimeDeltaUtils(unittest.TestCase): + def test_format_timedelta(self): + self.assertEqual(format_timedelta(timedelta(seconds=0)), "0:00:00") + self.assertEqual(format_timedelta(timedelta(hours=10)), "10:00:00") + self.assertEqual(format_timedelta(timedelta(hours=100)), "100:00:00") + self.assertEqual(format_timedelta(timedelta(seconds=100, microseconds=129)), "0:01:40.000129") + self.assertEqual(format_timedelta(timedelta(seconds=100, microseconds=12212199129)), "3:25:12.199129") + + def test_parse_timedelta(self): + self.assertEqual(parse_timedelta("0:0:0"), timedelta(seconds=0)) + self.assertEqual(parse_timedelta("10:0:0"), timedelta(hours=10)) + self.assertEqual(parse_timedelta("7 days, 0:32:18.192221"), timedelta(days=7, seconds=1938, microseconds=192221)) + self.assertEqual(parse_timedelta("7 days, 0:32:18"), timedelta(days=7, seconds=1938)) From e080eab06b9af2b6b47123ffcde36f160f87b7df Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 28 Jan 2022 18:38:13 +0530 Subject: [PATCH 35/36] style: Sort imports --- frappe/tests/test_utils.py | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/frappe/tests/test_utils.py b/frappe/tests/test_utils.py index 83ae6df191..dd4bd3518f 100644 --- a/frappe/tests/test_utils.py +++ b/frappe/tests/test_utils.py @@ -1,26 +1,28 @@ # Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE -from decimal import Decimal -from enum import Enum -import unittest -import frappe -from frappe.utils import evaluate_filters, money_in_words, scrub_urls, get_url -from frappe.utils import validate_url, validate_email_address -from frappe.utils import ceil, floor -from frappe.utils import format_timedelta, parse_timedelta -from frappe.utils.data import cast, validate_python_code -from frappe.utils.diff import get_version_diff, version_query, _get_value_from_version - -from PIL import Image -from frappe.utils.image import strip_exif_data, optimize_image import io +import json +import unittest +from datetime import date, datetime, time, timedelta +from decimal import Decimal +from enum import Enum from mimetypes import guess_type -from datetime import datetime, time, timedelta, date - from unittest.mock import patch import pytz +from PIL import Image + +import frappe +from frappe.utils import ceil, evaluate_filters, floor, format_timedelta +from frappe.utils import get_url, money_in_words, parse_timedelta, scrub_urls +from frappe.utils import validate_email_address, validate_url +from frappe.utils.data import cast, validate_python_code +from frappe.utils.diff import _get_value_from_version, get_version_diff, version_query +from frappe.utils.image import optimize_image, strip_exif_data +from frappe.utils.response import json_handler + + class TestFilters(unittest.TestCase): def test_simple_dict(self): @@ -337,9 +339,6 @@ class TestDateUtils(unittest.TestCase): class TestResponse(unittest.TestCase): def test_json_handler(self): - import json - from frappe.utils.response import json_handler - class TEST(Enum): ABC = "!@)@)!" BCE = "ENJD" From 229e259bd284625eff1a8b1d213e5b6483560273 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 31 Jan 2022 11:56:09 +0530 Subject: [PATCH 36/36] fix: Format timedelta object accurately --- frappe/query_builder/terms.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frappe/query_builder/terms.py b/frappe/query_builder/terms.py index f9751612b6..bbc316ca93 100644 --- a/frappe/query_builder/terms.py +++ b/frappe/query_builder/terms.py @@ -1,5 +1,6 @@ from datetime import timedelta from typing import Any, Dict, Optional +from frappe.utils.data import format_timedelta from pypika.terms import Function, ValueWrapper from pypika.utils import format_alias_sql @@ -54,7 +55,7 @@ class ParameterizedValueWrapper(ValueWrapper): else: # * BUG: pypika doesen't parse timedeltas if isinstance(self.value, timedelta): - self.value = str(self.value) + self.value = format_timedelta(self.value) sql = self.get_value_sql( quote_char=quote_char, secondary_quote_char=secondary_quote_char,