From 5a4c6b4cf066c5e532eaa4f110abbbc98b2fac23 Mon Sep 17 00:00:00 2001 From: Oleksandr Gorkun Date: Tue, 24 Sep 2019 20:08:49 -0500 Subject: [PATCH 1/7] Batch GraphQL resolver interface --- design-documents/graph-ql/batch-resolver.md | 117 ++++++++++++++++++++ 1 file changed, 117 insertions(+) create mode 100644 design-documents/graph-ql/batch-resolver.md diff --git a/design-documents/graph-ql/batch-resolver.md b/design-documents/graph-ql/batch-resolver.md new file mode 100644 index 000000000..ac46fc29a --- /dev/null +++ b/design-documents/graph-ql/batch-resolver.md @@ -0,0 +1,117 @@ +# Introduce new BatchResolverInterface as default resolver interface +## Problem +Current resolver interface is used to resolve one field at a time, +enabling batching by returning a Deferred result and GraphQL engine delaying it's execution for as long as possible. +But internal and 3rd party developers will not always understand this optimization mechanism or remember it's existence +or want to bother with callbacks. By introducing a new resolver interface which clearly conveys that fields can +be grouped before resolving takes place will make batch field resolving easier and more clear for developers. + +## Proposed SPI +* BatchResolverInterface + + The new resolver interface for developers to implement. _Resolve_ method receives context and field DTO (they would + the same for every value,args pair), value and args pairs requested. Value and arguments pairs would be gathered from + all _field_ occurrences that need resolving until the very last moment when actual results needed to fill + leaves/branches. Developers will have the ability to group requested entity IDs/search parameters/entity properties + requested from different branches and retrieve them all at once. + + The upside of this interface is that developers wouldn't have to bother with callbacks to perform batch resolving + and have a list of values,args pairs clearly lined for them to process. + + As you can see this interface does not extend existing resolver interface nor will BatchResolverInterface + implementations will be required to extend any classes. Gathering values,args pairs and providing deferred results + to the GraphQL engine will be handled by the framework itself. +```php +namespace Magento\Framework\GraphQl\Query\Resolver; + +use Magento\Framework\GraphQl\Config\Element\Field; + +interface BatchResolverInterface +{ + /** + * Resolve a field for multiple values and arguments requested. + * + * @param ContextInterface $context + * @param Field $field + * @param BatchRequestItem[] $requests + * @return BatchResponse + * @throws \Throwable + */ + public function resolve(ContextInterface $context, Field $field, array $requests): BatchResponse; +} +``` +* BatchRequestItem + + DTO. Info, value, args representing request data from trying to resolve a single branch/leaf +```php +namespace Magento\Framework\GraphQl\Query\Resolver; + +use Magento\Framework\GraphQl\Config\Element\Field; +use Magento\Framework\GraphQl\Schema\Type\ResolveInfo; + +class BatchRequestItem +{ + /** + * @return ResolveInfo + */ + public function getInfo(): ResolveInfo; + + /** + * @return array|null + */ + public function getValue(): ?array; + + /** + * @return array|null + */ + public function getArgs(): ?array; +} +``` +* BatchResponse + + DTO. Result of batch resolving batches/leaves. Resolved data is grouped by values,args pairs. +```php +namespace Magento\Framework\GraphQl\Query\Resolver; + +class BatchResponse +{ + /** + * Set resolved data for a request. + * + * @param BatchRequestItem $request + * @param array|int|string|float|Value $response + */ + public function addResponse(BatchRequestItem $request, $response): void; + + /** + * Retreive resolved data based on the request. + * + * @param BatchRequestItem $item + * @return mixed|Value + */ + public function findResponseFor(BatchRequestItem $item); +} +``` + +## How to introduce this interface +I propose to deprecate existing single-field ResolverInterface and treat the new interface as the default for Query +resolvers. Right now Query and Mutation resolvers are not differentiated but that can be changed - since Mutation +resolvers will never have _$values_ we could introduce new interface for Mutations and have BatchResolverInterface +dedicated to Queries. Hopefully this move will steer developer into the performance considering approach to resolvers. + +## POC +I have created POC by replacing Related/CrossSell/UpSell resolvers by batch resolvers and testing them against +multilevel complex query to _products_. + +POC can be found [here](https://github.com/AlexMaxHorkun/magento2/tree/batch-graphql-proto) + +Points of interest: +* [Performance testing information](https://github.com/AlexMaxHorkun/magento2/blob/batch-graphql-proto/batch-graphql-proto.md) +* [BatchRequestInterface](https://github.com/AlexMaxHorkun/magento2/blob/batch-graphql-proto/lib/internal/Magento/Framework/GraphQl/Query/Resolver/BatchResolverInterface.php) +* [UpSell/Related/CrossSell resolver](https://github.com/AlexMaxHorkun/magento2/blob/batch-graphql-proto/app/code/Magento/RelatedProductGraphQl/Model/Resolver/Batch/AbstractLikedProducts.php) + +Performance test results: + +With a complex recursive query described in the testing information batch resolver is invoked 2 times vs 20 times +for existing resolvers. The query is being resolved 200ms faster with batch resolver than with existing resolver +with Magento being in _Developer_ mode. From 31f7a49ee26c2acf5ee0ddc53e9c68b170e09a36 Mon Sep 17 00:00:00 2001 From: Oleksandr Gorkun Date: Wed, 23 Oct 2019 13:59:38 -0500 Subject: [PATCH 2/7] batch graphql resolvers --- design-documents/graph-ql/batch-resolver.md | 100 +++++++++++++++++++- 1 file changed, 96 insertions(+), 4 deletions(-) diff --git a/design-documents/graph-ql/batch-resolver.md b/design-documents/graph-ql/batch-resolver.md index ac46fc29a..a51f4deb6 100644 --- a/design-documents/graph-ql/batch-resolver.md +++ b/design-documents/graph-ql/batch-resolver.md @@ -93,11 +93,103 @@ class BatchResponse } ``` + +## Proposed SPI for resolvers utilizing new batch service contracts +* BatchContractResolverInterface + + This interface is for graphql resolvers that delegate batch query/operation to a + [batch service contract](../batch-query-services.md). Simple interface allows developers to gather criteria/argument items + from GraphQL requests and convert service contract result to GraphQL acceptable format. + + Only service contracts following batch services specification can be used with these resolvers since the mechanism + relies on results being returned in the same exact order as requests from a service contract. + +```php +/** + * Resolve multiple brunches/leaves by executing a batch service contract. + */ +interface BatchContractResolverInterface +{ + /** + * Service contract to use, 1st element - class, 2nd - method. + * + * @return array + */ + public function getServiceContract(): array; + + /** + * Convert GraphQL arguments into a batch service contract argument item. + * + * @param ResolveRequestInterface $request + * @return object + * @throws GraphQlInputException + */ + public function convertToArgument(ResolveRequestInterface $request); + + /** + * Convert service contract result item into resolved brunch/leaf. + * + * @param object $result Result item returned from service contract. + * @param ResolveRequestInterface $request Initial GraphQL request. + * @return mixed|Value Resolved GraphQL response. + * @throws GraphQlInputException + */ + public function prepareResponse($result, ResolveRequestInterface $request); +} +``` + +* ResolveRequestInterface + + Interface containing all GraphQL request data (for a single branch/leaf) + +```php +/** + * Request for a resolver. + */ +interface ResolveRequestInterface +{ + /** + * Field metadata. + * + * @return Field + */ + public function getField(): Field; + + /** + * GraphQL context. + * + * @return ContextInterface + */ + public function getContext(): ContextInterface; + + /** + * Information associated with the request. + * + * @return ResolveInfo + */ + public function getInfo(): ResolveInfo; + + /** + * Value passed from upper resolvers. + * + * @return array|null + */ + public function getValue(): ?array; + + /** + * Arguments from GraphQL request. + * + * @return array|null + */ + public function getArgs(): ?array; +} +``` + ## How to introduce this interface -I propose to deprecate existing single-field ResolverInterface and treat the new interface as the default for Query -resolvers. Right now Query and Mutation resolvers are not differentiated but that can be changed - since Mutation -resolvers will never have _$values_ we could introduce new interface for Mutations and have BatchResolverInterface -dedicated to Queries. Hopefully this move will steer developer into the performance considering approach to resolvers. +Existing `ResolverInterface` is still good for mutations so it cannot be deprecated in favour of these new interfaces. +But we have to nudge developers into writing more performance considerate resolvers by utilizing batch resolver interfaces +so we will create a static test that will propose to use either `BatchResolverInterface` or `BatchContractResolverInterface` +instead of regular `ResolverInterface`. ## POC I have created POC by replacing Related/CrossSell/UpSell resolvers by batch resolvers and testing them against From 8fc0e63d15b45e30c219c94e4f9a76ededd90e6b Mon Sep 17 00:00:00 2001 From: Oleksandr Gorkun Date: Wed, 23 Oct 2019 14:15:06 -0500 Subject: [PATCH 3/7] batch graphql resolvers --- design-documents/graph-ql/batch-resolver.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/design-documents/graph-ql/batch-resolver.md b/design-documents/graph-ql/batch-resolver.md index a51f4deb6..24735a0e4 100644 --- a/design-documents/graph-ql/batch-resolver.md +++ b/design-documents/graph-ql/batch-resolver.md @@ -1,4 +1,4 @@ -# Introduce new BatchResolverInterface as default resolver interface +# Introduce new BatchResolverInterface ## Problem Current resolver interface is used to resolve one field at a time, enabling batching by returning a Deferred result and GraphQL engine delaying it's execution for as long as possible. From e271eeb7cb91b8efc288401173ae653c953bb9bd Mon Sep 17 00:00:00 2001 From: Alexander Gorkun Date: Thu, 24 Oct 2019 13:19:33 -0500 Subject: [PATCH 4/7] Update design-documents/graph-ql/batch-resolver.md Co-Authored-By: Alex Paliarush --- design-documents/graph-ql/batch-resolver.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/design-documents/graph-ql/batch-resolver.md b/design-documents/graph-ql/batch-resolver.md index 24735a0e4..6841c5735 100644 --- a/design-documents/graph-ql/batch-resolver.md +++ b/design-documents/graph-ql/batch-resolver.md @@ -10,7 +10,7 @@ be grouped before resolving takes place will make batch field resolving easier a * BatchResolverInterface The new resolver interface for developers to implement. _Resolve_ method receives context and field DTO (they would - the same for every value,args pair), value and args pairs requested. Value and arguments pairs would be gathered from + be the same for every value,args pair), value and args pairs requested. Value and arguments pairs would be gathered from all _field_ occurrences that need resolving until the very last moment when actual results needed to fill leaves/branches. Developers will have the ability to group requested entity IDs/search parameters/entity properties requested from different branches and retrieve them all at once. From 2d7e39734f3eb80b44570f044a1d57b36591bf41 Mon Sep 17 00:00:00 2001 From: Oleksandr Gorkun Date: Thu, 24 Oct 2019 13:25:49 -0500 Subject: [PATCH 5/7] updates to graphql batch resolvers document --- design-documents/graph-ql/batch-resolver.md | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/design-documents/graph-ql/batch-resolver.md b/design-documents/graph-ql/batch-resolver.md index 24735a0e4..4ce2c0056 100644 --- a/design-documents/graph-ql/batch-resolver.md +++ b/design-documents/graph-ql/batch-resolver.md @@ -52,16 +52,22 @@ use Magento\Framework\GraphQl\Schema\Type\ResolveInfo; class BatchRequestItem { /** + * Information associated with a single GraphQL request. + * * @return ResolveInfo */ public function getInfo(): ResolveInfo; /** + * Data passed from upper resolvers. + * * @return array|null */ public function getValue(): ?array; /** + * Arguments from a single GraphQL request. + * * @return array|null */ public function getArgs(): ?array; @@ -95,7 +101,7 @@ class BatchResponse ## Proposed SPI for resolvers utilizing new batch service contracts -* BatchContractResolverInterface +* BatchServiceContractResolverInterface This interface is for graphql resolvers that delegate batch query/operation to a [batch service contract](../batch-query-services.md). Simple interface allows developers to gather criteria/argument items @@ -108,7 +114,7 @@ class BatchResponse /** * Resolve multiple brunches/leaves by executing a batch service contract. */ -interface BatchContractResolverInterface +interface BatchServiceContractResolverInterface { /** * Service contract to use, 1st element - class, 2nd - method. @@ -124,7 +130,7 @@ interface BatchContractResolverInterface * @return object * @throws GraphQlInputException */ - public function convertToArgument(ResolveRequestInterface $request); + public function prepareArgument(ResolveRequestInterface $request); /** * Convert service contract result item into resolved brunch/leaf. @@ -188,7 +194,7 @@ interface ResolveRequestInterface ## How to introduce this interface Existing `ResolverInterface` is still good for mutations so it cannot be deprecated in favour of these new interfaces. But we have to nudge developers into writing more performance considerate resolvers by utilizing batch resolver interfaces -so we will create a static test that will propose to use either `BatchResolverInterface` or `BatchContractResolverInterface` +so we will create a static test that will propose to use either `BatchResolverInterface` or `BatchServiceContractResolverInterface` instead of regular `ResolverInterface`. ## POC From b3e57d3f98e075604e262269215edea97b269dbf Mon Sep 17 00:00:00 2001 From: Oleksandr Gorkun Date: Thu, 24 Oct 2019 14:22:12 -0500 Subject: [PATCH 6/7] graphql batch resolvers updates --- design-documents/graph-ql/batch-resolver.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/design-documents/graph-ql/batch-resolver.md b/design-documents/graph-ql/batch-resolver.md index 7b964033e..7a642b3e8 100644 --- a/design-documents/graph-ql/batch-resolver.md +++ b/design-documents/graph-ql/batch-resolver.md @@ -130,7 +130,7 @@ interface BatchServiceContractResolverInterface * @return object * @throws GraphQlInputException */ - public function prepareArgument(ResolveRequestInterface $request); + public function convertToServiceArgument(ResolveRequestInterface $request); /** * Convert service contract result item into resolved brunch/leaf. @@ -140,7 +140,7 @@ interface BatchServiceContractResolverInterface * @return mixed|Value Resolved GraphQL response. * @throws GraphQlInputException */ - public function prepareResponse($result, ResolveRequestInterface $request); + public function convertFromServiceResult($result, ResolveRequestInterface $request); } ``` From 263d2fcbfac0a434fd148b358add1e5e959e02c5 Mon Sep 17 00:00:00 2001 From: Oleksandr Gorkun Date: Thu, 24 Oct 2019 14:38:24 -0500 Subject: [PATCH 7/7] graphql batch resolver --- design-documents/graph-ql/batch-resolver.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/design-documents/graph-ql/batch-resolver.md b/design-documents/graph-ql/batch-resolver.md index 7a642b3e8..86b3e2a37 100644 --- a/design-documents/graph-ql/batch-resolver.md +++ b/design-documents/graph-ql/batch-resolver.md @@ -59,7 +59,7 @@ class BatchRequestItem public function getInfo(): ResolveInfo; /** - * Data passed from upper resolvers. + * Data passed from parent resolvers. * * @return array|null */ @@ -176,7 +176,7 @@ interface ResolveRequestInterface public function getInfo(): ResolveInfo; /** - * Value passed from upper resolvers. + * Value passed from parent resolvers. * * @return array|null */