-
Notifications
You must be signed in to change notification settings - Fork 1k
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
FSharp API: fixed problem with JObject deserialization #1203
FSharp API: fixed problem with JObject deserialization #1203
Conversation
How will this play with F# Akka.Persistence? The problem is in the serializer, and I'm guessing that we might end up findning these JObjects in a lot of unexpected places eventually (?) The fix looks good otherwise |
Isn't the JObject on the read-side only? |
@rogeralsing @Aaronontheweb This should be also included in Akka.Persistence.FSharp. |
898abd9
to
a0d5a28
Compare
@Horusiath looks good to me, but a suggestion: could we add a spec to the F# specs somewhere that tests for this issue specifically? Or modify an existing DU serialization spec to address the issue that you pointed out earlier? Given the trouble we've had with this issue it seems like an area where it's worthwhile to do that :p |
It's worth noting that this is more of a workaround than a final fix, e.g. if a C# system would for some reason consume F# messages, the same error will show up again. And if a DU is passed remote to a consistent hash router to be forwarded to a F# actor, the hashing will occur on the JObject as it has not yet been converted yet. |
Argh! In that case... what would we need to do to completely solve the problem. |
Sorry, but this one still doesn't solve the problem - I've found that JSON.NET fails on deserialization of integer values inside DUs (as they are wrapped in dedicated struct to maintain int type info). |
Hold your horses :) You need to pass the So this line:
and
Where the ourNewtonSoftJsonSerializer is our newtonsoft json serialzer. |
@rogeralsing still won't work, because even if message passing between F# actors will be correct, everything will fail on F#/C# actors interop or even in ask operators (for both F# and C#) used outside an actor. This must be solved from the default serializer side. |
a0d5a28
to
7d891f8
Compare
case when JObject contains invalid type fix moved to Akka.Persistence.FSharp reliable DU serialization tests some serious hack for whole F# scope extended JObject hack on akka.persistence.fsharp
7d891f8
to
91c86c4
Compare
This time I've created a little bigger workaround that is reusable (applied in both Akka.FSharp and Akka.Persistence.FSharp) and works also on ask operator |
Add some comments on the affected code, if we find a better way to solve this later. |
👍 looks solid |
@@ -29,6 +29,7 @@ public class NewtonSoftJsonSerializer : Serializer | |||
private readonly JsonSerializer _serializer; | |||
|
|||
public JsonSerializerSettings Settings { get { return _settings; } } | |||
public object Serializer { get { return _serializer; } } |
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.
Should we mark this field as internal
here? Any reason to make it public?
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.
To implement this workaround, field must be public (or reacher via reflection) and it must return object and not direct serializer (to maintain Akka.FSharp free from direct dependency on JSON.NET).
Just so I understand this better - could you walk me through where we're using FsPickler and JSON.NET in the serialization flow for F#? I took a look through it and think I have an idea as to what's happening but I'm not 100% there (although my F# is getting better!) |
@Aaronontheweb JSON.NET is default serialization mechanism in F# .NET. But for few custom type based on F# quotations we use FsPickler rigth now. |
FSharp API: fixed problem with JObject deserialization
Did this make it in a release already? |
This should fix #999 . The issue was that F# actor applied pattern matching on the generic type in case when message was not fully deserialized (it was an intermediate representation in form of json.net
JObject
). In result any try of pattern matching was failing.This PR solves that problem by adding additional step - in case when we have a
JObject
instance passed it tries a build inToObject(type)
conversion and if it succeeds, pass the result to the actor's receive function.