Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support DocumentNode, AST in Helper #725

Closed
wants to merge 8 commits into from

Conversation

shmax
Copy link
Contributor

@shmax shmax commented Aug 31, 2020

Fix for #279

Recently I've started experimenting with graphql-tag. It's a node package popular with users of apollographql that does the parsing of graphql queries in the js layer (mainly so that Apollo can do merging and other operations on the AST, which is difficult on a string query).

As noted in issue #279, this library does not directly support an AST structure. I'm aware that you can doctor the incoming request using AST::fromArray before passing it to \GraphQL\GraphQL::executeQuery, but my original client code was using StandardServer and this does not expose any hook where this kind of preprocessing can happen.

So, in this PR I add knowledge of both DocumentNode and array representations of AST to Helper.

I'll wait for the green light before adding tests.

edit: note that I didn't actually change anything in the GraphQL::executeQuery entry point; wasn't sure how much push back I'm going to get on this, but I'm happy to modify it as well if anyone wants it. Added the functionality.

@coveralls
Copy link

coveralls commented Aug 31, 2020

Coverage Status

Coverage decreased (-0.009%) to 86.299% when pulling f5b2971 on shmax:support-ast-in-helper into 4f34309 on webonyx:master.

@shmax
Copy link
Contributor Author

shmax commented Sep 10, 2020

@vladar @simPod @spawnia Hey folks, please review.

src/Utils/AST.php Outdated Show resolved Hide resolved
src/Server/Helper.php Outdated Show resolved Hide resolved
src/Server/Helper.php Outdated Show resolved Hide resolved
shmax and others added 6 commits September 10, 2020 08:07
Co-authored-by: Benedikt Franke <benedikt@franke.tech>
Co-authored-by: Benedikt Franke <benedikt@franke.tech>
Co-authored-by: Benedikt Franke <benedikt@franke.tech>
@shmax
Copy link
Contributor Author

shmax commented Sep 11, 2020

@spawnia Any idea what's going on with some of those checks in the matrix? A few of them fail, but I don't see any information on the details page; it just says "This check failed": https://github.com/webonyx/graphql-php/pull/725/checks?check_run_id=1097480255

Something to do with exercising different executors? I couldn't figure out how to switch them around in my local environment; I tried setting the "executor" env var to "coroutine", but I don't see any calls to getEnv in the test flow. Any advice?

@simPod
Copy link
Collaborator

simPod commented Sep 11, 2020

might be github status issue 🤔

@shmax
Copy link
Contributor Author

shmax commented Sep 11, 2020

might be github status issue 🤔

Oh, hey @simPod, haven't seen you around in a while! You think something is awry on Github's end?

@simPod
Copy link
Collaborator

simPod commented Sep 11, 2020

hi @shmax :P been busy with some personal things lately. I tried to retrigger here and all passes https://github.com/simPod/graphql-php/actions/runs/249356988 so must be github. I restarted ur checks.

@shmax
Copy link
Contributor Author

shmax commented Sep 11, 2020

I guess you've got the magic touch! Thanks very much.

Copy link
Member

@vladar vladar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me in general (as I mentioned in the original issue). One concern I have is security. AST::fromArray was originally implemented for "safe" usage scenarios (like persisting AST on disk and then reading back).

I don't feel confident enough to expose it out of the box as we don't know how it will behave with malicious inputs.

So what we need with this PR:

  1. Make this feature opt-in and only in the StandardServer (should not be added to GraphQL facade - we can document how to convert an array to DocumentNode for those who really needs it)

  2. Do some kind of security audit for AST::fromArray and probably add tests for various malicious and invalid inputs.

@shmax
Copy link
Contributor Author

shmax commented Sep 16, 2020

So what we need with this PR:

Eh, I'm going to pass. I don't have time to be run through the wringer over this. I've already removed StandardServer from my own code. I was just trying to be helpful.

@shmax shmax closed this Sep 16, 2020
@shmax shmax deleted the support-ast-in-helper branch September 16, 2020 17:34
@vladar
Copy link
Member

vladar commented Sep 16, 2020

I understand. But if you use this AST::fromArray utility in your project with graphql-tag, keep in mind that it can be unsafe.

@shmax
Copy link
Contributor Author

shmax commented Sep 16, 2020

Well, never say never, but I use persisted queries, so in my particular case there are no queries being formed on the client side and in principle there should be no risk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants