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

Custom GET "action" endpoint not displaying query params #435

Closed
jourdanrodrigues opened this issue Jun 23, 2021 · 12 comments
Closed

Custom GET "action" endpoint not displaying query params #435

jourdanrodrigues opened this issue Jun 23, 2021 · 12 comments

Comments

@jourdanrodrigues
Copy link

Describe the bug
Query params don't show up in an "@action" that doesn't take a list serializer class.

I understand the point of view here of query params being for listing resources, but I'd like to know if there's any way to bypass this check to load the query params anyway.

To Reproduce

This won't display query params.

class UserViewSet(GenericViewSet, ListModelMixin):
    filterset_class = FilterSetClass

    @action(
        detail=False,
        methods=["GET"],
        # Using this serializer solely to display correct output
        serializer_class=NonListSerializer,
    )
    def export_csv(...):
        # Use query params to setup an async job
        return Response({"job_id": <uuid>})

Expected behavior
Should display query params correctly.

After a quick look at the code, seems like if if is_list_serializer(serializer) and if hasattr(self.view, 'action') happen after if isinstance(self.view, ListModelMixin) at drf_spectacular.openapi.AutoSchema._is_list_view, then it'd render the query params alright.

@tfranzel
Copy link
Owner

Hi @jourdanrodrigues! filters and pagination are always defined as lists by DRF. at least that is how i understand it. of course it can make sense to have parameters in a non-list case but that is not what the filterset classes were designed for afaik.

i'm not sure how we should support that. changing the list mechanics as you did there will likely break hundreds of users, so we cannot do this. as you can see in CI, this break half of all the tests. This function is already finely tuned and is hard to change at all.

@jourdanrodrigues
Copy link
Author

Yep, that was almost a total failure. Hahaha

I see. I also exhausted my options as I've been poking here and there for a few hours now and couldn't find a way to bypass those checks.

@tfranzel
Copy link
Owner

what you can do is either annotate those parameters manually with @extend_schema(parameters=[OpenApiParamter(...),...]) or use a patched version of our AutoSchema to account for your use case. This will likely be only applicable to you and cannot be upstreamed.

@tfranzel
Copy link
Owner

def _get_filter_parameters(self):

is where you would insert something that bypasses self._is_list_view() for your specific filterset class. i would not touch is_list_view itself but rather bypass that return [] only in your specific case emerges.

@jourdanrodrigues
Copy link
Author

Thanks for the clarification, @tfranzel! I finally managed to get it working. I actually made a mistake in that PR I sent. Here's the snipper for historical purpose:

from drf_spectacular.extensions import OpenApiFilterExtension
from drf_spectacular.openapi import AutoSchema as SpectacularAutoSchema


class AutoSchema(SpectacularAutoSchema):
    def _get_filter_parameters(self):
        if not (self._is_a_general_list_view() or self._is_list_view()):
            return []
        if getattr(self.view, 'filter_backends', None) is None:
            return []

        parameters = []
        for filter_backend in self.view.filter_backends:
            filter_extension = OpenApiFilterExtension.get_match(filter_backend())
            if filter_extension:
                parameters += filter_extension.get_schema_operation_parameters(self)
            else:
                parameters += filter_backend().get_schema_operation_parameters(self.view)
        return parameters

    def _is_a_general_list_view(self):
        return hasattr(self.view, "detail") and self.method.lower() == "get" and not self.view.detail

@tfranzel
Copy link
Owner

awesome! that is how i roughly imagined it. glad you got it working.

@realsuayip
Copy link

@tfranzel I think we should reconsider pushing this to upstream. It is more of a common problem. I've been using actions to perform bulk tasks on listed objects (similar to Django admin actions).

@tfranzel
Copy link
Owner

tfranzel commented May 9, 2023

I've been using actions to perform bulk tasks on listed objects (similar to Django admin actions).

That is a different thing.

I think we should reconsider pushing this to upstream. It is more of a common problem.

I disagree. How would you do this without breaking everything? And the general statement still holds that DRF does not define filter backend parameters on non-list views.

@andriijas
Copy link

andriijas commented Nov 26, 2023

This is how I solved it

from drf_spectacular.openapi import AutoSchema


class StatsSchema(AutoSchema):
    def get_filter_backends(self):
        return getattr(self.view, "filter_backends", [])

@method_decorator(cache_control(private=True, max_age=10), name="list")
@method_decorator(cache_control(private=True, max_age=10), name="retrieve")        
@method_decorator(cache_control(public=True, max_age=600), name="stats")
class ItemViewSet(ModelViewSet):
    queryset = (
        Item.objects.prefetch_related(
            "attachments",
            "likes",
        )
        .annotate(
            likes_count=Count("likes"),
        )
        .all()
    )

    filterset_class = ItemFilter
    serializer_class = ItemSerializer
    permission_classes = [ItemPermissions]
    search_fields = [
        "title",
        "subtitle",
    ]

    lookup_field = "slug"

    ordering_fields = [
        "created_at",
        "published_at",
        "updated_at",
        "likes_count",
    ]
    ordering = ["-published_at"]

    @extend_schema(
        description="Dictionaries of stats representing the current state of Items",
        responses=ItemStatsSerializer(),
    )
    @action(detail=False, methods=["get"], pagination_class=None, schema=StatsSchema())
    def stats(self, request, *args, **kwargs):
        # Operate on same queryset as super().list without pagination
        # to get correct stats
        queryset = self.filter_queryset(self.get_queryset())
        serializer = ItemStatsSerializer(queryset, context={"request": request})
        return Response(serializer.data)

@tfranzel
Copy link
Owner

Hey guys,

@andriijas comment just reminded me that we did fix this issue a couple of month later in Oct '21 with #407

@realsuayip sry I totally forgot about this. We added a filters=True/False parameter that left the default untouched. Now you can explicitly control whether the viewsets filter class should be used (or not) on that endpoint regardless of the response being many true or false.

@jourdanrodrigues hopefully this is still useful to you. Your hack hasn't been necessary for quite some time now, if you use the @extend_schema(...,filters=True) feature.

    @extend_schema(
        responses=ItemStatsSerializer(),
        filters=True,
    )
    @action(detail=False, methods=["get"])
    def stats(self, request, *args, **kwargs):
       ...

@andriijas
Copy link

Thanks, that cleaned up my messy example. It was a pleasent learning experience though :D

@sshishov
Copy link

sshishov commented Dec 9, 2023

Works like a charm @tfranzel . We used it for the same logic, represents some totals/summary/counts with ability to filter before this aggregation 👍

Just adding filters=True solved the issue.

I would just request you guys to add this info in README somewhere for future reference as it is kind of common approach 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants