fix: HttpClientWithCache could not be resolved from DI

[dterenin-the-dev]

Description

This pull request addresses the issue where the HttpClientWithCache instance could not be directly resolved as its concrete type through dependency injection (DI). The changes modify the HttpClientWithCacheExtensions class to support configuration via Action<HttpCacheOptions> instead of directly passing HttpCacheOptions, leveraging IOptionsSnapshot<HttpCacheOptions> for named client configuration retrieval. A new private factory method, CreateHttpClientWithCache, was added to handle the creation of HttpClientWithCache instances, utilizing IOptionsSnapshot to fetch cache options, ensuring proper DI resolution. New unit tests in HttpClientWithCacheExtensionsTests were written to verify the correct registration and configuration of the HTTP client with caching, including named client support and cache options application.

These changes improve the flexibility and usability of the HTTP client with caching by aligning with standard .NET DI practices and ensuring proper configuration scoping.

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update

Testing

• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings

Additional Notes

• The XML documentation for the new CreateHttpClientWithCache method includes details about parameters and possible exceptions.
• The changes are backward compatible, as the default behavior remains unchanged when no configuration action is provided.
• Related issue: Cannot resolve concrete HttpClientWithCache type from Dependency Injection #5 .

[codecov]

Codecov Report

❌ Patch coverage is 69.69697% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.04%. Comparing base (edf3312) to head (8bfc994).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...aching/Extensions/HttpClientWithCacheExtensions.cs	69.69%	8 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main       #6      +/-   ##
==========================================
+ Coverage   69.24%   71.04%   +1.79%     
==========================================
  Files          26       26              
  Lines        1099     1112      +13     
  Branches      132      133       +1     
==========================================
+ Hits          761      790      +29     
+ Misses        319      301      -18     
- Partials       19       21       +2     

Files with missing lines	Coverage Δ
...aching/Extensions/HttpClientWithCacheExtensions.cs	69.04% <69.69%> (+69.04%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[akrisanov]

After testing the PR in the actual branch, I can confirm that the core issue is resolved – both HttpClientWithCache and IHttpClientWithCache can now be resolved from DI. However, there are some architectural decisions that need discussion.

What Works

1. Core issue is fixed

   • serviceProvider.GetService<HttpClientWithCache>() now works
   • serviceProvider.GetService<IHttpClientWithCache>() still works
   • Both return the same singleton instance

2. Scoped Registration is correct

   • This is the proper approach for HttpClient when using IHttpClientFactory
   • Consistent with ServiceCollectionExtensions.cs pattern (line 59: TryAddScoped<HttpClientWithCache>)

3. Good Test Coverage

   • Comprehensive test suite covers most scenarios
   • Tests verify proper registration and configuration

Areas for Discussion (not blockers)

1. API Signature Change

- HttpCacheOptions? cacheOptions = null
+ Action<HttpCacheOptions>? configureCacheOptions = null

Question: Is this breaking change intentional? The original issue didn't request API changes.

Analysis:

• The new approach using Action<HttpCacheOptions> is more flexible
• Aligns with .NET configuration patterns (Configure<T>)
• But it is a breaking change for existing users

Options:

1. Keep the breaking change (current approach)
2. Add overload to maintain backward compatibility
3. Revert to original signature

2. IOptionsSnapshot Usage

The PR uses IOptionsSnapshot<HttpCacheOptions> for configuration:

Pros:

• Follows .NET configuration patterns
• Supports named options properly
• More scalable for complex scenarios

Cons:

• Adds complexity for simple use cases
• May be overkill for the original issue

Missing Tests

The PR should add this specific test to verify the fix:

[Fact]
public void AddHttpClientWithCache_CanResolveConcreteType_SameInstance()
{
    // Arrange
    var services = new ServiceCollection();
    services.AddLogging();
    services.AddHttpClient();
    services.AddSingleton<IHttpResponseHandler, DefaultHttpResponseHandler>();
    services.AddHttpClientWithCache("TestClient");
    
    var serviceProvider = services.BuildServiceProvider();

    // Act
    var concrete = serviceProvider.GetService<HttpClientWithCache>();
    var interface = serviceProvider.GetService<IHttpClientWithCache>();
    
    // Assert - This was the core issue from #5
    concrete.Should().NotBeNull();
    interface.Should().NotBeNull();
    ReferenceEquals(concrete, interface).Should().BeTrue();
}

---

I'm going to approve the PR and release v1.3.1 when PR is ready to merge.

[akrisanov]

While the IOptionsSnapshot approach follows .NET configuration patterns, it might be over-engineering for this specific issue.

Concerns:

• Issue Cannot resolve concrete HttpClientWithCache type from Dependency Injection #5 was about DI resolution, not complex configuration management
• IOptionsSnapshot adds complexity and performance overhead for simple use cases
• Named options are rarely needed for cache settings
• Most users just need simple configuration passing

Suggestion: Consider a simpler approach that maintains backward compatibility:

// Keep original signature (no breaking changes)
public static IServiceCollection AddHttpClientWithCache(
    this IServiceCollection services,
    string? httpClientName = null,
    HttpCacheOptions? cacheOptions = null)

// Add overload for Action<T> pattern when needed
public static IServiceCollection AddHttpClientWithCache(
    this IServiceCollection services,
    string? httpClientName,
    Action<HttpCacheOptions> configureCacheOptions)
{
    var options = new HttpCacheOptions();
    configureCacheOptions(options);
    return AddHttpClientWithCache(services, httpClientName, options);
}

This would:

• Maintain backward compatibility (no breaking changes)
• Keep simple cases simple
• Provide flexibility for those who need the Action<T> pattern
• Avoid IOptionsSnapshot complexity unless truly needed

[dterenin-the-dev]

Thanks for your in-depth investigation and feedback! I have followed suggested approach but also had to add backward compatible overloads for AddResilientHttpClientWithCache methods. See my latest commit with all the review suggestions applied

[akrisanov]

I've checked the latest commit. The solution is:

• better than original: maintains compatibility + adds flexibility
• production ready: handles both simple and complex scenarios optimally
• well tested: comprehensive test coverage including the missing test case

Ready for merge 🚀