Skip to content

Commit

Permalink
fix(http-client-csharp): remove setters for model properties collecti…
Browse files Browse the repository at this point in the history 
…on types (microsoft#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 <[email protected]>
Co-authored-by: Dapeng Zhang <[email protected]>
Co-authored-by: ShivangiReja <[email protected]>
  • Loading branch information
@jorgerangel-msft @ShivangiReja @ArcturusZhang
4 people authored Apr 29, 2024
1 parent 32a25ec commit d3a4454
Show file tree
Hide file tree
Showing 4 changed files with 163 additions and 13 deletions.
16 changes: 8 additions & 8 deletions ...CSharp.ClientModel.TestProjects/Unbranded-TypeSpec/src/Generated/Models/RoundTripModel.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
Expand All @@ -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; }
Expand All @@ -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; }
Expand Down
4 changes: 2 additions & 2 deletions ...enerator.CSharp.ClientModel.TestProjects/Unbranded-TypeSpec/src/Generated/Models/Thing.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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; }
Expand Down
42 changes: 39 additions & 3 deletions ...-client-csharp/generator/Microsoft.Generator.CSharp/src/Models/Types/ModelTypeProvider.cs
View file
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -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)
Expand Down
114 changes: 114 additions & 0 deletions ...t-csharp/generator/Microsoft.Generator.CSharp/test/Models/Types/ModelTypeProviderTests.cs
View file
Original file line number Diff line number Diff line change
@@ -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);
}
}
}
}

0 comments on commit d3a4454

Please sign in to comment.