From d4e09e5c8147b7117ba1ab095006a364fe25671a Mon Sep 17 00:00:00 2001 From: Miguel Angel Date: Mon, 2 Dec 2024 10:00:02 -0400 Subject: [PATCH 1/4] ref: remame setting cache manager test --- .../Cache/{SettingCacheTest.php => SettingCacheManagerTest.php} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename tests/Feature/Cache/{SettingCacheTest.php => SettingCacheManagerTest.php} (97%) diff --git a/tests/Feature/Cache/SettingCacheTest.php b/tests/Feature/Cache/SettingCacheManagerTest.php similarity index 97% rename from tests/Feature/Cache/SettingCacheTest.php rename to tests/Feature/Cache/SettingCacheManagerTest.php index 56288bec63..c14349e06f 100644 --- a/tests/Feature/Cache/SettingCacheTest.php +++ b/tests/Feature/Cache/SettingCacheManagerTest.php @@ -4,7 +4,7 @@ use Tests\TestCase; -class SettingCacheTest extends TestCase +class SettingCacheManagerTest extends TestCase { public function testGet() { From 655d803eb6c221ab228674b3cf11eca07bfb3768 Mon Sep 17 00:00:00 2001 From: Miguel Angel Date: Mon, 2 Dec 2024 14:48:11 -0400 Subject: [PATCH 2/4] test: get setting from cache --- tests/Feature/Cache/SettingCacheTest.php | 120 +++++++++++++++++++++++ 1 file changed, 120 insertions(+) create mode 100644 tests/Feature/Cache/SettingCacheTest.php diff --git a/tests/Feature/Cache/SettingCacheTest.php b/tests/Feature/Cache/SettingCacheTest.php new file mode 100644 index 0000000000..aa72738238 --- /dev/null +++ b/tests/Feature/Cache/SettingCacheTest.php @@ -0,0 +1,120 @@ +artisan('migrate', [ + '--path' => 'upgrades/2023_11_30_185738_add_password_policies_settings.php', + ])->run(); + } + + public static function trackQueries(): void + { + DB::enableQueryLog(); + } + + public static function flushQueryLog(): void + { + DB::flushQueryLog(); + } + + public static function getQueriesExecuted(): array + { + return DB::getQueryLog(); + } + + public static function getQueryCount(): int + { + return count(self::getQueriesExecuted()); + } + + public function testGetSettingByKeyCached(): void + { + $this->upgrade(); + + $key = 'password-policies.users_can_change'; + + $setting = Setting::where('key', $key)->first(); + \SettingCache::set($key, $setting); + + $this->trackQueries(); + + $setting = Setting::byKey($key); + + $this->assertEquals(0, self::getQueryCount()); + $this->assertEquals($key, $setting->key); + } + + public function testGetSettingByKeyNotCached(): void + { + $this->upgrade(); + $this->trackQueries(); + + $key = 'password-policies.uppercase'; + + $setting = Setting::byKey($key); + + $this->assertEquals(1, self::getQueryCount()); + $this->assertEquals($key, $setting->key); + + $this->flushQueryLog(); + + $setting = Setting::byKey($key); + $this->assertEquals(0, self::getQueryCount()); + $this->assertNotNull($setting); + $this->assertEquals($key, $setting->key); + } + + public function testGetSettingByKeyCachedAfterUpdate(): void + { + $this->upgrade(); + $this->trackQueries(); + + $key = 'password-policies.special'; + + $setting = Setting::byKey($key); + + $this->assertEquals(1, self::getQueryCount()); + $this->assertEquals($key, $setting->key); + $this->assertEquals($setting->config, 1); + + $data = array_merge($setting->toArray(), ['config' => false]); + + $response = $this->apiCall('PUT', route('api.settings.update', ['setting' => $setting->id]), $data); + $response->assertStatus(204); + + $this->flushQueryLog(); + + $setting = Setting::byKey($key); + $this->assertEquals(0, self::getQueryCount()); + $this->assertEquals($key, $setting->key); + $this->assertEquals($setting->config, 0); + } + + public function testGetSettingByNotExistingKey() + { + $key = 'non-existing-key'; + + $this->expectException(\InvalidArgumentException::class); + $setting = Setting::byKey($key); + + $this->assertNull($setting); + } +} From 1204f7c24e5b0b5106e62159fc9ad0e6497adc37 Mon Sep 17 00:00:00 2001 From: Miguel Angel Date: Tue, 3 Dec 2024 00:29:42 -0400 Subject: [PATCH 3/4] feat: get setting from cache --- ProcessMaker/Cache/CacheInterface.php | 4 ++-- .../Cache/Settings/SettingCacheManager.php | 24 +++++++++++++------ .../Controllers/Api/SettingController.php | 3 +++ ProcessMaker/Models/Setting.php | 17 +++++++++++-- tests/Feature/Cache/SettingCacheTest.php | 17 +++++++------ 5 files changed, 47 insertions(+), 18 deletions(-) diff --git a/ProcessMaker/Cache/CacheInterface.php b/ProcessMaker/Cache/CacheInterface.php index a89024c9d5..560d04ac4e 100644 --- a/ProcessMaker/Cache/CacheInterface.php +++ b/ProcessMaker/Cache/CacheInterface.php @@ -8,11 +8,11 @@ interface CacheInterface * Fetches a value from the cache. * * @param string $key The unique key of this item in the cache. - * @param mixed $default Default value to return if the key does not exist. + * @param callable $callback The callback that will return the value to store in the cache. * * @return mixed The value of the item from the cache, or $default in case of cache miss. */ - public function get(string $key, mixed $default = null): mixed; + public function get(string $key, callable $callback = null): mixed; /** * Persists data in the cache, uniquely referenced by a key with an optional expiration TTL time. diff --git a/ProcessMaker/Cache/Settings/SettingCacheManager.php b/ProcessMaker/Cache/Settings/SettingCacheManager.php index a330ac5cb6..80ab5bfc5e 100644 --- a/ProcessMaker/Cache/Settings/SettingCacheManager.php +++ b/ProcessMaker/Cache/Settings/SettingCacheManager.php @@ -32,19 +32,29 @@ public function __call($method, $arguments): mixed * Get a value from the settings cache. * * @param string $key - * @param mixed $default + * @param callable|null $callback * * @return mixed */ - public function get(string $key, mixed $default = null): mixed + public function get(string $key, callable $callback = null): mixed { - try { - return $this->cacheManager->get($key, $default); - } catch (Exception $e) { - Log::error('Cache error: ' . $e->getMessage()); + $value = $this->cacheManager->get($key); + + if ($value) { + return $value; + } + + if ($callback === null) { + return null; + } + + $value = $callback(); + + if ($value === null) { + throw new \InvalidArgumentException('The key does not exist.'); } - return null; + return $value; } /** diff --git a/ProcessMaker/Http/Controllers/Api/SettingController.php b/ProcessMaker/Http/Controllers/Api/SettingController.php index 4561d72bb3..d52141210f 100644 --- a/ProcessMaker/Http/Controllers/Api/SettingController.php +++ b/ProcessMaker/Http/Controllers/Api/SettingController.php @@ -243,6 +243,9 @@ public function update(Setting $setting, Request $request) $original = array_intersect_key($setting->getOriginal(), $setting->getDirty()); $setting->save(); + // Store the setting in the cache + \SettingCache::set($setting->key, $setting); + if ($setting->key === 'password-policies.2fa_enabled') { // Update all groups with the new 2FA setting Group::where('enabled_2fa', '!=', $setting->config) diff --git a/ProcessMaker/Models/Setting.php b/ProcessMaker/Models/Setting.php index e6167a2517..803352b295 100644 --- a/ProcessMaker/Models/Setting.php +++ b/ProcessMaker/Models/Setting.php @@ -142,13 +142,26 @@ public static function messages() * Get setting by key * * @param string $key + * @param bool $withCallback * * @return \ProcessMaker\Models\Setting|null * @throws \Exception */ - public static function byKey(string $key) + public static function byKey(string $key, bool $withCallback = false) { - return (new self)->where('key', $key)->first(); + $callback = null; + + if ($withCallback) { + $callback = fn() => (new self)->where('key', $key)->first(); + } + + $setting = \SettingCache::get($key, $callback); + + if (!is_null($setting)) { + \SettingCache::set($key, $setting); + } + + return $setting; } /** diff --git a/tests/Feature/Cache/SettingCacheTest.php b/tests/Feature/Cache/SettingCacheTest.php index aa72738238..ce900a7223 100644 --- a/tests/Feature/Cache/SettingCacheTest.php +++ b/tests/Feature/Cache/SettingCacheTest.php @@ -64,12 +64,13 @@ public function testGetSettingByKeyCached(): void public function testGetSettingByKeyNotCached(): void { + $key = 'password-policies.uppercase'; + \SettingCache::delete($key); + $this->upgrade(); $this->trackQueries(); - $key = 'password-policies.uppercase'; - - $setting = Setting::byKey($key); + $setting = Setting::byKey($key, true); $this->assertEquals(1, self::getQueryCount()); $this->assertEquals($key, $setting->key); @@ -84,12 +85,13 @@ public function testGetSettingByKeyNotCached(): void public function testGetSettingByKeyCachedAfterUpdate(): void { + $key = 'password-policies.special'; + \SettingCache::delete($key); + $this->upgrade(); $this->trackQueries(); - $key = 'password-policies.special'; - - $setting = Setting::byKey($key); + $setting = Setting::byKey($key, true); $this->assertEquals(1, self::getQueryCount()); $this->assertEquals($key, $setting->key); @@ -110,10 +112,11 @@ public function testGetSettingByKeyCachedAfterUpdate(): void public function testGetSettingByNotExistingKey() { + $this->withoutExceptionHandling(); $key = 'non-existing-key'; $this->expectException(\InvalidArgumentException::class); - $setting = Setting::byKey($key); + $setting = Setting::byKey($key, true); $this->assertNull($setting); } From 6546a4e7bf5447e18d7a1898ceca7c08ecb2c93f Mon Sep 17 00:00:00 2001 From: Miguel Angel Date: Tue, 3 Dec 2024 11:45:05 -0400 Subject: [PATCH 4/4] feat: enhance setting cache manager --- ProcessMaker/Cache/CacheInterface.php | 14 ++++++- .../Cache/Settings/SettingCacheManager.php | 39 +++++++++++++------ .../Controllers/Api/SettingController.php | 6 +-- ProcessMaker/Models/Setting.php | 18 ++++----- tests/Feature/Cache/SettingCacheTest.php | 15 +++---- 5 files changed, 56 insertions(+), 36 deletions(-) diff --git a/ProcessMaker/Cache/CacheInterface.php b/ProcessMaker/Cache/CacheInterface.php index 560d04ac4e..24b127750d 100644 --- a/ProcessMaker/Cache/CacheInterface.php +++ b/ProcessMaker/Cache/CacheInterface.php @@ -8,11 +8,23 @@ interface CacheInterface * Fetches a value from the cache. * * @param string $key The unique key of this item in the cache. + * @param mixed $default Default value to return if the key does not exist. + * + * @return mixed The value of the item from the cache, or $default in case of cache miss. + */ + public function get(string $key, mixed $default = null): mixed; + + /** + * Fetches a value from the cache, or stores the value from the callback if the key exists. + * + * @param string $key The unique key of this item in the cache. * @param callable $callback The callback that will return the value to store in the cache. * * @return mixed The value of the item from the cache, or $default in case of cache miss. + * + * @throws \InvalidArgumentException */ - public function get(string $key, callable $callback = null): mixed; + public function getOrCache(string $key, callable $callback): mixed; /** * Persists data in the cache, uniquely referenced by a key with an optional expiration TTL time. diff --git a/ProcessMaker/Cache/Settings/SettingCacheManager.php b/ProcessMaker/Cache/Settings/SettingCacheManager.php index 80ab5bfc5e..f9f5ff3814 100644 --- a/ProcessMaker/Cache/Settings/SettingCacheManager.php +++ b/ProcessMaker/Cache/Settings/SettingCacheManager.php @@ -2,9 +2,7 @@ namespace ProcessMaker\Cache\Settings; -use Exception; use Illuminate\Cache\CacheManager; -use Illuminate\Support\Facades\Log; use ProcessMaker\Cache\CacheInterface; class SettingCacheManager implements CacheInterface @@ -32,28 +30,45 @@ public function __call($method, $arguments): mixed * Get a value from the settings cache. * * @param string $key - * @param callable|null $callback + * @param mixed $default * * @return mixed */ - public function get(string $key, callable $callback = null): mixed + public function get(string $key, mixed $default = null): mixed { - $value = $this->cacheManager->get($key); + return $this->cacheManager->get($key, $default); + } - if ($value) { - return $value; - } + /** + * Get a value from the settings cache, or store the value from the callback if the key exists. + * + * @param string $key + * @param callable $callback + * + * @return mixed + * + * @throws \InvalidArgumentException + */ + public function getOrCache(string $key, callable $callback): mixed + { + $value = $this->get($key); - if ($callback === null) { - return null; + if ($value !== null) { + return $value; } - $value = $callback(); + try { + $value = $callback(); - if ($value === null) { + if ($value === null) { + throw new \InvalidArgumentException('The key does not exist.'); + } + } catch (\Exception $e) { throw new \InvalidArgumentException('The key does not exist.'); } + $this->set($key, $value); + return $value; } diff --git a/ProcessMaker/Http/Controllers/Api/SettingController.php b/ProcessMaker/Http/Controllers/Api/SettingController.php index d52141210f..d219c605e0 100644 --- a/ProcessMaker/Http/Controllers/Api/SettingController.php +++ b/ProcessMaker/Http/Controllers/Api/SettingController.php @@ -243,9 +243,6 @@ public function update(Setting $setting, Request $request) $original = array_intersect_key($setting->getOriginal(), $setting->getDirty()); $setting->save(); - // Store the setting in the cache - \SettingCache::set($setting->key, $setting); - if ($setting->key === 'password-policies.2fa_enabled') { // Update all groups with the new 2FA setting Group::where('enabled_2fa', '!=', $setting->config) @@ -255,6 +252,9 @@ public function update(Setting $setting, Request $request) // Register the Event SettingsUpdated::dispatch($setting, $setting->getChanges(), $original); + // Store the setting in the cache + \SettingCache::set($setting->key, $setting->refresh()); + return response([], 204); } diff --git a/ProcessMaker/Models/Setting.php b/ProcessMaker/Models/Setting.php index 803352b295..9fe9d2fd10 100644 --- a/ProcessMaker/Models/Setting.php +++ b/ProcessMaker/Models/Setting.php @@ -142,23 +142,21 @@ public static function messages() * Get setting by key * * @param string $key - * @param bool $withCallback * * @return \ProcessMaker\Models\Setting|null * @throws \Exception */ - public static function byKey(string $key, bool $withCallback = false) + public static function byKey(string $key) { - $callback = null; + $setting = \SettingCache::get($key); - if ($withCallback) { - $callback = fn() => (new self)->where('key', $key)->first(); - } - - $setting = \SettingCache::get($key, $callback); + if ($setting === null) { + $setting = (new self)->where('key', $key)->first(); - if (!is_null($setting)) { - \SettingCache::set($key, $setting); + // Store the setting in the cache if it exists + if ($setting !== null) { + \SettingCache::set($key, $setting); + } } return $setting; diff --git a/tests/Feature/Cache/SettingCacheTest.php b/tests/Feature/Cache/SettingCacheTest.php index ce900a7223..de68e68829 100644 --- a/tests/Feature/Cache/SettingCacheTest.php +++ b/tests/Feature/Cache/SettingCacheTest.php @@ -13,11 +13,6 @@ class SettingCacheTest extends TestCase use RequestHelper; use RefreshDatabase; - /* public function setUp(): void - { - parent::setUp(); - } */ - private function upgrade() { $this->artisan('migrate', [ @@ -65,12 +60,11 @@ public function testGetSettingByKeyCached(): void public function testGetSettingByKeyNotCached(): void { $key = 'password-policies.uppercase'; - \SettingCache::delete($key); $this->upgrade(); $this->trackQueries(); - $setting = Setting::byKey($key, true); + $setting = Setting::byKey($key); $this->assertEquals(1, self::getQueryCount()); $this->assertEquals($key, $setting->key); @@ -86,12 +80,11 @@ public function testGetSettingByKeyNotCached(): void public function testGetSettingByKeyCachedAfterUpdate(): void { $key = 'password-policies.special'; - \SettingCache::delete($key); $this->upgrade(); $this->trackQueries(); - $setting = Setting::byKey($key, true); + $setting = Setting::byKey($key); $this->assertEquals(1, self::getQueryCount()); $this->assertEquals($key, $setting->key); @@ -115,8 +108,10 @@ public function testGetSettingByNotExistingKey() $this->withoutExceptionHandling(); $key = 'non-existing-key'; + $callback = fn() => Setting::where('key', $key)->first(); + $this->expectException(\InvalidArgumentException::class); - $setting = Setting::byKey($key, true); + $setting = \SettingCache::getOrCache($key, $callback); $this->assertNull($setting); }