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 1 commit 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. |
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