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

Fix MRO to return 405 for unsupported methods #3175

Merged
merged 5 commits into from
Oct 13, 2023
Merged

Fix MRO to return 405 for unsupported methods #3175

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
178c6c6
Fix MRO
dhruvkb Oct 10, 2023
3728854
Convert between DRF and Django request/response types
dhruvkb Oct 10, 2023
fd2d0a2
Add test case for unsupported HTTP method
dhruvkb Oct 11, 2023
65604e1
Drop redundant args and kwargs
dhruvkb Oct 11, 2023
646d5c8
Update URL to test the right endpoint
dhruvkb Oct 12, 2023
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
9 changes: 6 additions & 3 deletions api/api/views/oauth2_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import json
import logging as log
import secrets
import smtplib
Expand Down Expand Up @@ -136,9 +137,9 @@ def get(self, request, code, format=None):


@extend_schema(tags=["auth"])
class TokenView(BaseTokenView, APIView):
class TokenView(APIView, BaseTokenView):
@token
def post(self, *args, **kwargs):
def post(self, request, *args, **kwargs):
dhruvkb marked this conversation as resolved.
Show resolved Hide resolved
"""
Get an access token using client credentials.

Expand All @@ -154,7 +155,9 @@ def post(self, *args, **kwargs):
endpoint.
"""

return super().post(*args, **kwargs)
res = super().post(request._request)
data = json.loads(res.content)
return Response(data, status=res.status_code)
Comment on lines +159 to +160
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it necessary to json.loads the content? Would something like this work without the need for that?

Suggested change
data = json.loads(res.content)
return Response(data, status=res.status_code)
data = json.loads(res.content)
return HttpResponse(content=res.content, content_type="application/json", status=res.status_code)

HttpResponse is the more low-level version from django.http that allows you to bypass the DRF Response data parsing. Doing json.loads and then passing it directly to Response means we're unnecessary marshalling the JSON string to Python and then immediately back to a string.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Although, more general question: why is it necessary to do anything manual here at all, other than just return res like we used to? Does APIView complain about not receiving the right type of response object or something?

Copy link
Member Author

@dhruvkb dhruvkb Oct 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Using the Response class simply provides a nicer interface for returning content-negotiated Web API responses, that can be rendered to multiple formats.
— DRF docs

We can use HttpResponse for sure, but sending DRF's Response is more consistent and it renders the DRF API UI in the browser like our other endpoints.

I agree that it's one more step of JSON parsing followed by serialization but DRF doesn't allow any other way to set data where the content_type can be negotiated automatically.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can use HttpResponse for sure, but sending DRF's Response is more consistent and it renders the DRF API UI in the browser like our other endpoints.

Ahh, interesting, I didn't realise that was a side effect. For this view, it doesn't seem like a tragic loss, though? It's not exactly a "browseable" response 🤔 At least not any less so if your browser just renders a nice JSON explorer (Firefox does this, not sure of others).

I also don't think we want to support anyone using this view over anything than JSON anyway, but I guess Response handles it regardless so 🤷 I would personally use HttpResponse but if you prefer to keep it, all good with me as well 👍



@extend_schema(tags=["auth"])
Expand Down