Skip to content

fix(post-processing): return 4xx when an operation is missing a required option - #45573

Closed
eschutho wants to merge 1 commit into
masterfrom
fix-sentry-19gt-cum-missing-columns
Closed

eschutho wants to merge 1 commit into
masterfrom
fix-sentry-19gt-cum-missing-columns

Conversation

@eschutho

Copy link
Copy Markdown
Member

SUMMARY

Root cause. A chart's stored query_context can contain a post-processing step whose options don't satisfy the operation's signature, e.g. {operation: 'cum', options: {operator: 'sum'}} with no columns. QueryObject.exec_post_processing called func(df, **options) directly, so the failure surfaced as a bare TypeError: cum() missing 1 required positional argument: 'columns' rather than a validation error.

Fix. After options are resolved (so resample's fill_time_range flow is unchanged), bind them against inspect.signature(func) and raise InvalidPostProcessingError naming the operation and the missing/invalid option. Only the bind is guarded; the operation call itself is not wrapped, so a genuine TypeError from inside an operation still propagates. Operations with **kwargs and ops registered via EXTRA_PANDAS_POSTPROCESSING_OPS bind naturally; callables without an introspectable signature skip the pre-check.

Tradeoffs.

  • Failure-mode change: a bare TypeError (potentially a 500) becomes InvalidPostProcessingError (4xx, via the existing QueryObjectValidationError conversion) with a clear message.
  • Stored charts missing a required option now get a 4xx with an explanatory message instead of a 500; the chart is still not renderable, but the error is actionable.
  • No behavior change for valid input. Cost is one inspect.signature call per operation.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

pytest tests/unit_tests/queries/query_object_test.py

New tests: cum with only operator raises InvalidPostProcessingError mentioning cum and columns; a valid cum still works; a TypeError raised inside an operation is not converted. Without the fix the first new test fails (bare TypeError); with it all 48 tests in the file pass. Related suites (models/helpers_test.py, pandas_postprocessing, semantic_layers/models_test.py) show the same 8 failures with and without this change (prophet / build_like_predicate), so they are unrelated. ruff check and ruff format --check pass.

ADDITIONAL INFORMATION

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

🤖 Generated with Claude Code

…red option

A chart's stored query_context can hold a post-processing operation whose
options no longer satisfy the operation's signature, e.g. a `cum` step with
only `operator` and no `columns`. exec_post_processing called
`func(df, **options)` directly, so the resulting bare TypeError escaped as a
server error even though the cause is a malformed request.

Bind the resolved options against the operation's signature before calling
it and raise InvalidPostProcessingError naming the operation and the
offending/missing option. Only the bind is guarded, so a TypeError raised
from inside an operation still propagates unchanged.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@eschutho

Copy link
Copy Markdown
Member Author

Closing: on current master get_query_result already converts a TypeError from post-processing into QueryObjectValidationError (4xx) since #44312, so this is redundant.

@eschutho eschutho closed this Oct 21, 2026
@eschutho
eschutho deleted the fix-sentry-19gt-cum-missing-columns branch October 21, 2026 15:08
@netlify

netlify Bot commented Oct 21, 2026

Copy link
Copy Markdown

✅ Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit 6fff74c
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/6ad8d96990761be85e5b7c4a
😎 Deploy Preview https://deploy-preview-45573--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

codecov Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 44.44444% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.50%. Comparing base (75be8f9) to head (6fff74c).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
superset/common/query_object.py 44.44% 4 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (75be8f9) and HEAD (6fff74c). Click for more details.

HEAD has 3 uploads less than BASE
Flag BASE (75be8f9) HEAD (6fff74c)
python 7 5
postgres 2 1
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #45573       +/-   ##
===========================================
- Coverage   84.06%   65.50%   -18.57%     
===========================================
  Files        3056     3056               
  Lines      201028   201136      +108     
  Branches    47025    47036       +11     
===========================================
- Hits       168990   131749    -37241     
- Misses      28799    66432    +37633     
+ Partials     3239     2955      -284     
Flag Coverage Δ
hive 34.91% <0.00%> (-0.01%) ⬇️
mysql 52.86% <44.44%> (-0.02%) ⬇️
postgres 52.86% <44.44%> (-0.03%) ⬇️
presto 36.64% <0.00%> (-0.01%) ⬇️
python 53.14% <44.44%> (-34.17%) ⬇️
sqlite 52.62% <44.44%> (-0.02%) ⬇️
unit ?

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.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant