Skip to content
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

Add new GraphQL query for single product retrieval #28316

Open
wants to merge 7 commits into
base: 2.4-develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\CatalogGraphQl\Model\Resolver\Product\DataProvider;

use Magento\Catalog\Api\Data\ProductInterface;
use Magento\Catalog\Model\Product;
use Magento\Catalog\Model\ProductFactory;
use Magento\Catalog\Model\ResourceModel\Product as ProductResourceModel;

/**
* Product data provider, used for GraphQL resolver processing.
*/
class ProductDataProvider implements ProductDataProviderInterface
{
/**
* @var ProductResourceModel
*/
private $productResourceModel;

/**
* @var ProductFactory
*/
private $productFactory;

/**
* ProductDataProvider constructor.
*
* @param ProductResourceModel $productResourceModel
* @param ProductFactory $productFactory
*/
public function __construct(
ProductResourceModel $productResourceModel,
ProductFactory $productFactory
) {
$this->productResourceModel = $productResourceModel;
$this->productFactory = $productFactory;
}

/**
* Get product data by ID with full data set
*
* @param int $productId
* @param array $attributeCodes
* @return ProductInterface|Product
*/
public function getProductById(int $productId, array $attributeCodes): ProductInterface
{
/** @var Product $product */
$product = $this->productFactory->create();
$this->productResourceModel->load($product, $productId, $attributeCodes);

return $product;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\CatalogGraphQl\Model\Resolver\Product\DataProvider;

use Magento\Catalog\Api\Data\ProductInterface;

/**
* Provides product data by product ID.
*/
interface ProductDataProviderInterface
{
/**
* Retrieve product by product ID.
*
* @param int $productId
* @param array $attributeCodes
* @return ProductInterface
*/
public function getProductById(int $productId, array $attributeCodes): ProductInterface;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\CatalogGraphQl\Model\Resolver\Product;

use Magento\Catalog\Model\Product;
use Magento\Framework\GraphQl\Query\Resolver\IdentityInterface;

/**
* Identity for resolved product by ID
*/
class ProductIdIdentity implements IdentityInterface
{
/**
* @var string
*/
private $cacheTag = Product::CACHE_TAG;

/**
* Get product id for cache tag
*
* @param array $resolvedData
* @return array
*/
public function getIdentities(array $resolvedData): array
{
return empty($resolvedData['id']) ?
[] : [$this->cacheTag, $resolvedData['id']];
}
}
72 changes: 72 additions & 0 deletions app/code/Magento/CatalogGraphQl/Model/Resolver/ProductId.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\CatalogGraphQl\Model\Resolver;

use Magento\CatalogGraphQl\Model\Resolver\Product\DataProvider\ProductDataProviderInterface;
use Magento\CatalogGraphQl\Model\Resolver\Product\ProductFieldsSelector;
use Magento\Framework\GraphQl\Config\Element\Field;
use Magento\Framework\GraphQl\Exception\GraphQlInputException;
use Magento\Framework\GraphQl\Query\ResolverInterface;
use Magento\Framework\GraphQl\Schema\Type\ResolveInfo;

/**
* Product by ID resolver, used for GraphQL request processing.
Copy link

Choose a reason for hiding this comment

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

Note: Filename/Classname/Code comment all still reference the old behavior in this PR (before we switched to multiple IDs as input)

*/
class ProductId implements ResolverInterface
{
/**
* @var ProductDataProviderInterface
*/
private $productDataProvider;

/**
* @var ProductFieldsSelector
*/
private $productFieldsSelector;

/**
* ProductId constructor.
*
* @param ProductDataProviderInterface $productDataProvider
* @param ProductFieldsSelector $productFieldsSelector
*/
public function __construct(
ProductDataProviderInterface $productDataProvider,
ProductFieldsSelector $productFieldsSelector
) {
$this->productDataProvider = $productDataProvider;
$this->productFieldsSelector = $productFieldsSelector;
}

/**
* @inheritdoc
*/
public function resolve(
Field $field,
$context,
ResolveInfo $info,
array $value = null,
array $args = null
) {
$productId = (int) ($args['id'] ?? 0);
$fields = $this->productFieldsSelector->getProductFieldsFromInfo($info);

if (!$productId) {
throw new GraphQlInputException(
__("'id' input argument is required.")
Copy link

Choose a reason for hiding this comment

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

Assuming this validation code can go away once the input arg is required via https://github.com/magento/magento2/pull/28316/files#r438190331

);
}

/** @var \Magento\Catalog\Model\Product $product */
$product = $this->productDataProvider->getProductById($productId, $fields);
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried the following scenario?
Pass "-1" as the product ID in the GraphQL query 😎

Copy link
Author

Choose a reason for hiding this comment

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

I have not tried this scenario before.
I've just added the additional validation rule for throwing the exception if the ID argument is negative.
Screenshot 2020-06-09 at 16 15 14
Thank you!

$data = $product->getData();
$data['model'] = $product;

return $data;
}
}
1 change: 1 addition & 0 deletions app/code/Magento/CatalogGraphQl/etc/di.xml
Original file line number Diff line number Diff line change
Expand Up @@ -74,4 +74,5 @@
<preference type="\Magento\CatalogGraphQl\Model\Resolver\Product\Price\Provider" for="\Magento\CatalogGraphQl\Model\Resolver\Product\Price\ProviderInterface"/>

<preference type="Magento\CatalogGraphQl\Model\Resolver\Products\Query\Search" for="Magento\CatalogGraphQl\Model\Resolver\Products\Query\ProductQueryInterface"/>
<preference type="Magento\CatalogGraphQl\Model\Resolver\Product\DataProvider\ProductDataProvider" for="Magento\CatalogGraphQl\Model\Resolver\Product\DataProvider\ProductDataProviderInterface"/>
</config>
4 changes: 4 additions & 0 deletions app/code/Magento/CatalogGraphQl/etc/schema.graphqls
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ type Query {
sort: ProductAttributeSortInput @doc(description: "Specifies which attributes to sort on, and whether to return the results in ascending or descending order.")
): Products
@resolver(class: "Magento\\CatalogGraphQl\\Model\\Resolver\\Products") @doc(description: "The products query searches for products that match the criteria specified in the search and filter attributes.") @cache(cacheIdentity: "Magento\\CatalogGraphQl\\Model\\Resolver\\Product\\Identity")
product (
Copy link
Contributor

Choose a reason for hiding this comment

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

why this endpoint does not support multiple IDs?

Copy link

@DrewML DrewML Jun 15, 2020

Choose a reason for hiding this comment

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

Seconding this - input should be a non-nullable list type of IDs.

Fun fact: GraphQL spec allows clients to pass a single value when the input is a list type, and it will coerce to a list length of 1 with that value:

image

In other words, a list type will work fine for both use-cases, with some nice syntax sugar from GraphQL.

Copy link

Choose a reason for hiding this comment

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

One other note: if we accept a list type as an input, we'll need a different name for this field that isn't singular.

Copy link

Choose a reason for hiding this comment

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

I'm not sure that Query.product really makes it clear now that it returns multiple products.

How about something like Query.productsByID?

id: Int @doc(description: "Id of the product.")
Copy link
Contributor

Choose a reason for hiding this comment

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

The argument should be required.

Also, @kokoc what if we change the type to ID, it seems to correspond our strategy. But may cause the need of type casting in strictly typed languages.

Suggested change
id: Int @doc(description: "Id of the product.")
id: ID! @doc(description: "Id of the product.")

Copy link

Choose a reason for hiding this comment

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

Optional, but don't really need this comment - the schema here is plenty declarative. If it's not, we can call the field productID

): ProductInterface
@resolver(class: "Magento\\CatalogGraphQl\\Model\\Resolver\\ProductId") @doc(description: "The product query searches for product that match the specified product id.") @cache(cacheIdentity: "Magento\\CatalogGraphQl\\Model\\Resolver\\Product\\ProductIdIdentity")
Copy link

Choose a reason for hiding this comment

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

Now that we're splitting product fetch and search into separate queries, we may want to avoid the word "search" in this description for the sake of clarity.

category (
id: Int @doc(description: "Id of the category.")
): CategoryTree
Expand Down