Copilot commented on code in PR #570:
URL: https://github.com/apache/ranger/pull/570#discussion_r2093781135
##########
agents-audit/core/src/main/java/org/apache/ranger/audit/provider/AuditProviderFactory.java:
##########
@@ -390,6 +382,20 @@ public synchronized void init(Properties props, String
appType) {
installJvmShutdownHook(props);
}
+ private AuditDestination createDestination(String destinationClass) {
+ AuditDestination ret = null;
+
+ if (StringUtils.isNotBlank(destinationClass)) {
+ try {
+ ret = (AuditDestination)
Class.forName(destinationClass).newInstance();
+ } catch (Exception e) {
+ LOG.error("Failed to instantiate audit destination {}",
destinationClass, e);
+ }
+ }
+
+ return ret;
Review Comment:
[nitpick] The `createDestination` method catches all exceptions and returns
`null` on failure, which can lead to NPEs when callers invoke methods on the
result. Consider throwing a descriptive exception or explicitly handling a
`null` return.
```suggestion
if (StringUtils.isBlank(destinationClass)) {
throw new IllegalArgumentException("Destination class name
cannot be null or empty");
}
try {
return (AuditDestination)
Class.forName(destinationClass).newInstance();
} catch (Exception e) {
LOG.error("Failed to instantiate audit destination {}",
destinationClass, e);
throw new RuntimeException("Failed to create AuditDestination
for class: " + destinationClass, e);
}
```
##########
agents-audit/core/src/main/java/org/apache/ranger/audit/provider/AuditProviderFactory.java:
##########
@@ -307,13 +307,13 @@ public synchronized void init(Properties props, String
appType) {
if (isAuditToHdfsEnabled) {
LOG.info("HdfsAuditProvider is enabled");
- HdfsAuditProvider hdfsProvider = new HdfsAuditProvider();
+ AuditDestination hdfsProvider =
createDestination("org.apache.ranger.audit.provider.hdfs.HdfsAuditProvider");
Review Comment:
The `createDestination` method returns an `AuditDestination`, but
`hdfsProvider` is later used as an `AuditHandler`. This will cause a compile
error or ClassCastException; consider changing `createDestination` to return
`AuditHandler` or casting its result to the correct interface.
```suggestion
AuditDestination destination =
createDestination("org.apache.ranger.audit.provider.hdfs.HdfsAuditProvider");
if (!(destination instanceof AuditHandler)) {
throw new IllegalStateException("The created destination
is not an instance of AuditHandler: " + destination.getClass().getName());
}
AuditHandler hdfsProvider = (AuditHandler) destination;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
