You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "abderbejaoui (via GitHub)" <gi...@apache.org> on 2026/03/07 17:18:26 UTC
[PR] fix(db): validate schema and table cache timeout to reject negative values [superset]
abderbejaoui opened a new pull request, #38490:
URL: https://github.com/apache/superset/pull/38490
### SUMMARY
Negative values for `schema_cache_timeout` and `table_cache_timeout` in the database connection advanced settings (Performance section) were accepted by both the UI and backend. This could cause runtime errors in Redis and caching backends, as reported in #35068.
This PR adds validation at multiple layers to reject negative values:
**Frontend:**
- Added `onExtraInputChangeNonNegative` handler in `ExtraOptions.tsx` that silently drops negative input values
- Added `min={0}` HTML attribute on both cache timeout inputs to prevent spinner arrows from going below zero
- Added `Math.max(0, ...)` clamping in the state reducer (`index.tsx`) as a safety net
- Changed `||` to `??` for input value display so `0` renders correctly instead of showing as empty
**Backend:**
- Added validation in `extra_validator()` (`schemas.py`) that rejects negative `schema_cache_timeout` and `table_cache_timeout` with a clear error message
- Added `Range(min=0)` constraint on `metadata_cache_timeout` dict values in `ImportV1DatabaseExtraSchema`
Fixes #35068
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
**Before:** User can type negative values like `-4` into Schema cache timeout and Table cache timeout fields, and the backend silently accepts them.
**After:** Negative values are blocked in the UI (input is ignored), and the backend returns a validation error: _"The schema_cache_timeout in metadata_cache_timeout must be a non-negative integer."_
### TESTING INSTRUCTIONS
1. Start Superset and navigate to **Settings → Database Connections**
2. Click **Edit** on any database → expand **Advanced → Performance**
3. Try typing `-4` in "Schema cache timeout" — the input should be ignored
4. Try typing `0` — should be accepted (cache never expires)
5. Try typing `600` — should be accepted normally
6. Verify via API:
```bash
# Should return validation error
curl -X POST /api/v1/database/ -d '{"database_name":"test","sqlalchemy_uri":"...","extra":"{\"metadata_cache_timeout\":{\"schema_cache_timeout\":-1}}"}'
```
7. Run backend tests: `pytest tests/unit_tests/databases/schema_tests.py -v -k cache_timeout`
8. Run frontend tests: `npx jest --testPathPatterns='ExtraOptions.test' --no-coverage`
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #35068
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
Made with [Cursor](https://cursor.com)
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "abderbejaoui (via GitHub)" <gi...@apache.org>.
abderbejaoui commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3553231447
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,277 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"table_cache_timeout": -5}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 0
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 0
+
+
+def test_extra_validator_accepts_positive_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts positive cache timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 600,
+ "table_cache_timeout": 1200,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 600
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 1200
+
+
+def test_extra_validator_rejects_non_dict_metadata_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects metadata_cache_timeout when it is not a dict.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": 600}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "metadata_cache_timeout must be a mapping" in str(exc_info.value)
+
+
+@pytest.mark.parametrize("value", [0, "", [], False, None])
+def test_extra_validator_rejects_non_dict_metadata_cache_timeout_values(
+ value: Any,
+) -> None:
+ """
+ Test that extra_validator rejects a present but non-dict
+ metadata_cache_timeout, including an explicit null (None) and other falsy
+ non-dict values (0, "", [], False), instead of silently treating them as
+ empty. Rejecting null keeps the API in sync with the import schema.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": value}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "metadata_cache_timeout must be a mapping" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_absent_metadata_cache_timeout() -> None:
+ """
+ Test that extra_validator treats an absent metadata_cache_timeout key as
+ unset (valid). Only a present-but-non-dict value is rejected.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_params": {}}),
+ }
+ result = schema.load(payload)
+ assert "metadata_cache_timeout" not in json.loads(result["extra"])
+
+
+@pytest.mark.parametrize(
+ "key",
+ ["schema_cache_timeout", "table_cache_timeout", "catalog_cache_timeout"],
+)
+def test_extra_validator_rejects_null_nested_cache_timeout(key: str) -> None:
+ """
+ Test that extra_validator rejects an explicit null nested cache timeout
+ (e.g. {"metadata_cache_timeout": {"schema_cache_timeout": null}}). Import
+ (fields.Integer, no allow_none) rejects it, so the API must too, keeping
+ the two paths in sync and avoiding an enabled-but-None model state.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {key: None}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+@pytest.mark.parametrize("value", [True, False])
+def test_extra_validator_rejects_boolean_nested_cache_timeout(value: bool) -> None:
+ """
+ Test that extra_validator rejects boolean nested cache timeouts. Python's
+ bool is a subclass of int, so true/false would otherwise pass as 1/0;
+ the import schema's fields.Integer rejects booleans, so the API must too.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {"metadata_cache_timeout": {"schema_cache_timeout": value}}
+ ),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_catalog_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative catalog_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"catalog_cache_timeout": -3}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_catalog_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts non-negative catalog_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"catalog_cache_timeout": 600}}),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["catalog_cache_timeout"] == 600
+
+
+def test_cache_timeout_rejects_values_below_minus_one() -> None:
+ """
+ Test that cache_timeout rejects values less than -1.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": -5,
+ }
+ with pytest.raises(ValidationError):
+ schema.load(payload)
+
+
+def test_cache_timeout_allows_minus_one() -> None:
+ """
+ Test that cache_timeout allows -1 (bypass cache).
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": -1,
+ }
+ result = schema.load(payload)
+ assert result["cache_timeout"] == -1
+
+
+def test_cache_timeout_allows_zero_and_positive() -> None:
+ """
+ Test that cache_timeout allows 0 and positive values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ for value in (0, 300, 86400):
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": value,
+ }
+ result = schema.load(payload)
Review Comment:
Leaving as-is for consistency. Local variables like `schema`/`payload`/`result`/`extra` aren't annotated anywhere in this test file — the whole suite relies on mypy's inference — so annotating only these would be inconsistent with the surrounding tests without adding type safety. Resolving.
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3526399597
##########
superset/databases/schemas.py:
##########
@@ -277,6 +277,29 @@ def extra_validator(value: str) -> str:
)
]
)
+
+ metadata_cache_timeout = extra_.get("metadata_cache_timeout") or {}
+ 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"):
Review Comment:
<!-- Bito Reply -->
The suggestion is appropriate as it ensures that any non-dictionary value provided for `metadata_cache_timeout` is explicitly rejected with a `ValidationError`, rather than silently defaulting to an empty dictionary. This improves the robustness of the input validation logic by enforcing the expected data type.
**superset/databases/schemas.py**
```
metadata_cache_timeout = extra_.get("metadata_cache_timeout")
if metadata_cache_timeout is not None:
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."
)
]
)
else:
metadata_cache_timeout = {}
```
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38490:
URL: https://github.com/apache/superset/pull/38490#issuecomment-4873310984
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - 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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "abderbejaoui (via GitHub)" <gi...@apache.org>.
abderbejaoui commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3553225552
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,277 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
Review Comment:
Leaving as-is for consistency. Local variables like `schema`/`payload`/`result`/`extra` aren't annotated anywhere in this test file — the whole suite relies on mypy's inference — so annotating only these would be inconsistent with the surrounding tests without adding type safety. Resolving.
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "abderbejaoui (via GitHub)" <gi...@apache.org>.
abderbejaoui commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3531487859
##########
superset/databases/schemas.py:
##########
@@ -906,7 +939,10 @@ def fix_schemas_allowed_for_csv_upload( # pylint: disable=invalid-name
metadata_params = fields.Dict(keys=fields.Str(), values=fields.Raw())
engine_params = fields.Dict(keys=fields.Str(), values=fields.Raw())
- metadata_cache_timeout = fields.Dict(keys=fields.Str(), values=fields.Integer())
+ metadata_cache_timeout = fields.Dict(
+ keys=fields.Str(),
+ values=fields.Integer(validate=Range(min=0)),
+ )
Review Comment:
Resolved in 19b675f5e1 . The API validator now only validates `metadata_cache_timeout` when the key is **present**, and requires a mapping — so an explicit `null` is rejected, matching `ImportV1DatabaseExtraSchema` (`fields.Dict` with no `allow_none`). An **absent** key still means "unset" (valid), so no legitimate flow regresses — the UI only ever sends integers or omits the key.
I also closed the symmetric nested case: `{"metadata_cache_timeout": {"schema_cache_timeout": null}}` is now rejected too, matching import's inner `fields.Integer`, which prevents an `enabled=True`/`timeout=None` state in the model accessors. Tests cover top-level null, nested null (all three keys), absent-key, and empty-dict.
Note: I aligned toward the "disallow null everywhere" option. The other option (allow + normalize null) would actually crash `Database.metadata_cache_timeout`, since it returns `get_extra().get("metadata_cache_timeout", {})` — which yields `None` for an explicit null, and `schema_cache_enabled` then does `"…" in None`. Resolving.
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3531231379
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -415,6 +416,15 @@ export function dbReducer(
[action.payload.name]: action.payload.checked,
};
}
+ if (action.payload.name === 'cache_timeout') {
+ const val = Number(action.payload.value);
+ return {
+ ...trimmedState,
+ cache_timeout: Number.isNaN(val)
+ ? undefined
+ : String(Math.max(-1, val)),
+ };
+ }
Review Comment:
<!-- Bito Reply -->
The implementation of the `cache_timeout` logic in the reducer, including the handling of `NaN` values and the clamping of negative values, is correctly addressed by the added tests. The changes effectively ensure that the cache timeout behavior is now covered by the test suite.
**superset-frontend/src/features/databases/DatabaseModal/index.tsx**
```
if (action.payload.name === 'cache_timeout') {
const val = Number(action.payload.value);
return {
...trimmedState,
cache_timeout: Number.isNaN(val)
? undefined
: String(Math.max(-1, val)),
};
}
```
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "abderbejaoui (via GitHub)" <gi...@apache.org>.
abderbejaoui commented on PR #38490:
URL: https://github.com/apache/superset/pull/38490#issuecomment-4402207260
Hi @villebro , just following up on this PR. I addressed the requested changes regarding the cache timeout validation behavior:
* `cache_timeout` now correctly allows `-1` for cache bypass
* values below `-1` are rejected
* `schema_cache_timeout` and `table_cache_timeout` remain non-negative only
* added frontend/backend validation and test coverage for all cases
All CI checks are now passing except for the cancelled pre-commit workflow runs.
Would appreciate another review whenever you have time. Thanks again!
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r2900103359
##########
superset/databases/schemas.py:
##########
@@ -277,6 +277,20 @@ def extra_validator(value: str) -> str:
)
]
)
+
+ metadata_cache_timeout = extra_.get("metadata_cache_timeout", {})
+ for key in ("schema_cache_timeout", "table_cache_timeout"):
+ timeout = metadata_cache_timeout.get(key)
+ if timeout is not None and (not isinstance(timeout, int) or timeout < 0):
+ raise ValidationError(
+ [
+ _(
+ "The %(key)s in metadata_cache_timeout must be a "
+ "non-negative integer.",
+ key=key,
+ )
+ ]
+ )
Review Comment:
**Suggestion:** The new validation assumes that `metadata_cache_timeout` is always a dict and calls `.get()` on it; if existing configs have `metadata_cache_timeout` set to a non-dict (e.g. an integer, string, list, or null), this will raise an AttributeError instead of a clean ValidationError, breaking schema loading for such payloads. Add a type check (and gracefully handle non-dict/None cases) before iterating over keys so that invalid structures are rejected with a proper validation error rather than causing an unexpected exception. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Database creation with malformed metadata_cache_timeout crashes backend.
- ❌ Database update using DatabasePutSchema fails with AttributeError.
- ⚠️ Test-connection flows using DatabaseTestConnectionSchema can error.
- ⚠️ Admin database connection UI may show generic server error.
- ⚠️ Invalid extra structure not reported as clear validation error.
```
</details>
```suggestion
metadata_cache_timeout = extra_.get("metadata_cache_timeout") or {}
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"):
timeout = metadata_cache_timeout.get(key)
if timeout is not None and (not isinstance(timeout, int) or timeout < 0):
raise ValidationError(
[
_(
"The %(key)s in metadata_cache_timeout must be a "
"non-negative integer.",
key=key,
)
]
)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset/databases/schemas.py`, note that `extra_validator` (around lines 246–294
in the final file) deserializes the JSON string `extra` and then executes:
`metadata_cache_timeout = extra_.get("metadata_cache_timeout", {})`
followed by `metadata_cache_timeout.get(...)` on lines 281–283.
2. Also in the same file, observe that multiple Marshmallow schemas attach
`extra_validator` to their `extra` field, for example:
- `DatabasePostSchema.extra` (defined later in `superset/databases/schemas.py`) with
`validate=extra_validator`
- `DatabasePutSchema.extra`, `DatabaseValidateParametersSchema.extra`,
`DatabaseTestConnectionSchema.extra`, and `DatabaseConnectionSchema.extra` all
similarly use `validate=extra_validator`.
3. In a real execution path, any of these schemas will be used to validate payloads
containing a string `extra` field. To reproduce via code, run the Superset backend (or
open a Python shell in the project) and execute:
```python
from superset.databases.schemas import DatabasePostSchema
# `metadata_cache_timeout` is incorrectly set to an integer, not a dict
data = {
"database_name": "test_db",
"sqlalchemy_uri": "sqlite:///test.db",
"extra": '{"metadata_cache_timeout": 600}'
}
DatabasePostSchema().load(data)
```
This triggers `extra_validator` via the `extra` field validation.
4. During `extra_validator` execution, `extra_["metadata_cache_timeout"]` will be the
integer `600`, so `metadata_cache_timeout = 600`. On line 283, `timeout =
metadata_cache_timeout.get(key)` is executed on an `int`, raising `AttributeError: 'int'
object has no attribute 'get'`. This bubbles up as an unexpected exception instead of a
controlled `ValidationError`, causing the database create/update/test flow that uses this
schema to fail with a 500-style error rather than a user-facing validation message.
```
</details>
<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:** 281:293
**Comment:**
*Type Error: The new validation assumes that `metadata_cache_timeout` is always a dict and calls `.get()` on it; if existing configs have `metadata_cache_timeout` set to a non-dict (e.g. an integer, string, list, or null), this will raise an AttributeError instead of a clean ValidationError, breaking schema loading for such payloads. Add a type check (and gracefully handle non-dict/None cases) before iterating over keys so that invalid structures are rejected with a proper validation error rather than causing an unexpected exception.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38490&comment_hash=ccbe8c8a905309512e50c4d029311390f181cf42d9471a51bb4022f24f0fcc55&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38490&comment_hash=ccbe8c8a905309512e50c4d029311390f181cf42d9471a51bb4022f24f0fcc55&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: 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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
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: 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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3531512159
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,215 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"table_cache_timeout": -5}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 0
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 0
+
+
+def test_extra_validator_accepts_positive_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts positive cache timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 600,
+ "table_cache_timeout": 1200,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 600
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 1200
+
+
+def test_extra_validator_rejects_non_dict_metadata_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects metadata_cache_timeout when it is not a dict.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": 600}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "metadata_cache_timeout must be a mapping" in str(exc_info.value)
+
+
+@pytest.mark.parametrize("value", [0, "", [], False])
+def test_extra_validator_rejects_falsy_non_dict_metadata_cache_timeout(
+ value: Any,
+) -> None:
+ """
+ Test that extra_validator rejects falsy non-dict metadata_cache_timeout
+ values (e.g. 0, "", []) instead of silently treating them as empty.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": value}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "metadata_cache_timeout must be a mapping" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_catalog_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative catalog_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"catalog_cache_timeout": -3}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_catalog_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts non-negative catalog_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"catalog_cache_timeout": 600}}),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["catalog_cache_timeout"] == 600
+
+
+def test_cache_timeout_rejects_values_below_minus_one() -> None:
+ """
+ Test that cache_timeout rejects values less than -1.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": -5,
+ }
+ with pytest.raises(ValidationError):
+ schema.load(payload)
+
+
+def test_cache_timeout_allows_minus_one() -> None:
+ """
+ Test that cache_timeout allows -1 (bypass cache).
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": -1,
+ }
+ result = schema.load(payload)
+ assert result["cache_timeout"] == -1
+
+
+def test_cache_timeout_allows_zero_and_positive() -> None:
+ """
+ Test that cache_timeout allows 0 and positive values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ for value in (0, 300, 86400):
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": value,
+ }
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Avoid flagging missing type annotations for local variables in test files; follow the existing test-suite style and rely on mypy inference unless a test file already uses explicit annotations consistently.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38490:
URL: https://github.com/apache/superset/pull/38490#issuecomment-4897928437
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - 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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3523713461
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -415,6 +416,15 @@ export function dbReducer(
[action.payload.name]: action.payload.checked,
};
}
+ if (action.payload.name === 'cache_timeout') {
+ const val = Number(action.payload.value);
+ return {
+ ...trimmedState,
+ cache_timeout: Number.isNaN(val)
+ ? undefined
+ : String(Math.max(-1, val)),
+ };
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing tests for new cache_timeout InputChange branch</b></div>
<div id="fix">
The new `InputChange` branch for `cache_timeout` (lines 419-427) implements NaN→undefined and negative-value clamping, but no existing test covers this logic. This is the only reducer branch that writes to `cache_timeout`, so the entire behavior is untested.
</div>
</div>
<small><i>Code Review Run #cd433e</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3526400520
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -17,7 +17,7 @@
# pylint: disable=import-outside-toplevel, invalid-name, unused-argument, redefined-outer-name
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not require type annotations for local variables in test files; keep test code concise unless typing is needed for clarity or correctness.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "abderbejaoui (via GitHub)" <gi...@apache.org>.
abderbejaoui commented on PR #38490:
URL: https://github.com/apache/superset/pull/38490#issuecomment-4017866450
@villebro Thanks for the great catch! You're right, -1 means "bypass cache" for the Chart cache timeout field, as documented here: https://superset.apache.org/admin-docs/configuration/cache/#chart-cache-timeout
I've updated the PR to handle this properly:
Chart cache timeout (cache_timeout): now allows -1 (bypass cache), 0 (never expires), and positive values. Only values below -1 are rejected.
Schema/Table cache timeout (metadata_cache_timeout): these remain non-negative only (≥ 0), since they have different semantics , their helper text states "If left unset, the cache never expires" with no -1 bypass behavior, and the backend treats negative values as unexpected for these fields.
Validation is enforced on both frontend (input handler + min attribute) and backend (Marshmallow Range(min=-1) on all cache_timeout schema fields). Added tests for all cases.
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38490:
URL: https://github.com/apache/superset/pull/38490#issuecomment-4016961376
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3529100210
##########
superset/databases/schemas.py:
##########
@@ -906,7 +939,10 @@ def fix_schemas_allowed_for_csv_upload( # pylint: disable=invalid-name
metadata_params = fields.Dict(keys=fields.Str(), values=fields.Raw())
engine_params = fields.Dict(keys=fields.Str(), values=fields.Raw())
- metadata_cache_timeout = fields.Dict(keys=fields.Str(), values=fields.Integer())
+ metadata_cache_timeout = fields.Dict(
+ keys=fields.Str(),
+ values=fields.Integer(validate=Range(min=0)),
+ )
Review Comment:
**Suggestion:** This import schema now rejects `metadata_cache_timeout: null` because the dict field does not allow `None`, while the API validator path currently accepts `null`; that creates an API/import contract mismatch where a database config accepted and exported by one path can fail on import. Align both paths by either disallowing `null` everywhere or allowing and normalizing it consistently. [api mismatch]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Exported database configs with null timeout cannot import.
- ⚠️ Assets export/import pipeline becomes non-round-trippable.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or update a database through the REST API (`POST /api/v1/database/` / `PUT
/api/v1/database/<id>` using `DatabasePostSchema` / `DatabasePutSchema`) with Extra JSON
containing `"metadata_cache_timeout": null`; `extra_validator` in
`superset/databases/schemas.py:254-309` accepts this value because it treats `None` as an
empty dict locally (`superset/databases/schemas.py:281-283`) and returns the original
string unchanged, so the database is stored with `metadata_cache_timeout` set to null.
2. Export all assets using `GET /api/v1/assets/export/` handled by
`ImportExportRestApi.export` (`superset/importexport/api.py:46-83`), which invokes
`ExportAssetsCommand` to serialize databases into YAML; the exported database config will
include `extra` as a mapping with `"metadata_cache_timeout": null` reflecting the
persisted state.
3. Import the same bundle via `POST /api/v1/assets/import/` (`ImportExportRestApi.import_`
in `superset/importexport/api.py:95-200`), which uses `ImportAssetsCommand`
(`superset/commands/importers/v1/assets.py:59-73`) and its `schemas` mapping to validate
database files against `ImportV1DatabaseSchema` (`superset/databases/schemas.py:79-123`),
where `extra` is a nested `ImportV1DatabaseExtraSchema`.
4. During import, `ImportV1DatabaseExtraSchema.metadata_cache_timeout`
(`superset/databases/schemas.py:61-66`) is a `fields.Dict` whose values are
`fields.Integer(validate=Range(min=0))` and the field does not allow `None` for the
top-level mapping; when it encounters `metadata_cache_timeout: null` from the exported
config, marshmallow raises a `ValidationError`, causing the database asset import to fail
even though the configuration was originally accepted and exported by the API.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a74392afa27541e6a132354bed77fd4d&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=a74392afa27541e6a132354bed77fd4d&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:** 942:945
**Comment:**
*Api Mismatch: This import schema now rejects `metadata_cache_timeout: null` because the dict field does not allow `None`, while the API validator path currently accepts `null`; that creates an API/import contract mismatch where a database config accepted and exported by one path can fail on import. Align both paths by either disallowing `null` everywhere or allowing and normalizing it consistently.
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=e0a7c9777dfd17a297cd4893a813db971d42b5e3bb047af9a34f9f1db78e8714&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38490&comment_hash=e0a7c9777dfd17a297cd4893a813db971d42b5e3bb047af9a34f9f1db78e8714&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: 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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "abderbejaoui (via GitHub)" <gi...@apache.org>.
abderbejaoui commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3531221927
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,215 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
Review Comment:
These match the existing style throughout schema_tests.py — local vars like schema/payload/result aren't annotated anywhere in this file, and mypy infers them, so annotating only the new ones would make them inconsistent with ~30 surrounding tests. Leaving as-is for consistency. Marking resolved.
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "abderbejaoui (via GitHub)" <gi...@apache.org>.
abderbejaoui commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3531509264
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,215 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"table_cache_timeout": -5}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 0
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 0
+
+
+def test_extra_validator_accepts_positive_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts positive cache timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 600,
+ "table_cache_timeout": 1200,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 600
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 1200
+
+
+def test_extra_validator_rejects_non_dict_metadata_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects metadata_cache_timeout when it is not a dict.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": 600}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "metadata_cache_timeout must be a mapping" in str(exc_info.value)
+
+
+@pytest.mark.parametrize("value", [0, "", [], False])
+def test_extra_validator_rejects_falsy_non_dict_metadata_cache_timeout(
+ value: Any,
+) -> None:
+ """
+ Test that extra_validator rejects falsy non-dict metadata_cache_timeout
+ values (e.g. 0, "", []) instead of silently treating them as empty.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": value}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "metadata_cache_timeout must be a mapping" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_catalog_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative catalog_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"catalog_cache_timeout": -3}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_catalog_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts non-negative catalog_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"catalog_cache_timeout": 600}}),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["catalog_cache_timeout"] == 600
+
+
+def test_cache_timeout_rejects_values_below_minus_one() -> None:
+ """
+ Test that cache_timeout rejects values less than -1.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": -5,
+ }
+ with pytest.raises(ValidationError):
+ schema.load(payload)
+
+
+def test_cache_timeout_allows_minus_one() -> None:
+ """
+ Test that cache_timeout allows -1 (bypass cache).
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": -1,
+ }
+ result = schema.load(payload)
+ assert result["cache_timeout"] == -1
+
+
+def test_cache_timeout_allows_zero_and_positive() -> None:
+ """
+ Test that cache_timeout allows 0 and positive values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ for value in (0, 300, 86400):
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": value,
+ }
Review Comment:
Leaving as-is for consistency. `payload` (and the other locals here) aren't annotated anywhere else in this test file — the suite relies on mypy's inference — so annotating just this one wouldn't add type safety and would be inconsistent with the surrounding tests. Resolving.
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3553234429
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,277 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"table_cache_timeout": -5}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 0
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 0
+
+
+def test_extra_validator_accepts_positive_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts positive cache timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 600,
+ "table_cache_timeout": 1200,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 600
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 1200
+
+
+def test_extra_validator_rejects_non_dict_metadata_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects metadata_cache_timeout when it is not a dict.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": 600}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "metadata_cache_timeout must be a mapping" in str(exc_info.value)
+
+
+@pytest.mark.parametrize("value", [0, "", [], False, None])
+def test_extra_validator_rejects_non_dict_metadata_cache_timeout_values(
+ value: Any,
+) -> None:
+ """
+ Test that extra_validator rejects a present but non-dict
+ metadata_cache_timeout, including an explicit null (None) and other falsy
+ non-dict values (0, "", [], False), instead of silently treating them as
+ empty. Rejecting null keeps the API in sync with the import schema.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": value}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "metadata_cache_timeout must be a mapping" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_absent_metadata_cache_timeout() -> None:
+ """
+ Test that extra_validator treats an absent metadata_cache_timeout key as
+ unset (valid). Only a present-but-non-dict value is rejected.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_params": {}}),
+ }
+ result = schema.load(payload)
+ assert "metadata_cache_timeout" not in json.loads(result["extra"])
+
+
+@pytest.mark.parametrize(
+ "key",
+ ["schema_cache_timeout", "table_cache_timeout", "catalog_cache_timeout"],
+)
+def test_extra_validator_rejects_null_nested_cache_timeout(key: str) -> None:
+ """
+ Test that extra_validator rejects an explicit null nested cache timeout
+ (e.g. {"metadata_cache_timeout": {"schema_cache_timeout": null}}). Import
+ (fields.Integer, no allow_none) rejects it, so the API must too, keeping
+ the two paths in sync and avoiding an enabled-but-None model state.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {key: None}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+@pytest.mark.parametrize("value", [True, False])
+def test_extra_validator_rejects_boolean_nested_cache_timeout(value: bool) -> None:
+ """
+ Test that extra_validator rejects boolean nested cache timeouts. Python's
+ bool is a subclass of int, so true/false would otherwise pass as 1/0;
+ the import schema's fields.Integer rejects booleans, so the API must too.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {"metadata_cache_timeout": {"schema_cache_timeout": value}}
+ ),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_catalog_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative catalog_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"catalog_cache_timeout": -3}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_catalog_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts non-negative catalog_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"catalog_cache_timeout": 600}}),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["catalog_cache_timeout"] == 600
+
+
+def test_cache_timeout_rejects_values_below_minus_one() -> None:
+ """
+ Test that cache_timeout rejects values less than -1.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": -5,
+ }
+ with pytest.raises(ValidationError):
+ schema.load(payload)
+
+
+def test_cache_timeout_allows_minus_one() -> None:
+ """
+ Test that cache_timeout allows -1 (bypass cache).
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": -1,
+ }
+ result = schema.load(payload)
+ assert result["cache_timeout"] == -1
+
+
+def test_cache_timeout_allows_zero_and_positive() -> None:
+ """
+ Test that cache_timeout allows 0 and positive values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ for value in (0, 300, 86400):
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": value,
+ }
+ result = schema.load(payload)
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Avoid flagging missing local variable type annotations in test files when the surrounding test suite consistently relies on type inference and does not annotate similar locals.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.<ext>`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #38490:
URL: https://github.com/apache/superset/pull/38490
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3531225040
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,215 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not require explicit type annotations for new local variables in this test file; follow the existing unannotated local-variable style used throughout schema_tests.py for consistency.
**Applied to:**
- `**/schema_tests.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "abderbejaoui (via GitHub)" <gi...@apache.org>.
abderbejaoui commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3531239885
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -343,13 +343,14 @@ export function dbReducer(
action.payload.name === 'schema_cache_timeout' ||
action.payload.name === 'table_cache_timeout'
) {
+ const timeoutValue = Math.max(0, Number(action.payload.value) || 0);
Review Comment:
Fixed in 19a8c26 per @rusackas's suggestion — empty string now maps to NaN before coercion and falls through to undefined, clearing back to the global default. Resolving.
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "abderbejaoui (via GitHub)" <gi...@apache.org>.
abderbejaoui commented on PR #38490:
URL: https://github.com/apache/superset/pull/38490#issuecomment-4893087079
Thanks @rusackas — applied both suggestions exactly (schema/table → undefined, cache_timeout empty → NaN → falls through to undefined), so clearing a field now returns it to unset/global default instead of pinning to 0. Also added dbReducer tests pinning the clear-to-empty behavior for both the metadata timeouts and cache_timeout. Ready for another look whenever you have a moment 🙏
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #38490:
URL: https://github.com/apache/superset/pull/38490#issuecomment-4896206378
The bot comments are piling up here. Let me know if you want help, but we should assess each of them, and either apply the suggestions (especially major/critical ones) or refute them in a comment and mark them as resolved.
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3526558039
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -415,6 +416,15 @@ export function dbReducer(
[action.payload.name]: action.payload.checked,
};
}
+ if (action.payload.name === 'cache_timeout') {
+ const val = Number(action.payload.value);
Review Comment:
Here's an apply-able version, `Number('')` is `0` not `NaN`, so treating an empty field as `NaN` lets it fall through to `undefined` and clear back to the default.\n\n```suggestion\n const val =\n action.payload.value === '' ? NaN : Number(action.payload.value);\n```
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -343,13 +343,14 @@ export function dbReducer(
action.payload.name === 'schema_cache_timeout' ||
action.payload.name === 'table_cache_timeout'
) {
+ const timeoutValue = Math.max(0, Number(action.payload.value) || 0);
Review Comment:
Same idea here, an empty field should drop the key rather than store `0`. Setting it `undefined` lets `JSON.stringify` omit it so it goes back to unset.
```suggestion
const timeoutValue =
action.payload.value === ''\n ? undefined\n : Math.max(0, Number(action.payload.value) || 0);\n```
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3526571268
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -415,6 +416,15 @@ export function dbReducer(
[action.payload.name]: action.payload.checked,
};
}
+ if (action.payload.name === 'cache_timeout') {
+ const val = Number(action.payload.value);
Review Comment:
Here's an apply-able version. `Number('')` is `0` not `NaN`, so treating an empty field as `NaN` lets it fall through to `undefined` and clear back to the default.
```suggestion
const val =
action.payload.value === '' ? NaN : Number(action.payload.value);
```
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -343,13 +343,14 @@ export function dbReducer(
action.payload.name === 'schema_cache_timeout' ||
action.payload.name === 'table_cache_timeout'
) {
+ const timeoutValue = Math.max(0, Number(action.payload.value) || 0);
Review Comment:
Same idea here, an empty field should drop the key rather than store `0`. Setting it `undefined` lets `JSON.stringify` omit it so it goes back to unset.
```suggestion
const timeoutValue =
action.payload.value === ''
? undefined
: Math.max(0, Number(action.payload.value) || 0);
```
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "abderbejaoui (via GitHub)" <gi...@apache.org>.
abderbejaoui commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3553227274
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,277 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"table_cache_timeout": -5}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 0
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 0
+
+
+def test_extra_validator_accepts_positive_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts positive cache timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 600,
+ "table_cache_timeout": 1200,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
Review Comment:
Leaving as-is for consistency. Local variables like `schema`/`payload`/`result`/`extra` aren't annotated anywhere in this test file — the whole suite relies on mypy's inference — so annotating only these would be inconsistent with the surrounding tests without adding type safety. Resolving.
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r2900325931
##########
superset/databases/schemas.py:
##########
@@ -277,6 +277,29 @@ def extra_validator(value: str) -> str:
)
]
)
+
+ metadata_cache_timeout = extra_.get("metadata_cache_timeout") or {}
+ 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"):
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Validation Logic Bug</b></div>
<div id="fix">
The current validation allows falsy non-dict values like 0 for metadata_cache_timeout to pass without error, defaulting to {}. This should raise a ValidationError for any non-dict value when the key is present, as invalid input should be rejected.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
metadata_cache_timeout = extra_.get("metadata_cache_timeout")
if metadata_cache_timeout is not None:
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."
)
]
)
else:
metadata_cache_timeout = {}
for key in ("schema_cache_timeout", "table_cache_timeout"):
````
</div>
</details>
</div>
<small><i>Code Review Run #35969a</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38490:
URL: https://github.com/apache/superset/pull/38490#issuecomment-4880395968
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - 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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #38490:
URL: https://github.com/apache/superset/pull/38490#issuecomment-4888546594
Heya, thanks for the contribution. We have a ton of bot comments to assess/apply/reject here... did you want to take those, or shall I/we push some commits to resolve them if warranted?
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #38490:
URL: https://github.com/apache/superset/pull/38490#issuecomment-4889056439
Thanks for this @abderbejaoui, the validation and the -1 bypass handling look solid. I cleared out a pile of bot threads that were either stale or over-applying the type-hint rule to test locals.
Two things I think are worth addressing before we merge:
The big one is the empty-input behavior in the reducer. When you clear one of the cache-timeout fields back to an empty string, `Number('')` is 0, not NaN, so `schema_cache_timeout`/`table_cache_timeout` end up stored as `0` and `cache_timeout` as `'0'`. That's "never expire" rather than unset/global default, so there's no way to clear a value back to the default once it's set. I think clearing should drop the key / go to undefined. The bot flagged this three times (schema/table + chart) and I think it's the same real issue in all of them.
Also the new `cache_timeout` branch in `dbReducer` isn't covered by a test, the frontend tests exercise the `ExtraOptions` inputs but not the reducer itself. A small test pinning the clear-to-empty behavior would let us merge with more confidence, especially since that's where the fix above lands. Thanks!
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3531498790
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,215 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"table_cache_timeout": -5}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Avoid flagging missing local variable type annotations in test files when the surrounding tests consistently rely on type inference and adding annotations would not improve type safety.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "abderbejaoui (via GitHub)" <gi...@apache.org>.
abderbejaoui commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3531475492
##########
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:
Good catch — fixed in `1221c121e7`. Since Python's `bool` is a subclass of `int`, `true`/`false` were slipping through the integer check as `1`/`0`. The validator now rejects booleans explicitly:
```python
if (
isinstance(timeout, bool)
or not isinstance(timeout, int)
or timeout < 0
):
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "abderbejaoui (via GitHub)" <gi...@apache.org>.
abderbejaoui commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3531568321
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -415,6 +416,15 @@ export function dbReducer(
[action.payload.name]: action.payload.checked,
};
}
+ if (action.payload.name === 'cache_timeout') {
+ const val = Number(action.payload.value);
Review Comment:
Applied as suggested in 19a8c261f6 — empty string now maps to `NaN` before coercion, so it falls through to `undefined` and clears back to the global default. Covered by a new `dbReducer` test. Thanks!
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3553232899
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,277 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"table_cache_timeout": -5}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 0
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 0
+
+
+def test_extra_validator_accepts_positive_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts positive cache timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 600,
+ "table_cache_timeout": 1200,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 600
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 1200
+
+
+def test_extra_validator_rejects_non_dict_metadata_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects metadata_cache_timeout when it is not a dict.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": 600}),
+ }
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not require explicit type annotations for new local variables in these database schema tests; rely on mypy inference to stay consistent with the surrounding test code.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,277 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"table_cache_timeout": -5}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 0
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 0
+
+
+def test_extra_validator_accepts_positive_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts positive cache timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 600,
+ "table_cache_timeout": 1200,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 600
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 1200
+
+
+def test_extra_validator_rejects_non_dict_metadata_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects metadata_cache_timeout when it is not a dict.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": 600}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "metadata_cache_timeout must be a mapping" in str(exc_info.value)
+
+
+@pytest.mark.parametrize("value", [0, "", [], False, None])
+def test_extra_validator_rejects_non_dict_metadata_cache_timeout_values(
+ value: Any,
+) -> None:
+ """
+ Test that extra_validator rejects a present but non-dict
+ metadata_cache_timeout, including an explicit null (None) and other falsy
+ non-dict values (0, "", [], False), instead of silently treating them as
+ empty. Rejecting null keeps the API in sync with the import schema.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": value}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "metadata_cache_timeout must be a mapping" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_absent_metadata_cache_timeout() -> None:
+ """
+ Test that extra_validator treats an absent metadata_cache_timeout key as
+ unset (valid). Only a present-but-non-dict value is rejected.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_params": {}}),
+ }
+ result = schema.load(payload)
+ assert "metadata_cache_timeout" not in json.loads(result["extra"])
+
+
+@pytest.mark.parametrize(
+ "key",
+ ["schema_cache_timeout", "table_cache_timeout", "catalog_cache_timeout"],
+)
+def test_extra_validator_rejects_null_nested_cache_timeout(key: str) -> None:
+ """
+ Test that extra_validator rejects an explicit null nested cache timeout
+ (e.g. {"metadata_cache_timeout": {"schema_cache_timeout": null}}). Import
+ (fields.Integer, no allow_none) rejects it, so the API must too, keeping
+ the two paths in sync and avoiding an enabled-but-None model state.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {key: None}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+@pytest.mark.parametrize("value", [True, False])
+def test_extra_validator_rejects_boolean_nested_cache_timeout(value: bool) -> None:
+ """
+ Test that extra_validator rejects boolean nested cache timeouts. Python's
+ bool is a subclass of int, so true/false would otherwise pass as 1/0;
+ the import schema's fields.Integer rejects booleans, so the API must too.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {"metadata_cache_timeout": {"schema_cache_timeout": value}}
+ ),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_catalog_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative catalog_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"catalog_cache_timeout": -3}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_catalog_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts non-negative catalog_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"catalog_cache_timeout": 600}}),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["catalog_cache_timeout"] == 600
+
+
+def test_cache_timeout_rejects_values_below_minus_one() -> None:
+ """
+ Test that cache_timeout rejects values less than -1.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": -5,
+ }
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not require explicit type annotations for local variables in test files when the surrounding test suite consistently relies on mypy inference and unannotated locals.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "abderbejaoui (via GitHub)" <gi...@apache.org>.
abderbejaoui commented on PR #38490:
URL: https://github.com/apache/superset/pull/38490#issuecomment-4880368828
Thanks @rusackas! I took both suggestions:
- `metadata_cache_timeout` now only defaults to `{}` when the key is absent/`None`, so falsy non-dict values (`0`, `""`, `[]`, `false`) are rejected instead of being treated as empty.
- Added the non-negative check for `catalog_cache_timeout` alongside `schema_cache_timeout` / `table_cache_timeout`.
Added tests covering both. Could you re-approve the workflows so CI can run? Thanks again for the review 🙏
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3526399112
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -17,7 +17,7 @@
# pylint: disable=import-outside-toplevel, invalid-name, unused-argument, redefined-outer-name
Review Comment:
Local in a test again, no annotation needed here. Resolving.
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -17,7 +17,7 @@
# pylint: disable=import-outside-toplevel, invalid-name, unused-argument, redefined-outer-name
-from typing import TYPE_CHECKING
+from typing import Any, TYPE_CHECKING
Review Comment:
Same, local test var. Resolving.
##########
superset/databases/schemas.py:
##########
@@ -277,6 +277,29 @@ def extra_validator(value: str) -> str:
)
]
)
+
+ metadata_cache_timeout = extra_.get("metadata_cache_timeout") or {}
+ 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"):
Review Comment:
Looks like the current version reads the raw value first and only defaults to {} when the key is actually missing, then the isinstance check catches falsy non-dicts. There's even a test covering 0/""/[]/False now. So this is handled. Resolving.
##########
superset/databases/schemas.py:
##########
@@ -277,6 +277,29 @@ def extra_validator(value: str) -> str:
)
]
)
+
+ metadata_cache_timeout = extra_.get("metadata_cache_timeout") or {}
+ if not isinstance(metadata_cache_timeout, dict):
Review Comment:
The current validator doesn't do `or {}` anymore, it reads the raw value and only substitutes {} when it's None, so falsy invalid types still hit the isinstance check. Handled in the current version. Resolving.
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3526558039
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -415,6 +416,15 @@ export function dbReducer(
[action.payload.name]: action.payload.checked,
};
}
+ if (action.payload.name === 'cache_timeout') {
+ const val = Number(action.payload.value);
Review Comment:
Here's an apply-able version, `Number('')` is `0` not `NaN`, so treating an empty field as `NaN` lets it fall through to `undefined` and clear back to the default.\n\n```suggestion\n const val =\n action.payload.value === '' ? NaN : Number(action.payload.value);\n```
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -343,13 +343,14 @@ export function dbReducer(
action.payload.name === 'schema_cache_timeout' ||
action.payload.name === 'table_cache_timeout'
) {
+ const timeoutValue = Math.max(0, Number(action.payload.value) || 0);
Review Comment:
Same idea here, an empty field should drop the key rather than store `0`. Setting it `undefined` lets `JSON.stringify` omit it so it goes back to unset.
```suggestion
const timeoutValue =
action.payload.value === ''\n ? undefined\n : Math.max(0, Number(action.payload.value) || 0);\n```
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38490:
URL: https://github.com/apache/superset/pull/38490#issuecomment-4017133962
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - 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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38490:
URL: https://github.com/apache/superset/pull/38490#issuecomment-4018009023
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - 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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "abderbejaoui (via GitHub)" <gi...@apache.org>.
abderbejaoui commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3531515276
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,215 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"table_cache_timeout": -5}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
Review Comment:
Leaving as-is for consistency. `extra` and the other locals here aren't annotated anywhere else in this test file — the suite relies on mypy's inference — so annotating only this one wouldn't add type safety and would be inconsistent with the surrounding tests. Resolving.
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "abderbejaoui (via GitHub)" <gi...@apache.org>.
abderbejaoui commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3531553274
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -343,13 +343,14 @@ export function dbReducer(
action.payload.name === 'schema_cache_timeout' ||
action.payload.name === 'table_cache_timeout'
) {
+ const timeoutValue = Math.max(0, Number(action.payload.value) || 0);
Review Comment:
Applied exactly as suggested in 19a8c261f6 — empty input now maps to `undefined`, so `JSON.stringify` omits the key and the timeout goes back to unset instead of being pinned to `0`. Added `dbReducer` tests covering the clear-to-empty behavior for `schema_cache_timeout`/`table_cache_timeout` (and the same fix for `cache_timeout`). Thanks!
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "abderbejaoui (via GitHub)" <gi...@apache.org>.
abderbejaoui commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3531496163
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,215 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"table_cache_timeout": -5}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
Review Comment:
Leaving as-is for consistency. Local variables like `result`/`schema`/`payload`/`extra` aren't annotated anywhere in this test file — the surrounding tests all rely on mypy's inference — so annotating only the new ones would make them inconsistent with the rest of the suite without adding type safety. Resolving.
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #38490:
URL: https://github.com/apache/superset/pull/38490#issuecomment-4420640963
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 59eb9f43aa9e1074606c280f3416214b8f4e4b12 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a01ca0a89955a00087d11ea |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-38490--superset-docs-preview.netlify.app](https://deploy-preview-38490--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcrLDYAgEEXRXmYtcVj47cAynjAGEhGiaDTG3mV17-K8lPYYUqaRqKJ5x2Zc-cXf9YXVW2RRh3ESoAxKVfZB4pkLtpLW-Ey28BasDRj9MDQNmLnvrNYC-n4) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-38490--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "abderbejaoui (via GitHub)" <gi...@apache.org>.
abderbejaoui commented on PR #38490:
URL: https://github.com/apache/superset/pull/38490#issuecomment-4026854612
@villebro Thanks again for the feedback earlier!
I've pushed an update addressing your comment:
* cache_timeout now allows -1 (bypass cache), 0, and positive values.
* Values below -1 are rejected.
* schema_cache_timeout and table_cache_timeout remain non-negative only.
Also, it looks like the CI checks are still pending because the workflows require maintainer approval ("22 workflows awaiting approval"). Could someone approve them so the tests can run?
I'd appreciate another look whenever you have time. Thanks!
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3553210764
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,277 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
Review Comment:
**Suggestion:** Add explicit type annotations for the new local variables used to build and validate the test payload. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new test introduces local variables `schema` and `payload` without type annotations. This matches the Python type-hint rule for newly added or modified code where relevant variables can be annotated.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=af463dc998664edd94d648689a9cca0e&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=af463dc998664edd94d648689a9cca0e&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:** tests/unit_tests/databases/schema_tests.py
**Line:** 354:360
**Comment:**
*Custom Rule: Add explicit type annotations for the new local variables used to build and validate the test payload.
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=4f93ec19d79dd82214e42c458222e0970b348f23374dc333878027bcd1e6df5c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38490&comment_hash=4f93ec19d79dd82214e42c458222e0970b348f23374dc333878027bcd1e6df5c&reaction=dislike'>👎</a>
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,277 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"table_cache_timeout": -5}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 0
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 0
+
+
+def test_extra_validator_accepts_positive_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts positive cache timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 600,
+ "table_cache_timeout": 1200,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 600
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 1200
+
+
+def test_extra_validator_rejects_non_dict_metadata_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects metadata_cache_timeout when it is not a dict.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": 600}),
+ }
Review Comment:
**Suggestion:** Annotate the new local `schema` and `payload` variables with explicit types in this test. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This added test setup contains unannotated local variables that can be typed, so it violates the requirement to add type hints for new Python code.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2817dadf3a27405cbb79569a07f95fee&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=2817dadf3a27405cbb79569a07f95fee&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:** tests/unit_tests/databases/schema_tests.py
**Line:** 436:440
**Comment:**
*Custom Rule: Annotate the new local `schema` and `payload` variables with explicit types in this test.
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=d0d20c8e9a77711d763a8645825165a8fa4a226d769862e38a32930733af54db&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38490&comment_hash=d0d20c8e9a77711d763a8645825165a8fa4a226d769862e38a32930733af54db&reaction=dislike'>👎</a>
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,277 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"table_cache_timeout": -5}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 0
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 0
+
+
+def test_extra_validator_accepts_positive_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts positive cache timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 600,
+ "table_cache_timeout": 1200,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
Review Comment:
**Suggestion:** Add type annotations to the local variables that hold schema output and parsed JSON content. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
These newly added local variables are untyped, and both `result` and `extra` are obvious candidates for annotation under the type-hint rule.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3c94adef21334b0e97ef70034677ee31&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=3c94adef21334b0e97ef70034677ee31&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:** tests/unit_tests/databases/schema_tests.py
**Line:** 424:425
**Comment:**
*Custom Rule: Add type annotations to the local variables that hold schema output and parsed JSON content.
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=833154c8e046a01346b12f3e7a3212725fd15ce78970f22af43f9a8a11b7c604&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38490&comment_hash=833154c8e046a01346b12f3e7a3212725fd15ce78970f22af43f9a8a11b7c604&reaction=dislike'>👎</a>
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,277 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"table_cache_timeout": -5}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 0
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 0
+
+
+def test_extra_validator_accepts_positive_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts positive cache timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 600,
+ "table_cache_timeout": 1200,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 600
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 1200
+
+
+def test_extra_validator_rejects_non_dict_metadata_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects metadata_cache_timeout when it is not a dict.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": 600}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "metadata_cache_timeout must be a mapping" in str(exc_info.value)
+
+
+@pytest.mark.parametrize("value", [0, "", [], False, None])
+def test_extra_validator_rejects_non_dict_metadata_cache_timeout_values(
+ value: Any,
+) -> None:
+ """
+ Test that extra_validator rejects a present but non-dict
+ metadata_cache_timeout, including an explicit null (None) and other falsy
+ non-dict values (0, "", [], False), instead of silently treating them as
+ empty. Rejecting null keeps the API in sync with the import schema.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": value}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "metadata_cache_timeout must be a mapping" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_absent_metadata_cache_timeout() -> None:
+ """
+ Test that extra_validator treats an absent metadata_cache_timeout key as
+ unset (valid). Only a present-but-non-dict value is rejected.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_params": {}}),
+ }
+ result = schema.load(payload)
+ assert "metadata_cache_timeout" not in json.loads(result["extra"])
+
+
+@pytest.mark.parametrize(
+ "key",
+ ["schema_cache_timeout", "table_cache_timeout", "catalog_cache_timeout"],
+)
+def test_extra_validator_rejects_null_nested_cache_timeout(key: str) -> None:
+ """
+ Test that extra_validator rejects an explicit null nested cache timeout
+ (e.g. {"metadata_cache_timeout": {"schema_cache_timeout": null}}). Import
+ (fields.Integer, no allow_none) rejects it, so the API must too, keeping
+ the two paths in sync and avoiding an enabled-but-None model state.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {key: None}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+@pytest.mark.parametrize("value", [True, False])
+def test_extra_validator_rejects_boolean_nested_cache_timeout(value: bool) -> None:
+ """
+ Test that extra_validator rejects boolean nested cache timeouts. Python's
+ bool is a subclass of int, so true/false would otherwise pass as 1/0;
+ the import schema's fields.Integer rejects booleans, so the API must too.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {"metadata_cache_timeout": {"schema_cache_timeout": value}}
+ ),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_catalog_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative catalog_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"catalog_cache_timeout": -3}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_catalog_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts non-negative catalog_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"catalog_cache_timeout": 600}}),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["catalog_cache_timeout"] == 600
+
+
+def test_cache_timeout_rejects_values_below_minus_one() -> None:
+ """
+ Test that cache_timeout rejects values less than -1.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": -5,
+ }
Review Comment:
**Suggestion:** Add explicit type annotations for local variables that are initialized in the cache-timeout boundary test. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new local variables `schema` and `payload` are not annotated, and this is exactly the kind of omission covered by the Python type-hint rule.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=39e39e10ad264df5af024818fb5e1b75&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=39e39e10ad264df5af024818fb5e1b75&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:** tests/unit_tests/databases/schema_tests.py
**Line:** 566:570
**Comment:**
*Custom Rule: Add explicit type annotations for local variables that are initialized in the cache-timeout boundary test.
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=e2980b8a91ee1a59642fdf8c1ff8b296fc56b0d97a6b6ea1d35fea0c8be9cfad&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38490&comment_hash=e2980b8a91ee1a59642fdf8c1ff8b296fc56b0d97a6b6ea1d35fea0c8be9cfad&reaction=dislike'>👎</a>
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,277 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"table_cache_timeout": -5}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 0
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 0
+
+
+def test_extra_validator_accepts_positive_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts positive cache timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 600,
+ "table_cache_timeout": 1200,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 600
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 1200
+
+
+def test_extra_validator_rejects_non_dict_metadata_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects metadata_cache_timeout when it is not a dict.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": 600}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "metadata_cache_timeout must be a mapping" in str(exc_info.value)
+
+
+@pytest.mark.parametrize("value", [0, "", [], False, None])
+def test_extra_validator_rejects_non_dict_metadata_cache_timeout_values(
+ value: Any,
+) -> None:
+ """
+ Test that extra_validator rejects a present but non-dict
+ metadata_cache_timeout, including an explicit null (None) and other falsy
+ non-dict values (0, "", [], False), instead of silently treating them as
+ empty. Rejecting null keeps the API in sync with the import schema.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": value}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "metadata_cache_timeout must be a mapping" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_absent_metadata_cache_timeout() -> None:
+ """
+ Test that extra_validator treats an absent metadata_cache_timeout key as
+ unset (valid). Only a present-but-non-dict value is rejected.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_params": {}}),
+ }
+ result = schema.load(payload)
+ assert "metadata_cache_timeout" not in json.loads(result["extra"])
+
+
+@pytest.mark.parametrize(
+ "key",
+ ["schema_cache_timeout", "table_cache_timeout", "catalog_cache_timeout"],
+)
+def test_extra_validator_rejects_null_nested_cache_timeout(key: str) -> None:
+ """
+ Test that extra_validator rejects an explicit null nested cache timeout
+ (e.g. {"metadata_cache_timeout": {"schema_cache_timeout": null}}). Import
+ (fields.Integer, no allow_none) rejects it, so the API must too, keeping
+ the two paths in sync and avoiding an enabled-but-None model state.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {key: None}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+@pytest.mark.parametrize("value", [True, False])
+def test_extra_validator_rejects_boolean_nested_cache_timeout(value: bool) -> None:
+ """
+ Test that extra_validator rejects boolean nested cache timeouts. Python's
+ bool is a subclass of int, so true/false would otherwise pass as 1/0;
+ the import schema's fields.Integer rejects booleans, so the API must too.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {"metadata_cache_timeout": {"schema_cache_timeout": value}}
+ ),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_catalog_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative catalog_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"catalog_cache_timeout": -3}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_catalog_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts non-negative catalog_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"catalog_cache_timeout": 600}}),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["catalog_cache_timeout"] == 600
+
+
+def test_cache_timeout_rejects_values_below_minus_one() -> None:
+ """
+ Test that cache_timeout rejects values less than -1.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": -5,
+ }
+ with pytest.raises(ValidationError):
+ schema.load(payload)
+
+
+def test_cache_timeout_allows_minus_one() -> None:
+ """
+ Test that cache_timeout allows -1 (bypass cache).
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": -1,
+ }
+ result = schema.load(payload)
+ assert result["cache_timeout"] == -1
+
+
+def test_cache_timeout_allows_zero_and_positive() -> None:
+ """
+ Test that cache_timeout allows 0 and positive values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ for value in (0, 300, 86400):
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": value,
+ }
+ result = schema.load(payload)
Review Comment:
**Suggestion:** Add type annotations for the newly introduced mutable test data object and loaded response variables. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The loop creates new local variables `payload` and `result` without type annotations. Since these values can be annotated, the suggestion correctly identifies a type-hint omission.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e2a915ac699642489c9a38f3badc4517&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=e2a915ac699642489c9a38f3badc4517&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:** tests/unit_tests/databases/schema_tests.py
**Line:** 597:602
**Comment:**
*Custom Rule: Add type annotations for the newly introduced mutable test data object and loaded response variables.
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=91e5015024d75e0826a9f891466104688f25b5ab2988d0fce016c1d2a6640bcf&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38490&comment_hash=91e5015024d75e0826a9f891466104688f25b5ab2988d0fce016c1d2a6640bcf&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: 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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "abderbejaoui (via GitHub)" <gi...@apache.org>.
abderbejaoui commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3553229580
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,277 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"table_cache_timeout": -5}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 0
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 0
+
+
+def test_extra_validator_accepts_positive_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts positive cache timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 600,
+ "table_cache_timeout": 1200,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 600
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 1200
+
+
+def test_extra_validator_rejects_non_dict_metadata_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects metadata_cache_timeout when it is not a dict.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": 600}),
+ }
Review Comment:
Leaving as-is for consistency. Local variables like `schema`/`payload`/`result`/`extra` aren't annotated anywhere in this test file — the whole suite relies on mypy's inference — so annotating only these would be inconsistent with the surrounding tests without adding type safety. Resolving.
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,277 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"table_cache_timeout": -5}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 0
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 0
+
+
+def test_extra_validator_accepts_positive_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts positive cache timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 600,
+ "table_cache_timeout": 1200,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 600
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 1200
+
+
+def test_extra_validator_rejects_non_dict_metadata_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects metadata_cache_timeout when it is not a dict.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": 600}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "metadata_cache_timeout must be a mapping" in str(exc_info.value)
+
+
+@pytest.mark.parametrize("value", [0, "", [], False, None])
+def test_extra_validator_rejects_non_dict_metadata_cache_timeout_values(
+ value: Any,
+) -> None:
+ """
+ Test that extra_validator rejects a present but non-dict
+ metadata_cache_timeout, including an explicit null (None) and other falsy
+ non-dict values (0, "", [], False), instead of silently treating them as
+ empty. Rejecting null keeps the API in sync with the import schema.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": value}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "metadata_cache_timeout must be a mapping" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_absent_metadata_cache_timeout() -> None:
+ """
+ Test that extra_validator treats an absent metadata_cache_timeout key as
+ unset (valid). Only a present-but-non-dict value is rejected.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_params": {}}),
+ }
+ result = schema.load(payload)
+ assert "metadata_cache_timeout" not in json.loads(result["extra"])
+
+
+@pytest.mark.parametrize(
+ "key",
+ ["schema_cache_timeout", "table_cache_timeout", "catalog_cache_timeout"],
+)
+def test_extra_validator_rejects_null_nested_cache_timeout(key: str) -> None:
+ """
+ Test that extra_validator rejects an explicit null nested cache timeout
+ (e.g. {"metadata_cache_timeout": {"schema_cache_timeout": null}}). Import
+ (fields.Integer, no allow_none) rejects it, so the API must too, keeping
+ the two paths in sync and avoiding an enabled-but-None model state.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {key: None}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+@pytest.mark.parametrize("value", [True, False])
+def test_extra_validator_rejects_boolean_nested_cache_timeout(value: bool) -> None:
+ """
+ Test that extra_validator rejects boolean nested cache timeouts. Python's
+ bool is a subclass of int, so true/false would otherwise pass as 1/0;
+ the import schema's fields.Integer rejects booleans, so the API must too.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {"metadata_cache_timeout": {"schema_cache_timeout": value}}
+ ),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_catalog_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative catalog_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"catalog_cache_timeout": -3}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_catalog_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts non-negative catalog_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"catalog_cache_timeout": 600}}),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["catalog_cache_timeout"] == 600
+
+
+def test_cache_timeout_rejects_values_below_minus_one() -> None:
+ """
+ Test that cache_timeout rejects values less than -1.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": -5,
+ }
Review Comment:
Leaving as-is for consistency. Local variables like `schema`/`payload`/`result`/`extra` aren't annotated anywhere in this test file — the whole suite relies on mypy's inference — so annotating only these would be inconsistent with the surrounding tests without adding type safety. Resolving.
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "abderbejaoui (via GitHub)" <gi...@apache.org>.
abderbejaoui commented on PR #38490:
URL: https://github.com/apache/superset/pull/38490#issuecomment-4896866595
Thanks for the thorough pass, @rusackas! I've gone through every open thread — applied the real ones and refuted the nits, all resolved:
**Applied:**
- Your two inline suggestions — empty input now clears `schema_cache_timeout`/`table_cache_timeout` to unset and `cache_timeout` to the global default (19a8c261f6), plus new `dbReducer` tests pinning the clear-to-empty behavior.
- CodeAnt **Critical** (null API/import mismatch) — the API now rejects `metadata_cache_timeout: null`, top-level and nested, matching `ImportV1DatabaseExtraSchema` (19b675f5e1).
- CodeAnt **Major** (booleans passing as ints) — the validator now rejects `true`/`false`, matching marshmallow's `fields.Integer` (1221c121e7).
**Refuted + resolved:** the Minor `[custom_rule]` type-annotation suggestions on the test locals — the surrounding tests in that file don't annotate `schema`/`result`/`payload`/`extra` and rely on mypy inference, so annotating only the new ones would be inconsistent without adding type safety.
One pre-existing, out-of-scope note: marshmallow coerces `"600"`/`1.5` on import while the API uses strict `isinstance` — happy to open a follow-up to fully unify the two schemas if you'd like.
Heads up on CI: the new pushes re-gate the workflows ("awaiting approval"), so they'll need a re-approval to run. All checks were green on the prior run and these changes are lint-clean and covered by tests.
I'm around for any further review — thanks again! 🙏
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38490:
URL: https://github.com/apache/superset/pull/38490#issuecomment-4883480924
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - 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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3524252845
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -415,6 +416,15 @@ export function dbReducer(
[action.payload.name]: action.payload.checked,
};
}
+ if (action.payload.name === 'cache_timeout') {
+ const val = Number(action.payload.value);
+ return {
+ ...trimmedState,
+ cache_timeout: Number.isNaN(val)
+ ? undefined
+ : String(Math.max(-1, val)),
+ };
+ }
Review Comment:
**Suggestion:** When the chart cache timeout input is cleared, `Number('')` becomes `0`, so the reducer stores `0` instead of removing the value. This makes it impossible to return to the documented “use global default if undefined” behavior via normal clearing. Handle empty string explicitly before numeric coercion. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Clearing chart cache timeout forces cache to never expire.
- ⚠️ Admins cannot restore global cache timeout by clearing field.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In the UI, open a database connection edit modal (DatabaseConnectionForm rendered from
`superset-frontend/src/features/databases/DatabaseModal/index.tsx`, lines 1940-1979) and
go to “Advanced → Performance” where the chart cache timeout input is rendered by
`ExtraOptions`.
2. In `ExtraOptions`
(`superset-frontend/src/features/databases/DatabaseModal/ExtraOptions.tsx`, lines
360-399), locate the “Chart cache timeout” input: `<Input type="number"
name="cache_timeout" min={-1} value={db?.cache_timeout ?? ''} ...
onChange={onInputChangeValidateTimeout} />`. Its helper text states that 0 means cache
never expires, -1 bypasses the cache, and “undefined” defaults to the global timeout.
3. Clear the “Chart cache timeout” field so the browser input value becomes `''`. The
change handler `onInputChangeValidateTimeout` (ExtraOptions.tsx lines 77-85) computes
`Number(target.value)`, which for `''` is `0`, and only blocks values less than `-1`.
Since `0 < -1` is false, it forwards the event to `onInputChange`.
4. `DatabaseConnectionForm` forwards this event to the reducer via `onInputChange`
(index.tsx lines 1940-21), dispatching `ActionType.InputChange` with `name:
'cache_timeout', value: ''`. In `dbReducer`’s `InputChange` case (index.tsx lines 153-168;
diff lines 413-426), the reducer runs:
- `const val = Number(action.payload.value);` → `val = 0` for `''`.
- `cache_timeout: Number.isNaN(val) ? undefined : String(Math.max(-1, val)),`
Since `val` is `0`, `Number.isNaN(val)` is false and `Math.max(-1, 0)` is `0`, so it
stores `cache_timeout = '0'` in state. On save, this is persisted to
`Database.cache_timeout` (`superset/models/core.py` line 27) and validated by
`DatabaseConnectionSchema.cache_timeout` (`superset/databases/schemas.py` lines
1185-1189) which allows `0`. As a result, clearing the field sets an explicit zero
timeout instead of `None`, making it impossible via normal UI clearing to return to the
documented “use global timeout when undefined” behavior referenced in
`cache_timeout_description` (`superset/databases/schemas.py` lines 90-94).
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=57af5dc73ba34b44bc34b88baaa815ed&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=57af5dc73ba34b44bc34b88baaa815ed&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-frontend/src/features/databases/DatabaseModal/index.tsx
**Line:** 419:427
**Comment:**
*Logic Error: When the chart cache timeout input is cleared, `Number('')` becomes `0`, so the reducer stores `0` instead of removing the value. This makes it impossible to return to the documented “use global default if undefined” behavior via normal clearing. Handle empty string explicitly before numeric coercion.
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=7320fde01ce15943fcb61d3e0dcd0fbd5b360095fee289f85f86a29e3cf987ef&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38490&comment_hash=7320fde01ce15943fcb61d3e0dcd0fbd5b360095fee289f85f86a29e3cf987ef&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: 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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38490:
URL: https://github.com/apache/superset/pull/38490#issuecomment-4885122702
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - 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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3524247572
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -17,7 +17,7 @@
# pylint: disable=import-outside-toplevel, invalid-name, unused-argument, redefined-outer-name
Review Comment:
**Suggestion:** Add a type annotation for `result` to make the loaded schema output explicitly typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
`result` is a new local variable in the added test code and can be type-annotated. Since the rule flags relevant variables that omit type hints, this is a genuine violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e9a4bef01d4148e794d2dfc43df0fc88&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=e9a4bef01d4148e794d2dfc43df0fc88&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:** tests/unit_tests/databases/schema_tests.py
**Line:** 19:19
**Comment:**
*Custom Rule: Add a type annotation for `result` to make the loaded schema output explicitly typed.
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=4bc8b8849ba0f0d087451b51fedc5a60e853a21b9c6da5e91d39496c2dcbfb4c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38490&comment_hash=4bc8b8849ba0f0d087451b51fedc5a60e853a21b9c6da5e91d39496c2dcbfb4c&reaction=dislike'>👎</a>
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -17,7 +17,7 @@
# pylint: disable=import-outside-toplevel, invalid-name, unused-argument, redefined-outer-name
-from typing import TYPE_CHECKING
+from typing import Any, TYPE_CHECKING
Review Comment:
**Suggestion:** Add an explicit type annotation to `extra` so JSON-decoded content has a declared type. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
`extra` is a newly introduced local variable holding JSON-decoded data, so it is an annotatable variable under the type-hint rule. The omission is therefore real.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d6ea067181974608998a828a380974b7&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=d6ea067181974608998a828a380974b7&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:** tests/unit_tests/databases/schema_tests.py
**Line:** 20:20
**Comment:**
*Custom Rule: Add an explicit type annotation to `extra` so JSON-decoded content has a declared type.
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=83b2dd8924e83ba5795089767f09a267243cb1467404d363f201c525319cefe1&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38490&comment_hash=83b2dd8924e83ba5795089767f09a267243cb1467404d363f201c525319cefe1&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: 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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3526399662
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,167 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {"metadata_cache_timeout": {"schema_cache_timeout": -1}}
+ ),
+ }
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not require explicit type annotations for local variables inside test files; allow mypy to infer them in test code.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,167 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {"metadata_cache_timeout": {"schema_cache_timeout": -1}}
+ ),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {"metadata_cache_timeout": {"table_cache_timeout": -5}}
+ ),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout (cache never expires).
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not require type annotations for local variables inside test files; mypy handles these cases in tests.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,167 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {"metadata_cache_timeout": {"schema_cache_timeout": -1}}
+ ),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {"metadata_cache_timeout": {"table_cache_timeout": -5}}
+ ),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout (cache never expires).
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not require concrete type annotations for newly introduced local variables in test files.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,167 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {"metadata_cache_timeout": {"schema_cache_timeout": -1}}
+ ),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {"metadata_cache_timeout": {"table_cache_timeout": -5}}
+ ),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout (cache never expires).
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 0
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 0
+
+
+def test_extra_validator_accepts_positive_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts positive cache timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 600,
+ "table_cache_timeout": 1200,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 600
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 1200
+
+
+def test_extra_validator_rejects_non_dict_metadata_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects metadata_cache_timeout when it is not a dict.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": 600}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "metadata_cache_timeout must be a mapping" in str(exc_info.value)
+
+
+def test_cache_timeout_rejects_values_below_minus_one() -> None:
+ """
+ Test that cache_timeout rejects values less than -1.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": -5,
+ }
+ with pytest.raises(ValidationError):
+ schema.load(payload)
+
+
+def test_cache_timeout_allows_minus_one() -> None:
+ """
+ Test that cache_timeout allows -1 (bypass cache).
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": -1,
+ }
+ result = schema.load(payload)
+ assert result["cache_timeout"] == -1
+
+
+def test_cache_timeout_allows_zero_and_positive() -> None:
+ """
+ Test that cache_timeout allows 0 and positive values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ for value in (0, 300, 86400):
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": value,
+ }
+ result = schema.load(payload)
+ assert result["cache_timeout"] == value
+
+
+def test_cache_timeout_allows_none() -> None:
+ """
+ Test that cache_timeout allows None (use global default).
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": None,
+ }
+ result = schema.load(payload)
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not require explicit type annotations for local dictionaries in test files; rely on mypy inference for test payloads and fixtures.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.<ext>`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #38490:
URL: https://github.com/apache/superset/pull/38490#issuecomment-4753126978
@villebro the `-1` bypass point you raised looks handled now. `cache_timeout` allows `-1` while `schema_cache_timeout`/`table_cache_timeout` stay non-negative, with tests for each. Your CHANGES_REQUESTED is still blocking, so would you mind giving it another pass?
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3526398692
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,167 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {"metadata_cache_timeout": {"schema_cache_timeout": -1}}
+ ),
+ }
Review Comment:
I don't think we need type annotations on locals inside a test like this, mypy infers these fine and we don't enforce it. Resolving.
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,167 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {"metadata_cache_timeout": {"schema_cache_timeout": -1}}
+ ),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {"metadata_cache_timeout": {"table_cache_timeout": -5}}
+ ),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout (cache never expires).
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
Review Comment:
Same here, we don't annotate locals inside tests. mypy handles it. Resolving.
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,167 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {"metadata_cache_timeout": {"schema_cache_timeout": -1}}
+ ),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {"metadata_cache_timeout": {"table_cache_timeout": -5}}
+ ),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout (cache never expires).
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
Review Comment:
Same as above, no need to annotate a local in a test. Resolving.
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,167 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {"metadata_cache_timeout": {"schema_cache_timeout": -1}}
+ ),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {"metadata_cache_timeout": {"table_cache_timeout": -5}}
+ ),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout (cache never expires).
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 0
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 0
+
+
+def test_extra_validator_accepts_positive_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts positive cache timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 600,
+ "table_cache_timeout": 1200,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 600
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 1200
+
+
+def test_extra_validator_rejects_non_dict_metadata_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects metadata_cache_timeout when it is not a dict.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": 600}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "metadata_cache_timeout must be a mapping" in str(exc_info.value)
+
+
+def test_cache_timeout_rejects_values_below_minus_one() -> None:
+ """
+ Test that cache_timeout rejects values less than -1.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": -5,
+ }
+ with pytest.raises(ValidationError):
+ schema.load(payload)
+
+
+def test_cache_timeout_allows_minus_one() -> None:
+ """
+ Test that cache_timeout allows -1 (bypass cache).
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": -1,
+ }
+ result = schema.load(payload)
+ assert result["cache_timeout"] == -1
+
+
+def test_cache_timeout_allows_zero_and_positive() -> None:
+ """
+ Test that cache_timeout allows 0 and positive values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ for value in (0, 300, 86400):
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": value,
+ }
+ result = schema.load(payload)
+ assert result["cache_timeout"] == value
+
+
+def test_cache_timeout_allows_none() -> None:
+ """
+ Test that cache_timeout allows None (use global default).
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": None,
+ }
+ result = schema.load(payload)
Review Comment:
Local test dict, mypy infers this. Not something we annotate. Resolving.
##########
superset/databases/schemas.py:
##########
@@ -277,6 +277,35 @@ def extra_validator(value: str) -> str:
)
]
)
+
+ metadata_cache_timeout = extra_.get("metadata_cache_timeout")
+ if metadata_cache_timeout is None:
+ 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",
+ ):
+ timeout = metadata_cache_timeout.get(key)
Review Comment:
These are locals inside the validator, mypy infers them fine. We annotate signatures, not every local. Resolving.
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3553228589
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,277 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Avoid flagging missing local type annotations in test files when the surrounding test suite consistently relies on mypy inference and unannotated local variables.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.<ext>`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3553230811
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,277 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"table_cache_timeout": -5}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 0
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 0
+
+
+def test_extra_validator_accepts_positive_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts positive cache timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 600,
+ "table_cache_timeout": 1200,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not require local variable type annotations in test files that already rely on mypy inference for consistency; leave inferred locals unannotated unless there is a clear need for explicit typing.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3522425088
##########
superset/databases/schemas.py:
##########
@@ -277,6 +277,35 @@ def extra_validator(value: str) -> str:
)
]
)
+
+ metadata_cache_timeout = extra_.get("metadata_cache_timeout")
+ if metadata_cache_timeout is None:
+ 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",
+ ):
+ timeout = metadata_cache_timeout.get(key)
Review Comment:
**Suggestion:** Add explicit type annotations for the new local variables used in metadata cache timeout validation to satisfy the type-hint requirement for newly introduced logic. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new validation logic introduces local variables whose types can be annotated, and they currently lack explicit type hints. This matches the custom rule requiring type hints on relevant newly added Python variables.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ae5d3e0f16b14a8d8ffa5784f3e14846&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=ae5d3e0f16b14a8d8ffa5784f3e14846&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:** 281:298
**Comment:**
*Custom Rule: Add explicit type annotations for the new local variables used in metadata cache timeout validation to satisfy the type-hint requirement for newly introduced logic.
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=125f84e754deec034436cff923f5237a025773623cfc82556462d51a50335c76&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38490&comment_hash=125f84e754deec034436cff923f5237a025773623cfc82556462d51a50335c76&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: 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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3529049404
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,215 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
Review Comment:
**Suggestion:** Add an explicit type annotation to `schema` so this new local variable is type-hinted. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This new local variable is untyped in newly added Python code, and the rule requires type hints for relevant variables that can be annotated.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bf01854eb8ab4f7ab6943e726bf07e06&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=bf01854eb8ab4f7ab6943e726bf07e06&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:** tests/unit_tests/databases/schema_tests.py
**Line:** 356:356
**Comment:**
*Custom Rule: Add an explicit type annotation to `schema` so this new local variable is type-hinted.
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=f3096b1dcb5c4d7e8a2c1438a31dfc3a1080845cad0ae4b34fba017596dbd113&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38490&comment_hash=f3096b1dcb5c4d7e8a2c1438a31dfc3a1080845cad0ae4b34fba017596dbd113&reaction=dislike'>👎</a>
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,215 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"table_cache_timeout": -5}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
Review Comment:
**Suggestion:** Type-annotate `result` to make the loaded schema output type explicit. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new local variable `result` is not type-annotated, so this is a real instance of missing type hints in added Python code.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fa2096ca79f64a628e5aefb8e46d0ca0&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=fa2096ca79f64a628e5aefb8e46d0ca0&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:** tests/unit_tests/databases/schema_tests.py
**Line:** 400:400
**Comment:**
*Custom Rule: Type-annotate `result` to make the loaded schema output type explicit.
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=986cb6caa29456b87b3a4ce5f40b3a965c8813a529ae757c51474f987f0e89bc&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38490&comment_hash=986cb6caa29456b87b3a4ce5f40b3a965c8813a529ae757c51474f987f0e89bc&reaction=dislike'>👎</a>
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,215 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"table_cache_timeout": -5}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
Review Comment:
**Suggestion:** Add a type annotation to `extra` so this parsed object is explicitly typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This added local variable is untyped despite being a candidate for annotation, so it violates the type-hint rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a9b22050be3c4e338864310bd4105cc9&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=a9b22050be3c4e338864310bd4105cc9&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:** tests/unit_tests/databases/schema_tests.py
**Line:** 401:401
**Comment:**
*Custom Rule: Add a type annotation to `extra` so this parsed object is explicitly typed.
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=15c0ea2ad5cb449256f6d19b9fe2b874adc7cf6daf882460f8bdd9d028f5a71f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38490&comment_hash=15c0ea2ad5cb449256f6d19b9fe2b874adc7cf6daf882460f8bdd9d028f5a71f&reaction=dislike'>👎</a>
##########
tests/unit_tests/databases/schema_tests.py:
##########
@@ -347,6 +347,215 @@ def test_import_schema_rejects_masked_fields_for_new_db(
assert "$.credentials_info.private_key" in error_messages
+def test_extra_validator_rejects_negative_schema_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative schema_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"schema_cache_timeout": -1}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_table_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative table_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"table_cache_timeout": -5}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_zero_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts zero as a valid cache timeout.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 0,
+ "table_cache_timeout": 0,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 0
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 0
+
+
+def test_extra_validator_accepts_positive_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts positive cache timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps(
+ {
+ "metadata_cache_timeout": {
+ "schema_cache_timeout": 600,
+ "table_cache_timeout": 1200,
+ }
+ }
+ ),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["schema_cache_timeout"] == 600
+ assert extra["metadata_cache_timeout"]["table_cache_timeout"] == 1200
+
+
+def test_extra_validator_rejects_non_dict_metadata_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects metadata_cache_timeout when it is not a dict.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": 600}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "metadata_cache_timeout must be a mapping" in str(exc_info.value)
+
+
+@pytest.mark.parametrize("value", [0, "", [], False])
+def test_extra_validator_rejects_falsy_non_dict_metadata_cache_timeout(
+ value: Any,
+) -> None:
+ """
+ Test that extra_validator rejects falsy non-dict metadata_cache_timeout
+ values (e.g. 0, "", []) instead of silently treating them as empty.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": value}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "metadata_cache_timeout must be a mapping" in str(exc_info.value)
+
+
+def test_extra_validator_rejects_negative_catalog_cache_timeout() -> None:
+ """
+ Test that extra_validator rejects negative catalog_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"catalog_cache_timeout": -3}}),
+ }
+ with pytest.raises(ValidationError) as exc_info:
+ schema.load(payload)
+ assert "non-negative integer" in str(exc_info.value)
+
+
+def test_extra_validator_accepts_catalog_cache_timeout() -> None:
+ """
+ Test that extra_validator accepts non-negative catalog_cache_timeout values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "extra": json.dumps({"metadata_cache_timeout": {"catalog_cache_timeout": 600}}),
+ }
+ result = schema.load(payload)
+ extra = json.loads(result["extra"])
+ assert extra["metadata_cache_timeout"]["catalog_cache_timeout"] == 600
+
+
+def test_cache_timeout_rejects_values_below_minus_one() -> None:
+ """
+ Test that cache_timeout rejects values less than -1.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": -5,
+ }
+ with pytest.raises(ValidationError):
+ schema.load(payload)
+
+
+def test_cache_timeout_allows_minus_one() -> None:
+ """
+ Test that cache_timeout allows -1 (bypass cache).
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": -1,
+ }
+ result = schema.load(payload)
+ assert result["cache_timeout"] == -1
+
+
+def test_cache_timeout_allows_zero_and_positive() -> None:
+ """
+ Test that cache_timeout allows 0 and positive values.
+ """
+ from superset.databases.schemas import DatabasePostSchema
+
+ schema = DatabasePostSchema()
+ for value in (0, 300, 86400):
+ payload = {
+ "database_name": "test_db",
+ "cache_timeout": value,
+ }
Review Comment:
**Suggestion:** Add a type annotation for the loop-assigned `payload` variable in this new block to satisfy variable type-hint requirements. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The loop-created `payload` variable is newly added and lacks a type hint, which is exactly what the rule flags for annotatable variables.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b5f01ba5809b4894b0466601201255ae&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=b5f01ba5809b4894b0466601201255ae&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:** tests/unit_tests/databases/schema_tests.py
**Line:** 536:539
**Comment:**
*Custom Rule: Add a type annotation for the loop-assigned `payload` variable in this new block to satisfy variable type-hint requirements.
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=41f03882c835cd6a4a14b2084feed5a7b1d9821fd07d364c6640b1abaa5f0565&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38490&comment_hash=41f03882c835cd6a4a14b2084feed5a7b1d9821fd07d364c6640b1abaa5f0565&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: 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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38490:
URL: https://github.com/apache/superset/pull/38490#issuecomment-4894935873
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - 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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "abderbejaoui (via GitHub)" <gi...@apache.org>.
abderbejaoui commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3531487859
##########
superset/databases/schemas.py:
##########
@@ -906,7 +939,10 @@ def fix_schemas_allowed_for_csv_upload( # pylint: disable=invalid-name
metadata_params = fields.Dict(keys=fields.Str(), values=fields.Raw())
engine_params = fields.Dict(keys=fields.Str(), values=fields.Raw())
- metadata_cache_timeout = fields.Dict(keys=fields.Str(), values=fields.Integer())
+ metadata_cache_timeout = fields.Dict(
+ keys=fields.Str(),
+ values=fields.Integer(validate=Range(min=0)),
+ )
Review Comment:
Resolved in `19b675f5e1`. The API validator now only validates `metadata_cache_timeout` when the key is **present**, and requires a mapping — so an explicit `null` is rejected, matching `ImportV1DatabaseExtraSchema` (`fields.Dict` with no `allow_none`). An **absent** key still means "unset" (valid), so no legitimate flow regresses — the UI only ever sends integers or omits the key.
I also closed the symmetric nested case: `{"metadata_cache_timeout": {"schema_cache_timeout": null}}` is now rejected too, matching import's inner `fields.Integer`, which prevents an `enabled=True`/`timeout=None` state in the model accessors. Tests cover top-level null, nested null (all three keys), absent-key, and empty-dict.
Note: I aligned toward the "disallow null everywhere" option. The other option (allow + normalize null) would actually crash `Database.metadata_cache_timeout`, since it returns `get_extra().get("metadata_cache_timeout", {})` — which yields `None` for an explicit null, and `schema_cache_enabled` then does `"…" in None`. Resolving.
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "abderbejaoui (via GitHub)" <gi...@apache.org>.
abderbejaoui commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3531475492
##########
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:
Good catch — fixed in 1221c121e7. Since Python's `bool` is a subclass of `int`, `true`/`false` were slipping through the integer check as `1`/`0`. The validator now rejects booleans explicitly:
```python
if (
isinstance(timeout, bool)
or not isinstance(timeout, int)
or timeout < 0
):
--
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(db): validate schema and table cache timeout to reject negative values [superset]
Posted by "abderbejaoui (via GitHub)" <gi...@apache.org>.
abderbejaoui commented on code in PR #38490:
URL: https://github.com/apache/superset/pull/38490#discussion_r3531230222
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -415,6 +416,15 @@ export function dbReducer(
[action.payload.name]: action.payload.checked,
};
}
+ if (action.payload.name === 'cache_timeout') {
+ const val = Number(action.payload.value);
+ return {
+ ...trimmedState,
+ cache_timeout: Number.isNaN(val)
+ ? undefined
+ : String(Math.max(-1, val)),
+ };
+ }
Review Comment:
Addressed in 19a8c26 — added dbReducer tests: InputChange stores cache_timeout, allowing -1 (bypass cache) and InputChange clears cache_timeout back to unset on empty input. Resolving.
--
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