Merged
Conversation
* chore(deps): update yarn to v4.12.0 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Jon Kafton <939376+jonkafton@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* fix(deps): update dependency react-slick to ^0.31.0 * Lockfile --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Jon Kafton <939376+jonkafton@users.noreply.github.com>
* fix: show placeholders for media and course urls --------- Co-authored-by: Ahtesham Quraish <ahtesham.quraish@A006-01455.local>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
#2930) * render course-in-programs data * display multiple dates * add some tests * show some info even when no nextrun availale * fix wording
* Provide a fetch utility to 404 is critical API responses 404 * Handle not found in course and program pages * Class-based extend query client * Mock isServer for tests
* catching inactive rpc error * add logging back in * more robust logging
* Add share popover * Generate metadata from article content * Remove image title from alt fallback - this is the original image filename
* Add option to serve S3 media from a custom domain * Modernize how STORAGES is configured (django 4.2+)
Comment on lines
+307
to
+315
| try: | ||
| similar = get_similar_resources( | ||
| resource_data, limit, 2, 3, use_embeddings=True | ||
| ) | ||
| return Response(LearningResourceSerializer(list(similar), many=True).data) | ||
| except _InactiveRpcError as ircp: | ||
| msg = f"Similar resources RPC error for resource {pk}: {ircp.details()}" | ||
| log.warning(msg) | ||
| raise Http404(msg) from ircp |
There was a problem hiding this comment.
Bug: The code catches a private gRPC exception, _InactiveRpcError, and incorrectly maps all resulting errors to an HTTP 404 status, which can mask the underlying issue.
Severity: MEDIUM
Suggested Fix
Refactor the exception handling to catch the public grpc.RpcError exception. Inspect the error's status code using the .code() method to return an appropriate HTTP status. For example, map grpc.StatusCode.UNAVAILABLE to HTTP 503 (Service Unavailable).
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: learning_resources/views.py#L307-L315
Potential issue: The exception handling for gRPC calls is fragile because it catches
`_InactiveRpcError`, a private implementation detail of the `grpc` library that could
change without notice. Furthermore, it incorrectly maps all such RPC errors to an HTTP
404 (Not Found) response. This is semantically incorrect for service availability issues
(like `UNAVAILABLE` or `DEADLINE_EXCEEDED`), which should return a 5xx error. This
misrepresentation can mislead clients and complicate debugging of infrastructure
problems.
Did we get this right? 👍 / 👎 to inform future reviews.
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
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.
Matt Bertrand
Jon Kafton
Shankar Ambady
Dan Subak
Chris Chudzicki
renovate[bot]
Ahtesham Quraish