codeant-ai-for-open-source[bot] commented on code in PR #37067:
URL: https://github.com/apache/superset/pull/37067#discussion_r2748310534
##########
tests/unit_tests/databases/api_test.py:
##########
@@ -709,7 +724,11 @@ def test_oauth2_permissions(
)
assert response.status_code == 200
- get_oauth2_token.assert_called_with({"id": "one", "secret": "two"}, "XXX")
+ get_oauth2_token.assert_called_with(
+ {"id": "one", "secret": "two"},
+ "XXX",
+ code_verifier=None,
Review Comment:
**Suggestion:** Similar to the happy-path test, this permissions test now
enforces that `get_oauth2_token` must always receive a `code_verifier` keyword
argument (even when PKCE is not used), which can break third-party engine specs
that still implement the older signature and undermines the stated
backward-compatibility guarantee; the test should only assert the positional
arguments for the non-PKCE case. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Unit tests fail in CI for engines with old signatures.
- ⚠️ Tests incorrectly force API contract change.
- ⚠️ May block PRs until tests/engine specs sync.
```
</details>
```suggestion
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the unit test test_oauth2_permissions in
tests/unit_tests/databases/api_test.py
(file lines ~665-739). The test patches SqliteEngineSpec.get_oauth2_token at
line 697 and
then triggers the OAuth2 redirect handling via the client call at lines
717-724.
2. If the engine spec implementation under test still uses the older
signature (two
positional args), the actual call will be recorded by the Mock as a
positional 2-arg call.
3. The test reaches the assertion at
./tests/unit_tests/databases/api_test.py:727-731
which enforces that the call included the keyword code_verifier=None.
Because Mock
recorded only positional arguments, Mock.assert_called_with(...,
code_verifier=None) fails
with an AssertionError describing argument mismatch.
4. Reproduce by running pytest -q
tests/unit_tests/databases/api_test.py::test_oauth2_permissions against an
environment
where engine specs have not been updated to accept a code_verifier kwarg —
the assertion
at lines 727-731 will fail even though runtime behavior (no PKCE) is
correct. Changing the
assertion to the improved_code (only positional args) verifies the existing
backward-compatible behavior.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/databases/api_test.py
**Line:** 730:730
**Comment:**
*Possible Bug: Similar to the happy-path test, this permissions test
now enforces that `get_oauth2_token` must always receive a `code_verifier`
keyword argument (even when PKCE is not used), which can break third-party
engine specs that still implement the older signature and undermines the stated
backward-compatibility guarantee; the test should only assert the positional
arguments for the non-PKCE case.
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>
##########
tests/unit_tests/databases/api_test.py:
##########
@@ -641,7 +648,11 @@ def test_oauth2_happy_path(
)
assert response.status_code == 200
- get_oauth2_token.assert_called_with({"id": "one", "secret": "two"}, "XXX")
+ get_oauth2_token.assert_called_with(
+ {"id": "one", "secret": "two"},
+ "XXX",
+ code_verifier=None,
Review Comment:
**Suggestion:** The new assertion forces `get_oauth2_token` to always be
called with a `code_verifier` keyword argument (even when it is `None`), which
contradicts the PR's claim of backward compatibility and can cause `TypeError:
got an unexpected keyword argument 'code_verifier'` for engine specs that
haven't been updated to accept this parameter; instead, the non-PKCE test
should assert the existing positional arguments only. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Unit tests fail in CI for engines with old signatures.
- ⚠️ Breaks backward-compatibility guarantee in tests.
- ⚠️ Prevents merges until tests adjusted.
```
</details>
```suggestion
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the unit test test_oauth2_happy_path in
tests/unit_tests/databases/api_test.py
(file lines ~590-663). The test patches SqliteEngineSpec.get_oauth2_token at
line 621 and
then exercises the API handler by calling the Flask test client at lines
642-648.
2. The production path under test calls the engine spec's get_oauth2_token
implementation.
If that implementation (third-party or older engine spec) calls/accepts only
positional
args (e.g., def get_oauth2_token(cfg, code): ...) the recorded call on the
patched method
will be a positional call with two arguments.
3. After exercising the endpoint, the test reaches the assertion at file
./tests/unit_tests/databases/api_test.py:651-655 which currently requires
the recorded
call to include the keyword argument code_verifier=None. Because the real
call was
recorded as two positional arguments, Mock.assert_called_with(...,
code_verifier=None)
will fail with an AssertionError (arg mismatch) even though the flow was
otherwise correct
and backward-compatible.
4. Reproducing locally: run pytest -q
tests/unit_tests/databases/api_test.py::test_oauth2_happy_path against a
codebase where an
engine spec's get_oauth2_token signature is the old positional-only form —
the test will
fail at the assert_called_with at lines 651-655 with a mismatch describing
positional vs
keyword args. The fix in improved_code removes the enforced keyword and
matches existing
two-arg call pattern, keeping backward compatibility.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/databases/api_test.py
**Line:** 654:654
**Comment:**
*Possible Bug: The new assertion forces `get_oauth2_token` to always be
called with a `code_verifier` keyword argument (even when it is `None`), which
contradicts the PR's claim of backward compatibility and can cause `TypeError:
got an unexpected keyword argument 'code_verifier'` for engine specs that
haven't been updated to accept this parameter; instead, the non-PKCE test
should assert the existing positional arguments only.
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>
--
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]
