microsoft-identity-web icon indicating copy to clipboard operation
microsoft-identity-web copied to clipboard

[Feature Request] introduce a base class for IDownstreamApi to simplify mocking in tests

Open Meir017 opened this issue 11 months ago • 6 comments

Is your feature request related to a problem? Please describe. A clear and concise description of what the problem is. Ex. I'm always frustrated when [...].

mocking the IDownstreamApi class in tests isn't trivial, the interface contains 36 methods (by creating a test class the implements this interface).

Describe the solution you'd like To have a base abstract class DownstreamApiBase that implements the interface and exposes just a single method

protected async Task<HttpResponseMessage> CallApiInternalAsync(
  HttpRequestMessage httpRequestMessage,
  DownstreamApiOptions effectiveOptions,
  bool appToken,
  HttpContent? content = null,
  ClaimsPrincipal? user = null,
  CancellationToken cancellationToken = default);

this would mean that the current https://github.com/AzureAD/microsoft-identity-web/blob/master/src/Microsoft.Identity.Web.DownstreamApi/DownstreamApi.cs#L455 API would be slightly different

internal /* for tests */ async Task<HttpResponseMessage> CallApiInternalAsync(
            string? serviceName,
            DownstreamApiOptions effectiveOptions,
            bool appToken,
            HttpContent? content = null,
            ClaimsPrincipal? user = null,
            CancellationToken cancellationToken = default)
        {
            // Downstream API URI
            string apiUrl = effectiveOptions.GetApiUrl();

            // Create an HTTP request message
            using HttpRequestMessage httpRequestMessage = new(
                new HttpMethod(effectiveOptions.HttpMethod),
                apiUrl);
            if (content != null)
            {
                httpRequestMessage.Content = content;
            }

            // call the new protected API
            return await CallApiInternalAsync(message, effectiveOptions, appToken, user, cancellationToken);

then the current DownstreamApi class would extend this abstract-class and override the CallApiInternalAsync method. This would allow any HttpClient mocking library to easily be leveraged here since the new API would be close to the HttpClient.SendAsync API.

Describe alternatives you've considered using the Moq library and mocking the interface IDownstreamApi - mocking the correct method is a bit of a nuisance since there are many similar API's.

Additional context Add any other context or screenshots about the feature request here.

Meir017 avatar Jan 14 '25 09:01 Meir017

@bgavrilMS: any thoughts?

jmprieur avatar Jan 14 '25 18:01 jmprieur

@jmprieur @bgavrilMS any update on this?

I'm willing to create the PR here, just want to be sure it's relevant and that the API change as suggested is approved.

Meir017 avatar Mar 14 '25 03:03 Meir017

Thanks for offering, @Meir017 What changes do you have in mind for the public API?

jmprieur avatar Mar 14 '25 20:03 jmprieur

@jmprieur I drafted an initial PR to highlight the API changes this would include

Meir017 avatar Mar 16 '25 07:03 Meir017

@jmprieur Maybe we could take another path and make many interface API's extension methods in the same assembly? This will result in the interface containing just the critical method, allowing for easy mocking.

This might be more complex as it involves multiple repositories.

Maybe we could start by making the class public and making the core method with the logic overrideable.

-internal partial class DownstreamApi : IDownstreamApi
+public partial class DownstreamApi : IDownstreamApi
{
-    internal /* for tests */ async Task<HttpResponseMessage> CallApiInternalAsync(
+    protected virtual async Task<HttpResponseMessage> CallApiInternalAsync(
}

This should be a good start, If this makes sense, I'll be happy to create a proper PR for this

Meir017 avatar Apr 04 '25 15:04 Meir017

all this is great but can someone show me how to currently do a unit test for a call to .GetForAppAsync<T>() i need to finish a pull and i am trying to get this solved.... thanks!

figuerres avatar May 09 '25 21:05 figuerres