strawberry icon indicating copy to clipboard operation
strawberry copied to clipboard

Add basic Channels integration

Open LucidDan opened this issue 2 years ago • 35 comments

Description

This is a draft PR implementing #1408 - so far it has pretty good coverage of the websocket side of things, but I need to finish:

  • the HTTP consumer
  • the high level API for the Channels Layers, to make it easy to work with, especially if you're in a sync Django view.
  • some more unit tests, especially in relation to the http consumer

I've just rewritten the PR to make the commits a little tidier, and also to clean up the implementation of the consumers which I wasn't happy with.

I'm currently treating it as separate from the Django integration, with its own package path in strawberry.channels - I can imagine before long all the integrations might get consolidated in a common namespace but for now I'm sticking with the same style as FastAPI, aiohttp, etc.

The most likely use case for this will be using it to provide subscriptions support on Django, with the channels layer offering a pretty nice way to integrate it with the rest of a Django project, even if it is a mostly synchronous one.

I've been building some tests, mostly re-using tests from the other GraphQL integrations (I have been wondering as well, do we really need separate test suites for each integration when it comes to websockets? Seems like something that could be done once for all of them, except perhaps for the issue that the context isn't implemented consitently?)

The Channels integration is pretty small and simple to be honest - the biggest part of this is likely to actually be a good example and some documentation (like a how-to) on how this can actually be used. I have something in progress for that, which I figure I should PR to the strawberry/examples repo...

The main thing I'm working on now that the websockets consumer implementation is fairly stable is to add the Layers high level API to make it easy to integrate with a Django app. See the issue ticket for thoughts on this.

Types of Changes

  • [ ] Core
  • [ ] Bugfix
  • [x] New feature
  • [ ] Enhancement/optimization
  • [ ] Documentation

Issues Fixed or Closed by This PR

Checklist

  • [x] My code follows the code style of this project.
  • [x] My change requires a change to the documentation.
  • [x] I have updated the documentation accordingly.
  • [x] I have read the CONTRIBUTING document.
  • [ ] I have added tests to cover my changes.
  • [ ] I have tested the changes and verified that they work and don't break anything (as well as I can manage).

LucidDan avatar Nov 05 '21 15:11 LucidDan

@LucidDan hey! thanks for this PR 😊 I'm going to take a look at it during the weekend :)

I've been building some tests, mostly re-using tests from the other GraphQL integrations (I have been wondering as well, do we really need separate test suites for each integration when it comes to websockets? Seems like something that could be done once for all of them, except perhaps for the issue that the context isn't implemented consitently?)

that's a good point, but I'd personally feel more confident in having one test for each integration 😊

patrick91 avatar Jan 14 '22 20:01 patrick91

Codecov Report

Merging #1407 (fdd6988) into main (091dd22) will increase coverage by 0.10%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1407      +/-   ##
==========================================
+ Coverage   98.09%   98.19%   +0.10%     
==========================================
  Files         152      160       +8     
  Lines        6095     6381     +286     
  Branches     1160     1200      +40     
==========================================
+ Hits         5979     6266     +287     
  Misses         58       58              
+ Partials       58       57       -1     

codecov[bot] avatar Jan 14 '22 20:01 codecov[bot]

/pre-release

patrick91 avatar Jan 14 '22 22:01 patrick91

Pre-release

:wave:

Pre-release 0.125.0.dev.1660322955 [af4caecee0cabd279682f6d585b29390c545a105] has been released on PyPi! :rocket: You can try it by doing:

poetry add strawberry-graphql==0.125.0.dev.1660322955

botberry avatar Jan 14 '22 22:01 botberry

I made some fixes for the HttpHandler, we might need some tests for it 😊

patrick91 avatar Jan 14 '22 23:01 patrick91

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


This release adds an integration with Django Channels. The integration will allow you to use GraphQL subscriptions via Django Channels.


Here's the preview release card for twitter:

Here's the tweet text:

🆕 Release (next) is out! This time with a new integration for Django channels. Feel free to test this out and let us know how it works!

Thanks to @Lucid_Dan, @_bellini666 and Nir (https://github.com/nrbnlulu) for working on this PR!

Get it here 👉 https://github.com/strawberry-graphql/strawberry/releases/tag/(next)

botberry avatar May 30 '22 01:05 botberry

Hey guys. I've been working on this PR on the past week and I think it should be ready to be reviewed.

I took over from where @LucidDan with the following changes:

  1. I integrated the code with the current state of strawberry, integrating some features like query execution using GET parameters, etc

  2. The HttpConsumer is finished and covered with tests

  3. I added a high level api to consume events from layers in a way similar to what @LucidDan proposed at https://github.com/strawberry-graphql/strawberry/issues/1408. That is also covered with tests

Will be ready to do any changes necessary for us to merge this as soon as possible :)

bellini666 avatar May 30 '22 01:05 bellini666

Thanks!

nrbnlulu avatar May 31 '22 04:05 nrbnlulu

Curious as too when this will be merged into a release. I am wanted to try and give it a spin as I have project that I would like to use it in.

lorddaren avatar Jun 16 '22 20:06 lorddaren

hi @Xorso, I'll do a pre-release for this so you can test it out :) I'll try to do a review soon :) I was a bit too busy up until a few days ago :)

patrick91 avatar Jun 16 '22 21:06 patrick91

/pre-release

patrick91 avatar Jun 16 '22 21:06 patrick91

This can be tested with poetry add strawberry-graphql==0.115.0.dev.1655415925 :)

patrick91 avatar Jun 16 '22 21:06 patrick91

Just thought I would bring this up as I am testing. It appears that when I use the GraphQLProtocolTypeRouter in my asgi.py I get a 500 error while trying to access the graphiql page in the browser. Just wondering if I am doing something wrong.

from daziadmin.backend.schema import schema

application = GraphQLProtocolTypeRouter(
    schema,
    django_application=django_asgi_app
)

Runserver logs

System check identified no issues (0 silenced).
June 21, 2022 - 21:29:44
Django version 4.0.4, using settings 'config.settings.local'
Starting ASGI/Channels version 3.0.4 development server at http://0.0.0.0:8000/
Quit the server with CONTROL-C.
INFO 2022-06-21 21:30:26,571 server 2623180 140310638642944 HTTP/2 support not enabled (install the http2 and tls Twisted extras)
INFO 2022-06-21 21:30:26,572 server 2623180 140310638642944 Configuring endpoint tcp:port=8000:interface=0.0.0.0
INFO 2022-06-21 21:30:26,576 server 2623180 140310638642944 Listening on TCP address 0.0.0.0:8000
HTTP GET /graphql 500 [0.01, 127.0.0.1:46024]
ERROR 2022-06-21 21:30:31,777 runserver 2623180 140310638642944 HTTP GET /graphql 500 [0.01, 127.0.0.1:46024]

EDIT: May have found where the problem exists. No solution yet. http_handler.py

    def should_render_graphiql(self):
        return self.graphiql and self.headers.get("accept", "") in ["text/html", "*/*"]

This seems to return false. Specifically the self.headers.get in:

self.headers.get("accept", "") in ["text/html", "*/*"]
False
self.headers.get("accept", "")
'text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.9'

If I force it to be true it simply renders the page with carriage returns.

lorddaren avatar Jun 21 '22 21:06 lorddaren

Hey @Xorso , I'll take a look at this probably tomorrow and send a fix. From what I can see the should_render_graphiql method is checking the accept header in the wrong way.

bellini666 avatar Jun 21 '22 23:06 bellini666

Thank you. Also, I haven't been able to get grahipql to come up either even when i set it to true. Again not sure if I am doing something wrong there.

lorddaren avatar Jun 22 '22 01:06 lorddaren

@Xorso I just installed the pre-release version on a project of mine and set it up using both the websocket consumer and the http consumer. I could replicate your issues and fixed both of them. From my tests, that was the only issue that I found.

@patrick91 can we get another pre-release?

bellini666 avatar Jun 22 '22 17:06 bellini666

/pre-release

patrick91 avatar Jun 22 '22 17:06 patrick91

this is the last version: poetry add strawberry-graphql==0.115.0.dev.1655918067

patrick91 avatar Jun 22 '22 17:06 patrick91

So far so good. Things are looking better. Thanks for the quick turn around. I will keep implementing it and let you know the results.

lorddaren avatar Jun 22 '22 20:06 lorddaren

Wondering if we can add some additional helper methods for cookies, headers, user etc. to ChannelsConsumer for more interoperability?

Something like

@property
def headers(self) -> Dict[str, str]:
    return {
        header_name.decode().lower(): header_value.decode()
        for header_name, header_value in self.scope["headers"]
    }

@property
def cookies(self) -> Dict[str, str]:
    return {
        cookie_name.decode().lower(): cookie_value.decode()
        for cookie_name, cookie_value in self.scope["cookies"]
    }

@property
def META(self):
    meta = {}

    for name, value in self.scope.get("headers", []):
        name = name.decode("latin1")
        if name == "content-length":
            corrected_name = "CONTENT_LENGTH"
        elif name == "content-type":
            corrected_name = "CONTENT_TYPE"
        else:
            corrected_name = "HTTP_%s" % name.upper().replace("-", "_")
        # HTTPbis say only ASCII chars are allowed in headers, but we
        # use latin1 just in case
        value = value.decode("latin1")
        if corrected_name in meta:
            value = meta[corrected_name] + "," + value
        meta[corrected_name] = value

    return meta

@property
def COOKIES(self):
    return self.cookies

@property
def user(self):
    return self.scope["user"]

@user.setter
def user(self, value):
    self.scope["user"] = value

Maybe this is too specific to Django, and needs to be elsewhere.

jaydensmith avatar Jun 24 '22 05:06 jaydensmith

Hey @jaydensmith . IMO I think that it is indeed to specific to Django, but you should be able to easily define your own custom consumer, inheriting from strawberry's one, and extend it with that. I actually do that for django itself in my projects, overriding the strawberry's view to use my custom context instead of the default one.

bellini666 avatar Jun 24 '22 16:06 bellini666

/pre-release

patrick91 avatar Jul 14 '22 13:07 patrick91

Hi 👋 You can find a preview of the docs here:

https://strawberry.rocks/docs/pr/1407/integrations/channels https://strawberry.rocks/docs/pr/1407/README https://strawberry.rocks/docs/pr/1407/general/subscriptions https://strawberry.rocks/docs/pr/1407/integrations/django

github-actions[bot] avatar Jul 27 '22 09:07 github-actions[bot]

I was wondering if it would be possible / useful to map event types to strawberry subscriptions. i.e

@strawberry.type
class Mutation:
    @strawberry.mutation
    async def send_chat_message(
        self,
        room: ChatRoom,
        message: str,
    ) -> None:
        channel_layer = get_channel_layer()
        await channel_layer.group_send(
            f"chat_{room.room_name}",
            {
                "type":  Subscription.join_chat_rooms,  #  <<<
                "room_id": room.room_name,
                "message": message,
            },
        )


@strawberry.type
class Subscription:
    @strawberry.subscription
    async def join_chat_rooms(
        self,
        info: Info,
        rooms: List[ChatRoom],
        user: str,
    ) -> AsyncGenerator[ChatRoomMessage, None]:
        """Join and subscribe to message sent to the given rooms."""
        ws = info.context.ws
        async for message in ws.channel_listen(groups=rooms):  # <<<
            yield ChatRoomMessage(
                room_name=message["room_id"], message=message["message"], current_user=user
            )

nrbnlulu avatar Jul 28 '22 09:07 nrbnlulu

The subscriptions has been working great! And it looks like the docs have come along as well. I would actually like to take this to production but need to have an official release for it. Do you know if this is scheduled to be merged? Or is it still up in the air?

Thank you again for your work on this.

lorddaren avatar Jul 29 '22 17:07 lorddaren

The subscriptions has been working great! And it looks like the docs have come along as well. I would actually like to take this to production but need to have an official release for it. Do you know if this is scheduled to be merged? Or is it still up in the air?

Thank you again for your work on this.

Hi @Xorso I'll do by best to review and get this merged in the next few days 😊

patrick91 avatar Jul 30 '22 15:07 patrick91

The subscriptions has been working great! And it looks like the docs have come along as well. I would actually like to take this to production but need to have an official release for it. Do you know if this is scheduled to be merged? Or is it still up in the air? Thank you again for your work on this.

Hi @Xorso I'll do by best to review and get this merged in the next few days blush

@patrick91 Could you keep in mind my while reviewing that my pr #2046 should replace the docs over here, I added there a tutorial continuing from the channels chat tutorial, some testing guides and minor API reference.

nrbnlulu avatar Jul 30 '22 19:07 nrbnlulu

The subscriptions has been working great! And it looks like the docs have come along as well. I would actually like to take this to production but need to have an official release for it. Do you know if this is scheduled to be merged? Or is it still up in the air? Thank you again for your work on this.

Hi @Xorso I'll do by best to review and get this merged in the next few days blush

@patrick91 Could you keep in mind my while reviewing that my pr #2046 should replace the docs over here, I added there a tutorial continuing from the channels chat tutorial, some testing guides and minor API reference.

@nrbnlulu yes, if you're ok I can merge your PR here 😊

patrick91 avatar Jul 30 '22 20:07 patrick91

@patrick91 Sure!

nrbnlulu avatar Jul 30 '22 20:07 nrbnlulu

Looks like this is getting closer. Anything I can do to help?

lorddaren avatar Aug 03 '22 16:08 lorddaren