Refactored Source For ImportExport module, related to #7469 #9480

IvanChepurnyi · 2017-05-03T01:10:34Z

Description

The following pull request provides a refactored Import/Export source file parsing.
These are the major new features of this pull requests:

Developer can create custom parsers in any format by implementing \Magento\ImportExport\Model\Import\Source\FileParser\ParserInterface
Developer can create custom parser factories, so any stream or external resource can be feed as import. For adding new factory, developer needs to implement \Magento\ImportExport\Model\Import\Source\FileParser\ParserFactoryInterface and add it to \Magento\ImportExport\Model\Import\Source\FileParser\CompositeParserFactory as an argument in di.xml
Zip files are now properly implemented
Whole file parser logic is covered with tests and driven by TDD

Fixed Issues

#7468: CatalogImportExport doesn't support empty row values
* Added possiblity to specify "null" option for CsvParser, but this option must be passed into file creation factory
* Added extension points \Magento\ImportExport\Controller\Adminhtml\Import\Validate::getSourceAdapterOptions() and to \Magento\ImportExport\Model\Import::getSourceAdapterOptions() for override by thirdparty, as it is not yet possible to completely replace current creation logic without breaking code that is not covered with tests.

Manual testing scenarios

Run integration tests with Import/Export related functionality
Run unit tests with Import/Export related functionality

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)

…CSV parser implementation

…bjectManager for test harness

…entations as depracated

…ort-empty-row-support

koenner01 · 2017-06-21T09:09:44Z

Hey IvanChepurnyi, thanks for incorporating the fix for my reported issue.
I have just found a troublesome situation when using the empty value logic (#7468 (comment)).

Just wanted to let you know in case you might run into a similiar problem

IvanChepurnyi · 2017-06-21T10:43:51Z

@koenner01 did you checked it with "null" option passed into CSV Parser Factory and then used this as an empty value for a special price?

maghamed

My main concerns for this Pull Request:

Incorrect following of Backward Compatibility policies (new Interfaces should be added independently to legacy code). No need to mix up legacy and new implementation in one class.
Not proper work with Exceptions. Sometimes Exceptions substituted with FALSE value and vice versa.
Not proper Object design for factories and parser instantiation
Re-implemented some core functions responsible for work with filesystem
Lot of public getters are introduced just for testing purposes (violating object encapsulation)
All existing Unit tests are actually Integration ones, in terms of Magento 2

maghamed · 2017-07-14T13:16:21Z

app/code/Magento/ImportExport/Controller/Adminhtml/Import/Validate.php

+     * @param $data
+     * @return array
+     */
+    protected function getSourceAdapterOptions($data)


we don't recommend to use Inheritance Based API, so all the methods should be whether public or private

maghamed · 2017-07-14T13:19:49Z

app/code/Magento/ImportExport/Model/Import.php

+     *
+     * @return array
+     */
+    protected function getSourceAdapterOptions()


we don't recommend to use Inheritance Based API, so all the methods should be whether public or private

maghamed · 2017-07-14T13:20:33Z

app/code/Magento/ImportExport/Model/Import/Adapter.php

+    /** @var FileFactory */
+    private $fileSourceFactory;
+
+    public function __construct(FileFactory $fileSourceFactory)


doc block absent for this constructor

What do you see as a comment for a constructor?

DocBlock is not just comment, but specification of all input types.

maghamed · 2017-07-14T13:26:24Z

app/code/Magento/ImportExport/Model/Import/Adapter.php

@@ -25,29 +38,12 @@ class Adapter
     * @return AbstractSource
     *
     * @throws \Magento\Framework\Exception\LocalizedException
+     * @deprecated
+     * @see Adapter::createSourceByPath()


It's better to avoid mixing up Deprecated logic and Newly created one in one class.
Thus, we recommend to introduce new Factory entity and mark it with @ api.
Current class to mark as @ deprecated.

Inject newly created factory as DI injection to Adapter class.
And make current class to proxy calls to newly created API

maghamed · 2017-07-14T13:26:56Z

app/code/Magento/ImportExport/Model/Import/Adapter.php

+    /** @var FileFactory */
+    private $fileSourceFactory;
+
+    public function __construct(FileFactory $fileSourceFactory)


Violation of BC policies, all the parameters should be optional (nullable)

maghamed · 2017-07-14T16:37:22Z

app/code/Magento/ImportExport/Test/Unit/Model/Import/Source/FileFactoryTest.php

+        $fileFactory->createFromFilePath('test.csv');
+    }
+
+    public function testGivenParserFactoryConfiguredWhenSourceIsCreatedByPathThenRightInstanceIsCreated()


it's not so easy to read this method name.
It's like 10 years ago when SMS cost a lot, you try to save space (140 symbols) for text not using spaces at all.

Method name looks like this because "_" are prohibited by Magento code style guideline. Previous versions had "_" as space.

maghamed · 2017-07-14T16:41:22Z

app/code/Magento/ImportExport/Test/Unit/Model/Import/Source/FileParser/FakeParserFactory.php

+use Magento\ImportExport\Model\Import\Source\FileParser\ParserFactoryInterface;
+use Magento\ImportExport\Model\Import\Source\FileParser\UnsupportedPathException;
+
+class FakeParserFactory implements ParserFactoryInterface


We have so many Fake objects here, and we need to update their code updating code of the real objects as well

Currently, we don't use this approach for Unit testing. And definitely it needs separate approval from AC

Because for now we introduced additional entities to maintain. But why do we need that if we already have Integration tests?

maghamed · 2017-07-14T16:42:53Z

app/code/Magento/ImportExport/Test/Unit/Model/Import/Source/FileParser/_files/test_options.csv

@@ -0,0 +1 @@
+column1|;column2;|column3


why do we need to use CSV files for Unit testing?

Integration tests are made for these purposes

There are no component level integration tests in Magento 2 repository and the one you are referring to requires to work with real database and other components enabled. However, this test case is quite isolated and does not leave any side effects visible to other test cases. Unit scope for this one is parsing a ZIP file, but integration test is more about testing integration of components versus just I/O operation.

Would love to get an advice how to overcome a problem of running fast "integration" test witout bootstrapping all database and other components.

maghamed · 2017-07-14T16:43:53Z

app/code/Magento/ImportExport/Model/Import/Source/FileParser/CompositeParserFactory.php

+namespace Magento\ImportExport\Model\Import\Source\FileParser;
+
+/**
+ * File Parser Composite


should be marked as @ api , because used as an Extension Point

maghamed · 2017-07-14T16:49:43Z

app/code/Magento/ImportExport/Test/Unit/Model/Import/Source/FileParser/FakeObjectManager.php

+            $resolvedArguments[] = $arguments[$parameter->getName()] ?? $parameter->getDefaultValue();
+        }
+
+        return $resolvedArguments;


when we provide an implementation of Fake objects, who and how will test this implementation?

For example, current function is 10 lines of code. How can we make sure that this code is safe to rely upon?

I will add a test case for it, and also probably move it into framework, as FakeObjectManager must be available for other factory instantiation in the test harness as well.

maghamed

My main concerns for this Pull Request:

Violation of Backward compatibility policies. No need to mix up the legacy (deprecated) code with newly created APIs. It's better to segregate classes.
There are many public getters added just for testing purposes and not used in code of business logic (violation of object encapsulation)
Not proper work with exceptions. Some of the exceptions are substituted with FALSE, and vice versa
Not proper Object Oriented design for ParserFactory instantiation
Some core functions for working with Filesystem are re-implemented on PHP level. Which is incorrect
All Unit tests looks like Integration tests in terms of Magento 2
Lot of Fake objects are introduced. These fake objects contain business logic, which is not covered with any kind of test. These fake objects should be also maintained in future

ishakhsuvarov · 2017-08-17T12:14:15Z

@IvanChepurnyi @maghamed
Is there any activity on this PR at the moment or shall I close it for now?

ishakhsuvarov · 2017-08-23T08:51:28Z

@IvanChepurnyi @maghamed I am closing this PR for now due to inactivity. Please reopen if you wish to continue work on it.
Thanks

PieterCappelle · 2017-10-10T19:36:59Z

Hi @IvanChepurnyi, we are talking about this PR in the Magento2 Slack Channel. Is it possible to join this channel and join the channel #import_export. Thanks!

IvanChepurnyi added 16 commits April 2, 2017 21:56

Add file parser interface to support various import formats, created …

d948d42

…CSV parser implementation

Remove wrong license header and specify proper one

8b8bcae

New parser implementation with composite

8c86c17

Move CSV parser instantiation via ObjectManager in factory, add FakeO…

0a898be

…bjectManager for test harness

Give up copyright to Magento Inc.

9b542f6

Rename variable in UnsupportedPathExceptionTest to add more consistency

43df7ef

Add ZipParserFactory composite implementation

ff04479

Remove redundant properties from CSVParser

2488330

Remove un-used fixture

2088f55

Add relative file path resolution for CSV and ZIP parser factories

99d6a17

Add file import source implementation based on FileParser

71c5288

Replace Adapter implementation with FileFactory, mark previous implem…

2ff1d0f

…entations as depracated

Fix braces and spaces for PSR2 standard compliency

e8d21fc

Add extension points for source options retrieval

1de2b68

Add default value to extension point

f654f06

Merge remote-tracking branch 'upstream/develop' into 7468-catalog-imp…

b1826bc

…ort-empty-row-support

maghamed self-assigned this May 3, 2017

maghamed self-requested a review May 3, 2017 15:09

IvanChepurnyi added 2 commits May 18, 2017 11:46

Fix static code analysis issues with test code

e5c6327

Improve readability of ZipParserFactoryTest

6b93516

okorshenko unassigned maghamed Jul 12, 2017

maghamed approved these changes Jul 14, 2017

View reviewed changes

maghamed suggested changes Jul 14, 2017

View reviewed changes

maghamed self-assigned this Jul 17, 2017

ishakhsuvarov added the Progress: needs update label Jul 18, 2017

okorshenko added this to the July 2017 milestone Jul 20, 2017

okorshenko added the develop label Jul 20, 2017

okorshenko removed this from the July 2017 milestone Aug 1, 2017

okorshenko modified the milestones: August 2017, July 2017 Aug 1, 2017

ishakhsuvarov closed this Aug 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored Source For ImportExport module, related to #7469 #9480

Refactored Source For ImportExport module, related to #7469 #9480

IvanChepurnyi commented May 3, 2017 •

edited by okorshenko

Loading

koenner01 commented Jun 21, 2017

IvanChepurnyi commented Jun 21, 2017

maghamed left a comment •

edited

Loading

maghamed Jul 14, 2017

maghamed Jul 14, 2017

maghamed Jul 14, 2017

IvanChepurnyi Jul 14, 2017

maghamed Jul 14, 2017

maghamed Jul 14, 2017

maghamed Jul 14, 2017

maghamed Jul 14, 2017

IvanChepurnyi Jul 14, 2017 •

edited

Loading

maghamed Jul 14, 2017

maghamed Jul 14, 2017

IvanChepurnyi Jul 14, 2017

IvanChepurnyi Jul 14, 2017

maghamed Jul 14, 2017

maghamed Jul 14, 2017

IvanChepurnyi Jul 14, 2017 •

edited

Loading

maghamed left a comment •

edited

Loading

ishakhsuvarov commented Aug 17, 2017

ishakhsuvarov commented Aug 23, 2017

PieterCappelle commented Oct 10, 2017

		@@ -0,0 +1 @@
		column1\|;column2;\|column3

Refactored Source For ImportExport module, related to #7469 #9480

Refactored Source For ImportExport module, related to #7469 #9480

Conversation

IvanChepurnyi commented May 3, 2017 • edited by okorshenko Loading

Description

Fixed Issues

Manual testing scenarios

Contribution checklist

koenner01 commented Jun 21, 2017

IvanChepurnyi commented Jun 21, 2017

maghamed left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanChepurnyi Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanChepurnyi Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

maghamed left a comment • edited Loading

Choose a reason for hiding this comment

ishakhsuvarov commented Aug 17, 2017

ishakhsuvarov commented Aug 23, 2017

PieterCappelle commented Oct 10, 2017

IvanChepurnyi commented May 3, 2017 •

edited by okorshenko

Loading

maghamed left a comment •

edited

Loading

IvanChepurnyi Jul 14, 2017 •

edited

Loading

IvanChepurnyi Jul 14, 2017 •

edited

Loading

maghamed left a comment •

edited

Loading