fix(dashboard): preserve native filter keys for dataset-less filters on save - #42898
fix(dashboard): preserve native filter keys for dataset-less filters on save#42898eschutho wants to merge 1 commit into
Conversation
…on save The filter-config modal's save transform discriminated between an in-progress form item and an already-saved filter by checking for a `dataset` key. Filter types without a dataset control (e.g. `filter_time`) have no such key, so their form state was misclassified as an already-saved filter and persisted verbatim. This dropped serialized keys (`targets`, `defaultDataMask`, `cascadeParentIds`) and leaked form-only keys (`defaultValueQueriesData`, raw `requiredFirst`) into the stored `native_filter_configuration`. Discriminate on the `targets` array instead, which every saved filter carries and no form item does, so dataset-less filters are serialized through the same path as dataset-backed ones. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #669f52Actionable 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
the
dashboard:native-filters
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| // dropping serialized keys such as `targets`, `defaultDataMask`, and | ||
| // `cascadeParentIds`, and leaking form-only keys such as | ||
| // `defaultValueQueriesData` and `requiredFirst`. | ||
| return !('targets' in formInputs); |
There was a problem hiding this comment.
Suggestion: Using property presence alone makes legacy or partially migrated saved filters without a targets field look like form inputs. The dashboard metadata schema accepts arbitrary filter dictionaries, so such entries can reach this path; they will then be rebuilt by transformFormInput, replacing the saved object and discarding fields such as chartsInScope, tabsInScope, and any other persisted target-related data. Preserve the saved-filter path for legacy entries using an additional saved-filter discriminator or normalize missing targets before this check. [api mismatch]
Severity Level: Major ⚠️
- ❌ Editing legacy filters discards persisted configuration fields.
- ❌ Filter scope metadata can be lost during dashboard saves.
- ⚠️ Unknown future filter properties are not preserved.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/transformers/filterTransformer.ts
**Line:** 79:79
**Comment:**
*Api Mismatch: Using property presence alone makes legacy or partially migrated saved filters without a `targets` field look like form inputs. The dashboard metadata schema accepts arbitrary filter dictionaries, so such entries can reach this path; they will then be rebuilt by `transformFormInput`, replacing the saved object and discarding fields such as `chartsInScope`, `tabsInScope`, and any other persisted target-related data. Preserve the saved-filter path for legacy entries using an additional saved-filter discriminator or normalize missing targets before this check.
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 original implementation relied on the presence of a The fix implemented in the PR correctly updates the I have verified the changes in superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/transformers/filterTransformer.ts |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42898 +/- ##
==========================================
- Coverage 66.38% 66.38% -0.01%
==========================================
Files 2857 2857
Lines 161163 161163
Branches 37074 37074
==========================================
- Hits 106988 106982 -6
- Misses 52153 52159 +6
Partials 2022 2022
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:
|
1 participant
SUMMARY
Saving any filter through the dashboard filter-configuration modal strips
several keys from the stored
native_filter_configurationentry of nativefilters that have no dataset — most notably Time Range (
filter_time)filters.
filter_selectfilters are unaffected.Problem
The modal's save path transforms each modified filter's form state into the
object written to
json_metadata. It decides whether a value is anin-progress form item (needs full serialization) or an already-saved filter
(pass-through) by checking for a
datasetkey:Filter types without a dataset control never populate a
datasetfield ontheir form item (their plugin declares
datasourceCount: 0, so the datasetcontrol is not rendered). Those form items are therefore misclassified as
already-saved filters and persisted verbatim. As a result the serialized
entry:
targets,defaultDataMask, andcascadeParentIds(built during theform → filter transform, which was skipped), and
defaultValueQueriesData: nulland a rawrequiredFirst: {}.Because the modal rewrites every modified entry, editing/renaming/reordering
any filter on the dashboard triggers this for the dataset-less filters.
Fix
Discriminate on the
targetsarray instead ofdataset. Every saved filtercarries a serialized
targetsarray and no form item ever does, sodataset-less filters now flow through the same serialization path as
dataset-backed ones. The change is limited to the affected entry — insertion
order and untouched entries are preserved.
TESTING INSTRUCTIONS
Automated:
transformers/filterTransformer.test.tscovers a dataset-less(
filter_time) form item, a dataset-backed (filter_select) form item, andan already-saved filter passed through from the config map. The first case
fails on
master(missingtargets) and passes with this change.Manual:
GET /api/v1/dashboard/{id}and note the Time Range entry inresult.json_metadatahastargets,defaultDataMask, andcascadeParentIds.without changing anything else.
GET /api/v1/dashboard/{id}again: only the name changes; the previouslystripped keys are preserved. (Verify against the API response, not the
Edit properties → JSON Metadata panel, which does not render the stored
document.)
ADDITIONAL INFORMATION
🤖 Generated with Claude Code