fix: Reenable party list cache, log party name look failure with negative cache TTL #1395
Conversation
elsand
changed the title
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to two classes: Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 1
🧹 Outside diff range and nitpick comments (2)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs (2)
Line range hint
1-215: Consider splitting caching responsibilities.The class currently handles both PDP and parties caching, which could be separated to follow the Single Responsibility Principle. Consider extracting the caching logic into dedicated cache providers for better maintainability and testability.
This would allow for:
- Independent cache configuration for different concerns
- Easier testing of caching strategies
- More focused error handling
Line range hint
134-139: Enhance error logging for cache misses.Consider adding detailed logging when no authorized parties are returned. This would help track the frequency and context of cache misses, especially for cases of missing Altinn profiles.
if (authorizedPartiesDto is null || authorizedPartiesDto.Count == 0) { + _logger.LogWarning( + "No authorized parties returned for request {@Request}. This may indicate a missing Altinn profile.", + authorizedPartiesRequest); throw new UpstreamServiceException("access-management returned no authorized parties, missing Altinn profile?"); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/NameRegistry/PartyNameRegistryClient.cs(3 hunks)
🔇 Additional comments (5)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs (1)
84-85: LGTM! Verify cache configuration.
The reactivation of caching for authorized parties aligns with the PR objectives. The implementation looks correct and maintains proper error handling through PerformAuthorizedPartiesRequest.
Let's verify the cache configuration to ensure it aligns with the PR objectives:
✅ Verification successful
Cache configuration verified and properly implemented
The cache implementation for authorized parties is correctly configured with appropriate TTL and security measures:
- Cache duration is set to 15 minutes (TimeSpan.FromMinutes(15))
- Cache key generation is secure using SHA256 hashing
- Memory cache is enabled for authorized parties since it has low cardinality
- Redis is configured as distributed cache with proper backplane setup
The implementation aligns perfectly with the PR objectives of reenabling party list cache.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify cache configuration and key generation
# Test 1: Look for cache configuration
rg -l "FusionCache.*Configuration.*TTL"
# Test 2: Verify the cache key generation implementation
ast-grep --pattern 'GenerateCacheKey()'
Length of output: 83
Script:
#!/bin/bash
# Let's try different approaches to find cache configuration and key generation
# Look for cache configuration with broader patterns
rg -A 5 "FusionCache|CacheOptions|TTL"
# Look for cache key generation method with broader pattern
ast-grep --pattern 'public string GenerateCacheKey'
# Look for the request class that might contain the key generation
rg -A 5 "AuthorizedPartiesRequest"
# Look for cache setup in startup/configuration
rg -A 5 "AddFusionCache|ConfigureFusionCache"
Length of output: 29543
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/NameRegistry/PartyNameRegistryClient.cs (4)
7-7: Approved: Added logging dependency
The inclusion of using Microsoft.Extensions.Logging; integrates the logging capabilities required for enhanced error handling.
16-16: Approved: Declared logger field
The declaration of the logger field _logger is appropriate and follows best practices for dependency injection.
24-27: Approved: Updated constructor with logger injection
The constructor correctly injects the ILogger<PartyNameRegistryClient> dependency and includes a null check, ensuring robustness.
33-45: Approved: Enhanced caching mechanism with negative caching
The use of GetOrSetAsync<string?> with a specified type parameter improves clarity. The implementation of a short negative cache for null results optimizes performance for missing data scenarios.
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/NameRegistry/PartyNameRegistryClient.cs
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs
Show resolved
Hide resolved
|
🤖 I have created a release *beep* *boop* --- ## [1.39.0](v1.38.0...v1.39.0) (2024-11-22) ### Features * **azure:** adjust SKU and storage for yt01 and prod ([b7e4909](b7e4909)) * **azure:** adjust SKU and storage for yt01 and prod ([#1508](#1508)) ([5478275](5478275)) * **graphql:** Create separate type for sub-parties ([#1510](#1510)) ([9c75f11](9c75f11)) ### Bug Fixes * **azure:** ensure correct properties are used when adjusting SKU and storage for postgres ([#1514](#1514)) ([c51d2f5](c51d2f5)) * Reenable party list cache, log party name look failure with negative cache TTL ([#1395](#1395)) ([d18bb76](d18bb76)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
Party list cache was by a mistake disabled, this reenables it. Also, add a log entry when party name lookup fails due to remote API response indicates no name (usually due to missing party profile). Cache negative responses for just 10 seconds (instead of the default 24 hours).
Related Issue(s)
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Refactor