codeant-ai-for-open-source[bot] commented on code in PR #36856:
URL: https://github.com/apache/superset/pull/36856#discussion_r3573281804
##########
tests/unit_tests/sql_lab_test.py:
##########
@@ -198,74 +198,82 @@ def test_get_sql_results_oauth2(mocker: MockerFixture,
app) -> None:
"""
Test that `get_sql_results` works with OAuth2.
"""
+ # Pushed/popped manually (rather than via a ``with`` block) so the
+ # ``finally`` below still pops it if an assertion fails, preventing the
+ # request context from leaking into later tests in the same session.
app_context = app.test_request_context()
app_context.push()
- mocker.patch(
- "superset.db_engine_specs.base.uuid4",
- return_value=UUID("fb11f528-6eba-4a8a-837e-6b0d39ee9187"),
- )
- mocker.patch(
- "superset.db_engine_specs.base.generate_code_verifier",
-
return_value="xkBPVZoFChVcy3VZ2l5u7d0FZPTU-olO7HtsAOok2IUGigyoZ62tG_oldy2xg9_HdqPKrWUmKZLmU-CUqz_SQ",
- )
- mocker.patch("superset.daos.key_value.KeyValueDAO.delete_expired_entries")
- mocker.patch("superset.daos.key_value.KeyValueDAO.create_entry")
- mocker.patch("superset.db_engine_specs.base.db.session.commit")
-
- g = mocker.patch("superset.db_engine_specs.base.g")
- g.user = mocker.MagicMock()
- g.user.id = 42
-
- database = Database(
- id=1,
- database_name="my_db",
- sqlalchemy_uri="sqlite://",
- encrypted_extra=json.dumps(oauth2_client_info),
- )
- database.db_engine_spec.oauth2_exception = OAuth2Error
- get_sqla_engine = mocker.patch.object(database, "get_sqla_engine")
- get_sqla_engine().__enter__().raw_connection.side_effect = OAuth2Error(
- "OAuth2 required"
- )
+ try:
+ mocker.patch(
+ "superset.db_engine_specs.base.uuid4",
+ return_value=UUID("fb11f528-6eba-4a8a-837e-6b0d39ee9187"),
+ )
+ mocker.patch(
+ "superset.db_engine_specs.base.generate_code_verifier",
+
return_value="xkBPVZoFChVcy3VZ2l5u7d0FZPTU-olO7HtsAOok2IUGigyoZ62tG_oldy2xg9_HdqPKrWUmKZLmU-CUqz_SQ",
+ )
+
mocker.patch("superset.daos.key_value.KeyValueDAO.delete_expired_entries")
+ mocker.patch("superset.daos.key_value.KeyValueDAO.create_entry")
+ mocker.patch("superset.db_engine_specs.base.db.session.commit")
+
+ g = mocker.patch("superset.db_engine_specs.base.g")
+ g.user = mocker.MagicMock()
+ g.user.id = 42
+
+ database = Database(
+ id=1,
+ database_name="my_db",
+ sqlalchemy_uri="sqlite://",
+ encrypted_extra=json.dumps(oauth2_client_info),
+ )
+ database.db_engine_spec.oauth2_exception = OAuth2Error
+ get_sqla_engine = mocker.patch.object(database, "get_sqla_engine")
+ get_sqla_engine().__enter__().raw_connection.side_effect = OAuth2Error(
+ "OAuth2 required"
+ )
- query = mocker.MagicMock(select_as_cta=False, database=database)
- mocker.patch("superset.sql_lab.get_query", return_value=query)
+ query = mocker.MagicMock(select_as_cta=False, database=database)
+ mocker.patch("superset.sql_lab.get_query", return_value=query)
+
+ payload = get_sql_results(query_id=1, rendered_query="SELECT 1")
Review Comment:
**Suggestion:** Add a concrete type annotation for this response payload
variable so the modified test code remains fully type-hinted. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This newly added local variable is unannotated in modified Python code, so
it matches the rule requiring type hints for relevant variables that 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=31f1485692274b77bf9b8d39ed07362e&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=31f1485692274b77bf9b8d39ed07362e&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/sql_lab_test.py
**Line:** 239:239
**Comment:**
*Custom Rule: Add a concrete type annotation for this response payload
variable so the modified test code remains fully 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%2F36856&comment_hash=7ea03a83840d78df3db1dee9cd07bffd4243c1505a3f503b6a6d6a3c3e8597ba&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F36856&comment_hash=7ea03a83840d78df3db1dee9cd07bffd4243c1505a3f503b6a6d6a3c3e8597ba&reaction=dislike'>๐</a>
##########
tests/unit_tests/sql_lab_test.py:
##########
@@ -198,74 +198,82 @@ def test_get_sql_results_oauth2(mocker: MockerFixture,
app) -> None:
"""
Test that `get_sql_results` works with OAuth2.
"""
+ # Pushed/popped manually (rather than via a ``with`` block) so the
+ # ``finally`` below still pops it if an assertion fails, preventing the
+ # request context from leaking into later tests in the same session.
app_context = app.test_request_context()
app_context.push()
- mocker.patch(
- "superset.db_engine_specs.base.uuid4",
- return_value=UUID("fb11f528-6eba-4a8a-837e-6b0d39ee9187"),
- )
- mocker.patch(
- "superset.db_engine_specs.base.generate_code_verifier",
-
return_value="xkBPVZoFChVcy3VZ2l5u7d0FZPTU-olO7HtsAOok2IUGigyoZ62tG_oldy2xg9_HdqPKrWUmKZLmU-CUqz_SQ",
- )
- mocker.patch("superset.daos.key_value.KeyValueDAO.delete_expired_entries")
- mocker.patch("superset.daos.key_value.KeyValueDAO.create_entry")
- mocker.patch("superset.db_engine_specs.base.db.session.commit")
-
- g = mocker.patch("superset.db_engine_specs.base.g")
- g.user = mocker.MagicMock()
- g.user.id = 42
-
- database = Database(
- id=1,
- database_name="my_db",
- sqlalchemy_uri="sqlite://",
- encrypted_extra=json.dumps(oauth2_client_info),
- )
- database.db_engine_spec.oauth2_exception = OAuth2Error
- get_sqla_engine = mocker.patch.object(database, "get_sqla_engine")
- get_sqla_engine().__enter__().raw_connection.side_effect = OAuth2Error(
- "OAuth2 required"
- )
+ try:
+ mocker.patch(
+ "superset.db_engine_specs.base.uuid4",
+ return_value=UUID("fb11f528-6eba-4a8a-837e-6b0d39ee9187"),
+ )
+ mocker.patch(
+ "superset.db_engine_specs.base.generate_code_verifier",
+
return_value="xkBPVZoFChVcy3VZ2l5u7d0FZPTU-olO7HtsAOok2IUGigyoZ62tG_oldy2xg9_HdqPKrWUmKZLmU-CUqz_SQ",
+ )
+
mocker.patch("superset.daos.key_value.KeyValueDAO.delete_expired_entries")
+ mocker.patch("superset.daos.key_value.KeyValueDAO.create_entry")
+ mocker.patch("superset.db_engine_specs.base.db.session.commit")
+
+ g = mocker.patch("superset.db_engine_specs.base.g")
+ g.user = mocker.MagicMock()
+ g.user.id = 42
+
+ database = Database(
+ id=1,
+ database_name="my_db",
+ sqlalchemy_uri="sqlite://",
+ encrypted_extra=json.dumps(oauth2_client_info),
+ )
+ database.db_engine_spec.oauth2_exception = OAuth2Error
+ get_sqla_engine = mocker.patch.object(database, "get_sqla_engine")
+ get_sqla_engine().__enter__().raw_connection.side_effect = OAuth2Error(
+ "OAuth2 required"
+ )
- query = mocker.MagicMock(select_as_cta=False, database=database)
- mocker.patch("superset.sql_lab.get_query", return_value=query)
+ query = mocker.MagicMock(select_as_cta=False, database=database)
+ mocker.patch("superset.sql_lab.get_query", return_value=query)
+
+ payload = get_sql_results(query_id=1, rendered_query="SELECT 1")
+ assert payload["status"] == QueryStatus.FAILED
+ assert payload["error"] == "You don't have permission to access the
data."
+ assert len(payload["errors"]) == 1
+
+ error = payload["errors"][0]
+ assert error["message"] == "You don't have permission to access the
data."
+ assert error["error_type"] == SupersetErrorType.OAUTH2_REDIRECT
+ assert error["level"] == ErrorLevel.WARNING
+ assert error["extra"]["tab_id"] ==
"fb11f528-6eba-4a8a-837e-6b0d39ee9187"
+ assert (
+ error["extra"]["redirect_uri"] ==
"http://localhost/api/v1/database/oauth2/";
+ )
- payload = get_sql_results(query_id=1, rendered_query="SELECT 1")
- assert payload["status"] == QueryStatus.FAILED
- assert payload["error"] == "You don't have permission to access the data."
- assert len(payload["errors"]) == 1
-
- error = payload["errors"][0]
- assert error["message"] == "You don't have permission to access the data."
- assert error["error_type"] == SupersetErrorType.OAUTH2_REDIRECT
- assert error["level"] == ErrorLevel.WARNING
- assert error["extra"]["tab_id"] == "fb11f528-6eba-4a8a-837e-6b0d39ee9187"
- assert error["extra"]["redirect_uri"] ==
"http://localhost/api/v1/database/oauth2/";
-
- # Parse the OAuth2 authorization URL and verify components individually,
- # since the JWT state and PKCE code_challenge are computed
deterministically
- # from mocked inputs but their exact encoding depends on library internals.
- url = urlparse(error["extra"]["url"])
- assert url.scheme == "https"
- assert url.netloc == "abcd1234.snowflakecomputing.com"
- assert url.path == "/oauth/authorize"
-
- params = parse_qs(url.query)
- assert params["scope"] == ["refresh_token session:role:USERADMIN"]
- assert params["response_type"] == ["code"]
- assert params["redirect_uri"] ==
["http://localhost/api/v1/database/oauth2/";]
- assert params["client_id"] == ["my_client_id"]
- assert params["code_challenge_method"] == ["S256"]
-
- # Verify PKCE code_challenge matches the mocked code_verifier
- from superset.utils.oauth2 import generate_code_challenge
-
- expected_code_challenge = generate_code_challenge(
-
"xkBPVZoFChVcy3VZ2l5u7d0FZPTU-olO7HtsAOok2IUGigyoZ62tG_oldy2xg9_HdqPKrWUmKZLmU-CUqz_SQ"
- )
- assert params["code_challenge"] == [expected_code_challenge]
+ # Parse the OAuth2 authorization URL and verify components
individually,
+ # since the JWT state and PKCE code_challenge are computed
deterministically
+ # from mocked inputs but their exact encoding depends on library
internals.
+ url = urlparse(error["extra"]["url"])
+ assert url.scheme == "https"
+ assert url.netloc == "abcd1234.snowflakecomputing.com"
+ assert url.path == "/oauth/authorize"
+
+ params = parse_qs(url.query)
Review Comment:
**Suggestion:** Add an explicit type annotation for this parsed
query-parameter mapping to satisfy the type-hint requirement for relevant
variables. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This new local variable is introduced without a type hint, and it is a
relevant variable that could be annotated under 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=8f6b1fb9c67f4d2a8164bcf5395736e0&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=8f6b1fb9c67f4d2a8164bcf5395736e0&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/sql_lab_test.py
**Line:** 261:261
**Comment:**
*Custom Rule: Add an explicit type annotation for this parsed
query-parameter mapping to satisfy the type-hint requirement for relevant
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%2F36856&comment_hash=1176bced6062cbb3bcd78aadd4816213fe43c39c69110a1bf6fafed7b9a05564&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F36856&comment_hash=1176bced6062cbb3bcd78aadd4816213fe43c39c69110a1bf6fafed7b9a05564&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]
