-
Notifications
You must be signed in to change notification settings - Fork 133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement v2 TPS ActivityService without vlv #4770
Conversation
86fc945
to
841112b
Compare
Sorkey array control generation get error when it is null. ActivityServlet reports wrong message when resource is not found
841112b
to
aa6315a
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some minor comments but the code seems to be consistent with the original one. Feel free to update/merge.
@@ -60,29 +58,37 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) thro | |||
PrintWriter out = response.getWriter(); | |||
out.print(re.getData().toJSON()); | |||
} catch(Exception ex) { | |||
logger.error("CAServlet: error setting error response {}", ex.getMessage()); | |||
logger.error(ERROR_RESPONSE, ex.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest that we attach the exception object as well so we can see the stack trace to help troublehooting. Same thing for the other logging statements below.
String activityID = activityRecord.getId(); | ||
try { | ||
URLEncoder.encode(activityID, "UTF-8"); | ||
} catch (UnsupportedEncodingException e) { | ||
throw new PKIException(e.getMessage()); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove this. The activity ID had to be encoded to generate a valid URL in REST response but we don't have that anymore.
ServletContext servletContext = getServletContext(); | ||
TPSEngine engine = (TPSEngine) servletContext.getAttribute("engine"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also call getTPSEngine()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it was using getTPSEngine()
in the original service but in the CA it is retrieved from the context. I decide to have the same approach in all cases. If there is a reason we can switch to retrieve the engine using the static method. For the moment I am leaving as it is.
Additionally, remove the url encoder test for the TPS activity id
@edewata Thanks! |
To implement the service the base servlet already implemented for the CA has been modified together with other small changes in the LDAP classes.