search

VNG-Realisatie / vng-api-common

Gedeelde code voor RESTful APIs
5 stars 12 forks source link

:bug: view should return Response object #148

Closed annashamray closed 4 years ago

annashamray commented 4 years ago

@joeribekker This method is used only if there is a search on geo-coordinates, which is implemented now only in Zaken API and Objects API Zaken API doesn't wrap it because it has pagination and doesn't go to this line of code.

I've fixed it in my component (Objects API), but I do think it's a bug because there is no consistency in the result, returned by this method. It could be now either ReturnList or Response object, which seems not correct for me.

joeribekker commented 4 years ago

Zaken API doesn't wrap it because it has pagination and doesn't go to this line of code.

Why would pagination prevent this code from being reached? How else can the zoek-endpoint return any data?

I've fixed it in my component (Objects API), but I do think it's a bug because there is no consistency in the result, returned by this method. It could be now either ReturnList or Response object, which seems not correct for me.

I don't see how it can switch between ReturnList and Response? Serializers always return a ReturnList or ReturnDict, but with many=True, it's always a ReturnList afaik.

I still think it's not a bug since you always need to wrap this function within an @action decorated function in your own viewset anyway. This is not done in the Zaken API which imo is the actual bug. In https://www.django-rest-framework.org/api-guide/viewsets/#marking-extra-actions-for-routing you see the response as always wrapped as a Response.

So, the question is do we want the Mixin to do it, or the Viewsets that uses this Mixin. It doesn't matter either way, as long as its clear on how to use this mixin.

I'll assign @sergei-maertens to make the final call.

annashamray commented 4 years ago

Why would pagination prevent this code from being reached? How else can the zoek-endpoint return any data?

https://github.com/VNG-Realisatie/vng-api-common/blob/eee9dfb4becc22327a556a5f28298b64d696f0f7/vng_api_common/search.py#L25 It the view has pagination it will go to this part of the code, and the result would be Response object (since self.get_paginated_response returns Response)

If the view doesn't have pagination, the result would be ReturnList

I'll assign @sergei-maertens to make the final call.

I agree

sergei-maertens commented 4 years ago

I agree with the discussion and the patch. Consistency in the return type is important, and there's no easy way around getting paginated non-response return value, so response object it is.