Skip to content

Commit

Permalink
[http-client-csharp] Fix: remove setters for props in readonly structs (
Browse files Browse the repository at this point in the history
#4809)

Removes the invalid setters from auto-implemented properties in
read-only structs.

fixes: #4351
  • Loading branch information
jorgerangel-msft authored Oct 21, 2024
1 parent 1a27660 commit b33fe85
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ private PropertyProvider(InputModelProperty inputProperty, CSharpType propertyTy
{
propertyType = propertyType.WithNullable(true);
}

EnclosingType = enclosingType;
var serializationFormat = CodeModelPlugin.Instance.TypeFactory.GetSerializationFormat(inputProperty.Type);
var propHasSetter = PropertyHasSetter(propertyType, inputProperty);
MethodSignatureModifiers setterModifier = propHasSetter ? MethodSignatureModifiers.Public : MethodSignatureModifiers.None;
Expand All @@ -89,7 +91,6 @@ private PropertyProvider(InputModelProperty inputProperty, CSharpType propertyTy
XmlDocSummary = PropertyDescriptionBuilder.BuildPropertyDescription(inputProperty, propertyType, serializationFormat, Description);
XmlDocs = GetXmlDocs();
WireInfo = new PropertyWireInformation(inputProperty);
EnclosingType = enclosingType;
IsDiscriminator = inputProperty.IsDiscriminator;

InitializeParameter(Name, FormattableStringHelpers.FromString(inputProperty.Description) ?? FormattableStringHelpers.Empty, Type);
Expand Down Expand Up @@ -141,7 +142,6 @@ private XmlDocProvider GetXmlDocs()
/// <summary>
/// Returns true if the property has a setter.
/// </summary>
/// <param name="type">The <see cref="CSharpType"/> of the property.</param>
protected virtual bool PropertyHasSetter(CSharpType type, InputModelProperty inputProperty)
{
if (inputProperty.IsDiscriminator)
Expand All @@ -164,6 +164,11 @@ protected virtual bool PropertyHasSetter(CSharpType type, InputModelProperty inp
return false;
}

if (EnclosingType.DeclarationModifiers.HasFlag(TypeSignatureModifiers.Struct | TypeSignatureModifiers.ReadOnly))
{
return false;
}

if (type.IsCollection && !type.IsReadOnlyMemory)
{
return type.IsNullable;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public void PreVisitsProperties()
_mockVisitor.Object.Visit(_mockPlugin.Object.OutputLibrary);

_mockVisitor.Protected().Verify<TypeProvider>("Visit", Times.Once(), inputModel, ItExpr.Is<ModelProvider>(m => m.Name == new ModelProvider(inputModel).Name));
_mockVisitor.Protected().Verify<PropertyProvider>("Visit", Times.Once(), inputModelProperty, ItExpr.Is<PropertyProvider>(m => m.Name == new PropertyProvider(inputModelProperty, new TestTypeProvider()).Name));
_mockVisitor.Protected().Verify<PropertyProvider>("Visit", Times.Once(), inputModelProperty, ItExpr.Is<PropertyProvider>(m => m.Name == new PropertyProvider(inputModelProperty, TestTypeProvider.Empty).Name));
}

[Test]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,10 @@ public void CollectionProperty(CSharpType coreType, InputModelProperty collectio
}

[TestCaseSource(nameof(BodyHasSetterTestCases))]
public void BodyHasSetterValidation(string name, InputModelType inputModel, bool expectedHasSetter)
public void BodyHasSetterValidation(string name, InputModelType inputModel, bool expectedHasSetter, TypeSignatureModifiers? typeSignatureModifiers = null)
{
var collectionProperty = inputModel.Properties.Single();
var property = new PropertyProvider(collectionProperty, new TestTypeProvider());
var property = new PropertyProvider(collectionProperty, new TestTypeProvider(typeSignatureModifiers));

Assert.AreEqual(expectedHasSetter, property.Body.HasSetter);
}
Expand Down Expand Up @@ -123,47 +123,73 @@ private static IEnumerable<TestCaseData> BodyHasSetterTestCases()
yield return new TestCaseData(
"readOnlyString",
InputFactory.Model("TestModel", properties: [InputFactory.Property("readOnlyString", InputPrimitiveType.String, isRequired: true, isReadOnly: true)]),
false);
false,
null);
yield return new TestCaseData(
"readOnlyStringOnInputModel",
InputFactory.Model("TestModel", usage: InputModelTypeUsage.Input, properties: [InputFactory.Property("readOnlyString", InputPrimitiveType.Int32, isRequired: true, isReadOnly: true)]),
false);
false,
null);
yield return new TestCaseData(
"intOnInputModel",
InputFactory.Model("TestModel", usage: InputModelTypeUsage.Input, properties: [InputFactory.Property("intProperty", InputPrimitiveType.Int32, isRequired: true)]),
true);
true,
null);
yield return new TestCaseData(
"readOnlyCollectionOnOutputModel",
InputFactory.Model("TestModel", usage: InputModelTypeUsage.Output, properties: [InputFactory.Property("readOnlyCollection", new InputNullableType(InputFactory.Array(InputPrimitiveType.String)), isRequired: true, isReadOnly: true)]),
false);
false,
null);
yield return new TestCaseData(
"readOnlyCollectionOnInputOutputModel",
InputFactory.Model("TestModel", usage: InputModelTypeUsage.Input | InputModelTypeUsage.Output, properties: [InputFactory.Property("readOnlyCollection", new InputNullableType(InputFactory.Array(InputPrimitiveType.String)), isRequired: true, isReadOnly: true)]),
false);
false,
null);
yield return new TestCaseData(
"nullableCollectionOnOutputModel",
InputFactory.Model("TestModel", usage: InputModelTypeUsage.Output, properties: [InputFactory.Property("nullableCollection", new InputNullableType(InputFactory.Array(InputPrimitiveType.String)), isRequired: true, isReadOnly: false)]),
false);
false,
null);
yield return new TestCaseData(
"nullableCollectionOnInputModel",
InputFactory.Model("TestModel", usage: InputModelTypeUsage.Input, properties: [InputFactory.Property("nullableCollection", new InputNullableType(InputFactory.Array(InputPrimitiveType.String)), isRequired: true, isReadOnly: false)]),
true);
true,
null);
yield return new TestCaseData(
"readOnlyDictionaryOnOutputModel",
InputFactory.Model("TestModel", usage: InputModelTypeUsage.Output, properties: [InputFactory.Property("readOnlyDictionary", InputFactory.Dictionary(InputPrimitiveType.Int32), isRequired: true)]),
false);
false,
null);
yield return new TestCaseData(
"readOnlyDictionaryOnInputOutputModel",
InputFactory.Model("TestModel", usage: InputModelTypeUsage.Output | InputModelTypeUsage.Input, properties: [InputFactory.Property("readOnlyDictionary", InputFactory.Dictionary(InputPrimitiveType.Int32), isRequired: true)]),
false);
false,
null);
yield return new TestCaseData(
"nullableDictionaryOnOutputModel",
InputFactory.Model("TestModel", usage: InputModelTypeUsage.Output, properties: [InputFactory.Property("nullableDictionary", new InputNullableType(InputFactory.Dictionary(InputPrimitiveType.String)), isRequired: true, isReadOnly: false)]),
false);
false,
null);
yield return new TestCaseData(
"nullableDictionaryOnInputModel",
InputFactory.Model("TestModel", usage: InputModelTypeUsage.Input, properties: [InputFactory.Property("nullableDictionary", new InputNullableType(InputFactory.Dictionary(InputPrimitiveType.String)), isRequired: true, isReadOnly: false)]),
true);
true,
null);
yield return new TestCaseData(
"nonReadOnlyStringPropOnStruct",
InputFactory.Model("TestModel", usage: InputModelTypeUsage.Input, properties: [InputFactory.Property("nonReadOnlyString", InputPrimitiveType.String)], modelAsStruct: true),
true,
TypeSignatureModifiers.Struct);
yield return new TestCaseData(
"requiredReadOnlyStringPropOnStruct",
InputFactory.Model("TestModel", usage: InputModelTypeUsage.Input, properties: [InputFactory.Property("readOnlyString", InputPrimitiveType.String, isReadOnly: true, isRequired: true)], modelAsStruct: true),
false,
TypeSignatureModifiers.Struct);
yield return new TestCaseData(
"propInReadOnlyStruct",
InputFactory.Model("TestModel", usage: InputModelTypeUsage.Input, properties: [InputFactory.Property("nonReadOnlyString", InputPrimitiveType.String)], modelAsStruct: true),
false,
TypeSignatureModifiers.Struct | TypeSignatureModifiers.ReadOnly);
}
}
}
Original file line number Diff line number Diff line change
@@ -1,16 +1,25 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using Microsoft.Generator.CSharp.Primitives;
using Microsoft.Generator.CSharp.Providers;

namespace Microsoft.Generator.CSharp.Tests
{
internal class TestTypeProvider : TypeProvider
{
private readonly TypeSignatureModifiers? _declarationModifiers;
protected override string BuildRelativeFilePath() => $"{Name}.cs";

protected override string BuildName() => "TestName";

public static readonly TypeProvider Empty = new TestTypeProvider();

internal TestTypeProvider(TypeSignatureModifiers? declarationModifiers = null)
{
_declarationModifiers = declarationModifiers;
}

protected override TypeSignatureModifiers GetDeclarationModifiers() => _declarationModifiers ?? base.GetDeclarationModifiers();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@ internal TestModel(string requiredString, int requiredInt, global::System.Collec
}

/// <summary> Description for requiredString. </summary>
public string RequiredString { get; set; }
public string RequiredString { get; }

/// <summary> Description for requiredInt. </summary>
public int RequiredInt { get; set; }
public int RequiredInt { get; }
}
}

0 comments on commit b33fe85

Please sign in to comment.