fix(http-client-csharp): remove setters for model properties collecti…

…on types (#3233)

This PR fixes the case where setters were being generated for model
properties that were collection types. Model properties that are
collection types will no longer generated a setter unless they are
nullable.

---------

Co-authored-by: ShivangiReja <45216704+ShivangiReja@users.noreply.github.com>
Co-authored-by: Dapeng Zhang <ufo54153@gmail.com>
Co-authored-by: ShivangiReja <shivangi.reja@microsoft.com>

packages/http-client-csharp/generator/Microsoft.Generator.CSharp.ClientModel.TestProjects/Unbranded-TypeSpec/src/Generated/Models/RoundTripModel.cs

@@ -18,10 +18,10 @@ public partial class RoundTripModel
         public int RequiredInt { get; set; }
 
         /// <summary> Required collection of enums. </summary>
-        public IList<string> RequiredCollection { get; set; }
+        public IList<string> RequiredCollection { get; }
 
         /// <summary> Required dictionary of enums. </summary>
-        public IDictionary<string, string> RequiredDictionary { get; set; }
+        public IDictionary<string, string> RequiredDictionary { get; }
 
         /// <summary> Required model. </summary>
         public Thing RequiredModel { get; set; }
@@ -30,25 +30,25 @@ public partial class RoundTripModel
         public string IntExtensibleEnum { get; set; }
 
         /// <summary> this is a collection of int based extensible enum. </summary>
-        public IList<string> IntExtensibleEnumCollection { get; set; }
+        public IList<string> IntExtensibleEnumCollection { get; }
 
         /// <summary> this is a float based extensible enum. </summary>
         public string FloatExtensibleEnum { get; set; }
 
         /// <summary> this is a collection of float based extensible enum. </summary>
-        public IList<string> FloatExtensibleEnumCollection { get; set; }
+        public IList<string> FloatExtensibleEnumCollection { get; }
 
         /// <summary> this is a float based fixed enum. </summary>
         public string FloatFixedEnum { get; set; }
 
         /// <summary> this is a collection of float based fixed enum. </summary>
-        public IList<string> FloatFixedEnumCollection { get; set; }
+        public IList<string> FloatFixedEnumCollection { get; }
 
         /// <summary> this is a int based fixed enum. </summary>
         public string IntFixedEnum { get; set; }
 
         /// <summary> this is a collection of int based fixed enum. </summary>
-        public IList<string> IntFixedEnumCollection { get; set; }
+        public IList<string> IntFixedEnumCollection { get; }
 
         /// <summary> this is a string based fixed enum. </summary>
         public string StringFixedEnum { get; set; }
@@ -60,10 +60,10 @@ public partial class RoundTripModel
         public System.BinaryData OptionalUnknown { get; set; }
 
         /// <summary> required record of unknown. </summary>
-        public IDictionary<string, System.BinaryData> RequiredRecordUnknown { get; set; }
+        public IDictionary<string, System.BinaryData> RequiredRecordUnknown { get; }
 
         /// <summary> optional record of unknown. </summary>
-        public IDictionary<string, System.BinaryData> OptionalRecordUnknown { get; set; }
+        public IDictionary<string, System.BinaryData> OptionalRecordUnknown { get; }
 
         /// <summary> required readonly record of unknown. </summary>
         public IDictionary<string, System.BinaryData> ReadOnlyRequiredRecordUnknown { get; }

packages/http-client-csharp/generator/Microsoft.Generator.CSharp.ClientModel.TestProjects/Unbranded-TypeSpec/src/Generated/Models/Thing.cs

@@ -18,7 +18,7 @@ public partial class Thing
         public System.BinaryData RequiredUnion { get; set; }
 
         /// <summary> required literal string. </summary>
-        public string RequiredLiteralString { get; set; } = "accept";
+        public string RequiredLiteralString { get; } = "accept";
 
         /// <summary> required literal int. </summary>
         public string RequiredLiteralInt { get; set; } = "123";
@@ -27,7 +27,7 @@ public partial class Thing
         public string RequiredLiteralFloat { get; set; } = "1.23";
 
         /// <summary> required literal bool. </summary>
-        public bool RequiredLiteralBool { get; set; } = false;
+        public bool RequiredLiteralBool { get; } = false;
 
         /// <summary> optional literal string. </summary>
         public string OptionalLiteralString { get; set; }

packages/http-client-csharp/generator/Microsoft.Generator.CSharp/src/Models/Types/ModelTypeProvider.cs

@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 
+using System;
 using Microsoft.Generator.CSharp.Expressions;
 using Microsoft.Generator.CSharp.Input;
 
@@ -48,21 +49,56 @@ protected override PropertyDeclaration[] BuildProperties()
         private PropertyDeclaration BuildPropertyDeclaration(InputModelProperty property)
         {
             var propertyType = CodeModelPlugin.Instance.TypeFactory.CreateCSharpType(property.Type);
-            MethodSignatureModifiers setterModifier = !property.IsReadOnly ? MethodSignatureModifiers.Public : MethodSignatureModifiers.None;
+            var propHasSetter = PropertyHasSetter(propertyType, property);
+            MethodSignatureModifiers setterModifier = propHasSetter ? MethodSignatureModifiers.Public : MethodSignatureModifiers.None;
 
-            // Exclude setter generation for collection properties https://github.com/Azure/autorest.csharp/issues/4616
             // Add Remarks and Example for BinaryData Properties https://github.com/Azure/autorest.csharp/issues/4617
             var propertyDeclaration = new PropertyDeclaration(
                     description: FormattableStringHelpers.FromString(property.Description),
                     modifiers: MethodSignatureModifiers.Public,
                     propertyType: propertyType,
                     name: property.Name.FirstCharToUpperCase(),
-                    propertyBody: new AutoPropertyBody(!property.IsReadOnly, setterModifier, GetPropertyInitializationValue(property, propertyType))
+                    propertyBody: new AutoPropertyBody(propHasSetter, setterModifier, GetPropertyInitializationValue(property, propertyType))
                     );
 
             return propertyDeclaration;
         }
 
+        /// <summary>
+        /// Returns true if the property has a setter.
+        /// </summary>
+        /// <param name="type">The <see cref="CSharpType"/> of the property.</param>
+        /// <param name="prop">The <see cref="InputModelProperty"/>.</param>
+        private bool PropertyHasSetter(CSharpType type, InputModelProperty prop)
+        {
+            if (prop.IsDiscriminator)
+            {
+                return true;
+            }
+
+            if (prop.IsReadOnly)
+            {
+                return false;
+            }
+
+            if (IsStruct)
+            {
+                return false;
+            }
+
+            if (type.IsLiteral && prop.IsRequired)
+            {
+                return false;
+            }
+
+            if (type.IsCollection && !type.IsReadOnlyMemory)
+            {
+                return type.IsNullable;
+            }
+
+            return true;
+        }
+
         private ConstantExpression? GetPropertyInitializationValue(InputModelProperty property, CSharpType propertyType)
         {
             if (!property.IsRequired)

packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/Models/Types/ModelTypeProviderTests.cs

@@ -0,0 +1,114 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Reflection;
+using Microsoft.Generator.CSharp.Input;
+using Moq;
+using NUnit.Framework;
+
+namespace Microsoft.Generator.CSharp.Tests
+{
+    public class ModelTypeProviderTests
+    {
+#pragma warning disable CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Consider declaring as nullable.
+        private GeneratorContext _generatorContext;
+#pragma warning restore CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Consider declaring as nullable.
+        private readonly string _configFilePath = Path.Combine(AppContext.BaseDirectory, "mocks");
+        private FieldInfo? _mockPlugin;
+
+        [SetUp]
+        public void Setup()
+        {
+            // initialize the mock singleton instance of the plugin
+            _mockPlugin = typeof(CodeModelPlugin).GetField("_instance", BindingFlags.Static | BindingFlags.NonPublic);
+            _generatorContext = new GeneratorContext(Configuration.Load(_configFilePath));
+        }
+
+        [TearDown]
+        public void Teardown()
+        {
+            _mockPlugin?.SetValue(null, null);
+        }
+
+        // Validates that the property body's setter is correctly set based on the property type
+        [TestCaseSource(nameof(BuildProperties_ValidatePropertySettersTestCases))]
+        public void BuildProperties_ValidatePropertySetters(InputModelProperty inputModelProperty, CSharpType type, bool hasSetter)
+        {
+            var mockPluginInstance = new Mock<CodeModelPlugin>(_generatorContext) { };
+            var mockTypeFactory = new Mock<TypeFactory>() { };
+            mockTypeFactory.Setup(t => t.CreateCSharpType(It.IsAny<InputType>())).Returns(type);
+            mockPluginInstance.SetupGet(p => p.TypeFactory).Returns(mockTypeFactory.Object);
+            _mockPlugin?.SetValue(null, mockPluginInstance.Object);
+
+            var props = new[]
+            {
+                inputModelProperty
+            };
+
+            var inputModel = new InputModelType("mockInputModel", "mockNamespace", "public", null, null, InputModelTypeUsage.RoundTrip, props, null, new List<InputModelType>(), null, null, null, false);
+            var modelTypeProvider = new ModelTypeProvider(inputModel, null);
+            var properties = modelTypeProvider.Properties;
+
+            Assert.IsNotNull(properties);
+            Assert.AreEqual(1, properties.Count);
+
+            // validate the setter
+            var prop1 = properties[0];
+            Assert.IsNotNull(prop1);
+
+            var autoPropertyBody = prop1.PropertyBody as AutoPropertyBody;
+            Assert.IsNotNull(autoPropertyBody);
+            Assert.AreEqual(hasSetter, autoPropertyBody?.HasSetter);
+        }
+
+        public static IEnumerable<TestCaseData> BuildProperties_ValidatePropertySettersTestCases
+        {
+            get
+            {
+                // list property
+                yield return new TestCaseData(
+                    new InputModelProperty("prop1", "prop1", "public", new InputList("mockProp", new InputPrimitiveType(InputTypeKind.String, false), false, false), false, false, false),
+                    new CSharpType(typeof(IList<string>)),
+                    false);
+                // read only list property
+                yield return new TestCaseData(
+                    new InputModelProperty("prop1", "prop1", "public", new InputList("mockProp", new InputPrimitiveType(InputTypeKind.String, false), false, false), false, true, false),
+                    new CSharpType(typeof(IReadOnlyList<string>)),
+                    false);
+                // nullable list property
+                yield return new TestCaseData(
+                    new InputModelProperty("prop1", "prop1", "public", new InputList("mockProp", new InputPrimitiveType(InputTypeKind.String, false), false, false), false, false, false),
+                    new CSharpType(typeof(IList<string>), true),
+                    true);
+                // dictionary property
+                yield return new TestCaseData(
+                    new InputModelProperty("prop1", "prop1", "public", new InputDictionary("mockProp", new InputPrimitiveType(InputTypeKind.String, false), new InputPrimitiveType(InputTypeKind.String, false), false), false, false, false),
+                    new CSharpType(typeof(IDictionary<string, string>)),
+                    false);
+                // nullable dictionary property
+                yield return new TestCaseData(
+                    new InputModelProperty("prop1", "prop1", "public", new InputDictionary("mockProp", new InputPrimitiveType(InputTypeKind.String, false), new InputPrimitiveType(InputTypeKind.String, false), false), false, false, false),
+                    new CSharpType(typeof(IDictionary<string, string>), true),
+                    true);
+                // primitive type property
+                yield return new TestCaseData(
+                    new InputModelProperty("prop1", "prop1", "public", new InputPrimitiveType(InputTypeKind.String, false), false, false, false),
+                    new CSharpType(typeof(string)),
+                    true);
+                // read only primitive type property
+                yield return new TestCaseData(
+                    new InputModelProperty("prop1", "prop1", "public", new InputPrimitiveType(InputTypeKind.String, false), false, true, false),
+                    new CSharpType(typeof(string)),
+                    false);
+                // readonlymemory property
+                yield return new TestCaseData(
+                    new InputModelProperty("prop1", "prop1", "public", new InputList("mockProp", new InputPrimitiveType(InputTypeKind.String, false), true, false), false, false, false),
+                    new CSharpType(typeof(ReadOnlyMemory<>)),
+                    true);
+            }
+        }
+    }
+}