Jetty 12 - Re-enabled RequestLog tests. #8705
Conversation
Re-implemented features that were commented out. Signed-off-by: Simone Bordet <[email protected]>
jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/CustomRequestLogTest.java
Show resolved
Hide resolved
jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/CustomRequestLogTest.java
Show resolved
Hide resolved
jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java
Show resolved
Hide resolved
Signed-off-by: Simone Bordet <[email protected]>
|
|
||
| long idleTO = _configuration.getIdleTimeout(); | ||
| if (idleTO >= 0 && getIdleTimeout() != _oldIdleTimeout) | ||
| setIdleTimeout(_oldIdleTimeout); | ||
|
|
||
| if (getServer().getRequestLog() != null) |
There was a problem hiding this comment.
Should this be an instanceof for CustomRequestLog check?
|
|
||
| long idleTO = _configuration.getIdleTimeout(); | ||
| if (idleTO >= 0 && getIdleTimeout() != _oldIdleTimeout) | ||
| setIdleTimeout(_oldIdleTimeout); | ||
|
|
||
| if (getServer().getRequestLog() != null) | ||
| { | ||
| Authentication authentication = apiRequest.getAuthentication(); |
There was a problem hiding this comment.
Can we add a TODO here for only adding the attributes if the custom request logger has these things in the format string? Better yet, in this PR, implement that check in the doStart method and have an EnumSet for if user name; real path; handlerName are needed, then only set the attributes if those booleans are set.
|
|
||
| long idleTO = _configuration.getIdleTimeout(); | ||
| if (idleTO >= 0 && getIdleTimeout() != _oldIdleTimeout) | ||
| setIdleTimeout(_oldIdleTimeout); | ||
|
|
||
| if (getServer().getRequestLog() != null) |
There was a problem hiding this comment.
Ditto comments from ServletChannel
| public static final String HANDLER_NAME = CustomRequestLog.class.getName() + ".handlerName"; | ||
| public static final String REAL_PATH = CustomRequestLog.class.getName() + ".realPath"; | ||
| public static final String USER_NAME = CustomRequestLog.class.getName() + ".userPrincipal"; |
There was a problem hiding this comment.
Make these an enum and then an EnumSet can be used to record which of them are required.
| { | ||
| Authentication authentication = apiRequest.getAuthentication(); | ||
| if (authentication instanceof Authentication.User userAuthentication) | ||
| _request.setAttribute(CustomRequestLog.USER_NAME, userAuthentication.getUserIdentity().getUserPrincipal().getName()); |
There was a problem hiding this comment.
Ultimately, it might be best to have an EnumMap<CustomRequestLog.ATTR,String> as part of the request, as a more efficient way to pass these items for requests that don't use the attributes.
This probably needs to be tested... but if we do decide that just using attributes is best/simplest, then perhaps we give up on the whole lazy creation of the attributes map underneath as needless complexity.
i.e. either we continue to strive to avoid routine request attributes or we remove lazy request attribute map. It is silly to be lazy and routine.
Re-implemented features that were commented out.
Signed-off-by: Simone Bordet [email protected]