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

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. _______________________________________________ Pki-devel mailing list Pki-devel(a)redhat.com https://www.redhat.com/mailman/listinfo/pki-devel