Re: [Pki-devel] [PATCH] 191 - Add strength and algorithm to KeyData and KeyInfo

On 2/5/2014 12:27 PM, Ade Lee wrote: ACK. Feel free to push after addressing these minor issues: 1. Exception messages are meant to be read by human, so it would be better to use a user-friendly name (e.g. client ID) instead of variable name (e.g. clientId). throw new BadRequestException( "Invalid key generation request. Missing clientId"); 2. The following alg/size validation should be done even for the default alg/size to make sure we're not hard-coding invalid values (or if it becomes invalid in the future). KeyGenAlgorithm alg = KeyRequestService.KEYGEN_ALGORITHMS.get(algName); if (alg == null) { throw new BadRequestException("Invalid Algorithm"); } if (!alg.isValidStrength(size)) { throw new BadRequestException( "Invalid key size for this algorithm"); } 3. If the keySize in SymKeyGenerationRequest is optional we should use Integer rather than int. This way we can detect and report missing keySize properly using null. Right now the above code seems to be generating "Invalid key size for this algorithm" on missing keySize since the value will be converted into 0. 4. It would be better to include the offending values in the error message: "Key size {keySize} is invalid for {algorithm} algorithm". 5. The terms "strength" and "size" aren't used consistently. Any preference? -- Endi S. Dewata