diff --git a/frappe/database/query.py b/frappe/database/query.py index e3166d398d..1945ee64d7 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -262,6 +262,8 @@ class Engine: self.field_aliases = set() self.db_query_compat = db_query_compat self.permitted_fields_cache = {} # Cache for get_permitted_fields results + self.is_aggregate_query = False + self._grouped_queries = set() if isinstance(table, Table): self.table = table @@ -308,19 +310,24 @@ class Engine: if for_update: self.query = self.query.for_update(skip_locked=skip_locked, nowait=not wait) + if any(isinstance(f, functions.AggregateFunction) for f in getattr(self, "fields", [])): + # check if any field in select is aggregated (done to prevent breaking queries in postgres due to order by rule) + self.is_aggregate_query = True + if group_by: + self.is_aggregate_query = True # for postgres (group by used with order by) self.apply_group_by(group_by) if order_by: if not ( - self.is_postgres and is_select and (distinct or group_by) + self.is_postgres and is_select and distinct ): # ignore in Postgres since order by fields need to appear in select distinct self.apply_order_by(order_by) else: warnings.warn( ( "ORDER BY fields have been ignored because PostgreSQL requires them to " - "appear in the SELECT list when using DISTINCT or GROUP BY." + "appear in the SELECT list when using with DISTINCT" ), UserWarning, stacklevel=2, @@ -340,7 +347,7 @@ class Engine: # Track field aliases for use in group_by/order_by for field in self.fields: - if isinstance(field, Field | DynamicTableField) and field.alias: + if isinstance(field, Field | DynamicTableField | AggregateFunction) and field.alias: self.field_aliases.add(field.alias) if self.apply_permissions: @@ -1190,8 +1197,24 @@ class Engine: # Note: Comma handling is done in parse_fields before this method is called return self.parse_string_field(field) + def _normalize_postgres_order_field(self, field): + """In PostgreSQL order_by fields need to either be in group_by or be aggregated + when used with select and group_by""" + current_sql = field.get_sql() if hasattr(field, "get_sql") else str(field) + if current_sql in self._grouped_queries: + return field + clean_name = current_sql.strip('"') + if clean_name in self.field_aliases: + return field + if not isinstance(field, functions.AggregateFunction): + return functions.Max(field) + return field + def apply_group_by(self, group_by: str | None = None): parsed_group_by_fields = self._validate_group_by(group_by) + self._grouped_queries = { + f.get_sql() if hasattr(f, "get_sql") else str(f) for f in parsed_group_by_fields + } self.query = self.query.groupby(*parsed_group_by_fields) def apply_order_by(self, order_by: str | None): @@ -1201,7 +1224,12 @@ class Engine: parsed_order_fields = self._validate_order_by(order_by) for order_field, order_direction in parsed_order_fields: - self.query = self.query.orderby(order_field, order=order_direction) + if self.is_postgres and self.is_aggregate_query: + self.query = self.query.orderby( + self._normalize_postgres_order_field(order_field), order=order_direction + ) + else: + self.query = self.query.orderby(order_field, order=order_direction) def _apply_default_order_by(self): """Apply default ordering based on configured DocType metadata""" @@ -1220,14 +1248,24 @@ class Engine: order_direction = Order.desc if spec_order == "desc" else Order.asc else: order_direction = Order.asc if spec_order == "asc" else Order.desc - self.query = self.query.orderby(field, order=order_direction) + if self.is_postgres and self.is_aggregate_query: + self.query = self.query.orderby( + self._normalize_postgres_order_field(field), order=order_direction + ) + else: + self.query = self.query.orderby(field, order=order_direction) else: field = self.table[sort_field] if self.db_query_compat: order_direction = Order.desc if sort_order.lower() == "desc" else Order.asc else: order_direction = Order.asc if sort_order.lower() == "asc" else Order.desc - self.query = self.query.orderby(field, order=order_direction) + if self.is_postgres and self.is_aggregate_query: + self.query = self.query.orderby( + self._normalize_postgres_order_field(field), order=order_direction + ) + else: + self.query = self.query.orderby(field, order=order_direction) def _parse_backtick_field_notation(self, field_name: str) -> tuple[str, str] | None: """ diff --git a/frappe/tests/test_query.py b/frappe/tests/test_query.py index 5e611c818f..86b4114b99 100644 --- a/frappe/tests/test_query.py +++ b/frappe/tests/test_query.py @@ -2345,6 +2345,100 @@ class TestQuery(IntegrationTestCase): # the filter should still apply and return no results self.assertEqual(len(result), 0, "Filter should not be bypassed by shared doc OR condition") + @run_only_if(db_type_is.POSTGRES) + def test_order_by_group_by_postgres(self): + """PostgreSQL specific test that tests if order_by fields are correctly handled when used with group_by""" + # test order by fields already in group by (no aggregate needed) + query = frappe.qb.get_query( + "User", + fields=["creation as created_date", {"COUNT": "*"}], + group_by="created_date", + order_by="created_date", + ).get_sql() + + self.assertQueryEqual( + query, + 'SELECT "creation" "created_date",COUNT(*) FROM "tabUser" GROUP BY "created_date" ORDER BY "created_date" DESC', + ) + + # test order by fields not in group by (aggregate needed) + query = frappe.qb.get_query( + "User", + fields=["creation as created_date", {"COUNT": "*"}], + group_by="created_date", + order_by="name", + ).get_sql() + + self.assertQueryEqual( + query, + 'SELECT "creation" "created_date",COUNT(*) FROM "tabUser" GROUP BY "created_date" ORDER BY MAX("name") DESC', + ) + + query = frappe.qb.get_query( + "User", + fields=["user_type as type", "enabled as status", {"COUNT": "*"}], + group_by="type, status", + order_by="status asc", + ).get_sql() + + self.assertQueryEqual( + query, + 'SELECT "user_type" "type","enabled" "status",COUNT(*) FROM "tabUser" GROUP BY "type","status" ORDER BY "status" ASC', + ) + + # test no double aggregation rule + query = frappe.qb.get_query( + "User", + fields=["creation", {"COUNT": "*", "as": "total"}], + group_by="creation", + order_by="total desc", + ).get_sql() + + self.assertQueryEqual( + query, + 'SELECT "creation",COUNT(*) "total" FROM "tabUser" GROUP BY "creation" ORDER BY "total" DESC', + ) + + # test multiple order_by fields not in group_by + query = frappe.qb.get_query( + "User", + fields=["user_type", {"COUNT": "*"}], + group_by="user_type", + order_by="creation desc, modified asc", + ).get_sql() + + self.assertIn('MAX("creation") DESC', query) + self.assertIn('MAX("modified") ASC', query) + + # for queries that have aggregate fields selected but not grouped (these queries are redundant but exist in some parts of codebase) + query = frappe.qb.get_query( + "User", fields=[{"COUNT": "*", "as": "result"}], order_by="creation desc" + ).get_sql() + + self.assertQueryEqual(query, 'SELECT COUNT(*) "result" FROM "tabUser" ORDER BY MAX("creation") DESC') + + # test in case user uses `original_col` name instead of alias + query = frappe.qb.get_query( + "User", fields=["name as user_name"], group_by="user_name", order_by="user_name" + ) + a = query.run() + + query = frappe.qb.get_query("User", fields=["name as user_name"], group_by="name", order_by="name") + b = query.run() + + query = frappe.qb.get_query( + "User", fields=["name as user_name"], group_by="name", order_by="user_name" + ) + c = query.run() + + query = frappe.qb.get_query( + "User", fields=["name as user_name"], group_by="user_name", order_by="name" + ) + d = query.run() + + for val in [b, c, d]: + self.assertEqual(a, val, "Query result mismatch detected.") + @run_only_if(db_type_is.POSTGRES) def test_ifnull_fallback_postgres(self): """Test ifnull fallback in postgres"""