Added caching of AuthroizedParties to GetAuthorizedParties #606
Conversation
📝 WalkthroughWalkthroughThe pull request introduces caching functionality to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/Altinn.Correspondence.Integrations/Altinn/AccessManagement/AltinnAccessManagementService.cs (2)
7-7: Consider extracting caching logic to a decorator pattern.The current implementation mixes caching responsibility with the core service logic, violating the Single Responsibility Principle. Consider:
- Implementing a decorator pattern to separate caching concerns
- Moving cache configuration to application settings
Example decorator pattern implementation:
public class CachedAltinnAccessManagementService : IAltinnAccessManagementService { private readonly IAltinnAccessManagementService _inner; private readonly IDistributedCache _cache; private readonly DistributedCacheEntryOptions _cacheOptions; public CachedAltinnAccessManagementService( IAltinnAccessManagementService inner, IDistributedCache cache, IOptions<CacheOptions> cacheOptions) { _inner = inner; _cache = cache; _cacheOptions = new DistributedCacheEntryOptions { AbsoluteExpirationRelativeToNow = cacheOptions.Value.ExpirationTime }; } // Implement caching logic here, delegating to _inner for actual work }Also applies to: 20-21
Line range hint
72-81: Improve error handling with specific exceptions.The error handling could be more specific and informative.
Consider creating custom exceptions:
public class AccessManagementException : Exception { public HttpStatusCode StatusCode { get; } public AccessManagementException(HttpStatusCode statusCode, string message, Exception? inner = null) : base($"Access Management API error: {message}. Status code: {statusCode}", inner) { StatusCode = statusCode; } }Then update the error handling:
if (!response.IsSuccessStatusCode) { - _logger.LogError(await response.Content.ReadAsStringAsync(cancellationToken)); - _logger.LogError("Error code in call against Authorization GetAuthorizedParties"); - throw new Exception("Error code in call against Authorization GetAuthorizedParties: " + response.StatusCode); + var content = await response.Content.ReadAsStringAsync(cancellationToken); + _logger.LogError("Failed to get authorized parties. Status: {StatusCode}, Content: {Content}", + response.StatusCode, content); + throw new AccessManagementException(response.StatusCode, content); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Altinn.Correspondence.Integrations/Altinn/AccessManagement/AltinnAccessManagementService.cs(4 hunks)
🔇 Additional comments (1)
src/Altinn.Correspondence.Integrations/Altinn/AccessManagement/AltinnAccessManagementService.cs (1)
Line range hint
72-81: Fix typo in class name "AuthroizedPartiesResponse".The class name contains a typo: "Authroized" should be "Authorized".
Apply this diff:
- internal sealed class AuthroizedPartiesResponse + internal sealed class AuthorizedPartiesResponseRemember to update all references to this class name in the codebase.
✅ Verification successful
Typo fix is isolated to a single file
The typo in "AuthroizedPartiesResponse" only needs to be fixed in
AltinnAccessManagementService.cs. Since the class is internal and all references are contained within this file, no other files in the codebase will be impacted by this change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all occurrences of the typo in the codebase rg "Authroized" --type csLength of output: 578
src/Altinn.Correspondence.Integrations/Altinn/AccessManagement/AltinnAccessManagementService.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Correspondence.Integrations/Altinn/AccessManagement/AltinnAccessManagementService.cs
Show resolved
Hide resolved
…prevent application crash on cache failure
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Altinn.Correspondence.Integrations/Altinn/AccessManagement/AltinnAccessManagementService.cs (1)
23-36: 🛠️ Refactor suggestionAdd null checks and externalize cache configuration.
The constructor needs validation for injected dependencies and the cache expiration time should be configurable.
This is a duplicate of a previous review comment. The suggested improvements include:
- Adding null checks for all dependencies
- Externalizing cache configuration
- Proper initialization order
🧹 Nitpick comments (1)
src/Altinn.Correspondence.Integrations/Altinn/AccessManagement/AltinnAccessManagementService.cs (1)
Line range hint
74-93: Enhance cache storage reliability and performance.While basic error handling is in place, the implementation could benefit from additional reliability features.
Consider implementing:
- Retry mechanism for transient cache failures
- Background cache refresh to prevent cache stampede
- Sliding cache expiration for frequently accessed items
Example implementation for retry mechanism:
+private async Task SetCacheWithRetryAsync(string key, string value, DistributedCacheEntryOptions options, CancellationToken cancellationToken) +{ + const int maxRetries = 3; + for (int i = 0; i < maxRetries; i++) + { + try + { + await _cache.SetStringAsync(key, value, options, cancellationToken); + return; + } + catch (Exception ex) when (i < maxRetries - 1) + { + _logger.LogWarning(ex, $"Failed to set cache (attempt {i + 1}/{maxRetries})"); + await Task.Delay(TimeSpan.FromMilliseconds(100 * Math.Pow(2, i)), cancellationToken); + } + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Altinn.Correspondence.Integrations/Altinn/AccessManagement/AltinnAccessManagementService.cs(4 hunks)
🔇 Additional comments (2)
src/Altinn.Correspondence.Integrations/Altinn/AccessManagement/AltinnAccessManagementService.cs (2)
7-7: LGTM! Necessary additions for caching implementation.The using statement and field declarations are correctly added to support distributed caching functionality.
Also applies to: 20-21
41-53: 🛠️ Refactor suggestionImprove cache key construction and error handling.
The current cache key construction needs improvement to handle special characters and potential conflicts.
Apply this diff to improve the implementation:
-string cacheKey = $"AuthorizedParties_{partyToRequestFor.PartyId}"; +string cacheKey = $"altinn:correspondence:authorizedparties:{partyToRequestFor.PartyId}";Consider adding:
- Key prefix to prevent conflicts with other services
- Key sanitization to handle special characters
- Key length validation
Likely invalid or redundant comment.
Description
Performance tests have revealed that the call to the AccessManagement API is a performance bottleneck. To address this, this PR introduces caching of AuthorizedParties to the AccessManagment service. By caching this data, unnecessary API calls are avoided, which helps reduce latency and significantly improve overall system performance.
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Summary by CodeRabbit