nevzheng commented on code in PR #13057:
URL: https://github.com/apache/gravitino/pull/13057#discussion_r3997869468
##########
server-common/src/main/java/org/apache/gravitino/server/authentication/AuthenticationFilter.java:
##########
@@ -183,7 +206,7 @@ protected void sendAuthErrorResponse(HttpServletResponse
response, Exception exc
response.setStatus(httpStatus);
response.setContentType("application/json");
response.setCharacterEncoding(StandardCharsets.UTF_8.name());
- ObjectMapperProvider.objectMapper().writeValue(response.getWriter(),
errorResponse);
+ objectMapper.writeValue(response.getWriter(), errorResponse);
Review Comment:
On the logging finding: fixed in
https://github.com/apache/gravitino/pull/13057/commits/c61ad0849410c63e4f692b610b743982e7552748
and
https://github.com/apache/gravitino/pull/13057/commits/bc31ef559c874e68ce18798fb336503c16cba8c6.
`doFilter` now runs the happy path (authenticate, then the downstream chain)
in one `try` and handles each failure type once. Unexpected exceptions are
logged at ERROR with the request URI, so the cause stays in the server log when
stack traces are omitted from responses. 401/403/400 client errors are not
logged, and checked downstream failures are not logged again because
`PrincipalUtils.doAs` already logs them. Covered by
`TestAuthenticationFilter#testUnexpectedAuthenticationErrorIsLogged` and
`#testDownstreamUncheckedFailureIsLoggedOnce`.
The `message` finding in this thread will get a separate reply.
--
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]
