Fix #23077: Fix condition Safe-NULL condition on Update Trigger

[sebastien-kathmandu]

Description (*)

Safe-NULL in the Update Trigger created by View is used in the wrong way. As we are using an external integration system using INSERT INTO ... ON DUPLICATE KEY UPDATE value= ... on most on our product ( for example stock, prices, varchar attribute) each integration is followed by a reindex of most of our products that are not really updated. And impact a lot the performances of our database.

Fixed Issues (if relevant)

1. Duplicated product is out of stock when index set to UPDATE BY SCHEDULE #15939: Duplicated product is out of stock when index set to UPDATE BY SCHEDULE
2. Triggers created by MView are triggered all the time #23077: Triggers created by MView are triggered all the time

Manual testing scenarios (*)

To understand the condition used before and after the commit df5597a2e and the solution proposed in this Pull Request.

I'm doing this benchmark playground :

CREATE TABLE `test_trigger_cond1` (
  `value` VARCHAR(255) DEFAULT NULL
) ENGINE=INNODB DEFAULT CHARSET=utf8;

CREATE TABLE `test_trigger_cond2` (
  `value` VARCHAR(255) DEFAULT NULL
) ENGINE=INNODB DEFAULT CHARSET=utf8;

INSERT INTO `test_trigger_cond1` (`value`)
VALUES
    ('Magento'),
    (NULL),
    ('1');

INSERT INTO `test_trigger_cond2` (`value`)
VALUES
    ('Magento'),
    (NULL),
    ('1');


SELECT
a.value "Value A",
b.value "Value B",
a.value != b.value "before commit df5597a2e4",
a.value <=> b.value "after commit df5597a2e4",
NOT a.value <=> b.value "Suggested Trigger Comparison"
FROM test_trigger_cond1 a, test_trigger_cond2 b;

here the result of the last query:

Value A	Value B	before commit df5597a2e4	after commit df5597a2e4	Suggested Trigger Comparison
Magento	Magento	0	1	0
NULL	Magento	NULL	0	1
1	Magento	1	0	1
Magento	NULL	NULL	0	1
NULL	NULL	NULL	1	0
1	NULL	NULL	0	1
Magento	1	1	0	1
NULL	1	NULL	0	1
1	1	0	1	0

Before

as we can see before the commit the NULL condition could cause a lot of trouble if for example a previous attribute were NULL or become NULL as change the

After

Only if the both value are the same we got true.

if we look a classic line in a trigger for example : trg_catalog_product_entity_int_after_update

BEGIN
SET @entity_id = (SELECT `entity_id` FROM `catalog_product_entity` WHERE `row_id` = NEW.`row_id`);
...
IF (
NEW.`value_id` <=> OLD.`value_id` OR 
NEW.`attribute_id` <=> OLD.`attribute_id` OR 
NEW.`store_id` <=> OLD.`store_id` OR 
NEW.`row_id` <=> OLD.`row_id` OR 
NEW.`value` <=> OLD.`value`
) THEN 
INSERT IGNORE INTO `catalog_product_flat_cl` (`entity_id`) values(@entity_id); END IF;
...
END

if at least one of these fields (value_id, attribute_id, store_id, row_id, value) didn't change we will always insert a new row inside catalog_product_flat_cl. And this is always the case.

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)

[m2-assistant]

Hi @sebastien-kathmandu. 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 give me test instance - deploy test instance based on PR changes
• @magento give me 2.3-develop instance - deploy vanilla Magento instance

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

[mattheo-geoffray]

For people looking at this PR there is also this logic in a Catalog Staging class in the Commerce version

[rodrigowebjump]

Hi @sebastien-kathmandu did you review the functional test that failed?

https://testing-service.magento-community.engineering/reports/magento/magento2/pull/23294/70914594-551e-495a-8f97-9a8e92625ac0/26168/Functional/allure-report-ce/index.html#suites/8187718e2fd144987673ef34ee62b41b

Regards

[seyuf]

Yep any update on why the test failed?
As the same test is ok on 2.2-develop (I.e here)
But failing on 2.3-develop i.e here

I have this fix on prod 2.3.1 ^^. It seems to be working fine...
Anyways, it would be nice if someone could confirm that this is just not a minor nonsense in the testing process.
@sebastien-kathmandu ?

Thanks

[sebastien-kathmandu]

Hi @seyuf and @rodrigowebjump ,

I saw the test failing, and I don't know too why this test fail, I need to setup this functional test on my local to understand why. If anyone has an idea, I would be happy to get any clue ;-) . I'm wondering if an other part also of indexing has an issue and that would explain this test failing.

Cheers,

Sebastien

[sivaschenko]

@magento run all tests

[magento-engcom-team]

Hi @sivaschenko, thank you for the review.
ENGCOM-5564 has been created to process this Pull Request
✳️ @sivaschenko, could you please add one of the following labels to the Pull Request?

Label	Description
Auto-Tests: Covered	All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered	Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required	Changes in Pull Request does not require coverage by auto-tests

[magento-engcom-team]

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

[hostep]

It looks like Magento core devs fixed this as well very recently in MC-19407: Number of the rows increase very fast in changelog table

[sivaschenko]

@hostep indeed, thanks for the notification. @sebastien-kathmandu thank you very much for the pull request, your contribution is appreciated! However, as you can see the issue has already been fixed so I have to close this pull request.

[m2-assistant]

Hi @sebastien-kathmandu, 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.