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

Akka.NET compatibility fixes #204

Merged
merged 12 commits into from
Jan 27, 2020

Conversation

Aaronontheweb
Copy link
Member

moved DebuggingExtensions into default Hocon namespace to make it easier to use in Visual Studio immediate Window

…ier to use in Visual Studio immediate Window
@Aaronontheweb Aaronontheweb added the akkadotnet-compat Legacy compatibility with Akka.Configuration label Jan 24, 2020
@Aaronontheweb
Copy link
Member Author

So as it turns out - #199 broke JSON serialization. We now get the following error:

Newtonsoft.Json.JsonSerializationException: 'Self referencing loop detected with type 'Hocon.HoconObject'. Path 'Root'.'

The problem is this call here:

for (var config = this; config != null; config = config.Fallback?.Copy())
{
elements.AddRange(config.Value);
}

Because we can't call this.Clone() to start, the Root.Value for the Config object becomes self-referencing, thus it can't be serialized.

cc @IgorFedchenko how can we simplify the object graph here so we don't have these kinds of issues? You're more familiar with the architecture here than I am.

@IgorFedchenko
Copy link
Contributor

IgorFedchenko commented Jan 25, 2020

@Aaronontheweb So while Root.Value is another object then Root, Json.NET serializer checks objects by Equals then by comparing references, and in HoconValue class we have defined equality as having same list of children.
And when Config does not have fallback values, Root's hash code equals to one of it's items (Value) hash code, and we get an exception when serializer get's to Value serialization.
So looks like this.Clone would not help even if we could use it, since Root is basically the same as Value here... I need to think about it.

So the issue is that config.Root.Equals(config.Root[0]), that's why Json.NET is blowing up...

@IgorFedchenko
Copy link
Contributor

Also, there is another issue that we will get with serialization: since Config object is immutable and does not have setters for public properties, it is unlikely that Json.NET will be able to deserialize Config instance to something other then Config.Empty, at least without special serializer settings tuning.

I think we could do something with it - but do we need config to be Json.NET serializable? Because config has ToString method, and ConfigurationFactory.Parse method also exists. Need to maintain backward compatibility here?

@IgorFedchenko
Copy link
Contributor

Actually, even without Root property serialization (for example with [JsonIgnore]) we get the same issue with Value property, because config.Value.Equals(config.Value[0])) == true.

So I really suspect that current implementation was not serializable with Json.NET from the beginning. Considering the fact that each IHoconElement has Parent property, that references it's parent object - most likely we would get self referencing loop exception there as well.

I can create an issue and work on making objects serializable in this library. Maybe we will also need to add Newtonsoft.Json package reference to core Hocon.csproj to get [JsonIgnore] attribute available... Will see if this will be required. Maybe converting some properties to be internal will be fine (but it may be useful for developers to keep them public). Or use GetParent()/SetParent() instead of Parent for example... Will discuss if we will need to work on it.

@Aaronontheweb
Copy link
Member Author

@IgorFedchenko

I can create an issue and work on making objects serializable in this library. Maybe we will also need to add Newtonsoft.Json package reference to core Hocon.csproj to get [JsonIgnore] attribute available... Will see if this will be required. Maybe converting some properties to be internal will be fine (but it may be useful for developers to keep them public). Or use GetParent()/SetParent() instead of Parent for example... Will discuss if we will need to work on it.

Let's do that in the short run as a work-around, but open a bigger issue for rearchitecting / simplifying the HOCON data structures internally.

@IgorFedchenko
Copy link
Contributor

IgorFedchenko commented Jan 27, 2020

@Aaronontheweb
Ok, created major issue #205
And local issue to help this PR: #206

Will work on the last one today

@Aaronontheweb
Copy link
Member Author

@IgorFedchenko thanks Igor - I'll get this test to pass so we can move forward with the stand-alone integration in Akka.NET.

@Aaronontheweb Aaronontheweb marked this pull request as ready for review January 27, 2020 19:42
@Aaronontheweb Aaronontheweb changed the title [WIP] Akka.NET compatibility fixes Akka.NET compatibility fixes Jan 27, 2020
@Aaronontheweb Aaronontheweb merged commit ad7ff94 into akkadotnet:dev Jan 27, 2020
@Aaronontheweb Aaronontheweb deleted the fix-akka-compat branch January 27, 2020 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
akkadotnet-compat Legacy compatibility with Akka.Configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants