[ovirt-devel] Abusing injection and DB access

[Roy Golan]

On Thu, Apr 6, 2017 at 11:26 AM Roy Golan <rgolan at redhat.com> wrote:

> 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.
>>
>
> Removed them all, https://gerrit.ovirt.org/75262 compile +1
> Now need to fix the tests - I'd appreciate help here
>
> Verified +1 the removal of all protected DAO's in AuditLogableBase.
Instead of constant injection of *66* dao fields for *every* command inject
only *9* which are most atm, They will be removed as well in the future.

Please help review, its a long, but stupid patch which just adds injections.
https://gerrit.ovirt.org/75262




>
>>
>> _______________________________________________
>>
>> Devel mailing list
>> Devel at ovirt.org
>> http://lists.ovirt.org/mailman/listinfo/devel
>>
>>
>>
>>
>> --
>> Regards,
>> Moti
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ovirt.org/pipermail/devel/attachments/20170424/ea6aec58/attachment-0001.html>