Add SecurityDomainService to Job v2 APIs #4805
Conversation
| public AuditServletBase(CMSEngine engine, String username) { | ||
| this.engine = engine; | ||
| this.userName = username; | ||
| } |
There was a problem hiding this comment.
It's not a concern now, but this means for each request we need to create a new AuditServletBase instance (which is what the current code does):
auditServlet = new AuditServletBase(getEngine(), request.getUserPrincipal().getName());
auditConfigNew = auditServlet.updateAuditConfig(auditConfig);
If there is a lot of requests it might be more efficient to reuse the existing instance:
init() {
auditServlet = new AuditServletBase(getEngine());
}
updateAuditConfig() {
auditConfigNew = auditServlet.updateAuditConfig(request.getUserPrincipal().getName(), auditConfig);
}
Just something to consider for future improvement.
There was a problem hiding this comment.
Yes, I have follow this approach in all the services because the initial design, without annotation, was differently organised. I think there is room for this optimisation in several services but not all since in same cases the initialisation requires information provided by the request (e.g. SecurityDomainServletBase require the locale) and need additional work.
I'll do this optimisation in separate PR.
| * @author Marco Fargetta {@literal <[email protected]>} | ||
| * @author alee | ||
| */ | ||
| public class SecurityDomainServletBase { |
There was a problem hiding this comment.
In the future we might be able to merge SecurityDomainServletBase and SecurityDomainProcessor.
| @@ -189,7 +158,7 @@ public Method getActionMethod(HttpMethod met, String path) { | |||
| } | |||
| String keyPath = webActions.keySet().stream(). | |||
| filter( key -> { | |||
| String keyRegex = key.replace("{}", "([A-Za-z0-9_\\-]+)"); | |||
| String keyRegex = key.replace("{}", "([A-Za-z0-9_\\-\\.\\\s]+)"); | |||
There was a problem hiding this comment.
Theoretically a URL path could also contain non-ASCII characters, so this might not be sufficient. Should we use something like ([^\/]+)?
There was a problem hiding this comment.
Yes, I was looking at old URL specification not considering non ASCII. I have updated to get everything until / following your comment since there is no need to verify which characters are in the path, the URL should already be validated by tomcat.
|
@edewata Thanks! |
|
No description provided.