codeant-ai-for-open-source[bot] commented on code in PR #38229:
URL: https://github.com/apache/superset/pull/38229#discussion_r2849857929
##########
superset/cli/test_db.py:
##########
@@ -181,7 +181,13 @@ def collect_connection_info(
"""
Collect ``engine_kwargs`` if needed.
"""
- console.print(f"[green]SQLAlchemy URI: [bold]{sqlalchemy_uri}")
+ try:
+ safe_uri = make_url_safe(str(sqlalchemy_uri)).render_as_string(
+ hide_password=True
+ )
+ except ArgumentError:
Review Comment:
**Suggestion:** `make_url_safe` does not raise
`sqlalchemy.exc.ArgumentError` (it wraps URL parsing failures in
`DatabaseInvalidError`), so malformed SQLAlchemy URIs will cause an unhandled
exception here instead of being gracefully masked as "<invalid database URI>".
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ `superset test-db` crashes on malformed SQLAlchemy URIs.
- ⚠️ Users lose the masked URI display and interactive prompts.
- ⚠️ Error handling inconsistent with `make_url_safe` abstraction intent.
```
</details>
```suggestion
except Exception:
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the CLI command `superset test-db` which is implemented by the
`test_db` click
command in `superset/cli/test_db.py:133-170`, passing a malformed SQLAlchemy
URI, e.g.:
`superset test-db 'not-a-valid-uri://:@/'`.
2. Inside `test_db` (`superset/cli/test_db.py:155-160`), the function calls
`collect_connection_info(console, sqlalchemy_uri, raw_engine_kwargs)`.
3. `collect_connection_info` (`superset/cli/test_db.py:176-204`) executes
the block at
lines 184-189:
it calls
`make_url_safe(str(sqlalchemy_uri)).render_as_string(hide_password=True)`.
4. `make_url_safe` in `superset/databases/utils.py:113-130` wraps
SQLAlchemy's `make_url`
and, on any parsing failure, catches `Exception` and raises
`DatabaseInvalidError()` (a
Superset exception), not `sqlalchemy.exc.ArgumentError`. Since
`collect_connection_info`
only catches `ArgumentError`, the `DatabaseInvalidError` propagates
uncaught, causing the
`superset test-db` CLI to terminate with an unhandled exception instead of
printing
`"<invalid database URI>"` and continuing.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/cli/test_db.py
**Line:** 188:188
**Comment:**
*Logic Error: `make_url_safe` does not raise
`sqlalchemy.exc.ArgumentError` (it wraps URL parsing failures in
`DatabaseInvalidError`), so malformed SQLAlchemy URIs will cause an unhandled
exception here instead of being gracefully masked as "<invalid database URI>".
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%2F38229&comment_hash=17fc81637d7569be9817b2256e516a16c50910b56a63c44103b793f504f068cf&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38229&comment_hash=17fc81637d7569be9817b2256e516a16c50910b56a63c44103b793f504f068cf&reaction=dislike'>👎</a>
##########
tests/unit_tests/commands/databases/test_connection_test.py:
##########
@@ -89,3 +89,34 @@ def test_command_with_oauth2(mocker: MockerFixture) -> None:
level=ErrorLevel.WARNING,
extra={"url": "url", "tab_id": "tab_id", "redirect_uri":
"redirect_uri"},
)
+
+
+def test_command_with_masking(mocker: MockerFixture) -> None:
+ """
+ Test that the command masks the sqlalchemy_uri in metadata.
+ """
+ database = mocker.MagicMock()
+ # Use a URI with a password that should be masked
+ database.sqlalchemy_uri = "postgresql://user:secret_password@localhost/db"
+ database.db_engine_spec.__name__ = "PostgresEngineSpec"
+
+ # Mock ping to raise a Timeout exception
+ mocker.patch(
+ "superset.commands.database.test_connection.ping",
+ side_effect=SupersetTimeoutException("Timeout"),
+ )
+
+ DatabaseDAO = mocker.patch( # noqa: N806
+ "superset.commands.database.test_connection.DatabaseDAO"
+ )
+ DatabaseDAO.build_db_for_connection_test.return_value = database
+
+ command = TestConnectionDatabaseCommand({"sqlalchemy_uri":
database.sqlalchemy_uri})
+
+ with pytest.raises(SupersetTimeoutException) as excinfo:
+ command.run()
+
+ # Verify that the password is NOT in the extra metadata
+ assert "secret_password" not in excinfo.value.extra["sqlalchemy_uri"]
+ assert "***" in excinfo.value.extra["sqlalchemy_uri"]
+ assert "localhost" in excinfo.value.extra["sqlalchemy_uri"]
Review Comment:
**Suggestion:** The test assumes the timeout exception exposes an `extra`
attribute directly on the exception instance, but `SupersetTimeoutException`
(via `SupersetErrorFromParamsException`) stores metadata on the nested `error`
object, so accessing `excinfo.value.extra` will raise an AttributeError and the
test will fail even though the production code is correct. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ `test_command_with_masking` fails accessing nonexistent exception.extra
attribute.
- ⚠️ Timeout masking behavior untested, possible regressions in future
changes.
```
</details>
```suggestion
sqlalchemy_uri = excinfo.value.error.extra["sqlalchemy_uri"]
assert "secret_password" not in sqlalchemy_uri
assert "***" in sqlalchemy_uri
assert "localhost" in sqlalchemy_uri
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the unit test `test_command_with_masking` in
`tests/unit_tests/commands/databases/test_connection_test.py` (lines 94–122)
via `pytest
tests/unit_tests/commands/databases/test_connection_test.py::test_command_with_masking`.
2. Inside the test, `ping` is patched to raise
`SupersetTimeoutException("Timeout")`
(lines 103–107), and `TestConnectionDatabaseCommand.run()` is invoked, which
catches the
timeout and re-raises a new `SupersetTimeoutException` with
`extra={"sqlalchemy_uri":
safe_uri}` in `superset/commands/database/test_connection.py` (except block
around lines
120–135).
3. Pytest captures this re-raised `SupersetTimeoutException` in `excinfo` in
the `with
pytest.raises(SupersetTimeoutException) as excinfo:` block at lines 116–117
of
`tests/unit_tests/commands/databases/test_connection_test.py`.
4. The assertions at lines 119–122 access
`excinfo.value.extra["sqlalchemy_uri"]`;
however, `SupersetTimeoutException` (defined in
`superset/exceptions.py:129`) inherits
from `SupersetErrorFromParamsException`, which stores metadata in the nested
`error:
SupersetError` object (`superset/errors.py:212–247`), so the exception
instance has
`error.extra` but no top-level `extra` attribute, causing `AttributeError:
'SupersetTimeoutException' object has no attribute 'extra'` and the test
fails even though
the production masking logic is correct.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/commands/databases/test_connection_test.py
**Line:** 120:122
**Comment:**
*Logic Error: The test assumes the timeout exception exposes an `extra`
attribute directly on the exception instance, but `SupersetTimeoutException`
(via `SupersetErrorFromParamsException`) stores metadata on the nested `error`
object, so accessing `excinfo.value.extra` will raise an AttributeError and the
test will fail even though the production code is correct.
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%2F38229&comment_hash=60afcae3621d4ac02fa58ae77312e3ff74ecf144a2bd3aa8213dd30a26253369&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38229&comment_hash=60afcae3621d4ac02fa58ae77312e3ff74ecf144a2bd3aa8213dd30a26253369&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]
