Parsing/setting content of error conditions (e.g. redirect)

Smack Smack Dev
1

Some error conditions (redirect, gone, etc.) contain a text node with information, in the case of redirect it is typically the IRI of the entity that is redirected to.

Example:

C: <presence

  from='juliet@im.example.com/balcony'

id='y2bs71v4'

  to='characters@muc.example.com/JulieC'>

  <x xmlns='[http://jabber.org/protocol/muc'/](http://jabber.org/protocol/muc'/)>

</presence>

E: <presence

  from='characters@muc.example.com/JulieC'

id='y2bs71v4'

  to='juliet@im.example.com/balcony'

type='error'>

<error type='modify'>

<redirect xmlns='urn:ietf:params:xml:ns:xmpp-stanzas'>

  xmpp:characters@conference.example.org

</redirect>

</error>

</presence>

See also: Extensible Messaging and Presence Protocol (XMPP): Core

Smack currently does not parse the text content and offers no way to set it.

This would only require these minor changes:

  • addition of parser code for text node of some error types in PacketParserUtils.parseError
  • extension of XMPPError to allow access to the content
  • extension of XMPPError.toXML() to include it in the serialization
2

SMACK-608, patches welcome

3

Hi,

would you see the following implementation approach fit for a patch?

  • add an optional String (bean-) property “content” to XMPPErorror.Condition

  • adding code to parse/serialize the text nodes to the provider.

  • add a Method XMPPError.getConditionObject() that returns the object instead of the string

It will not break the API but allow access to the condition’s content.

I did of course imply changing the tests accordingly.

4

add an optional String (bean-) property “content” to XMPPErorror.Condition
A String member is fine. I’m not sure if “content” is a good name for it. Maybe “text” or “optionalText”? What exactly makes a property a Java bean property for you? The existence of get and set? I don’t like having setters for Smack classes that represent stream elements or content of such. In the long term I’d like to get those classes immutable (with the exception of adding/removing PacketExtensions). So the value (however we call it) should only be set via the constructor and the field should be declared final.

add a Method XMPPError.getConditionObject() that returns the object instead of the string
I’m not a big fan of methods that return Object types in static typed languages. I think it’s ok if we change the return type of XMPPError.getCondition() from String to Condition. Not really that helpful, but Condition implements CharSequence, so the transition should be easy for users. I don’t follow semantic versioning in Smack (yet), so the API change is ok for the 4.1 release. I only keep API compatibility for patch level releases.

So, if you have an XMPPError, you can simply call xmppError.getCondition().getAdditionalText() (which may returns null) to get the additional text of the condition.

5

Flow wrote:

A String member is fine. I’m not sure if “content” is a good name for it. Maybe “text” or “optionalText”?

I would prefer text then or “additionalText” as you mentioned below, I did not see optional as a “can be null” Marker on other methods in smack, so I would stick to that.

What exactly makes a property a Java bean property for you? The existence of get and set? I don’t like having setters for Smack classes that represent stream elements or content of such. In the long term I’d like to get those classes immutable (with the exception of adding/removing PacketExtensions). So the value (however we call it) should only be set via the constructor and the field should be declared final.

Yes, I meant getter/setters but i understand your point and think its perfectly valid.

I’m not a big fan of methods that return Object types in static typed languages. I think it’s ok if we change the return type of XMPPError.getCondition() from String to Condition. Not really that helpful, but Condition implements CharSequence, so the transition should be easy for users. I don’t follow semantic versioning in Smack (yet), so the API change is ok for the 4.1 release. I only keep API compatibility for patch level releases.

So, if you have an XMPPError, you can simply call xmppError.getCondition().getAdditionalText() (which may returns null) to get the additional text of the condition.

It was of course the intention to avoid braking the API, but that is surely the cleaner approach.

6

I’ve pushed a commit named “Rework XMPP Error class design” to my repo at Flowdalic/Smack · GitHub . That commit should address issue SMACK-608 amongst others. Feedback appreciated. Otherwise it will land in the official repo after I’ve finished testing.