chore: Fix redundant call to resource registry API #1181
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request modifies the Changes
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: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (1)
- src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs (2 hunks)
Additional comments not posted (2)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs (2)
12-12: Addition of Cache Key is AppropriateThe new cache key
ServiceResourceInformationCacheKeyis well-named and consistent with the existing cache keys.
103-123: Efficient Caching ImplementationCaching the result of
FetchServiceResourceInformationusing_cache.GetOrSetAsynceffectively minimizes redundant API calls to the resource registry, enhancing performance as intended.
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs
Show resolved
Hide resolved
…egistry/ResourceRegistryClient.cs Co-authored-by: Ole Jørgen Skogstad <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (1)
- src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs (2 hunks)
Additional comments not posted (4)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs (4)
12-12: LGTM!The addition of the new cache key constant
ServiceResourceInformationCacheKeyis appropriate and follows the naming conventions.
103-126:⚠️ Potential issueSpecify Cache Expiration Policy for Freshness
Currently, the cache duration for
ServiceResourceInformationCacheKeyis not explicitly defined. To ensure that the cached service resource information remains up-to-date with the resource registry, consider specifying a cache expiration policy.You can set the cache duration by providing cache options:
return await _cache.GetOrSetAsync( ServiceResourceInformationCacheKey, async cToken => { // existing code }, + options: new FusionCacheEntryOptions + { + Duration = TimeSpan.FromMinutes(30) // Set an appropriate duration + }, token: cancellationToken);
107-109:⚠️ Potential issueEnsure Exception Handling Within Cache Delegate
The previous concern about exception handling within the cache delegate remains valid. If an exception occurs during the data fetch, it might affect the cache state or propagate unintended exceptions. Ensure exceptions are properly handled within the cache delegate to maintain cache integrity.
Consider wrapping the HTTP request in a try-catch block:
return await _cache.GetOrSetAsync( ServiceResourceInformationCacheKey, async cToken => { + try + { var response = await _client .GetFromJsonEnsuredAsync<List<ResourceListResponse>>(searchEndpoint, cancellationToken: cToken); + // existing code + } + catch (Exception ex) + { + // Handle exception or rethrow with additional context + throw new InvalidOperationException("Failed to fetch service resource information.", ex); + } }, token: cancellationToken);
103-126: 🛠️ Refactor suggestionConsider Redundancy of Multiple Caching Layers
Caching is now implemented in
FetchServiceResourceInformation, and there are additional caches inGetOrSetResourceInformationByOrgandGetOrSetResourceInformationByResourceId. This might introduce redundant caching layers.Evaluate whether caching at both levels is necessary. If the data transformation in the higher-level methods is minimal, you might simplify by relying solely on the lower-level cache to reduce complexity.
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs (3 hunks)
🔇 Additional comments (3)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs (3)
10-14: Consistent Naming of ConstantsThe addition of
ServiceResourceInformationCacheKeyandResourceRegistryResourceEndpointconstants enhances maintainability by centralizing important strings. This is a good practice that improves the readability and manageability of the code.
29-32: Efficient Retrieval of Resources by OrganizationThe method
GetResourceInformationForOrgnow efficiently retrieves resources by filtering the cached data based onOwnerOrgNumber. This change reduces redundant API calls and simplifies the code.
39-41: Simplified Retrieval of Resource Information by IDThe method
GetResourceInformationdirectly retrieves the desired resource from the cached data usingFirstOrDefault, improving performance and reducing complexity.
| var resources = await FetchServiceResourceInformation(cancellationToken); | ||
| return resources | ||
| .Where(x => x.OwnerOrgNumber == orgNumber) | ||
| .ToList(); | ||
| } | ||
|
|
||
| public async Task<ServiceResourceInformation?> GetResourceInformation( | ||
| string serviceResourceId, | ||
| CancellationToken cancellationToken) | ||
| { | ||
| var dic = await GetOrSetResourceInformationByResourceId(cancellationToken); | ||
| dic.TryGetValue(serviceResourceId, out var resource); | ||
| return resource; | ||
| var resources = await FetchServiceResourceInformation(cancellationToken); | ||
| return resources | ||
| .FirstOrDefault(x => x.ResourceId == serviceResourceId); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider Adding Unit Tests to Validate Resource Retrieval Methods
Adding unit tests for GetResourceInformationForOrg and GetResourceInformation will help ensure that the filtering and retrieval logic works correctly, and it will prevent future regressions.
Do you need assistance in writing these unit tests, or would you like me to open a new GitHub issue to track this task?
| private async Task<ServiceResourceInformation[]> FetchServiceResourceInformation(CancellationToken cancellationToken) | ||
| { | ||
| return await _cache.GetOrSetAsync( | ||
| ServiceResourceInformationByOrgCacheKey, | ||
| async cToken => | ||
| { | ||
| var resources = await FetchServiceResourceInformation(cToken); | ||
| return resources | ||
| .GroupBy(x => x.OwnerOrgNumber) | ||
| .ToDictionary(x => x.Key, x => x.ToArray()); | ||
| }, | ||
| token: cancellationToken); | ||
| } | ||
| const string searchEndpoint = $"{ResourceRegistryResourceEndpoint}resourcelist"; | ||
|
|
||
| private async Task<Dictionary<string, ServiceResourceInformation>> GetOrSetResourceInformationByResourceId( | ||
| CancellationToken cancellationToken) | ||
| { | ||
| return await _cache.GetOrSetAsync( | ||
| ServiceResourceInformationByResourceIdCacheKey, | ||
| ServiceResourceInformationCacheKey, | ||
| async cToken => | ||
| { | ||
| var resources = await FetchServiceResourceInformation(cToken); | ||
| return resources.ToDictionary(x => x.ResourceId); | ||
| var response = await _client | ||
| .GetFromJsonEnsuredAsync<List<ResourceListResponse>>(searchEndpoint, | ||
| cancellationToken: cToken); | ||
|
|
||
| return response | ||
| .Where(x => !string.IsNullOrWhiteSpace(x.HasCompetentAuthority.Organization)) | ||
| .Where(x => x.ResourceType is | ||
| ResourceTypeGenericAccess or | ||
| ResourceTypeAltinnApp or | ||
| ResourceTypeCorrespondence) | ||
| .Select(x => new ServiceResourceInformation( | ||
| $"{Constants.ServiceResourcePrefix}{x.Identifier}", | ||
| x.ResourceType, | ||
| x.HasCompetentAuthority.Organization!)) | ||
| .ToArray(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider Filtering Resources at the API Level to Improve Performance
In FetchServiceResourceInformation, all resources are fetched and then filtered in-memory. If the number of resources is large, this could lead to performance issues due to the amount of data being transferred and processed.
Consider modifying the API request to include query parameters that filter resources on the server side. This would reduce the amount of data transferred over the network and minimize in-memory processing.
For example, adjust the searchEndpoint to include filters:
-const string searchEndpoint = $"{ResourceRegistryResourceEndpoint}resourcelist";
+const string searchEndpoint = $"{ResourceRegistryResourceEndpoint}resourcelist?filter=ResourceType:{ResourceTypeGenericAccess},{ResourceTypeAltinnApp},{ResourceTypeCorrespondence}";Ensure that the API supports filtering by ResourceType and that HasCompetentAuthority.Organization is not null or empty.
Committable suggestion was skipped due to low confidence.
🛠️ Refactor suggestion
Consider Specifying Cache Expiration Policy for Freshness
The cache duration for ServiceResourceInformationCacheKey is not explicitly defined. To keep the cached service resource information up-to-date with the resource registry, consider specifying a cache expiration policy.
You can set the cache duration by providing cache options:
return await _cache.GetOrSetAsync(
ServiceResourceInformationCacheKey,
async cToken =>
{
// existing code
},
+ options: new FusionCacheEntryOptions
+ {
+ Duration = TimeSpan.FromMinutes(30) // Set an appropriate duration
+ },
token: cancellationToken);This ensures that the cache refreshes periodically, keeping the data current.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async Task<ServiceResourceInformation[]> FetchServiceResourceInformation(CancellationToken cancellationToken) | |
| { | |
| return await _cache.GetOrSetAsync( | |
| ServiceResourceInformationByOrgCacheKey, | |
| async cToken => | |
| { | |
| var resources = await FetchServiceResourceInformation(cToken); | |
| return resources | |
| .GroupBy(x => x.OwnerOrgNumber) | |
| .ToDictionary(x => x.Key, x => x.ToArray()); | |
| }, | |
| token: cancellationToken); | |
| } | |
| const string searchEndpoint = $"{ResourceRegistryResourceEndpoint}resourcelist"; | |
| private async Task<Dictionary<string, ServiceResourceInformation>> GetOrSetResourceInformationByResourceId( | |
| CancellationToken cancellationToken) | |
| { | |
| return await _cache.GetOrSetAsync( | |
| ServiceResourceInformationByResourceIdCacheKey, | |
| ServiceResourceInformationCacheKey, | |
| async cToken => | |
| { | |
| var resources = await FetchServiceResourceInformation(cToken); | |
| return resources.ToDictionary(x => x.ResourceId); | |
| var response = await _client | |
| .GetFromJsonEnsuredAsync<List<ResourceListResponse>>(searchEndpoint, | |
| cancellationToken: cToken); | |
| return response | |
| .Where(x => !string.IsNullOrWhiteSpace(x.HasCompetentAuthority.Organization)) | |
| .Where(x => x.ResourceType is | |
| ResourceTypeGenericAccess or | |
| ResourceTypeAltinnApp or | |
| ResourceTypeCorrespondence) | |
| .Select(x => new ServiceResourceInformation( | |
| $"{Constants.ServiceResourcePrefix}{x.Identifier}", | |
| x.ResourceType, | |
| x.HasCompetentAuthority.Organization!)) | |
| .ToArray(); | |
| private async Task<ServiceResourceInformation[]> FetchServiceResourceInformation(CancellationToken cancellationToken) | |
| { | |
| const string searchEndpoint = $"{ResourceRegistryResourceEndpoint}resourcelist"; | |
| return await _cache.GetOrSetAsync( | |
| ServiceResourceInformationCacheKey, | |
| async cToken => | |
| { | |
| var response = await _client | |
| .GetFromJsonEnsuredAsync<List<ResourceListResponse>>(searchEndpoint, | |
| cancellationToken: cToken); | |
| return response | |
| .Where(x => !string.IsNullOrWhiteSpace(x.HasCompetentAuthority.Organization)) | |
| .Where(x => x.ResourceType is | |
| ResourceTypeGenericAccess or | |
| ResourceTypeAltinnApp or | |
| ResourceTypeCorrespondence) | |
| .Select(x => new ServiceResourceInformation( | |
| $"{Constants.ServiceResourcePrefix}{x.Identifier}", | |
| x.ResourceType, | |
| x.HasCompetentAuthority.Organization!)) | |
| .ToArray(); | |
| }, | |
| options: new FusionCacheEntryOptions | |
| { | |
| Duration = TimeSpan.FromMinutes(30) // Set an appropriate duration | |
| }, | |
| token: cancellationToken); |
Ensure Exception Handling Within Cache Delegate
If an exception occurs during the HTTP request in FetchServiceResourceInformation, it could affect cache integrity or propagate unintended exceptions. To maintain stability, consider adding exception handling within the cache delegate.
Wrap the HTTP request in a try-catch block:
return await _cache.GetOrSetAsync(
ServiceResourceInformationCacheKey,
async cToken =>
{
+ try
+ {
var response = await _client
.GetFromJsonEnsuredAsync<List<ResourceListResponse>>(searchEndpoint,
cancellationToken: cToken);
return response
.Where(x => !string.IsNullOrWhiteSpace(x.HasCompetentAuthority.Organization))
.Where(x => x.ResourceType is
ResourceTypeGenericAccess or
ResourceTypeAltinnApp or
ResourceTypeCorrespondence)
.Select(x => new ServiceResourceInformation(
$"{Constants.ServiceResourcePrefix}{x.Identifier}",
x.ResourceType,
x.HasCompetentAuthority.Organization!))
.ToArray();
+ }
+ catch (Exception ex)
+ {
+ // Handle exception appropriately
+ throw new InvalidOperationException("Failed to fetch service resource information.", ex);
+ }
},
token: cancellationToken);This ensures that any exceptions are properly handled, and the cache remains consistent.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async Task<ServiceResourceInformation[]> FetchServiceResourceInformation(CancellationToken cancellationToken) | |
| { | |
| return await _cache.GetOrSetAsync( | |
| ServiceResourceInformationByOrgCacheKey, | |
| async cToken => | |
| { | |
| var resources = await FetchServiceResourceInformation(cToken); | |
| return resources | |
| .GroupBy(x => x.OwnerOrgNumber) | |
| .ToDictionary(x => x.Key, x => x.ToArray()); | |
| }, | |
| token: cancellationToken); | |
| } | |
| const string searchEndpoint = $"{ResourceRegistryResourceEndpoint}resourcelist"; | |
| private async Task<Dictionary<string, ServiceResourceInformation>> GetOrSetResourceInformationByResourceId( | |
| CancellationToken cancellationToken) | |
| { | |
| return await _cache.GetOrSetAsync( | |
| ServiceResourceInformationByResourceIdCacheKey, | |
| ServiceResourceInformationCacheKey, | |
| async cToken => | |
| { | |
| var resources = await FetchServiceResourceInformation(cToken); | |
| return resources.ToDictionary(x => x.ResourceId); | |
| var response = await _client | |
| .GetFromJsonEnsuredAsync<List<ResourceListResponse>>(searchEndpoint, | |
| cancellationToken: cToken); | |
| return response | |
| .Where(x => !string.IsNullOrWhiteSpace(x.HasCompetentAuthority.Organization)) | |
| .Where(x => x.ResourceType is | |
| ResourceTypeGenericAccess or | |
| ResourceTypeAltinnApp or | |
| ResourceTypeCorrespondence) | |
| .Select(x => new ServiceResourceInformation( | |
| $"{Constants.ServiceResourcePrefix}{x.Identifier}", | |
| x.ResourceType, | |
| x.HasCompetentAuthority.Organization!)) | |
| .ToArray(); | |
| private async Task<ServiceResourceInformation[]> FetchServiceResourceInformation(CancellationToken cancellationToken) | |
| { | |
| const string searchEndpoint = $"{ResourceRegistryResourceEndpoint}resourcelist"; | |
| return await _cache.GetOrSetAsync( | |
| ServiceResourceInformationCacheKey, | |
| async cToken => | |
| { | |
| try | |
| { | |
| var response = await _client | |
| .GetFromJsonEnsuredAsync<List<ResourceListResponse>>(searchEndpoint, | |
| cancellationToken: cToken); | |
| return response | |
| .Where(x => !string.IsNullOrWhiteSpace(x.HasCompetentAuthority.Organization)) | |
| .Where(x => x.ResourceType is | |
| ResourceTypeGenericAccess or | |
| ResourceTypeAltinnApp or | |
| ResourceTypeCorrespondence) | |
| .Select(x => new ServiceResourceInformation( | |
| $"{Constants.ServiceResourcePrefix}{x.Identifier}", | |
| x.ResourceType, | |
| x.HasCompetentAuthority.Organization!)) | |
| .ToArray(); | |
| } | |
| catch (Exception ex) | |
| { | |
| // Handle exception appropriately | |
| throw new InvalidOperationException("Failed to fetch service resource information.", ex); | |
| } | |
| }, | |
| token: cancellationToken); |
Description
This is due to the application caching two separate dictionaries of info from the resource registry, grouped by different keys.
Added caching of the raw response from the resource registry and removed the dependent caches to lower complexity.
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes