[ovirt-devel] Abusing injection and DB access

[Roy Golan]

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


>
> _______________________________________________
>
> 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/20170406/8160c942/attachment-0001.html>