Update Configuration class to support merging objects in local config files #782

ryanrath · 2019-01-23T20:07:09Z

Description

Updated Configuration\Configuration w/ the ability to merge objects. This is
so that Configuration\Configuration can eventually replace Config.
Added tests to EtlConfigurationTest that ensure
Configuration\Configuration can generate the same json as Config.
- The reason these tests have been placed here instead of ConfigurationTest
  is documented here:
  https://app.asana.com/0/807629084565719/1101232922862525/f
Updated the unit tests bootstrap.php so that we can use the
integration_tests TestFiles helper class.

Motivation and Context

Config currently supports a couple of features that Configuration\Configuration does not.

Merging .d configuration files that contain '+' prefixed property names
Handling the 'extends' key / value pair
Retrieving which configuration options have been provided by which module.

This PR implements the first of these features ( Merging '+' prefixed properties ). Future PR's will address the remaining features.

Tests performed

All existing tests were performed:

unit
component
integration
regression
UI

New tests were added to ensure, on a basic level, that Configuration can generate the same output as Config when presented with the same base configuration files. In this case both roles.json and datawarehouse.json are tested.

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.

- Updated `Configuration\Configuration` w/ the ability to merge objects. This is so that `Configuration\Configuration` can eventually replace `Config`. - Added tests to `EtlConfigurationTest` that ensure `Configuration\Configuration` can generate the same json as `Config`. - The reason these tests have been placed here instead of `ConfigurationTest` is documented here: https://app.asana.com/0/807629084565719/1101232922862525/f - Updated the unit tests `bootstrap.php` so that we can use the integration_tests `TestFiles` helper class.

smgallo · 2019-01-23T20:47:01Z

classes/Configuration/Configuration.php

            } else {
-                $this->logger->debug("Skip duplicate key in local config (overwrite == false)");
+                $msg = <<<TXT
+Unable to merge files due to mismatched types.


The files will still be merged per se, but we want to warn the user so they can fix the issue. This can be made more succinct as:

sprintf( "Type mismatch. Unable to merge local value for key '%s' (type: %s) with global value (type: %s) from local file %s", $k, gettype($v), gettype($this->transformedConfig->$k), $localConfigObj->getFilename() );

smgallo · 2019-01-23T20:48:25Z

classes/Configuration/Configuration.php

@@ -490,12 +490,38 @@ protected function merge(Configuration $localConfigObj, $overwrite = false)
        // it.

        foreach ( $localConfigObj->getTransformedConfig() as $k => $v ) {
+
+            // Normalize incoming keys starting w/ '+'
+            if (substr($k, 0, 1) === '+') {


This is specific to the XDMoD configuration files rather than the generic config files. Let's handle this using a iConfigFileKeyTransformer then we can assume that it has been stripped before we get to this stage.

*facepalm* yeah, completely forgot about that part of the convo. I'll add the transformer and push it up shortly.

You should be able to use the CommentTransformer with minimal changes to suit this.

smgallo · 2019-01-23T20:50:51Z

classes/Configuration/Configuration.php

+    {
+        $properties = get_object_vars($incoming);
+        foreach($properties as $property => $incomingValue) {
+            if (strpos($property, '+') !== false) {


If we use a transformer as mentioned above, we can get rid of this check.

smgallo · 2019-01-23T20:58:34Z

classes/Configuration/Configuration.php

+                } elseif (is_array($existingValue) && is_array($incomingValue)) {
+                    $existing->$property = array_merge($existingValue, $incomingValue);
+                } else {
+                    $existing->$property = $incomingValue;


Don't we need the check that both of these values are scalars? Otherwise if the existing value is an object and the incoming value is a scalar we will hit this clause. We should provide a warning here as well if we have a type mismatch.

Actually, if we fix up this method can't we just call it from the merge() method and get rid of the if-then-else there?

I think we still need to be able to handle types other than Object at the top level but it's entirely possible I'm missing something.

- Added the `PlusKeyTransformer.php` to handle '+' prefixed keys instead of handling it in the merge step of `Configuration`. - Updated `Configuration::merge` so that it: - Only allows merging of like-type values per key - Recursively merges objects

smgallo · 2019-01-25T17:14:41Z

classes/Configuration/PlusKeyTransformer.php

@@ -0,0 +1,38 @@
+<?php
+


We need a docblock here explaining what the plus key represents and how we're transforming it (e.g., why it is being removed - because merge is the default behavior of Configuration).

smgallo · 2019-01-25T17:15:38Z

classes/Configuration/Configuration.php

@@ -390,6 +390,7 @@ protected function preTransformTasks()
    {
        $this->addKeyTransformer(new CommentTransformer($this->logger));
        $this->addKeyTransformer(new JsonReferenceTransformer($this->logger));
+        $this->addKeyTransformer(new PlusKeyTransformer($this->logger));


Is there a better name for this transformer that hints at its function?

lol I'm sure there is, I honestly wasn't sure what to go with. Maybe, MergeKeyTransformer or something similar?

Maybe StripMergePrefixTransformer ?

That sounds good. I'll update the name + add the docblock in the next commit.

- Renamed `PlusKeyTransformer` -> `StripMergePrefixTransformer` - Added a doc block to `StripMergePrefixTransformer`

ryanrath added 3 commits January 23, 2019 14:28

Adding Required Test Artifacts

4cd8423

Adding newlines per travis

5f039d5

ryanrath requested a review from smgallo January 23, 2019 20:07

ryanrath added 3 commits January 23, 2019 15:44

Account for adding a newline @ the end of json files

9edf843

Ensure generated json files have newline @ the end

69f85ad

Derrrrr

617a204

smgallo suggested changes Jan 23, 2019

View reviewed changes

ryanrath added 2 commits January 25, 2019 11:16

Adding newline

02c7f5a

smgallo reviewed Jan 25, 2019

View reviewed changes

Updates per @smgallo

92c5900

- Renamed `PlusKeyTransformer` -> `StripMergePrefixTransformer` - Added a doc block to `StripMergePrefixTransformer`

smgallo added this to the 8.1.0 milestone Jan 25, 2019

smgallo added enhancement Enhancement of the functionality of an existing feature Category:ETL Extract Transform Load labels Jan 25, 2019

smgallo approved these changes Jan 25, 2019

View reviewed changes

Merge branch 'xdmod8.1' into refactor_config

5d3ff5a

ryanrath merged commit 7fc58d1 into ubccr:xdmod8.1 Jan 25, 2019

smgallo changed the title ~~Updating Configuration class to support merging objects w/ Tests~~ Updating Configuration class to support merging objects Apr 10, 2019

smgallo changed the title ~~Updating Configuration class to support merging objects~~ Update Configuration class to support merging objects Apr 10, 2019

smgallo changed the title ~~Update Configuration class to support merging objects~~ Update Configuration class to support merging objects in local config files Apr 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Configuration class to support merging objects in local config files #782

Update Configuration class to support merging objects in local config files #782

ryanrath commented Jan 23, 2019

smgallo Jan 23, 2019

smgallo Jan 23, 2019

ryanrath Jan 23, 2019

smgallo Jan 23, 2019

smgallo Jan 23, 2019

smgallo Jan 23, 2019

smgallo Jan 23, 2019

ryanrath Jan 25, 2019

smgallo Jan 25, 2019

smgallo Jan 25, 2019

ryanrath Jan 25, 2019

smgallo Jan 25, 2019

ryanrath Jan 25, 2019

Update Configuration class to support merging objects in local config files #782

Update Configuration class to support merging objects in local config files #782

Conversation

ryanrath commented Jan 23, 2019

Description

Motivation and Context

Tests performed

Types of changes

Checklist:

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

Choose a reason for hiding this comment