Credentials: Using Hard Delete & Updating is_active logic #360
Conversation
WalkthroughCredentials were re-scoped to include required project_id, soft-deletes were removed in favor of hard-deletes, and a DB unique constraint on (organization_id, project_id, provider) was added. CRUD, API routes, tests, seed data, and an Alembic migration were updated; Langfuse seed credentials and tracer mocking were introduced. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as Credentials API
participant CRUD as Credentials CRUD
participant DB as Database
rect rgb(246,249,255)
note over Client,DB: Create credential (project-scoped)
Client->>API: POST /credentials {org_id, project_id, provider, data}
API->>CRUD: set_creds_for_org(...)
CRUD->>DB: INSERT Credential (org_id, project_id, provider) UNIQUE
alt unique violation
DB-->>CRUD: IntegrityError
CRUD-->>API: HTTP 400 (duplicate)
API-->>Client: 400 Duplicate
else success
DB-->>CRUD: Row inserted
CRUD-->>API: Created Credential
API-->>Client: 200 Created
end
end
rect rgb(242,255,245)
note over Client,DB: Read provider / list
Client->>API: GET /credentials?org_id&project_id
API->>CRUD: get_creds_by_org(...)
CRUD-->>API: list[Credential] or HTTP 404
API-->>Client: 200 [] or list
Client->>API: GET /credentials/{provider}
API->>CRUD: get_provider_credential(..., raise_on_not_found=False)
CRUD-->>API: Credential | None
API-->>Client: 200 payload (possibly null)
end
rect rgb(255,247,240)
note over Client,DB: Hard delete
Client->>API: DELETE /credentials/{provider}
API->>CRUD: remove_provider_credential(...)
CRUD->>DB: DELETE FROM credential WHERE org_id, project_id, provider
DB-->>CRUD: rowcount
alt not found (rowcount 0)
CRUD-->>API: HTTP 404
API-->>Client: 404
else deleted
CRUD-->>API: None
API-->>Client: 200 {message}
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (3)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
backend/app/crud/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
backend/app/api/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (2)backend/app/crud/credentials.py (4)
backend/app/api/routes/credentials.py (4)
⏰ 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)
🔇 Additional comments (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
backend/app/api/routes/credentials.py (2)
58-60: Don’t raise bare Exception—use HTTPException.FastAPI won’t interpret a generic Exception with status_code/detail. This currently returns a 500 without your intended message.
- if not created_creds: - raise Exception(status_code=500, detail="Failed to create credentials") + if not created_creds: + raise HTTPException(status_code=500, detail="Failed to create credentials")
98-109: Enforce is_active filtering in get_provider_credential
get_provider_credential omitsCredential.is_active == True, so GET /provider/{provider} can return inactive secrets. Add anis_activefilter to its query to block disabled credentials.backend/app/crud/credentials.py (4)
146-151: Update flow excludes inactive records—may block legitimate updates.update_creds_for_org filters
is_active == True. If a record was deactivated historically, updates will 404. With hard delete, consider allowing updates regardless of is_active.- statement = select(Credential).where( - Credential.organization_id == org_id, - Credential.provider == creds_in.provider, - Credential.is_active == True, - Credential.project_id == project_id if project_id is not None else True, - ) + statement = select(Credential).where( + Credential.organization_id == org_id, + Credential.provider == creds_in.provider, + Credential.project_id == project_id if project_id is not None else True, + )
83-88: Enforce active filter in get_creds_by_org
AddCredential.is_active == Trueto the WHERE clause so that only active credentials are returned (consistent with other CRUD methods and removal tests).
109-115: Filter inactive credentials by default
- In
get_provider_credential(backend/app/crud/credentials.py≈lines 91–99), addCredential.is_active == Trueto the WHERE clause and introduceinclude_inactive: bool = Falseto opt-in inactive records.- In
get_creds_by_org(backend/app/crud/credentials.py≈lines 79–86), appendCredential.is_active == Trueto its query.- Update
get_providersto derive its list from only active credentials.
65-75: Decrypt credentials in get_key_by_org before accessing api_keyIn get_key_by_org (backend/app/crud/credentials.py lines 65–75),
creds.credentialis an encrypted string; you must calldecrypt_credentials(creds.credential)to obtain the dict before reading"api_key".- if creds and creds.credential and "api_key" in creds.credential: - return creds.credential["api_key"] + if creds and creds.credential: + decrypted = decrypt_credentials(creds.credential) + return decrypted.get("api_key")
🧹 Nitpick comments (4)
backend/app/alembic/versions/aaaca889d3ec_drop_deleted_at_and_is_active_from_.py (2)
1-7: Revision name mismatches behavior—clarify intent.Docstring/file name say “drop_deleted_at_and_is_active...”, but the migration only drops deleted_at and explicitly keeps is_active. Align naming to avoid confusion during ops/debugging.
Apply:
-"""drop_deleted_at_and_is_active_from_credential_table +"""drop_deleted_at_from_credential_table
20-27: Guard drop column with IF EXISTS in migration
UseALTER TABLE ... DROP COLUMN IF EXISTSto avoid failures if the column doesn’t exist:def upgrade(): # Drop only deleted_at column from credential table, keep is_active for flexibility - op.drop_column("credential", "deleted_at") + op.execute('ALTER TABLE "credential" DROP COLUMN IF EXISTS "deleted_at"')No other references to
credential.deleted_atfound inbackend/appafter grep.backend/app/crud/credentials.py (2)
123-126: Docstring mismatch.get_providers docstring says “active providers,” but it returns all providers without filtering.
Either update the docstring or filter by is_active.
196-201: Returning deleted model instances—prefer count or bool.Returning ORM instances that are now deleted can be misleading and fragile. Consider returning a count to the caller.
- session.commit() - return creds + session.commit() + return len(creds)You’d also adjust the route to check
deleted_count > 0.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
backend/app/alembic/versions/aaaca889d3ec_drop_deleted_at_and_is_active_from_.py(1 hunks)backend/app/api/routes/credentials.py(1 hunks)backend/app/crud/credentials.py(3 hunks)backend/app/models/credentials.py(0 hunks)backend/app/tests/api/routes/test_creds.py(0 hunks)
💤 Files with no reviewable changes (2)
- backend/app/tests/api/routes/test_creds.py
- backend/app/models/credentials.py
🧰 Additional context used
🧬 Code graph analysis (3)
backend/app/api/routes/credentials.py (1)
backend/app/tests/api/routes/test_creds.py (1)
test_delete_all_credentials(324-351)
backend/app/alembic/versions/aaaca889d3ec_drop_deleted_at_and_is_active_from_.py (5)
backend/app/alembic/versions/904ed70e7dab_added_provider_column_to_the_credential_.py (2)
upgrade(19-61)downgrade(64-93)backend/app/alembic/versions/e8ee93526b37_add_is_deleted_column_in_assistant_table.py (2)
upgrade(19-26)downgrade(29-31)backend/app/alembic/versions/0f205e3779ee_add_api_key_table.py (1)
upgrade(20-39)backend/app/alembic/versions/c43313eca57d_add_document_tables.py (1)
upgrade(20-36)backend/app/alembic/versions/4aa1f48c6321_add_inconistency_fixes.py (1)
upgrade(20-49)
backend/app/crud/credentials.py (3)
backend/app/crud/document.py (1)
delete(126-135)backend/app/tests/crud/test_credentials.py (3)
test_remove_creds_for_org(154-184)test_remove_provider_credential(127-151)test_update_creds_for_org(98-124)backend/app/models/credentials.py (1)
CredsBase(9-16)
⏰ 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/api/routes/credentials.py (2)
178-178: Docs update to “hard delete” looks good.The description now correctly reflects permanent deletion semantics.
195-197: No test updates needed: the API route test (backend/app/tests/api/routes/test_creds.py) already asserts"All credentials deleted successfully", matching the updated code.backend/app/crud/credentials.py (1)
180-184: Hard delete implementation LGTM.Straightforward and consistent with the PR’s “hard delete” direction.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/api/routes/credentials.py (1)
58-61: Raise HTTPException, not bare ExceptionRaising Exception(status_code=...) won’t produce a proper HTTP response.
- if not created_creds: - raise Exception(status_code=500, detail="Failed to create credentials") + if not created_creds: + raise HTTPException(status_code=500, detail="Failed to create credentials")backend/app/crud/credentials.py (1)
65-76: Decrypt before accessing api_key in get_key_by_orgCredentials are stored encrypted; checking for "api_key" on the stored blob will fail or be brittle. Decrypt first.
- if creds and creds.credential and "api_key" in creds.credential: - return creds.credential["api_key"] + if creds and creds.credential: + data = decrypt_credentials(creds.credential) + return data.get("api_key")
♻️ Duplicate comments (1)
backend/app/crud/credentials.py (1)
169-169: Return type changed to None — call sites/tests must not rely on returned modelThis is consistent with the new hard-delete semantics and updated tests.
🧹 Nitpick comments (6)
backend/app/tests/crud/test_credentials.py (1)
197-198: Bulk remove now returns empty list — LGTM; tiny assertion tweakAsserting directly on [] is a bit clearer than len(...) == 0.
- assert len(removed) == 0 + assert removed == []backend/app/api/routes/credentials.py (2)
98-104: Provider type consistency (Enum vs str)validate_provider may return an Enum; crud expects a string. Convert to the canonical value before passing down.
- credential = get_provider_credential( + provider_value = getattr(provider_enum, "value", provider_enum) + credential = get_provider_credential( session=session, org_id=_current_user.organization_id, - provider=provider_enum, + provider=provider_value, project_id=_current_user.project_id, )Apply the same pattern in delete_provider_credential (Lines 152-158) to avoid implicit Enum comparisons.
185-194: Avoid loading all rows just to check existenceget_creds_by_org fetches full rows. Consider an exists/count query to short-circuit and reduce memory/latency for orgs with many creds.
I can propose a tiny CRUD helper (exists_creds_for_org) using select(1).exists() if you want.
backend/app/crud/credentials.py (3)
124-126: Docstring vs implementation mismatchThe function returns providers for all credentials, not only “active”. Update the docstring or reintroduce an is_active filter; docstring tweak is simplest.
- """Returns a list of all active providers for which credentials are stored.""" + """Returns a list of all providers for which credentials are stored."""
146-151: Update path filters out inactive creds — confirm intended behaviorupdate_creds_for_org currently requires is_active == True. With hard deletes elsewhere, do you still want updates blocked for inactive rows? If not, drop the filter.
statement = select(Credential).where( Credential.organization_id == org_id, Credential.provider == creds_in.provider, - Credential.is_active == True, Credential.project_id == project_id if project_id is not None else True, )
197-201: Bulk delete for performanceLooping DELETEs can be slow. Use a single bulk delete.
- for cred in creds: - session.delete(cred) - - session.commit() - # Return empty list since we're doing hard deletes - return [] + from sqlalchemy import delete as sqla_delete + session.exec( + sqla_delete(Credential).where( + Credential.organization_id == org_id, + Credential.project_id == project_id if project_id is not None else True, + ) + ) + session.commit() + return []
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
backend/app/api/routes/credentials.py(1 hunks)backend/app/crud/credentials.py(3 hunks)backend/app/tests/crud/test_credentials.py(2 hunks)backend/app/tests/utils/test_data.py(0 hunks)
💤 Files with no reviewable changes (1)
- backend/app/tests/utils/test_data.py
🧰 Additional context used
🧬 Code graph analysis (3)
backend/app/tests/crud/test_credentials.py (1)
backend/app/tests/api/routes/test_creds.py (3)
test_delete_provider_credential(287-307)test_delete_all_credentials(324-351)test_delete_provider_credential_not_found(310-321)
backend/app/crud/credentials.py (2)
backend/app/crud/document.py (1)
delete(126-135)backend/app/models/credentials.py (1)
CredsBase(9-16)
backend/app/api/routes/credentials.py (4)
backend/app/models/user.py (1)
UserProjectOrg(68-69)backend/app/api/deps.py (1)
get_current_user_org_project(110-131)backend/app/crud/credentials.py (2)
get_creds_by_org(79-88)remove_creds_for_org(186-201)backend/app/tests/api/routes/test_creds.py (2)
test_delete_all_credentials(324-351)test_delete_all_credentials_not_found(354-364)
⏰ 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 (1)
159-160: Allremove_provider_credentialusages updated to expectNone
Verified via ripgrep that the only assignment occurs inbackend/app/tests/crud/test_credentials.pyand is correctly assertingNone.backend/app/api/routes/credentials.py (1)
178-178: Docs updated to “hard delete” — ensure tests and messages alignThe description now states hard delete. Make sure API route tests expect the new behavior and strings (see comment below on success message).
backend/app/crud/credentials.py (1)
180-183: Hard delete implementation — LGTMDirect deletion and commit are appropriate; existence is pre-checked at the route layer.
| return APIResponse.success_response( | ||
| {"message": "All credentials deleted successfully"} | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify success message with tests and other endpoints
Tests elsewhere expect "Credentials deleted successfully". Align message to reduce flakiness.
- return APIResponse.success_response(
- {"message": "All credentials deleted successfully"}
- )
+ return APIResponse.success_response({"message": "Credentials deleted successfully"})📝 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.
| return APIResponse.success_response( | |
| {"message": "All credentials deleted successfully"} | |
| ) | |
| return APIResponse.success_response({"message": "Credentials deleted successfully"}) |
🤖 Prompt for AI Agents
In backend/app/api/routes/credentials.py around lines 203 to 205, the returned
success message currently says "All credentials deleted successfully"; update
the response payload to use the exact message "Credentials deleted successfully"
so it matches tests and other endpoints; ensure only the string in the
APIResponse.success_response call is changed and preserved as the sole message
field.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/app/crud/credentials.py (3)
60-71: Bug: decrypt before reading api_key.
You encrypt the entire credentials object on write, but here you read creds.credential without decrypting, so "api_key" lookup can never succeed (or worse, leaks format assumptions).Apply:
- if creds and creds.credential and "api_key" in creds.credential: - return creds.credential["api_key"] - - return None + if not creds or not creds.credential: + return None + decrypted = decrypt_credentials(creds.credential) + return decrypted.get("api_key")
78-83: is_active semantics are inconsistent (risk of using deactivated creds).
get_creds_by_org and get_provider_credential no longer filter by is_active, but set/update still manage is_active and get_providers’ docstring says “active providers.” This can surface deactivated credentials.Two options; pick one and align tests/docs:
Option A (prefer): restore is_active filters.
@@ def get_creds_by_org(...): - statement = select(Credential).where( + statement = select(Credential).where( Credential.organization_id == org_id, + Credential.is_active == True, Credential.project_id == project_id if project_id is not None else True, ) @@ def get_provider_credential(...): - statement = select(Credential).where( + 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, )Option B: if returning all (active + inactive) is intentional, update wording:
@@ def get_providers(...): - """Returns a list of all active providers for which credentials are stored.""" + """Returns a list of all providers for which credentials are stored."""Also applies to: 104-109, 119-121
168-173: Add composite unique constraint on Credential(organization_id, provider, project_id). remove_provider_credential assumes only one row; enforce this at the DB level by adding a__table_args__UniqueConstraint inbackend/app/models/credential.pyand creating an Alembic migration.
♻️ Duplicate comments (1)
backend/app/crud/credentials.py (1)
162-164: API change to None return — OK and aligns with idempotent delete.
Past reviewer asked to update call sites/tests; PR notes indicate this was addressed.
🧹 Nitpick comments (3)
backend/app/crud/credentials.py (3)
168-172: Avoid where(True) pattern for optional project_id.
where(True) is unnecessary and can confuse compilers/linters. Build the clause conditionally.- statement = select(Credential).where( - Credential.organization_id == org_id, - Credential.provider == provider, - Credential.project_id == project_id if project_id is not None else True, - ) + statement = select(Credential).where( + Credential.organization_id == org_id, + Credential.provider == provider, + ) + if project_id is not None: + statement = statement.where(Credential.project_id == project_id)Apply similarly in other queries in this file.
181-189: Confirm FK cascade to prevent orphans on hard delete.
If other tables reference Credential, ensure ON DELETE CASCADE (or application-level cleanup) is in place before adopting hard deletes broadly.Would you like a follow-up patch to add/verify FK cascades in the model/migrations?
191-196: Use bulk delete; optionally return affected count.
Looping deletes issues one DELETE per row. Prefer a single bulk delete.- for cred in creds: - session.delete(cred) - - session.commit() - # Return empty list since we're doing hard deletes - return [] + # Bulk delete for efficiency + from sqlmodel import delete + del_stmt = delete(Credential).where( + Credential.organization_id == org_id, + Credential.project_id == project_id if project_id is not None else True, + ) + result = session.exec(del_stmt) + session.commit() + # Keep return type stable; consider returning result.rowcount instead. + return []If you’re open to a minor API tweak, return an int count and update tests accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
backend/app/alembic/versions/aaaca889d3ec_drop_deleted_at_and_is_active_from_.py(1 hunks)backend/app/crud/credentials.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/alembic/versions/aaaca889d3ec_drop_deleted_at_and_is_active_from_.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/crud/credentials.py (3)
backend/app/core/providers.py (2)
validate_provider(33-51)validate_provider_credentials(54-72)backend/app/tests/crud/test_credentials.py (2)
test_remove_provider_credential(142-166)test_remove_creds_for_org(169-203)backend/app/api/routes/credentials.py (1)
delete_provider_credential(146-171)
🔇 Additional comments (2)
backend/app/crud/credentials.py (2)
6-6: Import cleanup — LGTM.
Narrowing to only validate_provider and validate_provider_credentials is correct.
175-178: Hard delete implementation — LGTM.
The idempotent no-op when not found matches the route’s pre-check for 404.
|
The branch is named |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/app/alembic/versions/aaaca889d3ec_drop_deleted_at_and_is_active_from_.py (3)
1-7: Filename, revision prefix, and intent are inconsistent.Path suggests "aaaca889d3ec_drop_deleted_at_and_is_active...", but the revision in file is "7a0e8ab42c69" and the docstring mentions only dropping deleted_at. Recommend renaming to "7a0e8ab42c69_drop_deleted_at_from_credential_table.py" for clarity. No code change needed.
18-21: Prefer batch_alter_table for cross-DB safety (SQLite) and future-proofing.Using batch ops avoids DROP COLUMN issues on some SQLite setups.
Apply:
- op.drop_column("credential", "deleted_at") + with op.batch_alter_table("credential") as batch_op: + batch_op.drop_column("deleted_at")
18-21: Guard with existence checks to make the migration idempotent across environments.This helps on drifted DBs and avoids failures on reruns.
Apply:
def upgrade(): - # Drop only deleted_at column from credential table, keep is_active for flexibility - op.drop_column("credential", "deleted_at") + # Drop only deleted_at column from credential table, keep is_active for flexibility + bind = op.get_bind() + insp = sa.inspect(bind) + cols = [c["name"] for c in insp.get_columns("credential")] + if "deleted_at" in cols: + with op.batch_alter_table("credential") as batch_op: + batch_op.drop_column("deleted_at") def downgrade(): - # Add back deleted_at column to credential table - op.add_column("credential", sa.Column("deleted_at", sa.DateTime(), nullable=True)) + # Add back deleted_at column to credential table (schema-only) + bind = op.get_bind() + insp = sa.inspect(bind) + cols = [c["name"] for c in insp.get_columns("credential")] + if "deleted_at" not in cols: + with op.batch_alter_table("credential") as batch_op: + batch_op.add_column(sa.Column("deleted_at", sa.DateTime(), nullable=True))Also applies to: 23-26
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/app/alembic/versions/aaaca889d3ec_drop_deleted_at_and_is_active_from_.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/alembic/versions/aaaca889d3ec_drop_deleted_at_and_is_active_from_.py (5)
backend/app/alembic/versions/904ed70e7dab_added_provider_column_to_the_credential_.py (2)
upgrade(19-61)downgrade(64-93)backend/app/alembic/versions/0f205e3779ee_add_api_key_table.py (1)
upgrade(20-39)backend/app/alembic/versions/c43313eca57d_add_document_tables.py (1)
upgrade(20-36)backend/app/alembic/versions/e8ee93526b37_add_is_deleted_column_in_assistant_table.py (2)
upgrade(19-26)downgrade(29-31)backend/app/alembic/versions/4aa1f48c6321_add_inconistency_fixes.py (1)
upgrade(20-49)
⏰ 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/alembic/versions/aaaca889d3ec_drop_deleted_at_and_is_active_from_.py (3)
23-26: Downgrade recreates the column but not data/indexes—confirm this is acceptable.You won’t recover prior deleted_at values or any related indexes. If auditability matters, consider archiving before drop.
18-21: Core change aligns with hard-delete move. LGTM.Straightforward schema drop; rest of the PR should ensure code no longer references deleted_at.
18-21: No dependent references to credential.deleted_at found
Ranrgacrossbackend/appand all Alembic versions—no indexes, constraints, or other usages ofcredential.deleted_atbeyond this migration. Safe to drop the column.
| Revision ID: 7a0e8ab42c69 | ||
| Revises: 40307ab77e9f | ||
| Create Date: 2025-09-01 21:52:33.293932 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify migration chain and filename prefix vs. revision id.
Ensure down_revision "40307ab77e9f" exists and filenames match their revision to avoid confusion.
Also applies to: 11-15
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import glob, re, os
root="backend/app/alembic/versions"
re_rev=re.compile(r'^\s*revision\s*=\s*["\']([0-9a-f]+)["\']', re.M)
re_down=re.compile(r'^\s*down_revision\s*=\s*["\']([0-9a-f]+|None)["\']', re.M)
re_name=re.compile(r'/([0-9a-f]{12,})_')
revs={}
for f in glob.glob(os.path.join(root,"*.py")):
t=open(f,encoding="utf-8").read()
m1=re_rev.search(t); m2=re_down.search(t)
if m1 and m2:
rev,down=m1.group(1),m2.group(1)
revs[rev]=dict(file=f,down=down)
m3=re_name.search(f.replace('\\','/'))
if m3 and m3.group(1)!=rev:
print(f"MISMATCH filename prefix {m3.group(1)} != revision {rev} in {f}")
cur="7a0e8ab42c69"
print(f"{cur} down_revision -> {revs.get(cur,{}).get('down')}")
if revs.get(cur,{}).get('down') not in revs:
print("WARNING: down_revision not found")
PYLength of output: 334
Align migration filenames to their revision values
Rename the two mismatched files to match their in-file revision IDs:
- backend/app/alembic/versions/aaaca889d3ec_drop_deleted_at_and_is_active_from_.py → 7a0e8ab42c69_drop_deleted_at_and_is_active_from_.py
- backend/app/alembic/versions/8eefcfedc409_create_assistant_table.py → 8757b005d681_create_assistant_table.py
(The down_revision 40307ab77e9f for 7a0e8ab42c69 is present, so the chain is intact.)
🤖 Prompt for AI Agents
In
backend/app/alembic/versions/aaaca889d3ec_drop_deleted_at_and_is_active_from_.py
around lines 3-5 the in-file Revision ID is 7a0e8ab42c69 while the filename uses
aaaca889d3ec; rename that file to
7a0e8ab42c69_drop_deleted_at_and_is_active_from_.py so the filename matches the
revision ID, and also rename
backend/app/alembic/versions/8eefcfedc409_create_assistant_table.py to
8757b005d681_create_assistant_table.py to match its in-file revision; ensure Git
moves/renames are used (git mv) and verify no imports or references rely on the
old filenames and that Alembic down_revision links remain intact.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/crud/credentials.py (2)
80-83: Bug: get_key_by_org must decrypt before returning api_keyYou encrypt on write but don't decrypt here; this will return None or ciphertext. Decrypt first.
Apply this diff:
- if creds and creds.credential and "api_key" in creds.credential: - return creds.credential["api_key"] - - return None + if not creds or not creds.credential: + return None + decrypted = decrypt_credentials(creds.credential) + return decrypted.get("api_key")
128-133: Docstring vs behavior: get_providers returns all, not only activeDocstring says "active providers" but you now include inactive ones via get_creds_by_org. Filter to active or update the docstring.
Minimal fix to preserve current contract:
- creds = get_creds_by_org(session=session, org_id=org_id, project_id=project_id) - return [cred.provider for cred in creds] + creds = get_creds_by_org(session=session, org_id=org_id, project_id=project_id) + return [cred.provider for cred in creds if cred.is_active]
♻️ Duplicate comments (1)
backend/app/crud/credentials.py (1)
179-181: Signature change to None — call sites/tests updated?Looks good given PR notes; just double-check no usages expect a return model.
🧹 Nitpick comments (6)
backend/app/crud/credentials.py (6)
192-195: Harden delete: add rollback/logging and idempotent not-found pathCurrent code silently no-ops when not found and lacks rollback on failure. Add logging and safe transaction handling.
- if creds: - # Hard delete - remove from database - session.delete(creds) - session.commit() + if not creds: + logger.info( + f"[remove_provider_credential] No credentials found | organization_id {org_id}, provider {provider}, project_id {project_id}" + ) + return + try: + # Hard delete - remove from database + session.delete(creds) + session.commit() + logger.info( + f"[remove_provider_credential] Removed credentials | organization_id {org_id}, provider {provider}, project_id {project_id}" + ) + except Exception as e: + session.rollback() + logger.error( + f"[remove_provider_credential] Delete failed | organization_id {org_id}, provider {provider}, project_id {project_id}, error: {e}", + exc_info=True, + ) + raise
202-216: Prefer bulk delete over row-by-row loop; wrap in transactionBulk delete avoids loading rows and is faster. Also add rollback/logging.
Add import (outside this hunk):
from sqlalchemy import deleteReplace the body with:
- creds = session.exec(statement).all() - - for cred in creds: - session.delete(cred) - - session.commit() - # Return empty list since we're doing hard deletes - logger.info( - f"[remove_creds_for_org] Successfully removed all the credentials | organization_id {org_id}, project_id {project_id}" - ) - - return [] + try: + result = session.exec( + delete(Credential).where( + Credential.organization_id == org_id, + Credential.project_id == project_id if project_id is not None else True, + ) + ) + session.commit() + logger.info( + f"[remove_creds_for_org] Hard-deleted {getattr(result, 'rowcount', 0) or 0} credentials | organization_id {org_id}, project_id {project_id}" + ) + return [] + except Exception as e: + session.rollback() + logger.error( + f"[remove_creds_for_org] Bulk delete failed | organization_id {org_id}, project_id {project_id}, error: {e}", + exc_info=True, + ) + raise
27-31: Canonicalize provider to a single casing/valueYou validate but store/query the raw string; mismatched casing will miss rows. Canonicalize using the validator’s enum value.
- # Validate provider and credentials - validate_provider(provider) - validate_provider_credentials(provider, credentials) + # Validate + canonicalize provider and credentials + provider_enum = validate_provider(provider) + provider = provider_enum.value + validate_provider_credentials(provider, credentials)
64-71: Also canonicalize provider in get_key_by_orgTo avoid casing mismatches in queries.
- """Fetches the API key from the credentials for the given organization and provider.""" + """Fetches the API key from the credentials for the given organization and provider.""" + provider = validate_provider(provider).value
217-217: Add trailing newline (Ruff W292)Keep files POSIX-friendly and lint-clean.
192-195: Hard delete removes audit trail—consider minimal auditingGiven the switch to hard deletes, consider emitting structured audit logs or writing to an audit table to preserve who/when/what was removed.
Also applies to: 208-216
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/api/routes/credentials.py(1 hunks)backend/app/crud/credentials.py(4 hunks)backend/app/tests/utils/test_data.py(0 hunks)
💤 Files with no reviewable changes (1)
- backend/app/tests/utils/test_data.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/api/routes/credentials.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/crud/credentials.py (2)
backend/app/core/providers.py (2)
validate_provider(36-57)validate_provider_credentials(60-81)backend/app/crud/document.py (1)
delete(125-135)
🪛 Ruff (0.12.2)
backend/app/crud/credentials.py
217-217: No newline at end of file
Add trailing newline
(W292)
🔇 Additional comments (4)
backend/app/crud/credentials.py (4)
7-7: Import consolidation LGTMCombining the provider validators import is fine.
86-95: Confirm intent: is_active filter removed in get_creds_by_orgReturning inactive creds changes semantics and may surface disabled secrets to callers that expect only active ones (e.g., get_providers). If intentional, please confirm all call sites are okay with this broader result.
153-159: Update semantics still require is_active == TrueUpdate path filters active-only, while reads may include inactive. Ensure this is the intended invariant (only active creds are mutable), or adjust for consistency.
114-116: I’ve provided a script to verify thatget_provider_credentialno longer filters out inactive credentials, while other functions still do. Run it to confirm and then decide whether to reintroduce theis_active == Trueclause.
| def downgrade(): | ||
| # Add back deleted_at column to credential table | ||
| op.add_column("credential", sa.Column("deleted_at", sa.DateTime(), nullable=True)) |
There was a problem hiding this comment.
Restore deleted_at column definition in downgrade
The downgrade path should recreate the schema exactly as it existed before this migration. In backend/app/alembic/versions/543f97951bd0_add_credential_table.py, credential.deleted_at is declared as a timezone-aware DateTime. Reintroducing it here without timezone=True leaves the downgraded schema mismatched, which will show up in subsequent autogenerates and can break code expecting a tz-aware column. Please mirror the original definition.
- op.add_column("credential", sa.Column("deleted_at", sa.DateTime(), nullable=True))
+ op.add_column(
+ "credential",
+ sa.Column("deleted_at", sa.DateTime(timezone=True), nullable=True),
+ )📝 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.
| def downgrade(): | |
| # Add back deleted_at column to credential table | |
| op.add_column("credential", sa.Column("deleted_at", sa.DateTime(), nullable=True)) | |
| def downgrade(): | |
| # Add back deleted_at column to credential table | |
| op.add_column( | |
| "credential", | |
| sa.Column("deleted_at", sa.DateTime(timezone=True), nullable=True), | |
| ) |
🤖 Prompt for AI Agents
In backend/app/alembic/versions/6dcbc94dc165_add_new_column_credentials.py
around lines 25-27, the downgrade re-adds credential.deleted_at as sa.DateTime()
without timezone; update the op.add_column call to recreate the column with
sa.DateTime(timezone=True) so the downgraded schema exactly matches the original
migration (credential.deleted_at was timezone-aware).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/crud/credentials.py (1)
71-83: Bug: get_key_by_org reads encrypted blob without decrypting.set_creds_for_org encrypts credentials; this function checks for 'api_key' on the encrypted payload and will almost always return None. Decrypt before accessing.
Apply this diff:
- creds = session.exec(statement).first() - - if creds and creds.credential and "api_key" in creds.credential: - return creds.credential["api_key"] - - return None + creds = session.exec(statement).first() + if not creds or not creds.credential: + return None + decrypted = decrypt_credentials(creds.credential) + return decrypted.get("api_key")
🧹 Nitpick comments (3)
backend/app/crud/credentials.py (3)
131-133: Docstring mismatch: function returns all providers, not only active.- """Returns a list of all active providers for which credentials are stored.""" + """Returns a list of all providers for which credentials are stored."""
208-211: Use bulk delete for fewer round-trips.Looping deletes each row individually; prefer a single bulk delete.
- creds = session.exec(statement).all() - - for cred in creds: - session.delete(cred) + session.exec( + delete(Credential).where( + Credential.organization_id == org_id, + Credential.project_id == project_id if project_id is not None else True, + ) + )Add import (outside this hunk):
from sqlalchemy import delete
212-218: remove_creds_for_org return value unused in production Only tests capture the empty list; no other callers assign or rely on the returned credentials. Consider returning a deletion count instead of an empty list for observability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/crud/credentials.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/crud/credentials.py (2)
backend/app/core/providers.py (2)
validate_provider(36-57)validate_provider_credentials(60-81)backend/app/crud/document.py (1)
delete(125-135)
⏰ 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/credentials.py (4)
7-7: Import consolidation LGTM.
153-159: Confirm is_active constraint on updates.update_creds_for_org filters by is_active == True, while reads now ignore is_active. Is this intentional (i.e., only allow updating active creds)? If not, drop the filter.
Optional diff if you intend to update regardless of active state:
statement = select(Credential).where( Credential.organization_id == org_id, Credential.provider == creds_in.provider, - Credential.is_active == True, Credential.project_id == project_id if project_id is not None else True, )
179-181: Return type change to None—reconfirm call sites/tests updated.This was previously flagged and marked addressed. Re-run a quick check to avoid regressions.
#!/bin/bash # Find assignments/usages expecting a return value rg -nP '\b[\w\[\]\.]+\s*=\s*remove_provider_credential\s*\(' -C2
192-195: Hard delete logic LGTM.
| project_id=_current_user.project_id, | ||
| ) | ||
| if not creds: | ||
| if not existing_creds: |
There was a problem hiding this comment.
this logging and raising http exception part can be moved to the "get_creds_by_org" crud function maybe? if we are looking at writing code in a way that we don't crowd the router code as much
There was a problem hiding this comment.
@nishika26
get CRUD operations can return None, and that’s expected behavior. Logging this inside the get function isn’t necessary. If a specific use case requires logging when None is returned, we should handle that logging outside the CRUD function instead.
LMK if I am missing something.
backend/app/crud/credentials.py
Outdated
| *, session: Session, org_id: int, project_id: Optional[int] = None | ||
| ) -> List[Credential]: | ||
| """Removes all credentials for an organization.""" | ||
| statement = select(Credential).where( |
There was a problem hiding this comment.
or you can use "get creds by org" check inside the remove creds by org crud function only
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/tests/api/routes/test_creds.py (1)
228-254: Missing assertions after delete operation.The delete operation on lines 250-253 doesn't assert the response status or verify the deletion succeeded. Tests should always verify the expected outcome.
Apply this diff to add assertions:
client.delete( f"{settings.API_V1_STR}/credentials/provider/{Provider.OPENAI.value}", headers={"X-API-KEY": user_api_key.key}, ) + + # Verify delete succeeded by checking credential no longer exists + response = client.get( + f"{settings.API_V1_STR}/credentials/provider/{Provider.OPENAI.value}", + headers={"X-API-KEY": user_api_key.key}, + ) + assert response.status_code == 404
🧹 Nitpick comments (1)
backend/app/tests/api/routes/test_creds.py (1)
31-36: Consider using database fixtures for test isolation.Calling delete in the test setup creates interdependency—if the delete endpoint has a bug, this test will fail for the wrong reason. Consider using database fixtures or transaction rollbacks (which appear to be configured in conftest.py) to ensure clean state instead of relying on API calls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/tests/api/routes/test_creds.py(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/tests/api/routes/test_creds.py (5)
backend/app/tests/conftest.py (3)
client(52-55)user_api_key(89-91)db(24-41)backend/app/core/providers.py (1)
Provider(9-14)backend/app/tests/utils/utils.py (1)
generate_random_string(25-26)backend/app/tests/utils/test_data.py (1)
test_credential_data(82-100)backend/app/core/security.py (1)
decrypt_credentials(164-181)
🪛 Ruff (0.13.3)
backend/app/tests/api/routes/test_creds.py
437-437: Unused function argument: db
(ARG001)
⏰ 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/tests/api/routes/test_creds.py (4)
333-333: Good use of modern Pydantic API.Using
model_dump()instead ofdict()is correct for Pydantic v2 and provides better type safety.
409-433: LGTM! Thorough encryption verification.The test properly verifies:
- Credentials are encrypted at rest (stored as string)
- Decryption works correctly
- Decrypted structure contains expected fields
170-170: Substring matching for errors is acceptable here.Using
infor error message assertions (also at lines 269, 312-315) is more flexible than exact matching and allows for error message refinements without breaking tests. However, be aware this won't catch if the error semantics change significantly.Also applies to: 269-269, 312-315
151-154: Replace fallback with directdataaccess
APIResponse always wraps the payload under"data". In tests, dropresponse_data.get("data", response_data)and useresponse.json()["data"], and optionally assertresponse.json()["success"] is True.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/alembic/versions/570d18bd53e1_add_is_deleted_column_in_assistant_table.py (1)
1-27: Critical contradiction: Soft-delete implementation conflicts with PR objectives.The PR objectives explicitly state "Using Hard Delete & Updating is_active logic" and the AI summary confirms the PR "removes soft-deletion in favor of hard deletes." However, this migration adds
is_deletedanddeleted_atcolumns, which implement a soft-delete pattern, not hard delete.Additionally, the filename and docstring claim to add "column" (singular), but the migration adds two columns (
is_deletedanddeleted_at).Please clarify:
- Should this migration be reverted/removed if the assistant table should use hard deletes?
- If soft-delete is intentional for assistants (unlike credentials), update the filename, docstring, and PR description to reflect this distinction.
- If this migration was added by mistake, consider whether the assistant table needs any changes at all in this PR.
♻️ Duplicate comments (2)
backend/app/alembic/versions/e8a361613a87_drop_deleted_at_credentials.py (2)
32-32: Restoredeleted_atwith timezone-aware DateTime in downgrade.The downgrade should recreate the schema exactly as it existed before this migration. According to the original migration
543f97951bd0_add_credential_table.py(referenced in past reviews),credential.deleted_atwas defined assa.DateTime(timezone=True). Reintroducing it here withouttimezone=Truecreates a schema mismatch that will surface in subsequent autogenerates and may break code expecting a timezone-aware column.Apply this diff:
- op.add_column("credential", sa.Column("deleted_at", sa.DateTime(), nullable=True)) + op.add_column( + "credential", + sa.Column("deleted_at", sa.DateTime(timezone=True), nullable=True), + )
19-19: Remove misleading comment aboutis_active.The comment states "keep is_active for flexibility" but this migration doesn't modify or reference
is_activeat all—it only dropsdeleted_atand adds a unique constraint. This could confuse future maintainers about the migration's actual scope.Apply this diff:
- # Drop only deleted_at column from credential table, keep is_active for flexibility + # Drop deleted_at column from credential table to support hard deletes
🧹 Nitpick comments (1)
backend/app/alembic/versions/570d18bd53e1_add_is_deleted_column_in_assistant_table.py (1)
10-10: Remove unused import of sqlmodel.sql.sqltypes. No SQLModel types are referenced in this migration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/alembic/versions/570d18bd53e1_add_is_deleted_column_in_assistant_table.py(1 hunks)backend/app/alembic/versions/e8a361613a87_drop_deleted_at_credentials.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/alembic/versions/e8a361613a87_drop_deleted_at_credentials.pybackend/app/alembic/versions/570d18bd53e1_add_is_deleted_column_in_assistant_table.py
🔇 Additional comments (2)
backend/app/alembic/versions/570d18bd53e1_add_is_deleted_column_in_assistant_table.py (1)
21-27: Verify data migration requirements.The migration adds
is_deletedwith a server default offalseanddeleted_atas nullable. If theopenai_assistanttable contains existing data, all rows will receiveis_deleted=falseanddeleted_at=NULL.Confirm that:
- The
openai_assistanttable is expected to have existing data when this migration runs.- Setting all existing records to
is_deleted=falseis the correct behavior.- No backfill or data transformation is needed for existing records.
backend/app/alembic/versions/e8a361613a87_drop_deleted_at_credentials.py (1)
23-27: Verifyprovidercolumn nullability in thecredentialtable
Confirm in the initial Alembic migration under backend/app/alembic/versions that theprovidercolumn is declared with nullable=False.
AkhileshNegi
force-pushed
the
hotfix/credentials-updates
branch
from
9e29890 to
e4d939a
Compare
backend/app/crud/credentials.py
Outdated
| project_id: Optional[int] = None, | ||
| full: bool = False, | ||
| ) -> Optional[Union[Dict[str, Any], Credential]]: | ||
| raise_on_not_found: bool = True, |
There was a problem hiding this comment.
This should not be in arguments, either raise or not
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/crud/credentials.py (2)
89-94: Bug: decrypt before reading api_key in get_key_by_orgCredential.credential is an encrypted string. Current code treats it like a dict, so "api_key" lookup never works. Decrypt first, then access.
Apply this diff:
- creds = session.exec(statement).one_or_none() - - if creds and creds.credential and "api_key" in creds.credential: - return creds.credential["api_key"] - - return None + creds = session.exec(statement).one_or_none() + if not creds or not creds.credential: + return None + try: + decrypted = decrypt_credentials(creds.credential) + except ValueError as e: + logger.error( + f"[get_key_by_org] Failed to decrypt credentials | organization_id {org_id}, project_id {project_id}, provider {provider}: {e}" + ) + return None + return decrypted.get("api_key")
46-51: Make set_creds_for_org atomic (avoid per-item commits)Committing in the loop can leave partial inserts if a later provider fails (e.g., IntegrityError). Commit once after processing all providers; rollback on any error.
Apply this diff:
- session.add(credential) - session.commit() - session.refresh(credential) - created_credentials.append(credential) + session.add(credential) + # Defer commit; we'll commit once after the loop to ensure atomicity + created_credentials.append(credential) @@ - ) + ) # Check if it's a duplicate constraint violation @@ - ) + ) raise ValueError( f"Error while adding credentials for provider {provider}: {str(e)}" ) - logger.info( + # Commit once after all providers, so it's all-or-nothing + session.commit() + for cred in created_credentials: + session.refresh(cred) + logger.info( f"[set_creds_for_org] Successfully created credentials | organization_id {organization_id}, project_id {project_id}" )Also applies to: 69-72
♻️ Duplicate comments (1)
backend/app/crud/credentials.py (1)
113-118: Align is_active filtering across reads (docstring vs queries)get_providers doc says “active providers,” but get_creds_by_org and get_provider_credential don’t filter by is_active. Update queries or adjust docs.
Apply this diff to filter consistently:
def get_creds_by_org( @@ - statement = select(Credential).where( + statement = select(Credential).where( Credential.organization_id == org_id, - Credential.project_id == project_id, + Credential.project_id == project_id, + Credential.is_active.is_(True), ) @@ def get_provider_credential( @@ - statement = select(Credential).where( + statement = select(Credential).where( Credential.organization_id == org_id, Credential.provider == provider, - Credential.project_id == project_id, + Credential.project_id == project_id, + Credential.is_active.is_(True), )If inactive credentials must be readable, instead remove the is_active filter from update_creds_for_org. Which behavior do you want?
Also applies to: 162-168, 183-186
🧹 Nitpick comments (8)
backend/app/crud/credentials.py (4)
242-249: Avoid TOCTOU; treat zero-row delete as 404; drop pre-checkThe pre-check plus delete has a race. Delete once and, if rowcount == 0, return 404 (resource not found), not 500.
Apply this diff:
- # Verify credentials exist before attempting delete - get_provider_credential( - session=session, - org_id=org_id, - project_id=project_id, - provider=provider, - ) - # Build delete statement statement = delete(Credential).where( Credential.organization_id == org_id, Credential.provider == provider, Credential.project_id == project_id, ) @@ - if rows_deleted == 0: + if rows_deleted == 0: session.rollback() logger.error( f"[remove_provider_credential] Failed to delete credential | organization_id {org_id}, provider {provider}, project_id {project_id}" ) - raise HTTPException( - status_code=500, - detail="Failed to delete provider credential", - ) + raise HTTPException( + status_code=404, + detail=f"Credentials not found for provider '{provider}'", + )Also applies to: 251-270
51-66: IntegrityError detection via message string is brittleChecking "unique constraint" substrings may miss driver-specific messages. Prefer constraint-aware checks (e.g., psycopg2’s e.orig.diag.constraint_name) and safe fallbacks.
Example:
from psycopg2.errors import UniqueViolation # if using Postgres except IntegrityError as e: session.rollback() constraint = getattr(getattr(e, "orig", None), "diag", None) name = getattr(constraint, "constraint_name", None) if isinstance(getattr(e, "orig", None), UniqueViolation) or name == "uq_credential_org_project_provider": raise HTTPException( status_code=400, detail=f"Credentials for provider '{provider}' already exist for this organization and project combination", ) raiseIf SQLite in tests, also handle "UNIQUE constraint failed: ..." in str(e).lower() as a fallback.
75-82: Optional: validate provider in get_key_by_orgFor consistency with other entry points, validate provider before querying.
def get_key_by_org( @@ - provider: str = "openai", + provider: str = "openai", ) -> str | None: @@ - """Fetches the API key from the credentials for the given organization and provider.""" + """Fetches the API key from the credentials for the given organization and provider.""" + validate_provider(provider)
276-309: Optional: wrap bulk delete in an explicit transactionUsing session.begin() ensures atomicity if the expected_count check fails; you can rollback without relying on implicit transaction boundaries.
with session.begin(): result = session.exec(statement) rows_deleted = result.rowcount if rows_deleted < expected_count: logger.error("...") raise HTTPException(status_code=500, detail="Failed to delete all credentials") # No need for a separate session.commit()backend/app/tests/api/routes/test_creds.py (4)
19-22: Remove unused fixture create_test_credentialsDefined but never used in this module.
Apply this diff:
-@pytest.fixture -def create_test_credentials(db: Session): - return create_test_credential(db) +
126-126: Drop unused db parameter from testsdb is not used in these tests; remove to satisfy linters and reduce fixture churn.
-def test_read_credentials_not_found( - client: TestClient, db: Session, user_api_key: APIKeyPublic +def test_read_credentials_not_found( + client: TestClient, user_api_key: APIKeyPublic ): @@ -def test_read_provider_credential_not_found( - client: TestClient, db: Session, user_api_key: APIKeyPublic +def test_read_provider_credential_not_found( + client: TestClient, user_api_key: APIKeyPublic ): @@ -def test_update_credentials_not_found_for_provider( - client: TestClient, db: Session, user_api_key: APIKeyPublic +def test_update_credentials_not_found_for_provider( + client: TestClient, user_api_key: APIKeyPublic ): @@ -def test_delete_provider_credential_not_found( - client: TestClient, db: Session, user_api_key: APIKeyPublic +def test_delete_provider_credential_not_found( + client: TestClient, user_api_key: APIKeyPublic ): @@ -def test_delete_all_credentials_not_found( - client: TestClient, db: Session, user_api_key: APIKeyPublic +def test_delete_all_credentials_not_found( + client: TestClient, user_api_key: APIKeyPublic ): @@ -def test_credential_encryption( - client: TestClient, - db: Session, - user_api_key: APIKeyPublic, -): +def test_credential_encryption( + client: TestClient, + db: Session, + user_api_key: APIKeyPublic, +):Note: Keep db only where it's actually used (e.g., test_credential_encryption). Based on static analysis hints.
Also applies to: 158-158, 203-203, 257-257, 300-300, 411-411
232-253: Assert delete success and verify removalStrengthen the test by asserting DELETE status and confirming provider is gone.
- client.delete( + resp = client.delete( f"{settings.API_V1_STR}/credentials/provider/{Provider.OPENAI.value}", headers={"X-API-KEY": user_api_key.key}, ) + assert resp.status_code == 200 + # Verify subsequent GET returns 404 + get_resp = client.get( + f"{settings.API_V1_STR}/credentials/provider/{Provider.OPENAI.value}", + headers={"X-API-KEY": user_api_key.key}, + ) + assert get_resp.status_code == 404
108-112: Optional: assert POST succeeded before GET in test_read_credentials_with_credsIf the POST failed (e.g., due to duplicates), the subsequent GET may mask it.
- client.post( + post_resp = client.post( f"{settings.API_V1_STR}/credentials/", json=payload, headers={"X-API-KEY": user_api_key.key}, ) + assert post_resp.status_code in (200, 400) # 400 allowed if duplicate exists
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/crud/credentials.py(7 hunks)backend/app/tests/api/routes/test_creds.py(9 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/tests/api/routes/test_creds.pybackend/app/crud/credentials.py
backend/app/crud/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement database access operations in backend/app/crud/
Files:
backend/app/crud/credentials.py
🧬 Code graph analysis (2)
backend/app/tests/api/routes/test_creds.py (5)
backend/app/tests/conftest.py (3)
client(61-64)user_api_key(98-107)db(24-41)backend/app/core/providers.py (1)
Provider(9-14)backend/app/tests/utils/utils.py (1)
generate_random_string(25-26)backend/app/tests/utils/test_data.py (1)
test_credential_data(82-99)backend/app/core/security.py (1)
decrypt_credentials(164-181)
backend/app/crud/credentials.py (4)
backend/app/core/providers.py (2)
validate_provider(36-57)validate_provider_credentials(60-81)backend/app/core/security.py (2)
decrypt_credentials(164-181)encrypt_credentials(144-161)backend/app/core/util.py (1)
now(15-16)backend/app/models/credentials.py (3)
Credential(48-97)CredsCreate(19-29)CredsUpdate(32-45)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/app/alembic/versions/27c271ab6dd0_drop_deleted_at_credentials.py (2)
21-21: Misleading comment should be corrected.The comment mentions "keep is_active for flexibility," but this migration does not modify
is_activeat all—it only dropsdeleted_atand adds a unique constraint. This was flagged previously but appears to still be present.Apply this diff:
- # Drop only deleted_at column from credential table, keep is_active for flexibility + # Drop deleted_at column from credential table to support hard deletes
32-34: Restoredeleted_atcolumn with correct timezone setting.The downgrade path must recreate the schema exactly as it existed before this migration. In the original migration (
backend/app/alembic/versions/543f97951bd0_add_credential_table.py),credential.deleted_atwas declared assa.DateTime(timezone=True). Reintroducing it here withouttimezone=Truecreates a schema mismatch that will appear in subsequent autogenerates and can break code expecting a timezone-aware column.Apply this diff:
- op.add_column("credential", sa.Column("deleted_at", sa.DateTime(), nullable=True)) + op.add_column( + "credential", + sa.Column("deleted_at", sa.DateTime(timezone=True), nullable=True), + )
🧹 Nitpick comments (3)
backend/app/alembic/versions/27c271ab6dd0_drop_deleted_at_credentials.py (3)
10-10: Remove unused import.The
sqlmodel.sql.sqltypesimport is not used anywhere in this migration and can be safely removed.Apply this diff:
from alembic import op import sqlalchemy as sa -import sqlmodel.sql.sqltypes
20-20: Add return type hint.Per the coding guidelines, functions should include type hints. The
upgrade()function should be annotated with-> None.Apply this diff:
-def upgrade(): +def upgrade() -> None:As per coding guidelines.
32-32: Add return type hint.Per the coding guidelines, functions should include type hints. The
downgrade()function should be annotated with-> None.Apply this diff:
-def downgrade(): +def downgrade() -> None:As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/alembic/versions/27c271ab6dd0_drop_deleted_at_credentials.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/alembic/versions/27c271ab6dd0_drop_deleted_at_credentials.py
⏰ 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)
4 participants
Summary
Target issue is #361
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
New Features
Bug Fixes
Database
Documentation