feat(ZMSKVR-138): Improve caching response cache mapping in second cache #910
Conversation
WalkthroughThe changes introduce several new private constants for cache keys in the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant Z as ZmsApiFacadeService
participant Ch as Cache
participant D as DataSource
C->>Z: Request data (e.g., getOffices)
Z->>Ch: Check for cached data using key
alt Cache Hit
Ch-->>Z: Return cached data
else Cache Miss
Z->>D: Fetch required data
D-->>Z: Return fetched data
Z->>Ch: Set cache with key and TTL (log event)
end
Z->>C: Return data
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php (1)
59-62: Extract repeated caching pattern into a reusable method.
We notice this caching approach repeats across multiple methods. Consider encapsulating cache retrieval and storage in a helper function to reduce code duplication and improve maintainability (DRY principle).Below is a possible illustration of a refactor:
+/** + * Retrieve or set a cached result by key. + * + * @param string $cacheKey + * @param callable $callback + * @param int|null $ttl + * @return mixed + */ +private static function getOrSetCache(string $cacheKey, callable $callback, ?int $ttl = null) +{ + if (!\App::$cache) { + return $callback(); + } + $cachedValue = \App::$cache->get($cacheKey); + if ($cachedValue !== null) { + return $cachedValue; + } + $result = $callback(); + \App::$cache->set($cacheKey, $result, $ttl ?? \App::$SOURCE_CACHE_TTL); + LoggerService::logInfo('Second-level cache set', [ + 'key' => $cacheKey, + 'ttl' => $ttl ?? \App::$SOURCE_CACHE_TTL, + ]); + return $result; +}Usage example in relevant methods:
- if (\App::$cache && ($cachedData = \App::$cache->get($cacheKey))) { - return $cachedData; - } - $result = new OfficeList($offices); - \App::$cache->set($cacheKey, $result, \App::$SOURCE_CACHE_TTL); - LoggerService::logInfo('Second-level cache set', [ - 'key' => $cacheKey, - 'ttl' => \App::$SOURCE_CACHE_TTL - ]); - return $result; + $result = self::getOrSetCache($cacheKey, function () use ($offices) { + return new OfficeList($offices); + }); + return $result;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php(10 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.php`: Flag any usage of error_log() as it should be re...
**/*.php: Flag any usage of error_log() as it should be replaced with proper logging mechanisms:
- For error handling: Use a proper logging framework with error levels (PSR-3 LoggerInterface)
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- For application events: Use structured logging with proper log levels
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $logger->error("Import failed", ['error' => $e->getMessage()]);
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: call-php-unit-tests / zmsapi-test
🔇 Additional comments (30)
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php (30)
37-43: Well-defined private cache constants.
These constants are clearly named, improving readability and maintainability.
57-58: Cache key generation for offices.
No concerns here. The approach is consistent with the rest of the codebase.
106-107: Constructing OfficeList.
No issues observed. This is straightforward.
108-114: Repetitive cache storage logic.
This logic is equivalent to the pattern highlighted in lines 59-62.
116-116: Returning the final result.
No issues here.
121-122: Cache key generation for scopes.
Consistently follows the same approach as for offices.
123-126: Repetitive cache retrieval logic.
See lines 59-62 for our suggestion on reducing code duplication.
160-161: Creating ThinnedScopeList.
Implementation looks correct and consistent.
162-168: Repetitive cache storage logic.
See previous comments regarding extraction into a helper.
170-170: Return result.
No issues to report.
175-176: Cache key for services.
Naming is clear and consistent.
177-180: Repetitive cache retrieval logic.
See lines 59-62 for DRY suggestion.
196-197: Creating ServiceList.
Implementation is straightforward and appears correct.
198-204: Repetitive cache storage logic.
See previous comments for recommended refactoring approach.
206-206: Returning the result.
No additional feedback here.
211-212: Cache key for offices and services.
Consistent naming.
213-216: Repetitive cache retrieval logic.
Aligned with lines 59-62 suggestion.
229-230: Creating OfficeServiceAndRelationList.
No issues found.
231-237: Repetitive cache storage logic.
Refer to lines 59-62 for DRY approach.
239-239: Returning the result.
Implementation is fine.
307-308: Cache key for offices by service.
Approach is consistent.
309-312: Repetitive cache retrieval logic.
Refer to earlier DRY suggestion at lines 59-62.
348-349: Creating OfficeList from offices.
Looks correct.
350-356: Repetitive cache storage logic.
Refer to lines 59-62 for recommended DRY approach.
358-358: Return the final result.
No issue.
411-412: Cache key for services by office.
Naming remains consistent.
413-416: Repetitive cache retrieval logic.
See lines 59-62 comment for DRY approach.
449-450: Creating ServiceList.
Looks good.
451-457: Repetitive cache storage logic.
Refer to lines 59-62 for recommended DRY approach.
459-459: Returning the final result.
No concerns.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php (1)
35-711: Consider adding a cache invalidation mechanism.While the caching implementation is solid, there doesn't appear to be a mechanism for invalidating the cache when data changes. Depending on your application's requirements, you might need to implement cache invalidation to ensure data consistency.
You could add a method like:
private static function invalidateCache(string $cacheKey = null): void { if (\App::$cache) { if ($cacheKey) { \App::$cache->delete($cacheKey); LoggerService::logInfo('Cache invalidated', ['key' => $cacheKey]); } else { // Invalidate all known cache keys $cacheKeys = [ self::CACHE_KEY_OFFICES, self::CACHE_KEY_SCOPES, self::CACHE_KEY_SERVICES, self::CACHE_KEY_OFFICES_AND_SERVICES, // Add logic for dynamic keys if needed ]; foreach ($cacheKeys as $key) { \App::$cache->delete($key); } LoggerService::logInfo('All cache invalidated'); } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php(10 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.php`: Flag any usage of error_log() as it should be re...
**/*.php: Flag any usage of error_log() as it should be replaced with proper logging mechanisms:
- For error handling: Use a proper logging framework with error levels (PSR-3 LoggerInterface)
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- For application events: Use structured logging with proper log levels
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $logger->error("Import failed", ['error' => $e->getMessage()]);
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: call-owasp-security-checks / security-scan (zmsclient, 8.0)
- GitHub Check: call-php-code-quality / module-code-quality (zmscitizenapi, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsdb, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsslim, 8.0)
- GitHub Check: call-php-code-quality / module-code-quality (zmsapi, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsentities, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsdldb, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsticketprinter, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsstatistic, 8.0)
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: call-owasp-security-checks / security-scan (zmsmessaging, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmscitizenapi, 8.0)
- GitHub Check: call-php-unit-tests / zmsapi-test
- GitHub Check: call-owasp-security-checks / security-scan (zmscalldisplay, 8.0)
- GitHub Check: Analyze (python)
- GitHub Check: call-owasp-security-checks / security-scan (zmsapi, 8.0)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-owasp-security-checks / security-scan (zmsadmin, 8.0)
🔇 Additional comments (8)
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php (8)
37-42: Good addition of cache key constants.The addition of these constants improves code maintainability by centralizing cache key definitions and reducing the risk of typos or inconsistencies across the codebase.
55-64: Well-structured cache helper method with proper logging.This helper method effectively encapsulates the caching logic with appropriate null checks and logging. Using structured logging with context information (key and TTL) is a good practice.
68-72: Properly implemented caching for getOffices method.The implementation correctly checks for cached data before processing and sets the cache after processing. The cache key includes the showUnpublished parameter to ensure different result sets are properly cached.
Also applies to: 117-121
126-130: Correctly implemented caching for getScopes method.The caching pattern is consistently applied, checking for cached data first and setting cache after processing.
Also applies to: 165-169
174-178: Properly implemented caching for getServices method.The cache key correctly includes the showUnpublished parameter to differentiate between different result sets.
Also applies to: 195-199
204-208: Effective caching implementation for getServicesAndOffices method.The implementation follows the established pattern consistently.
Also applies to: 222-226
294-298: Successfully implemented caching for getOfficeListByServiceId method.The cache key appropriately includes both the service ID and showUnpublished flag to ensure proper cache differentiation.
Also applies to: 335-339
392-396: Well-implemented caching for getServicesByOfficeId method.The implementation maintains consistency with other caching patterns in the class.
Also applies to: 430-434
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php (8)
55-64: Consider adding a docblock and handling failure cases when setting the cache.
While this helper method is straightforward, providing a short docblock could help future maintainers understand its purpose and usage. Additionally, you might add logging or fallback logic in case the caching call fails.
68-72: Reduce duplicated caching logic with a wrapper function.
This snippet for fetching from (and later storing to) the cache is repeated across multiple methods. Consider creating a more generic helper method to handle the entire "load or compute and then cache" pattern, simplifying the code and following DRY principles.
126-130: DRY opportunity for caching pattern.
This snippet duplicates the logic seen in other methods. A dedicated caching workflow helper can enhance maintainability by centralizing this repeated pattern.
174-178: Repeated cache retrieval logic could be extracted into a shared method.
Aim for a unified approach to avoid scattering similar cache checks across methods.
204-208: DRY suggestion for shared cache pattern.
Extend your existing patterns or create a standardized utility to reduce repetition in caching code.
292-295: Existing TODO: Extract mapping logic.
This comment suggests a future refactor. Let me know if you need help opening an issue or implementing this separation.
298-302: Cache gateway pattern is repeated.
Standalone utility functions can centralize cache key generation, retrieval, and storage, reducing future maintenance overhead.
396-400: Repetitive caching code.
Again, a dedicated “retrieve or cache” utility would cleanly unify this logic across all similar getters.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php(10 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.php`: Flag any usage of error_log() as it should be re...
**/*.php: Flag any usage of error_log() as it should be replaced with proper logging mechanisms:
- For error handling: Use a proper logging framework with error levels (PSR-3 LoggerInterface)
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- For application events: Use structured logging with proper log levels
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $logger->error("Import failed", ['error' => $e->getMessage()]);
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: call-owasp-security-checks / security-scan (zmsclient, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsdb, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsslim, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsentities, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsdldb, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsticketprinter, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsstatistic, 8.0)
- GitHub Check: call-php-code-quality / module-code-quality (zmsdb, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsmessaging, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmscitizenapi, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmscalldisplay, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsapi, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsadmin, 8.0)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php (7)
37-42: Good use of descriptive constants for cache keys.
These carefully named constants help maintain clarity and consistency in your caching logic.
117-121: Return statement and caching integration look good.
No issues found in this section; the logic for persisting the processed OfficeList and returning it is clear.
165-169: Result creation and cache storage look fine.
TheThinnedScopeListconstruction and subsequent caching appear correct.
195-199: Cache and return pattern looks clean.
This section appears consistent and logically sound.
222-226: Overall caching flow is correct.
Returning the freshly constructedOfficeServiceAndRelationListafter storing it in the cache is clear and coherent.
339-343: Cache and return flow is properly handled.
No issues here; the resultingOfficeListis cached and returned correctly.
434-438: Clean finalization of caching logic here.
The data is appropriately stored, then returned. No issues detected.
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit