feat: Synchronization of resource policy metadata #1411
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several changes primarily focused on the deployment of a scheduled job within Azure using Bicep files. It includes the creation of parameter files for different environments (production, staging, test, and a specific environment named yt01), which define job scheduling and resource configurations. Additionally, it enhances the data access layer by introducing new repository interfaces and classes for managing resource policy information, along with updates to existing services and commands for synchronization tasks. 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
|
...ain.Dialogporten.Infrastructure/Persistence/Repositories/ResourcePolicyMetadataRepository.cs
Outdated
Show resolved
Hide resolved
…nged as much as possible in ConvertDomainEventsToOutboxMessagesInterceptor.cs. (#1423)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (40)
src/Digdir.Domain.Dialogporten.Domain/ResourcePolicyInformation/ResourcePolicyInformation.cs (1)
5-6: Add XML documentation for the public API.Since this class represents an important domain entity for resource policy information, consider adding XML documentation to describe its purpose and usage.
+/// <summary> +/// Represents resource policy information containing security level requirements for a resource. +/// </summary> public sealed class ResourcePolicyInformation : IEntitysrc/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/Synchronize/SynchronizeResourcePolicyInformationCommandValidator.cs (1)
5-11: Consider enhancing the validator implementation.While the basic validation is in place, there are several improvements that could make the code more maintainable and user-friendly:
- Document the rationale for the concurrent requests range (1-50)
- Extract magic numbers into named constants
- Add custom error messages for better user experience
Here's a suggested improvement:
internal sealed class SynchronizeResourcePolicyInformationCommandValidator : AbstractValidator<SynchronizeResourcePolicyInformationCommand> { + private const int MinConcurrentRequests = 1; + private const int MaxConcurrentRequests = 50; + public SynchronizeResourcePolicyInformationCommandValidator() { - RuleFor(x => x.NumberOfConcurrentRequests).InclusiveBetween(1, 50); + RuleFor(x => x.NumberOfConcurrentRequests) + .InclusiveBetween(MinConcurrentRequests, MaxConcurrentRequests) + .WithMessage("Number of concurrent requests must be between {MinValue} and {MaxValue}"); } }.azure/applications/sync-resource-policy-information-job/prod.bicepparam (1)
1-11: Consider adding parameter validation.Consider adding parameter validation to ensure required environment variables are present. Here's a suggested improvement:
using './main.bicep' param environment = 'prod' param location = 'norwayeast' +@minLength(1) param imageTag = readEnvironmentVariable('IMAGE_TAG') param jobSchedule = '*/30 * * * *' // Runs every 30 minutes //secrets +@minLength(1) param containerAppEnvironmentName = readEnvironmentVariable('AZURE_CONTAINER_APP_ENVIRONMENT_NAME') +@minLength(1) param environmentKeyVaultName = readEnvironmentVariable('AZURE_ENVIRONMENT_KEY_VAULT_NAME') +@secure() +@minLength(1) param appInsightConnectionString = readEnvironmentVariable('AZURE_APP_INSIGHTS_CONNECTION_STRING').azure/applications/sync-resource-policy-information-job/test.bicepparam (1)
1-11: Consider implementing parameter validation and documentationTo improve maintainability and prevent configuration drift:
- Consider adding parameter validation in the main.bicep template
- Document the expected values and constraints for each parameter
- Consider creating a parameter schema that can be shared across different environment parameter files (test, prod, staging)
Would you like assistance in generating parameter validation rules or documentation templates?
.azure/applications/sync-resource-policy-information-job/yt01.bicepparam (1)
1-11: Document the purpose and configuration of this job.Since this is a new scheduled job for syncing resource policy information, it would be beneficial to:
- Add comments explaining the job's purpose and impact
- Document the expected duration of each run
- Specify any dependencies or prerequisites
- Describe the consequences of job failure
Add a header comment block like this:
+ # Resource Policy Information Sync Job + # + # Purpose: Synchronizes resource policy metadata between systems + # Schedule: Runs every 30 minutes + # Dependencies: + # - Azure Container Apps Environment + # - Key Vault access + # - Application Insights for monitoring + # Failure Impact: [Describe what happens if the job fails] + using './main.bicep'.azure/applications/sync-resource-policy-information-job/staging.bicepparam (1)
5-6: Consider optimizing the job schedule and adding environment variable validation.The current schedule runs every 30 minutes, which might be too frequent for a staging environment. Consider:
- Reducing the frequency to hourly or less to minimize resource usage
- Adding validation for the IMAGE_TAG environment variable to prevent deployment failures
-param imageTag = readEnvironmentVariable('IMAGE_TAG') -param jobSchedule = '*/30 * * * *' // Runs every 30 minutes +param imageTag = readEnvironmentVariable('IMAGE_TAG', true) // Add validation +param jobSchedule = '0 * * * *' // Runs hourlysrc/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/HostingEnvironmentExtensions.cs (2)
6-13: LGTM with suggestions for robustness.The extension method implementation is clean and follows good practices. However, we could make the string comparison more robust.
Consider this improvement:
internal static class HostingEnvironmentExtensions { public static bool ShouldRunDevelopmentHostedService(this IHostEnvironment environment) { // Only run in development environments, but not when using Janitor - return environment.IsDevelopment() && !(Assembly.GetEntryAssembly()?.GetName().Name ?? "").Contains("Janitor"); + return environment.IsDevelopment() && + !string.Equals( + Assembly.GetEntryAssembly()?.GetName().Name ?? string.Empty, + "Janitor", + StringComparison.OrdinalIgnoreCase); } }This change:
- Uses explicit string comparison instead of Contains
- Uses StringComparison.OrdinalIgnoreCase for case-insensitive matching
- Uses string.Empty instead of "" for consistency
4-4: Consider documenting the namespace purpose.Since this is in a Development namespace, it would be helpful to add a comment explaining that this code is intended for development environments only.
Add XML documentation:
+/// <summary> +/// Contains extensions and utilities specifically for development environment configuration. +/// Not intended for use in production environments. +/// </summary> namespace Digdir.Domain.Dialogporten.Infrastructure.Persistence.Development;src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Configurations/ResourcePolicyInformation/ResourcePolicyInformationConfiguration.cs (1)
6-14: LGTM! Consider enhancing the index configuration.The implementation is correct, but could be improved with the following suggestions:
builder - .HasIndex(r => new { r.Resource }) - .IsUnique(); + .HasIndex(r => new { r.Resource }, "IX_ResourcePolicyInformation_Resource") + .IsUnique() + .HasFilter(null) // Add filter if soft delete is implemented + .HasDatabaseName("IX_ResourcePolicyInformation_Resource"); // Explicit name for better maintainabilitysrc/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentMigratorHostedService.cs (2)
20-23: Consider adding tests for environment-specific behavior.Since this service handles critical database migrations, we should ensure the environment check behavior is thoroughly tested.
Consider adding tests that:
- Verify migrations run in the correct environments
- Verify migrations are skipped in non-development environments
- Test edge cases in environment configuration
Would you like me to help create these test cases?
20-20: Document the new environment check method.The introduction of
ShouldRunDevelopmentHostedService()seems to be a significant change in how development services are controlled.Consider adding XML documentation to the extension method explaining:
- Its purpose and when it should be used
- The conditions it checks
- How it differs from the standard
IsDevelopment()checksrc/Digdir.Domain.Dialogporten.Janitor/README.md (3)
15-16: LGTM! Consider adding batch size impact details.The parameters are well documented. However, it would be helpful to add a brief note about how the batch size affects performance or resource usage.
- `-b` *Optional*: Override the batch size (default: 1000). + - `-b` *Optional*: Override the batch size for processing subject-resource mappings. Higher values may improve performance but increase memory usage (default: 1000).🧰 Tools
🪛 Markdownlint
15-15: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
16-16: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
18-25: Enhance documentation for concurrency behavior.While the command parameters are documented, additional information would be valuable for operations:
- How are failed requests handled during concurrent execution?
- Is there a timeout setting for individual requests?
- What's the recommended concurrency value for different environments?
- `-c` *Optional*: Number of concurrent requests to fetch policies (default: 10). + - `-c` *Optional*: Number of concurrent requests to fetch policies (default: 10). Adjust based on available system resources and API rate limits. Failed requests are retried up to 3 times before the command fails.🧰 Tools
🪛 Markdownlint
24-24: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
25-25: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
15-16: Fix list indentation for better markdown formatting.The unordered lists are currently using 4-space indentation. According to markdown standards, lists should be indented with 2 spaces.
- - `-s` *Optional*: Override the time of the last synchronization. This argument should be a `DateTimeOffset`, e.g., `2024-08-15` (default: newest in local copy) - - `-b` *Optional*: Override the batch size (default: 1000). + - `-s` *Optional*: Override the time of the last synchronization. This argument should be a `DateTimeOffset`, e.g., `2024-08-15` (default: newest in local copy) + - `-b` *Optional*: Override the batch size (default: 1000). - - `-s` *Optional*: Override the time of the last synchronization. This argument should be a `DateTimeOffset`, e.g., `2024-08-15` (default: newest in local copy) - - `-c` *Optional*: Number of concurrent requests to fetch policies (default: 10). + - `-s` *Optional*: Override the time of the last synchronization. This argument should be a `DateTimeOffset`, e.g., `2024-08-15` (default: newest in local copy) + - `-c` *Optional*: Number of concurrent requests to fetch policies (default: 10).Also applies to: 24-25
🧰 Tools
🪛 Markdownlint
15-15: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
16-16: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentResourcePolicyInformationSyncHostedService.cs (3)
8-12: Add XML documentation to explain the service's purpose.Consider adding XML documentation to describe:
- The purpose of this development-only service
- When it runs and what it synchronizes
- Why it's sealed (prevents inheritance as it's not designed for extension)
+/// <summary> +/// Development-only hosted service that synchronizes resource policy information during startup. +/// This service is sealed as it's not designed for extension and only runs in development environments. +/// </summary> internal sealed class DevelopmentResourcePolicyInformationSyncHostedService : IHostedService
13-17: Consider injecting required services directly instead of IServiceProvider.The use of
IServiceProvidersuggests a service locator pattern, which can make dependencies less explicit and harder to test. Consider injectingISenderdirectly if it's the only required service.-private readonly IServiceProvider _serviceProvider; +private readonly ISender _sender; -public DevelopmentResourcePolicyInformationSyncHostedService(IServiceProvider serviceProvider, IHostEnvironment environment) +public DevelopmentResourcePolicyInformationSyncHostedService(ISender sender, IHostEnvironment environment) { - _serviceProvider = serviceProvider ?? throw new ArgumentNullException(nameof(serviceProvider)); + _sender = sender ?? throw new ArgumentNullException(nameof(sender)); _environment = environment ?? throw new ArgumentNullException(nameof(environment)); }
32-33: Add a comment explaining the empty StopAsync implementation.Consider documenting why no cleanup is needed when the service stops.
+ /// <summary> + /// No cleanup needed as the synchronization is a one-time operation during startup. + /// </summary> public Task StopAsync(CancellationToken cancellationToken) => Task.CompletedTask;src/Digdir.Domain.Dialogporten.Application/Externals/IResourceRegistry.cs (1)
31-32: Consider adding validation for MinimumSecurityLevel.The
MinimumSecurityLevelproperty inUpdatedResourcePolicyInformationis defined as anintwithout any apparent constraints. Consider:
- Using an enum if there are predefined security levels
- Adding validation to ensure the value falls within an acceptable range
- Adding XML documentation to specify the valid range and meaning of different levels
Example implementation with an enum:
public enum SecurityLevel { Level0 = 0, Level1 = 1, Level2 = 2, Level3 = 3, Level4 = 4 } public sealed record UpdatedResourcePolicyInformation(Uri ResourceUrn, SecurityLevel MinimumSecurityLevel, DateTimeOffset UpdatedAt);src/Digdir.Domain.Dialogporten.Janitor/Commands.cs (2)
29-30: Add parameter validation and documentation.Consider adding:
- XML documentation to describe command purpose and parameters
- Validation for
numberOfConcurrentRequeststo ensure it's positive+ /// <summary> + /// Synchronizes resource policy information. + /// </summary> + /// <param name="ctx">The Cocona app context</param> + /// <param name="application">The mediator sender</param> + /// <param name="since">Optional timestamp to sync from</param> + /// <param name="numberOfConcurrentRequests">Optional number of concurrent requests (must be positive)</param> app.AddCommand("sync-resource-policy-information", async ( [FromService] CoconaAppContext ctx, [FromService] ISender application, [Option('s')] DateTimeOffset? since, - [Option('c')] int? numberOfConcurrentRequests) + [Option('c', Description = "Number of concurrent requests")] int? numberOfConcurrentRequests)
29-30: Consider standardizing option flags across commands.The existing command uses
-bfor batch size while the new command uses-cfor concurrent requests. Consider standardizing these option flags if they serve similar purposes.Also applies to: 8-14
src/Digdir.Domain.Dialogporten.Janitor/Digdir.Domain.Dialogporten.Janitor.csproj (1)
33-39: Ensure local settings file is properly ignored.Since
appsettings.local.jsontypically contains development-specific configurations, ensure it's added to.gitignoreto prevent accidental commits of local settings.Add this entry to
.gitignoreif not already present:+appsettings.local.jsonsrc/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241106143110_AddResourcePolicyInformation.cs (1)
29-34: Consider adding additional indexes for performanceWhile the unique index on Resource is good for ensuring data integrity, consider adding an index on MinimumSecurityLevel if you plan to query based on this column frequently.
+ migrationBuilder.CreateIndex( + name: "IX_ResourcePolicyInformation_MinimumSecurityLevel", + table: "ResourcePolicyInformation", + column: "MinimumSecurityLevel");src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentCleanupOutboxHostedService.cs (1)
Line range hint
7-12: Update XML documentation to reflect the new execution condition.The class XML documentation currently only mentions "development environments" but should be updated to reflect the new, potentially more specific condition for execution.
/// <summary> -/// This hosted service is only active in development environments. It cleans up the +/// This hosted service is only active when ShouldRunDevelopmentHostedService() returns true. It cleans up the /// outbox/event tables. However, it keeps the last 12 hours of data to allow for /// debugging throughout one workday. /// </summary>src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/LocalDevelopmentResourceRegistry.cs (1)
46-48: Add documentation for the new method.The implementation looks good and follows the established patterns. Consider adding XML documentation to explain:
- The purpose of this method in the local development context
- Why it returns an empty array
- The meaning of the parameters (since they're intentionally unused)
Example:
/// <summary> /// Returns an empty collection as this is a local development implementation. /// </summary> /// <param name="since">Timestamp to get updates since (unused in local development)</param> /// <param name="numberOfConcurrentRequests">Maximum concurrent requests (unused in local development)</param> /// <param name="cancellationToken">Cancellation token</param> /// <returns>Empty collection of resource policy information</returns>.azure/applications/sync-resource-policy-information-job/main.bicep (3)
34-36: Parameterize the base image URLThe base image URL is currently hardcoded. Consider making it a parameter to support different container registries in different environments or scenarios.
Apply this diff:
+@description('The base URL for container images') +@minLength(3) +param baseImageUrl string = 'ghcr.io/digdir/dialogporten-' + var namePrefix = 'dp-be-${environment}' -var baseImageUrl = 'ghcr.io/digdir/dialogporten-'
49-66: Consider adding connection string validation at startupWhile the connection strings are properly configured as secrets, consider adding runtime validation in the application startup to fail fast if these connections are invalid.
This could be implemented in the application's Program.cs or startup configuration:
public static IHostBuilder CreateHostBuilder(string[] args) => Host.CreateDefaultBuilder(args) .ConfigureServices((context, services) => { // Validate connection strings at startup var dbConnection = context.Configuration["Infrastructure:DialogDbConnectionString"]; var redisConnection = context.Configuration["Infrastructure:Redis:ConnectionString"]; if (!ValidateConnectionString(dbConnection)) throw new InvalidOperationException("Invalid database connection string"); if (!ValidateRedisConnectionString(redisConnection)) throw new InvalidOperationException("Invalid Redis connection string"); });
85-98: Add semantic version validation for imageTagConsider adding a regex pattern validation to ensure the imageTag follows semantic versioning.
Apply this diff at the parameters section:
@description('The tag of the image to be used') @minLength(3) +@pattern('^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$') param imageTag stringsrc/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/Synchronize/SynchronizeSubjectResourceMappingsCommand.cs (3)
10-14: Add XML documentation for the public command class and its properties.Since this is a public API, adding XML documentation would improve maintainability and help consumers understand the purpose and usage of the command.
+/// <summary> +/// Command to synchronize subject resource mappings from the resource registry. +/// </summary> public sealed class SynchronizeSubjectResourceMappingsCommand : IRequest<SynchronizeResourceMappingsResult> { + /// <summary> + /// Optional timestamp to synchronize changes since a specific point in time. + /// </summary> public DateTimeOffset? Since { get; set; } + /// <summary> + /// Optional size of batches to process during synchronization. + /// </summary> public int? BatchSize { get; set; } }
Line range hint
43-46: Document the purpose of the 1 microsecond time skew.The time skew value seems arbitrary. Consider adding a comment explaining why this specific value was chosen and its importance in preventing data synchronization issues.
var lastUpdated = request.Since ?? await _subjectResourceRepository.GetLastUpdatedAt( + // Add 1 microsecond to prevent re-processing the last synchronized item timeSkew: TimeSpan.FromMicroseconds(1), cancellationToken: cancellationToken);
Line range hint
50-85: Consider improving transaction and error handling.A few suggestions to enhance the robustness of the synchronization:
- The transaction scope covers the entire sync operation, which might lead to long-running transactions. Consider using smaller transaction batches.
- Add cancellation token checks in the main processing loop.
- Catch specific exceptions (e.g., DbException, TimeoutException) before catching the generic Exception.
try { var mergeCount = 0; var syncTime = DateTimeOffset.Now; - await _unitOfWork.BeginTransactionAsync(cancellationToken); await foreach (var resourceBatch in _resourceRegistry .GetUpdatedSubjectResources(lastUpdated, request.BatchSize ?? DefaultBatchSize, cancellationToken)) { + cancellationToken.ThrowIfCancellationRequested(); + await _unitOfWork.BeginTransactionAsync(cancellationToken); var mergeableSubjectResources = resourceBatch .Select(x => x.ToMergableSubjectResource(syncTime)) .ToList(); var batchMergeCount = await _subjectResourceRepository.Merge(mergeableSubjectResources, cancellationToken); + await _unitOfWork.SaveChangesAsync(cancellationToken); _logger.LogInformation("{BatchMergeCount} subject-resources added to transaction.", batchMergeCount); mergeCount += batchMergeCount; } - await _unitOfWork.SaveChangesAsync(cancellationToken); if (mergeCount > 0) { _logger.LogInformation("Successfully synced {UpdatedAmount} total subject-resources. Changes committed.", mergeCount); } else { _logger.LogInformation("Subject-resources are already up-to-date."); } return new Success(); } +catch (DbException e) +{ + _logger.LogError(e, "Database error while syncing subject-resources. Rolling back transaction."); + throw; +} +catch (TimeoutException e) +{ + _logger.LogError(e, "Timeout while syncing subject-resources. Rolling back transaction."); + throw; +} catch (Exception e) { _logger.LogError(e, "Failed to sync subject-resources. Rolling back transaction."); throw; }.github/workflows/workflow-deploy-apps.yml (1)
246-246: Documentation update required.As per PR objectives, please ensure:
- Documentation is added in the
docsdirectory explaining the resource policy synchronization job- Update Altinnpedia if applicable
- If needed, create a linked PR in the Altinn Studio documentation repository
src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs (2)
89-93: LGTM! Consider adding documentation.The new transient services are well-structured:
- Repository registration follows the established pattern
- Lazy loading of message handling services is appropriate for integration testing
Consider adding XML documentation to the
IResourcePolicyInformationRepositoryinterface to describe its purpose and usage.
230-245: Consider reducing configuration duplication.The pub-only configuration duplicates much of the pub/sub configuration. Consider extracting common configuration into a shared method to improve maintainability.
Example approach:
+ private static void ConfigureCommonMassTransitOptions(IBusRegistrationConfigurator x, InfrastructureBuilderContext builderContext) + { + x.AddEntityFrameworkOutbox<DialogDbContext>(o => + { + o.UsePostgres(); + o.UseBusOutbox(); + }); + + if (builderContext.Environment.IsDevelopment() && builderContext.DevSettings.UseInMemoryServiceBusTransport) + { + x.UsingInMemory(); + return; + } + } internal static void AddPubSubCapabilities(...) { builderContext.Services.AddMassTransit(x => { - x.AddEntityFrameworkOutbox<DialogDbContext>(...); - if (builderContext.Environment.IsDevelopment()...) - { - x.UsingInMemory(); - return; - } + ConfigureCommonMassTransitOptions(x, builderContext); // Add pub/sub specific configuration }); } internal static void AddPubCapabilities(...) { builderContext.Services.AddMassTransit(x => { - x.AddEntityFrameworkOutbox<DialogDbContext>(...); - if (builderContext.Environment.IsDevelopment()...) - { - x.UsingInMemory(); - return; - } + ConfigureCommonMassTransitOptions(x, builderContext); // Add pub specific configuration }); }src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Repositories/ResourcePolicyInformationRepository.cs (1)
33-48: Review usage of interpolated strings in SQL command.While using
nameofensures that the table and column names reflect the code definitions, interpolated strings in SQL commands can pose risks if any of the interpolated values can be influenced by external input.Verify that all interpolated values in the SQL command are safe and cannot be tampered with. If necessary, consider using parameterized queries for table and column names or building the SQL command using safer methods.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/Synchronize/SynchronizeResourcePolicyInformationCommand.cs (2)
49-49: UseUtcNowinstead ofNowfor time zone consistencyConsider using
DateTimeOffset.UtcNowto ensure synchronization times are consistent across different time zones.Apply this diff to update the timestamp:
-var syncTime = DateTimeOffset.Now; +var syncTime = DateTimeOffset.UtcNow;
26-34: Simplify constructor null checks using C# 9.0 featuresYou can simplify the null checks in the constructor by using the concise syntax available in newer C# versions for improved readability.
Apply this diff to streamline the constructor:
-public SynchronizeResourcePolicyInformationCommandHandler( - IResourceRegistry resourceRegistry, - IResourcePolicyInformationRepository resourcePolicyMetadataRepository, - ILogger<SynchronizeResourcePolicyInformationCommandHandler> logger) -{ - _resourceRegistry = resourceRegistry ?? throw new ArgumentNullException(nameof(resourceRegistry)); - _resourcePolicyMetadataRepository = resourcePolicyMetadataRepository ?? throw new ArgumentNullException(nameof(resourcePolicyMetadataRepository)); - _logger = logger ?? throw new ArgumentNullException(nameof(logger)); -} +public SynchronizeResourcePolicyInformationCommandHandler( + IResourceRegistry resourceRegistry, + IResourcePolicyInformationRepository resourcePolicyMetadataRepository, + ILogger<SynchronizeResourcePolicyInformationCommandHandler> logger) => + (_resourceRegistry, _resourcePolicyMetadataRepository, _logger) = + (resourceRegistry ?? throw new ArgumentNullException(nameof(resourceRegistry)), + resourcePolicyMetadataRepository ?? throw new ArgumentNullException(nameof(resourcePolicyMetadataRepository)), + logger ?? throw new ArgumentNullException(nameof(logger)));src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs (3)
28-32: Consider parameter ordering in the constructorPlacing the
loggerparameter afterclientandcacheProvidermaintains consistency and readability.
151-151: Log the full exception to capture detailed error informationLogging only
ex.Messageomits stack trace and inner exceptions. Logging the full exception provides more context for debugging.Apply this diff to enhance logging:
- _logger.LogError("Failed to process policy for \"{ResourceUrn}\": {ExceptionMessage}", resource.ResourceUrn, ex.Message); + _logger.LogError(ex, "Failed to process policy for \"{ResourceUrn}\"", resource.ResourceUrn);
221-228: Consider makingResourceRegistryEntrya struct or recordSince
ResourceRegistryEntryis a simple data holder, using areadonly structorrecordcan be more efficient and convey intent.src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Interceptors/ConvertDomainEventsToOutboxMessagesInterceptor.cs (1)
39-39: Remove Redundant Call toEnsureLazyLoadedServices()inSavingChangesAsyncThe method
EnsureLazyLoadedServices()is called at both line 39 and line 60 withinSavingChangesAsync. Since domain events are only processed when_domainEventsis not empty, andEnsureLazyLoadedServices()is necessary only in that case, the first call is redundant. Consider removing the initial call to avoid unnecessary initialization when there are no domain events.Apply this diff to remove the redundant call:
public override async ValueTask<InterceptionResult<int>> SavingChangesAsync( DbContextEventData eventData, InterceptionResult<int> result, CancellationToken cancellationToken = default) { - EnsureLazyLoadedServices(); var dbContext = eventData.Context; if (dbContext is null) { return await base.SavingChangesAsync(eventData, result, cancellationToken); } _domainEvents = dbContext.ChangeTracker.Entries() .SelectMany(x => x.Entity is IEventPublisher publisher ? publisher.PopDomainEvents() : []) .ToList(); if (_domainEvents.Count == 0) { return await base.SavingChangesAsync(eventData, result, cancellationToken); } EnsureLazyLoadedServices(); foreach (var domainEvent in _domainEvents) { domainEvent.OccuredAt = _transactionTime.Value; } await Task.WhenAll(_domainEvents .Select(x => _publishEndpoint.Value .Publish(x, x.GetType(), cancellationToken))); return await base.SavingChangesAsync(eventData, result, cancellationToken); }Also applies to: 60-60
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241106143110_AddResourcePolicyInformation.Designer.csis excluded by!**/Migrations/**/*Designer.cssrc/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/DialogDbContextModelSnapshot.csis excluded by!**/Migrations/DialogDbContextModelSnapshot.cs
📒 Files selected for processing (31)
.azure/applications/sync-resource-policy-information-job/main.bicep(1 hunks).azure/applications/sync-resource-policy-information-job/prod.bicepparam(1 hunks).azure/applications/sync-resource-policy-information-job/staging.bicepparam(1 hunks).azure/applications/sync-resource-policy-information-job/test.bicepparam(1 hunks).azure/applications/sync-resource-policy-information-job/yt01.bicepparam(1 hunks).github/workflows/workflow-deploy-apps.yml(1 hunks)src/Digdir.Domain.Dialogporten.Application/Externals/IDialogDbContext.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Externals/IResourcePolicyInformationRepository.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Externals/IResourceRegistry.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/Synchronize/SynchronizeResourcePolicyInformationCommand.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/Synchronize/SynchronizeResourcePolicyInformationCommandValidator.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/Synchronize/SynchronizeSubjectResourceMappingsCommand.cs(1 hunks)src/Digdir.Domain.Dialogporten.Domain/ResourcePolicyInformation/ResourcePolicyInformation.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/LocalDevelopmentResourceRegistry.cs(2 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs(4 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs(4 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Configurations/ResourcePolicyInformation/ResourcePolicyInformationConfiguration.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentCleanupOutboxHostedService.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentMigratorHostedService.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentResourcePolicyInformationSyncHostedService.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentSubjectResourceSyncHostedService.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/HostingEnvironmentExtensions.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/DialogDbContext.cs(2 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Interceptors/ConvertDomainEventsToOutboxMessagesInterceptor.cs(6 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241106143110_AddResourcePolicyInformation.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Repositories/ResourcePolicyInformationRepository.cs(1 hunks)src/Digdir.Domain.Dialogporten.Janitor/Commands.cs(1 hunks)src/Digdir.Domain.Dialogporten.Janitor/Digdir.Domain.Dialogporten.Janitor.csproj(1 hunks)src/Digdir.Domain.Dialogporten.Janitor/Properties/launchSettings.json(1 hunks)src/Digdir.Domain.Dialogporten.Janitor/README.md(1 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Common/DialogApplication.cs(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Interceptors/ConvertDomainEventsToOutboxMessagesInterceptor.cs (3)
Learnt from: MagnusSandgren
PR: digdir/dialogporten#1277
File: src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Interceptors/ConvertDomainEventsToOutboxMessagesInterceptor.cs:58-60
Timestamp: 2024-10-17T09:56:14.054Z
Learning: In the `ConvertDomainEventsToOutboxMessagesInterceptor` class located at `src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Interceptors/ConvertDomainEventsToOutboxMessagesInterceptor.cs`, using MassTransit's transaction outbox functionality ensures that events published using `_publishEndpoint.Publish` in the `SavingChangesAsync` method are handled transactionally, maintaining consistency without the need to move event publishing to `SavedChangesAsync` or retaining the outbox pattern.
Learnt from: oskogstad
PR: digdir/dialogporten#1187
File: src/Digdir.Domain.Dialogporten.Infrastructure/DomainEvents/Outbox/ConvertDomainEventsToOutboxMessagesInterceptor.cs:105-122
Timestamp: 2024-10-08T15:39:24.789Z
Learning: In the `ConvertDomainEventsToOutboxMessagesInterceptor` class, using a static in-memory cache (`UpdateEventThrottleCache`) for throttling in the `SendDialogUpdated` method is acceptable because only one instance of the WebAPI receives the REST request that generates these events. The throttling is meant to be local to prevent clients from receiving multiple notifications when both an update event and an activity created event are generated in the same transaction.
Learnt from: oskogstad
PR: digdir/dialogporten#1187
File: src/Digdir.Domain.Dialogporten.Infrastructure/DomainEvents/Outbox/ConvertDomainEventsToOutboxMessagesInterceptor.cs:105-122
Timestamp: 2024-09-24T22:26:57.178Z
Learning: In the `ConvertDomainEventsToOutboxMessagesInterceptor` class, using a static in-memory cache (`UpdateEventThrottleCache`) for throttling in the `SendDialogUpdated` method is acceptable because only one instance of the WebAPI receives the REST request that generates these events. The throttling is meant to be local to prevent clients from receiving multiple notifications when both an update event and an activity created event are generated in the same transaction.
🪛 Markdownlint
src/Digdir.Domain.Dialogporten.Janitor/README.md
15-15: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
16-16: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
24-24: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
25-25: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
🔇 Additional comments (56)
src/Digdir.Domain.Dialogporten.Domain/ResourcePolicyInformation/ResourcePolicyInformation.cs (1)
1-3: LGTM! File structure follows conventions.
The namespace organization and imports are clean and follow the project's structure.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/Synchronize/SynchronizeResourcePolicyInformationCommandValidator.cs (1)
1-4: LGTM! Good choice of validation framework and namespace organization.
The use of FluentValidation and the namespace structure follow good practices.
src/Digdir.Domain.Dialogporten.Janitor/Properties/launchSettings.json (2)
3-9: LGTM! Profile rename improves clarity.
The profile rename from "Digdir.Domain.Dialogporten.Janitor" to "subject-resources" better reflects its specific functionality.
10-15: Verify concurrency setting and add documentation.
The new profile introduces resource policy metadata synchronization with a concurrency limit of 1 (-c 1).
- Please verify if the single concurrency setting is intentional for the development environment.
- Consider documenting the command parameters in the PR description or README.
Let me help verify the command documentation:
✅ Verification successful
Concurrency setting is valid and well-defined
The concurrency setting of 1 (-c 1) in the development profile is acceptable as:
- The command has built-in validation that enforces concurrency between 1-50 (see
SynchronizeResourcePolicyInformationCommandValidator) - If not specified, it defaults to 15 concurrent requests (see
DefaultNumberOfConcurrentRequests = 15) - The setting can be overridden via command line, making the development profile's conservative setting a safe default
Consider documenting the following in the README:
- Command:
sync-resource-policy-metadata - Options:
-c: Number of concurrent requests (1-50, default: 15)-s: Since timestamp (optional)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the sync-resource-policy-metadata command is documented
rg -A 5 "sync-resource-policy-metadata" "README.md"
Length of output: 49
Script:
#!/bin/bash
# Search for command implementation to understand concurrency usage
ast-grep --pattern 'class $_ {
$$$
sync-resource-policy-metadata($$$) {
$$$
}
$$$
}'
# Search for any documentation or usage examples in source files
rg -i "sync-resource-policy-metadata" --type md --type cs -A 5
Length of output: 161
Script:
#!/bin/bash
# Try different pattern to find the command implementation
rg "sync.*resource.*policy.*metadata" --type cs -A 10
# Look for command line argument parsing
rg "AddCommand.*resource.*policy" --type cs -A 5
# Search for any configuration or settings files related to resource policy
fd -e json -e yaml -e yml -x rg -l "resource.*policy"
Length of output: 880
Script:
#!/bin/bash
# Check the command implementation details
rg -B 5 -A 15 "sync-resource-policy-information" src/Digdir.Domain.Dialogporten.Janitor/Commands.cs
# Look for any related configuration in swagger docs
rg "resource.*policy" docs/schema/V1/swagger.verified.json -A 5
Length of output: 9524
Script:
#!/bin/bash
# Look for the command handler implementation
rg "SynchronizeResourcePolicyInformationCommand" --type cs -B 5 -A 15
# Check for any related configuration files
fd -e config -e settings -x rg -l "NumberOfConcurrentRequests"
Length of output: 14280
.azure/applications/sync-resource-policy-information-job/prod.bicepparam (3)
1-4: LGTM! Basic configuration looks appropriate for production.
The environment and location settings are correctly specified for a production deployment in Norway East region.
5-6: Verify the job schedule configuration.
The cron expression '*/30 * * * *' will run the job every 30 minutes, which could be resource-intensive for a production environment. Please ensure:
- This frequency aligns with the business requirements
- The operation is idempotent
- Proper error handling and retry mechanisms are in place
Consider implementing a dead-letter queue or error logging mechanism if not already present in the main.bicep file.
8-11: Review secret management practices.
While using environment variables for sensitive configuration is good practice, please ensure:
- These environment variables are properly set in the Azure DevOps pipeline
- The Key Vault access policies are configured with minimum required permissions
Run this script to check for any hardcoded secrets or connection strings in the codebase:
✅ Verification successful
Let me gather more information about the environment variable usage and key vault configurations.
Let me try one more search with different patterns to find the relevant configurations.
Based on the extensive codebase analysis, I can now provide a final response regarding the secret management practices.
Secret management appears to be properly implemented with secure practices.
The codebase shows:
- Environment variables are read using
readEnvironmentVariable()function and properly passed through secure parameters - Key Vault integration is implemented with RBAC-based access control
- Secrets are managed through Azure Key Vault with proper access policies
- Connection strings and sensitive configurations are stored as secrets in Key Vault
- Container apps and jobs use managed identities to access Key Vault secrets
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential hardcoded secrets or connection strings
# Look for patterns that might indicate hardcoded secrets
rg -i "connectionstring|secret|password|key|token" \
--type-add "bicep:*.{bicep,bicepparam}" \
-g "*.{bicep,bicepparam,yaml,yml}"
Length of output: 1107
Script:
#!/bin/bash
# Check for environment variable references and key vault configurations in bicep files
rg -i "keyVault|environment" -g "*.{bicep,bicepparam}" -A 3 -B 3
Length of output: 215
Script:
#!/bin/bash
# Check Azure pipeline configurations for these environment variables
fd -e yml -e yaml . .azure/ -x grep -l "AZURE_CONTAINER_APP_ENVIRONMENT_NAME\|AZURE_ENVIRONMENT_KEY_VAULT_NAME\|AZURE_APP_INSIGHTS_CONNECTION_STRING"
Length of output: 151
Script:
#!/bin/bash
# Look for key vault access policy configurations
rg "access-policy|accessPolicies" -g "*.{bicep,json,yaml,yml}" -A 5
Length of output: 69
Script:
#!/bin/bash
# Search for bicep and pipeline files with broader patterns
fd -e bicep -e bicepparam -e yml -e yaml . .azure/ -x cat {}
Length of output: 115861
Script:
#!/bin/bash
# Look for environment variable usage in pipeline configurations
rg -i "env\.|environment\." -g "*.{yml,yaml}" -A 3 -B 3
Length of output: 57
Script:
#!/bin/bash
# Check for key vault references in the entire codebase
rg -i "@Microsoft.KeyVault|vault" -g "*.{bicep,bicepparam,yml,yaml}" -A 3 -B 3
Length of output: 80
.azure/applications/sync-resource-policy-information-job/test.bicepparam (3)
3-6: Verify region selection and cron schedule
The configuration looks good, but please confirm:
- Is 'norwayeast' the intended region for the test environment?
- Is the 30-minute interval appropriate for the test environment? Consider if this frequency needs to be different from production.
8-11: Verify secret management in deployment pipeline
The secret management approach using environment variables is secure and follows best practices. However, please ensure:
- These environment variables are properly configured in your deployment pipeline
- The key vault and app insights resources exist in the test environment
#!/bin/bash
# Check if these environment variables are referenced in workflow files
echo "Checking workflow files for environment variable references..."
rg "AZURE_(CONTAINER_APP_ENVIRONMENT_NAME|ENVIRONMENT_KEY_VAULT_NAME|APP_INSIGHTS_CONNECTION_STRING)" .github/workflows/1-1: Verify main.bicep template existence and parameter compatibility
Let's ensure the referenced template exists and that all required parameters are properly mapped.
✅ Verification successful
Template reference is valid and parameters are properly defined
The referenced main.bicep template exists in the same directory and contains all necessary parameter declarations including:
- imageTag
- environment
- location
- containerAppEnvironmentName
- environmentKeyVaultName
- jobSchedule
- appInsightConnectionString
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if main.bicep exists and verify its parameter declarations
if [ -f ".azure/applications/sync-resource-policy-information-job/main.bicep" ]; then
echo "Checking main.bicep parameters..."
# Look for parameter declarations in main.bicep
rg "param \w+" ".azure/applications/sync-resource-policy-information-job/main.bicep"
else
echo "Error: main.bicep not found in expected location"
exit 1
fi
Length of output: 5991
Script:
#!/bin/bash
# Let's check if main.bicep exists and examine its content
if [ -f ".azure/applications/sync-resource-policy-information-job/main.bicep" ]; then
echo "main.bicep exists. Showing content:"
cat ".azure/applications/sync-resource-policy-information-job/main.bicep"
else
echo "main.bicep not found in the expected location"
# Let's search for it in case it's in a different location
echo "Searching for main.bicep in the repository:"
fd "main.bicep" --type f
fi
Length of output: 3201
.azure/applications/sync-resource-policy-information-job/yt01.bicepparam (3)
1-5: LGTM! Basic configuration looks good.
The environment configuration and location settings are appropriate for the yt01 environment. The use of readEnvironmentVariable for the image tag ensures dynamic versioning.
8-11: Verify environment variable availability in deployment pipeline.
The configuration relies on several critical environment variables. Ensure these are properly set in the deployment pipeline:
- AZURE_CONTAINER_APP_ENVIRONMENT_NAME
- AZURE_ENVIRONMENT_KEY_VAULT_NAME
- AZURE_APP_INSIGHTS_CONNECTION_STRING
Consider:
- Adding validation steps in the deployment pipeline to verify these variables are set
- Documenting these required variables in the deployment documentation
- Providing example values or instructions for local development
✅ Verification successful
Environment variables are properly configured in deployment workflows
The required environment variables are correctly set up in the GitHub Actions workflows:
- All variables (AZURE_CONTAINER_APP_ENVIRONMENT_NAME, AZURE_ENVIRONMENT_KEY_VAULT_NAME, AZURE_APP_INSIGHTS_CONNECTION_STRING) are properly referenced as GitHub secrets in multiple deployment workflows
- The README.md explicitly documents these as required secrets for environment setup
- The variables are consistently used across different deployment scenarios (staging, production, pull requests)
The infrastructure is well-organized with proper secret management through GitHub environments, as recommended in the original architecture advice.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if these environment variables are properly configured in workflow files
# Look for environment variable definitions in workflow files
fd -e yml -e yaml --glob '.github/workflows/*' | xargs rg -A 5 'env:|environment:|AZURE_'
Length of output: 8949
Script:
#!/bin/bash
# Let's check for workflow files in a different way and specifically look for these environment variables
fd -H -e yml -e yaml | xargs rg -i "AZURE_CONTAINER_APP_ENVIRONMENT_NAME|AZURE_ENVIRONMENT_KEY_VAULT_NAME|AZURE_APP_INSIGHTS_CONNECTION_STRING"
# Also check the infrastructure deployment files for these variables
fd -e bicepparam -e bicep | xargs rg -i "AZURE_CONTAINER_APP_ENVIRONMENT_NAME|AZURE_ENVIRONMENT_KEY_VAULT_NAME|AZURE_APP_INSIGHTS_CONNECTION_STRING"
Length of output: 6434
6-6: Verify the cron schedule impact.
The 30-minute schedule (*/30 * * * *) might be too frequent for a resource policy sync job. Consider:
- Resource consumption and costs
- Rate limits of any external APIs
- Time taken to complete each sync
Consider implementing exponential backoff and proper error handling in the sync job to handle rate limits gracefully.
✅ Verification successful
The 30-minute schedule appears reasonable for this sync job
Based on the code analysis:
- The job has built-in concurrency control via
NumberOfConcurrentRequestsparameter - It includes proper error handling and logging
- It supports incremental sync through the
sinceparameter, making each run efficient - The implementation appears lightweight, mainly focused on policy information updates
The 30-minute frequency helps maintain policy information freshness while the implementation includes safeguards against resource overuse.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any rate limiting configurations or documentation about API limits
# that might affect the 30-minute schedule
# Look for rate limit configurations
rg -i "rate.?limit|throttle" --type yaml --type json
# Look for documentation about API limits
fd -e md -e txt | xargs rg -i "rate.?limit|api.?limit|throttle"
Length of output: 8949
Script:
#!/bin/bash
# Let's check for any existing sync jobs and their schedules to understand the pattern
rg -i "schedule|cron" --type bicepparam -A 3 -B 3
# Also check for any documentation or code related to this sync job
rg -i "sync.?resource.?policy" --type-add bicep:*.{bicep,bicepparam} --type bicep -A 5 -B 5
# Look for any logs or monitoring configurations that might indicate job duration
rg -i "duration|elapsed.?time|execution.?time" --type-add bicep:*.{bicep,bicepparam} --type bicep --type cs
Length of output: 428
Script:
#!/bin/bash
# Let's try a different approach using fd to find all bicepparam files and then examine their content
fd -e bicepparam | xargs cat
# Also search for any related job implementations or configurations
rg -g "*.{cs,yaml,yml,json}" -i "sync.?resource.?policy"
# Look for any monitoring or logging related to this job
rg -g "*.{cs,yaml,yml,json}" -i "sync.?resource.?policy.*(duration|monitor|log)"
Length of output: 642
Script:
#!/bin/bash
# Let's examine the implementation of the sync command to understand its complexity
ast-grep --pattern 'class SynchronizeResourcePolicyInformationCommand {
$$$
}'
# Also check for any error handling or retry logic
rg -g "*.cs" -A 10 "Failed to sync resource policy information"
# Look at the command implementation
rg -g "*.cs" -A 10 "sync-resource-policy-information"
Length of output: 2173
.azure/applications/sync-resource-policy-information-job/staging.bicepparam (2)
1-4: LGTM! Basic configuration follows best practices.
The environment and location parameters are well-defined and follow Azure/Bicep conventions.
8-11: 🛠️ Refactor suggestion
Enhance secret management with validation and documentation.
While the secret management follows Azure naming conventions, consider these improvements:
- Add validation for required environment variables
- Document expected formats/patterns for these secrets
//secrets
-param containerAppEnvironmentName = readEnvironmentVariable('AZURE_CONTAINER_APP_ENVIRONMENT_NAME')
-param environmentKeyVaultName = readEnvironmentVariable('AZURE_ENVIRONMENT_KEY_VAULT_NAME')
-param appInsightConnectionString = readEnvironmentVariable('AZURE_APP_INSIGHTS_CONNECTION_STRING')
+// Container App Environment name (required)
+param containerAppEnvironmentName = readEnvironmentVariable('AZURE_CONTAINER_APP_ENVIRONMENT_NAME', true)
+// Key Vault name (required, must be globally unique)
+param environmentKeyVaultName = readEnvironmentVariable('AZURE_ENVIRONMENT_KEY_VAULT_NAME', true)
+// App Insights connection string (required, format: InstrumentationKey=xxx;IngestionEndpoint=xxx)
+param appInsightConnectionString = readEnvironmentVariable('AZURE_APP_INSIGHTS_CONNECTION_STRING', true)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/HostingEnvironmentExtensions.cs (1)
8-12: Verify the impact on development hosted services.
The method affects the execution of development hosted services. Let's verify all its usages to ensure consistent implementation.
✅ Verification successful
The implementation of ShouldRunDevelopmentHostedService is consistently used across development hosted services
The verification shows that:
- All development hosted services (
DevelopmentCleanupOutboxHostedService,DevelopmentMigratorHostedService,DevelopmentResourcePolicyInformationSyncHostedService,DevelopmentSubjectResourceSyncHostedService) consistently use theShouldRunDevelopmentHostedServicemethod - The method correctly handles both development environment check and Janitor exclusion
- Other
IsDevelopment()usages in the codebase serve different purposes and don't need to be replaced with this method as they don't relate to development hosted services
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of ShouldRunDevelopmentHostedService to verify implementation consistency
# Search for all development hosted services and their usage of this method
rg -l "DevelopmentHostedService" | xargs rg -A 5 "ShouldRunDevelopmentHostedService"
# Search for any direct IsDevelopment() calls that might need to be replaced
rg "IsDevelopment\(\)" --type cs
Length of output: 6096
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Configurations/ResourcePolicyInformation/ResourcePolicyInformationConfiguration.cs (1)
10-12: Verify the indexing strategy for query patterns.
Given that the entity has additional properties (MinimumSecurityLevel, CreatedAt, UpdatedAt), consider if additional indexes are needed based on your query patterns.
Let's analyze the query patterns:
✅ Verification successful
Let me gather more information about the ResourcePolicyInformation entity and its usage patterns.
Let me try one more search to find the entity definition and its usage patterns.
Current index strategy is sufficient for the existing query patterns
Based on the analysis of the codebase:
- The entity is primarily queried by
Resourcewhich is already covered by the unique index - The repository only exposes two methods:
Mergeoperation for batch updatesGetLastUpdatedAtwhich usesUpdatedAtfor a simple MAX aggregation
- No complex queries or filters are performed on
MinimumSecurityLevel,CreatedAt, or other fields - The unique index on
Resourceis appropriate as it's the main lookup field and ensures data integrity
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find query patterns for ResourcePolicyInformation to determine if additional indexes are needed
# Search for LINQ queries or raw SQL involving ResourcePolicyInformation
echo "Searching for query patterns..."
rg -l "ResourcePolicyInformation" | xargs rg -A 5 "Where|OrderBy|Join|Include"
# Search for repository methods that might indicate query patterns
echo "Searching for repository methods..."
ast-grep --pattern 'interface IResourcePolicyInformationRepository {
$$$
}'
Length of output: 3478
Script:
#!/bin/bash
# Find the ResourcePolicyInformation entity definition and usages
echo "Searching for ResourcePolicyInformation entity definition..."
ast-grep --pattern 'class ResourcePolicyInformation {
$$$
}'
echo "Searching for repository implementations..."
rg -A 10 "class.*ResourcePolicyInformation.*Repository"
echo "Searching for queries in services/handlers..."
rg -l "ResourcePolicyInformation" | xargs rg -A 5 "GetAsync|FindAsync|FirstOrDefaultAsync|SingleOrDefaultAsync"
Length of output: 2186
Script:
#!/bin/bash
# Find the ResourcePolicyInformation entity definition and all its properties
echo "Searching for entity definition..."
rg -A 15 "class ResourcePolicyInformation"
# Find all methods in the repository
echo "Searching for repository methods..."
rg -A 20 "interface IResourcePolicyInformation"
# Find actual database queries
echo "Searching for database queries..."
rg -A 5 "_dbContext.*ResourcePolicyInformation"
Length of output: 11130
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentMigratorHostedService.cs (1)
20-20: Verify the extension method implementation.
The change to use ShouldRunDevelopmentHostedService() appears to be part of a broader standardization of environment checks. Let's verify the implementation exists and works as expected.
✅ Verification successful
Extension method implementation verified and looks correct
The ShouldRunDevelopmentHostedService() extension method is properly implemented in HostingEnvironmentExtensions.cs. It ensures development services only run in development environments while excluding Janitor-related assemblies. The method is consistently used across all development hosted services:
- DevelopmentMigratorHostedService
- DevelopmentSubjectResourceSyncHostedService
- DevelopmentCleanupOutboxHostedService
- DevelopmentResourcePolicyInformationSyncHostedService
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the extension method definition
ast-grep --pattern 'public static bool ShouldRunDevelopmentHostedService(this IHostEnvironment $_)'
# Search for all usages to ensure consistent implementation
rg "ShouldRunDevelopmentHostedService"
Length of output: 1047
Script:
#!/bin/bash
# Get the implementation of the extension method
rg -A 10 "public static bool ShouldRunDevelopmentHostedService" src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/HostingEnvironmentExtensions.cs
Length of output: 469
src/Digdir.Domain.Dialogporten.Application/Externals/IResourcePolicyInformationRepository.cs (2)
1-5: LGTM! Clean and well-structured imports and namespace.
The imports are minimal and relevant, and the namespace follows the project's structure.
6-10: Add XML documentation for the interface methods.
While the method signatures are clear, adding XML documentation would help explain:
- The purpose of the
Mergemethod's return value - The impact of the
timeSkewparameter - Expected behavior in error scenarios
Example documentation:
public interface IResourcePolicyInformationRepository
{
+ /// <summary>
+ /// Merges the provided resource policy information into the repository.
+ /// </summary>
+ /// <param name="resourceMetadata">Collection of resource policy information to merge.</param>
+ /// <param name="cancellationToken">Optional cancellation token.</param>
+ /// <returns>The number of records affected by the merge operation.</returns>
Task<int> Merge(IReadOnlyCollection<ResourcePolicyInformation> resourceMetadata, CancellationToken cancellationToken = default);
+ /// <summary>
+ /// Retrieves the timestamp of the last update, adjusted for potential time skew in distributed systems.
+ /// </summary>
+ /// <param name="timeSkew">Optional time adjustment to account for system clock differences.</param>
+ /// <param name="cancellationToken">Optional cancellation token.</param>
+ /// <returns>The timestamp of the last update.</returns>
Task<DateTimeOffset> GetLastUpdatedAt(TimeSpan? timeSkew = null, CancellationToken cancellationToken = default);
}Let's verify the interface implementation:
✅ Verification successful
XML documentation suggestion confirmed with implementation details
public interface IResourcePolicyInformationRepository
{
+ /// <summary>
+ /// Merges a collection of resource policy information into the database using an UPSERT operation.
+ /// For existing resources, updates the UpdatedAt timestamp. For new resources, inserts a complete record.
+ /// </summary>
+ /// <param name="resourceMetadata">Collection of resource policy information to merge.</param>
+ /// <param name="cancellationToken">Optional cancellation token.</param>
+ /// <returns>The number of records affected by the merge operation.</returns>
Task<int> Merge(IReadOnlyCollection<ResourcePolicyInformation> resourceMetadata, CancellationToken cancellationToken = default);
+ /// <summary>
+ /// Retrieves the maximum UpdatedAt timestamp from all resource policy records.
+ /// </summary>
+ /// <param name="timeSkew">Optional time adjustment added to the result, used to ensure no updates are missed in distributed systems.
+ /// In the synchronization process, this is set to 1 microsecond to catch concurrent updates.</param>
+ /// <param name="cancellationToken">Optional cancellation token.</param>
+ /// <returns>The latest update timestamp, adjusted by the timeSkew if provided. Returns default timestamp if no records exist.</returns>
Task<DateTimeOffset> GetLastUpdatedAt(TimeSpan? timeSkew = null, CancellationToken cancellationToken = default);
}The implementation details confirm:
Mergeperforms an UPSERT operation using a SQL merge statementGetLastUpdatedAtreturns the max UpdatedAt value with an optional skew- Both methods are used correctly in the synchronization process
- The suggested documentation accurately reflects the implementation and usage patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find and verify the implementation of IResourcePolicyInformationRepository
# Search for the implementation
echo "Searching for implementation class..."
ast-grep --pattern 'class $_ : $$$IResourcePolicyInformationRepository$$$'
# Search for usage patterns
echo "Searching for usage patterns..."
rg -l "IResourcePolicyInformationRepository"
Length of output: 696
Script:
#!/bin/bash
# Let's examine the implementation and usage to understand the documentation needs
# Check the implementation
echo "=== Implementation in ResourcePolicyInformationRepository.cs ==="
rg -A 10 "class ResourcePolicyInformationRepository" "src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Repositories/ResourcePolicyInformationRepository.cs"
# Check usage in SynchronizeResourcePolicyInformationCommand
echo -e "\n=== Usage in SynchronizeResourcePolicyInformationCommand.cs ==="
rg -A 10 "IResourcePolicyInformationRepository" "src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/Synchronize/SynchronizeResourcePolicyInformationCommand.cs"
# Check if XML documentation already exists in implementation
echo -e "\n=== Checking for existing XML documentation ==="
rg "///" -A 3 "src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Repositories/ResourcePolicyInformationRepository.cs"
Length of output: 2257
Script:
#!/bin/bash
# Let's check the implementation details of both methods to understand their behavior better
echo "=== Checking Merge method implementation ==="
rg -A 15 "public async Task<int> Merge" "src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Repositories/ResourcePolicyInformationRepository.cs"
echo -e "\n=== Checking GetLastUpdatedAt method implementation ==="
rg -A 15 "public async Task<DateTimeOffset> GetLastUpdatedAt" "src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Repositories/ResourcePolicyInformationRepository.cs"
# Let's also check how these methods are used in the command handler
echo -e "\n=== Checking method usage in command handler ==="
rg -A 20 "public async Task<SynchronizeResourcePolicyInformationResult> Handle" "src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/Synchronize/SynchronizeResourcePolicyInformationCommand.cs"
Length of output: 3797
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentSubjectResourceSyncHostedService.cs (2)
Line range hint 1-35: LGTM! The implementation follows best practices.
The service demonstrates good practices:
- Proper dependency injection and null checks
- Correct scope management
- Clean implementation of IHostedService
- Appropriate use of MediatR for command handling
21-21: Verify the implementation of ShouldRunDevelopmentHostedService extension method.
The change from IsDevelopment() to ShouldRunDevelopmentHostedService() suggests a more specific environment check. Let's verify its implementation to ensure consistent behavior across all development hosted services.
✅ Verification successful
Let's gather more information about the implementation and usage pattern.
Extension method implementation is correct and consistently used
The ShouldRunDevelopmentHostedService() extension method is properly implemented and provides additional safety by:
- Maintaining the original development environment check via
IsDevelopment() - Adding protection against running in Janitor context
- Being consistently used across all development hosted services
The change from IsDevelopment() to ShouldRunDevelopmentHostedService() is a valid improvement that adds more specific control over development services execution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find and analyze the implementation of ShouldRunDevelopmentHostedService
# and its usage across other development hosted services
# Find the extension method implementation
echo "Looking for ShouldRunDevelopmentHostedService implementation:"
rg -l "ShouldRunDevelopmentHostedService"
# Find all development hosted services using this method
echo -e "\nChecking usage in other development hosted services:"
ast-grep --pattern 'class Development$_HostedService : IHostedService {
$$$
StartAsync($_) {
$$$
ShouldRunDevelopmentHostedService()
$$$
}
$$$
}'
Length of output: 1026
Script:
#!/bin/bash
# Find the implementation of ShouldRunDevelopmentHostedService
echo "Implementation of ShouldRunDevelopmentHostedService:"
rg -A 5 "ShouldRunDevelopmentHostedService" "src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/HostingEnvironmentExtensions.cs"
# Check how it's used in each development service
echo -e "\nUsage in DevelopmentResourcePolicyInformationSyncHostedService:"
rg -A 3 "ShouldRunDevelopmentHostedService" "src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentResourcePolicyInformationSyncHostedService.cs"
echo -e "\nUsage in DevelopmentCleanupOutboxHostedService:"
rg -A 3 "ShouldRunDevelopmentHostedService" "src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentCleanupOutboxHostedService.cs"
echo -e "\nUsage in DevelopmentMigratorHostedService:"
rg -A 3 "ShouldRunDevelopmentHostedService" "src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentMigratorHostedService.cs"
Length of output: 1702
src/Digdir.Domain.Dialogporten.Application/Externals/IResourceRegistry.cs (1)
11-12: Add XML documentation for the new method.
Please add XML documentation for the GetUpdatedResourcePolicyInformation method to describe its purpose, parameters, and return value. This will help maintain consistency with C# documentation practices and improve code maintainability.
Let's verify if other interface methods have XML documentation:
src/Digdir.Domain.Dialogporten.Janitor/Commands.cs (1)
26-39: LGTM! Command implementation follows established patterns.
The implementation correctly follows the existing command pattern and properly handles command execution and error cases.
src/Digdir.Domain.Dialogporten.Janitor/Digdir.Domain.Dialogporten.Janitor.csproj (1)
33-39: Verify if explicit configuration is necessary.
The project already includes a wildcard pattern appsettings*.json that handles copying all settings files. The new configuration might be redundant except for the ExcludeFromSingleFile property. Consider simplifying if the explicit configuration isn't required for overriding the wildcard behavior.
✅ Verification successful
Explicit configuration is necessary and correct
The explicit appsettings.local.json configuration is required because:
- It adds the
ExcludeFromSingleFileproperty which is not present in the wildcard pattern - The documentation explicitly states that
appsettings.local.jsonis meant for developer-specific settings and should not be included in production builds - The file is properly excluded in
.gitignore - The codebase has specific handling for this file in
ConfigurationExtensions.csthat only loads it in Development environment
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any special handling requirements for appsettings.local.json
# that would justify the explicit configuration
# Look for references to appsettings.local.json in the codebase
rg -l "appsettings\.local\.json"
# Check if the file is already in .gitignore
grep -l "appsettings.local.json" .gitignore
Length of output: 290
Script:
#!/bin/bash
# Let's check the content of these files to understand the context better
# Check configuration documentation
cat docs/Configuration.md
# Check README mentions
rg -A 5 -B 5 "appsettings\.local\.json" README.md
# Check the configuration extensions implementation
cat "src/Digdir.Domain.Dialogporten.Application/Common/Extensions/ConfigurationExtensions.cs"
# Check the gitignore entry
grep "appsettings.local.json" .gitignore
# Check if there are any other appsettings patterns in the csproj
cat "src/Digdir.Domain.Dialogporten.Janitor/Digdir.Domain.Dialogporten.Janitor.csproj"
Length of output: 15152
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241106143110_AddResourcePolicyInformation.cs (4)
1-10: LGTM: Migration class setup follows best practices
The migration is properly structured with nullable disable pragma and inherits from the Migration base class.
37-41: LGTM: Clean rollback implementation
The Down method correctly drops the table, providing a clean rollback path.
14-28: Verify the column constraints and data types
The table creation looks good with appropriate data types and constraints. A few points to verify:
- Is 255 characters sufficient for the Resource column? Consider future needs.
- Should MinimumSecurityLevel be constrained to specific values?
- Consider adding a comment describing the purpose of this table.
Let's check if there are any existing security level definitions:
✅ Verification successful
Based on the search results, I can now provide a comprehensive verification of the table constraints and data types:
Column constraints and data types are appropriate
- The 255-character limit for the Resource column is sufficient as it stores URNs from the Resource Registry (seen in
ResourceRegistryClient.cs). - MinimumSecurityLevel is correctly defined as an integer with valid values between 1-4 based on the codebase:
- Default value is 1 (
DefaultMinimumSecurityLevel) - Authentication levels 3 and 4 are referenced in tests
- Values are obtained from Altinn's Resource Registry XACML policies
- Default value is 1 (
- The timestamps with UTC enforcement via
time zone 'utc'are properly configured
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for security level definitions or usage
rg -g "*.cs" -A 3 "SecurityLevel|AuthenticationLevel"
Length of output: 16284
18-22: Verify timestamp handling across time zones
The default values for CreatedAt and UpdatedAt use 'utc' timezone, which is correct. However, ensure that:
- Application code consistently uses UTC when writing timestamps
- Any reporting or UI layer properly handles timezone conversion
✅ Verification successful
Let me search for more information about timestamp handling in the application code to ensure consistent UTC usage.
Let me check for the implementation of these converters and any custom timestamp handling logic.
Timestamp handling is properly implemented with UTC enforcement
The codebase demonstrates robust UTC timestamp handling:
- Database uses
timestamp with time zonewith explicit UTC timezone UtcDateTimeOffsetConverterenforces explicit timezone offsets in JSONDateTimeNotSupportedConverterprevents usage of ambiguousDateTimeDateTimeOffsetConverterensures UTC conversion for database operationsIClockinterface providesUtcNowfor application code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for timestamp handling patterns in the codebase
rg -g "*.cs" "DateTimeOffset.*current.*|DateTime\..*UtcNow|TimeZone"
Length of output: 9212
Script:
#!/bin/bash
# Search for DateTimeOffset usage in entity definitions and services
ast-grep --pattern 'class $_ {
$$$
DateTimeOffset $_ { get; set; }
$$$
}'
# Search for any custom DateTime/DateTimeOffset handling
rg -g "*.cs" -B2 -A2 "new DateTime|new DateTimeOffset"
Length of output: 1105
Script:
#!/bin/bash
# Search for converter implementations
rg -g "*.cs" -A 10 "class.*DateTimeOffsetConverter|class.*DateTimeNotSupportedConverter"
# Search for any timestamp-related interfaces or base classes
ast-grep --pattern 'interface $_ {
$$$
DateTimeOffset $_ { get; }
$$$
}'
Length of output: 4163
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentCleanupOutboxHostedService.cs (1)
26-26: Verify the extension method implementation.
The change to use ShouldRunDevelopmentHostedService() aligns with the standardization of hosted service execution conditions. However, let's verify the implementation exists.
✅ Verification successful
Extension method implementation verified and consistent with usage
The implementation in HostingEnvironmentExtensions.cs is well-defined and serves a clear purpose - it ensures the service only runs in development environments while excluding Janitor-related assemblies. The change from IsDevelopment() to ShouldRunDevelopmentHostedService() adds this additional safety check, making it a valid improvement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for the extension method implementation
# Expected: Find the implementation of ShouldRunDevelopmentHostedService
# Search for the extension method definition
ast-grep --pattern 'public static bool ShouldRunDevelopmentHostedService(this IHostEnvironment $_)'
# Backup search using ripgrep in case it's defined differently
rg -l "ShouldRunDevelopmentHostedService"
Length of output: 706
Script:
#!/bin/bash
# Let's examine the implementation in HostingEnvironmentExtensions.cs
cat "src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/HostingEnvironmentExtensions.cs"
# Also check how it's used in other development services for consistency
rg -A 2 "ShouldRunDevelopmentHostedService" "src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/*.cs"
Length of output: 838
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/LocalDevelopmentResourceRegistry.cs (2)
4-4: LGTM!
The new using directive is correctly placed and necessary for the UpdatedResourcePolicyInformation type.
46-48: Verify test coverage for the new method.
Since this is a local development implementation that returns empty results, we should ensure it's properly covered by tests.
✅ Verification successful
Test coverage exists through integration tests
The LocalDevelopmentResourceRegistry is used in integration tests as the default implementation:
- It's registered in
DialogApplication.csfor integration testing - The class is properly registered for local development scenarios in
InfrastructureExtensions.cs
Since this is a test double meant for local development, and it's being used in the test infrastructure, explicit unit tests for this implementation are not necessary. The implementation is implicitly tested through integration tests that exercise the resource registry functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files that might contain tests for LocalDevelopmentResourceRegistry
rg -l "LocalDevelopmentResourceRegistry" --type cs --glob "*Tests.cs"
# Search for specific test methods related to GetUpdatedResourcePolicyInformation
rg "GetUpdatedResourcePolicyInformation.*Test" --type cs
Length of output: 128
Script:
#!/bin/bash
# Let's try a broader search to find test files and implementations
# First, find the exact namespace and class definition
rg -A 5 "class LocalDevelopmentResourceRegistry" --type cs
# Then search for any test files in the test directories
fd "Tests?.cs$" test src/*/test
# Finally, let's look for any usage of this class
rg "new LocalDevelopmentResourceRegistry" --type cs
Length of output: 1311
Script:
#!/bin/bash
# Let's try different test directory patterns and look for any tests
fd "Tests?.cs$" "test" "tests" "*.Tests" "*.Test"
# Search for any registration or usage of LocalDevelopmentResourceRegistry in startup or configuration files
rg -A 5 "LocalDevelopmentResourceRegistry" --type cs
# Look for test files specifically in the Infrastructure project
fd "Tests?.cs$" src/Digdir.Domain.Dialogporten.Infrastructure
Length of output: 7743
src/Digdir.Domain.Dialogporten.Application/Externals/IDialogDbContext.cs (2)
11-11: LGTM! Clean import addition.
The new using directive follows the existing pattern and is properly placed with other domain imports.
46-46: LGTM! Well-structured DbSet addition.
The new ResourcePolicyInformation DbSet property:
- Follows consistent naming and pattern with other DbSet properties
- Properly integrates with the existing interface structure
- Supports the PR objective of resource policy metadata synchronization
.azure/applications/sync-resource-policy-information-job/main.bicep (4)
100-106: LGTM! Follows security best practices
The Key Vault access policy configuration follows the principle of least privilege by only granting reader access to the required secrets.
108-109: LGTM! Essential outputs are provided
The outputs provide the necessary information for integration with other resources and monitoring.
68-83: Add deployment-time validation for Key Vault access
While the Key Vault configuration looks secure, consider adding deployment-time validation to ensure the secrets exist and are accessible.
#!/bin/bash
# Description: Check if Key Vault parameter files exist for all environments
fd -e bicepparam$ -x grep -l "environmentKeyVaultName"45-47: Verify the API version availability
The Container Apps API version '2024-03-01' appears to be set in the future. This might cause deployment issues if the version is not yet available in your target Azure regions.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/Synchronize/SynchronizeSubjectResourceMappingsCommand.cs (2)
17-17: LGTM!
Good use of OneOf for type-safe error handling and proper sealing of the result type.
19-37: Verify the default batch size value.
The default batch size of 1000 items might need validation against system performance characteristics and memory constraints.
✅ Verification successful
Default batch size of 1000 is properly validated and consistent with system constraints
The batch size value of 1000 is well-justified and properly validated:
- There's a validator (
SynchronizeSubjectResourceMappingsCommandValidator) that enforces a range of 1-1000 for the batch size - This limit aligns with other system constraints:
PaginationConstants.MaxLimitis also set to 1000OutboxCdcSubscriptionOptions.SnapshotBatchSizedefaults to 1000
- The value can be overridden through the command's
BatchSizeproperty, providing flexibility while maintaining the upper bound
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other batch size configurations in the codebase for consistency
rg -g '!*.{json,md}' --type-add 'config:*.{yaml,yml,xml}' -t config -t cs "batch.*size.*=.*1000" -C 2
Length of output: 103
Script:
#!/bin/bash
# Let's try a broader search for batch size configurations
rg -g '!*.{json,md}' "BatchSize|batchSize|batch_size" -C 2
# Also search for any numeric constants around 1000
rg -g '!*.{json,md}' "const.*=.*1000|const.*=.*500|const.*=.*2000" -C 2
Length of output: 11596
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/DialogDbContext.cs (2)
18-18: LGTM!
The using directive is properly placed with other domain entity imports.
50-50: LGTM! Verify configuration setup.
The DbSet property follows the established pattern and matches the interface requirement.
Let's verify the entity configuration is properly set up:
✅ Verification successful
Let me check the configuration details to ensure it's properly set up.
Configuration setup verified and properly implemented ✅
The verification confirms:
- ResourcePolicyInformation configuration exists with proper unique index on Resource property
- Configurations are automatically applied via
ApplyConfigurationsFromAssemblyin DbContext - Entity is properly registered as DbSet
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for ResourcePolicyInformation configuration and model builder registration
# Test 1: Verify configuration class exists
echo "Checking for configuration class..."
fd -e cs ResourcePolicyInformationConfiguration
# Test 2: Verify the configuration is applied correctly
echo "Checking configuration usage in model builder..."
rg -l "ResourcePolicyInformation.*ToTable|HasIndex|Configure.*ResourcePolicyInformation" -g "*.cs"
Length of output: 3548
Script:
#!/bin/bash
# Check the content of ResourcePolicyInformationConfiguration
echo "Content of ResourcePolicyInformationConfiguration:"
cat src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Configurations/ResourcePolicyInformation/ResourcePolicyInformationConfiguration.cs
# Check if it's registered in DbContext's OnModelCreating
echo -e "\nChecking OnModelCreating registration:"
rg -A 5 "OnModelCreating.*override" src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/DialogDbContext.cs
Length of output: 1030
Script:
#!/bin/bash
# Let's try a different approach to find OnModelCreating
echo "Searching for OnModelCreating with more context:"
ast-grep --pattern 'protected override void OnModelCreating(ModelBuilder $_) { $$$ }'
# Also check if there's an ApplyConfigurations call
echo -e "\nChecking for ApplyConfigurations usage:"
rg "ApplyConfigurations|modelBuilder.*Apply" -A 3 src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/DialogDbContext.cs
Length of output: 648
src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs (2)
183-215: LGTM! Robust message handling configuration.
The MassTransit configuration properly implements:
- Entity Framework outbox pattern for reliable message delivery
- Dead letter queue handling for failed messages
- Environment-specific transport configuration
71-71: Verify development environment restriction.
The new hosted service appears to be for development purposes. Ensure it's properly guarded against running in production environments.
✅ Verification successful
Development environment restriction is properly implemented
The service is well-protected against running in production environments through multiple safeguards:
- Located in the
Developmentnamespace and folder - Uses the
ShouldRunDevelopmentHostedService()extension method which checks for both development environment and non-Janitor assembly - Has proper environment checks in the
StartAsyncimplementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the hosted service is only used in development
# Test: Check if the service is properly restricted to development environment
ast-grep --pattern 'class DevelopmentResourcePolicyInformationSyncHostedService : IHostedService {
$$$
}'
Length of output: 109
Script:
#!/bin/bash
# Let's try a different approach to find the service implementation
# First, let's find the file containing the service
fd "DevelopmentResourcePolicyInformationSyncHostedService" --type f
# Then let's look for any environment checks in the infrastructure extensions
rg -A 5 "DevelopmentResourcePolicyInformationSyncHostedService"
# Also check for any IsDevelopment() checks around the service registration
rg -B 5 -A 2 "IsDevelopment|Development.*Environment" src/Digdir.Domain.Dialogporten.Infrastructure/
Length of output: 12940
Script:
#!/bin/bash
# Let's check the StartAsync implementation to verify environment checks
rg -A 10 "StartAsync" src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentResourcePolicyInformationSyncHostedService.cs
# Also check if there's a common extension method being used for environment checks
rg -A 5 "ShouldRunDevelopmentHostedService" src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentResourcePolicyInformationSyncHostedService.cs
Length of output: 891
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Repositories/ResourcePolicyInformationRepository.cs (4)
12-15: Constructor implementation is correct.
The constructor properly initializes the repository with a non-null DialogDbContext.
17-29: Verify behavior when the table is empty.
In the GetLastUpdatedAt method, if the ResourcePolicyInformation table is empty, lastUpdatedAt will default to DateTimeOffset.MinValue due to DefaultIfEmpty(). Ensure that this behavior is acceptable and that any consuming code can handle this value appropriately.
31-59: Ensure all arrays passed to unnest have the same length.
In the Merge method, the SQL command passes multiple arrays to the unnest function (lines 37-38). PostgreSQL requires that all arrays have the same length; otherwise, it will raise a runtime error.
Consider adding a validation check to ensure that all arrays derived from resourceMetadata have the same length before executing the SQL command.
31-59:
Fix syntax error in parameter array construction.
The square brackets used in lines 52-58 to create the array of parameters are syntactically incorrect in C#. Square brackets denote attributes or array indexing. You should use an explicit array creation or curly braces for initialization.
Apply this diff to fix the syntax error:
- : await _dbContext.Database.ExecuteSqlRawAsync(sql,
- [
- new NpgsqlParameter("ids", resourceMetadata.Select(x => x.Id).ToArray()),
- new NpgsqlParameter("resources", resourceMetadata.Select(x => x.Resource).ToArray()),
- new NpgsqlParameter("minimumSecurityLevels", resourceMetadata.Select(x => x.MinimumSecurityLevel).ToArray()),
- new NpgsqlParameter("createdAts", resourceMetadata.Select(x => x.CreatedAt).ToArray()),
- new NpgsqlParameter("updatedAts", resourceMetadata.Select(x => x.UpdatedAt).ToArray())
- ], cancellationToken);
+ : await _dbContext.Database.ExecuteSqlRawAsync(
+ sql,
+ new object[]
+ {
+ new NpgsqlParameter("ids", resourceMetadata.Select(x => x.Id).ToArray()),
+ new NpgsqlParameter("resources", resourceMetadata.Select(x => x.Resource).ToArray()),
+ new NpgsqlParameter("minimumSecurityLevels", resourceMetadata.Select(x => x.MinimumSecurityLevel).ToArray()),
+ new NpgsqlParameter("createdAts", resourceMetadata.Select(x => x.CreatedAt).ToArray()),
+ new NpgsqlParameter("updatedAts", resourceMetadata.Select(x => x.UpdatedAt).ToArray())
+ },
+ cancellationToken);Likely invalid or redundant comment.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/Synchronize/SynchronizeResourcePolicyInformationCommand.cs (1)
42-42: Verify availability of TimeSpan.FromMicroseconds method
The method TimeSpan.FromMicroseconds may not be available in all target frameworks. Confirm that your project's target framework supports this method to avoid compilation issues.
Run the following script to check for method availability:
✅ Verification successful
TimeSpan.FromMicroseconds is available in .NET 8.0
The project targets .NET 8.0 as specified in Directory.Build.props, and TimeSpan.FromMicroseconds was introduced in .NET 8.0. The method is available and safe to use in this codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if TimeSpan.FromMicroseconds exists in the target framework.
# Test: Search for TimeSpan.FromMicroseconds in the .NET documentation for the project's target framework.
# Expected result: Confirmation that TimeSpan.FromMicroseconds is supported.
# Since this cannot be directly checked via code, please manually verify the target framework's documentation.
Length of output: 0
Script:
#!/bin/bash
# Let's check the target framework in the project file
fd -e csproj . | xargs cat
Length of output: 26566
Script:
#!/bin/bash
# Let's check the target framework in the project file containing the code under review
fd "Digdir.Domain.Dialogporten.Application.csproj" | xargs grep -A 2 "<PropertyGroup>"
Length of output: 274
Script:
#!/bin/bash
# Let's check the Directory.Build.props file which might contain the target framework
fd "Directory.Build.props" | xargs cat
Length of output: 622
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs (2)
21-22: Good use of constants for authentication configuration
Defining DefaultMinimumSecurityLevel and AuthenticationLevelCategory as constants enhances readability and maintainability.
26-26: Injection of ILogger improves logging capabilities
Adding ILogger<ResourceRegistryClient> allows for better logging and debugging within the class.
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Interceptors/ConvertDomainEventsToOutboxMessagesInterceptor.cs (5)
16-16: Lazy Initialization of Dependencies is Appropriate
Using Lazy<ITopicEventSender> and Lazy<IPublishEndpoint> for deferred initialization of dependencies is suitable. This approach improves performance by delaying the instantiation of these services until they are actually needed.
Also applies to: 18-18
24-24: Constructor Parameters Updated for Lazy Dependencies
Updating the constructor to accept Lazy<ITopicEventSender> and Lazy<IPublishEndpoint> aligns with the changes to the class fields. This ensures consistency and proper initialization of the lazy-loaded services.
Also applies to: 26-26
55-58: Efficient Early Exit When No Domain Events
Adding an early return when _domainEvents.Count == 0 in SavingChangesAsync enhances performance by avoiding unnecessary processing when there are no domain events to handle.
76-79: Efficient Early Exit in SavedChangesAsync When No Domain Events
Similarly, introducing an early return in SavedChangesAsync when _domainEvents is empty prevents unnecessary execution of code, improving efficiency.
115-126: Proper Implementation of Lazy Service Initialization with Error Handling
The EnsureLazyLoadedServices() method correctly triggers the initialization of the lazy-loaded services _topicEventSender and _publishEndpoint. The try-catch block effectively captures any exceptions during initialization and throws an InvalidOperationException with a clear message. This provides better error detection and debugging support if the services are not properly registered.
src/Digdir.Domain.Dialogporten.Domain/ResourcePolicyInformation/ResourcePolicyInformation.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Domain/ResourcePolicyInformation/ResourcePolicyInformation.cs
Outdated
Show resolved
Hide resolved
...esourceRegistry/Commands/Synchronize/SynchronizeResourcePolicyInformationCommandValidator.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Application/Externals/IResourcePolicyInformationRepository.cs
Outdated
Show resolved
Hide resolved
...rastructure/Persistence/Development/DevelopmentResourcePolicyInformationSyncHostedService.cs
Outdated
Show resolved
Hide resolved
...ures/V1/ResourceRegistry/Commands/Synchronize/SynchronizeResourcePolicyInformationCommand.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
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
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (15)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/SyncPolicy/SyncPolicyCommandValidator.cs (2)
5-11: Consider adding validation for theSinceparameter.Based on the AI summary, the
SyncPolicyCommandincludes aSinceparameter that currently lacks validation. Consider adding validation to ensure it's within reasonable bounds.internal sealed class SyncPolicyCommandValidator : AbstractValidator<SyncPolicyCommand> { public SyncPolicyCommandValidator() { RuleFor(x => x.NumberOfConcurrentRequests).InclusiveBetween(1, 50); + RuleFor(x => x.Since) + .LessThanOrEqualTo(DateTime.UtcNow) + .When(x => x.Since.HasValue); } }
5-11: Add XML documentation for better maintainability.Consider adding XML documentation to describe the validator's purpose and validation rules.
+/// <summary> +/// Validates the SyncPolicyCommand ensuring proper constraints for resource policy synchronization. +/// </summary> +/// <remarks> +/// Validates: +/// - NumberOfConcurrentRequests: Must be between 1 and 50 +/// </remarks> internal sealed class SyncPolicyCommandValidator : AbstractValidator<SyncPolicyCommand>src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/SyncSubjectMap/SyncSubjectMapCommandValidator.cs (1)
9-9: Consider enhancing the batch size validation.While the validation logic is correct, it could be improved for better maintainability and clarity:
- Extract magic numbers into named constants
- Add a custom error message explaining the constraints
- Document the reasoning behind these specific limits
Consider applying this improvement:
- RuleFor(x => x.BatchSize).InclusiveBetween(1, 1000); + private const int MinBatchSize = 1; + private const int MaxBatchSize = 1000; + + RuleFor(x => x.BatchSize) + .InclusiveBetween(MinBatchSize, MaxBatchSize) + .WithMessage($"Batch size must be between {MinBatchSize} and {MaxBatchSize} to ensure optimal performance and resource utilization.");.azure/applications/sync-resource-policy-information-job/staging.bicepparam (1)
1-11: Consider implementing retry logic and monitoringSince this is a critical synchronization job running during off-hours, consider:
- Implementing retry logic for transient failures
- Setting up alerts for job failures
- Adding monitoring for job duration and success rate
This will help ensure reliable policy synchronization and quick detection of issues.
src/Digdir.Domain.Dialogporten.Application/LocalDevelopmentSettings.cs (1)
18-18: Add XML documentation for the new property.The property's purpose and impact on the application's behavior should be documented for better maintainability.
+ /// <summary> + /// When set to true, disables the automatic synchronization of policy information during application startup in local development. + /// This is useful when running without access to the Resource Registry. + /// </summary> public bool DisablePolicyInformationSyncOnStartup { get; set; } = true;src/Digdir.Domain.Dialogporten.Janitor/Commands.cs (1)
2-3: LGTM! Consider documenting the return values.The command implementation looks good with proper dependency injection and error handling. The switch to
SyncSubjectMapCommandaligns with the PR objectives.Consider adding XML documentation to clarify the meaning of return values (0 for success, -1 for validation errors).
Also applies to: 20-25
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241106143110_AddResourcePolicyInformation.cs (1)
1-43: Consider future extensibility needsGiven that this table is part of a larger feature set for resource policy metadata synchronization (issue #1214), consider if additional columns might be needed for:
- Policy version tracking
- Last sync timestamp
- Policy source/origin
- Additional XACML obligations beyond authentication level
This would help avoid multiple migrations as requirements evolve.
src/Digdir.Domain.Dialogporten.WebApi/appsettings.Development.json (1)
79-80: Document the new configuration settingPlease add documentation for the new
DisablePolicyInformationSyncOnStartupsetting:
- Its purpose and when it should be enabled/disabled
- The relationship with
UseInMemoryServiceBusTransport- Any impact on local development workflow
Consider adding these details to:
- Code comments in this file
- The project's development setup guide
- Related configuration documentation
src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/SyncSubjectMap/SyncSubjectMapCommand.cs (3)
17-17: Consider adding domain-specific error types.Given that this command handles resource policy synchronization, consider extending the result types to include more specific error cases such as
ResourceRegistryConnectionErrororPolicySyncError. This would provide better error handling and debugging capabilities.Example enhancement:
- public sealed partial class SyncSubjectMapResult : OneOfBase<Success, ValidationError>; + public sealed partial class SyncSubjectMapResult : OneOfBase<Success, ValidationError, ResourceRegistryConnectionError, PolicySyncError>; + + public record ResourceRegistryConnectionError(string Message, Exception? Exception = null); + public record PolicySyncError(string Message);
19-37: Document the DefaultBatchSize constant.Consider adding XML documentation to explain the rationale behind the chosen default batch size of 1000. This would help maintainers understand any performance implications or resource constraints that influenced this value.
Example enhancement:
+ /// <summary> + /// Default number of resources to process in a single batch. + /// This value was chosen to balance memory usage and database round-trips. + /// </summary> private const int DefaultBatchSize = 1000;
Line range hint
39-85: Consider transaction scope and document time skew.A few suggestions for improvement:
The transaction scope spans the entire operation. For large datasets, consider implementing a chunked transaction approach to prevent long-running transactions.
The time skew value of 1 microsecond needs documentation explaining its purpose and why this specific value was chosen.
The error handling could be more specific about different failure scenarios.
Example enhancement for time skew documentation:
+ // Add a constant with documentation + /// <summary> + /// Time skew applied to prevent missing updates due to timestamp precision differences + /// between systems. The value is minimal to ensure we don't miss any updates while + /// avoiding unnecessary reprocessing. + /// </summary> + private static readonly TimeSpan TimeSkew = TimeSpan.FromMicroseconds(1); var lastUpdated = request.Since ?? await _subjectResourceRepository.GetLastUpdatedAt( - timeSkew: TimeSpan.FromMicroseconds(1), + timeSkew: TimeSkew, cancellationToken: cancellationToken);Example enhancement for chunked transactions:
- await _unitOfWork.BeginTransactionAsync(cancellationToken); await foreach (var resourceBatch in _resourceRegistry .GetUpdatedSubjectResources(lastUpdated, request.BatchSize ?? DefaultBatchSize, cancellationToken)) { + await _unitOfWork.BeginTransactionAsync(cancellationToken); var mergeableSubjectResources = resourceBatch .Select(x => x.ToMergableSubjectResource(syncTime)) .ToList(); var batchMergeCount = await _subjectResourceRepository.Merge(mergeableSubjectResources, cancellationToken); _logger.LogInformation("{BatchMergeCount} subject-resources added to transaction.", batchMergeCount); mergeCount += batchMergeCount; + await _unitOfWork.SaveChangesAsync(cancellationToken); } - await _unitOfWork.SaveChangesAsync(cancellationToken);src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs (1)
201-219: Consider adding XML documentationThe
ResourceRegistryEntryclass handles important URN parsing logic. Consider adding XML documentation to explain:
- The expected URN format
- The significance of the prefixes
- Examples of valid and invalid URNs
+ /// <summary> + /// Utility class to parse resource URNs and determine policy availability in the Resource Registry. + /// </summary> + /// <remarks> + /// URN format: {prefix}:{identifier} + /// - Altinn 2 services use prefix "se_" + /// - Altinn Apps use prefix "app_" + /// - Other resources have their policies available in the Resource Registry + /// </remarks> private sealed class ResourceRegistryEntrysrc/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentResourcePolicyInformationSyncHostedService.cs (2)
16-21: Consider injectingISenderdirectly to simplify the code.Instead of creating a new scope and resolving
ISendereach timeStartAsyncis called, you could injectISenderdirectly into the constructor if its lifetime allows. This would reduce overhead and improve readability.Apply this diff to modify the constructor and class fields:
public class DevelopmentResourcePolicyInformationSyncHostedService : IHostedService { private readonly IServiceProvider _serviceProvider; private readonly IHostEnvironment _environment; private readonly IConfiguration _configuration; + private readonly ISender _sender; public DevelopmentResourcePolicyInformationSyncHostedService( - IServiceProvider serviceProvider, + ISender sender, IHostEnvironment environment, IConfiguration configuration) { - _serviceProvider = serviceProvider ?? throw new ArgumentNullException(nameof(serviceProvider)); + _sender = sender ?? throw new ArgumentNullException(nameof(sender)); _environment = environment ?? throw new ArgumentNullException(nameof(environment)); _configuration = configuration ?? throw new ArgumentNullException(nameof(configuration)); }Update the
StartAsyncmethod accordingly:public async Task StartAsync(CancellationToken cancellationToken) { if (!_environment.ShouldRunDevelopmentHostedService() || _configuration.GetLocalDevelopmentSettings().DisablePolicyInformationSyncOnStartup) { return; } - using var scope = _serviceProvider.CreateScope(); - var sender = scope.ServiceProvider.GetRequiredService<ISender>(); await _sender.Send(new SyncPolicyCommand(), cancellationToken); }
36-36: Add a comment explaining the implementation ofStopAsync.To enhance code clarity, consider adding a comment to indicate that no actions are required when stopping the service.
Apply this diff:
public Task StopAsync(CancellationToken cancellationToken) => + // No actions needed on service stop. Task.CompletedTask;src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/SyncPolicy/SyncPolicyCommand.cs (1)
65-65: Correct grammatical error in log messageThe message "Resource policy information are already up-to-date." should use "is" instead of "are" since "information" is singular.
Apply this diff to correct the grammar:
- _logger.LogInformation("Resource policy information are already up-to-date."); + _logger.LogInformation("Resource policy information is already up-to-date.");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241106143110_AddResourcePolicyInformation.Designer.csis excluded by!**/Migrations/**/*Designer.cssrc/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/DialogDbContextModelSnapshot.csis excluded by!**/Migrations/DialogDbContextModelSnapshot.cs
📒 Files selected for processing (21)
.azure/applications/sync-resource-policy-information-job/prod.bicepparam(1 hunks).azure/applications/sync-resource-policy-information-job/staging.bicepparam(1 hunks).azure/applications/sync-resource-policy-information-job/test.bicepparam(1 hunks).azure/applications/sync-resource-policy-information-job/yt01.bicepparam(1 hunks)src/Digdir.Domain.Dialogporten.Application/Externals/IResourcePolicyInformationRepository.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Externals/IResourceRegistry.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/SyncPolicy/SyncPolicyCommand.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/SyncPolicy/SyncPolicyCommandValidator.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/SyncSubjectMap/SyncSubjectMapCommand.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/SyncSubjectMap/SyncSubjectMapCommandValidator.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/Synchronize/SynchronizeSubjectResourceMappingsCommandValidator.cs(0 hunks)src/Digdir.Domain.Dialogporten.Application/LocalDevelopmentSettings.cs(1 hunks)src/Digdir.Domain.Dialogporten.Domain/ResourcePolicyInformation/ResourcePolicyInformation.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs(5 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentResourcePolicyInformationSyncHostedService.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentSubjectResourceSyncHostedService.cs(2 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241106143110_AddResourcePolicyInformation.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Repositories/ResourcePolicyInformationRepository.cs(1 hunks)src/Digdir.Domain.Dialogporten.Janitor/Commands.cs(2 hunks)src/Digdir.Domain.Dialogporten.Janitor/Properties/launchSettings.json(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/appsettings.Development.json(1 hunks)
💤 Files with no reviewable changes (1)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/Synchronize/SynchronizeSubjectResourceMappingsCommandValidator.cs
🚧 Files skipped from review as they are similar to previous changes (6)
- .azure/applications/sync-resource-policy-information-job/test.bicepparam
- src/Digdir.Domain.Dialogporten.Application/Externals/IResourcePolicyInformationRepository.cs
- src/Digdir.Domain.Dialogporten.Application/Externals/IResourceRegistry.cs
- src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentSubjectResourceSyncHostedService.cs
- src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Repositories/ResourcePolicyInformationRepository.cs
- src/Digdir.Domain.Dialogporten.Janitor/Properties/launchSettings.json
🔇 Additional comments (25)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/SyncPolicy/SyncPolicyCommandValidator.cs (1)
5-11: LGTM! The validator implementation looks good.
The validator correctly implements a reasonable limit for concurrent requests to prevent system overload.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/SyncSubjectMap/SyncSubjectMapCommandValidator.cs (1)
1-11: LGTM! The validator implementation follows good practices.
The class is properly structured with appropriate access modifiers and inheritance. The use of FluentValidation for command validation is a solid choice.
src/Digdir.Domain.Dialogporten.Domain/ResourcePolicyInformation/ResourcePolicyInformation.cs (2)
1-6: LGTM! Well-structured class declaration.
The class is properly sealed and follows the project's namespace conventions.
7-11: 🛠️ Refactor suggestion
Essential improvements needed for entity robustness.
The entity requires several improvements to ensure data integrity and proper initialization:
- Add validation for MinimumAuthenticationLevel to ensure valid authentication levels
- Add length constraints for the Resource property
- Protect audit fields from invalid modifications
- Ensure proper initialization of all fields
The previous review comment provided a comprehensive solution that addresses these concerns. Please implement those changes to make the entity more robust and secure.
Additionally, consider these security-specific recommendations:
- Document the valid range for MinimumAuthenticationLevel in XML comments
- Add validation to ensure Resource doesn't contain sensitive information patterns
- Consider adding a Version property for optimistic concurrency
Here's an additional improvement to consider on top of the previous suggestions:
public sealed class ResourcePolicyInformation : IEntity
{
public Guid Id { get; set; }
public string Resource { get; set; } = null!;
- public int MinimumAuthenticationLevel { get; set; }
+ /// <summary>
+ /// The minimum authentication level required for accessing this resource.
+ /// Valid values are:
+ /// 0 - No authentication required
+ /// 1 - Level 1 (MinID)
+ /// 2 - Level 2 (BankID on mobile)
+ /// 3 - Level 3 (BankID)
+ /// 4 - Level 4 (Physical presence)
+ /// </summary>
+ public int MinimumAuthenticationLevel
+ {
+ get => _minimumAuthenticationLevel;
+ set => _minimumAuthenticationLevel = value is >= 0 and <= 4
+ ? value
+ : throw new ArgumentOutOfRangeException(
+ nameof(value),
+ value,
+ "Authentication level must be between 0 and 4");
+ }
+ private int _minimumAuthenticationLevel;
+
+ /// <summary>
+ /// Version number for optimistic concurrency control
+ /// </summary>
+ public uint Version { get; set; }.azure/applications/sync-resource-policy-information-job/prod.bicepparam (3)
1-1: LGTM! Template import is correctly defined.
The relative path import follows best practices for Bicep template organization.
9-11: Verify error handling for missing environment variables
While using environment variables for sensitive configuration is good practice, ensure that the main Bicep template includes proper error handling if these environment variables are not set.
#!/bin/bash
# Check main.bicep for error handling of these parameters
echo "Checking error handling in main.bicep..."
rg 'readEnvironmentVariable|containerAppEnvironmentName|environmentKeyVaultName|appInsightConnectionString' './main.bicep'Additionally, consider adding validation for the environment variable values to ensure they match expected patterns:
- AZURE_CONTAINER_APP_ENVIRONMENT_NAME should follow Azure naming conventions
- AZURE_ENVIRONMENT_KEY_VAULT_NAME should be a valid Key Vault name
- AZURE_APP_INSIGHTS_CONNECTION_STRING should be a valid connection string format
6-6: Verify job schedule timing alignment with system maintenance windows
The job is scheduled to run at 3:10 AM daily. While this is generally a good time for maintenance tasks, please verify:
- This doesn't conflict with other system maintenance windows
- The timing allows for completion before peak usage hours
- There's sufficient buffer for error recovery if needed
✅ Verification successful
Given that we've searched through the codebase and found:
- No other conflicting job schedules in bicep/bicepparam files
- No maintenance window documentation
- No other cron schedules
I'll generate the final response:
Job schedule at 3:10 AM appears safe to proceed
The codebase analysis shows no conflicting scheduled jobs or documented maintenance windows that would interfere with the 3:10 AM schedule. The early morning timing provides:
- Minimal user impact during off-peak hours
- Sufficient buffer before business hours for error recovery
- Standard maintenance window alignment
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other scheduled jobs in the codebase to identify potential conflicts
rg -g '*.bicep*' -A 2 'param jobSchedule'
Length of output: 266
Script:
#!/bin/bash
# Search for cron schedules or job timing configurations across the codebase
rg -g '*.{bicep,bicepparam,yaml,yml}' -B 2 -A 2 '(\d+\s+\d+\s+\*\s+\*\s+\*|cron:|schedule:)'
Length of output: 94
Script:
#!/bin/bash
# Find all bicep and bicepparam files to understand the infrastructure setup
fd -e bicep -e bicepparam
Length of output: 27
Script:
#!/bin/bash
# Look for any documentation about maintenance windows or scheduling
fd -t f -e md -e txt | xargs rg -i "maintenance|schedule|cron|window"
Length of output: 834
.azure/applications/sync-resource-policy-information-job/yt01.bicepparam (3)
1-4: LGTM! Verify environment naming convention.
The basic configuration looks good. The environment name 'yt01' appears to follow a pattern seen in other environments (prod, staging, test).
✅ Verification successful
Environment naming follows established convention ✅
The environment name 'yt01' is consistent with the standard environment naming pattern used across the Azure infrastructure. The codebase shows a clear pattern of using:
- Production: 'prod'
- Staging: 'staging'
- Testing: 'test'
- YT Environment: 'yt01'
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify environment naming convention consistency across parameter files
# Expected: Similar environment naming pattern in other .bicepparam files
fd -e bicepparam . .azure/applications/sync-resource-policy-information-job/ -x grep -l "param environment ="
Length of output: 764
Script:
#!/bin/bash
# Let's try a different approach to check environment naming patterns
fd -e bicepparam . .azure -x cat {} \; | grep -B1 "param environment ="
Length of output: 1089
5-6: Verify scheduling alignment with other jobs.
The cron expression is valid, but running at 3:25 AM might conflict with other scheduled jobs. Please ensure this timing aligns with the overall system maintenance window.
✅ Verification successful
Schedule timing is well-coordinated with other jobs
The proposed schedule of 3:25 AM aligns well with the existing job schedules. The search revealed:
- The same job in other environments runs at staggered times:
- prod: 3:10 AM
- staging: 3:15 AM
- test: 3:20 AM
- yt01: 3:25 AM (proposed)
The only other scheduled job found (sync-subject-resource-mappings-job) runs every 5 minutes, which won't conflict with this nightly maintenance window.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other scheduled jobs that might conflict
# Expected: No overlapping job schedules around 3:25 AM
# Search for cron schedules in other job configurations
fd -e bicep -e bicepparam . .azure -x grep -l "param jobSchedule.*\*"
Length of output: 764
Script:
#!/bin/bash
# Let's check the actual cron schedules in these files
for file in $(fd -e bicepparam . .azure); do
echo "=== $file ==="
grep "param jobSchedule" "$file"
done
Length of output: 8656
8-11: Consider error handling for missing environment variables.
While using environment variables for secrets is a good practice, ensure that the deployment fails gracefully with clear error messages if any of these required environment variables are missing.
.azure/applications/sync-resource-policy-information-job/staging.bicepparam (3)
1-4: LGTM! Basic configuration follows best practices
The environment and location parameters are well-defined and follow Azure naming conventions.
8-11: Verify environment variable configuration in deployment pipeline
The secret management approach using environment variables is secure. However, we should verify that these variables are properly configured in the deployment pipeline.
Let's check for the presence of these environment variables in workflow files:
#!/bin/bash
# Description: Verify environment variable configuration in deployment workflows
# Expected: Environment variables should be defined in workflow files or referenced from secrets
echo "Checking for environment variable configuration in workflows:"
rg -A 5 "AZURE_(CONTAINER_APP_ENVIRONMENT_NAME|ENVIRONMENT_KEY_VAULT_NAME|APP_INSIGHTS_CONNECTION_STRING)" .github/workflows/5-6: Verify job schedule coordination across environments
The job is scheduled for 3:15 AM nightly. Based on the AI summary, this appears to be coordinated with other environments (prod: 3:10 AM, test: 3:20 AM).
Let's confirm the scheduling pattern across all environments:
✅ Verification successful
The previous output seems to be incorrect as it showed GitHub workflow configurations instead of the job schedules. Let's try again with a more focused search for the job schedules.
Job schedule coordination is properly configured ✅
The job schedules are correctly staggered across environments:
- Production: 3:10 AM
- Staging: 3:15 AM
- Test: 3:20 AM
- YT01: 3:25 AM
The scheduling pattern shows a clear 5-minute gap between environments, which helps prevent concurrent executions and resource contention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify job scheduling coordination across environments
# Expected: Different scheduling times for each environment to prevent concurrent execution
echo "Checking job schedules across environments:"
rg -g "param jobSchedule = '.*'" .azure/applications/sync-resource-policy-information-job/
Length of output: 12790
Script:
#!/bin/bash
# Let's check the job schedules across all environments correctly
echo "Checking job schedules across environments:"
rg "param jobSchedule = '.*'" .azure/applications/sync-resource-policy-information-job/
Length of output: 707
src/Digdir.Domain.Dialogporten.Application/LocalDevelopmentSettings.cs (1)
18-18: Verify the startup configuration usage.
The property looks good, following the established patterns. Let's verify its usage in startup configuration.
✅ Verification successful
Property is correctly used in development environment
The DisablePolicyInformationSyncOnStartup property is properly integrated:
- Defined in
LocalDevelopmentSettings.cswith a default value oftrue - Configured in
appsettings.Development.json - Used in
DevelopmentResourcePolicyInformationSyncHostedServiceto conditionally skip policy sync during development - Service is registered via
InfrastructureExtensions
The implementation follows a clean pattern for development-only configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how this setting is used in startup configuration
# Look for usage in Program.cs, Startup.cs, or similar files
rg -l "DisablePolicyInformationSyncOnStartup"
# Look for references in appsettings files
fd -e json | xargs rg "DisablePolicyInformationSyncOnStartup"
Length of output: 489
Script:
#!/bin/bash
# Let's check how this setting is used in the hosted service
rg -A 10 "DisablePolicyInformationSyncOnStartup" src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentResourcePolicyInformationSyncHostedService.cs
# Also check if there are any other similar sync services that might be affected
fd -e cs | xargs rg "PolicyInformationSync"
Length of output: 1710
src/Digdir.Domain.Dialogporten.Janitor/Commands.cs (2)
27-35: Consider architectural limitations mentioned in PR comments.
The PR comments indicate that the resource subjects endpoint only captures actual changes, potentially missing policy modifications. This limitation might affect the reliability of the synchronization process.
Consider:
- Adding retry logic with exponential backoff
- Implementing a full sync fallback mechanism
- Adding logging to track missed updates
Let's check for existing error handling and logging:
#!/bin/bash
# Look for error handling and logging in the sync implementation
rg -i "logger|log\." -A 5
rg -i "catch|error|exception" -A 5 -g "**/*Sync*.cs"27-35: Verify concurrent request handling and consider parameter naming consistency.
A few points to consider:
- The
numberOfConcurrentRequestsparameter might need bounds checking or rate limiting to prevent overwhelming the Resource Registry. - Consider aligning parameter naming with the other command (e.g.,
batchSizevsnumberOfConcurrentRequests).
Let's verify the implementation of concurrent request handling:
✅ Verification successful
Let me search for the SyncPolicyCommand implementation to verify the concurrent request handling:
Based on the search results, I can now provide a final response:
Concurrent request handling is properly implemented with appropriate bounds
The implementation has good safeguards for concurrent request handling:
- The
numberOfConcurrentRequestsparameter is validated with bounds checking between 1-50 requests - A default value of 15 concurrent requests is used if not specified
- The implementation uses
SemaphoreSlimto properly control concurrent access - Polly retry policies are implemented in the infrastructure layer to handle transient failures
Regarding parameter naming consistency - while there are different parameter names used (batchSize vs numberOfConcurrentRequests), this is actually appropriate as they serve different purposes - one for batching data operations and another specifically for concurrent HTTP requests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SyncPolicyCommand implementation to verify concurrent request handling
ast-grep --pattern 'class SyncPolicyCommand {
$$$
}'
# Look for any rate limiting or retry policies in the codebase
rg -i "rate.?limit|retry.?policy|polly" -A 5
Length of output: 335079
Script:
#!/bin/bash
# Search for SyncPolicyCommand implementation
ast-grep --pattern 'class SyncPolicyCommand {
$$$
}'
# Search for any concurrent request handling in the command handler
rg -i "numberOfConcurrentRequests" -A 5
Length of output: 7011
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241106143110_AddResourcePolicyInformation.cs (4)
1-10: LGTM: Namespace and class structure follow best practices
The migration class is properly structured with appropriate inheritance and namespace organization.
29-34: LGTM: Appropriate index design
The unique index on the Resource column is well-designed, ensuring that each resource can only have one policy entry.
37-41: LGTM: Clean rollback implementation
The Down migration properly cleans up by dropping the table, ensuring a clean rollback if needed.
14-27: Verify the column constraints and consider additional improvements
A few points to consider:
- The 255-character limit on the Resource column might be insufficient for long URIs. Please verify this limit against your expected resource identifiers.
- The UpdatedAt column won't automatically update on record changes. Consider adding a trigger or handling this in the application layer.
- The MinimumAuthenticationLevel column accepts any integer. Consider adding a check constraint if there's a valid range.
Let's check the resource lengths in your system:
Consider these improvements:
migrationBuilder.CreateTable(
name: "ResourcePolicyInformation",
columns: table => new
{
Id = table.Column<Guid>(type: "uuid", nullable: false, defaultValueSql: "gen_random_uuid()"),
- Resource = table.Column<string>(type: "character varying(255)", maxLength: 255, nullable: false),
+ Resource = table.Column<string>(type: "character varying(1024)", maxLength: 1024, nullable: false),
MinimumAuthenticationLevel = table.Column<int>(type: "integer", nullable: false),
CreatedAt = table.Column<DateTimeOffset>(type: "timestamp with time zone", nullable: false, defaultValueSql: "current_timestamp at time zone 'utc'"),
UpdatedAt = table.Column<DateTimeOffset>(type: "timestamp with time zone", nullable: false, defaultValueSql: "current_timestamp at time zone 'utc'")
},
constraints: table =>
{
table.PrimaryKey("PK_ResourcePolicyInformation", x => x.Id);
+ table.CheckConstraint("CK_ResourcePolicyInformation_MinimumAuthenticationLevel", "\"MinimumAuthenticationLevel\" BETWEEN 0 AND 4");
});
+migrationBuilder.Sql(@"
+ CREATE TRIGGER update_resourcepolicyinformation_updatedat
+ BEFORE UPDATE ON ""ResourcePolicyInformation""
+ FOR EACH ROW
+ EXECUTE FUNCTION trigger_set_timestamp();
+");✅ Verification successful
Based on the search results, I can now provide a final response:
Resource length and authentication constraints are appropriate
-
The 255-character limit for the Resource column is consistent with other resource-related fields in the codebase (e.g., ServiceResource, SubjectResource) and appears sufficient as the actual resources are URNs (e.g., "urn:altinn:resource", "urn:altinn:app:app_ttd_some-app_with_underscores").
-
The MinimumAuthenticationLevel column is correctly defined as an integer, as it's used to store authentication levels from IdPorten (levels 3-4) and Altinn. No check constraint is needed since the application layer handles the validation through the ResourceRegistry client.
-
The UpdatedAt handling is consistent with other tables in the codebase that use the same timestamp pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all resource URI definitions or usages to verify max length
rg -g '*.cs' -B 2 -A 2 'Resource.*=.*"https?://'
Length of output: 50
Script:
#!/bin/bash
# Let's try a broader search to understand the Resource field usage and any potential constraints
# First, let's look for any Resource field definitions or constants
rg -g '*.cs' -B 2 -A 2 'Resource\s+='
# Also search for any Resource-related models or configurations
ast-grep --pattern 'class $_ {
$$$
Resource
$$$
}'
# Check for any authentication level enums or constants that might define valid ranges
rg -g '*.cs' 'AuthenticationLevel|SecurityLevel'
Length of output: 28215
src/Digdir.Domain.Dialogporten.WebApi/appsettings.Development.json (1)
80-80: Verify the sync behavior and default value
The DisablePolicyInformationSyncOnStartup setting is disabled by default in development. Please verify:
- If this affects only startup sync or also runtime sync operations
- If developers need to explicitly enable this for local testing
- If this aligns with the expected developer experience
✅ Verification successful
Setting only affects startup sync and is correctly configured
The DisablePolicyInformationSyncOnStartup setting only affects the initial sync during service startup and does not impact any runtime sync operations. The default value of true in development is appropriate because:
- The setting is only checked in
DevelopmentResourcePolicyInformationSyncHostedServiceduring startup - It's combined with an environment check (
ShouldRunDevelopmentHostedService()) - The implementation shows it only prevents the initial
SyncPolicyCommandfrom running at startup - No runtime sync methods were found in the service
This configuration allows developers to opt-in to startup sync when needed while preventing unnecessary API calls during development.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of the DisablePolicyInformationSyncOnStartup setting
# to understand its impact on sync operations
# Look for references to the setting in code
rg -l "DisablePolicyInformationSyncOnStartup"
# Search for any sync-related hosted services
ast-grep --pattern 'class $_ : IHostedService {
$$$
}'
Length of output: 369
Script:
#!/bin/bash
# Let's examine the hosted service implementation and settings usage
rg -A 10 "DisablePolicyInformationSyncOnStartup" src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentResourcePolicyInformationSyncHostedService.cs
# Check the settings class definition
rg -A 5 "DisablePolicyInformationSyncOnStartup" src/Digdir.Domain.Dialogporten.Application/LocalDevelopmentSettings.cs
# Look for any runtime sync methods in the service
ast-grep --pattern 'class DevelopmentResourcePolicyInformationSyncHostedService {
$$$
public async Task $$$Sync$$$($$$) {
$$$
}
$$$
}'
Length of output: 1089
src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/SyncSubjectMap/SyncSubjectMapCommand.cs (2)
8-15: LGTM! Command structure follows CQRS best practices.
The command class is well-structured with appropriate immutability and nullable properties for optional parameters.
Line range hint 39-85: Verify impact of Resource Registry API limitations.
Based on the PR comments, there are limitations with the Resource Registry API where policy changes might not be reflected in the subject/resource mapping. Let's verify the potential impact on this implementation.
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs (1)
110-116: Verify the impact of ignoring the since parameter
The method ignores the since parameter and fetches all resources. This could impact performance and resource usage. Consider adding a TODO comment or tracking issue until the proper API is available.
✅ Verification successful
Let me check the implementation further to understand the impact of ignoring the since parameter.
Let me check one more thing to understand the frequency of these resource updates.
Based on the gathered information, I can now provide the final response:
Temporary implementation is properly documented and tracked
The current implementation ignoring the since parameter is already documented with a clear comment explaining the temporary nature of the solution. Additionally, there's an open issue (#1214) tracking the implementation of required service authentication levels, which is related to the Resource Registry API improvements. The temporary solution, while not optimal, is transparent about its limitations.
- The comment "Until we have an API in RR to fetch updated resources only, we have to fetch them all" clearly documents the reason
- The client is using
IFusionCachewhich helps mitigate performance impact - There's active tracking of Resource Registry API improvements via GitHub issue Fetch and store required authentication level for services #1214
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any existing issues or TODOs related to this temporary implementation
rg -l "TODO.*Resource.*Registry.*API" || gh issue list --search "Resource Registry API"
Length of output: 179
Script:
#!/bin/bash
# Check how GetUniqueUpdatedResources is used
ast-grep --pattern 'GetUniqueUpdatedResources'
# Check for any scheduling or periodic execution patterns
rg -A 5 "GetUniqueUpdatedResources"
Length of output: 1960
Script:
#!/bin/bash
# Check for any scheduling or background job patterns that might call this client
rg -B 5 "ResourceRegistryClient"
# Check the implementation of FetchServiceResourceInformation
ast-grep --pattern 'FetchServiceResourceInformation'
Length of output: 2943
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentResourcePolicyInformationSyncHostedService.cs (1)
25-28: Verify the conditional logic in the environment check.
Currently, the service does not run if either the environment check fails or the synchronization is disabled in the configuration due to the logical OR operator (||). Confirm that this matches the intended behavior. If the service should only run when both conditions are true, consider using the logical AND operator (&&).
Run the following script to ensure the logic aligns with the requirements:
✅ Verification successful
Let me gather more information about the environment check implementation to make a more informed decision.
The conditional logic in the environment check is correct
The current implementation using the logical OR (||) operator is the correct approach. Here's why:
ShouldRunDevelopmentHostedService()returns true only in development environments (and when not running Janitor)- The service should not run if either:
- We're not in a development environment (
!ShouldRunDevelopmentHostedService()) - OR if policy sync is explicitly disabled (
DisablePolicyInformationSyncOnStartup)
- We're not in a development environment (
- This matches the pattern used in other development services where they check environment first
- The configuration toggle (
DisablePolicyInformationSyncOnStartup) provides an additional way to disable the service even in development
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the intended behavior of the conditional logic.
# Expected: Service runs only if in development environment and sync is not disabled.
# Check the usage and comments of ShouldRunDevelopmentHostedService()
rg "ShouldRunDevelopmentHostedService\(\)" -A 5
# Display where DisablePolicyInformationSyncOnStartup is set and its default values
rg "DisablePolicyInformationSyncOnStartup" -A 3 -g '*.json' -t yaml -t json
# Suggest reviewing documentation or requirements for intended logic
Length of output: 3899
Script:
#!/bin/bash
# Check the implementation of ShouldRunDevelopmentHostedService
ast-grep --pattern 'ShouldRunDevelopmentHostedService() {
$$$
}'
# Check for any related environment configuration or settings
rg "IsDevelopment|IsProduction|Environment" -A 3 -g "*.cs" --no-filename
Length of output: 10129
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
...alogporten.Application/Features/V1/ResourceRegistry/Commands/SyncPolicy/SyncPolicyCommand.cs
Outdated
Show resolved
Hide resolved
...alogporten.Application/Features/V1/ResourceRegistry/Commands/SyncPolicy/SyncPolicyCommand.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/SyncPolicy/SyncPolicyCommand.cs (5)
10-14: Add XML documentation and consider non-nullable properties with defaultsThe command properties would benefit from XML documentation explaining their purpose and constraints. Consider making
NumberOfConcurrentRequestsnon-nullable with a default value for better API clarity.public sealed class SyncPolicyCommand : IRequest<SyncPolicyResult> { + /// <summary> + /// Optional timestamp to sync policies from. If not provided, uses last sync time from repository. + /// </summary> public DateTimeOffset? Since { get; set; } - public int? NumberOfConcurrentRequests { get; set; } + + /// <summary> + /// Number of concurrent requests to use when fetching policies. Defaults to 15. + /// </summary> + public int NumberOfConcurrentRequests { get; set; } = 15; }
23-23: Align field name with interface namingThe field name
_resourcePolicyMetadataRepositorydoesn't match the interface nameIResourcePolicyInformationRepository. Consider renaming for consistency:- private readonly IResourcePolicyInformationRepository _resourcePolicyMetadataRepository; + private readonly IResourcePolicyInformationRepository _resourcePolicyInformationRepository;
42-42: Document the purpose of time skewThe 1 microsecond time skew value seems arbitrary. Consider adding a comment explaining its purpose or making it a named constant with documentation.
+ // Small time skew to prevent edge cases in concurrent updates timeSkew: TimeSpan.FromMicroseconds(1),
57-57: Enhance log message clarityThe log message could be more descriptive about what was updated.
- _logger.LogInformation("{MergeCount} copies of resource policy information updated.", mergeCount); + _logger.LogInformation("Updated {MergeCount} resource policy information records in the database.", mergeCount);
70-74: Consider catching specific exceptionsThe catch block currently catches all exceptions. Consider catching and handling specific exceptions that might occur during resource registry operations or database updates.
- catch (Exception e) + catch (ResourceRegistryException e) + { + _logger.LogError(e, "Failed to fetch resource policy information from registry."); + throw; + } + catch (RepositoryException e) { - _logger.LogError(e, "Failed to sync resource policy information."); + _logger.LogError(e, "Failed to update resource policy information in database."); throw; }src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs (1)
82-94: Consider using a batch processing approachProcessing all tasks at once with
Task.WhenAllcould lead to memory pressure with large datasets. Consider processing in batches to maintain a more consistent memory footprint.- var metadataTasks = new List<Task<UpdatedResourcePolicyInformation?>>(); - - foreach (var updatedResource in updatedResources) - { - metadataTasks.Add(ProcessResourcePolicy(updatedResource, semaphore, cancellationToken)); - } - - // Filter out null values (indicating missing/invalid policies) - return (await Task.WhenAll(metadataTasks)) - .Where(x => x != null) - .Select(x => x!) - .ToList(); + var results = new List<UpdatedResourcePolicyInformation>(); + foreach (var batch in updatedResources.Chunk(50)) + { + var batchTasks = batch.Select(resource => + ProcessResourcePolicy(resource, semaphore, cancellationToken)); + var batchResults = await Task.WhenAll(batchTasks); + results.AddRange(batchResults.Where(x => x != null).Select(x => x!)); + } + return results;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/SyncPolicy/SyncPolicyCommand.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs(5 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/SyncPolicy/SyncPolicyCommand.cs (1)
Learnt from: elsand
PR: digdir/dialogporten#1411
File: src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/SyncPolicy/SyncPolicyCommand.cs:42-42
Timestamp: 2024-11-14T11:52:08.630Z
Learning: In .NET (C#), the `TimeSpan.FromMicroseconds(double value)` method exists and can be used to create a `TimeSpan` representing microseconds.
🔇 Additional comments (2)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ResourceRegistry/Commands/SyncPolicy/SyncPolicyCommand.cs (1)
16-17: LGTM! Good use of discriminated union pattern
The OneOf pattern is well-applied here for type-safe result handling.
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs (1)
110-116: Verify the impact of ignoring the since parameter
The method ignores the since parameter and fetches all resources, which could be inefficient for large datasets. While the comment explains this is temporary until an API is available, we should verify the performance impact.
✅ Verification successful
Based on the analysis of the codebase, I can now generate the final response:
Temporary implementation is acceptable with proper safeguards
The current implementation of GetUniqueUpdatedResources that ignores the since parameter is acceptable because:
- It's clearly documented in the code comment that this is a temporary solution until a proper API is available in Resource Registry.
- The implementation is part of a larger system that has built-in safeguards:
- The
SyncSubjectMapCommanduses configurable batch sizes (default 1000) to process resources in chunks - There's proper logging to monitor performance and resource usage
- The
ResourceRegistryClientis configured with timeouts (10 seconds) to prevent long-running operations
- The
- The local development implementation (
LocalDevelopmentResourceRegistry) uses an in-memory cache to prevent performance issues during development.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the size of the resource list
curl -s "resourceregistry/api/v1/resource/resourcelist" | jq length
# Check average response time
time curl -s "resourceregistry/api/v1/resource/resourcelist" > /dev/null
Length of output: 165
Script:
#!/bin/bash
# Let's analyze the codebase to understand the impact better
# Check if there are any other usages of GetUniqueUpdatedResources
ast-grep --pattern 'GetUniqueUpdatedResources'
# Check the implementation of FetchServiceResourceInformation
ast-grep --pattern 'FetchServiceResourceInformation'
# Look for any performance-related tests
rg -l "performance|benchmark" --type cs
# Check if there are any configurations or settings related to resource fetching
rg -i "resourceregistry.*settings|fetch.*limit|page.*size" --type cs
Length of output: 440
Script:
#!/bin/bash
# Let's check the implementation details and usage patterns
# Check the implementation of LocalDevelopmentResourceRegistry
rg -A 10 "class LocalDevelopmentResourceRegistry"
# Look for any pagination or batch processing patterns in the codebase
rg -i "take|skip|batch|page" --type cs -A 3
# Check if there are any configurations for resource fetching limits
fd -e json -e xml -e config . | xargs rg -i "resource.*limit|fetch.*size"
# Look for any logging related to performance or resource fetching
rg "LogInformation|LogWarning|LogError" -A 2 --type cs | rg -i "resource|fetch|performance"
Length of output: 42829
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs
Show resolved
Hide resolved
...rsistence/Configurations/ResourcePolicyInformation/ResourcePolicyInformationConfiguration.cs
Outdated
Show resolved
Hide resolved
...r.Domain.Dialogporten.Infrastructure/Persistence/Development/HostingEnvironmentExtensions.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs
Outdated
Show resolved
Hide resolved
...main.Dialogporten.Infrastructure/Altinn/ResourceRegistry/LocalDevelopmentResourceRegistry.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Application/Externals/IResourceRegistry.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Ole Jørgen Skogstad <skogstad@softis.net>
Co-authored-by: Ole Jørgen Skogstad <skogstad@softis.net> Co-authored-by: Magnus Sandgren <5285192+MagnusSandgren@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/Digdir.Domain.Dialogporten.Janitor/README.md (1)
15-16: Fix markdown list indentation for consistencyThe unordered lists under "Argument(s):" sections have inconsistent indentation. They should be indented by 2 spaces according to the markdown style guide.
Apply this formatting fix:
- - `-s` *Optional*: Override the time of the last synchronization... - - `-b` *Optional*: Override the batch size... + - `-s` *Optional*: Override the time of the last synchronization... + - `-b` *Optional*: Override the batch size...Similar fix needed for the arguments of
sync-resource-policy-information.Also applies to: 24-25
🧰 Tools
🪛 Markdownlint
15-15: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
16-16: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs (2)
78-79: Update misleading commentThe comment suggests serial fetching, but
GetUniqueUpdatedResourcesactually fetches all resources at once.Apply this diff to clarify the comment:
- // First fetch all unique updated resources serially using the supplied "since" value, then iterate them with a fan out to fetch and parse the policies - // concurrently using semaphores. + // First fetch all unique resources at once, then fan out to fetch and parse their policies + // concurrently using semaphores for rate limiting.
110-116: Document parameter usageThe
sinceparameter is ignored due to API limitations. This should be documented in the method signature.Apply this diff to improve documentation:
- private async Task<List<UpdatedResource>> GetUniqueUpdatedResources(DateTimeOffset _, CancellationToken cancellationToken) + private async Task<List<UpdatedResource>> GetUniqueUpdatedResources( + DateTimeOffset since, // Currently unused due to Resource Registry API limitations + CancellationToken cancellationToken)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
src/Digdir.Domain.Dialogporten.Application/Externals/IResourceRegistry.cs(2 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/LocalDevelopmentResourceRegistry.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs(5 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Configurations/ResourcePolicyInformation/ResourcePolicyInformationConfiguration.cs(1 hunks)src/Digdir.Domain.Dialogporten.Janitor/README.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Digdir.Domain.Dialogporten.Application/Externals/IResourceRegistry.cs
- src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/LocalDevelopmentResourceRegistry.cs
- src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Configurations/ResourcePolicyInformation/ResourcePolicyInformationConfiguration.cs
🧰 Additional context used
🪛 Markdownlint
src/Digdir.Domain.Dialogporten.Janitor/README.md
15-15: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
16-16: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
24-24: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
25-25: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
🔇 Additional comments (2)
src/Digdir.Domain.Dialogporten.Janitor/README.md (1)
18-25: LGTM! Clear and concise command documentation.
The documentation for the new sync-resource-policy-information command is well-structured and provides clear information about:
- Purpose of the command
- Available arguments with their types and default values
🧰 Tools
🪛 Markdownlint
24-24: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
25-25: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs (1)
210-218: 🛠️ Refactor suggestion
Add input validation for resourceUrn
The constructor should validate the URI format to prevent potential IndexOutOfRangeException.
Apply this diff to add validation:
public ResourceRegistryEntry(Uri resourceUrn)
{
+ ArgumentNullException.ThrowIfNull(resourceUrn);
var fullIdentifier = resourceUrn.ToString();
+ var lastSeparatorIndex = fullIdentifier.LastIndexOf(UrnSeparator);
+ if (lastSeparatorIndex == -1)
+ {
+ throw new ArgumentException("Invalid URN format: missing separator", nameof(resourceUrn));
+ }
- Identifier = fullIdentifier[(fullIdentifier.LastIndexOf(UrnSeparator) + 1)..];
+ Identifier = fullIdentifier[(lastSeparatorIndex + 1)..];
HasPolicyInResourceRegistry = !Identifier.StartsWith(Altinn2ServicePrefix, StringComparison.Ordinal)
&& !Identifier.StartsWith(AltinnAppPrefix, StringComparison.Ordinal);
}Likely invalid or redundant comment.
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs
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: 1
🧹 Outside diff range and nitpick comments (2)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentSubjectResourceSyncHostedService.cs (2)
14-14: Maintain consistent field naming convention.The field
_configurationfollows the underscore prefix convention, but consider renaming the parameter in the constructor to maintain consistency (currently it has the same name as the field but with an underscore prefix).- private readonly IConfiguration _configuration; + private readonly IConfiguration _configuration; - public DevelopmentSubjectResourceSyncHostedService(IServiceProvider serviceProvider, IHostEnvironment environment, IConfiguration _configuration) + public DevelopmentSubjectResourceSyncHostedService(IServiceProvider serviceProvider, IHostEnvironment environment, IConfiguration configuration)
Line range hint
1-36: Consider extracting configuration settings to a dedicated options class.Since this service is part of the development infrastructure, consider moving the configuration settings to a strongly-typed options class (e.g.,
DevelopmentSyncOptions). This would provide better type safety and IntelliSense support compared to usingGetLocalDevelopmentSettings()extension method.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
src/Digdir.Domain.Dialogporten.Application/LocalDevelopmentSettings.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentResourcePolicyInformationSyncHostedService.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentSubjectResourceSyncHostedService.cs(2 hunks)src/Digdir.Domain.Dialogporten.Janitor/appsettings.Development.json(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/appsettings.Development.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Digdir.Domain.Dialogporten.Application/LocalDevelopmentSettings.cs
- src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentResourcePolicyInformationSyncHostedService.cs
- src/Digdir.Domain.Dialogporten.WebApi/appsettings.Development.json
🧰 Additional context used
📓 Learnings (1)
src/Digdir.Domain.Dialogporten.Janitor/appsettings.Development.json (2)
Learnt from: elsand
PR: digdir/dialogporten#1182
File: src/Digdir.Domain.Dialogporten.Janitor/appsettings.staging.json:4-4
Timestamp: 2024-11-12T05:32:45.312Z
Learning: In this project, "TODO: Add to local secrets" placeholders in configuration files are intentionally used to ensure that missing configurations fail fast and loud. These settings are overridden by Azure app configuration mechanisms, as documented in `docs/Configuration.md`.
Learnt from: elsand
PR: digdir/dialogporten#1182
File: src/Digdir.Domain.Dialogporten.Janitor/appsettings.staging.json:4-4
Timestamp: 2024-11-12T05:32:45.311Z
Learning: In this project, "TODO: Add to local secrets" placeholders in configuration files are intentionally used to ensure that missing configurations fail fast and loud. These settings are overridden by Azure app configuration mechanisms, as documented in `docs/Configuration.md`.
🔇 Additional comments (4)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentSubjectResourceSyncHostedService.cs (2)
Line range hint 1-13: LGTM!
The dependencies are well-organized and the class is properly scoped as internal sealed.
25-33: Verify command behavior compatibility.
The change from SynchronizeSubjectResourceMappingsCommand to SyncSubjectMapCommand should maintain the same functionality. Let's verify the implementation of the new command.
✅ Verification successful
Based on the git history and code analysis, I can now generate the final response:
Command change maintains functionality and improves consistency
The change from SynchronizeSubjectResourceMappingsCommand to SyncSubjectMapCommand is a safe refactoring that maintains the same functionality:
- Both commands have identical properties (
SinceandBatchSize) - Both handlers use the same dependencies and core logic
- The validation rules remain the same (BatchSize between 1-1000)
- The change is part of a broader rename for consistency (from "Synchronize" prefix to "Sync" prefix)
- All references to the old command have been properly updated throughout the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of SyncSubjectMapCommand
# Search for the old and new command implementations
echo "Searching for old command implementation..."
rg -A 10 "class SynchronizeSubjectResourceMappingsCommand"
echo "Searching for new command implementation..."
rg -A 10 "class SyncSubjectMapCommand"
# Search for tests to verify behavior
echo "Searching for related tests..."
fd -e cs -e fs | rg -l "SyncSubjectMap.*Test"
Length of output: 4795
Script:
#!/bin/bash
# Let's search for the handler implementation to verify the behavior
echo "Searching for handler implementation..."
rg -A 30 "class SyncSubjectMapCommandHandler"
# Also search for the old command to see if it still exists
echo "Searching for any references to old command..."
rg "SynchronizeSubjectResourceMappings"
Length of output: 5613
Script:
#!/bin/bash
# Let's see the complete handler implementation to verify the behavior
echo "Searching for complete handler implementation..."
rg -B 5 -A 50 "public async Task<SyncSubjectMapResult> Handle"
# Also search for any git history of the old command to understand the transition
echo "Searching git history for the old command..."
git log -p | rg -A 10 -B 10 "SynchronizeSubjectResourceMappings"
Length of output: 36732
src/Digdir.Domain.Dialogporten.Janitor/appsettings.Development.json (2)
67-68: LGTM! The new configuration flags align with the synchronization feature.
The addition of DisableSubjectResourceSyncOnStartup and DisablePolicyInformationSyncOnStartup flags provides appropriate control over synchronization behavior during development.
66-68: Verify configuration structure across environments.
Let's ensure the new configuration flags are consistently defined across all environment configurations.
✅ Verification successful
Let me try a different approach to verify the configuration structure.
This will help us see the actual content and structure of all appsettings files to verify the configuration consistency.
Configuration structure is correctly placed in Development environment
After reviewing all appsettings files, I can confirm that the new configuration flags (UseInMemoryServiceBusTransport, DisableSubjectResourceSyncOnStartup, DisablePolicyInformationSyncOnStartup) are correctly placed under the LocalDevelopment section in appsettings.Development.json. These flags are development-specific settings and should only exist in the development configuration, which is the current case. Other environment configurations (Production, YT01) appropriately do not contain these development-specific settings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new configuration flags are present in other environment files
# and verify their structure matches.
# Find all appsettings files
fd appsettings.*json$ src/Digdir.Domain.Dialogporten.Janitor/ --exec sh -c '
echo "=== $1 ==="
# Extract LocalDevelopment section and check for the new flags
jq -r ".LocalDevelopment | select(. != null) | \
[\"UseInMemoryServiceBusTransport\", \"DisableSubjectResourceSyncOnStartup\", \"DisablePolicyInformationSyncOnStartup\"] as \$keys | \
\$keys | map({key: ., exists: (. as \$k | \$k | in(..))})" {} \
|| echo "No LocalDevelopment section found"
echo
' {}
Length of output: 727
Script:
#!/bin/bash
# Find all appsettings files and show their content
fd appsettings.*json$ src/Digdir.Domain.Dialogporten.Janitor/ --exec sh -c '
echo "=== $1 ==="
cat {}
echo -e "\n"
'
Length of output: 10078
...porten.Infrastructure/Persistence/Development/DevelopmentSubjectResourceSyncHostedService.cs
Outdated
Show resolved
Hide resolved
…lopment/DevelopmentSubjectResourceSyncHostedService.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentSubjectResourceSyncHostedService.cs (1)
25-25: Consider improving readability of startup conditions.The condition combines environment and configuration checks in a single line. Consider extracting these checks into well-named variables or methods for better readability.
- if (!_environment.IsDevelopment() || _configuration.GetLocalDevelopmentSettings().DisableSubjectResourceSyncOnStartup) + var isDevelopment = _environment.IsDevelopment(); + var isSyncDisabled = _configuration.GetLocalDevelopmentSettings().DisableSubjectResourceSyncOnStartup; + if (!isDevelopment || isSyncDisabled)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentSubjectResourceSyncHostedService.cs(2 hunks)
🔇 Additional comments (1)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Development/DevelopmentSubjectResourceSyncHostedService.cs (1)
33-33: Verify command type change impact.
The command type has changed from SynchronizeSubjectResourceMappingsCommand to SyncSubjectMapCommand. Ensure that all consumers of this functionality have been updated accordingly.
✅ Verification successful
Based on the git history and code search results, I can now provide a conclusive response:
Command rename is properly implemented across the codebase
The change from SynchronizeSubjectResourceMappingsCommand to SyncSubjectMapCommand was a coordinated rename that included:
- Complete file and namespace restructuring from
SynchronizetoSyncSubjectMap - Consistent updates to all related files (command, validator, handler)
- All usages updated in the codebase (DevelopmentSubjectResourceSyncHostedService.cs and Janitor/Commands.cs)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old command
rg "SynchronizeSubjectResourceMappingsCommand"
Length of output: 46
Script:
#!/bin/bash
# Let's verify the new command usage and its implementation
rg "SyncSubjectMapCommand" -A 3
Length of output: 5959
Script:
#!/bin/bash
# Let's check the git history for the command rename
git log -p | rg -A 3 -B 3 "SynchronizeSubjectResourceMappingsCommand|SyncSubjectMapCommand"
Length of output: 17325
Script:
#!/bin/bash
# Let's check for any handler implementations
ast-grep --pattern 'class $_ : IRequestHandler<SyncSubjectMapCommand, $_>'
Length of output: 76
🤖 I have created a release *beep* *boop* --- ## [1.35.0](v1.34.0...v1.35.0) (2024-11-15) ### Features * Synchronization of resource policy metadata ([#1411](#1411)) ([193b764](193b764)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: Are Almaas <arealmaas@gmail.com>
Description
This adds a command akin to sync-subject-resources which fetches information about resource policies and stores the information to a table in postgres. For now, this only parses and stores XACML obligations for required authentication level, but can be fairly easily extended to implement eg. #40
There are however some real limitations to the Resource Registry as of now for Altinn Apps, as they are not really stored there, but merely have a representation that does not include the policy.
For these, we store a default authentication level.Edit: We don't do this, as we cannot safely assume anything here. So we do not store anything unless we have an explicit obligation for authentication level set in a policy we can reach. For all other resources, no authentication level handling will be performed.Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores