Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enable relaxing of UnionEncoder requireRecordFields #62

Closed
wants to merge 1 commit into from
Closed

Enable relaxing of UnionEncoder requireRecordFields #62

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 37 additions & 15 deletions src/FsCodec.NewtonsoftJson/Codec.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,27 @@ module Core =
// TOCONSIDER as noted in the comments on RecyclableMemoryStream.ToArray, ideally we'd be continuing the rental and passing out a Span
ms.ToArray()

member __.Decode(json : byte[]) =
member __.Decode(json : byte[]) : 'a =
use ms = Utf8BytesEncoder.wrapAsStream json
use jsonReader = Utf8BytesEncoder.makeJsonReader ms
serializer.Deserialize<'T>(jsonReader)
let returnType = typeof<'a>
if returnType = typeof<Guid> then
json
|> System.Text.Encoding.ASCII.GetString
|> Guid.Parse
|> unbox
elif returnType = typeof<bool> then
json
|> System.Text.Encoding.ASCII.GetString
Copy link
Collaborator

@bartelink bartelink Feb 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this may be technically correct, I think I'd stick to using UTF8 in the interests of not overfitting? Same for Guid case.

Also this might make things easier / more sensible for STJ, which is v UTF8 centric

|> Boolean.Parse
|> unbox
elif returnType = typeof<char> then
json
|> System.Text.Encoding.UTF8.GetChars
|> Seq.head
|> unbox
else
serializer.Deserialize<'a> jsonReader

/// Provides Codecs that render to a UTF-8 array suitable for storage in Event Stores based using <c>Newtonsoft.Json</c> and the conventions implied by using
/// <c>TypeShape.UnionContract.UnionContractEncoder</c> - if you need full control and/or have have your own codecs, see <c>FsCodec.Codec.Create</c> instead
Expand All @@ -73,19 +90,18 @@ type Codec private () =
/// Configuration to be used by the underlying <c>Newtonsoft.Json</c> Serializer when encoding/decoding. Defaults to same as <c>Settings.Create()</c>
[<Optional; DefaultParameterValue(null)>] ?settings,
/// Enables one to fail encoder generation if union contains nullary cases. Defaults to <c>false</c>, i.e. permitting them
[<Optional; DefaultParameterValue(null)>] ?rejectNullaryCases)
[<Optional; DefaultParameterValue(null)>] ?rejectNullaryCases,
[<Optional; DefaultParameterValue(null)>] ?requireRecordFields)
: FsCodec.IEventCodec<'Event, byte[], 'Context> =

let settings = match settings with Some x -> x | None -> defaultSettings.Value
let bytesEncoder : TypeShape.UnionContract.IEncoder<_> = new Core.BytesEncoder(settings) :> _
let bytesEncoder : TypeShape.UnionContract.IEncoder<_> = Core.BytesEncoder(settings) :> _
let requireRecordFields = defaultArg requireRecordFields true
Internal.checkIfSupported<'Contract> requireRecordFields
let dataCodec =
TypeShape.UnionContract.UnionContractEncoder.Create<'Contract, byte[]>(
bytesEncoder,
// For now, we hard wire in disabling of non-record bodies as:
// a) it's extra yaks to shave
// b) it's questionable whether allowing one to define event contracts that preclude adding extra fields is a useful idea in the first instance
// See VerbatimUtf8EncoderTests.fs and InteropTests.fs - there are edge cases when `d` fields have null / zero-length / missing values
requireRecordFields = true,
requireRecordFields = requireRecordFields,
allowNullaryCases = not (defaultArg rejectNullaryCases false))

{ new FsCodec.IEventCodec<'Event, byte[], 'Context> with
Expand Down Expand Up @@ -119,14 +135,16 @@ type Codec private () =
/// Configuration to be used by the underlying <c>Newtonsoft.Json</c> Serializer when encoding/decoding. Defaults to same as <c>Settings.Create()</c>
[<Optional; DefaultParameterValue(null)>] ?settings,
/// Enables one to fail encoder generation if union contains nullary cases. Defaults to <c>false</c>, i.e. permitting them
[<Optional; DefaultParameterValue(null)>] ?rejectNullaryCases)
[<Optional; DefaultParameterValue(null)>] ?rejectNullaryCases,
/// Enables unions to contain a Guid or most primitives. Defaults to <c>true</c>, i.e. preventing Guids and primitives
Copy link
Collaborator

@bartelink bartelink Feb 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

require does not Enables ....; Perhaps something like:

/// Controls whether bodies of union cases are permitted to use primitive types,
/// which can be harder to evolve when compared to always wrapping in a record in order to force all fields to have names.
/// Defaults to <c>true</c>, i.e. preventing Guids and primitives from being used directly

[<Optional; DefaultParameterValue(null)>] ?requireRecordFields)
: FsCodec.IEventCodec<'Event, byte[], 'Context> =

let down (context, union) =
let c, m, t = down union
let m', eventId, correlationId, causationId = mapCausation (context, m)
c, m', eventId, correlationId, causationId, t
Codec.Create(up = up, down = down, ?settings = settings, ?rejectNullaryCases = rejectNullaryCases)
Codec.Create(up = up, down = down, ?settings = settings, ?rejectNullaryCases = rejectNullaryCases, ?requireRecordFields = requireRecordFields)

/// Generate an <code>IEventCodec</code> using the supplied <c>Newtonsoft.Json<c/> <c>settings</c>.
/// Uses <c>up</c> and <c>down</c> and <c>mapCausation</c> functions to facilitate upconversion/downconversion and correlation/causationId mapping
Expand All @@ -145,11 +163,13 @@ type Codec private () =
/// Configuration to be used by the underlying <c>Newtonsoft.Json</c> Serializer when encoding/decoding. Defaults to same as <c>Settings.Create()</c>
[<Optional; DefaultParameterValue(null)>] ?settings,
/// Enables one to fail encoder generation if union contains nullary cases. Defaults to <c>false</c>, i.e. permitting them
[<Optional; DefaultParameterValue(null)>] ?rejectNullaryCases)
[<Optional; DefaultParameterValue(null)>] ?rejectNullaryCases,
/// Enables unions to contain a Guid or most primitives. Defaults to <c>true</c>, i.e. preventing Guids and primitives
[<Optional; DefaultParameterValue(null)>] ?requireRecordFields)
: FsCodec.IEventCodec<'Event, byte[], obj> =

let mapCausation (_context : obj, m : 'Meta option) = m, Guid.NewGuid(), null, null
Codec.Create(up = up, down = down, mapCausation = mapCausation, ?settings = settings, ?rejectNullaryCases = rejectNullaryCases)
Codec.Create(up = up, down = down, mapCausation = mapCausation, ?settings = settings, ?rejectNullaryCases = rejectNullaryCases, ?requireRecordFields = requireRecordFields)

/// Generate an <code>IEventCodec</code> using the supplied <c>Newtonsoft.Json</c> <c>settings</c>.
/// The Event Type Names are inferred based on either explicit <c>DataMember(Name=</c> Attributes, or (if unspecified) the Discriminated Union Case Name
Expand All @@ -158,9 +178,11 @@ type Codec private () =
( // Configuration to be used by the underlying <c>Newtonsoft.Json</c> Serializer when encoding/decoding. Defaults to same as <c>Settings.Create()</c>
[<Optional; DefaultParameterValue(null)>] ?settings,
/// Enables one to fail encoder generation if union contains nullary cases. Defaults to <c>false</c>, i.e. permitting them
[<Optional; DefaultParameterValue(null)>] ?rejectNullaryCases)
[<Optional; DefaultParameterValue(null)>] ?rejectNullaryCases,
/// Enables unions to contain a Guid or most primitives. Defaults to <c>true</c>, i.e. preventing Guids and primitives
[<Optional; DefaultParameterValue(null)>] ?requireRecordFields)
: FsCodec.IEventCodec<'Union, byte[], obj> =

let up : FsCodec.ITimelineEvent<_> * 'Union -> 'Union = snd
let down (event : 'Union) = event, None, None
Codec.Create(up = up, down = down, ?settings = settings, ?rejectNullaryCases = rejectNullaryCases)
Codec.Create(up = up, down = down, ?settings = settings, ?rejectNullaryCases = rejectNullaryCases, ?requireRecordFields = requireRecordFields)
1 change: 0 additions & 1 deletion src/FsCodec.NewtonsoftJson/FsCodec.NewtonsoftJson.fsproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
<PackageReference Include="Microsoft.IO.RecyclableMemoryStream" Version="1.2.2" />
<PackageReference Include="Newtonsoft.Json" Version="11.0.2" />
<PackageReference Include="System.Buffers" Version="4.5.0" />
<PackageReference Include="TypeShape" Version="8.0.0" />
</ItemGroup>

<ItemGroup>
Expand Down
6 changes: 4 additions & 2 deletions src/FsCodec.NewtonsoftJson/VerbatimUtf8Converter.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,7 @@ type VerbatimUtf8JsonConverter() =

override __.ReadJson(reader : JsonReader, _ : Type, _ : obj, _ : JsonSerializer) =
let token = JToken.Load reader
if token.Type = JTokenType.Null then null
else token |> string |> enc.GetBytes |> box
match token.Type with
| JTokenType.Null -> null
| JTokenType.Float -> reader.Value :?> double |> fun x -> x.ToString "r" |> enc.GetBytes |> box
| _ -> token |> string |> enc.GetBytes |> box
1 change: 0 additions & 1 deletion src/FsCodec.SystemTextJson/FsCodec.SystemTextJson.fsproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
<PackageReference Include="FSharp.Core" Version="4.3.4" />

<PackageReference Include="System.Text.Json" Version="5.0.0-preview.3.20214.6" />
<PackageReference Include="TypeShape" Version="8.0.0" />
</ItemGroup>

<ItemGroup>
Expand Down
8 changes: 8 additions & 0 deletions src/FsCodec/FsCodec.fsproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
</PropertyGroup>

<ItemGroup>
<Compile Include="Internal.fs" />
<Compile Include="FsCodec.fs" />
<Compile Include="Codec.fs" />
<Compile Include="StreamName.fs" />
Expand All @@ -21,6 +22,13 @@
<PackageReference Include="FSharp.Core" Version="3.1.2.5" Condition=" '$(TargetFramework)' == 'net461' " />
<PackageReference Include="FSharp.Core" Version="4.3.4" Condition=" '$(TargetFramework)' == 'netstandard2.0' " />
<PackageReference Include="FSharp.UMX" Version="1.0.0" />
<PackageReference Include="TypeShape" Version="8.0.0" />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly reticent to add this dependency at this level.
My thinking is that the FsCodec package to date has largely only supplied contracts, and does not force many dependencies.
The scenario I had in my head is if someone is using an external library to provide the Codecs (and/or you're using the Create overloads that take an encode and tryDecode pair).
I'm not sure this matters.

This also slightly overlaps with my stance on using InternalsVisible and making things fully DRY by stuffing things into the core. My preferred technique is to have a source file somewhere in the tree and then add it as a reference to multiple projects. This allows one to e.g. keep FsCodec at 2.0.0 while still allowing one to update FsCodec.SystemTextJson to 2.4.0 without necessitating the same change for other modules in a subsystem that still use FsCodec.NewtonsoftJson.

I think this pattern should be followable here?

</ItemGroup>

<ItemGroup>
<AssemblyAttribute Include="System.Runtime.CompilerServices.InternalsVisibleTo">
<_Parameter1>FsCodec.NewtonsoftJson</_Parameter1>
</AssemblyAttribute>
</ItemGroup>

</Project>
43 changes: 43 additions & 0 deletions src/FsCodec/Internal.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
module internal Internal

open TypeShape.Core

let checkIfSupported<'Contract> requireRecordFields =
if not requireRecordFields then
let shape =
match shapeof<'Contract> with
| Shape.FSharpUnion (:? ShapeFSharpUnion<'Contract> as s) -> s
| _ ->
sprintf "Type '%O' is not an F# union" typeof<'Contract>
|> invalidArg "Union"
let isAllowed (scase : ShapeFSharpUnionCase<_>) =
match scase.Fields with
| [| field |] ->
match field.Member with
// non-primitives
| Shape.FSharpRecord _
| Shape.Guid _

// primitives
| Shape.Bool _
| Shape.Byte _
| Shape.SByte _
| Shape.Int16 _
| Shape.Int32 _
| Shape.Int64 _
//| Shape.IntPtr _ // unsupported
| Shape.UInt16 _
| Shape.UInt32 _
| Shape.UInt64 _
//| Shape.UIntPtr _ // unsupported
| Shape.Single _
| Shape.Double _
| Shape.Char _ -> true
| _ -> false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, consider explicitly listing the cases rather than leaving this open. If Half appears in this union, whats our stance?

| [||] -> true // allows all nullary cases, but a subsequent check is done by UnionContractEncoder.Create with `allowNullaryCases`
| _ -> false
shape.UnionCases
|> Array.tryFind (not << isAllowed)
|> function
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option.iter ?

| None -> ()
| Some x -> failwithf "The '%s' case has an unsupported type: '%s'" x.CaseInfo.Name x.Fields.[0].Member.Type.FullName
29 changes: 25 additions & 4 deletions tests/FsCodec.NewtonsoftJson.Tests/VerbatimUtf8ConverterTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,24 @@ type U =
//| DT of DateTime // Have not analyzed but seems to be same issue as DTO
| EDto of EmbeddedDateTimeOffset
| ES of EmbeddedString
//| I of int // works but removed as no other useful top level values work
| Guid of Guid
| N

// primitives
| Boolean of bool
| Byte of byte
| SByte of sbyte
| Int16 of int16
| UInt16 of uint16
| Int32 of int32
| UInt32 of uint32
| Int64 of int64
| UInt64 of uint64
//| IntPtr of IntPtr // unsupported
//| UIntPtr of UIntPtr // unsupported
| Char of char
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about byte, sbyte and char - stuff that naturally maps to JSON seems a reasonable baseline ?

(remember Cosmos, and also things like SqlStreamStore.Postgres will impose their own constraints)

Also, related to comments on checkIfSupported, maybe the rules as to what's roundtripable should live in the store?

A case in point is VerbatimUtf8Converter: it is used by Equinox.Cosmos, but Equinox.CosmosStore (the V3 master impl), uses a different local impl (which, when we take it from jet/equinox#197 will be based on System.Text.Json).

Moving the code and/or mechanisms to live with the store might be an approach to take?

A counterpoint: While EventStore is happy to store any byte[] blob, it also has an IsJson which allows one to write JS projections, and in V20+ it has a Content-Type.

This suggests that a reasonable codec library might be general in nature and allow one to define a profile you're targetting in the abstract, which a concrete store library can define.

Going down that road would also mesh will with the notion of a contract checker which can be used to compile time check a contract adheres to rules - i.e. we might want to be able to define a convention in an integration test library that says "For this Domain lib, event contracts must be rountrippable on Equinox.Cosmos >=V3 and Equinox.EventStore >=V2, and all enums must be represented as strings (which implies having to add the converter attributes on the type)"

In that world, perhaps an FsCodec.Contracts library might depend on TypeShape and implement all this rather than shoving everything into FsCodec ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A case in point is VerbatimUtf8Converter: it is used by Equinox.Cosmos, but Equinox.CosmosStore (the V3 master impl), uses a different local impl (which, when we take it from jet/equinox#197 will be based on System.Text.Json).

Moving the code and/or mechanisms to live with the store might be an approach to take?

Hm, pendulum swing: but Propulsion.Kafka.Codec also uses it.

OK, I think that suggests that there are multiple axes of constraints:

  • store-imposed: Equinox.Cosmos only wants JSON
  • more arbitrary: Propulsion.Kafka is looking to roundtrip a representation of an event from a store over Kafka
  • self-imposed: using Equinox.EventStore but we want to have all bodies be valid JSON so we can do JS projections
  • style: our team is using F# and we have decided our rule is we use records no matter what for reasons discussed in Relaxing requireRecordFields = true #61
  • not all roundtrips may be implemented in all libraries - i.e. some thorny thing might work in FsCodec.SystemTextJson but not in FsCodec.NewtonsoftJson

=> The rule checker may have a common impl to a degree, but we share any shareable bits by coimpiling the common code into a specific impl vs putting it into FsCodec

| Double of double
| Single of single
interface TypeShape.UnionContract.IUnionContract

type [<NoEquality; NoComparison; JsonObject(ItemRequired=Required.Always)>]
Expand Down Expand Up @@ -57,7 +73,7 @@ let mkBatch (encoded : FsCodec.IEventData<byte[]>) : Batch =

module VerbatimUtf8Tests = // not a module or CI will fail for net461

let eventCodec = Codec.Create<Union>()
let eventCodec = Codec.Create<Union>(requireRecordFields=false)

let [<Fact>] ``encodes correctly`` () =
let input = Union.A { embed = "\"" }
Expand All @@ -71,7 +87,7 @@ module VerbatimUtf8Tests = // not a module or CI will fail for net461
input =! decoded

let defaultSettings = Settings.CreateDefault()
let defaultEventCodec = Codec.Create<U>(defaultSettings)
let defaultEventCodec = Codec.Create<U>(defaultSettings, requireRecordFields=false)

let [<Property>] ``round-trips diverse bodies correctly`` (x: U) =
let encoded = defaultEventCodec.Encode(None,x)
Expand All @@ -80,7 +96,12 @@ module VerbatimUtf8Tests = // not a module or CI will fail for net461
let des = JsonConvert.DeserializeObject<Batch>(ser, defaultSettings)
let loaded = FsCodec.Core.TimelineEvent.Create(-1L, des.e.[0].c, des.e.[0].d)
let decoded = defaultEventCodec.TryDecode loaded |> Option.get
x =! decoded
match x, decoded with
| U.Double x, U.Double d when Double.IsNaN x && Double.IsNaN d -> ()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can preserve the symmetry of having a type check either way by doing something like
| U.Double x, U.Double d when Double.IsNaN x => test <@ Double.IsNaN d @> (or true =! Double.isNan d is you prefer)

| U.Single x, U.Single d when Single.IsNaN x && Single.IsNaN d -> ()
| U.Double x, U.Double d -> Assert.Equal( x, d, 10)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 does unquote provide a way to express this?

| U.Single x, U.Single d -> Assert.Equal(double x, double d, 10)
| _ -> x =! decoded

// https://github.com/JamesNK/Newtonsoft.Json/issues/862 // doesnt apply to this case
let [<Fact>] ``Codec does not fall prey to Date-strings being mutilated`` () =
Expand Down