bito-code-review[bot] commented on code in PR #44985: URL: https://github.com/apache/superset/pull/44985#discussion_r4188713345
########## superset/mcp_service/dashboard/tool/manage_native_filters.py: ########## @@ -80,6 +83,13 @@ ) +# Display strings the frontend uses when labelling a selected value; mirrored +# here so a stored default's label reads the same as an applied one. +_NULL_LABEL = "<NULL>" Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Duplicate NULL label constant</b></div> <div id="fix"> `_NULL_LABEL` duplicates `NULL_STRING` from `superset.constants` (same `"<NULL>"` literal, superset/constants.py:27), and this module already imports from `superset.constants` at line 33. Importing `NULL_STRING` keeps the stored-default label tied to the canonical constant and avoids silent divergence if the shared value changes. </div> </div> <small><i>Code Review Run #a6dd84</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset/mcp_service/dashboard/tool/manage_native_filters.py: ########## @@ -89,6 +99,105 @@ def _empty_data_mask() -> dict[str, Any]: return {"filterState": {"value": None}, "extraFormData": {}} +def _value_label(value: FilterSelectValue) -> str: + """Format one selected value the way the dashboard UI labels it.""" + if value is None: + return _NULL_LABEL + if isinstance(value, bool): + return _TRUE_LABEL if value else _FALSE_LABEL + return str(value) + + +def _select_data_mask( + conf: dict[str, Any], values: list[FilterSelectValue] +) -> dict[str, Any]: + """Build the data mask a filter_select filter produces for ``values``. + + Mirrors the frontend's ``getSelectExtraFormData``: a non-empty selection + becomes an ``IN`` predicate on the filter's target column, and an empty + selection on a filter marked ``enableEmptyFilter`` becomes an impossible + predicate (the "required filter, nothing chosen" state) rather than no + filtering at all. Shared by ``apply_dashboard_filters`` (applied values) + and this module (default values on create/update) so both paths agree. + """ + targets = [target for target in (conf.get("targets") or []) if target] + column = (targets[0].get("column") or {}).get("name") if targets else None Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Unguarded stored target shape</b></div> <div id="fix"> `_select_data_mask` assumes each stored target's `column` is a dict, but stored `native_filter_configuration` reaches here via `current_native_filter_config` (apply path, `apply_dashboard_filters.py:338`) and via `copy.deepcopy(existing)` on update, and only top-level items are validated as dicts. A truthy non-dict `column` raises AttributeError instead of `_FilterValidationError`. `_target_key` in this same file already handles both column shapes - reuse it. </div> </div> <small><i>Code Review Run #a6dd84</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset/mcp_service/dashboard/schemas.py: ########## @@ -2328,6 +2332,23 @@ class FilterSelectSpec(BaseNewFilterSpec): search_all_options: bool = Field( False, description="Query the database on search rather than client-side" ) + default_value: List[FilterSelectValue] | None = Field( + None, + description=( + "Default selected value(s), shown when a viewer opens the " + "dashboard unchanged. Omit for no default. Mutually exclusive " + "with default_to_first_item." + ), + ) + + @model_validator(mode="after") + def _validate_default_value_compat(self) -> "FilterSelectSpec": + if self.default_to_first_item and self.default_value is not None: + raise ValueError( + "default_to_first_item and default_value are mutually " + "exclusive; set at most one." + ) + return self Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Empty list bypasses exclusivity</b></div> <div id="fix"> The mutual-exclusion check uses `is not None`, so `default_value=[]` with `default_to_first_item=True` passes validation. On the create path `_build_select_config` then sets an empty `defaultDataMask` (manage_native_filters.py:305-306), silently overriding the first-item default. The update path catches this in `_merge_select_default`, but create does not. </div> </div> <div id="suggestion"> <div id="issue"><b>Duplicated validator logic</b></div> <div id="fix"> The `_validate_default_value_compat` model_validator is duplicated verbatim in `FilterSelectSpec` (2344-2351) and `NativeFilterUpdateSpec` (2480-2487), including the identical error string. If the rule or message changes, one site will drift. Extract a shared validator/helper and reuse it in both models. </div> </div> <small><i>Code Review Run #a6dd84</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
