----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2427/#review2803 -----------------------------------------------------------
The code needs documentation. There is a lot of code where the developer is expected to spend time figuring out the purpose of the methods and the logic inside the methods. /trunk/core/src/main/java/org/apache/oozie/DagELFunctions.java <https://reviews.apache.org/r/2427/#comment6267> Interesting. Your comment indicates that it should not happen. It could happen and the code will try to alleviate this problem by escaping special characters. The escaping may or may not solve the problem since the JDOMException could be for a range of issues. /trunk/core/src/test/java/org/apache/oozie/command/wf/TestActionStartXCommand.java <https://reviews.apache.org/r/2427/#comment6268> What is the intention of this test case? Could you please add a top level comment for the test case and comments in the test case to describe the logic and the criteria being evaluated? /trunk/core/src/test/java/org/apache/oozie/command/wf/TestActionStartXCommand.java <https://reviews.apache.org/r/2427/#comment6271> What is the purpose of this utility method? Is it something that can be used more generally, i.e., across test cases /trunk/core/src/test/java/org/apache/oozie/command/wf/TestActionStartXCommand.java <https://reviews.apache.org/r/2427/#comment6269> If this is a real host name, then it should be anonymized. /trunk/core/src/test/java/org/apache/oozie/command/wf/TestActionStartXCommand.java <https://reviews.apache.org/r/2427/#comment6270> The host name should be anonymized if its a real host name /trunk/core/src/test/java/org/apache/oozie/command/wf/TestActionStartXCommand.java <https://reviews.apache.org/r/2427/#comment6272> Will this line of code be executed? I was not sure about the semantics of fail. /trunk/core/src/test/java/org/apache/oozie/command/wf/TestActionStartXCommand.java <https://reviews.apache.org/r/2427/#comment6274> Can you please document the purpose of this method? /trunk/core/src/test/java/org/apache/oozie/command/wf/TestActionStartXCommand.java <https://reviews.apache.org/r/2427/#comment6273> Will this line of code be executed? I was not sure about the semantics of fail. /trunk/core/src/test/java/org/apache/oozie/command/wf/TestActionStartXCommand.java <https://reviews.apache.org/r/2427/#comment6275> Will this line of code be executed? I was not sure about the semantics of fail. - Santhosh On 2011-10-24 20:02:05, Angelo K. Huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/2427/ > ----------------------------------------------------------- > > (Updated 2011-10-24 20:02:05) > > > Review request for oozie. > > > Summary > ------- > > try { > String valueElem = "<value>"+value+"</value>"; > XmlUtils.parseXml(valueElem); > } > catch (JDOMException ex) { > // It should not happen, so escape the characters for xml > value = XmlUtils.escapeCharsForXML(value); > } > > With above check, for element CDATA can be avoided for escaping, EX: > > For these two elements, first one has to convert because of '&', however > second one can be avoided. > > <property> > <name>test.ampsign</name> > > <value>http://app1.soln-stage.nova.cp.vip.ne1.yahoo.com/nova-webservices?urlSigner=signUrl&namespace=nova.proxy</value> > </property> > <property> > <name>test.cdata</name> > > <value><![CDATA[?redirect=http%3A%2F%2Fapp1.soln-stage.nova.cp.vip.ne1.yahoo.com%3A4080%2Fnova-webservices%2Fv1%2FurlSigner%2FsignUrl&namespace=nova.proxy&keyDBHash=Vsy6n_C7K6NG0z4R2eBlKg--]]></value> > </property> > > > *** & has to convert to & *** > *** <![CDATA[]] does not need to convert. *** > > <property> > <name>test.ampsign</name> > > <value>http://app1.soln-stage.nova.cp.vip.ne1.yahoo.com/nova-webservices?urlSigner=signUrl&namespace=nova.proxy</value> > </property> > <property> > <name>test.cdata</name> > > <value><![CDATA[?redirect=http%3A%2F%2Fapp1.soln-stage.nova.cp.vip.ne1.yahoo.com%3A4080%2Fnova-webservices%2Fv1%2FurlSigner%2FsignUrl&namespace=nova.proxy&keyDBHash=Vsy6n_C7K6NG0z4R2eBlKg--]]></value> > </property> > > > This addresses bug OOZIE-580. > https://issues.apache.org/jira/browse/OOZIE-580 > > > Diffs > ----- > > /trunk/core/src/main/java/org/apache/oozie/DagELFunctions.java 1185461 > > /trunk/core/src/main/java/org/apache/oozie/command/wf/ActionStartXCommand.java > 1185461 > > /trunk/core/src/test/java/org/apache/oozie/command/wf/TestActionStartXCommand.java > 1185461 > /trunk/release-log.txt 1185461 > > Diff: https://reviews.apache.org/r/2427/diff > > > Testing > ------- > > > Thanks, > > Angelo K. > >
