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()); - auditLogableBase.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
Here is a list of cases which misuse injection. Please assign yourself and update the status (assigned/published/merged) https://docs.google.com/spreadsheets/d/1leEk0EYZgQPsG_Xddlk7FctBYYm4WOU8BsoE... Thanks, Moti On Wed, Apr 5, 2017 at 9:05 PM, Moti Asayag <masayag@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()); - auditLogableBase.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
-- Regards, Moti
On Wed, Apr 5, 2017 at 9:06 PM Moti Asayag <masayag@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@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
On Wed, Apr 5, 2017 at 11:17 PM, Roy Golan <rgolan@redhat.com> wrote:
On Wed, Apr 5, 2017 at 9:06 PM Moti Asayag <masayag@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/ja...
- 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@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
-- Regards, Moti
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@redhat.com> wrote:
On Wed, Apr 5, 2017 at 11:17 PM, Roy Golan <rgolan@redhat.com> wrote:
On Wed, Apr 5, 2017 at 9:06 PM Moti Asayag <masayag@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@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
-- Regards, Moti
_______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
On Wed, Apr 5, 2017 at 11:17 PM, Roy Golan <rgolan@redhat.com> wrote:
On Wed, Apr 5, 2017 at 9:06 PM Moti Asayag <masayag@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
+1
Frankly the fact that all the DAOs sets protected access in AuditLogableBase is a total abuse. Every command should declare its own deps.
+100 Can't agree more with declaring fine grained dependencies on each bean/command.
_______________________________________________
Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
_______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
On Wed, Apr 5, 2017 at 11:34 PM Moti Asayag <masayag@redhat.com> wrote:
On Wed, Apr 5, 2017 at 11:17 PM, Roy Golan <rgolan@redhat.com> wrote:
On Wed, Apr 5, 2017 at 9:06 PM Moti Asayag <masayag@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/ja...
- 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@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
-- Regards, Moti
On Thu, Apr 6, 2017 at 11:26 AM Roy Golan <rgolan@redhat.com> wrote:
On Wed, Apr 5, 2017 at 11:34 PM Moti Asayag <masayag@redhat.com> wrote:
On Wed, Apr 5, 2017 at 11:17 PM, Roy Golan <rgolan@redhat.com> wrote:
On Wed, Apr 5, 2017 at 9:06 PM Moti Asayag <masayag@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/ja...
- 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@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
-- Regards, Moti
participants (4)
-
Moti Asayag -
Roy Golan -
Tal Nisan -
Yevgeny Zaspitsky