refactor: centralize circular query dependency detection (#1032)

(cherry picked from commit b03eb3b)

Author: nextchamp-saqib

.gitignore

@@ -185,3 +185,4 @@ pids
 *.pid.lock
 todo
 frontend/package-lock.json
+.opencode/package-lock.json

frontend/src2/query/query.ts

@@ -125,15 +125,18 @@ export function makeQuery(name: string) {
 
 	const dataSource = computed(() => getDataSource(currentOperations.value))
 
-	function getDataSource(operations: Operation[]): string {
+	function getDataSource(operations: Operation[], _visited: Set<string> = new Set()): string {
 		const sourceOp = operations.find((op) => op.type === 'source')
 		if (!sourceOp) return ''
 		if (sourceOp.table.type === 'table') {
 			return sourceOp.table.data_source
 		}
 		if (sourceOp.table.type === 'query' && 'query_name' in sourceOp.table) {
-			const sourceQuery = useQuery(sourceOp.table.query_name)
-			return getDataSource(sourceQuery.currentOperations)
+			const queryName = sourceOp.table.query_name
+			if (_visited.has(queryName)) return ''
+			_visited.add(queryName)
+			const sourceQuery = useQuery(queryName)
+			return getDataSource(sourceQuery.currentOperations, _visited)
 		}
 		return ''
 	}
@@ -813,7 +816,7 @@ export function makeQuery(name: string) {
 		operations: Operation[],
 		currRow: QueryResultRow,
 		col: QueryResultColumn,
-		_visitedQueryNames: string[] = [],
+		_visitedQueries: Set<string> = new Set(),
 	): Promise<{ ops: Operation[]; filters: FilterArgs[] } | null> {
 		// If there's a local summarize/pivot, no inlining needed
 		const hasSummarizeOrPivot = operations.find(
@@ -857,7 +860,7 @@ export function makeQuery(name: string) {
 		const sourceQueryName = sourceOp.table.query_name
 
 		// Guard against circular references
-		if (_visitedQueryNames.includes(sourceQueryName)) {
+		if (_visitedQueries.has(sourceQueryName)) {
 			createToast({
 				title: __('Failed to drill down'),
 				message: __('Drill down is only supported on summarized data'),
@@ -884,7 +887,7 @@ export function makeQuery(name: string) {
 			mergedOps,
 			currRow,
 			col,
-			[..._visitedQueryNames, sourceQueryName],
+			_visitedQueries.add(sourceQueryName),
 		)
 	}
 

frontend/src2/workbook/workbook.ts

@@ -448,7 +448,10 @@ export function newWorkbookName() {
 	return `new-workbook-${unique_id}`
 }
 
-export function getLinkedQueries(query_name: string): string[] {
+export function getLinkedQueries(query_name: string, _visited: Set<string> = new Set()): string[] {
+	if (_visited.has(query_name)) return []
+	_visited.add(query_name)
+
 	const query = useQuery(query_name)
 	const linkedQueries = new Set<string>()
 
@@ -467,7 +470,7 @@ export function getLinkedQueries(query_name: string): string[] {
 		}
 	})
 
-	linkedQueries.forEach((q) => getLinkedQueries(q).forEach((q) => linkedQueries.add(q)))
+	linkedQueries.forEach((q) => getLinkedQueries(q, _visited).forEach((q) => linkedQueries.add(q)))
 
 	return Array.from(linkedQueries)
 }

insights/insights/doctype/insights_data_source_v3/ibis_utils.py

@@ -32,6 +32,12 @@
 from .ibis.utils import get_functions
 
 
+class CircularQueryReferenceError(frappe.ValidationError):
+    """Raised when a circular query reference is detected during query building."""
+
+    pass
+
+
 class IbisQueryBuilder:
     def __init__(self, doc, active_operation_idx=None):
         self.doc = doc
@@ -72,20 +78,34 @@ def set_operations(self):
         self.operations = operations
 
     def build(self) -> IbisQuery:
-        self.query = None
-        for idx, operation in enumerate(self.operations):
-            try:
-                operation = _dict(operation)
-                self.query = self.perform_operation(operation)
-            except BaseException as e:
-                operation_type_title = frappe.bold(operation.type.title())
-                create_toast(
-                    title=f"Failed to Build {self.title} Query",
-                    message=f"Please check the {operation_type_title} operation at position {idx + 1}",
-                    type="error",
-                )
-                raise e
-        return self.query
+        if not hasattr(frappe.local, "_insights_building_queries"):
+            frappe.local._insights_building_queries = set()
+
+        if self.doc.name in frappe.local._insights_building_queries:
+            raise CircularQueryReferenceError(
+                frappe._('Circular query reference detected while building "{0}"').format(self.title)
+            )
+
+        frappe.local._insights_building_queries.add(self.doc.name)
+        try:
+            self.query = None
+            for idx, operation in enumerate(self.operations):
+                try:
+                    operation = _dict(operation)
+                    self.query = self.perform_operation(operation)
+                except CircularQueryReferenceError:
+                    raise
+                except BaseException as e:
+                    operation_type_title = frappe.bold(operation.type.title())
+                    create_toast(
+                        title=f"Failed to Build {self.title} Query",
+                        message=f"Please check the {operation_type_title} operation at position {idx + 1}",
+                        type="error",
+                    )
+                    raise e
+            return self.query
+        finally:
+            frappe.local._insights_building_queries.discard(self.doc.name)
 
     def perform_operation(self, operation):
         if operation.type == "source":

insights/insights/doctype/insights_query_v3/insights_query_v3.py

@@ -13,10 +13,16 @@
 
 from insights.decorators import insights_whitelist
 from insights.insights.doctype.insights_data_source_v3.ibis_utils import (
+    CircularQueryReferenceError,
     IbisQueryBuilder,
     execute_ibis_query,
     get_columns_from_schema,
 )
+from insights.insights.query_utils import (
+    extract_query_deps_from_operations,
+    find_cycle,
+    transitive_closure,
+)
 from insights.utils import deep_convert_dict_to_dict
 
 
@@ -67,6 +73,27 @@ def on_trash(self):
         if self.folder:
             self.cleanup_empty_folder(self.folder)
 
+    def validate(self):
+        self._validate_no_circular_dependency()
+
+    def _validate_no_circular_dependency(self):
+        """Raise an error if the current operations would create a circular query reference."""
+        operations = frappe.parse_json(self.operations) or []
+        new_direct_deps = extract_query_deps_from_operations(operations)
+
+        if not new_direct_deps:
+            return
+
+        cycle = find_cycle(self.name, new_direct_deps)
+        if cycle:
+            path_str = " → ".join(
+                f'"{frappe.db.get_value("Insights Query v3", q, "title") or q}"' for q in cycle
+            )
+            frappe.throw(
+                f"Circular query reference detected: {path_str}",
+                exc=CircularQueryReferenceError,
+            )
+
     def before_save(self):
         self.set_linked_queries()
 
@@ -88,62 +115,33 @@ def cleanup_empty_folder(self, folder_name):
             frappe.delete_doc("Insights Folder", folder_name, force=True, ignore_permissions=True)
 
     def set_linked_queries(self):
-        operations = frappe.parse_json(self.operations)
-        if not operations:
-            return
-
-        linked_queries = []
-        for operation in operations:
-            if (
-                operation.get("table")
-                and operation.get("table").get("type") == "query"
-                and operation.get("table").get("query_name")
-            ):
-                linked_queries.append(operation.get("table").get("query_name"))
-        self.linked_queries = linked_queries
-
-    def get_source_tables(self, visited=None):
-        """Recursively collect all leaf table references from this query and its dependencies."""
-        if visited is None:
-            visited = set()
-
-        # Prevent infinite recursion from circular dependencies
-        if self.name in visited:
-            return []
-        visited.add(self.name)
+        operations = frappe.parse_json(self.operations) or []
+        self.linked_queries = extract_query_deps_from_operations(operations)
 
+    def get_source_tables(self):
+        """Collect all leaf table references from this query and its dependencies."""
+        all_query_names = {self.name} | transitive_closure(self.name)
         source_tables = []
-        operations = frappe.parse_json(self.operations)
-
-        if not operations:
-            return source_tables
-
-        # Collect direct table references from this query's operations
-        for operation in operations:
-            if operation.get("type") in ["source", "join", "union"]:
-                table = operation.get("table")
-                if table and table.get("type") == "table":
-                    table_info = {
-                        "data_source": table.get("data_source"),
-                        "table_name": table.get("table_name"),
-                    }
-                    if table_info not in source_tables:
-                        source_tables.append(table_info)
-
-        # Recursively collect tables from linked queries
-        linked_queries = frappe.parse_json(self.linked_queries)
-        if linked_queries:
-            for query_name in linked_queries:
-                if query_name not in visited:
-                    try:
-                        linked_query = frappe.get_doc("Insights Query v3", query_name)
-                        linked_tables = linked_query.get_source_tables(visited)
-                        for table_info in linked_tables:
-                            if table_info not in source_tables:
-                                source_tables.append(table_info)
-                    except Exception:
-                        # Skip if linked query doesn't exist or can't be loaded
-                        continue
+
+        for query_name in all_query_names:
+            if query_name == self.name:
+                operations = frappe.parse_json(self.operations) or []
+            else:
+                operations = (
+                    frappe.parse_json(frappe.db.get_value("Insights Query v3", query_name, "operations"))
+                    or []
+                )
+
+            for operation in operations:
+                if operation.get("type") in ["source", "join", "union"]:
+                    table = operation.get("table")
+                    if table and table.get("type") == "table":
+                        table_info = {
+                            "data_source": table.get("data_source"),
+                            "table_name": table.get("table_name"),
+                        }
+                        if table_info not in source_tables:
+                            source_tables.append(table_info)
 
         return source_tables
 

insights/insights/query_utils.py

@@ -0,0 +1,61 @@
+# Copyright (c) 2025, Frappe Technologies Pvt. Ltd. and contributors
+# For license information, please see license.txt
+
+import frappe
+
+
+def extract_query_deps_from_operations(operations: list) -> list[str]:
+    """Extract all referenced query names from a list of operations."""
+    return [
+        op["table"]["query_name"]
+        for op in operations
+        if op.get("table")
+        and op.get("table", {}).get("type") == "query"
+        and op.get("table", {}).get("query_name")
+    ]
+
+
+def get_direct_dependencies(query_name: str) -> list[str]:
+    """Return the list of query names this query directly depends on, from the DB."""
+    linked_queries = frappe.db.get_value("Insights Query v3", query_name, "linked_queries")
+    return frappe.parse_json(linked_queries) or []
+
+
+def transitive_closure(start: str) -> set[str]:
+    """Return all query names reachable from start (not including start itself)."""
+    reachable: set[str] = set()
+    stack = list(get_direct_dependencies(start))
+    while stack:
+        node = stack.pop()
+        if node in reachable:
+            continue
+        reachable.add(node)
+        stack.extend(get_direct_dependencies(node))
+    return reachable
+
+
+def find_cycle(start: str, new_direct_deps: list[str]) -> list[str] | None:
+    """
+    Check whether declaring `new_direct_deps` as the direct dependencies of `start`
+    would form a cycle. Returns the cycle path (e.g. ["A", "B", "A"]) or None.
+
+    Used during validate() to catch cycles before they are persisted.
+    """
+    for dep in new_direct_deps:
+        path = _find_path(dep, target=start, path=[start, dep], visited=set())
+        if path is not None:
+            return path
+    return None
+
+
+def _find_path(current: str, target: str, path: list[str], visited: set[str]) -> list[str] | None:
+    if current == target:
+        return path
+    if current in visited:
+        return None
+    visited.add(current)
+    for dep in get_direct_dependencies(current):
+        result = _find_path(dep, target, [*path, dep], visited)
+        if result is not None:
+            return result
+    return None