Re: [ovirt-devel] Abusing injection and DB access

On Wed, Apr 5, 2017 at 11:34 PM Moti Asayag <masayag(a)redhat.com&gt; wrote: > On Wed, Apr 5, 2017 at 11:17 PM, Roy Golan <rgolan(a)redhat.com&gt; wrote: > > > > On Wed, Apr 5, 2017 at 9:06 PM Moti Asayag <masayag(a)redhat.com&gt; 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... > > > > - 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.

> > _______________________________________________ > > Devel mailing list > Devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/devel > > > > > -- > Regards, > Moti >