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

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 On 03/04/2014 04:29 PM, Endi Sukma Dewata wrote: > Some comments: > > 1. As discussed over IRC the common code HttpConnection shouldn't > contain a subsystem-specific code, which in this case is setting the > content type to application/x-www-form-urlencoded for TPS. > > 2. Also discussed over IRC, there's an existing issue with > HttpConnection constructors: > https://fedorahosted.org/pki/ticket/891 > > Suppose that issue is fixed later, it should be possible to merge the > constructors. Then instead of calling different constructors for > different timeout values like this: > > if (timeout == 0) { > connector = > new HttpConnector( > null, nickname, remauthority, > resendInterval, conf); > } else { > connector = > new HttpConnector( > null, nickname, remauthority, > resendInterval, conf, timeout); > } > > it can be simplified like this: > > connector = > new HttpConnector( > null, nickname, remauthority, > resendInterval, conf, timeout); > > There's also a similar code in HttpConnFactory. > > 4. In HttpConnection the existing constructors were changed to pass > "" as the default value of op. I think we should just pass a null > value. This way the code can simply check the op like this: > > this(dest, factory, null); > > ... > > if (op == null) > > If we really want to accept an empty string, then probably we should > normalize the string by trimming the white spaces before checking for > empty string. > > 5. The HttpConnection.send() checks if the parameter is null or empty > string then returns a null value with a comment "throw later". This > error will only be detected later if another code tries to use the > null return value, but at that point we're looking at a secondary > issue, which makes it harder to troubleshoot. I think in general it's > better to throw an exception as soon as the error is detected. > > 6. The HttpConnection.send() checks for untrusted certificate by > checking the exception message. This may work now but could become > inaccurate later due to translation, text changes, or typo. It would > be better if there's a specific exception type or error code that we > can check for this specific case. > > 7. Similar to #5, HttpConnection.send() swallows > NumberFormatException. I think we should not catch this to detect the > error as early as possible and also NumberFormatException is already > a RuntimeException so it doesn't need to be wrapped/declared. > > 8. Similar to #5, the HttpConnector.send() (not HttpConnection) would > swallow the exception so some errors wouldn't be detected > immediately. I think the code could be simplified as follows (the > send() already throws EBaseException): > > try { > curConn = mConnFactory.getConn(op); > return curConn.send(msg); > > } finally { > if (curConn != null) { > mConnFactory.returnConn(curConn); > } > } > > 9. Similar to #5, the ConnectionManager.getXMLparser() would swallow > the exceptions. I think it would be better to wrap the exception in a > RuntimeException and rethrow it. > > try { > ByteArrayInputStream bis = > new ByteArrayInputStream(text.getBytes()); > return new XMLObject(bis); > > } catch (Exception e) { > CMS.debug("TPSSubsystem: getXMLparser(): failed: "+ > e.toString()); > throw new RuntimeException(e); > } > > We can change this code again in the future if necessary to use a > more specific class or add the original exceptions into the throws list. > > 10. In ConnectionManager, the createConnector() will never return a > null, so it's not necessary to check its return value in > initConnectors(). It will simplify the code and some code analyzers > like Coverity probably will report the part that handles the null > case as dead code anyway. > > 11. Just some minor things: > - The debug log in getXMLparser() should say "ConnectionManager" > instead of "TPSSubsystem". > - If we use camel-case consistently, the name should be > getXMLParser() (with upper-case P). > - Some toString() invocations are redundant because string > concatenation will call it automatically. > > I'll leave it to others to review the functionality correctness. > _______________________________________________ Pki-devel mailing list Pki-devel(a)redhat.com https://www.redhat.com/mailman/listinfo/pki-devel