fix: deprecation warnings and bug fixes #146
Conversation
WalkthroughIntroduces a new IntegrationNotFoundError in the integration service and updates API handlers to catch it; maps ValueError to 400 in some paths; adds Pydantic v2 model_config to IndexedRepository; adds WebSocket keep-alive params to server start; adjusts Discord bot heartbeat and updates multiple dependencies and subproject pyproject. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant API as integrations API
participant Service as integration_service
participant DB as persistence
API->>Service: update_integration(id, data)
Service->>DB: fetch integration
alt exists
DB-->>Service: integration
Service->>DB: update & save
DB-->>Service: success
Service-->>API: return updated integration
else not found
Service-->>API: raise IntegrationNotFoundError
Note right of API #ffeead: API catches IntegrationNotFoundError -> HTTP 404
end
sequenceDiagram
autonumber
participant API as integrations API
participant Service as integration_service
participant DB as persistence
API->>Service: delete_integration(id)
Service->>DB: fetch integration (pre-check)
alt found
DB-->>Service: integration
Service->>DB: delete
DB-->>Service: success
Service-->>API: success response
else not found
Service-->>API: raise ValueError (or IntegrationNotFoundError)
Note right of API #ffd6d6: API maps ValueError -> HTTP 400 (and IntegrationNotFoundError -> 404)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
♻️ Duplicate comments (2)
pyproject.toml (1)
42-42: Websockets version added, but conflicts with requirements.txt.This adds websockets
>=15.0.1,<16.0.0, butbackend/requirements.txt(line 227) specifies>=14.2,<15.0. This version mismatch was already flagged in the requirements.txt review and needs to be resolved for consistent dependency resolution.backend/app/api/v1/integrations.py (1)
97-98: Same ValueError issues as inupdate_integration.This endpoint has the same problems identified in lines 79-80:
- Using
ValueErrorfor "not found" is semantically incorrect- Broad catching of all
ValueErrorexceptions may incorrectly map validation errors (400) to 404 errorsAdditionally, note the inconsistency with
get_integration(lines 60-64), which checks forNoneand raises 404 directly, rather than catching an exception. This inconsistency makes the codebase harder to understand and maintain.Consider applying the same fix as suggested for
update_integration.
🧹 Nitpick comments (1)
backend/app/database/falkor/code-graph-backend/pyproject.toml (1)
10-23: LGTM! Dependency updates and additions for enhanced code analysis.The Python version range update to
>=3.10,<3.14is more explicit and includes Python 3.13. The addition of tree-sitter language parsers (C, Python, Java), Flask, and related dependencies appears to expand the code analysis capabilities of the code-graph-backend.Note: The multilspy dependency uses a git source (
git+https://github.com/AviAvni/multilspy.git@python-init-params), which could be fragile if the branch is rebased or deleted. Consider pinning to a specific commit SHA for reproducibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
backend/app/database/falkor/code-graph-backend/poetry.lockis excluded by!**/*.lockpoetry.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
backend/app/api/v1/integrations.py(3 hunks)backend/app/database/falkor/code-graph-backend/pyproject.toml(1 hunks)backend/app/models/database/supabase.py(2 hunks)backend/app/services/integration_service.py(2 hunks)backend/integrations/discord/bot.py(1 hunks)backend/main.py(1 hunks)backend/requirements.txt(2 hunks)backend/start_github_mcp_server.py(1 hunks)pyproject.toml(2 hunks)
🧰 Additional context used
🪛 Ruff (0.14.1)
backend/app/services/integration_service.py
145-145: Abstract raise to an inner function
(TRY301)
145-145: Avoid specifying long messages outside the exception class
(TRY003)
backend/start_github_mcp_server.py
20-20: Possible binding to all interfaces
(S104)
🔇 Additional comments (11)
backend/integrations/discord/bot.py (1)
20-25: LGTM! WebSocket keep-alive configuration added.The
heartbeat_timeout=60.0parameter properly configures the Discord gateway heartbeat timeout, which should help maintain stable WebSocket connections and align with the broader WebSocket keep-alive improvements made throughout this PR.backend/requirements.txt (1)
156-156: LGTM! Dependency update to address deprecation warnings.The py-cord version bump from 2.6.1 to 2.6.2 aligns with the PR objective of minimizing deprecation warnings.
backend/app/models/database/supabase.py (2)
1-1: LGTM! Pydantic v2 import added.The
ConfigDictimport is correctly added to support Pydantic v2 configuration syntax.
228-228: Remove the suggestion about addingfrom_attributes=Trueto other models—it's not needed for this codebase.The
from_attributes=Trueconfiguration is designed for Pydantic models instantiated from ORM objects with attribute access. However, the codebase instantiates all models via dictionary unpacking (Model(**result.data[0])), where Supabase REST API returns plain dictionaries. This pattern does not requirefrom_attributes=True. The configuration onIndexedRepositoryappears unnecessary or intended for future use; it provides no benefit for current instantiation patterns. No other models need this change.Likely an incorrect or invalid review comment.
backend/start_github_mcp_server.py (2)
20-20: LGTM! WebSocket keep-alive configuration added.The
ws_ping_interval=20andws_ping_timeout=20parameters properly configure WebSocket keep-alive, which should help maintain stable connections. These values are consistent with the configuration inbackend/main.py.Note: The static analysis hint about binding to all interfaces (0.0.0.0) is flagged but is likely acceptable for this server. If this service is exposed to untrusted networks, consider binding to 127.0.0.1 instead.
25-28: LGTM! Explicit exit codes added.The
sys.exit(1)calls in exception handlers make error handling more explicit and ensure the process exits with a failure code when startup fails.backend/main.py (1)
140-147: LGTM! WebSocket keep-alive configuration added.The
ws_ping_interval=20andws_ping_timeout=20parameters are correctly added to the uvicorn configuration. This matches the configuration inbackend/start_github_mcp_server.py, ensuring consistency across server startup scripts.backend/app/services/integration_service.py (1)
10-10: LGTM! Removed unused import.The
NotFoundErrorimport has been correctly removed as it's no longer used in this module.pyproject.toml (2)
29-29: LGTM! Discord-py version updated.The discord-py version bump to
>=2.6.4,<3.0.0aligns with the py-cord 2.6.2 update inrequirements.txtand should help address deprecation warnings.
26-26: Verification complete: uvicorn 0.38 supportsws_ping_intervalandws_ping_timeoutparameters.Uvicorn 0.38 exposes both ws_ping_interval and ws_ping_timeout as CLI flags and Config options, confirming compatibility with the WebSocket configuration used in the codebase. The version bump is safe to proceed.
backend/app/api/v1/integrations.py (1)
10-10: Import change is consistent with service layer refactoring.The removal of
NotFoundErroraligns with the service layer change. However, see comments below regarding semantic concerns with the new exception handling approach.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/api/v1/integrations.py(3 hunks)backend/app/services/integration_service.py(3 hunks)backend/requirements.txt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/requirements.txt
🧰 Additional context used
🧬 Code graph analysis (2)
backend/app/services/integration_service.py (2)
backend/app/models/integration.py (1)
IntegrationStatusResponse(44-49)backend/app/api/v1/integrations.py (1)
get_integration(52-68)
backend/app/api/v1/integrations.py (1)
backend/app/services/integration_service.py (1)
IntegrationNotFoundError(16-18)
🪛 Ruff (0.14.1)
backend/app/services/integration_service.py
150-150: Abstract raise to an inner function
(TRY301)
150-150: Avoid specifying long messages outside the exception class
(TRY003)
180-180: Abstract raise to an inner function
(TRY301)
180-180: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (4)
backend/app/services/integration_service.py (2)
16-18: Excellent addition for semantic clarity.The custom
IntegrationNotFoundErrorexception properly addresses the semantic concerns from previous reviews about usingValueErrorfor "not found" scenarios. This follows Python best practices for domain-specific exceptions.
148-150: Correct use of the new exception type.The
update_integrationmethod now properly raisesIntegrationNotFoundErrorwhen the integration doesn't exist, which is semantically correct and improves code clarity.backend/app/api/v1/integrations.py (2)
10-10: Import updated correctly.The import statement correctly reflects the new
IntegrationNotFoundErrorexception from the service layer.
79-82: Exception handling properly distinguishes error types.The exception handling correctly separates "not found" errors (404) from validation errors (400), providing clear semantic distinction. The exception order is also correct, catching from most specific to least specific.
smokeyScraper
force-pushed
the
bug_fixes
branch
from
92ea480 to
78c0318
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/app/services/integration_service.py (1)
150-150: Consider refactoring error messages per Ruff suggestions.Static analysis suggests abstracting the repeated
"Integration not found"message. While the current implementation is acceptable, you could reduce duplication by defining the message as a class constant if you expect to extend error messaging in the future:class IntegrationNotFoundError(Exception): """Raised when an integration is not found.""" - pass + DEFAULT_MESSAGE = "Integration not found" + + def __init__(self, message: str = None): + super().__init__(message or self.DEFAULT_MESSAGE)Then simplify the raise statements:
raise IntegrationNotFoundError()This addresses Ruff hints TRY003 and TRY301, though it's optional for your current use case.
Also applies to: 180-180
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/api/v1/integrations.py(3 hunks)backend/app/services/integration_service.py(3 hunks)backend/requirements.txt(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/app/services/integration_service.py (2)
backend/app/models/integration.py (1)
IntegrationStatusResponse(44-49)backend/app/api/v1/integrations.py (1)
get_integration(52-68)
backend/app/api/v1/integrations.py (1)
backend/app/services/integration_service.py (1)
IntegrationNotFoundError(16-18)
🪛 Ruff (0.14.1)
backend/app/services/integration_service.py
150-150: Abstract raise to an inner function
(TRY301)
150-150: Avoid specifying long messages outside the exception class
(TRY003)
180-180: Abstract raise to an inner function
(TRY301)
180-180: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (8)
backend/requirements.txt (2)
156-156: LGTM: Patch version update addresses deprecation warnings.The py-cord update from 2.6.1 to 2.6.2 is a safe patch version bump that aligns with the PR's objective to minimize deprecation warnings.
227-227: LGTM: websockets version now aligned with pyproject.toml.The websockets version constraint has been updated to match pyproject.toml (>=15.0.1,<16.0.0), resolving the previous version mismatch. This ensures consistent dependency resolution across the project.
backend/app/services/integration_service.py (3)
16-18: LGTM: Custom exception improves semantic clarity.The introduction of
IntegrationNotFoundErrorprovides clear, domain-specific exception handling that distinguishes "not found" scenarios from validation errors. This is a significant improvement over using genericValueError.
148-150: LGTM: Consistent error handling for not-found scenarios.The
update_integrationmethod now properly raisesIntegrationNotFoundErrorwhen the integration doesn't exist, providing consistent semantics across the service layer.
177-180: LGTM: Existence check prevents silent failures.The
delete_integrationmethod now validates the integration exists before attempting deletion and raisesIntegrationNotFoundErrorappropriately. This ensures the API layer can return proper 404 responses and addresses the critical issue from the previous review.backend/app/api/v1/integrations.py (3)
10-10: LGTM: Import updated to match service layer.The import correctly references
IntegrationNotFoundErrorfrom the service layer, maintaining consistency across the codebase.
79-82: LGTM: Exception handling properly maps domain errors to HTTP status codes.The
update_integrationendpoint now correctly distinguishes between not-found scenarios (404) and validation errors (400), addressing the semantic confusion from the previous review. The exception handling is clear and maintainable.
99-102: LGTM: Consistent exception handling across endpoints.The
delete_integrationendpoint uses the same exception handling pattern asupdate_integration, ensuring consistency. The customIntegrationNotFoundErrorprovides clear semantics, andValueErroris appropriately mapped to 400 Bad Request.
1 participant
Summary by CodeRabbit
Bug Fixes
Chores