Basic Automatic Persisted Queries (APQ) support

[illambo]

Summary

Basic support introduction for automatic persisted queries, implementing the server side apollo link persisted queries protocol. See Apollo docs for more information (quick link here) .

Type of change

• This change requires a documentation update
• New feature (non-breaking change which adds functionality)

Checklist

• Existing tests have been adapted and/or new tests have been added
• Add a CHANGELOG.md entry
• Update the README.md
• Code style has been fixed via composer fix-style

[mfn]

Interesting PR!

I have not yet worked with APQ, just judging from the code changes I'm wondering:

• if this can be achieved with the recent feature merge of GraphQL Middleware support => Resolver middleware #594
• how this plays together with Integrate laragraph/laravel-graphql-utils #668 which touches code parts in a similar area

There might be also ideas gathered from nuwave/lighthouse#651 (comment)

[illambo]

Hi,

• if this can be achieved with the recent feature merge of GraphQL Middleware support => Resolver middleware #594

I tried but u can only handling the ongoing query, so in the request scenario:

{
    "operationName":null,
    "variables":{},
    "extensions":{
        "persistedQuery":{
            "version":1,
            "sha256Hash":"9dd7ff987fac8d0d1979084ebde5ce8bd855cd066d1a34e98432275cc6bc264c"
        }
    }
}

I can't "expand" the request to try resolve the query, we are already in "Syntax Error: Unexpected ".

I also tried with normal middleware in config.grapqhl.middleware but it's very "ugly" rewrite the request input
especially in the case of batching queries because u can return mixed query results and errors.

Example query batching with mixed response:

// request
[{
    "operationName": null,
    "variables":{},
    "extensions":{
        "persistedQuery":{
            "version":1,
            "sha256Hash":"9dd7ff987fac8d0d1979084ebde5ce8bd855cd066d1a34e98432275cc6bc264c"
        }
    }
},
{
    "operationName": null,
    "variables":{},
    "extensions":{
        "persistedQuery":{
            "version":1,
            "sha256Hash":"351976068aae1d1686319dd49136d2b085e2a4758ede83323a3b43a6ebf1232s"
        }
    }
}]

// response
[
    {
        "data": {
            "hello": "hello"
        }
    },
    {
        "errors": [
            {
                "message": "PersistedQueryNotFound",
                "extensions": {
                    "code": "PERSISTED_QUERY_NOT_FOUND"
                }
            }
        ]
    }
]

So at the moment I have no other ideas on how to better manage, in case could you help me? 🙏

To verify the apollo automatic persisted queries protocol (request/response) I used a small apollo-server-express
apollo-server-express-example.zip

• how this plays together with Integrate laragraph/laravel-graphql-utils #668 which touches code parts in a similar area

I look at it as soon as I can

There might be also ideas gathered from nuwave/lighthouse#651 (comment)

I look at it as soon as I can too

Thank you for the support you give to the library!

[illambo]

• how this plays together with Integrate laragraph/laravel-graphql-utils #668 which touches code parts in a similar area

Maybe merging the code in :

    protected function executeOperation(string $schema, OperationParams $operation): array

in OperationParams $operation i think you should have available the extensions input data.

There might be also ideas gathered from nuwave/lighthouse#651 (comment)

Very interesting! It might be the best way, we need to investigate a bit this and this like spawnia said but at the moment it is complex for me (limited time).

[illambo]

If you want I can try to address the pr on you branch #668
About webonyx/graphql-php loadPersistedQuery I try to investigate a bit asap

[mfn]

If you want I can try to address the pr on you branch #668

Unless you find the changes in the PR itself superior useful, you're welcome!

However I was just thinking code-wise about this area and remembered that PR, but obviously haven't made progress and it's not finished so you may very well ignore it for your progress here, absolutely fine for me!

[illambo]

Ok i read a little bit about the implementation on webonyx, following the documentation see Server configuration options / persistentQueryLoader implement one of two pattern present for manage Persisted GraphQL Queries:

• persistgraphql — a library to statically analyze GraphQL queries at build time, the one implemented by webonyx server but his repo actually has been archived

• automatic persisted queries, what I have implemented, about the difference see “A brief history of (build) time”.
  His repo seems treats and "include" persistgraphql feature (build time generation) like an extra (I haven't implemented anything about it at the moment).

So at the moment I do not think it is possible to take advantage of the implementation present on webonyx as it provides a different flow which I have a feeling it is deprecated in favor of "automatic persisted queries".

Maybe it would be possible to treat the hash as queryId but I don't know where it would lead, however it deals with two different exchange flows.

What do you think about it ?

[mfn]

@illambo I'm very grateful for your details, thank you so much, learned a lot!

• what I have implemented, about the difference see “A brief history of (build) time”.

This is super useful to know, especially this part here:

Notably, it requires a build step that tightly couples your client code to your server in order to work. Any time a new query is added, it must be included in the server’s mapping well before it gets executed in production. This might not be so hard if your client and server are sharing a codebase, or even if they are owned by the same team. In practice, though, even in the best-case scenario this can mean a lot of added complexity.

His repo seems treats and "include" persistgraphql feature (build time generation) like an extra (I haven't implemented anything about it at the moment).

To be the honest, this is the part where I feel it's hard to follow for me, simply due to lack of knowledge for those whole thing and having a clear "big picture" how this works in practice.

---

The last thing I want is contributors putting effort in and then it drags on until it's included or we find showstoppers or worst, a disagreement. I still feel bad for #658 not being merged yet, but it takes a good chunk of time to properly not just review but also test this.

I see a similar (complexity) pattern arising here 😱

I briefly skimmed the PR in its current state, but since there are no tests, this makes it much harder right now for me. I usually need them for testing, playing around and further discussing technical things. I saw the client side readme example, but I need something without relying on this.

So as to avoid these risks here, I suggest we get to a point I've enough to review if possible. It doesn't have to be complete, but something for me to better understand if the direction feels correct for this library.

(but I don't want to kid myself, christmas holidays are arriving full force and more time to dedicate here will be actually harder for me)

How do you feel about this suggestion?

[illambo]

I agree, don't worry, and I'm sorry if I've written to you several times.
I also noticed by studying the underlying layer (webonyx) that this package does not seems wrap the standard webonyx server so any related usable logics (like the queryloader in this case, ..) are not applicable from my point of view (#702).

That said, I was thinking of completing the tests and documentation part, then when you have time (after holidays) and you will have the picture of the situation maybe we can try to merge.

If I can, in fact, I wanted to try to make a contribution on the cache and subscription part.

[illambo]

Hi @mfn any news about this PR?

[mfn]

@illambo no, I'm pretty swamped with the rest of my life currently, bigger PRs are on the back burner

[illambo]

ok I hope everything is ok, I wish you good days!

[mfn]

@illambo I've been working through the PR backlog and finally arrived at yours!

Are you still interested in moving forward?

The immediate feedback I gave is just benign stuff, but could you please update it with master and resolve the merge conflicts?

I wonder though, is this compatible with things like file uploads and batching for example?

[mfn]

The immediate feedback I gave is just benign stuff, but could you please update it with master and resolve the merge conflicts?

To be clear 😄 I hope that "soon"ish I'll find time for a proper review / test run here

[illambo]

Hi @mfn !
I solved the conflicts, about batching it is already covered by the tests, for the upload I needed a few days because now I don't have much time available.
As soon as I can I try to integrate the tests.
Thanks for now!

[mfn]

Thanks for updating the PR so far!

the upload I needed a few days

No problem and from where I stand, I don't require it from my side to move forward with the review (though since I can't tell how much time I can dedicated, you might even be faster).

[illambo]

Ok @mfn I integrated the test with the upload and realigned to the current master, hope everything is ok now.

[mfn]

Thanks for bearing with me with this PR, I finally found time to properly review it.

I also only feel gratitude for all the explanations you gave, I had to do a lot of catch-up and read all the materials to understand this.

My TL;DR is:

• I like it though got some feedback, see inline
• I think it should be disabled by default
  Otherwise for public schemas the feature is automagically enabled with the next update of this library and might surprise developers. I love surprises, but not those kind of.

I hope that we can improve the readability of \Rebing\GraphQL\GraphQLController::getQueryFromAPQIfNeeded a bit. Also not sure the name is the best 😅 but got no better suggestion at this time. I did not mention it in the inline feedback because it just occurred to me: maybe this method should only be called after it's established we have APQ support (i.e. making the config('graphql.apq.enable') check outside).

---

if this can be achieved with the recent feature merge of GraphQL Middleware support => #594

I do realize know that this is on the wrong level and never could work. This has to be directly baked it and makes sense now to me 👍

Maybe it would be possible to treat the hash as queryId but I don't know where it would lead, however it deals with two different exchange flows.

I realized that graphql-php actually treats the sha256Hash as the queryId for its built-in server, see https://github.com/webonyx/graphql-php/blob/57072e880020751656f65dea4fe4a68e97d9ddf0/src/Server/OperationParams.php#L114-L116

This ties back into #668 which uses https://github.com/laragraph/utils which uses \GraphQL\Server\OperationParams.

Maybe I feel finally motivated to finish it after this one has landed (otherwise we would merge-conflict each other).

Thank you!

[mfn]

Btw: I realized now (it was already late night yesterday… 💤 ) some of the feedback is petty codestyle stuff and can be automated to be "fixed" -> I plan to do this, were possible, soon-ish so PRs can concentrate more on architecture -> apologies for that.

[illambo]

I hope that we can improve the readability of \Rebing\GraphQL\GraphQLController::getQueryFromAPQIfNeeded a bit. Also not sure the name is the best 😅 but got no better suggestion at this time. I did not mention it in the inline feedback because it just occurred to me: maybe this method should only be called after it's established we have APQ support (i.e. making the config('graphql.apq.enable') check outside).

I removed the function and moved the code, try to see if it fits you better or in case feel free to fix it as you prefer, ok?

Keep in mind that this condition must still be verified (to maintain the same behavior found by apollo server):

$persistedQueryInput = data_get($input, 'extensions.persistedQuery');
if ($persistedQueryInput && (! config('graphql.apq.enable', false))) {
    throw Error\AutomaticPersistedQueriesError::persistedQueriesNotSupported();
}

I hope I have done it all 🏄 what do you think about it?
Ops...I have only now seen that I am missing revisions :)

[mfn]

#701 (comment)

The first feeling that comes to my mind is that it comes from $this->call (\Illuminate\Foundation\Testing\Concerns\MakesHttpRequests)

I give up figuring out the details, let's keep the trim()!

#701 (comment) and #701 (comment)

Same as above, i leave this type of style fix to you, ok?

This was about my suggestion to use null coalesce, which I implemented in my push!

#701 (comment)

It makes sense but if I can I ask you if you can update and optimize it ?

Roger, done in my push!

---

I removed the function and moved the code, try to see if it fits you better or in case feel free to fix it as you prefer, ok?

Thanks, however I did in fact prefer the method just not the way it's structured.

Since code is probably easier to reason at this point, I took the liberty and pushed my version of it.

Keep in mind that this condition must still be verified (to maintain the same behavior found by apollo server):

Thanks for pointing this and out I realized this -> I kept it!

(I could not respond inline to some of your comments anymore because the changes were already marked as "outdated" 😢 )

[mfn]

@illambo

Thanks, however I did in fact prefer the method just not the way it's structured.

Since code is probably easier to reason at this point, I took the liberty and pushed my version of it.

I did:

• adjust the readme slightly
• refactor \Rebing\GraphQL\GraphQL a bit so decorateExecutionResult is now available
• moved back the code to a dedicated method
• but changed it's structure a bit to "un-nest" and it and use early returns
• removed the explicit ClientAware interface on AutomaticPersistedQueriesError
  (seems my feedback fell through the cracks here)
• added two more tests for edge cases

Having the tests available helped a lot while refactoring the code! 👍

Would you be so kind a review my changes, what you think of them? Thank you!

[illambo]

I've seen the updates, great! Thanks for your help, you are strong 💪

Since code is probably easier to reason at this point, I took the liberty and pushed my version of it.

Certainly, in this way we integrate and have better control of everything also with a view to your next PR.

[mfn]

Thanks for your kind words!

Everything LGTM and I also did test this in a private project:

• no negative impact on existing code
• tested persistedQuery and worked (I faked this a bit as I don't have a real infrastructure to properly do it, but it looked good).

Thank you, will release this as 7.2.0 7.1.0 momentarily!

[mfn]

=> https://github.com/rebing/graphql-laravel/releases/tag/7.1.0