-
-
Notifications
You must be signed in to change notification settings - Fork 921
[GraphQL] Manage custom queries by using the internal resolvers #2655
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GraphQL] Manage custom queries by using the internal resolvers #2655
Conversation
Makes sense 👍 |
c17c3a3
to
bde68be
Compare
return $source[$info->fieldName]; | ||
} | ||
|
||
if (!isset($args['id'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this check required only for bundled resolver. Initially, one of the reasons of introduction of custom operations was an idea to create endpoints/resources without compulsory arguments. Probably iri
should be grabbed from custom operation configuration in case of unavailability of id
.
Otherwise, this PR seems useless, IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I think moving normalization to the factory makes sense, but the retrieval of items should be handled by the resolvers.
iri
should not be needed either, and the $item
parameter should be removed from QueryItemResolverInterface
. Maybe we can pass the iriConverter
and baseNormalizationContext
, or some callable preconfigured with those, to the resolver - so you can actually load items inside of it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there will be three ways to add custom operations in the future:
- custom queries (this one).
id
arg is mandatory, the custom resolver receives the item/collection, it does some things, it returns it. - custom mutations: an
input
is mandatory, the custom resolver receives the denormalized item, it does some things, it returns it and it will be persisted afterwards. - custom operations: nothing is mandatory for the args, the resolver does anything it wants, nothing is done if it returns something (it needs to do the normalization itself). I'm not sure about this one. And IMO it can be strange to define it on a resource since you can do anything you want.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My example (and currently implemented use-case) for custom queries was a "currentUser" query (returns logged in user based on token storage). Having an id
arg does not really make sense here, but using the resource normalization does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I will do the modifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! 🙂🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the use-case for such custom queries (having id
) then? It could be done by specifying @ApiResource({grapqhl={"query"}})
. My comment mentioned in the starting message of PR is about resources free from id
. It's more common case for GraphQl than endpoints having id
but not required collection operations (actually, this could be easily done by modifying SchemaBuilder
), IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Custom queries having an id
argument could be useful to modify the received item or to add side effects to your query.
WIth what I've done, you have an id
argument by default and you receive the item by default but you can easily change this behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe having the optional id
argument should be fine for now 👍 If you don't need / use it, just ignore the item argument inside your resolver.
More customization and configuration can and will probably be added in the future 🙂 (I'm using the GraphQL API heavily in a current project)
bde68be
to
99fb293
Compare
99fb293
to
eb32fd9
Compare
Should be good now 🙂 |
Looks good to me 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
in the QueryCollectionResolverInterface wouldn't it be better to have a default value for the array $context
?
Hi @alanpoulain , @lukasluecke , correct me if I'm wrong please. This PR brakes custom operations at all. |
Hi @lukasluecke , your patch is aimed for SchemaBuilder while I'm talking about |
There is a working Behat test for this: core/features/graphql/query.feature Lines 287 to 308 in bc0806b
As you can see there is no id involved in this query.Make sure the args are set like this in the entity: core/tests/Fixtures/TestBundle/Entity/DummyCustomQuery.php Lines 28 to 31 in bc0806b
This will be documented once I've done a required PR (possibility to manage types in args). |
Hi @alanpoulain , my apogolies, looks like |
Don't know why tests are not crashed but I found these changes breaking process of normalizing a result of id-less resolvers. Will do some more ingestigation later if nobody has an idea why iri is compulsory for now (and why tests are not broken). |
Heh. I found why tests are not crashed — because of ORM nature of resources. Resources not mapped to ORM Entites/Documents crash APIP with message of inability to generate IRI. |
Following @SCIF comment: #2445 (comment)
The custom queries are now always using the internal resolvers in order to avoid security issues and to let API Platform handle the normalization, pagination, etc.
@lukasluecke WDYT of it?