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

[Ade Lee]

New patch attached.  This patch replaces the previous one.
See comments below.

Ade
On Fri, 2012-06-29 at 10:29 -0500, Endi Sukma Dewata wrote:
> 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.
> 
Done.  See patch.

> 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.
> 
Agreed.

> 8. The ProfileProcessor can use the Auditor service to simplify the 
> auditing code. Is this going to be done separately?
> 
Yes - there is already a task to handle this.

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

> 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>
> 
Separate task.  We need to choose a standard and use it consistently
across the interface.

> 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.
> 
Good idea.  Done.
> 12. Formatting issue in CARestClient.java:125, PolicyConstraint.java:41.
> 
Fixed.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pki-vakwetu-0034-3-Adding-restful-interface-to-create-certificate-reque.patch
Type: text/x-patch
Size: 349646 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20120702/78668471/attachment.bin>