wivuu / Wivuu.JsonPolymorphism

Adds System.Text.Json serializer support for polymorphism with a discriminator attribute
MIT License
25 stars 3 forks source link

Does this work with NSwag? #3

Closed LarsKemmann closed 2 years ago

LarsKemmann commented 3 years ago

Hi there! So I feel rather silly right now... 😁 I just spent a couple hours teaching myself the new Source Generators feature in order to basically build the same thing you did: https://github.com/LarsKemmann/JsonPolymorph

Yay for duplicating effort because search engines didn't show me your repository. 🙄 It wasn't until I went to install my newly published NuGet package in Visual Studio that I saw your project show up.

So, I have an ASP.NET Core Web API and I'm currently using NSwag (which assumes Newtonsoft.Json) to generate an OpenAPI v3 schema as well as client-side TypeScript code. The core objective I have is to enable that scenario.

Is there a way to accomplish what I'm trying to do using your library? I'm (pleasantly) surprised that you were able to get polymorphic deserialization to work using System.Text.Json - kudos! Now I just need to know if I can generate a compatible OpenAPI v3 schema using that approach.

onionhammer commented 3 years ago

Hi @LarsKemmann

I've done some NSwag stuff in the past with https://github.com/wivuu/nswag-liquid-slim to generate smaller templates

I haven't done anything today to specifically support NSwag, what did you have in mind? Just newtonsoft Jason attributes? I'm not sure what we can do to fully support NSwag without surgery to NSwag itself.

Anyway, ideas and PRs are very welcome if you want to collaborate

LarsKemmann commented 3 years ago

Sorry I haven't responded sooner, I'm still trying to think through the right approach for my use case. I do have a somewhat-related question: have you been able to use this library successfully with nested record types?

LarsKemmann commented 3 years ago

So this is interesting... your library handles nested polymorphic record types without any issues, while mine is unable to do that. Did you have to do anything special to enable nested polymorphic serialization?

Here is the model I'm using for my test:

enum AgeType
{
    ExactAge,
    AgeInYears
}
abstract partial record Age([JsonDiscriminator] AgeType Type);
sealed record ExactAge(DateTime DateOfBirth) : Age(AgeType.ExactAge);
sealed record AgeInYears(int Years, DateTime AsOf) : Age(AgeType.AgeInYears);

enum AnimalType
{
    Insect,
    Mammal,
    Reptile,
    //Bird
}

abstract partial record Animal([JsonDiscriminator] AnimalType Type, string Name, Age Age);
sealed record Insect(Age Age, int NumLegs = 6, int NumEyes = 4) : Animal(AnimalType.Insect, "Insectoid", Age);
sealed record Mammal(Age Age, int NumNipples = 2) : Animal(AnimalType.Mammal, "Mammalian", Age);
sealed record Reptile(Age Age, bool ColdBlooded = true) : Animal(AnimalType.Reptile, "Reptilian", Age);
onionhammer commented 3 years ago

Nope.. the way it's implemented nesting should have no trouble; the inner content of a record would be deserialized the same way as the outer content, using the json converter attribute

LarsKemmann commented 3 years ago

Well, I am so thankful that I now am able to round-trip nested polymorphic types with your library. 🙂 However, the enum discriminator requires a fair amount of ceremony (see https://github.com/CareTogether/CareTogetherCMS/commit/8c2ce17a99487a72a294bf7ee1c17a96e2ca5367 -- it was painful 😂). I haven't looked too closely at the implementation in your library, but I wonder if just using the type names (maybe fully-qualified if necessary) would be sufficient to replace the switch logic? If you look up overloads by type name rather than by enum value, it's the same end result, right? And generating the discriminator property in the serialized JSON based on the runtime type (again, fully qualified if necessary) shouldn't be too difficult, I would think... in theory?

If you have some pointers on how that could be done, I'd like to try putting together a PR at some point to add the declarative/convention-based approach into your library as an alternative supported pattern. (I first have to see if what you've suggested for NSwag will work in my use case; that's my next challenge.)

Thanks for listening -- and for the excellent work you've shared here! I really appreciate it. 🙂

LarsKemmann commented 3 years ago

I haven't done anything today to specifically support NSwag, what did you have in mind? Just newtonsoft Jason attributes? I'm not sure what we can do to fully support NSwag without surgery to NSwag itself.

With the Newtonsoft-based approach I took in my library, Newtonsoft's JsonHierarchyConverter integrates with NSwag so the generated OpenAPI JSON correctly reflects the polymorphism via oneOf annotations. With this library, on the other hand, neither NSwag nor Swashbuckle are (as far as I can tell) able to detect the polymorphism and thus the generated OpenAPI JSON only ever includes the base type. I was planning to use TypeScript code generation to build a client SDK as a core part of the developer experience for my solution, but at this point it looks like System.Text.Json won't support polymorphism until .NET 7 so I'll have to go the manual route until then. Unless you have any ideas for annotating the type hierarchies in a way that NSwag or Swashbuckle will pick up?

onionhammer commented 3 years ago

@LarsKemmann It shouldnt be hard to generate any additional attributes you want to add, assuming it can be reasoned at compile-time. Perhaps a Wivuu.JsonPolymorphism.NSwag would be in order?

Unfortunately outside of bug-fixes I won't have a lot of time to devote to this library for the next couple months; it's currently serving its purpose for my needs in production. I'd be happy to accept collaboration though.

Can you give an example of how inferring by name of type would work (without enums)? I wanted to make the discriminator an actual property, rather than a hidden/magic property, so it needed a type, and an enum fit best.

Are you thinking:

record Insect();
record Grasshopper: Insect();
record Butterfly: Insect();

Where some automatic property would be serialized for these types?

LarsKemmann commented 3 years ago

Yes, exactly, that was the approach I took in my library, with a default property name of discriminator (which could in theory be overridden easily via an attribute constructor argument). I just apply a [JsonHierarchyBase] to the Insect record and the source generator does the rest.

And by the way, I just came back to post that when I said my library couldn't handle nested polymorphic types, I had actually just forgotten to add Microsoft.AspNetCore.Mvc.NewtonsoftJson to my web API project. 🤦‍♂️ So my library actually serves my needs for the time being, as well. 😂 However, I'm very interested in moving away from Newtonsoft in the future to shed some weight from the project, so I will probably look at this again around the .NET 7 preview timeframe!

onionhammer commented 3 years ago

For my usecase, I wanted to be able to search a database by the 'kind', so for example in Entity Framework (for Cosmos) or Cosmos SDK itself, you would do something like db.Insects.Where(i => i.Kind == InsectType.Grasshopper)