Port UnionConverter to STJ

[NickDarvey]

WIP reimplementing FsCodec.NewtonsoftJson.UnionConverter on System.Text.Json re #14 feeding into #38

To do

• [x] Ensure basic de/serialization passes, round-tripping
• [x] Ensure upconverting of missing properties to Nones or Nullables (this is covered by existing tests, like CaseJ and CaseT)
• [ ] Resolve all open review comments

[CLAassistant]

All committers have signed the CLA.

[bartelink]

Some generic commentary regarding reasoning behind my comments about the behavior of the converters in FsCodec (but also more broadly) when registered globally:

Especially for UnionConverter my high level opinions are: a) one should not be using unions or fancy things in JSON for simplicity of semantics across lots of consumer languages b) when used, you want to know about it

having that hardline stance also brings some benefits: a) less test cases - just prove it doesnt muck about with stuff not tagged with a converter b) (esp in Equinox, where its chief use is for roundtripping State unions in a state-machine stylee being used as snapshots) forcing the tagging makes people think a bit in the course of moving the state type to be in the module Events in the module Aggregate

I did a commit or two recently in stj to similarly reduce greediness in TypeSafeEnum.isTypeSafeEnum -

I've not expressed this philosophy well in the README as yet, but it also seems to align with how stuff is in STJ in general (vs newtonsoftland where greedier convention based converters are less frowned on)

finally in addition to Explicit being better than Implicit in general, it also tends to work well for perf - i.e. the converter gets detected at type level and then lives in the mappings cache, vs if it is global it is a tax on every type walk that happens not sure if there is an article on this; there should be!

aside: there are alternate views out there - for instance @ylibrach and his team have a wide array of converters, including registering quite a few globally

but bottom line is that in the FsCodec context:

• for TypeSafeEnum its good you're forced to tag them so people understand there needs to be a canonical mapping (and maybe an anticorruption layer if you dont like that)
• for RecordConverter and OptionConverter obv it would be a pain for them not to be global
• for JsonIsomorphism/JsonPickler, you want people to be aware right on the type
• there are no other converters you'd commonly use, esp for event codecs, but IME also for the bulk of request/response bodies (and I guess what gets supported in STJ in .NET 5 and later will also drive what people expect to do wrt binding)

... so UnionConverter being required to be marked explicitly on the concrete union type simply follows from that

[NickDarvey]

the converter gets detected at type level and then lives in the mappings cache, vs if it is global it is a tax on every type walk that happens

I'm not sure if this is what you mean, but they do cache converters created by the factory. Though I guess specifying it as a concrete type saves that first-run reflection still.

... so UnionConverter being required to be marked explicitly on the concrete union type simply follows from that

Understood, less greedy converters makes more sense for STJ and not having them so easily available pushes our users toward being more explicit about their serialization.

I'm going to remove the JsonConverterFactory so our users are pushed toward being more explicit when they're serializing DUs.

[bartelink]

I'm not sure if this is what you mean, but they do cache converters created by the factory. Though I guess specifying it as a concrete type saves that first-run reflection still.

I was referring to the fact that the set of globally registered converters will be consulted per property as it determines the mapping path that'll apply (even if that's cached per type after that).

[bartelink]

Semi related: stumbled on @ylibrach's https://github.com/dotnet/runtime/issues/1130 just now too

[bartelink]

I've had out of band discussions with Nick regarding this PR. The status is as follows:

• the logic is pretty much complete
• Nick has parked the work in order to be able to focus wholly on another time-sensitive side-project
• Nick is open to someone else bringing the PR to completion in the (likely) event he doesn't get back to it before someone needs it

[bartelink]

Argh! Didnt mean for this PR to be closed - I promise I'll get back on rebasing and merging it soon!

Can you make sure to keep this around for when that time comes please?