[ovirt-devel] Abusing injection and DB access

[Tal Nisan]

Took the storage ones, expect them early last week or right after the
holiday.

On Wed, Apr 5, 2017 at 11:34 PM, Moti Asayag <masayag at redhat.com> wrote:

>
>
> On Wed, Apr 5, 2017 at 11:17 PM, Roy Golan <rgolan at redhat.com> wrote:
>
>>
>>
>> On Wed, Apr 5, 2017 at 9:06 PM Moti Asayag <masayag at redhat.com> wrote:
>>
>>> Hi All,
>>>
>>> ATM, there are 78 occurrences of "Injector.injectMembers(new
>>> AuditLogableBase())" in ovirt-engine project, which their main purpose is
>>> to ease the resolve of the placeholders of the audit log message while
>>> logging an event.
>>>
>>> For instance AuditLogType.MAC_ADDRESS_IS_EXTERNAL is being used from
>>> ImportVmCommandBase.java in the following way:
>>>
>>> private AuditLogableBase createExternalMacsAuditLog(VM vm, Set<String>
>>> externalMacs) {
>>>         AuditLogableBase logable = *Injector.injectMembers*(new
>>> AuditLogableBase());
>>>         logable.setVmId(vm.getId());
>>>         logable.setCustomCommaSeparatedValues("MACAddr", externalMacs);
>>>         return logable;
>>>     }
>>>
>>> The entry in the properties file is:
>>> MAC_ADDRESS_IS_EXTERNAL=VM ${*VmName*} has MAC address(es) ${MACAddr},
>>> which is/are out of its MAC pool definitions.
>>>
>>> Therefore the only purpose of the injection is to allow the
>>> AuditLogDirector to resolve the ${*VmName*} which is already known at
>>> the time of creating the AuditLogableBase entry.
>>>
>>> The result is injecting the DAOs for the AuditLogableBase instance and
>>> using the VM dao to retrieve the VM entry from the DB.
>>> This is just a wastef of injection and DB access while both can be
>>> spared.
>>>
>>> This could have been easily replaced by one of the following:
>>>
>>>    - auditLogableBase.setVmName(vm.getName());
>>>
>>> - setVmName is protected so not usable as is
>>
>
> It will become public if we agree on
>
> https://gerrit.ovirt.org/#/c/75244/2/backend/manager/
> modules/dal/src/main/java/org/ovirt/engine/core/dal/
> dbbroker/auditloghandling/AuditLogableBase.java
>
>
>>
>>>    - auditLogableBase.addCustomValue("VmName", vm.getName());
>>>
>>> I prefer this, it is readable. and BTW it is fluent, it returns 'this'
>> so use
>>
>>   AuditLogDirector(new AuditLogableBase(type)
>>       .addCustomValue("VmName", vm.getName()));
>>
>
> I'm okay with this as well.
>
>
>> Please pick up any occurrence from your domain and send a patch to
>>> replace it where possible.
>>> Thanks in advance,
>>> Moti
>>>
>>
>> +1
>>
>> Frankly the fact that all the DAOs sets protected access in
>> AuditLogableBase is a total abuse. Every command should declare its own
>> deps.
>>
>>
> That will require a huge effort.
>
>
>> _______________________________________________
>>
>>> Devel mailing list
>>> Devel at ovirt.org
>>> http://lists.ovirt.org/mailman/listinfo/devel
>>
>>
>
>
> --
> Regards,
> Moti
>
> _______________________________________________
> Devel mailing list
> Devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ovirt.org/pipermail/devel/attachments/20170406/4f09d13b/attachment-0001.html>