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.

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

Enable relaxing of UnionEncoder requireRecordFields #62

Closed
wants to merge 1 commit into from

Conversation

dharmaturtle
Copy link
Contributor

@dharmaturtle dharmaturtle commented Feb 14, 2021

I only did Newtonsoft.Json so far because System.Text.Json is proving annoying. I'll elaborate further if you like what's here.

Ref: #61

@dharmaturtle dharmaturtle changed the title Added requireRecordFields option to Newtonsoft.Json Added requireRecordFields Feb 14, 2021
@dharmaturtle dharmaturtle force-pushed the requireRecordFields branch 4 times, most recently from 45854e8 to a374251 Compare February 15, 2021 01:38
@@ -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?

| 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

@@ -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)

match x, decoded with
| U.Double x, U.Double d when Double.IsNaN x && Double.IsNaN d -> ()
| 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?

| 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?

| _ -> 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 ?

|> unbox
elif returnType = typeof<bool> then
json
|> System.Text.Encoding.ASCII.GetString
Copy link
Collaborator

@bartelink bartelink Feb 15, 2021

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

@bartelink
Copy link
Collaborator

because System.Text.Json is proving annoying

I know that feeling (and many around the world do!) 😄 see #59

@bartelink
Copy link
Collaborator

Thank you for the work thus far - this is looking very promising. Re #61, I had not done much thinking through / imagining of how this might look overall as your implementation is making me do now.

I covered some of these tradeoffs more in the code review comments; there may be things I've yet to consider which will prove some of the following wrong/unworkable, but my thoughts are:

  • I had thought that in principle each store should implement it's own checker - you've shown it can live here, and I like it
  • I still think I don't want to stuff things into the FsCodec core module but I may ultimately be wrong - someone that wants to write their own mapping functions and/or use a different serializer, they get to define (and maintain) their own rules. Putting helper modules in there also gives a false impression that FsCodec is a massively complex library; the intent is really to define a very minimal set of contracts indeed at that level.
  • considering how a contract style checker might be organized can be a useful tool for thinking about where stuff should live; expanding below on that concept

There are multiple axes of constraints:

  • store-imposed: Equinox.Cosmos can only store JSON - i.e. embedded byte[] content needs to be base64 encoded
  • e.g. DynamoDb might impose slightly different constraints than Cosmos
  • more arbitrary: Propulsion.Kafka is looking to roundtrip a representation of an event from a store over Kafka, which in general is fine if its UTF8
  • self-imposed: if usin using Equinox.EventStore, any byte[] is fine 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
  • We want to define sets of rules that our IUnionContract types should observe as described in feat: Contract types validation #50 (One could imagine building a Roslyn analyzer checking the rules on the fly too)

@bartelink bartelink changed the title Added requireRecordFields Enable relaxing of UnionEncoder requireRecordFields Feb 15, 2021
@@ -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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants