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

OrderingFilter should call get_serializer_class() to determine default fields. #3964

Closed
wants to merge 1 commit into from

Conversation

bmampaey
Copy link

@bmampaey bmampaey commented Feb 25, 2016

refs #3957

I made the proposed change

Because the default get_serializer_class method of GenericAPIView throw an Assertion error if there is no serializer_class defined on the view, I made a try except block that catches the exception and return the message as defined previously. I don't know if that is the correct idea.

I also added 3 tests for this fix.

… view in OrderingFilter backend instead of calling the view's get_serializer_class method
@jpadilla jpadilla added this to the 3.3.3 Release milestone Feb 25, 2016
@jpadilla
Copy link
Member

jpadilla commented Feb 25, 2016

@bmampaey Thanks! LGTM 👍

@xordoquy
Copy link
Collaborator

xordoquy commented Feb 29, 2016

Need to figure out why the assertion is raised.

@kevin-brown
Copy link
Member

kevin-brown commented Feb 29, 2016

@xordoquy My guess is during the check for serializer_class.

@jpadilla
Copy link
Member

jpadilla commented Feb 29, 2016

Yep yep what @kevin-brown said

@xordoquy
Copy link
Collaborator

xordoquy commented Mar 1, 2016

ok, let me rephrase this. I'm a bit reluctant to leave the assertion error assume it'll only be the get_serializer_class configuration error.
On the other hand, I don't have better options yet.

@jpadilla
Copy link
Member

jpadilla commented Mar 1, 2016

@xordoquy I see what you mean, but I don't think we're not even initializing a serializer in there, so I'm not sure from where else we could get thrown an assertion error at that point.

if serializer_class is None:
try:
serializer_class = view.get_serializer_class()
except AssertionError:
Copy link
Member

@tomchristie tomchristie Mar 4, 2016

Choose a reason for hiding this comment

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

Might be better to use except (AssertionError, AttributeError) here, in case get_serializer_class does not exist on the view (eg using basic APIView, rather than the generic classes)

@kmwenja
Copy link
Contributor

kmwenja commented May 28, 2016

@jpadilla I totally missed this when I updated #3487. But I didn't cover the (AssertionError, AttributeError) case.

@tomchristie tomchristie modified the milestones: 3.3.4 Release, 3.4.0 Release Jun 1, 2016
@tomchristie tomchristie changed the title Fixed issue 3957 where serializer_class wass accessed directly on the… OrderingFilter should call get_serializer_class() to determine default fields. Jun 1, 2016
@tomchristie
Copy link
Member

tomchristie commented Jun 1, 2016

Thanks folks - now all resolved.

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.

None yet

6 participants