OF-210: Additional work. (#923)

* Support for Roster Versioning (without send the modifications via roster pushes)

* Roster versioning comparison clearing

* Implementation Note: This empty IQ-result is different from an empty <query/> element, thus disambiguating this usage from an empty roster.

* Avoid cache miss while updating roster

When the roster is updated via group renaming, group user adding or removing, the roster pushes only happen if there is a cache hit. If there is a cache miss (what can happen if the cache is full or if the admin cleaned up the cache) the user is not properly notified about the roster update. Thus only update rosters in memory can lead to this undesired behavior.

This commit avoids the use of the cache directly (where there can be a cache miss or a cache hit). It is using the method getRoster(username) that instantiante a new Roster in the case of a cache miss.

* Clarify the code

* OF-210: Base roster version on its hashCode.

This commit removes all fields from the Roster class that do not relate to its state
(replacing them with method variables - which seems harmless, as they're all final
singletons). This allows for an easy override of Object#hashCode() and equals().
These, in turn, are used to calculate the roster version from.

* Simplified loop

* Prevent potential NPEs.

* Log exceptions for exceptions that cannot happen.

If they cannot happen, we should scream murder if they do...

* OF-210: Roster versioning enabled by default.

src/java/org/jivesoftware/openfire/handler/IQRosterHandler.java

@@ -182,7 +182,23 @@ public class IQRosterHandler extends IQHandler implements ServerFeaturesProvider
 
             Roster cachedRoster = userManager.getUser(sender.getNode()).getRoster();
             if (IQ.Type.get == type) {
+
+                if (RosterManager.isRosterVersioningEnabled()) {
+                    String clientVersion = packet.getChildElement().attributeValue("ver");
+                    String latestVersion = String.valueOf( cachedRoster.hashCode() );
+                    // Whether or not the roster has been modified since the version ID enumerated by the client, ...
+                    if (!latestVersion.equals(clientVersion)) {
+                        // ... the server MUST either return the complete roster
+                        // (including a 'ver' attribute that signals the latest version)
+                        returnPacket = cachedRoster.getReset();
+                        returnPacket.getChildElement().addAttribute("ver", latestVersion );
+                    } else {
+                        // ... or return an empty IQ-result
+                        returnPacket = new org.xmpp.packet.IQ();
+                    }
+                } else {
                     returnPacket = cachedRoster.getReset();
+                }
                 returnPacket.setType(IQ.Type.result);
                 returnPacket.setTo(sender);
                 returnPacket.setID(packet.getID());

src/java/org/jivesoftware/openfire/roster/RosterManager.java

@@ -73,6 +73,15 @@ public class RosterManager extends BasicModule implements GroupEventListener, Us
         return JiveGlobals.getBooleanProperty("xmpp.client.roster.active", true);
     }
 
+    /**
+     * Returns true if the roster versioning is enabled.
+     *
+     * @return true if the roster versioning is enabled.
+     */
+    public static boolean isRosterVersioningEnabled() {
+        return JiveGlobals.getBooleanProperty("xmpp.client.roster.versioning.active", true);
+    }
+
     public RosterManager() {
         super("Roster Manager");
         rosterCache = CacheFactory.createCache("Roster");
@@ -143,6 +152,7 @@ public class RosterManager extends BasicModule implements GroupEventListener, Us
                 }
                 catch (SharedGroupException e) {
                     // Do nothing. We shouldn't have this exception since we disabled the checkings
+                    Log.warn( "Unexpected exception while deleting roster of user '{}' .", user, e );
                 }
             }
             // Remove the cached roster from memory
@@ -160,9 +170,10 @@ public class RosterManager extends BasicModule implements GroupEventListener, Us
                 }
                 catch (SharedGroupException e) {
                     // Do nothing. We shouldn't have this exception since we disabled the checkings
+                    Log.warn( "Unexpected exception while deleting roster of user '{}' .", user, e );
                 }
                 catch (UserNotFoundException e) {
-                    // Do nothing.
+                    // Deleted user had user that no longer exists on their roster. Ignore and move on.
                 }
             }
         }
@@ -348,13 +359,16 @@ public class RosterManager extends BasicModule implements GroupEventListener, Us
             // Iterate on all the affected users and update their rosters
             for (JID updatedUser : users) {
                 // Get the roster to update.
-                Roster roster = null;
+                Roster roster;
                 if (server.isLocal(updatedUser)) {
-                    roster = rosterCache.get(updatedUser.getNode());
-                }
-                if (roster != null) {
+                    try {
+                        roster = getRoster(updatedUser.getNode());
+
                         // Update the roster with the new group display name
                         roster.shareGroupRenamed(users);
+                    } catch (UserNotFoundException e) {
+                        Log.debug( "Unexpected exception while applying group modification for user '{}' .", updatedUser.getNode(), e );
+                    }
                 }
             }
         }
@@ -538,19 +552,17 @@ public class RosterManager extends BasicModule implements GroupEventListener, Us
                 // Get the roster to update
                 Roster roster = null;
                 if (server.isLocal(userToUpdate)) {
-                    // Check that the user exists, if not then continue with the next user
+                    // Get the roster. If the user does not exist then continue with the next user
                     try {
-                        UserManager.getInstance().getUser(userToUpdate.getNode());
+                        roster = getRoster(userToUpdate.getNode());
                     }
                     catch (UserNotFoundException e) {
                         continue;
                     }
-                    roster = rosterCache.get(userToUpdate.getNode());
                 }
-                // Only update rosters in memory
-                if (roster != null) {
+                // Update roster
                 roster.addSharedUser(group, newUserJID);
-                }
+
                 if (!server.isLocal(userToUpdate)) {
                     // Susbcribe to the presence of the remote user. This is only necessary for
                     // remote users and may only work with remote users that **automatically**
@@ -577,19 +589,17 @@ public class RosterManager extends BasicModule implements GroupEventListener, Us
                 // Get the roster to update
                 Roster roster = null;
                 if (server.isLocal(userToUpdate)) {
-                    // Check that the user exists, if not then continue with the next user
+                    // Get the roster. If the user does not exist then continue with the next user
                     try {
-                        UserManager.getInstance().getUser(userToUpdate.getNode());
+                        roster = getRoster(userToUpdate.getNode());
                     }
                     catch (UserNotFoundException e) {
                         continue;
                     }
-                    roster = rosterCache.get(userToUpdate.getNode());
                 }
-                // Only update rosters in memory
-                if (roster != null) {
+                // Update roster
                 roster.deleteSharedUser(group, userJID);
-                }
+
                 if (!server.isLocal(userToUpdate)) {
                     // Unsusbcribe from the presence of the remote user. This is only necessary for
                     // remote users and may only work with remote users that **automatically**
@@ -639,7 +649,11 @@ public class RosterManager extends BasicModule implements GroupEventListener, Us
         // Get the roster of the added user.
         Roster addedUserRoster = null;
         if (server.isLocal(addedUser)) {
-            addedUserRoster = rosterCache.get(addedUser.getNode());
+            try {
+                addedUserRoster = getRoster(addedUser.getNode());
+            } catch (UserNotFoundException e) {
+                Log.warn( "Unexpected exception while adding user '{}' to group '{}'.", addedUser, group, e );
+            }
         }
 
         // Iterate on all the affected users and update their rosters
@@ -648,23 +662,25 @@ public class RosterManager extends BasicModule implements GroupEventListener, Us
                 // Get the roster to update
                 Roster roster = null;
                 if (server.isLocal(userToUpdate)) {
-                    // Check that the user exists, if not then continue with the next user
+                    // Get the roster. If the user does not exist then continue with the next user
                     try {
-                        UserManager.getInstance().getUser(userToUpdate.getNode());
+                        roster = getRoster(userToUpdate.getNode());
                     }
                     catch (UserNotFoundException e) {
                         continue;
                     }
-                    roster = rosterCache.get(userToUpdate.getNode());
                 }
-                // Only update rosters in memory
-                if (roster != null) {
+                // Update roster
                 roster.addSharedUser(group, addedUser);
-                }
+
                 // Check if the roster is still not in memory
                 if (addedUserRoster == null && server.isLocal(addedUser)) {
+                    try {
                         addedUserRoster =
-                            rosterCache.get(addedUser.getNode());
+                                getRoster(addedUser.getNode());
+                    } catch (UserNotFoundException e) {
+                        Log.warn( "Unexpected exception while adding user '{}' to group '{}'.", addedUser, group, e );
+                    }
                 }
                 // Update the roster of the newly added group user.
                 if (addedUserRoster != null) {
@@ -708,7 +724,11 @@ public class RosterManager extends BasicModule implements GroupEventListener, Us
         // Get the roster of the deleted user.
         Roster deletedUserRoster = null;
         if (server.isLocal(deletedUser)) {
-            deletedUserRoster = rosterCache.get(deletedUser.getNode());
+            try {
+                deletedUserRoster = getRoster(deletedUser.getNode());
+            } catch (UserNotFoundException e) {
+                Log.warn( "Unexpected exception while deleting user '{}' from group '{}'.", deletedUser, group, e );
+            }
         }
 
         // Iterate on all the affected users and update their rosters
@@ -716,24 +736,17 @@ public class RosterManager extends BasicModule implements GroupEventListener, Us
             // Get the roster to update
             Roster roster = null;
             if (server.isLocal(userToUpdate)) {
-                // Check that the user exists, if not then continue with the next user
+                // Get the roster. If the user does not exist then continue with the next user
                 try {
-                    UserManager.getInstance().getUser(userToUpdate.getNode());
+                    roster = getRoster(userToUpdate.getNode());
                 }
                 catch (UserNotFoundException e) {
                     continue;
                 } 
-                roster = rosterCache.get(userToUpdate.getNode());
             }
-            // Only update rosters in memory
-            if (roster != null) {
+            // Update roster
             roster.deleteSharedUser(group, deletedUser);
-            }
-            // Check if the roster is still not in memory
-            if (deletedUserRoster == null && server.isLocal(deletedUser)) {
-                deletedUserRoster =
-                        rosterCache.get(deletedUser.getNode());
-            }
+
             // Update the roster of the newly deleted group user.
             if (deletedUserRoster != null) {
                 deletedUserRoster.deleteSharedUser(userToUpdate, group);

src/java/org/jivesoftware/openfire/session/LocalClientSession.java

@@ -29,6 +29,7 @@ import org.jivesoftware.openfire.cluster.ClusterManager;
 import org.jivesoftware.openfire.net.SASLAuthentication;
 import org.jivesoftware.openfire.privacy.PrivacyList;
 import org.jivesoftware.openfire.privacy.PrivacyListManager;
+import org.jivesoftware.openfire.roster.RosterManager;
 import org.jivesoftware.openfire.spi.ConnectionConfiguration;
 import org.jivesoftware.openfire.streammanagement.StreamManager;
 import org.jivesoftware.openfire.user.PresenceEventDispatcher;
@@ -877,6 +878,12 @@ public class LocalClientSession extends LocalSession implements ClientSession {
                     "<compression xmlns=\"http://jabber.org/features/compress\"><method>zlib</method></compression>");
         }
 
+        // If a server supports roster versioning, 
+        // then it MUST advertise the following stream feature during stream negotiation.
+        if (RosterManager.isRosterVersioningEnabled()) {
+            sb.append("<ver xmlns=\"urn:xmpp:features:rosterver\"/>");
+        }
+
         if (getAuthToken() == null) {
             // Advertise that the server supports Non-SASL Authentication
             if ( XMPPServer.getInstance().getIQRouter().supports( "jabber:iq:auth" ) ) {