OF-2710: Replace two group-member queries with one in DefaultGroupPro…#3382
OF-2710: Replace two group-member queries with one in DefaultGroupPro…#3382MilanTyagi2004 wants to merge 1 commit into
Conversation
…vider Optimize DefaultGroupProvider by replacing two separate SQL lookups (one for standard members and one for administrators) with a single combined query. - Remove LOAD_ADMINS and LOAD_MEMBERS SQL constants. - Add LOAD_MEMBERS_AND_ADMINS SQL constant. - Replace getMembers helper method with loadMembersAndAdmins to retrieve all users for a group and categorize them in memory by administrator status. - Update createGroup and getGroup call sites to use the new loadMembersAndAdmins helper.
📝 WalkthroughWalkthroughThe PR refactors group member and administrator loading in 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xmppserver/src/main/java/org/jivesoftware/openfire/group/DefaultGroupProvider.java (1)
521-527:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix component check to use the persisted username, not an uninitialized JID.
On Line 523,
server.matchesComponent(userJID)is evaluated beforeuserJIDis assigned. This makes the component branch ineffective and can misclassify component entries (without@) as local users.Suggested fix
JID userJID = null; if (user.indexOf('@') == -1) { - // Create JID of local user if JID does not match a component's JID - if (!server.matchesComponent(userJID)) { + // Create JID of local user if value does not match a component JID + if (!server.matchesComponent(user)) { userJID = server.createJID(user, null); } else { - // FIXME else... what? OF-2709 Note that the 'if' condition is _always_ true (should it use 'user' instead of 'userJID'?) + userJID = new JID(user); } } else { userJID = new JID(user); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xmppserver/src/main/java/org/jivesoftware/openfire/group/DefaultGroupProvider.java` around lines 521 - 527, In DefaultGroupProvider, the component check uses userJID before it is assigned causing incorrect classification; change the matchesComponent call to use the persisted username variable (user) instead of userJID and only create/assign userJID via server.createJID(user, null) when matchesComponent(user) returns false (i.e., keep the branch logic but evaluate server.matchesComponent(user) and then set userJID accordingly), ensuring the else branch handles the component case correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@xmppserver/src/main/java/org/jivesoftware/openfire/group/DefaultGroupProvider.java`:
- Around line 521-527: In DefaultGroupProvider, the component check uses userJID
before it is assigned causing incorrect classification; change the
matchesComponent call to use the persisted username variable (user) instead of
userJID and only create/assign userJID via server.createJID(user, null) when
matchesComponent(user) returns false (i.e., keep the branch logic but evaluate
server.matchesComponent(user) and then set userJID accordingly), ensuring the
else branch handles the component case correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f2b3c391-3f22-4e51-89b6-ab8ed9074495
📒 Files selected for processing (1)
xmppserver/src/main/java/org/jivesoftware/openfire/group/DefaultGroupProvider.java
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xmppserver/src/main/java/org/jivesoftware/openfire/group/DefaultGroupProvider.java (1)
84-94:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail the create path when
INSERT_GROUPfails.Line 84 logs the
SQLException, but Lines 90-94 still build and return aGroup. That makescreateGroupreport success for a group that was never written toofGroup;xmppserver/src/main/java/org/jivesoftware/openfire/commands/admin/group/AddGroup.java:104-166immediately mutates the returnedGroup, so the caller keeps operating on non-existent state. Propagate the failure here instead of falling through.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xmppserver/src/main/java/org/jivesoftware/openfire/group/DefaultGroupProvider.java` around lines 84 - 94, The create path currently swallows SQLException and continues to call loadMembersAndAdmins and return a new Group; update DefaultGroupProvider.createGroup so that failures to run the INSERT_GROUP do not fall through: check the result of the PreparedStatement.executeUpdate (or set a boolean/flag) and if it doesn't indicate a successful insert, or if an SQLException is caught, rethrow an exception (e.g., SQLException or IllegalStateException with the original exception as the cause) instead of continuing; ensure this change is applied around the INSERT_GROUP execution and catch block so loadMembersAndAdmins and the Group(...) return only happen on a successful insert.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@xmppserver/src/main/java/org/jivesoftware/openfire/group/DefaultGroupProvider.java`:
- Around line 484-490: The call to server.matchesComponent currently passes the
String user; change it to pass a JID by constructing a JID from the user String
for the component check inside DefaultGroupProvider.loadMembersAndAdmins:
parse/construct a JID (new JID(user) or equivalent) and use that JID when
calling XMPPServer.matchesComponent(jid), while keeping server.createJID(user,
null) for the normal local-username path and assigning userJID appropriately
(userJID = server.createJID(user, null) for local usernames, otherwise new
JID(user) for full JIDs/components).
---
Outside diff comments:
In
`@xmppserver/src/main/java/org/jivesoftware/openfire/group/DefaultGroupProvider.java`:
- Around line 84-94: The create path currently swallows SQLException and
continues to call loadMembersAndAdmins and return a new Group; update
DefaultGroupProvider.createGroup so that failures to run the INSERT_GROUP do not
fall through: check the result of the PreparedStatement.executeUpdate (or set a
boolean/flag) and if it doesn't indicate a successful insert, or if an
SQLException is caught, rethrow an exception (e.g., SQLException or
IllegalStateException with the original exception as the cause) instead of
continuing; ensure this change is applied around the INSERT_GROUP execution and
catch block so loadMembersAndAdmins and the Group(...) return only happen on a
successful insert.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 33dd30de-d40f-46ee-b2d9-a54c16588455
📒 Files selected for processing (1)
xmppserver/src/main/java/org/jivesoftware/openfire/group/DefaultGroupProvider.java
| if (user.indexOf('@') == -1) { | ||
| // Create JID of local user if JID does not match a component's JID | ||
| if (!server.matchesComponent(userJID)) { | ||
| if (!server.matchesComponent(user)) { | ||
| userJID = server.createJID(user, null); | ||
| } else { | ||
| // FIXME else... what? OF-2709 Note that the 'if' condition is _always_ true (should it use 'user' instead of 'userJID'?) | ||
| userJID = new JID(user); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspecting XMPPServer.matchesComponent signature(s):"
fd -p 'XMPPServer.java' | xargs -r rg -n -C2 'matchesComponent\s*\('
echo
echo "Inspecting the failing call site:"
sed -n '482,490p' xmppserver/src/main/java/org/jivesoftware/openfire/group/DefaultGroupProvider.javaRepository: igniterealtime/Openfire
Length of output: 621
Fix matchesComponent argument type in DefaultGroupProvider
XMPPServer.matchesComponent takes a JID, but DefaultGroupProvider.loadMembersAndAdmins(...) passes user (String) to server.matchesComponent(...), producing String cannot be converted to JID. Derive/parse a JID for the component check, and keep server.createJID(user, null) for the ordinary local-username path.
🧰 Tools
🪛 GitHub Actions: CodeQL / 0_Analyze (java).txt
[error] 486-486: Maven compilation failed (maven-compiler-plugin:3.15.0:compile). incompatible types: java.lang.String cannot be converted to org.xmpp.packet.JID
🪛 GitHub Actions: CodeQL / Analyze (java)
[error] 486-486: Maven compiler plugin (maven-compiler-plugin) compilation error: incompatible types: java.lang.String cannot be converted to org.xmpp.packet.JID.
🪛 GitHub Actions: Openfire CI / 28_Build Openfire from source (ubuntu-latest, 25, zulu).txt
[error] 486-486: COMPILATION ERROR: incompatible types: java.lang.String cannot be converted to org.xmpp.packet.JID (maven-compiler-plugin compile on project xmppserver).
🪛 GitHub Actions: Openfire CI / 31_Build Openfire from source (ubuntu-latest, 21, zulu).txt
[error] 486-486: Maven Compiler (maven-compiler-plugin) compilation failed: incompatible types. java.lang.String cannot be converted to org.xmpp.packet.JID at DefaultGroupProvider.java:[486,50].
🪛 GitHub Actions: Openfire CI / 32_Build Openfire from source (ubuntu-latest, 17, zulu).txt
[error] 486-486: Maven compiler failed: incompatible types: java.lang.String cannot be converted to org.xmpp.packet.JID. Error occurred in maven-compiler-plugin:3.15.0:compile (default-compile) for project xmppserver.
🪛 GitHub Actions: Openfire CI / 33_Build Openfire from source (macos-latest, 17, zulu).txt
[error] 486-486: Maven compiler plugin failed: incompatible types: java.lang.String cannot be converted to org.xmpp.packet.JID.
🪛 GitHub Actions: Openfire CI / 34_Build Openfire from source (macos-latest, 21, zulu).txt
[error] 486-486: Maven compiler error: incompatible types at DefaultGroupProvider.java:[486,50]. java.lang.String cannot be converted to org.xmpp.packet.JID.
🪛 GitHub Actions: Openfire CI / Build Openfire from source (macos-latest, 17, zulu)
[error] 486-486: Maven Compiler Plugin failed: incompatible types at line 486, column 50. Error: java.lang.String cannot be converted to org.xmpp.packet.JID.
🪛 GitHub Actions: Openfire CI / Build Openfire from source (macos-latest, 21, zulu)
[error] 486-486: maven-compiler-plugin compilation error: incompatible types: java.lang.String cannot be converted to org.xmpp.packet.JID
🪛 GitHub Actions: Openfire CI / Build Openfire from source (ubuntu-latest, 17, zulu)
[error] 486-486: maven-compiler-plugin compilation failed: incompatible types: java.lang.String cannot be converted to org.xmpp.packet.JID
🪛 GitHub Actions: Openfire CI / Build Openfire from source (ubuntu-latest, 21, zulu)
[error] 486-486: Maven compiler failed (maven-compiler-plugin:3.15.0:compile). Compilation error: incompatible types: java.lang.String cannot be converted to org.xmpp.packet.JID.
🪛 GitHub Actions: Openfire CI / Build Openfire from source (ubuntu-latest, 25, zulu)
[error] 486-486: Maven compiler failed in org.apache.maven.plugins:maven-compiler-plugin:3.15.0:compile (default-compile) with error: incompatible types: java.lang.String cannot be converted to org.xmpp.packet.JID.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@xmppserver/src/main/java/org/jivesoftware/openfire/group/DefaultGroupProvider.java`
around lines 484 - 490, The call to server.matchesComponent currently passes the
String user; change it to pass a JID by constructing a JID from the user String
for the component check inside DefaultGroupProvider.loadMembersAndAdmins:
parse/construct a JID (new JID(user) or equivalent) and use that JID when
calling XMPPServer.matchesComponent(jid), while keeping server.createJID(user,
null) for the normal local-username path and assigning userJID appropriately
(userJID = server.createJID(user, null) for local usernames, otherwise new
JID(user) for full JIDs/components).
| List<JID> members = new ArrayList<>(); | ||
| List<JID> administrators = new ArrayList<>(); | ||
| loadMembersAndAdmins(name, members, administrators); | ||
|
|
||
| return new Group(name, "", members, administrators); |
There was a problem hiding this comment.
Under what circumstances would a group already have members?
| if (userJID != null) { | ||
| if (isAdmin) { | ||
| administrators.add(userJID); | ||
| } else { | ||
| members.add(userJID); | ||
| } | ||
| } |
There was a problem hiding this comment.
Should these populate temporary lists until the db iteration is complete and error free, then mutate the original?
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
Yes. the main idea was to populate the list until db iteration is complete then mutate the original.
|
@Fishbowler This PR is under resolving state i am working on it. |
|
you can review my other PRs thanks for the point issue. |
2 participants
Description
DefaultGroupProvidercurrently performs two database queries when loading a group's users:As both user types are stored in the same table (
ofGroupUser) and differ only by the value of theadministratorcolumn, the data can be retrieved using a single query and categorized in memory.This PR optimizes group loading by replacing the two queries with a single query that retrieves both members and administrators.
Changes
LOAD_MEMBERS_AND_ADMINS, that retrieves bothusernameandadministratorvalues fromofGroupUser.LOAD_MEMBERSandLOAD_ADMINSquery constants.getMembers(String, boolean)helper method withloadMembersAndAdmins(String, Collection<JID>, Collection<JID>).createGroupandgetGroupto use the new helper method.Benefits
Testing
Compilation