Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return status 404 instead of 500 when media not found #3552

Merged
merged 1 commit into from
Dec 19, 2023
Merged

Return status 404 instead of 500 when media not found #3552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions api/api/controllers/elasticsearch/related.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ def related_media(uuid: str, index: str, filter_dead: bool) -> list[Hit]:

# Search the default index for the item itself as it might be sensitive.
item_search = Search(index=index)
# This will raise ``IndexError`` if no hits are found. This error is caught
# in the viewset handler function.
item_hit = item_search.query(Term(identifier=uuid)).execute().hits[0]

# Match related using title.
Expand Down
4 changes: 2 additions & 2 deletions api/api/views/media_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

from rest_framework import status
from rest_framework.decorators import action
from rest_framework.exceptions import APIException
from rest_framework.exceptions import APIException, NotFound
from rest_framework.response import Response
from rest_framework.viewsets import ReadOnlyModelViewSet

Expand Down Expand Up @@ -272,7 +272,7 @@ def related(self, request, identifier=None, *_, **__):
raise APIException(getattr(e, "message", str(e)))
# If there are no hits in the search controller
except IndexError:
raise APIException("Could not find items.", 404)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this was supposed to be raising a 404, why wasn't it working before?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because the second parameter isn't the status code, it's the error code: https://github.com/encode/django-rest-framework/blob/master/rest_framework/exceptions.py#L99-L114. Typically this is something like a slug that identifies the error type within the status code: https://github.com/encode/django-rest-framework/blob/master/rest_framework/exceptions.py#L183-L192. This lets you have multiple distinct error types within a single semantic HTTP status code that are identifiable on the client without needing to parse the potentially arbitrary error message string.

raise NotFound
Copy link
Collaborator

Choose a reason for hiding this comment

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

TIL raise accepts a class object rather than an instance for lazy evaluation of the parameterless constructor 😮 https://docs.python.org/3/reference/simple_stmts.html#the-raise-statement

It must be either a subclass or an instance of BaseException. If it is a class, the exception instance will be obtained when needed by instantiating the class with no arguments.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Woah, TIL too!


serializer_context = self.get_serializer_context()

Expand Down
Loading