fix(explore): require a value for simple adhoc filters before allowing save

The Save button in the adhoc filter popover stayed enabled when a comparator-taking operator had no value, because `AdhocFilter.isValid()` only rejected a `null` comparator. An unset comparator is `undefined`, not `null`: selecting a subject resets it (and falls back to the `IN` operator), and the value Select's clear affordance emits `undefined` as well. This was most visible on boolean columns, whose operator list is restricted to unary operators, so a freshly picked boolean column lands on `IN` with no value and the popover looks complete. Saving sent a filter with no `val` to the query API, which tripped a bare `assert isinstance(eq, (tuple, list))` in the query builder and surfaced as a generic error instead of inline client-side validation. Extend the existing check to treat `undefined` like `null`, matching the empty-array guard already applied to `IN`/`NOT IN` comparators. Unary operators are unaffected: they short-circuit earlier via DISABLE_INPUT_OPERATORS.
2026-08-19 06:31:14 +00:00 · 2026-08-18 23:55:54 +00:00
3 changed files with 59 additions and 2 deletions
@@ -207,6 +207,38 @@ describe('AdhocFilter', () => {
    expect(adhocFilter10.isValid()).toBe(true);
  });

+  test('is invalid when a comparator-taking operator has no comparator', () => {
+    // A comparator that was never set, or that was cleared through the value
+    // Select's clear affordance, is `undefined` rather than `null` or `[]`.
+    const adhocFilter1 = new AdhocFilter({
+      expressionType: ExpressionTypes.Simple,
+      subject: 'is_intro',
+      operator: 'IN',
+      comparator: undefined,
+      clause: Clauses.Where,
+    });
+    expect(adhocFilter1.isValid()).toBe(false);
+
+    const adhocFilter2 = new AdhocFilter({
+      expressionType: ExpressionTypes.Simple,
+      subject: 'is_intro',
+      operator: '==',
+      comparator: undefined,
+      clause: Clauses.Where,
+    });
+    expect(adhocFilter2.isValid()).toBe(false);
+
+    // `false` is a legitimate boolean comparator, not a missing value
+    const adhocFilter3 = new AdhocFilter({
+      expressionType: ExpressionTypes.Simple,
+      subject: 'is_intro',
+      operator: '==',
+      comparator: false,
+      clause: Clauses.Where,
+    });
+    expect(adhocFilter3.isValid()).toBe(true);
+  });
+
  test('can translate from simple expressions to sql expressions', () => {
    const adhocFilter1 = new AdhocFilter({
      expressionType: ExpressionTypes.Simple,
@@ -163,8 +163,10 @@ export default class AdhocFilter {
          // A non-empty array of values ('IN' or 'NOT IN' clauses)
          return this.comparator.length > 0;
        }
-        // A value has been selected or typed
-        return this.comparator !== null;
+        // A value has been selected or typed. An unset comparator is
+        // `undefined` rather than `null`: picking a new subject resets it, and
+        // the value Select's clear affordance emits `undefined` too.
+        return this.comparator != null;
      }
    }

@@ -181,6 +181,29 @@ describe('AdhocFilterEditPopover', () => {
    expect(saveButton).toBeDisabled();
  });

+  test('disables save button when a boolean column has no value selected', async () => {
+    const booleanColumn = { type: 'BOOL', column_name: 'is_intro' };
+    renderPopover({
+      adhocFilter: new AdhocFilter({
+        expressionType: ExpressionTypes.Simple,
+        clause: Clauses.Where,
+      }),
+      options: [booleanColumn],
+      datasource: { columns: [booleanColumn], filter_select: false },
+    });
+
+    // Picking the subject resets the comparator to `undefined`; the value
+    // control is then left untouched, mirroring the reported repro.
+    await userEvent.click(screen.getByTestId('select-element'));
+    await userEvent.click(
+      await screen.findByRole('option', { name: /is_intro/ }),
+    );
+
+    expect(
+      screen.getByTestId('adhoc-filter-edit-popover-save-button'),
+    ).toBeDisabled();
+  });
+
  test('initiates resize when resize handle is dragged', async () => {
    const onResize = jest.fn();
    renderPopover({ onResize });