fix: Collapse subject resource mappings before building sql query #1579
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces significant changes to the authorization logic within the dialog port application. Key modifications include the restructuring of the Changes
Possibly related PRs
Suggested reviewers
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: 4
🧹 Outside diff range and nitpick comments (1)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs (1)
179-227: Optimize Loop Efficiency inCollapseSubjectResourcesThe
CollapseSubjectResourcesmethod iterates over parties and authorized roles. Consider optimizing the loops to reduce computational complexity, especially if the number of parties or roles is large.For example, you might parallelize the loop or optimize data structures for faster access if performance becomes an issue.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
src/Digdir.Domain.Dialogporten.Application/Common/Extensions/DbSetExtensions.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Externals/AltinnAuthorization/DialogSearchAuthorizationResult.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs(6 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/LocalDevelopmentAltinnAuthorization.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs(2 hunks)
🔇 Additional comments (8)
src/Digdir.Domain.Dialogporten.Application/Common/Extensions/DbSetExtensions.cs (1)
23-30: Verify Correctness of Grouping Logic in PrefilterAuthorizedDialogs
The grouping operation in PrefilterAuthorizedDialogs relies on HashSetEqualityComparer<T>. Ensure that the equality comparer correctly compares hash sets to avoid incorrect grouping of parties and resources.
Please confirm that HashSetEqualityComparer<T> provides consistent and accurate comparisons for grouping, as any issues here could lead to improperly constructed SQL queries.
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs (1)
Line range hint 149-175: Ensure Correct Integration of CollapseSubjectResources Method
The method PerformDialogSearchAuthorization now calls CollapseSubjectResources. Verify that this integration correctly consolidates subject resources and that all authorized resources are accounted for.
Please ensure that the CollapseSubjectResources method accurately updates dialogSearchAuthorizationResult and that there are no regressions in authorization logic.
src/Digdir.Domain.Dialogporten.Application/Externals/AltinnAuthorization/DialogSearchAuthorizationResult.cs (2)
7-7: Update ResourcesByParties to Use HashSet<string>
Changing ResourcesByParties to use HashSet<string> ensures uniqueness of resources and improves lookup performance.
12-12: Verify Logic of HasNoAuthorizations Property
With the removal of SubjectsByParties, ensure that the HasNoAuthorizations property still correctly reflects authorization status based on ResourcesByParties and DialogIds.
Please confirm that this change does not introduce any unintended side effects in authorization checks.
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/LocalDevelopmentAltinnAuthorization.cs (2)
43-43: Use of HashSet for Resource Collection
Converting allResources to a HashSet<string> ensures resource uniqueness and may improve performance when adding to ResourcesByParties.
47-47: Consistent Use of HashSet<string> in ResourcesByParties
The update to initialize ResourcesByParties with HashSet<string> aligns with changes elsewhere in the codebase and ensures consistent data types.
src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs (2)
19-19: LGTM!
The new import is correctly placed and necessary for the SubjectResource cache configuration.
164-168: Verify cache configuration consistency and memory impact
Let's verify if similar cache configurations exist elsewhere in the codebase and assess the potential memory impact.
✅ Verification successful
Based on the search results, I can now make an informed assessment about the cache configuration:
- The
SubjectResourceentity is a relatively small entity with just basic fields (Id, Subject, Resource, UpdatedAt). - The cache is used in
AltinnAuthorizationClientto store the entire list of subject resources, which is queried frequently for authorization checks. - The data is updated through a sync process that merges changes from the resource registry.
- The table has a unique index on (Resource, Subject) columns, suggesting a controlled growth pattern.
Let's generate the final response:
Cache duration of 20 minutes is appropriate for SubjectResource
The 20-minute cache duration is well-balanced considering:
- The data is synced periodically from an external source (resource registry)
- The entity structure is lightweight
- The cache is used for frequent authorization checks
- The table has controlled growth through unique constraints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for similar cache configurations and estimate memory impact
# Check for other FusionCache configurations
echo "Searching for other cache configurations..."
rg -A 5 "ConfigureFusionCache.*SubjectResource"
# Check the size of SubjectResource table/collection
echo "Checking SubjectResource usage..."
rg -A 5 "SubjectResource" --type cs
Length of output: 67226
src/Digdir.Domain.Dialogporten.Application/Common/Extensions/DbSetExtensions.cs
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Application/Common/Extensions/DbSetExtensions.cs
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs
Show resolved
Hide resolved
|
e2e tests work as expected, but as this introduces quite a bit of logic it would be appropiate with some more unit/integration tests here. I'm looking into it. Edit: refactored and added test for CollapseSubjectResources and DbSetExtensions |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/DbSetExtensionsTests.cs (1)
68-71: Optimize Whitespace Removal in Test Helper MethodThe
RemoveWhitespacemethod could be optimized for better performance by using aStringBuilderinstead of concatenating strings, which can be inefficient in loops.Apply this diff to improve the method:
private static string RemoveWhitespace(string input) { - return string.Concat(input.Where(c => !char.IsWhiteSpace(c))); + var sb = new StringBuilder(input.Length); + foreach (var c in input) + { + if (!char.IsWhiteSpace(c)) + { + sb.Append(c); + } + } + return sb.ToString(); }tests/Digdir.Domain.Dialogporten.Infrastructure.Unit.Tests/AuthorizationHelperTests.cs (4)
10-50: Consider enhancing test coverage and maintainability.While the test data setup is comprehensive, consider the following improvements:
- Make the test name more specific to describe the exact scenario being tested (e.g.,
CollapseSubjectResources_WithMultipleRolesAndParties_ShouldCollapseToUniqueResources).- Add more test methods to cover edge cases:
- Empty resource lists
- Null inputs
- No matching resources
- All resources filtered out by constraints
- Consider using test data builders or constants for better maintainability.
Example test data builder approach:
private static class TestData { public static class Roles { public const string Role1 = "role1"; public const string Role2 = "role2"; public const string Role3 = "role3"; } public static class Resources { public const string Resource1 = "resource1"; public const string Resource2 = "resource2"; // ... } public static AuthorizedPartiesResult CreateAuthorizedParties() => new() { AuthorizedParties = new List<AuthorizedParty> { new() { Party = "party1", AuthorizedRoles = new[] { Roles.Role1, Roles.Role2 } }, // ... } }; }
57-64: Consider adding performance validation.Given that this PR aims to optimize search authorization performance, consider:
- Adding performance benchmarks or assertions
- Extracting the act part into a reusable method for multiple test cases
Example implementation:
private static async Task<TimeSpan> MeasureExecutionTime(Func<Task> action) { var sw = System.Diagnostics.Stopwatch.StartNew(); await action(); sw.Stop(); return sw.Elapsed; } [Fact] public async Task CollapseSubjectResources_ShouldCompleteWithinTimeLimit() { // Arrange var maxDuration = TimeSpan.FromMilliseconds(100); // Act var duration = await MeasureExecutionTime(() => AuthorizationHelper.CollapseSubjectResources( // ... parameters ... )); // Assert Assert.True(duration <= maxDuration, $"Execution took {duration.TotalMilliseconds}ms, " + $"which exceeds the limit of {maxDuration.TotalMilliseconds}ms"); }
65-77: Enhance assertion coverage and readability.The current assertions could be improved by:
- Adding negative case validation (e.g., party3 should not be present)
- Using more fluent assertions for better readability
- Ensuring order-independent collection comparisons
Example improvements:
// Assert var expected = new Dictionary<string, HashSet<string>> { ["party1"] = new() { "resource1", "resource2", "resource4" }, ["party2"] = new() { "resource2", "resource4" } }; Assert.Equal(expected.Keys, dialogSearchAuthorizationResult.ResourcesByParties.Keys); Assert.All(expected, kvp => { Assert.True(dialogSearchAuthorizationResult.ResourcesByParties.ContainsKey(kvp.Key)); Assert.Equal(kvp.Value, dialogSearchAuthorizationResult.ResourcesByParties[kvp.Key]); }); // Verify party3 is not present (negative case) Assert.DoesNotContain("party3", dialogSearchAuthorizationResult.ResourcesByParties.Keys);
1-78: Add more test scenarios for comprehensive coverage.Given the significant changes to authorization logic mentioned in the PR objectives, consider adding these test scenarios:
Large-scale tests:
- Test with a high number of parties (100+)
- Test with many resources per party
- Validate performance improvements
Edge cases:
- Empty or null inputs
- Duplicate resources across roles
- Invalid party/role combinations
Integration scenarios:
- End-to-end flow with actual database calls
- Caching behavior
- SQL query generation validation
Would you like assistance in generating these additional test scenarios?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Digdir.Domain.Dialogporten.Application/Common/Extensions/DbSetExtensions.cs(2 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs(6 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AuthorizationHelper.cs(1 hunks)tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/DbSetExtensionsTests.cs(1 hunks)tests/Digdir.Domain.Dialogporten.Infrastructure.Unit.Tests/AuthorizationHelperTests.cs(1 hunks)
🔇 Additional comments (5)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AuthorizationHelper.cs (1)
23-24:
Filter Subject Resources at the Database Level
The method getAllSubjectResources retrieves all SubjectResource entries and filters them in memory. This could lead to performance issues with large datasets.
Consider modifying getAllSubjectResources to accept parameters for filtering, so that the filters can be applied directly at the database level, retrieving only the necessary records.
Apply this diff to fix the issue:
public static async Task CollapseSubjectResources(
DialogSearchAuthorizationResult dialogSearchAuthorizationResult,
AuthorizedPartiesResult authorizedParties,
List<string> constraintResources,
- Func<CancellationToken, Task<List<SubjectResource>>> getAllSubjectResources,
+ Func<HashSet<string>, List<string>, CancellationToken, Task<List<SubjectResource>>> getFilteredSubjectResources,
CancellationToken cancellationToken)
{
var authorizedPartiesWithRoles = authorizedParties.AuthorizedParties
.Where(p => p.AuthorizedRoles.Count != 0)
.ToList();
var uniqueSubjects = authorizedPartiesWithRoles
.SelectMany(p => p.AuthorizedRoles)
.ToHashSet();
- var subjectResources = (await getAllSubjectResources(cancellationToken))
- .Where(x => uniqueSubjects.Contains(x.Subject) && (constraintResources.Count == 0 || constraintResources.Contains(x.Resource)))
- .ToList();
+ var subjectResources = await getFilteredSubjectResources(uniqueSubjects, constraintResources, cancellationToken);
// ... rest of the code ...
}src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs (1)
184-187:
Filter Subject Resources at the Database Level
The GetAllSubjectResources method retrieves all SubjectResource entries and filters them in memory within CollapseSubjectResources. This could lead to performance issues with large datasets.
Consider modifying GetAllSubjectResources to accept parameters for filtering, so that filters can be applied directly in the database query, retrieving only the necessary records.
Apply this diff to fix the issue:
- private async Task<List<SubjectResource>> GetAllSubjectResources(CancellationToken cancellationToken) =>
- await _subjectResourcesCache.GetOrSetAsync(nameof(SubjectResource), async ct
- => await _dialogDbContext.SubjectResources.ToListAsync(cancellationToken: ct),
- token: cancellationToken);
+ private async Task<List<SubjectResource>> GetFilteredSubjectResources(
+ HashSet<string> subjects,
+ List<string> constraintResources,
+ CancellationToken cancellationToken) =>
+ await _subjectResourcesCache.GetOrSetAsync(nameof(SubjectResource), async ct
+ => await _dialogDbContext.SubjectResources
+ .Where(x => subjects.Contains(x.Subject) &&
+ (constraintResources.Count == 0 || constraintResources.Contains(x.Resource)))
+ .ToListAsync(cancellationToken: ct),
+ token: cancellationToken);And update the call to CollapseSubjectResources:
await AuthorizationHelper.CollapseSubjectResources(
dialogSearchAuthorizationResult,
authorizedParties,
request.ConstraintServiceResources,
- GetAllSubjectResources,
+ GetFilteredSubjectResources,
cancellationToken);src/Digdir.Domain.Dialogporten.Application/Common/Extensions/DbSetExtensions.cs (2)
30-40:
Ensure Parameter Indices Match in SQL Construction
Using parameters.Count and parameters.Count + 1 within the query string before adding new parameters to parameters may lead to index mismatches if parameters is modified elsewhere.
Consider capturing the current index before appending to ensure consistency.
Apply this diff to fix the issue:
foreach (var (resources, parties) in groupedResult)
{
+ var index = parameters.Count;
sb.AppendLine(CultureInfo.InvariantCulture, $"""
OR (
- "{nameof(DialogEntity.Party)}" = ANY(@p{parameters.Count})
- AND "{nameof(DialogEntity.ServiceResource)}" = ANY(@p{parameters.Count + 1})
+ "{nameof(DialogEntity.Party)}" = ANY(@p{index})
+ AND "{nameof(DialogEntity.ServiceResource)}" = ANY(@p{index + 1})
)
""");
parameters.Add(parties);
parameters.Add(resources);
}61-65:
Ensure Consistent Hash Codes in HashSetEqualityComparer<T>
The GetHashCode method may produce inconsistent results due to the unordered nature of HashSet<T>. This inconsistency can lead to incorrect grouping behavior in dictionaries or hash-based collections.
To ensure consistent hash codes for equal sets, consider ordering the elements before computing the hash code.
Apply this diff to fix the issue:
public int GetHashCode(HashSet<T> obj)
{
ArgumentNullException.ThrowIfNull(obj);
unchecked
{
- return obj.Aggregate(0, (hash, item) => hash ^ (item?.GetHashCode() ?? 0));
+ int hash = 17;
+ foreach (var item in obj.OrderBy(e => e))
+ {
+ hash = hash * 31 + (item?.GetHashCode() ?? 0);
+ }
+ return hash;
}
}tests/Digdir.Domain.Dialogporten.Infrastructure.Unit.Tests/AuthorizationHelperTests.cs (1)
1-9: LGTM! Clean test structure and appropriate imports.
The test class follows good practices with proper namespace organization and relevant imports.
tests/Digdir.Domain.Dialogporten.Infrastructure.Unit.Tests/AuthorizationHelperTests.cs
Show resolved
Hide resolved
|
🤖 I have created a release *beep* *boop* --- ## [1.41.3](v1.41.2...v1.41.3) (2024-12-13) ### Bug Fixes * **azure:** adjust SKU and storage for staging ([#1601](#1601)) ([3fb9f95](3fb9f95)) * Collapse subject resource mappings before building sql query ([#1579](#1579)) ([b39c376](b39c376)) * **webapi:** Explicit null on non-nullable lists no longer causes 500 INTERNAL SERVER ERROR ([#1602](#1602)) ([2e8b3e6](2e8b3e6)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
This implements a few optimizations for search authorization, which particularly improves performance for users with access to a large number of parties.
Related Issue(s)
Verification