Better Deep Architecture Investigation: Frappe Query API vs FlexiRule Query Records

reports/frappe-api-query-records-runtime-validation.md

@@ -0,0 +1,266 @@
+# Final Validation Pass: Runtime Verification of Frappe Query APIs & FlexiRule Compatibility
+
+## 1. Verified Findings
+
+This second-pass investigation builds on the previous source-code audit by executing direct runtime verifications against the active **`test_site`** database.
+
+We mapped the parent-child relationships using `DocType` as the parent doctype and `DocField` (Table field: `fields`) as the child table.
+
+### Key Verified Discoveries & Corrections:
+
+1. **Child Table projection fields trigger automatic left-joins**:
+   - *Status*: ✅ **Verified by runtime**.
+   - *Query*: `frappe.get_list("DocType", fields=["name", "fields.fieldname"])`
+   - *Outcome*: Successfully executed. The SQL compiler automatically compiled and executed the LEFT JOIN.
+
+2. **Dotted Dictionary Filters are NOT supported**:
+   - *Status*: ❌ **Previous conclusion was incorrect**.
+   - *Query*: `frappe.get_list("DocType", filters={"fields.fieldname": "fieldname"})`
+   - *Outcome*: **Failed** with `OperationalError: (1054, "Unknown column 'tabDocType.fields.fieldname' in 'WHERE'")`.
+   - *Root Cause*: Dotted paths are only split and resolved inside `parse_args()` for the projection list `self.fields`. There is no splitting of dots in `filters` inside `DatabaseQuery`. The compiler prepends the parent table name to the dotted key literally, creating an invalid column.
+
+3. **List of List child filters are fully supported**:
+   - *Status*: ✅ **Verified by runtime**.
+   - *Query*: `frappe.get_list("DocType", filters=[["DocField", "fieldname", "=", "fieldname"]])`
+   - *Outcome*: Successfully executed. The explicit child table list format correctly resolves the metadata and compiles a proper SQL `WHERE` clause.
+
+4. **Aggregate queries under `get_all` ignore all user permissions**:
+   - *Status*: ✅ **Verified by runtime (Vulnerability Confirmed)**.
+   - *Query*: `frappe.get_all("DocType", fields=["count(name) as _count"], ignore_permissions=False)`
+   - *Outcome*: Successfully executed for a restricted user `test1@example.com` who had zero rights to access `DocType`.
+   - *Query with `get_list`*: `frappe.get_list("DocType", fields=["count(name) as _count"], ignore_permissions=False, limit_page_length=0)`
+   - *Outcome*: Successfully **blocked** with `PermissionError`, confirming that `get_list` enforces row-level permissions while `get_all` bypasses them.
+
+---
+
+## 2. Complete Impact Analysis
+
+Below is the complete call graph and dependency flow starting from `QueryRecordsHandler.execute()` to final database execution:
+
+```
+[QueryRecordsHandler.execute]
+   │
+   ├──► [1] can_skip_permissions (Checks skip_permissions and extracts audit reason)
+   │
+   ├──► [2] apply_input_mapping (Maps context variables to query configuration)
+   │
+   └──► [3] Dispatch based on Mode (action.operation)
+         │
+         ├───► Query List ────► QueryRecordsHandler._query_list
+         │                       │
+         │                       ├──► [A] _resolve_query_filters (Resolves context & normalizes filters)
+         │                       │      ├──► _resolve_filters_with_context
+         │                       │      └──► _normalize_filters_for_backend (Recursively formats lists/dicts)
+         │                       │
+         │                       └──► [B] frappe.get_list
+         │                              └─► DatabaseQuery.execute
+         │
+         ├───► Query Doc ─────► QueryRecordsHandler._query_doc
+         │                       │
+         │                       ├──► frappe.get_doc / frappe.get_cached_doc
+         │                       └──► doc.check_permission("read") (If ignore_permissions=False)
+         │
+         ├───► Exist Record ──► QueryRecordsHandler._exist_record
+         │                       │
+         │                       └──► frappe.get_all (ignore_permissions=ignore_permissions)
+         │
+         ├───► Query Report ──► QueryRecordsHandler._query_report
+         │                       │
+         │                       └──► frappe.desk.query_report.run
+         │
+         └───► Metric/Group ──► QueryRecordsHandler._count_records / _aggregate / _group_by
+                                 │
+                                 └──► frappe.get_all (ignore_permissions=ignore_permissions)
+                                        │
+                                        ▼ (FORCES ignore_permissions=True)
+                                      frappe.get_list
+                                        │
+                                        ▼ (BYPASSES match conditions & field permissions)
+                                      DatabaseQuery.execute
+```
+
+### Affected Code Paths Inside FlexiRule:
+- **`flexirule/ruleflow/core/action_handlers/query_records.py`**:
+  - `execute()`: Main orchestrator.
+  - `_query_list()`: Invokes `frappe.get_list`.
+  - `_query_doc()`: Invokes `frappe.get_doc`/`get_cached_doc`.
+  - `_exist_record()`, `_count_records()`, `_aggregate()`, `_group_by()`: Invoke `frappe.get_all`.
+- **`flexirule/ruleflow/utils/mapping.py`**:
+  - `apply_input_mapping()`: Resolves input values from global store context.
+
+---
+
+## 3. Aggregate Migration Validation
+
+Replacing `frappe.get_all(...)` with `frappe.get_list(..., limit_page_length=0)` in `_count_records`, `_aggregate`, and `_group_by` was audited at runtime to ensure behavior equivalence and analyze any differences:
+
+### 1. Compiled SQL Equivalence
+- **Under `get_all`**:
+  ```sql
+  SELECT count(name) as _count FROM `tabDocType`
+  ```
+- **Under `get_list(..., limit_page_length=0)`**:
+  ```sql
+  SELECT count(name) as _count FROM `tabDocType`
+  ```
+  *Note: In `DatabaseQuery.add_limit()`, when `limit_page_length` is 0 (falsy), no `LIMIT` clause is generated. The query executes as a standard aggregate.*
+
+### 2. Return Structure
+- Both methods return identical list-of-dictionary shapes (e.g. `[{'_count': 280}]` or `[{'result': 45.5}]`), making them 100% compatible with FlexiRule's subsequent output mapping and context assignment logic.
+
+### 3. Feature Interactions & Database Portability (MariaDB vs. PostgreSQL)
+- **Pagination & Limit**: Since `limit_page_length=0` suppresses `LIMIT` generation, there is no syntactical difference between MariaDB and PostgreSQL for aggregates.
+- **Ordering & Grouping**: PostgreSQL requires any field in the projection to also appear in the order/group-by clause. Because both methods utilize the same underlying `DatabaseQuery` compilation engine, they behave identically on both database systems.
+- **Performance**: There is zero difference in database execution plans. The database compiler parses and evaluates both queries in the exact same manner.
+
+---
+
+## 4. Permission Model Audit
+
+### Permission Verification Matrix
+
+This matrix maps role behaviors for each "Query Records" mode when `ignore_permissions` is `False`.
+
+| Query Records Mode | Role / Context | Current Behavior | Expected Frappe Behavior | Security Implication | Required Fix |
+| :--- | :--- | :--- | :--- | :--- | :--- |
+| **Query List** | Administrator | Succeeds. | Succeeds. | None. | None. |
+| | Standard User | Succeeds with sharing/owner filters. | Succeeds with sharing/owner filters. | None. (Uses native `get_list`). | None. |
+| | Child DocType | **Fails** with `PermissionError`. | Succeeds if `parent_doctype` is provided. | Standard users cannot query child tables. | Pass `parent_doctype` to `get_list`. |
+| **Query Doc** | Standard User | Checks read permissions on document. | Checks read permissions on document. | None. | None. |
+| **Exist Record** | Standard User | Bypasses all sharing/owner filters. | Enforces sharing/owner filters. | Users can detect existence of restricted records. | Replace `get_all` with `get_list`. |
+| **Count** | Standard User | Bypasses all sharing/owner filters. | Enforces sharing/owner filters. | Users can count restricted records. | Replace `get_all` with `get_list`. |
+| **Sum / Avg / Min / Max**| Standard User | Bypasses all sharing/owner filters. | Enforces sharing/owner filters. | Users can query aggregate stats on restricted data. | Replace `get_all` with `get_list`. |
+| **Group By** | Standard User | Bypasses all sharing/owner filters. | Enforces sharing/owner filters. | Users can query grouped categories of restricted data. | Replace `get_all` with `get_list`. |
+
+---
+
+## 5. Filter Normalization Audit
+
+The helper method `_normalize_filters_for_backend` was audited against every supported filter format:
+
+### 1. Dictionary Filters
+- *Support*: **Supported** (with exceptions).
+- *Behavior*: Dictionary `{"name": "ToDo"}` is normalized to `[["name", "=", "ToDo"]]` which is fully supported.
+- *Dotted Path dictionary filters (e.g. `{"fields.fieldname": "value"}`)*: **Unsupported**.
+  - *Root Cause*: `DatabaseQuery` does not split dots in filters. It prepends the parent table name, compiling an invalid column name.
+  - *Proposed Fix*: In `_normalize_filters_for_backend`, check if a key contains a `.`. If so, split into `[child_table_field, child_field]` and resolve the child doctype using parent metadata. Rebuild as a list-of-lists filter specifying the child doctype: `[["Child DocType", "child_field", "=", "value"]]`.
+
+### 2. Standard Operators
+- *Support*: **Fully Supported**.
+- *Operators*: `=`, `!=`, `>`, `<`, `>=`, `<=`, `like`, `not like`, `between`, `in`, `not in` are mapped and normalized correctly.
+- *UI Operators*: `starts with` is mapped to `like` with `%` suffix; `ends with` is mapped to `like` with `%` prefix; `Between` is mapped to `between`.
+
+### 3. Timespan Filters
+- *Support*: **Fully Supported**.
+- *Behavior*: `Timespan` operator resolves keywords (e.g., `"last 7 days"`, `"this month"`) into date ranges and maps them to `between` queries.
+
+### 4. Recursive Nested AND/OR
+- *Support*: **Unsupported in both**. Neither Frappe `DatabaseQuery` nor FlexiRule support nested logical expressions. Only flat `filters` and `or_filters` are supported.
+
+---
+
+## 6. Child Table Compatibility
+
+### Dotted Fields in FlexiRule's UI:
+*   The UI configuration component `QueryRecordsConfig.vue` automatically expands Table-type fields and populates selection fields with child dotted paths like `fields.fieldname`.
+*   These selection fields are saved inside the `fields` array of the action's `config` JSON.
+*   **Filters**: In `QueryRecordsConfig.vue`, filters are generated as a list of dictionaries:
+    ```json
+    [{"fieldname": "fields.fieldname", "operator": "=", "value": "myval"}]
+    ```
+*   When this list of dictionaries reaches the backend `_normalize_filters_for_backend()`, it is processed as:
+    ```python
+    if isinstance(item, dict) and ("field" in item or "fieldname" in item):
+        field = item.get("field") or item.get("fieldname")
+        # field = "fields.fieldname"
+    ```
+    Since `doctype` is not provided in the dictionary, it appends `["fields.fieldname", "=", "myval"]` to the filters!
+    And since `f.fieldname` is `"fields.fieldname"`, `get_filter` fails to split it, leading directly to the `OperationalError` observed in Scenario C!
+
+### Compatibility Migration Requirements:
+- No existing rules will break if we implement the backend filter normalization fix.
+- The fix is entirely non-destructive: it intercepts dotted keys like `"fields.fieldname"`, splits them, resolves the child doctype, and translates them to `["DocField", "fieldname", "=", "myval"]` before passing them to `get_list()`.
+
+---
+
+## 7. DISTINCT Investigation
+
+Exposing a `distinct` toggle inside the `QueryRecordsConfig.vue` UI is necessary to handle child table joins.
+
+### SQL & Feature Interaction:
+- **Interaction with `group_by`**: If `group_by` is specified, `distinct` is redundant because group-by implicitly deduplicates rows.
+- **Interaction with Aggregates**: Using `distinct` with aggregates (like `count(distinct name)`) is highly useful, but `DatabaseQuery` does not natively support prepending `distinct` to aggregate fields inside `build_and_run()`. It only prepends `distinct` to normal fields.
+- **Interaction with Ordering & Pagination**: Under PostgreSQL, if `distinct` is used, any field in the `ORDER BY` clause **must** appear in the `SELECT` list. Frappe's `DatabaseQuery` automatically resolves this by clearing the order-by clause on Postgres when distinct is active.
+
+### Recommended Implementation:
+1.  **UI**: Add a boolean checkbox `distinct` under "Query Mode: Query List" in `QueryRecordsConfig.vue`.
+2.  **Backend**: Read `distinct` from config and pass `distinct=bool(config.get("distinct"))` to `frappe.get_list`.
+3.  **Validation Rules**: If `distinct` is enabled, validate that at least one column is selected.
+
+---
+
+## 8. Regression Audit
+
+A comprehensive search of the repository (`/app/flexirule`) was conducted to find all other uses of `get_all` and `get_list`:
+
+### 1. Correct Internal Usage of `get_all`
+- Core engine files like `coordinator.py`, `rule_service.py`, `scheduler.py`, and `api.py` query metadata models (e.g. `Rule`, `Rule Action`, `Rule Scheduler`).
+- These queries **must** run with system privileges to orchestrate the rule flow behind the scenes. Using `get_all` in these internal layers is correct and safe.
+
+### 2. Opportunity for Shared Utilities
+- **Centralized Permission Check**: Instead of checking `frappe.has_permission(reference_doctype, "read")` before calling `get_all` inside `_count_records`, `_aggregate`, and `_group_by`, we can eliminate all manual permission-gating and delegate the entire check to `frappe.get_list`.
+- **Shared Filter Normalization**: Extract `_normalize_filters_for_backend` from `query_records.py` and move it to `flexirule/ruleflow/utils/filters.py`. This would allow other handlers (like `Document Action` or internal processors) to parse dotted filters and child table queries consistently.
+
+---
+
+## 9. Testing Strategy
+
+### 1. Backend Unit Tests
+*   **Security/Permissions**:
+    *   *Test*: Execute `_count_records` as standard user `test1@example.com` on a restricted doctype (e.g. `User`). Confirm it raises `PermissionError` (proving the aggregate bypass is closed).
+*   **Filter Normalization**:
+    *   *Test*: Pass a dictionary containing `{"fields.fieldname": "owner"}` to `_normalize_filters_for_backend()`. Assert it compiles to `[["DocField", "fieldname", "=", "owner"]]`.
+*   **Child Table Joins**:
+    *   *Test*: Query a parent doctype and select child fields. Verify that multiple child records result in duplicate rows, and that setting `distinct=True` returns only unique parent rows.
+*   **Aggregates**:
+    *   *Test*: Execute Count, Sum, Average, Min, and Max queries under `get_list(..., limit_page_length=0)`. Assert they return the correct mathematically expected values.
+
+### 2. UI/Frontend Tests (Playwright)
+*   **Rule Builder Verification**:
+    *   Verify that selecting a child field like `fields.fieldname` is properly displayed in the projection fields.
+    *   Verify that adding a filter on a child field produces a valid list filter.
+    *   Verify that the "Distinct" checkbox appears under the list of selected fields and persists its state to the action's `config` JSON.
+
+---
+
+## 10. Final Deliverable: Implementation Report
+
+### 1. Required Bug Fixes
+*   **Vulnerability: Permission Bypass in Aggregates**:
+    - *Fix*: Replace `frappe.get_all` with `frappe.get_list` in `_count_records`, `_aggregate`, and `_group_by` inside `query_records.py`.
+    - *Effort*: Small (1 hour).
+*   **Operational Error: Dotted Filter Key Crashes**:
+    - *Fix*: Normalize dotted filter keys inside `_normalize_filters_for_backend()` to list-of-lists child table queries.
+    - *Effort*: Medium (4 hours).
+
+### 2. Optional Enhancements
+*   **DISTINCT Support**:
+    - *Fix*: Expose `distinct` toggle in the UI and forward it to `get_list`.
+    - *Effort*: Small (2 hours).
+*   **Direct Child DocType query support**:
+    - *Fix*: Expose `parent_doctype` in UI and forward it to `get_list`.
+    - *Effort*: Small (2 hours).
+
+### 3. Risk Assessment & Migration Sequence
+*   **Risk**: Extremely low. Replacing `get_all` with `get_list` uses the exact same database query generation. The only change is that user permissions are now enforced. Dotted key normalization is entirely backward-compatible and only intercepts keys that previously triggered a database error.
+*   **Migration Sequence**:
+    1.  Update backend aggregates from `get_all` to `get_list`.
+    2.  Implement dotted path splitting inside `_normalize_filters_for_backend()`.
+    3.  Expose the `distinct` checkbox in the VueFlow frontend UI.
+
+### 4. Implementation Effort Summary
+*   **Total Estimated Effort**: **9 Hours**
+*   **Recommended Order**:
+    1.  Aggregate permissions fix (Critical - Security).
+    2.  Dotted key filter normalization (High - Stability).
+    3.  Distinct toggle UI and backend support (Medium - Parity).