From 7d5f6bab7548bbec9e687a76a1ace59ce5f8ed2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Tue, 1 Oct 2024 13:45:36 +0200 Subject: [PATCH] fix: support both snake_case and camelCase versions of impression data (#227) Impression data should have always been camelCase in unleash-edge. It was not by accident, and the php sdk is using the snake_case version which prevents users from upgrading to newer versions of edge where impression_data was changed to camelCase. Fixes #226 --- src/DTO/DefaultProxyFeature.php | 12 +- .../DefaultUnleashProxyRepository.php | 11 +- tests/DTO/DefaultProxyFeatureTest.php | 23 ++- tests/DefaultProxyUnleashTest.php | 16 +- .../DefaultUnleashProxyRepositoryTest.php | 4 +- ...ositoryWithSnakeCaseImpressionDataTest.php | 170 ++++++++++++++++++ 6 files changed, 218 insertions(+), 18 deletions(-) create mode 100644 tests/Repository/DefaultUnleashProxyRepositoryWithSnakeCaseImpressionDataTest.php diff --git a/src/DTO/DefaultProxyFeature.php b/src/DTO/DefaultProxyFeature.php index b08b3a82..c46ff262 100644 --- a/src/DTO/DefaultProxyFeature.php +++ b/src/DTO/DefaultProxyFeature.php @@ -28,7 +28,7 @@ * value: string * } * }, - * impression_data: bool + * impressionData: bool * } $response */ public function __construct(array $response) @@ -37,12 +37,15 @@ public function __construct(array $response) // tries to new this up manually and isn't interesting to tests // @codeCoverageIgnoreStart - assert(isset($response['name'], $response['enabled'], $response['variant'], $response['impression_data'])); + assert( + isset($response['name'], $response['enabled'], $response['variant']) + && (isset($response['impressionData']) || isset($response['impression_data'])) + ); // @codeCoverageIgnoreEnd $this->name = $response['name']; $this->enabled = $response['enabled']; - $this->impressionData = $response['impression_data']; + $this->impressionData = $response['impressionData'] ?? $response['impression_data'] ?? false; $payload = null; @@ -87,7 +90,8 @@ public function jsonSerialize(): array 'name' => $this->name, 'enabled' => $this->enabled, 'variant' => $this->variant, - 'impression_data' => $this->impressionData, + 'impression_data' => $this->impressionData, // deprecated + 'impressionData' => $this->impressionData, // if you were reading the snake then you should read the camel ]; } } diff --git a/src/Repository/DefaultUnleashProxyRepository.php b/src/Repository/DefaultUnleashProxyRepository.php index b7016691..288697e8 100644 --- a/src/Repository/DefaultUnleashProxyRepository.php +++ b/src/Repository/DefaultUnleashProxyRepository.php @@ -153,16 +153,21 @@ private function addQuery(string $url, string $query): string * value: string * } * }, - * impression_data: bool + * impressionData: bool * }|null */ private function validateResponse(array $response): ?array { - if (!isset($response['name'], $response['enabled'], $response['variant'], $response['impression_data'])) { + // fix impression data: + if (isset($response['impression_data'])) { + $response['impressionData'] = $response['impression_data']; + unset($response['impression_data']); + } + if (!isset($response['name'], $response['enabled'], $response['variant'], $response['impressionData'])) { return null; } - if (!is_string($response['name']) || !is_bool($response['enabled']) || !is_bool($response['impression_data']) || !is_array($response['variant'])) { + if (!is_string($response['name']) || !is_bool($response['enabled']) || !is_bool($response['impressionData']) || !is_array($response['variant'])) { return null; } diff --git a/tests/DTO/DefaultProxyFeatureTest.php b/tests/DTO/DefaultProxyFeatureTest.php index ce4bffc9..9724e822 100755 --- a/tests/DTO/DefaultProxyFeatureTest.php +++ b/tests/DTO/DefaultProxyFeatureTest.php @@ -9,6 +9,20 @@ final class DefaultProxyFeatureTest extends TestCase { public function testGetProperties() + { + $instance = new DefaultProxyFeature(['name' => 'test', 'enabled' => true, 'impressionData' => true, 'variant' => ['name' => 'someVariant', 'enabled' => true, 'payload' => ['type' => 'string', 'value' => 'test']]]); + self::assertIsString($instance->getName()); + self::assertIsBool($instance->isEnabled()); + self::assertIsBool($instance->hasImpressionData()); + self::assertInstanceOf(DefaultVariant::class, $instance->getVariant()); + + self::assertEquals('test', $instance->getName()); + self::assertTrue($instance->isEnabled()); + self::assertTrue($instance->hasImpressionData()); + self::assertEquals('someVariant', $instance->getVariant()->getName()); + } + + public function testGetPropertiesImpressionDataSnakeCaseBackwardsCompatibility() { $instance = new DefaultProxyFeature(['name' => 'test', 'enabled' => true, 'impression_data' => true, 'variant' => ['name' => 'someVariant', 'enabled' => true, 'payload' => ['type' => 'string', 'value' => 'test']]]); self::assertIsString($instance->getName()); @@ -23,9 +37,16 @@ public function testGetProperties() } public function testToJson() + { + $instance = new DefaultProxyFeature(['name' => 'test', 'enabled' => true, 'impressionData' => true, 'variant' => ['name' => 'someVariant', 'enabled' => true, 'payload' => ['type' => 'string', 'value' => 'test']]]); + $json = json_encode($instance); + self::assertEquals('{"name":"test","enabled":true,"variant":{"name":"someVariant","enabled":true,"payload":{"type":"string","value":"test"}},"impression_data":true,"impressionData":true}', $json); + } + + public function testToJsonFromSnakeCaseImpressionDataForBackwardsCompatibility() { $instance = new DefaultProxyFeature(['name' => 'test', 'enabled' => true, 'impression_data' => true, 'variant' => ['name' => 'someVariant', 'enabled' => true, 'payload' => ['type' => 'string', 'value' => 'test']]]); $json = json_encode($instance); - self::assertEquals('{"name":"test","enabled":true,"variant":{"name":"someVariant","enabled":true,"payload":{"type":"string","value":"test"}},"impression_data":true}', $json); + self::assertEquals('{"name":"test","enabled":true,"variant":{"name":"someVariant","enabled":true,"payload":{"type":"string","value":"test"}},"impression_data":true,"impressionData":true}', $json); } } diff --git a/tests/DefaultProxyUnleashTest.php b/tests/DefaultProxyUnleashTest.php index 766e6087..a010592e 100644 --- a/tests/DefaultProxyUnleashTest.php +++ b/tests/DefaultProxyUnleashTest.php @@ -36,7 +36,7 @@ public function testBasicResolveFeature() 'name' => 'some-variant', 'enabled' => true, ], - 'impression_data' => false, + 'impressionData' => false, ]); $unleash = $builder->build(); @@ -89,7 +89,7 @@ public function testBasicResolveVariant() ], 'enabled' => true, ], - 'impression_data' => false, + 'impressionData' => false, ]); $unleash = $builder->build(); $variant = $unleash->getVariant('test'); @@ -107,7 +107,7 @@ public function testVariantWithoutPayload() 'name' => 'some-variant', 'enabled' => true, ], - 'impression_data' => false, + 'impressionData' => false, ]); $unleash = $builder->build(); $variant = $unleash->getVariant('test'); @@ -138,7 +138,7 @@ public function testVariantWithNullPayload() 'payload' => null, 'enabled' => true, ], - 'impression_data' => false, + 'impressionData' => false, ]); $unleash = $builder->build(); $variant = $unleash->getVariant('test'); @@ -159,7 +159,7 @@ public function testCachingIsRespected() 'name' => 'some-variant', 'enabled' => true, ], - 'impression_data' => false, + 'impressionData' => false, ]); $builder = new TestBuilder(); @@ -236,7 +236,7 @@ public function testPoisonedResponsesReturnFalse() 'name' => 'some-variant', 'enabled' => true, ], - 'impression_data' => false, + 'impressionData' => false, ], [ 'name' => 'test', @@ -244,7 +244,7 @@ public function testPoisonedResponsesReturnFalse() 'variant' => [ 'poisoned' => 'variant', ], - 'impression_data' => false, + 'impressionData' => false, ], [ 'name' => 'test', @@ -257,7 +257,7 @@ public function testPoisonedResponsesReturnFalse() 'value' => 'stuff', ], ], - 'impression_data' => false, + 'impressionData' => false, ], ]; diff --git a/tests/Repository/DefaultUnleashProxyRepositoryTest.php b/tests/Repository/DefaultUnleashProxyRepositoryTest.php index 1db4a6ac..d880726e 100644 --- a/tests/Repository/DefaultUnleashProxyRepositoryTest.php +++ b/tests/Repository/DefaultUnleashProxyRepositoryTest.php @@ -71,7 +71,7 @@ public function test200ResponseResolvesCorrectly() ], 'enabled' => true, ], - 'impression_data' => false, + 'impressionData' => false, ]) ), ]); @@ -112,7 +112,7 @@ public function testCacheTtlIsRespected() ], 'enabled' => true, ], - 'impression_data' => false, + 'impressionData' => false, ]); $mock = new MockHandler([ diff --git a/tests/Repository/DefaultUnleashProxyRepositoryWithSnakeCaseImpressionDataTest.php b/tests/Repository/DefaultUnleashProxyRepositoryWithSnakeCaseImpressionDataTest.php new file mode 100644 index 00000000..3ad19161 --- /dev/null +++ b/tests/Repository/DefaultUnleashProxyRepositoryWithSnakeCaseImpressionDataTest.php @@ -0,0 +1,170 @@ +push($history); + + $client = new Client(['handler' => $handlerStack]); + $config = (new UnleashConfiguration('', '', '')) + ->setCache($this->getCache()) + ->setProxyKey('some-key') + ->setTtl(5); + + $requestFactory = new HttpFactory(); + + $repository = new DefaultUnleashProxyRepository($config, $client, $requestFactory); + $resolvedFeature = $repository->findFeatureByContext('some-feature'); + $this->assertNull($resolvedFeature); + } + + public function test200ResponseResolvesCorrectly() + { + $container = []; + $history = Middleware::history($container); + + $mock = new MockHandler([ + new Response( + 200, + ['ETag' => 'etag value'], + json_encode([ + 'name' => 'test', + 'enabled' => true, + 'variant' => [ + 'name' => 'some-variant', + 'payload' => [ + 'type' => 'string', + 'value' => 'some-value', + ], + 'enabled' => true, + ], + 'impression_data' => false, + ]) + ), + ]); + + $handlerStack = HandlerStack::create($mock); + $handlerStack->push($history); + + $client = new Client(['handler' => $handlerStack]); + $config = (new UnleashConfiguration('', '', '')) + ->setCache($this->getCache()) + ->setProxyKey('some-key') + ->setTtl(5); + + $requestFactory = new HttpFactory(); + + $repository = new DefaultUnleashProxyRepository($config, $client, $requestFactory); + $resolvedFeature = $repository->findFeatureByContext('test'); + + $expectedVariant = new DefaultVariant('some-variant', true, 0, Stickiness::DEFAULT, new DefaultVariantPayload('string', 'some-value')); + + $this->assertEquals('test', $resolvedFeature->getName()); + $this->assertTrue($resolvedFeature->isEnabled()); + $this->assertEquals($expectedVariant, $resolvedFeature->getVariant()); + } + + public function testCacheTtlIsRespected() + { + $container = []; + $history = Middleware::history($container); + $response = json_encode([ + 'name' => 'test', + 'enabled' => true, + 'variant' => [ + 'name' => 'some-variant', + 'payload' => [ + 'type' => 'string', + 'value' => 'some-value', + ], + 'enabled' => true, + ], + 'impression_data' => false, + ]); + + $mock = new MockHandler([ + new Response( + 200, + ['ETag' => 'etag value'], + $response + ), + new Response( + 200, + ['ETag' => 'etag value'], + $response + ), + ]); + + $handlerStack = HandlerStack::create($mock); + $handlerStack->push($history); + + $client = new Client(['handler' => $handlerStack]); + $config = (new UnleashConfiguration('', '', '')) + ->setCache($this->getRealCache()) + ->setProxyKey('some-key') + ->setTtl(1); + + $requestFactory = new HttpFactory(); + $repository = new DefaultUnleashProxyRepository($config, $client, $requestFactory); + + $repository->findFeatureByContext('test'); + //cache is still warm so this should fall back to that + $repository->findFeatureByContext('test'); + + sleep(1); + + //ttl should have expired so this should trigger an API call + $repository->findFeatureByContext('test'); + + $this->assertCount(2, $container); + } + + public function testUrl() + { + $configuration = (new UnleashConfiguration( + new Url('https://localhost/api', 'somePrefix'), + '', + '' + ))->setCache($this->getCache())->setProxyKey('test'); + $instance = new DefaultUnleashProxyRepository($configuration, $this->httpClient, new HttpFactory()); + $this->pushResponse([]); + + $instance->findFeatureByContext('testFeature'); + self::assertCount(1, $this->requestHistory); + self::assertSame('https://localhost/api/frontend/features/testFeature?namePrefix=somePrefix', (string) $this->requestHistory[0]['request']->getUri()); + } +}