[ovirt-devel] Abusing injection and DB access

[Roy Golan]

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

>
>    - 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()));

> 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.

_______________________________________________

> 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/20170405/18696f56/attachment-0001.html>