Hello Taher,
Just few nitpicks.
ta...@apache.org writes:
> + protected XmlRpcRequest getRequest(final XmlRpcStreamRequestConfig
> pConfig, InputStream pStream)
> + throws XmlRpcException {
> + final XmlRpcRequestParser parser = new XmlRpcRequestParser(pConfig,
> getTypeFactory());
> + final XMLReader xr = SAXParsers.newXMLReader();
> + xr.setContentHandler(parser);
> + try {
> +
> xr.setFeature("http://apache.org/xml/features/disallow-doctype-decl";, true);
> +
> xr.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd";,
> false);
> +
> xr.setFeature("http://xml.org/sax/features/external-general-entities";, false);
> +
> xr.setFeature("http://xml.org/sax/features/external-parameter-entities";,
> false);
> + xr.parse(new InputSource(pStream));
> + } catch (SAXException | IOException e) {
> + throw new XmlRpcException("Failed to parse / read XML-RPC
> request: " + e.getMessage(), e);
> + }
> + final List<?> params = parser.getParams();
> + return new XmlRpcRequest() {
> + public XmlRpcRequestConfig getConfig() {
> + return pConfig;
> + }
> + public String getMethodName() {
> + return parser.getMethodName();
> + }
> + public int getParameterCount() {
> + return params == null ? 0 : params.size();
> + }
> + public Object getParameter(int pIndex) {
> + return params.get(pIndex);
> + }
> + };
> + }
> +
I would recommend against using explicit ‘final’ outside of class
fields where it conveys the semantic of immutable objects which is
meaningful since object are inherently stateful. In the case of method
parameter and local variables I think being explicit about the
immutability of the reference is a bit noisy IMHO. I like the citation
of Remi Forax [1] where in one of his french version of “Design Pattern
Reloaded” presentation [2] said “I want to read code, not to read
‘final’”. :-)
besides that, I have found ‘XmlRpcClientRequestImpl’ which is a default
implementation of ‘XmlRpcRequest’ which can be used to avoid the
anonymous class.
Here is a cosmetic *untested* patch applying those two recommendations.
diff --git framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/XmlRpcEventHandler.java framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/XmlRpcEventHandler.java
index fa1ae4f9bc..bf08208881 100644
--- framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/XmlRpcEventHandler.java
+++ framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/XmlRpcEventHandler.java
@@ -29,7 +29,6 @@ import java.io.InputStreamReader;
import java.io.OutputStream;
import java.io.Writer;
import java.util.HashMap;
-import java.util.List;
import java.util.Locale;
import java.util.Map;
@@ -53,7 +52,7 @@ import org.apache.ofbiz.webapp.control.ConfigXMLReader.RequestMap;
import org.apache.xmlrpc.XmlRpcException;
import org.apache.xmlrpc.XmlRpcHandler;
import org.apache.xmlrpc.XmlRpcRequest;
-import org.apache.xmlrpc.XmlRpcRequestConfig;
+import org.apache.xmlrpc.client.XmlRpcClientRequestImpl;
import org.apache.xmlrpc.common.ServerStreamConnection;
import org.apache.xmlrpc.common.XmlRpcHttpRequestConfig;
import org.apache.xmlrpc.common.XmlRpcHttpRequestConfigImpl;
@@ -272,10 +271,10 @@ public class XmlRpcEventHandler extends XmlRpcHttpServer implements EventHandler
}
}
- protected XmlRpcRequest getRequest(final XmlRpcStreamRequestConfig pConfig, InputStream pStream)
+ protected XmlRpcRequest getRequest(XmlRpcStreamRequestConfig pConfig, InputStream pStream)
throws XmlRpcException {
- final XmlRpcRequestParser parser = new XmlRpcRequestParser(pConfig, getTypeFactory());
- final XMLReader xr = SAXParsers.newXMLReader();
+ XmlRpcRequestParser parser = new XmlRpcRequestParser(pConfig, getTypeFactory());
+ XMLReader xr = SAXParsers.newXMLReader();
xr.setContentHandler(parser);
try {
xr.setFeature("http://apache.org/xml/features/disallow-doctype-decl";, true);
@@ -286,21 +285,7 @@ public class XmlRpcEventHandler extends XmlRpcHttpServer implements EventHandler
} catch (SAXException | IOException e) {
throw new XmlRpcException("Failed to parse / read XML-RPC request: " + e.getMessage(), e);
}
- final List<?> params = parser.getParams();
- return new XmlRpcRequest() {
- public XmlRpcRequestConfig getConfig() {
- return pConfig;
- }
- public String getMethodName() {
- return parser.getMethodName();
- }
- public int getParameterCount() {
- return params == null ? 0 : params.size();
- }
- public Object getParameter(int pIndex) {
- return params.get(pIndex);
- }
- };
+ return new XmlRpcClientRequestImpl(pConfig, parser.getMethodName(), parser.getParams());
}
class ServiceRpcHandler extends AbstractReflectiveHandlerMapping implements XmlRpcHandler {
Feel free to do whatever you want with it. I am not sure to understand
what is the purpose of the ‘getRequest’ method so it would be nice if
you could provide a docstring for it.
Thanks.
[1] https://forax.github.io/
[2] https://www.youtube.com/watch?v=aC1wGHDOQic
--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761 070D 0ADE E100 9460 4D37
