Fix performance leak in salesrule collection

[david-fuehr]

Description

Refactored sql query that created a huge temporary table for each request, when a greater amount of salesrules and coupon codes exists in database. The sorting of this table took a lot of cpu time.
The statement now consists of two subselects that drill down the remaining lines as far as possible, so that the remaining temporary table is minimal and easily sorted.

Fixed Issues

1. High Database Load for Sales Rule Validation #19117: High Database Load for Sales Rule Validation

Manual testing scenarios

1. Create an active salesrule with no coupon code (A)
2. Create an inactive salesrule with no coupon code (B)
3. Create an active salesrule with coupon_type = 'autogenerated' and generate 5000 codes (C)
4. Create an active salesrule with coupon_type = 'specific' (D)
5. Create an inactive salesrule with coupon code (E)
6. Add a product to the cart (qualified for all active salesrules)
7. Go to checkout/cart
   1. The subtotal for salesrules only lists label A
8. Add one of the coupon codes of C to the cart
   1. the coupon code can be applied
   2. The subtotal for salesrules lists label A,C
9. Remove the applied coupon code
   1. the coupon code can be removed
10. Add the coupon code D to the cart
    1. the coupon code can be applied
    2. The subtotal for salesrules lists label A,D
11. Add the coupon code E to the cart
    1. the coupon code can not be applied
12. Check out the order
    1. checkout succeeds
13. Configure rule A to priority = 1
14. Configure rule D to priority = 2
15. Add the product to a new quote
    1. The subtotal for salesrules only lists label A
16. Apply Coupon for salesrule D
    1. The subtotal for salesrules lists label A,D
17. Configure rule A to priority = 3
18. Update cart items quantity
    1. The subtotal for salesrules lists label D,A

Additional Information

We use this fix in a magento commerce environment

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-cicd2]

All committers have signed the CLA.

[magento-engcom-team]

Hi @david-fuehr. 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

[mslabko]

Why do you merge sales rules without coupons ($noCouponRules) to select in case, when we filter by specific coupon?
If you will have a big amount of sales rules without a coupon code (coupon_type == Rule::COUPON_TYPE_NO_COUPON) then you will have big performance degradation.
I think we should optimize this flow

BTW, you can experiment with a big amount of sales rules using generation toolkit: https://devdocs.magento.com/guides/v2.3/config-guide/cli/config-cli-subcommands-perf-data.html (you can adjust [car_price_rule](https://github.com/magento/magento2/blob/2.3-develop/setup/performance-toolkit/profiles/ce/small.xml#L40 for generate any amount of sales rules)

[mslabko]

For this PR you can ignore ^^ my comment, due to it relates to current process how we apply sales rules to the cart. It can be implemented in separate PR.

[david-fuehr]

Thank you for the review and your comment. As I understand, it is necessary to return all valid rules when a coupon code is available - even those without a coupon code, because they may be applied automatically (I suppose you referred to that in your second comment).

And you are right - a lot of active salesrules with Rule::COUPON_TYPE_NO_COUPON will have an impact on the performance. But I would say that a big amount of salesrules is maybe 5,000 rules. When you create five salesrules with a autogenerated coupon for each of your 100,000 newsletter subscribers, you will have 500,000 coupon codes. In my opinion this is the most likely case to massively increase the size of the temporary table in the original statement. The refactoring addresses the latter.

[larsroettig]

@magento-engcom-team give me test instance

[magento-engcom-team]

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

[magento-engcom-team]

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

[magento-engcom-team]

Hi @larsroettig, thank you for the review.
ENGCOM-3979 has been created to process this Pull Request

[magento-engcom-team]

@david-fuehr thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

[larsroettig]

✅ Review
✅ Lokal testing with Branch

I we need this fix for one of our customer and it works well

[sivaschenko]

Hi @david-fuehr can you please update the branch, resolve conflicts and check if the issue is still actual (it may have been fixed considering the conflicting code changes).

[david-fuehr]

Hi @sivaschenko,

thanks for asking. I checked the changes from magento/2.3-develop. They address the issue I was working on.

In two details I prefer my solution:

1. I did some refactoring to improve readability of the code, e.g.

        $orWhereConditions = [
            "(" . implode($autoGeneratedCouponCondition, " AND ") . ")",
            $connection->quoteInto(
                '(main_table.coupon_type = ? AND main_table.use_auto_generation = 1 AND rule_coupons.type = 1)',
                \Magento\SalesRule\Model\Rule::COUPON_TYPE_SPECIFIC
            ),
            $connection->quoteInto(
                '(main_table.coupon_type = ? AND main_table.use_auto_generation = 0 AND rule_coupons.type = 0)',
                \Magento\SalesRule\Model\Rule::COUPON_TYPE_SPECIFIC
            ),
        ];

vs

        $isValidSpecific =
            $this->getConnection()->quoteInto('(main_table.coupon_type = ?)', Rule::COUPON_TYPE_SPECIFIC)
            . ' AND (' .
            '(main_table.use_auto_generation = 1 AND rule_coupons.type = 1)'
            . ' OR ' .
            '(main_table.use_auto_generation = 0 AND rule_coupons.type = 0)'
            . ')';

2. The union query is faster than the current solution from 2.3-develop with two queries. Compared with the original query this is a micro optimization. This is the query execution time from my test setup:

original	current 2.3-devlop	pr
~1.4s	47ms	24ms

I resolved the conflict, so you could merge this pull request. But I am also totally fine, if you choose to keep the current changes and close this pull request.

[magento-engcom-team]

Hi @sivaschenko, thank you for the review.
ENGCOM-3979 has been created to process this Pull Request

[ghost]

Hi @david-fuehr, 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.