Automatic Persisted Queries

[SlavaUtesinov]

Implementation of Automatic Persisted Queries (APQ) based on IConfigureExecution middleware and in-memory cache. It is a possible solution of the issue graphql-dotnet/server#132. The feature has been implemented in accordance with APQ protocol, Apollo logic and general architecture:

Typical incoming request looks the following way:

{
  extensions: {
    persistedQuery: {
      version: '1',
      sha256Hash: 'd7b0dfafc61a1f0618f4f346911d5aa87bef97b134f2943383223bdac4410134'
    }
  }
}

[Shane32]

Is this targeting v5.3.0 or v6? Assuming we are targeting v5.3.0 then we cannot make breaking changes to existing classes and interfaces.

[SlavaUtesinov]

I was trying to make breaking changes as little as possible. It seems like only GetMemoryCacheEntryOptions method's signature has been changed. I think we can deal with it - I can return it back.

[Shane32]

I think the biggest issue is that IDocumentCache has changed. Interfaces can't be changed because adding, changing or removing methods in an interface are all breaking changes. The inheritance tree of an interface might be able to be changed.... but certainly not if it implements any new methods. So pretty much interfaces are locked once created.

[Shane32]

I do see that IDocumentCache will have the same method signatures before as after. I would need to check to see if it's a breaking change or not. I think it's still a breaking change. @sungam3r do you know?

For example -- would this compile anymore??

class test : IDocumentCache
{
    System.Threading.Tasks.ValueTask<GraphQLParser.AST.GraphQLDocument?> IDocumentCache.GetAsync(string key)
    {
        //do some stuff
    }
}

I can't remember -- wouldn't it have to change to this?

class test : IDocumentCache
{
    System.Threading.Tasks.ValueTask<GraphQLParser.AST.GraphQLDocument?> ICache<GraphQLParser.AST.GraphQLDocument>.GetAsync(string key)
    {
        //do some stuff
    }
}

[sungam3r]

Regarding biggest issue. For me this implementation is overcomplicated and contains a lot of things that have no value. I'm completely against ICache<TValue>/IQueryCache "abstractions". I suggest to implement APQ with embedded memory cache. It's simple, it's small, it has no breaking changes, it does not touch existing interfaces. Moreover - almost any implementation different from memory cache will suffer from the same problem - big network data interchange pulling query text from some outer service/storage. So for me it makes no sense to provide ICache<TValue>/IQueryCache and try to build something around them. If someone really wants to use some other storage then he may copy-paste required code.

[Shane32]

@sungam3r Don't you think that a REDIS implementation could be significant to users? You do not think we should at least support some type of extension supporting such a scenario?

I suggest that even if we eliminate the interfaces you mentioned (okay with me), implement the APQ functionality as an abstract class, with the memory implementation as a derived class on top of it. In that manner, anyone can implement their own caching implementation and there are less abstractions that need to be maintained without completely blocking derived implementations.

Something like this: (quick draft)

public abstract class APQBase : IConfigureExecution
{
    public virtual Task<ExecutionResult> ExecuteAsync(ExecutionOptions options, ExecutionDelegate next)
    {
        //implementation here
    }

    // add some methods to generate errors, which can be overridden if logging is desired
    public virtual ValueTask<ExecutionResult> HandleNotInCacheErrorAsync(ExecutionOptions options)
        => new(new ExecutionResult { ExecutionErrors = new() { new NotInCacheError() } }); //or whatever
    // similar for invalid version and similar errors

    public virtual string HashQuery(string query) { } // default SHA hash code here, which can be overridden (??)

    public abstract ValueTask<string?> GetQueryAsync(string hash)
    {
    }

    public abstract Task SetQueryAsync(string hash, string query)
    {
    }
}

Without a REDIS cache, APQ usefulness is somewhat limited to a single-server design, as a request through a load balancer to one server may have a cache miss, and the subsequent request containing the full query may hit a different server, which could already have a populated cache entry. Of course we can optimize for an in-memory cache, but it seems that we should also support a custom cache implementation. Some users may even want to stack a memory cache with a distributed cache.

[sungam3r]

You do not think we should at least support some type of extension supporting such a scenario?

No if this causes a significant complication of the main scenario - keeping hash-query dictionary into memory.

I suggest that even if we eliminate the interfaces you mentioned (okay with me), implement the APQ functionality as an abstract class, with the memory implementation as a derived class on top of it.

Of course, I argee with provided example. APQ may and should be abstracted itself as a whole concept. It seems that abstracting on top of ICache<TValue>/IQueryCache interfaces is a wrong place like abstracting for a small part inside a fixed Apollo implementation .

Without a REDIS cache, APQ usefulness is somewhat limited to a single-server design, as a request through a load balancer to one server may have a cache miss, and the subsequent request containing the full query may hit a different server, which could already have a populated cache entry

I understand but eventually (sooner or later) every app node's hash-query cache will be filled with all needed data.

[sungam3r]

which I would consider a new feature for v5.3.0

I'm fine either way. In case v5.3.0 it should at least be mentioned in release notes.

[Shane32]

which I would consider a new feature for v5.3.0

I'm fine either way. In case v5.3.0 it should at least be mentioned in release notes.

I agree

[Shane32]

Looks great!

[sungam3r]

I'm fine with this PR. Only 1 thing - make methods from AutomaticPersistedQueriesExecutionBase protected

[codecov-commenter]

Codecov Report

Merging #3134 (710ed66) into master (03a868c) will increase coverage by 0.11%.
The diff coverage is 89.16%.

@@            Coverage Diff             @@
##           master    #3134      +/-   ##
==========================================
+ Coverage   84.31%   84.43%   +0.11%     
==========================================
  Files         363      371       +8     
  Lines       15949    16087     +138     
  Branches     2581     2599      +18     
==========================================
+ Hits        13448    13583     +135     
+ Misses       1883     1882       -1     
- Partials      618      622       +4     

Impacted Files	Coverage Δ
src/GraphQL/Execution/Errors/DocumentError.cs	100.00% <ø> (ø)
src/GraphQL/Execution/Errors/ExecutionError.cs	95.12% <ø> (ø)
src/GraphQL/Execution/Errors/ExecutionErrors.cs	83.33% <ø> (ø)
.../GraphQL/Execution/Errors/InvalidOperationError.cs	100.00% <ø> (ø)
src/GraphQL/Execution/Errors/NoOperationError.cs	100.00% <ø> (ø)
src/GraphQL/Execution/Errors/SyntaxError.cs	100.00% <ø> (ø)
src/GraphQL/Execution/Errors/UnhandledError.cs	100.00% <ø> (ø)
src/GraphQL/Execution/Errors/RequestError.cs	64.28% <64.28%> (ø)
...istedQueries/AutomaticPersistedQueriesExecution.cs	71.42% <71.42%> (ø)
...edQueries/AutomaticPersistedQueriesCacheOptions.cs	83.33% <83.33%> (ø)
... and 12 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03a868c...710ed66. Read the comment docs.