codeant-ai-for-open-source[bot] commented on code in PR #43351:
URL: https://github.com/apache/superset/pull/43351#discussion_r3826188389
##########
tests/integration_tests/databases/commands_tests.py:
##########
@@ -1138,6 +1138,10 @@ def test_validate_partial(is_port_open,
is_hostname_valid, app_context):
def test_validate_partial_invalid_hostname(is_hostname_valid, app_context):
"""
Test parameter validation when only some parameters are present.
+
+ ``port`` is intentionally absent from the payload (and from the expected
+ "missing" list below): it is no longer a required parameter for
+ Postgres, since a blank port falls back to the default (5432).
Review Comment:
**Suggestion:** The added docstring says `port` is absent from the payload,
but the payload still includes `"port": None`. This means the test does not
cover the missing-key case described by the comment; either remove the key to
exercise that path or update the documentation to describe a null port.
[docstring mismatch]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Test documentation inaccurately describes the payload.
- โ ๏ธ Omitted-key validation coverage is not demonstrated here.
- โ ๏ธ PostgreSQL runtime defaulting remains implemented at
`postgres.py:715-741`.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/databases/commands_tests.py
**Line:** 1142:1144
**Comment:**
*Docstring Mismatch: The added docstring says `port` is absent from the
payload, but the payload still includes `"port": None`. This means the test
does not cover the missing-key case described by the comment; either remove the
key to exercise that path or update the documentation to describe a null port.
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%2F43351&comment_hash=d20c85f4b26e62f0bf991977e4e5642dff03111325907a51c313c49d64137756&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43351&comment_hash=d20c85f4b26e62f0bf991977e4e5642dff03111325907a51c313c49d64137756&reaction=dislike'>๐</a>
##########
superset/db_engine_specs/base.py:
##########
@@ -3010,6 +3010,11 @@ class BasicParametersMixin:
# for Databend this would be `{"sslmode": "disable"}`, eg.
encryption_disable_parameters: dict[str, str] = {}
+ # parameters that `validate_parameters` treats as mandatory; subclasses
+ # override this to relax a parameter (e.g. `port`) without duplicating
+ # the rest of `validate_parameters`
+ required_parameters: set[str] = {"host", "port", "username", "database"}
Review Comment:
**Suggestion:** Making `port` optional causes an explicitly supplied port of
`0` to bypass validation: the unchanged `if not port` early return runs before
the range and open-port checks. PostgreSQL therefore proceeds with port `0`
because the URI override preserves it, instead of reporting the port validation
error that applies when a port is supplied. Distinguish a missing/blank port
from an explicit zero before returning. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ PostgreSQL dynamic form accepts port zero without validation.
- โ ๏ธ Users receive connection failure instead of port diagnostics.
- โ ๏ธ Port-openness checks are skipped for this input.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/base.py
**Line:** 3016:3016
**Comment:**
*Incorrect Condition Logic: Making `port` optional causes an explicitly
supplied port of `0` to bypass validation: the unchanged `if not port` early
return runs before the range and open-port checks. PostgreSQL therefore
proceeds with port `0` because the URI override preserves it, instead of
reporting the port validation error that applies when a port is supplied.
Distinguish a missing/blank port from an explicit zero before returning.
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%2F43351&comment_hash=6ca5aa8902fb6dde9a13809a779442f028b260fb2dc758fd988b2e703b72b1ce&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43351&comment_hash=6ca5aa8902fb6dde9a13809a779442f028b260fb2dc758fd988b2e703b72b1ce&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]
