Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] ES 2.7 M 2.3 InventoryData loads all products #1223

Closed
Swahjak opened this issue Dec 8, 2018 · 8 comments
Closed

[BUG] ES 2.7 M 2.3 InventoryData loads all products #1223

Swahjak opened this issue Dec 8, 2018 · 8 comments
Assignees
Labels

Comments

@Swahjak
Copy link
Contributor

Swahjak commented Dec 8, 2018

Preconditions

Magento Version : 2.3

ElasticSuite Version : 2.7

Environment : develop / production

Third party modules :

Steps to reproduce

  1. Try reindexing after upgrading to ES 2.7

Expected result

  1. Products are loaded into the index

Actual result

  1. Products only return a stock status and only the final batch is actually complete. The entire index before the last batch is overwritten by a stock only document.

Commit 010f0a8#diff-59575d408d4bffc1107134de0085acdaL86 addresses multi inventory support but removes the filter on product ids.

The variable $productIds is never used. This causes the index to overwrite itself because it loads ALL product.

Smile\ElasticsuiteCatalog\Model\ResourceModel\Product\Indexer\Fulltext\Datasource\InventoryData

    public function loadInventoryData($storeId, $productIds)
    {
        $websiteId = $this->getWebsiteId($storeId);
        $stockId   = $this->getStockId($websiteId);
        $tableName = $this->stockIndexTableProvider->execute($stockId);

        $select = $this->getConnection()->select()
            ->from(['product' => $this->getTable('catalog_product_entity')], [])
            ->join(
                ['stock_index' => $tableName],
                'product.sku = stock_index.' . IndexStructure::SKU,
                [
                    'product_id'    => 'product.entity_id',
                    'stock_status'  => 'stock_index.' . IndexStructure::IS_SALABLE,
                    'qty'           => 'stock_index.' . IndexStructure::QUANTITY,
                ]
            );

        return $this->getConnection()->fetchAll($select);
    }
Swahjak added a commit to Swahjak/elasticsuite that referenced this issue Dec 8, 2018
Swahjak added a commit to Swahjak/elasticsuite that referenced this issue Dec 8, 2018
@Swahjak
Copy link
Contributor Author

Swahjak commented Dec 8, 2018

Created two slightly different pull requests since there was some room for improvement that wasn't directly related to fixing the bug. Up to the maintainers to merge the one they want.

@rbayet
Copy link
Collaborator

rbayet commented Dec 10, 2018

Hello @Swahjak,

I'll look into it. I don't know why the issue didn't popped up.
What is your catalog size ?

I'll close your second PR, it actually breaks MSI support.

Regards,

@Swahjak
Copy link
Contributor Author

Swahjak commented Dec 10, 2018

Hi @rbayet our catalog size is roughly 8000 sku's. But anything > 1000 products (batch size) should have this problem.

Why would the second PR break MSI? It's the same as the original query, but removes the unnecessary dependency on catalog_product_entity. I don't see how that is required for a successful query.

@rbayet
Copy link
Collaborator

rbayet commented Dec 10, 2018

Hello @Swahjak,

OK, I'll look into it.

See my comments in the second PR (#1225 (comment)).
You need to declare additional inventory sources and stocks to see the problem :

mysql> show columns from inventory_stock_1;
+------------+----------------------+------+-----+---------+-------+
| Field      | Type                 | Null | Key | Default | Extra |
+------------+----------------------+------+-----+---------+-------+
| product_id | int(10) unsigned     | NO   |     | NULL    |       |
| website_id | smallint(5) unsigned | NO   |     | NULL    |       |
| stock_id   | smallint(5) unsigned | NO   |     | NULL    |       |
| quantity   | decimal(12,4)        | NO   |     | 0.0000  |       |
| is_salable | smallint(5) unsigned | NO   |     | NULL    |       |
| sku        | varchar(64)          | YES  |     | NULL    |       |
+------------+----------------------+------+-----+---------+-------+
6 rows in set (0.00 sec)

versus

mysql> show columns from inventory_stock_2;
+------------+---------------+------+-----+---------+-------+
| Field      | Type          | Null | Key | Default | Extra |
+------------+---------------+------+-----+---------+-------+
| sku        | varchar(64)   | NO   | PRI | NULL    |       |
| quantity   | decimal(10,4) | NO   |     | 0.0000  |       |
| is_salable | tinyint(1)    | NO   |     | NULL    |       |
+------------+---------------+------+-----+---------+-------+
3 rows in set (0.00 sec)

@Swahjak
Copy link
Contributor Author

Swahjak commented Dec 10, 2018

@rbayet Ah, Magento always has some trick up its sleeve to throw you off 😂 never expect the obvious. It's still a riddle to me as to why they are so keen on the SKU being used for everything.

@rbayet
Copy link
Collaborator

rbayet commented Dec 10, 2018

@Swahjak concerning the main issue, it's indeed quite obvious on the sample data with a batch size of 10.

With the default batch size of 1000 and the sample data, something funny happens : the only products with an indexed "name" attribute are the 181 visible products (simple, bundle, configurable), while the affected products are all the "not visible individually" products.
Hence why it's not blatantly visible and I missed this.

Good catch, thanks for the report !

Regards,

@rbayet
Copy link
Collaborator

rbayet commented Dec 11, 2018

Re-opened for visibility until 2.7.1 actual release.

@rbayet rbayet reopened this Dec 11, 2018
@rbayet
Copy link
Collaborator

rbayet commented Dec 12, 2018

Fixed by PR #1224

@rbayet rbayet closed this as completed Dec 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants