-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
28563: GraphQL product search does not consider Category Permissions configuration - Pass the context as argument #28757
28563: GraphQL product search does not consider Category Permissions configuration - Pass the context as argument #28757
Conversation
Hi @pmarjan. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. |
@magento run all tests |
* @return SearchResultsInterface | ||
*/ | ||
public function getList( | ||
SearchCriteriaInterface $searchCriteria, | ||
SearchResultInterface $searchResult, | ||
array $attributes = [] | ||
array $attributes = [], | ||
ContextInterface $context |
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.
We have to make this optional, or put it before attributes, because attributes is optional.
\Magento\CatalogGraphQl\Model\Resolver\Products\DataProvider\Product\CollectionProcessorInterface::process has attributes mandatory and we also have to make it optional
...phQl/Model/Resolver/Products/DataProvider/Product/CollectionProcessor/AttributeProcessor.php
Show resolved
Hide resolved
*/ | ||
public function process( | ||
Collection $collection, | ||
SearchCriteriaInterface $searchCriteria, | ||
array $attributeNames | ||
array $attributeNames, | ||
ContextInterface $context |
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 it's best to do here attributes and context optional, because not all implementations use it
also code becomes very complicated to pass all this around then not use it
Plus adding suppress unused params later..it all points optional is better
@cpartica context parameter is now optional, as per your request. the PR is updated, please let me know if further adjustments are needed. |
@magento run all tests |
@magento run all tests |
@@ -11,7 +11,8 @@ | |||
"magento/module-store": "*", | |||
"magento/module-eav-graph-ql": "*", | |||
"magento/module-catalog-search": "*", | |||
"magento/framework": "*" | |||
"magento/framework": "*", | |||
"magento/module-graph-ql": "*" | |||
}, | |||
"suggest": { | |||
"magento/module-graph-ql": "*", |
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.
we can remove graphql from suggest
|
||
if ($this->isCustomer($currentUserId, $currentUserType)) { | ||
try { | ||
$customerGroupId = $this->customerRepository->getById($currentUserId)->getGroupId(); |
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.
is this request cached when the previous Context sets the user id?
it is weird how we do do this check for group but not for the currentUserId
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 would say no (although not entirely sure). As far as I can tell, if FPC does not return a result (Magento\PageCache\Model\App\FrontController\BuiltinPlugin::aroundDispatch, line 73) processing of the authorization token will always flow in the same way. Otherwise, it would not run at all.
Therefore, line 62 here if ($this->isCustomer...) is probably trying to avoid unnecessary calls to customerRepository->getById(). Reasoning is there may be many requests that are not bearing any tokens, or are bearing invalid ones, and for them we do not want to allocate resources to load entity that we know for sure will not be loaded.
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 still think this is problematic, and it will probably trigger a significant degradation in query time
it should be called somewhere else when used like getExtensionAttribute->getCustomerGroupId
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.
@cpartica the check for the customer group is now removed.
* @param FieldSelection $fieldSelection | ||
* @param SearchCriteriaBuilder $searchCriteriaBuilder | ||
* @param ScopeConfigInterface $scopeConfig | ||
*/ | ||
public function __construct( | ||
SearchResultFactory $searchResultFactory, | ||
ProductProvider $productDataProvider, | ||
LayerResolver $layerResolver, |
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.
@cpartica, @danielrenaud
when scanning with PHPMD for design issues, I got
The class Filter has a coupling between objects value of 13. Consider to reduce the number of dependencies under 13.
To solve for that, LayerResolver is removed, since anyway is not used anymore in this class.
But, is this a backward incompatible change? If so, please advice, what might be a good way to proceed?
As a background, the MD test fails after adding the use of the ContextInterface, so the $context
to the getResult()
can be properly passed with specifying its type
@param ContextInterface $context
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.
If the dependency is not used at all, it should be ok to remove it. In GraphQl area, we only treat schema as our public api, so we can make changes like this to code that is not marked @api
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.
It's not so bad to remove it actually
Do this change and we'll get it approved. Since this implements an interface, the class was not meant to be extended anyway.
* @param FieldSelection $fieldSelection | ||
* @param SearchCriteriaBuilder $searchCriteriaBuilder | ||
* @param ScopeConfigInterface $scopeConfig | ||
*/ | ||
public function __construct( | ||
SearchResultFactory $searchResultFactory, | ||
ProductProvider $productDataProvider, | ||
LayerResolver $layerResolver, |
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.
It's not so bad to remove it actually
Do this change and we'll get it approved. Since this implements an interface, the class was not meant to be extended anyway.
… Permissions configuration - Pass the context as argument #28757
Hi @pmarjan, thank you for your contribution! |
Description (*)
Fixes #28563
Consider catalog permissions when searchng for products - approach by passing context as parameter
Related Pull Requests
https://github.com/magento/partners-magento2ee/pull/271
https://github.com/magento/partners-magento2-infrastructure/pull/5
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)