Skip to content

[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

Merged
merged 1 commit into from
Mar 28, 2019

Conversation

alanpoulain
Copy link
Member

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

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?

@lukasluecke
Copy link
Contributor

Makes sense 👍

@alanpoulain alanpoulain force-pushed the graphql-custom-queries branch 2 times, most recently from c17c3a3 to bde68be Compare March 26, 2019 19:01
return $source[$info->fieldName];
}

if (!isset($args['id'])) {
Copy link

@SCIF SCIF Mar 27, 2019

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.

Copy link
Contributor

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?

Copy link
Member Author

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?

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! 🙂🚀

Copy link

@SCIF SCIF Mar 27, 2019

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.

Copy link
Member Author

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.

Copy link
Contributor

@lukasluecke lukasluecke Mar 28, 2019

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)

@alanpoulain alanpoulain force-pushed the graphql-custom-queries branch from bde68be to 99fb293 Compare March 27, 2019 13:04
@alanpoulain alanpoulain force-pushed the graphql-custom-queries branch from 99fb293 to eb32fd9 Compare March 27, 2019 13:38
@alanpoulain
Copy link
Member Author

Should be good now 🙂

@lukasluecke
Copy link
Contributor

Looks good to me 🙂

Copy link
Member

@soyuka soyuka left a 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?

@alanpoulain alanpoulain merged commit bcffdca into api-platform:master Mar 28, 2019
@SCIF
Copy link

SCIF commented Apr 19, 2019

Hi @alanpoulain , @lukasluecke , correct me if I'm wrong please. This PR brakes custom operations at all.
Does anybody use custom operations using latest master? It's just broken :( Specifying of valid id is required, otherwise IriConverter::getItemFromIri() throws NOT ItemNotFoundException, but InvalidArgumentException.
I really disappointed since invested a lot of time into dropping my own Schema and Resolver and investigating how to use out of box ones. And they are not usable atm :(

@lukasluecke
Copy link
Contributor

@SCIF Take a look at #2738 for a quick patch, which I‘m using for my custom mutations. Didn‘t get around to adding tests yet, so it will probably get merged next week - but it work‘s fine for me in production.

@SCIF
Copy link

SCIF commented Apr 19, 2019

Hi @lukasluecke , your patch is aimed for SchemaBuilder while I'm talking about ItemResolverFactory bug.

@alanpoulain
Copy link
Member Author

There is a working Behat test for this:

Scenario: Custom not retrieved item query
When I send the following GraphQL request:
"""
{
testNotRetrievedItemDummyCustomQuery {
message
}
}
"""
Then the response status code should be 200
And the response should be in JSON
And the header "Content-Type" should be equal to "application/json"
And the JSON should be equal to:
"""
{
"data": {
"testNotRetrievedItemDummyCustomQuery": {
"message": "Success (not retrieved)!"
}
}
}
"""

As you can see there is no id involved in this query.
Make sure the args are set like this in the entity:
* "testNotRetrievedItem"={
* "item_query"="app.graphql.query_resolver.dummy_custom_not_retrieved_item",
* "args"={}
* },

This will be documented once I've done a required PR (possibility to manage types in args).

@SCIF
Copy link

SCIF commented Apr 19, 2019

Hi @alanpoulain , my apogolies, looks like "args"={} is required and I missed that.

@SCIF
Copy link

SCIF commented Apr 20, 2019

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).
@teohhanhui , any clue?

@SCIF
Copy link

SCIF commented May 19, 2019

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.

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.

4 participants