codeant-ai-for-open-source[bot] commented on code in PR #38568:
URL: https://github.com/apache/superset/pull/38568#discussion_r2915392183
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts:
##########
@@ -27,6 +27,24 @@ import { FILTER_SUPPORTED_TYPES } from './constants';
const FILTERS_FIELD_NAME = 'filters';
+type TimeGrainTuple = [string, string];
+type TimeGrainObject = { value: string; label?: string };
+
+export const getTimeGrainOptions = (
+ timeGrains: Array<TimeGrainTuple | TimeGrainObject> = [],
+) =>
+ timeGrains.map(timeGrain => {
+ if (Array.isArray(timeGrain)) {
+ const [value, label] = timeGrain;
+ return { value, label: label || value };
+ }
+
+ return {
+ value: timeGrain.value,
+ label: timeGrain.label || timeGrain.value,
+ };
+ });
Review Comment:
**Suggestion:** The current `getTimeGrainOptions` implementation forwards
`null` values from the dataset's `time_grain_sqla` (e.g., the `[null, 'Time
Column']` entry) into the `Select` options and subsequently into the saved
`time_grains` allowlist, even though the rest of the system (types and the
`TimeGrainFilterPlugin` allowlist filter) expects time grain identifiers to be
non-null strings; this can result in a saved allowlist containing `null`
entries that never match any actual time grain option at runtime. The fix is to
filter out entries whose underlying value is not a non-empty string before
mapping them into options and to reflect this in the tuple/object typings so
the allowlist is always a `string[]` of valid grain identifiers. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Time grain pre-filter can save unusable null allowlist entries.
- ⚠️ Selecting only "Time column" yields empty TimeGrain filter options.
- ⚠️ Dashboard authors see configuration that doesn't match runtime behavior.
- ⚠️ TimeGrainFilterPlugin allowlist logic underutilized due to bad values.
```
</details>
```suggestion
type TimeGrainTuple = [string | null, string];
type TimeGrainObject = { value: string | null; label?: string };
export const getTimeGrainOptions = (
timeGrains: Array<TimeGrainTuple | TimeGrainObject> = [],
) =>
timeGrains
// Filter out non-string values (e.g. null "Time Column" placeholder)
.filter(timeGrain => {
const value = Array.isArray(timeGrain)
? timeGrain[0]
: timeGrain.value;
return typeof value === 'string' && value.length > 0;
})
.map(timeGrain => {
if (Array.isArray(timeGrain)) {
const [value, label] = timeGrain;
return { value: value as string, label: label || value };
}
return {
value: timeGrain.value as string,
label: timeGrain.label || (timeGrain.value as string),
};
});
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `FiltersConfigForm` for a native filter of type `'filter_timegrain'`
in the
dashboard UI. The configuration component is implemented in
`superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:276-1807`.
When a dataset is selected, a `useEffect` at lines 682-721 calls
`cachedSupersetGet` to
`/api/v1/dataset/${datasetId}` requesting the `time_grain_sqla` field, and
stores the
result in `datasetDetails.time_grain_sqla`.
2. With a dataset whose `time_grain_sqla` list includes a non-string
placeholder entry for
the "Time column" option (e.g. `[null, 'Time column']`) coming from the
backend, scroll to
the "Pre-filter available values" section for the time grain filter. The
rendering of this
section is at `FiltersConfigForm.tsx:1209-1276`, where the `Select` options
are built via
`options={getTimeGrainOptions(datasetDetails.time_grain_sqla)}`.
3. In that UI, check the "Pre-filter available values" `CollapsibleControl`
and, in the
time grain `Select` (rendered at `FiltersConfigForm.tsx:1251-1257`), select
only the "Time
column" option. Because `getTimeGrainOptions` in `utils.ts:30-46` simply
maps the raw
tuples/objects without filtering, the "Time column" placeholder `[null,
'Time column']`
becomes an option `{ value: null, label: 'Time column' }`. The `onChange`
handler at
`FiltersConfigForm.tsx:1258-1267` then saves `time_grains` in the form state
as an array
containing that `null` value (typed as `string[]` but containing `null` at
runtime).
4. Save the dashboard and load it in view mode. The `FilterTimeGrainPlugin`
(registered in
`superset-frontend/src/filters/components/TimeGrain/index.ts:26-42` and
exercised in
`TimeGrainFilterPlugin.test.tsx:79-98`) receives `formData.time_grains` from
native filter
configuration and filters its `data` options based on that allowlist. The
tests at
`TimeGrainFilterPlugin.test.tsx:79-98` show it uses `time_grains` to include
only matching
`duration` values (e.g. `['P1D', 'P1W']` yields only "Day" and "Week"). With
`time_grains`
containing only `null`, no `data[i].duration` matches, so the rendered
dropdown has no
selectable options even though the configuration UI allowed selecting "Time
column". The
saved allowlist thus contains an entry that can never match at runtime.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts
**Line:** 30:46
**Comment:**
*Logic Error: The current `getTimeGrainOptions` implementation forwards
`null` values from the dataset's `time_grain_sqla` (e.g., the `[null, 'Time
Column']` entry) into the `Select` options and subsequently into the saved
`time_grains` allowlist, even though the rest of the system (types and the
`TimeGrainFilterPlugin` allowlist filter) expects time grain identifiers to be
non-null strings; this can result in a saved allowlist containing `null`
entries that never match any actual time grain option at runtime. The fix is to
filter out entries whose underlying value is not a non-empty string before
mapping them into options and to reflect this in the tuple/object typings so
the allowlist is always a `string[]` of valid grain identifiers.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38568&comment_hash=d6ba505bf86232969bc8f13430355176d293e54048814427d486cb38e380fe35&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38568&comment_hash=d6ba505bf86232969bc8f13430355176d293e54048814427d486cb38e380fe35&reaction=dislike'>👎</a>
--
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]
