Comments: 1. We should create indexes for the new attributes -- certainly for clientID 2. In IKeyRecord, you have an accessor for clientID. Should we add accessors for the other two attributes added? 3. In IEncryptionUnit.java, there is no javadoc comments for three of the functions added. 4. Documentation in ITransportKeyUnit.java? 5. KeyResourceService.java still has incorrect check for Request.Complete 6. KeysResourceService.java -- remove bad code (don't just comment out) 7. KeyDao -- rename params --> requestParams what happens in the case where no wrapping parameters are passed? we should check for that. do we need to check for new() failing for KeyData? 8. In KeyRequestResourceService -> you return NOT_FOUND - should be BAD_REQUEST instead? 9. Remove check for duplicate clientID in KeyRequestDAO in submitRequest(ArchivalRequesData ..) Instead it should be a check for active keys with the same client ID. Also remove the TODO in the same function. And add a trac class to add status changes to the interface/implemntation. 10. In KeyRequestDAO that you added queue.updateRequest(request); to approveRequest(), please also add to cancelRequest() and rejectRequest() 11. typo in KeyRepository.java 12. Need getters for status and dataType in KeyRecord.java More to come as I delve into the depths of KRA changes .. On Wed, 2012-02-08 at 14:36 -0500, John Magne wrote:

On 2/8/2012 1:36 PM, John Magne wrote: These are just some minor issues/nitpicking, but I hope it will lead to cleaner code in the long run. Some of these might have been discussed already. 1. The throw statement in KeyDAO.java:124 for null params can be moved right after the params assignment in line 111. In general I think we should check null right after assignment and before doing anything else. 2. The keyData null checking In KeyDAO.java:160,168 is unnecessary because in all possible code execution the keyData always gets a new instance (line 128 and 159). The 'new' operation never returns a null, if it fails for any reason it will throw an exception so the check will never be executed. 3. The null initialization for params in KeyRequestDAO.java:230 is unnecessary because the code guarantees that params will be initialized with a value (line 231) before it's read for the first time (line 233). In general if we forgot to initialize a variable the code will fail to build, so we don't need to worry about removing unnecessary null assignments. 4. Formatting issue, some expressions/params between parentheses have unbalanced spaces: if ( expression) { method( params); I'm not sure if we have a coding standard for this, but at least it should be consistent within a single statement, either use spaces in both positions or not at all: if ( expression ) { if (expression) { 5. Similar to #3, the null initialization for IV in DRMTest.java:133 is unnecessary because the following try-catch clause guarantees that it will be initialized regardless of exceptions. Similar reason for IV_server in line 134. 6. According to our coding standard the variable names should begin with lower case. So in #5 it should be iv and iv_server. 7. The toString() in DRMTest.java:139 (and some other places) is unnecessary. In general any string concatenation will automatically call the toString() of the object. 8. The extra parentheses in DRMTest.java:562 is unnecessary because the compiler can distinguish the constructor from the method. The following code will work just fine: new PKIArchiveOptions.Template().decode(inStream); 9. In ArchiveOptions class (and some other new classes) the member variables are prefixed with 'm': private String mSymmAlgOID = null; private byte mSymmAlgParams[] = null; private byte mEncSymmKey[] = null; private byte mEncValue[] = null; I think we discussed previously that the prefix is unnecessary, so no need to add it in new code. The null initialization is also unnecessary because all member variables are assigned to null by default during instantiation. 10. Array constants like in EncryptionUnit.java:652 can be defined like this for simplicity: SymmetricKey.Usage usages[] = { SymmetricKey.Usage.WRAP, SymmetricKey.Usage.UNWRAP }; 11. In SecurityDataRecoveryService.java the assignment in line 168 can be moved into the catch clause above it because iv will only be null if there's an exception. The nextBytes() will not make iv null because method parameters are passed by value. The clone() is not necessary either, iv can just point to the iv_default. The rnd can also be moved inside the try block to minimize variable scope. iv = new byte[8]; try { Random rnd = new Random(); rnd.nextBytes(iv); } catch (Exception e) { iv = iv_default; } -- Endi S. Dewata

On 2/9/2012 6:46 PM, Endi Sukma Dewata wrote: Two more files are missing from pki/.classpath: /usr/share/candlepin/lib/jaxb-impl-2.1.12.jar /usr/share/candlepin/lib/resteasy-jaxb-provider-2.2.1.GA.jar After fixing the .classpath the test works now. -- Endi S. Dewata