Allow OPTIONS request to be used on RelationshipView #633

n2ygk · 2019-05-20T18:40:34Z

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 of instance (if there are ever any) will lead to an exception in to_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 just type and id strings so doesn't actually need to serialize it.

Checklist

PR only contains one change (considered splitting up PR)
unit-test added
documentation updated
CHANGELOG.md updated (only for user relevant changes)
author name in AUTHORS

…l work

sliverc

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."

n2ygk · 2019-05-21T19:41:47Z

Thanks @sliverc, I'll try go get to adding a test case in the next few days.

n2ygk · 2019-05-22T19:32:41Z

@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.

rest_framework_json_api/serializers.py

codecov · 2019-05-22T20:51:17Z

Codecov Report

Merging #633 into master will increase coverage by 0.21%.
The diff coverage is 100%.

@@            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

Impacted Files	Coverage Δ
example/tests/test_views.py	`100% <100%> (ø)`	⬆️
rest_framework_json_api/compat.py	`50% <0%> (-50%)`	⬇️
rest_framework_json_api/django_filters/backends.py	`86.66% <0%> (-13.34%)`	⬇️
rest_framework_json_api/metadata.py	`48.33% <0%> (+23.33%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db5cd72...80673d4. Read the comment docs.

codecov · 2019-05-22T20:51:17Z

Codecov Report

Merging #633 into master will increase coverage by 0.49%.
The diff coverage is 100%.

@@            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

Impacted Files	Coverage Δ
example/tests/test_views.py	`100% <100%> (ø)`	⬆️
rest_framework_json_api/metadata.py	`48.33% <0%> (+23.33%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22476fe...29e1099. Read the comment docs.

n2ygk · 2019-05-23T16:31:45Z

@sliverc I don't understand why these py27 travis jobs are failing. Running tox -e py27-df11-django111-drf36 succeeds locally.

sliverc · 2019-05-24T07:27:33Z

They don't fail on your machine because of cached files I assume. See #634 for a fix.

As we used pinned test dependencies we shouldn't use setup scripts to run tests which doesn't have pinned deps. See #635 Hope I will get to it at some point.

n2ygk · 2019-05-24T14:12:56Z

@sliverc I merged in #634 so checks are passing now. I added OPTIONS test as requested, but see my question above regarding adding self.fields = {}. Ready for your (final? I hope) review. Thanks!

sliverc

Small question otherwise this looks good.

rest_framework_json_api/serializers.py

n2ygk · 2019-05-24T14:53:18Z

Small question otherwise this looks good.

@sliverc Looking for the "small question" but don't see it? 😕

sliverc · 2019-05-24T14:58:55Z

@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?

n2ygk · 2019-05-24T15:24:45Z

@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?

sliverc · 2019-05-24T15:36:14Z

I see your point. I have marked this PR to close #315 and let's close the issue #314 manually then.

remove superfluous test for 'instance' so that DRF generateschema wil…

4500303

…l work

n2ygk requested a review from sliverc May 20, 2019 18:40

sliverc requested changes May 21, 2019

View reviewed changes

n2ygk self-assigned this May 21, 2019

add test for OPTIONS on RelationshipView

80673d4

n2ygk commented May 22, 2019

View reviewed changes

rest_framework_json_api/serializers.py Show resolved Hide resolved

n2ygk requested a review from sliverc May 23, 2019 16:19

Merge branch 'master' into issue-314/no_instance_kwarg

5510ba1

sliverc reviewed May 24, 2019

View reviewed changes

rest_framework_json_api/serializers.py Show resolved Hide resolved

Merge branch 'master' into issue-314/no_instance_kwarg

29e1099

n2ygk mentioned this pull request May 24, 2019

Incompatibility with Django Rest Swagger when using RelationshipView #314

Closed

sliverc changed the title ~~remove superfluous test for 'instance' in ResourceIdentifierObjectSerializer~~ Allow OPTIONS request to be used on RelationshipView May 24, 2019

sliverc approved these changes May 24, 2019

View reviewed changes

sliverc merged commit c77fe0f into django-json-api:master May 24, 2019

n2ygk deleted the issue-314/no_instance_kwarg branch June 9, 2019 17:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow OPTIONS request to be used on RelationshipView #633

Allow OPTIONS request to be used on RelationshipView #633

n2ygk commented May 20, 2019 •

edited by sliverc

Loading

sliverc left a comment •

edited

Loading

n2ygk commented May 21, 2019

n2ygk commented May 22, 2019

codecov bot commented May 22, 2019

codecov bot commented May 22, 2019 •

edited

Loading

n2ygk commented May 23, 2019

sliverc commented May 24, 2019

n2ygk commented May 24, 2019

sliverc left a comment

n2ygk commented May 24, 2019

sliverc commented May 24, 2019

n2ygk commented May 24, 2019

sliverc commented May 24, 2019

Allow OPTIONS request to be used on RelationshipView #633

Allow OPTIONS request to be used on RelationshipView #633

Conversation

n2ygk commented May 20, 2019 • edited by sliverc Loading

Description of the Change

Checklist

sliverc left a comment • edited Loading

Choose a reason for hiding this comment

n2ygk commented May 21, 2019

n2ygk commented May 22, 2019

codecov bot commented May 22, 2019

Codecov Report

codecov bot commented May 22, 2019 • edited Loading

Codecov Report

n2ygk commented May 23, 2019

sliverc commented May 24, 2019

n2ygk commented May 24, 2019

sliverc left a comment

Choose a reason for hiding this comment

n2ygk commented May 24, 2019

sliverc commented May 24, 2019

n2ygk commented May 24, 2019

sliverc commented May 24, 2019

n2ygk commented May 20, 2019 •

edited by sliverc

Loading

sliverc left a comment •

edited

Loading

codecov bot commented May 22, 2019 •

edited

Loading