Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions backend/api/routes_sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
from sqlalchemy.orm import Session

from api.config import get_settings
from api.db import get_db
from api.db import SessionLocal, get_db
from api.models import SyncEvent
from api.security import require_write_token, validate_hygraph_request
from api.metrics import (
Expand Down Expand Up @@ -110,8 +110,9 @@ async def hygraph_webhook(

async def _process(event_id_local: Optional[str], body_sha_local: str) -> None:
t0 = time.perf_counter()
session = SessionLocal()
try:
counts = await HygraphService.pull_all(db)
counts = await HygraphService.pull_all(session)
for t, c in counts.items():
sync_records_upserted_total.labels(t).inc(int(c or 0))
sync_success_total.labels("all").inc()
Expand All @@ -135,6 +136,8 @@ async def _process(event_id_local: Optional[str], body_sha_local: str) -> None:
"error": str(e),
},
)
finally:
session.close()

background.add_task(_process, event_id, body_sha)
# Explicit background attachment ensures Starlette runs it before TestClient returns
Expand Down
11 changes: 11 additions & 0 deletions backend/api/security.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,17 @@

from api.config import Settings, get_settings


def _load_expected_write_token(settings: Settings) -> str:
"""Resolve the configured API write token from settings or environment."""

token = getattr(settings, "api_write_token", None)
if isinstance(token, str) and token.strip():
return token.strip()
env_token = os.getenv("API_WRITE_TOKEN", "")
return env_token.strip()


# Write token (admin) auth
api_key_header = APIKeyHeader(name="Authorization", auto_error=False)

Expand Down
33 changes: 29 additions & 4 deletions backend/tests/test_sync_routes_metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,14 @@
import hmac
import json
import re
import uuid

import pytest
from fastapi.testclient import TestClient
from sqlalchemy import text

from api.main import app
from api.routes_sync import settings as sync_settings


def _sig(secret: str, body: bytes) -> str:
Expand All @@ -16,8 +19,7 @@ def _sig(secret: str, body: bytes) -> str:


@pytest.fixture()
def client(monkeypatch):
monkeypatch.setenv("HYGRAPH_WEBHOOK_SECRET", "whsec")
def client():
return TestClient(app)


Expand All @@ -30,8 +32,9 @@ async def fake_pull_all(db, page_size=None):

monkeypatch.setattr(svc.HygraphService, "pull_all", fake_pull_all)

body = json.dumps({"ping": "ok"}).encode()
sig = _sig("whsec", body)
unique_payload = {"ping": str(uuid.uuid4())}
body = json.dumps(unique_payload).encode()
sig = _sig(sync_settings.hygraph_webhook_secret, body)

# First delivery -> 202 Accepted, background processing logs counts
r1 = client.post("/api/sync/hygraph", data=body, headers={"x-hygraph-signature": sig})
Expand All @@ -55,6 +58,28 @@ async def fake_pull_all(db, page_size=None):
assert found, "missing structured sync summary log"


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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

LGTM with a clarification on test coverage.

The test successfully verifies that:

  1. The background task receives an active database session
  2. The session can execute SQL queries
  3. 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).


def test_pull_alias_and_page_size_validation(client, monkeypatch):
from services import hygraph_service as svc

Expand Down
Loading