Skip to content

Commit

Permalink
Remove virtual keyword from struct serial methods & Return Default Fo…
Browse files Browse the repository at this point in the history
…r 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: #4348,
#4349
  • Loading branch information
jorgerangel-msft authored Oct 8, 2024
1 parent 854024a commit e35a3c0
Show file tree
Hide file tree
Showing 4 changed files with 187 additions and 34 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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<Utf8JsonWriter>();
_mrwOptionsParameterSnippet = _serializationOptionsParameter.As<ModelReaderWriterOptions>();
_jsonElementParameterSnippet = _jsonElementDeserializationParam.As<JsonElement>();
Expand Down Expand Up @@ -304,7 +304,9 @@ internal MethodProvider BuildPersistableModelCreateMethodObjectDeclaration()
/// </summary>
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;
Expand All @@ -323,7 +325,10 @@ internal MethodProvider BuildJsonModelWriteCoreMethod()
/// </summary>
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;
Expand All @@ -344,7 +349,10 @@ internal MethodProvider BuildPersistableModelWriteCoreMethod()
/// </summary>
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;
Expand Down Expand Up @@ -378,7 +386,10 @@ internal MethodProvider BuildJsonModelCreateMethod()
/// </summary>
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;
Expand Down Expand Up @@ -545,9 +556,11 @@ private MethodBodyStatement[] BuildDeserializationMethodBody()
BuildDeserializePropertiesStatements(prop.As<JsonProperty>())
};

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()))
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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;
Expand Down Expand Up @@ -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();

Expand All @@ -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;
Expand Down Expand Up @@ -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();

Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// <auto-generated/>

#nullable disable

using System;
using System.ClientModel.Primitives;
using System.Collections.Generic;
using System.Text.Json;
using Sample;

namespace Sample.Models
{
/// <summary></summary>
public readonly partial struct TestModel : global::System.ClientModel.Primitives.IJsonModel<global::Sample.Models.TestModel>, global::System.ClientModel.Primitives.IJsonModel<object>
{
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<string, global::System.BinaryData> additionalBinaryDataProperties = new global::Sample.ChangeTrackingDictionary<string, global::System.BinaryData>();
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);
}
}
}

0 comments on commit e35a3c0

Please sign in to comment.