Serializee is a managed class and we should use always an injected instance #839
Conversation
…stance Signed-off-by: Otávio Garcia <otavio@otavio.com.br>
|
+1 for this change. Did you test in a real app? |
| @@ -36,11 +36,12 @@ | |||
| public class VRaptorClassMapper extends MapperWrapper { | |||
|
|
|||
| private final Supplier<TypeNameExtractor> extractor; | |||
| private final Serializee serializee = new Serializee(); | |||
| private final Supplier<Serializee> serializee; | |||
There was a problem hiding this comment.
Not sure about the benefit of using Supplier here. Can you explain?
There was a problem hiding this comment.
Because the VRaptorXStream.wrapMapper is called by super constructor when XStream is instantiated. So we get a null instance because the injection is only did after instantiation. So we need to use lazy instance.
There was a problem hiding this comment.
and Instance won't solve this problem?
There was a problem hiding this comment.
Ohhhh my dear lord, good point. Where is my head that I don't think in this? I'll try some tests with Instance. Thank you.
There was a problem hiding this comment.
@Turini I think that is better to leave as is, because this class is vetoed, and receives instances from VRaptorXStream that is vetoed too, that receives instances from XStreamBuilderImpl. I tried to take this change, and a lot of classes will be changed its contructors to receive Instance<> instead of the object.
And more: there are some classes used in tests like MockSerializationResult, forcing users to rewrite their classes when migrate.
What you think to leave as is, with Supplier? Do you want I send to you a patch with changes?
There was a problem hiding this comment.
No worries, we can use Supplier :) so 🚢
|
Yes, I did this test in my personal app. I've tested Json and XML serialization, and works fine. |
Serializee is a managed class and we should use always an injected instance
This change breaks compatibility because change constructor signature. Of course only for users who extends serialization internal classes. No public API was changed.
But this change is really necessary because we are not using managed
Serializeefor all classes. There are some places that uses a non managed instance that isn't the same instance in the places that uses managed instance.