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)
+
+
[email protected]("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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
