Re: svn commit: r1837697 - /ofbiz/ofbiz-framework/trunk/f
Locked
Re: svn commit: r1837697 - /ofbiz/ofbiz-framework/trunk/f
 
|
Hello Taher,
Just few nitpicks. [hidden email] 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); > + } > + }; > + } > + 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 |
|
Hi Mathieu,
To make it short, I did not look deeply into this code nor did I care to. This was essentially a copy-paste of the parent getRequest method which belongs to XML-RPC library. My opinion is that the whole thing is ugly but I'm not necessairly interested in refactoring. The reason why I copied it into the child class is to get my hands on the parser so I can enforce certain attributes to disable DTDs. I would rather not touch the code beyond that because it is not maintained and might cause other issues. However if you feel comfortable and have the time to refactor properly, then please issue a JIRA and I would gladly assist in pushing your code. On Fri, Aug 10, 2018, 11:54 AM Mathieu Lirzin <[hidden email]> wrote: > Hello Taher, > > Just few nitpicks. > > [hidden email] 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. > > > 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 > |
«
Return to OFBiz - Dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |