required attribute set id filter on attribute group repository getList

[tzyganu]

Preconditions

1. Magento 2.1 or 2.2 (probably reproducible on earlier versions)

Steps to reproduce

1. Try calling Magento\Eav\Model\Attribute\GroupRepository::getList with a \Magento\Framework\Api\SearchCriteriaInterface parameter that does not contain a filter for attribute_set_id

Expected result

1. I Should get as response the list of attribute groups based on my search criteria

Actual result

1. I get a Magento\Framework\Exception\NoSuchEntityException with the message No such entity with %fieldName = %fieldValue.

Additional info:

Full description and code can be found here https://magento.stackexchange.com/q/195740/146

The exception happens because the getList method of the GroupRepository requires a filter on attribute_set_id: https://github.com/magento/magento2/blob/2.2-develop/app/code/Magento/Eav/Model/Attribute/GroupRepository.php#L120-L128
I don't really see the need for this. The repository should let me retrieve the groups as I want to.

If I'm wrong, please explain the reason, if I'm right, just tell me and I'll come back with a PR.

[orlangur]

It reminded me #7847 a bit :)

You can see from getList and save methods of GroupRepository implementation that it is not intended to work without attribute set. Just because attribute group is a sub-entity of attribute set and if you want to manipulate attribute groups directly without relation to attribute sets, you would break encapsulation (it's like managing attribute values of multiple products without relation to products).

So until new persistence layer is implemented https://github.com/magento/magento2/blob/2.2-develop/lib/internal/Magento/Framework/EntityManager/EntityManager.php#L24 is your best option. You can also implement your custom GroupRepository removing all the attribute set checks but there is nothing to "fix" in core.

[tzyganu]

@orlangur I see where this is going but I don't really agree with your reasoning.
Having a 1 to many relation and managing the entities involved in the relation does not necessarily mean I'm breaking encapsulation.
Why isn't the same logic applied to the ProductRepository? I mean, Attribute Set is the parent entity for the products. Why isn't there a mandatory filter on attribute_set_id for the product repository getList method?
Why isn't there a mandatory filter on order_id for InvoiceRepository or better yet, a mandatory filter on the Invoice/ItemRepository::getList() for invoice_id.
I'm sure that "if you want to manipulate invoice items directly without relation invoices you would break encapsulation" (quote adapted from your reply),

I suggest reconsidering or asking for a second opinion.

[maghamed]

Hey @orlangur and @tzyganu , it's a pretty interesting question you raised here.

Here is what I think about this:

1. API must be self-descriptive. A client of the API should not look at the implementation details to understand how does API work. It's enough to see Interface declaration to get a clue how to use API. Current interface of the AttributeGroupRepositoryInterface does not imply any limitations on its usage. One can't see that all the manipulation with Attribute Groups should be done in the scope of the specified attribute set.
2. Attribute Group from Magento point of view is not a sub-entity of Attribute Set. Attribute Group exists in the scope of the Attribute set. Which is different to what you @orlangur described. For example, Magento Products also exist in the scope of some Website, but they don't belong to the Website Aggregate Root (in DDD terms). Magento interfaces proof my words because Magento provides independent Repositories (aka ways for manipulation) for both AttributeSet and AttributeGroup. If Magento considered that AttributeGroup is a sub-entity of Attribute Set - it would not provide an ability to manipulate Attribute Group independently out of Attribute Set.
3. If some entity supposed to be manipulated only in pre-defined scope - it should be clearly represented in its contract. For example, that's how we do that for MSI
   GetProductQuantityInStockInterface::execute(string $sku, int $stockId) where $stockId is a required argument which defines a scope for current operation.
4. Also, you can't consider retrieving of all Attribute Groups which are assigned to different Attribute Sets as a violation of encapsulation because you still can get the same result with several getList calls applied to a different attributeset_id filter.

Based on all the above, I consider that proposal of @tzyganu makes sense, thus I reopened the issue.

[orlangur]

Started to reply on that issue that day with intention to reopen, maybe will find that comment in some other tab.

ProductRepository or InvoiceRepository are irrelevant as invoices and products have perfect sense be themselves.

On the other hand AttributeGroup has no sense in core without AttributeSet. Just like, as far as I know, we don't have a repository to manipulate categories to products relations separately.

Attribute Group from Magento point of view is not a sub-entity of Attribute Set. Attribute Group exists in the scope of the Attribute set.

@maghamed, please provide an example in core where attribute groups are manipulated directly.

As I mentioned previously, existing implementation of GroupRepository cannot be changed as existing third party code may rely on such validations, another repository could be created for direct manipulations, but I don't see how it would be useful in core. Using resource models for such direct manipulations is the best option until we have a new persistence layer.

[tzyganu]

@orlangur You don't need an example from the core. The service contracts don't serve only the core. It should serve my needs also.
And here is what I need.
I have one requirement that all attribute groups called "This is my Group" should be renamed to "This is NOT my group" (names are fictional).
I can let the admin do that for all 54 attribute sets manually, but this will only solve it for the live env.
But the groups are generated via an install script so I thought about making it in the proper magento way and have an upgrade script for it in which I retrieve all the attribute groups with a certain name, change them and save them.
Guess how I'm doing it right now....
I get all the attribute sets, then loop through the results, pass the attribute set id as a filter to the group repository and make 54 queries when I could make 1 to retrieve the data I need.

I know I could use the updateAttributeGroup method from the EavSetup class, but that one takes a setId parameter also.

Does this make sense?

Also the GroupRepository can be changed in this case... all other third party implementations that depend on it will still work. Having a filter on attribute set it will still remain valid filter. The signature is not changed.

[orlangur]

That's absolutely clear from the SO question.

Please don't suggest calling getList for each attribute set (I have a lot of them so a foreach is not a nice solution).

As far as I understand main concern here is performance and "lack of joins" is a common problem for current service layer. If we start changing implementations that way, not considering usefulness for core, than every real-world application would request "I need to do it in only one query!!!".

I don't see why resource models are bad for the case described.

[tzyganu]

@orlangur resource models and collections would work, but I'm trying to build my code the proper magento way. If there is a repository I use it.

[orlangur]

If there is a repository I use it.

And now there is no ready-to-use repository. Does it mean that in every such situation repositories functionality should be expanded?

[tzyganu]

@orlangur I don't know about every repository, but in this case I think I should. it's an easy fix and the impact should not be big.

[maghamed]

As I mentioned previously, existing implementation of GroupRepository cannot be changed as existing third party code may rely on such validations

Current changes proposed by @tzyganu are backward compatible and could be done in the scope of existing Attribute Group Repository implementation.
There could not be a situation where we would break existing code.
All that we do is changing required filter parameter (attributeSetId) and making it optional.
Thus, currently, all the existing code pass sttributeSetId into the Search Criteria filter - and that would be supported after the changes proposed along with providing the new possibility of extracting AttributeGroups assigned to different attribute sets.

another repository could be created for direct manipulations

we don't recommend to introduce new APIs for replicating the functionality we already have in the code. Because that would duplicate extension/customization points for Extension developers

Based on the above, @tzyganu feel free to create Pull Request with proposed changes and we would process it.

@orlangur if there are other open questions I propose to discuss them in Slack and not to overwhelm this thread.