[SEPURI-SAI-KRISHNA]

SUMMARY

A malformed post_processing option returned HTTP 500 where the contract pinned by test_chart_data_invalid_post_processing is 400. Fuzzing found 39 distinct sites that escape to app.errorhandler(Exception); this PR converts the 38 that are bad requests and leaves the 1 that is genuinely a server-side condition. Background and the full measurement in #44952.

#44463 widened the guard at superset/models/helpers.py so raw pandas TypeError and pandas.errors.DataError become QueryObjectValidationError, and #44502 did the same for superset/semantic_layers/models.py. ValueError, KeyError and AttributeError escape the same way: not caught there, not caught by get_df_payload (which handles only QueryObjectValidationError), and ChartDataRestApi.data carries no @safe, so they land in app.errorhandler(Exception) whose json_error_response defaults to status=500.

The handling goes where the failing operation is known, inside QueryObject.exec_post_processing's loop, rather than at the two call sites:

@staticmethod
def _exec_operation(func, operation, is_builtin, df, options):
    try:
        return func(df, **options)
    except (AttributeError, DataError, KeyError, TypeError, ValueError) as ex:
        if not is_builtin:
            # operator-registered code, so a fault in it is a server fault
            raise
        logger.warning(..., exc_info=True)
        raise InvalidPostProcessingError(...) from ex

Both call sites already map InvalidPostProcessingError to QueryObjectValidationError, so neither needed a line added. Three things follow from doing it here instead:

• A failure inside a custom operation stays a 500. An operation registered through EXTRA_PANDAS_POSTPROCESSING_OPS is the operator's own code, which this module cannot reason about, so a ValueError from its internals is a fault in that operation rather than a malformed request. Only the loop knows which kind of operation it is holding.
• The message names the operation. rank with an unknown metric answers "The rank post-processing operation references a column or level that is not in the query result: does_not_exist", where the previous shape of this fix could only say does_not_exist.
• The guard is written once. The earlier revision of this PR added the same block to both call sites, which is the duplication that got the two sites fixed eleven days apart in the first place.

KeyError gets its own message because str(KeyError("x")) is just "'x'", so reusing the shared one would answer a request with Error: 'does_not_exist'. All ten KeyError sites name a column or MultiIndex level: boxplot(metrics), rank(metric), rank(group_by), histogram(column), histogram(groupby), flatten(drop_levels), geohash_decode(geohash), geohash_encode(latitude), geohash_encode(longitude), geodetic_parse(geodetic), so the specific wording is accurate for every one. Since only built-in operations reach it, the case where that wording would be a guess no longer arises.

No operator internals are modified and no option is newly rejected, only the status and message of a request that already failed.

ImportError is deliberately left out. The one case is rolling with a win_type, which needs scipy; a missing optional dependency is a deployment matter rather than a bad request, so 500 remains correct there. That is the single site this PR does not convert, and a test pins it.

Why these values reach pandas at all: ChartDataPostProcessingOperationSchema.options is a bare fields.Dict (schemas.py:1123), so no option is validated before dispatch. The 11 ChartData*OptionsSchema classes are referenced only from CHART_SCHEMAS for OpenAPI output and validate nothing, and 8 operations have no options schema at all. Wiring those up would be the root-cause fix, but it risks rejecting payloads that work today, so it is left out of this PR.

The trade-off, stated plainly. ValueError and AttributeError are broad: an AttributeError reading 'NoneType' object has no attribute ... is the classic signature of a genuine bug, and for a built-in operation it now answers 400 rather than 500. Two things keep that acceptable:

• The pre-existing handler already makes exactly this trade for TypeError, which is no narrower.
• get_df_payload records str(ex) on the response and never logs a traceback, so a mis-classified internal fault would vanish silently. This logs with exc_info=True first, so the stack trace still reaches operators.

If reviewers would rather keep AttributeError as a 500, dropping it from the tuple is a one-line change and costs 7 of the 38 converted sites.

The call-site handlers are removed. #44463 and #44502 each added except (TypeError, pd.errors.DataError) next to the InvalidPostProcessingError handler. Built-in operations no longer need them, since _exec_operation wraps those two types before they get there, and leaving them in would mean a custom operation's TypeError stays a 400 while its ValueError and AttributeError become 500, which is an odd split to leave behind. So both come out (-2 and -3 lines, the third being semantic_layers/models.py's now-unused import pandas as pd).

Neither #44463's nor #44502's test needed editing. Both drive a real built-in operation (resample on an object-dtype column) and assert only that QueryObjectValidationError is raised, which still holds through the InvalidPostProcessingError path. A new test at each call site pins the part that did change: a custom operation's TypeError propagates instead of being converted. Both fail if either handler is put back.

Translations. Two new translatable strings, so superset/translations/messages.pot gains both entries with their python-format flags (+8/-0). Without that the required babel-extract check fails on scripts/translations/check_pot_drift.py. No .po catalogue is touched, matching how other string-adding PRs land.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A: backend only. The error response for an already-failing request changes from an opaque 500 to a 400 carrying the underlying message.

options	before	after
rank metric: "does_not_exist"	HTTP 500	400, "The rank post-processing operation references a column or level that is not in the query result: does_not_exist"
diff periods: 1.5	HTTP 500	400, "The diff post-processing operation failed: periods must be an integer"
a custom op raising ValueError	HTTP 500	HTTP 500 (unchanged, deliberately)
a custom op raising TypeError	HTTP 400	HTTP 500

TESTING INSTRUCTIONS

pytest tests/unit_tests/queries/query_object_test.py \
       tests/unit_tests/models/helpers_test.py \
       tests/unit_tests/semantic_layers/models_test.py

Ten cases are added, eight of which fail on master:

test	covers	on master
..._reports_a_malformed_option_as_a_bad_request	diff periods, rolling window, both real operations	fails
..._names_a_column_the_result_does_not_have	real rank with an unknown metric	fails
..._wraps_the_remaining_pandas_errors	AttributeError, DataError, TypeError	fails
..._leaves_a_custom_operation_fault_unwrapped	a custom op's ValueError propagates	passes, guards the new behaviour
..._leaves_a_missing_dependency_unwrapped	ImportError propagates	passes, guards the new behaviour
test_get_query_result_leaves_a_custom_operation_fault_unwrapped	a custom op's TypeError is not converted at the dataset call site	fails
test_semantic_view_get_query_result_leaves_a_custom_operation_fault_unwrapped	the same at the semantic-layer call site	fails

The first three drive real built-in operations rather than mocks. No existing test changed behaviour: tests/unit_tests/{queries,models,semantic_layers,pandas_postprocessing,charts} run clean at 883 passed, 2 xfailed (both pre-existing). ruff check, ruff format --check and pre-commit run mypy are clean, and pylint --rcfile=.pylintrc rates all three changed source files 10.00/10.

To size the change I fuzzed every built-in operation's options one at a time away from a known-good baseline, 910 calls across all 19 operations, classifying each failure by whether the guard catches it. Before: 39 distinct unhandled-exception sites across 14 of the 19 operations (ValueError 21, KeyError 10, AttributeError 7, ImportError 1). After: 1, the scipy ImportError above. That sweep is not exhaustive over every possible option value, so it is a lower bound.

ADDITIONAL INFORMATION

• Has associated issue: Fixes Malformed post_processing options return 500 instead of 400 for ValueError, KeyError and AttributeError #44952
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.31%. Comparing base (96e22f4) to head (1ab2555).
⚠️ Report is 250 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #44953      +/-   ##
==========================================
- Coverage   83.41%   83.31%   -0.11%     
==========================================
  Files        3040     3039       -1     
  Lines      198718   198296     -422     
  Branches    46381    46180     -201     
==========================================
- Hits       165764   165202     -562     
- Misses      29730    29870     +140     
  Partials     3224     3224              

Flag	Coverage Δ
hive	34.98% <25.00%> (-0.01%)	⬇️
mysql	52.99% <50.00%> (+<0.01%)	⬆️
postgres	53.00% <50.00%> (-0.01%)	⬇️
presto	36.74% <25.00%> (-0.01%)	⬇️
python	86.92% <100.00%> (+<0.01%)	⬆️
sqlite	52.76% <50.00%> (+<0.01%)	⬆️
unit	80.92% <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.

[bito-code-review]

Code Review Agent Run #0d1040

Actionable Suggestions - 1

• superset/models/helpers.py - 1

  • KeyError Branch Unlogged · Line 2693-2704

Additional Suggestions - 3

• tests/unit_tests/models/helpers_test.py - 1

  • Duplicated test setup block · Line 5899-5917
    This setup block (imports, `SqlaTable`, DataFrame, `QueryResult`, both `patch.object` calls) duplicates `test_get_query_result_wraps_post_processing_type_error` (line 5782) verbatim. Extract a shared fixture/helper — or fold the sibling's resample case in as a fifth `pytest.param` — so a change to `QueryResult` or the mock setup needs only one edit.
• superset/semantic_layers/models.py - 1

  • Broad catch masks server faults · Line 398-410
    The broad `except (TypeError, ValueError, AttributeError, pd.errors.DataError)` converts any genuine fault inside a post-processing operation into a 400 bad-request, masking real server bugs from the error path (only the `logger.warning` reveals them). The comment acknowledges this tradeoff, but a genuine operation bug now reaches users as a misleading "bad request". Consider narrowing to pandas-specific errors or re-raising non-option faults.
• superset/models/helpers.py - 1

  • Custom Op KeyError Mislabeled · Line 2698-2704
    `ChartDataPostProcessingOperationSchema` and `exec_post_processing` also dispatch `EXTRA_PANDAS_POSTPROCESSING_OPS` custom operations, for which a `KeyError` may come from the op's own logic rather than a missing column; the fixed message would then mislabel the cause. Consider neutral wording or logging the operation name. No in-repo custom op triggers this today.

Review Details

• Files reviewed - 5 · Commit Range: 0d7bdb8..0d7bdb8

  • superset/models/helpers.py
  • superset/semantic_layers/models.py
  • superset/translations/messages.pot
  • tests/unit_tests/models/helpers_test.py
  • tests/unit_tests/semantic_layers/models_test.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ 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 an incremental AI Review.

• /review full - 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

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[SEPURI-SAI-KRISHNA]

Fixed the KeyError arm so it logs with exc_info=True, like the branch below it.

Both reviews land on the same gap: a custom op registered through EXTRA_PANDAS_POSTPROCESSING_OPS can raise KeyError from its own internals, where the "column or level" wording is a guess. exec_post_processing does dispatch those ops, so the path is real. Naming the operation in the message would need the name from inside that loop, which means moving the translation into QueryObject, so I left it and kept the traceback instead.

One correction: cum, diff and select all have validate_column_args. Of those four only rank lacks it. The point still holds via rank, boxplot, histogram, flatten, resample, prophet and geography, and because the decorator only checks the args it is given (aggregate checks groupby, not the columns inside aggregates).

Keeping the catch broad. Narrowing to pandas errors would drop the numpy ValueError and the AttributeError cases, 28 of the 38 sites here. TypeError was already caught by #44463 and #44502 and is no narrower than ValueError.

Leaving the duplicated test setup. Deduping means editing the sibling test, and helpers_test.py is also touched by the open #44897.

[bito-code-review]

Code Review Agent Run #96245a

Actionable Suggestions - 1

• superset/models/helpers.py - 1

  • Broad except masks server faults · Line 2712-2717

Additional Suggestions - 2

• superset/semantic_layers/models.py - 1

  • Duplicated warning logging · Line 394-397
    The `KeyError` handler (394-397) and the `(TypeError, ValueError, AttributeError, DataError)` handler (414-417) repeat the identical `logger.warning(..., exc_info=True)` call. A single combined except that logs once and branches on `isinstance(ex, KeyError)` for the raised message removes the copy and keeps log wording/level in one place.
• tests/unit_tests/models/helpers_test.py - 1

  • Version-coupled error strings · Line 5833-5872
    The parametrized cases assert on pandas/numpy internal error strings (`periods must be an integer`, `has no attribute 'items'`, `Percentiles must be in the range [0, 100]`). These are library-internal and version-dependent; a pandas/numpy upgrade that rewords them breaks the test even though the wrapping behavior in `get_query_result` is unchanged. Consider asserting primarily on the `QueryObjectValidationError` type and matching the message loosely.

Review Details

• Files reviewed - 5 · Commit Range: 4ebf204..4ebf204

  • superset/models/helpers.py
  • superset/semantic_layers/models.py
  • superset/translations/messages.pot
  • tests/unit_tests/models/helpers_test.py
  • tests/unit_tests/semantic_layers/models_test.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ 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 an incremental AI Review.

• /review full - 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

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[codeant-ai-for-open-source]

CodeAnt PR Risk: Medium Risk

• The PR needs attention before merging because a credible error-classification concern remains.
• Built-in KeyErrors are all reported as missing result columns or levels, even when they may come from an internal lookup failure.

Assessed commit: 1ab255525c0d

[SEPURI-SAI-KRISHNA]

Fair point, and bito raised the same thing earlier in the thread, so I have restructured rather than argued it.

The handling now lives inside QueryObject.exec_post_processing's loop instead of at the two call sites. The loop already distinguishes a built-in operation from one registered through EXTRA_PANDAS_POSTPROCESSING_OPS, so a failure inside operator-owned code is left for the error handler and stays a 500:

except (AttributeError, DataError, KeyError, TypeError, ValueError) as ex:
    if not is_builtin:
        raise
    ...
    raise InvalidPostProcessingError(...) from ex

Three things fell out of it:

• The KeyError message can name the operation, so the "missing column" wording is only used where it is accurate. That was the other half of your comment, and it was the case I flagged as unfixable in the previous description because the operation name was not in scope there.
• Both call sites go back to master untouched. The duplicated block is gone, which was the duplication that got these two sites fixed eleven days apart to begin with.
• Two tests now pin the exclusions: a custom operation's ValueError propagates, and so does an ImportError.

The PR is 3 files instead of 5, and the suites run clean at 881 passed.

One thing I did not change: #44502's call-site handler for TypeError and pandas.errors.DataError is still there, so those two types from a custom operation remain a 400. Built-in operations no longer rely on it. Removing it would make custom operations consistently 500, but it changes what #44502 decided and needs its tests edited, so I have left it as a follow-up rather than folding it in here. Happy to do it in this PR if a maintainer would rather see it settled in one go.

[SEPURI-SAI-KRISHNA]

Right on both counts, and it is now fixed rather than documented as out of scope.

SemanticView.get_query_result and SqlaTable.get_query_result each carried an except (TypeError, pd.errors.DataError) next to the InvalidPostProcessingError handler, added by #44502 and #44463. With the wrapping moved into exec_post_processing's loop those two no longer serve built-in operations, and keeping them left a custom operation's TypeError as a 400 while its ValueError and AttributeError became 500. Both handlers are removed, so a custom operation's fault is consistently a server error.

I had expected this to need #44502's and #44463's tests edited, which is why the previous body deferred it. It does not. Both drive a real built-in operation, resample on an object-dtype column, and assert only that QueryObjectValidationError is raised, which still holds via the InvalidPostProcessingError path. 883 pass across tests/unit_tests/{queries,models,semantic_layers,pandas_postprocessing,charts} with no existing test touched.

One test is added at each call site for the behaviour that did change: a custom operation registered through EXTRA_PANDAS_POSTPROCESSING_OPS raising TypeError propagates rather than being converted. Putting either handler back fails both, so the removal stays removed.

semantic_layers/models.py loses its import pandas as pd along with the handler, since nothing else in the file used it.