-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Add separate settings for parties cache, don't cache invalid response from Altinn 2 #1194
Conversation
…sponse from Altinn 2
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications primarily focused on enhancing caching mechanisms and error handling within 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs (6 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/Common/Exceptions/UpstreamServiceException.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs (1 hunks)
🔇 Additional comments not posted (6)
src/Digdir.Domain.Dialogporten.Infrastructure/Common/Exceptions/UpstreamServiceException.cs (1)
4-8
: LGTM! Well-structured exception class with improved flexibility.The changes to the
UpstreamServiceException
class are well-implemented and provide several benefits:
- The explicit class definition with braces improves readability.
- The addition of a new constructor taking a string message increases flexibility in creating exceptions.
- The
internal sealed
modifier appropriately restricts the class's accessibility and prevents unintended inheritance.- Implementing
IUpstreamServiceError
provides a clear contract for upstream service errors.These modifications enhance the exception handling capabilities while maintaining good coding practices.
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs (5)
22-23
: Separation of caches enhances modularityIntroducing separate cache variables
_pdpCache
and_partiesCache
improves the separation of concerns, allowing for independent caching strategies for PDP and authorized parties data.
60-60
: Correct use of_pdpCache
for dialog authorizationThe
GetDialogDetailsAuthorization
method now appropriately uses_pdpCache
to cache dialog details authorization requests. This ensures that PDP-related caching is consistent and isolated.
78-78
: Consistent caching in search authorizationThe method
GetAuthorizedResourcesForSearch
correctly utilizes_pdpCache
, maintaining consistency in caching PDP requests across different methods.
86-86
: Proper caching of authorized partiesUsing
_partiesCache
inGetAuthorizedParties
separates the caching of authorized parties from PDP caching. This aligns with the PR objective to introduce distinct cache settings for parties.
124-128
: Exception handling for empty authorized parties responseThrowing an
UpstreamServiceException
whenauthorizedPartiesDto
isnull
or empty effectively handles cases where users lack an Altinn profile. This change ensures that such scenarios are flagged as errors rather than caching and potentially misrepresenting empty data.
src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs
Show resolved
Hide resolved
Quality Gate passedIssues Measures |
🤖 I have created a release *beep* *boop* --- ## [1.20.1](v1.20.0...v1.20.1) (2024-10-02) ### Bug Fixes * Add separate settings for parties cache, don't cache invalid response from Altinn 2 ([#1194](#1194)) ([dbb79dc](dbb79dc)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
🤖 I have created a release *beep* *boop* --- ## [1.20.1](v1.20.0...v1.20.1) (2024-10-02) ### Bug Fixes * Add separate settings for parties cache, don't cache invalid response from Altinn 2 ([#1194](#1194)) ([dbb79dc](dbb79dc)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
Currently, if the user is missing a profile in Altinn 2, the reportee list returned from all auth/am APIs are empty. This is an error condition, so we treat it as such and don't cache the empty response.
Also, this introduces a separate cache setting for parties, as this does not have the cardinality issues as the PDP cache, allowing us to utilize memory cache for this.
Related Issue(s)
Verification
Summary by CodeRabbit
New Features
Bug Fixes
UpstreamServiceException
class to allow custom messages.Documentation