Validate Inputs for activate/track/getVariation (#105)

optimizely · Jul 2, 2018 · 29db920 · 29db920
1 parent a9156d0
commit 29db920
Show file tree

Hide file tree

Showing 5 changed files with 243 additions and 5 deletions.
diff --git a/src/Optimizely/Optimizely.php b/src/Optimizely/Optimizely.php
@@ -36,6 +36,7 @@
 use Optimizely\Notification\NotificationCenter;
 use Optimizely\Notification\NotificationType;
 use Optimizely\UserProfile\UserProfileServiceInterface;
+use Optimizely\Utils\Errors;
 use Optimizely\Utils\Validator;
 use Optimizely\Utils\VariableTypeUtils;
 
@@ -46,6 +47,10 @@
  */
 class Optimizely
 {
+    const USER_ID = 'User ID';
+    const EVENT_KEY = 'Event Key';
+    const EXPERIMENT_KEY ='Experiment Key';
+
     /**
      * @var ProjectConfig
      */
@@ -288,6 +293,16 @@ public function activate($experimentKey, $userId, $attributes = null)
             return null;
         }
 
+        if (!$this->validateInputs(
+            [
+                self::EXPERIMENT_KEY =>$experimentKey,
+                self::USER_ID => $userId
+            ]
+        	)
+        ) {
+            return null;
+        }
+
         $variationKey = $this->getVariation($experimentKey, $userId, $attributes);
         if (is_null($variationKey)) {
             $this->_logger->log(Logger::INFO, sprintf('Not activating user "%s".', $userId));
@@ -314,6 +329,16 @@ public function track($eventKey, $userId, $attributes = null, $eventTags = null)
             return;
         }
 
+        if (!$this->validateInputs(
+            [
+                self::EVENT_KEY =>$eventKey,
+                self::USER_ID => $userId
+            ]
+        	)
+        ) {
+            return null;
+        }
+
         if (!$this->validateUserInputs($attributes, $eventTags)) {
             return;
         }
@@ -401,6 +426,16 @@ public function getVariation($experimentKey, $userId, $attributes = null)
             return null;
         }
 
+        if (!$this->validateInputs(
+            [
+                self::EXPERIMENT_KEY =>$experimentKey,
+                self::USER_ID => $userId
+            ]
+        	)
+        ) {
+            return null;
+        }
+
         $experiment = $this->_config->getExperimentFromKey($experimentKey);
 
         if (is_null($experiment->getKey())) {
@@ -728,4 +763,27 @@ public function getFeatureVariableString($featureFlagKey, $variableKey, $userId,
 
         return $variableValue;
     }
+
+    /**
+    * Calls Validator::validateNonEmptyString for each value in array
+    * Logs for each invalid value
+    *
+    * @param array values to validate
+    * @param logger
+    *
+    * @return bool True if all of the values are valid, False otherwise
+    */
+    protected function validateInputs(array $values, $logLevel = Logger::ERROR)
+    {
+        $isValid = true;
+        foreach ($values as $key => $value) {
+            if (!Validator::validateNonEmptyString($value)) {
+                $isValid = false;
+                $message = sprintf(Errors::INVALID_FORMAT, $key);
+                $this->_logger->log($logLevel, $message);
+            }
+        }
+
+        return $isValid;
+    }
 }
diff --git a/src/Optimizely/Utils/Errors.php b/src/Optimizely/Utils/Errors.php
@@ -0,0 +1,22 @@
+<?php
+/**
+ * Copyright 2018, Optimizely
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+namespace Optimizely\Utils;
+
+class Errors
+{
+    const INVALID_FORMAT = 'Provided %s is in an invalid format.';
+}
diff --git a/src/Optimizely/Utils/Validator.php b/src/Optimizely/Utils/Validator.php
@@ -1,6 +1,6 @@
 <?php
 /**
- * Copyright 2016-2017, Optimizely
+ * Copyright 2016-2018, Optimizely
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -137,4 +137,20 @@ public static function isFeatureFlagValid($config, $featureFlag)
 
         return true;
     }
+
+    /**
+     * Checks if the provided value is a non-empty string
+     *
+     * @param $value The value to validate
+     *
+     * @return boolean True if $value is a non-empty string
+     */
+    public static function validateNonEmptyString($value)
+    {
+        if (is_string($value) && $value!='') {
+            return true;
+        }
+
+        return false;
+    }
 }
diff --git a/tests/OptimizelyTest.php b/tests/OptimizelyTest.php
@@ -179,6 +179,29 @@ public function testActivateInvalidOptimizelyObject()
         $this->expectOutputRegex('/Datafile has invalid format. Failing "activate"./');
     }
 
+    public function testActivateCallsValidateInputs()
+    {
+        $optimizelyMock = $this->getMockBuilder(Optimizely::class)
+            ->setConstructorArgs(array($this->datafile))
+            ->setMethods(array('validateInputs'))
+            ->getMock();
+
+        $experimentKey = 'test_experiment';
+        $userId = 'test_user';
+        $inputArray = [
+            Optimizely::EXPERIMENT_KEY => $experimentKey,
+            Optimizely::USER_ID => $userId
+        ];
+
+        // assert that validateInputs gets called with exactly same keys
+        $optimizelyMock->expects($this->once())
+            ->method('validateInputs')
+            ->with($inputArray)
+            ->willReturn(false);
+
+        $this->assertNull($optimizelyMock->activate($experimentKey, $userId));
+    }
+
     public function testActivateInvalidAttributes()
     {
         $this->loggerMock->expects($this->exactly(2))
@@ -460,6 +483,29 @@ public function testGetVariationInvalidOptimizelyObject()
         $this->expectOutputRegex('/Datafile has invalid format. Failing "getVariation"./');
     }
 
+    public function testGetVariationCallsValidateInputs()
+    {
+        $optimizelyMock = $this->getMockBuilder(Optimizely::class)
+            ->setConstructorArgs(array($this->datafile))
+            ->setMethods(array('validateInputs'))
+            ->getMock();
+
+        $experimentKey = 'test_experiment';
+        $userId = 'test_user';
+        $inputArray = [
+            Optimizely::EXPERIMENT_KEY => $experimentKey,
+            Optimizely::USER_ID => $userId
+        ];
+
+        // assert that validateInputs gets called with exactly same keys
+        $optimizelyMock->expects($this->once())
+            ->method('validateInputs')
+            ->with($inputArray)
+            ->willReturn(false);
+
+        $this->assertNull($optimizelyMock->getVariation($experimentKey, $userId));
+    }
+
     public function testGetVariationInvalidAttributes()
     {
         $this->loggerMock->expects($this->once())
@@ -679,6 +725,29 @@ public function testTrackInvalidOptimizelyObject()
         $this->expectOutputRegex('/Datafile has invalid format. Failing "track"./');
     }
 
+    public function testTrackCallsValidateInputs()
+    {
+        $optimizelyMock = $this->getMockBuilder(Optimizely::class)
+            ->setConstructorArgs(array($this->datafile))
+            ->setMethods(array('validateInputs'))
+            ->getMock();
+
+        $eventKey = 'test_event';
+        $userId = 'test_user';
+        $inputArray = [
+            Optimizely::EVENT_KEY => $eventKey,
+            Optimizely::USER_ID => $userId
+        ];
+
+        // assert that validateInputs gets called with exactly same keys
+        $optimizelyMock->expects($this->once())
+            ->method('validateInputs')
+            ->with($inputArray)
+            ->willReturn(false);
+
+        $this->assertNull($optimizelyMock->track($eventKey, $userId));
+    }
+
     public function testTrackInvalidAttributes()
     {
         $this->loggerMock->expects($this->once())
@@ -1632,14 +1701,17 @@ public function testSetForcedVariationWithInvalidUserIDs()
             'location' => 'San Francisco'
         ];
 
-        // null user ID --> set should fail, normal bucketing  [TODO (Alda): getVariation on a null userID should return null]
+        // null user ID should fail setForcedVariation. getVariation on a null userID should return null
         $this->assertFalse($optlyObject->setForcedVariation('test_experiment', null, 'bad_variation'), 'Set variation for null user ID should have failed.');
+
         $variationKey = $optlyObject->getVariation('test_experiment', null, $userAttributes);
-        $this->assertEquals('variation', $variationKey, sprintf('Invalid variation "%s" for null user ID.', $variationKey));
-        // empty string user ID --> set should fail, normal bucketing [TODO (Alda): getVariation on an empty userID should return null]
+        $this->assertNull($variationKey);
+
+        // empty string user ID should fail setForcedVariation. getVariation on an empty userID should return null
         $this->assertFalse($optlyObject->setForcedVariation('test_experiment', '', 'bad_variation'), 'Set variation for empty string user ID should have failed.');
+
         $variationKey = $optlyObject->getVariation('test_experiment', '', $userAttributes);
-        $this->assertEquals('variation', $variationKey, sprintf('Invalid variation "%s" for empty string user ID.', $variationKey));
+        $this->assertNull($variationKey);
     }
 
     public function testSetForcedVariationWithInvalidExperimentKeys()
@@ -2850,4 +2922,66 @@ public function testSendImpressionEventWithAttributes()
 
         $optlyObject->sendImpressionEvent('test_experiment', 'control', 'test_user', $userAttributes);
     }
+
+    /*
+    * test ValidateInputs method validates and logs for different and multiple keys
+    */
+    public function testValidateInputs()
+    {
+        $optlyObject = new OptimizelyTester($this->datafile, null, $this->loggerMock);
+
+        $INVALID_USER_ID_LOG = 'Provided User ID is in an invalid format.';
+        $INVALID_EVENT_KEY_LOG = 'Provided Event Key is in an invalid format.';
+        $INVALID_RANDOM_KEY_LOG = 'Provided ABCD is in an invalid format.';
+
+        $this->loggerMock->expects($this->at(0))
+            ->method('log')
+            ->with(Logger::ERROR, $INVALID_USER_ID_LOG);
+        $this->assertFalse($optlyObject->validateInputs([Optimizely::USER_ID => null]));
+
+        $this->loggerMock->expects($this->at(0))
+            ->method('log')
+            ->with(Logger::ERROR, $INVALID_RANDOM_KEY_LOG);
+        $this->assertFalse($optlyObject->validateInputs(['ABCD' => null]));
+
+        // Verify that multiple messages are logged.
+        $this->loggerMock->expects($this->at(0))
+            ->method('log')
+            ->with(Logger::ERROR, $INVALID_EVENT_KEY_LOG);
+
+        $this->loggerMock->expects($this->at(1))
+            ->method('log')
+            ->with(Logger::ERROR, $INVALID_USER_ID_LOG);
+        $this->assertFalse($optlyObject->validateInputs([Optimizely::EVENT_KEY => null, Optimizely::USER_ID => null]));
+
+        // Verify that logger level is taken into account
+        $this->loggerMock->expects($this->at(0))
+            ->method('log')
+            ->with(Logger::INFO, $INVALID_RANDOM_KEY_LOG);
+        $this->assertFalse($optlyObject->validateInputs(['ABCD' => null], Logger::INFO));
+
+        // Verify that it returns true and nothing is logged if valid values given
+        $this->loggerMock->expects($this->never())
+            ->method('log');
+        $this->assertTrue($optlyObject->validateInputs([Optimizely::EVENT_KEY => 'test_event', Optimizely::USER_ID => 'test_user']));
+    }
+
+    /*
+    * test ValidateInputs method only returns true for non-empty string
+    */
+    public function testValidateInputsWithDifferentValues()
+    {
+        $optlyObject = new OptimizelyTester($this->datafile);
+
+        $this->assertTrue($optlyObject->validateInputs(['key' => '0']));
+        $this->assertTrue($optlyObject->validateInputs(['key' => 'test_user']));
+
+        $this->assertFalse($optlyObject->validateInputs(['key' => '']));
+        $this->assertFalse($optlyObject->validateInputs(['key' => null]));
+        $this->assertFalse($optlyObject->validateInputs(['key' => false]));
+        $this->assertFalse($optlyObject->validateInputs(['key' => true]));
+        $this->assertFalse($optlyObject->validateInputs(['key' => 2]));
+        $this->assertFalse($optlyObject->validateInputs(['key' => 2.0]));
+        $this->assertFalse($optlyObject->validateInputs(['key' => array()]));
+    }
 }
diff --git a/tests/TestData.php b/tests/TestData.php
@@ -17,6 +17,9 @@
 namespace Optimizely\Tests;
 
 use Exception;
+
+use Monolog\Logger;
+
 use Optimizely\Bucketer;
 use Optimizely\Event\Dispatcher\EventDispatcherInterface;
 use Optimizely\Event\LogEvent;
@@ -797,6 +800,11 @@ public function sendImpressionEvent($experimentKey, $variationKey, $userId, $att
     {
         parent::sendImpressionEvent($experimentKey, $variationKey, $userId, $attributes);
     }
+
+    public function validateInputs(array $values, $logLevel = Logger::ERROR)
+    {
+        return parent::validateInputs($values, $logLevel);
+    }
 }
 
 class FireNotificationTester