Skip to content

fix(datasource): validate expressions through the shared adhoc-expression checks#41427

Open
sha174n wants to merge 2 commits into
apache:masterfrom
sha174n:fix/validate-expression-parsing-consistency
Open

fix(datasource): validate expressions through the shared adhoc-expression checks#41427
sha174n wants to merge 2 commits into
apache:masterfrom
sha174n:fix/validate-expression-parsing-consistency

Conversation

@sha174n

@sha174n sha174n commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

SUMMARY

SqlaTable.validate_expression (POST /api/v1/datasource/<type>/<id>/validate_expression/) builds its validation query directly from the submitted expression. This routes the expression through validate_stored_expression first, so expression validation applies the same parsing policy already enforced for stored adhoc column and metric expressions: a single statement, no set operations, and no sub-queries unless ALLOW_ADHOC_SUBQUERY is enabled. The result is one consistent parsing policy across the query pipeline.

TESTING INSTRUCTIONS

pytest tests/unit_tests/models/test_validate_expression.py — adds coverage that the validator reports a sub-query or set-operation expression as invalid before the validation query is built or run, alongside the existing valid-expression paths.

ADDITIONAL INFORMATION

  • Has associated issue
  • Required feature flags
  • Changes UI
  • Includes DB Migration
  • Introduces new feature or API
  • Removes existing feature or API

…sion checks

Route validate_expression through validate_stored_expression so custom
expression validation applies the same single-statement / no-set-operation /
no-subquery (unless ALLOW_ADHOC_SUBQUERY) parsing policy already used for
stored adhoc column and metric expressions, keeping one parsing policy across
the query pipeline.

Adds regression coverage that the validator rejects sub-queries and set
operations before the validation query is built or run.
@dosubot dosubot Bot added api Related to the REST API data:dataset Related to dataset configurations labels Jun 25, 2026
@bito-code-review

bito-code-review Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #157f2a

Actionable Suggestions - 0
Filtered by Review Rules

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

  • superset/models/helpers.py - 1
Review Details
  • Files reviewed - 2 · Commit Range: 772a94a..772a94a
    • superset/models/helpers.py
    • tests/unit_tests/models/test_validate_expression.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ 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

@github-actions github-actions Bot removed the api Related to the REST API label Jun 25, 2026
Comment thread tests/unit_tests/models/test_validate_expression.py
@bito-code-review

Copy link
Copy Markdown
Contributor

The suggestion to add type hints to the newly added test methods is correct, as it aligns with standard Python practices for maintaining type safety in test suites. You can resolve this by adding the unittest.mock.MagicMock type to the mock parameters and None as the return type.

Would you like me to apply this change to the other similar test methods added in this PR as well?

tests/unit_tests/models/test_validate_expression.py

@patch("superset.models.helpers.is_feature_enabled", return_value=False)
    @patch("superset.connectors.sqla.models.SqlaTable._execute_validation_query")
    def test_validate_expression_rejects_subquery(
        self, mock_execute: MagicMock, mock_ff: MagicMock
    ) -> None:
        """A sub-query expression is rejected by the same validate_adhoc_subquery
        gate used for stored adhoc expressions, before any validation query is
        built or run (with ALLOW_ADHOC_SUBQUERY off, the default). Locks in that
        expression validation never executes the sub-query."""

@codecov

codecov Bot commented Jun 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.45%. Comparing base (a90c8e0) to head (8e13c80).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #41427      +/-   ##
==========================================
- Coverage   64.48%   64.45%   -0.03%     
==========================================
  Files        2661     2661              
  Lines      145642   145664      +22     
  Branches    33613    33615       +2     
==========================================
- Hits        93912    93889      -23     
- Misses      50029    50074      +45     
  Partials     1701     1701              
Flag Coverage Δ
hive ?
mysql 57.99% <100.00%> (-0.01%) ⬇️
postgres 58.05% <100.00%> (-0.02%) ⬇️
presto 40.82% <0.00%> (-0.01%) ⬇️
python 59.45% <100.00%> (-0.05%) ⬇️
sqlite 57.71% <100.00%> (-0.01%) ⬇️
unit 100.00% <ø> (ø)

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.

@rusackas rusackas requested review from rusackas and sadpandajoe June 25, 2026 17:08
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@bito-code-review

bito-code-review Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #a6eeeb

Actionable Suggestions - 0
Review Details
  • Files reviewed - 1 · Commit Range: 772a94a..8e13c80
    • tests/unit_tests/models/test_validate_expression.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ 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

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

Labels

data:dataset Related to dataset configurations size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant