Product import doesn't import all products, but still gives success message

[piotrekkaminski]

Original issue: https://github.com/magento/magento2/issues/5846

Preconditions

1. Magento default product importer
2. Simple products in a Magento-format CSV

   Steps to reproduce

1. Run Check Data on product CSV. Check Data returns valid
2. Import products. Magento shows success

   Expected result

1. Products are imported

   Actual result

1. Not all products are imported

When Check Data is run, it parses and batches product data into groups of 100 records, and stores them in the importexport_importdata table using https://github.com/magento/magento2/blob/develop/app/code/Magento/ImportExport/Model/ResourceModel/Import/Data.php#L159.

However, there is no check to see that $this->jsonHelper->jsonEncode($data) returns valid data. When it returns null, this is entered into importexport_importdata. At https://github.com/magento/magento2/blob/develop/app/code/Magento/CatalogImportExport/Model/Import/Product.php#L868, this null data is returned, which causes the while() to end.

Magento thinks the product import was a success as it reached the end of the data to import, when in fact it hit a null value, and there was additional data to import after. This is how this bug was discovered, as it was failing in random batches of 100. The first four data bunches appeared as JSON in importexport_importdata, but five and six were null; there was valid JSON in bunches seven and eight but these were never hit.

This is particularly dangerous as a merchant could update product prices, get a message they went in, and they haven't gone in.

Ultimately the fault is in the data passed to the JSON encoder, but an error should be thrown here so merchants know the product data couldn't be imported.

[piotrekkaminski]

Reproduced on Enterprise 2.1.0 - from @jameswithers

[piotrekkaminski]

@jameswithers: @IlnitskiyArtem The error is still present in the code:

https://github.com/magento/magento2/blob/2.2.0-preview/app/code/Magento/CatalogImportExport/Model/Import/Product.php#L1539 stops product imports when it encounters a non-true value e.g. null. null is being entered into the import data tables by https://github.com/magento/magento2/blob/2.2.0-preview/app/code/Magento/ImportExport/Model/ResourceModel/Import/Data.php#L159, as it does not check that $this->jsonHelper->jsonEncode($data) returns a response other than null, so it enters null into the tables. This null value is then returned by https://github.com/magento/magento2/blob/2.2.0-preview/app/code/Magento/ImportExport/Model/ResourceModel/Import/Data.php#L131, which causes the import to end prematurely when this value is returned by the database (https://github.com/magento/magento2/blob/2.2.0-preview/app/code/Magento/CatalogImportExport/Model/Import/Product.php#L1539).

As in the opening of this ticket a year ago, the fundamental issue is that $this->jsonHelper->jsonEncode($data) is not checked for a value before being used in https://github.com/magento/magento2/blob/2.2.0-preview/app/code/Magento/ImportExport/Model/ResourceModel/Import/Data.php#L159. If $this->jsonHelper->jsonEncode($data) returns null, Magento should notify the user that there was a problem processing that bunch as the data could not be JSON encoded. Instead, users see a message that the import was successful, even though not all records have processed. @IlnitskiyArtem: I tried to reproduce it importing 100+ simple products about 20 times, each time changing price and quantity but still cannot cannot observe this behavior. Please, provide more information about your .csv's sensitive data, with my .csv everything works fine. Without reproducing the case i can't create internal ticket. @jameswithers: the problem is not the file (I no longer have access to this as it was over a year ago), it's in Magento not raising an error. If you unit test this, you should be able to make jsonEncode return null once, and you will see the behaviour in action. @IlnitskiyArtem: Created internal issue MAGETWO-70963 to investigate and track this issue. Thanks for your help.

[piotrekkaminski]

@lewisvoncken, @experius-nl My college has already created a fix for this. I will come back to you this week with a pull request with his fix. A pull request with a fix has been made for branch 2.1-develop in the partner Github

[piotrekkaminski]

@vkublytskyi: Thank you all who participated in the discussion on the issue. @experius-nl created a PR in partners repository to add handling of JSON encoding error. We are going to accept and merge these changes.

However, such fix cannot be counted as a full as any invalid data should be detected before Magento\ImportExport\Model\ResourceModel\Import\Data::saveBunch call. So if some data lead to failure in json_encode (e.g. JSON_ERROR_UTF8) this also shows that there is some issue with validation in earlier stages that should be found and fixed. The help of the Community in this highly welcome and appreciated.

[dmanners]

The plan here would be to show the status of the import on the success page. So in that way if you have imported 2000 products it should say so. Show the validation messages on the success page would also be good in this ticket.

[dmanners]

This PR has now been merged into 2.3-develop.