Implementation of XEP-0313: Message Archive Management (MAM) #76

ramabit · 2016-05-02T13:06:06Z

Documentation: http://xmpp.org/extensions/xep-0313.html

Flowdalic · 2016-05-02T13:30:05Z

After a quick look: Appears to be missing the provider registration and the entry in https://github.com/igniterealtime/Smack/blob/master/documentation/extensions/index.md

Flowdalic · 2016-05-02T13:33:13Z

smack-experimental/src/main/java/org/jivesoftware/smackx/mam/provider/MamFinProvider.java

+ *      Archive Management</a>
+ *
+ */
+public class MamFinProvider {


This and the following provider look unfinished

Didn't know if that provider was necessary

@Flowdalic I am adding providers implementation. Also I must add new tests so this will take several hours.

No problem, you are not in a race against the time. This will likely take some time to merge. In fact, I would welcome if could also write some integration tests for MAM.

@Flowdalic I am having some trouble running the integration tests ("gradle integrationTest" command). Seems to display always errors, even when I didn't change anything

ramabit · 2016-05-02T13:34:56Z

@Flowdalic ah sorry. When I implemented xep-308 I didn't change that file (index.md). Should I do it?

Flowdalic · 2016-05-02T13:35:57Z

When I implemented xep-308 I didn't change that file (index.md). Should I do it?

Sure :)

ramabit · 2016-05-03T18:51:51Z

@Flowdalic I added providers implementation + providers tests + MAM description in index.md

steckbrief · 2016-05-04T11:43:48Z

smack-experimental/src/main/java/org/jivesoftware/smackx/mam/MamManager.java

+
+        List<Forwarded> messages = new ArrayList<>(resultCollector.getCollectedCount());
+
+        for (Message resultMessage = resultCollector.pollResult(); resultMessage != null;) {


This line leads to an endless loop, because only the first message is polled from the queue.
Quick fix:
for (Message resultMessage = resultCollector.pollResult(); resultMessage != null; resultMessage = resultCollector.pollResult()) {

@steckbrief thanks! :)

Flowdalic · 2016-05-13T13:01:51Z

The provider registration is still missing: You need to add them to https://github.com/igniterealtime/Smack/blob/master/smack-experimental/src/main/resources/org.jivesoftware.smack.experimental/experimental.providers so that they are registered when Smack initializes itself.

White at it, please rename MamPacket to MamElements. I want to fade out the term "Packet" from Smack, as it has no definition in the XMPP world.

When done, please squash the commits into one.

ramabit · 2016-05-13T13:49:43Z

@Flowdalic should i also change the name of the package "org.jivesoftware.smackx.mam.packet" to "org.jivesoftware.smackx.mam.elements"?

annovanvliet · 2016-05-17T06:57:23Z

Good initiative and highly appreciated!

Which server did you use as a reference to test against?

Sometime ago I also made an implementation but not pushed it, while there was no consensus about the version of the XEP and the way the server it had implemented.

Flowdalic · 2016-05-17T07:59:06Z

to "org.jivesoftware.smackx.mam.elements"?

'element' please, so that it is consistent with what is done in the message_correct package.

ramabit · 2016-05-17T13:35:39Z

@Flowdalic done

ramabit · 2016-05-17T13:54:19Z

@annovanvliet thanks! :) I am testing it with a MongooseIM server

ramabit · 2016-05-17T21:06:26Z

@Flowdalic what do you think about those last fixes? ^

ramabit · 2016-05-19T20:42:14Z

@Flowdalic I found some bugs here. Fixes coming this week

ramabit · 2016-05-20T18:40:32Z

@Flowdalic Fixes done. You can continue testing it :)

Flowdalic · 2016-05-21T10:25:22Z

smack-experimental/src/main/java/org/jivesoftware/smackx/mam/MamManager.java

+        return queryArchive(mamQueryIQ, 0);
+    }
+
+    private void addAdditionalFields(List<AdditionalField> additionalFields, DataForm dataForm) {


Is there any reason why this method and the following are not static? Also if you design them null safe, then please go with the if (foo == null) return approach, do keep the level indentation down.

@Flowdalic I think that is not necessary to make them static. Those methods were made to refactor the "queryArchive" method.
I didn't understand why the "if (foo == null) return approach" would be correct. If one of those values is null, I have to continue with the query but without applying that filter.

You think in the wrong direction. ;) It is not necessary to make them non-static i.e. virtual methods.

Instead of

static void eventuallyDoSomething(Foo foo, Bar bar) { if (foo != null) { foo.someting(bar); } }

you should write

static void eventuallyDoSomething(Foo foo, Bar bar) { if (foo == null) return; foo.something(bar); }

Ok I'll do that 👍

Flowdalic · 2016-05-21T10:58:57Z

Smack does not model two classes for request and result types of the same IQ. Please remove MamPrefsResultIQ, only MamPrefsIQ should be used.
MamFinExtension: That extension can be removed. I guess you took it from my MAM patch. But this patch was written in a time when there was actually a extension, i.e. the was send as extension element within a message stanza. The new MAM version (0.4 IIRC) does it right and signals the end of the MAM query results with the IQ result of the IQ initating the query. I guess that's where your confusion comes from. So there should only be a MamFinIQ

I did not finish the review, because I've run out of time for today. So there is likely more to come, but the overall impression is good.

For small PRs I'd usually ask people to squash their commits, but since this is a rather larger patch, I'd ask you to add from now on changes on top of 846a (Ideally using --fixup 57ba if you want to be super git leet). This keeps it easier for me to review the delta. Once your branch is ready for merge, we'll squash merge it.

ramabit · 2016-05-23T18:56:07Z

@Flowdalic Several fixes done. I'll wait the feedback :)

naeems · 2016-05-24T12:50:24Z

Hi Flow,
Please make it quick to finalize this PR. I am now a days trying to implement XEP-0313. But this implementation seems more concrete. So please finalize this asap.

Thanks

Flowdalic · 2016-05-31T21:07:21Z

More remarks in order of severity:

queryMamPrefs()'s try block is broken, the code cannot work. This shows that an integration test for this PR would be really nice. Or at least some real world testing.
- also the last LOC of the try block can be done autoside the try block
- the whole code of the function seems to carry a lot of cruft, likely because it is copy&pasted from the other function, e.g. there is no need for extraTimeout
AdditionalField should be removed. I see no reason why those are not simply FormFields
withJid, neverJids and Co are String types, when they should be Jid type
prepareRetrievePreferencesStanza should be simply the no-argument constructor of MamPrefsIQ
Don't announce the MAM feature, this is only for entities providing MAM (although, this is something I was doing wrong in the initial implementation)

ramabit · 2016-06-01T12:57:07Z

@Flowdalic Thanks for the feedback. I am testing MAM implementation using it on an app connected to a XMPP server. I'll fix those things you found and all the bugs I find while testing, I tell you when that is ready :)

Flowdalic · 2016-07-20T06:19:48Z

smack-experimental/src/main/java/org/jivesoftware/smackx/mam/provider/MamPrefsIQProvider.java

+        List<Jid> alwaysJids = null;
+        List<Jid> neverJids = null;
+
+        boolean done = false;


Please remove the done boolean as explained above.

Flowdalic · 2016-07-21T07:02:43Z

Thanks, please now squash all commits of this branch into one. Note that the branch currently conflicts with master, and you have to resolve those conflicts.

An example of a typical git workflow to do this would be:

merge the official master branch into your branch and resolve the commits
create and switch to a new branch mam-squashed, and make sure that this branch HEAD is identical with the latest HEAD of the official master branch
use git merge --squash <old-mam-branch> to squash the commits of the old-mam-branch as a single commit into mam-squashed

Now you have to update this PR with that single commit. You would usually set the HEAD of this PRs source branch to that particular commit.

Flowdalic · 2016-07-22T07:41:52Z

Thanks, your current author string looks like this:

Author: ramabit <f.e.ramirez94@gmail.com

Is this what you want, or do you want your full name, e.g.

Author: John Doe <john@doe.com>

as this will show up in some places and I feel you may want to take the credits for being the major author.

Fixes SMACK-625

Flowdalic · 2016-07-23T12:23:41Z

smack-experimental/src/main/java/org/jivesoftware/smackx/mam/provider/MamFinIQProvider.java

+    public MamFinIQ parse(XmlPullParser parser, int initialDepth) throws Exception {
+        String queryId = parser.getAttributeValue("", "queryid");
+        boolean complete = Boolean.parseBoolean(parser.getAttributeValue("", "complete"));
+        boolean stable = Boolean.parseBoolean(parser.getAttributeValue("", "stable"));


Careful when parsing. stable defaults to true according to xep313. Should have used ParserUtils.getBooleanAttribute().

Flowdalic · 2016-07-23T12:24:22Z

I take over from here but will add notes about the things I found while testing your code.

Flowdalic · 2016-07-23T12:57:47Z

smack-experimental/src/main/java/org/jivesoftware/smackx/mam/MamManager.java

+     * @throws InterruptedException
+     * @throws NotLoggedInException
+     */
+    public MamPrefsResult updateArchivingPreferences(List<Jid> alwaysJids, List<Jid> neverJids, String defaultField)


defaultField should have been an enum. Never give the user a chance to use false values.

Flowdalic · 2016-07-23T13:21:38Z

smack-experimental/src/test/java/org/jivesoftware/smackx/mam/MamFinProviderTest.java

+        Assert.assertNull(mamFinIQ.getQueryId());
+
+        RSMSet rsmSet = mamFinIQ.getRSMSet();
+        Assert.assertEquals(rsmSet.getAfter(), "09af3-cc343-b409f");


The first argument of assertEquals should always be the expected value.

Flowdalic · 2016-07-23T13:48:23Z

Applied with 189cac0. And I've put some modifications on top with aeb385a. I hope those do not break anything, but even if so, then we can still fix it. Merged it without intensive integration testing as I wanted to have some progress here.

Thanks @ramabit for your help.

ojuniour · 2016-10-03T13:21:59Z

ANy Reason why I'm getting this error (even though the archives were successfully received?

java.lang.ClassCastException: org.jivesoftware.smack.packet.EmptyResultIQ cannot be cast to org.jivesoftware.smackx.mam.element.MamFinIQ

ramabit · 2016-10-03T13:31:43Z

@ojuniour I would need more information about the fin IQ stanza received.
Please open a question or report an issue:
here https://community.igniterealtime.org/community/developers/smack
following this https://github.com/igniterealtime/Smack/wiki/How-to-ask-for-help,-report-an-issue-and-possible-solve-the-problem-yourself
and adding more descriptive context

ojuniour · 2016-10-04T01:57:33Z

Thanks for the fast response.. I'm gonna do as you said.. But just a quick question; what version of XMPP-0313 protocol did you implement? I would guess it's between between v0.3, 0.4, 0.4.1 and v0.5? Because I receive an packet first, then data comes after, and finally a to conclude..(mam1 namespace btw).
I will start a ticket at the link, and post the link here so you can follow (especially since you contributed most to the code.. so perhaps you have better understanding of the issue). Thank you.

EDIT: This is the link to the FULL details :https://community.igniterealtime.org/message/260514

Flowdalic · 2016-10-04T13:41:39Z

Please do not hijack pull requests when having issues and use the forums instead. Thank you.

Flowdalic reviewed May 2, 2016
View reviewed changes

ramabit force-pushed the master branch 4 times, most recently from b2e02be to b43b304 Compare May 3, 2016 18:30

ramabit force-pushed the master branch 2 times, most recently from 68b2460 to bc791a1 Compare May 3, 2016 22:00

steckbrief reviewed May 4, 2016
View reviewed changes

ramabit force-pushed the master branch from bc791a1 to 8cd010e Compare May 4, 2016 18:49

ramabit force-pushed the master branch from 8cd010e to afc61ac Compare May 17, 2016 13:24

ramabit force-pushed the master branch from bc7af3e to 846ac6f Compare May 20, 2016 17:37

Flowdalic reviewed May 21, 2016
View reviewed changes

Flowdalic reviewed Jul 20, 2016
View reviewed changes

ramabit force-pushed the master branch 2 times, most recently from e7bebf4 to 642cb6f Compare July 20, 2016 19:35

ramabit force-pushed the master branch 3 times, most recently from 3f58a3b to ac972b9 Compare July 21, 2016 15:33

ramabit force-pushed the master branch from ac972b9 to 8290b48 Compare July 22, 2016 14:49

Implement Message Archive Management (MAM) XEP-0313

6e8b96e

Fixes SMACK-625

ramabit force-pushed the master branch from 8290b48 to 6e8b96e Compare July 22, 2016 14:55

Flowdalic reviewed Jul 23, 2016
View reviewed changes

igniterealtime unlocked this conversation Jul 23, 2016

Flowdalic reviewed Jul 23, 2016
View reviewed changes

Flowdalic closed this Jul 23, 2016

igniterealtime locked and limited conversation to collaborators Oct 4, 2016


		List<Forwarded> messages = new ArrayList<>(resultCollector.getCollectedCount());

		for (Message resultMessage = resultCollector.pollResult(); resultMessage != null;) {

Implementation of XEP-0313: Message Archive Management (MAM) #76

Implementation of XEP-0313: Message Archive Management (MAM) #76

Conversation

ramabit commented May 2, 2016

Flowdalic commented May 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramabit commented May 2, 2016

Flowdalic commented May 2, 2016

ramabit commented May 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flowdalic commented May 13, 2016

ramabit commented May 13, 2016

annovanvliet commented May 17, 2016

Flowdalic commented May 17, 2016

ramabit commented May 17, 2016

ramabit commented May 17, 2016

ramabit commented May 17, 2016

ramabit commented May 19, 2016 • edited Loading

ramabit commented May 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flowdalic commented May 21, 2016 • edited Loading

ramabit commented May 23, 2016

naeems commented May 24, 2016

Flowdalic commented May 31, 2016

ramabit commented Jun 1, 2016

Choose a reason for hiding this comment

Flowdalic commented Jul 21, 2016

Flowdalic commented Jul 22, 2016

Choose a reason for hiding this comment

Flowdalic commented Jul 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flowdalic commented Jul 23, 2016

ojuniour commented Oct 3, 2016

ramabit commented Oct 3, 2016 • edited Loading

ojuniour commented Oct 4, 2016 • edited Loading

Flowdalic commented Oct 4, 2016

ramabit commented May 19, 2016 •

edited

Loading

Flowdalic commented May 21, 2016 •

edited

Loading

ramabit commented Oct 3, 2016 •

edited

Loading

ojuniour commented Oct 4, 2016 •

edited

Loading