[Pki-devel] PATCH 34-1 - Restful interface to create certificate requests

[Endi Sukma Dewata]

On 6/28/2012 11:47 PM, Ade Lee wrote:
> I have attached an updated patch to address the comments (see below).
> In addition, the patch:
> * modifies the restful interface for approve/reject etc.
> from /certrequest/approve to /certrequest/{id}/approve
> * removes an unneeded class and some unneeded annotations.
> * changes the name of one class (PolicyAttribute) to ProfileAttribute
>    and uses this class in new jaxb model classes for ProfileOutputs
> * adds ProfileOutputs to the AgentEnrollmentRequestData class.
> * Refactors the ProfileProcess servlet to use the ProfileProcessor.
>
> Please review.  The attached patch replaces the existing patch.

More comments:

6. The ProfileProcessor is very big creating maintenance issue. It 
probably can be split into smaller classes that handle specific actions, 
e.g. CertEnrollmentProcessor, CertRenewalProcessor, 
CertApprovalProcessor. Common fields & methods could be moved into a 
Processor base class.

7. The DAO layer doesn't seem to be necessary because it used 
exclusively by the REST service and the majority of work is done in 
Processor layer. I guess when we merge the plural and singular REST 
service later we can merge DAO too.

8. The ProfileProcessor can use the Auditor service to simplify the 
auditing code. Is this going to be done separately?

9. In ProfileServlet the statEvents is initialized in startTiming. It's 
safer to initialize it in the field declaration as in ProfileProcessor.

10. To be consistent XML element name should be capitalized and XML 
attribute names should be lower case, for example:

   <PolicyDefault id="...">
       <Description>...</Description>
   </PolicyDefault>

11. Since REST data model is going to be shared with the client, it 
should not depend on classes that are used for the server only. This is 
not a problem now because certserv package contains both shared and 
server-only classes, but they will be difficult to separate later if the 
shared classes depend on server-only classes. For example PolicyDefault 
is a REST data model, but it depends on IRequest which is most likely a 
server-only interface. It might be better to have a separate factory 
class (e.g. PolicyDefaultFactory) which can construct a PolicyDefault 
object and then set its attributes with values obtained from server-only 
objects.

12. Formatting issue in CARestClient.java:125, PolicyConstraint.java:41.

-- 
Endi S. Dewata