[ovirt-devel] removing parameterized constructors from queries.

[Martin Mucha]

----- 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 <mmucha at redhat.com> 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
> > Devel at ovirt.org
> > http://lists.ovirt.org/mailman/listinfo/devel
> 
> 
> 
> 
> --
> Regards,
> Moti
>