Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(http-client-csharp): remove setters for model properties collection types #3233

Merged
merged 5 commits into from
Apr 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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
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";
ShivangiReja marked this conversation as resolved.
Show resolved Hide resolved

/// <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
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
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);
}
}
}
}