Fix to request options optionality

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

@@ -30,23 +30,40 @@ internal static class ScmKnownParameters
         public static readonly ParameterProvider JsonElement = new("element", FormattableStringHelpers.Empty, typeof(JsonElement));
         public static readonly ParameterProvider Data = new("data", FormattableStringHelpers.Empty, typeof(BinaryData));
         public static ParameterProvider ClientOptions(CSharpType clientOptionsType)
-            => new("options", $"The options for configuring the client.", clientOptionsType.WithNullable(true), initializationValue: New.Instance(clientOptionsType.WithNullable(true)));
+            => new("options", $"The options for configuring the client.", clientOptionsType, initializationValue: New.Instance(clientOptionsType));
         public static readonly ParameterProvider OptionalRequestOptions = 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.WithNullable(true),
+            ScmCodeModelGenerator.Instance.TypeFactory.HttpRequestOptionsApi.HttpRequestOptionsType,
             defaultValue: Null);
         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);
-        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 };
         public static readonly ParameterProvider CancellationToken = new("cancellationToken", $"The cancellation token that can be used to cancel the operation.", new CSharpType(typeof(CancellationToken)), defaultValue: Default);
 
-        // There is intentionally no default value here to avoid ambiguous calls between convenience and protocol methods.
-        public static readonly ParameterProvider OptionalRequestContent = new(
-            "content",
-            $"The content to send as the body of the request.",
-            ScmCodeModelGenerator.Instance.TypeFactory.RequestContentApi.RequestContentType.WithNullable(true),
+        private static readonly FormattableString RequestContentDescription = $"The content to send as the body of the request.";
+        private const string RequestContentParameterName = "content";
+
+        public static readonly ParameterProvider RequestContent = new(
+            RequestContentParameterName,
+            RequestContentDescription,
+            ScmCodeModelGenerator.Instance.TypeFactory.RequestContentApi.RequestContentType,
+            location: ParameterLocation.Body)
+        {
+            Validation = ParameterValidationType.AssertNotNull
+        };
+
+        public static readonly ParameterProvider NullableRequiredRequestContent = new(
+            RequestContentParameterName,
+            RequestContentDescription,
+            ScmCodeModelGenerator.Instance.TypeFactory.RequestContentApi.RequestContentType,
             location: ParameterLocation.Body);
 
+        public static readonly ParameterProvider OptionalRequestContent = new(
+            RequestContentParameterName,
+            RequestContentDescription,
+            ScmCodeModelGenerator.Instance.TypeFactory.RequestContentApi.RequestContentType,
+            location: ParameterLocation.Body,
+            defaultValue: Null);
+
         // Known header parameters
         public static readonly ParameterProvider RepeatabilityRequestId = new("repeatabilityRequestId", FormattableStringHelpers.Empty, typeof(Guid))
         {

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

@@ -32,6 +32,7 @@ public class ScmMethodProviderCollection : IReadOnlyList<ScmMethodProvider>
         private IReadOnlyList<ParameterProvider>? _convenienceMethodParameters;
         private readonly InputPagingServiceMethod? _pagingServiceMethod;
         private IReadOnlyList<ScmMethodProvider>? _methods;
+        private readonly bool _generateConvenienceMethod;
 
         private ClientProvider Client { get; }
         protected InputServiceMethod ServiceMethod { get; }
@@ -69,6 +70,8 @@ public ScmMethodProviderCollection(InputServiceMethod serviceMethod, TypeProvide
 
             Client = enclosingType as ClientProvider ?? throw new InvalidOperationException("Scm methods can only be built for client types.");
             _createRequestMethod = Client.RestClient.GetCreateRequestMethod(ServiceMethod.Operation);
+            _generateConvenienceMethod = ServiceMethod.Operation is
+                { GenerateConvenienceMethod: true, IsMultipartFormData: false };
 
             if (serviceMethod is InputPagingServiceMethod pagingServiceMethod)
             {
@@ -92,7 +95,7 @@ protected virtual IReadOnlyList<ScmMethodProvider> BuildMethods()
             var syncProtocol = BuildProtocolMethod(_createRequestMethod, false);
             var asyncProtocol = BuildProtocolMethod(_createRequestMethod, true);
 
-            if (ServiceMethod.Operation.GenerateConvenienceMethod && !ServiceMethod.Operation.IsMultipartFormData)
+            if (_generateConvenienceMethod)
             {
                 return
                 [
@@ -539,13 +542,21 @@ private ScmMethodProvider BuildProtocolMethod(MethodProvider createRequestMethod
             {
                 // If there are optional parameters, but the request options parameter is not optional, make the optional parameters nullable required.
                 // This is to ensure that the request options parameter is always the last parameter.
+                var newlyRequiredParameters = new List<ParameterProvider>(optionalParameters.Count);
                 foreach (var parameter in optionalParameters)
                 {
+                    // don't mutate the static request content parameter
+                    if (parameter.Equals(ScmKnownParameters.OptionalRequestContent))
+                    {
+                        newlyRequiredParameters.Add(ScmKnownParameters.NullableRequiredRequestContent);
+                        continue;
+                    }
                     parameter.DefaultValue = null;
                     parameter.Type = parameter.Type.WithNullable(true);
+                    newlyRequiredParameters.Add(parameter);
                 }
 
-                requiredParameters.AddRange(optionalParameters);
+                requiredParameters.AddRange(newlyRequiredParameters);
                 optionalParameters.Clear();
             }
 
@@ -691,19 +702,23 @@ private CSharpType GetConvenienceReturnType(IReadOnlyList<InputOperationResponse
         private bool ShouldAddOptionalRequestOptionsParameter()
         {
             var convenienceMethodParameterCount = ConvenienceMethodParameters.Count;
-            if (convenienceMethodParameterCount == 0)
+            if (!_generateConvenienceMethod)
             {
-                return false;
+                return true;
             }
-
-            // the request options parameter is optional if the methods have different parameters.
-            if (ProtocolMethodParameters.Count != convenienceMethodParameterCount)
+            if (convenienceMethodParameterCount == 0)
             {
-                return true;
+                return false;
             }
 
             for (int i = 0; i < convenienceMethodParameterCount; i++)
             {
+                // Once we hit an optional parameter, then we need to make the request options required
+                // to prevent ambiguous callsites.
+                if (ConvenienceMethodParameters[i].DefaultValue != null)
+                {
+                    return false;
+                }
                 if (!ProtocolMethodParameters[i].Type.Equals(ConvenienceMethodParameters[i].Type))
                 {
                     return true;

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

@@ -710,9 +710,9 @@ public void ValidateClientWithSpread(InputClient inputClient)
             Assert.AreEqual(2, protocolMethods[1].Signature.Parameters.Count);
 
             Assert.AreEqual(new CSharpType(typeof(BinaryContent)), protocolMethods[0].Signature.Parameters[0].Type);
-            Assert.AreEqual(new CSharpType(typeof(RequestOptions), true), protocolMethods[0].Signature.Parameters[1].Type);
+            Assert.AreEqual(new CSharpType(typeof(RequestOptions)), protocolMethods[0].Signature.Parameters[1].Type);
             Assert.AreEqual(new CSharpType(typeof(BinaryContent)), protocolMethods[1].Signature.Parameters[0].Type);
-            Assert.AreEqual(new CSharpType(typeof(RequestOptions), true), protocolMethods[1].Signature.Parameters[1].Type);
+            Assert.AreEqual(new CSharpType(typeof(RequestOptions)), protocolMethods[1].Signature.Parameters[1].Type);
 
             var convenienceMethods = methods.Where(m => m.Signature.Parameters.Any(p => p.Type.Equals(typeof(string)))).ToList();
             Assert.AreEqual(2, convenienceMethods.Count);
@@ -733,14 +733,14 @@ public void TestRequestOptionsParameterInSignature(InputServiceMethod inputServi
 
             var requestOptionsParameterInSyncMethod = syncMethod!.Signature.Parameters.FirstOrDefault(p => p.Type.Name == "RequestOptions");
             Assert.IsNotNull(requestOptionsParameterInSyncMethod);
-            Assert.AreEqual(shouldBeOptional, requestOptionsParameterInSyncMethod!.Type.IsNullable);
+            Assert.AreEqual(shouldBeOptional, requestOptionsParameterInSyncMethod!.DefaultValue != null);
 
             var asyncMethod = protocolMethods.FirstOrDefault(m => m.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Async));
             Assert.IsNotNull(asyncMethod);
 
             var requestOptionsParameterInAsyncMethod = asyncMethod!.Signature.Parameters.FirstOrDefault(p => p.Type.Name == "RequestOptions");
             Assert.IsNotNull(requestOptionsParameterInAsyncMethod);
-            Assert.AreEqual(shouldBeOptional, requestOptionsParameterInAsyncMethod!.Type.IsNullable);
+            Assert.AreEqual(shouldBeOptional, requestOptionsParameterInAsyncMethod!.DefaultValue != null);
 
             // request options should always be last parameter
             Assert.AreEqual("RequestOptions", syncMethod.Signature.Parameters[^1].Type.Name);
@@ -1605,7 +1605,7 @@ public static IEnumerable<TestCaseData> RequestOptionsParameterInSignatureTestCa
                                 isRequired: true),
                         ]), false, true);
 
-                // convenience method only has a body param, so RequestOptions should be optional in protocol method.
+                // convenience method only has a body param, but it is optional, so RequestOptions should be optional in protocol method.
                 yield return new TestCaseData(
                      InputFactory.BasicServiceMethod(
                         "TestServiceMethod",
@@ -1624,7 +1624,7 @@ public static IEnumerable<TestCaseData> RequestOptionsParameterInSignatureTestCa
                                 "p1",
                                 InputPrimitiveType.String,
                                 location: InputRequestLocation.Body),
-                        ]), true, false);
+                        ]), false, false);
 
                 // Protocol & convenience methods will have different parameters since there is a model body param, so RequestOptions should be optional.
                 yield return new TestCaseData(
@@ -1659,8 +1659,7 @@ public static IEnumerable<TestCaseData> RequestOptionsParameterInSignatureTestCa
                                 isRequired: true),
                         ]), true, false);
 
-                // Protocol & convenience methods will have different parameters since there is a model body param, so RequestOptions should be optional.
-                // One parameter is optional
+                // Protocol & convenience methods will have different parameters but since the body parameter is optional, the request options should be required in protocol method.
                 yield return new TestCaseData(
                     InputFactory.BasicServiceMethod(
                         "TestServiceMethod",
@@ -1691,7 +1690,7 @@ public static IEnumerable<TestCaseData> RequestOptionsParameterInSignatureTestCa
                                 InputFactory.Model("SampleModel"),
                                 location: InputRequestLocation.Body,
                                 isRequired: false),
-                        ]), true, true);
+                        ]), false, true);
 
 
                 // 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

@@ -191,17 +191,19 @@ public void ListMethodWithImplicitPaging()
             Assert.IsTrue(signature.ReturnType!.Equals(expectedReturnType));
         }
 
-        [TestCase(true)]
-        [TestCase(false)]
-        public void RequestOptionsOptionality(bool inBody)
+        [TestCase(true, false, true)]
+        [TestCase(true, true, false)]
+        [TestCase(false, false, false)]
+        [TestCase(false, true, false)]
+        public void RequestOptionsOptionality(bool inBody, bool hasOptionalParameter, bool shouldBeOptional)
         {
             MockHelpers.LoadMockGenerator();
             List<InputParameter> parameters =
             [
                 InputFactory.Parameter(
                     "message",
                     InputPrimitiveType.Boolean,
-                    isRequired: true,
+                    isRequired: !hasOptionalParameter,
                     location: inBody ? InputRequestLocation.Body : InputRequestLocation.Query)
             ];
             var inputOperation = InputFactory.Operation(
@@ -217,19 +219,34 @@ public void RequestOptionsOptionality(bool inBody)
             Assert.AreEqual(inputServiceMethod, protocolMethod!.ServiceMethod);
 
             var optionsParameter = protocolMethod!.Signature.Parameters.Single(p => p.Name == "options");
-            if (inBody)
-            {
-                // When the parameter is in the body, the signatures of the protocol and convenience methods
-                // will differ due to the presence of the BinaryContent parameter, which means the options parameter
-                // can remain optional.
-                Assert.IsNotNull(optionsParameter.DefaultValue);
-            }
-            else
+            Assert.AreEqual(shouldBeOptional, optionsParameter.DefaultValue != null);
+
+            if (!shouldBeOptional)
             {
-                Assert.IsNull(optionsParameter.DefaultValue);
+                Assert.IsTrue(protocolMethod.Signature.Parameters.All(p => p.DefaultValue == null));
             }
         }
 
+        [Test]
+        public void RequestOptionsIsOptionalWhenNoConvenience()
+        {
+            MockHelpers.LoadMockGenerator();
+            var inputOperation = InputFactory.Operation(
+                "TestOperation",
+                generateConvenienceMethod: false);
+            var inputServiceMethod = InputFactory.BasicServiceMethod("Test", inputOperation);
+            var inputClient = InputFactory.Client("TestClient", methods: [inputServiceMethod]);
+            var client = ScmCodeModelGenerator.Instance.TypeFactory.CreateClient(inputClient);
+            var methodCollection = new ScmMethodProviderCollection(inputServiceMethod, client!);
+            var protocolMethod = methodCollection.FirstOrDefault(
+                m => m.Signature.Parameters.Any(p => p.Name == "options") && m.Signature.Name == "TestOperation");
+            Assert.IsNotNull(protocolMethod);
+            Assert.AreEqual(inputServiceMethod, protocolMethod!.ServiceMethod);
+
+            var optionsParameter = protocolMethod!.Signature.Parameters.Single(p => p.Name == "options");
+            Assert.IsNotNull(optionsParameter.DefaultValue);
+        }
+
         [Test]
         public void OperationWithOptionalEnum()
         {

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

@@ -429,7 +429,8 @@ public static InputOperation Operation(
             IEnumerable<string>? requestMediaTypes = null,
             string uri = "",
             string path = "",
-            string httpMethod = "GET")
+            string httpMethod = "GET",
+            bool generateConvenienceMethod = true)
         {
             return new InputOperation(
                 name,
@@ -447,7 +448,7 @@ public static InputOperation Operation(
                 requestMediaTypes is null ? null : [.. requestMediaTypes],
                 false,
                 true,
-                true,
+                generateConvenienceMethod,
                 name);
         }
 

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

@@ -522,7 +522,7 @@ public virtual async Task<ClientResult<Thing>> TopActionAsync(DateTimeOffset act
         /// <param name="options"> The request options, which can override default behaviors of the client pipeline on a per-call basis. </param>
         /// <exception cref="ClientResultException"> Service returned a non-success status code. </exception>
         /// <returns> The response returned from the service. </returns>
-        public virtual ClientResult TopAction2(RequestOptions options)
+        public virtual ClientResult TopAction2(RequestOptions options = null)
         {
             using PipelineMessage message = CreateTopAction2Request(options);
             return ClientResult.FromResponse(Pipeline.ProcessMessage(message, options));
@@ -539,7 +539,7 @@ public virtual ClientResult TopAction2(RequestOptions options)
         /// <param name="options"> The request options, which can override default behaviors of the client pipeline on a per-call basis. </param>
         /// <exception cref="ClientResultException"> Service returned a non-success status code. </exception>
         /// <returns> The response returned from the service. </returns>
-        public virtual async Task<ClientResult> TopAction2Async(RequestOptions options)
+        public virtual async Task<ClientResult> TopAction2Async(RequestOptions options = null)
         {
             using PipelineMessage message = CreateTopAction2Request(options);
             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

@@ -16,17 +16,17 @@ public partial class OptionalExplicit
 
         public ClientPipeline Pipeline => throw null;
 
-        public virtual ClientResult Set(BinaryContent content, RequestOptions options = null) => throw null;
+        public virtual ClientResult Set(BinaryContent content, RequestOptions options) => throw null;
 
-        public virtual Task<ClientResult> SetAsync(BinaryContent content, RequestOptions options = null) => throw null;
+        public virtual Task<ClientResult> SetAsync(BinaryContent content, RequestOptions options) => throw null;
 
         public virtual ClientResult Set(BodyModel body = default, CancellationToken cancellationToken = default) => throw null;
 
         public virtual Task<ClientResult> SetAsync(BodyModel body = default, CancellationToken cancellationToken = default) => throw null;
 
-        public virtual ClientResult Omit(BinaryContent content, RequestOptions options = null) => throw null;
+        public virtual ClientResult Omit(BinaryContent content, RequestOptions options) => throw null;
 
-        public virtual Task<ClientResult> OmitAsync(BinaryContent content, RequestOptions options = null) => throw null;
+        public virtual Task<ClientResult> OmitAsync(BinaryContent content, RequestOptions options) => throw null;
 
         public virtual ClientResult Omit(BodyModel body = default, CancellationToken cancellationToken = default) => throw null;