codeant-ai-for-open-source[bot] commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3531365778
##########
superset/databases/schemas.py:
##########
@@ -277,6 +277,48 @@ def extra_validator(value: str) -> str:
)
]
)
+
+ # Only validate when the key is present. An absent key means "unset"
+ # (the cache is simply not configured), which is valid. When the key
+ # is present it must be a mapping — this rejects an explicit ``null``
+ # as well as other non-dict values (0, "", []), keeping the API in
+ # sync with ``ImportV1DatabaseExtraSchema`` (whose ``fields.Dict`` also
+ # rejects null) and avoiding a stored ``null`` that would break the
+ # ``Database.metadata_cache_timeout`` accessors.
+ if "metadata_cache_timeout" in extra_:
+ metadata_cache_timeout = extra_["metadata_cache_timeout"]
+ if not isinstance(metadata_cache_timeout, dict):
+ raise ValidationError(
+ [
+ _(
+ "The metadata_cache_timeout must be a mapping from
"
+ "string keys to non-negative integer values."
+ )
+ ]
+ )
+ for key in (
+ "schema_cache_timeout",
+ "table_cache_timeout",
+ "catalog_cache_timeout",
+ ):
+ # An absent key is unset (valid). When the key is present the
+ # value must be a non-negative integer — a present ``null`` is
+ # rejected here too, matching the import schema (its
+ # ``fields.Integer`` has no ``allow_none``) and preventing an
+ # enabled-but-None state in the model accessors.
+ if key not in metadata_cache_timeout:
+ continue
+ timeout = metadata_cache_timeout[key]
+ if not isinstance(timeout, int) or timeout < 0:
Review Comment:
**Suggestion:** The new integer validation accepts booleans because Python
`bool` is a subclass of `int`, so payloads like `true` pass as `1` instead of
being rejected. This violates the stated non-negative integer contract and can
silently turn invalid JSON into unintended cache timeouts. Reject boolean
values explicitly before (or along with) the integer check. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Boolean metadata cache timeouts silently accepted as integers.
- ⚠️ Misconfigured cache durations for metadata are hard to diagnose.
- ⚠️ Test-connection and create endpoints allow invalid timeout shapes.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The `extra` field used to validate database connection parameters is
defined in
`superset/databases/schemas.py:220-222` inside
`DatabaseValidateParametersSchema` as
`extra = fields.String(metadata={"description": extra_description},
validate=extra_validator)`, and this schema is wired into the REST API in
`superset/databases/api.py:98` and referenced in the OpenAPI spec at
`superset/databases/api.py:2027` via `$ref:
"#/components/schemas/DatabaseValidateParametersSchema"` for the
`/api/v1/database/test_connection` endpoint.
2. The `extra_validator` function at `superset/databases/schemas.py:254-73`
parses the
`extra` string with `json.loads(value)` (lines 10-16), obtains the `extra_`
dict, and when
`"metadata_cache_timeout"` is present it iterates over the keys
`"schema_cache_timeout"`,
`"table_cache_timeout"`, and `"catalog_cache_timeout"` (diff lines 50-54),
reading
`timeout = metadata_cache_timeout[key]` and then applying the check `if not
isinstance(timeout, int) or timeout < 0:` (diff line 312).
3. Send a request to an endpoint that uses
`DatabaseValidateParametersSchema`, such as
`/api/v1/database/test_connection` or database create/update, where the
payload includes
`extra` as a JSON string like
`{"metadata_cache_timeout":{"schema_cache_timeout": true}}`;
`json.loads` in `extra_validator` deserializes the JSON boolean `true` into
Python `True`,
so `timeout` becomes `True` for the `schema_cache_timeout` key.
4. Because Python `bool` subclasses `int`, `isinstance(True, int)` and
`isinstance(False,
int)` both return `True`, and `timeout < 0` is `False` for `True` and
`False`, the
condition `if not isinstance(timeout, int) or timeout < 0:` does not
trigger, so
`extra_validator` accepts the boolean value as a “non-negative integer”; the
database
connection is validated and persisted with
`metadata_cache_timeout.schema_cache_timeout`
set to `True` or `False`, which downstream cache logic will treat as `1` or
`0` seconds
rather than rejecting the invalid boolean input as the contract intends.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2626c56597ab4d6bb6f41a270d429931&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=2626c56597ab4d6bb6f41a270d429931&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/databases/schemas.py
**Line:** 312:312
**Comment:**
*Type Error: The new integer validation accepts booleans because Python
`bool` is a subclass of `int`, so payloads like `true` pass as `1` instead of
being rejected. This violates the stated non-negative integer contract and can
silently turn invalid JSON into unintended cache timeouts. Reject boolean
values explicitly before (or along with) the integer check.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38490&comment_hash=43540695d02e128df8599d8c938bf289504d7a33943dc1fe3c8182c9f1d0af0f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38490&comment_hash=43540695d02e128df8599d8c938bf289504d7a33943dc1fe3c8182c9f1d0af0f&reaction=dislike'>👎</a>
--
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]
