You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "EnxDev (via GitHub)" <gi...@apache.org> on 2026/08/24 13:14:44 UTC
[PR] fix(dataset): surface real validation message in save-error dialog [superset]
EnxDev opened a new pull request, #43459:
URL: https://github.com/apache/superset/pull/43459
### SUMMARY
Saving a dataset with an invalid metric SQL expression correctly blocks the save, but the error dialog showed only the single character "C" instead of the backend's validation message, leaving the user with no way to tell what
was wrong.
Root cause: the metric/column expression validator in `commands/dataset/update.py` raises a marshmallow `ValidationError` with a `flask_babel.LazyString` message (from `lazy_gettext`). `LazyString` isn't a `str` subclass, so it fails marshmallow's own
`isinstance(message, (str, bytes))` check in `ValidationError.__init__`, which means the message is stored bare instead of being wrapped in a list.
The API response ends up as `{"message": {"metrics.0.expression": "Custom SQL fields cannot be parsed as a single SQL statement."}}` — a plain string where the frontend expected `string[]`.
`parseErrorJson()` in `getClientErrorObject.ts` assumed every field's value was always an array and read `Object.values(error.message)[0]?.[0]` — the "first message of the first field". Against a scalar string, that second
`[0]` indexes into the string itself, returning just its first character.
Fixed `parseErrorJson()` to check `Array.isArray()` before indexing, so it uses the value directly when it's a string and the first element when it's an array — mirroring the same defensive pattern already used in `parsedErrorMessage` (`views/CRUD/hooks.ts`).
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
https://github.com/user-attachments/assets/40a22d01-a48e-4a70-b937-4841f6ab68c7
### TESTING INSTRUCTIONS
1. Edit any dataset → Metrics tab → Add item.
2. Enter a malformed SQL expression, e.g. `SUM(num))`.
3. Save → confirm "save and apply changes".
4. Error dialog now shows the full backend message instead of "C".
Added a regression test in `getClientErrorObject.test.ts` covering a field
message that is a plain string rather than a `string[]`; ran the full
`getClientErrorObject` suite (23 passing) plus `pre-commit run` on the
changed files.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): surface real validation message in save-error dialog [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on PR #43459:
URL: https://github.com/apache/superset/pull/43459#issuecomment-5451651705
Thanks for the review, @msyavuz — addressed both:
1. Fixed the root cause at `commands/dataset/update.py` (both `_validate_expressions:386` and `_validate_fetch_values_predicate:414`): the `ValidationError` message is now wrapped in a list (`[message]`) explicitly, so the 422 response keeps its documented `{field: [msg]}` shape regardless of whether the underlying message is a `str` or a `LazyString` (marshmallow's own `isinstance(message, (str, bytes))` check doesn't cover `LazyString`, which is what caused the bare-string storage). Added a regression assertion in `update_test.py` checking `messages` stays a list.
2. Checked the nested-object case (`{"recipients": {"0": {"type": [...]}}}`) — confirmed it's already handled safely by the `getFirstValidationError` helper that landed independently on master via #43631 (merged into this branch): a non-string/non-array field value returns `undefined` and falls back to `t('Invalid input')` rather than leaking an object into `error.error`. Verified with a scratch test. Left out of scope for this PR since it's pre-existing generic-fallback behavior, not a regression.
Also resolved the merge conflicts with master — #43631 landed the same frontend fix (`getFirstValidationError`) independently while this PR was open, so I kept master's implementation for `getClientErrorObject.ts`/test and layered the backend fix on top.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): surface real validation message in save-error dialog [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43459:
URL: https://github.com/apache/superset/pull/43459#issuecomment-5395715826
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): surface real validation message in save-error dialog [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43459:
URL: https://github.com/apache/superset/pull/43459#issuecomment-5453458542
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>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.<br/>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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): surface real validation message in save-error dialog [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #43459:
URL: https://github.com/apache/superset/pull/43459#discussion_r3880025250
##########
superset-frontend/packages/superset-ui-core/src/query/getClientErrorObject.ts:
##########
@@ -126,13 +126,17 @@ export function parseErrorJson(responseJson: JsonObject): ClientErrorObject {
error.error = error.description = error.errors[0].message;
error.link = error.errors[0]?.extra?.link;
}
- // Marshmallow field validation returns the error message in the format
- // of { message: { field1: [msg1, msg2], field2: [msg], } }
+ // Marshmallow field validation returns { field1: [msg1, msg2], ... },
+ // but some validators attach a bare string instead of a list.
Review Comment:
Fixed the root cause at commands/dataset/update.py (both _validate_expressions:386 and _validate_fetch_values_predicate:414): the ValidationError message is now wrapped in a list ([message]) explicitly, so the 422 response keeps its documented {field: [msg]} shape regardless of whether the underlying message is a str or a LazyString (marshmallow's own isinstance(message, (str, bytes)) check doesn't cover LazyString, which is what caused the bare-string storage). Added a regression assertion in update_test.py checking messages stays a list.
##########
superset-frontend/packages/superset-ui-core/src/query/getClientErrorObject.ts:
##########
@@ -126,13 +126,17 @@ export function parseErrorJson(responseJson: JsonObject): ClientErrorObject {
error.error = error.description = error.errors[0].message;
error.link = error.errors[0]?.extra?.link;
}
- // Marshmallow field validation returns the error message in the format
- // of { message: { field1: [msg1, msg2], field2: [msg], } }
+ // Marshmallow field validation returns { field1: [msg1, msg2], ... },
+ // but some validators attach a bare string instead of a list.
if (!error.error && error.message) {
if (typeof error.message === 'object') {
+ const firstFieldMessage = Object.values(
+ error.message as Record<string, string[] | string>,
Review Comment:
Checked the nested-object case ({"recipients": {"0": {"type": [...]}}}) — confirmed it's already handled safely by the getFirstValidationError helper that landed independently on master via https://github.com/apache/superset/pull/43631 (merged into this branch): a non-string/non-array field value returns undefined and falls back to t('Invalid input') rather than leaking an object into error.error. Verified with a scratch test. Left out of scope for this PR since it's pre-existing generic-fallback behavior, not a regression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): surface real validation message in save-error dialog [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #43459:
URL: https://github.com/apache/superset/pull/43459#discussion_r3845166496
##########
superset-frontend/packages/superset-ui-core/src/query/getClientErrorObject.ts:
##########
@@ -126,13 +126,17 @@ export function parseErrorJson(responseJson: JsonObject): ClientErrorObject {
error.error = error.description = error.errors[0].message;
error.link = error.errors[0]?.extra?.link;
}
- // Marshmallow field validation returns the error message in the format
- // of { message: { field1: [msg1, msg2], field2: [msg], } }
+ // Marshmallow field validation returns { field1: [msg1, msg2], ... },
+ // but some validators attach a bare string instead of a list.
Review Comment:
Root cause looks like `_validate_expressions` passing a bare `lazy_gettext` message while the other dataset `ValidationError`s wrap it (`[_("...")]` in `commands/dataset/exceptions.py`) — worth fixing at `commands/dataset/update.py:386` too, so the 422 keeps its documented `{field: [msg]}` shape for non-frontend clients?
##########
superset-frontend/packages/superset-ui-core/src/query/getClientErrorObject.ts:
##########
@@ -126,13 +126,17 @@ export function parseErrorJson(responseJson: JsonObject): ClientErrorObject {
error.error = error.description = error.errors[0].message;
error.link = error.errors[0]?.extra?.link;
}
- // Marshmallow field validation returns the error message in the format
- // of { message: { field1: [msg1, msg2], field2: [msg], } }
+ // Marshmallow field validation returns { field1: [msg1, msg2], ... },
+ // but some validators attach a bare string instead of a list.
if (!error.error && error.message) {
if (typeof error.message === 'object') {
+ const firstFieldMessage = Object.values(
+ error.message as Record<string, string[] | string>,
Review Comment:
Nested marshmallow errors still hit this non-array branch and assign an object to `error.error` — a bad report recipient gives `{"recipients": {"0": {"type": [...]}}}`. In scope here, or deliberately left alone?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): surface real validation message in save-error dialog [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #43459:
URL: https://github.com/apache/superset/pull/43459#issuecomment-5395892383
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/43459?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 78.85%. Comparing base ([`c3ed8b3`](https://app.codecov.io/gh/apache/superset/commit/c3ed8b312d5428d9577ecc5d026c611874e2b4a5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e860446`](https://app.codecov.io/gh/apache/superset/commit/e8604469646fa35b59cb0a4bd63943dfccdf9a6d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #43459 +/- ##
==========================================
- Coverage 78.86% 78.85% -0.01%
==========================================
Files 2876 2876
Lines 164601 164603 +2
Branches 38015 38016 +1
==========================================
- Hits 129806 129802 -4
- Misses 32348 32354 +6
Partials 2447 2447
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/43459/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/43459/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `74.21% <100.00%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/43459?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): surface real validation message in save-error dialog [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #43459:
URL: https://github.com/apache/superset/pull/43459#discussion_r3880251537
##########
superset/commands/dataset/update.py:
##########
@@ -384,7 +384,7 @@ def _validate_expressions(
)
exceptions.append(
ValidationError(
- message,
+ [message],
Review Comment:
Same bare-`LazyString` shape exists in `superset/commands/report/exceptions.py` (~15 sites) and `database/exceptions.py:42` — e.g. `POST /api/v1/report/` with a bad `database` still returns a scalar `{"message": {"database": "..."}}`; worth a follow-up issue rather than widening this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): surface real validation message in save-error dialog [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev merged PR #43459:
URL: https://github.com/apache/superset/pull/43459
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org