rebenitez1802 commented on code in PR #43261:
URL: https://github.com/apache/superset/pull/43261#discussion_r3924508593
##########
superset/commands/importers/v1/utils.py:
##########
@@ -324,6 +324,20 @@ def load_configs(
)
exc.messages = {file_name: exc.messages}
exceptions.append(exc)
+ except json.JSONDecodeError as exc:
Review Comment:
🟡 Medium (non-blocking) — the merge block still 500s on two sibling inputs
This clause only handles `json.JSONDecodeError` from `json.loads` (line
291), but the same merge (lines 291–293) has two other user-reachable raw exits
that still escape as the exact opaque 500 this PR set out to eliminate:
1. **Non-string `masked_encrypted_extra`** — a YAML mapping/list/number is
truthy at the `config.get("masked_encrypted_extra")` guard, so
`simplejson.loads` receives a non-`str` and raises `TypeError` (which is *not*
a `ValueError`/`JSONDecodeError`, so neither `except` catches it).
2. **Malformed JSONPath key** in `encrypted_extra_secrets` —
`set_masked_fields` → `jsonpath_ng.parse` (line 292 → `superset/utils/json.py`)
raises `JsonPathParserError`.
Both are reachable by the same write-privileged importer with a hand-edited
bundle, and neither is covered by the new tests.
Minimal one-click fix for the non-string case (the JSONPath case needs
`JSONPathError` + its import, so it's cleaner to fold into the relocation in my
other comment):
```suggestion
except (json.JSONDecodeError, TypeError) as exc:
```
##########
superset/commands/importers/v1/utils.py:
##########
@@ -324,6 +324,20 @@ def load_configs(
)
exc.messages = {file_name: exc.messages}
exceptions.append(exc)
+ except json.JSONDecodeError as exc:
+ # masked_encrypted_extra comes straight from the imported YAML
+ # (before schema validation) and may not be valid JSON. Convert
+ # the raw decode error into a ValidationError so it flows into
+ # the aggregated CommandInvalidError like every other per-file
+ # validation failure, instead of escaping as an opaque 500.
+ logger.error(
+ "Invalid JSON in masked_encrypted_extra for %s: %s",
+ file_name,
+ exc,
+ )
+ exceptions.append(
+ ValidationError({file_name: {"masked_encrypted_extra":
[str(exc)]}})
Review Comment:
🟡 Medium (non-blocking) — this catch also mislabels unrelated JSON errors
from `schema.load`
Because the `except json.JSONDecodeError` wraps the entire per-file `try`
(including `schema.load(config)` at line 301), it also catches JSON-decode
errors raised *inside* schema loading — not just the `masked_encrypted_extra`
merge. `ImportV1ColumnSchema`/`ImportV1MetricSchema` `@pre_load` hooks call
`json.loads(data["extra"])` unguarded (`superset/datasets/schemas.py`), so
importing a dataset whose column/metric `extra` is non-JSON now gets reported
here as `{file_name: {"masked_encrypted_extra": [...]}}` — but datasets have no
`masked_encrypted_extra` field, so the 422 blames a field that doesn't exist
and hides the real culprit. (Still better than the prior 500, but the
diagnostic is wrong, and this misattribution is newly introduced by the diff.)
This and the "too narrow" note above are both fixed by scoping the catch to
just the merge. It can't be a one-click suggestion because it edits lines
outside this PR's diff, so here's the shape:
```python
if file_name in encrypted_extra_secrets and config.get(
"masked_encrypted_extra"
):
normalized_secrets = {...}
try:
temp_dict =
json.loads(config["masked_encrypted_extra"])
temp_dict = json.set_masked_fields(temp_dict,
normalized_secrets)
config["masked_encrypted_extra"] =
json.dumps(temp_dict)
except (json.JSONDecodeError, TypeError, JSONPathError)
as exc:
logger.error(
"Invalid masked_encrypted_extra for %s: %s",
file_name, exc
)
exceptions.append(
ValidationError(
{file_name: {"masked_encrypted_extra":
[str(exc)]}}
)
)
continue # skip this file; don't add it to configs
```
(`from jsonpath_ng.exceptions import JSONPathError` — the base class of both
`JsonPathParserError` and `JsonPathLexerError`.) The top-level `except
json.JSONDecodeError` clause this PR adds can then be dropped.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
