Closed schnerring closed 7 months ago
schnerring
commented
1 year ago As an aside, it would be really, really nice to also generate Open API inheritance discriminators from the JsonPolymorphicAttribute instead of the JsonConverterAttribute. I think the magic happens here:
Do you think it's feasible to add support for this?
schnerring
commented
1 year ago My use case is that I deserialize a document via Marten. The class used for deserialization is already annotated with the STJ polymorphism. I'd have to add JsonConverter and KnownType attributes just for NSwag.
[JsonPolymorphic(TypeDiscriminatorPropertyName = TypeDiscriminatorName)]
[JsonDerivedType(typeof(FieldString), typeDiscriminator: nameof(FieldString))]
[Newtonsoft.Json.JsonConverter(typeof(JsonInheritanceConverter), TypeDiscriminatorName)]
[KnownType(typeof(FieldString))]
public abstract class FieldBase
{
public const string TypeDiscriminatorName = "$t";
}
public class FieldString : FieldBase {}Mixing STJ and Newtonsoft attributes like this is a bit ugly. This would be resolved if the NSwag schema generator could also understand JsonPolymorphic and JsonDerivedType attributes.
Sorry for the noise, let me know if I should also open a corresponding issue for discussion.
schnerring
commented
1 year ago Do you think it's feasible to add support for this?
Turns out it wasn't too hard (but a bit dirty) to make the C# generator aware of the new STJ polymorphic attributes. Have a look at the unit tests I've added to see how it works.
I implemented it in such a way that the schema generator looks for the new attributes first, and if it doesn't find them executes the old logic as is.
Todos:
CSharpGeneratorSettings so users are able to choose between STJ and "legacy" attributes. I'll be happy to add this to NSwagStudio etc.
kev-andrews
commented
1 year ago It would be great if this could be merged. Nswag is literally the last piece of code we have to migrate before we can move to STJ.
franklixuefei
commented
1 year ago This is still open? Please take a look @RicoSuter thanks!
jflygare
commented
12 months ago A word of caution before migrating to STJ native polymorphism. STJ currently (.NET 7) only supports the discriminator as the first value found in the json object. There is an open request to address this. https://github.com/dotnet/runtime/issues/72604 I ran into issues with this myself and had to implement my own converter anyway.
FWIW, here is my implementation of the STJ JsonInheritanceConverter/Attribute which addresses the StackOverflowException caused by recursive calls by the JsonSerializer.
using System.Reflection;
using System.Text.Json;
using System.Text.Json.Serialization;
using System.Text.Json.Serialization.Metadata;
namespace .....
{
/// <summary>
/// <inheritdoc/>
/// </summary>
/// <typeparam name="TBase"></typeparam>
public class JsonInheritanceConverter<TBase> : JsonConverter<TBase>
{
private readonly string _discriminatorName;
private readonly Type _baseType;
public JsonInheritanceConverter(string discriminatorName)
{
_discriminatorName = discriminatorName;
_baseType = typeof(TBase);
}
/// <summary>
/// <inheritdoc/>
/// </summary>
/// <param name="reader"></param>
/// <param name="typeToConvert"></param>
/// <param name="options"></param>
/// <returns></returns>
public override TBase Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
var document = JsonDocument.ParseValue(ref reader);
var typeToReturn = document.RootElement.TryGetProperty(_discriminatorName, out var discriminator)
? GetObjectSubtype(typeToConvert, discriminator.GetString()!)
: typeToConvert;
// NOTE: To avoid calling this coverter recursively (StackOverflowException), we get the default converter for the base type
// and add to a "copy" of the input JsonSerializerOptions. This will take priority over the annotation and can be de-serialized
// as usual
JsonSerializerOptions readOptions = options;
if (typeToReturn == _baseType)
{
var converter = (JsonConverter<TBase>)JsonTypeInfo.CreateJsonTypeInfo<TBase>(options).Converter;
readOptions = new JsonSerializerOptions(options);
readOptions.Converters.Add(converter);
}
return (TBase)JsonSerializer.Deserialize(document, typeToReturn, readOptions)!;
}
/// <summary>
/// <inheritdoc/>
/// </summary>
/// <param name="writer"></param>
/// <param name="value"></param>
/// <param name="options"></param>
public override void Write(Utf8JsonWriter writer, TBase? value, JsonSerializerOptions options)
{
Type typeToWrite = value!.GetType();
var discriminatorValue = GetSubtypeDiscriminator(typeToWrite);
// NOTE: To avoid calling this coverter recursively (StackOverflowException), we get the default converter for the base type
// and add to a "copy" of the input JsonSerializerOptions. This will take priority over the annotation and can be serialized
// as usual
JsonSerializerOptions writeOptions = options;
if (typeToWrite == _baseType)
{
var converter = (JsonConverter<TBase>)JsonTypeInfo.CreateJsonTypeInfo<TBase>(options).Converter;
writeOptions = new JsonSerializerOptions(options);
writeOptions.Converters.Add(converter);
}
var rootElement = JsonSerializer.SerializeToElement(value, typeToWrite, writeOptions);
writer.WriteStartObject();
// Look to see if discriminatorValue is not already a property and explicitly write if missing
if (!rootElement.EnumerateObject().Any(e => e.Name.Equals(_discriminatorName, StringComparison.InvariantCultureIgnoreCase)))
writer.WriteString(_discriminatorName, discriminatorValue);
rootElement.EnumerateObject().ToList().ForEach(property => property.WriteTo(writer));
writer.WriteEndObject();
}
private Type GetObjectSubtype(Type objectType, string discriminatorValue)
=> CustomAttributeExtensions.GetCustomAttributes<JsonInheritanceAttribute>(IntrospectionExtensions.GetTypeInfo(objectType), true)
.Where(attrib => attrib.Key == discriminatorValue).Select(attrib => attrib.Type).FirstOrDefault() ?? objectType;
private string GetSubtypeDiscriminator(Type objectType)
=> CustomAttributeExtensions.GetCustomAttributes<JsonInheritanceAttribute>(IntrospectionExtensions.GetTypeInfo(objectType), true)
.Where(attrib => attrib.Type == objectType).Select(attrib => attrib.Key).FirstOrDefault() ?? objectType.Name;
}
/// <summary>
/// <inheritdoc/>
/// </summary>
[AttributeUsage(AttributeTargets.Class)]
internal class JsonInheritanceConverterAttribute : JsonConverterAttribute
{
public string DiscriminatorName { get; }
public JsonInheritanceConverterAttribute(Type _, string discriminatorName = "discriminatorValue")
{
DiscriminatorName = discriminatorName;
}
public override JsonConverter? CreateConverter(Type baseType)
{
var converterType = typeof(JsonInheritanceConverter<>).MakeGenericType(baseType);
return (JsonConverter?)Activator.CreateInstance(converterType, DiscriminatorName)
?? throw new InvalidOperationException($"Unable to create {converterType}");
}
}
}See also: https://github.com/JasperFx/marten/issues/2586
Another hack that works is choosing a discriminator property name like this:
[JsonPolymorphic(TypeDiscriminatorPropertyName = "_")]
RicoSuter
commented
11 months ago I think we need to put this behind an eg UseNativeInheritance flag (defalt false for now) so that you can opt-in to this new generator... if you look at the generated code as a black box then there should not really be a difference assuming both have the same features/performance...
schnerring
commented
11 months ago I see your point, but wouldn't it be confusing for STJ users having to opt into this explicitly? Would it be reasonable to enable this feature by default if STJ is selected instead of Newtonsoft?
jeremia
commented
10 months ago I see your point, but wouldn't it be confusing for STJ users having to opt into this explicitly? Would it be reasonable to enable this feature by default if STJ is selected instead of Newtonsoft?
Since there is a possible complication with the issue https://github.com/dotnet/runtime/issues/72604 it wouldn't it be good to have the flag for now? The flag can be removed in the future when no such complications are known.
schnerring
commented
9 months ago The flag can be removed in the future when no such complications are known.
Fair enough.
If I understand correctly, some of the work from this PR has been integrated into the master branch (https://github.com/RicoSuter/NJsonSchema/commit/16bc1c038b899e41bba73bdd0061b85ff28af513). I'm not sure if there's a NuGet preview, though.
I haven't kept up with the latest NSwag changes the past couple of months, so unless Rico or someone else tells me how to continue with the work of this PR, I won't add any changes.
jeremia
commented
8 months ago Hi!
I was hoping that this fix would solve the issue https://github.com/RicoSuter/NSwag/issues/4375 thats bothering me.
So I was going to make a try to get this pull request finished.
It's my understanding that the actual code generation in NSwag studio is done by NJsonSchema in NJsonSchema.CodeGeneration.CodeArtifact in one of the constructors where the template's Render method is executed.
The call hierarchy goes here from both the code generation in NSwag Studio and the inheritance tests in NJsonSchema.
I cloned the schnerring branch of NJsonSchema and the main branch of NSwag.
I built the NJsonSchema and NJsonSchema.CodeGeneration NuGet packages with a new version number and put the packages in a local NuGet repo. I then updated the package references in NSwag for those packages and started NSwag Studio from Visual Studio. When I generated clients with .NET 7.0 using System.Text.Json with a schema containing inheritance the generated client did not contain the new native attributes JsonDerivedType or JsonPolymorphic, but still the old JsonInheritanceConverter and JsonInheritanceAttribute. It seems I'm missing something. Can anybody give me a clue?
It seems like NJsonSchema doesn't have a method for generating a client from a complete Open Api spec, but just the different parts. I was hoping to test the code generation of a complete example Open Api spec directly in NJsonSchema. Is that possible?
EelcoLos
commented
7 months ago I'm wondering if the case of enum/int casting with STJ is also covered in this PR: https://github.com/RicoSuter/NJsonSchema/issues/1666 .
schnerring
commented
7 months ago I finally got the chance to take another look at this. The schema generation changes of this PR have already been incorporated in this commit: https://github.com/RicoSuter/NJsonSchema/commit/16bc1c038b899e41bba73bdd0061b85ff28af513
Since a lot of changes have been made since then, I created a new PR instead of rebasing this one. The new PR is my attempt to add the missing STJ polymorphism features to the C# client generator: https://github.com/RicoSuter/NJsonSchema/pull/1675
Beginning with .NET 7, System.Text.Json supports polymorphic type hierarchy serialization and deserialization with attribute annotations.. This PR adds support for the generation of
JsonPolymorphicandJsonDerivedTypeattributes to the C# client generator.This basically deprecates https://github.com/RicoSuter/NJsonSchema/blob/ed250a8b9c4f86a4da40cf0061cf0172cdea0449/src/NJsonSchema.Tests/Generation/SystemTextJson/JsonInheritanceConverter.cs#L14 for .NET 7 and later.
It Works On My Machine™, but the addition of unit tests and the changes I've made are pretty naive and the generated STJ code only works for .NET 7 or later. I was looking for something like a ".NET target type" parameter in the Liquid templates, but couldn't find any of the like. @RicoSuter I'd appreciate if you could point me to where else I gotta flesh out the code.
Related issues: