-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Port UnionConverter to STJ #43
Changes from all commits
f78df5d
1ac36de
f225bf5
1d7850a
1c37108
6febb49
b512640
c237c58
8ae5deb
92e5f36
3b138f2
95ac2da
104dbaf
a5cfd76
a61af4f
a5ad973
824a903
ab45ca3
aeb3363
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,47 @@ | ||
namespace FsCodec.SystemTextJson | ||
|
||
open FsCodec.SystemTextJson | ||
open FsCodec.SystemTextJson.Core | ||
open FSharp.Reflection | ||
open System | ||
open System.Reflection | ||
open System.Text.Json | ||
|
||
type IUnionConverterOptions = | ||
abstract member Discriminator : string with get | ||
abstract member CatchAllCase : string option with get | ||
|
||
/// <summary>Use this attribute in combination with a JsonConverter/UnionConverter attribute to specify | ||
/// your own name for a discriminator and/or a catch-all case for a specific discriminated union. | ||
/// If this attribute is set, its values take precedence over the values set on the converter via its constructor. | ||
/// Example: <c>[<JsonConverter(typeof<UnionConverter>); JsonUnionConverterOptions("type")>]</c></summary> | ||
/// <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) = | ||
inherit Attribute() | ||
member val CatchAllCase: string = null with get, set | ||
interface IUnionConverterOptions with | ||
member _.Discriminator = discriminator | ||
member x.CatchAllCase = Option.ofObj x.CatchAllCase | ||
|
||
type UnionConverterOptions = | ||
{ | ||
discriminator : string | ||
catchAllCase : string option | ||
} | ||
interface IUnionConverterOptions with | ||
member x.Discriminator = x.discriminator | ||
member x.CatchAllCase = x.catchAllCase | ||
|
||
[<NoComparison; NoEquality>] | ||
type private Union = | ||
type internal Union = | ||
{ | ||
cases: UnionCaseInfo[] | ||
tagReader: obj -> int | ||
fieldReader: (obj -> obj[])[] | ||
caseConstructor: (obj[] -> obj)[] | ||
options: IUnionConverterOptions option | ||
} | ||
|
||
module private Union = | ||
|
@@ -23,5 +55,115 @@ module private Union = | |
tagReader = FSharpValue.PreComputeUnionTagReader(t, true) | ||
fieldReader = cases |> Array.map (fun c -> FSharpValue.PreComputeUnionReader(c, true)) | ||
caseConstructor = cases |> Array.map (fun c -> FSharpValue.PreComputeUnionConstructor(c, true)) | ||
options = | ||
t.GetCustomAttributes(typeof<JsonUnionConverterOptionsAttribute>, false) | ||
|> Array.tryHead // AttributeUsage(AllowMultiple = false) | ||
|> Option.map (fun a -> a :?> IUnionConverterOptions) | ||
} |> Some | ||
let tryGetUnion : Type -> Union option = memoize _tryGetUnion | ||
|
||
|
||
/// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
|| t.IsArray | ||
|| (t.IsGenericType | ||
&& (typedefof<Option<_>> = t.GetGenericTypeDefinition() | ||
|| t.GetGenericTypeDefinition().IsValueType)) // Nullable<T> | ||
|
||
let typeHasJsonConverterAttribute = memoize (fun (t : Type) -> t.IsDefined(typeof<Serialization.JsonConverterAttribute>, false)) | ||
|
||
let propTypeRequiresConstruction (propertyType : Type) = | ||
not (isInlinedIntoUnionItem propertyType) | ||
&& not (typeHasJsonConverterAttribute propertyType) | ||
|
||
/// Prepare arguments for the Case class ctor based on the kind of case and how F# maps that to a Type | ||
/// and/or whether we need to defer to System.Text.Json | ||
let mapTargetCaseArgs (element : JsonElement) options (props : PropertyInfo[]) : obj [] = | ||
match props with | ||
| [| singleCaseArg |] when propTypeRequiresConstruction singleCaseArg.PropertyType -> | ||
[| JsonSerializer.DeserializeElement (element, singleCaseArg.PropertyType, options) |] | ||
| multipleFieldsInCustomCaseType -> | ||
[| for fi in multipleFieldsInCustomCaseType -> | ||
match element.TryGetProperty fi.Name with | ||
| false, _ when fi.PropertyType.IsValueType -> Activator.CreateInstance fi.PropertyType | ||
| false, _ -> null | ||
| true, el when el.ValueKind = JsonValueKind.Null -> null | ||
| true, el -> JsonSerializer.DeserializeElement (el, fi.PropertyType, options) |] | ||
|
||
type UnionConverter<'T> (converterOptions) = | ||
inherit Serialization.JsonConverter<'T>() | ||
|
||
static let defaultConverterOptions = { discriminator = "case"; catchAllCase = None } | ||
|
||
let getOptions union = | ||
converterOptions :> IUnionConverterOptions | ||
|> defaultArg union.options | ||
|
||
new() = UnionConverter<'T>(defaultConverterOptions) | ||
new(discriminator: string, catchAllCase: string) = // Compatibility with Newtonsoft UnionConverter constructor | ||
UnionConverter<'T>({ discriminator = discriminator; catchAllCase = Option.ofObj catchAllCase}) | ||
|
||
override __.CanConvert(_) = Union.tryGetUnion (typeof<'T>) |> Option.isSome | ||
|
||
override __.Write(writer, value, options) = | ||
let value = box value | ||
let union = Union.tryGetUnion (typeof<'T>) |> Option.get | ||
let unionOptions = getOptions union | ||
let tag = union.tagReader value | ||
let case = union.cases.[tag] | ||
let fieldValues = union.fieldReader.[tag] value | ||
let fieldInfos = case.GetFields() | ||
|
||
writer.WriteStartObject() | ||
|
||
writer.WritePropertyName(unionOptions.Discriminator) | ||
writer.WriteStringValue(case.Name) | ||
|
||
match fieldInfos with | ||
| [| fi |] -> | ||
match fieldValues.[0] with | ||
| null when options.IgnoreNullValues -> () | ||
| fv -> | ||
let element = JsonSerializer.SerializeToElement(fv, options) | ||
match element.ValueKind with | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if? |
||
| JsonValueKind.Object -> | ||
// flatten the object properties into the same one as the discriminator | ||
for prop in element.EnumerateObject() do | ||
prop.WriteTo writer | ||
| _ -> | ||
writer.WritePropertyName(fi.Name) | ||
element.WriteTo writer | ||
| _ -> | ||
for fieldInfo, fieldValue in Seq.zip fieldInfos fieldValues do | ||
if fieldValue <> null || not options.IgnoreNullValues then | ||
writer.WritePropertyName(fieldInfo.Name) | ||
JsonSerializer.Serialize(writer, fieldValue, options) | ||
|
||
writer.WriteEndObject() | ||
|
||
override __.Read(reader, t : Type, options) = | ||
reader.ValidateTokenType(JsonTokenType.StartObject) | ||
use document = JsonDocument.ParseValue &reader | ||
let union = Union.tryGetUnion (typeof<'T>) |> Option.get | ||
let unionOptions = getOptions union | ||
let element = document.RootElement | ||
|
||
let targetCaseIndex = | ||
let inputCaseNameValue = element.GetProperty unionOptions.Discriminator |> string | ||
let findCaseNamed x = union.cases |> Array.tryFindIndex (fun case -> case.Name = x) | ||
match findCaseNamed inputCaseNameValue, unionOptions.CatchAllCase with | ||
| None, None -> | ||
sprintf "No case defined for '%s', and no catchAllCase nominated for '%s' on type '%s'" | ||
inputCaseNameValue typeof<UnionConverter<_>>.Name t.FullName |> invalidOp | ||
| Some foundIndex, _ -> foundIndex | ||
| None, Some catchAllCaseName -> | ||
match findCaseNamed catchAllCaseName with | ||
| None -> | ||
sprintf "No case defined for '%s', nominated catchAllCase: '%s' not found in type '%s'" | ||
inputCaseNameValue catchAllCaseName t.FullName |> invalidOp | ||
| Some foundIndex -> foundIndex | ||
|
||
let targetCaseFields, targetCaseCtor = union.cases.[targetCaseIndex].GetFields(), union.caseConstructor.[targetCaseIndex] | ||
targetCaseCtor (Union.mapTargetCaseArgs element options targetCaseFields) :?> 'T |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
module FsCodec.SystemTextJson.Tests.Samples | ||
|
||
open FsCodec.SystemTextJson | ||
open System | ||
open System.Runtime.Serialization | ||
open System.Text.Json.Serialization | ||
|
||
/// Endows any type that inherits this class with standard .NET comparison semantics using a supplied token identifier | ||
[<AbstractClass>] | ||
type Comparable<'TComp, 'Token when 'TComp :> Comparable<'TComp, 'Token> and 'Token : comparison>(token : 'Token) = | ||
member private __.Token = token // I can haz protected? | ||
override x.Equals y = match y with :? Comparable<'TComp, 'Token> as y -> x.Token = y.Token | _ -> false | ||
override __.GetHashCode() = hash token | ||
interface IComparable with | ||
member x.CompareTo y = | ||
match y with | ||
| :? Comparable<'TComp, 'Token> as y -> compare x.Token y.Token | ||
| _ -> invalidArg "y" "invalid comparand" | ||
|
||
/// SkuId strongly typed id | ||
[<Sealed; JsonConverter(typeof<SkuIdJsonConverter>); AutoSerializable(false); StructuredFormatDisplay("{Value}")>] | ||
// (Internally a string for most efficient copying semantics) | ||
type SkuId private (id : string) = | ||
inherit Comparable<SkuId, string>(id) | ||
[<IgnoreDataMember>] // Prevent swashbuckle inferring there's a "value" field | ||
member __.Value = id | ||
override __.ToString () = id | ||
new (guid: Guid) = SkuId (guid.ToString("N")) | ||
// NB tests (specifically, empty) lean on having a ctor of this shape | ||
new() = SkuId(Guid.NewGuid()) | ||
// NB for validation [and XSS] purposes we prove it translatable to a Guid | ||
static member Parse(input: string) = SkuId (Guid.Parse input) | ||
/// Represent as a Guid.ToString("N") output externally | ||
and SkuIdJsonConverter() = | ||
inherit JsonIsomorphism<SkuId, string>() | ||
/// Renders as per Guid.ToString("N") | ||
override __.Pickle value = value.Value | ||
/// Input must be a Guid.Parseable value | ||
override __.UnPickle input = SkuId.Parse input | ||
|
||
/// CartId strongly typed id | ||
[<Sealed; JsonConverter(typeof<CartIdJsonConverter>); AutoSerializable(false); StructuredFormatDisplay("{Value}")>] | ||
// (Internally a string for most efficient copying semantics) | ||
type CartId private (id : string) = | ||
inherit Comparable<CartId, string>(id) | ||
[<IgnoreDataMember>] // Prevent swashbuckle inferring there's a "value" field | ||
member __.Value = id | ||
override __.ToString () = id | ||
// NB tests lean on having a ctor of this shape | ||
new (guid: Guid) = CartId (guid.ToString("N")) | ||
// NB for validation [and XSS] purposes we must prove it translatable to a Guid | ||
static member Parse(input: string) = CartId (Guid.Parse input) | ||
/// Represent as a Guid.ToString("N") output externally | ||
and CartIdJsonConverter() = | ||
inherit JsonIsomorphism<CartId, string>() | ||
/// Renders as per Guid.ToString("N") | ||
override __.Pickle value = value.Value | ||
/// Input must be a Guid.Parseable value | ||
override __.UnPickle input = CartId.Parse input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason to make this mandatory ? the catchall is probably more likely to be customized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
andCatchAllCase
are both nullable onJsonUnionConverterOptionsAttribute
, 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
andCatchAllCaseOption
to represent the possible user intents when they are setting options via an attribute:Expected behaviour
Inferring a user's intended
DiscriminatorOption
when they set theDiscriminator
of the attribute is fine as it's binary and quite understandable when represented with null/not-null:However
CatchAllCaseOption
is ternary and I don't know what users would expect: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
andCatchAllCaseOption = 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
Stick with
JsonUnionConverterOptionsAttribute
but make the discriminator not required, and infer a nullCatchAllCase
to 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 the
catchAllCase
constructor parameter fromUnionConverter
so 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔... 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 casec) only by applying the JsonUnionConverterOptions would you every end up with catch-all semantics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, excellent. That works for me.
I will stick with a single options attribute and remove the notion of
CatchAllCaseOption.Default
by removingcatchAllCase
from theUnionConverter
constructor.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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,
and then use the attribute to specify a custom discriminator (but left out the catch-all case in said attribute)?
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?
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh okay, that was our misalignment. I had assumed you/the user might use
JsonUnionConverterOptionsAttribute
withoutJsonConverterAttribute
.('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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compact impl sounds good