Merge pull request #15354 from cpdeethree/frappe-postgres-changes

fix: SQL queries to improve Postgres support

frappe/core/doctype/user_type/user_type.py

@@ -37,16 +37,14 @@ class UserType(Document):
 			return
 
 		modules = frappe.get_all("DocType",
-			fields=["module"],
 			filters={"name": ("in", [d.document_type for d in self.user_doctypes])},
 			distinct=True,
+			pluck="module",
 		)
 
-		self.set('user_type_modules', [])
-		for row in modules:
-			self.append('user_type_modules', {
-				'module': row.module
-			})
+		self.set("user_type_modules", [])
+		for module in modules:
+			self.append("user_type_modules", {"module": module})
 
 	def validate_document_type_limit(self):
 		limit = frappe.conf.get('user_type_doctype_limit', {}).get(frappe.scrub(self.name))

frappe/database/postgres/database.py

@@ -74,9 +74,15 @@ class PostgresDatabase(Database):
 		return conn
 
 	def escape(self, s, percent=True):
-		"""Excape quotes and percent in given string."""
+		"""Escape quotes and percent in given string."""
 		if isinstance(s, bytes):
 			s = s.decode('utf-8')
+				
+		# MariaDB's driver treats None as an empty string
+		# So Postgres should do the same
+
+		if s is None: 
+			s = ''
 
 		if percent:
 			s = s.replace("%", "%%")

frappe/desk/doctype/bulk_update/bulk_update.py

@@ -7,6 +7,7 @@ from frappe.model.document import Document
 from frappe import _
 from frappe.utils import cint
 
+
 class BulkUpdate(Document):
 	pass
 
@@ -22,7 +23,7 @@ def update(doctype, field, value, condition='', limit=500):
 		frappe.throw(_('; not allowed in condition'))
 
 	docnames = frappe.db.sql_list(
-		'''select name from `tab{0}`{1} limit 0, {2}'''.format(doctype, condition, limit)
+		'''select name from `tab{0}`{1} limit {2} offset 0'''.format(doctype, condition, limit)
 	)
 	data = {}
 	data[field] = value

frappe/desk/doctype/dashboard/dashboard.py

@@ -1,23 +1,33 @@
-# -*- coding: utf-8 -*-
-# Copyright (c) 2019, Frappe Technologies and contributors
+# Copyright (c) 2022, Frappe Technologies and contributors
 # License: MIT. See LICENSE
 
-from frappe.model.document import Document
-from frappe.modules.export_file import export_to_files
-from frappe.config import get_modules_from_all_apps_for_user
+import json
+
 import frappe
 from frappe import _
-import json
+from frappe.config import get_modules_from_all_apps_for_user
+from frappe.model.document import Document
+from frappe.modules.export_file import export_to_files
+from frappe.query_builder import DocType
+
 
 class Dashboard(Document):
 	def on_update(self):
 		if self.is_default:
 			# make all other dashboards non-default
-			frappe.db.sql('''update
-				tabDashboard set is_default = 0 where name != %s''', self.name)
+			DashBoard = DocType("Dashboard")
+
+			frappe.qb.update(DashBoard).set(
+				DashBoard.is_default, 0
+			).where(
+				DashBoard.name != self.name
+			).run()
 
 		if frappe.conf.developer_mode and self.is_standard:
-			export_to_files(record_list=[['Dashboard', self.name, self.module + ' Dashboard']], record_module=self.module)
+			export_to_files(
+				record_list=[["Dashboard", self.name, f"{self.module} Dashboard"]],
+				record_module=self.module
+			)
 
 	def validate(self):
 		if not frappe.conf.developer_mode and self.is_standard:

frappe/desk/page/setup_wizard/setup_wizard.py

@@ -388,7 +388,6 @@ def make_records(records, debug=False):
 
 	# LOG every success and failure
 	for record in records:
-
 		doctype = record.get("doctype")
 		condition = record.get('__condition')
 
@@ -405,6 +404,7 @@ def make_records(records, debug=False):
 
 		try:
 			doc.insert(ignore_permissions=True)
+			frappe.db.commit()
 
 		except frappe.DuplicateEntryError as e:
 			# print("Failed to insert duplicate {0} {1}".format(doctype, doc.name))
@@ -417,6 +417,7 @@ def make_records(records, debug=False):
 				raise
 
 		except Exception as e:
+			frappe.db.rollback()
 			exception = record.get('__exception')
 			if exception:
 				config = _dict(exception)

frappe/desk/treeview.py

@@ -4,6 +4,7 @@
 import frappe
 from frappe import _
 
+
 @frappe.whitelist()
 def get_all_nodes(doctype, label, parent, tree_method, **filters):
 	'''Recursively gets all data from tree nodes'''
@@ -40,8 +41,8 @@ def get_children(doctype, parent='', **filters):
 
 def _get_children(doctype, parent='', ignore_permissions=False):
 	parent_field = 'parent_' + doctype.lower().replace(' ', '_')
-	filters = [['ifnull(`{0}`,"")'.format(parent_field), '=', parent],
-		['docstatus', '<' ,'2']]
+	filters = [["ifnull(`{0}`,'')".format(parent_field), '=', parent],
+		['docstatus', '<' ,2]]
 
 	meta = frappe.get_meta(doctype)
 

frappe/email/doctype/email_queue/email_queue.py

@@ -475,28 +475,20 @@ class QueueBuilder:
 		if self._unsubscribed_user_emails is not None:
 			return self._unsubscribed_user_emails
 
-		all_ids = tuple(set(self.recipients + self.cc))
-
-		unsubscribed = frappe.db.sql_list('''
-			SELECT
-				distinct email
-			from
-				`tabEmail Unsubscribe`
-			where
-				email in %(all_ids)s
-				and (
-					(
-						reference_doctype = %(reference_doctype)s
-						and reference_name = %(reference_name)s
-					)
-					or global_unsubscribe = 1
-				)
-		''', {
-			'all_ids': all_ids,
-			'reference_doctype': self.reference_doctype,
-			'reference_name': self.reference_name,
-		})
-
+		all_ids = list(set(self.recipients + self.cc))
+
+		EmailUnsubscribe = frappe.qb.DocType("Email Unsubscribe")
+
+		unsubscribed = (frappe.qb.from_(EmailUnsubscribe)
+						.select(EmailUnsubscribe.email)
+						.where(EmailUnsubscribe.email.isin(all_ids) & 	
+									(
+										(
+											(EmailUnsubscribe.reference_doctype == self.reference_doctype) & (EmailUnsubscribe.reference_name == self.reference_name)
+										) | EmailUnsubscribe.global_unsubscribe == 1
+									)
+								).distinct()
+						).run(pluck=True)
 		self._unsubscribed_user_emails = unsubscribed or []
 		return self._unsubscribed_user_emails
 

frappe/model/base_document.py

@@ -768,7 +768,9 @@ class BaseDocument(object):
 
 				else:
 					self_value = self.get_value(key)
-
+				# Postgres stores values as `datetime.time`, MariaDB as `timedelta`
+				if isinstance(self_value, datetime.timedelta) and isinstance(db_value, datetime.time):
+					db_value = datetime.timedelta(hours=db_value.hour, minutes=db_value.minute, seconds=db_value.second, microseconds=db_value.microsecond)
 				if self_value != db_value:
 					frappe.throw(_("Not allowed to change {0} after submission").format(df.label),
 						frappe.UpdateAfterSubmitError)

frappe/model/db_query.py

@@ -130,6 +130,11 @@ class DatabaseQuery(object):
 			args.fields = 'distinct ' + args.fields
 			args.order_by = '' # TODO: recheck for alternative
 
+		# Postgres requires any field that appears in the select clause to also
+		# appear in the order by and group by clause
+		if frappe.db.db_type == 'postgres' and args.order_by and args.group_by:
+			args = self.prepare_select_args(args)
+
 		query = """select %(fields)s
 			from %(tables)s
 			%(conditions)s
@@ -203,6 +208,19 @@ class DatabaseQuery(object):
 
 		return args
 
+	def prepare_select_args(self, args):
+		order_field = re.sub(r"\ order\ by\ |\ asc|\ ASC|\ desc|\ DESC", "", args.order_by)
+
+		if order_field not in args.fields:
+			extracted_column = order_column = order_field.replace("`", "")
+			if "." in extracted_column:
+				extracted_column = extracted_column.split(".")[1]
+
+			args.fields += f", MAX({extracted_column}) as `{order_column}`"
+			args.order_by = args.order_by.replace(order_field, f"`{order_column}`")
+
+		return args
+
 	def parse_args(self):
 		"""Convert fields and filters from strings to list, dicts"""
 		if isinstance(self.fields, str):

frappe/test_runner.py

@@ -335,7 +335,10 @@ def make_test_records_for_doctype(doctype, verbose=0, force=False):
 		frappe.local.test_objects[doctype] += test_module._make_test_records(verbose)
 
 	elif hasattr(test_module, "test_records"):
-		frappe.local.test_objects[doctype] += make_test_objects(doctype, test_module.test_records, verbose, force)
+		if doctype in frappe.local.test_objects:
+			frappe.local.test_objects[doctype] += make_test_objects(doctype, test_module.test_records, verbose, force)
+		else: 
+			frappe.local.test_objects[doctype] = make_test_objects(doctype, test_module.test_records, verbose, force)
 
 	else:
 		test_records = frappe.get_test_records(doctype)

frappe/tests/test_db_query.py

@@ -1,6 +1,8 @@
 # Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors
 # License: MIT. See LICENSE
-import frappe, unittest
+import frappe
+import datetime
+import unittest
 
 from frappe.model.db_query import DatabaseQuery
 from frappe.desk.reportview import get_filters_cond
@@ -380,6 +382,22 @@ class TestReportview(unittest.TestCase):
 		owners = DatabaseQuery("DocType").execute(filters={"name": "DocType"}, pluck="owner")
 		self.assertEqual(owners, ["Administrator"])
 
+	def test_prepare_select_args(self):
+		# frappe.get_all inserts modified field into order_by clause
+		# test to make sure this is inserted into select field when postgres
+		doctypes = frappe.get_all("DocType",
+			filters={"docstatus": 0, "document_type": ("!=", "")},
+			group_by="document_type",
+			fields=["document_type", "sum(is_submittable) as is_submittable"],
+			limit=1,
+			as_list=True,
+		)
+		if frappe.conf.db_type == "mariadb":
+			self.assertTrue(len(doctypes[0]) == 2)
+		else:
+			self.assertTrue(len(doctypes[0]) == 3)
+			self.assertTrue(isinstance(doctypes[0][2], datetime.datetime))
+
 	def test_column_comparison(self):
 		"""Test DatabaseQuery.execute to test column comparison
 		"""

frappe/tests/test_website.py

@@ -197,6 +197,7 @@ class TestWebsite(unittest.TestCase):
 		frappe.cache().delete_key('app_hooks')
 
 	def test_printview_page(self):
+		frappe.db.value_cache[('DocType', 'Language', 'name')] = (('Language',),)
 		content = get_response_content('/Language/ru')
 		self.assertIn('<div class="print-format">', content)
 		self.assertIn('<div>Language</div>', content)

frappe/utils/data.py

@@ -113,6 +113,9 @@ def get_timedelta(time: Optional[str] = None) -> Optional[datetime.timedelta]:
 def to_timedelta(time_str):
 	from dateutil import parser
 
+	if isinstance(time_str, datetime.time):
+		time_str = str(time_str)
+
 	if isinstance(time_str, str):
 		t = parser.parse(time_str)
 		return datetime.timedelta(hours=t.hour, minutes=t.minute, seconds=t.second, microseconds=t.microsecond)

frappe/utils/make_random.py

@@ -35,9 +35,13 @@ def get_random(doctype, filters=None, doc=False):
 		condition = " where " + " and ".join(condition)
 	else:
 		condition = ""
-
-	out = frappe.db.sql("""select name from `tab%s` %s
-		order by RAND() limit 0,1""" % (doctype, condition))
+	
+	out = frappe.db.multisql({
+		'mariadb': """select name from `tab%s` %s
+		order by RAND() limit 1 offset 0""" % (doctype, condition),
+		'postgres': """select name from `tab%s` %s
+		order by RANDOM() limit 1 offset 0""" % (doctype, condition)
+	})
 
 	out = out and out[0][0] or None
 

frappe/utils/nestedset.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
 
 # Tree (Hierarchical) Nested Set Model (nsm)
@@ -109,7 +109,6 @@ def update_move_node(doc, parent_field):
 		new_parent = frappe.db.sql("""select lft, rgt from `tab%s`
 			where name = %s for update""" % (doc.doctype, '%s'), parent, as_dict=1)[0]
 
-
 		# set parent lft, rgt
 		frappe.db.sql("""update `tab{0}` set rgt = rgt + %s
 			where name = %s""".format(doc.doctype), (diff, parent))
@@ -134,6 +133,7 @@ def update_move_node(doc, parent_field):
 	frappe.db.sql("""update `tab{0}` set lft = -lft + %s, rgt = -rgt + %s
 		where lft < 0""".format(doc.doctype), (new_diff, new_diff))
 
+
 @frappe.whitelist()
 def rebuild_tree(doctype, parent_field):
 	"""
@@ -153,7 +153,6 @@ def rebuild_tree(doctype, parent_field):
 	right = 1
 	table = DocType(doctype)
 	column = getattr(table, parent_field)
-
 	result = (
 		frappe.qb.from_(table)
 		.where(

frappe/utils/response.py

@@ -125,7 +125,7 @@ def json_handler(obj):
 	# serialize date
 	import collections.abc
 
-	if isinstance(obj, (datetime.date, datetime.timedelta, datetime.datetime)):
+	if isinstance(obj, (datetime.date, datetime.timedelta, datetime.datetime, datetime.time)):
 		return str(obj)
 
 	elif isinstance(obj, decimal.Decimal):

frappe/website/doctype/blog_post/test_blog_post.py

@@ -58,15 +58,18 @@ class TestBlogPost(unittest.TestCase):
 		category_page_link = list(soup.find_all('a', href=re.compile(blog.blog_category)))[0]
 		category_page_url = category_page_link["href"]
 
+		cached_value = frappe.db.value_cache[('DocType', 'Blog Post', 'name')]
+		frappe.db.value_cache[('DocType', 'Blog Post', 'name')] = (('Blog Post',),)
+
 		# Visit the category page (by following the link found in above stage)
 		set_request(path=category_page_url)
 		category_page_response = get_response()
 		category_page_html = frappe.safe_decode(category_page_response.get_data())
-
 		# Category page should contain the blog post title
 		self.assertIn(blog.title, category_page_html)
 
 		# Cleanup
+		frappe.db.value_cache[('DocType', 'Blog Post', 'name')] = cached_value
 		frappe.delete_doc("Blog Post", blog.name)
 		frappe.delete_doc("Blog Category", blog.blog_category)