Re: [Pki-devel] [PATCH] 862 TPS rewrite: provide connector service for JAVA-based TPS subsystem
On 3/5/2014 6:12 PM, Christina Fu wrote:
Hi,
First of all, thank you Endi for your through review.
Per irc discussions, I have filed the following tickets:
* https://fedorahosted.org/pki/ticket/892 Need better error handling for
HttpConnection send()
- this ticket will address the comments irt pre-existing code
* https://fedorahosted.org/pki/ticket/893 subsystems (ca, kra, tks, tps)
mixed-up their subsystem certs when sharing same Tomcat instance
- this ticket will provide investigation for an issue I found irt
shared tomcat instance causing mixed-up subsystem certs among subsystems
Attached please find the patch that addressed the remaining comments.
thanks,
Christina
Some additional comments:
12. As discussed over IRC, the empty string checking in
HttpConnection.java:126 is unnecessary:
if ((contentType != null) && (!contentType.equals(""))) {
If we don't want an empty string to be a possible value, we probably
should not accept it in the first place by adding this code in
RemoteAuthority.java:59:
if (contentType.equals(""))
throw new IllegalArgumentException(
"Content type cannot be empty");
13. The code in HttpConnection.java:125-129 that sets the content type
currently will only be executed if HttpConnection if op is not null. Is
having a content type dependent on op being not null? The code probably
can be changed like this:
if (op == null)
mHttpreq.setURI(mDest.getURI());
else
mHttpreq.setURI(mDest.getURI(op));
// set content type regardless of op value
String contentType = dest.getContentType();
if (contentType != null) {
CMS.debug("HttpConnection: setting Content-Type");
mHttpreq.setHeader("Content-Type", contentType );
}
14. The above code that sets the content type currently only exists in
the HttpConnection constructor that takes a timeout parameter. The other
constructor that doesn't have a timeout parameter doesn't have this code
(see line 81). This will lead to further divergence of the constructors.
See https://fedorahosted.org/pki/ticket/891. I think until #891 is
addressed we should make any changes consistent in all constructors.
15. In ConnectionManager.java:42, the HTTP_CONTENT_TYPE_APP_URLENCODED
probably should be final & static since it's a constant. Or you can use
this constant:
http://docs.oracle.com/javaee/7/api/javax/ws/rs/core/MediaType.html#APPLI...
16. Since ConnectionManager will work in TPS only, the following comment
probably can be removed.
/* start is for resender only. no Resender for TPS
conn.start();
*/
That's it. ACK after addressing these issues.
--
Endi S. Dewata