Openfire’s functionality that provides user grouping is pretty complex. Apart from being pluggable, it provides support for roles ('member' and ‘admin’), properties (which is used for roster sharing functionality, amongst others) and event listeners.
At least one issue exists in the current implementation: when a group instance is removed, but later gets a new member added to it, an incorrect representation of the group is added back to a cache, which makes it ‘available’ to a certain extend.
This snippet illustrates that issue:
final GroupManager groupManager = GroupManager.getInstance();
final Group testGroup = groupManager.createGroup("testGroup2");
groupManager.deleteGroup(testGroup);
testGroup.getMembers().add(new JID("john@example.org"));
groupManager.createGroup("testGroup2");
The interaction on line 4 is invalid. Invoking it does cause an error to be logged, but also causes state internal to the GroupManager (the cache) to become invalid. The last line, in the existing implementation, unexpectedly throws an GroupAlreadyExistsException. This should not occur, as the group is deleted in line 3.
Apart from this bug needing a fix, the code should be improved:
responsibility of updating caches and dispatching events to event listeners should be centralized, instead of spread over several classes.
the implementation should not depend on the event listening API for which it also dispatches events, as this mixes use-cases of the event listener mechanism in a complex way (and is a cause of the original issue).
Openfire’s functionality that provides user grouping is pretty complex. Apart from being pluggable, it provides support for roles ('member' and ‘admin’), properties (which is used for roster sharing functionality, amongst others) and event listeners.
At least one issue exists in the current implementation: when a group instance is removed, but later gets a new member added to it, an incorrect representation of the group is added back to a cache, which makes it ‘available’ to a certain extend.
This snippet illustrates that issue:
final GroupManager groupManager = GroupManager.getInstance(); final Group testGroup = groupManager.createGroup("testGroup2"); groupManager.deleteGroup(testGroup); testGroup.getMembers().add(new JID("john@example.org")); groupManager.createGroup("testGroup2");The interaction on line 4 is invalid. Invoking it does cause an error to be logged, but also causes state internal to the GroupManager (the cache) to become invalid. The last line, in the existing implementation, unexpectedly throws an
GroupAlreadyExistsException. This should not occur, as the group is deleted in line 3.Apart from this bug needing a fix, the code should be improved:
responsibility of updating caches and dispatching events to event listeners should be centralized, instead of spread over several classes.
the implementation should not depend on the event listening API for which it also dispatches events, as this mixes use-cases of the event listener mechanism in a complex way (and is a cause of the original issue).