Martin Mucha has uploaded a new change for review.
Change subject: core: added missing logging + refactoring
......................................................................
core: added missing logging + refactoring
org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector#saveToDb
was missing logging in situation, when 'bundles/AuditLogMessages'
resource bundle does not contain specific translation. In that case
AuditLog was silently not created and nothing was logged.
- added missing logging
- overgrown method saveToDb split into few smaller ones.
- removed coding smell when dealing with variables 'message' and
'resolvedMessage'
- simplified test for empty string.
Change-Id: Ic737ace1808e1f242d0eb08ee458869a89be500e
Signed-off-by: Martin Mucha <mmu...@redhat.com>
---
M
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/AuditLogDirector.java
1 file changed, 69 insertions(+), 47 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/44/29244/1
diff --git
a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/AuditLogDirector.java
b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/AuditLogDirector.java
index 0c78641..caeecee 100644
---
a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/AuditLogDirector.java
+++
b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/AuditLogDirector.java
@@ -109,40 +109,19 @@
}
private static void saveToDb(AuditLogableBase auditLogable, AuditLogType
logType, String loggerString) {
- String message = null;
- String resolvedMessage = null;
AuditLogSeverity severity = logType.getSeverity();
+
+ //TODO MM: shouldn't this be rather warning? This assumption is hardly
valid.
+ //TODO MM: there's NO such situation when this can happen; and since
we find this invalid anyway, it should be 'blocked' in AuditLogType constructor.
if (severity == null) {
severity = AuditLogSeverity.NORMAL;
log.infoFormat("No severity for {0} audit log type, assuming
Normal severity", logType);
}
- AuditLog auditLog = null;
- // handle external log messages invoked by plugins via the API
- if (auditLogable.isExternal()) {
- resolvedMessage = message = loggerString; // message is sent as an
argument, no need to resolve.
- auditLog = new AuditLog(logType,
- severity,
- resolvedMessage,
- auditLogable.getUserId(),
- auditLogable.getUserId() != null ?
getDbFacadeInstance().getDbUserDao().get(auditLogable.getUserId()).getLoginName()
: null,
- auditLogable.getVmIdRef(),
- auditLogable.getVmIdRef() != null ?
getDbFacadeInstance().getVmDao().get(auditLogable.getVmIdRef()).getName() :
null,
- auditLogable.getVdsIdRef(),
- auditLogable.getVdsIdRef() != null ?
getDbFacadeInstance().getVdsDao().get(auditLogable.getVdsIdRef()).getName() :
null,
- auditLogable.getVmTemplateIdRef(),
- auditLogable.getVmTemplateIdRef() != null ?
getDbFacadeInstance().getVmTemplateDao().get(auditLogable.getVmTemplateIdRef()).getName()
: null,
- auditLogable.getOrigin(),
- auditLogable.getCustomEventId(),
- auditLogable.getEventFloodInSec(),
- auditLogable.getCustomData());
- } else if ((message = messages.get(logType)) != null) { // Application
log message from AuditLogMessages
- resolvedMessage = resolveMessage(message, auditLogable);
- auditLog = new AuditLog(logType, severity, resolvedMessage,
auditLogable.getUserId(),
- auditLogable.getUserName(), auditLogable.getVmIdRef(),
auditLogable.getVmName(),
- auditLogable.getVdsIdRef(), auditLogable.getVdsName(),
auditLogable.getVmTemplateIdRef(),
- auditLogable.getVmTemplateName());
- }
- if (auditLog != null) {
+ AuditLog auditLog = createAuditLog(auditLogable, logType,
loggerString, severity);
+
+ if (auditLog == null) {
+ log.warn("Unable to create AuditLog");
+ } else {
auditLog.setstorage_domain_id(auditLogable.getStorageDomainId());
auditLog.setstorage_domain_name(auditLogable.getStorageDomainName());
auditLog.setstorage_pool_id(auditLogable.getStoragePoolId());
@@ -159,27 +138,70 @@
auditLog.setCallStack(auditLogable.getCallStack());
auditLog.setMacPoolId(auditLogable.getMacPoolIdForLog());
auditLog.setMacPoolName(auditLogable.getMacPoolNameForLog());
+
getDbFacadeInstance().getAuditLogDao().save(auditLog);
- String logMessage;
- if (!"".equals(loggerString)) {
- logMessage = log.transform(loggerString, resolvedMessage);
- } else {
- logMessage = auditLog.toStringForLogging();
- }
+ logMessage(severity, getMessageToLog(loggerString, auditLog));
+ }
+ }
- switch (severity) {
- case NORMAL:
- log.info(logMessage);
- break;
- case ERROR:
- log.error(logMessage);
- break;
- case ALERT:
- case WARNING:
- log.warn(logMessage);
- break;
- }
+ private static void logMessage(AuditLogSeverity severity, String
logMessage) {
+ //TODO MM: rather than this, there could(should?) be OO approach:
introducing 'log' method on AuditLogSeverity.
+ switch (severity) {
+ case NORMAL:
+ log.info(logMessage);
+ break;
+ case ERROR:
+ log.error(logMessage);
+ break;
+ case ALERT:
+ case WARNING:
+ default:
+ log.warn(logMessage);
+ break;
+ }
+ }
+ private static String getMessageToLog(String loggerString, AuditLog
auditLog) {
+ String logMessage;
+ if (loggerString.isEmpty()) {
+ logMessage = auditLog.toStringForLogging();
+ } else {
+ logMessage = log.transform(loggerString, auditLog.getmessage());
+ }
+ return logMessage;
+ }
+
+ private static AuditLog createAuditLog(AuditLogableBase auditLogable,
AuditLogType logType, String loggerString, AuditLogSeverity severity) {
+ // handle external log messages invoked by plugins via the API
+ if (auditLogable.isExternal()) {
+ String resolvedMessage = loggerString; // message is sent as an
argument, no need to resolve.
+ return new AuditLog(logType,
+ severity,
+ resolvedMessage,
+ auditLogable.getUserId(),
+ auditLogable.getUserId() != null ?
getDbFacadeInstance().getDbUserDao().get(auditLogable.getUserId()).getLoginName()
: null,
+ auditLogable.getVmIdRef(),
+ auditLogable.getVmIdRef() != null ?
getDbFacadeInstance().getVmDao().get(auditLogable.getVmIdRef()).getName() :
null,
+ auditLogable.getVdsIdRef(),
+ auditLogable.getVdsIdRef() != null ?
getDbFacadeInstance().getVdsDao().get(auditLogable.getVdsIdRef()).getName() :
null,
+ auditLogable.getVmTemplateIdRef(),
+ auditLogable.getVmTemplateIdRef() != null ?
getDbFacadeInstance().getVmTemplateDao().get(auditLogable.getVmTemplateIdRef()).getName()
: null,
+ auditLogable.getOrigin(),
+ auditLogable.getCustomEventId(),
+ auditLogable.getEventFloodInSec(),
+ auditLogable.getCustomData());
+ }
+
+ final String messageByType = messages.get(logType);
+ if (messageByType == null) {
+ return null;
+ } else {
+ // Application log message from AuditLogMessages
+ String resolvedMessage = resolveMessage(messageByType,
auditLogable);
+ return new AuditLog(logType, severity, resolvedMessage,
auditLogable.getUserId(),
+ auditLogable.getUserName(), auditLogable.getVmIdRef(),
auditLogable.getVmName(),
+ auditLogable.getVdsIdRef(), auditLogable.getVdsName(),
auditLogable.getVmTemplateIdRef(),
+ auditLogable.getVmTemplateName());
}
}
--
To view, visit http://gerrit.ovirt.org/29244
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic737ace1808e1f242d0eb08ee458869a89be500e
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Martin Mucha <mmu...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
