GraphQLHandler: GET requests

graphql-python / graphene-gae

GraphQL Support for Google AppEngine [DEPRECATED - Looking for maintainers]

http://docs.graphene-python.org/projects/gae/en/latest/

BSD 3-Clause "New" or "Revised" License

117 stars 14 forks source link

GraphQLHandler: GET requests #27

Closed nolanw closed 7 years ago

nolanw commented 7 years ago

Hello! Since handling query parameters is already done for POST requests in GraphQLHandler, it seems like we could trivially do GET requests. So, here that is!

I've augmented all relevant tests to test GET as well as POST.

I'm new to GraphQL but it seems kinda funny to accept mutations via an HTTP GET?

ekampf commented 7 years ago

Hey @nolanw thats for the PR :)

Regarding your questionn on mutations via GET - it doesn't really matter in GraphQL. GraphQL doesn't care about the http layer calling it... you can call it via GET\POST or whatever you want. You don't need to query via GET and mutate via POST or anything like that (infant, you can send a graphql something thats both a query and a mutation in one structure)

Personally I don't see how GET makes sense because you're limited in size when sending via data via URL, a limitation you don't have when using POSY body.

What is your use case for GET?

nolanw commented 7 years ago

@ekampf GET makes it real easy to test simple queries in my browser, see if I've got everything hooked up right. That's my only use case really! I agree that POST makes more sense the vast majority of the time.

That makes sense about GraphQL not caring about HTTP methods. I was thinking more about the convention in HTTP that GET only does retrieval, and ideally is idempotent. But that's probably just a reason not to send GraphQL queries as GET requests in production!

I only tried it in the first place because the GraphQL website mentions it:

Your GraphQL HTTP server should handle the HTTP GET and POST methods.

I'll understand if you'd prefer to limit the surface area of the handler. I'll just bolt on a get handler for my use and carry on!

ekampf commented 7 years ago

@nolanw builds is failing. Other than that I don't have a problem merging GET support

coveralls commented 7 years ago

Coverage increased (+0.09%) to 91.645% when pulling af43f8dfe361531b34fba9a31fa054aed9b7041f on nolanw:graphql-handler-get into 38f78948b90b0400516f64fd49abbdcf36100ead on graphql-python:master.

coveralls commented 7 years ago

Coverage increased (+0.09%) to 91.645% when pulling 958757468d3604fcc150c7b5662315109cda681a on nolanw:graphql-handler-get into 38f78948b90b0400516f64fd49abbdcf36100ead on graphql-python:master.

nolanw commented 7 years ago

@ekampf fixed the flake8 issues, squashed, and we're all set I think!

ekampf commented 7 years ago

Will check and merge ASAP