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

UTF-8 Request Parser #834

Closed
michaelstaib opened this issue Jun 12, 2019 · 7 comments · Fixed by #869
Closed

UTF-8 Request Parser #834

michaelstaib opened this issue Jun 12, 2019 · 7 comments · Fixed by #869
Assignees
Milestone

Comments

@michaelstaib
Copy link
Member

In order to optimize parsing a GraphQL request we want to build a request parser that is built on top of out utf-8 GraphQL Reader.

There are two kinds of GraphQL request that this new parser should be able to handle:

Single Query

{
  "query": "...",
  "operationName": "...",
  "variables": { "myVariable": "someValue", ... },
  "namedQuery" : "....",
  "extension" : { },
}

Query Batch

[
  {
    "query": "...",
    "operationName": "...",
    "variables": { "myVariable": "someValue", ... },
    "namedQuery" : "....",
    "extension" : { },
  }
]

The parser should parse the json and only fully deserialize the properties operationName and namedQuery.

If the query is named before consuming the other properties we need to check if we have a parsed representation of this query in our cache.

If the query is not named we need to hash the query byte representation and use that as query cache.

If the query is not in the cache we will try to parse the query by unescaping the ReadOnlySpan<byte> that is representing the query at this moment.

For this we need to get a temporary array (ArrayPool or stackalloc depending on size).

The resulting unescaped Span<byte> will be used to the parse the query with the Utf8GraphQLParser.

Only if the query can be successfully parsed we will deserialize the other properties that we are still holding as ReadOnlySpan<byte>. This means that if we are running into any parsing issues with the query we will not event try to deserialize the variables and other properties.

note: we need to think about using a sequence for the JSON parser.

@michaelstaib michaelstaib self-assigned this Jun 12, 2019
@michaelstaib michaelstaib added this to the 9.1.0 milestone Jun 12, 2019
@michaelstaib
Copy link
Member Author

This issue will essentially move the query parsing out of the query engine when we are using ASP.net Core.

The query engine request should allow us to pass in a parsed request.

Moreover, we need to think about how we can access the query cache and query persistence from outside the query request pipeline.

Do we want to have a simple abstraction here that abstracts even the cache and persistence away into one service?

Furthermore, this means that we have to produce query parser errors within the server part. So, should we maybe have a parser service here that can do that?

@michaelstaib
Copy link
Member Author

We should definitely put these new APIs into a new HotChocolate.Server package that can be used by custom server implementation like @OneCyrus's Azure function implementation.

@michaelstaib
Copy link
Member Author

michaelstaib commented Jun 12, 2019

We need options to restrict allowed request sizes in bytes.

@michaelstaib
Copy link
Member Author

michaelstaib commented Jun 12, 2019

New query engine request:

    public interface IReadOnlyQueryRequest
    {
        ISource Query { get; } 

        string NamedQuery { get; }

        string OperationName { get; }

        IReadOnlyDictionary<string, object> VariableValues { get; }

        object InitialValue { get; }

        IReadOnlyDictionary<string, object> Properties { get; }

        IServiceProvider Services { get; }
    }

   public interface IRawQuery : ISource
   {
       ReadOnlyMemory<byte> Body { get; }
   }

   public interface IStringQuery  : ISource
   {
       string Body { get; }
   }

   public interface IParsedQuery  : ISource
   {
       DocumentNode Body { get; }
   }

@michaelstaib
Copy link
Member Author

WIth this issue we should also introduce a new interface to rewrite the queries in the middleware. Currently we only have a delegate for this.

@michaelstaib
Copy link
Member Author

We need a query cache abstraction that encapsulates read-only access to the cache and query storage.

Only the middleware should add things to the cache or the query storage.

public interface IDocumentCache 
{
    bool TryGet(string queryName, out IDocument document);
}

@michaelstaib
Copy link
Member Author

Hi @willwolfram18,
I will finally start work on the request parser today.
I have added some information about the interfaces of the new request object here.

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 a pull request may close this issue.

1 participant