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

Orleans unable to deep copy JObject #299

Closed
nehmebilal opened this issue Apr 2, 2015 · 14 comments
Closed

Orleans unable to deep copy JObject #299

nehmebilal opened this issue Apr 2, 2015 · 14 comments
Assignees

Comments

@nehmebilal
Copy link

Orleans is generating the following code to deep copy JObject; which doesn't compile because JPropertyKeyedCollection is an internal class to JObject.

object temp12 = ((Newtonsoft.Json.Linq.JPropertyKeyedCollection)(Orleans.Serialization.SerializationManager.DeepCopyInner(fieldInfo12.GetValue(input))));

Console output:
error CS0122: 'Newtonsoft.Json.Linq.JPropertyKeyedCollection' is inaccessible due to its protection level

@gabikliot
Copy link
Contributor

Can you please share the value of this JObject, the string it was created from.

@nehmebilal
Copy link
Author

The value doesn't really matter; the error is happening at compile time. Just create a method in a grain that returns Task<JObject> and compile.

@gabikliot
Copy link
Contributor

We looked into this issue and it's a bit subtle.
For context:
http://www.newtonsoft.com/json/help/html/T_Newtonsoft_Json_Linq_JObject.htm
https://github.com/JamesNK/Newtonsoft.Json/blob/master/Src/Newtonsoft.Json/Linq/JObject.cs

JObject is not serializable.
As such, it will fail to serialize if attempted in any framework, such as for example WCF or passing it via app domain boundaries.
In Orleans, we sometimes go way and beyond and try to create Orleans custom serializers even for types that are not serializable. We do that for any type that appears in the grain interface signature. In this particular case, we attempt and fail (rightfully). The issue I see here is that we fail with a rather cryptic exception about some inner type inside JObject. Instead, we should have failed saying JObject is not serializable, or maybe even not attempt to create a serializer for that type and just let the application fail at runtime, when it tries to pas that object (since we did not generate Orleans serializer we will use .NET serializer and it will fail since JObject is not serializable).
So on that issue, except for better error reporting, we don't see any issue.

The bigger question is whether you really need to pass the non-serializable JObject in remote calls. Why don't you turn it into string (JObject.ToString) and on the recipient side parse it back (JObject.Parse). Would that solve your problem?

@nehmebilal
Copy link
Author

Thanks for the detailed explanation!
Yes converting to/from a string is what we're currently doing and it works fine; it just seemed like an extra step that both the caller and callee must perform. Considering that JObject is commonly used, would it make sense for Orleans to do the conversion to/from a string behind the scenes? Isn't that what web apis do with http requests containing a JObject?

@sergeybykov
Copy link
Contributor

I see more than one option here.

If JObject was .NET serializable, everything would presumably just work. I think this would be best because it would encapsulate the conversion logic within the Newtonsoft library and would evolve in the future as necessary. Not sure how likely this is, considering that we are talking about making types of a serialization library themselves serializable in a different context.

We could add special serializers for JObject and related types. A little bit of work. The concern is it could break with any breaking change or extension of the Newtonsoft code.

Another potential option is to treat JObject and related types as special cases and convert them to/from strings on the fly like we convert this references passed by grain code as arguments in codegen. This would only work for simple case, not for arrays, dictionaries, etc. So I don't think this is a good option.

Isn't that what web apis do with http requests containing a JObject?

Can you point to an example?

@ReubenBond
Copy link
Member

I've run into this problem multiple times, too. Eg, when I first started trying to implement Event Sourcing, properties of type object would always deserialize as JObject/JValue, which would cause exceptions to be thrown. I fixed it by deserializing into the correct types, which required a bunch of codegen (which was needed anyway).

Could we add an attribute to tell Orleans' to codegen serializers for types in other assemblies? Something akin to KnownType.

We could fix this for JObject/JValue by being able to externally attach a serializer to those types, then we could just serialize/deserialize using JSON.NET instead of the generated serializer.

@gabikliot
Copy link
Contributor

@ReubenBond , we have been thinking about this issue recently, so let me please share some of our thoughts.
Edit: There are 2 different questions:
(a) How to take an object of type JObject and turn it into byte[].
(b) How to take a general object graph with application types inside and serialize them into Json format (by using JSON.NET or whatever).

Question (b) is an important and valid question, but this issue is NOT about it. We can open a separate issue about (b). For me, it would be somewhat similar to issue #37 - how to use Bond in Orleans.

We are discussing question (a) here:
We can serialize JObject to byte[] in Orleans in multiple ways:

  1. We can write our own custom serializer for JObject/JToken/… by simply using JObject.ToString/JObject.Parse. We already do that for a bunch of heavily used .NET types, such as Dictionaries/Lists/… (see here).
  2. We can provide an extension point for application to define its own Serializer for JObject. We already have such capability. This will NOT work out of the box now, since currently this extensibility relies on annotating the type, and since the application does not own JObject type, it cannot do this. But of course it would be easy to remove this requirement and enable this extensibility.
    So doing (a) is easy.

The question is: should we?
Why would anyone want to serialize JObject to binary:

  • Despite the fact that JObject is not serializable. Meaning that whomever designed JObject did not intend it to be serialized. Why would anyone want to force serializing something that was not meant to be serialized?
  • What are the scenarios where one needs to pass JObject across the wite into a remote component? Shouldn’t he be passing the json document/json string, instead of the navigatable/parsed data structure that represents it? This would be similar to serializing XMLElement, which is btw also not serializable?

Perhaps there are scenarios where serializing JObject is required?

@ReubenBond
Copy link
Member

My apologies, I wasn't clear. I'm suggesting we implement (2), the extension point for attaching serializers to arbitrary types.

JObject is not [Serializable], but it can be serialized quite easily.

Edit: I'd serialize JObject & XmlElement to their native types, JSON & XML respectively, which I believe is the intention behind them not being [Serializable].

@nehmebilal
Copy link
Author

Isn't that what web apis do with http requests containing a JObject?

When you return a JObject from an ApiController, it just works. In fact, tools such as Postman are able to recognize JSON and nicely format the output of a request returning a JObject. I guess this means that they are doing the conversion to/from string automatically. Probably not a very strong use case though.

What are the scenarios where one needs to pass JObject across the wite into a remote component? Shouldn’t he be passing the json document/json string, instead of the navigatable/parsed data structure that represents it? This would be similar to serializing XMLElement, which is btw also not serializable?

I personally don't have a very strong use case. It seems like a nice to have but not necessary feature.
In on example, we wanted to codegen the ApiController based on the grain interface (i.e. we created some attribute that we place on the grain interface and then generate an ApiController that simply delegates the call to the grain). However, we couldn't codegen an ApiController that returns a JObject because the corresponding grain is not allowed to return a JObject.

@ReubenBond
Copy link
Member

@nehmebilal usually you would return a concrete type from your ApiController, so that should be fine - you can codegen from your grain interfaces (just be wary of possibly security implications).

However, if you have a method which accepts a parameter of type object or has an object field, then WebApi will deserialize them into JObject/JValue instead of their native .NET type, and it will have trouble making the journey from WebApi into Orleans.

@gabikliot
Copy link
Contributor

We already solved the compile time error that @nehmebilal reported above ('Newtonsoft.Json.Linq.JPropertyKeyedCollection' is inaccessible due to its protection level) in #305.
We will now not even attempt to automatically create a serializer for JObject.

We will provide a way to do (2) from above: "provide an extension point for application to define its own Serializer for JObject."

@gabikliot gabikliot added this to the Work in progress milestone Apr 7, 2015
@nehmebilal
Copy link
Author

We will provide a way to do (2) from above: "provide an extension point for application to define its own Serializer for JObject."

Great!

@gabikliot
Copy link
Contributor

Ok, good news: so apparently we supported (2) all way long!
#310 has a test example that shows how to write a custom serializer for JObject. The example code is now here: https://github.com/dotnet/orleans/blob/master/src/Tester/SerializationTests.cs
I have also updated the docs here: http://dotnet.github.io/orleans/Advanced-Concepts/Serialization to describe that option.

Kudos to @alan-geller for doing that work years ago and @sergeybykov for pointing me to this!

@gabikliot
Copy link
Contributor

I think the issue can be closed now?

@gabikliot gabikliot removed this from the Work in progress milestone Apr 11, 2015
@ghost ghost locked as resolved and limited conversation to collaborators Sep 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants