Magento2: Repository Filter Groups Applied Inconsistently

[astorm]

Per conventional wisdom that's floating around the Magento programmer community, it's my understanding that, when creating object repositories and implementing a getList method that

1. Filters within a group should be applied as OR filters
2. Filter groups themselves should be applied as AND filter

However, this informal guideline is not followed consistently in the core Magento systems code. For example, the CMS page repository does not apply this AND/OR behavior, and applies each filter as an AND

#File: vendor/magento/module-cms/Model/PageRepository.php
foreach ($criteria->getFilterGroups() as $filterGroup) {
    foreach ($filterGroup->getFilters() as $filter) {
        if ($filter->getField() === 'store_id') {
            $collection->addStoreFilter($filter->getValue(), false);
            continue;
        }
        $condition = $filter->getConditionType() ?: 'eq';
        $collection->addFieldToFilter($filter->getField(), [$condition => $filter->getValue()]);
    }
}

Whereas the coupon repository does follow this guideline.

#File: vendor/magento/module-sales-rule/Model/CouponRepository.php

protected function addFilterGroupToCollection(
    \Magento\Framework\Api\Search\FilterGroup $filterGroup,
    Collection $collection
) {
    $fields = [];
    $conditions = [];
    foreach ($filterGroup->getFilters() as $filter) {
        $condition = $filter->getConditionType() ? $filter->getConditionType() : 'eq';
        $fields[] = $filter->getField();
        $conditions[] = [$condition => $filter->getValue()];
    }
    if ($fields) {
        $collection->addFieldToFilter($fields, $conditions);
    }
}

This makes it unclear how filters and filter groups should be applied.

Expected Behavior: Ideally, all repositories with a getList method that accepts search criteria should behave the same, and apply a "filters are OR", "filter groups are AND" logic. If you really wanted to knock this out of the park, a helper or trait that provided and ventral consistent place for filter group adding logic would be ideal. This would remove the burden from the repository creator as to how filters and filter groups should be applied.

If neither is possible, then the behavior of individual repositories should b documented.

[astorm]

Pinging because two weeks and no acknowledgment.

Is this issue not a priority for Magento 2 at this time? Consistent behavior of the filters in the Web API methods seems like it should be, but if it's not it would still be good to know so we can work around it.

If the issue isn't clear please let me know -- it's not the sort of thing that lends itself to easy, clear, and reproducible test steps, and as it's a core part of how Magento's API works it's not something that an outsider should really be making calls on.

/cc @piotrekkaminski

[Vinai]

Consist filtering behavior over the API (Web API and PHP API) is very important.

[pboisvert]

@mbrinton01 can you pass this along for investigation?

[piotrekkaminski]

internal issue MAGETWO-53262 to address that

[slavvka]

Hi all. The fix for CMS Pages and Blocks Repositories has been merged to mainline.

[astorm]

@slavvka This bug was reporting that filter groups are applied inconsistently across the entire repository system in Magento -- i.e. it's my assumption that filters and groups would behave the same regardless of where they're at. You've closed this issue reporting that the CMS Pages and Blocks have been "fixed". However, all the other repositories remain.

Is Magento saying that it's intended behavior that repositories act differently?

Or was this ticket incorrectly closed?

[piotrekkaminski]

Reopening for investigation @choukalos

[choukalos]

Should be Mark. @astorm - yes, we should have addressed all the services and made them behave consistently.

[slavvka]

Hi all. The bug for CMS repos has been fixed and now all the repos which use SearchCriteria works consistently. Though it is implemented individually in each repo. We have stories for making them use a generic logic to apply SearchCriteria and we are currently working on them. But I want to repeat that the only repos that worked inconsitently were fixed. Since that the problem doesn't exist anymore.

[astorm]

all the repos which use SearchCriteria works consistently

But I want to repeat that the only repos that worked inconsistently were fixed. Since that the problem doesn't exist anymore.

@slavvka @piotrekkaminski I'm not sure if I understand the above correctly -- but those don't appear to be true statements. While the CMS Page Repository in the develop branch does appear fixed

https://github.com/magento/magento2/blob/develop/app/code/Magento/Cms/Model/PageRepository.php#L228
private function addFilterGroupToCollection(
    \Magento\Framework\Api\Search\FilterGroup $filterGroup,
    \Magento\Cms\Model\ResourceModel\Page\Collection $collection
) {
    $fields = [];
    $conditions = [];
    foreach ($filterGroup->getFilters() as $filter) {
        if ($filter->getField() === 'store_id') {
            $collection->addStoreFilter($filter->getValue(), false);
            continue;
        }
        $condition = $filter->getConditionType() ?: 'eq';
        $fields[] = $filter->getField();
        $conditions[] = [$condition => $filter->getValue()];
    }
    if ($fields) {
        $collection->addFieldToFilter($fields, $conditions);
    }
}

This is far from "all the repos which use SearchCriteria". For example, if I look at the Payment Token Repository

protected function addFilterGroupToCollection(FilterGroup $filterGroup, Collection $collection)
{
    foreach ($filterGroup->getFilters() as $filter) {
        $condition = $filter->getConditionType() ? $filter->getConditionType() : 'eq';
        $collection->addFieldToFilter($filter->getField(), [$condition => $filter->getValue()]);
    }
}

This sure looks like the behavior where every filter in a single group is applied as an AND query. I'd bet even money there are other repositories like this as well.

Am I missing something?

[piotrekkaminski]

cc @choukalos @mbrinton01

[astorm]

Any update on this? The conversation above leads me to believe that unifying the filter group interfaces isn't a priority, and that the inconsistent behavior is a deliberate design decision. Closing out, but please re-open if the above is incorrect. @piotrekkaminski

[schmengler]

Another inconsistent repository is Magento\Eav\Model\AttributeRepository, where addFilterGroupToCollection() adds each filter separately, i.e. with "AND" instead of "OR" as expected from a filter group.

We have stories for making them use a generic logic to apply SearchCriteria and we are currently working on them.

A status update about this would be very appreciated. @slavvka @piotrekkaminski

[slavvka]

Hi all,

Currently all repos that had ability to work with Search Criteria are unified to use common mechanism which is described in this issue (FilterGroups are combined with AND while Filters inside them are combined with OR). Just check the latest mainline code.

[schmengler]

Right, in the develop branch it looks good now:
https://github.com/magento/magento2/commits/develop/app/code/Magento/Eav/Model/AttributeRepository.php (MAGETWO-56079)

Thanks!