Bug in PacketParserUtils

Hi,

On line 329 of PacketParserUtils (parseContentDepth()) you have:

else if (event == XmlPullParser.TEXT) {

xml.append(parser.getText());

}

this should be:

else if (event == XmlPullParser.TEXT) {

xml.append(StringUtils.escapeForXML(parser.getText()));

}

Someone else had a similar issue somewhere else and you fixed it, I can’t find this discussion anymore unfortunately, I have had this fix in my code for several months and I thought I should report it. I think the issue was that the parser unescaped the text so you have to re-escape it here. I had escaped the text before sending it and I had verified that it was received escaped by Smack. I don’t remember all the details as this was several months ago. I believe this class has this issue in several places.

Cheers,

Gabriel

It’s actual a tricky question where the escaping should happen. I don’t think that it should happen in parseContentDepth(). Why do you think you want here escapeForXML()? Maybe we can add that somewhere else where it’s more appropriate.

Yes, agreed this is tricky. I believe the parser should hand the code above it an unaltered version of what it parsed. In this sense it should be done (the escaping) as far down as possible. The ideal solution IMO would be to abstract the actual parser implementation and implement this code inside a concrete implementation (as this issue this is a side effect of the parser being used I believe). This would allow us to switch out the implementation used if needed.

What do you think?

The ideal solution IMO would be to abstract the actual parser implementation and implement this code inside a concrete implementation (as this issue this is a side effect of the parser being used I believe).
No, the XmlPullParser context is pretty well defined and it says that getText() returns the String unescaped.

But anyway, I have a slight feeling that if we add the escapeForXML in content depth, then some things would break or escaped XML would appear where the user would expect/want unsecaped XML.

Could you describe your use case? So that I can assess the situation better. What made you think that there’s an escapeForXML() missing? What was the trigger?

No, the XmlPullParser context is pretty well defined and it says that getText() returns the String unescaped.

Yes, I meant more in the sense that it is a side effect from Smack’s perspective, if you were to change parsers the new parser may not have the same way of dealing with CData and may hand you excaped data, this is the case an XML parser I am using on the server side.

But anyway, I have a slight feeling that if we add the escapeForXML in content depth, then some things would break or escaped XML would appear where the user would expect/want unsecaped XML.

You may be right, even thought I am certain you had done something similar in response to a bug report a while back, I can’t find it anymore.

Could you describe your use case? So that I can assess the situation better. What made you think that there’s an escapeForXML() missing? What was the trigger?

Yes, the trigger was I got a exceptions in Smack and I had traced it down to this. I don’t remember the specifics though, just that this fixed it. I unfortunately don’t have more time to spend on this, so I suggest just to park the issue for now since I undestand you can’t implement it without being sure. I will just keep on using my version.

Thanks