Skip to content

Comments

[GraphQL] Add custom queries#2445

Merged
alanpoulain merged 12 commits intoapi-platform:masterfrom
whatwedo:custom-graphql
Mar 24, 2019
Merged

[GraphQL] Add custom queries#2445
alanpoulain merged 12 commits intoapi-platform:masterfrom
whatwedo:custom-graphql

Conversation

@lukasluecke
Copy link
Contributor

@lukasluecke lukasluecke commented Jan 10, 2019

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR TODO

Possibly related: #1700 #473 #2309 #1870

Currently partially implements custom queries on existing resources, but the goal would be at least full custom query and mutation support on existing resources, possibly even for "new" resources, custom types etc. (maybe in another PR)

Usage example

User.php (Resource class)

/**
 * @ApiResource(graphql={
 *     "query",
 *     "delete",
 *     "update",
 *     "create",
 *     "current"={
 *          "item"=CurrentUserResolver::class,
 *          "description"="Get currently logged in user"
 *     },
 *     "active"={
 *          "collection"=ActiveUsersResolver::class,
 *          "description"="Get all active users"
 *     }
 * })
 */

CurrentUserResolver.php

class CurrentUserResolver implements QueryResolverInterface
{
    private $tokenStorage;
    private $normalizer;

    public function __construct(TokenStorageInterface $tokenStorage, NormalizerInterface $normalizer)
    {
        $this->tokenStorage = $tokenStorage;
        $this->normalizer = $normalizer;
    }

    public function __invoke($source, $args, $context, ResolveInfo $info)
    {
        $token = $this->tokenStorage->getToken();

        if (!$token) return null;

        return $this->normalizer->normalize($token->getUser(), ItemNormalizer::FORMAT);
    }
}

ActiveUsersResolver.php

class ActiveUsersResolver implements QueryResolverInterface
{
    private $repository;
    private $normalizer;

    public function __construct(UserRepository $repository, NormalizerInterface $normalizer)
    {
        $this->repository = $repository;
        $this->normalizer = $normalizer;
    }

    public function __invoke($source, $args, $context, ResolveInfo $info)
    {
        $collection = $this->repository->findActive();

        $offset = 0;
        $data = ['totalCount' => 0.0, 'edges' => [], 'pageInfo' => ['endCursor' => null, 'hasNextPage' => false]];

        foreach ($collection as $index => $object) {
            $data['edges'][$index] = [
                'node' => $this->normalizer->normalize($object, ItemNormalizer::FORMAT),
                'cursor' => base64_encode((string) ($index + $offset)),
            ];
        }

        return $data;
    }
}

@lukasluecke
Copy link
Contributor Author

lukasluecke commented Jan 10, 2019

@alanpoulain Any early input from you, while I continue working on this? (e.g. the configuration structure, scope of this feature, handling the pass-through to the default resolver, ..)

@alanpoulain
Copy link
Member

I will have a look this weekend. @raoulclais is working on the custom mutations, he will probably make a PR soon.

@lukasluecke
Copy link
Contributor Author

lukasluecke commented Jan 12, 2019

@alanpoulain Thank you

@alanpoulain
Copy link
Member

alanpoulain commented Jan 13, 2019

I think it should be better to have something like this:

/**
 * @ApiResource(graphql={
 *     "query",
 *     "delete",
 *     "update",
 *     "create",
 *     "myQuery"={
 *         "query"=MyQuery::class
 *     }
 * })
 */

Like the PR for the custom mutations: #2447
You should be able to identify them with the query key.

@lukasluecke lukasluecke changed the title WIP: [GraphQL] Add configuration for custom GraphQL queries on ApiResource WIP: Add custom queries Jan 14, 2019
@lukasluecke lukasluecke force-pushed the custom-graphql branch 10 times, most recently from a8201d5 to 86f5bce Compare January 14, 2019 14:24
@lukasluecke
Copy link
Contributor Author

@alanpoulain Thank you for your feedback. I have incorporated your suggestions and updated the resulting code sample in the PR description.

I'm not really sure about the configuration still. We need more than just a query field, as suggested by you. For now I have chosen item and collection fields, which serve both as the resolver class, and also as an indicator for what kind of query it is.

But if we go further, e.g custom input-types or result types for collections I'm not sure this will hold up.

@alanpoulain
Copy link
Member

IMO it should not be an issue in the future.
We would have something like this:

/**
 * @ApiResource(graphql={
 *     "query",
 *     "delete",
 *     "update",
 *     "create",
 *     "current"={
 *          "item"=CurrentUserResolver::class,
 *          "description"="Get currently logged in user",
 *          "input"={"type"=MyType::class, "description"="Custom description input", args=[]}
 *     },
 *     "active"={
 *          "collection"=ActiveUsersResolver::class,
 *          "description"="Get all active users"
 *     }
 * })
 */

WDYT?

I think we need to add the "query" term anyway. My suggestion: item_query and collection_query. Are you OK with this?

@lukasluecke
Copy link
Contributor Author

lukasluecke commented Feb 3, 2019 via email

@SCIF
Copy link

SCIF commented Feb 17, 2019

Guys, don't you think that custom operation controller/invoked class should return the type of resource/iterable collection of this resource objects like it's done for REST?

However, the resource term doesn't fit very good for graphql and it's more about operations and types. What do you think about idea to introduce ApiOperation annotation providing easy way to handle custom operations? Something like:

/**
 * @ApiOperation(iri="somecustomname", arguments={…)
 */
final class AddSubscription
{
    public function __constructor(…)
    {
        …
    }

    public function __invoke(…): Subscription
    {
        …
    } 
}

Returning type could easily detected by analysing __invoke() method.

Such way we can distinct operation and resources/library methods without any tries to make resources overloaded with some deep notations.

My apologies in case if I'm wrong. it's just a suggestion.

@lukasluecke
Copy link
Contributor Author

lukasluecke commented Feb 21, 2019

I will try to get some work done on tests soon, but the rest of the week will be quite busy for me, so not sure when I'll get to it. Thanks for your feedback so far! 🥇

Planned for next weekend (16.03.) right now 🙈

@SCIF
Copy link

SCIF commented Feb 25, 2019

Hi @lukasluecke , my message is not about resource/operation definition way but about DX. I tried to suggest a way simpler DX flow of implementation of custom operations. I think that normalization and composing an edge information should not have place in custom operation executor.

@lukasluecke
Copy link
Contributor Author

@alanpoulain I have added a simple behat test for item_query, do you think it's enough if I add a similar one for collection_query for now?

I am still not quite content with the configuration format (item_query, collection_query) and would prefer a cleaner approach, but I think that can be discussed later.

@SCIF
Copy link

SCIF commented Mar 23, 2019

Hi @lukasluecke ,

It would be nice to have tests of custom actoins with parameters.

@alanpoulain
Copy link
Member

@SCIF It is planned in another PR 🙂 This is a first step.

@alanpoulain alanpoulain force-pushed the custom-graphql branch 3 times, most recently from 918c86d to f1721e4 Compare March 24, 2019 19:52
@lukasluecke
Copy link
Contributor Author

Thank you very much @alanpoulain 🚀

@SCIF
Copy link

SCIF commented Mar 24, 2019

Hi @lukasluecke , @alanpoulain , are you going to implement custom mutations in separate PR?

@lukasluecke
Copy link
Contributor Author

There is still #2447 for custom mutations, but I‘ll probably work on mutations and more options in a new PR (if it‘s not merged by the time I get to it).

@SCIF
Copy link

SCIF commented Mar 24, 2019

@lukasluecke , @alanpoulain
But guys, what do you think on idea to extract Normalizing stage out of custom operations implementation? In REST it's out of custom operation action, which is very convenient. Moreover, it will bring mismatch of behavior between REST and GraphQL in case if you will leave current behaviour.

@alanpoulain
Copy link
Member

@lukasluecke I will finish the PR for custom mutations soon since the major part of the work has already been done.
I have also worked on a way to modify easily the type system.
@SCIF I don't understand what you mean, could you elaborate?

@alanpoulain alanpoulain merged commit d0bde7e into api-platform:master Mar 24, 2019
@alanpoulain
Copy link
Member

Great addition, thank you @lukasluecke!

@SCIF
Copy link

SCIF commented Mar 24, 2019

@alanpoulain
Current PR implements custom resolver as a way to execute/invoke custom operation. That means a developer MUST handle normalizing WITHIN every custom operation.

While framework's REST implementation handles data type/resource and the developer MUST NOT care on normalizing.

I've tried to express this in comment above.

@SCIF
Copy link

SCIF commented Mar 24, 2019

I'm sure, that ability even to implement normalizing stage could be useful, while these are really not common cases.

@alanpoulain
Copy link
Member

OK thank you for the explanation. It could have been done this way if we reused the existing resolvers. But it would have meant less flexibility. I will try to think about it.

@SCIF
Copy link

SCIF commented Mar 24, 2019

@alanpoulain ,
So, to be clear, it's much better to call this PR's feature as custom resolvers rather than custom operation to distinct different DX.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants