From 7aee12fa439e83c544c932b196e2daaebc151ae9 Mon Sep 17 00:00:00 2001 From: Jorge Rangel <102122018+jorgerangel-msft@users.noreply.github.com> Date: Tue, 8 Oct 2024 14:15:39 -0500 Subject: [PATCH] Remove virtual keyword from struct serial methods & Return Default For Null Elements (#4630) This PR fixes several issues with generated structs: - The `protected virtual` modifiers were removed from the serialization core methods as the modifiers are not valid for structs. - When deserializing the struct, `default` is returned rather than `null` when encountering an JsonElement that is Null. fixes: https://github.com/microsoft/typespec/issues/4348, https://github.com/microsoft/typespec/issues/4349 --- .../MrwSerializationTypeDefinition.cs | 25 +++-- .../DeserializationTests.cs | 50 +++++++++ .../MrwSerializationTypeDefinitionTests.cs | 102 +++++++++++++----- .../DeserializationTests/DeserializeStruct.cs | 44 ++++++++ 4 files changed, 187 insertions(+), 34 deletions(-) create mode 100644 packages/http-client-csharp/generator/Microsoft.Generator.CSharp.ClientModel/test/Providers/MrwSerializationTypeDefinitions/DeserializationTests.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.Generator.CSharp.ClientModel/test/Providers/MrwSerializationTypeDefinitions/TestData/DeserializationTests/DeserializeStruct.cs diff --git a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs index c67842c302..7399051b5e 100644 --- a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs +++ b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs @@ -72,7 +72,7 @@ public MrwSerializationTypeDefinition(InputModelType inputModel, ModelProvider m _rawDataField = _model.Fields.FirstOrDefault(f => f.Name == AdditionalPropertiesHelper.AdditionalBinaryDataPropsFieldName); _additionalBinaryDataProperty = GetAdditionalBinaryDataPropertiesProp(); _additionalProperties = new([.. _model.Properties.Where(p => p.IsAdditionalProperties)]); - _shouldOverrideMethods = _model.Type.BaseType != null && _model.Type.BaseType is { IsFrameworkType: false }; + _shouldOverrideMethods = _model.Type.BaseType != null && !_isStruct && _model.Type.BaseType is { IsFrameworkType: false }; _utf8JsonWriterSnippet = _utf8JsonWriterParameter.As(); _mrwOptionsParameterSnippet = _serializationOptionsParameter.As(); _jsonElementParameterSnippet = _jsonElementDeserializationParam.As(); @@ -304,7 +304,9 @@ internal MethodProvider BuildPersistableModelCreateMethodObjectDeclaration() /// internal MethodProvider BuildJsonModelWriteCoreMethod() { - MethodSignatureModifiers modifiers = MethodSignatureModifiers.Protected | MethodSignatureModifiers.Virtual; + MethodSignatureModifiers modifiers = _isStruct + ? MethodSignatureModifiers.Private + : MethodSignatureModifiers.Protected | MethodSignatureModifiers.Virtual; if (_shouldOverrideMethods) { modifiers = MethodSignatureModifiers.Protected | MethodSignatureModifiers.Override; @@ -323,7 +325,10 @@ internal MethodProvider BuildJsonModelWriteCoreMethod() /// internal MethodProvider BuildPersistableModelWriteCoreMethod() { - MethodSignatureModifiers modifiers = MethodSignatureModifiers.Protected | MethodSignatureModifiers.Virtual; + MethodSignatureModifiers modifiers = _isStruct + ? MethodSignatureModifiers.Private + : MethodSignatureModifiers.Protected | MethodSignatureModifiers.Virtual; + if (_shouldOverrideMethods) { modifiers = MethodSignatureModifiers.Protected | MethodSignatureModifiers.Override; @@ -344,7 +349,10 @@ internal MethodProvider BuildPersistableModelWriteCoreMethod() /// internal MethodProvider BuildPersistableModelCreateCoreMethod() { - MethodSignatureModifiers modifiers = MethodSignatureModifiers.Protected | MethodSignatureModifiers.Virtual; + MethodSignatureModifiers modifiers = _isStruct + ? MethodSignatureModifiers.Private + : MethodSignatureModifiers.Protected | MethodSignatureModifiers.Virtual; + if (_shouldOverrideMethods) { modifiers = MethodSignatureModifiers.Protected | MethodSignatureModifiers.Override; @@ -378,7 +386,10 @@ internal MethodProvider BuildJsonModelCreateMethod() /// internal MethodProvider BuildJsonModelCreateCoreMethod() { - MethodSignatureModifiers modifiers = MethodSignatureModifiers.Protected | MethodSignatureModifiers.Virtual; + MethodSignatureModifiers modifiers = _isStruct + ? MethodSignatureModifiers.Private + : MethodSignatureModifiers.Protected | MethodSignatureModifiers.Virtual; + if (_shouldOverrideMethods) { modifiers = MethodSignatureModifiers.Protected | MethodSignatureModifiers.Override; @@ -545,9 +556,11 @@ private MethodBodyStatement[] BuildDeserializationMethodBody() BuildDeserializePropertiesStatements(prop.As()) }; + var valueKindEqualsNullReturn = _isStruct ? Return(Default) : Return(Null); + return [ - new IfStatement(_jsonElementParameterSnippet.ValueKindEqualsNull()) { Return(Null) }, + new IfStatement(_jsonElementParameterSnippet.ValueKindEqualsNull()) { valueKindEqualsNullReturn }, GetPropertyVariableDeclarations(), deserializePropertiesForEachStatement, Return(New.Instance(_model.Type, GetSerializationCtorParameterValues())) diff --git a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.ClientModel/test/Providers/MrwSerializationTypeDefinitions/DeserializationTests.cs b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.ClientModel/test/Providers/MrwSerializationTypeDefinitions/DeserializationTests.cs new file mode 100644 index 0000000000..9153ab2c9f --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.ClientModel/test/Providers/MrwSerializationTypeDefinitions/DeserializationTests.cs @@ -0,0 +1,50 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System.Linq; +using Microsoft.Generator.CSharp.ClientModel.Providers; +using Microsoft.Generator.CSharp.Input; +using Microsoft.Generator.CSharp.Primitives; +using Microsoft.Generator.CSharp.Providers; +using Microsoft.Generator.CSharp.Tests.Common; +using NUnit.Framework; + +namespace Microsoft.Generator.CSharp.ClientModel.Tests.Providers.MrwSerializationTypeDefinitions +{ + public class DeserializationTests + { + public DeserializationTests() + { + MockHelpers.LoadMockPlugin(createSerializationsCore: (inputType, typeProvider) + => inputType is InputModelType modeltype ? [new MockMrwProvider(modeltype, (typeProvider as ModelProvider)!)] : []); + } + + private class MockMrwProvider : MrwSerializationTypeDefinition + { + public MockMrwProvider(InputModelType inputModel, ModelProvider modelProvider) + : base(inputModel, modelProvider) + { + } + + protected override MethodProvider[] BuildMethods() + { + return [.. base.BuildMethods().Where(m => m.Signature.Name.StartsWith("Deserialize"))]; + } + + protected override FieldProvider[] BuildFields() => []; + protected override ConstructorProvider[] BuildConstructors() => []; + } + + [Test] + public void DeserializeStruct() + { + var inputModel = InputFactory.Model("TestModel", modelAsStruct: true); + + var mrwProvider = new ModelProvider(inputModel).SerializationProviders.FirstOrDefault(); + Assert.IsNotNull(mrwProvider); + var writer = new TypeProviderWriter(mrwProvider!); + var file = writer.Write(); + Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content); + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.ClientModel/test/Providers/MrwSerializationTypeDefinitions/MrwSerializationTypeDefinitionTests.cs b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.ClientModel/test/Providers/MrwSerializationTypeDefinitions/MrwSerializationTypeDefinitionTests.cs index 744283ab88..126c6e3f52 100644 --- a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.ClientModel/test/Providers/MrwSerializationTypeDefinitions/MrwSerializationTypeDefinitionTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.ClientModel/test/Providers/MrwSerializationTypeDefinitions/MrwSerializationTypeDefinitionTests.cs @@ -57,10 +57,11 @@ public void TestBuildImplements() } // This test validates the json model serialization write method is built correctly - [Test] - public void TestBuildJsonModelWriteCoreMethod() + [TestCase(true)] + [TestCase(false)] + public void TestBuildJsonModelWriteCoreMethod(bool isStruct) { - var inputModel = InputFactory.Model("mockInputModel"); + var inputModel = InputFactory.Model("mockInputModel", modelAsStruct: isStruct); var (model, serialization) = CreateModelAndSerialization(inputModel); var method = serialization.BuildJsonModelWriteCoreMethod(); @@ -74,17 +75,25 @@ public void TestBuildJsonModelWriteCoreMethod() Assert.IsNull(methodSignature?.ReturnType); // Check method modifiers - var expectedModifiers = MethodSignatureModifiers.Protected; - if (model.Type.BaseType != null) + MethodSignatureModifiers expectedModifiers; + if (isStruct) { - expectedModifiers |= MethodSignatureModifiers.Override; + expectedModifiers = MethodSignatureModifiers.Private; } else { - expectedModifiers |= MethodSignatureModifiers.Virtual; + expectedModifiers = MethodSignatureModifiers.Protected; + if (model.Type.BaseType != null) + { + expectedModifiers |= MethodSignatureModifiers.Override; + } + else + { + expectedModifiers |= MethodSignatureModifiers.Virtual; + } } - Assert.AreEqual(expectedModifiers, methodSignature?.Modifiers, "Method modifiers do not match the expected value."); + Assert.AreEqual(expectedModifiers, methodSignature?.Modifiers, "Method modifiers do not match the expected value."); // Validate body var methodBody = method?.BodyStatements; @@ -146,10 +155,11 @@ public void TestBuildJsonModelCreateMethod() } // This test validates the json model serialization create core method is built correctly - [Test] - public void TestBuildJsonModelCreateCoreMethod() + [TestCase(true)] + [TestCase(false)] + public void TestBuildJsonModelCreateCoreMethod(bool isStruct) { - var inputModel = InputFactory.Model("mockInputModel"); + var inputModel = InputFactory.Model("mockInputModel", modelAsStruct: isStruct); var (model, serialization) = CreateModelAndSerialization(inputModel); var method = serialization.BuildJsonModelCreateCoreMethod(); @@ -163,17 +173,25 @@ public void TestBuildJsonModelCreateCoreMethod() Assert.AreEqual(model.Type, methodSignature?.ReturnType); // Check method modifiers - var expectedModifiers = MethodSignatureModifiers.Protected; - if (model.Type.BaseType != null) + MethodSignatureModifiers expectedModifiers; + if (isStruct) { - expectedModifiers |= MethodSignatureModifiers.Override; + expectedModifiers = MethodSignatureModifiers.Private; } else { - expectedModifiers |= MethodSignatureModifiers.Virtual; + expectedModifiers = MethodSignatureModifiers.Protected; + if (model.Type.BaseType != null) + { + expectedModifiers |= MethodSignatureModifiers.Override; + } + else + { + expectedModifiers |= MethodSignatureModifiers.Virtual; + } } - Assert.AreEqual(expectedModifiers, methodSignature?.Modifiers, "Method modifiers do not match the expected value."); + Assert.AreEqual(expectedModifiers, methodSignature?.Modifiers, "Method modifiers do not match the expected value."); // Validate body var methodBody = method?.BodyStatements; @@ -236,10 +254,11 @@ public void TestBuildPersistableModelWriteMethod() } // This test validates the persistable model serialization write core method is built correctly - [Test] - public void TestBuildPersistableModelWriteCoreMethod() + [TestCase(true)] + [TestCase(false)] + public void TestBuildPersistableModelWriteCoreMethod(bool isStruct) { - var inputModel = InputFactory.Model("mockInputModel"); + var inputModel = InputFactory.Model("mockInputModel", modelAsStruct: isStruct); var (model, serialization) = CreateModelAndSerialization(inputModel); var method = serialization.BuildPersistableModelWriteCoreMethod(); @@ -253,17 +272,26 @@ public void TestBuildPersistableModelWriteCoreMethod() Assert.AreEqual(new CSharpType(typeof(BinaryData)), methodSignature?.ReturnType); // Check method modifiers - var expectedModifiers = MethodSignatureModifiers.Protected; - if (model.Type.BaseType != null) + // Check method modifiers + MethodSignatureModifiers expectedModifiers; + if (isStruct) { - expectedModifiers |= MethodSignatureModifiers.Override; + expectedModifiers = MethodSignatureModifiers.Private; } else { - expectedModifiers |= MethodSignatureModifiers.Virtual; + expectedModifiers = MethodSignatureModifiers.Protected; + if (model.Type.BaseType != null) + { + expectedModifiers |= MethodSignatureModifiers.Override; + } + else + { + expectedModifiers |= MethodSignatureModifiers.Virtual; + } } - Assert.AreEqual(expectedModifiers, methodSignature?.Modifiers, "Method modifiers do not match the expected value."); + Assert.AreEqual(expectedModifiers, methodSignature?.Modifiers, "Method modifiers do not match the expected value."); // Validate body var methodBody = method?.BodyStatements; @@ -335,10 +363,11 @@ public void TestBuildPersistableModelCreateMethod() } // This test validates the persistable model serialization create core method is built correctly - [Test] - public void TestBuildPersistableModelCreateCoreMethod() + [TestCase(true)] + [TestCase(false)] + public void TestBuildPersistableModelCreateCoreMethod(bool isStruct) { - var inputModel = InputFactory.Model("mockInputModel"); + var inputModel = InputFactory.Model("mockInputModel", modelAsStruct: isStruct); var (model, serialization) = CreateModelAndSerialization(inputModel); Assert.IsNotNull(serialization); @@ -355,7 +384,24 @@ public void TestBuildPersistableModelCreateCoreMethod() Assert.AreEqual(model.Type, methodSignature?.ReturnType); // Check method modifiers - var expectedModifiers = MethodSignatureModifiers.Protected | MethodSignatureModifiers.Virtual; + MethodSignatureModifiers expectedModifiers; + if (isStruct) + { + expectedModifiers = MethodSignatureModifiers.Private; + } + else + { + expectedModifiers = MethodSignatureModifiers.Protected; + if (model.Type.BaseType != null) + { + expectedModifiers |= MethodSignatureModifiers.Override; + } + else + { + expectedModifiers |= MethodSignatureModifiers.Virtual; + } + } + Assert.AreEqual(expectedModifiers, methodSignature?.Modifiers, "Method modifiers do not match the expected value."); // Validate body diff --git a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.ClientModel/test/Providers/MrwSerializationTypeDefinitions/TestData/DeserializationTests/DeserializeStruct.cs b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.ClientModel/test/Providers/MrwSerializationTypeDefinitions/TestData/DeserializationTests/DeserializeStruct.cs new file mode 100644 index 0000000000..161c68dde7 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.ClientModel/test/Providers/MrwSerializationTypeDefinitions/TestData/DeserializationTests/DeserializeStruct.cs @@ -0,0 +1,44 @@ +// + +#nullable disable + +using System; +using System.ClientModel.Primitives; +using System.Collections.Generic; +using System.Text.Json; +using Sample; + +namespace Sample.Models +{ + /// + public readonly partial struct TestModel : global::System.ClientModel.Primitives.IJsonModel, global::System.ClientModel.Primitives.IJsonModel + { + internal static global::Sample.Models.TestModel DeserializeTestModel(global::System.Text.Json.JsonElement element, global::System.ClientModel.Primitives.ModelReaderWriterOptions options) + { + if ((element.ValueKind == global::System.Text.Json.JsonValueKind.Null)) + { + return default; + } + string stringProperty = default; + global::System.Collections.Generic.IDictionary additionalBinaryDataProperties = new global::Sample.ChangeTrackingDictionary(); + foreach (var prop in element.EnumerateObject()) + { + if (prop.NameEquals("stringProperty"u8)) + { + if ((prop.Value.ValueKind == global::System.Text.Json.JsonValueKind.Null)) + { + stringProperty = null; + continue; + } + stringProperty = prop.Value.GetString(); + continue; + } + if ((options.Format != "W")) + { + additionalBinaryDataProperties.Add(prop.Name, global::System.BinaryData.FromString(prop.Value.GetRawText())); + } + } + return new global::Sample.Models.TestModel(stringProperty, additionalBinaryDataProperties); + } + } +}