Testing : Utility functions for test data setup #275
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
nishika26
changed the title
nishika26
changed the title
| ): | ||
| org, creds_data = create_organization_and_creds | ||
| set_creds_for_org(session=db, creds_add=creds_data) | ||
| creds = create_test_credentials[0] |
There was a problem hiding this comment.
why have [0]? if we are fetching a specific credential then we should use filter by name or some sort
There was a problem hiding this comment.
set_creds_for_org() returns a list of credentials, not a single credential. That’s why create_test_credentials ends up being a list, and we access the first element with [0]
There was a problem hiding this comment.
and how are these ordered? so what today create_test_credentials[0] is by sort by name may not be same when we add some new credentials. Can you clean this up and if needed make a function to fetch credentials for a specific provider by passing provider name
WalkthroughTest suites for API keys, credentials, organizations, and projects were refactored to use new shared utility functions for creating test data, replacing manual setup. A new test data helper module was introduced, and supporting utility functions were added. The CRUD exports were updated to include new credential-related functions. Minor updates were made to test code organization and assertions. The Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Function
participant Utils as test_data/utils
participant DB as Database
Test->>Utils: create_test_organization()
Utils->>DB: Insert Organization
DB-->>Utils: Organization instance
Utils-->>Test: Organization
Test->>Utils: create_test_project()
Utils->>DB: Insert Organization (if needed)
Utils->>DB: Insert Project
DB-->>Utils: Project instance
Utils-->>Test: Project
Test->>Utils: create_test_api_key()
Utils->>DB: Insert User
Utils->>DB: Insert Project
Utils->>DB: Insert APIKey
DB-->>Utils: APIKey instance
Utils-->>Test: APIKey
Test->>Utils: create_test_credential()
Utils->>DB: Insert Credential for Project
DB-->>Utils: Credential instance
Utils-->>Test: Credential
Poem
✨ Finishing Touches
🪧 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: 1
♻️ Duplicate comments (2)
backend/app/tests/api/routes/test_creds.py (2)
83-83: Remove debug print statement.This print statement should be removed as it's not needed in production test code.
- print(response_invalid.json())
296-296: Remove debug print statement.This print statement should be removed as it's not needed in production test code.
- print(response.json())
🧹 Nitpick comments (9)
backend/app/tests/utils/utils.py (2)
4-4: Update to use modern type annotation.The
typing.Listimport is deprecated in favor of the built-inlisttype.-from typing import List +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from app.models import CredentialOr simply use
listinstead ofListin the type annotation.
63-71: LGTM! Well-implemented credential lookup function.The
get_credential_by_providerfunction provides a clean way to find credentials by provider with appropriate error handling.Consider updating the type annotation to use the modern
listtype:-def get_credential_by_provider(creds: List[Credential], provider: str) -> Credential: +def get_credential_by_provider(creds: list[Credential], provider: str) -> Credential:backend/app/tests/api/routes/test_org.py (3)
8-8: Remove unused import.The
create_organizationfunction is no longer used since the refactoring to usecreate_test_organization.-from app.crud.organization import create_organization, get_organization_by_id +from app.crud.organization import get_organization_by_id
15-16: Remove unused parameter from fixture.The
superuser_token_headersparameter is not used in the fixture.-def test_organization(db: Session, superuser_token_headers: dict[str, str]): +def test_organization(db: Session):
40-47: Remove unused parameter.The
dbparameter is not used in this test function.-def test_read_organizations(db: Session, superuser_token_headers: dict[str, str]): +def test_read_organizations(superuser_token_headers: dict[str, str]):backend/app/tests/api/routes/test_project.py (1)
51-58: Remove unused parameter.The
dbparameter is not used in this test function.-def test_read_projects(db: Session, superuser_token_headers: dict[str, str]): +def test_read_projects(superuser_token_headers: dict[str, str]):backend/app/tests/api/routes/test_api_key.py (2)
12-12: Remove unused import.The
create_test_organizationfunction is imported but not used in this file.-from app.tests.utils.test_data import ( - create_test_api_key, - create_test_project, - create_test_organization, -) +from app.tests.utils.test_data import ( + create_test_api_key, + create_test_project, +)
90-90: Remove debug print statement.The print statement appears to be leftover debugging code and should be removed.
- print(api_key_id)backend/app/tests/utils/test_data.py (1)
9-9: Remove unused import.The
APIKeyPublicimport is not used in this file.-from app.models import ( - Organization, - Project, - APIKey, - Credential, - OrganizationCreate, - ProjectCreate, - APIKeyPublic, - CredsCreate, -) +from app.models import ( + Organization, + Project, + APIKey, + Credential, + OrganizationCreate, + ProjectCreate, + CredsCreate, +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
backend/app/crud/__init__.py(1 hunks)backend/app/tests/api/routes/test_api_key.py(6 hunks)backend/app/tests/api/routes/test_creds.py(19 hunks)backend/app/tests/api/routes/test_org.py(2 hunks)backend/app/tests/api/routes/test_project.py(2 hunks)backend/app/tests/crud/test_api_key.py(3 hunks)backend/app/tests/crud/test_credentials.py(7 hunks)backend/app/tests/crud/test_org.py(3 hunks)backend/app/tests/crud/test_project.py(3 hunks)backend/app/tests/utils/collection.py(3 hunks)backend/app/tests/utils/test_data.py(1 hunks)backend/app/tests/utils/utils.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
backend/app/tests/utils/collection.py (2)
backend/app/tests/utils/test_data.py (1)
create_test_project(34-50)backend/app/tests/conftest.py (1)
db(27-41)
backend/app/tests/crud/test_project.py (5)
backend/app/models/project.py (2)
Project(28-49)ProjectCreate(16-17)backend/app/crud/project.py (4)
create_project(10-17)get_project_by_id(20-22)get_projects_by_organization(25-27)validate_project(30-41)backend/app/tests/utils/utils.py (2)
random_lower_string(26-27)get_non_existent_id(74-76)backend/app/tests/utils/test_data.py (2)
create_test_project(34-50)create_test_organization(23-31)backend/app/tests/conftest.py (1)
db(27-41)
backend/app/crud/__init__.py (1)
backend/app/crud/credentials.py (2)
get_provider_credential(90-107)remove_provider_credential(154-175)
backend/app/tests/api/routes/test_project.py (3)
backend/app/models/project.py (2)
Project(28-49)ProjectCreate(16-17)backend/app/tests/utils/test_data.py (2)
create_test_organization(23-31)create_test_project(34-50)backend/app/tests/conftest.py (3)
client(45-47)db(27-41)superuser_token_headers(51-52)
backend/app/tests/crud/test_org.py (3)
backend/app/tests/utils/test_data.py (1)
create_test_organization(23-31)backend/app/tests/conftest.py (1)
db(27-41)backend/app/crud/organization.py (1)
get_organization_by_id(23-25)
backend/app/tests/utils/test_data.py (4)
backend/app/models/credentials.py (2)
Credential(46-90)CredsCreate(15-24)backend/app/crud/credentials.py (1)
set_creds_for_org(17-52)backend/app/core/providers.py (1)
Provider(6-11)backend/app/tests/utils/utils.py (2)
random_lower_string(26-27)generate_random_string(30-31)
backend/app/tests/utils/utils.py (2)
backend/app/models/api_key.py (1)
APIKeyPublic(23-25)backend/app/models/credentials.py (1)
Credential(46-90)
backend/app/tests/api/routes/test_org.py (4)
backend/app/models/organization.py (1)
Organization(34-54)backend/app/crud/organization.py (1)
create_organization(10-19)backend/app/tests/utils/test_data.py (1)
create_test_organization(23-31)backend/app/tests/conftest.py (3)
client(45-47)db(27-41)superuser_token_headers(51-52)
🪛 Ruff (0.11.9)
backend/app/crud/__init__.py
40-40: .credentials.get_provider_credential imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
41-41: .credentials.remove_provider_credential imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
backend/app/tests/api/routes/test_project.py
51-51: Unused function argument: db
(ARG001)
backend/app/tests/utils/test_data.py
9-9: app.models.APIKeyPublic imported but unused
Remove unused import: app.models.APIKeyPublic
(F401)
backend/app/tests/utils/utils.py
4-4: typing.List is deprecated, use list instead
(UP035)
63-63: Use list instead of List for type annotation
Replace with list
(UP006)
backend/app/tests/api/routes/test_org.py
8-8: app.crud.organization.create_organization imported but unused
Remove unused import: app.crud.organization.create_organization
(F401)
15-15: Unused function argument: superuser_token_headers
(ARG001)
40-40: Unused function argument: db
(ARG001)
52-52: Unused function argument: db
(ARG001)
backend/app/tests/api/routes/test_api_key.py
12-12: app.tests.utils.test_data.create_test_organization imported but unused
Remove unused import: app.tests.utils.test_data.create_test_organization
(F401)
backend/app/tests/api/routes/test_creds.py
115-115: Unused function argument: db
(ARG001)
148-148: Unused function argument: db
(ARG001)
179-179: Unused function argument: db
(ARG001)
269-269: Unused function argument: db
(ARG001)
301-301: Unused function argument: db
(ARG001)
🔇 Additional comments (28)
backend/app/crud/__init__.py (1)
40-41: LGTM! Re-export pattern is correct.The static analysis warning about unused imports is a false positive. These functions are being re-exported to make them available through the
crudmodule's public API, which is a common and appropriate pattern for Python packages.backend/app/tests/crud/test_org.py (2)
6-6: LGTM! Good refactoring to use shared test utilities.The import of
create_test_organizationaligns with the PR objective to centralize test data creation.
22-22: LGTM! Consistent use of test utility function.The replacement of manual organization creation with the shared
create_test_organizationfunction improves maintainability and consistency across the test suite.backend/app/tests/utils/utils.py (1)
30-31: LGTM! Useful utility function for generating random strings.The
generate_random_stringfunction provides a clean way to generate random alphanumeric strings for test data.backend/app/tests/utils/collection.py (3)
10-10: LGTM! Consistent with test utility refactoring.The import of
create_test_projectaligns with the broader refactoring to use shared test utilities.
28-28: LGTM! Simplified test data creation.The replacement of manual organization and project creation with the shared
create_test_projectutility function improves maintainability and consistency.
32-35: LGTM! Properly uses the project's organization reference.The code correctly uses
project.organization_idsincecreate_test_projectcreates both the organization and project.backend/app/tests/crud/test_api_key.py (4)
3-7: LGTM! Well-organized imports using shared test utilities.The updated imports align with the PR objective to centralize test data creation using shared utility functions.
11-16: LGTM! Consistent use of test utilities.The replacement of manual entity creation with
create_random_userandcreate_test_projectimproves maintainability and consistency across the test suite.
26-27: LGTM! Simplified test setup.Using
create_test_api_keyeliminates boilerplate code and ensures consistent test data creation.
36-37: LGTM! Improved negative test approach.Using
get_non_existent_idinstead of hardcoded values makes the test more robust and maintainable.backend/app/tests/api/routes/test_org.py (1)
21-22: LGTM! Good refactoring to use fixed test data.The change from random organization names to fixed strings improves test determinism and readability.
backend/app/tests/crud/test_project.py (3)
12-13: LGTM! Excellent refactoring to use test data utilities.The addition of utility functions for test data creation and non-existent ID generation improves test maintainability and robustness.
53-67: Good improvement to test determinism.Using fixed project names instead of random ones makes tests more predictable and easier to debug.
77-77: Excellent improvement using dynamic non-existent IDs.Replacing hardcoded values like
999withget_non_existent_idmakes tests more robust and less prone to conflicts with actual data.backend/app/tests/api/routes/test_project.py (1)
15-16: LGTM! Great simplification of test fixture.Using
create_test_project(db)directly makes the fixture much cleaner and more maintainable.backend/app/tests/api/routes/test_api_key.py (1)
19-20: LGTM! Excellent refactoring to use utility functions.The refactoring simplifies test setup and makes the code more maintainable.
backend/app/tests/utils/test_data.py (1)
23-31: LGTM! Well-implemented utility function.The function is properly documented and follows good practices for creating test data with unique identifiers.
backend/app/tests/crud/test_credentials.py (4)
4-19: LGTM! Well-structured imports and utility function usage.The refactoring to use individual CRUD function imports and utility functions from
test_datamodule improves code organization and reusability. The imports are clean and follow the new pattern established in the PR.
22-48: Good refactoring to use utility functions.The test correctly uses
create_test_project(db)and updates assertions to check bothorganization_idandproject_id. This aligns with the PR objective of using reusable utility functions.
84-98: Improved test data setup with utility functions.The use of
test_credential_data(db)and accessing the original API key for verification is a good pattern. This makes the test more maintainable and consistent.
100-119: Excellent use of helper functions.The combination of
create_test_credential(db)andget_credential_by_providerhelper function makes the test more readable and robust compared to manual data setup.backend/app/tests/api/routes/test_creds.py (6)
11-21: Well-organized imports using utility functions.The refactoring to use utility functions from
test_dataandutilsmodules is clean and follows the established pattern in the PR. This improves code reusability and maintainability.
28-29: Good fixture pattern for test data.The
create_test_credentialsfixture that wraps the utility function is a clean approach for providing test data to multiple test functions.
32-61: Excellent refactoring with proper test data setup.The test correctly uses
create_test_project(db)and updates the request data structure to include bothorganization_idandproject_id. The assertions are properly updated to verify the correct organization ID.
94-95: Good use of utility functions for robust testing.Using
create_test_organization(db)andget_non_existent_id(db, Project)makes the test more reliable and maintainable compared to hardcoded values.
115-119: Excellent pattern for credential testing.The combination of the fixture and
get_credential_by_providerhelper function creates a clean and maintainable test pattern.
341-345: Good use of utility function for test data.The use of
test_credential_data(db)provides consistent test data structure and the.dict()call properly serializes it for the API request.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/app/tests/api/routes/test_creds.py (1)
295-295: Remove unnecessary f-string prefix.The f-string has no placeholders, so the
fprefix is unnecessary.- assert response.json()["error"] == f"Provider credentials not found" + assert response.json()["error"] == "Provider credentials not found"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/app/tests/api/routes/test_api_key.py(6 hunks)backend/app/tests/api/routes/test_creds.py(18 hunks)backend/app/tests/api/routes/test_org.py(2 hunks)backend/app/tests/utils/test_data.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/tests/api/routes/test_api_key.py
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/app/tests/api/routes/test_creds.py
114-114: Unused function argument: db
(ARG001)
147-147: Unused function argument: db
(ARG001)
178-178: Unused function argument: db
(ARG001)
268-268: Unused function argument: db
(ARG001)
295-295: f-string without any placeholders
Remove extraneous f prefix
(F541)
299-299: Unused function argument: db
(ARG001)
backend/app/tests/api/routes/test_org.py
40-40: Unused function argument: db
(ARG001)
52-52: Unused function argument: db
(ARG001)
backend/app/tests/utils/test_data.py
1-1: typing.List is deprecated, use list instead
(UP035)
95-95: Use list instead of List for type annotation
Replace with list
(UP006)
⏰ 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)
🔇 Additional comments (10)
backend/app/tests/api/routes/test_org.py (3)
15-16: Good refactoring to use shared utility function.The fixture now uses the centralized
create_test_organizationfunction, improving consistency across tests.
21-22: Fixed string literals improve test clarity.Using fixed strings like "Test-Org" instead of random strings makes tests more predictable and easier to debug.
40-48: Ignore unuseddbfixture warning intest_read_organizations
Thedb: Sessionparameter is a pytest fixture used for database session setup via dependency injection. Although not referenced directly in the test body, it must remain for proper test execution.· File: backend/app/tests/api/routes/test_org.py, lines 40–48
backend/app/tests/utils/test_data.py (3)
25-33: Well-designed utility function with proper encapsulation.The function correctly creates a test organization with a unique name and handles database persistence. The docstring clearly explains its purpose and behavior.
36-52: Good hierarchical test data creation.The function properly creates the dependency chain (organization → project) and uses descriptive naming. The relationship setup through
organization_idis correct.
95-116: Function correctly returns list of credentials.The function annotation and implementation are consistent. The return type correctly reflects that
set_creds_for_orgreturns a list of credentials. The test data setup is comprehensive with realistic OpenAI provider configuration.backend/app/tests/api/routes/test_creds.py (4)
28-29: Good refactoring to use centralized test data creation.The fixture now uses
create_test_credentialfrom the utility module, improving consistency and reducing code duplication.
114-119: Excellent solution to the list indexing issue.Using
get_credential_by_providerutility function eliminates the brittle list indexing approach ([0]) that was previously discussed. This makes the tests more robust and explicit about which credential is being accessed.
336-357: Well-structured test using utility functions.The test now uses
test_credential_datautility function which provides clean test data without persisting it to the database. This is appropriate for testing duplicate creation scenarios.
418-446: Comprehensive encryption testing with utility functions.The test properly verifies both encryption in storage and decryption functionality. Using
test_credential_datautility keeps the test focused on encryption behavior rather than test data setup.
backend/app/tests/utils/test_data.py
Outdated
| @@ -0,0 +1,116 @@ | |||
| from typing import List | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update deprecated typing import.
The typing.List import is deprecated in Python 3.9+. Use the built-in list type instead.
-from typing import List
+from typing import TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from typing import ListOr simply use lowercase list in the type annotations throughout the file.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from typing import List | |
| -from typing import List | |
| +from typing import TYPE_CHECKING | |
| + | |
| +if TYPE_CHECKING: | |
| + from typing import List |
🧰 Tools
🪛 Ruff (0.11.9)
1-1: typing.List is deprecated, use list instead
(UP035)
🤖 Prompt for AI Agents
In backend/app/tests/utils/test_data.py at line 1, the import of List from
typing is deprecated in Python 3.9 and later. Remove the import statement for
List and update all type annotations in the file to use the built-in lowercase
list type instead.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
backend/app/tests/utils/test_data.py (2)
1-1: Update deprecated typing importThe
typing.Listimport is deprecated in Python 3.9+. Use the built-inlisttype instead.-from typing import ListThen update all type annotations to use lowercase
list.
95-95: Update type annotation to use modern syntaxUse lowercase
listinstead ofListfor type annotations as recommended by Python 3.9+.-def create_test_credential(db: Session) -> List[Credential]: +def create_test_credential(db: Session) -> list[Credential]:
🧹 Nitpick comments (4)
backend/app/tests/crud/test_credentials.py (1)
15-15: Remove unused importThe
get_credential_by_providerfunction is imported but never used in this file.-from app.tests.utils.utils import get_credential_by_providerbackend/app/tests/api/routes/test_creds.py (3)
15-15: Remove unused importThe
get_credential_by_providerfunction is imported but never used in this file.- get_credential_by_provider,
152-180: Remove unused function parameterThe
dbparameter is not used in this test function.-def test_read_provider_credential_not_found( - db: Session, superuser_token_headers: dict[str, str] -): +def test_read_provider_credential_not_found( + db: Session, superuser_token_headers: dict[str, str] +):Wait, looking more carefully, the
dbparameter is used on line 171 to create a test organization. The static analysis hint appears to be incorrect in this case.
317-317: Remove unnecessary f-string prefixThe f-string has no placeholders, so the
fprefix is not needed.- assert response.json()["error"] == f"Provider credentials not found" + assert response.json()["error"] == "Provider credentials not found"
📜 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/crud/credentials.py(1 hunks)backend/app/tests/api/routes/test_creds.py(19 hunks)backend/app/tests/crud/test_credentials.py(7 hunks)backend/app/tests/utils/test_data.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
backend/app/tests/utils/test_data.py (6)
backend/app/models/organization.py (2)
Organization(34-54)OrganizationCreate(23-24)backend/app/models/project.py (2)
Project(28-49)ProjectCreate(16-17)backend/app/models/credentials.py (2)
Credential(46-90)CredsCreate(15-24)backend/app/crud/credentials.py (1)
set_creds_for_org(17-52)backend/app/core/providers.py (1)
Provider(6-11)backend/app/tests/utils/utils.py (2)
random_lower_string(26-27)generate_random_string(30-31)
backend/app/crud/__init__.py (1)
backend/app/crud/credentials.py (3)
get_provider_credential(90-107)remove_provider_credential(174-195)get_full_provider_credential(110-127)
backend/app/tests/api/routes/test_creds.py (7)
backend/app/crud/credentials.py (1)
get_full_provider_credential(110-127)backend/app/core/providers.py (1)
Provider(6-11)backend/app/models/credentials.py (1)
Credential(46-90)backend/app/core/security.py (1)
decrypt_credentials(164-181)backend/app/tests/utils/utils.py (3)
generate_random_string(30-31)get_non_existent_id(74-76)get_credential_by_provider(63-71)backend/app/tests/utils/test_data.py (4)
create_test_credential(95-116)create_test_organization(25-33)create_test_project(36-52)test_credential_data(72-92)backend/app/tests/conftest.py (3)
client(45-47)db(27-41)superuser_token_headers(51-52)
backend/app/tests/crud/test_credentials.py (6)
backend/app/crud/credentials.py (7)
set_creds_for_org(17-52)get_creds_by_org(77-87)get_provider_credential(90-107)update_creds_for_org(138-171)remove_provider_credential(174-195)remove_creds_for_org(198-215)get_full_provider_credential(110-127)backend/app/models/credentials.py (2)
CredsCreate(15-24)CredsUpdate(27-43)backend/app/core/providers.py (1)
Provider(6-11)backend/app/tests/utils/utils.py (1)
get_credential_by_provider(63-71)backend/app/tests/utils/test_data.py (3)
create_test_project(36-52)create_test_credential(95-116)test_credential_data(72-92)backend/app/tests/conftest.py (1)
db(27-41)
🪛 Ruff (0.11.9)
backend/app/crud/credentials.py
111-111: Use X | Y for type annotations
Convert to X | Y
(UP007)
112-112: Use X | Y for type annotations
Convert to X | Y
(UP007)
112-112: Use dict instead of Dict for type annotation
Replace with dict
(UP006)
119-119: Avoid equality comparisons to True; use if Credential.is_active: for truth checks
Replace with Credential.is_active
(E712)
backend/app/tests/utils/test_data.py
1-1: typing.List is deprecated, use list instead
(UP035)
95-95: Use list instead of List for type annotation
Replace with list
(UP006)
backend/app/crud/__init__.py
40-40: .credentials.get_provider_credential imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
41-41: .credentials.remove_provider_credential imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
42-42: .credentials.get_full_provider_credential imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
backend/app/tests/api/routes/test_creds.py
15-15: app.tests.utils.utils.get_credential_by_provider imported but unused
Remove unused import: app.tests.utils.utils.get_credential_by_provider
(F401)
153-153: Unused function argument: db
(ARG001)
317-317: f-string without any placeholders
Remove extraneous f prefix
(F541)
backend/app/tests/crud/test_credentials.py
15-15: app.tests.utils.utils.get_credential_by_provider imported but unused
Remove unused import: app.tests.utils.utils.get_credential_by_provider
(F401)
⏰ 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)
🔇 Additional comments (4)
backend/app/crud/__init__.py (1)
40-42: LGTM - Proper CRUD function exportsThe new credential-related functions are correctly imported and re-exported. The static analysis "unused" warnings are false positives for init.py exports, which is expected behavior for module interface exposure.
backend/app/tests/utils/test_data.py (1)
25-116: LGTM - Well-structured test utilitiesThe utility functions are well-designed with clear docstrings and consistent patterns. They effectively centralize test data creation and will improve maintainability across the test suite.
backend/app/tests/crud/test_credentials.py (1)
16-20: LGTM - Excellent refactoring to use centralized utilitiesThe refactoring to use
create_test_project,create_test_credential, andtest_credential_datagreatly improves test maintainability and consistency across the test suite.backend/app/tests/api/routes/test_creds.py (1)
28-31: LGTM - Excellent refactoring to use centralized test utilitiesThe refactoring to use
create_test_credentialand other utility functions from the test_data module greatly improves code maintainability and consistency across the test suite.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
backend/app/tests/crud/test_credentials.py (1)
15-15: Remove unused import.The static analysis tool correctly identifies that
get_credential_by_provideris imported but never used in the test file.-from app.tests.utils.utils import get_credential_by_provider
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/tests/crud/test_credentials.py(5 hunks)backend/app/tests/crud/test_org.py(1 hunks)backend/app/tests/crud/test_project.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/tests/crud/test_org.py
- backend/app/tests/crud/test_project.py
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/app/tests/crud/test_credentials.py
15-15: app.tests.utils.utils.get_credential_by_provider imported but unused
Remove unused import: app.tests.utils.utils.get_credential_by_provider
(F401)
⏰ 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)
🔇 Additional comments (3)
backend/app/tests/crud/test_credentials.py (3)
23-52: Well-structured test refactoring.The test has been successfully refactored to use the utility function
create_test_project(db)and properly validates both organization and project associations. The assertions correctly verify the credential properties including the new project_id field.
85-99: Improved test data handling.Good refactoring to use
test_credential_data(db)and extract the original API key for comparison rather than using hardcoded values. This makes the test more robust and maintainable.
103-125: Proper use of new CRUD functions.The test correctly uses
create_test_credential(db)with tuple unpacking andget_full_provider_credentialwith the required project_id parameter. The function calls are appropriately updated to match the new API.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/app/crud/credentials.py (1)
110-112: Fix return type annotation and modernize type syntax.The function returns a
Credentialobject but is annotated to returnOptional[Dict[str, Any]]. Also, the type annotations should use modern Python 3.9+ syntax.-def get_full_provider_credential( - *, session: Session, org_id: int, provider: str, project_id: Optional[int] = None -) -> Optional[Dict[str, Any]]: +def get_full_provider_credential( + *, session: Session, org_id: int, provider: str, project_id: Optional[int] = None +) -> Optional[Credential]:You'll also need to ensure
Credentialis imported if not already available.
🧹 Nitpick comments (2)
backend/app/crud/credentials.py (1)
125-127: Fix misleading comment.The comment mentions "Decrypt entire credentials object" but the function doesn't decrypt anything - it returns the raw
Credentialobject.if creds and creds.credential: - # Decrypt entire credentials object + # Return the full Credential object without decryption return credsbackend/app/tests/utils/test_data.py (1)
1-1: Remove unused and deprecated import.The
typing.Listimport is unused since all functions use lowercaselisttype annotations, and this import is deprecated in Python 3.9+.-from typing import List
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/crud/credentials.py(1 hunks)backend/app/tests/utils/test_data.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/crud/credentials.py (2)
backend/app/core/providers.py (1)
validate_provider(33-51)backend/app/models/credentials.py (1)
Credential(46-90)
🪛 Ruff (0.11.9)
backend/app/crud/credentials.py
111-111: Use X | Y for type annotations
Convert to X | Y
(UP007)
112-112: Use X | Y for type annotations
Convert to X | Y
(UP007)
112-112: Use dict instead of Dict for type annotation
Replace with dict
(UP006)
120-120: Avoid equality comparisons to True; use if Credential.is_active: for truth checks
Replace with Credential.is_active
(E712)
backend/app/tests/utils/test_data.py
1-1: typing.List is deprecated, use list instead
(UP035)
1-1: typing.List imported but unused
Remove unused import: typing.List
(F401)
⏰ 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)
🔇 Additional comments (1)
backend/app/tests/utils/test_data.py (1)
25-117: Excellent test utility module design.The utility functions are well-structured and provide good separation of concerns:
create_test_organizationandcreate_test_projectfor basic entity creationcreate_test_api_keyfor API key setuptest_credential_datafor non-persisted credential datacreate_test_credentialfor full credential setupThe return type annotations are correct and the functions provide clear, reusable test data creation patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/app/crud/credentials.py (1)
110-128: Fix return type annotation and misleading commentThis function has multiple issues that need to be addressed:
- Incorrect return type: The function returns a
Credentialobject but is annotated to returnOptional[Dict[str, Any]]- Misleading comment: The comment mentions "Decrypt entire credentials object" but the function returns the raw
Credentialobject without decryption- Boolean comparison: Use
Credential.is_activeinstead ofCredential.is_active == TrueApply this diff to fix the issues:
def get_full_provider_credential( *, session: Session, org_id: int, provider: str, project_id: Optional[int] = None -) -> Optional[Dict[str, Any]]: +) -> Optional[Credential]: - """Fetches credentials for a specific provider of an organization.""" + """Fetches the full Credential object for a specific provider of an organization.""" validate_provider(provider) statement = select(Credential).where( Credential.organization_id == org_id, Credential.provider == provider, - Credential.is_active == True, + Credential.is_active, Credential.project_id == project_id if project_id is not None else True, ) creds = session.exec(statement).first() if creds and creds.credential: - # Decrypt entire credentials object + # Return the full Credential object return creds return NoneYou'll also need to ensure
Credentialis imported at the top of the file if not already imported.
🧹 Nitpick comments (1)
backend/app/tests/crud/test_credentials.py (1)
15-15: Remove unused importThe import
get_credential_by_providerfromapp.tests.utils.utilsis not used in this file.Apply this diff to remove the unused import:
-from app.tests.utils.utils import get_credential_by_provider from app.tests.utils.test_data import ( create_test_project, create_test_credential, test_credential_data, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/crud/credentials.py(1 hunks)backend/app/tests/crud/test_credentials.py(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/crud/credentials.py (2)
backend/app/core/providers.py (1)
validate_provider(33-51)backend/app/models/credentials.py (1)
Credential(46-90)
🪛 Ruff (0.11.9)
backend/app/crud/credentials.py
111-111: Use X | Y for type annotations
Convert to X | Y
(UP007)
112-112: Use X | Y for type annotations
Convert to X | Y
(UP007)
112-112: Use dict instead of Dict for type annotation
Replace with dict
(UP006)
119-119: Avoid equality comparisons to True; use if Credential.is_active: for truth checks
Replace with Credential.is_active
(E712)
backend/app/tests/crud/test_credentials.py
15-15: app.tests.utils.utils.get_credential_by_provider imported but unused
Remove unused import: app.tests.utils.utils.get_credential_by_provider
(F401)
⏰ 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)
🔇 Additional comments (3)
backend/app/tests/crud/test_credentials.py (3)
23-51: LGTM: Well-structured test using utility functionsThe test has been successfully refactored to use the new utility functions. The test setup is clean and consistent with the project's testing patterns.
85-99: LGTM: Proper use of test utility functionsThe test correctly uses the
test_credential_datautility function and properly validates the retrieved credentials.
103-125: LGTM: Effective use of the new get_full_provider_credential functionThe test properly demonstrates the usage of the new
get_full_provider_credentialfunction to retrieve the full credential object for testing purposes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/app/crud/credentials.py (2)
110-112: Fix return type annotation and modernize type hints.The function is annotated to return
Optional[Dict[str, Any]]but actually returns aCredentialobject. Additionally, the type hints can be modernized to use Python 3.10+ union syntax.def get_full_provider_credential( - *, session: Session, org_id: int, provider: str, project_id: Optional[int] = None -) -> Optional[Dict[str, Any]]: + *, session: Session, org_id: int, provider: str, project_id: int | None = None +) -> Credential | None:Also update the import statement to remove unused imports:
-from typing import Optional, Dict, Any, List +from typing import Any, List
119-119: Simplify boolean comparison.Avoid explicit comparison to
Truefor boolean values.- Credential.is_active == True, + Credential.is_active,
🧹 Nitpick comments (2)
backend/app/crud/credentials.py (2)
113-113: Improve function documentation to distinguish from similar function.The docstring is identical to
get_provider_credentialbut doesn't explain the key difference: this function returns the fullCredentialobject whileget_provider_credentialreturns only the decrypted credentials as a dictionary.- """Fetches credentials for a specific provider of an organization.""" + """Fetches the full Credential object for a specific provider of an organization. + + Unlike get_provider_credential, this returns the complete Credential model instance + rather than just the decrypted credential data. + """
124-127: Consider consistent return pattern.The function returns
creds(aCredentialobject) when credentials exist, but the logic checksif creds and creds.credential:. This means it only returns the credential if thecredentialfield is not null, but then returns the full object. Consider if this logic is intentional or if it should match the pattern of other functions.If the intention is to return the full
Credentialobject regardless of whether the credential data is encrypted/present, consider:- if creds and creds.credential: + if creds: # Return the full Credential object return creds return NoneOtherwise, if the current logic is intentional, consider adding a comment explaining why the credential field check is necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/crud/credentials.py(1 hunks)backend/app/tests/utils/test_data.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/tests/utils/test_data.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/crud/credentials.py (2)
backend/app/core/providers.py (1)
validate_provider(33-51)backend/app/models/credentials.py (1)
Credential(46-90)
🪛 Ruff (0.11.9)
backend/app/crud/credentials.py
111-111: Use X | Y for type annotations
Convert to X | Y
(UP007)
112-112: Use X | Y for type annotations
Convert to X | Y
(UP007)
112-112: Use dict instead of Dict for type annotation
Replace with dict
(UP006)
119-119: Avoid equality comparisons to True; use if Credential.is_active: for truth checks
Replace with Credential.is_active
(E712)
⏰ 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)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/app/crud/credentials.py (1)
113-113: Improve the docstring to differentiate from similar function.The docstring is identical to
get_provider_credentialbut doesn't explain the key difference - this function returns the fullCredentialobject while the other returns decrypted credential data.Apply this diff to improve the docstring:
- """Fetches credentials for a specific provider of an organization.""" + """Fetches the full Credential object for a specific provider of an organization."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/crud/credentials.py(1 hunks)backend/app/tests/utils/test_data.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/app/tests/utils/test_data.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/crud/credentials.py (2)
backend/app/models/credentials.py (1)
Credential(46-90)backend/app/core/providers.py (1)
validate_provider(33-51)
🪛 Ruff (0.11.9)
backend/app/crud/credentials.py
111-111: Use X | Y for type annotations
Convert to X | Y
(UP007)
112-112: Use X | Y for type annotations
Convert to X | Y
(UP007)
119-119: Avoid equality comparisons to True; use if Credential.is_active: for truth checks
Replace with Credential.is_active
(E712)
🪛 GitHub Check: codecov/patch
backend/app/crud/credentials.py
[warning] 127-127: backend/app/crud/credentials.py#L127
Added line #L127 was not covered by tests
| statement = select(Credential).where( | ||
| Credential.organization_id == org_id, | ||
| Credential.provider == provider, | ||
| Credential.is_active == True, | ||
| Credential.project_id == project_id if project_id is not None else True, | ||
| ) |
There was a problem hiding this comment.
Fix the WHERE clause logic for project_id filtering.
The current implementation has two critical issues:
- The boolean comparison
Credential.is_active == Trueshould be simplified toCredential.is_active - The conditional logic
Credential.project_id == project_id if project_id is not None else Trueis incorrect. Whenproject_idisNone, this becomesCredential.project_id == True, which doesn't make logical sense and will cause unexpected filtering behavior.
Apply this diff to fix the WHERE clause:
statement = select(Credential).where(
Credential.organization_id == org_id,
Credential.provider == provider,
- Credential.is_active == True,
- Credential.project_id == project_id if project_id is not None else True,
+ Credential.is_active,
+ Credential.project_id == project_id if project_id is not None else Credential.project_id.is_not(None),
)Note: This assumes that when project_id is None, you want to filter for records that have a non-null project_id. If you want to match records regardless of their project_id value when the parameter is None, you should restructure the query to conditionally add the filter instead.
🧰 Tools
🪛 Ruff (0.11.9)
119-119: Avoid equality comparisons to True; use if Credential.is_active: for truth checks
Replace with Credential.is_active
(E712)
🤖 Prompt for AI Agents
In backend/app/crud/credentials.py around lines 116 to 121, simplify the
condition `Credential.is_active == True` to just `Credential.is_active`. For the
`project_id` filter, do not use a conditional expression inside the `where`
clause; instead, build the query so that if `project_id` is not None, add a
filter `Credential.project_id == project_id`, otherwise either omit this filter
or explicitly filter for non-null `project_id` depending on the intended logic.
This avoids incorrect comparisons and ensures proper filtering behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/app/crud/credentials.py (1)
110-111: Fix boolean comparison and WHERE clause logic.The same issues flagged in previous reviews are still present and need to be addressed.
Apply this diff to fix both issues:
- Credential.is_active == True, - Credential.project_id == project_id if project_id is not None else True, + Credential.is_active, + *(Credential.project_id == project_id,) if project_id is not None else (),The boolean comparison should be simplified, and the conditional WHERE clause needs proper handling to avoid comparing
project_idtoTrue.
🧹 Nitpick comments (4)
backend/app/crud/credentials.py (1)
1-1: Modernize type annotations for Python 3.10+.Consider updating the type imports to use modern syntax for better consistency with current Python standards.
-from typing import Optional, Dict, Any, List, Union +from typing import Optional, AnyAnd update the function signature to use built-in types:
-) -> Optional[Union[Dict[str, Any], Credential]]: +) -> Optional[Union[dict[str, Any], Credential]]:backend/app/tests/crud/test_credentials.py (1)
14-14: Remove unused import.The
get_credential_by_providerfunction is imported but not used in this file.-from app.tests.utils.utils import get_credential_by_providerbackend/app/tests/api/routes/test_creds.py (2)
15-15: Remove unused import.The
get_credential_by_providerfunction is imported but not used in this file.- get_credential_by_provider,
321-321: Remove unnecessary f-string prefix.The string doesn't contain any placeholders, so the f-string prefix is unnecessary.
- assert response.json()["error"] == f"Provider credentials not found" + assert response.json()["error"] == "Provider credentials not found"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/app/crud/__init__.py(1 hunks)backend/app/crud/credentials.py(3 hunks)backend/app/tests/api/routes/test_creds.py(19 hunks)backend/app/tests/crud/test_credentials.py(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
backend/app/crud/__init__.py (1)
backend/app/crud/credentials.py (2)
get_provider_credential(90-117)remove_provider_credential(164-185)
backend/app/crud/credentials.py (2)
backend/app/models/credentials.py (1)
Credential(46-90)backend/app/core/security.py (1)
decrypt_credentials(164-181)
backend/app/tests/api/routes/test_creds.py (8)
backend/app/crud/credentials.py (1)
get_provider_credential(90-117)backend/app/models/organization.py (1)
Organization(34-54)backend/app/models/project.py (1)
Project(28-49)backend/app/core/providers.py (1)
Provider(6-11)backend/app/models/credentials.py (1)
Credential(46-90)backend/app/core/security.py (1)
decrypt_credentials(164-181)backend/app/tests/utils/utils.py (3)
generate_random_string(30-31)get_non_existent_id(74-76)get_credential_by_provider(63-71)backend/app/tests/utils/test_data.py (4)
create_test_credential(93-114)create_test_organization(23-31)create_test_project(34-50)test_credential_data(70-90)
🪛 Ruff (0.11.9)
backend/app/crud/__init__.py
40-40: .credentials.get_provider_credential imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
41-41: .credentials.remove_provider_credential imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
backend/app/crud/credentials.py
1-1: typing.Dict is deprecated, use dict instead
(UP035)
1-1: typing.List is deprecated, use list instead
(UP035)
95-95: Use X | Y for type annotations
Convert to X | Y
(UP007)
97-97: Use X | Y for type annotations
Convert to X | Y
(UP007)
97-97: Use X | Y for type annotations
Convert to X | Y
(UP007)
97-97: Use dict instead of Dict for type annotation
Replace with dict
(UP006)
backend/app/tests/api/routes/test_creds.py
15-15: app.tests.utils.utils.get_credential_by_provider imported but unused
Remove unused import: app.tests.utils.utils.get_credential_by_provider
(F401)
154-154: Unused function argument: db
(ARG001)
321-321: f-string without any placeholders
Remove extraneous f prefix
(F541)
backend/app/tests/crud/test_credentials.py
14-14: app.tests.utils.utils.get_credential_by_provider imported but unused
Remove unused import: app.tests.utils.utils.get_credential_by_provider
(F401)
⏰ 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)
🔇 Additional comments (7)
backend/app/crud/__init__.py (1)
40-41: LGTM! Exposing credential management functions through the public API.The addition of
get_provider_credentialandremove_provider_credentialto the module exports aligns with the refactored test infrastructure and makes these functions available for broader use across the application.backend/app/crud/credentials.py (1)
95-116: Well-implemented enhancement for flexible credential retrieval.The addition of the
fullparameter provides good flexibility for callers to choose between getting decrypted credential data or the full database object. The conditional return logic is clean and well-documented.backend/app/tests/crud/test_credentials.py (2)
4-19: Excellent refactoring to use centralized test utilities.The updated imports align well with the PR objective of introducing reusable utility functions for test data setup. This reduces code duplication and improves maintainability across the test suite.
104-110: Good use of the enhanced get_provider_credential function.The test correctly uses the new
full=Trueparameter to retrieve the complete Credential object, which is appropriate for this test case that needs access to the full database record.backend/app/tests/api/routes/test_creds.py (3)
6-22: Excellent adoption of centralized test utilities.The refactored imports demonstrate consistent use of the new test data utilities, which aligns perfectly with the PR objectives of reducing redundancy and improving test consistency.
33-33: Good test function renaming for clarity.The rename from
test_set_creds_for_orgtotest_set_credentialimproves clarity and follows better naming conventions.
119-125: Proper use of enhanced credential retrieval.The test correctly leverages the new
full=Trueparameter inget_provider_credentialto retrieve the complete Credential object, which is necessary for accessing the organization_id used in subsequent API calls.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/tests/api/routes/test_creds.py(19 hunks)backend/app/tests/crud/test_credentials.py(5 hunks)backend/app/tests/utils/utils.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/tests/crud/test_credentials.py
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/app/tests/api/routes/test_creds.py
153-153: Unused function argument: db
(ARG001)
backend/app/tests/utils/utils.py
4-4: typing.List is deprecated, use list instead
(UP035)
4-4: typing.List imported but unused
Remove unused import: typing.List
(F401)
13-13: app.models.Credential imported but unused
Remove unused import: app.models.Credential
(F401)
⏰ 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)
🔇 Additional comments (12)
backend/app/tests/utils/utils.py (1)
30-32: LGTM! Well-implemented utility function.The
generate_random_stringfunction is correctly implemented and provides a useful utility for generating random alphanumeric strings for test data.backend/app/tests/api/routes/test_creds.py (11)
6-21: LGTM! Good refactoring to use centralized utilities.The import changes properly bring in the new utility functions and test data helpers, which will improve code reusability and consistency across the test suite.
28-29: LGTM! Improved fixture implementation.The fixture now uses the centralized
create_test_credentialfunction, which is more maintainable than custom inline setup.
32-33: LGTM! Clear function naming and proper test setup.The function was appropriately renamed from
test_set_creds_for_orgtotest_set_credentialfor clarity, and now uses the utility function to create test data.
67-68: LGTM! Proper use of utility functions.Creating separate organizations and projects for testing invalid relationships is the correct approach and using utility functions ensures consistency.
94-94: LGTM! Robust test data generation.Using
get_non_existent_idis a more robust approach than hardcoding IDs, as it ensures the ID truly doesn't exist in the database.
118-124: LGTM! Proper use of the enhanced CRUD function.The use of
get_provider_credentialwithfull=Truecorrectly retrieves the complete credential object, which aligns with the API enhancement mentioned in the summary.
187-193: LGTM! Consistent pattern for credential retrieval.The pattern of using
get_provider_credentialwithfull=Trueis consistently applied across the test suite, which is good for maintainability.
369-369: LGTM! Consistent use of test data utilities.Using
test_credential_dataprovides consistent test data generation and improves maintainability.
448-450: LGTM! Good test setup for encryption verification.The test properly sets up test data and extracts the original API key for comparison, which is essential for validating encryption functionality.
460-471: LGTM! Thorough encryption validation.The test correctly verifies that:
- The credential exists in the database
- The stored value is encrypted (different from original)
- The stored value can be decrypted back to the original
This provides comprehensive coverage of the encryption functionality.
168-171: Remove unused function argument.The
dbparameter is not used in this test function and should be removed.-def test_read_provider_credential_not_found( - db: Session, superuser_token_headers: dict[str, str] -): +def test_read_provider_credential_not_found(superuser_token_headers: dict[str, str]):Likely an incorrect or invalid review comment.
| from app.core.config import settings | ||
| from app.crud.user import get_user_by_email | ||
| from app.models import APIKeyPublic | ||
| from app.models import APIKeyPublic, Credential |
There was a problem hiding this comment.
Remove unused import.
The Credential import from app.models is not used in this file and should be removed.
-from app.models import APIKeyPublic, Credential
+from app.models import APIKeyPublic📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from app.models import APIKeyPublic, Credential | |
| from app.models import APIKeyPublic |
🧰 Tools
🪛 Ruff (0.11.9)
13-13: app.models.Credential imported but unused
Remove unused import: app.models.Credential
(F401)
🤖 Prompt for AI Agents
In backend/app/tests/utils/utils.py at line 13, the import statement includes
Credential from app.models, which is not used anywhere in the file. Remove
Credential from the import statement to clean up unused imports and improve code
clarity.
| import random | ||
| import string | ||
| from uuid import UUID | ||
| from typing import List |
There was a problem hiding this comment.
Remove unused import.
The List import from typing is not used anywhere in this file and should be removed. Additionally, typing.List is deprecated in favor of the built-in list type.
-from typing import List📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from typing import List |
🧰 Tools
🪛 Ruff (0.11.9)
4-4: typing.List is deprecated, use list instead
(UP035)
4-4: typing.List imported but unused
Remove unused import: typing.List
(F401)
🤖 Prompt for AI Agents
In backend/app/tests/utils/utils.py at line 4, remove the unused import of List
from typing since it is not used anywhere in the file and typing.List is
deprecated; replace any usage of typing.List with the built-in list type if
applicable.
| existing_credential = ( | ||
| db.query(Credential) | ||
| .filter(Credential.organization_id == org_without_creds.id) | ||
| .filter(credential.organization_id == org_without_credential.id) | ||
| .all() | ||
| ) | ||
| assert len(existing_creds) == 0 | ||
| assert len(existing_credential) == 0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve readability of the assertion logic.
The current filter logic is confusing. The intention seems to be checking that no credentials exist for the new organization, but the current implementation is unclear.
- existing_credential = (
- db.query(Credential)
- .filter(credential.organization_id == org_without_credential.id)
- .all()
- )
- assert len(existing_credential) == 0
+ existing_credentials = (
+ db.query(Credential)
+ .filter(Credential.organization_id == org_without_credential.id)
+ .all()
+ )
+ assert len(existing_credentials) == 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| existing_credential = ( | |
| db.query(Credential) | |
| .filter(Credential.organization_id == org_without_creds.id) | |
| .filter(credential.organization_id == org_without_credential.id) | |
| .all() | |
| ) | |
| assert len(existing_creds) == 0 | |
| assert len(existing_credential) == 0 | |
| existing_credentials = ( | |
| db.query(Credential) | |
| .filter(Credential.organization_id == org_without_credential.id) | |
| .all() | |
| ) | |
| assert len(existing_credentials) == 0 |
🤖 Prompt for AI Agents
In backend/app/tests/api/routes/test_creds.py around lines 234 to 239, the
filter condition uses 'credential.organization_id' which is unclear and likely
incorrect. Replace 'credential.organization_id' with
'Credential.organization_id' to properly reference the model's attribute in the
query filter. This change will clarify the intent and improve readability by
explicitly querying credentials belonging to the specified organization.
| *, | ||
| session: Session, | ||
| org_id: int, | ||
| provider: str, | ||
| project_id: Optional[int] = None, | ||
| full: bool = False, | ||
| ) -> Optional[Union[Dict[str, Any], Credential]]: | ||
| """ | ||
| Fetches credentials for a specific provider of a project. | ||
| Returns: | ||
| Optional[Union[Dict[str, Any], Credential]]: If full is True, returns full Credential object. | ||
| Otherwise returns just the decrypted credentials dict. | ||
| """ |
There was a problem hiding this comment.
is this formatted well?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/app/crud/credentials.py (1)
108-113: Fix WHERE clause logic issues (duplicate from past reviews).The WHERE clause still has the same critical issues that were flagged in previous reviews:
Credential.is_active == Trueshould be simplified toCredential.is_active- The conditional logic
Credential.project_id == project_id if project_id is not None else Trueis incorrect and will causeCredential.project_id == Truewhenproject_idis NoneApply this diff to fix the WHERE clause logic:
statement = select(Credential).where( Credential.organization_id == org_id, Credential.provider == provider, - Credential.is_active == True, - Credential.project_id == project_id if project_id is not None else True, + Credential.is_active, ) + if project_id is not None: + statement = statement.where(Credential.project_id == project_id)This approach conditionally adds the project_id filter only when needed, avoiding the problematic ternary expression in the WHERE clause.
🧹 Nitpick comments (1)
backend/app/crud/credentials.py (1)
1-1: Update deprecated type annotations.The import uses deprecated
typing.Dictandtyping.Listinstead of the moderndictandlistannotations.Apply this diff to use modern type annotations:
-from typing import Optional, Dict, Any, List, Union +from typing import Optional, Any, UnionThen update the return type annotation on line 97 to use
dictinstead ofDict[str, Any]:-) -> Optional[Union[Dict[str, Any], Credential]]: +) -> Optional[Union[dict[str, Any], Credential]]:For Python 3.10+, you can also use the union operator:
-) -> Optional[Union[dict[str, Any], Credential]]: +) -> Optional[dict[str, Any] | Credential]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/crud/credentials.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/crud/credentials.py (2)
backend/app/models/credentials.py (1)
Credential(46-90)backend/app/core/security.py (1)
decrypt_credentials(164-181)
🪛 Ruff (0.11.9)
backend/app/crud/credentials.py
1-1: typing.Dict is deprecated, use dict instead
(UP035)
1-1: typing.List is deprecated, use list instead
(UP035)
95-95: Use X | Y for type annotations
Convert to X | Y
(UP007)
97-97: Use X | Y for type annotations
Convert to X | Y
(UP007)
97-97: Use X | Y for type annotations
Convert to X | Y
(UP007)
97-97: Use dict instead of Dict for type annotation
Replace with dict
(UP006)
⏰ 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)
🔇 Additional comments (3)
backend/app/crud/credentials.py (3)
90-97: LGTM! Function signature enhancement looks good.The addition of the
fullparameter with proper default value and updated return type annotation correctly supports the new functionality.
98-105: LGTM! Clear and comprehensive docstring.The updated docstring properly documents the new parameter and return behavior.
117-117: LGTM! Conditional return logic is implemented correctly.The return logic properly handles both the full Credential object and decrypted credentials dictionary based on the
fullparameter.
3 participants
Summary
Target issue is #276
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
Added reusable test utility functions:
Summary by CodeRabbit
Tests
Chores
New Features