From b7d351b35e319f9558d4260535e184d01779b2a9 Mon Sep 17 00:00:00 2001 From: Jorge Rangel <102122018+jorgerangel-msft@users.noreply.github.com> Date: Fri, 1 Nov 2024 15:11:07 -0500 Subject: [PATCH] [http-client-csharp] fix: gen. unknown discriminator model when discriminator prop exists (#4936) fixes: https://github.com/microsoft/typespec/issues/4935 --- .../MrwSerializationTypeDefinition.cs | 27 ++++++- .../src/InputTypes/InputModelType.cs | 55 ++++++------- .../TypeSpecInputModelTypeConverter.cs | 4 + .../ModelFactoryProviderTests.cs | 2 +- .../ModelProviders/DiscriminatorTests.cs | 78 ++++++++++++++++++- 5 files changed, 133 insertions(+), 33 deletions(-) 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 08b9cdc74c..135d9a5391 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 @@ -442,17 +442,36 @@ internal MethodProvider BuildDeserializationMethod() private MethodBodyStatement[] BuildAbstractDeserializationMethodBody() { var unknownVariant = _model.DerivedModels.First(m => m.IsUnknownDiscriminatorModel); + bool onlyContainsUnknownDerivedModel = _model.DerivedModels.Count == 1; + var deserializeDiscriminatedModelsConditions = BuildDiscriminatedModelsCondition( + GetAbstractSwitchCases(unknownVariant), + onlyContainsUnknownDerivedModel, + _jsonElementParameterSnippet); + return [ new IfStatement(_jsonElementParameterSnippet.ValueKindEqualsNull()) { Return(Null) }, - new IfStatement(_jsonElementParameterSnippet.TryGetProperty("kind", out var discriminator)) - { - new SwitchStatement(discriminator.GetString(), GetAbstractSwitchCases(unknownVariant)) - }, + deserializeDiscriminatedModelsConditions, Return(unknownVariant.Type.Deserialize(_jsonElementParameterSnippet, _serializationOptionsParameter)) ]; } + private static MethodBodyStatement BuildDiscriminatedModelsCondition( + SwitchCaseStatement[] abstractSwitchCases, + bool onlyContainsUnknownDerivedModel, + ScopedApi jsonElementParameterSnippet) + { + if (!onlyContainsUnknownDerivedModel) + { + return new IfStatement(jsonElementParameterSnippet.TryGetProperty("kind", out var discriminator)) + { + new SwitchStatement(discriminator.GetString(), abstractSwitchCases) + }; + } + + return MethodBodyStatement.Empty; + } + private SwitchCaseStatement[] GetAbstractSwitchCases(ModelProvider unknownVariant) { SwitchCaseStatement[] cases = new SwitchCaseStatement[_model.DerivedModels.Count - 1]; diff --git a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.Input/src/InputTypes/InputModelType.cs b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.Input/src/InputTypes/InputModelType.cs index bdac4f9795..6c0c800c9c 100644 --- a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.Input/src/InputTypes/InputModelType.cs +++ b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.Input/src/InputTypes/InputModelType.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +using System; using System.Collections.Generic; using System.Diagnostics; @@ -9,6 +10,7 @@ namespace Microsoft.Generator.CSharp.Input [DebuggerDisplay("{GetDebuggerDisplay(),nq}")] public class InputModelType : InputType { + private const string UnknownDiscriminatorValue = "unknown"; private IReadOnlyList _properties = []; private IList _derivedModels = []; @@ -38,7 +40,7 @@ public InputModelType(string name, string crossLanguageDefinitionId, string? acc DiscriminatorProperty = discriminatorProperty; DiscriminatedSubtypes = discriminatedSubtypes!; AdditionalProperties = additionalProperties; - IsUnknownDiscriminatorModel = DiscriminatorValue == "unknown"; + IsUnknownDiscriminatorModel = DiscriminatorValue == UnknownDiscriminatorValue; IsPropertyBag = false; ModelAsStruct = modelAsStruct; } @@ -79,35 +81,36 @@ public IReadOnlyDictionary DiscriminatedSubtypes get => _discriminatedSubtypes ??= new Dictionary(); internal set { - if (value is null || value.Count == 0) + if (value is null || DiscriminatorProperty == null || DiscriminatorValue == UnknownDiscriminatorValue) return; _discriminatedSubtypes = new Dictionary(value); + var cleanBaseName = Name.ToCleanName(); - _discriminatedSubtypes.Add("unknown", - new InputModelType( - $"Unknown{cleanBaseName}", - $"Unknown{cleanBaseName}", - "internal", - null, - $"Unknown variant of {cleanBaseName}", - Usage | InputModelTypeUsage.Json, - [], - this, - [], - "unknown", - new InputModelProperty( - DiscriminatorProperty!.Name, - DiscriminatorProperty.SerializedName, - DiscriminatorProperty.Description, - DiscriminatorProperty.Type, - DiscriminatorProperty.IsRequired, - DiscriminatorProperty.IsReadOnly, - DiscriminatorProperty.IsDiscriminator), - new Dictionary(), - null, - false) - ); + _discriminatedSubtypes.Add(UnknownDiscriminatorValue, + new InputModelType( + $"Unknown{cleanBaseName}", + $"Unknown{cleanBaseName}", + "internal", + null, + $"Unknown variant of {cleanBaseName}", + Usage | InputModelTypeUsage.Json, + [], + this, + [], + UnknownDiscriminatorValue, + new InputModelProperty( + DiscriminatorProperty!.Name, + DiscriminatorProperty.SerializedName, + DiscriminatorProperty.Description, + DiscriminatorProperty.Type, + DiscriminatorProperty.IsRequired, + DiscriminatorProperty.IsReadOnly, + DiscriminatorProperty.IsDiscriminator), + new Dictionary(), + null, + false) + ); } } public InputType? AdditionalProperties { get; internal set; } diff --git a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.Input/src/InputTypes/Serialization/TypeSpecInputModelTypeConverter.cs b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.Input/src/InputTypes/Serialization/TypeSpecInputModelTypeConverter.cs index 0eabb24d81..0dc5a8e8ba 100644 --- a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.Input/src/InputTypes/Serialization/TypeSpecInputModelTypeConverter.cs +++ b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp.Input/src/InputTypes/Serialization/TypeSpecInputModelTypeConverter.cs @@ -109,6 +109,10 @@ public static InputModelType CreateModelType(ref Utf8JsonReader reader, string? { model.DiscriminatedSubtypes = discriminatedSubtypes; } + else if (model.DiscriminatorProperty != null) + { + model.DiscriminatedSubtypes = new Dictionary(); + } model.ModelAsStruct = modelAsStruct; if (decorators != null) { diff --git a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/Providers/ModelFactories/ModelFactoryProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/Providers/ModelFactories/ModelFactoryProviderTests.cs index 80be39ccff..90d4bf861c 100644 --- a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/Providers/ModelFactories/ModelFactoryProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/Providers/ModelFactories/ModelFactoryProviderTests.cs @@ -143,7 +143,7 @@ private static InputModelType[] GetTestModels() values: [InputFactory.EnumMember.String("foo", "bar")]), isDiscriminator: true) }).ToArray(); - var derivedModel = InputFactory.Model("DerivedModel", properties: inheritanceProperties, discriminatedKind: "unknown"); + var derivedModel = InputFactory.Model("DerivedModel", properties: inheritanceProperties, discriminatedKind: "foo"); return [ InputFactory.Model("InternalModel", "internal", properties: properties), diff --git a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/Providers/ModelProviders/DiscriminatorTests.cs b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/Providers/ModelProviders/DiscriminatorTests.cs index e0eb9f973e..8e1354f8b6 100644 --- a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/Providers/ModelProviders/DiscriminatorTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/Providers/ModelProviders/DiscriminatorTests.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +using System; using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; @@ -16,6 +17,24 @@ namespace Microsoft.Generator.CSharp.Tests.Providers.ModelProviders { public class DiscriminatorTests { + private static readonly InputModelType _dinosaurModel = InputFactory.Model("dinosaur", discriminatedKind: "dinosaur", properties: + [ + InputFactory.Property("type", InputPrimitiveType.String, isRequired: true), + InputFactory.Property("dinosaurKind", InputPrimitiveType.String, isRequired: true, isDiscriminator: true) + ]); + + private static readonly InputModelType _animalModel = InputFactory.Model( + "animal", + properties: + [ + InputFactory.Property("type", InputPrimitiveType.String, isRequired: true, isDiscriminator: true), + InputFactory.Property("name", InputPrimitiveType.Boolean, isRequired: true) + ], + discriminatedModels: new Dictionary() + { + { "dinosaur", _dinosaurModel } + }); + private static readonly InputModelType _catModel = InputFactory.Model("cat", discriminatedKind: "cat", properties: [ InputFactory.Property("kind", InputPrimitiveType.String, isRequired: true, isDiscriminator: true), @@ -149,9 +168,38 @@ public void CreateUnknownVariant() MockHelpers.LoadMockPlugin(inputModelTypes: [_baseModel, _catModel, _dogModel]); var outputLibrary = CodeModelPlugin.Instance.OutputLibrary; var models = outputLibrary.TypeProviders.OfType(); + Assert.AreEqual(6, models.Count()); + // since each model has a discriminator, there should be 3 additional models for their unknown variants + var unknownPet = models.FirstOrDefault(t => t.Name == "UnknownPet"); + Assert.IsNotNull(unknownPet); + var unknownCat = models.FirstOrDefault(t => t.Name == "UnknownCat"); + Assert.IsNotNull(unknownCat); + var unknownDog = models.FirstOrDefault(t => t.Name == "UnknownDog"); + Assert.IsNotNull(unknownDog); + } + + // This test validates that a nested discriminated model with its' own discriminator property will have an unknown variant + [Test] + public void DiscriminatedModelWithNoSubTypesHasUnknownVariant() + { + MockHelpers.LoadMockPlugin(inputModelTypes: [_animalModel, _dinosaurModel]); + var outputLibrary = CodeModelPlugin.Instance.OutputLibrary; + var models = outputLibrary.TypeProviders.OfType(); Assert.AreEqual(4, models.Count()); - var unknownModel = models.FirstOrDefault(t => t.Name == "UnknownPet"); - Assert.IsNotNull(unknownModel); + + var animalModel = models.FirstOrDefault(t => t.Name == "Animal"); + Assert.IsNotNull(animalModel); + Assert.IsNull(animalModel!.BaseModelProvider); + var unknownAnimal = models.FirstOrDefault(t => t.Name == "UnknownAnimal"); + Assert.IsNotNull(unknownAnimal); + Assert.AreEqual(animalModel, unknownAnimal!.BaseModelProvider); + + var dinosaurModel = models.FirstOrDefault(t => t.Name == "Dinosaur"); + Assert.IsNotNull(dinosaurModel); + Assert.AreEqual(animalModel, dinosaurModel!.BaseModelProvider); + var unknownDinosaur = models.FirstOrDefault(t => t.Name == "UnknownDinosaur"); + Assert.IsNotNull(unknownDinosaur); + Assert.AreEqual(dinosaurModel, unknownDinosaur!.BaseModelProvider); } [Test] @@ -174,6 +222,32 @@ public void UnknownVariantHasPetBase() Assert.AreEqual("Pet", unknownModel!.Type.BaseType!.Name); } + // This test validates that a discriminator model whose discriminator value is "unknown" will throw + [Test] + public void DiscriminatedModelWithUnknownValueThrows() + { + Assert.Throws(() => + { + var unknownPlantModel = InputFactory.Model( + "unknownPlant", discriminatedKind: "unknown", properties: + [ + InputFactory.Property("type", InputPrimitiveType.String, isRequired: true), + ]); + + var plantModel = InputFactory.Model( + "plant", + properties: + [ + InputFactory.Property("type", InputPrimitiveType.String, isRequired: true, isDiscriminator: true), + InputFactory.Property("name", InputPrimitiveType.Boolean, isRequired: true) + ], + discriminatedModels: new Dictionary() + { + { "unknown", unknownPlantModel } + }); + }); + } + [Test] public void SerializationCtorForUnknownTakesDiscriminator() {