From 6ab347d9f51f4c54c4a9f66c80e6266ac0f134f2 Mon Sep 17 00:00:00 2001 From: marination Date: Mon, 7 Oct 2024 18:45:50 +0200 Subject: [PATCH 1/2] fix: 'not set' must translate to the same condition whether used via QB or not --- frappe/database/operator_map.py | 3 ++- frappe/tests/test_db_query.py | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/frappe/database/operator_map.py b/frappe/database/operator_map.py index 72ed8b4a75..26a7f7e7a9 100644 --- a/frappe/database/operator_map.py +++ b/frappe/database/operator_map.py @@ -8,6 +8,7 @@ import frappe from frappe.database.utils import NestedSetHierarchy from frappe.model.db_query import get_timespan_date_range from frappe.query_builder import Field +from frappe.query_builder.functions import Coalesce def like(key: Field, value: str) -> frappe.qb: @@ -94,7 +95,7 @@ def func_between(key: Field, value: list | tuple) -> frappe.qb: def func_is(key, value): "Wrapper for IS" - return key.isnotnull() if value.lower() == "set" else key.isnull() + return Coalesce(key, "") != "" if value.lower() == "set" else Coalesce(key, "") == "" def func_timespan(key: Field, value: str) -> frappe.qb: diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index 7d539cdf47..fae28dab7f 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -1392,6 +1392,20 @@ class TestReportView(FrappeTestCase): response = execute_cmd("frappe.desk.reportview.get") self.assertListEqual(response["keys"], ["published", "title", "test_field"]) + def test_db_filter_not_set(self): + """ + Test if the 'not set' filter always translates correctly with/without qb under the hood. + """ + frappe.get_doc({"doctype": "ToDo", "description": "filter test"}).insert() + frappe.get_doc({"doctype": "ToDo", "description": "filter test", "reference_name": ""}).insert() + + # `get_all` does not use QueryBuilder while `count` does. Both should return the same result. + # `not set` must consider empty strings and NULL values both. + self.assertEqual( + len(frappe.get_all("ToDo", filters={"reference_name": ["is", "not set"]})), + frappe.db.count("ToDo", {"reference_name": ["is", "not set"]}), + ) + def add_child_table_to_blog_post(): child_table = frappe.get_doc( From 42e7b9223fb3f1becc8f9b87877ba5d564e91d85 Mon Sep 17 00:00:00 2001 From: marination Date: Tue, 8 Oct 2024 11:24:03 +0200 Subject: [PATCH 2/2] fix(test): `test_is` to check for coalesce as ifnull is legacy for a `not set`/`set` filter --- frappe/tests/test_db.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/frappe/tests/test_db.py b/frappe/tests/test_db.py index 6d2994dd90..fe906c20af 100644 --- a/frappe/tests/test_db.py +++ b/frappe/tests/test_db.py @@ -960,10 +960,12 @@ class TestDDLCommandsPost(FrappeTestCase): def test_is(self): user = frappe.qb.DocType("User") self.assertIn( - "is not null", frappe.db.get_values(user, filters={user.name: ("is", "set")}, run=False).lower() + 'coalesce("name",', + frappe.db.get_values(user, filters={user.name: ("is", "set")}, run=False).lower(), ) self.assertIn( - "is null", frappe.db.get_values(user, filters={user.name: ("is", "not set")}, run=False).lower() + 'coalesce("name",', + frappe.db.get_values(user, filters={user.name: ("is", "not set")}, run=False).lower(), )