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

Add parse and validate functions #773

Conversation

mirosval
Copy link

@mirosval mirosval commented Oct 4, 2020

Fix: #726

Split out parse and validate and then re-use them in all execute functions.

This is a draft, please let me know if this direction is acceptable. Also, the current approach does not actually work in case of execute and resolve_into_stream because of lifetime issues.

I would be very happy to get some guidance on this, if this approach works, I'll add:

  • Tests
  • Docs
  • Examples

Split out parse and validate and then re-use them in all execute functions.
@ccbrown
Copy link
Contributor

ccbrown commented Oct 5, 2020

Glad to see someone taking this on! At a high level, I have a few thoughts here, aimed mainly towards simplifying the API:

  1. This is up for debate, but I personally don't think the ValidDocument type is actually necessary. If a user of the library wants to skip validation that should be fine.

    An invalid request is still technically executable, and will always produce a stable result as defined by the algorithms in the Execution section

    https://spec.graphql.org/June2018/#sec-Validation

    The spec goes on to say that execution "should" only occur for valid requests, but I'm not sure we need to enforce that.

  2. It's seems pretty off to me for parsing to require the schema as an argument. In every other GraphQL implementation I've looked at, the parsing process just takes in a string and outputs an AST.

    Juniper's parser currently requires the schema to convert scalar tokens to custom scalar values, but I don't see why this has to be done in the parser. Maybe we could move that into the validator and executor instead? If we did that, I don't think Document would even require the ScalarValue parameter any more.

@mirosval
Copy link
Author

mirosval commented Oct 5, 2020

@ccbrown Thank you for looking into this!

  1. don't think the ValidDocument type is actually necessary.
    I was basing this on the fact that the execute functions are named execute_validated_query*. I would then suggest to have 2 APIs:

  • Validate + Execute validated (named just execute). This would have a type like ValidDocument in between. So we can enforce that you validate the doc up front. It would be the default use case.
  • Just execute without validation. This would then be named execute_unvalidated_query* or similar. To suggest you're doing something unsafe (since the spec says you might end up with ambiguous or surprising results for unvalidated requests). This is probably not what you want most of the time.
  1. Seems like a reasonable idea. I will have to look into it more in-depth, but I would make a separate PR for that.

@ccbrown
Copy link
Contributor

ccbrown commented Oct 5, 2020

This is a lower-level API that only people who know what they're doing should be using to begin with, so I think the hand-holding is unnecessary, but if we do think a type like ValidDocument is needed, let's hold off on the second execute_unvalidated_query API. That can come in a later PR if someone has a use-case for it. My goal with the suggestion was to simplify the API, not complicate it.

@ccbrown
Copy link
Contributor

ccbrown commented Oct 5, 2020

Btw, these changes don't actually have to be in the juniper crate's API. We could satisfy #726 with public APIs in juniper::parser, juniper::validation, and juniper::executor. That would keep the public juniper API narrow and make it less likely that someone would stumble upon these and try to use them for something inappropriate.

@LegNeato
Copy link
Member

LegNeato commented Oct 6, 2020

I agree with these comments :-)

@mirosval
Copy link
Author

mirosval commented Oct 6, 2020

Okay, thanks! That makes sense to me. I'll close this and create new PRs for the 3 new public APIs.

I spent my evening yesterday investigating the ScalarValue type parameter and I understand now what you meant with removing it from Document.

  • I think you can delay the parsing and so remove the type parameter.
  • For validation, you should just be able to pass in a validator of sorts that would try to parse the unparsed scalars
  • I believe it should also be possible to remove the parameter from execution as well, but I need to look deeper into it

Let me know what you think.

@mirosval mirosval closed this Oct 6, 2020
@tyranron tyranron changed the title WIP: Add parse and validate functions Add parse and validate functions Oct 6, 2020
@mirosval mirosval deleted the parse-validate-without-executing branch October 6, 2020 08:40
jerel added a commit to jerel/juniper that referenced this pull request Feb 13, 2021
LegNeato added a commit that referenced this pull request Apr 3, 2021
…and execution (#874)

* Make the executor and validation APIs public to enable splitting parsing and execution into two stages

Based on #773 (comment) and #773 (comment)

* Fix fmt warning for visit_all_rules

* Add `Definition` to the public API so it's available for the result of compilation

* Make OperationType public so that user land code can tell the difference between query/mutation and subscription

* Add integrations tests for execute_validated_query_async, visit_all_rules, get_operation, and resolve_validated_subscription

Co-authored-by: Christian Legnitto <[email protected]>
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.

Add support for parsing and validating a query without executing it
3 participants