Skip to content

Commit

Permalink
fix: support both snake_case and camelCase versions of impression data (
Browse files Browse the repository at this point in the history
#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
  • Loading branch information
gastonfournier authored Oct 1, 2024
1 parent 7353c74 commit 7d5f6ba
Show file tree
Hide file tree
Showing 6 changed files with 218 additions and 18 deletions.
12 changes: 8 additions & 4 deletions src/DTO/DefaultProxyFeature.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
* value: string
* }
* },
* impression_data: bool
* impressionData: bool
* } $response
*/
public function __construct(array $response)
Expand All @@ -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;

Expand Down Expand Up @@ -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
];
}
}
11 changes: 8 additions & 3 deletions src/Repository/DefaultUnleashProxyRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
23 changes: 22 additions & 1 deletion tests/DTO/DefaultProxyFeatureTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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);
}
}
16 changes: 8 additions & 8 deletions tests/DefaultProxyUnleashTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public function testBasicResolveFeature()
'name' => 'some-variant',
'enabled' => true,
],
'impression_data' => false,
'impressionData' => false,
]);
$unleash = $builder->build();

Expand Down Expand Up @@ -89,7 +89,7 @@ public function testBasicResolveVariant()
],
'enabled' => true,
],
'impression_data' => false,
'impressionData' => false,
]);
$unleash = $builder->build();
$variant = $unleash->getVariant('test');
Expand All @@ -107,7 +107,7 @@ public function testVariantWithoutPayload()
'name' => 'some-variant',
'enabled' => true,
],
'impression_data' => false,
'impressionData' => false,
]);
$unleash = $builder->build();
$variant = $unleash->getVariant('test');
Expand Down Expand Up @@ -138,7 +138,7 @@ public function testVariantWithNullPayload()
'payload' => null,
'enabled' => true,
],
'impression_data' => false,
'impressionData' => false,
]);
$unleash = $builder->build();
$variant = $unleash->getVariant('test');
Expand All @@ -159,7 +159,7 @@ public function testCachingIsRespected()
'name' => 'some-variant',
'enabled' => true,
],
'impression_data' => false,
'impressionData' => false,
]);

$builder = new TestBuilder();
Expand Down Expand Up @@ -236,15 +236,15 @@ public function testPoisonedResponsesReturnFalse()
'name' => 'some-variant',
'enabled' => true,
],
'impression_data' => false,
'impressionData' => false,
],
[
'name' => 'test',
'enabled' => true,
'variant' => [
'poisoned' => 'variant',
],
'impression_data' => false,
'impressionData' => false,
],
[
'name' => 'test',
Expand All @@ -257,7 +257,7 @@ public function testPoisonedResponsesReturnFalse()
'value' => 'stuff',
],
],
'impression_data' => false,
'impressionData' => false,
],
];

Expand Down
4 changes: 2 additions & 2 deletions tests/Repository/DefaultUnleashProxyRepositoryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public function test200ResponseResolvesCorrectly()
],
'enabled' => true,
],
'impression_data' => false,
'impressionData' => false,
])
),
]);
Expand Down Expand Up @@ -112,7 +112,7 @@ public function testCacheTtlIsRespected()
],
'enabled' => true,
],
'impression_data' => false,
'impressionData' => false,
]);

$mock = new MockHandler([
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
<?php

namespace Unleash\Client\Tests\Repository;

use GuzzleHttp\Client;
use GuzzleHttp\Handler\MockHandler;
use GuzzleHttp\HandlerStack;
use GuzzleHttp\Middleware;
use GuzzleHttp\Psr7\HttpFactory;
use GuzzleHttp\Psr7\Response;
use Unleash\Client\Configuration\UnleashConfiguration;
use Unleash\Client\DTO\DefaultVariant;
use Unleash\Client\DTO\DefaultVariantPayload;
use Unleash\Client\Enum\Stickiness;
use Unleash\Client\Helper\Url;
use Unleash\Client\Repository\DefaultUnleashProxyRepository;
use Unleash\Client\Tests\AbstractHttpClientTestCase;
use Unleash\Client\Tests\Traits\FakeCacheImplementationTrait;
use Unleash\Client\Tests\Traits\RealCacheImplementationTrait;

// this tests backward compatibility with a buggy old version of Unleash Edge
final class DefaultUnleashProxyRepositoryWithSnakeCaseImpressionDataTest extends AbstractHttpClientTestCase
{
use FakeCacheImplementationTrait, RealCacheImplementationTrait {
FakeCacheImplementationTrait::getCache insteadof RealCacheImplementationTrait;

RealCacheImplementationTrait::getCache as getRealCache;
}

public function testNon200ResponseDegradesGracefully()
{
$container = [];
$history = Middleware::history($container);

$mock = new MockHandler([
new Response(400, [], 'Error, bad request'),
]);

$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('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());
}
}

0 comments on commit 7d5f6ba

Please sign in to comment.