Skip to content

OF-2710: Replace two group-member queries with one in DefaultGroupPro… #3382

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MilanTyagi2004 wants to merge 1 commit into
base: main
Choose a base branch
from
+19 −20
Open

OF-2710: Replace two group-member queries with one in DefaultGroupPro… #3382

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 19 additions & 20 deletions xmppserver/src/main/java/org/jivesoftware/openfire/group/DefaultGroupProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,8 @@ public class DefaultGroupProvider extends AbstractGroupProvider {
private static final String DELETE_GROUP =
"DELETE FROM ofGroup WHERE groupName=?";
private static final String GROUP_COUNT = "SELECT count(*) FROM ofGroup";
private static final String LOAD_ADMINS =
"SELECT username FROM ofGroupUser WHERE administrator=1 AND groupName=? ORDER BY username";
private static final String LOAD_MEMBERS =
"SELECT username FROM ofGroupUser WHERE administrator=0 AND groupName=? ORDER BY username";
private static final String LOAD_MEMBERS_AND_ADMINS =
"SELECT username, administrator FROM ofGroupUser WHERE groupName=? ORDER BY username";
private static final String LOAD_GROUP =
"SELECT description FROM ofGroup WHERE groupName=?";
private static final String REMOVE_USER =
Expand Down Expand Up @@ -105,9 +103,9 @@ public Group createGroup(String name) throws GroupNameInvalidException, GroupAlr
DbConnectionManager.closeConnection(pstmt, con);
}

// TODO Replace these two database queries with one. OF-2710
Collection<JID> members = getMembers(name, false);
Collection<JID> administrators = getMembers(name, true);
List<JID> members = new ArrayList<>();
List<JID> administrators = new ArrayList<>();
loadMembersAndAdmins(name, members, administrators);

return new Group(name, "", members, administrators);
Comment on lines +106 to 110

@Fishbowler Fishbowler Jun 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under what circumstances would a group already have members?

}
Expand Down Expand Up @@ -137,9 +135,9 @@ public Group getGroup(String name) throws GroupNotFoundException {
DbConnectionManager.closeConnection(rs, pstmt, con);
}

// TODO Replace these two database queries with one. OF-2710
Collection<JID> members = getMembers(name, false);
Collection<JID> administrators = getMembers(name, true);
List<JID> members = new ArrayList<>();
List<JID> administrators = new ArrayList<>();
loadMembersAndAdmins(name, members, administrators);

return new Group(name, description, members, administrators);
}
Expand Down Expand Up @@ -507,23 +505,18 @@ public boolean isSharingSupported() {
return true;
}

private Collection<JID> getMembers(String groupName, boolean adminsOnly) {
List<JID> members = new ArrayList<>();
private void loadMembersAndAdmins(String groupName, Collection<JID> members, Collection<JID> administrators) {
Connection con = null;
PreparedStatement pstmt = null;
ResultSet rs = null;
try {
con = DbConnectionManager.getConnection();
if (adminsOnly) {
pstmt = con.prepareStatement(LOAD_ADMINS);
}
else {
pstmt = con.prepareStatement(LOAD_MEMBERS);
}
pstmt = con.prepareStatement(LOAD_MEMBERS_AND_ADMINS);
pstmt.setString(1, groupName);
rs = pstmt.executeQuery();
while (rs.next()) {
String user = rs.getString(1);
boolean isAdmin = rs.getInt(2) == 1;
JID userJID = null;
if (user.indexOf('@') == -1) {
// Create JID of local user if JID does not match a component's JID
Expand All @@ -541,7 +534,14 @@ private Collection<JID> getMembers(String groupName, boolean adminsOnly) {
Log.warn("Group '{}' has a member that is persisted using its full JID, instead of a bare JID: '{}'. This is unexpected, and possibly a data consistency error. Groups should only contain bare JIDs.", groupName, userJID);
userJID = userJID.asBareJID();
}
members.add(userJID);

if (userJID != null) {
if (isAdmin) {
administrators.add(userJID);
} else {
members.add(userJID);
}
}
Comment on lines +538 to +544

@Fishbowler Fishbowler Jun 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these populate temporary lists until the db iteration is complete and error free, then mutate the original?

@Fishbowler Fishbowler Jun 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g.

List<JID> tmpMembers = new ArrayList<>();
List<JID> tmpAdmins = new ArrayList<>();
// fill tmp lists from ResultSet...
// on success:
members.addAll(tmpMembers);
administrators.addAll(tmpAdmins);

@MilanTyagi2004 MilanTyagi2004 Jun 25, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. the main idea was to populate the list until db iteration is complete then mutate the original.

}
}
catch (SQLException e) {
Expand All @@ -550,6 +550,5 @@ private Collection<JID> getMembers(String groupName, boolean adminsOnly) {
finally {
DbConnectionManager.closeConnection(rs, pstmt, con);
}
return members;
}
}
Loading