Skip to content

Commit 6ad4b00

Browse files
CopilotJoshLove-msft
Copilot
and
JoshLove-msft
authored
Replace explicit operator modification with FromLroResponse method in LroVisitor (#53686)
* Initial plan * Initial analysis of LroVisitor issue Co-authored-by: JoshLove-msft <[email protected]> * Add FromLroResponse method and remove explicit operator for LRO-only models - Added internal static FromLroResponse method to model serialization providers for LRO operations with result paths - Updated convenience methods to call FromLroResponse instead of using explicit operator cast - Removed explicit operator from models that are only used in LRO contexts (checked across all clients in OutputLibrary) - Updated test to verify FromLroResponse method creation and explicit operator removal Co-authored-by: JoshLove-msft <[email protected]> * Address code review feedback - Extract ModelSerializationExtensionsDefinition to avoid repeated instantiation - Use Except() for better performance when removing explicit operator - Improve null handling in IsModelOnlyUsedInLro to avoid null-forgiving operator Co-authored-by: JoshLove-msft <[email protected]> * Update eng/packages/http-client-csharp/generator/Azure.Generator/src/Visitors/LroVisitor.cs * Match signature and return type when checking for existing FromLroResponse method - Check method modifiers (Internal | Static) - Check return type matches model type - Check parameters count and type (single Response parameter) - This prevents accidentally matching a different method with the same name Co-authored-by: JoshLove-msft <[email protected]> * Address code review feedback - Define FromLroResponse method name as a constant - Use interpolated string instead of FormattableStringFactory.Create - Rename test to AddsFromLroResponseMethodAndRemovesExplicitOperatorForLroOnlyModel - Add test for retaining explicit operator when model used in non-LRO context - Add test for not adding FromLroResponse when no result path - Revert unintended package-lock.json change Co-authored-by: JoshLove-msft <[email protected]> * fix tests * Simplify IsModelOnlyUsedInLro method using LINQ - Use Any() instead of nested foreach loops - Combine all conditions in a single LINQ expression - Use 'is not' pattern for type checking - Remove null-forgiving operator with explicit null check Co-authored-by: JoshLove-msft <[email protected]> * Use KnownAzureParameters.Response for response parameter - Replace custom ParameterProvider instantiation with KnownAzureParameters.Response - Add using statement for Azure.Generator.Primitives Co-authored-by: JoshLove-msft <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: JoshLove-msft <[email protected]> Co-authored-by: jolov <[email protected]>
1 parent bb64695 commit 6ad4b00

File tree

5 files changed

+236
-43
lines changed

5 files changed

+236
-43
lines changed

‎eng/packages/http-client-csharp/generator/Azure.Generator/src/Visitors/LroVisitor.cs‎

Lines changed: 110 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,11 @@
99
using System.Threading.Tasks;
1010
using Azure.Core;
1111
using Azure.Generator.Extensions;
12+
using Azure.Generator.Primitives;
1213
using Azure.Generator.Providers;
1314
using Microsoft.TypeSpec.Generator.ClientModel;
1415
using Microsoft.TypeSpec.Generator.ClientModel.Providers;
16+
using Microsoft.TypeSpec.Generator.ClientModel.Snippets;
1517
using Microsoft.TypeSpec.Generator.Expressions;
1618
using Microsoft.TypeSpec.Generator.Input;
1719
using Microsoft.TypeSpec.Generator.Primitives;
@@ -24,64 +26,126 @@ namespace Azure.Generator.Visitors
2426
{
2527
internal class LroVisitor : ScmLibraryVisitor
2628
{
29+
private const string FromLroResponseMethodName = "FromLroResponse";
30+
2731
protected override ScmMethodProviderCollection? Visit(
2832
InputServiceMethod serviceMethod,
2933
ClientProvider client,
3034
ScmMethodProviderCollection? methods)
3135
{
3236
if (serviceMethod is InputLongRunningServiceMethod { Response.Type: InputModelType responseModel } lroServiceMethod)
3337
{
34-
UpdateExplicitOperatorMethod(responseModel, lroServiceMethod);
38+
AddFromLroResponseMethod(responseModel, lroServiceMethod, client);
3539
}
3640

3741
return methods;
3842
}
3943

40-
private static void UpdateExplicitOperatorMethod(
44+
private static void AddFromLroResponseMethod(
4145
InputModelType responseModel,
42-
InputLongRunningServiceMethod lroServiceMethod)
46+
InputLongRunningServiceMethod lroServiceMethod,
47+
ClientProvider client)
4348
{
4449
var model = AzureClientGenerator.Instance.TypeFactory.CreateModel(responseModel);
4550
if (model == null)
4651
{
4752
return;
4853
}
4954

50-
// Update the explicit cast from response in LRO models to use the result path
51-
var explicitOperator = model.SerializationProviders[0].Methods
52-
.FirstOrDefault(m => m.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Explicit) &&
53-
m.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Operator));
54-
5555
var resultSegment = lroServiceMethod.LongRunningServiceMetadata.ResultPath;
56-
if (explicitOperator == null || string.IsNullOrEmpty(resultSegment))
56+
if (string.IsNullOrEmpty(resultSegment))
5757
{
5858
return;
5959
}
6060

61-
foreach (var statement in explicitOperator.BodyStatements!)
61+
var serializationProvider = model.SerializationProviders[0];
62+
63+
// Check if FromLroResponse method already exists with matching signature
64+
var existingMethod = serializationProvider.Methods
65+
.FirstOrDefault(m => m.Signature.Name == FromLroResponseMethodName &&
66+
m.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Internal) &&
67+
m.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Static) &&
68+
m.Signature.ReturnType?.Equals(model.Type) == true &&
69+
m.Signature.Parameters.Count == 1 &&
70+
m.Signature.Parameters[0].Type.Equals(typeof(Response)));
71+
if (existingMethod != null)
6272
{
63-
if (statement is ExpressionStatement
64-
{
65-
Expression: KeywordExpression
66-
{
67-
Keyword: "return", Expression: InvokeMethodExpression invokeMethodExpression
68-
}
69-
})
73+
return;
74+
}
75+
76+
// Create the FromLroResponse method
77+
var responseParameter = KnownAzureParameters.Response;
78+
var methodSignature = new MethodSignature(
79+
FromLroResponseMethodName,
80+
$"Converts a response to a {model.Type.Name} using the LRO result path.",
81+
MethodSignatureModifiers.Internal | MethodSignatureModifiers.Static,
82+
model.Type,
83+
null,
84+
[responseParameter]);
85+
86+
// Build method body similar to explicit operator but with result path
87+
var modelSerializationExtensions = Static(new ModelSerializationExtensionsDefinition().Type);
88+
var statements = new MethodBodyStatement[]
89+
{
90+
UsingDeclare("document", typeof(JsonDocument),
91+
Static<JsonDocument>().Invoke(nameof(JsonDocument.Parse),
92+
[responseParameter.Property("Content"),
93+
modelSerializationExtensions.Property("JsonDocumentOptions")]),
94+
out var documentVariable),
95+
Return(Static(model.Type).Invoke(
96+
$"Deserialize{model.Type.Name}",
97+
[
98+
documentVariable.Property("RootElement").Invoke("GetProperty", Literal(resultSegment)),
99+
modelSerializationExtensions.Property("WireOptions")
100+
]))
101+
};
102+
103+
var fromLroResponseMethod = new MethodProvider(methodSignature, statements, serializationProvider);
104+
105+
// Add the method to the serialization provider
106+
serializationProvider.Update(methods: [..serializationProvider.Methods, fromLroResponseMethod]);
107+
108+
// Check if we should remove the explicit operator
109+
// Only remove it if the model is ONLY used in LRO contexts across all clients
110+
bool isOnlyUsedInLro = IsModelOnlyUsedInLro(responseModel);
111+
if (isOnlyUsedInLro)
112+
{
113+
var explicitOperator = serializationProvider.Methods
114+
.FirstOrDefault(m => m.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Explicit) &&
115+
m.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Operator));
116+
117+
if (explicitOperator != null)
70118
{
71-
if (invokeMethodExpression.Arguments.Count > 0 && invokeMethodExpression.Arguments[0] is ScopedApi<JsonElement>)
72-
{
73-
invokeMethodExpression.Update(
74-
arguments:
75-
[
76-
invokeMethodExpression.Arguments[0]
77-
.Invoke("GetProperty", Literal(resultSegment)),
78-
..invokeMethodExpression.Arguments.Skip(1)
79-
]);
80-
}
119+
var updatedMethods = serializationProvider.Methods.Except([explicitOperator]).ToList();
120+
serializationProvider.Update(methods: updatedMethods);
81121
}
82122
}
83123
}
84124

125+
private static bool IsModelOnlyUsedInLro(InputModelType responseModel)
126+
{
127+
// Check all clients in the output library to see if any non-LRO method returns this model type
128+
var outputLibrary = AzureClientGenerator.Instance.OutputLibrary;
129+
var allClients = outputLibrary.TypeProviders.OfType<ClientProvider>();
130+
131+
foreach (var client in allClients)
132+
{
133+
// Check if any non-LRO method in this client uses the response model
134+
bool usedInNonLro = client.Methods
135+
.OfType<ScmMethodProvider>()
136+
.Any(m => m.ServiceMethod != null &&
137+
m.ServiceMethod is not InputLongRunningServiceMethod &&
138+
m.ServiceMethod.Response?.Type == responseModel);
139+
140+
if (usedInNonLro)
141+
{
142+
return false; // Model is used in non-LRO context, keep the explicit operator
143+
}
144+
}
145+
146+
return true; // Model is only used in LRO contexts
147+
}
148+
85149
protected override ScmMethodProvider? VisitMethod(ScmMethodProvider method)
86150
{
87151
if (method.IsLroMethod())
@@ -171,13 +235,31 @@ private static void UpdateMethodSignature(ScmMethodProvider method)
171235
var client = (ClientProvider)scmMethod.EnclosingType;
172236
var diagnosticsProperty = client.GetClientDiagnosticProperty();
173237
var scopeName = scmMethod.GetScopeName();
238+
239+
// Use FromLroResponse static method instead of explicit operator cast
240+
var responseModel = serviceMethod.Response.Type as InputModelType;
241+
var lroServiceMethod = scmMethod.ServiceMethod as InputLongRunningServiceMethod;
242+
var resultSegment = lroServiceMethod?.LongRunningServiceMetadata.ResultPath;
243+
244+
ValueExpression conversionExpression;
245+
if (!string.IsNullOrEmpty(resultSegment) && responseModel != null)
246+
{
247+
// Call the FromLroResponse static method
248+
conversionExpression = Static(responseType).Invoke(FromLroResponseMethodName, [response]);
249+
}
250+
else
251+
{
252+
// Fall back to explicit operator cast for models without result path
253+
conversionExpression = new CastExpression(response, responseType);
254+
}
255+
174256
invokeMethodExpression.Update(
175257
instanceReference: Static(typeof(ProtocolOperationHelpers)),
176258
methodName: "Convert",
177259
arguments:
178260
[
179261
(invokeMethodExpression.Arguments[0] as CastExpression)!.Inner,
180-
new FuncExpression([response.Declaration], new CastExpression(response, responseType)),
262+
new FuncExpression([response.Declaration], conversionExpression),
181263
diagnosticsProperty,
182264
Literal(scopeName),
183265
]);

‎eng/packages/http-client-csharp/generator/Azure.Generator/test/Visitors/LroVisitorTests.cs‎

Lines changed: 126 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ public void UpdatesLroSignatureWithResponseBody()
120120
}
121121

122122
[Test]
123-
public void UpdatesExplicitOperatorToUseResultSegment()
123+
public void AddsFromLroResponseMethodAndRemovesExplicitOperatorForLroOnlyModel()
124124
{
125125
var visitor = new TestLroVisitor();
126126
List<InputMethodParameter> parameters =
@@ -154,23 +154,140 @@ public void UpdatesExplicitOperatorToUseResultSegment()
154154
visitor.InvokeVisitServiceMethod(lroServiceMethod, clientProvider!, methodCollection);
155155

156156
var serializationProvider = responseModelProvider!.SerializationProviders[0];
157+
158+
// Check that FromLroResponse method was added
159+
var fromLroResponseMethod = serializationProvider.Methods
160+
.FirstOrDefault(m => m.Signature.Name == "FromLroResponse");
161+
162+
Assert.IsNotNull(fromLroResponseMethod);
163+
Assert.IsNotNull(fromLroResponseMethod!.BodyStatements);
164+
var result = fromLroResponseMethod!.BodyStatements!.ToDisplayString();
165+
Assert.AreEqual(Helpers.GetExpectedFromFile(), result);
166+
167+
// Check that explicit operator was removed since model is only used in LRO
157168
var explicitOperator = serializationProvider.Methods
158169
.FirstOrDefault(m => m.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Explicit) &&
159170
m.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Operator));
171+
Assert.IsNull(explicitOperator);
160172

161-
Assert.IsNotNull(explicitOperator);
162-
Assert.IsNotNull(explicitOperator!.BodyStatements);
163-
var result = explicitOperator!.BodyStatements!.ToDisplayString();
173+
// does not add the method again on subsequent calls
174+
visitor.InvokeVisitServiceMethod(lroServiceMethod, clientProvider!, methodCollection);
175+
serializationProvider = responseModelProvider!.SerializationProviders[0];
176+
fromLroResponseMethod = serializationProvider.Methods
177+
.FirstOrDefault(m => m.Signature.Name == "FromLroResponse");
178+
result = fromLroResponseMethod!.BodyStatements!.ToDisplayString();
164179
Assert.AreEqual(Helpers.GetExpectedFromFile(), result);
180+
}
181+
182+
[Test]
183+
public void RetainsExplicitOperatorWhenModelUsedInNonLroContext()
184+
{
185+
var visitor = new TestLroVisitor();
186+
List<InputMethodParameter> parameters =
187+
[
188+
InputFactory.MethodParameter("p1", InputPrimitiveType.String)
189+
];
190+
var responseModel = InputFactory.Model("foo");
191+
192+
// Create an LRO method
193+
var lro = InputFactory.Operation(
194+
"lroOp",
195+
parameters: parameters,
196+
responses: [InputFactory.OperationResponse(bodytype: responseModel)]);
197+
var lroServiceMethod = InputFactory.LongRunningServiceMethod(
198+
"lroOp",
199+
lro,
200+
parameters: parameters,
201+
response: InputFactory.ServiceMethodResponse(responseModel, ["result"]),
202+
longRunningServiceMetadata: InputFactory.LongRunningServiceMetadata(
203+
finalState: 1,
204+
finalResponse: InputFactory.OperationResponse(),
205+
resultPath: "someResultPath"));
165206

166-
// does not mutate an already mutated operator
207+
// Create a non-LRO method that also returns the same model
208+
var nonLroOp = InputFactory.Operation(
209+
"nonLroOp",
210+
parameters: parameters,
211+
responses: [InputFactory.OperationResponse(bodytype: responseModel)]);
212+
var nonLroServiceMethod = InputFactory.BasicServiceMethod(
213+
"nonLroOp",
214+
nonLroOp,
215+
parameters: parameters,
216+
response: InputFactory.ServiceMethodResponse(responseModel, ["result"]));
217+
218+
var inputClient = InputFactory.Client("TestClient", methods: [lroServiceMethod, nonLroServiceMethod]);
219+
MockHelpers.LoadMockGenerator(clients: () => [inputClient]);
220+
221+
var clientProvider = AzureClientGenerator.Instance.TypeFactory.CreateClient(inputClient);
222+
Assert.IsNotNull(clientProvider);
223+
224+
var responseModelProvider = AzureClientGenerator.Instance.TypeFactory.CreateModel(responseModel);
225+
Assert.IsNotNull(responseModelProvider);
226+
227+
var methodCollection = new ScmMethodProviderCollection(lroServiceMethod, clientProvider!);
167228
visitor.InvokeVisitServiceMethod(lroServiceMethod, clientProvider!, methodCollection);
168-
serializationProvider = responseModelProvider!.SerializationProviders[0];
169-
explicitOperator = serializationProvider.Methods
229+
230+
var serializationProvider = responseModelProvider!.SerializationProviders[0];
231+
232+
// Check that FromLroResponse method was added
233+
var fromLroResponseMethod = serializationProvider.Methods
234+
.FirstOrDefault(m => m.Signature.Name == "FromLroResponse");
235+
Assert.IsNotNull(fromLroResponseMethod);
236+
237+
// Check that explicit operator was RETAINED since model is also used in non-LRO context
238+
var explicitOperator = serializationProvider.Methods
170239
.FirstOrDefault(m => m.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Explicit) &&
171240
m.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Operator));
172-
result = explicitOperator!.BodyStatements!.ToDisplayString();
173-
Assert.AreEqual(Helpers.GetExpectedFromFile(), result);
241+
Assert.IsNotNull(explicitOperator);
242+
}
243+
244+
[Test]
245+
public void DoesNotAddFromLroResponseMethodWhenNoResultPath()
246+
{
247+
var visitor = new TestLroVisitor();
248+
List<InputMethodParameter> parameters =
249+
[
250+
InputFactory.MethodParameter("p1", InputPrimitiveType.String)
251+
];
252+
var responseModel = InputFactory.Model("foo");
253+
var lro = InputFactory.Operation(
254+
"foo",
255+
parameters: parameters,
256+
responses: [InputFactory.OperationResponse(bodytype: responseModel)]);
257+
// LRO service method without result path
258+
var lroServiceMethod = InputFactory.LongRunningServiceMethod(
259+
"foo",
260+
lro,
261+
parameters: parameters,
262+
response: InputFactory.ServiceMethodResponse(responseModel, ["result"]),
263+
longRunningServiceMetadata: InputFactory.LongRunningServiceMetadata(
264+
finalState: 1,
265+
finalResponse: InputFactory.OperationResponse(),
266+
resultPath: null)); // No result path
267+
var inputClient = InputFactory.Client("TestClient", methods: [lroServiceMethod]);
268+
MockHelpers.LoadMockGenerator(clients: () => [inputClient]);
269+
270+
var clientProvider = AzureClientGenerator.Instance.TypeFactory.CreateClient(inputClient);
271+
Assert.IsNotNull(clientProvider);
272+
273+
var responseModelProvider = AzureClientGenerator.Instance.TypeFactory.CreateModel(responseModel);
274+
Assert.IsNotNull(responseModelProvider);
275+
276+
var methodCollection = new ScmMethodProviderCollection(lroServiceMethod, clientProvider!);
277+
visitor.InvokeVisitServiceMethod(lroServiceMethod, clientProvider!, methodCollection);
278+
279+
var serializationProvider = responseModelProvider!.SerializationProviders[0];
280+
281+
// Check that FromLroResponse method was NOT added
282+
var fromLroResponseMethod = serializationProvider.Methods
283+
.FirstOrDefault(m => m.Signature.Name == "FromLroResponse");
284+
Assert.IsNull(fromLroResponseMethod);
285+
286+
// Explicit operator should still be present
287+
var explicitOperator = serializationProvider.Methods
288+
.FirstOrDefault(m => m.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Explicit) &&
289+
m.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Operator));
290+
Assert.IsNotNull(explicitOperator);
174291
}
175292

176293
[Test]

eng/packages/http-client-csharp/generator/Azure.Generator/test/Visitors/TestData/LroVisitorTests/UpdatesExplicitOperatorToUseResultSegment.cs renamed to eng/packages/http-client-csharp/generator/Azure.Generator/test/Visitors/TestData/LroVisitorTests/AddsFromLroResponseMethodAndRemovesExplicitOperatorForLroOnlyModel.cs

File renamed without changes.

‎eng/packages/http-client-csharp/generator/TestProjects/Spector/http/azure/core/lro/rpc/src/Generated/GenerationResult.Serialization.cs‎

Lines changed: 0 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎eng/packages/http-client-csharp/generator/TestProjects/Spector/http/azure/core/lro/standard/src/Generated/ExportedUser.Serialization.cs‎

Lines changed: 0 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)