V2: Major architecture upgrade to .NET 10

[ahmet-cetinkaya]

Summary

• Upgrade to .NET 10
• Restructure solution with modular architecture
• Add comprehensive pipeline behaviors (auth, caching, logging, performance, validation)
• Implement CQRS patterns with MediatR-style approach
• Enhanced security with JWT authentication and role-based authorization
• Improved persistence layer with Entity Framework integration

Test plan

• Build and test all projects
• Verify pipeline behaviors work correctly
• Test authentication and authorization flows
• Validate Entity Framework integration
• Run comprehensive test suite

[gemini-code-assist]

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

[ahmet-cetinkaya]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a significant and impressive architectural upgrade to .NET 10, modularizing the solution and improving many aspects like developer workflow, performance, and security abstractions. The new structure is much cleaner and more maintainable.

My review has identified a few issues, including a critical security vulnerability in the authorization logic that needs immediate attention. I've also found a potential security concern with the use of sudo in scripts and a few minor issues in configuration and documentation. Overall, this is a fantastic step forward for the project. Once the identified issues are addressed, this will be a solid foundation to build upon.

[gemini-code-assist]

There's a security vulnerability in the HasAnyRequiredRole method. An empty or whitespace string in the requiredRoles array will incorrectly grant access.
Specifically, this line is problematic:
if (string.IsNullOrWhiteSpace(required)) return true;
This should probably be continue; to ignore invalid role entries.
The logic with return !hasAnyValidRequiredRole; is also a bit confusing. A clearer and safer implementation is recommended.

    public bool HasAnyRequiredRole()
    {
        if (!IsAuthenticated)
            return false;

        if (_hasAdminRole)
            return true;

        if (_requiredRoles is null || _requiredRoles.Length == 0)
            return true;

        // If there are no valid roles to check against, access is granted.
        if (!_requiredRoles.Any(r => !string.IsNullOrWhiteSpace(r)))
            return true;

        foreach (string required in _requiredRoles)
        {
            if (string.IsNullOrWhiteSpace(required))
                continue;

            ReadOnlySpan<char> requiredSpan = required.AsSpan().Trim();
            foreach (string identity in _identityRoles!)
            {
                if (string.IsNullOrWhiteSpace(identity))
                    continue;

                if (identity.AsSpan().Trim().Equals(requiredSpan, StringComparison.OrdinalIgnoreCase))
                    return true;
            }
        }

        return false;
    }

[gemini-code-assist]

The benchmark scripts use sudo. This can pose a security risk and may cause issues in CI/CD environments or on developer machines where the user doesn't have sudo privileges. Is sudo strictly necessary? If it's for accessing performance counters, consider if there are alternative ways to run the benchmarks without requiring elevated privileges, or document why it's needed.

[gemini-code-assist]

The version of roslynator.dotnet.cli has been downgraded from 0.10.0 to 0.9.3. Was this intentional? Downgrading a tool might re-introduce old bugs or remove features. If this is to fix a compatibility issue, it would be good to add a comment here for future reference.

[gemini-code-assist]

The NArchitecture logo image link appears to be broken. The URL points to a GitHub user attachment asset which might not be accessible to everyone or could be temporary. It's recommended to commit the image to the repository (e.g., in a docs/images folder) and use a relative path to ensure it always loads correctly.

[gemini-code-assist]

There seems to be a typo in the example for ISecuredRequest. The type RoleClaims is used, but based on the new AuthorizationBehavior, it should probably be AuthOptions.

Suggested change

	public RoleClaims RoleClaims => new(CurrentUser.Roles, ["Admin"]);
	public AuthOptions AuthOptions => new(CurrentUser.Roles, ["Admin"]);

[ahmet-cetinkaya]

/gemini review

[gemini-code-assist]

Code Review

This is an extensive and impressive architectural upgrade. The move to a more modular structure, the introduction of a custom high-performance Mediator, and the abstraction of core services like mapping are excellent improvements. The code demonstrates a strong focus on performance and modern C# practices. I've found a few issues, including a critical one in the new Mediator's service registration that would prevent some pipeline behaviors from working, and a couple of other suggestions for improvement.

[gemini-code-assist]

The logic to skip behaviors with constructor parameters seems incorrect for dependency injection scenarios. The DI container is responsible for resolving constructor dependencies. This check will prevent any behavior that has dependencies (like ILogger or IHttpContextAccessor) from being registered, which is a common use case. This check should be removed to allow the DI container to correctly instantiate behaviors with their dependencies.

[gemini-code-assist]

The AddYamlResourceLocalization extension method currently uses Assembly.GetExecutingAssembly() to find localization resources. This forces the resources to be in the output directory of the NArchitecture.Core.Localization.Resource.Yaml.DependencyInjection library, which is not ideal for consumers of this library who would expect to keep resources in their own application projects. A more flexible approach would be to allow the consumer to specify which assemblies to scan, similar to how AddMediator and AddNArchitectureAutoMapper are implemented. I recommend changing the method signature to accept assemblies to scan.

public static IServiceCollection AddYamlResourceLocalization(this IServiceCollection services, params Assembly[] assembliesToScan)
    {
        _ = services.AddScoped<ILocalizationService, ResourceLocalizationManager>(_ =>
        {
            Dictionary<string, Dictionary<string, string>> resources = GetLocalizationResources(assembliesToScan);
            return new ResourceLocalizationManager(resources);
        });

        return services;
    }

    private static Dictionary<string, Dictionary<string, string>> GetLocalizationResources(Assembly[] assembliesToScan)
    {

[gemini-code-assist]

The explicit call to transactionScope.Dispose() within the catch block is redundant. The using statement on line 21 already ensures that Dispose() will be called on the transactionScope when the try block is exited, whether normally or due to an exception. If an exception occurs, transactionScope.Complete() is not called, and the transaction is automatically rolled back upon disposal by the using block. Removing this redundant call makes the code cleaner and relies on the standard using pattern for resource management.

[ahmet-cetinkaya]

/gemini review

[gemini-code-assist]

Code Review

This is an impressive and substantial upgrade to the project's architecture. The move to a more modular structure with .NET 10, the introduction of a custom Mediator, and the focus on performance through modern C# features are all excellent changes. The new pipeline behaviors for caching, logging, and authorization are well-designed and robust. My review includes a few suggestions to improve cross-platform compatibility and enhance the flexibility of the new logging features. Overall, this is a very strong pull request that significantly modernizes the codebase.

[gemini-code-assist]

The clean script uses Unix-specific commands (find, rm -rf), which will cause it to fail on Windows development environments. For better cross-platform compatibility, consider using a tool that works on all supported platforms. A cross-platform PowerShell equivalent would be:

"Get-ChildItem -Path . -Include bin,obj -Recurse -Directory | ForEach-Object { Remove-Item -Path $_.FullName -Recurse -Force }; dotnet restore; dotnet build --configuration Release"

This would require developers to have PowerShell Core installed, which is a reasonable expectation for .NET development.

[gemini-code-assist]

The MaskEmail method contains hardcoded logic that checks for a specific property name, "SensitiveData". This creates a tight coupling between the logging behavior and specific property names in your requests, making it less reusable and harder to maintain. It's better to rely on the configuration provided in the LogExcludeParameter (KeepStartChars, KeepEndChars) for all properties. If a default masking strategy for emails is desired, it should be applied when KeepStartChars and KeepEndChars are not specified, rather than being tied to a property name.

        int startChars = param.KeepStartChars;
        int endChars = param.KeepEndChars;

[gemini-code-assist]

The MaskNumeric method uses hardcoded values for keepStart and keepEnd, ignoring any values that might be set in the LogExcludeParameter. This limits flexibility. The method should respect the KeepStartChars and KeepEndChars from the param argument, perhaps with a default fallback if they are not set to allow for configurable masking of numeric values.

        int keepStart = param.KeepStartChars;
        int keepEnd = param.KeepEndChars;
        if (keepStart == 0 && keepEnd == 0)
        {
            keepStart = 2;
            keepEnd = 2;
        }

[gemini-code-assist]

The methods in this base class are async but wrap synchronous Serilog calls (e.g., Logger.Fatal(message)), returning Task.CompletedTask. This doesn't provide true asynchrony and can block the calling thread if the underlying sink performs I/O. To achieve non-blocking logging, you should configure Serilog with Serilog.Sinks.Async. This would involve wrapping your sinks in the LoggerConfiguration, for example: .WriteTo.Async(a => a.File(...)). This ensures that log messages are passed to a background thread for processing, preventing your application threads from being blocked by logging operations.

[ahmet-cetinkaya]

/gemini review

[gemini-code-assist]

Code Review

This is an impressive and extensive architectural upgrade. The move to a modular structure, the introduction of abstractions for core services like mailing and mapping, and the implementation of a custom Mediator pattern are all excellent changes that will improve maintainability and flexibility. The code shows a strong focus on performance and modern C# features. My feedback focuses on a few areas to further enhance robustness and performance.

[gemini-code-assist]

The current implementation for discovering localization resources relies on scanning the file system based on assembly.Location. This approach can be fragile and may fail in certain deployment scenarios, such as single-file executables, containerized environments where file paths might differ, or when assemblies are loaded from memory. A more robust solution would be to use embedded resources. By embedding the YAML files into the assemblies and loading them via assembly.GetManifestResourceStream(), you would decouple the localization system from the physical file structure on disk, making it more reliable across different environments.

[gemini-code-assist]

The await on the logging call inside the finally block can introduce latency, which is undesirable in a performance monitoring behavior. The request pipeline will be blocked until the logging operation completes. To avoid this, the logging call should be fire-and-forget.

                _ = _logger.InformationAsync(
                    $"Performance -> {requestName} took {_stopwatch.Elapsed.TotalSeconds}s, exceeding the threshold of {request.IntervalOptions.Interval}s"
                );

[gemini-code-assist]

The current mediator implementation uses reflection on every SendAsync call to find and invoke handlers and behaviors. This can introduce performance overhead, especially for frequently used requests. Caching the constructed handlers and pipeline delegates would significantly improve performance. A ConcurrentDictionary could be used to store delegates created for each request type, so that the reflection work is only done once per request type.