search

graphql-python / graphene-tornado

MIT License
52 stars 14 forks source link

graphene-tornado does not understand when an Observable is returned #27

Closed kinow closed 5 years ago

kinow commented 5 years ago

Hi,

I am using graphql-core and haven't tried graphql-core-next, so forgive me if what I'm saying here has been fixed there already.

I'm currently experimenting with subscriptions with graphene-tornado and graphql-ws (there is a pull request for supporting subscriptions there with Tornado).

Subscriptions are required to returned an RxPy Observable. However, when that is returned by graphql-core, graphene-tornado fails as it treats it as a graphql-core's ExecutionResult, trying to access .errors and .invalid, which results in an exception.

So sending a query like

subscription {
  fruit {
    name
  }
}

Always results in an exception. As the types don't match between graphql-core and graphene-tornado.

ewhauser commented 5 years ago

The code that graphene-tornado uses is a bit old and may have some incompatibilities with what other servers are doing. If I have a chance, I'm planning to upgrade it to use https://github.com/graphql-python/graphql-server-core as a basis for request execution like the other Python GraphQL servers have moved to.

JacobMillner commented 5 years ago

I could be wrong here but wouldn't you need to implement websockets first before subscriptions would work with this repo? I only looked briefly but this codebase has no websocket handlers.

kinow commented 5 years ago

Hi @JacobMillner

I could be wrong here but wouldn't you need to implement websockets first before subscriptions would work with this repo? I only looked briefly but this codebase has no websocket handlers.

Good point. I believe that's because the PR in graphql-ws is using graphene-tornado. So there a websockets handler is used, supporting subscriptions. I don't think we will have websockets support directly in this repo.

When the handlers created in the PR for graphql-ws use subscriptions, then the data returned by graphql-core isn't a ExecutionResult (which works fine for the current code here), but in a RxPy Observable (defined down in the dependency chain in graphql-core).

So graphene-tornado needs to support Observable and ExecutionResult in the result of the graphql-core execution, to prevent exceptions when a user has subscriptions.

Looking at graphql-core-next's setup.py, I don't see a dependency to RxPy any longer. So it may change in the next releases? (but not sure if that's just temporary and may be added before the initial release)

Hope it makes sense. Bruno

JacobMillner commented 5 years ago

@kinow Thanks Bruno! That helps a lot. Looking at your tornado-sandbox repo, did you ever get subscriptions working? Your TornadoSubscriptionServer class looks like the missing puzzle piece for me.

kinow commented 5 years ago

@kinow Thanks Bruno! That helps a lot. Looking at your tornado-sandbox repo, did you ever get subscriptions working? Your TornadoSubscriptionServer class looks like the missing puzzle piece for me.

Yes with @dwsutherland 's help. The example under web4 is very close to what's proposed in the PR. The example in web5 includes a small Vue.js app that we used to validate a scenario closer to what we need for a Vue.js app we are developing.

Interestingly, it is working with the current graphene-tornado. Let me create a small example that should trigger this issue.

kinow commented 5 years ago

@JacobMillner I think you were correct. I re-did my steps last time, and looks like the whole mess started as my GraphQL subscription queries were going to a TornadoGraphQLHandler, instead of to a WebSocketHandler.

The graphql-core may indeed return an Observable, but that only happens upon subscription. For other operations, they are returned correctly as ExecutionResult. So in theory Observable won't ever reach graphene-tornado.

Closing the issue.

Thanks and sorry for the noise! Bruno