-
Notifications
You must be signed in to change notification settings - Fork 1
fix: catch cache error when cache is unavailable #8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| use OffloadProject\Toggle\Contracts\Driver; | ||
| use OffloadProject\Toggle\Exceptions\ToggleNotFoundException; | ||
| use RuntimeException; | ||
| use Throwable; | ||
|
|
||
| class ToggleManager | ||
| { | ||
|
|
@@ -33,11 +34,20 @@ public function active(string|BackedEnum $name): bool | |
| $key = $this->normalizeKey($name); | ||
|
|
||
| if ($this->cacheEnabled()) { | ||
| return $this->getCache()->remember( | ||
| $this->cacheKey($key), | ||
| $this->cacheTtl(), | ||
| fn () => $this->resolve($key) | ||
| ); | ||
| try { | ||
| return $this->getCache()->remember( | ||
| $this->cacheKey($key), | ||
| $this->cacheTtl(), | ||
| fn () => $this->resolve($key) | ||
| ); | ||
| } catch (Throwable $exception) { | ||
| // Rethrow toggle-specific exceptions so that genuine bugs are not masked. | ||
| if ($exception instanceof ToggleNotFoundException) { | ||
| throw $exception; | ||
| } | ||
| // Other exceptions (QueryException, PDOException, etc.) indicate cache | ||
| // unavailability - fall through to resolve without cache | ||
| } | ||
| } | ||
|
|
||
| return $this->resolve($key); | ||
|
|
@@ -113,22 +123,34 @@ public function forgetCache(string|BackedEnum $name): bool | |
| { | ||
| $key = $this->normalizeKey($name); | ||
|
|
||
| return $this->getCache()->forget($this->cacheKey($key)); | ||
| try { | ||
| return $this->getCache()->forget($this->cacheKey($key)); | ||
| } catch (Throwable) { | ||
| // Cache unavailable | ||
| return false; | ||
| } | ||
|
shavonn marked this conversation as resolved.
|
||
| } | ||
|
|
||
| /** | ||
| * Clear cache for all toggles. | ||
| */ | ||
| public function flushCache(): bool | ||
| { | ||
| $cache = $this->getCache(); | ||
| // Get toggles from driver first (let driver exceptions propagate) | ||
| $toggles = $this->getDriver()->all(); | ||
|
|
||
| // Clear all known toggles from cache | ||
| foreach ($this->getDriver()->all() as $name => $value) { | ||
| $cache->forget($this->cacheKey($name)); | ||
| } | ||
| try { | ||
| $cache = $this->getCache(); | ||
|
|
||
| return true; | ||
| foreach ($toggles as $name => $value) { | ||
| $cache->forget($this->cacheKey($name)); | ||
| } | ||
|
|
||
| return true; | ||
| } catch (Throwable) { | ||
| // Cache unavailable | ||
| return false; | ||
| } | ||
|
Comment on lines
+150
to
+153
|
||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| declare(strict_types=1); | ||
|
|
||
| use Illuminate\Database\QueryException; | ||
| use Illuminate\Support\Facades\Cache; | ||
| use OffloadProject\Toggle\Facades\Toggle; | ||
|
|
||
|
|
@@ -39,3 +40,38 @@ | |
| expect(Cache::has('toggle:test-flag'))->toBeFalse(); | ||
| expect(Cache::has('toggle:disabled-flag'))->toBeFalse(); | ||
| }); | ||
|
|
||
| it('gracefully handles cache unavailability during active check', function () { | ||
| // Verify caching is enabled for this test | ||
| expect(config('toggle.cache.enabled'))->toBeTrue(); | ||
|
|
||
| // Mock cache to throw a QueryException (simulating database not existing) | ||
| $exception = new QueryException('sqlite', 'select * from cache', [], new Exception('Database does not exist')); | ||
|
||
| Cache::shouldReceive('store') | ||
| ->atLeast()->once() | ||
| ->andThrow($exception); | ||
|
|
||
| // Should still resolve the toggle without cache | ||
| expect(Toggle::active('test-flag'))->toBeTrue(); | ||
| expect(Toggle::active('disabled-flag'))->toBeFalse(); | ||
| }); | ||
|
|
||
| it('gracefully handles cache unavailability during forgetCache', function () { | ||
| $exception = new QueryException('sqlite', 'select * from cache', [], new Exception('Database does not exist')); | ||
|
||
| Cache::shouldReceive('store') | ||
| ->once() | ||
| ->andThrow($exception); | ||
|
|
||
| // Should return false but not throw | ||
| expect(Toggle::forgetCache('test-flag'))->toBeFalse(); | ||
| }); | ||
|
|
||
| it('gracefully handles cache unavailability during flushCache', function () { | ||
| $exception = new QueryException('sqlite', 'select * from cache', [], new Exception('Database does not exist')); | ||
|
||
| Cache::shouldReceive('store') | ||
| ->once() | ||
| ->andThrow($exception); | ||
|
|
||
| // Should return false but not throw | ||
| expect(Toggle::flushCache())->toBeFalse(); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
forgetCache()method wrapsgetCache()in a try-catch, butgetCache()is also called in theenable(),disable(), anddelete()methods at lines 69, 84, and 99 without error handling. If cache is unavailable when these methods are called, they will throw an exception. Consider adding similar error handling to these call sites or ensuringforgetCache()can be called safely in all contexts.