Magento2: Serious bug in Reports module (Magento 1&2)

[bgalileo]

I just found a really bad bug in the Reports module that can make it completely impossible for a specific customer to view a specific product page. I could reliably reproduce this problem in a Magento 1 instance (MySQL version: 5.5) and the buggy code has remained unchanged in M2.

Excerpt from exception.log (Magento 1.8):

Next exception 'Zend_Db_Statement_Exception' with message 'SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '42-27' for key 'UNQ_REPORT_VIEWED_PRODUCT_INDEX_CUSTOMER_ID_PRODUCT_ID'' in /[redacted]/lib/Zend/Db/Statement/Pdo.php:234
Stack trace:
#0 /[redacted]/lib/Varien/Db/Statement/Pdo/Mysql.php(110): Zend_Db_Statement_Pdo->_execute(Array)
#1 /[redacted]/app/code/core/Zend/Db/Statement.php(291): Varien_Db_Statement_Pdo_Mysql->_execute(Array)
#2 /[redacted]/lib/Zend/Db/Adapter/Abstract.php(479): Zend_Db_Statement->execute(Array)
#3 /[redacted]/lib/Zend/Db/Adapter/Pdo/Abstract.php(238): Zend_Db_Adapter_Abstract->query('INSERT INTO `re...', Array)
#4 /[redacted]/lib/Varien/Db/Adapter/Pdo/Mysql.php(428): Zend_Db_Adapter_Pdo_Abstract->query('INSERT INTO `re...', Array)
#5 /[redacted]/lib/Varien/Db/Adapter/Pdo/Mysql.php(1936): Varien_Db_Adapter_Pdo_Mysql->query('INSERT INTO `re...', Array)
#6 /[redacted]/app/code/core/Mage/Reports/Model/Resource/Helper/Mysql4.php(48): Varien_Db_Adapter_Pdo_Mysql->insertOnDuplicate('report_viewed_p...', Array, Array)
#7 /[redacted]/app/code/core/Mage/Reports/Model/Resource/Product/Index/Abstract.php(150): Mage_Reports_Model_Resource_Helper_Mysql4->mergeVisitorProductIndex('report_viewed_p...', Array, Array)
#8 /[redacted]/app/code/core/Mage/Core/Model/Abstract.php(318): Mage_Reports_Model_Resource_Product_Index_Abstract->save(Object(Mage_Reports_Model_Product_Index_Viewed))
#9 /[redacted]/app/code/core/Mage/Reports/Model/Event/Observer.php(130): Mage_Core_Model_Abstract->save()
#10 /[redacted]/app/code/core/Mage/Core/Model/App.php(1338): Mage_Reports_Model_Event_Observer->catalogProductView(Object(Varien_Event_Observer))
#11 /[redacted]/app/code/core/Mage/Core/Model/App.php(1317): Mage_Core_Model_App->_callObserverMethod(Object(Mage_Reports_Model_Event_Observer), 'catalogProductV...', Object(Varien_Event_Observer))
#12 /[redacted]/app/Mage.php(448): Mage_Core_Model_App->dispatchEvent('catalog_control...', Array)
#13 /[redacted]/app/code/core/Mage/Catalog/Helper/Product/View.php(135): Mage::dispatchEvent('catalog_control...', Array)
#14 /[redacted]/app/code/core/Mage/Catalog/controllers/ProductController.php(132): Mage_Catalog_Helper_Product_View->prepareAndRender(27, Object(Mage_Catalog_ProductController), Object(Varien_Object))
#15 /[redacted]/app/code/core/Mage/Core/Controller/Varien/Action.php(418): Mage_Catalog_ProductController->viewAction()
#16 /[redacted]/app/code/core/Mage/Core/Controller/Varien/Router/Standard.php(250): Mage_Core_Controller_Varien_Action->dispatch('view')
#17 /[redacted]/app/code/core/Mage/Core/Controller/Varien/Front.php(172): Mage_Core_Controller_Varien_Router_Standard->match(Object(Mage_Core_Controller_Request_Http))
#18 /[redacted]/app/code/core/Mage/Core/Model/App.php(354): Mage_Core_Controller_Varien_Front->dispatch()
#19 /[redacted]/app/Mage.php(684): Mage_Core_Model_App->run(Array)
#20 /[redacted]/index.php(87): Mage::run('', 'store')
#21 {main}

Problem description / Steps to reproduce

The Reports module updates the database table report_viewed_product_index whenever a customer views a product:

• It registers an observer for the event catalog_controller_product_view
• The observer then calls Magento\Reports\Model\Resource\Helper::mergeVisitorProductIndex() indirectly (when saving the index).
• This method then calls Magento\Framework\DB\Adapter\Pdo::insertOnDuplicate() which creates a INSERT […] ON DUPLICATE KEY UPDATE […] query

Problem

There are 2 unique indices that might collide with the new entry Magento tries to insert:

• (visitor_id, product_id)
• (customer_id, product_id)

So there may be 2 rows which conflict with the new one. If that happens, MySQL refuses to update any row in the table (despite the ON DUPLICATE KEY UPDATE clause) and produces the above (fatal) error.

Example data

| visitor_id | customer_id | product_id |
| --- | --- | --- |
| 42 | 3 | 123 |
| 43 | null | 123 |

If Magento tries to insert the tuple (43, 3, 123) into the table now, the above error will occur and the customer will be prevented from viewing the product page. (It just took me two hours to figure out why.)

[bgalileo]

Suggested fix: Remove Magento\Framework\DB\Adapter\Pdo::insertOnDuplicate() entirely and rewrite the few Model classes that use it, since ON DUPLICATE KEY UPDATE clauses don't work as expected when multiple unique indices exist. This will prevent further bugs like this one from occurring and possibly fixes bugs that have not been discovered yet.

[otoolec]

Thanks for reporting this bug. We've opened up ticket MAGETWO-33594 internally to work on a fix.

[kokoc]

@bgalileo Magento2 solves this bug on application level in \Magento\Reports\Model\Resource\Product\Index\AbstractIndex::updateCustomerFromVisitor method.
We decided to keep insertOnDuplicate method in place as useful feature. Other related issues will be fixed in the same way or on DB level.

Could you please verify the fix?

[inkysplat]

I'm experiencing the same problem in Magento 1.9.11, any update on a fix for this?

[matthewjfberry]

Also experiencing this issue in Magento 1.9.1.0. Reported on Magento SE.

Still waiting on a fix.

[melvyn-sopacua]

@matthewjfberry Your problem is not related. This is about the indexer, not the loading of a model with wrong parameters set upstream.

[melvyn-sopacua]

The better fix here is to drop at least one of the unique indexes. They try to assert that a human entity can only place one review with one product. The identifier for the human entity is however not well defined:

1. The same human can have multiple visitor ID's
2. A different human can be associated with one account ID [1]
3. The same human with same visitor ID can have 2 values for account ID
4. Multiple visitor ID's resolve to one distinct account ID (NULL), for which the number of human entities lies between 1 and N(visitor_id).
5. Recycling of visitor_id causes a number of false positives that are extremely hard to debug, while truncation and reset of increment ID for visitor log tables is quite common.

The practical solution is to disallow anonymous review postings and the stricter requirement that the table shall not be touched till an account_id is known. So the account_id <=> product_id can remain.

@kokoc
In addition MySQL discourages the use of ON DUPLICATE KEY triggers when mulitple unique indexes are present:

If a=1 OR b=2 matches several rows, only one row is updated. In general, you should try to avoid using an ON DUPLICATE KEY UPDATE clause on tables with multiple unique indexes.

So even if this would do the right thing in the selection process, it would perform an incomplete update. Keeping insertOnDuplicate should therefore deny service if more then one unique index is present on the table.

[1] An account can have multiple email addresses associated with it. While this assumes alternatives for the same human, it may be used as a shared account for a group like a family or business.

[matthewjfberry]

@melvyn-sopacua, updated question to include correct stack trace, my apologies. I definitely think the issue is related as by disabling the report product view index, this issue was resolved.

This obviously isn't an optimal solution, but having all users be able to view all products is preferential over being able to track product views. At least as an interim solution. I'm hoping that this issue becomes resolved soon.

[kokoc]

Appropriate item is placed in Magento 1 backlog. Closing this one as irrelevant for M2 project

[ajmudrak]

In magento 1.7, I decided that the best course of action was to make the following change in app/code/core/Mage/Reports/Model/Resource/Product/Index/Abstract.php (Mage_Reports_Model_Resource_Product_Index_Abstract):

        $matchFields = array('product_id', 'store_id');

// UPDATE HERE
        $this->updateCustomerFromVisitor($object);
// END OF UPDATE

        Mage::getResourceHelper('reports')->mergeVisitorProductIndex(
            $this->getMainTable(),
            $data,

This will merge the visitor_id and customer_id records together before attempting to insert any data. The problem is that MySQL will try to insert on duplicate key of visitor_id/product_id, but then also find that customer_id/product_id key is duplicated, which is not a case it can handle effectively. This existing updateCustomerFromVisitor function works pretty well and seems to preserve the most functionality.

[andrr]

@ajmudrak this is not correct way to fix it. All data which related to current customer_id will be lost in DB.
I've added identical function to this class Mage_Reports_Model_Resource_Product_Index_Abstract

   public function updateCustomerFromVisitorByProductId(Mage_Reports_Model_Product_Index_Abstract $object)
    {
        /**
         * Do nothing if customer not logged in
         */
        if (!$object->getCustomerId() || !$object->getVisitorId() || !$object->getProductId()) {
            return $this;
        }

        $adapter = $this->_getWriteAdapter();
        $select  = $adapter->select()
            ->from($this->getMainTable())
            ->where('visitor_id = ?', $object->getVisitorId())
            ->where('product_id = ?', $object->getProductId());

        $rowSet = $select->query()->fetchAll();
        foreach ($rowSet as $row) {

            $select = $adapter->select()
                ->from($this->getMainTable())
                ->where('customer_id = ?', $object->getCustomerId())
                ->where('product_id = ?', $row['product_id']);

            $idx = $adapter->fetchRow($select);
            if ($idx) {
                /* If we are here it means that we have two rows: one with known customer, but second just visitor is set
                 * One row should be updated with customer_id, second should be deleted
                 */
                $adapter->delete($this->getMainTable(), array('index_id = ?' => $row['index_id']));
                $where = array('index_id = ?' => $idx['index_id']);
                $data  = array(
                    'visitor_id'    => $object->getVisitorId(),
                    'store_id'      => $object->getStoreId(),
                    'added_at'      => Varien_Date::now(),
                );
            } else {
                $where = array('index_id = ?' => $row['index_id']);
                $data  = array(
                    'customer_id'   => $object->getCustomerId(),
                    'store_id'      => $object->getStoreId(),
                    'added_at'      => Varien_Date::now()
                );
            }

            $adapter->update($this->getMainTable(), $data, $where);

        }
        return $this;
    }

and call of this in save() method

     unset($data[$this->getIdFieldName()]);

        if(Mage::getSingleton('customer/session')->isLoggedIn()){
            $this->updateCustomerFromVisitorByProductId($object);
        }

        $matchFields = array('product_id', 'store_id');

Expected result:
Magento stopped throw the Exceptions and data in "report_viewed_product_index" looks fine.

[postadelmaga]

should we expect any fix for Magento 1.x ?