Make Azure.ResourceManager AOT-compatible #50129

agocke · 2025-05-16T21:42:46Z

There were only two places which were not AOT compatible:

Use of non-source generated JSON.
One method which uses non-generated configuration bindings, which I've simply marked incompatible for now.

Note: almost all of this code was generated by VS Code CoPilot w/ Agent Mode. If you're interested in the prompts I used and the chat session, contact me privately and I'd be happy to share the whole session.

github-actions · 2025-05-16T21:43:07Z

Thank you for your contribution @agocke! We will review the pull request and get back to you soon.

Copilot

Pull Request Overview

This PR updates Azure.ResourceManager to be AOT-compatible by replacing non-source generated JSON with source-generated contexts and marking incompatible configuration bindings.

Added attributes in ArmClientBuilderExtensions to flag reflection and dynamic code usage.
Introduced new JSON context classes and updated serialization/deserialization calls to use them.
Updated the project file and ArmClientOptions to support AOT compatibility.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
sdk/resourcemanager/Azure.ResourceManager/src/Extensions/ArmClientBuilderExtensions.cs	Added AOT-related attributes to the AddArmClient method.
sdk/resourcemanager/Azure.ResourceManager/src/Common/Custom/Models/ManagedServiceIdentityJsonContext.cs	Added a new JSON source generation context.
sdk/resourcemanager/Azure.ResourceManager/src/Common/Custom/Models/ManagedServiceIdentity.Serialization.cs	Updated JSON serialization/deserialization to use the new context.
sdk/resourcemanager/Azure.ResourceManager/src/Azure.ResourceManager.csproj	Added AOT compatibility property.
sdk/resourcemanager/Azure.ResourceManager/src/ArmEnvironmentJsonContext.cs	Introduced a context for ArmEnvironment JSON serialization.
sdk/resourcemanager/Azure.ResourceManager/src/ArmEnvironment.cs	Updated ToString to utilize the source-generated context.
sdk/resourcemanager/Azure.ResourceManager/src/ArmClientOptionsJsonContext.cs	Added a JSON context for ArmClientOptions.
sdk/resourcemanager/Azure.ResourceManager/src/ArmClientOptions.cs	Migrated deserialization to use the new JSON context.

sdk/resourcemanager/Azure.ResourceManager/src/ArmClientOptions.cs

jsquire · 2025-05-16T23:32:06Z

@m-nash: Could you give this another pair of eyes from the management + MRW perspective?
@m-redding: Could you give this an AOT look?

agocke · 2025-05-16T23:33:34Z

FYI, still trying to repro those code generation failures locally. For some reason restore is taking forever

jsquire

LGTM, though I lack in-depth knowledge of the management packages. I've pulled a couple of others in for additional perspectives.

jsquire · 2025-05-16T23:38:06Z

@agocke: The failures are:

The new attributes constitute a change in the API shape, so you'll need to export via: eng\scripts\Export-API.ps1 core
The serialization format for managed identity seems to have changed enough that the recording no longer matches. That may or may not be a concern.

agocke · 2025-05-19T21:37:44Z

I've fixed up all the formatting and added the attribute, so I think this is ready to go.

m-redding

This LGTM from my perspective although I also don't have much knowledge on the mgmt packages

m-redding · 2025-05-19T23:12:41Z

sdk/resourcemanager/Azure.ResourceManager/src/Extensions/ArmClientBuilderExtensions.cs

@@ -23,6 +24,8 @@ public static IAzureClientBuilder<ArmClient, ArmClientOptions> AddArmClient<TBui
        /// <summary>
        /// Registers an <see cref="ArmClient"/> instance with connection options loaded from the provided <paramref name="configuration"/> instance.
        /// </summary>
+        [RequiresUnreferencedCode("Uses reflection for registration")]
+        [RequiresDynamicCode("Uses code generation for registration")]


nit - The message could be more descriptive like what I have in this one here"This utilizes reflection-based JSON serialization and deserialization which is not compatible with trimming."
idk if there's an AOT safe alternative path here but if there is adding that would also be a plus

Here is what we used in another place https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/appconfiguration/Azure.Data.AppConfiguration/src/ConfigurationClientBuilderExtensions.cs#L42-L43

jsquire · 2025-05-21T16:13:57Z

@ArthurMa1978, @ArcturusZhang, @live1206, @m-nash: Folks, can you please give this a quick look from the management ownership perspective? Since everything is green, I'm going to move forward with the merge tomorrow EOD if nobody registers any objections or questions.

m-nash · 2025-05-22T17:03:23Z

I think we want to switch this to SCM SG to stay in line with the other core libraries. Spoke with @agocke today offline and we will come up with a overarching plan to fix the cross library JsonSerializer calls

m-nash · 2025-05-22T17:09:26Z

sdk/resourcemanager/Azure.ResourceManager/src/Extensions/ArmClientBuilderExtensions.cs

@@ -23,6 +24,8 @@ public static IAzureClientBuilder<ArmClient, ArmClientOptions> AddArmClient<TBui
        /// <summary>
        /// Registers an <see cref="ArmClient"/> instance with connection options loaded from the provided <paramref name="configuration"/> instance.
        /// </summary>
+        [RequiresUnreferencedCode("Uses reflection for registration")]
+        [RequiresDynamicCode("Uses code generation for registration")]


Here is what we used in another place https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/appconfiguration/Azure.Data.AppConfiguration/src/ConfigurationClientBuilderExtensions.cs#L42-L43

m-nash · 2025-05-22T17:11:55Z

...emanager/Azure.ResourceManager/src/Common/Custom/Models/ManagedServiceIdentityJsonContext.cs

+    [JsonSerializable(typeof(ManagedServiceIdentity))]
+    [JsonSerializable(typeof(ManagedServiceIdentityType))]
+    [JsonSerializable(typeof(UserAssignedIdentity))]
+    internal partial class ManagedServiceIdentityJsonContext : JsonSerializerContext


Lets drop this context file and use MRWContext instead which is already created https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/resourcemanager/Azure.ResourceManager/src/Common/Generated/Models/AzureResourceManagerContext.cs.

I think the other JsonSerializerContext files you created above (https://github.com/Azure/azure-sdk-for-net/pull/50129/files#diff-db2b8de102500dff3ce1b4cca617984a46fc86d3b50072de5dd2982af021a09a, https://github.com/Azure/azure-sdk-for-net/pull/50129/files#diff-ac7d79bf6d6b455a3e36744cfa1bc766421ac11e520f80cd6b667c53625764a3) are fine since those are not IJsonModel's and aren't intended to be used in service communication. The serialization there is for utility only.

m-nash · 2025-05-22T17:13:03Z

...nager/Azure.ResourceManager/src/Common/Custom/Models/ManagedServiceIdentity.Serialization.cs

@@ -45,7 +50,7 @@ internal void Write(Utf8JsonWriter writer, ModelReaderWriterOptions options, Jso
                foreach (var item in UserAssignedIdentities)
                {
                    writer.WritePropertyName(item.Key);
-                    JsonSerializer.Serialize(writer, item.Value);
+                    JsonSerializer.Serialize(writer, item.Value, jsonContext.UserAssignedIdentity);


Lets swap the serialize calls to a cast to IJsonModel and call the write method on there passing in the MRWOptions and MRWContext

m-nash · 2025-05-22T17:13:52Z

...nager/Azure.ResourceManager/src/Common/Custom/Models/ManagedServiceIdentity.Serialization.cs

@@ -202,7 +211,7 @@ internal static ManagedServiceIdentity DeserializeManagedServiceIdentity(JsonEle
                }
                if (property.NameEquals("type"u8))
                {
-                    type = JsonSerializer.Deserialize<ManagedServiceIdentityType>($"{{{property}}}", jOptions);
+                    type = JsonSerializer.Deserialize<ManagedServiceIdentityType>($"{{{property}}}", jsonContext.ManagedServiceIdentityType);


The deserialize here I believe is an internal static we should have access to.

m-nash · 2025-05-22T17:14:04Z

...nager/Azure.ResourceManager/src/Common/Custom/Models/ManagedServiceIdentity.Serialization.cs

@@ -214,7 +223,7 @@ internal static ManagedServiceIdentity DeserializeManagedServiceIdentity(JsonEle
                    Dictionary<ResourceIdentifier, UserAssignedIdentity> dictionary = new Dictionary<ResourceIdentifier, UserAssignedIdentity>();
                    foreach (var property0 in property.Value.EnumerateObject())
                    {
-                        dictionary.Add(new ResourceIdentifier(property0.Name), JsonSerializer.Deserialize<UserAssignedIdentity>(property0.Value.GetRawText()));
+                        dictionary.Add(new ResourceIdentifier(property0.Name), JsonSerializer.Deserialize<UserAssignedIdentity>(property0.Value.GetRawText(), ManagedServiceIdentityJsonContext.Default.UserAssignedIdentity));


The deserialize here I believe is an internal static we should have access to.

agocke · 2025-05-23T16:21:42Z

...nager/Azure.ResourceManager/src/Common/Custom/Models/ManagedServiceIdentity.Serialization.cs

@@ -27,7 +28,7 @@ internal void Write(Utf8JsonWriter writer, ModelReaderWriterOptions options, Jso
            }

            writer.WriteStartObject();
-            JsonSerializer.Serialize(writer, ManagedServiceIdentityType, jOptions);


@m-nash the problem here is that ManagedServiceIdentityType doesn’t implement IJsonModel. What’s the appropriate fix? Implement manually?

ManagedServiceIdentityType doesn't need to because its an extensible enum. ToString() is the "serialization" and ctor(string) is the "deserialization".

agocke added 2 commits May 16, 2025 13:38

Start fixing warnings

a0a9ec3

Finish annotation

7c7c606

Copilot AI review requested due to automatic review settings May 16, 2025 21:42

agocke requested review from ArcturusZhang and ArthurMa1978 as code owners May 16, 2025 21:42

github-actions bot added Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. Mgmt This issue is related to a management-plane library. labels May 16, 2025

Copilot AI reviewed May 16, 2025

View reviewed changes

sdk/resourcemanager/Azure.ResourceManager/src/ArmClientOptions.cs Show resolved Hide resolved

jsquire requested review from m-nash and m-redding May 16, 2025 23:31

jsquire approved these changes May 16, 2025

View reviewed changes

agocke added 5 commits May 16, 2025 22:32

Pass through serializer options

435777b

Fix jsonserializeroptions read-only

2cdd06d

Add missing options context

515e5e2

Typo

1808670

Update API to include new attribute

649547d

m-redding approved these changes May 19, 2025

View reviewed changes

ArthurMa1978 approved these changes May 22, 2025

View reviewed changes

m-nash reviewed May 22, 2025

View reviewed changes

Partially implement some of the changes suggested

671f2cc

agocke commented May 23, 2025

View reviewed changes

Switch over some callsites

2011577

Make Azure.ResourceManager AOT-compatible #50129

Are you sure you want to change the base?

Make Azure.ResourceManager AOT-compatible #50129

Uh oh!

Conversation

agocke commented May 16, 2025

Uh oh!

github-actions bot commented May 16, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

jsquire commented May 16, 2025

Uh oh!

agocke commented May 16, 2025

Uh oh!

jsquire left a comment

Choose a reason for hiding this comment

Uh oh!

jsquire commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agocke commented May 19, 2025

Uh oh!

m-redding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsquire commented May 21, 2025

Uh oh!

m-nash commented May 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jsquire commented May 16, 2025 •

edited

Loading