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#APPLICAT... 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