fix(JsonPicker, JsonIsomorphism, UnionConverter)!: Reduce greediness #113
Conversation
| [<AbstractClass>] | ||
| type JsonPickler<'T>() = | ||
| inherit Serialization.JsonConverter<'T>() | ||
|
|
||
| static let isMatchingType = |
There was a problem hiding this comment.
@eiriktsarpalis Apologies for yet more atting but I don't think I can answer it without you...
- what desirable behaviors am I likely to be removing/breaking with this?
- can you point me to anything that implements similar behavior, perhaps with some tests/docs/use cases?
My thinking is that I should have removed it long ago, as it does not fail the tests, and I can't think of a use case...
i.e. am I missing anything regarding some polymorphic behavior that's going over my head?
There was a problem hiding this comment.
Assuming you only use it to deserialize values (which is a naturally covariant operation) and never to serialize then it should work expected.
There was a problem hiding this comment.
Hm can you expand on that please? i.e. can you paint me a scenario as to how you do that, composing something with multiple pickers or something? i.e. what sort of a global hook would it afford one?
There was a problem hiding this comment.
Assuming you have Foo <: IFoo and you define a converter JsonConverter<Foo> whose CanConvert method returns true when given typeof<IFoo>, then the serializer will use that as the converter for IFoo. The downside however is that it will fail with a cast exception if you ever attempt to serialize values of type IFoo (since they may not be castable to Foo).
There was a problem hiding this comment.
Thanks! The main thing is that there's a pretty clear extension point for people to override the default behavior
(and it matches the general expectations one would have from STJ in terms of not being greedy by default)
Thankfully I've the luxury of not thinking too deeply about polymorphism wrt serialization in my day to day work ;)
Also now (V3 RC) is the right time to take out the behavior, if it's ever going to be done...
bartelink
changed the title
bartelink
force-pushed
the
cleanups
branch
3 times, most recently
from
097c9ec
to
e4d99a2
Compare
bartelink
changed the title
| abstract Read: reader: byref<Utf8JsonReader> * options: JsonSerializerOptions -> 'T | ||
|
|
||
| override _.CanConvert t = isMatchingType t |
There was a problem hiding this comment.
note base class has an (=) t semantic (which also matches the impl the NSJ version implements directly)
| @@ -5,12 +5,13 @@ open System.Text.Json.Serialization | |||
| type UnionOrTypeSafeEnumConverterFactory(typeSafeEnum, union) = | |||
| inherit JsonConverterFactory() | |||
|
|
|||
| static let hasConverterAttribute: System.Type -> bool = memoize _.IsDefined(typeof<JsonConverterAttribute>, true) | |||
| static let cache = System.Collections.Concurrent.ConcurrentDictionary<System.Type, bool>() | |||
| static let typeHasConverterAttribute t: bool = cache.GetOrAdd(t, fun (t: System.Type) -> t.IsDefined(typeof<JsonConverterAttribute>, ``inherit`` = false)) | |||
There was a problem hiding this comment.
note flipping behavior to be less greedy (not honoring attributes on base types), to match Newtonsoft impl
Removes two speculative 'features' that can be reaonably argued to be misfeatures on the basis that they were not documented, nor were they covered by tests:
JsonPicklerno longer supports implicit application (via inclusion in the converters list) to parent types or interfacesJsonIsomorphism, whichinherits from it)SystemTextJson.Options:autoTypeSafeEnumToJsonString, union = autoUnionToJsonObjectare skipped if the type nominates a specific converter directly (used to also honor tags on base classes/interfaces)In both cases, the workaround if this greediness was intentionally required, is to override the
CanConvertmember on the converter (orUnionOrTypeSafeEnumConverterFactory)