Skip to content

fix(plugin-chart-ag-grid-table): enforce numeric bounds for range (BETWEEN) filters#40607

Merged
rusackas merged 4 commits into
masterfrom
fix/ag-grid-range-filter-numeric-coercion
Jun 4, 2026
Merged

fix(plugin-chart-ag-grid-table): enforce numeric bounds for range (BETWEEN) filters#40607
rusackas merged 4 commits into
masterfrom
fix/ag-grid-range-filter-numeric-coercion

Conversation

@rusackas
Copy link
Copy Markdown
Member

@rusackas rusackas commented Jun 1, 2026

SUMMARY

In the AG Grid table plugin's filter converter, range (IN_RANGEBETWEEN) filter operands are interpolated into the generated clause without quoting, so they need to be finite numbers. Previously the raw filter/filterTo values were passed straight through, even though the filter value path accepts strings — which could produce a malformed clause if the operands weren't numeric.

This coerces both bounds with Number() and skips the clause entirely when either is not a finite number, so only well-formed numeric ranges are emitted. Valid numeric ranges are unchanged.

BEFORE / AFTER

For a metric range filter, the generated HAVING:

  • Before: revenue BETWEEN <raw> AND <raw> (operands passed through as-is)
  • After: emitted only when both bounds coerce to finite numbers (revenue BETWEEN 10 AND 20); otherwise no clause is generated.

TESTING INSTRUCTIONS

cd superset-frontend && npx jest plugins/plugin-chart-ag-grid-table/test/utils/agGridFilterConverter.test.ts

Adds two regression tests (numeric bounds → BETWEEN clause; non-numeric bound → no clause). Full file: 47/47 pass.

ADDITIONAL INFORMATION

  • Has associated issue: n/a
  • Changes UI: No
  • Includes DB Migration: No
  • Introduces new feature or API: No
  • Removes existing feature or API: No

🤖 Generated with Claude Code

@bito-code-review
Copy link
Copy Markdown
Contributor

bito-code-review Bot commented Jun 1, 2026

Code Review Agent Run #3b2601

Actionable Suggestions - 0
Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

  • superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/agGridFilterConverter.ts - 1
    • NICE_TO_HAVE: Missing unit test for bounds validation · Line 381-391
Review Details
  • Files reviewed - 2 · Commit Range: cb45f37..cb45f37
    • superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/agGridFilterConverter.ts
    • superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/agGridFilterConverter.test.ts
  • Files skipped - 0
  • Tools
    • Eslint (Linter) - ✔︎ Successful
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@netlify
Copy link
Copy Markdown

netlify Bot commented Jun 1, 2026

Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit 0f77556
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/6a21a08c83df4f0008cfe15f
😎 Deploy Preview https://deploy-preview-40607--superset-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 1, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.04%. Comparing base (6967057) to head (0f77556).

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #40607   +/-   ##
=======================================
  Coverage   64.03%   64.04%           
=======================================
  Files        2663     2663           
  Lines      143619   143630   +11     
  Branches    33030    33034    +4     
=======================================
+ Hits        91973    91985   +12     
+ Misses      50044    50043    -1     
  Partials     1602     1602           
Flag Coverage Δ
javascript 67.58% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the AG Grid table plugin’s filter-to-SQL conversion for numeric range (inRangeBETWEEN) filters on metrics, to avoid emitting malformed/unquoted SQL when bounds are not valid numbers.

Changes:

  • Coerce inRange bounds to numbers and skip emitting the clause when bounds are not finite numbers.
  • Add Jest regression tests covering numeric metric ranges and a non-numeric bound case.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/agGridFilterConverter.ts Updates metric range (BETWEEN) clause generation to coerce bounds to finite numbers and skip invalid ranges.
superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/agGridFilterConverter.test.ts Adds tests ensuring numeric ranges emit BETWEEN and invalid bounds drop the HAVING clause.

@bito-code-review
Copy link
Copy Markdown
Contributor

bito-code-review Bot commented Jun 2, 2026

Code Review Agent Run #fde6f8

Actionable Suggestions - 0
Review Details
  • Files reviewed - 2 · Commit Range: cb45f37..dc6429c
    • superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/agGridFilterConverter.test.ts
    • superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/agGridFilterConverter.ts
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • Eslint (Linter) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@sha174n sha174n added the merge-if-green If approved and tests are green, please go ahead and merge it for me label Jun 4, 2026
@rusackas rusackas force-pushed the fix/ag-grid-range-filter-numeric-coercion branch from dc6429c to 0f77556 Compare June 4, 2026 15:58
claude and others added 4 commits June 4, 2026 09:52
…ters

Range (BETWEEN) filter operands are interpolated into the generated
clause without quoting, so they must be finite numbers. Coerce both
bounds with Number() and skip the clause entirely if either is not a
finite number, rather than passing the raw value through.

Adds regression tests covering a metric range filter with numeric bounds
(emits the BETWEEN clause) and with a non-numeric bound (no clause).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…rmatting

Number('') coerces to 0, so an empty range bound slipped through the
finite-number guard and produced a BETWEEN clause instead of being dropped.
Add a toFiniteNumber helper that rejects empty/whitespace-only strings
before coercion. Also apply prettier formatting to the test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ound

Hoist IN_RANGE handling out of the filterTo !== undefined guard so a
missing or cleared upper bound is dropped instead of falling through to
the generic clause and emitting an invalid single-operand BETWEEN. Also
reject nullish bounds in toFiniteNumber. Adds a regression test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@rusackas rusackas force-pushed the fix/ag-grid-range-filter-numeric-coercion branch from 0f77556 to 8a8d8cd Compare June 4, 2026 16:52
@rusackas rusackas merged commit ddb09f4 into master Jun 4, 2026
63 checks passed
@rusackas rusackas deleted the fix/ag-grid-range-filter-numeric-coercion branch June 4, 2026 17:14
@github-project-automation github-project-automation Bot moved this from Needs Review to Approved and/or Merged in Superset Review Help Wanted Jun 4, 2026
@bito-code-review
Copy link
Copy Markdown
Contributor

Bito Automatic Review Skipped – PR Already Merged

Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.
No action is needed if you didn't intend to review it. To get a review, you can type /review in a comment and save it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-if-green If approved and tests are green, please go ahead and merge it for me plugins size/M

Projects

Status: Approved and/or Merged

Development

Successfully merging this pull request may close these issues.

4 participants