Parsing custom attributes of message element

Hi all,

just a message to describe a pull request I will make soon after writing this.

Why ?

I am developing an Android application using Smack Android and I face a case where the server does not respect the XMPP standards by putting some custom attributes inside the element. I don’t remember where but I read a post from @Flow saying that, as this is not a standard XMPP practice, the Smack library don’t want to be polluted with bad practices use-case handling and that the server should change it’s workflow to respect standards (“XMPP is Sacred”).

I could not agree more on this, however in my use case I don’t have access to the server code and I imagine that other people may face the same issue.

How ?

In order to solve my problem I added a small chunk of code inside the parsing of a stanza that retrieve custom attributes and bundle them inside a custom extension element.

This way the object model of Smack stays relevant and standard to XMPP protocol and the “ugly” server use-case is treated in a standard and clean way from the client side.

I know that allowing those “wrong” use-case in the library may not encourage servers to respect the XMPP standards but treating them in a clean way may educate them to good practices.

Associated PR : Parsing of custom message attributes by Judas · Pull Request #69 · igniterealtime/Smack · GitHub

So ?

Anyway what do you think of this approach ?

I only modified the code for the stanza and not or , do you think those would also need this feature ?

The XMPP protocol is extensible to a ridiculous degree. The X in XMPP means exactly that: Extensible. The few points that it is strict in, serves interoperability - based on those few rules, XMPP can be used across many implementations. I’m with Flow if he feels that we shouldn’t allow for spec-breaking changes in Smack - in the long run, that will do more harm than good.

First I’d like to say that I’m really happy to see a PR with unit tests. That’s seldom those days. +1

I’m actually considering such a change if it meets the following requirements:

  1. It should not be designed as ExtensionElement. For whatever reason you thought that this would be a good idea: It is not. Also we are not talking about “custom message attributes” but about “custom stanza attributes” and as such it should be designed as an API of the Stanza class, i.e. a simply setCustomAttribute(String, String) and getCustomAttribute(String), where the attribute is (can be) a qName!

  2. Using the API should throw an Exception is it’s not explicitly enabled (e.g. by setting a static boolean). The Exception message and the Javadoc of the method enabling the API should come with a big bold disclaimer stating explicitly that this is not a good idea.

  3. The API should initialize its data structures, I imagine this would be only a LinkedHashMap, lazily.

Hi @Flow, sorry about the late response, I have been quite busy recently. First of all thank you for considering this change.

The reason I chose to use an ExtensionElement to achieve this modification may come from the road that led me to create this PR.

When I found that the XMPP server I depend on uses custom attributes inside message stanzas (and thus that Smack was not handling them because it is a bad practice), I first thought of the good way the server should behave. And following the documentation I read the good way of achieving this would have been to create a custom ExtensionElement with the data (maybe I’m wrong about this and there is a better way to do this).

That in mind I decided to create a small piece of code transforming this bad practice (custom attributes) into a standard & client-supported way (ExtensionElement).

I only made the change in the message class because it was fitting my use case and I did not want to go further if the modification was not in line with Smack philosophy. I will for sure move it to stanza.

Talk to you soon with a PR updated with your pointers !


It’s up right now

An IllegalStateException is thrown in case the feature is not explicitly enabled by the user but I’m wondering if it would’nt be better to use a custom Exception for this. What’s your thought ?