From ad237e5ec56cfac5dfa3d8b11de0aca75ad3fb37 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Mon, 22 Jun 2026 23:22:52 +0530 Subject: [PATCH] ci(postgres): flag get_all(distinct=True, order_by=...) in the static checker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit frappe's db_query SILENTLY drops ORDER BY for distinct queries on Postgres (the ORDER BY column must appear in the SELECT-DISTINCT list), so `get_all/get_list(distinct=True, order_by="")` is a no-op there and the result comes back unordered — the root cause of the Sales Register, Purchase Register and Sales Analytics ordering fixes. Add an AST rule to .github/helper/postgres_compat.py that flags this (literal order_by only; an empty order_by="" suppression and a dynamic/variable order_by are not flagged). `# pg-ok` escape hatch as usual. Grandfather the three pre-existing low-impact sites the rule surfaces (paging/iteration order only, not data): job_card operation autocomplete, inventory_dimension config list, and a work_order test loop. --- .github/helper/postgres_compat.py | 24 +++++++++++++++++++ .../doctype/job_card/job_card.py | 2 +- .../doctype/work_order/test_work_order.py | 2 +- .../inventory_dimension.py | 2 +- 4 files changed, 27 insertions(+), 3 deletions(-) diff --git a/.github/helper/postgres_compat.py b/.github/helper/postgres_compat.py index a02e82eb953..273c4218193 100755 --- a/.github/helper/postgres_compat.py +++ b/.github/helper/postgres_compat.py @@ -71,6 +71,11 @@ MYSQL_RESULT_KEYS = {"Column_name", "Key_name", "Seq_in_index", "Non_unique", "I SET_BOOL_FUNCS = {"set_value", "db_set"} +# frappe.get_all / get_list: frappe's db_query SILENTLY drops ORDER BY for `distinct` queries on +# Postgres (the ORDER BY column must appear in the SELECT-DISTINCT list), so `distinct=True` together +# with a literal `order_by` is a no-op on PG and the result comes back unordered. +DISTINCT_ORDER_FUNCS = {"get_all", "get_list"} + def _docstring_ids(tree: ast.AST) -> set[int]: """ids of Constant nodes that are docstrings (so prose describing the rules isn't flagged).""" @@ -154,6 +159,25 @@ class Visitor(ast.NodeVisitor): if isinstance(a, ast.Constant) and isinstance(a.value, bool): self._flag(node, f"{name}(..., {a.value}) sets an int/Check column with a bool -> pass 1/0 (Postgres rejects bool->smallint)") + # frappe.get_all/get_list(..., distinct=True, order_by="") -> ORDER BY is silently dropped + # for distinct queries on Postgres, so the result is unordered there. Sort in python instead + # (e.g. sorted(frappe.get_all(..., distinct=True), key=str.casefold)). An empty order_by="" (the + # explicit "suppress the injected default" idiom) and a dynamic/variable order_by are not flagged. + if name in DISTINCT_ORDER_FUNCS: + has_distinct = any( + kw.arg == "distinct" and isinstance(kw.value, ast.Constant) and kw.value.value + for kw in node.keywords + ) + order_kw = next((kw for kw in node.keywords if kw.arg == "order_by"), None) + has_literal_order = ( + order_kw is not None + and isinstance(order_kw.value, ast.Constant) + and isinstance(order_kw.value.value, str) + and order_kw.value.value.strip() + ) + if has_distinct and has_literal_order: + self._flag(node, f"{name}(distinct=True, order_by=...) -> frappe drops ORDER BY for distinct queries on Postgres; sort in python instead, e.g. sorted(..., key=str.casefold)") + self.generic_visit(node) def visit_Subscript(self, node: ast.Subscript) -> None: diff --git a/erpnext/manufacturing/doctype/job_card/job_card.py b/erpnext/manufacturing/doctype/job_card/job_card.py index 51ca3180948..f36373323d4 100644 --- a/erpnext/manufacturing/doctype/job_card/job_card.py +++ b/erpnext/manufacturing/doctype/job_card/job_card.py @@ -1725,7 +1725,7 @@ def get_operations(doctype: str, txt: str, searchfield: str, start: int, page_le fields=["operation"], limit_start=start, limit_page_length=page_len, - order_by="idx asc", + order_by="idx asc", # pg-ok: dropped under distinct on PG — paging order of an operation autocomplete only as_list=1, distinct=True, ) diff --git a/erpnext/manufacturing/doctype/work_order/test_work_order.py b/erpnext/manufacturing/doctype/work_order/test_work_order.py index f65cd78f178..34ba1696768 100644 --- a/erpnext/manufacturing/doctype/work_order/test_work_order.py +++ b/erpnext/manufacturing/doctype/work_order/test_work_order.py @@ -1048,7 +1048,7 @@ class TestWorkOrder(ERPNextTestSuite): job_cards = frappe.get_all( "Job Card Time Log", fields=["parent as name", "docstatus"], - order_by="creation asc", + order_by="creation asc", # pg-ok: dropped under distinct on PG — set is just iterated to cancel, order irrelevant distinct=True, ) diff --git a/erpnext/stock/doctype/inventory_dimension/inventory_dimension.py b/erpnext/stock/doctype/inventory_dimension/inventory_dimension.py index 4a3c89d8eae..17ae0cc6c61 100644 --- a/erpnext/stock/doctype/inventory_dimension/inventory_dimension.py +++ b/erpnext/stock/doctype/inventory_dimension/inventory_dimension.py @@ -409,7 +409,7 @@ def get_inventory_dimensions(): "validate_negative_stock", "name as dimension_name", ], - order_by="creation", + order_by="creation", # pg-ok: dropped under distinct on PG — config-list iteration order only, not data distinct=True, )