Port UnionConverter to STJ #43
Conversation
|
|
| //[<Fact>] | ||
| //let ``Implementation ensures no internal errors escape (which would render a WebApi ModelState.Invalid)`` () = | ||
| // let s = JsonSerializer.CreateDefault() | ||
| // let mutable gotError = false | ||
| // s.Error.Add(fun _ -> gotError <- true) | ||
|
|
||
| // let dJson = """{"case":"CaseD","a":"hi"}""" | ||
| // use dReader = new StringReader(dJson) | ||
| // use dJsonReader = new JsonTextReader(dReader) | ||
| // let d = s.Deserialize<TestDU>(dJsonReader) | ||
|
|
||
| // test <@ (CaseD "hi") = d @> | ||
| // test <@ false = gotError @> |
There was a problem hiding this comment.
TODO: I'll need to poke around to see how they have integrated STJ with ASP.NET's modelstate to make sure I can test for this behaviour. (JsonSerializer.Error is not a thing.)
| test <@ CaseK (1, Nullable 2) = deserialize """{"case":"CaseK", "a":1, "b":2 }""" @> | ||
| test <@ CaseL (Nullable 1, Nullable 2) = deserialize """{"case":"CaseL", "a": 1, "b": 2 }""" @> | ||
|
|
||
| // TOINVESTIGATE: It looks like Newtonsoft Settings.Create() behaviour is to always add |
There was a problem hiding this comment.
Yes, Options.Create adds a JsonOptionsConverter and Settings.Create similarly - you would end up with a Value nesting if its not in the mix - at one point defaultOptions might have referred to settings created by Settings.CreateDefault(), which adds no converters of any kind
It'd be nice for this to be as tight as possible in terms of numbers of (esp global) converters it throws into the mix
| | Catchall | ||
|
|
||
| [<Fact>] | ||
| let ``UnionConverter supports a nominated catchall via options`` () = |
There was a problem hiding this comment.
not sure it did this before; not sure its a good idea as its a recipe for catchall case not found exceptions
(In general, global converters are a bad idea as they're magic IMO)
Not sure if you agree, but in my book this test should demonstrate random unions don't get affected (people like building SCDU converters etc, but again, I'd prefer not to conflict with such semantics as UnionConverter has plenty semantics just dealing with what it already covers)
| type DuWithMissingCatchAll = | ||
| | Known | ||
|
|
||
| [<Fact>] |
|
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 having that hardline stance also brings some benefits: I did a commit or two recently in 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 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:
... so |
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.
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. |
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). |
| /// <remarks>Not inherited because JsonConverters don't get inherited right now. | ||
| /// https://github.com/dotnet/runtime/issues/30427#issuecomment-610080138</remarks> | ||
| [<AttributeUsage(AttributeTargets.Class ||| AttributeTargets.Struct, AllowMultiple = false, Inherited = false)>] | ||
| type JsonUnionConverterOptionsAttribute(discriminator : string) = |
There was a problem hiding this comment.
any reason to make this mandatory ? the catchall is probably more likely to be customized?
There was a problem hiding this comment.
This wall of text relates to a couple of items of feedback, and I realized caused me a bit of churn in my commits, but this is the piece of feedback that made me realized I needed to articulate my thoughts so I've placed it here.
I think I was requiring the discriminator to be specified as a way to solve an issue I was having about inferring the user's intentions with these annotations and the outcome they'd expect.
Context
Let's imagine the discriminator is not required right now, both Discriminator and CatchAllCase are both nullable on JsonUnionConverterOptionsAttribute, a user can set one, both, or none. We also have default options set on the converter, "case" as a discriminator and no catch-all case.
I'll use two imaginary DUs here called DiscriminatorOption and CatchAllCaseOption to represent the possible user intents when they are setting options via an attribute:
type DiscriminatorOption =
| CustomDiscriminator of string
| DefaultDiscriminator
type CatchAllCaseOption =
| CustomCase of string
| NoCase
| DefaultExpected behaviour
Inferring a user's intended DiscriminatorOption when they set the Discriminator of the attribute is fine as it's binary and quite understandable when represented with null/not-null:
[JsonUnionConverterOptions(Discriminator = "type")]
= DiscriminatorOption.CustomDiscriminator "type"
= "I want to set a specific discriminator for this union"
[JsonUnionConverterOptions()]
= DiscriminatorOption.DefaultDiscriminator
= "I want to use whatever the default discriminator is."
However CatchAllCaseOption is ternary and I don't know what users would expect:
[JsonUnionConverterOptions(CatchAllCase = "other")]
= CatchAllCaseOption.CustomCase "other"
= "I want to set a specific catch-all case for this union"
[JsonUnionConverterOptions()]
= ...? Does this mean they want no catch-all case or that they want to use the default catch-all case option
Attempted solution
My attempted solution for that was making the discriminator mandatory so it was explicit users were choosing custom options.
If they set the attribute they are saying DiscriminatorOption = CustomDiscriminator and CatchAllCaseOption = CustomCase | NoCase; conversely 'DefaultDiscriminator' and 'DefaultCase' were selectable by not having the attribute at all.
However you raise a good point that the use case for this attribute is mainly setting the CatchAllCaseOption (and users will probably be happy with the default discriminator).
Potential solutions
Here's are a couple of options I thought of to try to address understanding the user's intent but also that use case:
-
Split these options into two attributes:
JsonUnionConverterDiscriminatorAttribute;JsonUnionConverterCatchAllCaseAttribute[JsonUnionConverterDiscriminator("type")] = DiscriminatorOption.CustomDiscriminator "type" = "I want to set a specific discriminator for this union"(*no JsonUnionConverterDiscriminator attribute*) = DiscriminatorOption.DefaultDiscriminator = "I want to use whatever the default discriminator is."`[JsonUnionConverterCatchAllCase(CaseName = "other")] = CatchAllCaseOption.CustomCase "other" = "I want to set a specific catch-all case for this union"[JsonUnionConverterCatchAllCase()] = CatchAllCaseOption.NoCase = "I want no catch-all case for this union"(*no JsonUnionConverterCatchAllCase attribute*) = CatchAllCaseOption.Default = "I want the default catch-all case for this union, if there is one" -
Stick with
JsonUnionConverterOptionsAttributebut make the discriminator not required, and infer a nullCatchAllCaseto meanCatchAllCaseOption.NoCase, overriding whatever is the default option (which as it happens, is also no catch-all case).
If this is done, I think I also ought to remove thecatchAllCaseconstructor parameter fromUnionConverterso users can't set a catchAllCase if they are manually creating and registering a converter viaOptions.Create. This is because if they then set wanted to set a custom discriminator via an attribute, this solution would now infer it as them also choosingCatchAllCaseOption.NoCase(which would override what they set in the constructor of the converter which I think would be unexpected behaviour).
Next steps
If you have a strong feeling about those options, spot flaws in my reasoning, or better suggestions please let me know, otherwise I'll go ahead an implement the solution described in 1 in the next couple of days.
There was a problem hiding this comment.
A catch-all is not the common case. As with TypeSafeEnumConverter, simply rejecting by throwing is fine.
There never was the notion of a 'default catch-all' (it was added as a feature later, and e.g. for that to become a default would be annoying/breaking).
I thus prefer a single options attribute, and removing the notion of CatchAllCaseOption.Default
Having said all that, it's up to you - my default stance as an implementer is to work toward an easy to scan impl and easy to test (meaning easy to define) spec (which I think matches the preceding - KISS principle ;)).
There was a problem hiding this comment.
🤔... unless the default catch-all was defined as the single case with a body that's a JsonElement, but thats a pretty loose approach not in alignment with STJ design in general, I'd say ;)
I think with these things, in practice, explicit is better than implicit - the default mode of operation is that a) unions dont get converted b) applying the JsonUnionConverter means they get converted a single way (using "case") with no default case
c) only by applying the JsonUnionConverterOptions would you every end up with catch-all semantics
There was a problem hiding this comment.
Okay, excellent. That works for me.
I will stick with a single options attribute and remove the notion of CatchAllCaseOption.Default by removing catchAllCase from the UnionConverter constructor.
There was a problem hiding this comment.
RE: Catch-all in ctor arg
I think this has the issue around understanding the user's intent
What if they have registered a converter and specified a catch-all case via the ctor,
Options.Create(UnionConverter<MyDU>((*discriminator*) "type", (*catchAllCase*) "other")and then use the attribute to specify a custom discriminator (but left out the catch-all case in said attribute)?
[JsonUnionConverterOptions(Discriminator = "kind")]Would you (the user) expect the converter to use the catch-all case as specified in the ctor, or for it to not use a catch-all case?
And then what would the user be trying to do if they did this?
[JsonUnionConverterOptions(CatchAllCase = null)](I know we, the converter, can't differentiate that from the previous example, but in terms of user intent...)
There was a problem hiding this comment.
If this attribute is set, its values take precedence over the values set on the converter via its constructor.
not getting why this needs to be ?
I thought I/the user would only expect the JsonUnionConverterOptionsAttribute to come into play if I'd asked for the conversion via JsonConverterAttribute; if I programmatically configure it by calling a ctor with 2 defaulting args (with xmldoc saying this is the choice, attributes are not looked up)
i.e. the default ctor could do the lookup and then pass the opts to the primary constructor:
type JsonUnionConverter<'T>(discriminator, ?catchAll) =
...
do match catchAll with
| None -> ()
| Some ca ->
<throw if invalid case nominated>
new() =
let d, ca = <attribute lookup on T>, default to "case" for d
JsonUnionConverter(discriminator=d, ?catchAll=ca)
There was a problem hiding this comment.
Ohhh okay, that was our misalignment. I had assumed you/the user might use JsonUnionConverterOptionsAttribute without JsonConverterAttribute.
('might', I merely mean we can't stop someone applying the options attribute even though they're registering the converter themselves, even though it might not be a great idea.)
I feel like I understand your position now. I'm happy to implement so we only fetch the attributes there are no ctor arguments supplied. (And yes, validating the nominated case is a good idea.)
There was a problem hiding this comment.
Cool, sorry for any confusion caused to steer you into this direction (I'm extremely lazy and would never even have considered going to the lengths of completeness you were considering; a lack of imagination on my part!)
I think ultimately this should yield a compact impl that one can scan and grok without referring to this whole dialogue in the end!
| /// Paralells F# behavior wrt how it generates a DU's underlying .NET Type | ||
| let inline isInlinedIntoUnionItem (t : Type) = | ||
| t = typeof<string> | ||
| //|| t.IsValueType |
There was a problem hiding this comment.
🤔 if you have a union of int * int, is this not needed? (no need to respond/explain, have not looked at full code)
There was a problem hiding this comment.
Yeah I think you're right. I think what this actually needs is a check for JsonElement for when we're keeping a JsonElement field, see #43 (comment)
| | null when options.IgnoreNullValues -> () | ||
| | fv -> | ||
| let element = JsonSerializer.SerializeToElement(fv, options) | ||
| match element.ValueKind with |
|
|
||
| new() = UnionConverter("case", null) | ||
| new(discriminator: string, catchAllCase: string) = // Compatibility with Newtonsoft UnionConverter constructor | ||
| UnionConverter(discriminator, match catchAllCase with null -> None | x -> Some x) |
There was a problem hiding this comment.
Option.ofObj catchAllCase ?
| static let converterType = typedefof<UnionConverter<_>> | ||
|
|
||
| new() = UnionConverter("case", null) | ||
| new(discriminator: string, catchAllCase: string) = // Compatibility with Newtonsoft UnionConverter constructor |
There was a problem hiding this comment.
can this collapse into a primary ctor with 2 optional args which defaults to "case" for discriminator ?
| override _.CreateConverter(t, _) = | ||
| let options = t.GetCustomAttributes(typeof<JsonUnionConverterOptionsAttribute>, false) | ||
| |> Array.tryHead // AttributeUsage(AllowMultiple = false) | ||
| |> Option.map (fun a -> a :?> JsonUnionConverterOptionsAttribute) |
There was a problem hiding this comment.
I think :?> can become :>
| let options = t.GetCustomAttributes(typeof<JsonUnionConverterOptionsAttribute>, false) | ||
| |> Array.tryHead // AttributeUsage(AllowMultiple = false) | ||
| |> Option.map (fun a -> a :?> JsonUnionConverterOptionsAttribute) | ||
| let discriminator = options |> Option.map (fun o -> o.Discriminator) |
There was a problem hiding this comment.
can/should that defaulting go into the converter itself?
| roundtripProperty ignoreNulls profile x | ||
|
|
||
| //[<Fact>] | ||
| //let ``Implementation ensures no internal errors escape (which would render a WebApi ModelState.Invalid)`` () = |
There was a problem hiding this comment.
As discussed, I think this can simply be removed - there's no anologous facility in STJ, and we'll know from exceptions if there are problems
| type DuWithoutAttributes = | ||
| | Case1 | ||
|
|
||
| [<JsonUnionConverterOptions("kind")>] |
There was a problem hiding this comment.
this name suggests the converter should prob be renamed to JsonUnionConverter?
|
|
||
| open Samples | ||
|
|
||
| // TODO support [<Struct>] |
* Replaces Newtonsoft bits from scaffold with the actual STJ bits * Tries to retain symmetry with Newtonsoft UnionConverter impl
(Oh, they're all failing)
* Wrong namespace lol, and * It seems like boxing a DU means that STJ won't use the convert for serialization. Maybe this is something to do with derived types and JsonConverter dotnet/runtime#30427
…own discriminator and catch-all cases
https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-migrate-from-newtonsoft-how-to Newtonsoft.Json can be configured to throw exceptions during deserialization if the JSON includes properties that are missing in the target type. System.Text.Json ignores extra properties in the JSON, except when you use the [JsonExtensionData] attribute. There's no workaround for the missing member feature.
Uses the provided type parameter so that we strictly only convert the union that we were asked to convert
| @@ -153,7 +153,7 @@ let ``deserializes properly`` () = | |||
|
|
|||
| let (|Q|) (s: string) = JsonSerializer.Serialize(s, defaultOptions) | |||
|
|
|||
| // Renderings when NullValueHandling=Include, which is the default for Json.net, and used by the recommended Settings.CreateCorrect profile | |||
| // Renderings when NullValueHandling=Include, which is used by the recommended Settings.Create profile | |||
|
Semi related: stumbled on @ylibrach's dotnet/runtime#1130 just now too |
|
I've had out of band discussions with Nick regarding this PR. The status is as follows:
|
|
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? |
WIP reimplementing
FsCodec.NewtonsoftJson.UnionConverteronSystem.Text.Jsonre #14 feeding into #38To do
Nones orNullables(this is covered by existing tests, like CaseJ and CaseT)