[Pki-devel] [PATCH] 862 TPS rewrite: provide connector service for JAVA-based TPS subsystem

[Endi Sukma Dewata]

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#APPLICATION_FORM_URLENCODED

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