GraphQL Query support #1358

alanpoulain · 2017-09-11T17:17:43Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	api-platform/api-platform#230
License	MIT
Doc PR

Support of GraphQL Queries by using https://github.com/webonyx/graphql-php.
Mutations will be done later.
Based on the work of @raoulclais.
Endpoint is /graphql.
GraphiQL is used for the UI.

soyuka · 2017-09-11T19:43:15Z

src/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php

@@ -142,6 +143,7 @@ private function handleConfig(ContainerBuilder $container, array $config, array
        $container->setParameter('api_platform.eager_loading.max_joins', $config['eager_loading']['max_joins']);
        $container->setParameter('api_platform.eager_loading.fetch_partial', $config['eager_loading']['fetch_partial']);
        $container->setParameter('api_platform.eager_loading.force_eager', $config['eager_loading']['force_eager']);
+        $container->setParameter('api_platform.enable_graphql', $config['enable_graphql']);


I'd prefer:

graphdql: enabled: true

So that we try not to face the same issue as with swagger when adding more features.

dunglas · 2017-09-11T20:03:48Z

composer.json

@@ -23,7 +23,8 @@
        "symfony/property-access": "^2.7 || ^3.0 || ^4.0",
        "symfony/property-info": "^3.1 || ^4.0",
        "symfony/serializer": "^3.1 || ^4.0",
-        "willdurand/negotiation": "^2.0.3"
+        "willdurand/negotiation": "^2.0.3",
+        "webonyx/graphql-php": "^0.10.2"


This dep should be moved in require-dev and added to the suggest section.

dunglas · 2017-09-11T20:11:30Z

src/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php

@@ -253,7 +253,7 @@ private function getSerializerContext(string $resourceClass, string $contextType
            $request = $this->requestStack->getCurrentRequest();
        }

-        if (null !== $this->serializerContextBuilder && null !== $request) {
+        if (null !== $this->serializerContextBuilder && null !== $request && 'api_graphql_entrypoint' !== $request->attributes->get('_route')) {


I suggest to check for the application/graphql MIME type instead. It will be more reliable.

Forget about that, content-type can also be application/json, so it's a bad idea. I'm still not a found of checking the route name (it creates a hard coupling with the Symfony Routing component).

Maybe can you just set a new _graphql attributes to true in the action, and check it here. It will be easier for end users to hook into this.

dunglas · 2017-09-11T20:12:10Z

src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php


-    public function __construct(ManagerRegistry $managerRegistry, RequestStack $requestStack, ResourceMetadataFactoryInterface $resourceMetadataFactory, bool $enabled = true, bool $clientEnabled = false, bool $clientItemsPerPage = false, int $itemsPerPage = 30, string $pageParameterName = 'page', string $enabledParameterName = 'pagination', string $itemsPerPageParameterName = 'itemsPerPage', int $maximumItemPerPage = null)
+    public function __construct(ManagerRegistry $managerRegistry, RequestStack $requestStack, ResourceMetadataFactoryInterface $resourceMetadataFactory, bool $enabled = true, bool $clientEnabled = false, bool $clientItemsPerPage = false, int $itemsPerPage = 30, string $pageParameterName = 'page', string $enabledParameterName = 'pagination', string $itemsPerPageParameterName = 'itemsPerPage', int $maximumItemPerPage = null, bool $enableGraphql)


You must set a default value (false?) to $enableGraphql or it's a BC break.

dunglas · 2017-09-11T20:13:39Z

src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php

@@ -76,6 +78,10 @@ public function applyToCollection(QueryBuilder $queryBuilder, QueryNameGenerator
        }

        $itemsPerPage = $resourceMetadata->getCollectionOperationAttribute($operationName, 'pagination_items_per_page', $this->itemsPerPage, true);
+        if ($this->enableGraphql) {
+            // @TODO Retrieve GraphQL 'first' argument using the request?


Maybe can you create another PaginationExtension class specific to GraphQL if the handling is really different than this one.

dunglas · 2017-09-11T20:13:58Z

src/Bridge/Graphql/Executor.php

+ *
+ * @author Alan Poulain <contact@alanpoulain.eu>
+ */
+class Executor


dunglas · 2017-09-11T20:24:19Z

src/Bridge/Graphql/Type/SchemaBuilder.php

+                continue;
+            }
+
+            // @TODO Manage multiple identifiers?


Maybe can you use just reuse this method: https://github.com/api-platform/core/blob/master/src/Bridge/Doctrine/Orm/Util/IdentifierManagerTrait.php#L40

dunglas · 2017-09-11T20:24:57Z

src/Bridge/Graphql/Type/SchemaBuilder.php

+                $graphqlType = $this->getResourceType($className, $resourceMetadata);
+                break;
+
+            case Type::BUILTIN_TYPE_ARRAY:


You can remove all those useless cases.

dunglas · 2017-09-11T20:29:35Z

src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php

@@ -68,6 +68,7 @@ public function getConfigTreeBuilder()
                ->booleanNode('enable_nelmio_api_doc')->defaultValue(false)->info('Enable the Nelmio Api doc integration.')->end()
                ->booleanNode('enable_swagger')->defaultValue(true)->info('Enable the Swagger documentation and export.')->end()
                ->booleanNode('enable_swagger_ui')->defaultValue(class_exists(TwigBundle::class))->info('Enable Swagger ui.')->end()
+                ->booleanNode('enable_graphql')->defaultValue(false)->info('Enable GraphQL.')->end()


I suggest defaultValue(class_exists(GraphQL::class)) (as for Swagger UI the line before).

dunglas · 2017-09-11T20:30:20Z

src/Bridge/Symfony/Routing/ApiLoader.php

    private $resourceClassDirectories;
    private $subresourceOperationFactory;

-    public function __construct(KernelInterface $kernel, ResourceNameCollectionFactoryInterface $resourceNameCollectionFactory, ResourceMetadataFactoryInterface $resourceMetadataFactory, OperationPathResolverInterface $operationPathResolver, ContainerInterface $container, array $formats, array $resourceClassDirectories = [], SubresourceOperationFactoryInterface $subresourceOperationFactory = null)
+    public function __construct(KernelInterface $kernel, ResourceNameCollectionFactoryInterface $resourceNameCollectionFactory, ResourceMetadataFactoryInterface $resourceMetadataFactory, OperationPathResolverInterface $operationPathResolver, ContainerInterface $container, array $formats, bool $enableGraphql, array $resourceClassDirectories = [], SubresourceOperationFactoryInterface $subresourceOperationFactory = null)


bool $enableGraphql must be the last parameter and have a default value or it's a BC break.

Could you explain why? It seems to me this is an internal service. I don't see what it could break.

not marked as @internal

Some people (including me :D) use API Platform as a reusable library in other contexts (Silex, Laravel...). This change will break this class of uses.

OK thanks for the explanation.

dunglas · 2017-09-11T20:32:54Z

src/Bridge/Graphql/Type/SchemaBuilder.php

+                'resolve' => function ($root, $args, $context, ResolveInfo $resolveInfo) use ($resource, $operationName) {
+                    // @TODO Pagination
+                    $collection = $this->collectionDataProvider->getCollection($resource, $operationName);
+                    return $this->normalizer->normalize($collection, null, ['graphql_selected_fields' => $resolveInfo->getFieldSelection()]);


Cant' we use symfony/symfony#18834 instead of graphql_selected_fields? I've introduced this option exactly for GraphQL support :)

I can replace graphql_selected_fields by attributes, yes. But it will not change the initial issue: to replace the generated URI (call to normalizeRelation) by the subresource data.

I'm not even sure this is a good idea. If I replace it, it will have consequences when using attributes in a REST context. See its use here: https://github.com/alanpoulain/core/blob/2ed4e8c54ac8579857fc401ad32b18ece0f962a4/src/Serializer/AbstractItemNormalizer.php#L416

Ok, keep it as is for now :)

Using attributes is not useful in this context because the fields are filtered by graphql-php. Maybe it can improves performance though?

It will definitely improve performances, serialization is a costly operation, we should definitely prevent serializing useless data.

dunglas · 2017-09-11T20:40:18Z

I'm wondering if we should not totally separate our current (REST) "operations" subsystem from GraphQL. They are not the same concept and aren't really compatible. CRUD fit well with REST (resource based API), but I don't think it fits well with GraphQL (more service oriented).
Maybe should we provide in the future a different way to implement REST operations than mutations. I guess it will be very difficult to implement generic operations that work for both REST and GraphQL.

WDYT @alanpoulain @raoulclais @api-platform/core-team?

dunglas · 2017-09-11T20:42:36Z

On the other end, it should be easy to provide basic CRUD mutations for GraphQL... Maybe can we just add a new option to operations to define if it's a rest operation, a graphql mutation, or both.

dunglas · 2017-09-11T20:50:18Z

src/Action/GraphqlEntrypointAction.php

+        $request = $this->requestStack->getCurrentRequest();
+
+        // @TODO Manage GET request.
+        $input = json_decode($request->getContent(), true);


You should check that the incoming Content-Type is application/json or application/graphql:

if it's application/json, the current behavior is OK

if it's application/graphql, the raw content is a GraphQL query (so no json_decode needed)

throw an error for all other content types (maybe can you also serve https://github.com/graphql/graphiql if the content type is text/html)?

theofidry · 2017-09-11T23:06:06Z

@dunglas in my experience what I find painfully laborious with GraphQL is to re-define those crudish operations. It's not like you need all the CRUD endpoints all the time, even with a REST API, but they are a PITA to re-define again and again. Another thing that API Platform nailed right is to provide a standardised structure whereas with a GraphQL API, you tend to have a bit of everything simply because there is no standard "enforced".

mvrhov · 2017-09-13T08:44:26Z

Watch out. the server part of GraphQL has a patent on it! I don't care for software patents as I'm from EU, but others using this inside US should be careful.

dunglas · 2017-09-13T08:59:53Z

We must indeed make that clear in the documentation. And it's why it will be an optional disablable feature.

meyerbaptiste · 2017-09-13T13:10:45Z

features/bootstrap/GraphqlContext.php

+class GraphqlContext implements Context
+{
+    /**
+     * @var \Behatch\Context\RestContext


Can you import the class and use its short name instead?

meyerbaptiste · 2017-09-13T13:15:09Z

src/Action/GraphqlEntrypointAction.php

+
+    public function __construct(RequestStack $requestStack, SchemaBuilderInterface $schemaBuilder, ExecutorInterface $executor)
+    {
+        $this->requestStack = $requestStack;


This is useless, just type hint Request in your __invoke() method definition:

public function __invoke(Request $request): Response

meyerbaptiste · 2017-09-13T13:15:44Z

src/Action/GraphqlEntrypointAction.php

+        $this->executor = $executor;
+    }
+
+    public function __invoke(): JsonResponse


Response instead of JsonResponse.

meyerbaptiste · 2017-09-13T13:24:04Z

src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php

@@ -76,6 +78,10 @@ public function applyToCollection(QueryBuilder $queryBuilder, QueryNameGenerator
        }

        $itemsPerPage = $resourceMetadata->getCollectionOperationAttribute($operationName, 'pagination_items_per_page', $this->itemsPerPage, true);
+        if ($this->graphqlEnabled) {


It should be $request->attributes->get('_graphql'), nop?

Yes... Nice catch!

meyerbaptiste · 2017-09-13T13:25:59Z

src/Bridge/Graphql/Executor.php

+     */
+    public function executeQuery(...$args): ExecutionResult
+    {
+        return call_user_func_array(GraphQL::class.'::executeQuery', $args);


return GraphQL::executeQuery(...$args);

meyerbaptiste · 2017-09-13T14:08:36Z

src/Bridge/Graphql/Resolver/CollectionResolverFactory.php

+            $request = $this->requestStack->getCurrentRequest();
+            $collectionArgs = $request->attributes->get('_graphql_collections_args', []);
+            $collectionArgs[$resourceClass] = $args;
+            $request->attributes->set('_graphql_collections_args', $collectionArgs);


$request->attributes->set('_graphql_collections_args', [$resourceClass => $args] + $request->attributes->get('_graphql_collections_args', []));

meyerbaptiste · 2017-09-13T14:09:54Z

src/Bridge/Graphql/Resolver/CollectionResolverFactory.php

+    {
+        return function ($root, $args, $context, ResolveInfo $resolveInfo) use ($resourceClass, $operationName) {
+            // @TODO Pagination.
+            $request = $this->requestStack->getCurrentRequest();


You have to test RequestStack::getCurrentRequest() is not null.

In which cases it could occur?

When? We don't know but it could occur this is what the contract says and we have to respect the method signature!

It definitely occurs in when running this class in command line.

meyerbaptiste · 2017-09-13T14:13:42Z

src/Bridge/Graphql/Resolver/ItemResolverFactory.php

+            }
+            $selectedFields = $resolveInfo->getFieldSelection();
+
+            return $this->normalizer->normalize($item, null, ['graphql_selected_fields' => $selectedFields]);


One line:

return ($item = $itemDataProvider->getItem($resourceClass, $args['id'], $operationName)) ? $normalizer->normalize($item, null, ['graphql_selected_fields' => $resolveInfo->getFieldSelection()]) : null;

Sometimes, I prefer to be more verbose to avoid potential bugs and to modify more easily the code. But if it is preferred in API Platform, no problem.

meyerbaptiste · 2017-09-13T15:19:26Z

src/Bridge/Graphql/Type/SchemaBuilder.php

+        foreach ($this->resourceNameCollectionFactory->create() as $resource) {
+            $resourceMetadata = $this->resourceMetadataFactory->create($resource);
+
+            $queryFields = array_merge($queryFields, $this->getQueryFields($resource, $resourceMetadata));


$queryFields = $this->getQueryFields($resource, $resourceMetadata) + $queryFields;

meyerbaptiste · 2017-09-13T15:57:51Z

src/Bridge/Graphql/Type/SchemaBuilder.php

+            $propertyMetadata = $this->propertyMetadataFactory->create($resource, $property);
+
+            $propertyType = $propertyMetadata->getType();
+            if (null !== $propertyType) {


if ($property = $propertyMetadata->getType()) {

Are you sure @meyerbaptiste? We need an explicit check for null values (not for booleans).

Even if the return type is object|null?

null !== $property = $propertyMetadata->getType() then.

We should double check.

meyerbaptiste · 2017-09-13T16:04:51Z

src/Bridge/Graphql/Type/SchemaBuilder.php

+                $graphqlType = GraphQLType::string();
+                break;
+            case Type::BUILTIN_TYPE_OBJECT:
+                if (\DateTime::class === $type->getClassName()) {


if (is_subclass_of($type->getClassName(), \DateTimeInterface::class)) {

We can even nitpick with is_a($type->getClassName(), \DateTimeInterface::class, true)

meyerbaptiste · 2017-09-13T16:24:02Z

src/Bridge/Graphql/Type/SchemaBuilder.php

+     */
+    private function getOperations(ResourceMetadata $resourceMetadata, bool $isQuery, bool $isItem): array
+    {
+        return array_filter($isItem ? $resourceMetadata->getItemOperations() : $resourceMetadata->getCollectionOperations(), function ($operation) use ($isQuery) {


Prefer a foreach with a generator here:

$operations = $isItem ? $resourceMetadata->getItemOperations() : $resourceMetadata->getCollectionOperations(); foreach ($operations as $operationName => $operation) { if (isset($operation['controller']) || !isset($operation['method'])) { continue; } if ($isQuery && Request::METHOD_GET !== $operation['method'] || !$isQuery && Request::METHOD_GET === $operation['method']) { continue; } yield $operationName => $operation; }

meyerbaptiste · 2017-09-13T16:26:27Z

src/Bridge/Graphql/Type/SchemaBuilder.php

+            ];
+        }
+
+        throw new \LogicException(sprintf('Missing identifier field for resource "%s"', $resource));


"Missing identifier field for resource \"$resource\"."

meyerbaptiste · 2017-09-13T16:30:35Z

src/Bridge/Graphql/Type/SchemaBuilder.php

+            $propertyIsIdentifier = $propertyMetadata->isIdentifier() ?? false;
+            $propertyType = $propertyMetadata->getType();
+
+            if (!$propertyIsIdentifier || null === $propertyType) {


|| !$propertyType

Sructinizr (legitimately) add warnings when not checking explicitly the type when dealing with nullvalues. I would keep this as is.

meyerbaptiste · 2017-09-13T16:32:16Z

src/Bridge/Graphql/Type/SchemaBuilder.php

+    }
+
+    /**
+     * Gets the field arguments of the identifier of a given resource.


Missing @throws annotation.

meyerbaptiste · 2017-09-13T16:32:31Z

src/Bridge/Graphql/Type/SchemaBuilder.php

+    }
+
+    /**
+     * Converts a built-in type to its GraphQL equivalent.


Missing @throws annotation.

alanpoulain · 2017-09-15T19:47:37Z

Added: pagination support.

dunglas · 2017-09-18T14:07:43Z

src/Bridge/Graphql/Resolver/CollectionResolverFactory.php

+    private $requestStack;
+    private $paginationEnabled;
+
+    public function __construct(CollectionDataProviderInterface $collectionDataProvider, SubresourceDataProviderInterface $subresourceDataProvider, NormalizerInterface $normalizer, IdentifiersExtractorInterface $identifiersExtractor, RequestStack $requestStack, bool $paginationEnabled)


IMO the request should not be injected that deep. The low level GraphQL implementation should be agnostic of the transport layer (HTTP, AMQP...).

Can't we extract the offset and other data needed in the controller layer and pass the base64-encoded values to createCollectionResolver?

This resolver does not need the request to resolve the query. It's the PaginationExtension who needs to retrieve the offset and the limit.
The resolver adds the offset / limit argument of the GraphQL query to the request attributes to be used by the PaginationExtension.

dunglas · 2017-09-18T14:10:40Z

src/Bridge/Graphql/Resolver/CollectionResolverFactory.php

+                if (isset($args['after'])) {
+                    $after = base64_decode($args['after'], true);
+                    $offset = (int) $after;
+                    $offset = false === $after ? $offset : ++$offset;


Shouldn't we throw an error if base64_decode returns an error?

I can yes, if you prefer. I thought it was the responsibility of the client to use the cursors we give them.

Yes but we cannot trust client input.

dunglas · 2017-09-18T14:11:45Z

src/Bridge/Graphql/Resolver/CollectionResolverFactory.php

+                $data = ['edges' => [], 'pageInfo' => ['endCursor' => null, 'hasNextPage' => false]];
+                if ($collection instanceof PaginatorInterface) {
+                    $totalItems = $collection->getTotalItems();
+                    if ($totalItems > 0) {


Can be merge with the previous if:

if ($collection instanceof PaginatorInterface && $totalItems = $collection->getTotalItems() > 0)

dunglas · 2017-09-18T14:13:01Z

src/Bridge/Graphql/Resolver/CollectionResolverFactory.php

+                $collection = $this->collectionDataProvider->getCollection($resourceClass, $operationName);
+            }
+
+            if ($this->paginationEnabled) {


I would move all the following block in a new method to improve readability and decrease the cyclomatic complexity.

dunglas · 2017-09-21T06:10:43Z

.travis.yml

@@ -11,7 +11,7 @@ matrix:
  include:
    - php: '7.0'
    - php: '7.1'
-      env: coverage=1 lint=1
+      env: coverage=0 lint=1


We need to restore this later, but I've no idea how to fix the problem for now.

What is the problem with the coverage?

We reach the ulimit defined by Travis... Too many open files.

can't we ulimit -n 4000 before install? It happens on my local computer too when testing the Extension.

Unfortunately no, Travis disallow to increase the default value.

soyuka · 2017-09-21T08:08:42Z

src/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php

@@ -253,7 +253,7 @@ private function getSerializerContext(string $resourceClass, string $contextType
            $request = $this->requestStack->getCurrentRequest();
        }

-        if (null !== $this->serializerContextBuilder && null !== $request) {
+        if (null !== $this->serializerContextBuilder && null !== $request && !$request->attributes->get('_graphql')) {


Because of https://github.com/api-platform/core/blob/master/src/Serializer/SerializerContextBuilder.php#L42. There is no serialization context in the request.

Shouldn't there be one? I mean, usually _api_resource_class is declared on every route no?

Also maybe that the SerializerContextBuilder shouldn't throw at all.

Maybe can we find something expliciter, but as GraphQL isn't REST, it looks okish that _api_resource_class doesn't exist.

soyuka · 2017-09-21T08:11:44Z

src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php

+        if ($request->attributes->get('_graphql')) {
+            $collectionArgs = $request->attributes->get('_graphql_collections_args', []);
+            if (isset($collectionArgs[$resourceClass]['after'])) {
+                $after = base64_decode($collectionArgs[$resourceClass]['after'], true);


\base64_decode as below? Don't remember the policy on this but we need it to be consistent.

We should use \ everywhere, it's faster. IIRC, there is a PHP CS Fixer rule for that.

soyuka · 2017-09-21T08:15:40Z

src/Bridge/Graphql/Resolver/ItemResolverFactory.php

+                    }
+
+                    if (\is_array($args[$rootIdentifier])) {
+                        if (\count($args[$rootIdentifier]) > 1) {


Same on these, we currently are not prefixing with \ in the base code.

So we should add and enable this fixer in our php-cs-fixer config!

Yes but it is a risky one, IIRC, it breaks our tests.

wdyt we remove \ for now and we will do this in another pr.

soyuka · 2017-09-21T08:19:36Z

src/Bridge/Graphql/Resolver/ItemResolverFactory.php

+                    return null;
+                }
+
+                $item = $this->itemDataProvider->getItem($resourceClass, \count($identifiers) > 1 ? implode(';', $identifiers) : $uniqueIdentifier[0]);


Just a note to myself but this would need refactoring if we can implement some kind of identifiers denormalization (#1309) not linked to any bridge (code duplication).

soyuka · 2017-09-21T08:22:49Z

src/DataProvider/ChainSubresourceDataProvider.php

@@ -35,7 +35,7 @@ public function __construct(array $dataProviders)
    /**
     * {@inheritdoc}
     */
-    public function getSubresource(string $resourceClass, array $identifiers, array $context, string $operationName)
+    public function getSubresource(string $resourceClass, array $identifiers, array $context, string $operationName = null)


will conflict haha

soyuka · 2017-09-21T08:26:30Z

update-js.sh

+mkdir -p $dest
+cp node_modules/graphiql/graphiql.min.js $dest
+cp node_modules/graphiql/graphiql.css $dest
+


gosh 🐱 maybe we should kinda create a js repository for all the graphql stuff in the future and just une one module for all these.

This also means that we're now publishing react sources in the source code of api-platform, totally 👎 on this personally...

Creating our own JS repo will not help. AFAIK, it's not possible to upload artifacts on Packagist, so the JS must be included in the repo. It's still a dependency, the way it is downloaded doesn't change anything from a legal POV.

This script can be dramatically improved but we can still handle that later

dunglas · 2017-09-21T06:15:27Z

update-js.sh

+  rm -Rf $dest
+fi
+mkdir -p $dest
+cp node_modules/es6-promise/dist/es6-promise.auto.min.js $dest


We can assume devs will use a modern browser, and all modern browsers support promises now: https://caniuse.com/#feat=promises

I would just rely on the browser without downloading the polyfill.

dunglas · 2017-09-21T06:15:47Z

update-js.sh

+  rm -Rf $dest
+fi
+mkdir -p $dest
+cp node_modules/fetch/lib/fetch.js $dest


Same for fetch: https://caniuse.com/#feat=fetch

dunglas · 2017-09-21T06:17:49Z

update-js.sh

+
+dest=src/Bridge/Symfony/Bundle/Resources/public/react/
+
+yarn add --production --no-lockfile react react-dom


Can't we just run yarn one time, copy all the files we need then remove node_modules?

yarn add --production --no-lockfile swagger-ui-dist react react-dom graphiql

It will speed up the install and simplify this script.

dunglas · 2017-09-21T06:18:57Z

features/bootstrap/GraphqlContext.php

+     * @var array
+     */
+    private $graphqlRequest;
+    /**


Missing blank line before.

dunglas · 2017-09-21T06:20:43Z

src/Action/GraphqlEntrypointAction.php

+    private $title;
+    private $graphiqlEnabled;
+
+    public function __construct(SchemaBuilderInterface $schemaBuilder, ExecutorInterface $executor, \Twig_Environment $twig, bool $debug, bool $graphiqlEnabled, string $title)


Can you add default values to make it easier to use this class alone?

bool $debug = false, bool $graphiqlEnabled = false, string $title = ''

dunglas · 2017-09-21T09:38:30Z

src/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php

@@ -253,7 +253,7 @@ private function getSerializerContext(string $resourceClass, string $contextType
            $request = $this->requestStack->getCurrentRequest();
        }

-        if (null !== $this->serializerContextBuilder && null !== $request) {
+        if (null !== $this->serializerContextBuilder && null !== $request && !$request->attributes->get('_graphql')) {


Maybe can we find something expliciter, but as GraphQL isn't REST, it looks okish that _api_resource_class doesn't exist.

dunglas · 2017-09-21T09:39:05Z

src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php

+        if ($request->attributes->get('_graphql')) {
+            $collectionArgs = $request->attributes->get('_graphql_collections_args', []);
+            if (isset($collectionArgs[$resourceClass]['after'])) {
+                $after = base64_decode($collectionArgs[$resourceClass]['after'], true);


We should use \ everywhere, it's faster. IIRC, there is a PHP CS Fixer rule for that.

dunglas · 2017-09-21T09:41:21Z

update-js.sh

+mkdir -p $dest
+cp node_modules/graphiql/graphiql.min.js $dest
+cp node_modules/graphiql/graphiql.css $dest
+


Creating our own JS repo will not help. AFAIK, it's not possible to upload artifacts on Packagist, so the JS must be included in the repo. It's still a dependency, the way it is downloaded doesn't change anything from a legal POV.

This script can be dramatically improved but we can still handle that later

meyerbaptiste · 2017-09-21T09:01:29Z

src/Action/GraphqlEntrypointAction.php

+    {
+        $query = $request->query->get('query');
+        $operation = $request->query->get('operation');
+        if ([] !== $variables = $request->query->get('variables', [])) {


if ($variables = $request->...) {

meyerbaptiste · 2017-09-21T09:08:22Z

src/Api/IdentifiersExtractor.php

+        foreach ($this->propertyNameCollectionFactory->create($resourceClass) as $property) {
+            $propertyMetadata = $this->propertyMetadataFactory->create($resourceClass, $property);
+            $propertyIsIdentifier = $propertyMetadata->isIdentifier() ?? false;
+            if (!$propertyIsIdentifier) {


if ($propertyMetadata->isIdentifier() ?? false) { $identifiers[] = $property; }

meyerbaptiste · 2017-09-21T09:11:38Z

src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php

@@ -83,6 +83,10 @@ public function applyToCollection(QueryBuilder $queryBuilder, QueryNameGenerator
        }

        $itemsPerPage = $resourceMetadata->getCollectionOperationAttribute($operationName, 'pagination_items_per_page', $this->itemsPerPage, true);
+        if ($request->attributes->get('_graphql')) {


has() instead of get()?

get is safer IMO (in case it is explicitly set to false)

Okay, there are a lot of places where he uses has() so it would also be safer to replace them!

meyerbaptiste · 2017-09-21T09:16:52Z

src/Bridge/Graphql/Resolver/AbstractResolverFactory.php

+ *
+ * @internal
+ */
+class AbstractResolverFactory


Missing abstract here!

meyerbaptiste · 2017-09-21T09:21:46Z

src/Bridge/Graphql/Resolver/AbstractResolverFactory.php

+
+        return $this->subresourceDataProvider->getSubresource($subresourceClass, $identifiers, [
+            'property' => $rootProperty,
+            'identifiers' => array_map(function ($rootIdentifier) use ($rootClass) {return [$rootIdentifier, $rootClass]; }, $rootIdentifiers),


I prefer a foreach for performances.

Btw both loops can be merged. I take care of your comments.

meyerbaptiste · 2017-09-21T09:22:48Z

src/Bridge/Graphql/Resolver/CollectionResolverFactory.php

+    }
+
+    /**
+     * @throws \InvalidArgumentException


You have to remove the backslash.

meyerbaptiste · 2017-09-21T09:28:55Z

src/Bridge/Graphql/Resolver/CollectionResolverFactory.php

+                if (false === $after) {
+                    throw new InvalidArgumentException(sprintf('Cursor %s is invalid', $args['after']));
+                }
+                $offset = (int) $after;


1 + (int) $after;

meyerbaptiste · 2017-09-21T09:31:27Z

src/Bridge/Graphql/Resolver/CollectionResolverFactory.php

+            }
+
+            $data = ['edges' => [], 'pageInfo' => ['endCursor' => null, 'hasNextPage' => false]];
+            if ($collection instanceof PaginatorInterface && ($totalItems = $collection->getTotalItems()) > 0) {


Can't we also support PartialPaginatorInterface?

👍 but in another PR please

meyerbaptiste · 2017-09-21T09:37:51Z

src/Bridge/Graphql/Resolver/ItemResolverFactory.php

+                            throw new InvalidArgumentException('Composite identifiers are not allowed for a resource already used as a composite identifier');
+                        }
+
+                        $identifiers[] = $rootIdentifier.'='.current($args[$rootIdentifier]);


If you use current() to get the first element then it's not really safe!

meyerbaptiste · 2017-09-21T09:52:36Z

src/Bridge/Graphql/Resolver/ItemResolverFactory.php

+                            throw new InvalidArgumentException('Composite identifiers are not allowed for a resource already used as a composite identifier');
+                        }
+
+                        $identifiers[] = $rootIdentifier.'='.\current($args[$rootIdentifier]);


If you use current() to get the first item then it's not really safe!

alanpoulain · 2017-09-26T21:54:02Z

https://medium.com/@leeb/relicensing-the-graphql-specification-e7d07a52301b

theofidry · 2017-10-06T07:23:48Z

🎉 I will try it out ASAP but that's unlikely to be before 2-3 weeks

* GraphQL Query support (api-platform#1358) * Fix missing service when no Varnish URL is defined * [PropertyFilter] Fix whitelist comparison (api-platform#1379) * Remove wrong doc * Swagger subcollection documentation issue (api-platform#1395) * Make the requirements configurable Closes api-platform#1401 * Provide a better exception message and type Getting "Not found" on a route where you are getting an object can get really confusing. * Bump branch alias to 2.2.x-dev 2.1.x-dev is already taken by the 2.1 branch, and this should represent the next minor version anyway. * Fix tests * Reuse PriorityTaggedServiceTrait from symfony * Improve payload support and remove duplicate code in ConstraintViolationListNormalizer (api-platform#1416) * Filter Annotation implementation Parent class filters (needs test) Support for nested properties Tests Fix some comments and remove id=>id in compiler pass * Throw on abstract data providers / filters * Remove an unused var * Remove useless badges * Enable the coverage * Fix some quality issues * Add job to test upstream libs deprecations If api-platform uses upstream libs in a deprecated way, we should be aware of it. * Add job to test upstream libs deprecations If api-platform uses upstream libs in a deprecated way, we should be aware of it. * Fix missing cache tag on empty collections * Allow plain IDs with `allow_plain_identifiers` * Fix indentation for GraphQL features. * Add JSON API basic support (api-platform#785, api-platform#1036, api-platform#1175) * Clean Behat tests * Fix tags addition with an empty value * Document swagger-specific description options … and where to find them should there be newer ones. * Fix PHPUnit tests * Fix missing return statement * Support & compatibility for PHP7.2 * Add feature to update swagger context for properties * Generator compat improvements (api-platform#1429) * Add support for resource names without namespace * [SF 4.0] Make actions explicitly public * Allow phpdocumentor/reflection-docblock 4 * fix hydra documentation normalizer with subresources * Create a base collection normalizer * Fix request auto-runner * Update the changelog * Update changelog

ghost · 2017-11-19T17:48:17Z

Hello there!
There 's a little quick-start for grapqh and apiplatform!! Or something explaining how start!!
have a nice day

dunglas · 2017-11-19T18:44:32Z

@jorgeluisacostaalonso, you need to use api-platform/core:dev-master. Then require webonyx/graphql-php, and you're down, the GraphQL API is available at the /graphql endpoint.

soyuka reviewed Sep 11, 2017

View reviewed changes

dunglas requested changes Sep 11, 2017

View reviewed changes

alanpoulain force-pushed the feature/graphql branch 2 times, most recently from d39539d to 63e6fe6 Compare September 12, 2017 17:22

alanpoulain force-pushed the feature/graphql branch 2 times, most recently from 7c05c1d to f1212fe Compare September 13, 2017 10:47

meyerbaptiste requested changes Sep 13, 2017

View reviewed changes

alanpoulain force-pushed the feature/graphql branch from f1212fe to ca321d8 Compare September 13, 2017 15:44

meyerbaptiste requested changes Sep 13, 2017

View reviewed changes

alanpoulain force-pushed the feature/graphql branch from 51b8fdd to 7f30a56 Compare September 15, 2017 19:46

alanpoulain force-pushed the feature/graphql branch 5 times, most recently from 2a01be4 to 9097958 Compare September 18, 2017 10:35

dunglas requested changes Sep 18, 2017

View reviewed changes

alanpoulain force-pushed the feature/graphql branch from 8da30cf to 00ebee7 Compare September 19, 2017 14:38

alanpoulain changed the title ~~[WIP] GraphQL support~~ GraphQL Query support Sep 20, 2017

alanpoulain force-pushed the feature/graphql branch 3 times, most recently from 546d9e4 to d09ee00 Compare September 20, 2017 20:02

dunglas approved these changes Sep 21, 2017

View reviewed changes

dunglas reviewed Sep 21, 2017

View reviewed changes

dunglas force-pushed the feature/graphql branch from 73575a9 to 7a395ad Compare September 21, 2017 08:11

soyuka requested changes Sep 21, 2017

View reviewed changes

dunglas approved these changes Sep 21, 2017

View reviewed changes

meyerbaptiste requested changes Sep 21, 2017

View reviewed changes

meyerbaptiste reviewed Sep 21, 2017

View reviewed changes

alanpoulain force-pushed the feature/graphql branch from 93759cb to 5c10f7b Compare September 29, 2017 14:08

alanpoulain and others added 10 commits October 1, 2017 19:33

Support of GraphQL queries

e143e2f

Deactivate coverage in Travis

78cff4e

Minor refactoring

4fc0e56

Fix some comments

e702fda

Improve update-js.sh

9004263

Fix most comments

e72bdf5

Remove es6-promise and fetch files

394caf7

Remove hardcoded app_dev in init-graphiql

f754304

Use reset instead of current

e7b5c49

Better error output

0277faf

alanpoulain force-pushed the feature/graphql branch from cfc6269 to 0277faf Compare October 1, 2017 17:33

Do not add a field to the schema if it is not readable/writable

6664085

alanpoulain force-pushed the feature/graphql branch from 29d6f90 to 6664085 Compare October 4, 2017 17:52

dunglas merged commit 7852f10 into api-platform:master Oct 6, 2017

alanpoulain deleted the feature/graphql branch October 15, 2017 14:30

hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018

GraphQL Query support (api-platform#1358)

8fa30a5

kiler129 mentioned this pull request Feb 11, 2019

DateTimeZone Is Not Accessible In GraphQL #2509

Closed


		public function __construct(ManagerRegistry $managerRegistry, RequestStack $requestStack, ResourceMetadataFactoryInterface $resourceMetadataFactory, bool $enabled = true, bool $clientEnabled = false, bool $clientItemsPerPage = false, int $itemsPerPage = 30, string $pageParameterName = 'page', string $enabledParameterName = 'pagination', string $itemsPerPageParameterName = 'itemsPerPage', int $maximumItemPerPage = null)
		public function __construct(ManagerRegistry $managerRegistry, RequestStack $requestStack, ResourceMetadataFactoryInterface $resourceMetadataFactory, bool $enabled = true, bool $clientEnabled = false, bool $clientItemsPerPage = false, int $itemsPerPage = 30, string $pageParameterName = 'page', string $enabledParameterName = 'pagination', string $itemsPerPageParameterName = 'itemsPerPage', int $maximumItemPerPage = null, bool $enableGraphql)


		dest=src/Bridge/Symfony/Bundle/Resources/public/react/

		yarn add --production --no-lockfile react react-dom

Uh oh!

GraphQL Query support #1358

GraphQL Query support #1358

Uh oh!

Conversation

alanpoulain commented Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanpoulain Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas commented Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dunglas commented Sep 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theofidry commented Sep 11, 2017

Uh oh!

mvrhov commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dunglas commented Sep 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanpoulain commented Sep 11, 2017 •

edited

Loading

alanpoulain Sep 12, 2017 •

edited

Loading

dunglas commented Sep 11, 2017 •

edited

Loading

mvrhov commented Sep 13, 2017 •

edited

Loading

dunglas Sep 13, 2017 •

edited

Loading