graphql-python / graphene-django

Build powerful, efficient, and flexible GraphQL APIs with seamless Django integration.
http://docs.graphene-python.org/projects/django/en/latest/
MIT License
4.3k stars 769 forks source link

GraphQLView should detect batch requests #967

Open McPo opened 4 years ago

McPo commented 4 years ago

Is there a valid reason why the GraphQLView input is validated to either only ever accept batched requests or never accept batched requests. Is it not reasonable to accept both?

https://github.com/graphql-python/graphene-django/blob/ee120c48e16dda818cb253fdc36b7053402956b0/graphene_django/views.py#L217

jkimbo commented 4 years ago

I'm not sure why it was setup that way @McPo but I guess the view could handle both kinds of requests. I would expect a client to either always batch requests or not though. Do you have a client that will sometimes batch requests?

McPo commented 4 years ago

Yes,

So if you use apollo-upload-client, it doesn't support batching. The solution is to then deactivate batching when uploading file https://github.com/jaydenseric/apollo-upload-client/issues/34#issuecomment-638901324.

As such I think this would be a fairly common use-case, and if theres no advantage to the server enforcing one or the other, it should be flexible. I see some people are making a separate /batch endpoint to support this, which seems redundant.

Anyway, Ive managed to monkey patch it for the time being.

class CustomGraphQLView(GraphQLView):
    schema = schema
    graphiql = settings.DEBUG

    def parse_body(self, request):
        # Allow for variable batch
        try:
            body = request.body.decode("utf-8")
            request_json = json.loads(body)
            self.batch = isinstance(request_json, list)
        except:
            self.batch = False
        return super().parse_body(request)
Eraldo commented 4 years ago

I am using apollo's upload client as well. Same (or similar) challenge. Thanks for bringing this up @McPo. :)

jkimbo commented 4 years ago

Makes sense. Sounds like this would be a good enhancement if anyone wants to create a PR.

Eraldo commented 4 years ago

Makes sense. Sounds like this would be a good enhancement if anyone wants to create a PR.

While at it.

I am actually using this:

import json

from graphene_file_upload.django import FileUploadGraphQLView

class GraphQLView(FileUploadGraphQLView):
    """Handles multipart/form-data content type in django views"""

    def parse_body(self, request):
        """
        Allow for variable batch
        https://github.com/graphql-python/graphene-django/issues/967#issuecomment-640480919
        :param request:
        :return:
        """
        try:
            body = request.body.decode("utf-8")
            request_json = json.loads(body)
            self.batch = isinstance(request_json, list)
        except:
            self.batch = False
        return super().parse_body(request)

=> To support batching and file multipart file uploads. Would be awesome to include both capabilities. 👍

jaydenseric commented 4 years ago

To support batching and file multipart file uploads.

Note that the GraphQL multipart request spec supports file uploads with batching:

https://github.com/jaydenseric/graphql-multipart-request-spec#batching

apollo-upload-client only doesn't do this because an Apollo Client implementation is tricky (see https://github.com/jaydenseric/apollo-upload-client/issues/34#issuecomment-358830808), I don't have the time to work on it and no one else has stepped up to the plate (https://github.com/jaydenseric/apollo-upload-client/issues/34#issuecomment-376818641).

A GraphQL server will ideally accept batched and unbatched requests, for both regular and multipart requests.

Eraldo commented 6 months ago

My linter (rightfully) complains about except: being to generic. Are there any specific anticipated/expected exception types? If I don't get an answer, I will remove the catch and see what I get. ^^

Found so far: RawPostDataException