-
Notifications
You must be signed in to change notification settings - Fork 299
Allow OPTIONS request to be used on RelationshipView #633
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.
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
Allow OPTIONS request to be used on RelationshipView #633
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code snippet looks odd indeed especially as self.instance is not used anywhere.
I think this can be safely removed. This fix is also related to #315 so it would be good to add a test running OPTIONS on RelationshipView so to check that everything works.
A changelog entry would also be good stating something like "Allow swagger and OPTIONS request to be used on RelationshipView."
Thanks @sliverc, I'll try go get to adding a test case in the next few days.
@sliverc hmm, I added a test and options didn't go so smoothly as I would have expected:
def test_options_entry_relationship_blog(self):
url = reverse(
'entry-relationships', kwargs={'pk': self.first_entry.id, 'related_field': 'blog'}
)
response = self.client.options(url)
expected_data = {'type': format_resource_type('Blog'), 'id': str(self.first_entry.blog.id)}
print("RESPONSE: {}".format(response))
# assert response.data == expected_data
=================================== FAILURES ===================================
__________ TestRelationshipView.test_options_entry_relationship_blog ___________
self = <example.tests.test_views.TestRelationshipView testMethod=test_options_entry_relationship_blog>
def test_options_entry_relationship_blog(self):
url = reverse(
'entry-relationships', kwargs={'pk': self.first_entry.id, 'related_field': 'blog'}
)
> response = self.client.options(url)
example/tests/test_views.py:281:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
env/lib/python3.7/site-packages/rest_framework/test.py:332: in options
path, data=data, format=format, content_type=content_type, **extra)
env/lib/python3.7/site-packages/rest_framework/test.py:229: in options
return self.generic('OPTIONS', path, data, content_type, **extra)
env/lib/python3.7/site-packages/rest_framework/test.py:238: in generic
method, path, data, content_type, secure, **extra)
env/lib/python3.7/site-packages/django/test/client.py:414: in generic
return self.request(**r)
env/lib/python3.7/site-packages/rest_framework/test.py:289: in request
return super(APIClient, self).request(**kwargs)
env/lib/python3.7/site-packages/rest_framework/test.py:241: in request
request = super(APIRequestFactory, self).request(**kwargs)
env/lib/python3.7/site-packages/django/test/client.py:495: in request
raise exc_value
env/lib/python3.7/site-packages/django/core/handlers/exception.py:34: in inner
response = get_response(request)
env/lib/python3.7/site-packages/django/core/handlers/base.py:126: in _get_response
response = self.process_exception_by_middleware(e, request)
env/lib/python3.7/site-packages/django/core/handlers/base.py:124: in _get_response
response = wrapped_callback(request, *callback_args, **callback_kwargs)
env/lib/python3.7/site-packages/django/views/decorators/csrf.py:54: in wrapped_view
return view_func(*args, **kwargs)
env/lib/python3.7/site-packages/django/views/generic/base.py:68: in view
return self.dispatch(request, *args, **kwargs)
env/lib/python3.7/site-packages/rest_framework/views.py:495: in dispatch
response = self.handle_exception(exc)
env/lib/python3.7/site-packages/rest_framework/views.py:455: in handle_exception
self.raise_uncaught_exception(exc)
env/lib/python3.7/site-packages/rest_framework/views.py:492: in dispatch
response = handler(request, *args, **kwargs)
env/lib/python3.7/site-packages/rest_framework/views.py:506: in options
data = self.metadata_class().determine_metadata(request, self)
rest_framework_json_api/metadata.py:67: in determine_metadata
actions = self.determine_actions(request, view)
env/lib/python3.7/site-packages/rest_framework/metadata.py:96: in determine_actions
actions[method] = self.get_serializer_info(serializer)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <rest_framework_json_api.metadata.JSONAPIMetadata object at 0x1098952b0>
serializer = ResourceIdentifierObjectSerializer(context={'request': <rest_framework.request.Request object>, 'format': None, 'view': <example.views.EntryRelationshipView object>})
def get_serializer_info(self, serializer):
"""
Given an instance of a serializer, return a dictionary of metadata
about its fields.
"""
if hasattr(serializer, 'child'):
# If this is a `ListSerializer` then we want to examine the
# underlying child serializer instance instead.
serializer = serializer.child
# Remove the URL field if present
> serializer.fields.pop(api_settings.URL_FIELD_NAME, None)
E AttributeError: 'ResourceIdentifierObjectSerializer' object has no attribute 'fields'
rest_framework_json_api/metadata.py:83: AttributeError
------------------------------ Captured log call -------------------------------
ERROR django.request:log.py:228 Internal Server Error: /entries/1/relationships/blog
Traceback (most recent call last):
File "/Users/alan/src/django-rest-framework-json-api/env/lib/python3.7/site-packages/django/core/handlers/exception.py", line 34, in inner
response = get_response(request)
File "/Users/alan/src/django-rest-framework-json-api/env/lib/python3.7/site-packages/django/core/handlers/base.py", line 126, in _get_response
response = self.process_exception_by_middleware(e, request)
File "/Users/alan/src/django-rest-framework-json-api/env/lib/python3.7/site-packages/django/core/handlers/base.py", line 124, in _get_response
response = wrapped_callback(request, *callback_args, **callback_kwargs)
File "/Users/alan/src/django-rest-framework-json-api/env/lib/python3.7/site-packages/django/views/decorators/csrf.py", line 54, in wrapped_view
return view_func(*args, **kwargs)
File "/Users/alan/src/django-rest-framework-json-api/env/lib/python3.7/site-packages/django/views/generic/base.py", line 68, in view
return self.dispatch(request, *args, **kwargs)
File "/Users/alan/src/django-rest-framework-json-api/env/lib/python3.7/site-packages/rest_framework/views.py", line 495, in dispatch
response = self.handle_exception(exc)
File "/Users/alan/src/django-rest-framework-json-api/env/lib/python3.7/site-packages/rest_framework/views.py", line 455, in handle_exception
self.raise_uncaught_exception(exc)
File "/Users/alan/src/django-rest-framework-json-api/env/lib/python3.7/site-packages/rest_framework/views.py", line 492, in dispatch
response = handler(request, *args, **kwargs)
File "/Users/alan/src/django-rest-framework-json-api/env/lib/python3.7/site-packages/rest_framework/views.py", line 506, in options
data = self.metadata_class().determine_metadata(request, self)
File "/Users/alan/src/django-rest-framework-json-api/rest_framework_json_api/metadata.py", line 67, in determine_metadata
actions = self.determine_actions(request, view)
File "/Users/alan/src/django-rest-framework-json-api/env/lib/python3.7/site-packages/rest_framework/metadata.py", line 96, in determine_actions
actions[method] = self.get_serializer_info(serializer)
File "/Users/alan/src/django-rest-framework-json-api/rest_framework_json_api/metadata.py", line 83, in get_serializer_info
serializer.fields.pop(api_settings.URL_FIELD_NAME, None)
AttributeError: 'ResourceIdentifierObjectSerializer' object has no attribute 'fields'
==================== 1 failed, 196 passed in 66.11 seconds =====================
Not sure if fixing ResourceIdentifierObjectSerializer is the right thing to do.... Digging some more.
Codecov Report
@@ Coverage Diff @@ ## master #633 +/- ## ========================================== + Coverage 95.66% 95.88% +0.21% ========================================== Files 56 56 Lines 2861 2866 +5 ========================================== + Hits 2737 2748 +11 + Misses 124 118 -6
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@ ## master #633 +/- ## ========================================== + Coverage 95.66% 96.16% +0.49% ========================================== Files 56 56 Lines 2861 2866 +5 ========================================== + Hits 2737 2756 +19 + Misses 124 110 -14
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small question otherwise this looks good.
Small question otherwise this looks good.
@sliverc Looking for the "small question" but don't see it? 😕
@n2ygk Odd... I added a comment to CHANGELOG entry whether this PR actually also fixes #314 and whether this should be mentioned in the changelog as well?
@sliverc (Hmm, I suspect that github is eventually consistent and your comment might show up one day;-)
I was uncomfortable specifically asserting that it fixes #314 without a test case. In general the problem with Swagger-UI javascript client is that they do an OPTIONS request so I expect this is also the case here. So the DRS code might work, but it will be incorrect JSONAPI:
The Django REST Swagger schema is incorrect (not JSONAPI) as it uses DRF's coreapi schema. The need for that library will likely go away with DRF 3.10's generateschema along with the JSONAPI extension of it. So maybe just close #314 with a reference to #604?
Uh oh!
There was an error while loading. Please reload this page.
Fixes #315
Description of the Change
Remove test for 'instance' from
__init__()so that a call to get the RelationshipSerializer without an instance can be used by DRF's generateschema (3.10 milestone WIP). Missing values ofinstance(if there are ever any) will lead to an exception into_representation(). There are no negative test cases that currently check for this. The JSONAPI openapi schema generator (WIP; not yet submitted here) "knows" that the openapi schema for a Resource Identifier is justtypeandidstrings so doesn't actually need to serialize it.Checklist
CHANGELOG.mdupdated (only for user relevant changes)AUTHORS