Ensure webhook sync uses independent sessions#90
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughA helper function resolves API write tokens by checking settings first, then falling back to environment variables. Sync webhook tests now use dynamic UUID-based payloads and secrets sourced from settings instead of hard-coded values. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/api/security.py (1)
39-42: Fix line length and return type annotation.Line 42 exceeds the 88-character limit. Additionally, consider updating the return type of
require_write_tokentoNonesince it raises exceptions on failure rather than returning a boolean value that callers can check.Apply this diff to fix the line length:
- if not expected or not secrets.compare_digest(token, expected): - raise HTTPException(status_code=HTTP_403_FORBIDDEN, detail="Invalid API write token.") + if not expected or not secrets.compare_digest(token, expected): + raise HTTPException( + status_code=HTTP_403_FORBIDDEN, detail="Invalid API write token." + )Consider also changing the return type annotation from
booltoNone:async def require_write_token( authorization: str | None = Security(api_key_header), settings: Settings = Depends(get_settings), -) -> bool: +) -> None:backend/api/routes_sync.py (3)
111-141: LGTM with a suggestion for commit handling.The session lifecycle management is correct: creating a new session with
SessionLocal(), using it for the background pull operation, and closing it in afinallyblock. This ensures the background task has an independent session that won't be affected by the outer request session's lifecycle.Consider adding an explicit commit before closing the session if
pull_allmakes any database changes that need to be persisted:except Exception as e: # noqa: BLE001 sync_failure_total.labels("all").inc() logger.exception( "hygraph_webhook_failure", extra={ "event_id": event_id_local, "dedup": False, "error": str(e), }, ) finally: + session.commit() # Persist any changes made during pull_all session.close()However, if
pull_allalready manages its own transaction commits, this is not necessary.
66-144: Fix return type annotation mismatch.The function signature declares a return type of
Dict[str, Any], but both return statements (lines 104 and 144) returnJSONResponseobjects. This causes a type checking failure.Update the return type annotation to match the actual return type:
async def hygraph_webhook( request: Request, background: BackgroundTasks, db: Session = Depends(get_db), -) -> Dict[str, Any]: +) -> JSONResponse:
46-58: Fix line length.Line 46 exceeds the 88-character limit.
Apply this diff to fix the formatting:
def _error_envelope(code: str, message: str, details: Optional[dict] = None) -> Dict[str, Any]: """ Builds a standardized error envelope for API responses. Parameters: - code (str): Machine-readable error code identifying the error. - message (str): Human-readable error message describing the failure. - details (Optional[dict]): Additional contextual information; defaults to an empty dict when not provided. + code (str): Machine-readable error code. + message (str): Human-readable error message. + details (Optional[dict]): Additional context; defaults to empty dict. Returns: - Dict[str, Any]: Dictionary with keys `ok` (False) and `error` containing `code`, `message`, and `details`. + Dict[str, Any]: Dictionary with keys `ok` (False) and `error` + containing `code`, `message`, and `details`. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
backend/api/routes_sync.py(3 hunks)backend/api/security.py(1 hunks)backend/tests/test_sync_routes_metrics.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
backend/api/security.py (1)
backend/api/config.py (1)
Settings(10-62)
backend/api/routes_sync.py (2)
backend/api/db.py (1)
get_db(53-59)backend/services/hygraph_service.py (2)
HygraphService(11-80)pull_all(62-64)
backend/tests/test_sync_routes_metrics.py (1)
backend/services/hygraph_service.py (1)
HygraphService(11-80)
🪛 GitHub Actions: CI
backend/api/security.py
[error] 42-42: Line too long (94 > 88)
backend/api/routes_sync.py
[error] 46-46: Line too long (95 > 88)
[error] 110-110: Incompatible return value type: JSONResponse returned where dict[str, Any] expected
🔇 Additional comments (4)
backend/api/routes_sync.py (1)
23-23: LGTM!The import of
SessionLocalenables the webhook background task to create its own database session, ensuring proper session lifecycle management independent of the request scope.backend/tests/test_sync_routes_metrics.py (3)
6-6: LGTM!The new imports support the enhanced test functionality:
uuidfor generating unique payloads to ensure test isolationtextfrom SQLAlchemy for executing raw SQL in the session verification testsync_settingsfor accessing the webhook secret configurationAlso applies to: 10-10, 13-13
21-23: LGTM!Simplifying the client fixture from environment-based setup to a plain
TestClient(app)improves test clarity and removes unnecessary complexity.
35-37: LGTM!Using dynamic payloads with
uuid.uuid4()and sourcing the webhook secret fromsync_settings.hygraph_webhook_secretimproves test isolation and eliminates hardcoded values, making the tests more maintainable and reliable.
| def test_webhook_background_uses_new_session(client, caplog, monkeypatch): | ||
| from services import hygraph_service as svc | ||
|
|
||
| calls: list[bool] = [] | ||
|
|
||
| async def fake_pull_all(db, page_size=None): | ||
| db.execute(text("SELECT 1")) | ||
| calls.append(db.is_active) | ||
| return {"materials": 0, "modules": 0, "systems": 0} | ||
|
|
||
| monkeypatch.setattr(svc.HygraphService, "pull_all", fake_pull_all) | ||
|
|
||
| body = json.dumps({"ping": str(uuid.uuid4())}).encode() | ||
| sig = _sig(sync_settings.hygraph_webhook_secret, body) | ||
|
|
||
| r = client.post("/api/sync/hygraph", data=body, headers={"x-hygraph-signature": sig}) | ||
| assert r.status_code == 202 | ||
| assert r.json()["ok"] is True | ||
| assert calls == [True] | ||
| assert not any(rec.getMessage() == "hygraph_webhook_failure" for rec in caplog.records) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM with a clarification on test coverage.
The test successfully verifies that:
- The background task receives an active database session
- The session can execute SQL queries
- No failure logs are emitted during processing
This confirms the webhook background processing uses a live, independent session as intended by the PR objectives.
Consider expanding the test to verify that the background session is truly independent from the request session. For example, you could verify that changes made in the background session are not visible to the request session until committed:
async def fake_pull_all(db, page_size=None):
# Verify session independence by checking it's a different instance
db.execute(text("SELECT 1"))
calls.append({
"is_active": db.is_active,
"session_id": id(db) # Track session instance
})
return {"materials": 0, "modules": 0, "systems": 0}However, the current implementation adequately verifies the core requirement.
🤖 Prompt for AI Agents
In backend/tests/test_sync_routes_metrics.py around lines 61 to 81, update the
fake_pull_all used in the test to record the background session instance id in
addition to is_active, and then update the assertions to expect a dict with both
keys and validate the session_id is an integer (and if you can obtain the
request-session id in the test, assert they are not equal to prove
independence). Specifically, change fake_pull_all to append {"is_active":
db.is_active, "session_id": id(db)} to calls, and replace the existing
assertions to check calls == [{"is_active": True, "session_id": <int>}] (and if
request session id is available, assert calls[0]["session_id"] !=
request_session_id).
|
Note Docstrings generation - SUCCESS |
…ssion` Docstrings generation was requested by @shayancoin. * #90 (comment) The following files were modified: * `backend/api/routes_sync.py` * `backend/api/security.py` * `backend/tests/test_sync_routes_metrics.py`
…ssion` (#154) Docstrings generation was requested by @shayancoin. * #90 (comment) The following files were modified: * `backend/api/routes_sync.py` * `backend/api/security.py` * `backend/tests/test_sync_routes_metrics.py` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Shayan <shayan@coin.link>
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68f12bd677208330b058f2bfd68f31f8
Summary by CodeRabbit