----- Original Message -----
> Hi Martin,
>
> I'd like to address this with the same approach as suggested by Idan for
> Commands [1] .
>
I may not understand your comment right, so I do apologize if i'm saying
something nonsensical.
Having parameterized constructors is OK, and their benefits are well known.
However having CDI and parameterized constructors is somewhat contradictory —
if it does something, then it's problems.
Another side effect of this removal is also removal of some of reflection code,
which is also good.
> 1. It preserves the support for postConstruct() which is implemented
> explicitly in your patch by setup() method.
postConstruct generally is preserved even without parameterized
constructors. It's not too common in our code (if I'm not mistaken) to touch
parameters/context in postconstruct, which is only true reason, why we *need*
parameterized constructors. Even then, it's only wrong — these manipulations
should not be done in postconstruct methods, but we can still workaround it,
since, as we're calling injection manually, we can create bean, setup it, and
then call injection. It's not good, sure, but it's better to have few bad
places, that all places bad.
> 2. The suggested patch allows to define the classes as mutable - where
> the affect of it is unknown and probably not welcome.
I did simplest implementation for code review. It's not problematic to
implement setup method so it can be called only once, thus modification of
parameters / context are not possible just like with constructors.
> 3. It blocks queries from any future treatment of the parameters are
> creation time.
I don't understand this sentence.
If you're talking about final fields immutability, then it's valid only
for queryType. Other 3 fields (parameters, returnValue, engineContext) are
mutable, so even if pointer remains unchanged, queries may fiddle with its
content as they wish. If that was your point, there's actually no difference (I
believe) in these 2 approaches.
>
> Having a single c'tor which expects both parameters will solve that issue,
> same as done for the commands.
yes, it can be solved by requiring *one* full constructor with all required
parameters through whole hierarchy tree. That way reflection cannot fail. It'd
be smaller change, but as I said, getting rid of those constructors, we can
probably get rid of all reflection (not in my patch) and probably also of
special CDI treatment.
>
> [1] https://gerrit.ovirt.org/#/c/52657/
>
> On Tue, Jan 26, 2016 at 11:01 AM, Martin Mucha <[email protected]> wrote:
>
> > Hi,
> >
> > I got another bug about missing constructor:
> >
> > public …(P parameters, EngineContext engineContext) {
> > super(parameters, engineContext);
> > }
> >
> > so I looked into:
> > org.ovirt.engine.core.bll.QueriesCommandBase
> >
> > and it seems, that sole 'problem' is with initialization of user in
> > @PostConstruct method. It seems, that we can easily set all those fields
> > via setters, and user initialization can be done later, for example before
> > calling executeQueryCommand in:
> > org.ovirt.engine.core.bll.QueriesCommandBase#executeCommand
> >
> > doing this will move us closer to CDI and avoid errors because of
> > forgotten constructor. Is there a obstacle blocking us from doing this?
> >
> > thanks,
> > Mar.
> >
> > Edit: I had some time left so I tried to implement it, and it seems to be
> > working. It's big patch, but mostly there's just constructors removal.
> > Actual changes are in:
> >
> > QueriesCommandBase
> > CommandsFactory
> > RegisterVdsQuery
> > GetAllTemplateBasedEntityQuery
> > GetAllInstanceTypesQuery
> > GetAllVmTemplatesQuery
> > GetAllImageTypesQuery
> >
> > and test files.
> > _______________________________________________
> > Devel mailing list
> > [email protected]
> > http://lists.ovirt.org/mailman/listinfo/devel
>
>
>
>
> --
> Regards,
> Moti
>
_______________________________________________
Devel mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/devel
