Skip to content

Commit 38e1ed6

Browse files
JoshLove-msftJoshLove-msft
authored
Fix to request options optionality (#7641)
Fixes #7636
1 parent 00a8044 commit 38e1ed6

File tree

8 files changed

+97
-48
lines changed

8 files changed

+97
-48
lines changed

‎packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Primitives/ScmKnownParameters.cs

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,23 +30,40 @@ internal static class ScmKnownParameters
3030
public static readonly ParameterProvider JsonElement = new("element", FormattableStringHelpers.Empty, typeof(JsonElement));
3131
public static readonly ParameterProvider Data = new("data", FormattableStringHelpers.Empty, typeof(BinaryData));
3232
public static ParameterProvider ClientOptions(CSharpType clientOptionsType)
33-
=> new("options", $"The options for configuring the client.", clientOptionsType.WithNullable(true), initializationValue: New.Instance(clientOptionsType.WithNullable(true)));
33+
=> new("options", $"The options for configuring the client.", clientOptionsType, initializationValue: New.Instance(clientOptionsType));
3434
public static readonly ParameterProvider OptionalRequestOptions = new(
3535
ScmCodeModelGenerator.Instance.TypeFactory.HttpRequestOptionsApi.ParameterName,
3636
$"The request options, which can override default behaviors of the client pipeline on a per-call basis.",
37-
ScmCodeModelGenerator.Instance.TypeFactory.HttpRequestOptionsApi.HttpRequestOptionsType.WithNullable(true),
37+
ScmCodeModelGenerator.Instance.TypeFactory.HttpRequestOptionsApi.HttpRequestOptionsType,
3838
defaultValue: Null);
3939
public static readonly ParameterProvider RequestOptions = new(ScmCodeModelGenerator.Instance.TypeFactory.HttpRequestOptionsApi.ParameterName, $"The request options, which can override default behaviors of the client pipeline on a per-call basis.", ScmCodeModelGenerator.Instance.TypeFactory.HttpRequestOptionsApi.HttpRequestOptionsType);
40-
public static readonly ParameterProvider RequestContent = new("content", $"The content to send as the body of the request.", ScmCodeModelGenerator.Instance.TypeFactory.RequestContentApi.RequestContentType, location: ParameterLocation.Body) { Validation = ParameterValidationType.AssertNotNull };
4140
public static readonly ParameterProvider CancellationToken = new("cancellationToken", $"The cancellation token that can be used to cancel the operation.", new CSharpType(typeof(CancellationToken)), defaultValue: Default);
4241

43-
// There is intentionally no default value here to avoid ambiguous calls between convenience and protocol methods.
44-
public static readonly ParameterProvider OptionalRequestContent = new(
45-
"content",
46-
$"The content to send as the body of the request.",
47-
ScmCodeModelGenerator.Instance.TypeFactory.RequestContentApi.RequestContentType.WithNullable(true),
42+
private static readonly FormattableString RequestContentDescription = $"The content to send as the body of the request.";
43+
private const string RequestContentParameterName = "content";
44+
45+
public static readonly ParameterProvider RequestContent = new(
46+
RequestContentParameterName,
47+
RequestContentDescription,
48+
ScmCodeModelGenerator.Instance.TypeFactory.RequestContentApi.RequestContentType,
49+
location: ParameterLocation.Body)
50+
{
51+
Validation = ParameterValidationType.AssertNotNull
52+
};
53+
54+
public static readonly ParameterProvider NullableRequiredRequestContent = new(
55+
RequestContentParameterName,
56+
RequestContentDescription,
57+
ScmCodeModelGenerator.Instance.TypeFactory.RequestContentApi.RequestContentType,
4858
location: ParameterLocation.Body);
4959

60+
public static readonly ParameterProvider OptionalRequestContent = new(
61+
RequestContentParameterName,
62+
RequestContentDescription,
63+
ScmCodeModelGenerator.Instance.TypeFactory.RequestContentApi.RequestContentType,
64+
location: ParameterLocation.Body,
65+
defaultValue: Null);
66+
5067
// Known header parameters
5168
public static readonly ParameterProvider RepeatabilityRequestId = new("repeatabilityRequestId", FormattableStringHelpers.Empty, typeof(Guid))
5269
{

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

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ public class ScmMethodProviderCollection : IReadOnlyList<ScmMethodProvider>
3232
private IReadOnlyList<ParameterProvider>? _convenienceMethodParameters;
3333
private readonly InputPagingServiceMethod? _pagingServiceMethod;
3434
private IReadOnlyList<ScmMethodProvider>? _methods;
35+
private readonly bool _generateConvenienceMethod;
3536

3637
private ClientProvider Client { get; }
3738
protected InputServiceMethod ServiceMethod { get; }
@@ -69,6 +70,8 @@ public ScmMethodProviderCollection(InputServiceMethod serviceMethod, TypeProvide
6970

7071
Client = enclosingType as ClientProvider ?? throw new InvalidOperationException("Scm methods can only be built for client types.");
7172
_createRequestMethod = Client.RestClient.GetCreateRequestMethod(ServiceMethod.Operation);
73+
_generateConvenienceMethod = ServiceMethod.Operation is
74+
{ GenerateConvenienceMethod: true, IsMultipartFormData: false };
7275

7376
if (serviceMethod is InputPagingServiceMethod pagingServiceMethod)
7477
{
@@ -92,7 +95,7 @@ protected virtual IReadOnlyList<ScmMethodProvider> BuildMethods()
9295
var syncProtocol = BuildProtocolMethod(_createRequestMethod, false);
9396
var asyncProtocol = BuildProtocolMethod(_createRequestMethod, true);
9497

95-
if (ServiceMethod.Operation.GenerateConvenienceMethod && !ServiceMethod.Operation.IsMultipartFormData)
98+
if (_generateConvenienceMethod)
9699
{
97100
return
98101
[
@@ -539,13 +542,21 @@ private ScmMethodProvider BuildProtocolMethod(MethodProvider createRequestMethod
539542
{
540543
// If there are optional parameters, but the request options parameter is not optional, make the optional parameters nullable required.
541544
// This is to ensure that the request options parameter is always the last parameter.
545+
var newlyRequiredParameters = new List<ParameterProvider>(optionalParameters.Count);
542546
foreach (var parameter in optionalParameters)
543547
{
548+
// don't mutate the static request content parameter
549+
if (parameter.Equals(ScmKnownParameters.OptionalRequestContent))
550+
{
551+
newlyRequiredParameters.Add(ScmKnownParameters.NullableRequiredRequestContent);
552+
continue;
553+
}
544554
parameter.DefaultValue = null;
545555
parameter.Type = parameter.Type.WithNullable(true);
556+
newlyRequiredParameters.Add(parameter);
546557
}
547558

548-
requiredParameters.AddRange(optionalParameters);
559+
requiredParameters.AddRange(newlyRequiredParameters);
549560
optionalParameters.Clear();
550561
}
551562

@@ -694,19 +705,23 @@ private CSharpType GetConvenienceReturnType(IReadOnlyList<InputOperationResponse
694705
private bool ShouldAddOptionalRequestOptionsParameter()
695706
{
696707
var convenienceMethodParameterCount = ConvenienceMethodParameters.Count;
697-
if (convenienceMethodParameterCount == 0)
708+
if (!_generateConvenienceMethod)
698709
{
699-
return false;
710+
return true;
700711
}
701-
702-
// the request options parameter is optional if the methods have different parameters.
703-
if (ProtocolMethodParameters.Count != convenienceMethodParameterCount)
712+
if (convenienceMethodParameterCount == 0)
704713
{
705-
return true;
714+
return false;
706715
}
707716

708717
for (int i = 0; i < convenienceMethodParameterCount; i++)
709718
{
719+
// Once we hit an optional parameter, then we need to make the request options required
720+
// to prevent ambiguous callsites.
721+
if (ConvenienceMethodParameters[i].DefaultValue != null)
722+
{
723+
return false;
724+
}
710725
if (!ProtocolMethodParameters[i].Type.Equals(ConvenienceMethodParameters[i].Type))
711726
{
712727
return true;

‎packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ClientProviders/ClientProviderTests.cs

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -710,9 +710,9 @@ public void ValidateClientWithSpread(InputClient inputClient)
710710
Assert.AreEqual(2, protocolMethods[1].Signature.Parameters.Count);
711711

712712
Assert.AreEqual(new CSharpType(typeof(BinaryContent)), protocolMethods[0].Signature.Parameters[0].Type);
713-
Assert.AreEqual(new CSharpType(typeof(RequestOptions), true), protocolMethods[0].Signature.Parameters[1].Type);
713+
Assert.AreEqual(new CSharpType(typeof(RequestOptions)), protocolMethods[0].Signature.Parameters[1].Type);
714714
Assert.AreEqual(new CSharpType(typeof(BinaryContent)), protocolMethods[1].Signature.Parameters[0].Type);
715-
Assert.AreEqual(new CSharpType(typeof(RequestOptions), true), protocolMethods[1].Signature.Parameters[1].Type);
715+
Assert.AreEqual(new CSharpType(typeof(RequestOptions)), protocolMethods[1].Signature.Parameters[1].Type);
716716

717717
var convenienceMethods = methods.Where(m => m.Signature.Parameters.Any(p => p.Type.Equals(typeof(string)))).ToList();
718718
Assert.AreEqual(2, convenienceMethods.Count);
@@ -733,14 +733,14 @@ public void TestRequestOptionsParameterInSignature(InputServiceMethod inputServi
733733

734734
var requestOptionsParameterInSyncMethod = syncMethod!.Signature.Parameters.FirstOrDefault(p => p.Type.Name == "RequestOptions");
735735
Assert.IsNotNull(requestOptionsParameterInSyncMethod);
736-
Assert.AreEqual(shouldBeOptional, requestOptionsParameterInSyncMethod!.Type.IsNullable);
736+
Assert.AreEqual(shouldBeOptional, requestOptionsParameterInSyncMethod!.DefaultValue != null);
737737

738738
var asyncMethod = protocolMethods.FirstOrDefault(m => m.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Async));
739739
Assert.IsNotNull(asyncMethod);
740740

741741
var requestOptionsParameterInAsyncMethod = asyncMethod!.Signature.Parameters.FirstOrDefault(p => p.Type.Name == "RequestOptions");
742742
Assert.IsNotNull(requestOptionsParameterInAsyncMethod);
743-
Assert.AreEqual(shouldBeOptional, requestOptionsParameterInAsyncMethod!.Type.IsNullable);
743+
Assert.AreEqual(shouldBeOptional, requestOptionsParameterInAsyncMethod!.DefaultValue != null);
744744

745745
// request options should always be last parameter
746746
Assert.AreEqual("RequestOptions", syncMethod.Signature.Parameters[^1].Type.Name);
@@ -1605,7 +1605,7 @@ public static IEnumerable<TestCaseData> RequestOptionsParameterInSignatureTestCa
16051605
isRequired: true),
16061606
]), false, true);
16071607

1608-
// convenience method only has a body param, so RequestOptions should be optional in protocol method.
1608+
// convenience method only has a body param, but it is optional, so RequestOptions should be optional in protocol method.
16091609
yield return new TestCaseData(
16101610
InputFactory.BasicServiceMethod(
16111611
"TestServiceMethod",
@@ -1624,7 +1624,7 @@ public static IEnumerable<TestCaseData> RequestOptionsParameterInSignatureTestCa
16241624
"p1",
16251625
InputPrimitiveType.String,
16261626
location: InputRequestLocation.Body),
1627-
]), true, false);
1627+
]), false, false);
16281628

16291629
// Protocol & convenience methods will have different parameters since there is a model body param, so RequestOptions should be optional.
16301630
yield return new TestCaseData(
@@ -1659,8 +1659,7 @@ public static IEnumerable<TestCaseData> RequestOptionsParameterInSignatureTestCa
16591659
isRequired: true),
16601660
]), true, false);
16611661

1662-
// Protocol & convenience methods will have different parameters since there is a model body param, so RequestOptions should be optional.
1663-
// One parameter is optional
1662+
// Protocol & convenience methods will have different parameters but since the body parameter is optional, the request options should be required in protocol method.
16641663
yield return new TestCaseData(
16651664
InputFactory.BasicServiceMethod(
16661665
"TestServiceMethod",
@@ -1691,7 +1690,7 @@ public static IEnumerable<TestCaseData> RequestOptionsParameterInSignatureTestCa
16911690
InputFactory.Model("SampleModel"),
16921691
location: InputRequestLocation.Body,
16931692
isRequired: false),
1694-
]), true, true);
1693+
]), false, true);
16951694

16961695

16971696
// Convenience method has no parameters, RequestOptions should be required in protocol method.

‎packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmMethodProviderCollectionTests.cs

Lines changed: 30 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -276,17 +276,19 @@ public void ListMethodWithEnumParameter(bool isExtensible, InputRequestLocation
276276
}
277277
}
278278

279-
[TestCase(true)]
280-
[TestCase(false)]
281-
public void RequestOptionsOptionality(bool inBody)
279+
[TestCase(true, false, true)]
280+
[TestCase(true, true, false)]
281+
[TestCase(false, false, false)]
282+
[TestCase(false, true, false)]
283+
public void RequestOptionsOptionality(bool inBody, bool hasOptionalParameter, bool shouldBeOptional)
282284
{
283285
MockHelpers.LoadMockGenerator();
284286
List<InputParameter> parameters =
285287
[
286288
InputFactory.Parameter(
287289
"message",
288290
InputPrimitiveType.Boolean,
289-
isRequired: true,
291+
isRequired: !hasOptionalParameter,
290292
location: inBody ? InputRequestLocation.Body : InputRequestLocation.Query)
291293
];
292294
var inputOperation = InputFactory.Operation(
@@ -302,19 +304,34 @@ public void RequestOptionsOptionality(bool inBody)
302304
Assert.AreEqual(inputServiceMethod, protocolMethod!.ServiceMethod);
303305

304306
var optionsParameter = protocolMethod!.Signature.Parameters.Single(p => p.Name == "options");
305-
if (inBody)
306-
{
307-
// When the parameter is in the body, the signatures of the protocol and convenience methods
308-
// will differ due to the presence of the BinaryContent parameter, which means the options parameter
309-
// can remain optional.
310-
Assert.IsNotNull(optionsParameter.DefaultValue);
311-
}
312-
else
307+
Assert.AreEqual(shouldBeOptional, optionsParameter.DefaultValue != null);
308+
309+
if (!shouldBeOptional)
313310
{
314-
Assert.IsNull(optionsParameter.DefaultValue);
311+
Assert.IsTrue(protocolMethod.Signature.Parameters.All(p => p.DefaultValue == null));
315312
}
316313
}
317314

315+
[Test]
316+
public void RequestOptionsIsOptionalWhenNoConvenience()
317+
{
318+
MockHelpers.LoadMockGenerator();
319+
var inputOperation = InputFactory.Operation(
320+
"TestOperation",
321+
generateConvenienceMethod: false);
322+
var inputServiceMethod = InputFactory.BasicServiceMethod("Test", inputOperation);
323+
var inputClient = InputFactory.Client("TestClient", methods: [inputServiceMethod]);
324+
var client = ScmCodeModelGenerator.Instance.TypeFactory.CreateClient(inputClient);
325+
var methodCollection = new ScmMethodProviderCollection(inputServiceMethod, client!);
326+
var protocolMethod = methodCollection.FirstOrDefault(
327+
m => m.Signature.Parameters.Any(p => p.Name == "options") && m.Signature.Name == "TestOperation");
328+
Assert.IsNotNull(protocolMethod);
329+
Assert.AreEqual(inputServiceMethod, protocolMethod!.ServiceMethod);
330+
331+
var optionsParameter = protocolMethod!.Signature.Parameters.Single(p => p.Name == "options");
332+
Assert.IsNotNull(optionsParameter.DefaultValue);
333+
}
334+
318335
[Test]
319336
public void OperationWithOptionalEnum()
320337
{

‎packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/common/InputFactory.cs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -429,7 +429,8 @@ public static InputOperation Operation(
429429
IEnumerable<string>? requestMediaTypes = null,
430430
string uri = "",
431431
string path = "",
432-
string httpMethod = "GET")
432+
string httpMethod = "GET",
433+
bool generateConvenienceMethod = true)
433434
{
434435
return new InputOperation(
435436
name,
@@ -447,7 +448,7 @@ public static InputOperation Operation(
447448
requestMediaTypes is null ? null : [.. requestMediaTypes],
448449
false,
449450
true,
450-
true,
451+
generateConvenienceMethod,
451452
name);
452453
}
453454

‎packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/SampleTypeSpecClient.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -522,7 +522,7 @@ public virtual async Task<ClientResult<Thing>> TopActionAsync(DateTimeOffset act
522522
/// <param name="options"> The request options, which can override default behaviors of the client pipeline on a per-call basis. </param>
523523
/// <exception cref="ClientResultException"> Service returned a non-success status code. </exception>
524524
/// <returns> The response returned from the service. </returns>
525-
public virtual ClientResult TopAction2(RequestOptions options)
525+
public virtual ClientResult TopAction2(RequestOptions options = null)
526526
{
527527
using PipelineMessage message = CreateTopAction2Request(options);
528528
return ClientResult.FromResponse(Pipeline.ProcessMessage(message, options));
@@ -539,7 +539,7 @@ public virtual ClientResult TopAction2(RequestOptions options)
539539
/// <param name="options"> The request options, which can override default behaviors of the client pipeline on a per-call basis. </param>
540540
/// <exception cref="ClientResultException"> Service returned a non-success status code. </exception>
541541
/// <returns> The response returned from the service. </returns>
542-
public virtual async Task<ClientResult> TopAction2Async(RequestOptions options)
542+
public virtual async Task<ClientResult> TopAction2Async(RequestOptions options = null)
543543
{
544544
using PipelineMessage message = CreateTopAction2Request(options);
545545
return ClientResult.FromResponse(await Pipeline.ProcessMessageAsync(message, options).ConfigureAwait(false));

‎packages/http-client-csharp/generator/TestProjects/Spector/http/parameters/body-optionality/src/Generated/OptionalExplicit.cs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,17 +16,17 @@ public partial class OptionalExplicit
1616

1717
public ClientPipeline Pipeline => throw null;
1818

19-
public virtual ClientResult Set(BinaryContent content, RequestOptions options = null) => throw null;
19+
public virtual ClientResult Set(BinaryContent content, RequestOptions options) => throw null;
2020

21-
public virtual Task<ClientResult> SetAsync(BinaryContent content, RequestOptions options = null) => throw null;
21+
public virtual Task<ClientResult> SetAsync(BinaryContent content, RequestOptions options) => throw null;
2222

2323
public virtual ClientResult Set(BodyModel body = default, CancellationToken cancellationToken = default) => throw null;
2424

2525
public virtual Task<ClientResult> SetAsync(BodyModel body = default, CancellationToken cancellationToken = default) => throw null;
2626

27-
public virtual ClientResult Omit(BinaryContent content, RequestOptions options = null) => throw null;
27+
public virtual ClientResult Omit(BinaryContent content, RequestOptions options) => throw null;
2828

29-
public virtual Task<ClientResult> OmitAsync(BinaryContent content, RequestOptions options = null) => throw null;
29+
public virtual Task<ClientResult> OmitAsync(BinaryContent content, RequestOptions options) => throw null;
3030

3131
public virtual ClientResult Omit(BodyModel body = default, CancellationToken cancellationToken = default) => throw null;
3232

0 commit comments

Comments
 (0)