[Pki-devel] PATCH 0029 - RESTful servlet to configure system in a single servlet

[Ade Lee]

On Mon, 2012-04-30 at 19:09 -0500, Endi Sukma Dewata wrote:
> On 4/26/2012 2:49 PM, Ade Lee wrote:
> > Please review:
> >
> >      New RESTful servlet that does system configuration in a single servlet.
> >
> >      Installation code common to the panels and the installation servlet are extracted to a
> >      ConfigurationUtils file.  The panel code will be cleaned up to use the code in this
> >      class in a later commit.
> >
> >      Contains restful client and test driver code.  The test driver code should be modified
> >      and placed in a junit/system test framework.  Installation has been tested to work with
> >      the following installations: master CA, clone CA, KRA, OCSP, TKS, subordinate CA, CA
> >      signed by external CA (parts 1 and 2).
> >
> >      Ticket #155
> >
> > Thanks,
> > Ade
> 
> Here's some initial feedback:
> 
> 1. The ConfigurationErrorInterceptor is basically identical to the 
> existing DRMErrorInterceptor. To avoid duplicating the code for each 
> test components we can move the DRMErrorInterceptor from the 
> base/kra/functional into the base/test and call it CMSErrorInterceptor 
> (or something like that) which then can be used by all test components. 
> Or we can put it into the common package so it can be used by any 
> clients including the test tools. Ideally it should be put into a 
> separate jar file that contains the client libraries only.
> 
I agree with this completely.  We will need to extract these interfaces
and model classes and put them in a common package for use by both
servers and clients.  This includes putting all the common code (the
interceptors and the common jss connection code).   I suggest we check
this code in - as well as Jack's code for the CA, and then create a task
to do this consolidation.  I'll create the task.

> 2. Classes such as ConfigurationTest and ConfigurationUtils are defined 
> as containers of static functions. It might be possible to convert these 
> static functions into object methods and the parameters can be passed as 
> attributes, for example:
> 
>      public class ConfigurationTest {
> 
>          String host;
> 
>          public void parse(...) {
>              host = cmd.getOptionValue("h");
>          }
> 
>          public static void main(String args[]) {
>              ConfigurationTest test = new ConfigurationTest();
>              test.parse(args);
>              ConfigurationData data = test.construct();
>              ...
>          }
>      }
> 
We could certainly do that for ConfigurationTest - although I think we
may want to consider changing ConfigurationTest to some sort of junit
test.  The purpose of ConfigurationTest was just to get something quick
and dirty up to test the new servlet and provide a model for the
pkicreate configure script.

ConfigurationUtils is supposed to be used as a common set of functions
for both the new servlet and the installation panels. In fact, basically
what this patch does is extract (and clean up a bit) all the old logic
in the installation panels and put them in a common place
(ConfigurationUtils).  My next set of patches will replace the logic in
the existing panels to calls in ConfigurationUtils.

You can't really keep objects around from panel to panel - so these
really are static functions.  And I'm not sure what that would buy
you .. 

> 3. The ConfigurationUtils.getDomainXML() returns an XML string, but 
> before it's used it's always parsed into DOM. It might be better to 
> modify this method to return DOM directly. Is the XML string equivalent 
> to DomainInfo class? If that's the case the method can also return a 
> DomainInfo directly.
> 
In the panels, we will want to return the string for storage in CS.cfg.
See next patch.

> 4. It might be possible to convert some fields in ConfigurationData such 
> as dsPort or isClone into integer or boolean.
> 
Yeah, I agree.  I figured you would pick up on that.  Let me open that
as a separate task.

> 5. In InstallTokenRequest constructors the super() invocations aren't 
> necessary.
> 
Will remove.

> 6. The SystemConfigurationResourceService contains a static variable 
> mRandom. The 'static' and the 'm' prefix don't seem to be necessary.
> 
agreed - will fix.

> 7. The auto-format doesn't seem to be working. There are some trailing 
> whitespaces too. Eclipse should have fixed it automatically.
> 
Weird .. maybe the trailing whitespaces are in web.xml and other config
files?  I'll check it out ..