Allow DataEndpoint singleton to auto-discover available data endpoints #157

smgallo · 2017-06-12T20:40:37Z

Allow DataEndpoint singleton to auto-discover available data endpoints

Description

Rather than require a developer to modify ETL\DataEndpoint.php and explicitly list each available data endpoint and it's configuration code, we now allow the DataEndpoint singleton to automatically discover what data endpoints are available. As per PSR-4 the contiguous sub-namespace names after the "namespace prefix" (ETL in our case) correspond to a subdirectory within a "base directory", in which the namespace separators represent directory separators. From this, we can assume the ETL\DataEndpoint sub-namespace is in the DataEndpoint subdirectory relative to where the DataEndpoint singleton class is defined. Using this knowledge we use reflection to recursively examine each file in the DataEndpoint subdirectory and any class implementing iDataEndpoint and also defining an ENDPOINT_NAME constant will be a valid endpoint definition. The ENDPOINT_NAME constant defines the endpoint name to be used in configuration files. Conflicts in endpoint names are logged.

The etl_overseer script has been modified to list the available endpoints:

$ php etl_overseer.php -c ../../../etc/etl/etl.json -l endpoint-types                       
Name    Class
directoryscanner        ETL\DataEndpoint\DirectoryScanner
file    ETL\DataEndpoint\File
jsonfile        ETL\DataEndpoint\JsonFile
mysql   ETL\DataEndpoint\Mysql
oracle  ETL\DataEndpoint\Oracle
postgres        ETL\DataEndpoint\Postgres
rest    ETL\DataEndpoint\Rest

Motivation and Context

Make it easier to define data endpoints.

Tests performed

Ran existing tests, several of which use the File and Json endpoints.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project as found in the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

ryanrath

Just a few minor misspellings / questions is all.

ryanrath · 2017-06-15T18:08:58Z

classes/ETL/DataEndpoint.php

+     * @var string
+     */
+
+    private static $dataEndpointRelativeNs = 'DataEndpoint';


Is it intended that these be changed at some point during the classes life time? Just wanting to understand the choice of private static vs const. This same question applies to $dataEndpointRequiredInterface and $endpointNameConstant

Actually, no they don't change programmatically so it does make sense for them to be constants. I'll make that change.

ryanrath · 2017-06-15T18:16:04Z

classes/ETL/DataEndpoint.php

+    }  // getDataEndpointInfo()
+
+    /** -----------------------------------------------------------------------------------------
+     * Discover the list of currently supported data endpoints and constuct a list mapping


s/constuct/construct

ryanrath · 2017-06-15T18:18:26Z

classes/ETL/DataEndpoint.php

+        // file resides represent sub-namespaces.
+
+        // The endpoint directory is relative to the directory where this file is found
+        $endpointDir =  dirname(__FILE__) . '/' . strtr(self::$dataEndpointRelativeNs, '\\', '/');


More for myself than anything else are hard coded '/' cool as opposed to using say, DIRECTORY_SEPARATOR?

DIRECTORY_SEPARATOR is a more portable way to specify it (not that it matters on the platforms that we support). I'll make that change as well.

ryanrath · 2017-06-15T18:54:56Z

classes/ETL/DataEndpoint.php


 class DataEndpoint
 {
+    /**
+     * Namesapce, relative to the current namespace, where data endpoint classes are


s/Namesapce/Namespace

ryanrath · 2017-06-15T18:58:23Z

classes/ETL/DataEndpoint.php

+    /** -----------------------------------------------------------------------------------------
+     * Discover the list of currently supported data endpoints and constuct a list mapping
+     * their names to the classes that implement them.  All data endpoints must implement
+     * the interface specified in self::$dataEndpointRequiredInterface.  By automatically


Should either mention that they also need to define the const ENDPOINT_NAME or add a function to iDataEndpoint that returns this value so that all that is required is to implement the interface.

ryanrath · 2017-06-15T19:00:55Z

classes/ETL/DataEndpoint.php

-     * @param $options A DataEndpointOptions object containing options parsed from the ETL config.
+     * @param DataEndpointOptions $options A DataEndpointOptions object containing options
+     *   parsed from the ETL config.
+     * @param Log $logger A PEAR Log object or null to use the null logger.
     *
     * @return A data endpoint object implementing the iDataEndpoint interface.


missing a return type

ryanrath · 2017-06-15T19:15:41Z

classes/ETL/DataEndpoint/DirectoryScanner.php

+            $dotDirFilterIterator = new \CallbackFilterIterator(
+                $iterator,
+                function ($current, $key, $iterator) {
+                    if ( $iterator->isDot() ) {


This can be simplified to return ! $iterator->isDot();

Good points.

ryanrath · 2017-06-15T19:29:42Z

classes/ETL/DataEndpoint/DirectoryScanner.php

+            );
+        }
+
+        if ( null !== $this->maxRecursionDepth ) {


May want to move this line to 464 into the try above as all of these operations depend on $flattenedIterator existing. It would also then conform to the pattern seen elsewhere in the code of: try block that creates new iterator, maybe does a few things and ultimately sets iterator = new iterator

ryanrath

Looks good to me

ubccr#157) * Remove commented out code, improve control structure flow * Allow DataEndpoint to auto-discover classes implementing iDataEndpoint * Add overseer option to list available data endpoint types

smgallo added 3 commits June 12, 2017 16:27

Remove commented out code, improve control structure flow

4687f32

Allow DataEndpoint to auto-discover classes implementing iDataEndpoint

495daac

Add overseer option to list available data endpoint types

29292af

smgallo added enhancement Enhancement of the functionality of an existing feature Category:ETL Extract Transform Load labels Jun 12, 2017

smgallo added this to the v7.0.0 milestone Jun 12, 2017

smgallo requested review from plessbd, jpwhite4 and tyearke June 12, 2017 20:40

smgallo added 2 commits June 12, 2017 22:40

Satisfy the PHP 5.3 demons

c5767ed

PHP 5.3 hates me

6370679

smgallo requested a review from ryanrath June 13, 2017 13:43

ryanrath reviewed Jun 15, 2017

View reviewed changes

smgallo added 2 commits June 15, 2017 16:11

Changes as per @ryanrath code review

f047a09

Fix comment grammar

e6dfe12

ryanrath approved these changes Jun 19, 2017

View reviewed changes

smgallo merged commit 9800e7c into ubccr:xdmod7.0 Jun 19, 2017

smgallo deleted the etl/auto-discover-data-endpoints branch June 19, 2017 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow DataEndpoint singleton to auto-discover available data endpoints #157

Allow DataEndpoint singleton to auto-discover available data endpoints #157

smgallo commented Jun 12, 2017

ryanrath left a comment

ryanrath Jun 15, 2017

smgallo Jun 15, 2017

ryanrath Jun 15, 2017

ryanrath Jun 15, 2017

smgallo Jun 15, 2017

ryanrath Jun 15, 2017

ryanrath Jun 15, 2017

ryanrath Jun 15, 2017

ryanrath Jun 15, 2017

smgallo Jun 15, 2017

ryanrath Jun 15, 2017

ryanrath left a comment

Allow DataEndpoint singleton to auto-discover available data endpoints #157

Allow DataEndpoint singleton to auto-discover available data endpoints #157

Conversation

smgallo commented Jun 12, 2017

Description

Motivation and Context

Tests performed

Types of changes

Checklist:

ryanrath left a 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

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

ryanrath left a comment

Choose a reason for hiding this comment