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 <mathieu.lir...@nereide.fr>
wrote:
> 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.
>
>
> 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
>
