[EMAIL PROTECTED] writes: > aevers 2002/10/21 06:08:51 > > Modified: src/java/org/apache/xmlrpc DefaultHandlerMapping.java > SystemHandler.java XmlRpcRequest.java > XmlRpcRequestProcessor.java XmlRpcServer.java > XmlRpcWorker.java > src/test/org/apache/xmlrpc ClientServerRpcTest.java > xdocs changes.xml > Added: src/java/org/apache/xmlrpc ContextXmlRpcHandler.java > DefaultXmlRpcContext.java MultiCall.java > XmlRpcContext.java > Log: > Add context support for server side handlers, use > context support to re-implement system handlers. > Test case for new "system.multicall" handler. (Andrew Evers)
Andrew, thanks for checking in this code. Sorry it's taken me so long to get to it. Comments inline. > 1.5 +76 -35 xml-rpc/src/java/org/apache/xmlrpc/SystemHandler.java > > Index: SystemHandler.java > =================================================================== > RCS file: /home/cvs/xml-rpc/src/java/org/apache/xmlrpc/SystemHandler.java,v > retrieving revision 1.4 > retrieving revision 1.5 > diff -u -r1.4 -r1.5 > --- SystemHandler.java 26 Aug 2002 20:21:51 -0000 1.4 > +++ SystemHandler.java 21 Oct 2002 13:08:50 -0000 1.5 > @@ -55,11 +55,11 @@ > * <http://www.apache.org/>. > */ > > -import java.util.*; > +import java.util.Vector; > > /** > - * Implements the XML-RPC standard system.* methods (such as > - * <code>system.multicall</code>. > + * Wraps calls to the XML-RPC standard system.* methods (such as > + * <code>system.multicall</code>). > * > * @author <a href="mailto:adam@;megacz.com">Adam Megacz</a> > * @author <a href="mailto:andrew@;kungfoocoder.org">Andrew Evers</a> > @@ -67,23 +67,46 @@ > * @since 1.2 > */ > public class SystemHandler > +implements ContextXmlRpcHandler Some indentation to indicate the continuation of the declaration makes this easier to read (and is more consistent with the formatting in the rest of the code). > { > - private XmlRpcHandlerMapping handlerMapping = null; > + private DefaultHandlerMapping systemMapping = null; Can we avoid making the type of this reference a concretion instead of an interface? Perhaps the add/remove operations should be part of the XmlRpcHandlerMapping interface? > /** > + * Creates a new instance. This instance contains no system calls. Use the > + * addDefaultSystemHandlers() method to add the 'default' set of handlers, > + * or add handlers manually. > + */ > + public SystemHandler() > + { > + this.systemMapping = new DefaultHandlerMapping(); > + } > + > + /** > * Creates a new instance that delegates calls via the > - * specified {@link org.apache.xmlrpc.XmlRpcHandlerMapping} > + * specified {@link org.apache.xmlrpc.XmlRpcHandlerMapping}. This > + * method will add the system.multicall handler when a non-null > + * handlerMapping is specified. The value itself is ignored. > + * > + * @deprecated use new SystemHandler() and addDefaultSystemHandlers() instead. > */ > public SystemHandler(XmlRpcHandlerMapping handlerMapping) > { > - this.handlerMapping = handlerMapping; > + this(); > + if (handlerMapping != null) > + { > + addDefaultSystemHandlers(); > + } > } A four character indent is used throughout the source. The handlerMapping parameter to the constructor which takes a XmlRpcHandlerMapping is now never used, which is at odds with the documentation. > /** > * Creates a new instance that delegates its multicalls via > * the mapping used by the specified {@link org.apache.xmlrpc.XmlRpcServer}. > + * This method will add the default handlers when the specfied server's > + * getHandlerMapping() returns a non-null handler mapping. > * > * @param server The server to retrieve the XmlRpcHandlerMapping from. > + * > + * @deprecated use new SystemHandler() and addDefaultSystemHandlers() instead. > */ > protected SystemHandler(XmlRpcServer server) > { > @@ -91,40 +114,58 @@ > } > > /** > - * The <code>system.multicall</code> handler performs several RPC > - * calls at a time. > - * > - * @param requests The request containing multiple RPC calls. > - * @return The RPC response. > + * Add the default system handlers. The default system handlers are: > + * <dl> > + * <dt>system.multicall</dt> > + * <dd>Make multiple XML-RPC calls in one request and receive multiple > + * responses.</dd> > + * </dl> > + */ > + public void addDefaultSystemHandlers() > + { > + addSystemHandler("multicall", new MultiCall()); > + } Does the addDefaultSystemHandlers() method really need to be public? It's called by at least one of the constructors -- why not by both? > 1.3 +2 -16 xml-rpc/src/java/org/apache/xmlrpc/XmlRpcRequest.java > > Index: XmlRpcRequest.java > =================================================================== > RCS file: /home/cvs/xml-rpc/src/java/org/apache/xmlrpc/XmlRpcRequest.java,v > retrieving revision 1.2 > retrieving revision 1.3 > diff -u -r1.2 -r1.3 > --- XmlRpcRequest.java 26 Aug 2002 20:19:22 -0000 1.2 > +++ XmlRpcRequest.java 21 Oct 2002 13:08:50 -0000 1.3 > @@ -61,22 +61,18 @@ > * Encapsulates an XML-RPC request. > * > * @author <a href="mailto:andrew@;kungfoocoder.org">Andrew Evers</a> > + * @version $Id$ I really don't like CVS tokens. IMHO, they unnecessarilly complicate merging between branches and accepting patches (uh-humm ;). > 1.35 +23 -5 xml-rpc/src/java/org/apache/xmlrpc/XmlRpcServer.java > > Index: XmlRpcServer.java > =================================================================== > RCS file: /home/cvs/xml-rpc/src/java/org/apache/xmlrpc/XmlRpcServer.java,v > retrieving revision 1.34 > retrieving revision 1.35 > diff -u -r1.34 -r1.35 > --- XmlRpcServer.java 26 Aug 2002 17:56:21 -0000 1.34 > +++ XmlRpcServer.java 21 Oct 2002 13:08:50 -0000 1.35 > @@ -120,22 +120,35 @@ > * Parse the request and execute the handler method, if one is > * found. Returns the result as XML. The calling Java code > * doesn't need to know whether the call was successful or not > - * since this is all packed into the response. > + * since this is all packed into the response. No context information > + * is passed. > */ > public byte[] execute(InputStream is) > { > - return execute(is, null, null); > + return execute(is, new DefaultXmlRpcContext(null, null, >getHandlerMapping())); > } > > /** > * Parse the request and execute the handler method, if one is > * found. If the invoked handler is AuthenticatedXmlRpcHandler, > - * use the credentials to authenticate the user. > + * use the credentials to authenticate the user. No context information > + * is passed. > */ > public byte[] execute(InputStream is, String user, String password) > { > + return execute(is, new DefaultXmlRpcContext(user, password, >getHandlerMapping())); > + } Will you explain why the comments for the previous two methods say "No context information is passed" when I see a context created internally? > @@ -208,5 +211,18 @@ > + " millis in request/process/response"); > } > } > + } > + > + /** > + * Factory method to return a default context object for the execute() method. > + * This method can be overridden to return a custom sub-class of >XmlRpcContext. > + * > + * @param user the username of the user making the request. > + * @param password the password of the user making the request. > + * @return XmlRpcContext the context for the reqeust. > + */ > + protected XmlRpcContext defaultContext(String user, String password) > + { > + return new DefaultXmlRpcContext(user, password, handlerMapping); > } > } I like to use the verbNoun method naming scheme. I realize that "default" could be used as a verb, but it is a little ambiguous. Anyone got a better name for this one. > 1.1 xml-rpc/src/java/org/apache/xmlrpc/ContextXmlRpcHandler.java > > Index: ContextXmlRpcHandler.java ... > /** > * An XML-RPC handler that also handles user authentication. > * > * @author <a href="mailto:hannes@;apache.org">Hannes Wallnoefer</a> > * @see org.apache.xmlrpc.AuthenticationFailed > * @version $Id: ContextXmlRpcHandler.java,v 1.1 2002/10/21 13:08:50 aevers Exp $ > * @since 1.2 > */ > public interface ContextXmlRpcHandler > { > /** > * Return the result, or throw an Exception if something went wrong. > * > * @throws AuthenticationFailed If authentication fails, an > * exception of this type must be thrown. > * @see org.apache.xmlrpc.AuthenticationFailed > */ > public Object execute(String method, Vector params, XmlRpcContext context) > throws Exception; > } After reading the documentation for this class, I'm not exactly sure what this it does. The docs don't match up to the API. > 1.1 xml-rpc/src/java/org/apache/xmlrpc/DefaultXmlRpcContext.java > > Index: DefaultXmlRpcContext.java ... > public class DefaultXmlRpcContext > implements XmlRpcContext Formatting. > { > protected String userName, password; One decl per line definitely is preferred for instance fields. JavaDoc should be supplied for fields like this which have visibility external to the package. > 1.1 xml-rpc/src/java/org/apache/xmlrpc/MultiCall.java > > Index: MultiCall.java > public class MultiCall > implements ContextXmlRpcHandler Formatting. > 1.13 +49 -4 xml-rpc/src/test/org/apache/xmlrpc/ClientServerRpcTest.java > > Index: ClientServerRpcTest.java Yay unit tests!! - Dan
