Skip to content

Conversation

tsliang
Copy link

@tsliang tsliang commented Aug 1, 2016

The current JsonNetSerializer implementation creates the default serializers within its constructor, which then calls ModifyJsonSerializerSettings. This means that a subclass of JsonNetSerializer cannot modify the
JsonSerializerSettings with fields set by the constructor, because the order of operations is:

  1. Call subclass constructor with parameter A, which will set subclass
    field A; field A will be used in ModifyJsonSerializerSettings.

  2. Subclass constructor calls JsonNetSerializer constructor with
    :base()

  3. JsonNetSerializer constructor calls ModifyJsonSerializerSettings
    as part of JsonSerializer.Create

  4. ModifyJsonSerializerSettings makes use of default value for
    field A, because it has not been set yet.

  5. JsonSerializer constructor exits

  6. Subclass constructor sets value of field A from parameter
    A.

    By using the Lazy class, we can ensure that ModifyJsonSerializerSettings is
    called after the subclass constructor has exited, which allows fields to
    be set and thereby used in ModifyJsonSerializerSettings. The unit test
    in Connection.doc.cs has been expanded to demonstrate this capability
    with a (fairly trivial) use case.

@karmi
Copy link

karmi commented Aug 1, 2016

Hi @tsliang, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

The current JsonNetSerializer implementation creates the default
serializers
within its constructor, which then calls ModifyJsonSerializerSettings.
This
means that a subclass of JsonNetSerializer cannot modify the
JsonSerializerSettings with fields set by the constructor, because the
order of
operations is:

  1. Call subclass constructor with parameter A, which will set subclass
  field A; field A will be used in ModifyJsonSerializerSettings.
  2. Subclass constructor calls JsonNetSerializer constructor with
     :base()
  3. JsonNetSerializer constructor calls ModifyJsonSerializerSettings
     as part of JsonSerializer.Create
  4. ModifyJsonSerializerSettings makes use of default value for
     field A, because it has not been set yet.
  5. JsonSerializer constructor exits
  6. Subclass constructor sets value of field A from parameter
     A.

  By using the Lazy<T> class, we can ensure that ModifyJsonSerializerSettings is
  called after the subclass constructor has exited, which allows fields to
  be set and thereby used in ModifyJsonSerializerSettings. The unit test
  in Connection.doc.cs has been expanded to demonstrate this capability
  with a (fairly trivial) use case.
@tsliang
Copy link
Author

tsliang commented Aug 1, 2016

@karmi fixed author and email

@gmarz
Copy link
Contributor

gmarz commented Aug 1, 2016

Thanks for this @tsliang 👍

For the most part, this LGTM. However, calling a virtual method in a base constructor as we're doing here when calling ModifyJsonSerializerSettings() is generally a bad idea, for reasons exactly like this. Lazy instantiating the serializers feels like we're working around the problem rather than addressing it...but it might be the only way to fix this though.

Would love to get @Mpdreamz and @russcam s opinion on this.

gmarz added a commit that referenced this pull request Aug 2, 2016
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
Copy link
Contributor

gmarz commented Aug 2, 2016

@tsliang I've proposed an alternate solution in the PR attached above which also ties into the issue raised in #2183.

@tsliang
Copy link
Author

tsliang commented Aug 2, 2016

@gmarz I usually try to avoid construct-with-init (hence the use of Lazy) - IMHO classes should be fully initialized upon construction and not require a separate init method to be called afterwards, lest someone forget to call the init method after construction. Still, this should get the job done. What is the general ES policy on construct-with-init? Are those used a lot in the project?

@SayliS
Copy link

SayliS commented Aug 2, 2016

What about custom resolvers? Right now I have problems setting my own :(

@tsliang
Copy link
Author

tsliang commented Aug 2, 2016

@SayliS what sort of problems?

@gmarz
Copy link
Contributor

gmarz commented Aug 3, 2016

IMHO classes should be fully initialized upon construction and not require a separate init method to be called afterwards

@tsliang I totally agree but I think this is a scenario where I can live with it, especially because it gives us a way to solve #2183 as well. I like the idea of using the lazy constructs, but still feels like we're working around the problem rather than fixing the poor design.

What is the general ES policy on construct-with-init? Are those used a lot in the project?

We have no policy on this and not something we do a lot at all. This may be one of the only instances.

@tsliang
Copy link
Author

tsliang commented Aug 3, 2016

@gmarz ok, that makes sense. I have proposed one other solution on #2189. Maybe you can take a look and see what you think? I think it'll solve the problem without requiring any significant changes in how the JsonNetSerializer is used. If it doesn't suit you, though, then let's just do #2189 and I'll close this PR.

@gmarz
Copy link
Contributor

gmarz commented Aug 3, 2016

@SayliS I'm assuming you mean adding your own custom converters? You can do this by overriding ContractConverters in your custom serializer. Is that not working for you?

@mynkow
Copy link

mynkow commented Aug 3, 2016

@SayliS is talking about Newtonsoft-Json Resolvers actually. The problem is that it is impossible to use dependency injection with virtual methods called in the base class.

Here is an example what is my problem and @SayliS

https://msdn.microsoft.com/en-us/library/ms182331.aspx?f=255&MSPPError=-2147217396

@gmarz
Copy link
Contributor

gmarz commented Aug 4, 2016

@mynkow gotcha. Yea, that's exactly what this PR and #2189 is aiming to solve. Let's close this issue and move the discussion over there to eliminate the redundancy.

@tsliang I'll pull these changes into #2189 if we choose to go this route.

@gmarz gmarz closed this Aug 4, 2016
Mpdreamz added a commit that referenced this pull request Aug 5, 2016
Taking another approach then #2189.

Here we yield responsibility of creating a new client to
ISerializerFactory.

The overloads taking a Func factory are now obsolete and others taking
ISerializerFactory are added
@Mpdreamz Mpdreamz mentioned this pull request Aug 5, 2016
Mpdreamz added a commit that referenced this pull request Aug 20, 2016
Taking another approach then #2189.

Here we yield responsibility of creating a new client to
ISerializerFactory.

The overloads taking a Func factory are now obsolete and others taking
ISerializerFactory are added
Mpdreamz added a commit that referenced this pull request Aug 20, 2016
Taking another approach then #2189.

Here we yield responsibility of creating a new client to
ISerializerFactory.

The overloads taking a Func factory are now obsolete and others taking
ISerializerFactory are added

applied feedback on #2197

fix null reference is test setup post switch to serializerfactory
Mpdreamz added a commit that referenced this pull request Aug 20, 2016
Taking another approach then #2189.

Here we yield responsibility of creating a new client to
ISerializerFactory.

The overloads taking a Func factory are now obsolete and others taking
ISerializerFactory are added

applied feedback on #2197

fix null reference is test setup post switch to serializerfactory
Mpdreamz added a commit that referenced this pull request Aug 20, 2016
Taking another approach then #2189.

Here we yield responsibility of creating a new client to
ISerializerFactory.

The overloads taking a Func factory are now obsolete and others taking
ISerializerFactory are added

applied feedback on #2197

fix null reference is test setup post switch to serializerfactory
Mpdreamz added a commit that referenced this pull request Dec 21, 2017
…) was being injected, because we use a custom ConnectionSettingsAwareSerializerBase implementation in our tests this was not caught. Also this PR renames our internal json converter from JsonNetSerializer to InternalSerializer, otherwise you would get a confusing error message about using an internal type before actually referencing Nest.JsonNetSerializer
Mpdreamz added a commit that referenced this pull request Dec 22, 2017
…) was being injected, because we use a custom ConnectionSettingsAwareSerializerBase implementation in our tests this was not caught. Also this PR renames our internal json converter from JsonNetSerializer to InternalSerializer, otherwise you would get a confusing error message about using an internal type before actually referencing Nest.JsonNetSerializer
Mpdreamz added a commit that referenced this pull request Dec 22, 2017
…) was being injected, because we use a custom ConnectionSettingsAwareSerializerBase implementation in our tests this was not caught. Also this PR renames our internal json converter from JsonNetSerializer to InternalSerializer, otherwise you would get a confusing error message about using an internal type before actually referencing Nest.JsonNetSerializer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants