Fixed Issue #17295 : Search REST API returns wrong total_count #20253

ronak2ram · 2019-01-12T19:53:41Z

Description (*)

I added function for getting total_count of result item.

Fixed Issues (if relevant)

Search REST API returns wrong total_count #17295: Search REST API returns wrong total_count

Manual testing scenarios (*)

Request :

rest/V1/search?searchCriteria[requestName]=quick_search_container&searchCriteria[filterGroups][0][filters][0][field]=search_term&searchCriteria[filterGroups][0][filters][0][conditionType]=like&searchCriteria[filterGroups][0][filters][0][value]=test&searchCriteria[current_page]=1&searchCriteria[page_size]=2

Response :

{
    "items": [
        {
            "id": 126,
            "custom_attributes": [
                {
                    "attribute_code": "score",
                    "value": "73.7660923004150400"
                }
            ]
        },
        {
            "id": 61,
            "custom_attributes": [
                {
                    "attribute_code": "score",
                    "value": "73.7660923004150400"
                }
            ]
        }
    ],
    "aggregations": {
        "buckets": [
            {
                "name": "category_bucket",
                "values": [
                    {
                        "value": "32",
                        "metrics": [
                            "32",
                            "1"
                        ]
                    }
                ]
            },
            {
                "name": "stock_status_bucket",
                "values": []
            },
            {
                "name": "rating_summary_bucket",
                "values": []
            },
            {
                "name": "am_is_new_bucket",
                "values": []
            },
            {
                "name": "am_on_sale_bucket",
                "values": []
            },
            {
                "name": "color_bucket",
                "values": []
            },
            {
                "name": "sub_category_bucket",
                "values": []
            }
        ],
        "bucket_names": [
            "category_bucket",
            "stock_status_bucket",
            "rating_summary_bucket",
            "am_is_new_bucket",
            "am_on_sale_bucket",
            "color_bucket",
            "sub_category_bucket"
        ]
    },
    "search_criteria": {
        "request_name": "quick_search_container",
        "filter_groups": [
            {
                "filters": [
                    {
                        "field": "search_term",
                        "value": "test",
                        "condition_type": "like"
                    }
                ]
            }
        ],
        "page_size": 2,
        "current_page": 1
    },
    "total_count": 9
}

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

magento-engcom-team · 2019-01-12T19:53:48Z

Hi @ronak2ram. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento-engcom-team give me test instance - deploy test instance based on PR changes
@magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

ronak2ram · 2019-01-12T19:56:06Z

@magento-engcom-team give me test instance

magento-engcom-team · 2019-01-12T19:56:12Z

Hi @ronak2ram. Thank you for your request. I'm working on Magento instance for you

magento-engcom-team · 2019-01-12T19:56:37Z

Hi @ronak2ram, here is your new Magento instance.
Admin access: https://pr-20253.instances.magento-community.engineering/admin
Login: admin Password: 123123q

ronak2ram · 2019-01-12T20:06:18Z

Confirmed on the test instance.

davidverholen · 2019-01-12T21:08:18Z

@ronak2ram currently your solution introduces a huge performance issue by sending a second search on each request to the search engine.

I did not go into detail yet, but wouldn't it be possible, to resolve this issue by fixing the original call to the search engine so it calculates the correct total count?

davidverholen · 2019-01-12T21:10:34Z

lib/internal/Magento/Framework/Search/Search.php

+     */
+    public function getTotalCount(SearchCriteriaInterface $searchCriteria)
+    {
+        $requestBuilder = \Magento\Framework\App\ObjectManager::getInstance()


ObjectManager must not be used directly here

@davidverholen I will remove ObjectManager in next commit.

davidverholen · 2019-01-12T21:11:09Z

lib/internal/Magento/Framework/Search/Search.php

+        }
+
+        $request = $requestBuilder->create();
+        $searchResponse = $this->searchEngine->search($request);


the search engine should not require a second search just to figure out the correct count

I do not find any other solution can you please suggest me a better solution to eliminate searchEngine?

ronak2ram · 2019-01-17T20:20:57Z

@magento-engcom-team give me test instance

magento-engcom-team · 2019-01-17T20:21:02Z

Hi @ronak2ram. Thank you for your request. I'm working on Magento instance for you

magento-engcom-team · 2019-01-17T20:21:47Z

Hi @ronak2ram, here is your new Magento instance.
Admin access: https://pr-20253.instances.magento-community.engineering/admin
Login: admin Password: 123123q

ronak2ram · 2019-01-23T06:28:55Z

Hello @davidverholen @orlangur
Can you please review this pull. I changed the flow to the resolved issue.

davidverholen · 2019-01-30T11:21:06Z

Hi @ronak2ram ,

this solution seems to be more "On the point" since it mostly extends the mysql search adapter to reflect the correct total count (even if the page size is lower than the total count).

However, you are introducing a new public method in the Query Response of the search framework which then probably needs to be implemented in all additional Search engines (like elasticsearch and 3rd party modules).

I will try to investigate a bit deeper if I see a solution, without changing the public api of the search framework. If not, it seems to be a design problem of the search framework api, which seems to be missing exactly the method you introduced in your change.

ronak2ram · 2019-02-15T05:11:37Z

Hello @davidverholen
Is there any update regarding this PR?

sivaschenko

Hi @ronak2ram thanks for the pull request, please see my code review comments.

@buskamuza @melnikovi please review this contribution, in order to process it we'll need an approval of adding \Magento\Framework\Search\Response\QueryResponse::getTotalCount public method.

sivaschenko · 2019-03-07T16:28:01Z

lib/internal/Magento/Framework/Search/Adapter/Mysql/Adapter.php

+     *
+     * @var array
+     */
+    protected $countSqlSkipParts = [


Please change the access modifier to private

sivaschenko · 2019-03-07T16:29:13Z

lib/internal/Magento/Framework/Search/Response/QueryResponse.php

+    /**
+     * {@inheritdoc}
+     */
+    public function getTotalCount()


@buskamuza @melnikovi can you please take a look if we can allow adding this method to the framework. It appears there is no other way to return the total count of the search result.

This class has count method already, that seem to have similar purpose. Are there cases when count that it uses is not efficient?

@melnikovi Count method returns a total count of the search result items of the current page.
We make getTotalCount for the return all pages items count.

sivaschenko · 2019-03-07T16:29:58Z

lib/internal/Magento/Framework/Search/Adapter/Mysql/Adapter.php

+     *
+     * @return Select
+     */
+    protected function getSelectCountSql($query)


Please change method access modifier to private

sivaschenko · 2019-03-07T16:30:03Z

lib/internal/Magento/Framework/Search/Adapter/Mysql/Adapter.php

+     *
+     * @return int
+     */
+    public function getSize($query)


Please change method access modifier to private

sivaschenko · 2019-03-07T16:30:38Z

lib/internal/Magento/Framework/Search/Response/QueryResponse.php

+     *
+     * @var int
+     */
+    protected $totalCount;


Please change property access modifier to private

sivaschenko · 2019-03-07T16:32:07Z

lib/internal/Magento/Framework/Search/Response/QueryResponse.php

    /**
     * @param Document[] $documents
     * @param AggregationInterface $aggregations
+     * @param Total size int $size


Please correct the phpdoc: @param int $size Total size

ronak2ram · 2019-03-12T12:06:03Z

Hello Masters @sivaschenko @melnikovi @davidverholen
Thanks for the review of this PR.
By mistake, I deleted the base branch of this pull request. So I can't able to update the changes in this PR.
So I created new PR with the suggested changes by @sivaschenko and latest code.

Please Review PR : #21713

sivaschenko · 2019-03-12T12:26:51Z

Thanks @ronak2ram ! As all the concerns in the review were fixed, I approved the new pull request, closing this one as outdated.

ghost · 2019-03-12T12:26:52Z

Hi @ronak2ram, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

magento-engcom-team added Component: Framework/Search Release Line: 2.3 Partner: Krish TechnoLabs partners-contribution Pull Request is created by Magento Partner labels Jan 12, 2019

davidverholen suggested changes Jan 12, 2019

View reviewed changes

davidverholen added the Progress: needs update label Jan 12, 2019

orlangur assigned davidverholen Jan 12, 2019

davidverholen added the help wanted label Jan 17, 2019

Resolve Issue : Search REST API returns wrong total_count

d0a44f8

Update code based on travis review

7c1e728

ronak2ram requested a review from orlangur January 22, 2019 13:14

davidverholen removed the Progress: needs update label Jan 30, 2019

sivaschenko requested changes Mar 7, 2019

View reviewed changes

ghost assigned sivaschenko Mar 7, 2019

ghost added the Progress: needs update label Mar 7, 2019

orlangur removed their request for review March 7, 2019 16:42

ronak2ram mentioned this pull request Mar 12, 2019

Resolve Issue : Search REST API returns wrong total_count #21713

Merged

4 tasks

sivaschenko closed this Mar 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed Issue #17295 : Search REST API returns wrong total_count #20253

Fixed Issue #17295 : Search REST API returns wrong total_count #20253

ronak2ram commented Jan 12, 2019

magento-engcom-team commented Jan 12, 2019

ronak2ram commented Jan 12, 2019

magento-engcom-team commented Jan 12, 2019

magento-engcom-team commented Jan 12, 2019

ronak2ram commented Jan 12, 2019

davidverholen commented Jan 12, 2019

davidverholen Jan 12, 2019

ronak2ram Jan 17, 2019

davidverholen Jan 12, 2019

ronak2ram Jan 17, 2019

ronak2ram commented Jan 17, 2019

magento-engcom-team commented Jan 17, 2019

magento-engcom-team commented Jan 17, 2019

ronak2ram commented Jan 23, 2019 •

edited

Loading

davidverholen commented Jan 30, 2019

ronak2ram commented Feb 15, 2019

sivaschenko left a comment

sivaschenko Mar 7, 2019

sivaschenko Mar 7, 2019

melnikovi Mar 8, 2019

ronak2ram Mar 8, 2019 •

edited

Loading

sivaschenko Mar 7, 2019

sivaschenko Mar 7, 2019

sivaschenko Mar 7, 2019

sivaschenko Mar 7, 2019

ronak2ram commented Mar 12, 2019

sivaschenko commented Mar 12, 2019

ghost commented Mar 12, 2019

Fixed Issue #17295 : Search REST API returns wrong total_count #20253

Fixed Issue #17295 : Search REST API returns wrong total_count #20253

Conversation

ronak2ram commented Jan 12, 2019

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Request :

Response :

Contribution checklist (*)

magento-engcom-team commented Jan 12, 2019

ronak2ram commented Jan 12, 2019

magento-engcom-team commented Jan 12, 2019

magento-engcom-team commented Jan 12, 2019

ronak2ram commented Jan 12, 2019

davidverholen commented Jan 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronak2ram commented Jan 17, 2019

magento-engcom-team commented Jan 17, 2019

magento-engcom-team commented Jan 17, 2019

ronak2ram commented Jan 23, 2019 • edited Loading

davidverholen commented Jan 30, 2019

ronak2ram commented Feb 15, 2019

sivaschenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronak2ram Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronak2ram commented Mar 12, 2019

sivaschenko commented Mar 12, 2019

ghost commented Mar 12, 2019

ronak2ram commented Jan 23, 2019 •

edited

Loading

ronak2ram Mar 8, 2019 •

edited

Loading