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.

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 a bug in options related to read-only RelatedField #2981

Merged
merged 1 commit into from Jun 2, 2015

Conversation

jannon
Copy link

@jannon jannon commented May 28, 2015

This fixes #2811

Problem
The issue was discovered when an OPTIONS request to an endpoint blew up with the following error:

...
rest_framework/views.py:451: in dispatch
    response = self.handle_exception(exc)
rest_framework/views.py:448: in dispatch
    response = handler(request, *args, **kwargs)
rest_framework/views.py:462: in options
    data = self.metadata_class().determine_metadata(request, self)
rest_framework/metadata.py:64: in determine_metadata
    actions = self.determine_actions(request, view)
rest_framework/metadata.py:90: in determine_actions
    actions[method] = self.get_serializer_info(serializer)
rest_framework/metadata.py:107: in get_serializer_info
    for field_name, field in serializer.fields.items()
rest_framework/metadata.py:107: in <listcomp>
    for field_name, field in serializer.fields.items()
rest_framework/metadata.py:130: in get_field_info
    if hasattr(field, 'choices'):
rest_framework/relations.py:382: in choices
    for item in iterable

Tracing the problem, I found that iterable is None because the queryset from which it derives is None because the PrimaryKeyRelatedField from which it derives is read-only.

Solution
It is expected that the OPTIONS request should succeed and simply not list choices for read-only fields since, a user cannot submit data to them anyway.

This pull request contains both a test that fails on existing code and a fix in SimpleMetaData.get_field_info that checks if a field is read-only before attempting to get choices

}
for choice_value, choice_name in field.choices.items()
]
if 'read_only' in field_info and not field_info['read_only']:
Copy link
Member

@tomchristie tomchristie May 29, 2015

Choose a reason for hiding this comment

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

Can we instead do if not field_info.get('read_only'): ...?

Copy link
Author

@jannon jannon May 30, 2015

Choose a reason for hiding this comment

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

Done

@jannon jannon force-pushed the fix-model-serializer-metadata branch from 8fd2585 to 9b22f43 Compare May 30, 2015
for choice_value, choice_name in field.choices.items()
]
if not field_info.get('read_only'):
if hasattr(field, 'choices'):
Copy link
Member

@kevin-brown kevin-brown May 30, 2015

Choose a reason for hiding this comment

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

In theory can't these two be combined?

if not field_info.get('read_only') and hasattr(field, 'choices'):

Copy link
Member

@tomchristie tomchristie Jun 1, 2015

Choose a reason for hiding this comment

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

Seconded

@jannon jannon force-pushed the fix-model-serializer-metadata branch from 9b22f43 to a97c989 Compare Jun 2, 2015
@xordoquy
Copy link
Collaborator

xordoquy commented Jun 2, 2015

Looks good to me. Thanks !!

xordoquy added a commit that referenced this pull request Jun 2, 2015
Read-Only RelatedField Metadata Failure Test and Fix
@xordoquy xordoquy merged commit 734bf3c into encode:master Jun 2, 2015
1 check passed
@xordoquy xordoquy changed the title Read-Only RelatedField Metadata Failure Test and Fix Fix a bug with read-only RelatedField in OPTIONS Jun 3, 2015
@xordoquy xordoquy changed the title Fix a bug with read-only RelatedField in OPTIONS Fix a bug in options related to read-only RelatedField Jun 3, 2015
@jannon jannon deleted the fix-model-serializer-metadata branch Jun 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ManyRelatedField.choices tries to iterate over None
4 participants