Conversation
## Walkthrough
The changes introduce fixtures for API key authentication headers in test configurations, refactor tests to use these fixtures instead of hardcoded keys, and enhance seed data logic to use environment variables for user emails and API keys. Additional utility and CRUD functions are added to support these improvements.
## Changes
| File(s) | Change Summary |
|----------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------|
| backend/app/tests/api/routes/collections/test_collection_info.py,<br>backend/app/tests/api/routes/collections/test_create_collections.py,<br>backend/app/tests/api/routes/test_responses.py | Refactored tests to use the `normal_user_api_key_headers` fixture for API authentication headers instead of hardcoded keys; updated function signatures accordingly. |
| backend/app/tests/conftest.py | Added fixtures `superuser_api_key_headers` and `normal_user_api_key_headers` to provide API key headers for tests. |
| backend/app/tests/utils/utils.py | Added utility function `get_api_key_by_email` to retrieve an API key string by user email; updated `get_user_id_by_email` to use test user email. |
| backend/app/crud/__init__.py,<br>backend/app/crud/api_key.py | Exported and implemented `get_api_key_by_user_id` to fetch API keys by user ID. |
| backend/app/seed_data/seed_data.json | Replaced hardcoded user emails and API key user emails with template placeholders. |
| backend/app/seed_data/seed_data.py | Enhanced seeding logic to substitute placeholders with environment-configured emails for users and API keys. |
| backend/app/tests/api/routes/test_assistants.py | Removed old fixture returning API key header string; updated test to use new fixture returning header dict. |
| backend/app/tests/api/routes/documents/test_route_document_upload.py | Changed document upload tests to use `normal_user_api_key_headers` instead of superuser token headers. |
| backend/app/tests/utils/document.py | Updated `WebCrawler` dataclass and fixture to use `normal_user_api_key_headers` instead of superuser headers. |
| backend/app/tests/crud/test_api_key.py | Added test for `get_api_key_by_user_id` CRUD function. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Test as Test Function
participant Fixture as normal_user_api_key_headers Fixture
participant Utils as get_api_key_by_email
participant DB as Database
Test->>Fixture: Request API key headers
Fixture->>Utils: Call get_api_key_by_email(db, email)
Utils->>DB: Query user by email
DB-->>Utils: Return user object
Utils->>DB: Query API key by user_id
DB-->>Utils: Return API key
Utils-->>Fixture: Return API key string
Fixture-->>Test: Return headers {"X-API-KEY": api_key}
Test->>API: Make authenticated request with headersPossibly related PRs
Suggested reviewers
Poem
|
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
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/test_responses.py(4 hunks)backend/app/tests/conftest.py(3 hunks)backend/app/tests/utils/api_keys.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
backend/app/tests/api/routes/collections/test_create_collections.py (1)
backend/app/tests/conftest.py (1)
superuser_api_key_headers(67-72)
backend/app/tests/conftest.py (1)
backend/app/tests/utils/api_keys.py (1)
get_api_key_by_user_email(21-30)
backend/app/tests/api/routes/test_responses.py (1)
backend/app/tests/conftest.py (3)
db(20-37)superuser_api_key_headers(67-72)client(48-51)
backend/app/tests/api/routes/collections/test_collection_info.py (1)
backend/app/tests/conftest.py (3)
db(20-37)client(48-51)superuser_api_key_headers(67-72)
🪛 GitHub Check: codecov/patch
backend/app/tests/utils/api_keys.py
[warning] 30-30: backend/app/tests/utils/api_keys.py#L30
Added line #L30 was not covered by tests
backend/app/tests/conftest.py
[warning] 80-81: backend/app/tests/conftest.py#L80-L81
Added lines #L80 - L81 were not covered by tests
🪛 Ruff (0.12.2)
backend/app/tests/utils/api_keys.py
3-3: typing.Dict is deprecated, use dict instead
(UP035)
4-4: pytest imported but unused
Remove unused import: pytest
(F401)
5-5: fastapi.testclient.TestClient imported but unused
Remove unused import: fastapi.testclient.TestClient
(F401)
6-6: sqlalchemy.orm.Session imported but unused
Remove unused import: sqlalchemy.orm.Session
(F401)
13-13: Use dict instead of Dict for type annotation
Replace with dict
(UP006)
🔇 Additional comments (12)
backend/app/tests/conftest.py (3)
15-15: LGTM: Import addition.The import of
get_api_key_by_user_emailis correctly placed and follows the existing import organization.
66-72: LGTM: Well-structured fixture.The
superuser_api_key_headersfixture follows pytest conventions and provides a clean abstraction for API key headers.
75-81: LGTM: Consistent fixture pattern.The
normal_user_api_key_headersfixture follows the same pattern as the superuser fixture, ensuring consistency.backend/app/tests/api/routes/collections/test_create_collections.py (2)
49-51: LGTM: Clean fixture integration.The addition of the
superuser_api_key_headersfixture parameter correctly replaces the hardcoded API key approach, improving maintainability.
63-66: LGTM: Simplified header usage.The direct assignment of
superuser_api_key_headersto theheadersvariable is clean and removes the need for manual header construction.backend/app/tests/api/routes/test_responses.py (4)
18-20: LGTM: Clean fixture integration.The addition of
superuser_api_key_headersparameter with proper type annotation follows pytest conventions and improves test maintainability.
51-53: LGTM: Consistent fixture usage.The direct use of
superuser_api_key_headersin the POST request is clean and consistent with the refactoring approach.
64-66: LGTM: Consistent pattern.The same fixture integration pattern is correctly applied to the second test function.
105-107: LGTM: Maintains consistency.The fixture usage in the second test maintains consistency with the first test and the overall refactoring approach.
backend/app/tests/api/routes/collections/test_collection_info.py (3)
39-42: LGTM: Clean fixture integration.The addition of
superuser_api_key_headersparameter and its direct assignment toheadersfollows the established pattern and improves test maintainability.
60-63: LGTM: Consistent pattern.The same fixture integration pattern is correctly applied to the second test function.
83-86: LGTM: Maintains consistency.The fixture usage in the third test function maintains consistency with the other functions and the overall refactoring approach.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
backend/app/tests/utils/utils.py (2)
10-10: Update deprecated import.The
typing.Typeimport is deprecated. Usetypeinstead for Python 3.9+.-from typing import Type, TypeVar +from typing import TypeVar
14-14: Remove unused imports.The
UserandAPIKeymodels are imported but not used in this file. Clean up these unused imports.-from app.models import APIKeyPublic, User, APIKey +from app.models import APIKeyPublicbackend/app/crud/api_key.py (1)
167-182: Fix boolean comparison style.The function logic is correct, but use the more pythonic boolean comparison style.
api_key = ( session.query(APIKey) - .filter(APIKey.user_id == user_id, APIKey.is_deleted == False) + .filter(APIKey.user_id == user_id, APIKey.is_deleted.is_(False)) .first() )Or even better, use the
notoperator:api_key = ( session.query(APIKey) - .filter(APIKey.user_id == user_id, APIKey.is_deleted == False) + .filter(APIKey.user_id == user_id, not APIKey.is_deleted) .first() )backend/app/tests/conftest.py (2)
4-4: Remove unused import.The
timemodule is imported but never used in the code.-import time
17-17: Remove commented import.This commented import line appears to be leftover from development and should be cleaned up.
-# from app.tests.utils.api_keys import get_api_key_by_user_email
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/app/crud/__init__.py(1 hunks)backend/app/crud/api_key.py(1 hunks)backend/app/seed_data/seed_data.json(1 hunks)backend/app/seed_data/seed_data.py(3 hunks)backend/app/tests/conftest.py(3 hunks)backend/app/tests/utils/utils.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
backend/app/crud/api_key.py (1)
backend/app/models/api_key.py (2)
APIKeyPublic(23-25)APIKey(28-38)
backend/app/crud/__init__.py (1)
backend/app/crud/api_key.py (1)
get_api_key_by_user_id(167-182)
backend/app/tests/conftest.py (1)
backend/app/tests/utils/utils.py (1)
get_api_key_by_email(50-54)
🪛 Ruff (0.12.2)
backend/app/tests/utils/utils.py
10-10: typing.Type is deprecated, use type instead
(UP035)
14-14: app.models.User imported but unused
Remove unused import
(F401)
14-14: app.models.APIKey imported but unused
Remove unused import
(F401)
backend/app/crud/api_key.py
173-173: Avoid equality comparisons to False; use not APIKey.is_deleted: for false checks
Replace with not APIKey.is_deleted
(E712)
backend/app/crud/__init__.py
32-32: .api_key.get_api_key_by_user_id imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
backend/app/tests/conftest.py
4-4: time imported but unused
Remove unused import: time
(F401)
🔇 Additional comments (5)
backend/app/crud/__init__.py (1)
32-32: Import Usage Confirmed: keepget_api_key_by_user_idin__init__.pyThe
get_api_key_by_user_idexport is indeed used in backend/app/tests/utils/utils.py (inside theget_api_key_by_emailhelper), so the import should remain.• Referenced in:
- backend/app/tests/utils/utils.py:
from app.crud import get_user_by_email, get_api_key_by_user_id ... api_key = get_api_key_by_user_id(db, user_id=user.id)No changes required.
backend/app/seed_data/seed_data.json (1)
39-39: LGTM! Good use of template placeholders.The replacement of hardcoded emails with template placeholders (
{{SUPERUSER_EMAIL}}and{{ADMIN_EMAIL}}) is a good approach that makes the seed data more flexible and environment-specific.Also applies to: 47-47
backend/app/seed_data/seed_data.py (2)
315-329: LGTM! Well-implemented dynamic email assignment.The logic properly handles different user roles and provides good error handling for edge cases. The warning message for missing emails is helpful for debugging.
343-347: LGTM! Clean template placeholder replacement.The placeholder replacement logic is clean and happens at the right time before API key processing. This integrates well with the template placeholders in the JSON file.
backend/app/tests/conftest.py (1)
71-71: Seed data leverages .env variables – no changes needed
- In backend/app/seed_data/seed_data.py you’re already mapping
user_data["email"] = settings.FIRST_SUPERUSER/settings.EMAIL_TEST_USERapi_key_data["user_email"] = settings.FIRST_SUPERUSER/settings.EMAIL_TEST_USER- There are no hard-coded emails left in seed_data.json, so tests using
get_api_key_by_email(db, settings.FIRST_SUPERUSER)will resolve correctly
as long as your.envdefines those values.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/app/tests/utils/utils.py (1)
50-54: Add error handling for null cases.The function should handle cases where the user or API key might not exist to prevent AttributeError exceptions.
Apply this diff to add proper error handling:
def get_api_key_by_email(db: Session, email: EmailStr) -> str: user = get_user_by_email(session=db, email=email) + if not user: + raise ValueError(f"User with email {email} not found") + api_key = get_api_key_by_user_id(db, user_id=user.id) + if not api_key: + raise ValueError(f"No API key found for user {email}") return api_key.key
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/app/crud/__init__.py(1 hunks)backend/app/seed_data/seed_data.json(1 hunks)backend/app/seed_data/seed_data.py(3 hunks)backend/app/tests/api/routes/test_responses.py(4 hunks)backend/app/tests/utils/utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/app/seed_data/seed_data.json
- backend/app/seed_data/seed_data.py
- backend/app/tests/api/routes/test_responses.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/app/crud/__init__.py (1)
backend/app/crud/api_key.py (1)
get_api_key_by_user_id(167-182)
backend/app/tests/utils/utils.py (5)
backend/app/crud/user.py (1)
get_user_by_email(34-37)backend/app/crud/api_key.py (2)
get_api_key_by_user_id(167-182)get_api_key_by_value(104-122)backend/app/models/api_key.py (2)
APIKeyPublic(23-25)APIKey(28-38)backend/app/models/user.py (1)
User(48-60)backend/app/tests/conftest.py (1)
db(23-40)
🪛 Ruff (0.12.2)
backend/app/crud/__init__.py
33-33: .api_key.get_api_key_by_user_id imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
backend/app/tests/utils/utils.py
13-13: app.models.User imported but unused
Remove unused import
(F401)
13-13: app.models.APIKey imported but unused
Remove unused import
(F401)
🪛 GitHub Actions: AI Platform CI
backend/app/tests/utils/utils.py
[error] 1-1: Black formatting check failed. The file was reformatted by Black. Run 'black backend/app/tests/utils/utils.py' to apply formatting.
🔇 Additional comments (2)
backend/app/crud/__init__.py (1)
33-33: Import correctly exposes the new CRUD function.The addition of
get_api_key_by_user_idto the import list properly exposes this function from the CRUD module, enabling its use in test utilities as described in the PR objectives.The static analysis tool's unused import warning is a false positive - this import is part of the module's public API and is used by test utilities in
backend/app/tests/utils/utils.py.backend/app/tests/utils/utils.py (1)
7-7: Import addition looks good.The addition of
EmailStrfrompydanticis appropriate for type annotation in the new utility function.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/app/tests/conftest.py (2)
66-69: Add error handling for missing API keys.The fixture doesn't handle cases where the user or API key might not exist in the database, which could cause test failures with unclear error messages.
@pytest.fixture(scope="function") def superuser_api_key_headers(db: Session) -> dict[str, str]: - api_key = get_api_key_by_email(db, settings.FIRST_SUPERUSER) - return {"X-API-KEY": api_key} + try: + api_key = get_api_key_by_email(db, settings.FIRST_SUPERUSER) + if not api_key: + pytest.fail(f"No API key found for superuser {settings.FIRST_SUPERUSER}") + return {"X-API-KEY": api_key} + except Exception as e: + pytest.fail(f"Failed to get API key for superuser {settings.FIRST_SUPERUSER}: {e}")
72-75: Add error handling for missing API keys.Similar to the superuser fixture, this needs error handling for cases where the user or API key might not exist.
@pytest.fixture(scope="function") def normal_user_api_key_headers(db: Session) -> dict[str, str]: - api_key = get_api_key_by_email(db, settings.EMAIL_TEST_USER) - return {"X-API-KEY": api_key} + try: + api_key = get_api_key_by_email(db, settings.EMAIL_TEST_USER) + if not api_key: + pytest.fail(f"No API key found for user {settings.EMAIL_TEST_USER}") + return {"X-API-KEY": api_key} + except Exception as e: + pytest.fail(f"Failed to get API key for user {settings.EMAIL_TEST_USER}: {e}")
🧹 Nitpick comments (1)
backend/app/tests/conftest.py (1)
4-4: Remove unused import.The
timemodule is imported but never used in this file.-import time
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/tests/conftest.py(3 hunks)backend/app/tests/utils/utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/tests/utils/utils.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/tests/conftest.py (1)
backend/app/tests/utils/utils.py (2)
get_superuser_token_headers(37-46)get_api_key_by_email(49-53)
🪛 Ruff (0.12.2)
backend/app/tests/conftest.py
4-4: time imported but unused
Remove unused import: time
(F401)
🔇 Additional comments (1)
backend/app/tests/conftest.py (1)
15-15: LGTM!The import of
get_api_key_by_emailis properly used in the new fixtures below.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/tests/crud/test_api_key.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
| "is_superuser": true | ||
| }, | ||
| { | ||
| "email": "org_admin@example.com", |
There was a problem hiding this comment.
why we are removing this?
There was a problem hiding this comment.
The emails for the "SUPERUSER" and "ADMIN" are removed because they will now be fetched from the environment variables. If you want to add a third user, you can simply include the email in the seed data json the way it was there before
| def test_collection_info_successful( | ||
| db: Session, client: TestClient, superuser_api_key_headers | ||
| ): | ||
| headers = superuser_api_key_headers |
There was a problem hiding this comment.
why we need superuser_api_key_headers and not just user_api_key_headers
maybe just the function name is incorrect but if in production we use user API key for that project as authentication then testcases should follow same behaviour. so Instead of superuser it should be user for that org/project
There was a problem hiding this comment.
I agree that, for tests to accurately reflect production behavior, we should be using a user API key rather than a superuser API key. However, half of our endpoints require the user to be a superuser to access them. That being said, I could use the normal user API key fixture for endpoints where superuser access is not required, ensuring that the tests are aligned with the actual behavior of the system while still testing the necessary use cases.
backend/app/seed_data/seed_data.py
Outdated
| logging.warning( | ||
| f"Email not provided for user {user_data['full_name']}. Skipping user creation." | ||
| ) | ||
| continue # Skip if no email is provided for new users | ||
| logging.info( | ||
| f"Email manually provided for user: {user_data['full_name']}" | ||
| ) |
There was a problem hiding this comment.
do we need this since its been used by dev only in dev env
4 participants
Summary
Target issue is #262
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
This pull request introduces improvements to the test setup, API key management, and seed data configuration:
Fixtures:
Added two new fixtures:
superuser_api_key_headers: Fetches the API key associated with the FIRST_SUPERUSER email from the .env file.
normal_api_key_headers: Fetches the API key associated with the EMAIL_TEST_USER email from the .env file.
Aligned with the existing pattern where:
The superuser token is generated using the FIRST_SUPERUSER and FIRST_SUPERUSER_PASSWORD.
The normal user token uses the EMAIL_TEST_USER.
Tests:
Enhanced the test setup by replacing hardcoded values across multiple test files with the new fixtures for API key authentication headers.
Added utility functions and fixtures to simplify API key retrieval for both superuser and normal user scenarios.
Chores:
Summary by CodeRabbit
Tests
Chores