Amila,
The regression caused by your change is in the dhr == null case. In
this case, your code calls reader.getText() at most once and this
causes a problem if the base64 data is represented as multiple
CHARACTER events, i.e. if the reader is non coalescing. The code also
fails to enforce the postcondition (current event = END_ELEMENT) in
the dhr == null case. I've added the relevant unit tests to prove
this.
On the other hand, you correctly pointed out that there is an issue in
the dhr != null case if the element is empty.
I fixed this properly and the diff with respect to the original
version of the method now looks as follows:
if (dhr == null) {
// In this case the best way to get the content of the
element is using
// the getElementText method
base64 = reader.getElementText();
} else {
- if (reader.next() != XMLStreamConstants.CHARACTERS) {
+ int event = reader.next();
+ if (event == XMLStreamConstants.END_ELEMENT) {
+ // This means that the element is actaullay empty ->
return empty DataHandler
+ return new DataHandler(new ByteArrayDataSource(new byte[0]));
+ } else if (event != XMLStreamConstants.CHARACTERS) {
throw new XMLStreamException("Expected a CHARACTER event");
}
if (dhr.isBinary()) {
Thus, the change is only in the dhr != null code path. This is
expected since the dhr == null case already worked correctly in the
original version.
Thanks,
Andreas
On Thu, Oct 8, 2009 at 10:07, Amila Suriarachchi
<[email protected]> wrote:
> On Thu, Oct 8, 2009 at 2:15 AM, Andreas Veithen
> <[email protected]>wrote:
>
>> -1 for this change.
>>
>> Reasons:
>>
>> 1. It breaks the contract of the method in two ways:
>>
>> - It doesn't work if the XMLStreamReader is non coalescing.
>
> I am not clear how you going to explain this.
>
> In earlier code, when it comes to while loop it has executed
> 1. reader.next() which is at the if statement
> 2. reader.getText() which is just above
>
> after this change same thing applies. Now it has the reader.next() at the
> top.
>
>
>> In this
>> case it would only read part of the content. Please refer to the
>> second part of the code in that method, more precisely the part tagged
>> with the comment "Take into account that in non coalescing mode, there
>> may be additional CHARACTERS events".
>> - Even if the parser is coalescing, the change still violates the
>> contract for elements that don't have the expected content, i.e. which
>> have child nodes other than text nodes. In this case, the changed
>> method neither satisfies the postcondition, nor does it throw an
>> exception.
>>
>
> Can you please explain the lines of codes which has broken this?
>
>
>>
>> 2. The description of the change is not accurate: "supporting empty
>> elements for base64Binary elements." The previous version of the code
>> does support empty elements, at least if getElementText conforms to
>> the StAX specs.
>
> I think you refer here.
>
> if (dhr == null) {
> // since we have advance the reader to next have to use the
> // reader.getText
> base64 = reader.getText();
> reader.next();
> }
>
> this code get executed only if dhr is null. that is not the case for this.
>
> try to invoke this service[1] RetByteArray method with an empty datahandler.
>
> i.e. new DataHandler(new ByteArrayDataSource(new byte[0])); with generated
> adb code.
>
> [1]
> http://131.107.72.15/SoapWsdl_BaseDataTypes_XmlFormatter_Service_Indigo/BaseDataTypesDocLitB.svc?wsdl
>
>
> Here is the problem.
>
> ADB parse method invoke this method when the current location is at the
> start element of this element.
>
> <ns3:inByteArray xmlns:ns3="http://tempuri.org/";></ns3:inByteArray>
>
> this method throws an exception here.
> I think this is wrong and it should have return an empty datahandler.
>
> i.e. new DataHandler(new ByteArrayDataSource(new byte[0]))
>
>
>
>> I will provide a unit test that shows this.
>>
> +1. Please provide a unit test case which does work with earlier version and
> does not work with the changed
> version.
>
>
>> If there is a problem around this method, then probably the root cause
>> is located in a completely different area. There are two things that
>> come to my mind:
>>
>> - Could it be that the XMLStreamReader that is passed to the method
>> has an incorrect implementation of getElementText?
>> - Could it be that the method is called in the wrong context, e.g. in
>> a context where the element represented by the current event may have
>> a child element?
>>
>
> please try to invoke the service I have given and have a look at.
>
> thanks,
> Amila.
>
>>
>> Andreas
>>
>>
>> On Wed, Oct 7, 2009 at 16:08, <[email protected]> wrote:
>> > Author: amilas
>> > Date: Wed Oct 7 14:08:20 2009
>> > New Revision: 822747
>> >
>> > URL: http://svn.apache.org/viewvc?rev=822747&view=rev
>> > Log:
>> > supporting empty elements for base64Binary elements.
>> >
>> > Modified:
>> >
>> webservices/commons/trunk/modules/axiom/modules/axiom-api/src/main/java/org/apache/axiom/util/stax/XMLStreamReaderUtils.java
>> >
>> > Modified:
>> webservices/commons/trunk/modules/axiom/modules/axiom-api/src/main/java/org/apache/axiom/util/stax/XMLStreamReaderUtils.java
>> > URL:
>> http://svn.apache.org/viewvc/webservices/commons/trunk/modules/axiom/modules/axiom-api/src/main/java/org/apache/axiom/util/stax/XMLStreamReaderUtils.java?rev=822747&r1=822746&r2=822747&view=diff
>> >
>> ==============================================================================
>> > ---
>> webservices/commons/trunk/modules/axiom/modules/axiom-api/src/main/java/org/apache/axiom/util/stax/XMLStreamReaderUtils.java
>> (original)
>> > +++
>> webservices/commons/trunk/modules/axiom/modules/axiom-api/src/main/java/org/apache/axiom/util/stax/XMLStreamReaderUtils.java
>> Wed Oct 7 14:08:20 2009
>> > @@ -64,17 +64,26 @@
>> > */
>> > public static DataHandler getDataHandlerFromElement(XMLStreamReader
>> reader)
>> > throws XMLStreamException {
>> > +
>> > + // according to the pre and post conditions it is possible to
>> have an
>> > + // empty element eg. <ns3:inByteArray xmlns:ns3="
>> http://tempuri.org/";></ns3:inByteArray> for empty data handlers
>> > + // in that case we return a new data handler.
>> > + // This method is used by adb parser we can not return null
>> since this element is not null.
>> > + reader.next();
>> > + if (!reader.hasText()){
>> > + DataHandler dataHandler = new DataHandler(new
>> ByteArrayDataSource(new byte[0]));
>> > + // return from here since reader at the end element
>> > + return dataHandler;
>> > + }
>> >
>> > DataHandlerReader dhr = getDataHandlerReader(reader);
>> > String base64;
>> > if (dhr == null) {
>> > - // In this case the best way to get the content of the
>> element is using
>> > - // the getElementText method
>> > - base64 = reader.getElementText();
>> > + // since we have advance the reader to next have to use the
>> > + // reader.getText
>> > + base64 = reader.getText();
>> > + reader.next();
>> > } else {
>> > - if (reader.next() != XMLStreamConstants.CHARACTERS) {
>> > - throw new XMLStreamException("Expected a CHARACTER
>> event");
>> > - }
>> > if (dhr.isBinary()) {
>> > DataHandler dh = dhr.getDataHandler();
>> > reader.next();
>> >
>> >
>> >
>>
>
>
>
> --
> Amila Suriarachchi
> WSO2 Inc.
> blog: http://amilachinthaka.blogspot.com/
>
