4:37 p.m.
New patch attached.
On 5/24/2012 2:00 PM, Ade Lee wrote:
1. First of all, the location of the new classes. Earlier in the
project, we made the decision to integrate the new RESTful classes
within the existing servlet structure. This is why the Profile RESTful
classes show up under com.netscape.cms.servlet.profile and the requests
(key and cert) show up under com.netscape.cms.servlet.request.
The current packaging structure is no longer adequate for REST client
and CLI. The packages are supposed to be reorganized later in ticket
#114. As mentioned in the last meeting, I'm going to write a proposal
about this on a wiki page.
Here are the package names that I used in the original patch:
- com.netscape.cms.user (common classes: REST interface & data model)
- com.netscape.cms.user.client (client classes: REST client & CLI)
- com.netscape.cms.user.server (server classes: REST services)
After some more consideration, I tend to go with a slightly different
structure which will be simpler to build and package into jar files:
- com.netscape.cms.user
- com.netscape.cms.client.user
- com.netscape.cms.server.user
In this case, if we are following the existing structure and being
consistent with the other RESTful servlets, we expect the REST servlets
for users and groups to show up where the UsrGroupAdminServlet resides -
which is under - com.netscape.cms.servlet.admin (like the
SystemCertificateResource)
This is OK except that the REST services aren't servlets even though
they might be running inside the RESTEasy servlet. The REST services
don't implement the Servlet interface and they don't follow servlet's
life cycle. So having a "servlet" in the package name is inaccurate. I
would suggest that we rename it to "server" because these classes (both
REST services and servlets) are only used by the server. So the
com.netscape.cms.servlet is actually identical with
com.netscape.cms.server that I'm proposing.
Along the same lines, the JAXB and DAO classes should reside under
com.netscape.cms.servlet.admin.model.
This would not be correct because the current "cms/servlet" package is
intended for server only. The data model classes are used for data
transfer, so they should be shared by both client and server. Right now
we don't have a "common" package (the jar file, not the folder). The
closest thing is "certsrv", but this package is intended to store server
plugin interfaces which the client would never use. The "certsrv" might
also have some dependencies that should not be required on the client side.
After looking at the package layout doc, it appears that both your
structure and the structure of the other new servlets is wrong. Rather,
in this particular case, the interfaces (UserResource.java and
UserCertResource.java as well as the JAXB classes UserData.java et. al.
should reside under com.netscape.certsrv.usrgrp
Same as above, they should belong to the "common" package, but
"certsrv"
is the closest thing that's available now, so for now I'll put it there.
The UserCertResourceService and UserResourceService classes should
reside under com.netscape.cms.servlet.admin
This is not a big issue, but "admin" package contains a mix of servlets,
some of which are subsystem-specific. It would be better to put them in
separate Java packages (e.g. cms.server.user) to simplify packaging.
CLI should go under com.netscape.cmstools
This doesn't work because the REST interfaces and the data models are
stored in "certsrv" or "cms", and the "cmstools" is compiled
before
them. So if we put the CLI in "cmstools" there will be a dependency
issue. I couldn't put it in "certsrv" either because the CLI depends on
the CMSRestClient which is in "cms" and the "certsrv" isn't
suppose to
depend on "cms".
We could move CMSRestClient to "certsrv" too, but the CLI should really
belong to a new "client" package. For now I'm going to leave the CLI in
com.netscape.cms.client.user under the "cms" package. We can move this
again if necessary in ticket #114.
2. I like the use of the atom link. Please open a task to replace
the
Links in the other restful services.
The Atom links are now still created manually. We should utilize link
injection. Ticket #192.
3. The valueOf() function in UserData is incorrect. Should return
UserData.
Fixed.
4. The getUserMessage() functions are perhaps not needed. If
anything,
they should be written as a single vargs method.
Changed to use varargs. This method is used to simplify the code, compare:
- old: CMS.getUserMessage(getLocale(), "CMS_ADMIN_SRVLT_NULL_RS_ID")
- new: getUserMessage("CMS_ADMIN_SRVLT_NULL_RS_ID")
But a better way to do
this would be to specify locale the local as a protected variable in
CMSResourceService.java. Then getLocale() would return this variable if
it is non-null, or would find it from the headers.
You mean caching the value returned by getLocale()? That can be done,
but the "locale" variable will be null until we call getLocale() at
least once.
Then you can call the regular CMS.getUserMessage(locale, ....)
without
the getUserMessage() calls.
For this to work we'd have to set the "locale" first, so we need to call
getLocale() somewhere. I'm not sure if calling it in the
CMSResourceService constructor will work because the "headers" is
injected, which I suppose can only happen after the object is
constructed. We can call it in the beginning of each REST method, but
that's not pretty. So for now I'm keeping the getUserMessage().
Its important we do it this way because when we are writing RESTful
servlets for things like cert issuance and revocation, we want to try to
reduce code duplication as much as possible.
5. Similar considerations exist for the audit() functions. We want to
use existing functions so that code can be extracted and reused by both
restful and restful servlets.
It's been updated based on the new Auditor patch.
6. Right now, the Delete user operation fails if the user is part of
any
groups. As discussed on #irc, we should create a resource
at /users/{id}/groups for the client to check for group membership. And
then allow the DELETE to remove users from groups automatically.
Fixed. The user-group membership will be done in ticket #190.
7. The paging mechanism in findUser() is not really the best - in
that
you basically get all of the results back from ldap each time. We
really need paged searches instead. Its unlikely that this will be big
concern for users/groups - but we should add a task to do proper paging.
Right now it's using a very simple, maybe slower, but correct
implementation. To do paging properly we will need VLV or simple paged
results, which have their own limitations and possibly require sessions
(not RESTful). The UGSubsystem doesn't seem to support paging either.
How many users/groups do we need to be able to support? If its not large
probably the current solution is adequate.
8. The code in stripBrackets() and getEncodedCert() are used in many
places in the code. After your changes, do all these still work?
As discussed on IRC, the change in getEncodedCert() fixes the extra
blank line in certificates. The blank line got added when we change the
Base64 encoder/decoder sometime ago, so it's now back to the correct
behavior.
The change in stripBrackets() has been removed. The
UserCertResourceService now normalizes the cert first.
--
Endi S. Dewata