feat: support for GET method for existing servers #180

DxCx · 2016-10-20T18:22:45Z

TODO:

DxCx · 2016-10-26T08:55:31Z

helfer · 2016-10-27T17:57:08Z

@DxCx Thanks, I'll review it before the weekend!

helfer

This is really great, I just had a few comments. I think we should keep the readme as simple as possible, and try to find a good name for the variable that's now called buffer.

After that, I think it will be sufficient to add just enough tests on the branches to make sure that each branch is covered. Once the PR is ready to be merged, I'll make a corresponding PR to the docs and link it here.

helfer · 2016-10-20T20:47:20Z

README.md

@@ -43,7 +43,23 @@ app.use('/graphql', bodyParser.json(), apolloExpress({ schema: myGraphQLSchema }
 app.listen(PORT);
 ```

-### Connect
+### Express ( GET )


Why do we need separate instructions for GET and POST? I think we could just keep the old ones and explain that you don't need bodyparser with GET (which people can figure out, if they want).

Alternatively, we could comment the lines in the code that enable POST requests and GET requests, and then users can comment out the lines that they don't want/need.

helfer · 2016-10-31T21:37:48Z

packages/graphql-server-express/src/expressApollo.ts

@@ -42,33 +42,48 @@ export function graphqlExpress(options: GraphQLOptions | ExpressGraphQLOptionsFu
    }

    const formatErrorFn = optionsObject.formatError || graphql.formatError;
+    let buffer;


can we call this requestPayload or something else that's more descriptive than buffer?

helfer · 2016-10-31T21:40:16Z

packages/graphql-server-hapi/src/hapiApollo.ts

+
+  switch ( method ) {
+      case 'get':
+          return reply(false);


We should document the fact that you cannot send batched queries with a GET request.

Ack, Where?
i just think that opening batching for GET is going to get monstrous URLs..

Yeah, but I don't think monstrous URLs should be our problem. There's a real risk for that already with normal queries. Most servers don't support URLs longer than 2000 characters, and I think even the introspection query might be longer than that. But that's really not something we should worry about. Maybe we can just put a warning in the README.

Is there a good reason (from the server's perspective) not to allow batched requests via GET?

helfer · 2016-10-31T21:40:39Z

packages/graphql-server-hapi/src/hapiApollo.ts

 }

-function getGraphQLParams(payload, isBatch, reply) {
+function getGraphQLParams(buffer, isBatch, reply) {


Why rename this variable?

to match to the code above it.
i can use requestPayload just like you suggested above

helfer · 2016-10-31T21:45:50Z

packages/graphql-server-express/src/expressApollo.ts

+        break;
+      case 'GET':
+        if ( !req.query ) {
+          res.statusCode = 500;


Shouldn't that be a 400?

i just kept the same error code as post.
do you prefer it will be 400?

DxCx · 2016-11-14T20:15:13Z

hi @helfer just seen your comments, i've replied on some and +1 on some.
please answer and i'll fix and rebase :)

DxCx · 2016-11-14T21:48:16Z

packages/graphql-server-express/src/expressApollo.ts

+        break;
+      case 'GET':
+        if ( !req.query ) {
+          res.statusCode = 500;


This is not outdated actually.
i kept the same error code as POST.
@helfer, do you prefer it will be 400?

Yeah, I think 400 is better, because in the case of GET, it means a request was sent without a query, which is definitely not an internal server error. Although... that reminds me that we should also think about how prepared queries would work with GET. I definitely think they should be supported as well, so either this test is in the wrong place, or prepared queries should be a middleware that adds the query to the request before it hits graphql-server.

ohhh
i think you are abit confused (express <-> graphql naming coalition) :)

if you will take a deeper look, req.query for GET is equal to req.body of POST.
which means, in req.query you will get all the query params as key value.
(?key=value&seckey=value) will become:

{ "key": "value", "seckey": "value", }

therefore, if i was checking req.query.query, you were correct on your assumption.
is it more clear now? do you still think it should be 400 and not 500?

@DxCx right, but you're checking req.query. If that's not present, it means it wasn't supplied by the user. In that case I would expect a 400, not a 500.

DxCx · 2016-11-20T21:19:44Z

@helfer / @stubailo how do we progress?
is the implementation looks ok?
there are still unresolved review notes..

as far as i can see we need to extend testings to coverage all new logics
and we are good to go.

helfer · 2016-11-29T11:00:45Z

@DxCx looks 99% good to me. Let's add tests and then merge this. The PR has certainly been waiting for long enough!

helfer · 2016-12-13T09:26:00Z

@DxCx can you check off all the boxes that can be checked off, and then tell me what else needs to be done before we can merge this? I think GET support would be a nice present for the end of the year 😄

DxCx · 2016-12-13T14:01:59Z

i still have work to do here, hoped someone else who needs the feature will take it from there.
anyway, i'll try to make it before the end of the year :D
can you take the documentation while i'll finish testing?

helfer · 2016-12-13T17:12:13Z

@DxCx okay, sounds good. I think someone else could definitely help or even finish it completely, but for that you should first write down a more detailed list of what still needs to be done. I don't think it would take too long to write that down. 🙂

DxCx · 2016-12-13T17:23:46Z

just make sure all new code is covered by spec file,
and maybe add some more edge cases to the test suite case that is relevant to get (just need to think of some)

DxCx · 2016-12-16T13:49:52Z

hi @helfer we are ready on my side.

helfer

Thanks a lot @DxCx! I think if we want to add more tests, we can always do that later. I'd rather not hold this up any longer for no good reason.

One thing we might want to consider adding is a check that makes sure GET requests don't contain mutations. express-graphql does that, and I think it's a neat little feature.

@nnance @HriBB could you take a minute to review the hapi and Koa parts respectively?

helfer · 2016-12-19T14:18:00Z

packages/graphql-server-hapi/src/hapiApollo.ts

+
+  switch ( method ) {
+      case 'get':
+          return reply(false);


Is there a good reason (from the server's perspective) not to allow batched requests via GET?

DxCx · 2016-12-20T10:52:18Z

cool.

Yeah i think we spoke about it before (either here or on slack)
but, giving the ability to create batched requests over query will create monstrous urls.
i think that at least for start we should block it,
then we can experiment more with it and understand how to allow this in the best way.

DxCx · 2016-12-20T10:52:31Z

i did not understand why not allowing mutation via GET?

helfer · 2016-12-21T16:24:15Z

@DxCx The reason express-graphql doesn't allow mutations over GET is that GET requests in HTTP aren't supposed to have any side-effects. Mutations have side-effects by definition, so express-graphql doesn't allow mutations via GET.

DxCx · 2016-12-22T08:54:35Z

Ok @helfer, sounds right. so you want me to block mutation for all GET requests?

DxCx · 2017-01-12T09:26:12Z

@helfer?

helfer · 2017-01-12T15:28:46Z

@DxCx sorry I dropped the ball on this one. Yes, I think blocking mutations on GET would be good, just to match the behavior of express-graphql. Once you're done with that, please ping me on slack and I will take the PR from there and make sure it gets merged soon. Thanks a lot!

DxCx · 2017-01-12T16:53:42Z

@helfer, no problem, so, we should return 405 error and header says that only POST is allowed..
i guess we should 'lock' get for query and not explicitly block mutation, do you agree?
if so will take care of it in the next 24 hours ;)

Closes apollographql#186

DxCx · 2017-01-13T14:38:41Z

@helfer , please review the commits from today (the rest is just rebase).
i've moved http logic into core package, then added guard for non-query on GET.

helfer · 2017-01-18T01:47:43Z

packages/graphql-server-core/src/runHttpQuery.ts

@@ -68,6 +70,13 @@ export async function runHttpQuery(handlerArguments: Array<any>, request: HttpQu

  let responses: Array<ExecutionResult> = [];
  for (let requestParams of requestPayload) {
+    if ( isGetRequest && !requestParams.query.trim().startsWith('query')) {


According to the spec the query keyword is optional. I know it's annoying, but that's how it is. So we'll have to check here whether it's an anonymous query that starts with {.

Also, the query isn't necessarily the first thing in the document right? For example:

fragment X on Y { ... } { ... }

Is also a valid query.

I would check for "document doesn't contain the keyword mutation outside of all brackets"

@helfer => i was already figured it out, and actually, you were reviewing outdated chunk, which this was already fixed.
@stubailo => about the fragment, this is really usecase i didn't take into consideration,
i'll try to use GraphQL Parser instead to get the exact operation GraphQL will try to execute.

…perly

DxCx · 2017-01-18T08:50:23Z

ok @stubailo @helfer
please check latest commit which is using GraphQL parser to determinate the query operation.

helfer · 2017-01-19T01:44:06Z

@DxCx Perfect, I think parsing the query is great. And thanks to how the core works it won't be parsed twice. I'll update the docs and roll a new release!

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

zol added the 🛫 in-progress label Oct 20, 2016

DxCx changed the title ~~feat: support for GET method for existing servers~~ feat: support for GET method for existing servers (WIP) Oct 20, 2016

DxCx force-pushed the get-support branch from 956540a to 0a610f4 Compare October 23, 2016 09:06

DxCx added the tag: new feature label Oct 27, 2016

helfer reviewed Oct 31, 2016

View reviewed changes

DxCx force-pushed the get-support branch from 0a610f4 to 7e81697 Compare November 14, 2016 21:46

DxCx commented Nov 14, 2016

View reviewed changes

DxCx force-pushed the get-support branch from 7e81697 to 2295749 Compare December 1, 2016 12:58

DxCx force-pushed the get-support branch from 2295749 to 4e0420b Compare December 16, 2016 12:03

DxCx changed the title ~~feat: support for GET method for existing servers (WIP)~~ feat: support for GET method for existing servers Dec 16, 2016

helfer reviewed Dec 19, 2016

View reviewed changes

helfer requested a review from nnance December 19, 2016 14:21

chore(get-support): support for GET method for express and connect

a2d7c85

DxCx added 6 commits January 13, 2017 11:45

doc(get-support): update README.md for GET method examples

7abcee8

chore(get-support): support for GET method for hapi

aa251f0

chore(get-support): support for GET method for koa

d7b724f

chore(get-support): Update README.md

977f6df

chore(get-support): rename buffer to requestPayload

93d7966

fix(get-support): fixed returned status for get without query

5b46db4

DxCx force-pushed the get-support branch from 2adbb0a to 5b46db4 Compare January 13, 2017 09:59

DxCx added 3 commits January 13, 2017 16:26

chore(get-support): refactor to share http logic in core module

32140cb

Closes apollographql#186

chore(get-support): blocked GET for queries only

aa3a96a

chore(package): Update CHANGELOG.md

0ad6d42

DxCx added ready and removed 🛫 in-progress labels Jan 13, 2017

DxCx added 2 commits January 13, 2017 16:43

fix(get-support): unnamed operation was not treated as query

11bd610

chore(get-support): cleanup

be573a6

helfer reviewed Jan 18, 2017

View reviewed changes

DxCx mentioned this pull request Jan 18, 2017

Adding in restify integration, docs, tests more... #189

Merged

fix(get-support): preparse query for GET requests to handle query pro…

01e3010

…perly

DxCx mentioned this pull request Jan 19, 2017

batching implementations should (optionally?) run queries in parallel #176

Closed

helfer merged commit 454e2a4 into apollographql:master Jan 19, 2017

helfer removed the ready label Jan 19, 2017

DxCx deleted the get-support branch January 19, 2017 07:32

trevor-scheer pushed a commit that referenced this pull request Oct 17, 2022

Version Packages (#180)

91a0996

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

trevor-scheer pushed a commit that referenced this pull request Oct 17, 2022

Version Packages (#180)

1c4099f

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

trevor-scheer pushed a commit that referenced this pull request Oct 19, 2022

Version Packages (#180)

d86f0b0

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

github-actions bot locked as resolved and limited conversation to collaborators Apr 23, 2023

feat: support for GET method for existing servers #180

feat: support for GET method for existing servers #180

Conversation

DxCx commented Oct 20, 2016 • edited Loading

DxCx commented Oct 26, 2016

helfer commented Oct 27, 2016

helfer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DxCx commented Nov 14, 2016

DxCx Nov 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DxCx Dec 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DxCx commented Nov 20, 2016

helfer commented Nov 29, 2016

helfer commented Dec 13, 2016

DxCx commented Dec 13, 2016

helfer commented Dec 13, 2016

DxCx commented Dec 13, 2016

DxCx commented Dec 16, 2016

helfer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DxCx commented Dec 20, 2016

DxCx commented Dec 20, 2016

helfer commented Dec 21, 2016

DxCx commented Dec 22, 2016

DxCx commented Jan 12, 2017

helfer commented Jan 12, 2017

DxCx commented Jan 12, 2017

DxCx commented Jan 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DxCx commented Jan 18, 2017

helfer commented Jan 19, 2017

DxCx commented Oct 20, 2016 •

edited

Loading

DxCx Nov 14, 2016 •

edited

Loading

DxCx Dec 1, 2016 •

edited

Loading

DxCx commented Jan 13, 2017 •

edited

Loading