353 Add GitHub Webhook Handling and Cache Invalidation #433
Conversation
WalkthroughA new GitHub webhook router and service were added; the main app now includes the router under Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub
participant API as FastAPI (/api/v1/github)
participant WSvc as GitHubWebhookService
participant DB as Database (Session)
participant GSvc as GithubService (cache ops)
Note over GH,API: Incoming webhook
GH->>API: POST /webhook (payload + headers)
API->>WSvc: process_webhook(request)
WSvc->>DB: obtain session
WSvc->>WSvc: verify signature (HMAC SHA256) [if secret]
alt signature invalid
WSvc-->>API: HTTP 401
else valid
WSvc->>WSvc: parse event & action
alt event == "public" or relevant "repository" actions
WSvc->>GSvc: clear repo cache
GSvc-->>WSvc: result
WSvc-->>API: success payload
else
WSvc-->>API: ignored/ack payload
end
end
Note over API,GH: HTTP response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 |
|
@dhirenmathur please check |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
app/modules/code_provider/github/github_webhook_router.py (1)
1-1: Remove unused import.The
loggingimport is not used in this file.-import loggingapp/modules/code_provider/github/github_webhook_service.py (3)
6-6: Remove unused import.
Optionalfrom typing is imported but never used.-from typing import Dict, Any, Optional +from typing import Dict, Any
11-11: Remove unused import.
config_provideris imported but never used in this file.-from app.core.config_provider import config_provider
38-41: Simplify nested conditional statements.The nested if statements can be combined for better readability.
- if self.webhook_secret: - if not self._verify_webhook_signature(payload, headers): + if self.webhook_secret and not self._verify_webhook_signature(payload, headers): logger.warning("Webhook signature verification failed") raise HTTPException(status_code=401, detail="Invalid webhook signature")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/main.py(2 hunks)app/modules/code_provider/github/github_service.py(1 hunks)app/modules/code_provider/github/github_webhook_router.py(1 hunks)app/modules/code_provider/github/github_webhook_service.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
app/modules/code_provider/github/github_webhook_router.py (2)
app/core/database.py (1)
get_db(29-34)app/modules/code_provider/github/github_webhook_service.py (3)
GitHubWebhookService(17-169)process_webhook(28-68)get_webhook_stats(162-169)
app/modules/code_provider/github/github_webhook_service.py (2)
app/modules/code_provider/github/github_service.py (1)
GithubService(28-772)app/modules/code_provider/github/github_webhook_router.py (1)
get_webhook_stats(38-46)
🪛 Ruff (0.12.2)
app/modules/code_provider/github/github_webhook_router.py
1-1: logging imported but unused
Remove unused import: logging
(F401)
16-16: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
34-34: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
39-39: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
46-46: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
app/modules/code_provider/github/github_webhook_service.py
6-6: typing.Optional imported but unused
Remove unused import: typing.Optional
(F401)
11-11: app.core.config_provider.config_provider imported but unused
Remove unused import: app.core.config_provider.config_provider
(F401)
38-39: Use a single if statement instead of nested if statements
(SIM102)
48-48: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
68-68: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (7)
app/modules/code_provider/github/github_service.py (1)
772-772: LGTM! Whitespace cleanup.The trailing newline removal is a minor formatting improvement with no functional impact.
app/main.py (2)
16-16: LGTM! Proper router import.The import follows the established pattern used by other routers in the application.
104-104: LGTM! Router integration follows established patterns.The router inclusion is consistent with the existing codebase patterns, using the same prefix and tagging approach as other routers.
app/modules/code_provider/github/github_webhook_router.py (1)
10-46: Webhook endpoint security and cache invalidation verified
process_webhookinvokes_verify_webhook_signature(payload, headers)whenwebhook_secretis set, rejecting invalid signatures with HTTP 401.- JSON decoding errors return HTTP 400 and unexpected errors return HTTP 500 as intended.
- Cache is cleared for both
"public"and relevant"repository"events via_clear_repository_cache, which callsself.github_service.clear_repository_cache(repo_name)and updatesstats["cache_purges"].No changes required.
app/modules/code_provider/github/github_webhook_service.py (3)
140-160: LGTM! Excellent webhook signature verification implementation.The HMAC SHA256 signature verification follows GitHub's webhook security best practices:
- Proper signature header validation
- Secure HMAC comparison using
hmac.compare_digest()- Correct handling of the 'sha256=' prefix
- Appropriate error handling for verification failures
This implementation effectively prevents webhook spoofing attacks.
17-27: LGTM! Well-structured service initialization.The service properly initializes with:
- Database session dependency injection
- GithubService integration for cache operations
- Optional webhook secret configuration
- Statistics tracking for monitoring
The architecture follows good separation of concerns.
54-61: LGTM! Proper event filtering and handling.The event type filtering correctly handles the supported GitHub webhook events ('public' and 'repository') while gracefully ignoring unsupported events. This approach ensures the service is resilient to new GitHub webhook event types.
| @router.post("/webhook") | ||
| async def handle_github_webhook( | ||
| request: Request, | ||
| db: Session = Depends(get_db) | ||
| ) -> Dict[str, Any]: | ||
| """ | ||
| Handle GitHub webhooks for repository visibility changes. | ||
| Supported events: | ||
| - 'public': Repository changed from private to public | ||
| - 'repository': Repository events including privatization, publicization, deletion | ||
| This endpoint automatically purges cache when repository visibility changes. | ||
| """ | ||
| try: | ||
| webhook_service = GitHubWebhookService(db) | ||
| result = await webhook_service.process_webhook(request) | ||
| return result | ||
| except HTTPException: | ||
| raise | ||
| except Exception as e: | ||
| raise HTTPException(status_code=500, detail=f"Webhook processing error: {str(e)}") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling with proper chaining.
The current exception handling loses the original error context. Use raise ... from err to preserve the exception chain for better debugging.
except HTTPException:
raise
except Exception as e:
- raise HTTPException(status_code=500, detail=f"Webhook processing error: {str(e)}")
+ raise HTTPException(status_code=500, detail=f"Webhook processing error: {str(e)}") from e📝 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.
| @router.post("/webhook") | |
| async def handle_github_webhook( | |
| request: Request, | |
| db: Session = Depends(get_db) | |
| ) -> Dict[str, Any]: | |
| """ | |
| Handle GitHub webhooks for repository visibility changes. | |
| Supported events: | |
| - 'public': Repository changed from private to public | |
| - 'repository': Repository events including privatization, publicization, deletion | |
| This endpoint automatically purges cache when repository visibility changes. | |
| """ | |
| try: | |
| webhook_service = GitHubWebhookService(db) | |
| result = await webhook_service.process_webhook(request) | |
| return result | |
| except HTTPException: | |
| raise | |
| except Exception as e: | |
| raise HTTPException(status_code=500, detail=f"Webhook processing error: {str(e)}") | |
| @router.post("/webhook") | |
| async def handle_github_webhook( | |
| request: Request, | |
| db: Session = Depends(get_db) | |
| ) -> Dict[str, Any]: | |
| """ | |
| Handle GitHub webhooks for repository visibility changes. | |
| Supported events: | |
| - 'public': Repository changed from private to public | |
| - 'repository': Repository events including privatization, publicization, deletion | |
| This endpoint automatically purges cache when repository visibility changes. | |
| """ | |
| try: | |
| webhook_service = GitHubWebhookService(db) | |
| result = await webhook_service.process_webhook(request) | |
| return result | |
| except HTTPException: | |
| raise | |
| except Exception as e: | |
| raise HTTPException( | |
| status_code=500, | |
| detail=f"Webhook processing error: {str(e)}" | |
| ) from e |
🧰 Tools
🪛 Ruff (0.12.2)
16-16: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
34-34: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In app/modules/code_provider/github/github_webhook_router.py around lines 13 to
35, the exception handling in the webhook endpoint catches generic exceptions
but raises a new HTTPException without preserving the original error context.
Modify the except block to use "raise HTTPException(...) from e" to maintain the
exception chain and improve debugging.
| @router.get("/webhook/stats") | ||
| async def get_webhook_stats( | ||
| db: Session = Depends(get_db) | ||
| ) -> Dict[str, Any]: | ||
| """Get webhook service statistics and status""" | ||
| try: | ||
| webhook_service = GitHubWebhookService(db) | ||
| return await webhook_service.get_webhook_stats() | ||
| except Exception as e: | ||
| raise HTTPException(status_code=500, detail=f"Error getting webhook stats: {str(e)}") No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling with proper chaining.
Same issue as the webhook handler - preserve the exception chain for better error traceability.
except Exception as e:
- raise HTTPException(status_code=500, detail=f"Error getting webhook stats: {str(e)}")
+ raise HTTPException(status_code=500, detail=f"Error getting webhook stats: {str(e)}") from e📝 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.
| @router.get("/webhook/stats") | |
| async def get_webhook_stats( | |
| db: Session = Depends(get_db) | |
| ) -> Dict[str, Any]: | |
| """Get webhook service statistics and status""" | |
| try: | |
| webhook_service = GitHubWebhookService(db) | |
| return await webhook_service.get_webhook_stats() | |
| except Exception as e: | |
| raise HTTPException(status_code=500, detail=f"Error getting webhook stats: {str(e)}") | |
| @router.get("/webhook/stats") | |
| async def get_webhook_stats( | |
| db: Session = Depends(get_db) | |
| ) -> Dict[str, Any]: | |
| """Get webhook service statistics and status""" | |
| try: | |
| webhook_service = GitHubWebhookService(db) | |
| return await webhook_service.get_webhook_stats() | |
| except Exception as e: | |
| raise HTTPException(status_code=500, detail=f"Error getting webhook stats: {str(e)}") from e |
🧰 Tools
🪛 Ruff (0.12.2)
39-39: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
46-46: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In app/modules/code_provider/github/github_webhook_router.py around lines 37 to
46, the exception handling in get_webhook_stats does not preserve the original
exception chain, which reduces error traceability. Modify the except block to
use "raise HTTPException(...) from e" to properly chain the original exception
when raising the HTTPException.
| except json.JSONDecodeError as e: | ||
| logger.error(f"Invalid JSON in webhook payload: {e}") | ||
| raise HTTPException(status_code=400, detail="Invalid JSON payload") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling with proper chaining.
Preserve the original JSON decode error for better debugging.
except json.JSONDecodeError as e:
logger.error(f"Invalid JSON in webhook payload: {e}")
- raise HTTPException(status_code=400, detail="Invalid JSON payload")
+ raise HTTPException(status_code=400, detail="Invalid JSON payload") from e📝 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.
| except json.JSONDecodeError as e: | |
| logger.error(f"Invalid JSON in webhook payload: {e}") | |
| raise HTTPException(status_code=400, detail="Invalid JSON payload") | |
| except json.JSONDecodeError as e: | |
| logger.error(f"Invalid JSON in webhook payload: {e}") | |
| raise HTTPException(status_code=400, detail="Invalid JSON payload") from e |
🧰 Tools
🪛 Ruff (0.12.2)
48-48: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In app/modules/code_provider/github/github_webhook_service.py around lines 46 to
48, the JSONDecodeError is caught and logged but the original exception is not
chained when raising the HTTPException. Modify the raise statement to include
"from e" to preserve the original exception context for better debugging and
traceback.
| except Exception as e: | ||
| self.stats["errors"] += 1 | ||
| logger.error(f"Error processing webhook: {e}") | ||
| raise HTTPException(status_code=500, detail=f"Webhook processing error: {str(e)}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling with proper chaining.
Preserve the original exception for better error traceability.
except Exception as e:
self.stats["errors"] += 1
logger.error(f"Error processing webhook: {e}")
- raise HTTPException(status_code=500, detail=f"Webhook processing error: {str(e)}")
+ raise HTTPException(status_code=500, detail=f"Webhook processing error: {str(e)}") from e📝 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.
| except Exception as e: | |
| self.stats["errors"] += 1 | |
| logger.error(f"Error processing webhook: {e}") | |
| raise HTTPException(status_code=500, detail=f"Webhook processing error: {str(e)}") | |
| except Exception as e: | |
| self.stats["errors"] += 1 | |
| logger.error(f"Error processing webhook: {e}") | |
| raise HTTPException(status_code=500, detail=f"Webhook processing error: {str(e)}") from e |
🧰 Tools
🪛 Ruff (0.12.2)
68-68: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In app/modules/code_provider/github/github_webhook_service.py around lines 65 to
68, the exception handling currently logs and raises a new HTTPException without
preserving the original exception context. Modify the raise statement to use
"raise HTTPException(...) from e" to maintain proper exception chaining and
improve error traceability.
| async def _clear_repository_cache(self, repo_name: str): | ||
| """Clear cache for a specific repository""" | ||
| try: | ||
| await self.github_service.clear_repository_cache(repo_name) | ||
| self.stats["cache_purges"] += 1 | ||
| logger.info(f"Cache cleared for repository: {repo_name}") | ||
| except Exception as e: | ||
| logger.error(f"Error clearing cache for {repo_name}: {e}") | ||
| raise |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Verify that GithubService has the clear_repository_cache method.
The service calls self.github_service.clear_repository_cache(repo_name) but this method doesn't appear to exist in the GithubService class based on the provided code.
🏁 Script executed:
#!/bin/bash
# Description: Verify if clear_repository_cache method exists in GithubService
# Search for the clear_repository_cache method implementation
ast-grep --pattern 'async def clear_repository_cache($$$) {
$$$
}'
# Also check for any cache clearing related methods
rg -A 5 "clear.*cache|cache.*clear" app/modules/code_provider/github/github_service.pyLength of output: 159
Implement missing clear_repository_cache in GithubService
The call in GithubWebhookService._clear_repository_cache (app/modules/code_provider/github/github_webhook_service.py:130–138) invokes self.github_service.clear_repository_cache(...), but no such method exists. Add it to your service:
# app/modules/code_provider/github/github_service.py
class GithubService:
# … existing methods …
async def clear_repository_cache(self, repo_name: str):
"""
Clear any in-memory or persisted cache for the given repository.
"""
# TODO: implement cache-clearing logic here
pass• app/modules/code_provider/github/github_webhook_service.py (lines 130–138) – invocation of the nonexistent method
• app/modules/code_provider/github/github_service.py – implement the new method
🤖 Prompt for AI Agents
In app/modules/code_provider/github/github_service.py, add a new async method
clear_repository_cache that accepts repo_name as a parameter. This method should
contain the logic to clear any in-memory or persisted cache related to the
specified repository. This will resolve the missing method error when called
from app/modules/code_provider/github/github_webhook_service.py lines 130 to
138.
| self.stats = { | ||
| "webhooks_received": 0, | ||
| "cache_purges": 0, | ||
| "errors": 0 | ||
| } |
There was a problem hiding this comment.
Can you please review these changes. It's storing a local state of stats which isn't needed. Please clean this up
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/main.py(2 hunks)
🔇 Additional comments (1)
app/main.py (1)
16-16: LGTM: Import follows existing conventions.The import statement is correctly formatted and follows the same pattern as other router imports in the file.
| self.app.include_router(projects_router, prefix="/api/v1", tags=["Projects"]) | ||
| self.app.include_router(search_router, prefix="/api/v1", tags=["Search"]) | ||
| self.app.include_router(github_router, prefix="/api/v1", tags=["Github"]) | ||
| self.app.include_router(github_webhook_router, prefix="/api/v1", tags=["GitHub Webhooks"]) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify webhook route paths and address tag capitalization inconsistency.
The router registration follows the correct pattern. However, there are two items to address:
-
Tag capitalization inconsistency: Line 105 uses
tags=["Github"]while this line usestags=["GitHub Webhooks"]. Consider using consistent capitalization (e.g.,"GitHub"on line 105). -
Route path verification: According to the PR objectives, the final webhook endpoints should be at
/api/v1/github/webhookand/api/v1/github/webhook/stats. Since this registration usesprefix="/api/v1", thegithub_webhook_routermust define its routes with a/githubprefix. Please verify this is correctly implemented in the router definition.
🏁 Script executed:
#!/bin/bash
# Verify that the github_webhook_router defines routes with /github prefix
# to ensure final endpoints match /api/v1/github/webhook as stated in PR objectives
echo "Checking github_webhook_router route definitions..."
rg -n -A 5 'APIRouter\(' app/modules/code_provider/github/github_webhook_router.py
echo -e "\nChecking route decorator definitions..."
rg -n '@router\.(get|post|put|delete|patch)' app/modules/code_provider/github/github_webhook_router.pyLength of output: 323
CRITICAL: Fix router prefix definition to prevent duplicate API path segments.
The verification reveals a critical issue with the route paths:
- github_webhook_router.py line 10 defines:
prefix="/api/v1/github" - main.py line 106 includes it with:
prefix="/api/v1"
In FastAPI, nested prefixes are concatenated. This produces routes at /api/v1/api/v1/github/webhook instead of the intended /api/v1/github/webhook.
Fix required: Change the router definition in app/modules/code_provider/github/github_webhook_router.py line 10 from:
router = APIRouter(prefix="/api/v1/github", tags=["GitHub Webhooks"])to:
router = APIRouter(prefix="/github", tags=["GitHub Webhooks"])Additionally, update main.py line 105 tag from "Github" to "GitHub" for consistency.
🤖 Prompt for AI Agents
In app/main.py around line 106 and
app/modules/code_provider/github/github_webhook_router.py around line 10: the
router prefix is duplicated because main.py includes the router with prefix
"/api/v1" while the router itself uses "/api/v1/github", producing paths like
"/api/v1/api/v1/github/...". Remove the duplicated segment by changing the
router declaration in app/modules/code_provider/github/github_webhook_router.py
line 10 to use prefix "/github" instead of "/api/v1/github"; also update the tag
string in app/main.py line 105 from "Github" to "GitHub" for consistent casing.
3 participants
Add GitHub Webhook Handling and Cache Invalidation
What
publicandrepositoryeventsHow it Works
/api/v1/github/webhookwhen repository visibility changesEvents Supported
public: Repository made public/privaterepository: Repository events (privatized, deleted, transferred)Files Changed
app/modules/code_provider/github/github_webhook_service.py(new)app/modules/code_provider/github/github_webhook_router.py(new)app/modules/code_provider/github/github_service.pyapp/main.pyTesting
Environment Variables
GITHUB_WEBHOOK_SECRET(optional) - for signature verificationSummary by CodeRabbit
New Features
Bug Fixes
Chores