Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[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 this issue Jul 15, 2024 · 5 comments
Assignees
Labels
enhancement New feature or request feature request

Comments

@dualtagh
Copy link
Contributor

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
Copy link
Contributor

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

@dualtagh
Copy link
Contributor Author

dualtagh commented Jul 16, 2024

@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
Copy link
Collaborator

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:

  • AddAccountToCacheFromAuthorizationCodeAsync is not used in web APIs (it's for web apps).
  • same for the last one it's for web apps. Do we use it in Mise native?

dualtagh added a commit to AzureAD/microsoft-identity-abstractions-for-dotnet that referenced this issue Jul 29, 2024
…enable source generated Json deserialization for NativeAOT (#131)

AzureAD/microsoft-identity-web#2930

Adding overloads to IDownstreamApi that accept JsonTypeInfo for inputs / outputs so that source generated Json serialization can work in a NativeAOT context.
@jmprieur jmprieur closed this as completed Aug 1, 2024
@bgavrilMS
Copy link
Member

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

@dualtagh dualtagh reopened this Aug 22, 2024
@dualtagh
Copy link
Contributor Author

This is still open - the prerequisite PR in abstractions has been merged and release but this PR implements the changes in microsoft-identity-web #2959

@dualtagh dualtagh self-assigned this Aug 22, 2024
dualtagh added a commit that referenced this issue Aug 27, 2024
…ameter to enable source generated Json deserialization for NativeAOT (#2959)

Addresses #2930

Update Microsoft.Identity.Abstractions to 7.0.0
Implementation for new DownstreamApi interfaces in Microsoft.Identity.Abstractions for source gen Json serialization. Implementaitons are generated or copied as the existing with just the addition of jsonTypeInfo params
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature request
Projects
None yet
Development

No branches or pull requests

4 participants