fix(query): resolve search filter against adhoc column labels in Table chart - #42216
fix(query): resolve search filter against adhoc column labels in Table chart#42216prathamesh04 wants to merge 2 commits into
Conversation
Code Review Agent Run #ba5f1bActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
dosubot
Bot
added
explore:filter
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42216 +/- ##
==========================================
- Coverage 65.19% 65.18% -0.01%
==========================================
Files 2768 2768
Lines 156081 156088 +7
Branches 35719 35720 +1
==========================================
- Hits 101754 101751 -3
- Misses 52365 52373 +8
- Partials 1962 1964 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| applied_filter_columns = [ | ||
| col | ||
| for col in filter_columns | ||
| if col | ||
| and not is_adhoc_column(col) | ||
| and (col in self.column_names or col in applied_template_filters) | ||
| and ( | ||
| col in self.column_names | ||
| or col in applied_template_filters | ||
| or col in adhoc_columns_by_label | ||
| ) | ||
| ] + applied_adhoc_filters_columns |
There was a problem hiding this comment.
Suggestion: The applied filter column list now treats any key present in adhoc_columns_by_label as applied, even if that filter failed to resolve and was explicitly rejected earlier. This can produce contradictory metadata where the same filter appears as both applied and rejected; only mark adhoc-label filters as applied when they were successfully resolved. [logic error]
Severity Level: Major ⚠️
- ⚠️ Filter metadata marks same adhoc label applied and rejected.
- ⚠️ Debugging filter issues harder due to inconsistent metadata.Steps of Reproduction ✅
1. Create a dataset with an adhoc column label (e.g. "Id") whose definition references a
physical column that is later removed from the datasource schema, leaving the adhoc
definition stale but still present in `adhoc_columns_by_label`
(superset/models/helpers.py, around lines 3852-3863).
2. Build a Table chart using this dataset and enable server-side pagination and search;
the chart query pipeline calls `get_sqla_query` in `superset/models/helpers.py` (filter
processing loop around lines 3841-3870) with a filter containing `{"col": "Id", ...}`.
3. During filter resolution, the block at lines 3852-3869 attempts
`self.adhoc_column_to_sqla(adhoc_columns_by_label[flt_col], ...)`; because the underlying
physical column is missing, `ColumnNotFoundException` is raised, the exception handler
appends "Id" to `rejected_adhoc_filters_columns` and continues.
4. After the loop, `get_sqla_query` computes `rejected_filter_columns` and
`applied_filter_columns` (lines 4336-4357). "Id" appears in `rejected_filter_columns` via
`+ rejected_adhoc_filters_columns`, but the comprehension at lines 4347-4356 also includes
"Id" in `applied_filter_columns` solely because `col in adhoc_columns_by_label` is true,
even though resolution failed, so the returned `SqlaQuery` reports the same filter column
as both applied and rejected.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 4347:4357
**Comment:**
*Logic Error: The applied filter column list now treats any key present in `adhoc_columns_by_label` as applied, even if that filter failed to resolve and was explicitly rejected earlier. This can produce contradictory metadata where the same filter appears as both applied and rejected; only mark adhoc-label filters as applied when they were successfully resolved.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The flagged issue is correct. The current implementation of To resolve this, the comprehension should be updated to only include adhoc columns that were successfully resolved and added to Proposed FixModify the applied_filter_columns = [
col
for col in filter_columns
if col
and not is_adhoc_column(col)
and (
col in self.column_names
or col in applied_template_filters
or col in applied_adhoc_filters_columns
)
] + applied_adhoc_filters_columnsThis ensures that only successfully resolved adhoc filters are included in the superset/models/helpers.py |
…e chart
When a column label is renamed in the Table chart (e.g. 'CustomerID' to
'Id'), the search filter sends the display label ('Id') but the backend
columns_by_name dict is keyed by physical column names. This causes the
filter to be silently rejected with 'not_in_datasource'.
Add a fallback that resolves the filter column against adhoc_columns_by_label
(the same pattern used by PR apache#37521 for ORDER BY). Also update the
rejected/applied filter columns computation to recognize adhoc column labels.
Fixes apache#38339
be1ff79 to
7ed8fa6
Compare
Code Review Agent Run #31201bActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Reviewed at HEAD 7ed8fa604. This is a clean, well-scoped fix — walked the resolution path in superset/models/helpers.py, compared it against #37521's ORDER BY handling, and traced the SQL-generation path through adhoc_column_to_sqla/sanitize_clause. Found one real (non-security) correctness bug in the applied/rejected bookkeeping and a couple of test gaps. Nothing here should block merge.
Correctness — looks right
- Resolution order in the filter loop (
superset/models/helpers.py:3813-3870) is:DTTM_ALIAS→ full adhoc-filter-object (is_adhoc_column(flt_col)) → physicalcolumns_by_name→verbose_name→ new adhoc-label fallback (adhoc_columns_by_label) → metric name. The new block only runs whencol_obj is None and sqla_col is None, so it can't shadow a physical column filter — no regression risk for the common (non-renamed) path. - It reuses the same
adhoc_columns_by_labeldict already built athelpers.py:3572-3578for ORDER BY resolution (introduced by #37371's refactor of #37521's pattern), rather than rebuilding it — good DRY, matches the ask in the PR description.
Security verdict: within the existing trust model, not a new injection surface
Traced this end-to-end because the filter column now resolves to a user-authored adhoc sqlExpression:
adhoc_columns_by_label(helpers.py:3572-3578) is populated only from thecolumnsalready selected in this query object — i.e. the expression must already be part of the chart's SELECT list. This PR doesn't let the filter itself smuggle in a new expression; it only lets an already-selected adhoc column's SQL be reached from a search-filter label instead of a full adhoc-filter object.- The resolution call (
adhoc_column_to_sqla,superset/connectors/sqla/models.py:1775) is the exact function already used for SELECT, GROUP BY (helpers.py:3671) and ORDER BY (helpers.py:3610-3614), and for the pre-existing "full adhoc filter object in WHERE" path that already shipped before this PR (helpers.py:3828-3838,is_adhoc_column(flt_col)). Non-metadata expressions go through_process_select_expression→sanitize_clause(models.pyaround line 1832,helpers.py:1332) — same guard, not bypassed. - Value binding is untouched:
sqla_col.ilike(eq)(helpers.py:4060) uses SQLAlchemy bind params regardless of whethersqla_colcame from a physical column or an adhoc expression —valwas never string-concatenated before or after this change. - Net: arbitrary adhoc-expression-in-WHERE was already reachable pre-PR via the full adhoc-filter-object path. This PR narrows, rather than widens, that same capability to a specific already-selected label. No new principal gains WHERE-reachability they didn't already have.
Bug found — applied/rejected filter columns can disagree (MEDIUM)
helpers.py:4337-4357:
rejected_filter_columns = [... and col not in adhoc_columns_by_label] + rejected_adhoc_filters_columns
applied_filter_columns = [... or col in adhoc_columns_by_label] + applied_adhoc_filters_columnsBoth list comprehensions gate on col in adhoc_columns_by_label — i.e. "the label matched", not "resolution succeeded." Compare to the pre-existing full-adhoc-object path, which is cleanly excluded from both base comprehensions via is_adhoc_column(col) == True and is governed only by the try/except append. For the new string-label path, is_adhoc_column(col) is False (it's a plain string, not a dict — see utils/core.py:1287-1290), so the base-list membership check fires independently of whether adhoc_column_to_sqla(force_type_check=True) at helpers.py:3861-3866 actually succeeded.
Concretely: if a label matches adhoc_columns_by_label but the type-probe DB query raises ColumnNotFoundException (connectors/sqla/models.py:1843-1879 — this is a real DB round-trip, e.g. SELECT expr WHERE FALSE, that can genuinely fail), the column is correctly appended to rejected_adhoc_filters_columns (helpers.py:3869, filter is skipped via continue) — but it also satisfies col in adhoc_columns_by_label in the applied_filter_columns comprehension, so it ends up in both rejected_filter_columns and applied_filter_columns simultaneously. On the success path it's merely a harmless duplicate (added once via the base list, once via applied_adhoc_filters_columns), but on the failure path it's a genuine contradiction in the API response metadata that could mislead the frontend/user about whether the filter was actually applied.
Suggested fix: add and col not in adhoc_columns_by_label to the base applied_filter_columns comprehension (mirroring how the full-adhoc-object case is excluded via is_adhoc_column), and let applied_adhoc_filters_columns be the sole source of truth for label-resolved filters — same as it already is for the dict-object case.
Test quality
test_filter_by_adhoc_column_label_resolves_to_sql_expressionis non-vacuous — compiles the query withliteral_bindsand asserts on the actual WHERE clause text, not just no-exception. Good.test_unknown_column_still_rejected_without_adhoc_matchandtest_mixed_adhoc_and_physical_column_filterscover the negative and no-regression cases well.- Gaps (not blocking, nice to add):
- No test for the resolution-failure path (
ColumnNotFoundExceptionafter a label match) — this is exactly the scenario that triggers the applied/rejected dual-membership bug above. - No test for a label that collides with an existing physical column name (confirms physical columns win, per the
col_obj is Noneguard). test_adhoc_column_label_filter_not_in_rejected_columns(the==op case) only asserts list membership, not the compiled WHERE clause — would be stronger if it also asserted the SQL text like the ILIKE test does.
- No test for the resolution-failure path (
Nice work overall — the fix is targeted, reuses established infrastructure instead of introducing a parallel code path, and the security-sensitive part checks out against the existing #37521/#37371 pattern. The applied/rejected list issue is worth a follow-up fix but is a UI/metadata correctness nit, not a blocker.
|
Thanks for tracking this down, the adhoc-label fallback mirroring #37521's ORDER BY handling makes sense. Codeant's note on the applied/rejected metadata looks right though: a column in Worth a fix before this merges. Thanks in advance, holler if you want help. |
The applied_filter_columns comprehension used 'col in adhoc_columns_by_label' which marked ALL matched labels as applied, even when adhoc_column_to_sqla raised ColumnNotFoundException. This caused columns to appear in both applied and rejected lists simultaneously. Change to check against applied_adhoc_filters_columns which only contains successfully resolved labels. Also add a test for the failure path.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| ) | ||
|
|
||
| def raise_on_bad_label(col, force_type_check=False, template_processor=None): | ||
| if getattr(col, "label", None) == "BadLabel": |
There was a problem hiding this comment.
Suggestion: The mocked argument col is a dictionary, as shown by the subsequent col.get(...) call, so getattr(col, "label", None) always returns None. Consequently, the BadLabel branch never raises ColumnNotFoundException, and this test does not exercise failed adhoc-column resolution; inspect the label using the dictionary key instead. [possible bug]
Severity Level: Major ⚠️
- ❌ Failed adhoc-resolution regression path is not exercised.
- ⚠️ Future resolution regressions may pass this test undetected.Steps of Reproduction ✅
1. Run `test_failed_adhoc_resolution_not_in_applied_columns()` at
`tests/unit_tests/models/helpers_test.py:3966`.
2. The test patches `SqlaTable.adhoc_column_to_sqla` with `raise_on_bad_label()` at
`tests/unit_tests/models/helpers_test.py:3994-3996`.
3. `get_sqla_query()` is called with the `BadLabel` adhoc column and filter at
`tests/unit_tests/models/helpers_test.py:3997-4008`, and the mocked `col` value is later
treated as a dictionary by `col.get("sqlExpression", "")` at line 3991.
4. At line 3987, `getattr(col, "label", None)` reads an attribute rather than the
dictionary key, so it returns `None` and never raises `ColumnNotFoundException` for
`BadLabel`; the test therefore does not exercise the intended failed-resolution path
before checking the applied and rejected columns at lines 4010-4012.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/models/helpers_test.py
**Line:** 3987:3987
**Comment:**
*Possible Bug: The mocked argument `col` is a dictionary, as shown by the subsequent `col.get(...)` call, so `getattr(col, "label", None)` always returns `None`. Consequently, the `BadLabel` branch never raises `ColumnNotFoundException`, and this test does not exercise failed adhoc-column resolution; inspect the label using the dictionary key instead.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
Thanks @aminghadersohi for the thorough review! I've addressed the applied/rejected filter columns bug:
Ready for re-review. cc @rusackas |
Code Review Agent Run #2917f7Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Re-reviewed at new HEAD 5531e8b4e. Verified the fix against the standing MEDIUM by actually running it, not just reading it.
Production fix (helpers.py:4352-4356): correct. Ran the exact failure scenario with a proper mock (adhoc_column_to_sqla raising ColumnNotFoundException for one label, succeeding for another) — confirms the failed label now lands only in rejected_filter_columns, not also in applied_filter_columns. The original contradiction is genuinely fixed.
But the new regression test is broken and fails when run — see inline comment. side_effect=raise_on_bad_label checks getattr(col, "label", None), but col is a plain dict (it comes straight from adhoc_columns_by_label, e.g. {"label": "BadLabel", "sqlExpression": ...}), and getattr() doesn't read dict keys — it always returns None. I ran the test as-committed and it fails:
AssertionError: assert 'BadLabel' not in ['GoodLabel', 'BadLabel', 'GoodLabel', 'BadLabel']
So test_failed_adhoc_resolution_not_in_applied_columns doesn't exercise the failure path at all — every column goes through the mock's else branch and is treated as resolved. This needs col.get("label") instead of getattr(col, "label", None). Once fixed, the test should also account for the pre-existing duplicate-entry behavior noted below (currently applied_filter_columns contains the successfully-resolved label twice — see next point — so the assertion should be assert sqla_query.applied_filter_columns.count("GoodLabel") == ... or dedupe, not just membership).
Still open (lower severity, not introduced by this commit): duplicate entries in applied_filter_columns on the success path. With a correct mock, a successfully-resolved adhoc-label filter appears twice in applied_filter_columns (once via the base list comprehension's col in applied_adhoc_filters_columns check, once via the trailing + applied_adhoc_filters_columns). Not a correctness bug in the WHERE clause, but worth a follow-up — same root pattern as the fix above (dedup or make the base comprehension exclude anything already covered by applied_adhoc_filters_columns, matching how the full-adhoc-object path is excluded via is_adhoc_column).
Also noting: CI hasn't actually run the full suite at this HEAD yet (only labeler/sync/docs-preview checks are present — the main test matrix appears to be pending trigger/approval for this fork push), so this isn't yet visible as a red CI check. mergeable is still CONFLICTING / mergeStateStatus: DIRTY.
Leaving as COMMENT (external contributor, per policy) — needs another round on the test before this is mergeable.
| ) | ||
|
|
||
| def raise_on_bad_label(col, force_type_check=False, template_processor=None): | ||
| if getattr(col, "label", None) == "BadLabel": |
There was a problem hiding this comment.
BLOCKER: this mock never triggers the failure path. col here is a plain dict (e.g. {"label": "BadLabel", "sqlExpression": ...}), and getattr(col, "label", None) does not read dict keys — it always returns None, so this condition is always False. Every call falls through to the else branch and is treated as a successful resolution.
Confirmed by actually running this test as committed:
AssertionError: assert 'BadLabel' not in ['GoodLabel', 'BadLabel', 'GoodLabel', 'BadLabel']
Fix: use col.get("label") instead of getattr(col, "label", None).
| and ( | ||
| col in self.column_names | ||
| or col in applied_template_filters | ||
| or col in applied_adhoc_filters_columns |
There was a problem hiding this comment.
Verified this correctly fixes the standing MEDIUM (ran the exact failure scenario with a proper col.get("label")-based mock — a failed adhoc-label resolution now lands only in rejected_filter_columns, not also here).
One residual issue this doesn't address: on the success path, a resolved adhoc-label filter still appears twice in applied_filter_columns — once via this comprehension (col in applied_adhoc_filters_columns) and once via the trailing + applied_adhoc_filters_columns below. Not a correctness bug in the WHERE clause, but worth a follow-up dedupe.
|
Hi @aminghadersohi — friendly ping. The PR has 4 reviews and all comments addressed. Could you take another look when you get a chance? Thanks! |
3 participants
SUMMARY
When a column label is renamed in the Table chart (e.g.
CustomerID→Id), the search filter sends{"col":"Id","op":"ILIKE","val":"..."}but the backendcolumns_by_namedict is keyed by physical column names (CustomerID). This causes the filter to be silently rejected withnot_in_datasource.Add a fallback that resolves the filter column against
adhoc_columns_by_label— the same pattern used by PR #37521 for ORDER BY. Also update the rejected/applied filter columns computation to recognize adhoc column labels.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: Search filter with renamed column produces no WHERE clause, filter is rejected:
After: Filter resolves to the underlying SQL expression and produces correct WHERE clause:
TESTING INSTRUCTIONS
CustomerID→Id)Unit test added:
test_filter_by_adhoc_column_label_resolves_to_sql_expressionAll 127 tests in
tests/unit_tests/models/helpers_test.pypass.ADDITIONAL INFORMATION