pzampino commented on code in PR #914:
URL: https://github.com/apache/knox/pull/914#discussion_r1639811375
##########
gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java:
##########
@@ -282,33 +281,15 @@ public Enumeration<String> getInitParameterNames() {
}
}
- private Exception sanitizeException(Exception e) {
+ private SanitizedException logAndSanitizeException(Exception e) {
+ LOG.failedToExecuteFilter(e);
if (e == null || e.getMessage() == null) {
- return e;
+ return new SanitizedException(e.getMessage());
}
if (!isErrorMessageSanitizationEnabled || e.getMessage() == null) {
- return e;
+ return new SanitizedException(e.getMessage());
}
String sanitizedMessage =
e.getMessage().replaceAll(errorMessageSanitizationPattern, "[hidden]");
-
- return createSanitizedException(e, sanitizedMessage);
- }
-
- private <T extends Exception> T createSanitizedException(T e, String
sanitizedMessage) {
- try {
- Constructor<? extends Exception> constructor =
e.getClass().getConstructor(String.class, Throwable.class);
- T sanitizedException = (T) constructor.newInstance(sanitizedMessage,
e.getCause());
- sanitizedException.setStackTrace(e.getStackTrace());
- return sanitizedException;
- } catch (Exception ex) {
- Exception genericException = new Exception(sanitizedMessage,
e.getCause());
- genericException.setStackTrace(e.getStackTrace());
- return (T) genericException;
- }
- }
-
- private <T extends Exception> T logAndSanitizeException(Exception e) throws
T {
- LOG.failedToExecuteFilter(e);
- throw (T) sanitizeException(e);
+ return new SanitizedException(sanitizedMessage);
Review Comment:
I'm not sure I agree about the Single Responsibility Principle. What is the
responsibility of the SanitizationException if not to provide a sanitized the
message? Otherwise, the burden of message sanitization is on all entities that
throw it, AND there is no guarantee that a thrower will have sanitized the
message.
If there is a need to customize the sanitization, IMO that is what
extensions are for. Nothing would prevent the subsequent creation of classes
that extends SanitizationException to override the sanitizing logic.
Personally, I don't think level of effort for refactoring counts for much in
these types of decisions, especially refactoring of test code.
--
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: dev-unsubscr...@knox.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
