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

Improve query parsing and validation #121

Closed
4 tasks done
Maximilien-R opened this issue Feb 12, 2019 · 1 comment · Fixed by #272
Closed
4 tasks done

Improve query parsing and validation #121

Maximilien-R opened this issue Feb 12, 2019 · 1 comment · Fixed by #272
Assignees
Labels
enhancement New feature or request question Further information is requested
Milestone

Comments

@Maximilien-R
Copy link
Member

Maximilien-R commented Feb 12, 2019

It might be interesting to change the way queries are parsed and validated.

Currently we use the libgraphqlparser to browse the nodes of the query using callback functions that are called each time the parser encounters a new node in the document. At each node encountered the callback function generates a VisitorElement object which is then passed when calling the update method of the Visitor instance which is responsible of business logic (validation, addition of information to the visitor context, creation of Node object...).

These different processes are complex and long to execute. The addition of the management of new errors gradually make the code less and less readable and more difficult to maintain.

To tackle this problem, we could dissociate this unique treatment into separate steps:

  1. Parse the query to generate a DocumentNode object containing all the information related to the query.
    This step can be easily done by calling the graphql_ast_to_json libgraphqlparser function which analyzes the query and returns a string in JSON format containing all the information related to the query. This string could then be computed into a DocumentNode object.
  2. Apply the set of validation rules defined by the GraphQL specification to this DocumentNode object.

Proceed in this way to several advantages:

  1. Allow to cache for each query the generation of the DocumentNode and the application of validation rules.
  2. Delete a lot of code (Visitor, VisitorContext & most of the code contained in cffi parser) and gain in simplicity/readability/maintainability.
  3. Have a DocumentNode object conforming to the GraphQL language specifications. This would allow to be more faithful to the specifications but also to facilitate the on-boarding of new collaborators.
  4. Be closer to the specification and allow to deal with some particular case still poorly managed currently (such as the merge of SelectionSet).

TODO

  • Implements functions which parse a SDL or query string to a DocumentNode instance
  • Cache SDL/query parsing & validation
  • Use DocumentNode to build schema
  • Use DocumentNode to execute query
@Maximilien-R Maximilien-R added enhancement New feature or request question Further information is requested labels Feb 12, 2019
@Maximilien-R Maximilien-R added this to the road-to-v1 milestone Feb 13, 2019
@abusi
Copy link
Contributor

abusi commented Mar 14, 2019

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants