Skip to content

Commit ab5caa4

Browse files
[http-client-csharp] Place contentType after body per .NET SDK parameter ordering guideline (#10403)
Operations with a `contentType` header placed it before body, violating the <a href="https://azure.github.io/azure-sdk/dotnet_implementation.html#parameter-presence-and-ordering">.NET SDK parameter ordering guideline</a>. This applies to all operations (both multipart and non-multipart). **Before:** ```csharp UpdateSkillDefaultVersion(string skillId, string contentType, BinaryContent content, RequestOptions options = null) ``` **After:** ```csharp UpdateSkillDefaultVersion(string skillId, BinaryContent content, string contentType, RequestOptions options = null) ``` ### Changes - **`RestClientProvider.GetMethodParameters`**: Added a `contentType` sorting bucket (350) between body (200/300) and optional query/header (400). Content-type headers detected via `InputHeaderParameter.IsContentType` are routed to this bucket instead of the regular required/optional header buckets. Updated the `IsMultipartFormData` content-type insertion to also use the `contentType` bucket for consistency. - **Back-compat via `BackCompatProvider`**: Added `HasContentTypeBeforeBodyInLastContract` helper that checks `client.BackCompatProvider.LastContractView` for a matching method where `contentType` appears before `content` (body). If the previous contract had that ordering, the content-type parameter falls through to the regular header bucket to preserve backward compatibility. - **Tests**: Added parameterized `TestGetMethodParameters_ContentTypeAfterBody(bool isRequired, bool isExtensibleEnum)` using `[TestCase]` to cover all four combinations of required/optional and string/extensible-enum content-type headers. Added `ContentTypeOrderPreservedFromLastContractView` validating that the previous ordering is preserved when the last contract had contentType before body, and `ContentTypeAfterBodyInLastContractView` validating correct ordering when the last contract already had contentType after body. - **Documentation**: Updated `backward-compatibility.md` with a new "Content-Type Parameter Ordering" section documenting the back-compat preservation case. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
1 parent c6bc569 commit ab5caa4

5 files changed

Lines changed: 269 additions & 2 deletions

File tree

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/RestClientProvider.cs

Lines changed: 64 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1007,6 +1007,7 @@ internal static List<ParameterProvider> GetMethodParameters(
10071007
int required = 100;
10081008
int bodyRequired = 200;
10091009
int bodyOptional = 300;
1010+
int contentType = 350;
10101011
int optional = 400;
10111012

10121013
var operation = serviceMethod.Operation;
@@ -1124,7 +1125,12 @@ internal static List<ParameterProvider> GetMethodParameters(
11241125
break;
11251126
case ParameterLocation.Query:
11261127
case ParameterLocation.Header:
1127-
if (parameter.DefaultValue == null)
1128+
if (inputParam is InputHeaderParameter { IsContentType: true }
1129+
&& !HasContentTypeBeforeBodyInLastContract(serviceMethod.Name, client.BackCompatProvider))
1130+
{
1131+
sortedParams.Add(contentType++, parameter);
1132+
}
1133+
else if (parameter.DefaultValue == null)
11281134
{
11291135
sortedParams.Add(required++, parameter);
11301136
}
@@ -1144,7 +1150,7 @@ internal static List<ParameterProvider> GetMethodParameters(
11441150

11451151
if (operation.IsMultipartFormData)
11461152
{
1147-
sortedParams.Add(bodyRequired++, ScmKnownParameters.ContentType);
1153+
sortedParams.Add(contentType++, ScmKnownParameters.ContentType);
11481154
}
11491155

11501156
if (methodType == ScmMethodKind.CreateRequest)
@@ -1159,6 +1165,62 @@ internal static List<ParameterProvider> GetMethodParameters(
11591165
return [.. sortedParams.Values];
11601166
}
11611167

1168+
/// <summary>
1169+
/// Checks if the last contract view contains a method matching the given name where
1170+
/// a "contentType" parameter appears before the body ("content") parameter.
1171+
/// If so, we should preserve that ordering for backward compatibility.
1172+
/// </summary>
1173+
private static bool HasContentTypeBeforeBodyInLastContract(string methodName, TypeProvider backCompatProvider)
1174+
{
1175+
const string contentTypeParamName = "contentType";
1176+
const string contentParamName = "content";
1177+
1178+
var lastContractMethods = backCompatProvider.LastContractView?.Methods;
1179+
if (lastContractMethods == null || lastContractMethods.Count == 0)
1180+
{
1181+
return false;
1182+
}
1183+
1184+
var syncMethodName = methodName;
1185+
var asyncMethodName = methodName + "Async";
1186+
1187+
foreach (var method in lastContractMethods)
1188+
{
1189+
if (!string.Equals(method.Signature.Name, syncMethodName, StringComparison.OrdinalIgnoreCase)
1190+
&& !string.Equals(method.Signature.Name, asyncMethodName, StringComparison.OrdinalIgnoreCase))
1191+
{
1192+
continue;
1193+
}
1194+
1195+
int contentTypeIndex = -1;
1196+
int bodyIndex = -1;
1197+
for (int i = 0; i < method.Signature.Parameters.Count; i++)
1198+
{
1199+
var param = method.Signature.Parameters[i];
1200+
if (string.Equals(param.Name, contentTypeParamName, StringComparison.OrdinalIgnoreCase))
1201+
{
1202+
contentTypeIndex = i;
1203+
}
1204+
else if (string.Equals(param.Name, contentParamName, StringComparison.OrdinalIgnoreCase))
1205+
{
1206+
bodyIndex = i;
1207+
}
1208+
1209+
if (contentTypeIndex >= 0 && bodyIndex >= 0)
1210+
{
1211+
break;
1212+
}
1213+
}
1214+
1215+
if (contentTypeIndex >= 0 && bodyIndex >= 0 && contentTypeIndex < bodyIndex)
1216+
{
1217+
return true;
1218+
}
1219+
}
1220+
1221+
return false;
1222+
}
1223+
11621224
internal static InputModelType GetSpreadParameterModel(InputParameter inputParam)
11631225
{
11641226
if (inputParam.Type is InputModelType model)

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/RestClientProviders/RestClientProviderTests.cs

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,135 @@ public void TestGetMethodParameters_ProperOrdering()
233233
Assert.AreEqual("b", orderedPathParams[2].Name);
234234
}
235235

236+
[TestCase(true, false)]
237+
[TestCase(false, false)]
238+
[TestCase(true, true)]
239+
[TestCase(false, true)]
240+
public void TestGetMethodParameters_ContentTypeAfterBody(bool isRequired, bool isExtensibleEnum)
241+
{
242+
InputType contentTypeType = isExtensibleEnum
243+
? InputFactory.StringEnum("ContentTypeEnum",
244+
[("application/json", "application/json"), ("application/xml", "application/xml")],
245+
isExtensible: true)
246+
: InputPrimitiveType.String;
247+
var contentTypeHeader = InputFactory.HeaderParameter(
248+
"contentType",
249+
contentTypeType,
250+
isRequired: isRequired,
251+
isContentType: true,
252+
serializedName: "Content-Type",
253+
defaultValue: isRequired ? null : InputFactory.Constant.String("application/json"));
254+
var bodyParam = InputFactory.BodyParameter("body", InputPrimitiveType.String, isRequired: true);
255+
var pathParam = InputFactory.PathParameter("skillId", InputPrimitiveType.String, isRequired: true);
256+
257+
var operation = InputFactory.Operation(
258+
"UpdateSkillDefaultVersion",
259+
parameters: [pathParam, contentTypeHeader, bodyParam]);
260+
261+
var serviceMethod = InputFactory.BasicServiceMethod(
262+
"UpdateSkillDefaultVersion",
263+
operation);
264+
265+
var inputClient = InputFactory.Client("TestClient", methods: [serviceMethod]);
266+
var clientProvider = ScmCodeModelGenerator.Instance.TypeFactory.CreateClient(inputClient);
267+
Assert.IsNotNull(clientProvider);
268+
269+
var methodParameters = RestClientProvider.GetMethodParameters(serviceMethod, ScmMethodKind.Protocol, clientProvider!);
270+
271+
Assert.AreEqual(3, methodParameters.Count);
272+
Assert.AreEqual("skillId", methodParameters[0].Name);
273+
Assert.AreEqual("content", methodParameters[1].Name);
274+
Assert.AreEqual("contentType", methodParameters[2].Name);
275+
}
276+
277+
[Test]
278+
public async Task ContentTypeOrderPreservedFromLastContractView()
279+
{
280+
// Create an operation with a content-type header (union) and body
281+
var contentTypeEnum = InputFactory.StringEnum("ContentTypeEnum",
282+
[("application/json", "application/json"), ("application/xml", "application/xml")],
283+
isExtensible: true);
284+
var contentTypeHeader = InputFactory.HeaderParameter(
285+
"contentType",
286+
contentTypeEnum,
287+
isRequired: true,
288+
isContentType: true,
289+
serializedName: "Content-Type");
290+
var bodyParam = InputFactory.BodyParameter("body", InputPrimitiveType.String, isRequired: true);
291+
var pathParam = InputFactory.PathParameter("skillId", InputPrimitiveType.String, isRequired: true);
292+
293+
var operation = InputFactory.Operation(
294+
"UpdateSkillDefaultVersion",
295+
parameters: [pathParam, contentTypeHeader, bodyParam]);
296+
297+
var serviceMethod = InputFactory.BasicServiceMethod(
298+
"UpdateSkillDefaultVersion",
299+
operation);
300+
301+
var client = InputFactory.Client("TestClient", methods: [serviceMethod]);
302+
303+
// Load with a last contract that has contentType before body
304+
var generator = await MockHelpers.LoadMockGeneratorAsync(
305+
clients: () => [client],
306+
lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync());
307+
308+
var clientProvider = generator.Object.OutputLibrary.TypeProviders.OfType<ClientProvider>().FirstOrDefault();
309+
Assert.IsNotNull(clientProvider);
310+
Assert.IsNotNull(clientProvider!.LastContractView);
311+
312+
var methodParameters = RestClientProvider.GetMethodParameters(serviceMethod, ScmMethodKind.Protocol, clientProvider!);
313+
314+
// When the last contract had contentType before body, the ordering should be preserved
315+
Assert.AreEqual(3, methodParameters.Count);
316+
Assert.AreEqual("skillId", methodParameters[0].Name);
317+
Assert.AreEqual("contentType", methodParameters[1].Name); // contentType stays before body for back-compat
318+
Assert.AreEqual("content", methodParameters[2].Name);
319+
}
320+
321+
[Test]
322+
public async Task ContentTypeAfterBodyInLastContractView()
323+
{
324+
// Create an operation with a content-type header (union) and body
325+
var contentTypeEnum = InputFactory.StringEnum("ContentTypeEnum",
326+
[("application/json", "application/json"), ("application/xml", "application/xml")],
327+
isExtensible: true);
328+
var contentTypeHeader = InputFactory.HeaderParameter(
329+
"contentType",
330+
contentTypeEnum,
331+
isRequired: true,
332+
isContentType: true,
333+
serializedName: "Content-Type");
334+
var bodyParam = InputFactory.BodyParameter("body", InputPrimitiveType.String, isRequired: true);
335+
var pathParam = InputFactory.PathParameter("skillId", InputPrimitiveType.String, isRequired: true);
336+
337+
var operation = InputFactory.Operation(
338+
"UpdateSkillDefaultVersion",
339+
parameters: [pathParam, contentTypeHeader, bodyParam]);
340+
341+
var serviceMethod = InputFactory.BasicServiceMethod(
342+
"UpdateSkillDefaultVersion",
343+
operation);
344+
345+
var client = InputFactory.Client("TestClient", methods: [serviceMethod]);
346+
347+
// Load with a last contract that already has contentType after body
348+
var generator = await MockHelpers.LoadMockGeneratorAsync(
349+
clients: () => [client],
350+
lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync());
351+
352+
var clientProvider = generator.Object.OutputLibrary.TypeProviders.OfType<ClientProvider>().FirstOrDefault();
353+
Assert.IsNotNull(clientProvider);
354+
Assert.IsNotNull(clientProvider!.LastContractView);
355+
356+
var methodParameters = RestClientProvider.GetMethodParameters(serviceMethod, ScmMethodKind.Protocol, clientProvider!);
357+
358+
// When the last contract already had contentType after body, the new ordering is used
359+
Assert.AreEqual(3, methodParameters.Count);
360+
Assert.AreEqual("skillId", methodParameters[0].Name);
361+
Assert.AreEqual("content", methodParameters[1].Name);
362+
Assert.AreEqual("contentType", methodParameters[2].Name); // contentType after body
363+
}
364+
236365
[TestCase(true, true)]
237366
[TestCase(true, false)]
238367
[TestCase(false, true)]
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
#nullable disable
2+
3+
using System.ClientModel;
4+
using System.ClientModel.Primitives;
5+
using System.Threading.Tasks;
6+
7+
namespace Sample
8+
{
9+
public partial class TestClient
10+
{
11+
// This represents a previous contract where contentType already appears after body (content)
12+
public virtual Task<ClientResult> UpdateSkillDefaultVersionAsync(string skillId, BinaryContent content, string contentType, RequestOptions options = null) { return null; }
13+
public virtual ClientResult UpdateSkillDefaultVersion(string skillId, BinaryContent content, string contentType, RequestOptions options = null) { return null; }
14+
}
15+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
#nullable disable
2+
3+
using System.ClientModel;
4+
using System.ClientModel.Primitives;
5+
using System.Threading.Tasks;
6+
7+
namespace Sample
8+
{
9+
public partial class TestClient
10+
{
11+
// This represents a previous contract where contentType appears before body (content)
12+
public virtual Task<ClientResult> UpdateSkillDefaultVersionAsync(string skillId, string contentType, BinaryContent content, RequestOptions options = null) { return null; }
13+
public virtual ClientResult UpdateSkillDefaultVersion(string skillId, string contentType, BinaryContent content, RequestOptions options = null) { return null; }
14+
}
15+
}

packages/http-client-csharp/generator/docs/backward-compatibility.md

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
- [Parameter Naming](#parameter-naming)
1515
- [Page Size Parameter Casing Correction](#scenario-page-size-parameter-casing-correction)
1616
- [Top Parameter Conversion to MaxCount](#scenario-top-parameter-conversion-to-maxcount)
17+
- [Content-Type Parameter Ordering](#content-type-parameter-ordering)
18+
- [Content-Type Before Body Preserved from Last Contract](#scenario-content-type-before-body-preserved-from-last-contract)
1719

1820
## Overview
1921

@@ -590,3 +592,47 @@ public virtual AsyncPageable<Item> GetItemsAsync(int? maxCount = null, Cancellat
590592
- This conversion is specific to paging operations only
591593
- Existing client code with `top` continues to compile without changes
592594
- New code benefits from the standardized `maxCount` naming convention
595+
596+
### Content-Type Parameter Ordering
597+
598+
The generator places the `contentType` parameter after the body (`content`) parameter in method signatures. However, backward compatibility is maintained when the last contract had a different ordering.
599+
600+
#### Scenario: Content-Type Before Body Preserved from Last Contract
601+
602+
**Description:** The generator places `contentType` after the `content` (body) parameter. However, if the last contract had `contentType` before `content`, the generator preserves that ordering to avoid breaking existing code.
603+
604+
This commonly occurs when a library was previously generated with contentType before body and has already been released (GA'd).
605+
606+
**Example:**
607+
608+
**contentType before body exists in LastContractView - preserved for backward compatibility**
609+
610+
Previous version had `contentType` before `content`:
611+
612+
```csharp
613+
public virtual ClientResult UpdateSkillDefaultVersion(string skillId, string contentType, BinaryContent content, RequestOptions options = null)
614+
{
615+
// ...
616+
}
617+
```
618+
619+
Current TypeSpec defines a content type:
620+
621+
```typespec
622+
op UpdateSkillDefaultVersion(
623+
@path skill_id: string,
624+
@header contentType: string,
625+
@body body: SetDefaultSkillVersionBody,
626+
): SkillResource;
627+
```
628+
629+
**Generated Compatibility Result:**
630+
631+
The generator detects that the previous contract had `contentType` before `content` and preserves that ordering:
632+
633+
```csharp
634+
public virtual ClientResult UpdateSkillDefaultVersion(string skillId, string contentType, BinaryContent content, RequestOptions options = null)
635+
{
636+
// contentType stays before content for backward compatibility
637+
}
638+
```

0 commit comments

Comments
 (0)