AzureAD / microsoft-identity-web

Helps creating protected web apps and web APIs with Microsoft identity platform and Azure AD B2C
MIT License
671 stars 207 forks source link

[Feature Request] Overloaded GetForUserAsync that takes a JsonTypeInfo<T> as a parameter to enable source generated Json deserialization (For NativeAOT compliance) #2930

Open dualtagh opened 1 month ago

dualtagh commented 1 month ago

Is your feature request related to a problem? Please describe. microsoft-identity-web emits NativeAOT trim warnings when GetForUserAsync is called from a NativeAOT compiled library. This is caused by using generics for JSON serialization/deserialization in DownstreamApi.cs.

ILC : Trim analysis error IL2026: Microsoft.Identity.Web.DownstreamApi.<DeserializeOutput>d__17`1.MoveNext(): Using member 'System.Text.Json.JsonSerializer.Deserialize<TOutput>(String,JsonSerializerOptions)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. JSON serialization and deserialization might require types that cannot be statically analyzed. Use the overload that takes a JsonTypeInfo or JsonSerializerContext, or make sure all of the required types are preserved.

Describe the solution you'd like Public interfaces (particularly GetForUserAsync) should accept a JsonTypeInfo so that source generation can be used for Json serialization in a NativeAOT context. Existing interfaces should be marked with the [RequiresUnreferencedCode] attribute. New interfaces that accept JsonTypeInfo should be made available only for net8+.

Example change for IDownstreamApi:

 [RequiresUnreferencedCode]
public Task<TOutput?> GetForUserAsync<TOutput>(
    string? serviceName,
    Action<DownstreamApiOptionsReadOnlyHttpMethod>? downstreamApiOptionsOverride = null,
    ClaimsPrincipal? user = null,
    CancellationToken cancellationToken = default) where TOutput : class;

public Task<TOutput?> GetForUserAsync<TOutput>(
    string? serviceName,
    JsonTypeInfo<TOutput> responseJsonTypeInfo,
    Action<DownstreamApiOptionsReadOnlyHttpMethod>? downstreamApiOptionsOverride = null,
    ClaimsPrincipal? user = null,
    CancellationToken cancellationToken = default) where TOutput : class;

Describe alternatives you've considered Open to suggestions

Additional context There is a short term need for GetForUserAsync to be NativeAOT compliant - but there are other interfaces with AOT warnings that could require similar changes. Will follow up with more details on this

keegan-caruso commented 1 month ago

This sounds right. Only question I have is how many of the other interfaces should we patch in the same release.

dualtagh commented 1 month ago

@keegan-caruso
These are the other interfaces that have AOT warnings for Json serialization. I could use some help understanding if these should be patched too? The fix would be the same as above - providing an overload with JsonTypeInfo.

DownstreamApi.cs CallApiForAppAsync<TInput, TOutput> CallApiForUserAsync CallApiForAppAsync<TInput, TOutput>

DownstreamApi.HttpMethods.cs GetForUserAsync GetForUserAsync<TInput, TOutput> GetForAppAsync GetForAppAsync<TInput, TOutput> PostForUserAsync<TInput, TOutput> PostForAppAsync<TInput, TOutput> PutForUserAsync<TInput, TOutput> PutForAppAsync<TInput, TOutput> PatchForUserAsync<TInput, TOutput> PatchForAppAsync<TInput, TOutput> DeleteForUserAsync<TInput, TOutput> DeleteForAppAsync<TInput, TOutput>

DownstreamWebApiGenericExtensions.cs GetForUserAsync

Microsoft.Identity.Web.TokenAcquisition\TokenAcquisition.cs AddAccountToCacheFromAuthorizationCodeAsync

Microsoft.Identity.Web\WebAppExtensions\MicrosoftIdentityWebAppAuthenticationBuilder.cs WebAppCallsWebApiImplementation

jmprieur commented 1 month ago

the ones in DownstreamApi.cs and DownstreamApi.HttpMethods.cs should be if we can patch for DownstreamWebApiGenericExtensions.cs this would be great.

I would not do it for the last two:

bgavrilMS commented 2 weeks ago

Just curious how this was fixed? I see a draft PR still open for this topic.

dualtagh commented 6 days ago

This is still open - the prerequisite PR in abstractions has been merged and release but this PR implements the changes in microsoft-identity-web https://github.com/AzureAD/microsoft-identity-web/pull/2959