Add Hygraph circuit breaker and caching with fallback metrics#124
Conversation
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughCircuit breaker fallback handling is introduced for Hygraph GraphQL operations in the sync routes. Changes include a payload normalization helper, CircuitOpenError exception handling with fallback response support, circuit-breaker-specific metrics tracking, and operational documentation for cache warming and breaker state transitions. Changes
Sequence DiagramsequenceDiagram
participant Client
participant hygraph_pull as hygraph_pull()
participant Hygraph
participant Cache
participant Metrics
rect rgb(240, 248, 255)
Note over Client,Metrics: Normal Flow (Breaker Closed)
Client->>hygraph_pull: request
hygraph_pull->>Hygraph: GraphQL query
Hygraph-->>hygraph_pull: success
hygraph_pull->>Cache: update cache
hygraph_pull-->>Client: 200 + data
end
rect rgb(255, 240, 245)
Note over Client,Metrics: Circuit Open with Fallback
Client->>hygraph_pull: request
hygraph_pull->>Hygraph: GraphQL query
Hygraph-->>hygraph_pull: CircuitOpenError
hygraph_pull->>Metrics: increment hygraph_cb_trips_total
hygraph_pull->>Cache: retrieve fallback
Cache-->>hygraph_pull: cached result
hygraph_pull->>Metrics: increment hygraph_cb_fallback_total
hygraph_pull-->>Client: 200 + fallback + breaker_open flag
end
rect rgb(255, 228, 225)
Note over Client,Metrics: Circuit Open without Fallback
Client->>hygraph_pull: request
hygraph_pull->>Hygraph: GraphQL query
Hygraph-->>hygraph_pull: CircuitOpenError
hygraph_pull->>Metrics: increment hygraph_cb_trips_total
hygraph_pull->>Cache: retrieve fallback
Cache-->>hygraph_pull: none
hygraph_pull-->>Client: 503 + error envelope
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes New conditional logic for CircuitOpenError handling with metrics increments and fallback response construction requires careful tracing through error paths. Changes are contained to one endpoint but introduce multiple decision points and state dependencies that warrant systematic review across both success and failure scenarios. 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 (1)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
backend/services/hygraph_service.py
Outdated
| async def pull_all(cls, db, page_size: Optional[int] = None) -> Dict[str, Any]: | ||
| try: | ||
| materials = await cls.pull_materials(db, page_size=page_size) | ||
| modules = await cls.pull_modules(db, page_size=page_size) | ||
| systems = await cls.pull_systems(db, page_size=page_size) | ||
| except CircuitOpenError as err: | ||
| fallback_raw = err.fallback | ||
| if isinstance(fallback_raw, dict) and "resource" in fallback_raw: | ||
| fallback = {fallback_raw["resource"]: fallback_raw} | ||
| elif isinstance(fallback_raw, dict): | ||
| fallback = fallback_raw | ||
| else: | ||
| fallback = {} | ||
| if not fallback: | ||
| fallback = {} | ||
| for key in ("materials", "modules", "systems"): | ||
| cached = _CACHE.get(key) or [] | ||
| if cached: | ||
| fallback[key] = _fallback_envelope(key, cached) | ||
| raise CircuitOpenError("Hygraph circuit open", fallback=fallback) from err |
There was a problem hiding this comment.
When pull_all catches CircuitOpenError, it constructs a fallback payload from the exception but only populates caches for other resources if the fallback is empty. If the breaker trips while fetching one resource (e.g., modules) and the exception contains fallback data for that single resource, the if not fallback: block is skipped and no cached data for the other resource types is added, even though it is available or was just fetched successfully. Callers of pull_all therefore receive a partial response missing keys such as materials and systems, making the sync API inconsistent and dropping metrics for the resources that were actually processed. Consider merging the cached envelopes for any missing resource keys before re‑raising the circuit‑open error so the fallback response always contains all resource types.
Useful? React with 👍 / 👎.
backend/services/cache.py
Outdated
| def get(self, key: str) -> Any | None: | ||
| namespaced = self._ns(key) | ||
| if self._client is not None: | ||
| raw = self._client.get(namespaced) | ||
| return json.loads(raw) if raw else None | ||
| with self._lock: | ||
| value = self._memory.get(namespaced) | ||
| if value is None: | ||
| return None | ||
| raw, expires_at = value | ||
| if expires_at is not None and expires_at < time.time(): | ||
| del self._memory[namespaced] | ||
| return None | ||
| return json.loads(raw) | ||
|
|
||
| def set(self, key: str, value: Any, *, ttl: int | None = None) -> None: | ||
| payload = json.dumps(value) | ||
| namespaced = self._ns(key) | ||
| if self._client is not None: | ||
| if ttl is not None: | ||
| self._client.setex(namespaced, ttl, payload) | ||
| else: | ||
| self._client.set(namespaced, payload) | ||
| return | ||
| expires_at = (time.time() + ttl) if ttl else None | ||
| with self._lock: | ||
| self._memory[namespaced] = (payload, expires_at) | ||
|
|
||
| def clear(self, key: str) -> None: | ||
| namespaced = self._ns(key) | ||
| if self._client is not None: | ||
| self._client.delete(namespaced) | ||
| return |
There was a problem hiding this comment.
The cache helper advertises falling back to an in‑memory store, but get, set, and clear call the Redis client without handling redis.exceptions.RedisError. If Redis is configured but unavailable or times out, these methods will raise and propagate the connection error, causing sync endpoints to fail instead of transparently using the in‑process cache. Wrapping the Redis operations in try/except and falling back to _memory on failure would align the implementation with the intended resilience.
Useful? React with 👍 / 👎.
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
backend/api/metrics.py(1 hunks)backend/api/routes_sync.py(7 hunks)backend/pyproject.toml(1 hunks)backend/services/cache.py(1 hunks)backend/services/circuit_breaker.py(1 hunks)backend/services/hygraph_client.py(1 hunks)backend/services/hygraph_service.py(2 hunks)docs/backend-development.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/services/hygraph_client.py (1)
backend/services/circuit_breaker.py (3)
CircuitBreaker(22-84)CircuitOpenError(13-18)call(37-54)
backend/services/hygraph_service.py (4)
backend/api/config.py (1)
get_settings(66-69)backend/services/cache.py (2)
RedisCache(17-67)get(33-46)backend/services/circuit_breaker.py (1)
CircuitOpenError(13-18)backend/services/hygraph_client.py (2)
HygraphClient(12-69)execute(32-65)
🪛 GitHub Actions: CI
backend/api/routes_sync.py
[error] 109-109: Mypy: Incompatible return value type: JSONResponse cannot be assigned to dict[str, Any].
backend/services/hygraph_service.py
[error] 68-68: Mypy: Function is missing a type annotation for one or more arguments.
backend/services/cache.py
[warning] 12-14: Unused type: ignore comments flagged by linter (could remove or fix instead).
🪛 LanguageTool
docs/backend-development.md
[grammar] ~177-~177: Use correct spacing
Context: ...edoc ## Hygraph Circuit Breaker Runbook The Hygraph service layer now caches pay...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~179-~179: Use correct spacing
Context: ...hecklist when operating the integration: ### Warming the Cache 1. Ensure the Redis i...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~181-~181: Use correct spacing
Context: ... the integration: ### Warming the Cache 1. Ensure the Redis instance defined by `HY...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~185-~185: Use correct spacing
Context: ...l page size to populate cached datasets: - POST /api/sync/hygraph/pull with {"type": "materials"} - `POST ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~186-~186:
Context: ... - POST /api/sync/hygraph/pull with {"type": "materials"} - POST /api/sync/hygraph/pull with {"type": "modules"} - `POST /a...
(QB_NEW_EN_OTHER_ERROR_IDS_)
[grammar] ~187-~187:
Context: ... - POST /api/sync/hygraph/pull with {"type": "modules"} - POST /api/sync/hygraph/pull with {"type": "systems"} 4. Confirm th...
(QB_NEW_EN_OTHER_ERROR_IDS_)
[grammar] ~188-~188: There might be a mistake here.
Context: ... - POST /api/sync/hygraph/pull with {"type": "systems"} 4. Confirm the Redis keys `hygraph:material...
(QB_NEW_EN)
[grammar] ~189-~189: Use correct spacing
Context: ... JSON payloads with the expected counts. ### Exercising Breaker States 1. **Closed →...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~191-~191: Use correct spacing
Context: ...d counts. ### Exercising Breaker States 1. Closed → Open: Stop outbound connectiv...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~193-~193: There might be a mistake here.
Context: ...r failure_threshold attempts (default 5) the breaker opens and the Prometheus co...
(QB_NEW_EN_OTHER)
[grammar] ~193-~193: There might be a mistake here.
Context: ...ter hygraph_cb_trips_total increments. 2. Open → Fallback: While the breaker is ...
(QB_NEW_EN)
🔇 Additional comments (1)
backend/api/routes_sync.py (1)
71-168: ...
| async def pull_materials(cls, db, page_size: Optional[int] = None) -> Dict[str, Any]: | ||
| query = """ | ||
| query Materials($first: Int!) { | ||
| materials(first: $first) { | ||
| id | ||
| updatedAt | ||
| } | ||
| } | ||
| """ | ||
| return await cls._pull_resource("materials", query, page_size=page_size) | ||
|
|
||
| @classmethod | ||
| async def pull_modules(cls, db, page_size: Optional[int] = None) -> Dict[str, Any]: | ||
| query = """ | ||
| query Modules($first: Int!) { | ||
| modules(first: $first) { | ||
| id | ||
| updatedAt | ||
| } | ||
| } | ||
| """ | ||
| return await cls._pull_resource("modules", query, page_size=page_size) | ||
|
|
||
| @classmethod | ||
| async def pull_systems(cls, db, page_size: Optional[int] = None) -> Dict[str, Any]: | ||
| query = """ | ||
| query Systems($first: Int!) { | ||
| systems(first: $first) { | ||
| id | ||
| updatedAt | ||
| } | ||
| } | ||
| """ | ||
| return await cls._pull_resource("systems", query, page_size=page_size) | ||
|
|
||
| @classmethod | ||
| async def pull_all(cls, db, page_size: Optional[int] = None) -> Dict[str, Any]: | ||
| try: | ||
| materials = await cls.pull_materials(db, page_size=page_size) | ||
| modules = await cls.pull_modules(db, page_size=page_size) | ||
| systems = await cls.pull_systems(db, page_size=page_size) | ||
| except CircuitOpenError as err: | ||
| fallback_raw = err.fallback | ||
| if isinstance(fallback_raw, dict) and "resource" in fallback_raw: | ||
| fallback = {fallback_raw["resource"]: fallback_raw} | ||
| elif isinstance(fallback_raw, dict): | ||
| fallback = fallback_raw | ||
| else: | ||
| fallback = {} | ||
| if not fallback: | ||
| fallback = {} | ||
| for key in ("materials", "modules", "systems"): | ||
| cached = _CACHE.get(key) or [] | ||
| if cached: | ||
| fallback[key] = _fallback_envelope(key, cached) | ||
| raise CircuitOpenError("Hygraph circuit open", fallback=fallback) from err | ||
|
|
||
| return { | ||
| "materials": materials["processed"], | ||
| "modules": modules["processed"], | ||
| "systems": systems["processed"], | ||
| } |
There was a problem hiding this comment.
Annotate the db parameter to unblock mypy.
pull_materials, pull_modules, pull_systems, and pull_all now fail CI with “Function is missing a type annotation for one or more arguments” because db is untyped. Please add an annotation (e.g. Any or the concrete session type you expect) to each of these signatures.
- async def pull_materials(cls, db, page_size: Optional[int] = None) -> Dict[str, Any]:
+ async def pull_materials(cls, db: Any, page_size: Optional[int] = None) -> Dict[str, Any]:Repeat the same fix for the other pull_* methods so the mypy check passes.
📝 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.
| async def pull_materials(cls, db, page_size: Optional[int] = None) -> Dict[str, Any]: | |
| query = """ | |
| query Materials($first: Int!) { | |
| materials(first: $first) { | |
| id | |
| updatedAt | |
| } | |
| } | |
| """ | |
| return await cls._pull_resource("materials", query, page_size=page_size) | |
| @classmethod | |
| async def pull_modules(cls, db, page_size: Optional[int] = None) -> Dict[str, Any]: | |
| query = """ | |
| query Modules($first: Int!) { | |
| modules(first: $first) { | |
| id | |
| updatedAt | |
| } | |
| } | |
| """ | |
| return await cls._pull_resource("modules", query, page_size=page_size) | |
| @classmethod | |
| async def pull_systems(cls, db, page_size: Optional[int] = None) -> Dict[str, Any]: | |
| query = """ | |
| query Systems($first: Int!) { | |
| systems(first: $first) { | |
| id | |
| updatedAt | |
| } | |
| } | |
| """ | |
| return await cls._pull_resource("systems", query, page_size=page_size) | |
| @classmethod | |
| async def pull_all(cls, db, page_size: Optional[int] = None) -> Dict[str, Any]: | |
| try: | |
| materials = await cls.pull_materials(db, page_size=page_size) | |
| modules = await cls.pull_modules(db, page_size=page_size) | |
| systems = await cls.pull_systems(db, page_size=page_size) | |
| except CircuitOpenError as err: | |
| fallback_raw = err.fallback | |
| if isinstance(fallback_raw, dict) and "resource" in fallback_raw: | |
| fallback = {fallback_raw["resource"]: fallback_raw} | |
| elif isinstance(fallback_raw, dict): | |
| fallback = fallback_raw | |
| else: | |
| fallback = {} | |
| if not fallback: | |
| fallback = {} | |
| for key in ("materials", "modules", "systems"): | |
| cached = _CACHE.get(key) or [] | |
| if cached: | |
| fallback[key] = _fallback_envelope(key, cached) | |
| raise CircuitOpenError("Hygraph circuit open", fallback=fallback) from err | |
| return { | |
| "materials": materials["processed"], | |
| "modules": modules["processed"], | |
| "systems": systems["processed"], | |
| } | |
| async def pull_materials(cls, db: Any, page_size: Optional[int] = None) -> Dict[str, Any]: | |
| query = """ | |
| query Materials($first: Int!) { | |
| materials(first: $first) { | |
| id | |
| updatedAt | |
| } | |
| } | |
| """ | |
| return await cls._pull_resource("materials", query, page_size=page_size) |
🤖 Prompt for AI Agents
In backend/services/hygraph_service.py around lines 133-194, the db parameter on
pull_materials, pull_modules, pull_systems, and pull_all is untyped which fails
mypy; add an explicit type annotation (e.g. db: Any or the concrete session type
you use) to each of those function signatures and ensure Any is imported from
typing (or replace Any with your session type), and apply the same annotation to
any other pull_* methods in the file so the mypy check passes.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
backend/services/hygraph_service.py (2)
133-166: Annotate thedbparameter to unblock mypy.The
dbparameter lacks a type annotation, causing mypy failures. Add an explicit type annotation (e.g.,Anyor the concrete session type) topull_materials,pull_modules, andpull_systems.Apply this diff:
@classmethod - async def pull_materials(cls, db, page_size: Optional[int] = None) -> Dict[str, Any]: + async def pull_materials(cls, db: Any, page_size: Optional[int] = None) -> Dict[str, Any]: query = """ query Materials($first: Int!) { materials(first: $first) { id updatedAt } } """ return await cls._pull_resource("materials", query, page_size=page_size) @classmethod - async def pull_modules(cls, db, page_size: Optional[int] = None) -> Dict[str, Any]: + async def pull_modules(cls, db: Any, page_size: Optional[int] = None) -> Dict[str, Any]: query = """ query Modules($first: Int!) { modules(first: $first) { id updatedAt } } """ return await cls._pull_resource("modules", query, page_size=page_size) @classmethod - async def pull_systems(cls, db, page_size: Optional[int] = None) -> Dict[str, Any]: + async def pull_systems(cls, db: Any, page_size: Optional[int] = None) -> Dict[str, Any]: query = """ query Systems($first: Int!) { systems(first: $first) { id updatedAt } } """ return await cls._pull_resource("systems", query, page_size=page_size)Ensure
Anyis imported fromtypingat the top of the file.
169-169: Annotate thedbparameter to unblock mypy.The
dbparameter lacks a type annotation, causing mypy failures. Add an explicit type annotation (e.g.,Anyor the concrete session type).Apply this diff:
@classmethod - async def pull_all(cls, db, page_size: Optional[int] = None) -> Dict[str, Any]: + async def pull_all(cls, db: Any, page_size: Optional[int] = None) -> Dict[str, Any]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
backend/services/cache.py(1 hunks)backend/services/hygraph_service.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/services/hygraph_service.py (4)
backend/api/config.py (1)
get_settings(66-69)backend/services/cache.py (2)
RedisCache(25-87)get(41-58)backend/services/circuit_breaker.py (1)
CircuitOpenError(13-18)backend/services/hygraph_client.py (1)
execute(32-65)
🪛 GitHub Actions: CI
backend/services/cache.py
[error] 12-12: Typos/lint issues detected by ruff/typos; possibly unused type-ignore comments or redefinition.
🔇 Additional comments (4)
backend/services/cache.py (3)
25-36: LGTM!The initialization logic correctly handles the optional Redis dependency, falls back to environment variables for configuration, and sets up thread-safe in-memory storage. The lazy initialization pattern for the Redis client is appropriate.
41-58: Redis error handling now addresses past review concerns, but consider observability.The try/except block correctly handles
RedisErrorand falls back to the in-memory store, resolving the critical issue flagged in previous reviews. However, when Redis fails, the cache silently degrades with no logging or metrics, making it difficult for operators to detect and respond to Redis outages.Consider adding logging when Redis becomes unavailable:
try: raw = self._client.get(namespaced) except RedisError: + # Log or emit metric: Redis unavailable, falling back to memory self._client = NoneAdditionally, note that once
_clientis set toNone, it never reconnects. If Redis has a transient issue, the cache will remain in memory-only mode for the lifetime of this instance. Depending on your deployment model, you may want to add a reconnection mechanism or rely on pod/process restarts.
77-87: LGTM with same observability note.The
clearmethod correctly implements the same Redis-first, memory-fallback pattern. The use ofpop(namespaced, None)is safe and appropriate. As withgetandset, consider adding logging when Redis becomes unavailable at line 83 for operational visibility.backend/services/hygraph_service.py (1)
192-207: Fallback merging logic addresses the prior concern.The logic at lines 192-207 attempts to preserve cached results for all resources when the circuit opens, which addresses the concern raised in the previous review. The implementation checks if each resource is already in the fallback, then tries successful results, then falls back to cache. While complex, this ensures all three resource types are included in the fallback response.
Note: The refactor suggested in the previous comment (replacing
locals()) will make this logic clearer without changing its behavior.
backend/services/cache.py
Outdated
| try: # pragma: no cover - optional dependency | ||
| import redis # type: ignore | ||
| except Exception: # pragma: no cover - optional dependency | ||
| redis = None # type: ignore[assignment] |
There was a problem hiding this comment.
Address the pipeline failure by refining the type-ignore comment.
The pipeline failure at line 12 indicates that the broad # type: ignore comment may be unnecessary or should be more specific. Consider using a more precise suppression like # type: ignore[import-not-found] or remove it if the import can be typed correctly with TYPE_CHECKING guards.
-try: # pragma: no cover - optional dependency
- import redis # type: ignore
-except Exception: # pragma: no cover - optional dependency
- redis = None # type: ignore[assignment]
+try: # pragma: no cover - optional dependency
+ import redis
+except ImportError: # pragma: no cover - optional dependency
+ redis = None # type: ignore[assignment]Note: Also consider catching only ImportError rather than the broad Exception for clarity.
📝 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.
| try: # pragma: no cover - optional dependency | |
| import redis # type: ignore | |
| except Exception: # pragma: no cover - optional dependency | |
| redis = None # type: ignore[assignment] | |
| try: # pragma: no cover - optional dependency | |
| import redis | |
| except ImportError: # pragma: no cover - optional dependency | |
| redis = None # type: ignore[assignment] |
🧰 Tools
🪛 GitHub Actions: CI
[error] 12-12: Typos/lint issues detected by ruff/typos; possibly unused type-ignore comments or redefinition.
🤖 Prompt for AI Agents
In backend/services/cache.py around lines 11 to 14, the import block uses a
broad Exception catch and an unspecific "# type: ignore" which triggered the
pipeline; change the except to catch ImportError only, and replace the blanket
"# type: ignore" on the import with a targeted suppression like "# type:
ignore[import-not-found]" (or remove it if you add a conditional TYPE_CHECKING
guard and proper typing), and keep the assignment to redis = None with a
specific "# type: ignore[assignment]" if mypy still complains.
backend/services/cache.py
Outdated
| def set(self, key: str, value: Any, *, ttl: int | None = None) -> None: | ||
| payload = json.dumps(value) | ||
| namespaced = self._ns(key) | ||
| if self._client is not None: | ||
| try: | ||
| if ttl is not None: | ||
| self._client.setex(namespaced, ttl, payload) | ||
| else: | ||
| self._client.set(namespaced, payload) | ||
| except RedisError: | ||
| self._client = None | ||
| else: | ||
| return | ||
| expires_at = (time.time() + ttl) if ttl else None | ||
| with self._lock: | ||
| self._memory[namespaced] = (payload, expires_at) |
There was a problem hiding this comment.
Handle JSON serialization errors to prevent cache failures.
If value cannot be serialized to JSON (e.g., it contains non-serializable objects), line 61 will raise a TypeError or ValueError, causing the entire set operation to fail. Consider wrapping the serialization in a try/except to gracefully handle such cases.
def set(self, key: str, value: Any, *, ttl: int | None = None) -> None:
- payload = json.dumps(value)
+ try:
+ payload = json.dumps(value)
+ except (TypeError, ValueError) as e:
+ # Log the error or raise a custom exception
+ return # or handle appropriately
namespaced = self._ns(key)Additionally, similar to the get method, consider adding logging when Redis fails at lines 69-70.
📝 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.
| def set(self, key: str, value: Any, *, ttl: int | None = None) -> None: | |
| payload = json.dumps(value) | |
| namespaced = self._ns(key) | |
| if self._client is not None: | |
| try: | |
| if ttl is not None: | |
| self._client.setex(namespaced, ttl, payload) | |
| else: | |
| self._client.set(namespaced, payload) | |
| except RedisError: | |
| self._client = None | |
| else: | |
| return | |
| expires_at = (time.time() + ttl) if ttl else None | |
| with self._lock: | |
| self._memory[namespaced] = (payload, expires_at) | |
| def set(self, key: str, value: Any, *, ttl: int | None = None) -> None: | |
| try: | |
| payload = json.dumps(value) | |
| except (TypeError, ValueError) as e: | |
| # Log the error or raise a custom exception | |
| return # or handle appropriately | |
| namespaced = self._ns(key) | |
| if self._client is not None: | |
| try: | |
| if ttl is not None: | |
| self._client.setex(namespaced, ttl, payload) | |
| else: | |
| self._client.set(namespaced, payload) | |
| except RedisError: | |
| self._client = None | |
| else: | |
| return | |
| expires_at = (time.time() + ttl) if ttl else None | |
| with self._lock: | |
| self._memory[namespaced] = (payload, expires_at) |
backend/services/hygraph_service.py
Outdated
| successful_results = { | ||
| "materials": locals().get("materials"), | ||
| "modules": locals().get("modules"), | ||
| "systems": locals().get("systems"), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace locals() with explicit variable tracking.
Using locals() to capture successful results is fragile and non-Pythonic. The dictionary won't contain materials, modules, or systems if the exception is raised before they're assigned, and relies on implementation details of variable scoping.
Refactor to track results explicitly:
+ materials_result: Dict[str, Any] | None = None
+ modules_result: Dict[str, Any] | None = None
+ systems_result: Dict[str, Any] | None = None
+
try:
- materials = await cls.pull_materials(db, page_size=page_size)
- modules = await cls.pull_modules(db, page_size=page_size)
- systems = await cls.pull_systems(db, page_size=page_size)
+ materials_result = await cls.pull_materials(db, page_size=page_size)
+ modules_result = await cls.pull_modules(db, page_size=page_size)
+ systems_result = await cls.pull_systems(db, page_size=page_size)
except CircuitOpenError as err:
fallback_raw = err.fallback
fallback: dict[str, dict[str, Any]] = {}
if isinstance(fallback_raw, dict) and "resource" in fallback_raw:
resource_name = fallback_raw.get("resource")
if isinstance(resource_name, str):
fallback[resource_name] = fallback_raw
elif isinstance(fallback_raw, dict):
for key, value in fallback_raw.items():
if isinstance(key, str) and isinstance(value, dict):
fallback[key] = value
successful_results = {
- "materials": locals().get("materials"),
- "modules": locals().get("modules"),
- "systems": locals().get("systems"),
+ "materials": materials_result,
+ "modules": modules_result,
+ "systems": systems_result,
}
for key in ("materials", "modules", "systems"):
if key in fallback and isinstance(fallback[key], dict):
continue
result = successful_results.get(key)
if isinstance(result, dict):
items = result.get("items")
if not isinstance(items, list):
items = []
fallback[key] = _fallback_envelope(key, items)
continue
cached_items = _CACHE.get(key) or []
if not isinstance(cached_items, list):
cached_items = []
fallback[key] = _fallback_envelope(key, cached_items)
raise CircuitOpenError("Hygraph circuit open", fallback=fallback) from err
return {
- "materials": materials["processed"],
- "modules": modules["processed"],
- "systems": systems["processed"],
+ "materials": materials_result["processed"],
+ "modules": modules_result["processed"],
+ "systems": systems_result["processed"],
}📝 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.
| successful_results = { | |
| "materials": locals().get("materials"), | |
| "modules": locals().get("modules"), | |
| "systems": locals().get("systems"), | |
| } | |
| materials_result: Dict[str, Any] | None = None | |
| modules_result: Dict[str, Any] | None = None | |
| systems_result: Dict[str, Any] | None = None | |
| try: | |
| materials_result = await cls.pull_materials(db, page_size=page_size) | |
| modules_result = await cls.pull_modules(db, page_size=page_size) | |
| systems_result = await cls.pull_systems(db, page_size=page_size) | |
| except CircuitOpenError as err: | |
| fallback_raw = err.fallback | |
| fallback: dict[str, dict[str, Any]] = {} | |
| if isinstance(fallback_raw, dict) and "resource" in fallback_raw: | |
| resource_name = fallback_raw.get("resource") | |
| if isinstance(resource_name, str): | |
| fallback[resource_name] = fallback_raw | |
| elif isinstance(fallback_raw, dict): | |
| for key, value in fallback_raw.items(): | |
| if isinstance(key, str) and isinstance(value, dict): | |
| fallback[key] = value | |
| successful_results = { | |
| "materials": materials_result, | |
| "modules": modules_result, | |
| "systems": systems_result, | |
| } | |
| for key in ("materials", "modules", "systems"): | |
| if key in fallback and isinstance(fallback[key], dict): | |
| continue | |
| result = successful_results.get(key) | |
| if isinstance(result, dict): | |
| items = result.get("items") | |
| if not isinstance(items, list): | |
| items = [] | |
| fallback[key] = _fallback_envelope(key, items) | |
| continue | |
| cached_items = _CACHE.get(key) or [] | |
| if not isinstance(cached_items, list): | |
| cached_items = [] | |
| fallback[key] = _fallback_envelope(key, cached_items) | |
| raise CircuitOpenError("Hygraph circuit open", fallback=fallback) from err | |
| return { | |
| "materials": materials_result["processed"], | |
| "modules": modules_result["processed"], | |
| "systems": systems_result["processed"], | |
| } |
🤖 Prompt for AI Agents
In backend/services/hygraph_service.py around lines 186–190, the code uses
locals() to assemble successful_results which is fragile and can omit keys if an
exception occurs; instead explicitly track and assemble the results: ensure
materials, modules, and systems are declared/initialized (e.g., set to None)
before the try block or assigned as you compute them, and then build
successful_results = {"materials": materials, "modules": modules, "systems":
systems} (or populate keys only when values are available), removing the
locals() call.
|
Note Docstrings generation - SUCCESS |
…nt-m5f9cc` Docstrings generation was requested by @shayancoin. * #124 (comment) The following files were modified: * `backend/api/routes_sync.py` * `backend/services/cache.py` * `backend/services/circuit_breaker.py` * `backend/services/hygraph_client.py` * `backend/services/hygraph_service.py`
…nt-m5f9cc` (#283) Docstrings generation was requested by @shayancoin. * #124 (comment) The following files were modified: * `backend/api/routes_sync.py` * `backend/services/cache.py` * `backend/services/circuit_breaker.py` * `backend/services/hygraph_client.py` * `backend/services/hygraph_service.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_68f13509f73c83308d2ebd4ab42db96d
Summary by CodeRabbit
Bug Fixes
Documentation