OF-278: Avoid loading all shared groups into memory/cache when possible.

NOTE: This is a partial refactor of the Openfire groups component. Additional review and testing is requested. Please update the corresponding JIRA ticket with comments or concerns.

git-svn-id: http://svn.igniterealtime.org/svn/repos/openfire/trunk@13380 b35dd754-fafc-0310-a699-88a17e54d16e

src/java/org/jivesoftware/openfire/clearspace/ClearspaceGroupProvider.java

@@ -18,25 +18,31 @@
  */
 package org.jivesoftware.openfire.clearspace;
 
-import org.dom4j.Element;
-import org.dom4j.Node;
-import org.jivesoftware.openfire.XMPPServer;
 import static org.jivesoftware.openfire.clearspace.ClearspaceManager.HttpType.GET;
 import static org.jivesoftware.openfire.clearspace.WSUtils.getReturn;
 import static org.jivesoftware.openfire.clearspace.WSUtils.parseStringArray;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+
+import org.dom4j.Element;
+import org.dom4j.Node;
+import org.jivesoftware.openfire.XMPPServer;
+import org.jivesoftware.openfire.group.AbstractReadOnlyGroupProvider;
 import org.jivesoftware.openfire.group.Group;
-import org.jivesoftware.openfire.group.GroupAlreadyExistsException;
+import org.jivesoftware.openfire.group.GroupCollection;
 import org.jivesoftware.openfire.group.GroupNotFoundException;
-import org.jivesoftware.openfire.group.GroupProvider;
 import org.jivesoftware.openfire.user.UserNotFoundException;
 import org.xmpp.packet.JID;
 
-import java.util.*;
-
 /**
  * @author Daniel Henninger
  */
-public class ClearspaceGroupProvider implements GroupProvider {
+public class ClearspaceGroupProvider extends AbstractReadOnlyGroupProvider {
     protected static final String URL_PREFIX = "socialGroupService/";
 
     private static final String TYPE_ID_OWNER = "0";
@@ -45,26 +51,10 @@ public class ClearspaceGroupProvider implements GroupProvider {
     public ClearspaceGroupProvider() {
     }
 
-    public Group createGroup(String name) throws UnsupportedOperationException, GroupAlreadyExistsException {
-        throw new UnsupportedOperationException("Could not create groups.");
-    }
-
-    public void deleteGroup(String name) throws UnsupportedOperationException {
-        throw new UnsupportedOperationException("Could not delete groups.");
-    }
-
     public Group getGroup(String name) throws GroupNotFoundException {
         return translateGroup(getGroupByName(name));
     }
 
-    public void setName(String oldName, String newName) throws UnsupportedOperationException, GroupAlreadyExistsException {
-        throw new UnsupportedOperationException("Could not modify groups.");
-    }
-
-    public void setDescription(String name, String description) throws GroupNotFoundException {
-        throw new UnsupportedOperationException("Could not modify groups.");
-    }
-
     public int getGroupCount() {
         try {
             String path = URL_PREFIX + "socialGroupCount";
@@ -76,12 +66,29 @@ public class ClearspaceGroupProvider implements GroupProvider {
         }
     }
 
-    public Collection<String> getSharedGroupsNames() {
+    public boolean isSharingSupported() {
+		return true;
+	}
+
+    public Collection<String> getSharedGroupNames() {
         // Return all social group names since every social group is a shared group
         return getGroupNames();
     }
 
-    public Collection<String> getGroupNames() {
+	public Collection<String> getSharedGroupNames(JID user) {
+		// TODO: is there a better way to get the shared Clearspace groups for a given user?
+		Collection<String> result = new ArrayList<String>();
+		Iterator<Group> sharedGroups = new GroupCollection(getGroupNames()).iterator();
+		while (sharedGroups.hasNext()) {
+			Group group = sharedGroups.next();
+			if (group.isUser(user)) {
+				result.add(group.getName());
+			}
+		}
+		return result;
+	}
+
+	public Collection<String> getGroupNames() {
         try {
             String path = URL_PREFIX + "socialGroupNames";
             Element element = ClearspaceManager.getInstance().executeRequest(GET, path);
@@ -120,22 +127,6 @@ public class ClearspaceGroupProvider implements GroupProvider {
         }
     }
 
-    public void addMember(String groupName, JID user, boolean administrator) throws UnsupportedOperationException {
-        throw new UnsupportedOperationException("Could not modify groups.");
-    }
-
-    public void updateMember(String groupName, JID user, boolean administrator) throws UnsupportedOperationException {
-        throw new UnsupportedOperationException("Could not modify groups.");
-    }
-
-    public void deleteMember(String groupName, JID user) throws UnsupportedOperationException {
-        throw new UnsupportedOperationException("Could not modify groups.");
-    }
-
-    public boolean isReadOnly() {
-        return true;
-    }
-
     public Collection<String> search(String query) {
         throw new UnsupportedOperationException("Group search is not supported");
     }

src/java/org/jivesoftware/openfire/group/AbstractReadOnlyGroupProvider.java

+package org.jivesoftware.openfire.group;
+
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Map;
+
+import org.jivesoftware.util.Immutable;
+import org.xmpp.packet.JID;
+
+/**
+ * Common base class for immutable (read-only) GroupProvider implementations.
+ *
+ * @author Tom Evans
+ */
+public abstract class AbstractReadOnlyGroupProvider implements GroupProvider {
+	
+	// Mutator methods are marked final for read-only group providers
+
+	/**
+	 * @throws UnsupportedOperationException
+	 */
+    public final void addMember(String groupName, JID user, boolean administrator) throws UnsupportedOperationException
+    {
+        throw new UnsupportedOperationException("Cannot add members to read-only groups");
+    }
+
+	/**
+	 * @throws UnsupportedOperationException
+	 */
+    public final void updateMember(String groupName, JID user, boolean administrator) throws UnsupportedOperationException
+    {
+        throw new UnsupportedOperationException("Cannot update members for read-only groups");
+    }
+
+	/**
+	 * @throws UnsupportedOperationException
+	 */
+    public final void deleteMember(String groupName, JID user) throws UnsupportedOperationException
+    {
+        throw new UnsupportedOperationException("Cannot remove members from read-only groups");
+    }
+
+    /**
+     * Always true for a read-only provider
+     */
+    public final boolean isReadOnly() {
+        return true;
+    }
+
+	/**
+	 * @throws UnsupportedOperationException
+	 */
+    public final Group createGroup(String name) throws UnsupportedOperationException {
+        throw new UnsupportedOperationException("Cannot create groups via read-only provider");
+    }
+
+	/**
+	 * @throws UnsupportedOperationException
+	 */
+    public final void deleteGroup(String name) throws UnsupportedOperationException {
+        throw new UnsupportedOperationException("Cannot remove groups via read-only provider");
+    }
+
+	/**
+	 * @throws UnsupportedOperationException
+	 */
+    public final void setName(String oldName, String newName) throws UnsupportedOperationException {
+        throw new UnsupportedOperationException("Cannot modify read-only groups");
+    }
+
+	/**
+	 * @throws UnsupportedOperationException
+	 */
+    public final void setDescription(String name, String description) throws UnsupportedOperationException {
+        throw new UnsupportedOperationException("Cannot modify read-only groups");
+    }
+
+    // Search methods may be overridden by read-only group providers
+    
+    /**
+     * Returns true if the provider supports group search capability. This implementation
+     * always returns false.
+     */
+    public boolean isSearchSupported() {
+        return false;
+    }
+
+    /**
+     * Returns a collection of group search results. This implementation
+     * returns an empty collection.
+     */
+    public Collection<String> search(String query) {
+    	return Collections.emptyList();
+    }
+
+    /**
+     * Returns a collection of group search results. This implementation
+     * returns an empty collection.
+     */
+    public Collection<String> search(String query, int startIndex, int numResults) {
+    	return Collections.emptyList();
+    }
+
+    /**
+     * Returns a collection of group search results. This implementation
+     * returns an empty collection.
+     */
+	public Collection<String> search(String key, String value) {
+		return Collections.emptyList();
+	}
+
+	// Shared group methods may be overridden by read-only group providers
+
+    /**
+     * Returns true if the provider supports group sharing. This implementation
+     * always returns false.
+     */
+    public boolean isSharingSupported() {
+        return false;
+    }
+
+    /**
+     * Returns a collection of shared group names. This implementation
+     * returns an empty collection.
+     */
+    public Collection<String> getSharedGroupNames() {
+    	return Collections.emptyList();
+	}
+
+    /**
+     * Returns a collection of shared group names for the given user. This 
+     * implementation returns an empty collection.
+     */
+    public Collection<String> getSharedGroupNames(JID user) {
+    	return Collections.emptyList();
+	}
+
+    /**
+     * Returns a collection of shared public group names. This 
+     * implementation returns an empty collection.
+     */
+	public Collection<String> getPublicSharedGroupNames() {
+		return Collections.emptyList();
+	}
+
+    /**
+     * Returns a collection of groups shared with the given group. This 
+     * implementation returns an empty collection.
+     */
+	public Collection<String> getVisibleGroupNames(String userGroup) {
+		return Collections.emptyList();
+	}
+	
+    /**
+     * Returns a map of properties for the given group. This 
+     * implementation returns an empty immutable map.
+     */
+	public Map<String, String> loadProperties(Group group) {
+		return new Immutable.Map<String,String>();
+	}
+	
+}

src/java/org/jivesoftware/openfire/group/DefaultGroupProvider.java

@@ -27,7 +27,10 @@ import java.sql.SQLException;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
+import java.util.Set;
 
 import org.jivesoftware.database.DbConnectionManager;
 import org.jivesoftware.openfire.XMPPServer;
@@ -75,8 +78,25 @@ public class DefaultGroupProvider implements GroupProvider {
         "UPDATE ofGroupUser SET administrator=? WHERE groupName=? AND username=?";
     private static final String USER_GROUPS =
         "SELECT groupName FROM ofGroupUser WHERE username=?";
+    private static final String GROUPLIST_CONTAINERS =
+            "SELECT groupName from ofGroupProp " +
+            "where name='sharedRoster.groupList' " +
+            "AND propValue LIKE ?";
+    private static final String PUBLIC_GROUPS = 
+    		"SELECT groupName from ofGroupProp " +
+    		"WHERE name='sharedRoster.showInRoster' " +
+    		"AND propValue='everybody'";
+    private static final String GROUPS_FOR_PROP = 
+    		"SELECT groupName from ofGroupProp " +
+    		"WHERE name=? " +
+    		"AND propValue=?";
     private static final String ALL_GROUPS = "SELECT groupName FROM ofGroup ORDER BY groupName";
     private static final String SEARCH_GROUP_NAME = "SELECT groupName FROM ofGroup WHERE groupName LIKE ? ORDER BY groupName";
+    private static final String LOAD_SHARED_GROUPS =
+            "SELECT groupName FROM ofGroupProp WHERE name='sharedRoster.showInRoster' " +
+            "AND propValue IS NOT NULL AND propValue <> 'nobody'";
+    private static final String LOAD_PROPERTIES =
+            "SELECT name, propValue FROM ofGroupProp WHERE groupName=?";    	
 
     private XMPPServer server = XMPPServer.getInstance();
 
@@ -240,11 +260,113 @@ public class DefaultGroupProvider implements GroupProvider {
         return count;
     }
 
-    public Collection<String> getSharedGroupsNames() {
-        // Get the list of shared groups from the database
-        return Group.getSharedGroupsNames();
+    /**
+     * Returns the name of the groups that are shared groups.
+     *
+     * @return the name of the groups that are shared groups.
+     */
+    public Collection<String> getSharedGroupNames() {
+        Collection<String> groupNames = new HashSet<String>();
+        Connection con = null;
+        PreparedStatement pstmt = null;
+        ResultSet rs = null;
+        try {
+            con = DbConnectionManager.getConnection();
+            pstmt = con.prepareStatement(LOAD_SHARED_GROUPS);
+            rs = pstmt.executeQuery();
+            while (rs.next()) {
+                groupNames.add(rs.getString(1));
+            }
+        }
+        catch (SQLException sqle) {
+            Log.error(sqle.getMessage(), sqle);
+        }
+        finally {
+            DbConnectionManager.closeConnection(rs, pstmt, con);
+        }
+        return groupNames;
     }
 
+    public Collection<String> getSharedGroupNames(JID user) {
+    	Set<String> answer = new HashSet<String>();
+    	Collection<String> userGroups = getGroupNames(user);
+    	answer.addAll(userGroups);
+    	for (String userGroup : userGroups) {
+    		answer.addAll(getVisibleGroupNames(userGroup));
+    	}
+        answer.addAll(getPublicSharedGroupNames());
+        return answer;
+    }
+
+	public Collection<String> getVisibleGroupNames(String userGroup) {
+		Set<String> groupNames = new HashSet<String>();
+        Connection con = null;
+		PreparedStatement pstmt = null;
+		ResultSet rs = null;
+		try {
+		    con = DbConnectionManager.getConnection();
+		    pstmt = con.prepareStatement(GROUPLIST_CONTAINERS);
+		    pstmt.setString(1, "%" + userGroup + "%");
+		    rs = pstmt.executeQuery();
+		    while (rs.next()) {
+		        groupNames.add(rs.getString(1));
+		    }
+		}
+		catch (SQLException sqle) {
+		    Log.error(sqle.getMessage(), sqle);
+		}
+		finally {
+		    DbConnectionManager.closeConnection(rs, pstmt, con);
+		}
+		return groupNames;
+	}
+    
+	public Collection<String> search(String key, String value) {
+		Set<String> groupNames = new HashSet<String>();
+        Connection con = null;
+		PreparedStatement pstmt = null;
+		ResultSet rs = null;
+		try {
+		    con = DbConnectionManager.getConnection();
+		    pstmt = con.prepareStatement(GROUPS_FOR_PROP);
+		    pstmt.setString(1, key);
+		    pstmt.setString(2, value);
+		    rs = pstmt.executeQuery();
+		    while (rs.next()) {
+		        groupNames.add(rs.getString(1));
+		    }
+		}
+		catch (SQLException sqle) {
+		    Log.error(sqle.getMessage(), sqle);
+		}
+		finally {
+		    DbConnectionManager.closeConnection(rs, pstmt, con);
+		}
+		return groupNames;
+	}
+
+	public Collection<String> getPublicSharedGroupNames() {
+		Set<String> groupNames = new HashSet<String>();
+        Connection con = null;
+        PreparedStatement pstmt = null;
+        ResultSet rs = null;
+        try {
+            con = DbConnectionManager.getConnection();
+            pstmt = con.prepareStatement(PUBLIC_GROUPS);
+            rs = pstmt.executeQuery();
+            while (rs.next()) {
+                groupNames.add(rs.getString(1));
+            }
+        }
+        catch (SQLException sqle) {
+            Log.error(sqle.getMessage(), sqle);
+        }
+        finally {
+            DbConnectionManager.closeConnection(rs, pstmt, con);
+        }
+        return groupNames;
+	}
+
     public Collection<String> getGroupNames() {
         List<String> groupNames = new ArrayList<String>();
         Connection con = null;
@@ -432,6 +554,10 @@ public class DefaultGroupProvider implements GroupProvider {
         return true;
     }
 
+    public boolean isSharingSupported() {
+        return true;
+    }
+
     private Collection<JID> getMembers(String groupName, boolean adminsOnly) {
         List<JID> members = new ArrayList<JID>();
         Connection con = null;
@@ -470,4 +596,50 @@ public class DefaultGroupProvider implements GroupProvider {
         }
         return members;
     }
+    
+    /**
+     * Returns a custom {@link Map} that updates the database whenever
+     * a property value is added, changed, or deleted.
+     * 
+     * @param name The target group
+     * @return The properties for the given group
+     */
+    public Map<String,String> loadProperties(Group group) {
+    	// custom map implementation persists group property changes
+    	// whenever one of the standard mutator methods are called
+    	String name = group.getName();
+    	DefaultGroupPropertyMap<String,String> result = new DefaultGroupPropertyMap<String,String>(group);
+        Connection con = null;
+        PreparedStatement pstmt = null;
+        ResultSet rs = null;
+        try {
+            con = DbConnectionManager.getConnection();
+            pstmt = con.prepareStatement(LOAD_PROPERTIES);
+            pstmt.setString(1, name);
+            rs = pstmt.executeQuery();
+            while (rs.next()) {
+                String key = rs.getString(1);
+                String value = rs.getString(2);
+                if (key != null) {
+                    if (value == null) {
+                        result.remove(key);
+                        Log.warn("Deleted null property " + key + " for group: " + name);
+                    } else {
+                    	result.put(key, value, false); // skip persistence during load
+                    }
+                }
+                else { // should not happen, but ...
+                    Log.warn("Ignoring null property key for group: " + name);
+                }
+            }
+        }
+        catch (SQLException sqle) {
+            Log.error(sqle.getMessage(), sqle);
+        }
+        finally {
+            DbConnectionManager.closeConnection(rs, pstmt, con);
+        }
+        return result;
+    }
+	
 }
\ No newline at end of file

src/java/org/jivesoftware/openfire/group/GroupCollection.java

@@ -51,7 +51,7 @@ public class GroupCollection extends AbstractCollection {
 
     @Override
 	public Iterator iterator() {
-        return new UserIterator();
+        return new GroupIterator();
     }
 
     @Override
@@ -59,7 +59,7 @@ public class GroupCollection extends AbstractCollection {
         return elements.length;
     }
 
-    private class UserIterator implements Iterator {
+    private class GroupIterator implements Iterator {
 
         private int currentIndex = -1;
         private Object nextElement = null;

src/java/org/jivesoftware/openfire/group/GroupProvider.java

@@ -20,9 +20,10 @@
 
 package org.jivesoftware.openfire.group;
 
-import org.xmpp.packet.JID;
-
 import java.util.Collection;
+import java.util.Map;
+
+import org.xmpp.packet.JID;
 
 /**
  * Provider interface for groups. Users that wish to integrate with
@@ -37,6 +38,8 @@ import java.util.Collection;
  *     </group>
  *   &lt;/provider&gt;</pre>
  *
+ * @see AbstractReadOnlyGroupProvider
+ * 
  * @author Matt Tucker
  */
 public interface GroupProvider {
@@ -112,13 +115,46 @@ public interface GroupProvider {
      */
     Collection<String> getGroupNames();
 
+    /**
+     * Returns true if this GroupProvider allows group sharing. Shared groups
+     * enable roster sharing.
+     *
+     * @return true if the group provider supports group sharing.
+     */
+    boolean isSharingSupported();
+
     /**
      * Returns an unmodifiable Collection of all shared groups in the system.
      *
      * @return unmodifiable Collection of all shared groups in the system.
      */
-    Collection<String> getSharedGroupsNames();
-
+    Collection<String> getSharedGroupNames();
+    
+    /**
+     * Returns an unmodifiable Collection of all shared groups in the system for a given user.
+     * 
+     * @param JID The bare JID for the user (node@domain)
+     * @return unmodifiable Collection of all shared groups in the system for a given user.
+     */
+    Collection<String> getSharedGroupNames(JID user);
+    
+    /**
+     * Returns an unmodifiable Collection of all public shared groups in the system.
+     *
+     * @return unmodifiable Collection of all public shared groups in the system.
+     */
+    Collection<String> getPublicSharedGroupNames();
+    
+    /**
+     * Returns an unmodifiable Collection of groups that are visible
+     * to the members of the given group.
+     * 
+     * @param userGroup The given group
+     * @return unmodifiable Collection of group names that are visible
+     * to the given group.
+     */
+    Collection<String> getVisibleGroupNames(String userGroup);
+    
     /**
      * Returns the Collection of all groups in the system.
      *
@@ -209,10 +245,45 @@ public interface GroupProvider {
      */
     Collection<String> search(String query, int startIndex, int numResults);
 
+    /**
+     * Returns the names of groups that have a property matching the given
+     * key/value pair. This provides an simple extensible search mechanism
+     * for providers with differing property sets and storage models.
+     * 
+     * The semantics of the key/value matching (wildcard support, scoping, etc.) 
+     * are unspecified by the interface and may vary for each implementation.
+     * 
+     * Before searching or showing a search UI, use the {@link #isSearchSupported} method
+     * to ensure that searching is supported.
+     *
+     * @param key The name of a group property (e.g. "sharedRoster.showInRoster")
+     * @param value The value to match for the given property
+     * @return unmodifiable Collection of group names that match the
+     * 			given key/value pair.
+     */
+    Collection<String> search(String key, String value);
+    
     /**
      * Returns true if group searching is supported by the provider.
      *
      * @return true if searching is supported.
      */
     boolean isSearchSupported();
+    
+    /**
+     * Loads the group properties (if any) from the backend data store. If
+     * the properties can be changed, the provider implementation must ensure
+     * that updates to the resulting {@link Map} are persisted to the
+     * backend data store. Otherwise if a mutator method is called, the
+     * implementation should throw an {@link UnsupportedOperationException}.
+     * 
+     * If there are no corresponding properties for the given group, or if the
+     * provider does not support group properties, this method should return
+     * an empty Map rather than null.
+     * 
+     * @param group The target group
+     * @return The properties for the given group
+     */
+    Map<String,String> loadProperties(Group group);
+    
 }
\ No newline at end of file

src/java/org/jivesoftware/openfire/group/JDBCGroupProvider.java

@@ -27,10 +27,13 @@ import java.sql.SQLException;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 import org.jivesoftware.database.DbConnectionManager;
 import org.jivesoftware.openfire.XMPPServer;
+import org.jivesoftware.util.Immutable;
 import org.jivesoftware.util.JiveGlobals;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -53,7 +56,9 @@ import org.xmpp.packet.JID;
  * <li><tt>jdbcProvider.driver = com.mysql.jdbc.Driver</tt></li>
  * <li><tt>jdbcProvider.connectionString = jdbc:mysql://localhost/dbname?user=username&amp;password=secret</tt></li>
  * <li><tt>jdbcGroupProvider.groupCountSQL = SELECT count(*) FROM myGroups</tt></li>
+ * <li><tt>jdbcGroupProvider.groupPropsSQL = SELECT propName, propValue FROM myGroupProps WHERE groupName=?</tt></li>
  * <li><tt>jdbcGroupProvider.allGroupsSQL = SELECT groupName FROM myGroups</tt></li>
+ * <li><tt>jdbcGroupProvider.sharedGroupsSQL = SELECT groupName FROM myGroups WHERE shared='true'</tt></li>
  * <li><tt>jdbcGroupProvider.userGroupsSQL = SELECT groupName FORM myGroupUsers WHERE username=?</tt></li>
  * <li><tt>jdbcGroupProvider.descriptionSQL = SELECT groupDescription FROM myGroups WHERE groupName=?</tt></li>
  * <li><tt>jdbcGroupProvider.loadMembersSQL = SELECT username FORM myGroupUsers WHERE groupName=? AND isAdmin='N'</tt></li>
@@ -69,7 +74,7 @@ import org.xmpp.packet.JID;
  *
  * @author David Snopek
  */
-public class JDBCGroupProvider implements GroupProvider {
+public class JDBCGroupProvider extends AbstractReadOnlyGroupProvider {
 
 	private static final Logger Log = LoggerFactory.getLogger(JDBCGroupProvider.class);
 
@@ -77,7 +82,9 @@ public class JDBCGroupProvider implements GroupProvider {
 
     private String groupCountSQL;
     private String descriptionSQL;
+    private String groupPropsSQL;
     private String allGroupsSQL;
+    private String sharedGroupsSQL;
     private String userGroupsSQL;
     private String loadMembersSQL;
     private String loadAdminsSQL;
@@ -93,7 +100,9 @@ public class JDBCGroupProvider implements GroupProvider {
         JiveGlobals.migrateProperty("jdbcProvider.driver");
         JiveGlobals.migrateProperty("jdbcProvider.connectionString");
         JiveGlobals.migrateProperty("jdbcGroupProvider.groupCountSQL");
+        JiveGlobals.migrateProperty("jdbcGroupProvider.groupPropsSQL");
         JiveGlobals.migrateProperty("jdbcGroupProvider.allGroupsSQL");
+        JiveGlobals.migrateProperty("jdbcGroupProvider.sharedGroupsSQL");
         JiveGlobals.migrateProperty("jdbcGroupProvider.userGroupsSQL");
         JiveGlobals.migrateProperty("jdbcGroupProvider.descriptionSQL");
         JiveGlobals.migrateProperty("jdbcGroupProvider.loadMembersSQL");
@@ -116,33 +125,15 @@ public class JDBCGroupProvider implements GroupProvider {
 
         // Load SQL statements
         groupCountSQL = JiveGlobals.getProperty("jdbcGroupProvider.groupCountSQL");
+        groupPropsSQL = JiveGlobals.getProperty("jdbcGroupProvider.groupPropsSQL");
         allGroupsSQL = JiveGlobals.getProperty("jdbcGroupProvider.allGroupsSQL");
+        sharedGroupsSQL = JiveGlobals.getProperty("jdbcGroupProvider.sharedGroupsSQL");
         userGroupsSQL = JiveGlobals.getProperty("jdbcGroupProvider.userGroupsSQL");
         descriptionSQL = JiveGlobals.getProperty("jdbcGroupProvider.descriptionSQL");
         loadMembersSQL = JiveGlobals.getProperty("jdbcGroupProvider.loadMembersSQL");
         loadAdminsSQL = JiveGlobals.getProperty("jdbcGroupProvider.loadAdminsSQL");
     }
 
-    /**
-     * Always throws an UnsupportedOperationException because JDBC groups are read-only.
-     *
-     * @param name the name of the group to create.
-     * @throws UnsupportedOperationException when called.
-     */
-    public Group createGroup(String name) throws UnsupportedOperationException {
-        throw new UnsupportedOperationException();
-    }
-
-    /**
-     * Always throws an UnsupportedOperationException because JDBC groups are read-only.
-     *
-     * @param name the name of the group to delete
-     * @throws UnsupportedOperationException when called.
-     */
-    public void deleteGroup(String name) throws UnsupportedOperationException {
-        throw new UnsupportedOperationException();
-    }
-
     private Connection getConnection() throws SQLException {
         if (useConnectionProvider)
             return DbConnectionManager.getConnection();
@@ -220,29 +211,6 @@ public class JDBCGroupProvider implements GroupProvider {
         return members;
     }
 
-    /**
-     * Always throws an UnsupportedOperationException because JDBC groups are read-only.
-     *
-     * @param oldName the current name of the group.
-     * @param newName the desired new name of the group.
-     * @throws UnsupportedOperationException when called.
-     */
-    public void setName(String oldName, String newName) throws UnsupportedOperationException {
-        throw new UnsupportedOperationException();
-    }
-
-    /**
-     * Always throws an UnsupportedOperationException because JDBC groups are read-only.
-     *
-     * @param name the group name.
-     * @param description the group description.
-     * @throws UnsupportedOperationException when called.
-     */
-    public void setDescription(String name, String description)
-            throws UnsupportedOperationException {
-        throw new UnsupportedOperationException();
-    }
-
     public int getGroupCount() {
         int count = 0;
         Connection con = null;
@@ -265,9 +233,29 @@ public class JDBCGroupProvider implements GroupProvider {
         return count;
     }
 
-    public Collection<String> getSharedGroupsNames() {
-        // Get the list of shared groups from the database
-        return Group.getSharedGroupsNames();
+    public Collection<String> getSharedGroupNames() {
+    	if (sharedGroupsSQL == null) {
+    		return Collections.emptyList();
+    	}
+    	List<String> groupNames = new ArrayList<String>();
+        Connection con = null;
+        PreparedStatement pstmt = null;
+        ResultSet rs = null;
+        try {
+            con = getConnection();
+            pstmt = con.prepareStatement(sharedGroupsSQL);
+            rs = pstmt.executeQuery();
+            while (rs.next()) {
+                groupNames.add(rs.getString(1));
+            }
+        }
+        catch (SQLException e) {
+            Log.error(e.getMessage(), e);
+        }
+        finally {
+            DbConnectionManager.closeConnection(rs, pstmt, con);
+        }
+        return groupNames;
     }
 
     public Collection<String> getGroupNames() {
@@ -340,65 +328,28 @@ public class JDBCGroupProvider implements GroupProvider {
         return groupNames;
     }
 
-    /**
-     * Always throws an UnsupportedOperationException because JDBC groups are read-only.
-     *
-     * @param groupName name of a group.
-     * @param user the JID of the user to add
-     * @param administrator true if is an administrator.
-     * @throws UnsupportedOperationException when called.
-     */
-    public void addMember(String groupName, JID user, boolean administrator)
-            throws UnsupportedOperationException
-    {
-        throw new UnsupportedOperationException();
-    }
-
-    /**
-     * Always throws an UnsupportedOperationException because JDBC groups are read-only.
-     *
-     * @param groupName the naame of a group.
-     * @param user the JID of the user with new privileges
-     * @param administrator true if is an administrator.
-     * @throws UnsupportedOperationException when called.
-     */
-    public void updateMember(String groupName, JID user, boolean administrator)
-            throws UnsupportedOperationException
-    {
-        throw new UnsupportedOperationException();
-    }
-
-    /**
-     * Always throws an UnsupportedOperationException because JDBC groups are read-only.
-     *
-     * @param groupName the name of a group.
-     * @param user the JID of the user to delete.
-     * @throws UnsupportedOperationException when called.
-     */
-    public void deleteMember(String groupName, JID user)
-            throws UnsupportedOperationException
-    {
-        throw new UnsupportedOperationException();
-    }
-
-    /**
-     * Always returns true because JDBC groups are read-only.
-     *
-     * @return true because all JDBC functions are read-only.
-     */
-    public boolean isReadOnly() {
-        return true;
-    }
-
-    public Collection<String> search(String query) {
-        return Collections.emptyList();
-    }
-
-    public Collection<String> search(String query, int startIndex, int numResults) {
-        return Collections.emptyList();
-    }
-
-    public boolean isSearchSupported() {
-        return false;
-    }
+	public Map<String, String> loadProperties(Group group) {
+		Map<String,String> properties = new HashMap<String,String>();
+    	if (groupPropsSQL != null) {
+	        Connection con = null;
+	        PreparedStatement pstmt = null;
+	        ResultSet rs = null;
+	        try {
+	            con = DbConnectionManager.getConnection();
+	            pstmt = con.prepareStatement(groupPropsSQL);
+	            pstmt.setString(1, group.getName());
+	            rs = pstmt.executeQuery();
+	            while (rs.next()) {
+	                properties.put(rs.getString(1), rs.getString(2));
+	            }
+	        }
+	        catch (SQLException sqle) {
+	            Log.error(sqle.getMessage(), sqle);
+	        }
+	        finally {
+	            DbConnectionManager.closeConnection(rs, pstmt, con);
+	        }
+    	}
+        return new Immutable.Map<String,String>(properties);
+	}
 }

src/java/org/jivesoftware/openfire/ldap/LdapGroupProvider.java

@@ -38,9 +38,9 @@ import javax.naming.ldap.LdapContext;
 import javax.naming.ldap.LdapName;
 
 import org.jivesoftware.openfire.XMPPServer;
+import org.jivesoftware.openfire.group.AbstractReadOnlyGroupProvider;
 import org.jivesoftware.openfire.group.Group;
 import org.jivesoftware.openfire.group.GroupNotFoundException;
-import org.jivesoftware.openfire.group.GroupProvider;
 import org.jivesoftware.openfire.user.UserManager;
 import org.jivesoftware.openfire.user.UserNotFoundException;
 import org.jivesoftware.util.JiveConstants;
@@ -54,7 +54,7 @@ import org.xmpp.packet.JID;
  *
  * @author Matt Tucker, Greg Ferguson and Cameron Moore
  */
-public class LdapGroupProvider implements GroupProvider {
+public class LdapGroupProvider extends AbstractReadOnlyGroupProvider {
 
 	private static final Logger Log = LoggerFactory.getLogger(LdapGroupProvider.class);
 
@@ -76,26 +76,6 @@ public class LdapGroupProvider implements GroupProvider {
         standardAttributes[2] = manager.getGroupMemberField();
     }
 
-    /**
-     * Always throws an UnsupportedOperationException because LDAP groups are read-only.
-     *
-     * @param name the name of the group to create.
-     * @throws UnsupportedOperationException when called.
-     */
-    public Group createGroup(String name) throws UnsupportedOperationException {
-        throw new UnsupportedOperationException();
-    }
-
-    /**
-     * Always throws an UnsupportedOperationException because LDAP groups are read-only.
-     *
-     * @param name the name of the group to delete
-     * @throws UnsupportedOperationException when called.
-     */
-    public void deleteGroup(String name) throws UnsupportedOperationException {
-        throw new UnsupportedOperationException();
-    }
-
     public Group getGroup(String groupName) throws GroupNotFoundException {
         LdapContext ctx = null;
         try {
@@ -123,30 +103,6 @@ public class LdapGroupProvider implements GroupProvider {
         }
     }
 
-    /**
-     * Always throws an UnsupportedOperationException because LDAP groups are read-only.
-     *
-     * @param oldName the current name of the group.
-     * @param newName the desired new name of the group.
-     * @throws UnsupportedOperationException when called.
-     */
-    public void setName(String oldName, String newName) throws UnsupportedOperationException {
-        throw new UnsupportedOperationException();
-    }
-
-    /**
-     * Always throws an UnsupportedOperationException because LDAP groups are read-only.
-     *
-     * @param name the group name.
-     * @param description the group description.
-     * @throws UnsupportedOperationException when called.
-     */
-    public void setDescription(String name, String description)
-            throws UnsupportedOperationException
-    {
-        throw new UnsupportedOperationException();
-    }
-
     public int getGroupCount() {
         if (manager.isDebugEnabled()) {
             Log.debug("LdapGroupProvider: Trying to get the number of groups in the system.");
@@ -163,11 +119,6 @@ public class LdapGroupProvider implements GroupProvider {
         return this.groupCount;
     }
 
-    public Collection<String> getSharedGroupsNames() {
-        // Get the list of shared groups from the database
-        return Group.getSharedGroupsNames();
-    }
-
     public Collection<String> getGroupNames() {
         return getGroupNames(-1, -1);
     }
@@ -207,13 +158,17 @@ public class LdapGroupProvider implements GroupProvider {
         if (username == null || "".equals(username)) {
             return Collections.emptyList();
         }
+    	return search(manager.getGroupMemberField(), username);
+    }
+
+    public Collection<String> search(String key, String value) {
         StringBuilder filter = new StringBuilder();
         filter.append("(&");
         filter.append(MessageFormat.format(manager.getGroupSearchFilter(), "*"));
-        filter.append("(").append(manager.getGroupMemberField()).append("=").append(username);
+        filter.append("(").append(key).append("=").append(value);
         filter.append("))");
         if (Log.isDebugEnabled()) {
-            Log.debug("Trying to find group names for user: " + user + " using query: " + filter.toString());
+            Log.debug("Trying to find group names using query: " + filter.toString());
         }
         // Perform the LDAP query
         return manager.retrieveList(
@@ -225,53 +180,6 @@ public class LdapGroupProvider implements GroupProvider {
         );
     }
 
-    /**
-     * Always throws an UnsupportedOperationException because LDAP groups are read-only.
-     *
-     * @param groupName name of a group.
-     * @param user the JID of the user to add
-     * @param administrator true if is an administrator.
-     * @throws UnsupportedOperationException when called.
-     */
-    public void addMember(String groupName, JID user, boolean administrator)
-            throws UnsupportedOperationException
-    {
-        throw new UnsupportedOperationException();
-    }
-
-    /**
-     * Always throws an UnsupportedOperationException because LDAP groups are read-only.
-     *
-     * @param groupName the naame of a group.
-     * @param user the JID of the user with new privileges
-     * @param administrator true if is an administrator.
-     * @throws UnsupportedOperationException when called.
-     */
-    public void updateMember(String groupName, JID user, boolean administrator)
-            throws UnsupportedOperationException {
-        throw new UnsupportedOperationException();
-    }
-
-    /**
-     * Always throws an UnsupportedOperationException because LDAP groups are read-only.
-     *
-     * @param groupName the name of a group.
-     * @param user the JID of the user to delete.
-     * @throws UnsupportedOperationException when called.
-     */
-    public void deleteMember(String groupName, JID user) throws UnsupportedOperationException {
-        throw new UnsupportedOperationException();
-    }
-
-    /**
-     * Returns true because LDAP groups are read-only.
-     *
-     * @return true because all LDAP functions are read-only.
-     */
-    public boolean isReadOnly() {
-        return true;
-    }
-
     public Collection<String> search(String query) {
         return search(query, -1, -1);
     }

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

@@ -326,13 +326,18 @@ public class Roster implements Cacheable, Externalizable {
             throws UserAlreadyExistsException, SharedGroupException {
         if (groups != null && !groups.isEmpty()) {
             // Raise an error if the groups the item belongs to include a shared group
-            Collection<Group> sharedGroups = GroupManager.getInstance().getSharedGroups();
-            for (String group : groups) {
-                for (Group sharedGroup : sharedGroups) {
-                    if (group.equals(sharedGroup.getProperties().get("sharedRoster.displayName"))) {
-                        throw new SharedGroupException("Cannot add an item to a shared group");
-                    }
-                }
+            for (String groupDisplayName : groups) {
+				Collection<Group> groupsWithProp = GroupManager
+						.getInstance()
+						.search("sharedRoster.displayName", groupDisplayName);
+				Iterator<Group> itr = groupsWithProp.iterator();
+            	while(itr.hasNext()) {
+            		Group group = itr.next();
+            		String showInRoster = group.getProperties().get("sharedRoster.showInRoster");
+            		if(showInRoster != null && !showInRoster.equals("nobody")) {
+            			throw new SharedGroupException("Cannot add an item to a shared group");
+            		}
+            	}
             }
         }
         org.xmpp.packet.Roster roster = new org.xmpp.packet.Roster();

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

@@ -359,42 +359,25 @@ public class RosterItem implements Cacheable, Externalizable {
             }
 
             // Remove shared groups from the param
-            Collection<Group> existingGroups = GroupManager.getInstance().getSharedGroups();
             for (Iterator<String> it=groups.iterator(); it.hasNext();) {
                 String groupName = it.next();
                 try {
-                    // Optimistic approach for performance reasons. Assume first that the shared
-                    // group name is the same as the display name for the shared roster
-
-                    // Check if exists a shared group with this name
                     Group group = GroupManager.getInstance().getGroup(groupName);
-                    // Get the display name of the group
-                    String displayName = group.getProperties().get("sharedRoster.displayName");
-                    if (displayName != null && displayName.equals(groupName)) {
-                        // Remove the shared group from the list (since it exists)
-                        try {
-                            it.remove();
-                        }
-                        catch (IllegalStateException e) {
-                            // Do nothing
-                        }
-                    }
-                }
-                catch (GroupNotFoundException e) {
+                	if (RosterManager.isSharedGroup(group)) {
+                		it.remove();
+                	}
+                } catch (GroupNotFoundException e) {
                     // Check now if there is a group whose display name matches the requested group
-                    for (Group group : existingGroups) {
-                        // Get the display name of the group
-                        String displayName = group.getProperties().get("sharedRoster.displayName");
-                        if (displayName != null && displayName.equals(groupName)) {
-                            // Remove the shared group from the list (since it exists)
-                            try {
-                                it.remove();
-                            }
-                            catch (IllegalStateException ise) {
-                                // Do nothing
-                            }
-                        }
-                    }
+                	Collection<Group> groupsWithProp = GroupManager
+    						.getInstance()
+    						.search("sharedRoster.displayName", groupName);
+                	Iterator<Group> itr = groupsWithProp.iterator();
+                	while(itr.hasNext()) {
+                		Group group = itr.next();
+                    	if (RosterManager.isSharedGroup(group)) {
+                    		it.remove();
+                    	}
+                	}
                 }
             }
             this.groups = groups;

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

@@ -187,7 +187,7 @@ public class RosterManager extends BasicModule implements GroupEventListener, Us
      */
     public Collection<Group> getSharedGroups(String username) {
         Collection<Group> answer = new HashSet<Group>();
-        Collection<Group> groups = GroupManager.getInstance().getSharedGroups();
+        Collection<Group> groups = GroupManager.getInstance().getSharedGroups(username);
         for (Group group : groups) {
             String showInRoster = group.getProperties().get("sharedRoster.showInRoster");
             if ("onlyGroup".equals(showInRoster)) {
@@ -219,16 +219,7 @@ public class RosterManager extends BasicModule implements GroupEventListener, Us
      * @return the list of shared groups whose visibility is public.
      */
     public Collection<Group> getPublicSharedGroups() {
-        Collection<Group> answer = new HashSet<Group>();
-        Collection<Group> groups = GroupManager.getInstance().getSharedGroups();
-        for (Group group : groups) {
-            String showInRoster = group.getProperties().get("sharedRoster.showInRoster");
-            if ("everybody".equals(showInRoster)) {
-                // Anyone can see this group so add the group to the answer
-                answer.add(group);
-            }
-        }
-        return answer;
+    	return GroupManager.getInstance().getPublicSharedGroups();
     }
 
     /**
@@ -753,26 +744,7 @@ public class RosterManager extends BasicModule implements GroupEventListener, Us
     }
 
     private Collection<Group> getVisibleGroups(Group groupToCheck) {
-        Collection<Group> answer = new HashSet<Group>();
-        Collection<Group> groups = GroupManager.getInstance().getSharedGroups();
-        for (Group group : groups) {
-            if (group.equals(groupToCheck)) {
-                continue;
-            }
-            String showInRoster = group.getProperties().get("sharedRoster.showInRoster");
-            if ("onlyGroup".equals(showInRoster)) {
-                // Check if the user belongs to a group that may see this group
-                Collection<String> groupList =
-                        parseGroupNames(group.getProperties().get("sharedRoster.groupList"));
-                if (groupList.contains(groupToCheck.getName())) {
-                    answer.add(group);
-                }
-            }
-            else if ("everybody".equals(showInRoster)) {
-                answer.add(group);
-            }
-        }
-        return answer;
+    	return GroupManager.getInstance().getVisibleGroups(groupToCheck);
     }
 
     /**

src/java/org/jivesoftware/util/Immutable.java

+package org.jivesoftware.util;
+
+import java.util.AbstractMap;
+import java.util.AbstractSet;
+import java.util.Collections;
+import java.util.Set;
+
+public class Immutable {
+	
+	/**
+	 * Wraps an existing {@link Collection} to provide read-only access to its contents.
+	 */
+	public static class Collection<V> extends java.util.AbstractCollection<V> {
+
+		private java.util.Collection<V> delegate;
+		
+		public Collection(java.util.Collection<V> delegate) {
+			this.delegate = delegate;
+		}
+
+		@Override
+		public Iterator<V> iterator() {
+			return new Iterator<V>(delegate.iterator());
+		}
+
+		@Override
+		public int size() {
+			return delegate.size();
+		}
+	}
+
+	/**
+	 * Read-only {@link Iterator} prevents removal of objects
+	 */
+	public static class Iterator<V> implements java.util.Iterator<V> {
+
+		private java.util.Iterator<V> delegate;
+		
+		public Iterator(java.util.Iterator<V> delegate) {
+			this.delegate = delegate;
+		}
+		public boolean hasNext() {
+			return delegate.hasNext();
+		}
+
+		public V next() {
+			return delegate.next();
+		}
+
+		public void remove() {
+			throw new UnsupportedOperationException();
+		}
+	}
+	
+	/**
+	 * Wraps a {@link Map} to provide read-only access to its elements.
+	 */
+	public static class Map<K,V> extends AbstractMap<K,V> {
+
+		private java.util.Map<K,V> backingMap;
+		
+		/**
+		 * Use this constructor to provide a pre-populated map that will be
+		 * made read-only via this wrapper class
+		 * @param backingMap
+		 */
+		public Map(java.util.Map<K,V> backingMap) {
+			this.backingMap = backingMap;
+		}
+		/**
+		 * Default constructor (empty map)
+		 */
+		public Map() { }
+
+		@Override
+		public Set<Map.Entry<K,V>> entrySet() {
+			if (backingMap == null) {
+				return Collections.emptySet();
+			} else {
+				return new AbstractSet<Map.Entry<K,V>>() {
+					public Iterator<Map.Entry<K,V>> iterator() {
+						return new Iterator<Entry<K, V>>(backingMap.entrySet().iterator());
+					}
+					public int size() {
+						return backingMap.size();
+					}
+				};
+			}
+		}
+	}
+	
+}