Implement mgmt resource #47944

live1206 · 2025-01-23T00:10:43Z

Resolves Implement ResourceProvdier #47744
Resolves Generate OperationSource in Azure plugin #48135
Adjust to make MPG have similar file structure as DPG.
- Put CreateRequest methods from RestClientProvider to ClientProvider
- Implement client methods in Resource directly
- Omit RestClientProvider
Add test to verify the generation
Generated MgmtTypeSpec test can build now
Leverage existing resource related decorators directly from TypeSpec

eng/packages/http-client-csharp/generator/Azure.Generator/test/TestHelpers/Configuration.json

…Mgmt/Models/RequestPath.cs Co-authored-by: Copilot <[email protected]>

m-nash · 2025-02-24T17:52:54Z

eng/packages/http-client-csharp/generator/Azure.Generator/src/Providers/ResourceProvider.cs

+
+namespace Azure.Generator.Providers
+{
+    internal class ResourceProvider : TypeProvider


Lets call this ResourceClientProvider and see if we can inherit from ClientProvider.

As offline discussed, we would like to reorg the rest clients, operation clients and resource clients. So that, the structure of mgmt resource clients are similar to the DPG clients.

m-nash · 2025-02-24T17:54:18Z

...tp-client-csharp/generator/Azure.Generator/src/Providers/MgmtLongRunningOperationProvider.cs

@@ -174,7 +174,7 @@ private ConstructorProvider BuildInitializationConstructor()
                    skipApiVersionOverrideParameter,
                    apiVersionOverrideValueParameter,
                };
-            var signature = new ConstructorSignature(Type, $"", MethodSignatureModifiers.Internal, parameters, null);
+            var signature = new ConstructorSignature(Type, $"", MethodSignatureModifiers.Internal, parameters);


Is there any reason this class is prefixed with Mgmt? I think Dpg and Mpg LROs work the same other than the type that gets used ArmOperation vs Operation?

For now, it only contains logic for Mgmt, rename it to LongRunningOperationProvider.

eng/packages/http-client-csharp/eng/scripts/Generate.ps1

m-nash · 2025-02-24T19:47:34Z

eng/packages/http-client-csharp/generator/Azure.Generator/src/AzureOutputLibrary.cs

-        private Dictionary<string, OperationSet> _pathToOperationSetMap;
-        private Dictionary<string, HashSet<OperationSet>> _resourceDataBySpecNameMap;
+        //TODO: Move these to InputLibrary instead
+        private Dictionary<RequestPath, OperationSet> _pathToOperationSetMap;


There is a lot of code in here that is mgmt specific and it starts to get difficult to tell which is which have we considered a mgmt plugin?

Most likely.
Given I am fully working with MPG now, I think it would be better to split them when we have more implementation on DPG, then we can clarify the things in common for both.

#48481 to track the split, it will happen soon after this PR.

m-nash · 2025-02-24T19:49:22Z

eng/packages/http-client-csharp/generator/Azure.Generator/src/AzureClientPlugin.cs

@@ -30,6 +31,9 @@ public class AzureClientPlugin : ScmCodeModelPlugin
    public override AzureOutputLibrary OutputLibrary => _azureOutputLibrary ??= new();

    internal ResourceDetection ResourceDetection { get; } = new();
+    internal ParentDetection ParentDetection { get; } = new();


Isn't your parent just a property on the client from typespec?

For TypeSpec, we should be able to get the resource hierarchy directly via #48281.
The resource detection logic is mainly for swagger input.
For now, given getArmResources API is not ready for integration. We use it as a temporary workaround.

Eventually, when we have the native way ready, we should get rid of them. And the resource detection should be part of the M4 output conversion to tspCodeModel.json in autorest.csharp.
Added TODO to remove them.

As offline discussed, we need to clarify the contract of input types containing the resource hierarchy. And it should be an extensible way of holding it in Azure plugin only.
Then, we can put the temp workaround of resource detection close to the input instead of output.

Isolated Resource related logic in ResourceBuilder, so we have separated the temp workaround to a central place.

I think we will need to have resource related logic in MPG emitter, such as call getArmResources API to get the resource hierarchy. MGC doesn't need to provide any extension for this. MPG can have additional input models to hold them.

If this is only needed for swagger input, have we considered having this in autorest.csharp only?

We can make it part of the codemodel -> input type conversion before we serialize out to the new plugin. That way the new plugin only has 1 way to do things which is to read the information directly from the codemodel.json file and it doesn't care how the data got there.

...ckages/http-client-csharp/generator/Azure.Generator/src/Providers/OperationSourceProvider.cs

eng/packages/http-client-csharp/generator/Azure.Generator/src/Providers/ResourceProvider.cs

m-nash · 2025-02-24T19:56:05Z

...s/http-client-csharp/generator/TestProjects/Local/Mgmt-TypeSpec/src/Generated/FooResource.cs

+            TryGetApiVersion(ResourceType, out string fooApiVersion);
+            _fooRestClient = new Foos(Pipeline, Endpoint, fooApiVersion);
+#if DEBUG
+            global::MgmtTypeSpec.FooResource.ValidateResourceId(id);


Is this not getting reduced because its in an if debug?

oh, you were talking about the namespace not reduced.
Yes. I tried to remove the debug block, the namespace was simplified.

raise issue in MGC to fix this
we can replace it with Debug.Assert here

Debug.Assert can't be used here, it needs a bool condition input, but this method is void.
Use [Conditional("DEBUG")] in the method signature instead.

microsoft/typespec#6160 to track the issue in MGC

m-nash · 2025-02-24T19:57:22Z

...Generator/test/Providers/TestData/ResourceProviderTests/Verify_ResourceProviderGeneration.cs

+namespace Samples
+{
+    /// <summary></summary>
+    public partial class ResponseTypeResource : global::Azure.ResourceManager.ArmResource


Lets trim this down to at most 1 method validation at a time. Otherwise this test will be susceptible to any change in generation of any portion of a resource

Is there a way to write a method of TypeProvider?
As I can see, there is no public API from TypeProviderWriter. Do we want to expose such API?

Separated the tests for ResourceClientProvider.
And after exposing AzureOutputLibrary.CreateResourceCore, I realized that we need to simplify the creation of ResourceClientProvider. And it ties to the contract of Input types from MGC.

Isolated Resource related logic to ResourceBuilder.

...ackages/http-client-csharp/generator/Azure.Generator/test/Providers/ResourceProviderTests.cs

eng/Packages.Data.props

ArcturusZhang · 2025-03-03T07:10:29Z

eng/packages/http-client-csharp/generator/Azure.Generator/src/ResourceBuilder.cs

+        public IReadOnlyList<(InputClient ResourceClient, string RequestPath, bool IsSingleton, string RequestType, string SpecName)> BuildResourceClients()
+        {
+            var result = new List<(InputClient ResourceClient, string RequestPath, bool IsSingleton, string RequestType, string SpecName)>();
+            var singletonDetection = new SingletonDetection();


I think this should be a readonly field.

The only usage I have now is this place, so that I make it a local variable for now.
We can make it a field if we have other usages later.

ArcturusZhang · 2025-03-03T07:11:34Z

eng/packages/http-client-csharp/generator/Azure.Generator/src/ResourceBuilder.cs

+
+        private Dictionary<string, (RequestPath ResourcePath, InputClient Client)> EnsureOperationsetMap()
+        {
+            var pathToOperationSetMap = CategorizeClients();


ok - looks like now we never generate operations by operation groups even in "rest operations" class, I like this a lot.

well - no - we still may have the issue that our rest operations are still organized by input clients instead of resources.
But in the future if we could get the extra information about resources from typespec side, this should be resolved automatically therefore this is fine now.

Let's say we will have resource across inputClients, we can change the output to Dictionary<string, (RequestPath ResourcePath, IReadOnlyList<InputClient> Clients).
The point is to leverage the existing clients without constructing extra objects.

ArcturusZhang · 2025-03-03T07:20:05Z

...enerator/TestProjects/Local/Mgmt-TypeSpec/src/Generated/RestOperations/FoosRestOperations.cs

-
-            Response result = await ListAsync(subscriptionId, resourceGroupName, cancellationToken.CanBeCanceled ? new RequestContext { CancellationToken = cancellationToken } : null).ConfigureAwait(false);
-            return Response.FromValue((FooListResult)result, result);
+        internal HttpMessage CreateCreateOrUpdateRequest(Guid subscriptionId, string resourceGroupName, string fooName, RequestContent content, RequestContext context)


so we made a step ahead that in the rest client, we no longer keep the protocol methods and convenience methods, and the resource class would just call the create request methods here.

Do we consider making another step ahead that we could remove this class, and move all these create request methods into the resource class, and put them in another partial class like FooResource.RestClient.cs?
This could save us an object in resource class - now it has to construct this class to create the request.

Currently, I have removed Foos.RestClient.cs and put its CreateRequest methods to FoosRestOperations.cs. I am fine with renaming it to Foos.RestClient.cs. The thing is, we only keep the internal CreateRequest methods.

m-nash · 2025-03-05T23:14:30Z

eng/packages/http-client-csharp/generator/Azure.Generator/src/AzureOutputLibrary.cs

+        /// <summary>
+        /// Create a resource client provider
+        /// </summary>
+        /// <param name="inputClient"></param>


Fill in comments with something meaningful

m-nash · 2025-03-05T23:15:26Z

eng/packages/http-client-csharp/generator/Azure.Generator/src/AzureOutputLibrary.cs

+        /// <param name="resourceType"></param>
+        /// <param name="isSingleton"></param>
+        /// <returns></returns>
+        public virtual TypeProvider CreateResourceCore(InputClient inputClient, string requestPath, string schemaName, ModelProvider resourceData, string resourceType, bool isSingleton)


since this is returning a resource client should it be named CreateResourceClientCore?

Does it need to be public or can it be protected?

m-nash · 2025-03-05T23:17:34Z

...ackages/http-client-csharp/generator/Azure.Generator/src/Providers/ResourceClientProvider.cs

+        private FieldProvider _restClientField;
+        private FieldProvider _resourcetypeField;
+
+        public ResourceClientProvider(InputClient inputClient, string requestPath, string specName, ModelProvider resourceData, string resrouceType, bool isSingleton)


Isn't specName part of inputClient? the name from the spec?

I think requestPath and resourceType are redundant or perhaps resourceType is redundant with the resourceData?

m-nash · 2025-03-05T23:19:48Z

eng/packages/http-client-csharp/generator/Azure.Generator/src/ResourceBuilder.cs

+
+namespace Azure.Generator
+{
+    internal class ResourceBuilder


What is a resource builder and how does it fit into the new design of Providers?

m-nash · 2025-03-05T23:21:36Z

eng/packages/http-client-csharp/generator/Azure.Generator/src/ResourceVisitor.cs

@@ -27,7 +27,7 @@ internal class ResourceVisitor : ScmLibraryVisitor

        private static void TransformResource(TypeProvider type)
        {
-            if (type is ModelProvider && AzureClientPlugin.Instance.OutputLibrary.IsResource(type.Name))
+            if (type is ModelProvider && AzureClientPlugin.Instance.ResourceBuilder.IsResource(type.Name))


can't we type check here if its a ResourceDataProvider which inherits from ModelProvider?

m-nash · 2025-03-05T23:23:34Z

eng/packages/http-client-csharp/generator/Azure.Generator/src/Utilities/SingletonDetection.cs

+
+namespace Azure.Generator.Utilities
+{
+    internal class SingletonDetection


I am not sure these Detection classes fit into the new design. It feels like we are pulling in autorest.csharp concepts.

m-nash · 2025-03-05T23:24:50Z

...or/test/Providers/TestData/OperationSourceProviderTests/Verify_ResourceProviderGeneration.cs

+            _client = client;
+        }
+
+        global::Samples.ResponseTypeResource global::Azure.Core.IOperationSource<global::Samples.ResponseTypeResource>.CreateResult(global::Azure.Response response, global::System.Threading.CancellationToken cancellationToken)


I recommend trimming down these test files to single method only to minimize test churn when we tweak internal implementations.

live1206 added 5 commits January 16, 2025 11:02

wip

f041f24

wip

badf1a5

wip

69fa28a

Merge remote-tracking branch 'origin/main' into implement-mgmt-resource

615fec1

wip

d136be2

github-actions bot added Client This issue points to a problem in the data-plane of the library. CodeGen Issues that relate to code generation labels Jan 23, 2025

live1206 added 7 commits January 26, 2025 09:21

wip

a040627

Merge remote-tracking branch 'origin/main' into implement-mgmt-resource

0296a71

generate operation methods for Resource

fda752a

add package back

580508a

Merge remote-tracking branch 'origin/main' into implement-mgmt-resource

ba12225

update to latest MGC

ef83882

fix test

120e164

live1206 mentioned this pull request Feb 6, 2025

Expose RestClient from ClientProvider microsoft/typespec#5888

Merged

live1206 added 3 commits February 7, 2025 13:41

Merge branch 'main' into implement-mgmt-resource

e4bbde0

Add test for ResourceProvider

b2842c4

Merge remote-tracking branch 'origin/main' into implement-mgmt-resource

5f5cee9

live1206 commented Feb 8, 2025

View reviewed changes

eng/packages/http-client-csharp/generator/Azure.Generator/test/TestHelpers/Configuration.json Outdated Show resolved Hide resolved

Add RequestPath

41cf53c

live1206 marked this pull request as ready for review February 8, 2025 07:42

live1206 requested review from JoshLove-msft, m-nash, jorgerangel-msft, jsquire, annelo-msft, christothes, KrzysztofCwalina, tg-msft and ArthurMa1978 as code owners February 8, 2025 07:42

live1206 and others added 8 commits February 19, 2025 09:27

Update eng/packages/http-client-csharp/generator/Azure.Generator/src/…

c5ca437

…Mgmt/Models/RequestPath.cs Co-authored-by: Copilot <[email protected]>

update

3ed339d

Merge branch 'main' into implement-mgmt-resource

d800d35

fix build

8d78f7f

update to use CanonicalView

8fbc8aa

update test

081696d

Use ModelSerializationExtensions.WireOptions instead of new instance

f9e8afd

update test

8d7b831

m-nash reviewed Feb 24, 2025

View reviewed changes

jsquire reviewed Feb 24, 2025

View reviewed changes

eng/Packages.Data.props Outdated Show resolved Hide resolved

live1206 added 13 commits February 25, 2025 11:57

Address comments

4dcccc7

typo

d00ee3a

Merge remote-tracking branch 'origin/main' into implement-mgmt-resource

0086b11

resolve comments

e34db7b

fix build

417af63

update test

965e951

separate tests for ResourceClientProvider

579dd9b

Add TODO

682e704

fix tests

22e08c6

Move resource related logic to ResourceBuilder

541193c

Remove clientProvider and implement client methods in Resource directly

20cdaaf

update

78b5cb5

simplify resource detection

ff44b6b

ArcturusZhang reviewed Mar 3, 2025

View reviewed changes

Merge branch 'main' into implement-mgmt-resource

33c30fa

m-nash reviewed Mar 5, 2025

View reviewed changes

live1206 added 2 commits March 6, 2025 17:39

replace resource detection with decorators

ca43fe6

cleanup

7ff96ba

Implement mgmt resource #47944

Are you sure you want to change the base?

Implement mgmt resource #47944

Conversation

live1206 commented Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

live1206 Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

live1206 Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

live1206 Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

live1206 Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

live1206 commented Jan 23, 2025 •

edited

Loading

live1206 Feb 25, 2025 •

edited

Loading

live1206 Feb 25, 2025 •

edited

Loading

live1206 Feb 25, 2025 •

edited

Loading

live1206 Feb 26, 2025 •

edited

Loading