added reproduction for DU serialization #5196

Aaronontheweb · 2021-08-12T19:33:24Z

Aaronontheweb · 2021-08-12T19:38:18Z

src/core/Akka.FSharp.Tests/RemoteSpecs.fs

+    | D of int
+
+type RemoteSpecs(output:ITestOutputHelper) as this =
+    inherit AkkaSpec((remoteConfig 0), output)


A little convoluted, but this is how you can pass through the XUnit ITestOutputHelper and the AkkaSpec / TestKit base members into an F# test

Aaronontheweb · 2021-08-12T19:51:49Z

src/core/Akka.FSharp.Tests/RemoteSpecs.fs

+            spawne clientSys "a-1" <@ actorOf2 (fun mailbox msg -> 
+                   match msg with
+                   | C("a-11", B(11, "a-12")) -> mailbox.Sender() <! msg
+                   | _ -> mailbox.Unhandled msg) @>


We have a clear reproduction here - we receive a JObject instead of a DU here.

Updated this to use Hyperion - even a basic Newtonsoft.Json serialization setup can't handle DUs

If I remember correctly - but I might be wrong - the issue might be caused by the fact that you either didn't configured JSON.NET to include type name as part of the payload or to use it if it was included. Since at the deserialization time payload type is still unknown and JSON.NET will use JObject as representation of System.Object instance. Casting it to typed actor message afterwards is already too late and will fail.

Yes, with type hints Newtonsoft.Json correctly deserializes DUs:

akka.net/src/core/Akka.FSharp.Tests/SerializationSpecs.fs

Lines 41 to 50 in d201452

member this.VerifySerialization msg =

let serializer = this.Sys.Serialization.FindSerializerFor msg

let t = msg.GetType()

let bytes = this.Sys.Serialization.Serialize msg

let deserialized =

match serializer :> obj with

| :? SerializerWithStringManifest as str -> this.Sys.Serialization.Deserialize(bytes, serializer.Identifier, str.Manifest msg)

| _ -> this.Sys.Serialization.Deserialize(bytes, serializer.Identifier, t)

Assert.Equal(msg, deserialized)

We disabled exposing type hints during the advent of Akka.NET v1.3 because of mscorlib namespace issues introduced by the creation of .NET Core: https://twitter.com/Danthar/status/1426204694310014981 cc @Danthar

We could fix the issue by turning type hints on, but it would blow up anyone doing .NET Framework --> .NET Core interop with Newtonsoft.Json (population of people doing that is great than zero) - or we could make it configurable to turn that setting on or off.

I think you can potentially handle that as special case inside of typed actor itself:

akka.net/src/core/Akka.FSharp/FsApi.fs

Lines 291 to 297 in d201452

override x.OnReceive msg =

match state with

| Func f ->

match msg with

| :? 'Message as m -> state <- f m

| _ -> x.Unhandled msg

| Return _ -> x.PostStop()

Add another match case to check if obj is JObject and if so use its ToObject method.

Now checking for whether or not manifests are emitted

Aaronontheweb

Described my work on adding better F# DU testing support going forward - looks like Newtonsoft.Json really can't support DUs even with DiscriminatedUnionConverter

Aaronontheweb · 2021-08-13T01:55:54Z

src/core/Akka.FSharp.Tests/RemoteSpecs.fs

+            spawne clientSys "a-1" <@ actorOf2 (fun mailbox msg -> 
+                   match msg with
+                   | C("a-11", B(11, "a-12")) -> mailbox.Sender() <! msg
+                   | _ -> mailbox.Unhandled msg) @>


Updated this to use Hyperion - even a basic Newtonsoft.Json serialization setup can't handle DUs

Aaronontheweb · 2021-08-13T01:56:45Z

src/core/Akka.FSharp.Tests/SerializationSpecs.fs

+    [<MemberData(nameof(SerializationSpecs.FSharpTypes))>]
+    member _.``Must verify serialization of F#-specific types`` (t:obj) =
+        this.VerifySerialization t
+        // uncomment the section below when experimenting with IncludeManifest = true on the Newtonsoft.Json serializer


If we enable IncludeManifest = true in the NewtonsoftJsonSerializer all of these tests will pass, including the ones currently using Hyperion right now. However, I'm not sure how significant of a change that might be and I'm loathe to touch it given that it powers so many Akka.Persistence serialization schemes.

Aaronontheweb · 2021-08-13T01:56:52Z

src/core/Akka.FSharp/FsApi.fs

@@ -35,7 +35,7 @@ module Serialization =
        override __.FromBinary(bytes, _) = deserializeFromBinary fsp bytes


-    let internal exprSerializationSupport (system: ActorSystem) =
+    let exprSerializationSupport (system: ActorSystem) =


made this public

Aaronontheweb · 2021-08-13T01:57:57Z

src/core/Akka.FSharp.Tests/SerializationSpecs.fs

+
+
+    [<Fact(Skip="JSON.NET really does not support even basic DU serialization")>]
+    member _.``JSON.NET must serialize DUs`` () =


Smoking gun - JsonConvert can't deserialize a DU even with DiscriminatedUnionConverter and none of our other customizations to Newtonsoft.Json. Will need to file an issue over there.

Arkatufus

LGTM

added reproduction for DU serialization

68beb1b

close akkadotnet#5194

Aaronontheweb added akka-fsharp serialization labels Aug 12, 2021

Aaronontheweb commented Aug 12, 2021

View reviewed changes

exposed exprSerializationSupport to public

540413c

Aaronontheweb commented Aug 12, 2021

View reviewed changes

Aaronontheweb mentioned this pull request Aug 12, 2021

reuse the Newtonsoft.Json serializer #5197

Closed

Aaronontheweb added 8 commits August 12, 2021 19:36

validated that record-based serialization works out of the box

922c9c9

added dedicated serialization specs

5fec47b

cleaned up RemoteSpecs and SerializationSpecs

66f3e14

Now checking for whether or not manifests are emitted

added hardened test case to validate that DU serialization does not work

c519b95

added spec that proves this is a Newtonsoft.Json issue

3670c8a

restored common.props

5c7636f

disabled JSON.NET repro spec

f12ae76

migrated to using Hyperion for DU tests

cbcb338

Aaronontheweb commented Aug 13, 2021

View reviewed changes

Aaronontheweb marked this pull request as ready for review August 13, 2021 01:58

Aaronontheweb added 2 commits August 12, 2021 20:58

removed bad test

1289790

Merge branch 'dev' into fsharp/fix-5194-DUSerialization

9f32abd

Arkatufus approved these changes Aug 13, 2021

View reviewed changes

Arkatufus merged commit d201452 into akkadotnet:dev Aug 13, 2021

Aaronontheweb deleted the fsharp/fix-5194-DUSerialization branch August 13, 2021 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added reproduction for DU serialization #5196

added reproduction for DU serialization #5196

Aaronontheweb commented Aug 12, 2021

Aaronontheweb Aug 12, 2021

Aaronontheweb Aug 12, 2021

Aaronontheweb Aug 13, 2021

Horusiath Aug 13, 2021 •

edited

Loading

Aaronontheweb Aug 13, 2021

Horusiath Aug 14, 2021 •

edited

Loading

Aaronontheweb left a comment

Aaronontheweb Aug 13, 2021

Aaronontheweb Aug 13, 2021

Aaronontheweb Aug 13, 2021

Aaronontheweb Aug 13, 2021

Arkatufus left a comment

	member this.VerifySerialization msg =
	let serializer = this.Sys.Serialization.FindSerializerFor msg
	let t = msg.GetType()
	let bytes = this.Sys.Serialization.Serialize msg
	let deserialized =
	match serializer :> obj with
	\| :? SerializerWithStringManifest as str -> this.Sys.Serialization.Deserialize(bytes, serializer.Identifier, str.Manifest msg)
	\| _ -> this.Sys.Serialization.Deserialize(bytes, serializer.Identifier, t)

	Assert.Equal(msg, deserialized)

	override x.OnReceive msg =
	match state with
	\| Func f ->
	match msg with
	\| :? 'Message as m -> state <- f m
	\| _ -> x.Unhandled msg
	\| Return _ -> x.PostStop()



		[<Fact(Skip="JSON.NET really does not support even basic DU serialization")>]
		member _.``JSON.NET must serialize DUs`` () =

added reproduction for DU serialization #5196

added reproduction for DU serialization #5196

Conversation

Aaronontheweb commented Aug 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Horusiath Aug 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Horusiath Aug 14, 2021 • edited Loading

Choose a reason for hiding this comment

Aaronontheweb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arkatufus left a comment

Choose a reason for hiding this comment

Horusiath Aug 13, 2021 •

edited

Loading

Horusiath Aug 14, 2021 •

edited

Loading