Projects tigase _server server-core Issues #1066
Setter for injected private member require public access modified? (#1066)
In QA
wojciech.kapcia@tigase.net opened 5 years ago
Due Date
2019-09-07

Not sure if this is a but per se, but I just stumbled against rather odd issue - it turned out that setters associated with private fields annoted wit @Inject require public access modifier. Is it intentional? Shouldn't we allow private setter methods in this case?

Andrzej Wójcik (Tigase) commented 5 years ago

No, we should not allow private setters in this case. Those methods are called from a different class and may be called on the unknown thread so their implementation should be prepared for that - the same as it was a public member of a class. By allowing private setters, coders implementing setter method could expect that it is called only from the class and would omit proper protections in the implementation.

wojciech.kapcia@tigase.net commented 5 years ago

I'm not sure I agree with that logic - following it we should mandate that all members annoted with @Inject should be public… In case of Kernel setters may be used to affect injection process (doing some additional work), and while they may be called "from the outside" (i.e. Kernel) for the sake of setting everything up, we don't necessarily may want to exposed setting some member directly.

This was an afterthought of fix for #helpdeskpr-174 (commit: c220607bfa5fb78bd2c25e12f1ac496673417f7e).

Andrzej Wójcik (Tigase) commented 5 years ago

To be honest, any extensive work in setters for @Inject fields should not be done. This is a possibility to be able to do some work like setting a value of other field or calling initializer after all fields were injected.

I do not think that exposing setters is bad in this case, as with use of kernel, configuration and injection the same thing could be done with private setters, so having them marked as public looks ok.

wojciech.kapcia@tigase.net commented 5 years ago

To be honest, any extensive work in setters for @Inject fields should not be done. This is a possibility to be able to do some work like setting a value of other field or calling initializer after all fields were injected.

I completely agree on both points, wasn't arguing against that.

I do not think that exposing setters is bad in this case, as with use of kernel, configuration and injection the same thing could be done with private setters, so having them marked as public looks ok.

I don't see how that can be achieved with only configuration and injection. Yes, you can configure different class for injected member but that would (roughly) stay the same during the whole lifecycle, and if you inject a class with private member/setter, without reflection I don't see changing that private member. If we force public setter (in case when we want/have to do some light work on private member) then we expose it to programmatic change.

It boils down to two things basically, in case we want to stick with public setter:

  • add it as a note to documentation;
  • (maybe) inform developer, that it has a setter for injected member, but it's private so it won't be called (warning? exception?)

Figuring it out with debugger was kinda PITA…

Andrzej Wójcik (Tigase) commented 5 years ago

We could add information to the documentation and possibly warn developer at runtime with a warning. @bmalkow What do you think about that?

Referenced from commit 10 months ago
issue 1 of 1
Type
Question
Priority
Normal
Assignee
Subsystem
Kernel
Issue Votes (0)
Watchers (0)
Reference
tigase/_server/server-core#1066
Please wait...
Page is in error, reload to recover