Open
Conversation
…logging - Updated Cloud Run domain regex to support optional trailing slash (fixes CORS error for some clients). - Added `CORSLoggingMiddleware` to log CORS headers for easier debugging. - Updated tests to verify fix. Co-authored-by: KnellBalm <90038472+KnellBalm@users.noreply.github.com>
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideAdjusts CORS handling to accept Cloud Run origins with optional trailing slashes and introduces a CORS-focused logging middleware, along with a targeted test to verify the updated behavior. Sequence diagram for CORS logging and handling middlewaresequenceDiagram
actor Browser
participant App
participant CORSLoggingMiddleware
participant CORSMiddleware
participant Endpoint
Browser->>App: HTTP request with Origin header
App->>CORSLoggingMiddleware: pass request
CORSLoggingMiddleware->>CORSLoggingMiddleware: read origin from request.headers
CORSLoggingMiddleware->>CORSLoggingMiddleware: log CORS Request with origin
CORSLoggingMiddleware->>CORSMiddleware: call_next(request)
CORSMiddleware->>Endpoint: forward request
Endpoint-->>CORSMiddleware: response
CORSMiddleware-->>CORSLoggingMiddleware: response with access-control-allow-origin
CORSLoggingMiddleware->>CORSLoggingMiddleware: read access-control-allow-origin from response.headers
alt missing access-control-allow-origin
CORSLoggingMiddleware->>CORSLoggingMiddleware: log CORS Failed warning
end
CORSLoggingMiddleware-->>App: response
App-->>Browser: HTTP response
Class diagram for new CORSLoggingMiddlewareclassDiagram
class BaseHTTPMiddleware {
<<external>>
+dispatch(request, call_next) async
}
class CORSLoggingMiddleware {
+dispatch(request, call_next) async
}
BaseHTTPMiddleware <|-- CORSLoggingMiddleware
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
CORSLoggingMiddleware, you recreate the logger twice; consider instantiating it once at the top of the middleware (or module) to avoid repeatedget_loggercalls on every request. - The new test uses an Origin with a trailing slash, but browsers typically omit the trailing slash in the
Originheader; consider normalizing or explicitly handling both forms in the application logic so tests reflect real-world requests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `CORSLoggingMiddleware`, you recreate the logger twice; consider instantiating it once at the top of the middleware (or module) to avoid repeated `get_logger` calls on every request.
- The new test uses an Origin with a trailing slash, but browsers typically omit the trailing slash in the `Origin` header; consider normalizing or explicitly handling both forms in the application logic so tests reflect real-world requests.
## Individual Comments
### Comment 1
<location> `backend/common/middleware.py:38-47` </location>
<code_context>
content={"detail": "Internal Server Error"}
)
+
+class CORSLoggingMiddleware(BaseHTTPMiddleware):
+ """
+ Middleware to log CORS related headers for debugging.
+ """
+ async def dispatch(self, request: Request, call_next):
+ origin = request.headers.get("origin")
+ if origin:
+ logger = get_logger("backend.middleware.cors")
+ logger.info(f"CORS Request: {request.method} {request.url} - Origin: {origin}")
+
+ response = await call_next(request)
+
+ if origin:
+ acao = response.headers.get("access-control-allow-origin")
+ if not acao:
+ logger = get_logger("backend.middleware.cors")
+ logger.warning(f"CORS Failed? Origin: {origin} - No Access-Control-Allow-Origin in response")
+
+ return response
</code_context>
<issue_to_address>
**suggestion:** Avoid calling `get_logger` multiple times per request and promote a shared logger instance.
`get_logger("backend.middleware.cors")` is called in both branches of `dispatch`, causing repeated lookups per request. Since the name is static, define a module-level logger (e.g. `CORS_LOGGER = get_logger("backend.middleware.cors")`) and reuse it, which reduces overhead and keeps `dispatch` simpler.
Suggested implementation:
```python
)
CORS_LOGGER = get_logger("backend.middleware.cors")
class CORSLoggingMiddleware(BaseHTTPMiddleware):
```
```python
origin = request.headers.get("origin")
if origin:
CORS_LOGGER.info(f"CORS Request: {request.method} {request.url} - Origin: {origin}")
```
```python
if origin:
acao = response.headers.get("access-control-allow-origin")
if not acao:
CORS_LOGGER.warning(
f"CORS Failed? Origin: {origin} - No Access-Control-Allow-Origin in response"
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+38
to
+47
| class CORSLoggingMiddleware(BaseHTTPMiddleware): | ||
| """ | ||
| Middleware to log CORS related headers for debugging. | ||
| """ | ||
| async def dispatch(self, request: Request, call_next): | ||
| origin = request.headers.get("origin") | ||
| if origin: | ||
| logger = get_logger("backend.middleware.cors") | ||
| logger.info(f"CORS Request: {request.method} {request.url} - Origin: {origin}") | ||
|
|
There was a problem hiding this comment.
suggestion: Avoid calling get_logger multiple times per request and promote a shared logger instance.
get_logger("backend.middleware.cors") is called in both branches of dispatch, causing repeated lookups per request. Since the name is static, define a module-level logger (e.g. CORS_LOGGER = get_logger("backend.middleware.cors")) and reuse it, which reduces overhead and keeps dispatch simpler.
Suggested implementation:
)
CORS_LOGGER = get_logger("backend.middleware.cors")
class CORSLoggingMiddleware(BaseHTTPMiddleware): origin = request.headers.get("origin")
if origin:
CORS_LOGGER.info(f"CORS Request: {request.method} {request.url} - Origin: {origin}") if origin:
acao = response.headers.get("access-control-allow-origin")
if not acao:
CORS_LOGGER.warning(
f"CORS Failed? Origin: {origin} - No Access-Control-Allow-Origin in response"
)- Updated Cloud Run domain regex to support optional trailing slash (fixes CORS error for some clients). - Added `CORSLoggingMiddleware` to log CORS headers for easier debugging. - Fixed `tests/test_integration.py` to temporarily force `ENV=development` to prevent failures caused by global `ENV` modifications in other tests. - Updated tests to verify fix. Co-authored-by: KnellBalm <90038472+KnellBalm@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Updated the CORS regex in
backend/main.pyto allow optional trailing slashes for Cloud Run domains, which was likely causing CORS failures for the frontend. Added a newCORSLoggingMiddlewareto log Origin and Access-Control-Allow-Origin headers to assist with future debugging. Updatedtests/test_cors_config.pyto include a test case for trailing slashes.PR created automatically by Jules for task 17524954021399206428 started by @KnellBalm
Summary by Sourcery
Relax CORS origin matching and add middleware to improve CORS debugging.
New Features:
Enhancements:
Tests: