[PATCH] 53 Check the actual result when revoking/unrevoking a certificate in CLI. Ticket 217
Please review the patch for trac ticket 217. Added an additional check over the actual result of a revoke/unrevoke operation. --Abhishek
On 4/19/2013 1:42 PM, Abhishek Koneru wrote:
Please review the patch for trac ticket 217.
Added an additional check over the actual result of a revoke/unrevoke operation.
Some comments: 1. In CertRequestInfo the synchronized keywords don't seem to be necessary. CertRequestInfo objects are created to pass REST operation results to the client. They don't seem to be used by multiple threads. 2. In CertRequestInfoFactory the operationResult uses 'pass/fail' values which are duplicated in several places. It might be better to use 'success/error' to match RES_SUCCESS and RES_ERROR constants, and these values can be put in constant variables in CertRequestInfo: public static final String REQ_SUCCESS = "success"; public static final String REQ_ERROR = "error"; This way we can be sure the operationResult will be used consistently. 3. In CertRequestInfoFactory the code for operationResult assignment seems to be incorrect because it will assign 'fail' to completed requests without error. Try cert-request-find, it will show all requests for system certs as failed. 4. In CertCLI if the request status is COMPLETE but the operation result is 'fail' the status will appear as REJECTED. This could be confusing because the request is not actually rejected. I think we should display the operation result separately from the status. -- Endi S. Dewata
Please review the patch with fixes for review comments on patch 53. On Mon, 2013-04-22 at 14:41 -0500, Endi Sukma Dewata wrote:
On 4/19/2013 1:42 PM, Abhishek Koneru wrote:
Please review the patch for trac ticket 217.
Added an additional check over the actual result of a revoke/unrevoke operation.
Some comments:
1. In CertRequestInfo the synchronized keywords don't seem to be necessary. CertRequestInfo objects are created to pass REST operation results to the client. They don't seem to be used by multiple threads.
--Removed the synchronized keywords
2. In CertRequestInfoFactory the operationResult uses 'pass/fail' values which are duplicated in several places. It might be better to use 'success/error' to match RES_SUCCESS and RES_ERROR constants, and these values can be put in constant variables in CertRequestInfo:
public static final String REQ_SUCCESS = "success"; public static final String REQ_ERROR = "error";
This way we can be sure the operationResult will be used consistently.
-- Added the constant variables in CertRequestInfo
3. In CertRequestInfoFactory the code for operationResult assignment seems to be incorrect because it will assign 'fail' to completed requests without error. Try cert-request-find, it will show all requests for system certs as failed.
-- Corrected. Null value for result is given as a SUCCESS
4. In CertCLI if the request status is COMPLETE but the operation result is 'fail' the status will appear as REJECTED. This could be confusing because the request is not actually rejected. I think we should display the operation result separately from the status.
-- Printing the operation result seperately. --Abhishek
Some additional comments below. On 4/23/2013 1:19 AM, Abhishek Koneru wrote:
2. In CertRequestInfoFactory the operationResult uses 'pass/fail' values which are duplicated in several places. It might be better to use 'success/error' to match RES_SUCCESS and RES_ERROR constants, and these values can be put in constant variables in CertRequestInfo:
public static final String REQ_SUCCESS = "success"; public static final String REQ_ERROR = "error";
This way we can be sure the operationResult will be used consistently.
-- Added the constant variables in CertRequestInfo
Actually they should use RES_ prefix instead of REQ_ to match IRequest.RES_SUCCESS/ERROR since it's for 'result', or don't use prefix at all. Also, the new constants use uppercase values "SUCCESS/ERROR", but the the request status is in lower case. It would be more consistent to use lowercase values too for the result. Request ID: 5 Type: enrollment Request Status: complete Operation Result: SUCCESS <-- use lower case Certificate ID: 0x5
3. In CertRequestInfoFactory the code for operationResult assignment seems to be incorrect because it will assign 'fail' to completed requests without error. Try cert-request-find, it will show all requests for system certs as failed.
-- Corrected. Null value for result is given as a SUCCESS
The code can be simplified as follows: if (result == null || result.equals(IRequest.RES_SUCCESS)) { info.setOperationResult(CertRequestInfo.RES_SUCCESS); } else { info.setOperationResult(CertRequestInfo.RES_ERROR); String error = request.getExtDataInString(IRequest.ERROR); info.setErrorMessage(error); <-- no need to check for null } Everything else is good. ACK. -- Endi S. Dewata
Added the latest suggestion. Pushed to master. --Abhishek On Tue, 2013-04-23 at 14:53 -0500, Endi Sukma Dewata wrote:
Some additional comments below.
On 4/23/2013 1:19 AM, Abhishek Koneru wrote:
2. In CertRequestInfoFactory the operationResult uses 'pass/fail' values which are duplicated in several places. It might be better to use 'success/error' to match RES_SUCCESS and RES_ERROR constants, and these values can be put in constant variables in CertRequestInfo:
public static final String REQ_SUCCESS = "success"; public static final String REQ_ERROR = "error";
This way we can be sure the operationResult will be used consistently.
-- Added the constant variables in CertRequestInfo
Actually they should use RES_ prefix instead of REQ_ to match IRequest.RES_SUCCESS/ERROR since it's for 'result', or don't use prefix at all.
Also, the new constants use uppercase values "SUCCESS/ERROR", but the the request status is in lower case. It would be more consistent to use lowercase values too for the result.
Request ID: 5 Type: enrollment Request Status: complete Operation Result: SUCCESS <-- use lower case Certificate ID: 0x5
-- Changed as mentioned.
3. In CertRequestInfoFactory the code for operationResult assignment seems to be incorrect because it will assign 'fail' to completed requests without error. Try cert-request-find, it will show all requests for system certs as failed.
-- Corrected. Null value for result is given as a SUCCESS
The code can be simplified as follows:
if (result == null || result.equals(IRequest.RES_SUCCESS)) { info.setOperationResult(CertRequestInfo.RES_SUCCESS);
} else { info.setOperationResult(CertRequestInfo.RES_ERROR); String error = request.getExtDataInString(IRequest.ERROR); info.setErrorMessage(error); <-- no need to check for null }
-- Changed as suggested.
Everything else is good. ACK.
participants (2)
-
Abhishek Koneru -
Endi Sukma Dewata