Execution middleware #762

crissi · 2021-05-03T12:04:36Z

Summary

Execution hook/middleware based on #753

TODO

convert current middleware like functions to use this instead
docs
test for batched requests
document breaking changes in more detail
test per-schema execution middleware

Type of change:

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update
Misc. change (internal, infrastructure, maintenance, etc.)

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

crissi · 2021-05-03T17:56:24Z

@mfn can you take a look when you have some time!

mfn

I like 👍

Not sure were we end up with this, but I see this as a promising start! Also: so little code 😱

I think we should try hard with the actual data we pass and I fully accept further breaking changes for the upcoming 8 release cycle; it's the best time now.

Feel free to react to the feedback already, but I definitely need more time to play around with this. Now that we've something working, it's really best probably to try / see implement:

APC
detect_unused_variables
…

to better understand what is / what is not possible and what's missing etc.

Thanks for the initiative 🙏

mfn · 2021-05-03T18:58:51Z

src/GraphQL.php

+        if (1 === count($middlewareResponse)) {
+            return $middlewareResponse;
+        }


What is this doing exactly? Is it for the case of already returning a response?

this catches, if you returning directly in the middleware.

After thinking about it this might not be enough to know for sure that it was a response that has been returned, will investigate further

mfn · 2021-05-03T19:04:04Z

src/GraphQL.php

@@ -118,6 +119,17 @@ public function schema($schema = null): Schema
     */
    public function query($query, ?array $variables = [], array $opts = []): array
    {
+        $middlewareResponse = app()->make(Pipeline::class)
+            ->send([$query, $variables])


I think we should add $opts here too.

Or, well at least the most important bit would be the schemaName.

TBH I despise the whole signature of this query method, it just does not make any sense to me.

I think the one from \Rebing\GraphQL\GraphQLController::executeQuery makes much more sense:

executeQuery(string $schemaName, OperationParams $params):

with the addition of being able to carry over the parsed query (either freshly parsed or retrieved e.g. from APQ).

mfn · 2021-05-03T19:06:54Z

src/GraphQL.php

+     */
+    public function executionMiddleware(): array
+    {
+        return config('graphql.execution_middleware', []);


I think it's important we consider the schema name / per schema execution middleware too (*), something like:

return config("graphql.$schemaName.execution_middleware") ?? config('graphql.execution_middleware') ?? [];

(*) which isn't in this context yet, I know

src/Support/ExecutionMiddleware.php

mfn · 2021-05-03T19:16:17Z

Btw, why are tests not running? I know there were some recent GHA changes, but you're not a first time contributor and I also don't see a button for approval of the requests:

Or are tests on public repos in draft mode not executed anymore? 🤔

mfn · 2021-05-03T19:30:59Z

Just thinking out loud, no need to do anything right now:

No matter how we name this middleware, we probably should rename \Rebing\GraphQL\Support\Middleware into ResolverMiddleware. I wonder what the guide of "where to place things" should be. It would not be unlikely a project will use a mix of resolver and execution middlewares; and then where should we but them? Should we stick to \GraphQL\Middleware

graphql-laravel/src/Console/MiddlewareMakeCommand.php

Line 21 in 8b156d5

return $rootNamespace . '\GraphQL\Middleware';

for both "kinds" of middlewares?

Maybe moving both to \Rebing\GraphQL\Middleware\ would be a good idea (ResolverMiddleware and ExecutionMiddleware)

crissi · 2021-05-03T21:09:18Z

Feel free to react to the feedback already, but I definitely need more time to play around with this. Now that we've something working, it's really best probably to try / see implement:

APC

detect_unused_variables

…

to better understand what is / what is not possible and what's missing etc.

I will try to move them to the middleware

The $opts[schema] supports ad-hoc schemas, i.e. not just a name referencing the config, thus the boilerplate. I'm wonder if this is truly needed and playing with the idea to actually remove support for this.

I don't see it useful being publicly available, it's more of an internal helper which can be overriden if necessary.

crissi · 2021-05-04T20:03:26Z

I tried to move the apc part but got into problems with the queryContext missing a query string, to fix this the query context needs to be added to the Graphql class instead of the controller. But maybe we can let the apc be at is!?

…he query

…leware

crissi · 2021-05-08T23:03:45Z

All good man, the feature is the most important thing for me😊

Like it alot that everything is a brick that you can choose to add or remove.
Will try to get some time to play with it next week!

# Conflicts: # phpstan-baseline.neon

Some of the previous breaking changes for the next major release have become obsolete due to the further refactoring.

mfn · 2021-05-10T06:04:13Z

CHANGELOG.md

@@ -94,6 +94,7 @@ CHANGELOG
      new: `public function queryAndReturnResult($query, ?array $variables = [], array $opts = []): ExecutionResult`

 ### Added
+- The primary execution of the GraphQL requested is now modeled via middlewares [\#762 / crissi and mfn](https://github.com/rebing/graphql-laravel/pull/762)


@crissi I don't want it to go unmentioned I also mentioned me as secondary author, is this fine with you?

all good 🙂

crissi · 2021-05-10T15:11:07Z

Getting "Type default not found". Checked the config, but looks similar.

I am firing a query from graphiql UI. Are you getting the same?

mfn · 2021-05-10T18:32:10Z

Getting "Type default not found". Checked the config, but looks similar.

I've recently changed the behaviour that invalid schemas are not silently ignored anymore when building the routes => #766

Is your config graphql.default_schema set to default?
Do you have a graphql.schemas.default?

That would be my first idea…

IMHO this is more readable and also gives clearer diffs in case of mismatches

The surrounding code has changed enough to not require this anymore.

mfn · 2021-05-10T19:36:57Z

I ran it on a private project with ~5k GraphQL tests, multiple schemas and custom context with no issues (except adapting the config to inject the context via middleware now)

crissi · 2021-05-11T08:54:02Z

Getting "Type default not found". Checked the config, but looks similar.

I've recently changed the behaviour that invalid schemas are not silently ignored anymore when building the routes => #766

Is your config graphql.default_schema set to default?

Do you have a graphql.schemas.default?

That would be my first idea…

thanks , error 40:)

crissi · 2021-05-11T10:57:46Z

do we need a getter and setter on OperationParams for the variables? now there is only the public variables

mfn · 2021-05-11T15:29:33Z

do we need a getter and setter on OperationParams for the variables? now there is only the public variables

I wanted to stick to and follow \GraphQL\Server\OperationParams which works that way.

I'm not really happy how \Rebing\GraphQL\Support\OperationParams turns out regarding the decoration and might revisit this, but otherwise no real plans here.

thanks , error 40:)

Do you mean it works for you?

crissi · 2021-05-11T15:32:22Z

Yes works fine😊

mfn · 2021-05-11T15:37:32Z

Ok, I think I'll go ahead and merge this then later today.

I still want to work on a better handling of the three middlewares now supported (http laravel, execution, resolve) but I'll do this in a follow-up PRs

Christian Nielsen added 2 commits May 3, 2021 13:55

execution middleware

471dacd

fix styles

daa8bc5

mfn reviewed May 3, 2021

View reviewed changes

Christian Nielsen and others added 5 commits May 4, 2021 14:30

unused variables

a4dcaf4

unused variables

30747b7

fix style

ccfe692

Use the app instance we already have

faad9af

Fix "Required parameter $next follows optional parameter $opts"

6743cf2

This comment has been minimized.

Sign in to view

Merge branch 'master' into execution_middleware

a6413fb

This comment has been minimized.

Sign in to view

mfn added 3 commits May 4, 2021 20:48

Fix phpdoc, thanks to phpstan

48473b1

Derive executionMiddleware from current schema, if available

ede66f8

The $opts[schema] supports ad-hoc schemas, i.e. not just a name referencing the config, thus the boilerplate. I'm wonder if this is truly needed and playing with the idea to actually remove support for this.

Change visibility of executionMiddleware to protected

a48eb9c

I don't see it useful being publicly available, it's more of an internal helper which can be overriden if necessary.

mfn added 11 commits May 4, 2021 23:37

Remove unused class

10f6a4d

Introduce own OperationParams which additionally can handle parsing t…

020fe09

…he query

Refine signature of the execution middleware

6b975f7

Rewrite the execution layer to primarily work with the execution midd…

306ff80

…leware

Add AddAuthUserContextValueMiddleware

14a8af1

Add AutomaticPersistedQueriesMiddleware

bf37460

Add ValidateOperationParamsMiddleware

81e0f2e

Add GraphqlExecutionMiddleware

7230a5f

Define the default set of execution middlewares

ed3c067

Adapt tests

ebed06b

Update phpstan baseline

5ec4e78

mfn added 4 commits May 9, 2021 21:38

Be explicit about the returned value and improve phpdoc

ff5bf93

Add overview about the different middleware concepts available

367cc0c

Add changelog entry

d72fb24

Merge branch 'master' into execution_middleware

ad784d1

# Conflicts: # phpstan-baseline.neon

mfn marked this pull request as ready for review May 9, 2021 19:54

mfn added 2 commits May 10, 2021 08:10

Clarify the behaviour of AutomaticPersistedQueriesMiddleware

ffa50d5

changelog: adapt and clean up the breaking changes section

588cb31

Some of the previous breaking changes for the next major release have become obsolete due to the further refactoring.

mfn force-pushed the execution_middleware branch from 6554763 to 588cb31 Compare May 10, 2021 06:35

Fix bug with not properly getting the per-schema execution middleware

34f2ce3

mfn reviewed May 10, 2021

View reviewed changes

mfn added 8 commits May 10, 2021 20:38

Fix order of test arguments and just use the whole payload

f769fd0

IMHO this is more readable and also gives clearer diffs in case of mismatches

The schemaName doesn't need to be nullable

ed2d937

The surrounding code has changed enough to not require this anymore.

Test precedence of per-schema middelware

315619b

Merge branch 'master' into execution_middleware

0594d42

Fix typo in changelog

d8d09bd

Remove extra white space

3b3a68e

Update how to enable the unused variables check

16b2c42

Clarify use of middleware vs. execution_middleware in the config

d1a748f

mfn merged commit 85a9f71 into rebing:master May 11, 2021

mfn added this to the 8.0.0 milestone May 11, 2021

mfn mentioned this pull request May 12, 2021

Add command to make an execution middleware #772

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Execution middleware #762

Execution middleware #762

crissi commented May 3, 2021 •

edited by mfn

Loading

crissi commented May 3, 2021

mfn left a comment

mfn May 3, 2021

crissi May 3, 2021

mfn May 3, 2021 •

edited

Loading

mfn May 3, 2021 •

edited

Loading

mfn commented May 3, 2021

mfn commented May 3, 2021

crissi commented May 3, 2021

This comment has been minimized.

This comment has been minimized.

crissi commented May 4, 2021 •

edited

Loading

crissi commented May 8, 2021 •

edited

Loading

mfn May 10, 2021

crissi May 10, 2021

crissi commented May 10, 2021

mfn commented May 10, 2021

mfn commented May 10, 2021

crissi commented May 11, 2021

crissi commented May 11, 2021

mfn commented May 11, 2021

crissi commented May 11, 2021

mfn commented May 11, 2021

Execution middleware #762

Execution middleware #762

Conversation

crissi commented May 3, 2021 • edited by mfn Loading

Summary

TODO

crissi commented May 3, 2021

mfn left a comment

Choose a reason for hiding this comment

I like 👍

mfn May 3, 2021

Choose a reason for hiding this comment

crissi May 3, 2021

Choose a reason for hiding this comment

mfn May 3, 2021 • edited Loading

Choose a reason for hiding this comment

mfn May 3, 2021 • edited Loading

Choose a reason for hiding this comment

mfn commented May 3, 2021

mfn commented May 3, 2021

crissi commented May 3, 2021

This comment has been minimized.

This comment has been minimized.

crissi commented May 4, 2021 • edited Loading

crissi commented May 8, 2021 • edited Loading

mfn May 10, 2021

Choose a reason for hiding this comment

crissi May 10, 2021

Choose a reason for hiding this comment

crissi commented May 10, 2021

mfn commented May 10, 2021

mfn commented May 10, 2021

crissi commented May 11, 2021

crissi commented May 11, 2021

mfn commented May 11, 2021

crissi commented May 11, 2021

mfn commented May 11, 2021

crissi commented May 3, 2021 •

edited by mfn

Loading

mfn May 3, 2021 •

edited

Loading

mfn May 3, 2021 •

edited

Loading

crissi commented May 4, 2021 •

edited

Loading

crissi commented May 8, 2021 •

edited

Loading