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 1 commit
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,58 @@
<?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\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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, maybe this part was agreed previously. But I would use return types hinting since we use strict_types declaration.

Copy link
Author

Choose a reason for hiding this comment

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

It wasn't agreed previously, I just missed the strict type here.
I've just added it.
Thanks!

{
/** @var \Magento\Catalog\Model\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);
}
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 criteria specified in the search and filter attributes.") @cache(cacheIdentity: "Magento\\CatalogGraphQl\\Model\\Resolver\\Product\\ProductIdIdentity")

Choose a reason for hiding this comment

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

"that match the criteria specified in the search and filter attributes" doesn't seem correct if the query only has an attribute to specify the id of the product.

"The product query searches for a product that matches the specified id." feels a bit more closer to what it should be, looking at the attribute.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your suggestion, the description has been adjusted.

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