Fixtures: Adding API key model #294
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis change standardizes the naming of API key-related fixtures, parameters, and variables across multiple test files, renaming Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Function
participant Fixture as Pytest Fixture
participant DB as Database
participant Client as HTTP Client
Test->>Fixture: Request user_api_key_header
Fixture->>DB: Retrieve API key for user
Fixture-->>Test: Return {"X-API-KEY": api_key.key}
Test->>Client: Make HTTP request with user_api_key_header
Client->>DB: Validate API key
DB-->>Client: Respond
Client-->>Test: Return API response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
backend/app/tests/utils/utils.py (1)
49-53: Fix the return type annotation to match the actual return value.The function now returns the full
api_keyobject instead of just the key string, but the return type annotation still indicatesstr. This creates a mismatch between the annotation and actual behavior.Update the return type annotation to reflect the actual return value:
-def get_api_key_by_email(db: Session, email: EmailStr) -> str: +def get_api_key_by_email(db: Session, email: EmailStr) -> APIKeyPublic:Also add the missing import at the top of the file if not already present:
from app.models import APIKeyPublic, Project, Assistant
🧹 Nitpick comments (2)
backend/app/tests/api/routes/test_assistants.py (1)
117-147: Consider using the fixture for consistency.While the manual API key header construction works correctly, consider using the
user_api_key_headerfixture for consistency with other tests in this file. This would make the test setup more uniform.You could refactor this test to use the fixture:
def test_update_assistant_success( client: TestClient, db: Session, + user_api_key_header: dict[str, str], ):Then replace the manual header construction with the fixture parameter. However, this would also require ensuring the fixture uses the same project/user context as the assistant being tested.
backend/app/tests/api/routes/test_responses.py (1)
68-68: Add type annotation for consistency.The parameter renaming is correct, but consider adding the type annotation for consistency with other test functions.
- user_api_key_header, + user_api_key_header: dict[str, str],Also applies to: 108-108
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/app/tests/api/routes/collections/test_collection_info.py(3 hunks)backend/app/tests/api/routes/collections/test_create_collections.py(2 hunks)backend/app/tests/api/routes/documents/test_route_document_upload.py(2 hunks)backend/app/tests/api/routes/test_assistants.py(11 hunks)backend/app/tests/api/routes/test_responses.py(4 hunks)backend/app/tests/conftest.py(1 hunks)backend/app/tests/utils/document.py(2 hunks)backend/app/tests/utils/utils.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
backend/app/tests/utils/utils.py (2)
backend/app/tests/crud/test_api_key.py (4)
test_get_api_key_by_user_id(103-117)test_get_api_key_by_value(52-66)test_get_api_key(25-32)test_get_api_key_by_project_user(69-81)backend/app/tests/utils/user.py (1)
authentication_token_from_email(30-49)
backend/app/tests/api/routes/test_responses.py (1)
backend/app/tests/conftest.py (3)
db(20-37)user_api_key_header(73-75)client(48-51)
backend/app/tests/api/routes/documents/test_route_document_upload.py (1)
backend/app/tests/conftest.py (2)
user_api_key_header(73-75)client(48-51)
backend/app/tests/utils/document.py (1)
backend/app/tests/conftest.py (2)
user_api_key_header(73-75)client(48-51)
🔇 Additional comments (17)
backend/app/tests/api/routes/collections/test_create_collections.py (1)
49-63: LGTM! Consistent parameter renaming.The parameter and variable renaming from
normal_user_api_key_headerstouser_api_key_headeris consistent and aligns with the broader standardization effort across the test suite.backend/app/tests/api/routes/test_assistants.py (7)
29-43: LGTM! Consistent parameter renaming.The parameter renaming from
normal_user_api_key_headerstouser_api_key_headeris consistent and correctly applied in the test function signature and usage.
51-66: LGTM! Consistent parameter renaming.The parameter and variable updates are correctly applied and maintain the test logic.
90-110: LGTM! Consistent parameter renaming.The parameter updates are correctly applied in both the function signature and the HTTP request headers.
176-189: LGTM! Consistent parameter renaming.The parameter and usage updates are correctly applied.
218-228: LGTM! Consistent parameter renaming.The parameter and usage updates are correctly applied.
259-283: LGTM! Consistent parameter renaming.The parameter updates are correctly applied across all the pagination test scenarios.
305-315: LGTM! Consistent parameter renaming.The parameter and usage updates are correctly applied.
backend/app/tests/utils/document.py (2)
113-128: LGTM! Consistent attribute and usage renaming.The dataclass attribute renaming from
normal_user_api_key_headerstouser_api_key_headeris correctly applied, including the type annotation and all method usages.
160-162: LGTM! Consistent fixture parameter renaming.The fixture parameter renaming is consistent with the dataclass changes and properly passes the parameter to the WebCrawler constructor.
backend/app/tests/api/routes/documents/test_route_document_upload.py (2)
22-32: LGTM! Consistent attribute usage update.The method correctly uses the renamed
user_api_key_headerattribute, maintaining consistency with the parentWebCrawlerclass changes.
47-49: LGTM! Consistent fixture parameter renaming.The fixture parameter renaming is consistent with the broader standardization effort and correctly passes the parameter to the WebUploader constructor.
backend/app/tests/api/routes/test_responses.py (1)
19-19: LGTM! Parameter renaming is consistent with fixture changes.The renaming from
normal_user_api_key_headerstouser_api_key_headeraligns with the fixture standardization inconftest.pyand maintains consistency throughout the function.Also applies to: 51-51
backend/app/tests/api/routes/collections/test_collection_info.py (1)
40-42: LGTM! Consistent parameter renaming across all test functions.The renaming from
normal_user_api_key_headerstouser_api_key_headeris applied consistently across all three test functions and aligns with the fixture standardization.Also applies to: 61-63, 83-84
backend/app/tests/conftest.py (3)
67-69: LGTM! Fixture standardization improves test clarity.The renaming to singular form and returning a header dictionary instead of the raw API key object makes the fixture usage more explicit and consistent across tests.
73-75: LGTM! Consistent with superuser fixture changes.The renaming and return value change follow the same standardization pattern as the superuser fixture.
69-69: get_api_key_by_email returns an object with.key, fixtures are correct
Theget_api_key_by_user_idfunction returns anAPIKeyPublicmodel (with a decryptedkeyfield), soget_api_key_by_emailindeed returns an object that has a.keyattribute. No changes required.
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
2 participants
Summary
Target issue is #295
Explain the motivation for making this change. What existing problem does the pull request solve?
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Please add here if any other information is required for the reviewer.
Summary by CodeRabbit
Summary by CodeRabbit