From 40a946d5253038de20b71abc2995bbc158bafeb6 Mon Sep 17 00:00:00 2001 From: Ty Potter Date: Mon, 7 Apr 2025 12:44:17 -0600 Subject: [PATCH 1/6] fixes ff-3947: throwOnFailedInit param to suppress Exceptions on initialization --- src/Cache/DefaultCacheFactory.php | 3 --- src/EppoClient.php | 34 ++++++++++++++++++++----------- tests/EppoClientTest.php | 16 +++++++++++---- 3 files changed, 34 insertions(+), 19 deletions(-) diff --git a/src/Cache/DefaultCacheFactory.php b/src/Cache/DefaultCacheFactory.php index 1311818..f6d8864 100644 --- a/src/Cache/DefaultCacheFactory.php +++ b/src/Cache/DefaultCacheFactory.php @@ -9,9 +9,6 @@ class DefaultCacheFactory { - /** - * @throws Exception - */ public static function create(): CacheInterface { $psr6Cache = new FilesystemAdapter( diff --git a/src/EppoClient.php b/src/EppoClient.php index 0e0db51..720634c 100644 --- a/src/EppoClient.php +++ b/src/EppoClient.php @@ -84,6 +84,7 @@ public static function init( RequestFactoryInterface $requestFactory = null, ?bool $isGracefulMode = true, ?PollingOptions $pollingOptions = null, + ?bool $throwOnFailedInit = false, ): EppoClient { // Get SDK metadata to pass as params in the http client. $sdkData = new SDKData(); @@ -93,11 +94,7 @@ public static function init( ]; if (!$cache) { - try { - $cache = (new DefaultCacheFactory())->create(); - } catch (Exception $e) { - throw EppoClientInitializationException::from($e); - } + $cache = (new DefaultCacheFactory())->create(); } $configStore = new ConfigurationStore($cache); @@ -143,27 +140,33 @@ function () use ($configLoader) { } ); - self::$instance = self::createAndInitClient($configLoader, $poller, $assignmentLogger, $isGracefulMode); + self::$instance = self::createAndInitClient($configLoader, $poller, $assignmentLogger, $isGracefulMode, throwOnFailedInit: $throwOnFailedInit); return self::$instance; } /** - * @throws EppoClientInitializationException + * @throws EppoClientInitializationException|InvalidConfigurationException */ private static function createAndInitClient( ConfigurationLoader $configLoader, PollerInterface $poller, ?LoggerInterface $assignmentLogger, ?bool $isGracefulMode, - ?IBanditEvaluator $banditEvaluator = null + ?IBanditEvaluator $banditEvaluator = null, + ?bool $throwOnFailedInit = false, ): EppoClient { try { $configLoader->reloadConfigurationIfExpired(); } catch (HttpRequestException | InvalidApiKeyException $e) { - throw new EppoClientInitializationException( - 'Unable to initialize Eppo Client: ' . $e->getMessage() - ); + $message = 'Unable to initialize Eppo Client: ' . $e->getMessage(); + if ($throwOnFailedInit) { + throw new EppoClientInitializationException( + $message + ); + } else { + syslog(LOG_INFO, "[Eppo SDK] " . $message); + } } return new self($configLoader, $poller, $assignmentLogger, $isGracefulMode, $banditEvaluator); } @@ -643,6 +646,13 @@ public static function createTestClient( ?bool $isGracefulMode = false, ?IBanditEvaluator $banditEvaluator = null ): EppoClient { - return self::createAndInitClient($configurationLoader, $poller, $logger, $isGracefulMode, $banditEvaluator); + return self::createAndInitClient( + $configurationLoader, + $poller, + $logger, + $isGracefulMode, + $banditEvaluator, + throwOnFailedInit: true + ); } } diff --git a/tests/EppoClientTest.php b/tests/EppoClientTest.php index e9df4b6..49bb4e2 100644 --- a/tests/EppoClientTest.php +++ b/tests/EppoClientTest.php @@ -179,7 +179,12 @@ public function testReturnsDefaultWhenExperimentConfigIsAbsent() public function testRepoTestCases(): void { try { - $client = EppoClient::init('dummy', self::$mockServer->serverAddress, isGracefulMode: false); + $client = EppoClient::init( + 'dummy', + self::$mockServer->serverAddress, + isGracefulMode: false, + throwOnFailedInit: true + ); } catch (Exception $exception) { self::fail('Failed to initialize EppoClient: ' . $exception->getMessage()); } @@ -315,7 +320,9 @@ public function testInitWithPollingOptions(): void ); $response = new Response(stream: Utils::streamFor(file_get_contents(__DIR__ . '/data/ufc/flags-v1.json'))); - $secondResponse = new Response(stream: Utils::streamFor(file_get_contents(__DIR__ . '/data/ufc/bandit-flags-v1.json'))); + $secondResponse = new Response(stream: Utils::streamFor( + file_get_contents(__DIR__ . '/data/ufc/bandit-flags-v1.json') + )); $httpClient = $this->createMock(ClientInterface::class); $httpClient->expects($this->atLeast(2)) @@ -327,7 +334,8 @@ public function testInitWithPollingOptions(): void "fake address", httpClient: $httpClient, isGracefulMode: false, - pollingOptions: $pollingOptions + pollingOptions: $pollingOptions, + throwOnFailedInit: true ); $this->assertEquals( @@ -335,7 +343,7 @@ public function testInitWithPollingOptions(): void $client->getNumericAssignment(self::EXPERIMENT_NAME, 'subject-10', [], 0) ); // Wait a little bit for the cache to age out and the mock server to spin up. - usleep(75*1000); + usleep(75 * 1000); $this->assertEquals( 0, From 76a3df2e2530a8e7e620f953de3a8863baefb05a Mon Sep 17 00:00:00 2001 From: Ty Potter Date: Mon, 7 Apr 2025 13:13:04 -0600 Subject: [PATCH 2/6] only set timestamp and eTag if data is modified --- src/Config/ConfigurationLoader.php | 8 +-- tests/Config/ConfigurationLoaderTest.php | 70 +++++++++++++++++++++++- 2 files changed, 73 insertions(+), 5 deletions(-) diff --git a/src/Config/ConfigurationLoader.php b/src/Config/ConfigurationLoader.php index 367922c..36e12db 100644 --- a/src/Config/ConfigurationLoader.php +++ b/src/Config/ConfigurationLoader.php @@ -109,11 +109,11 @@ function ($json) { if ($indexer->hasBandits()) { $this->fetchBanditsAsRequired($indexer); } - } - // Store metadata for next time. - $this->configurationStore->setMetadata(self::KEY_FLAG_TIMESTAMP, $this->millitime()); - $this->configurationStore->setMetadata(self::KEY_FLAG_ETAG, $response->ETag); + // Store metadata for next time. + $this->configurationStore->setMetadata(self::KEY_FLAG_TIMESTAMP, $this->millitime()); + $this->configurationStore->setMetadata(self::KEY_FLAG_ETAG, $response->ETag); + } } private function getCacheAgeInMillis(): int diff --git a/tests/Config/ConfigurationLoaderTest.php b/tests/Config/ConfigurationLoaderTest.php index b478f08..2d2dc66 100644 --- a/tests/Config/ConfigurationLoaderTest.php +++ b/tests/Config/ConfigurationLoaderTest.php @@ -69,7 +69,7 @@ public function testLoadsConfiguration(): void $configStore = new ConfigurationStore(DefaultCacheFactory::create()); - $loader = new ConfigurationLoader($apiWrapper, $configStore); + $loader = new ConfigurationLoader($apiWrapper, $configStore, 25); $loader->fetchAndStoreConfigurations(null); @@ -89,6 +89,74 @@ public function testLoadsConfiguration(): void $this->assertEquals('cold_start_bandit', $bandit->banditKey); } + + public function testSetsConfigurationTimestamp(): void + { + // Load mock response data + $flagsRaw = file_get_contents(self::MOCK_RESPONSE_FILENAME); + $flagsResourceResponse = new APIResource( + $flagsRaw, + true, + "ETAG" + ); + $flagsJson = json_decode($flagsRaw, true); + $flags = array_map(fn($flag) => (new UFCParser())->parseFlag($flag), $flagsJson['flags']); + $banditsRaw = '{ + "bandits": { + "cold_start_bandit": { + "banditKey": "cold_start_bandit", + "modelName": "falcon", + "updatedAt": "2023-09-13T04:52:06.462Z", + "modelVersion": "cold start", + "modelData": { + "gamma": 1.0, + "defaultActionScore": 0.0, + "actionProbabilityFloor": 0.0, + "coefficients": {} + } + } + } + }'; + + $apiWrapper = $this->getMockBuilder(APIRequestWrapper::class)->setConstructorArgs( + ['', [], new Psr18Client(), new Psr17Factory()] + )->getMock(); + + // Mocks verify interaction of loader <--> API requests and loader <--> config store + + $apiWrapper->expects($this->exactly(2)) + ->method('getUFC') + ->willReturnCallback( + function (?string $eTag) use ($flagsResourceResponse, $flagsRaw) { + return $eTag == null ? $flagsResourceResponse : new APIResource( + $flagsRaw, + false, + "ETAG" + ); + } + ); + + $apiWrapper->expects($this->once()) + ->method('getBandits') + ->willReturn(new APIResource($banditsRaw, true, null)); + + $configStore = new ConfigurationStore(DefaultCacheFactory::create()); + + $loader = new ConfigurationLoader($apiWrapper, $configStore, 25); + $loader->fetchAndStoreConfigurations(null); + + $timestamp1 = $configStore->getMetadata("flagTimestamp"); + $storedEtag = $configStore->getMetadata("flagETag"); + $this->assertEquals("ETAG", $storedEtag); + + usleep(50 * 1000); // Sleep long enough for cache to expire. + + $loader->fetchAndStoreConfigurations("ETAG"); + + $this->assertEquals("ETAG", $configStore->getMetadata("flagETag")); + $this->assertEquals($timestamp1, $configStore->getMetadata("flagTimestamp")); + } + public function testLoadsOnGet(): void { // Arrange: Load some flag data to be returned by the APIRequestWrapper From b7362fd1a9fcb29a52f82e7853d06dd691d19e47 Mon Sep 17 00:00:00 2001 From: Ty Potter Date: Mon, 7 Apr 2025 13:21:00 -0600 Subject: [PATCH 3/6] PR template typo and test enhance --- .github/pull_request_template.md | 2 +- tests/Config/ConfigurationLoaderTest.php | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 8061996..7708cf9 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -9,7 +9,7 @@ [//]: # (Describe your changes in detail) ## How has this been documented? -[//]: # (Please describe how you documented the developer impact of your changes; link to PRs or issues or explan why no documentation changes are required) +[//]: # (Please describe how you documented the developer impact of your changes; link to PRs or issues or explain why no documentation changes are required) ## How has this been tested? [//]: # (Please describe in detail how you tested your changes) diff --git a/tests/Config/ConfigurationLoaderTest.php b/tests/Config/ConfigurationLoaderTest.php index 2d2dc66..a7c3be0 100644 --- a/tests/Config/ConfigurationLoaderTest.php +++ b/tests/Config/ConfigurationLoaderTest.php @@ -69,7 +69,7 @@ public function testLoadsConfiguration(): void $configStore = new ConfigurationStore(DefaultCacheFactory::create()); - $loader = new ConfigurationLoader($apiWrapper, $configStore, 25); + $loader = new ConfigurationLoader($apiWrapper, $configStore); $loader->fetchAndStoreConfigurations(null); @@ -99,8 +99,6 @@ public function testSetsConfigurationTimestamp(): void true, "ETAG" ); - $flagsJson = json_decode($flagsRaw, true); - $flags = array_map(fn($flag) => (new UFCParser())->parseFlag($flag), $flagsJson['flags']); $banditsRaw = '{ "bandits": { "cold_start_bandit": { @@ -142,7 +140,7 @@ function (?string $eTag) use ($flagsResourceResponse, $flagsRaw) { $configStore = new ConfigurationStore(DefaultCacheFactory::create()); - $loader = new ConfigurationLoader($apiWrapper, $configStore, 25); + $loader = new ConfigurationLoader($apiWrapper, $configStore); $loader->fetchAndStoreConfigurations(null); $timestamp1 = $configStore->getMetadata("flagTimestamp"); From d405d5c553acf0d5f9fa3f8768e343195c8008b6 Mon Sep 17 00:00:00 2001 From: Ty Potter Date: Mon, 7 Apr 2025 13:22:05 -0600 Subject: [PATCH 4/6] drop comment --- tests/Config/ConfigurationLoaderTest.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/Config/ConfigurationLoaderTest.php b/tests/Config/ConfigurationLoaderTest.php index a7c3be0..f9a4aa7 100644 --- a/tests/Config/ConfigurationLoaderTest.php +++ b/tests/Config/ConfigurationLoaderTest.php @@ -120,8 +120,6 @@ public function testSetsConfigurationTimestamp(): void ['', [], new Psr18Client(), new Psr17Factory()] )->getMock(); - // Mocks verify interaction of loader <--> API requests and loader <--> config store - $apiWrapper->expects($this->exactly(2)) ->method('getUFC') ->willReturnCallback( From 6b4e2dec5f082255b8dd4f5c30e4997847b2fd3e Mon Sep 17 00:00:00 2001 From: Ty Potter Date: Mon, 7 Apr 2025 14:15:17 -0600 Subject: [PATCH 5/6] test comments --- tests/Config/ConfigurationLoaderTest.php | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/tests/Config/ConfigurationLoaderTest.php b/tests/Config/ConfigurationLoaderTest.php index f9a4aa7..9120df2 100644 --- a/tests/Config/ConfigurationLoaderTest.php +++ b/tests/Config/ConfigurationLoaderTest.php @@ -99,22 +99,7 @@ public function testSetsConfigurationTimestamp(): void true, "ETAG" ); - $banditsRaw = '{ - "bandits": { - "cold_start_bandit": { - "banditKey": "cold_start_bandit", - "modelName": "falcon", - "updatedAt": "2023-09-13T04:52:06.462Z", - "modelVersion": "cold start", - "modelData": { - "gamma": 1.0, - "defaultActionScore": 0.0, - "actionProbabilityFloor": 0.0, - "coefficients": {} - } - } - } - }'; + $banditsRaw = '{"bandits": {}}'; $apiWrapper = $this->getMockBuilder(APIRequestWrapper::class)->setConstructorArgs( ['', [], new Psr18Client(), new Psr17Factory()] @@ -124,6 +109,7 @@ public function testSetsConfigurationTimestamp(): void ->method('getUFC') ->willReturnCallback( function (?string $eTag) use ($flagsResourceResponse, $flagsRaw) { + // Return not modified if the etag sent is not null. return $eTag == null ? $flagsResourceResponse : new APIResource( $flagsRaw, false, @@ -150,6 +136,8 @@ function (?string $eTag) use ($flagsResourceResponse, $flagsRaw) { $loader->fetchAndStoreConfigurations("ETAG"); $this->assertEquals("ETAG", $configStore->getMetadata("flagETag")); + + // The timestamp should not have changed; the config did not change, so the timestamp should not be updated. $this->assertEquals($timestamp1, $configStore->getMetadata("flagTimestamp")); } From bf9a56752b16a5cb7c17d546622f12f92a45978f Mon Sep 17 00:00:00 2001 From: Ty Potter Date: Mon, 7 Apr 2025 14:37:42 -0600 Subject: [PATCH 6/6] test that init exception can be suppressed --- src/EppoClient.php | 5 +++-- tests/EppoClientTest.php | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/EppoClient.php b/src/EppoClient.php index 720634c..c651023 100644 --- a/src/EppoClient.php +++ b/src/EppoClient.php @@ -644,7 +644,8 @@ public static function createTestClient( PollerInterface $poller, ?LoggerInterface $logger = null, ?bool $isGracefulMode = false, - ?IBanditEvaluator $banditEvaluator = null + ?IBanditEvaluator $banditEvaluator = null, + ?bool $throwOnFailedInit = true, ): EppoClient { return self::createAndInitClient( $configurationLoader, @@ -652,7 +653,7 @@ public static function createTestClient( $logger, $isGracefulMode, $banditEvaluator, - throwOnFailedInit: true + throwOnFailedInit: $throwOnFailedInit ); } } diff --git a/tests/EppoClientTest.php b/tests/EppoClientTest.php index 49bb4e2..0fc0bdb 100644 --- a/tests/EppoClientTest.php +++ b/tests/EppoClientTest.php @@ -160,6 +160,38 @@ public function testGracefulModeThrowsOnInit() ); } + public function testSuppressInitExceptionThrow() + { + $pollerMock = $this->getPollerMock(); + + $apiRequestWrapper = $this->getMockBuilder(APIRequestWrapper::class)->setConstructorArgs( + ['', [], new Psr18Client(), new Psr17Factory()] + )->getMock(); + + $apiRequestWrapper->expects($this->any()) + ->method('getUFC') + ->willThrowException(new HttpRequestException()); + + $configStore = $this->getMockBuilder(IConfigurationStore::class)->getMock(); + $configStore->expects($this->any())->method('getMetadata')->willReturn(null); + $mockLogger = $this->getMockBuilder(LoggerInterface::class)->getMock(); + + $client = EppoClient::createTestClient( + new ConfigurationLoader($apiRequestWrapper, $configStore), + $pollerMock, + $mockLogger, + true, + throwOnFailedInit: false + ); + + $this->assertEquals( + 'default', + $client->getStringAssignment(self::EXPERIMENT_NAME, 'subject-10', [], 'default') + ); + + // No exceptions thrown, default assignments. + } + public function testReturnsDefaultWhenExperimentConfigIsAbsent() { $configLoaderMock = $this->getFlagConfigurationLoaderMock([]);