Skip to content

Commit db33781

Browse files
jorgerangel-msftjorgerangel-msft
authored
fix: implicitly mark discriminated model types as dynamic (#8933)
This PR fixes an issue where a dynamic model referencing a base model that was not dynamic would lead to generated code that would not compile. The dynamic model decorator will not be propagated to these base types only if the dynamic model subtype is a discriminated type. fixes: #8926
1 parent bc30a9f commit db33781

File tree

12 files changed

+960
-23
lines changed

12 files changed

+960
-23
lines changed

‎packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs‎

Lines changed: 44 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -684,7 +684,8 @@ private MethodBodyStatement[] BuildJsonModelWriteMethodBody(MethodProvider jsonM
684684

685685
private MethodBodyStatement[] BuildJsonModelWriteCoreMethodBody()
686686
{
687-
var propertiesStatements = CreateWritePropertiesStatements();
687+
bool isDynamicModelWithNonDynamicBase = _model is ScmModelProvider { IsDynamicModel: true } && _model.BaseModelProvider is ScmModelProvider { IsDynamicModel: false };
688+
var propertiesStatements = CreateWritePropertiesStatements(isDynamicModelWithNonDynamicBase);
688689
var additionalPropertiesStatements = CreateWriteAdditionalPropertiesStatement();
689690
List<MethodBodyStatement> writePropertiesStatements =
690691
[
@@ -720,7 +721,7 @@ private MethodBodyStatement[] BuildJsonModelWriteCoreMethodBody()
720721
return
721722
[
722723
CreateValidateJsonFormat(_persistableModelTInterface, WriteAction),
723-
CallBaseJsonModelWriteCore(),
724+
CallBaseJsonModelWriteCore(isDynamicModelWithNonDynamicBase),
724725
writePropertiesStatements,
725726
CreateWriteAdditionalRawDataStatement()
726727
];
@@ -853,10 +854,12 @@ private MethodBodyStatement[] BuildPersistableModelCreateCoreMethodBody()
853854
];
854855
}
855856

856-
private MethodBodyStatement CallBaseJsonModelWriteCore()
857+
private MethodBodyStatement CallBaseJsonModelWriteCore(bool isDynamicModelWithNonDynamicBase)
857858
{
858859
// base.<JsonModelWriteCore>()
859-
return _shouldOverrideMethods ?
860+
bool callBaseWriteMethod = _shouldOverrideMethods
861+
&& (_jsonPatchProperty is null || !isDynamicModelWithNonDynamicBase);
862+
return callBaseWriteMethod ?
860863
Base.Invoke(JsonModelWriteCoreMethodName, [_utf8JsonWriterParameter, _serializationOptionsParameter]).Terminate()
861864
: MethodBodyStatement.Empty;
862865
}
@@ -999,15 +1002,6 @@ private List<MethodBodyStatement> BuildDeserializePropertiesStatements(ScopedApi
9991002
propertyDeserializationStatements.Add(
10001003
_additionalBinaryDataProperty.Value.AsVariableExpression.AsDictionary(_additionalBinaryDataProperty.Value.Type).Add(jsonProperty.Name(), binaryDataDeserializationValue));
10011004
}
1002-
else if (rawBinaryData != null)
1003-
{
1004-
var elementType = rawBinaryData.Type.Arguments[1].FrameworkType;
1005-
var rawDataDeserializationValue = ScmCodeModelGenerator.Instance.TypeFactory.DeserializeJsonValue(elementType, jsonProperty.Value(), _dataParameter.As<BinaryData>(), _mrwOptionsParameterSnippet, SerializationFormat.Default);
1006-
propertyDeserializationStatements.Add(new IfStatement(_isNotEqualToWireConditionSnippet)
1007-
{
1008-
rawBinaryData.AsVariableExpression.AsDictionary(rawBinaryData.Type).Add(jsonProperty.Name(), rawDataDeserializationValue)
1009-
});
1010-
}
10111005
else if (_jsonPatchProperty != null)
10121006
{
10131007
// If we have a JsonPatch property, we want to add any unknown properties to the patch
@@ -1018,6 +1012,15 @@ private List<MethodBodyStatement> BuildDeserializePropertiesStatements(ScopedApi
10181012
propertyDeserializationStatements.Add(jsonPatchSet);
10191013
#pragma warning restore SCME0001 // Type is for evaluation purposes only and is subject to change or removal in future updates.
10201014
}
1015+
else if (rawBinaryData != null)
1016+
{
1017+
var elementType = rawBinaryData.Type.Arguments[1].FrameworkType;
1018+
var rawDataDeserializationValue = ScmCodeModelGenerator.Instance.TypeFactory.DeserializeJsonValue(elementType, jsonProperty.Value(), _dataParameter.As<BinaryData>(), _mrwOptionsParameterSnippet, SerializationFormat.Default);
1019+
propertyDeserializationStatements.Add(new IfStatement(_isNotEqualToWireConditionSnippet)
1020+
{
1021+
rawBinaryData.AsVariableExpression.AsDictionary(rawBinaryData.Type).Add(jsonProperty.Name(), rawDataDeserializationValue)
1022+
});
1023+
}
10211024

10221025
return propertyDeserializationStatements;
10231026
}
@@ -1530,10 +1533,37 @@ private static MethodBodyStatement ThrowValidationFailException(ValueExpression
15301533
/// <summary>
15311534
/// Constructs the body statements for the JsonModelWriteCore method containing the serialization for the model properties.
15321535
/// </summary>
1533-
private MethodBodyStatement[] CreateWritePropertiesStatements()
1536+
private MethodBodyStatement[] CreateWritePropertiesStatements(bool isDynamicModelWithNonDynamicBase)
15341537
{
15351538
List<MethodBodyStatement> propertyStatements = new();
15361539

1540+
if (isDynamicModelWithNonDynamicBase)
1541+
{
1542+
var baseModelProvider = _model.BaseModelProvider;
1543+
while (baseModelProvider != null)
1544+
{
1545+
foreach (var property in baseModelProvider.CanonicalView.Properties)
1546+
{
1547+
if (property.WireInfo == null || property.WireInfo.IsHttpMetadata)
1548+
{
1549+
continue;
1550+
}
1551+
propertyStatements.Add(CreateWritePropertyStatement(property.WireInfo, property.Type, property.Name, property));
1552+
}
1553+
1554+
foreach (var field in baseModelProvider.CanonicalView.Fields)
1555+
{
1556+
if (field.WireInfo == null || field.WireInfo.IsHttpMetadata)
1557+
{
1558+
continue;
1559+
}
1560+
propertyStatements.Add(CreateWritePropertyStatement(field.WireInfo, field.Type, field.Name, field));
1561+
}
1562+
1563+
baseModelProvider = baseModelProvider.BaseModelProvider;
1564+
}
1565+
}
1566+
15371567
// we should only write those properties with wire info and are payload properties.
15381568
// Those properties without wireinfo indicate they are not spec properties.
15391569
foreach (var property in _model.CanonicalView.Properties)

‎packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ScmModelProvider.cs‎

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ namespace Microsoft.TypeSpec.Generator.ClientModel.Providers
2121
{
2222
public sealed class ScmModelProvider : ModelProvider
2323
{
24+
private readonly InputModelType _inputModel;
2425
private const string JsonPatchFieldName = "_patch";
2526
#pragma warning disable SCME0001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
2627
private readonly CSharpType _jsonPatchFieldType = typeof(JsonPatch);
@@ -44,6 +45,7 @@ public sealed class ScmModelProvider : ModelProvider
4445

4546
public ScmModelProvider(InputModelType inputModel) : base(inputModel)
4647
{
48+
_inputModel = inputModel;
4749
IsDynamicModel = inputModel.IsDynamicModel;
4850
BaseJsonPatchProperty = new(GetBaseJsonPatchProperty());
4951
}
@@ -111,17 +113,21 @@ protected override ConstructorProvider[] BuildConstructors()
111113
if (constructor.BodyStatements != null)
112114
{
113115
List<MethodBodyStatement> updatedBody = [];
114-
foreach (var statement in constructor.BodyStatements)
116+
if (RawDataField is null)
115117
{
116-
// Remove RawDataField assignment if it exists
117-
if (RawDataField != null &&
118-
statement is ExpressionStatement expressionStatement &&
119-
expressionStatement.Expression is AssignmentExpression assignmentExpression &&
120-
assignmentExpression.Value == RawDataField.AsParameter == true)
118+
updatedBody.AddRange(constructor.BodyStatements);
119+
}
120+
else
121+
{
122+
foreach (var statement in constructor.BodyStatements)
121123
{
122-
continue;
124+
if (statement is ExpressionStatement { Expression: AssignmentExpression assignmentExpression }
125+
&& assignmentExpression.Value.Equals(RawDataField.AsParameter))
126+
{
127+
continue;
128+
}
129+
updatedBody.Add(statement);
123130
}
124-
updatedBody.Add(statement);
125131
}
126132

127133
if (JsonPatchField != null)
@@ -268,6 +274,10 @@ private void UpdateFullConstructorParameters()
268274
currentProvider = currentProvider.BaseModelProvider;
269275
}
270276

277+
bool isDiscriminatedType = currentProvider is ScmModelProvider { DiscriminatorValue: not null }
278+
|| currentProvider is ScmModelProvider { _inputModel.DiscriminatorProperty: not null };
279+
bool hasDynamicModelSupport = currentProvider is ScmModelProvider { IsDynamicModel: true };
280+
271281
if (baseRawDataField != null)
272282
{
273283
var updatedArguments = new List<ValueExpression>(FullConstructor.Signature.Initializer.Arguments.Count);
@@ -276,7 +286,10 @@ private void UpdateFullConstructorParameters()
276286
VariableExpression rawDataFieldAsVar = baseRawDataField.AsParameter;
277287
if (rawDataFieldAsVar.Equals(argument))
278288
{
279-
updatedArguments.Add(jsonPatchParameter);
289+
var replacement = !isDiscriminatedType && !hasDynamicModelSupport
290+
? Default
291+
: jsonPatchParameter;
292+
updatedArguments.Add(replacement);
280293
}
281294
else
282295
{

‎packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/MrwSerializationTypeDefinitions/DynamicModelSerializationTests.cs‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1398,5 +1398,93 @@ public void DerivedModelWithoutDynamicPropertiesButBaseHasDynamicProperties()
13981398
StringAssert.DoesNotContain("SetPropagators", derivedConstructor.BodyStatements!.ToDisplayString(), "Derived model constructor should not call SetPropagators when it has no dynamic properties");
13991399
}
14001400
}
1401+
1402+
[Test]
1403+
public void WriteDynamicDerivedModelWithNonDiscriminatedBase()
1404+
{
1405+
var baseModel = InputFactory.Model(
1406+
"animal",
1407+
isDynamicModel: false,
1408+
properties:
1409+
[
1410+
InputFactory.Property("species", InputPrimitiveType.String, isRequired: true)
1411+
]);
1412+
1413+
var dynamicDerivedModel = InputFactory.Model(
1414+
"dog",
1415+
isDynamicModel: true,
1416+
baseModel: baseModel,
1417+
properties:
1418+
[
1419+
InputFactory.Property("barks", InputPrimitiveType.Boolean, isRequired: true)
1420+
]);
1421+
1422+
MockHelpers.LoadMockGenerator(inputModels: () => [baseModel, dynamicDerivedModel]);
1423+
1424+
// Verify base model is NOT dynamic
1425+
var baseModelProvider = ScmCodeModelGenerator.Instance.TypeFactory.CreateModel(baseModel) as ClientModel.Providers.ScmModelProvider;
1426+
Assert.IsNotNull(baseModelProvider);
1427+
Assert.IsFalse(baseModelProvider!.IsDynamicModel, "Base model should NOT be dynamic");
1428+
1429+
// Get the derived model and validate it's dynamic
1430+
var derivedModelProvider = ScmCodeModelGenerator.Instance.TypeFactory.CreateModel(dynamicDerivedModel) as ClientModel.Providers.ScmModelProvider;
1431+
Assert.IsNotNull(derivedModelProvider);
1432+
Assert.IsTrue(derivedModelProvider!.IsDynamicModel, "Derived model should be dynamic");
1433+
Assert.IsTrue(derivedModelProvider.Constructors.Count > 0);
1434+
1435+
var serialization = derivedModelProvider.SerializationProviders.SingleOrDefault();
1436+
Assert.IsNotNull(serialization);
1437+
1438+
var writer = new TypeProviderWriter(new FilteredMethodsTypeProvider(
1439+
serialization!,
1440+
name => name is "JsonModelWriteCore" or "Write"));
1441+
1442+
var file = writer.Write();
1443+
Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content);
1444+
}
1445+
1446+
[Test]
1447+
public void DeserializeDynamicDerivedModelWithNonDiscriminatedBase()
1448+
{
1449+
var baseModel = InputFactory.Model(
1450+
"animal",
1451+
isDynamicModel: false,
1452+
properties:
1453+
[
1454+
InputFactory.Property("species", InputPrimitiveType.String, isRequired: true)
1455+
]);
1456+
1457+
var dynamicDerivedModel = InputFactory.Model(
1458+
"dog",
1459+
isDynamicModel: true,
1460+
baseModel: baseModel,
1461+
properties:
1462+
[
1463+
InputFactory.Property("barks", InputPrimitiveType.Boolean, isRequired: true)
1464+
]);
1465+
1466+
MockHelpers.LoadMockGenerator(inputModels: () => [baseModel, dynamicDerivedModel]);
1467+
1468+
// Verify base model is NOT dynamic
1469+
var baseModelProvider = ScmCodeModelGenerator.Instance.TypeFactory.CreateModel(baseModel) as ClientModel.Providers.ScmModelProvider;
1470+
Assert.IsNotNull(baseModelProvider);
1471+
Assert.IsFalse(baseModelProvider!.IsDynamicModel, "Base model should NOT be dynamic");
1472+
1473+
// Get the derived model and validate it's dynamic
1474+
var derivedModelProvider = ScmCodeModelGenerator.Instance.TypeFactory.CreateModel(dynamicDerivedModel) as ClientModel.Providers.ScmModelProvider;
1475+
Assert.IsNotNull(derivedModelProvider);
1476+
Assert.IsTrue(derivedModelProvider!.IsDynamicModel, "Derived model should be dynamic");
1477+
Assert.IsTrue(derivedModelProvider.Constructors.Count > 0);
1478+
1479+
var serialization = derivedModelProvider.SerializationProviders.SingleOrDefault();
1480+
Assert.IsNotNull(serialization);
1481+
1482+
var writer = new TypeProviderWriter(new FilteredMethodsTypeProvider(
1483+
serialization!,
1484+
name => name.StartsWith("Deserialize")));
1485+
1486+
var file = writer.Write();
1487+
Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content);
1488+
}
14011489
}
14021490
}

‎packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/MrwSerializationTypeDefinitions/TestData/DynamicModelSerializationTests/DeserializeDynamicDerivedModelWithNonDiscriminatedBase.cs‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
// <auto-generated/>
2+
3+
#nullable disable
4+
5+
using System;
6+
using System.ClientModel.Primitives;
7+
using System.Text;
8+
using System.Text.Json;
9+
using Sample.Models;
10+
11+
namespace Sample
12+
{
13+
public partial class Dog
14+
{
15+
internal static global::Sample.Models.Dog DeserializeDog(global::System.Text.Json.JsonElement element, global::System.BinaryData data, global::System.ClientModel.Primitives.ModelReaderWriterOptions options)
16+
{
17+
if ((element.ValueKind == global::System.Text.Json.JsonValueKind.Null))
18+
{
19+
return null;
20+
}
21+
string species = default;
22+
#pragma warning disable SCME0001 // Type is for evaluation purposes only and is subject to change or removal in future updates.
23+
global::System.ClientModel.Primitives.JsonPatch patch = new global::System.ClientModel.Primitives.JsonPatch((data is null) ? global::System.ReadOnlyMemory<byte>.Empty : data.ToMemory());
24+
#pragma warning restore SCME0001 // Type is for evaluation purposes only and is subject to change or removal in future updates.
25+
bool barks = default;
26+
foreach (var prop in element.EnumerateObject())
27+
{
28+
if (prop.NameEquals("species"u8))
29+
{
30+
species = prop.Value.GetString();
31+
continue;
32+
}
33+
if (prop.NameEquals("barks"u8))
34+
{
35+
barks = prop.Value.GetBoolean();
36+
continue;
37+
}
38+
patch.Set([.. "$."u8, .. global::System.Text.Encoding.UTF8.GetBytes(prop.Name)], prop.Value.GetUtf8Bytes());
39+
}
40+
return new global::Sample.Models.Dog(species, patch, barks);
41+
}
42+
}
43+
}

‎packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/MrwSerializationTypeDefinitions/TestData/DynamicModelSerializationTests/WriteDynamicDerivedModelWithNonDiscriminatedBase.cs‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
// <auto-generated/>
2+
3+
#nullable disable
4+
5+
using System;
6+
using System.ClientModel.Primitives;
7+
using System.Text.Json;
8+
using Sample.Models;
9+
10+
namespace Sample
11+
{
12+
public partial class Dog
13+
{
14+
void global::System.ClientModel.Primitives.IJsonModel<global::Sample.Models.Dog>.Write(global::System.Text.Json.Utf8JsonWriter writer, global::System.ClientModel.Primitives.ModelReaderWriterOptions options)
15+
{
16+
#pragma warning disable SCME0001 // Type is for evaluation purposes only and is subject to change or removal in future updates.
17+
if (Patch.Contains("$"u8))
18+
{
19+
writer.WriteRawValue(Patch.GetJson("$"u8));
20+
return;
21+
}
22+
#pragma warning restore SCME0001 // Type is for evaluation purposes only and is subject to change or removal in future updates.
23+
24+
writer.WriteStartObject();
25+
this.JsonModelWriteCore(writer, options);
26+
writer.WriteEndObject();
27+
}
28+
29+
protected override void JsonModelWriteCore(global::System.Text.Json.Utf8JsonWriter writer, global::System.ClientModel.Primitives.ModelReaderWriterOptions options)
30+
{
31+
string format = (options.Format == "W") ? ((global::System.ClientModel.Primitives.IPersistableModel<global::Sample.Models.Dog>)this).GetFormatFromOptions(options) : options.Format;
32+
if ((format != "J"))
33+
{
34+
throw new global::System.FormatException($"The model {nameof(global::Sample.Models.Dog)} does not support writing '{format}' format.");
35+
}
36+
#pragma warning disable SCME0001 // Type is for evaluation purposes only and is subject to change or removal in future updates.
37+
if (!Patch.Contains("$.species"u8))
38+
{
39+
writer.WritePropertyName("species"u8);
40+
writer.WriteStringValue(Species);
41+
}
42+
if (!Patch.Contains("$.barks"u8))
43+
{
44+
writer.WritePropertyName("barks"u8);
45+
writer.WriteBooleanValue(Barks);
46+
}
47+
48+
Patch.WriteTo(writer);
49+
#pragma warning restore SCME0001 // Type is for evaluation purposes only and is subject to change or removal in future updates.
50+
}
51+
52+
global::System.BinaryData global::System.ClientModel.Primitives.IPersistableModel<global::Sample.Models.Dog>.Write(global::System.ClientModel.Primitives.ModelReaderWriterOptions options) => this.PersistableModelWriteCore(options);
53+
}
54+
}

0 commit comments

Comments
 (0)