From 40ba1ac9ba3909ffff30c03029e70a43b520c483 Mon Sep 17 00:00:00 2001
From: Conor <conor@millstone.studio>
Date: Sun, 19 Dec 2021 12:16:34 -0600
Subject: [PATCH] fix: Postgres Compatibility

* Handle inconsistencies in type handling in DatabaseQuery & Database
  APIs
* Update incompatible queries with frappe.qb notation
* Fixed use cases discovered by failing ERPNext CI tests

fix: db independent syntax for user_type

fix: handle postgres datetime values

feat: add ability to auto commit on db inserts

feat: add ability to escape underscore in postgres

fix: handle missing data in test runner bootstrapping

fix: db independent syntax for queries

fix: refactor to use qb

fix: update cache for language

fix: use pluck in email_queue

Co-authored-by: gavin <gavin18d@gmail.com>

fix: don't auto insert on tests for make_property_setter

fix: remove auto_commit in custom_field insertion

fix: remove auto_commit functionality

fix: review comments

fix: revert link validation

fix: style suggestion for readability

Co-authored-by: gavin <gavin18d@gmail.com>

fix: revert .lower() in link validation

fix: add rollback for setup_wizard

Revert "fix: add rollback for setup_wizard"

This reverts commit 83b3b0913db17718ccd5edae01858cff15603829.

Revert "feat: add ability to escape underscore in postgres"

This reverts commit 8ed9c2aa3306438e94bb813f60e65b416d0b947b.

fix: more concise representation of order fields

Co-authored-by: gavin <gavin18d@gmail.com>
---
 frappe/core/doctype/user_type/user_type.py    | 12 +++++--
 frappe/database/postgres/database.py          |  8 ++++-
 .../desk/doctype/bulk_update/bulk_update.py   |  3 +-
 frappe/desk/doctype/dashboard/dashboard.py    |  5 +--
 frappe/desk/page/setup_wizard/setup_wizard.py |  3 +-
 frappe/desk/treeview.py                       |  5 +--
 .../email/doctype/email_queue/email_queue.py  | 36 ++++++++-----------
 frappe/model/base_document.py                 |  4 ++-
 frappe/model/db_query.py                      | 13 ++++---
 frappe/test_runner.py                         |  5 ++-
 frappe/tests/test_website.py                  |  1 +
 frappe/utils/data.py                          |  3 ++
 frappe/utils/make_random.py                   | 10 ++++--
 frappe/utils/nestedset.py                     | 14 ++++----
 frappe/utils/response.py                      |  2 +-
 .../doctype/blog_post/test_blog_post.py       |  5 ++-
 16 files changed, 77 insertions(+), 52 deletions(-)

diff --git a/frappe/core/doctype/user_type/user_type.py b/frappe/core/doctype/user_type/user_type.py
index 6a07b5a23b..5775d499bd 100644
--- a/frappe/core/doctype/user_type/user_type.py
+++ b/frappe/core/doctype/user_type/user_type.py
@@ -36,13 +36,19 @@ class UserType(Document):
 		if not self.user_doctypes:
 			return
 
-		modules = frappe.get_all('DocType', fields=['distinct module as module'],
-			filters={'name': ('in', [d.document_type for d in self.user_doctypes])}, group_by='module')
+		DocType = frappe.qb.DocType("DocType")
+
+		document_types = [d.document_type for d in self.user_doctypes] or ['']
+		modules = (frappe.qb.from_(DocType)
+					.select(DocType.module)
+					.where(DocType.name.isin(document_types))
+					.groupby(DocType.module)
+					.distinct()).run()
 
 		self.set('user_type_modules', [])
 		for row in modules:
 			self.append('user_type_modules', {
-				'module': row.module
+				'module': row[0]
 			})
 
 	def validate_document_type_limit(self):
diff --git a/frappe/database/postgres/database.py b/frappe/database/postgres/database.py
index 3cea1440cf..795d36eaeb 100644
--- a/frappe/database/postgres/database.py
+++ b/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("%", "%%")
diff --git a/frappe/desk/doctype/bulk_update/bulk_update.py b/frappe/desk/doctype/bulk_update/bulk_update.py
index b512ca175c..a0523d90cd 100644
--- a/frappe/desk/doctype/bulk_update/bulk_update.py
+++ b/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
diff --git a/frappe/desk/doctype/dashboard/dashboard.py b/frappe/desk/doctype/dashboard/dashboard.py
index 833447a00b..55044cda4b 100644
--- a/frappe/desk/doctype/dashboard/dashboard.py
+++ b/frappe/desk/doctype/dashboard/dashboard.py
@@ -13,8 +13,9 @@ 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 = frappe.qb.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)
diff --git a/frappe/desk/page/setup_wizard/setup_wizard.py b/frappe/desk/page/setup_wizard/setup_wizard.py
index 6d7ecce462..b42d8c58b7 100755
--- a/frappe/desk/page/setup_wizard/setup_wizard.py
+++ b/frappe/desk/page/setup_wizard/setup_wizard.py
@@ -151,7 +151,7 @@ def update_system_settings(args):
 	system_settings = frappe.get_doc("System Settings", "System Settings")
 	system_settings.update({
 		"country": args.get("country"),
-		"language": get_language_code(args.get("language")),
+		"language": get_language_code(args.get("language")) or 'en',
 		"time_zone": args.get("timezone"),
 		"float_precision": 3,
 		'date_format': frappe.db.get_value("Country", args.get("country"), "date_format"),
@@ -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)
diff --git a/frappe/desk/treeview.py b/frappe/desk/treeview.py
index f40c135653..7e3efb5d48 100644
--- a/frappe/desk/treeview.py
+++ b/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)
 
diff --git a/frappe/email/doctype/email_queue/email_queue.py b/frappe/email/doctype/email_queue/email_queue.py
index d89a3d83be..9730004065 100644
--- a/frappe/email/doctype/email_queue/email_queue.py
+++ b/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
 
diff --git a/frappe/model/base_document.py b/frappe/model/base_document.py
index 1fd3784fcc..d24c1722a8 100644
--- a/frappe/model/base_document.py
+++ b/frappe/model/base_document.py
@@ -771,7 +771,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)
diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py
index 3c5d6e81ac..73273917f7 100644
--- a/frappe/model/db_query.py
+++ b/frappe/model/db_query.py
@@ -127,19 +127,18 @@ class DatabaseQuery(object):
 		if self.distinct:
 			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 args.order_by and args.group_by:
 			order_field = args.order_by
 
 			for r in (" order by ", " asc", " ASC", " desc", " DESC"):
 				order_field = order_field.replace(r, "")
 
-			if not order_field in args.fields:
-				order_fieldm = order_field.replace("`", "")
-				if "." in order_fieldm:
-					args.fields += ", MAX(" + order_fieldm.split(".")[1] + ") as `" + order_fieldm + "`"
-				else:
-					args.fields += ", MAX(" + order_fieldm + ") as `" + order_fieldm + "`"
+			if order_field not in args.fields:
+				order_fieldm = order_field.replace("`", '"')
+				args.fields += f", MAX({order_fieldm}) as `{order_fieldm}`"
 				args.order_by = args.order_by.replace(order_field, "`" + order_fieldm + "`")
 
 		query = """select %(fields)s
diff --git a/frappe/test_runner.py b/frappe/test_runner.py
index 5f26842be2..1839f15ae8 100644
--- a/frappe/test_runner.py
+++ b/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)
diff --git a/frappe/tests/test_website.py b/frappe/tests/test_website.py
index 992d876243..e40a07c0ec 100644
--- a/frappe/tests/test_website.py
+++ b/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)
diff --git a/frappe/utils/data.py b/frappe/utils/data.py
index de0242df07..cdd4d3d277 100644
--- a/frappe/utils/data.py
+++ b/frappe/utils/data.py
@@ -98,6 +98,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)
diff --git a/frappe/utils/make_random.py b/frappe/utils/make_random.py
index 2ebabb78f9..bb4395b626 100644
--- a/frappe/utils/make_random.py
+++ b/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
 
diff --git a/frappe/utils/nestedset.py b/frappe/utils/nestedset.py
index 5c4beda71e..1fc2d7779a 100644
--- a/frappe/utils/nestedset.py
+++ b/frappe/utils/nestedset.py
@@ -16,6 +16,9 @@ import frappe
 from frappe import _
 from frappe.model.document import Document
 from frappe.query_builder import DocType, Order
+from pypika import CustomFunction
+
+Replace = CustomFunction('REPLACE', ['input', 'substr_to_replace', 'substr_to_replace_with'])
 
 class NestedSetRecursionError(frappe.ValidationError): pass
 class NestedSetMultipleRootsError(frappe.ValidationError): pass
@@ -88,7 +91,7 @@ def update_move_node(doc, parent_field):
 
 	if parent:
 		new_parent = frappe.db.sql("""select lft, rgt from `tab{0}`
-			where name = %s for update""".format(doc.doctype), parent, as_dict=1)[0]
+		where name = %s for update""".format(doc.doctype), parent, as_dict=1)[0]
 
 		validate_loop(doc.doctype, doc.name, new_parent.lft, new_parent.rgt)
 
@@ -107,8 +110,7 @@ def update_move_node(doc, parent_field):
 
 	if parent:
 		new_parent = frappe.db.sql("""select lft, rgt from `tab%s`
-			where name = %s for update""" % (doc.doctype, '%s'), parent, as_dict=1)[0]
-
+		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
@@ -134,6 +136,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):
 	"""
@@ -148,13 +151,12 @@ def rebuild_tree(doctype, parent_field):
 	right = 1
 	table = DocType(doctype)
 	column = getattr(table, parent_field)
-
 	result = (
 		frappe.qb.from_(table)
 		.where(
 			(column == "") | (column.isnull())
 		)
-		.orderby(table.name, order=Order.asc)
+		.orderby(Replace(table.name, '_', ''), order=Order.asc)
 		.select(table.name)
 	).run()
 
@@ -177,7 +179,7 @@ def rebuild_node(doctype, parent, left, parent_field):
 	column = getattr(table, parent_field)
 
 	result = (
-		frappe.qb.from_(table).where(column == parent).select(table.name)
+		frappe.qb.from_(table).where(column == parent).select(table.name).orderby(Replace(table.name, '_', ''), order=Order.asc)
 	).run()
 
 	for r in result:
diff --git a/frappe/utils/response.py b/frappe/utils/response.py
index 104c48527c..f6ad91dbd2 100644
--- a/frappe/utils/response.py
+++ b/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):
diff --git a/frappe/website/doctype/blog_post/test_blog_post.py b/frappe/website/doctype/blog_post/test_blog_post.py
index 7a09d0a3ca..d649d25f7e 100644
--- a/frappe/website/doctype/blog_post/test_blog_post.py
+++ b/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)