Fixes for custom serialization settings

[gmarz]

Previously, ModifyJsonSerializationSettings() did not respect properties
initialized in a custom serializer's constructor, since it was being
called in the base class constructor. This is bad practice (hence the
ReSharper warning). This change moves all of the initialization code
that was in the ctor to a new method Initialize() which must be called
after the serializer is instantiated.

This change also fixes another issue where our stateful converters were
totally disregarding any registered custom serializer. For this reason,
Initialize() also accepts an optional JsonConverter which is used to
return a new serializer (custom or default) with the stateful converter
applied.

Relates #2183
Relates #2182

[gmarz]

The only obvious downside to this approach is the need to call Initialize() after the serializer is instantiated, but at least it's not something a user needs to concern themselves with as it's automatically called in ConnectionSettingsBase.

I still think it's a better trade-off rather than calling ModifyJsonSerializationSettings() in an invalid state.

[mitchknife]

Thanks @gmarz for looking into this so quickly!

I'm not familiar enough with NEST's threading model, but it seems calling JsonNetSerializer.Initialize from different threads could be problematic.

Perhaps a new overload to JsonNetSerializer.Deserialize that takes a JsonConverter which creates its own JsonSerializer using the settings from the member _defaultSerializer?

[tsliang]

Another possible approach: what if we extract the settings creation to a factory object, which gets passed into the JsonNetSerializer? This would change the constructor to something like this:

internal JsonNetSerializer(IConnectionSettingsValues settings, JsonConverter stateFullConverter, ISerializerSettingsFactory factory = null)
{
    factory = factory ?? new DefaultSerializerSettingsFactory();
    // ...
    this._defaultSerializer = JsonSerializer.Create(factory.CreateSettings(SerializationFormatting.None));
    var indentedSerializer = JsonSerializer.Create(factory.CreateSettings(SerializationFormatting.Indented));
    // ...
}

This eliminates the initialization order problem by moving the pertinent data outside of the JsonNetSerializer. It does, however, make things a bit strange, since we're dictating behavior via the passed-in Factory, yet also dictating behavior via overloaded methods (I assume there will be no changes to other methods). Thoughts @mitchknife @gmarz ?

[gmarz]

I'm not familiar enough with NEST's threading model, but it seems calling JsonNetSerializer.Initialize from different threads could be problematic.

You're absolutely right about that @mitchknife, Initialize() wasn't thead-safe. I added a lock with my last commit, unfortunately.

Perhaps a new overload to JsonNetSerializer.Deserialize that takes a JsonConverter which creates its own JsonSerializer using the settings from the member _defaultSerializer?

Deserialize() is part of the IElasticsearchSerializer that's part ofElasticsearch.Net which doesn't have a dependency on Json.Net, so we can't introduce an overload that takes a JsonConverter.

@tsliang the factory seems like a good approach, but I don't think there's a way to introduce it without breaking the API in 2.x?

[mitchknife]

I added a lock with my last commit

@gmarz Adding the lock within Initialize fixes multiple calls to that method. I was more concerned with the following scenario:

• Thread A calls serializer.Initialize(customJsonConverter)
• Thread B calls serializer.Initialize()
• Thread A calls serializer.Deserialize(stream) with Thread B's serialization settings.

The lock needs to live for the duration of the (de)serialize call.

Deserialize() is part of the IElasticsearchSerializer that's part ofElasticsearch.Net which doesn't have a dependency on Json.Net, so we can't introduce an overload that takes a JsonConverter.

My thought was that the Deserialize overload would on exist on JsonNetSerializer. The endpoints that currently do the statefull (de)serialization would use it (those methods are already casting the IElasticsearchSerializer to JsonNetSerializer anyway). If it doesn't cast than new up a JsonNetSerializer as it currently does.

Better yet, wrap the original IElasticsearchSerializer in a JsonNetSerializer from the beginning that does the correct thing if no serializerFactory is supplied to ConnectionSettings. That way a hard cast could be used instead of the nullable cast. Or perhaps even stick the JsonNetSerializer as a protected member on ElasticClient? That way no cast is necessary.

[gmarz]

The lock needs to live for the duration of the (de)serialize call.

True that. To be honest, I don't like the idea of using locks or this approach at all. I think it's back to the drawing board entirely with this change.

[gmarz]

Closing in favor of #2197