Misalignment Between AddRedaction Behavior and Expectations

[GabriMS]

Description

Hello,

I have encountered an issue with Microsoft.Extensions.DependencyInjection.RedactionServiceCollectionExtensions.AddRedaction. Despite its name, AddRedaction internally registers the IRedactorProvider using TryAddSingleton instead of AddSingleton.

Although this may seem like a minor implementation detail, it has significant implications for certain use cases, including mine. Let me elaborate on the scenario:

---

Context

In our library, we currently use Microsoft.R9.Extensions.Redaction.IRedactorProvider but are transitioning to support Microsoft.Extensions.Compliance.Redaction.IRedactorProvider. This change is driven by our need to integrate with HttpClient logging, where the latter provider is required. Our strategy involves:

1. Supporting Microsoft.Extensions.Compliance.Redaction.IRedactorProvider as an alternative to the R9 provider.
2. Asking users to migrate gradually to the new provider.

Internally, however, some components still rely on the R9 redactor, while others, such as HttpClient logging logic, depend on the public redactor. To facilitate this, we implemented the following registration logic:

public IServiceCollection AddLibraryX(this IServiceCollection services)
{
    // Register R9 redactor if not already registered
    services.TryAddSingleton<Microsoft.R9.Extensions.Redaction.IRedactorProvider>(sp =>
    {
        var publicRedactorProvider = sp.GetService<Microsoft.Extensions.Compliance.Redaction.IRedactorProvider>();
        if (publicRedactorProvider == null) return null;

        // Wrap public redactor to implement R9 provider
        return new R9RedactorProviderWrapperForPublicRedaction(publicRedactorProvider);
    });

    // Register public redactor if not already registered
    services.TryAddSingleton<Microsoft.Extensions.Compliance.Redaction.IRedactorProvider>(sp =>
    {
        var r9RedactorProvider = sp.GetService<Microsoft.R9.Extensions.Redaction.IRedactorProvider>();
        if (r9RedactorProvider == null) return null;

        // Wrap R9 redactor to implement public provider
        return new PublicRedactorProviderWrapperForR9Redaction(r9RedactorProvider);
    });

    // Other service configurations
}

---

The Problem

When a partner integrates our library and adds AddRedaction as follows:

services
    .AddLibraryX()
    .AddRedaction(); // Adds the public redactor

We encounter an issue. The internal TryAddSingleton logic in AddRedaction does not execute because our registrations take precedence. Specifically:

• Our implementation of TryAddSingleton returns null when the corresponding provider is not already registered (e.g., Microsoft.R9.Extensions.Redaction.IRedactorProvider).
• Consequently, the expected redaction functionality is not configured correctly.

This issue surfaced during our end-to-end testing and caused unexpected failures. The root cause is that a method named AddRedaction implies the use of AddSingleton, aligning with the expectation that it actively registers the service. However, the current behavior of using TryAddSingleton is counterintuitive and caused integration challenges.

---

Suggested Resolution

To address this issue, I propose the following:

1. Change the behavior of AddRedaction to use AddSingleton instead of TryAddSingleton. This aligns with its name and makes its behavior more predictable.
2. If preserving the current TryAddSingleton behavior is necessary for compatibility, consider introducing a separate TryAddRedaction method that explicitly reflects its intended use.

These changes would ensure that the framework adheres to intuitive naming conventions and avoids surprises for developers integrating redaction functionality.

---

Thank you for considering this issue. Please let me know if you need further details or assistance.

Reproduction Steps

The following steps demonstrate the issue with the current behavior of AddRedaction:

// Create a service collection
var services = new ServiceCollection();

// Add a custom registration for IRedactorProvider
services.TryAddSingleton<Microsoft.Extensions.Compliance.Redaction.IRedactorProvider>(sp =>
{
    // Attempt to retrieve the R9 redactor provider
    var r9RedactorProvider = sp.GetService<Microsoft.R9.Extensions.Redaction.IRedactorProvider>();
    if (r9RedactorProvider == null) return null;

    // Wrap the R9 redactor provider to implement the public provider
    return new PublicRedactorProviderWrapperForR9Redaction(r9RedactorProvider);
});

// Add the redaction service
services.AddRedaction();

// Build the service provider
var serviceProvider = services.BuildServiceProvider();

// Attempt to retrieve the public redactor provider
// Expected: An instance of Microsoft.Extensions.Compliance.Redaction.IRedactorProvider
// Actual: null
var redactorProvider = serviceProvider.GetService<Microsoft.Extensions.Compliance.Redaction.IRedactorProvider>();

Expected behavior

The redactorProvider should be a valid instance of Microsoft.Extensions.Compliance.Redaction.IRedactorProvider because AddRedaction is expected to actively register the redactor provider using AddSingleton.

Actual behavior

The redactorProvider is null. This happens because:

1. The custom TryAddSingleton registration is executed first and returns null if the R9 redactor provider is not registered.
2. The AddRedaction method internally uses TryAddSingleton, which does not override the existing registration.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

[amadeuszl]

Why exactly do you need those wrappers around each RedactionProvider? Not sure I understand need of Dotnet extensions RedactionProvider when R9 one is injected somewhere or vice versa.
Could you prevent/recommend your partner from running public AddRedaction() and do it via AddLibraryX()? Eg.:

public static IServiceCollection AddLibraryX(this IServiceCollection services, Action<IRedactionBuilder> configure)
{
    // Register R9 redactor if not already registered
    services.TryAddSingleton<Microsoft.R9.Extensions.Redaction.IRedactorProvider>(sp =>
    {
        // Register R9 provider, so it works for R9 dependent components
    });

    services.AddRedaction(configure); // Add Dotnet Extensions provider
}