From 0392663dc89925d7abe8cf74f3a2c721d2a73fcd Mon Sep 17 00:00:00 2001 From: spawnia Date: Tue, 16 Oct 2018 23:46:01 +0200 Subject: [PATCH 01/10] Run middleware on a per-field basis --- src/Execution/ContextFactory.php | 2 +- src/GraphQL.php | 11 -- src/Providers/LighthouseServiceProvider.php | 2 - src/Schema/Context.php | 19 +-- .../Directives/Fields/MiddlewareDirective.php | 100 ++++++------ src/Schema/MiddlewareRegistry.php | 147 ------------------ src/Schema/SchemaBuilder.php | 12 -- .../Http/Controllers/GraphQLController.php | 90 +++++------ tests/TestCase.php | 21 ++- .../Fields/MiddlewareDirectiveTest.php | 113 ++++++-------- .../Directives/Nodes/GroupDirectiveTest.php | 24 +-- .../Schema/Execution/ContextFactoryTest.php | 27 ++-- tests/Utils/Middleware/AddFooProperty.php | 18 +++ tests/Utils/Middleware/Authenticate.php | 17 ++ 14 files changed, 234 insertions(+), 369 deletions(-) delete mode 100644 src/Schema/MiddlewareRegistry.php create mode 100644 tests/Utils/Middleware/AddFooProperty.php create mode 100644 tests/Utils/Middleware/Authenticate.php diff --git a/src/Execution/ContextFactory.php b/src/Execution/ContextFactory.php index 89a0b18ef8..22e1df104d 100644 --- a/src/Execution/ContextFactory.php +++ b/src/Execution/ContextFactory.php @@ -18,6 +18,6 @@ class ContextFactory implements CreatesContext */ public function generate(Request $request): GraphQLContext { - return new Context($request, $request->user()); + return new Context($request); } } diff --git a/src/GraphQL.php b/src/GraphQL.php index a85c78ad19..8c5497c832 100644 --- a/src/GraphQL.php +++ b/src/GraphQL.php @@ -17,7 +17,6 @@ use Nuwave\Lighthouse\Schema\AST\ASTBuilder; use Nuwave\Lighthouse\Schema\AST\DocumentAST; use Nuwave\Lighthouse\Schema\DirectiveRegistry; -use Nuwave\Lighthouse\Schema\MiddlewareRegistry; use GraphQL\Validator\Rules\DisableIntrospection; use Nuwave\Lighthouse\Execution\DataLoader\BatchLoader; use Nuwave\Lighthouse\Schema\Source\SchemaSourceProvider; @@ -326,16 +325,6 @@ public function schema(): TypeRegistry return $this->types(); } - /** - * @return MiddlewareRegistry - * - * @deprecated Use resolve() instead, will be removed in v3 - */ - public function middleware(): MiddlewareRegistry - { - return resolve(MiddlewareRegistry::class); - } - /** * @return NodeRegistry * diff --git a/src/Providers/LighthouseServiceProvider.php b/src/Providers/LighthouseServiceProvider.php index ed29b8c414..8b4cbfbf27 100644 --- a/src/Providers/LighthouseServiceProvider.php +++ b/src/Providers/LighthouseServiceProvider.php @@ -10,7 +10,6 @@ use Nuwave\Lighthouse\Schema\TypeRegistry; use Nuwave\Lighthouse\Execution\ContextFactory; use Nuwave\Lighthouse\Schema\DirectiveRegistry; -use Nuwave\Lighthouse\Schema\MiddlewareRegistry; use Nuwave\Lighthouse\Execution\GraphQLValidator; use Nuwave\Lighthouse\Schema\Source\SchemaStitcher; use Nuwave\Lighthouse\Schema\Factories\ValueFactory; @@ -67,7 +66,6 @@ public function register() $this->app->singleton(DirectiveRegistry::class); $this->app->singleton(ExtensionRegistry::class); $this->app->singleton(NodeRegistry::class); - $this->app->singleton(MiddlewareRegistry::class); $this->app->singleton(TypeRegistry::class); $this->app->singleton(CreatesContext::class, ContextFactory::class); diff --git a/src/Schema/Context.php b/src/Schema/Context.php index 960ab4b108..3deb2cf27b 100644 --- a/src/Schema/Context.php +++ b/src/Schema/Context.php @@ -15,35 +15,26 @@ class Context implements GraphQLContext */ public $request; - /** - * Authenticated user. - * - * May be null since some fields may be accessible without authentication. - * - * @var User|null - */ - public $user; - /** * Create new context. * * @param Request $request - * @param User|null $user */ - public function __construct(Request $request, User $user = null) + public function __construct(Request $request) { $this->request = $request; - $this->user = $user; } /** - * Get instance of authorized user. + * Get instance of authenticated user. + * + * May be null since some fields may be accessible without authentication. * * @return User|null */ public function user() { - return $this->user; + return $this->request->user(); } /** diff --git a/src/Schema/Directives/Fields/MiddlewareDirective.php b/src/Schema/Directives/Fields/MiddlewareDirective.php index 2367508a06..e8aac5d05a 100644 --- a/src/Schema/Directives/Fields/MiddlewareDirective.php +++ b/src/Schema/Directives/Fields/MiddlewareDirective.php @@ -2,19 +2,40 @@ namespace Nuwave\Lighthouse\Schema\Directives\Fields; +use Illuminate\Http\Request; +use Illuminate\Routing\Router; +use GraphQL\Type\Definition\ResolveInfo; +use Nuwave\Lighthouse\Support\Pipeline; +use Illuminate\Routing\MiddlewareNameResolver; use Nuwave\Lighthouse\Schema\Values\FieldValue; -use Nuwave\Lighthouse\Schema\MiddlewareRegistry; use Nuwave\Lighthouse\Schema\Directives\BaseDirective; +use Nuwave\Lighthouse\Support\Contracts\CreatesContext; +use Nuwave\Lighthouse\Support\Contracts\GraphQLContext; use Nuwave\Lighthouse\Support\Contracts\FieldMiddleware; class MiddlewareDirective extends BaseDirective implements FieldMiddleware { + /** @var Pipeline */ + protected $pipeline; + /** @var CreatesContext */ + protected $createsContext; + + /** + * @param Pipeline $pipeline + * @param CreatesContext $createsContext + */ + public function __construct(Pipeline $pipeline, CreatesContext $createsContext) + { + $this->pipeline = $pipeline; + $this->createsContext = $createsContext; + } + /** * Name of the directive. * * @return string */ - public function name() + public function name(): string { return 'middleware'; } @@ -27,52 +48,41 @@ public function name() * * @return FieldValue */ - public function handleField(FieldValue $value, \Closure $next) - { - $checks = $this->getChecks($value); - - if ($checks) { - $middlewareRegistry = resolve(MiddlewareRegistry::class); - - if ('Query' === $value->getNodeName()) { - $middlewareRegistry->registerQuery( - $value->getFieldName(), - $checks - ); - } elseif ('Mutation' === $value->getNodeName()) { - $middlewareRegistry->registerMutation( - $value->getFieldName(), - $checks - ); - } - } - - return $next($value); - } - - /** - * Get middleware checks. - * - * @param FieldValue $value - * - * @return array|null - */ - protected function getChecks(FieldValue $value) + public function handleField(FieldValue $value, \Closure $next): FieldValue { - if (! in_array($value->getNodeName(), ['Mutation', 'Query'])) { - return null; - } - - $checks = $this->directiveArgValue('checks'); + /** @var Router $router */ + $router = resolve('router'); + $middleware = $router->getMiddleware(); + $middlewareGroups = $router->getMiddlewareGroups(); - if (! $checks) { - return null; - } + $middleware = collect($this->directiveArgValue('checks')) + ->map(function ($name) use ($middleware, $middlewareGroups){ + return (array) MiddlewareNameResolver::resolve($name, $middleware, $middlewareGroups); + }) + ->flatten(); - if (is_string($checks)) { - $checks = [$checks]; - } + $resolver = $value->getResolver(); - return $checks; + return $next( + $value->setResolver( + function ($root, array $args, GraphQLContext $context, ResolveInfo $resolveInfo) use ($resolver, $middleware) { + return $this->pipeline + ->send( + $context->request() + ) + ->through( + $middleware + ) + ->then(function (Request $request) use ($resolver, $root, $args, $resolveInfo){ + return $resolver( + $root, + $args, + $this->createsContext->generate($request), + $resolveInfo + ); + }); + } + ) + ); } } diff --git a/src/Schema/MiddlewareRegistry.php b/src/Schema/MiddlewareRegistry.php deleted file mode 100644 index e0befc203b..0000000000 --- a/src/Schema/MiddlewareRegistry.php +++ /dev/null @@ -1,147 +0,0 @@ -fragmentDefinitions(); - - return $queryAST->operationDefinitions() - ->map(function (OperationDefinitionNode $node) use ($fragments) { - $definition = AST::toArray($node); - $operation = array_get($definition, 'operation'); - $fields = array_map(function ($selection) use ($fragments) { - $field = array_get($selection, 'name.value'); - - if ('FragmentSpread' === array_get($selection, 'kind')) { - $fragment = $fragments->first(function ($def) use ($field) { - return data_get($def, 'name.value') === $field; - }); - - return array_pluck( - data_get($fragment, 'selectionSet.selections', []), - 'name.value' - ); - } - - return [$field]; - }, array_get($definition, 'selectionSet.selections', [])); - - return $this->operation($operation, array_unique(array_flatten($fields))); - }) - ->collapse() - ->unique() - ->toArray(); - } - - /** - * Register query middleware. - * - * @param string $name - * @param array $middleware - * - * @return MiddlewareRegistry - */ - public function registerQuery(string $name, array $middleware): MiddlewareRegistry - { - $this->queries = array_merge($this->queries, [$name => $middleware]); - - return $this; - } - - /** - * Register mutation middleware. - * - * @param string $name - * @param array $middleware - * - * @return MiddlewareRegistry - */ - public function registerMutation($name, array $middleware): MiddlewareRegistry - { - $this->mutations = array_merge($this->mutations, [$name => $middleware]); - - return $this; - } - - /** - * Get middleware for operation. - * - * @param string $operation - * @param array $fields - * - * @return array - */ - public function operation(string $operation, array $fields): array - { - if ('mutation' === $operation) { - return array_collapse(array_map(function ($field) { - return $this->mutation($field); - }, $fields)); - } elseif ('query' === $operation) { - return array_collapse(array_map(function ($field) { - return $this->query($field); - }, $fields)); - } - - return []; - } - - /** - * Get middleware for query. - * - * @param string $name - * - * @return array - */ - public function query(string $name): array - { - return array_get($this->queries, $name, []); - } - - /** - * Get middleware for mutation. - * - * @param string $name - * - * @return array - */ - public function mutation(string $name): array - { - return array_get($this->mutations, $name, []); - } -} diff --git a/src/Schema/SchemaBuilder.php b/src/Schema/SchemaBuilder.php index 544bd82a19..bcd7af37f8 100644 --- a/src/Schema/SchemaBuilder.php +++ b/src/Schema/SchemaBuilder.php @@ -78,19 +78,9 @@ public function build($documentAST) } } - /** - * The fields for the root operations have to be loaded in advance. - * - * This is because they might have to register middleware. - * Other fields can be lazy-loaded to improve performance. - * - * Calling getFields() causes the fields MiddlewareDirective to run - * and thus registers the (Laravel)-Middleware for the fields. - */ if(empty($queryType)){ throw new InvariantViolation("The root Query type must be present in the schema."); } - $queryType->getFields(); $config = SchemaConfig::create() // Always set Query since it is required @@ -109,11 +99,9 @@ public function build($documentAST) // Those are optional so only add them if they are present in the schema if (isset($mutationType)) { - $mutationType->getFields(); $config->setMutation($mutationType); } if (isset($subscriptionType)) { - $subscriptionType->getFields(); $config->setSubscription($subscriptionType); } diff --git a/src/Support/Http/Controllers/GraphQLController.php b/src/Support/Http/Controllers/GraphQLController.php index c335715569..be401e9ac8 100644 --- a/src/Support/Http/Controllers/GraphQLController.php +++ b/src/Support/Http/Controllers/GraphQLController.php @@ -7,7 +7,8 @@ use Nuwave\Lighthouse\GraphQL; use Illuminate\Routing\Controller; use GraphQL\Executor\ExecutionResult; -use Nuwave\Lighthouse\Schema\MiddlewareRegistry; +use Nuwave\Lighthouse\Exceptions\ParseException; +use Nuwave\Lighthouse\Exceptions\DirectiveException; use Nuwave\Lighthouse\Support\Contracts\CreatesContext; use Nuwave\Lighthouse\Schema\Extensions\ExtensionRequest; use Nuwave\Lighthouse\Schema\Extensions\ExtensionRegistry; @@ -23,54 +24,21 @@ class GraphQLController extends Controller /** @var ExtensionRegistry */ protected $extensionRegistry; - /** @var bool */ - protected $batched = false; - /** * Inject middleware into request. * - * @param Request $request * @param ExtensionRegistry $extensionRegistry - * @param MiddlewareRegistry $middlewareRegistry * @param GraphQL $graphQL * @param CreatesContext $createsContext */ public function __construct( - Request $request, ExtensionRegistry $extensionRegistry, - MiddlewareRegistry $middlewareRegistry, GraphQL $graphQL, CreatesContext $createsContext ) { $this->graphQL = $graphQL; $this->extensionRegistry = $extensionRegistry; $this->createsContext = $createsContext; - - if ($request->route()) { - $this->batched = isset($request[0]) && config('lighthouse.batched_queries', true); - - $extensionRegistry->requestDidStart( - new ExtensionRequest($request, $this->batched) - ); - - $graphQL->prepSchema(); - $middleware = ! $this->batched - ? $middlewareRegistry->forRequest($request->input('query', '')) - : array_reduce( - $request->toArray(), - function ($middleware, $req) use ($middlewareRegistry) { - $query = array_get($req, 'query', ''); - - return array_merge( - $middleware, - $middlewareRegistry->forRequest($query) - ); - }, - [] - ); - - $this->middleware($middleware); - } } /** @@ -82,7 +50,13 @@ function ($middleware, $req) use ($middlewareRegistry) { */ public function query(Request $request) { - $response = $this->batched + $batched = isset($request[0]) && config('lighthouse.batched_queries', true); + + $this->extensionRegistry->requestDidStart( + new ExtensionRequest($request, $batched) + ); + + $response = $batched ? $this->executeBatched($request) : $this->execute($request); @@ -94,20 +68,22 @@ public function query(Request $request) /** * @param Request $request * + * @throws DirectiveException + * @throws ParseException + * * @return array */ protected function execute(Request $request) { - $query = $request->input('query', ''); - $variables = is_string($vars = $request->input('variables', [])) - ? json_decode($vars, true) - : $vars; - return $this->graphQL->executeQuery( - $query, + $request->input('query', ''), $this->createsContext->generate($request), - $variables - )->toArray($this->getDebug()); + $this->ensureVariablesAreArray( + $request->input('variables', []) + ) + )->toArray( + $this->getDebugSetting() + ); } /** @@ -122,9 +98,14 @@ protected function executeBatched(Request $request) $this->createsContext->generate($request) ); - return array_map(function (ExecutionResult $result) { - return $result->toArray($this->getDebug()); - }, $data); + return array_map( + function (ExecutionResult $result) { + return $result->toArray( + $this->getDebugSetting() + ); + }, + $data + ); } /** @@ -132,16 +113,25 @@ protected function executeBatched(Request $request) * * @return array */ - protected function getVariables($variables): array + protected function ensureVariablesAreArray($variables): array { - return is_string($variables) ? json_decode($variables, true) : $variables; + return is_string($variables) + ? json_decode($variables, true) + : $variables; } /** + * Get the GraphQL debug setting. + * * @return int|bool */ - protected function getDebug() + protected function getDebugSetting() { - return config('app.debug') ? config('lighthouse.debug') : false; + // If debugging is set to false globally, do not add GraphQL specific + // debugging info either. If it is true, then we fetch the debug + // level from the Lighthouse configuration. + return config('app.debug') + ? config('lighthouse.debug') + : false; } } diff --git a/tests/TestCase.php b/tests/TestCase.php index 4d87dd9200..7aa5f97912 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -91,7 +91,8 @@ protected function executeQuery(string $schema, string $query, array $variables */ protected function executeWithoutDebug(string $schema, string $query, array $variables = []): array { - return $this->executeQuery($schema, $query, $variables)->toArray(); + return $this->executeQuery($schema, $query, $variables) + ->toArray(); } /** @@ -106,7 +107,8 @@ protected function executeWithoutDebug(string $schema, string $query, array $var protected function execute(string $schema, string $query, array $variables = []): array { // For test execution, it is more convenient to throw Exceptions so they show up in the PHPUnit command line - return $this->executeQuery($schema, $query, $variables)->toArray(Debug::RETHROW_INTERNAL_EXCEPTIONS); + return $this->executeQuery($schema, $query, $variables) + ->toArray(Debug::RETHROW_INTERNAL_EXCEPTIONS); } /** @@ -137,4 +139,19 @@ protected function buildSchemaFromString(string $schema): Schema return graphql()->prepSchema(); } + + /** + * Execute a query as if it was sent as a request to the server. + * + * @param string $query + * + * @return array + */ + protected function queryViaHttp(string $query): array + { + return $this->postJson( + 'graphql', + ['query' => $query] + )->json(); + } } diff --git a/tests/Unit/Schema/Directives/Fields/MiddlewareDirectiveTest.php b/tests/Unit/Schema/Directives/Fields/MiddlewareDirectiveTest.php index 76e16e5717..a444733be5 100644 --- a/tests/Unit/Schema/Directives/Fields/MiddlewareDirectiveTest.php +++ b/tests/Unit/Schema/Directives/Fields/MiddlewareDirectiveTest.php @@ -3,94 +3,77 @@ namespace Tests\Unit\Schema\Directives\Fields; use Tests\TestCase; -use Nuwave\Lighthouse\Schema\MiddlewareRegistry; +use Nuwave\Lighthouse\Schema\Context; +use Tests\Utils\Middleware\Authenticate; +use Tests\Utils\Middleware\AddFooProperty; class MiddlewareDirectiveTest extends TestCase { - /** @var MiddlewareRegistry */ - protected $middlewareRegistry; - - public function setUp() + /** + * @test + * @dataProvider fooMiddlewareQueries + * + * @param string $query + */ + public function itCallsFooMiddleware(string $query) { - parent::setUp(); + $this->schema = ' + type Query { + foo: Int + @middleware(checks: ["Tests\\\Utils\\\Middleware\\\AddFooProperty"]) + @field(resolver: "'. addslashes(self::class).'@resolveFooMiddleware") + } + '; - $this->middlewareRegistry = resolve(MiddlewareRegistry::class); + $result = $this->queryViaHttp($query); + + $this->assertSame(42, array_get($result, 'data.foo')); } - /** - * @test - */ - public function itCanRegisterMiddleware() + public function fooMiddlewareQueries() { - $this->buildSchemaFromString(' - type Query { - foo: String! @middleware(checks: ["auth:web", "auth:admin"]) - bar: String! + return [ + [' + { + foo } - type Mutation { - foo(bar: String!): String! @middleware(checks: ["auth:api"]) - bar(baz: String!): String! + '], + [' + query FooQuery { + ...Foo_Fragment } - '); - $query = ' - query FooQuery { - foo - } - '; + + fragment Foo_Fragment on Query { + foo + } + '] + ]; + } - $middleware = $this->middlewareRegistry->forRequest($query); - $this->assertCount(2, $middleware); - $this->assertContains('auth:web', $middleware); - $this->assertContains('auth:admin', $middleware); + public function resolveFooMiddleware($root, $args, Context $context): int + { + $this->assertSame(AddFooProperty::VALUE, $context->request->foo); - $mutation = ' - mutation CreateFoo { - foo(bar:"baz") - } - '; - $middleware = $this->middlewareRegistry->forRequest($mutation); - $this->assertCount(1, $middleware); - $this->assertContains('auth:api', $middleware); + return 42; } /** * @test */ - public function itCanRegisterMiddlewareWithFragments() + public function itWrapsExceptionFromMiddlewareInResponse() { - $this->buildSchemaFromString(' + $this->schema = ' type Query { - foo: String! @middleware(checks: ["auth:web", "auth:admin"]) - bar: String! - } - - type Mutation { - foo(bar: String!): String! @middleware(checks: ["auth:api"]) - bar(baz: String!): String! + foo: Int @middleware(checks: ["Tests\\\Utils\\\Middleware\\\Authenticate"]) } - '); + '; - $query = ' - query FooQuery { - ...Foo_Fragment - } - - fragment Foo_Fragment on Query { + $result = $this->queryViaHttp(' + { foo } - '; - $middleware = $this->middlewareRegistry->forRequest($query); - $this->assertCount(2, $middleware); - $this->assertContains('auth:web', $middleware); - $this->assertContains('auth:admin', $middleware); + '); - $mutation = ' - mutation CreateFoo { - foo(bar:"baz") - } - '; - $middleware = $this->middlewareRegistry->forRequest($mutation); - $this->assertCount(1, $middleware); - $this->assertContains('auth:api', $middleware); + $this->assertSame(Authenticate::MESSAGE, array_get($result, 'errors.0.message')); } } diff --git a/tests/Unit/Schema/Directives/Nodes/GroupDirectiveTest.php b/tests/Unit/Schema/Directives/Nodes/GroupDirectiveTest.php index 3eab634b29..505cfdff1a 100644 --- a/tests/Unit/Schema/Directives/Nodes/GroupDirectiveTest.php +++ b/tests/Unit/Schema/Directives/Nodes/GroupDirectiveTest.php @@ -3,7 +3,8 @@ namespace Tests\Unit\Schema\Directives\Nodes; use Tests\TestCase; -use Nuwave\Lighthouse\Schema\MiddlewareRegistry; +use Nuwave\Lighthouse\Schema\Context; +use Tests\Utils\Middleware\AddFooProperty; class GroupDirectiveTest extends TestCase { @@ -49,13 +50,13 @@ public function itCanSetNamespaces() */ public function itCanSetMiddleware() { - $schema = ' + $this->schema = ' type Query { dummy: Int } - extend type Query @group(middleware: ["foo", "bar"]) { - me: String @field(resolver: "Tests\\\Utils\\\Resolvers\\\Foo@bar") + extend type Query @group(middleware: ["Tests\\\Utils\\\Middleware\\\AddFooProperty"]) { + me: String @field(resolver: "'. addslashes(self::class).'@resolve") } '; $query = ' @@ -63,11 +64,16 @@ public function itCanSetMiddleware() me } '; - $this->executeQuery($schema, $query); + $result = $this->queryViaHttp($query); + # TODO this throws because of the escape slashes + + $this->assertSame('Mario', array_get($result, 'data.me')); + } + + public function resolve($root, $args, Context $context): string + { + $this->assertSame(AddFooProperty::VALUE, $context->request->foo); - $middleware = resolve(MiddlewareRegistry::class)->query('me'); - $this->assertCount(2, $middleware); - $this->assertEquals('foo', $middleware[0]); - $this->assertEquals('bar', $middleware[1]); + return 'Mario'; } } diff --git a/tests/Unit/Schema/Execution/ContextFactoryTest.php b/tests/Unit/Schema/Execution/ContextFactoryTest.php index e9b0755437..33e3122df7 100644 --- a/tests/Unit/Schema/Execution/ContextFactoryTest.php +++ b/tests/Unit/Schema/Execution/ContextFactoryTest.php @@ -4,7 +4,6 @@ use Tests\TestCase; use Illuminate\Http\Request; -use Illuminate\Foundation\Application; use Nuwave\Lighthouse\Support\Contracts\CreatesContext; use Nuwave\Lighthouse\Support\Contracts\GraphQLContext; @@ -58,20 +57,26 @@ public function itCanGenerateCustomContext() $this->schema = " type Query { context: String @field(resolver:\"{$resolver}\") - }"; - $json = ['query' => '{ context }']; + } + "; + $query = ' + { + context + } + '; + $result = $this->queryViaHttp($query); - $result = $this->postJson('graphql', $json)->json(); - $execution = [ - 'data' => [ - 'context' => 'custom.context', + $this->assertSame( + [ + 'data' => [ + 'context' => 'custom.context', + ], ], - ]; - - $this->assertEquals($execution, $result); + $result + ); } - public function resolve($root, array $args, $context) + public function resolve($root, array $args, GraphQLContext $context): string { return $context->foo(); } diff --git a/tests/Utils/Middleware/AddFooProperty.php b/tests/Utils/Middleware/AddFooProperty.php new file mode 100644 index 0000000000..35cecb2a09 --- /dev/null +++ b/tests/Utils/Middleware/AddFooProperty.php @@ -0,0 +1,18 @@ +offsetSet('foo', self::VALUE); + + return $next($request); + } +} diff --git a/tests/Utils/Middleware/Authenticate.php b/tests/Utils/Middleware/Authenticate.php new file mode 100644 index 0000000000..65ab372234 --- /dev/null +++ b/tests/Utils/Middleware/Authenticate.php @@ -0,0 +1,17 @@ + Date: Wed, 17 Oct 2018 20:33:51 +0200 Subject: [PATCH 02/10] Polish the tests, show the actual benefit --- tests/Integration/GraphQLTest.php | 5 +- .../Directives/Fields/InjectDirectiveTest.php | 2 +- .../Fields/MiddlewareDirectiveTest.php | 80 +++++++++++++++++++ .../Extensions/GraphQLExtensionTest.php | 7 +- .../Extensions/TracingExtensionTest.php | 6 +- 5 files changed, 90 insertions(+), 10 deletions(-) diff --git a/tests/Integration/GraphQLTest.php b/tests/Integration/GraphQLTest.php index 0eda3c7334..a0102ebbec 100644 --- a/tests/Integration/GraphQLTest.php +++ b/tests/Integration/GraphQLTest.php @@ -108,7 +108,7 @@ public function itCanResolveQuery() public function itCanResolveQueryThroughController() { $this->be($this->user); - $query = ' + $data = $this->queryViaHttp(' query UserWithTasks { user { email @@ -117,9 +117,8 @@ public function itCanResolveQueryThroughController() } } } - '; + '); - $data = $this->postJson('graphql', ['query' => $query])->json(); $expected = [ 'data' => [ diff --git a/tests/Unit/Schema/Directives/Fields/InjectDirectiveTest.php b/tests/Unit/Schema/Directives/Fields/InjectDirectiveTest.php index 4fda1cde38..3d2e0cc5f8 100644 --- a/tests/Unit/Schema/Directives/Fields/InjectDirectiveTest.php +++ b/tests/Unit/Schema/Directives/Fields/InjectDirectiveTest.php @@ -33,7 +33,7 @@ public function itCanInjectDataFromContextIntoArgs() } '; - $this->postJson('graphql',['query' => $query]); + $this->queryViaHttp($query); } public function resolveUser($root, $args) diff --git a/tests/Unit/Schema/Directives/Fields/MiddlewareDirectiveTest.php b/tests/Unit/Schema/Directives/Fields/MiddlewareDirectiveTest.php index a444733be5..ddbf82b721 100644 --- a/tests/Unit/Schema/Directives/Fields/MiddlewareDirectiveTest.php +++ b/tests/Unit/Schema/Directives/Fields/MiddlewareDirectiveTest.php @@ -2,6 +2,8 @@ namespace Tests\Unit\Schema\Directives\Fields; +use Illuminate\Routing\Router; +use Orchestra\Testbench\Http\Kernel; use Tests\TestCase; use Nuwave\Lighthouse\Schema\Context; use Tests\Utils\Middleware\Authenticate; @@ -76,4 +78,82 @@ public function itWrapsExceptionFromMiddlewareInResponse() $this->assertSame(Authenticate::MESSAGE, array_get($result, 'errors.0.message')); } + + /** + * @test + */ + public function itRunsAliasedMiddleware() + { + /** @var Router $router */ + $router = $this->app['router']; + $router->aliasMiddleware('foo', AddFooProperty::class); + + $this->schema = ' + type Query { + foo: Int + @middleware(checks: ["foo"]) + @field(resolver: "'. addslashes(self::class).'@resolveFooMiddleware") + } + '; + + $result = $this->queryViaHttp(' + { + foo + } + '); + + $this->assertSame(42, array_get($result, 'data.foo')); + } + + /** + * @test + */ + public function itRunsMiddlewareGroup() + { + /** @var Router $router */ + $router = $this->app['router']; + $router->middlewareGroup('bar', [Authenticate::class]); + + $this->schema = ' + type Query { + foo: Int + @middleware(checks: ["bar"]) + } + '; + + $result = $this->queryViaHttp(' + { + foo + } + '); + + $this->assertSame(Authenticate::MESSAGE, array_get($result, 'errors.0.message')); + } + + /** + * @test + */ + public function itPassesOneFieldButThrowsInAnother() + { + $this->schema = ' + type Query { + fail: Int @middleware(checks: ["Tests\\\Utils\\\Middleware\\\Authenticate"]) + pass: Int + @middleware(checks: ["Tests\\\Utils\\\Middleware\\\AddFooProperty"]) + @field(resolver: "'. addslashes(self::class).'@resolveFooMiddleware") + } + '; + + $result = $this->queryViaHttp(' + { + fail + pass + } + '); + + $this->assertSame(42, array_get($result, 'data.pass')); + $this->assertSame(Authenticate::MESSAGE, array_get($result, 'errors.0.message')); + $this->assertSame('fail', array_get($result, 'errors.0.path.0')); + $this->assertNull(array_get($result, 'data.fail')); + } } diff --git a/tests/Unit/Schema/Extensions/GraphQLExtensionTest.php b/tests/Unit/Schema/Extensions/GraphQLExtensionTest.php index 3b8e8118cc..9969904347 100644 --- a/tests/Unit/Schema/Extensions/GraphQLExtensionTest.php +++ b/tests/Unit/Schema/Extensions/GraphQLExtensionTest.php @@ -39,8 +39,11 @@ public function itCanManipulateResponseData() foo: String }'; - $json = ['query' => '{ foo }']; - $data = $this->postJson('/graphql', $json)->json(); + $data = $this->queryViaHttp(' + { + foo + } + '); $this->assertArrayHasKey('meta', $data); $this->assertEquals('data', $data['meta']); diff --git a/tests/Unit/Schema/Extensions/TracingExtensionTest.php b/tests/Unit/Schema/Extensions/TracingExtensionTest.php index d079eba9e2..f7bcc5fae1 100644 --- a/tests/Unit/Schema/Extensions/TracingExtensionTest.php +++ b/tests/Unit/Schema/Extensions/TracingExtensionTest.php @@ -31,13 +31,11 @@ protected function getEnvironmentSetUp($app) */ public function itCanAddTracingExtensionMetaToResult() { - $query = ' + $result = $this->queryViaHttp(' { foo } - '; - - $result = $this->postJson('graphql', ['query' => $query])->json(); + '); $this->assertArrayHasKey('tracing', array_get($result, 'extensions')); $this->assertArrayHasKey('resolvers', array_get($result, 'extensions.tracing.execution')); From 55bbb062a5d99c607d7b4a2bcd8aef1fbe7b9078 Mon Sep 17 00:00:00 2001 From: spawnia Date: Wed, 17 Oct 2018 21:29:44 +0200 Subject: [PATCH 03/10] Fix a bug in GroupDirective that removed escape backslashes --- .../Directives/Nodes/GroupDirective.php | 59 ++++++++++--------- .../Directives/Nodes/GroupDirectiveTest.php | 1 - 2 files changed, 31 insertions(+), 29 deletions(-) diff --git a/src/Schema/Directives/Nodes/GroupDirective.php b/src/Schema/Directives/Nodes/GroupDirective.php index cb9501a3c8..30d90f5d99 100644 --- a/src/Schema/Directives/Nodes/GroupDirective.php +++ b/src/Schema/Directives/Nodes/GroupDirective.php @@ -31,7 +31,7 @@ class GroupDirective extends BaseDirective implements NodeManipulator * * @return string */ - public function name() + public function name(): string { return 'group'; } @@ -40,20 +40,12 @@ public function name() * @param Node $node * @param DocumentAST $documentAST * - * @throws DirectiveException + * @throws \Exception * * @return DocumentAST */ - public function manipulateSchema(Node $node, DocumentAST $documentAST) + public function manipulateSchema(Node $node, DocumentAST $documentAST): DocumentAST { - $nodeName = $node->name->value; - - if (! in_array($nodeName, ['Query', 'Mutation'])) { - $message = "The group directive can only be placed on a Query or Mutation [$nodeName]"; - - throw new DirectiveException($message); - } - $node = $this->setMiddlewareDirectiveOnFields($node); $node = $this->setNamespaceDirectiveOnFields($node); @@ -77,14 +69,20 @@ protected function setMiddlewareDirectiveOnFields($objectType) return $objectType; } - $middlewareValues = '["'.implode('", "', $middlewareValues).'"]'; - $middlewareDirective = PartialParser::directive("@middleware(checks: $middlewareValues)"); + $middlewareValues = addslashes( + implode('", "', $middlewareValues) + ); + $middlewareDirective = PartialParser::directive("@middleware(checks: [\"$middlewareValues\"])"); - $objectType->fields = new NodeList(collect($objectType->fields)->map(function (FieldDefinitionNode $fieldDefinition) use ($middlewareDirective) { - $fieldDefinition->directives = $fieldDefinition->directives->merge([$middlewareDirective]); + $objectType->fields = new NodeList( + collect($objectType->fields) + ->map(function (FieldDefinitionNode $fieldDefinition) use ($middlewareDirective) { + $fieldDefinition->directives = $fieldDefinition->directives->merge([$middlewareDirective]); - return $fieldDefinition; - })->toArray()); + return $fieldDefinition; + }) + ->toArray() + ); return $objectType; } @@ -110,19 +108,24 @@ protected function setNamespaceDirectiveOnFields($objectType) $namespaceValue = addslashes($namespaceValue); - $objectType->fields = new NodeList(collect($objectType->fields)->map(function (FieldDefinitionNode $fieldDefinition) use ($namespaceValue) { - $previousNamespaces = ASTHelper::directiveDefinition( - $fieldDefinition, - (new NamespaceDirective)->name() - ); + $objectType->fields = new NodeList( + collect($objectType->fields) + ->map(function (FieldDefinitionNode $fieldDefinition) use ($namespaceValue) { + $existingNamespaces = ASTHelper::directiveDefinition( + $fieldDefinition, + (new NamespaceDirective)->name() + ); + + $newNamespaceDirective = $existingNamespaces + ? $this->mergeNamespaceOnExistingDirective($namespaceValue, $existingNamespaces) + : PartialParser::directive("@namespace(field: \"$namespaceValue\", complexity: \"$namespaceValue\")"); - $previousNamespaces = $previousNamespaces - ? $this->mergeNamespaceOnExistingDirective($namespaceValue, $previousNamespaces) - : PartialParser::directive("@namespace(field: \"$namespaceValue\", complexity: \"$namespaceValue\")"); - $fieldDefinition->directives = $fieldDefinition->directives->merge([$previousNamespaces]); + $fieldDefinition->directives = $fieldDefinition->directives->merge([$newNamespaceDirective]); - return $fieldDefinition; - })->toArray()); + return $fieldDefinition; + }) + ->toArray() + ); return $objectType; } diff --git a/tests/Unit/Schema/Directives/Nodes/GroupDirectiveTest.php b/tests/Unit/Schema/Directives/Nodes/GroupDirectiveTest.php index 505cfdff1a..5642fee503 100644 --- a/tests/Unit/Schema/Directives/Nodes/GroupDirectiveTest.php +++ b/tests/Unit/Schema/Directives/Nodes/GroupDirectiveTest.php @@ -65,7 +65,6 @@ public function itCanSetMiddleware() } '; $result = $this->queryViaHttp($query); - # TODO this throws because of the escape slashes $this->assertSame('Mario', array_get($result, 'data.me')); } From 9b2db47f18da9c3054f0fcdd0d75ffbff7067e53 Mon Sep 17 00:00:00 2001 From: spawnia Date: Wed, 17 Oct 2018 21:56:27 +0200 Subject: [PATCH 04/10] Add a hint to the config that warns from overusing global middleware --- config/config.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/config/config.php b/config/config.php index 3be496d90b..4c7e769655 100644 --- a/config/config.php +++ b/config/config.php @@ -33,10 +33,16 @@ | Additional configuration for the route group. | Check options here https://lumen.laravel.com/docs/routing#route-groups | + | Beware that middleware defined here runs before the GraphQL execution phase. + | This means that errors will cause the whole query to abort and return a + | response that is not spec-compliant. It is preferable to use directives + | to add middleware to single fields in the schema. + | Read more about this in the docs https://lighthouse-php.netlify.com/docs/auth.html#apply-auth-middleware + | */ 'route' => [ 'prefix' => '', - // 'middleware' => ['web','api'], // [ 'loghttp'] + // 'middleware' => ['loghttp'] ], /* From a04e73507d81f0b9a947d5185874f9e30f8b7dc6 Mon Sep 17 00:00:00 2001 From: spawnia Date: Wed, 17 Oct 2018 22:54:32 +0200 Subject: [PATCH 05/10] WIP explore request lifecycle seperation bug --- .../Directives/Fields/MiddlewareDirective.php | 13 +++++- .../Directives/Fields/NamespaceDirective.php | 7 ++- .../Directives/Nodes/GroupDirective.php | 8 +++- .../Fields/MiddlewareDirectiveTest.php | 25 ++++------- .../Directives/Nodes/GroupDirectiveTest.php | 44 ++++++++++++++----- tests/Utils/Middleware/AddFooProperty.php | 11 ++++- 6 files changed, 75 insertions(+), 33 deletions(-) diff --git a/src/Schema/Directives/Fields/MiddlewareDirective.php b/src/Schema/Directives/Fields/MiddlewareDirective.php index e8aac5d05a..990f0f22e9 100644 --- a/src/Schema/Directives/Fields/MiddlewareDirective.php +++ b/src/Schema/Directives/Fields/MiddlewareDirective.php @@ -15,6 +15,9 @@ class MiddlewareDirective extends BaseDirective implements FieldMiddleware { + /** @var string todo remove as soon as name() is static itself */ + const NAME = 'middleware'; + /** @var Pipeline */ protected $pipeline; /** @var CreatesContext */ @@ -66,14 +69,22 @@ public function handleField(FieldValue $value, \Closure $next): FieldValue return $next( $value->setResolver( function ($root, array $args, GraphQLContext $context, ResolveInfo $resolveInfo) use ($resolver, $middleware) { + dump($resolveInfo->fieldName, $middleware); return $this->pipeline ->send( - $context->request() + Request::createFrom( + $context->request() + ) + // Duplicate the request so we have independent lifecycles for the fields +// $context->request() +// ->instance() +// ->duplicate() ) ->through( $middleware ) ->then(function (Request $request) use ($resolver, $root, $args, $resolveInfo){ + dump(data_get($request,'foo')); return $resolver( $root, $args, diff --git a/src/Schema/Directives/Fields/NamespaceDirective.php b/src/Schema/Directives/Fields/NamespaceDirective.php index 91d89efe74..2f042917a1 100644 --- a/src/Schema/Directives/Fields/NamespaceDirective.php +++ b/src/Schema/Directives/Fields/NamespaceDirective.php @@ -19,13 +19,16 @@ */ class NamespaceDirective implements Directive { + /** @var string todo remove as soon as name() is static itself */ + const NAME = 'namespace'; + /** * Name of the directive. * * @return string */ - public function name() + public function name(): string { - return 'namespace'; + return self::NAME; } } diff --git a/src/Schema/Directives/Nodes/GroupDirective.php b/src/Schema/Directives/Nodes/GroupDirective.php index 30d90f5d99..23226c88d9 100644 --- a/src/Schema/Directives/Nodes/GroupDirective.php +++ b/src/Schema/Directives/Nodes/GroupDirective.php @@ -77,6 +77,12 @@ protected function setMiddlewareDirectiveOnFields($objectType) $objectType->fields = new NodeList( collect($objectType->fields) ->map(function (FieldDefinitionNode $fieldDefinition) use ($middlewareDirective) { + // If the field already has middleware defined, skip over it + // Field middleware are more specific then those defined by @group +// if (ASTHelper::directiveDefinition($fieldDefinition, MiddlewareDirective::NAME)){ +// return $fieldDefinition; +// } + $fieldDefinition->directives = $fieldDefinition->directives->merge([$middlewareDirective]); return $fieldDefinition; @@ -113,7 +119,7 @@ protected function setNamespaceDirectiveOnFields($objectType) ->map(function (FieldDefinitionNode $fieldDefinition) use ($namespaceValue) { $existingNamespaces = ASTHelper::directiveDefinition( $fieldDefinition, - (new NamespaceDirective)->name() + NamespaceDirective::NAME ); $newNamespaceDirective = $existingNamespaces diff --git a/tests/Unit/Schema/Directives/Fields/MiddlewareDirectiveTest.php b/tests/Unit/Schema/Directives/Fields/MiddlewareDirectiveTest.php index ddbf82b721..bab3650958 100644 --- a/tests/Unit/Schema/Directives/Fields/MiddlewareDirectiveTest.php +++ b/tests/Unit/Schema/Directives/Fields/MiddlewareDirectiveTest.php @@ -21,15 +21,15 @@ public function itCallsFooMiddleware(string $query) { $this->schema = ' type Query { - foo: Int + foo: String @middleware(checks: ["Tests\\\Utils\\\Middleware\\\AddFooProperty"]) - @field(resolver: "'. addslashes(self::class).'@resolveFooMiddleware") + @field(resolver: "Tests\\\Utils\\\Middleware\\\AddFooProperty@resolve") } '; $result = $this->queryViaHttp($query); - $this->assertSame(42, array_get($result, 'data.foo')); + $this->assertSame(AddFooProperty::DID_RUN, array_get($result, 'data.foo')); } public function fooMiddlewareQueries() @@ -52,13 +52,6 @@ public function fooMiddlewareQueries() ]; } - public function resolveFooMiddleware($root, $args, Context $context): int - { - $this->assertSame(AddFooProperty::VALUE, $context->request->foo); - - return 42; - } - /** * @test */ @@ -90,9 +83,9 @@ public function itRunsAliasedMiddleware() $this->schema = ' type Query { - foo: Int + foo: String @middleware(checks: ["foo"]) - @field(resolver: "'. addslashes(self::class).'@resolveFooMiddleware") + @field(resolver: "Tests\\\Utils\\\Middleware\\\AddFooProperty@resolve") } '; @@ -102,7 +95,7 @@ public function itRunsAliasedMiddleware() } '); - $this->assertSame(42, array_get($result, 'data.foo')); + $this->assertSame(AddFooProperty::DID_RUN, array_get($result, 'data.foo')); } /** @@ -138,9 +131,9 @@ public function itPassesOneFieldButThrowsInAnother() $this->schema = ' type Query { fail: Int @middleware(checks: ["Tests\\\Utils\\\Middleware\\\Authenticate"]) - pass: Int + pass: String @middleware(checks: ["Tests\\\Utils\\\Middleware\\\AddFooProperty"]) - @field(resolver: "'. addslashes(self::class).'@resolveFooMiddleware") + @field(resolver: "Tests\\\Utils\\\Middleware\\\AddFooProperty@resolve") } '; @@ -151,7 +144,7 @@ public function itPassesOneFieldButThrowsInAnother() } '); - $this->assertSame(42, array_get($result, 'data.pass')); + $this->assertSame(AddFooProperty::DID_RUN, array_get($result, 'data.pass')); $this->assertSame(Authenticate::MESSAGE, array_get($result, 'errors.0.message')); $this->assertSame('fail', array_get($result, 'errors.0.path.0')); $this->assertNull(array_get($result, 'data.fail')); diff --git a/tests/Unit/Schema/Directives/Nodes/GroupDirectiveTest.php b/tests/Unit/Schema/Directives/Nodes/GroupDirectiveTest.php index 5642fee503..a8bb4454bd 100644 --- a/tests/Unit/Schema/Directives/Nodes/GroupDirectiveTest.php +++ b/tests/Unit/Schema/Directives/Nodes/GroupDirectiveTest.php @@ -3,7 +3,7 @@ namespace Tests\Unit\Schema\Directives\Nodes; use Tests\TestCase; -use Nuwave\Lighthouse\Schema\Context; +use Tests\Utils\Middleware\Authenticate; use Tests\Utils\Middleware\AddFooProperty; class GroupDirectiveTest extends TestCase @@ -51,12 +51,8 @@ public function itCanSetNamespaces() public function itCanSetMiddleware() { $this->schema = ' - type Query { - dummy: Int - } - - extend type Query @group(middleware: ["Tests\\\Utils\\\Middleware\\\AddFooProperty"]) { - me: String @field(resolver: "'. addslashes(self::class).'@resolve") + type Query @group(middleware: ["Tests\\\Utils\\\Middleware\\\AddFooProperty"]) { + me: String @field(resolver: "Tests\\\Utils\\\Middleware\\\AddFooProperty@resolve") } '; $query = ' @@ -66,13 +62,39 @@ public function itCanSetMiddleware() '; $result = $this->queryViaHttp($query); - $this->assertSame('Mario', array_get($result, 'data.me')); + $this->assertSame(AddFooProperty::DID_RUN, array_get($result, 'data.me')); } - public function resolve($root, $args, Context $context): string + /** + * @test + */ + public function itCanOverrideGroupMiddlewareInField() { - $this->assertSame(AddFooProperty::VALUE, $context->request->foo); + $this->schema = ' + type Query @group(middleware: ["Tests\\\Utils\\\Middleware\\\Authenticate"]) { + withFoo: String + @middleware(checks: ["Tests\\\Utils\\\Middleware\\\AddFooProperty"]) + @field(resolver: "Tests\\\Utils\\\Middleware\\\AddFooProperty@resolve") + withNothing: String + @middleware(checks: []) + @field(resolver: "Tests\\\Utils\\\Middleware\\\AddFooProperty@resolve") + fail: Int + } + '; + $query = ' + { + withFoo + withNothing + fail + } + '; + $result = $this->queryViaHttp($query); - return 'Mario'; + $this->assertSame(AddFooProperty::DID_RUN, array_get($result, 'data.withFoo')); + # TODO make sure the request is cleared between middlewares + $this->assertSame(AddFooProperty::DID_NOT_RUN, array_get($result, 'data.withNothing')); + $this->assertSame(Authenticate::MESSAGE, array_get($result, 'errors.0.message')); + $this->assertSame('fail', array_get($result, 'errors.0.path.0')); + $this->assertNull(array_get($result, 'data.fail')); } } diff --git a/tests/Utils/Middleware/AddFooProperty.php b/tests/Utils/Middleware/AddFooProperty.php index 35cecb2a09..48d2ad49cd 100644 --- a/tests/Utils/Middleware/AddFooProperty.php +++ b/tests/Utils/Middleware/AddFooProperty.php @@ -4,15 +4,22 @@ namespace Tests\Utils\Middleware; use Illuminate\Http\Request; +use Nuwave\Lighthouse\Schema\Context; class AddFooProperty { - const VALUE = 'This value is set on the foo attribute.'; + const DID_RUN = 'The middleware did run successfully.'; + const DID_NOT_RUN = 'The middleware did not run.'; public function handle(Request $request, \Closure $next) { - $request->offsetSet('foo', self::VALUE); + $request->offsetSet('foo', self::DID_RUN); return $next($request); } + + public function resolve($root, $args, Context $context): string + { + return data_get($context->request, 'foo', self::DID_NOT_RUN); + } } From 4cdc829588d3f6ff34853a9e8dc777a1d4d993b5 Mon Sep 17 00:00:00 2001 From: spawnia Date: Fri, 19 Oct 2018 19:44:32 +0200 Subject: [PATCH 06/10] Test middleware in a way that does not require seperate lifecycle I chose to implement it this way because Context is supposed to be shared between fields, so changes to the Request object are fine to propagate between fields. Should the need for seperate lifecycles pop back up, it is not necessary to store a clone of the middleware before resolving each field, as webonyx/graphql-php shares the context object by default. --- .../Directives/Fields/MiddlewareDirective.php | 18 ++--------- .../Directives/Nodes/GroupDirective.php | 7 +++-- tests/TestCase.php | 7 +++++ .../Fields/MiddlewareDirectiveTest.php | 30 +++++++++---------- .../Directives/Nodes/GroupDirectiveTest.php | 22 +++++++------- tests/Utils/Middleware/AddFooProperty.php | 25 ---------------- tests/Utils/Middleware/CountRuns.php | 28 +++++++++++++++++ 7 files changed, 66 insertions(+), 71 deletions(-) delete mode 100644 tests/Utils/Middleware/AddFooProperty.php create mode 100644 tests/Utils/Middleware/CountRuns.php diff --git a/src/Schema/Directives/Fields/MiddlewareDirective.php b/src/Schema/Directives/Fields/MiddlewareDirective.php index 990f0f22e9..8b6176affb 100644 --- a/src/Schema/Directives/Fields/MiddlewareDirective.php +++ b/src/Schema/Directives/Fields/MiddlewareDirective.php @@ -4,8 +4,8 @@ use Illuminate\Http\Request; use Illuminate\Routing\Router; -use GraphQL\Type\Definition\ResolveInfo; use Nuwave\Lighthouse\Support\Pipeline; +use GraphQL\Type\Definition\ResolveInfo; use Illuminate\Routing\MiddlewareNameResolver; use Nuwave\Lighthouse\Schema\Values\FieldValue; use Nuwave\Lighthouse\Schema\Directives\BaseDirective; @@ -69,22 +69,10 @@ public function handleField(FieldValue $value, \Closure $next): FieldValue return $next( $value->setResolver( function ($root, array $args, GraphQLContext $context, ResolveInfo $resolveInfo) use ($resolver, $middleware) { - dump($resolveInfo->fieldName, $middleware); return $this->pipeline - ->send( - Request::createFrom( - $context->request() - ) - // Duplicate the request so we have independent lifecycles for the fields -// $context->request() -// ->instance() -// ->duplicate() - ) - ->through( - $middleware - ) + ->send($context->request()) + ->through($middleware) ->then(function (Request $request) use ($resolver, $root, $args, $resolveInfo){ - dump(data_get($request,'foo')); return $resolver( $root, $args, diff --git a/src/Schema/Directives/Nodes/GroupDirective.php b/src/Schema/Directives/Nodes/GroupDirective.php index 23226c88d9..3f5b004e0f 100644 --- a/src/Schema/Directives/Nodes/GroupDirective.php +++ b/src/Schema/Directives/Nodes/GroupDirective.php @@ -15,6 +15,7 @@ use Nuwave\Lighthouse\Schema\Directives\BaseDirective; use Nuwave\Lighthouse\Support\Contracts\NodeManipulator; use Nuwave\Lighthouse\Schema\Directives\Fields\NamespaceDirective; +use Nuwave\Lighthouse\Schema\Directives\Fields\MiddlewareDirective; /** * Class GroupDirective. @@ -79,9 +80,9 @@ protected function setMiddlewareDirectiveOnFields($objectType) ->map(function (FieldDefinitionNode $fieldDefinition) use ($middlewareDirective) { // If the field already has middleware defined, skip over it // Field middleware are more specific then those defined by @group -// if (ASTHelper::directiveDefinition($fieldDefinition, MiddlewareDirective::NAME)){ -// return $fieldDefinition; -// } + if (ASTHelper::directiveDefinition($fieldDefinition, MiddlewareDirective::NAME)){ + return $fieldDefinition; + } $fieldDefinition->directives = $fieldDefinition->directives->merge([$middlewareDirective]); diff --git a/tests/TestCase.php b/tests/TestCase.php index 7aa5f97912..857f0ba6d5 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -6,6 +6,7 @@ use GraphQL\Type\Schema; use GraphQL\Executor\ExecutionResult; use Laravel\Scout\ScoutServiceProvider; +use Tests\Utils\Middleware\CountRuns; use Tests\Utils\Policies\AuthServiceProvider; use Orchestra\Database\ConsoleServiceProvider; use Orchestra\Testbench\TestCase as BaseTestCase; @@ -65,6 +66,12 @@ function () { ]); } + protected function tearDown() + { + parent::tearDown(); + CountRuns::$runCounter = 0; + } + /** * Execute query/mutation. * diff --git a/tests/Unit/Schema/Directives/Fields/MiddlewareDirectiveTest.php b/tests/Unit/Schema/Directives/Fields/MiddlewareDirectiveTest.php index bab3650958..c051f4b02a 100644 --- a/tests/Unit/Schema/Directives/Fields/MiddlewareDirectiveTest.php +++ b/tests/Unit/Schema/Directives/Fields/MiddlewareDirectiveTest.php @@ -2,12 +2,10 @@ namespace Tests\Unit\Schema\Directives\Fields; -use Illuminate\Routing\Router; -use Orchestra\Testbench\Http\Kernel; use Tests\TestCase; -use Nuwave\Lighthouse\Schema\Context; +use Illuminate\Routing\Router; +use Tests\Utils\Middleware\CountRuns; use Tests\Utils\Middleware\Authenticate; -use Tests\Utils\Middleware\AddFooProperty; class MiddlewareDirectiveTest extends TestCase { @@ -21,15 +19,15 @@ public function itCallsFooMiddleware(string $query) { $this->schema = ' type Query { - foo: String - @middleware(checks: ["Tests\\\Utils\\\Middleware\\\AddFooProperty"]) - @field(resolver: "Tests\\\Utils\\\Middleware\\\AddFooProperty@resolve") + foo: Int + @middleware(checks: ["Tests\\\Utils\\\Middleware\\\CountRuns"]) + @field(resolver: "Tests\\\Utils\\\Middleware\\\CountRuns@resolve") } '; $result = $this->queryViaHttp($query); - $this->assertSame(AddFooProperty::DID_RUN, array_get($result, 'data.foo')); + $this->assertSame(1, array_get($result, 'data.foo')); } public function fooMiddlewareQueries() @@ -79,13 +77,13 @@ public function itRunsAliasedMiddleware() { /** @var Router $router */ $router = $this->app['router']; - $router->aliasMiddleware('foo', AddFooProperty::class); + $router->aliasMiddleware('foo', CountRuns::class); $this->schema = ' type Query { - foo: String + foo: Int @middleware(checks: ["foo"]) - @field(resolver: "Tests\\\Utils\\\Middleware\\\AddFooProperty@resolve") + @field(resolver: "Tests\\\Utils\\\Middleware\\\CountRuns@resolve") } '; @@ -95,7 +93,7 @@ public function itRunsAliasedMiddleware() } '); - $this->assertSame(AddFooProperty::DID_RUN, array_get($result, 'data.foo')); + $this->assertSame(1, array_get($result, 'data.foo')); } /** @@ -131,9 +129,9 @@ public function itPassesOneFieldButThrowsInAnother() $this->schema = ' type Query { fail: Int @middleware(checks: ["Tests\\\Utils\\\Middleware\\\Authenticate"]) - pass: String - @middleware(checks: ["Tests\\\Utils\\\Middleware\\\AddFooProperty"]) - @field(resolver: "Tests\\\Utils\\\Middleware\\\AddFooProperty@resolve") + pass: Int + @middleware(checks: ["Tests\\\Utils\\\Middleware\\\CountRuns"]) + @field(resolver: "Tests\\\Utils\\\Middleware\\\CountRuns@resolve") } '; @@ -144,7 +142,7 @@ public function itPassesOneFieldButThrowsInAnother() } '); - $this->assertSame(AddFooProperty::DID_RUN, array_get($result, 'data.pass')); + $this->assertSame(1, array_get($result, 'data.pass')); $this->assertSame(Authenticate::MESSAGE, array_get($result, 'errors.0.message')); $this->assertSame('fail', array_get($result, 'errors.0.path.0')); $this->assertNull(array_get($result, 'data.fail')); diff --git a/tests/Unit/Schema/Directives/Nodes/GroupDirectiveTest.php b/tests/Unit/Schema/Directives/Nodes/GroupDirectiveTest.php index a8bb4454bd..6d48408693 100644 --- a/tests/Unit/Schema/Directives/Nodes/GroupDirectiveTest.php +++ b/tests/Unit/Schema/Directives/Nodes/GroupDirectiveTest.php @@ -4,7 +4,6 @@ use Tests\TestCase; use Tests\Utils\Middleware\Authenticate; -use Tests\Utils\Middleware\AddFooProperty; class GroupDirectiveTest extends TestCase { @@ -51,8 +50,8 @@ public function itCanSetNamespaces() public function itCanSetMiddleware() { $this->schema = ' - type Query @group(middleware: ["Tests\\\Utils\\\Middleware\\\AddFooProperty"]) { - me: String @field(resolver: "Tests\\\Utils\\\Middleware\\\AddFooProperty@resolve") + type Query @group(middleware: ["Tests\\\Utils\\\Middleware\\\CountRuns"]) { + me: Int @field(resolver: "Tests\\\Utils\\\Middleware\\\CountRuns@resolve") } '; $query = ' @@ -62,7 +61,7 @@ public function itCanSetMiddleware() '; $result = $this->queryViaHttp($query); - $this->assertSame(AddFooProperty::DID_RUN, array_get($result, 'data.me')); + $this->assertSame(1, array_get($result, 'data.me')); } /** @@ -72,12 +71,12 @@ public function itCanOverrideGroupMiddlewareInField() { $this->schema = ' type Query @group(middleware: ["Tests\\\Utils\\\Middleware\\\Authenticate"]) { - withFoo: String - @middleware(checks: ["Tests\\\Utils\\\Middleware\\\AddFooProperty"]) - @field(resolver: "Tests\\\Utils\\\Middleware\\\AddFooProperty@resolve") - withNothing: String + withFoo: Int + @middleware(checks: ["Tests\\\Utils\\\Middleware\\\CountRuns"]) + @field(resolver: "Tests\\\Utils\\\Middleware\\\CountRuns@resolve") + withNothing: Int @middleware(checks: []) - @field(resolver: "Tests\\\Utils\\\Middleware\\\AddFooProperty@resolve") + @field(resolver: "Tests\\\Utils\\\Middleware\\\CountRuns@resolve") fail: Int } '; @@ -90,9 +89,8 @@ public function itCanOverrideGroupMiddlewareInField() '; $result = $this->queryViaHttp($query); - $this->assertSame(AddFooProperty::DID_RUN, array_get($result, 'data.withFoo')); - # TODO make sure the request is cleared between middlewares - $this->assertSame(AddFooProperty::DID_NOT_RUN, array_get($result, 'data.withNothing')); + $this->assertSame(1, array_get($result, 'data.withFoo')); + $this->assertSame(1, array_get($result, 'data.withNothing')); $this->assertSame(Authenticate::MESSAGE, array_get($result, 'errors.0.message')); $this->assertSame('fail', array_get($result, 'errors.0.path.0')); $this->assertNull(array_get($result, 'data.fail')); diff --git a/tests/Utils/Middleware/AddFooProperty.php b/tests/Utils/Middleware/AddFooProperty.php deleted file mode 100644 index 48d2ad49cd..0000000000 --- a/tests/Utils/Middleware/AddFooProperty.php +++ /dev/null @@ -1,25 +0,0 @@ -offsetSet('foo', self::DID_RUN); - - return $next($request); - } - - public function resolve($root, $args, Context $context): string - { - return data_get($context->request, 'foo', self::DID_NOT_RUN); - } -} diff --git a/tests/Utils/Middleware/CountRuns.php b/tests/Utils/Middleware/CountRuns.php new file mode 100644 index 0000000000..7812380d2b --- /dev/null +++ b/tests/Utils/Middleware/CountRuns.php @@ -0,0 +1,28 @@ + Date: Sat, 20 Oct 2018 11:31:30 +0200 Subject: [PATCH 07/10] Enable both @middleware and @group directives to be used on object types to set field middleware --- .../Directives/Fields/MiddlewareDirective.php | 113 ++++++++++++++++-- .../Directives/Nodes/GroupDirective.php | 44 +------ .../Fields/MiddlewareDirectiveTest.php | 40 +++++++ 3 files changed, 145 insertions(+), 52 deletions(-) diff --git a/src/Schema/Directives/Fields/MiddlewareDirective.php b/src/Schema/Directives/Fields/MiddlewareDirective.php index 8b6176affb..78b313ad32 100644 --- a/src/Schema/Directives/Fields/MiddlewareDirective.php +++ b/src/Schema/Directives/Fields/MiddlewareDirective.php @@ -4,16 +4,28 @@ use Illuminate\Http\Request; use Illuminate\Routing\Router; +use GraphQL\Language\AST\Node; +use GraphQL\Language\AST\NodeList; +use Illuminate\Support\Collection; use Nuwave\Lighthouse\Support\Pipeline; use GraphQL\Type\Definition\ResolveInfo; +use Nuwave\Lighthouse\Schema\AST\ASTHelper; +use Nuwave\Lighthouse\Schema\AST\DocumentAST; +use GraphQL\Language\AST\FieldDefinitionNode; use Illuminate\Routing\MiddlewareNameResolver; +use Nuwave\Lighthouse\Schema\AST\PartialParser; use Nuwave\Lighthouse\Schema\Values\FieldValue; +use Nuwave\Lighthouse\Exceptions\ParseException; +use GraphQL\Language\AST\ObjectTypeExtensionNode; +use GraphQL\Language\AST\ObjectTypeDefinitionNode; +use Nuwave\Lighthouse\Exceptions\DirectiveException; use Nuwave\Lighthouse\Schema\Directives\BaseDirective; use Nuwave\Lighthouse\Support\Contracts\CreatesContext; use Nuwave\Lighthouse\Support\Contracts\GraphQLContext; +use Nuwave\Lighthouse\Support\Contracts\NodeManipulator; use Nuwave\Lighthouse\Support\Contracts\FieldMiddleware; -class MiddlewareDirective extends BaseDirective implements FieldMiddleware +class MiddlewareDirective extends BaseDirective implements FieldMiddleware, NodeManipulator { /** @var string todo remove as soon as name() is static itself */ const NAME = 'middleware'; @@ -53,17 +65,9 @@ public function name(): string */ public function handleField(FieldValue $value, \Closure $next): FieldValue { - /** @var Router $router */ - $router = resolve('router'); - $middleware = $router->getMiddleware(); - $middlewareGroups = $router->getMiddlewareGroups(); - - $middleware = collect($this->directiveArgValue('checks')) - ->map(function ($name) use ($middleware, $middlewareGroups){ - return (array) MiddlewareNameResolver::resolve($name, $middleware, $middlewareGroups); - }) - ->flatten(); - + $middleware = $this->getQualifiedMiddlewareNames( + $this->directiveArgValue('checks') + ); $resolver = $value->getResolver(); return $next( @@ -84,4 +88,89 @@ function ($root, array $args, GraphQLContext $context, ResolveInfo $resolveInfo) ) ); } + + /** + * @param Node $node + * @param DocumentAST $documentAST + * + * @throws ParseException + * @throws DirectiveException + * + * @return DocumentAST + */ + public function manipulateSchema(Node $node, DocumentAST $documentAST): DocumentAST + { + return $documentAST->setDefinition( + self::addMiddlewareDirectiveToFields( + $node, + $this->directiveArgValue('checks') + ) + ); + } + + /** + * @param ObjectTypeDefinitionNode|ObjectTypeExtensionNode $objectType + * @param array $middlewareArgValue + * + * @throws ParseException + * @throws DirectiveException + * + * @return ObjectTypeDefinitionNode|ObjectTypeExtensionNode + */ + public static function addMiddlewareDirectiveToFields($objectType, $middlewareArgValue) + { + if ( ! $objectType instanceof ObjectTypeDefinitionNode + && ! $objectType instanceof ObjectTypeExtensionNode + ) { + throw new DirectiveException( + 'The ' . self::NAME . ' directive may only be placed on fields or object types.' + ); + } + + $middlewareArgValue = collect($middlewareArgValue) + ->map(function(string $middleware){ + // Add slashes, as re-parsing of the values removes a level of slashes + return addslashes($middleware); + }) + ->implode('", "'); + + $middlewareDirective = PartialParser::directive("@middleware(checks: [\"$middlewareArgValue\"])"); + + $objectType->fields = new NodeList( + collect($objectType->fields) + ->map(function (FieldDefinitionNode $fieldDefinition) use ($middlewareDirective) { + // If the field already has middleware defined, skip over it + // Field middleware are more specific then those defined on a type + if (ASTHelper::directiveDefinition($fieldDefinition, MiddlewareDirective::NAME)){ + return $fieldDefinition; + } + + $fieldDefinition->directives = $fieldDefinition->directives->merge([$middlewareDirective]); + + return $fieldDefinition; + }) + ->toArray() + ); + + return $objectType; + } + + /** + * @param $middlewareArgValue + * + * @return \Illuminate\Support\Collection + */ + protected static function getQualifiedMiddlewareNames($middlewareArgValue): Collection + { + /** @var Router $router */ + $router = resolve('router'); + $middleware = $router->getMiddleware(); + $middlewareGroups = $router->getMiddlewareGroups(); + + return collect($middlewareArgValue) + ->map(function (string $name) use ($middleware, $middlewareGroups) { + return (array) MiddlewareNameResolver::resolve($name, $middleware, $middlewareGroups); + }) + ->flatten(); + } } diff --git a/src/Schema/Directives/Nodes/GroupDirective.php b/src/Schema/Directives/Nodes/GroupDirective.php index 3f5b004e0f..9dd97b4b0b 100644 --- a/src/Schema/Directives/Nodes/GroupDirective.php +++ b/src/Schema/Directives/Nodes/GroupDirective.php @@ -47,7 +47,10 @@ public function name(): string */ public function manipulateSchema(Node $node, DocumentAST $documentAST): DocumentAST { - $node = $this->setMiddlewareDirectiveOnFields($node); + if($middlewareValues = $this->directiveArgValue('middleware')){ + $node = MiddlewareDirective::addMiddlewareDirectiveToFields($node, $middlewareValues); + } + $node = $this->setNamespaceDirectiveOnFields($node); $documentAST->setDefinition($node); @@ -55,45 +58,6 @@ public function manipulateSchema(Node $node, DocumentAST $documentAST): Document return $documentAST; } - /** - * @param ObjectTypeDefinitionNode|ObjectTypeExtensionNode $objectType - * - * @throws \Exception - * - * @return ObjectTypeDefinitionNode|ObjectTypeExtensionNode - */ - protected function setMiddlewareDirectiveOnFields($objectType) - { - $middlewareValues = $this->directiveArgValue('middleware'); - - if (! $middlewareValues) { - return $objectType; - } - - $middlewareValues = addslashes( - implode('", "', $middlewareValues) - ); - $middlewareDirective = PartialParser::directive("@middleware(checks: [\"$middlewareValues\"])"); - - $objectType->fields = new NodeList( - collect($objectType->fields) - ->map(function (FieldDefinitionNode $fieldDefinition) use ($middlewareDirective) { - // If the field already has middleware defined, skip over it - // Field middleware are more specific then those defined by @group - if (ASTHelper::directiveDefinition($fieldDefinition, MiddlewareDirective::NAME)){ - return $fieldDefinition; - } - - $fieldDefinition->directives = $fieldDefinition->directives->merge([$middlewareDirective]); - - return $fieldDefinition; - }) - ->toArray() - ); - - return $objectType; - } - /** * @param ObjectTypeDefinitionNode|ObjectTypeExtensionNode $objectType * diff --git a/tests/Unit/Schema/Directives/Fields/MiddlewareDirectiveTest.php b/tests/Unit/Schema/Directives/Fields/MiddlewareDirectiveTest.php index c051f4b02a..0f9e697c5a 100644 --- a/tests/Unit/Schema/Directives/Fields/MiddlewareDirectiveTest.php +++ b/tests/Unit/Schema/Directives/Fields/MiddlewareDirectiveTest.php @@ -6,6 +6,9 @@ use Illuminate\Routing\Router; use Tests\Utils\Middleware\CountRuns; use Tests\Utils\Middleware\Authenticate; +use Nuwave\Lighthouse\Schema\AST\ASTHelper; +use Nuwave\Lighthouse\Schema\AST\ASTBuilder; +use Nuwave\Lighthouse\Exceptions\DirectiveException; class MiddlewareDirectiveTest extends TestCase { @@ -147,4 +150,41 @@ public function itPassesOneFieldButThrowsInAnother() $this->assertSame('fail', array_get($result, 'errors.0.path.0')); $this->assertNull(array_get($result, 'data.fail')); } + + /** + * @test + */ + public function itThrowsWhenDefiningMiddlewareOnInvalidTypes() + { + $this->expectException(DirectiveException::class); + $this->buildSchemaWithDefaultQuery(' + scalar Foo @middleware + '); + } + + /** + * @test + */ + public function itAddsMiddlewareDirectiveToFields() + { + $document = ASTBuilder::generate(' + type Query @middleware(checks: ["auth", "Tests\\\Utils\\\Middleware\\\Authenticate", "api"]) { + foo: Int + } + '); + + $queryType = $document->queryTypeDefinition(); + + $middlewareOnFooArguments = $queryType->fields[0]->directives[0]; + $fieldMiddlewares = ASTHelper::directiveArgValue($middlewareOnFooArguments, 'checks'); + + $this->assertSame( + [ + 'auth', + 'Tests\\Utils\\Middleware\\Authenticate', + 'api' + ], + $fieldMiddlewares + ); + } } From 585ee5a0fed70a334429bd5f1c01f6b82863bae9 Mon Sep 17 00:00:00 2001 From: spawnia Date: Sat, 20 Oct 2018 11:34:11 +0200 Subject: [PATCH 08/10] Fix merge --- tests/Unit/Schema/Directives/Fields/InjectDirectiveTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Unit/Schema/Directives/Fields/InjectDirectiveTest.php b/tests/Unit/Schema/Directives/Fields/InjectDirectiveTest.php index fa6586c556..2c78b95ece 100644 --- a/tests/Unit/Schema/Directives/Fields/InjectDirectiveTest.php +++ b/tests/Unit/Schema/Directives/Fields/InjectDirectiveTest.php @@ -34,7 +34,7 @@ public function itCanInjectDataFromContextIntoArgs() } '; - $result = $this->queryViaHttp($query)->json(); + $result = $this->queryViaHttp($query); $this->assertSame(1, array_get($result, 'data.me.id')); } From 79084821963054f4f1a3efbcd37c029bbcb65f19 Mon Sep 17 00:00:00 2001 From: spawnia Date: Sat, 20 Oct 2018 11:40:32 +0200 Subject: [PATCH 09/10] Revert the Context so it has the public property "user" available for access --- src/Schema/Context.php | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/Schema/Context.php b/src/Schema/Context.php index 3deb2cf27b..3337d496de 100644 --- a/src/Schema/Context.php +++ b/src/Schema/Context.php @@ -3,18 +3,25 @@ namespace Nuwave\Lighthouse\Schema; use Illuminate\Http\Request; -use Illuminate\Contracts\Auth\Authenticatable as User; +use Illuminate\Contracts\Auth\Authenticatable; use Nuwave\Lighthouse\Support\Contracts\GraphQLContext; class Context implements GraphQLContext { /** - * Http request. + * An instance of the incoming HTTP request. * * @var Request */ public $request; + /** + * An instance of the currently authenticated user. + * + * @var Authenticatable|null + */ + public $user; + /** * Create new context. * @@ -23,6 +30,7 @@ class Context implements GraphQLContext public function __construct(Request $request) { $this->request = $request; + $this->user = $request->user(); } /** @@ -30,11 +38,11 @@ public function __construct(Request $request) * * May be null since some fields may be accessible without authentication. * - * @return User|null + * @return Authenticatable|null */ public function user() { - return $this->request->user(); + return $this->user; } /** From 61bb0cb7bd6fc059b544f81edf2fb62b36dd6278 Mon Sep 17 00:00:00 2001 From: spawnia Date: Sat, 20 Oct 2018 11:59:30 +0200 Subject: [PATCH 10/10] Cleanup --- tests/TestCase.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/TestCase.php b/tests/TestCase.php index 857f0ba6d5..2dd66e11b1 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -4,9 +4,9 @@ use GraphQL\Error\Debug; use GraphQL\Type\Schema; +use Tests\Utils\Middleware\CountRuns; use GraphQL\Executor\ExecutionResult; use Laravel\Scout\ScoutServiceProvider; -use Tests\Utils\Middleware\CountRuns; use Tests\Utils\Policies\AuthServiceProvider; use Orchestra\Database\ConsoleServiceProvider; use Orchestra\Testbench\TestCase as BaseTestCase;