Within our organization we have deployed a multitude of chat servers, and use our own chat client developed on top of Smack. Our users use MAM (XEP-0313) to query the message archive.
The chat servers we use are of various vendors and versions, and they support various versions of the MAM extension: MAMv1 and MAMv2 (some might still use MAMv0 even). As the Smack implementation of MAM only supports one version, we’ve until now used copies of the smack code, slightly modified to support a different MAM version. This is pretty ugly though, and makes upgrading to new Smack versions cumbersome.
So we would like to propose a patch for the Smack MAM implementation to support multiple versions of the MAM (v1 and v2). Before I submit a pull request for such a patch, I would like know if you think this would be a good idea.
My current idea about the implementation would be to make the following changes:
change the org.jivesoftware.smackx.mam.element.*IQ classes to have a configurable mam version, that defines what namespace (urn:xmpp:mam:1 or urn:xmpp:mam:2) is used
create subclasses of MamResultExtension for each version of MAM
change the org.jivesoftware.smackx.mam.provider.*Provider classes to create IQ’s and Extension elements for both v1 and v2 URIs. The providers can then be registered for both mam namespaces.
change the MamManager to do feature discovery to find out what version of MAM is supported by the server, and use the highest version that is also supported by Smack. Possible this behavior should be enabled explicitly, defaulting to v2?
somehow ensure we are compliant with the mam version being used/supported.
I’d like to hear your thoughts and comments! I’ve already started implementing, and hope to have a working implementation to show soon.
The delta between mam:1 and mam:2 is not that big. Hence I wonder if your time and effort wouldn’t better invested in upgrading the MAM services you have to support mam:2. Especially considering that Multi namespace version support is always a maintenance burden. It is something I try to avoid as part of my spare time development of Smack. Smack not supporting mam:1 is not an issue for the use cases I like to support with my spare-time work, i.e. free installations, enthusiasts and FOSS. It is always interesting how large scale corporations and organizations seem to perform worse here (although, I’ve collected enough experience from my professional work to see why this is the case).
That said, if you really want to push this forward, then here is some feedback after a quick glance at the PR:
Design mamNamespace as enum, not as String, and pass it around as such
I think in most cases you can use parser.getNamespace() instead of xmlEnvironment.getEffectiveNamepsace()
rebase the PR on the current master
MamV1ResultExtension MamV2ResultExtension
with static QNAME field
there are a ton of unrelated whitespace changes, please remove these
I don’t believe that the methods that add new mam namespaces should be part of Smack’s public API. Their access modifier should be more restrictive (I assume probably mostly package protected). The reason is that I don’t see how you can simply add future MAM namespaces to the thingy, without modification of the logic in MamManager. And this last part is not plugable (nor should it be).
If I am not mistaken, then the idea of the current PR is that the user specifies the MAM namespace to use. That is a no go, as it causes unnecessary API parts like MamManager.setNamespace(String)? Why should a user ever want to use this, as opposed to the MamManager automatically selecting the best namespace?
Thank you for your feedback, I much appreciate the effort you are putting into Smack, and completely understand that you want to prevent any unneeded maintenance burden.
We’ve been discussing the possibility of upgrading to servers supporting MAMv2, however in our organization and the organizations we support this is kinda unlikely to happen at a reasonable pace. We just do not have the control as a chat client provider.
So I would like to see if we can add the multiversion support for MAM. I’ve create a new PR, hopefully resolving all your feedback. I’ve restricted access to prevent extending the public API, except for a getMamNamespace() method that returns the namespace used by the manager.