Add MUC room config extension SPI and disco query passthrough#3420
Open
romanbsd wants to merge 1 commit into
Open
Add MUC room config extension SPI and disco query passthrough#3420romanbsd wants to merge 1 commit into
romanbsd wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis change adds a default Changes
Sequence Diagram(s)sequenceDiagram
participant IQDiscoItemsHandler
participant DiscoItemsProvider
participant IQOwnerHandler
participant MultiUserChatServiceImpl
participant MUCRoomConfigExtensionManager
IQDiscoItemsHandler->>DiscoItemsProvider: getItems(name, node, senderJID, iq.createCopy())
IQOwnerHandler->>MUCRoomConfigExtensionManager: processConfigSubmit(completedForm, room)
IQOwnerHandler->>MUCRoomConfigExtensionManager: contributeConfigForm(configurationForm, room)
IQOwnerHandler->>MUCRoomConfigExtensionManager: populateConfigForm(configurationForm, room)
MultiUserChatServiceImpl->>MUCRoomConfigExtensionManager: contributeRoomDiscoFeatures(features, room)
MultiUserChatServiceImpl->>MUCRoomConfigExtensionManager: contributeRoomDiscoForms(dataForms, room)
Estimated code review effort: 3/5 Related issues: None specified. Related PRs: None specified. Suggested labels: enhancement, muc, disco Suggested reviewers: guusdk, akrherz 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
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.
Actionable comments posted: 1
🧹 Nitpick comments (3)
xmppserver/src/main/java/org/jivesoftware/openfire/disco/IQDiscoItemsHandler.java (1)
159-166: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider passing a defensive copy of the query element to providers.
iqis a live, mutable reference topacket.getChildElement(). It's now handed to third-partyDiscoItemsProvider#getItems(..., Element query)implementations (including future plugin extensions per this PR's objective) beforeiq.createCopy()is used to build the reply. If a provider implementation mutatesquery, that mutation leaks into the reply (and potentially into any other code still holdingpacket.getChildElement()). Passingiq.createCopy()instead removes this class of risk with negligible cost.♻️ Proposed fix
- Iterator<DiscoItem> itemsItr = itemsProvider.getItems(name, node, packet.getFrom(), iq); + Iterator<DiscoItem> itemsItr = itemsProvider.getItems(name, node, packet.getFrom(), iq.createCopy());🤖 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/disco/IQDiscoItemsHandler.java` around lines 159 - 166, The `IQDiscoItemsHandler` flow is passing the live `iq` element from `packet.getChildElement()` into `DiscoItemsProvider#getItems(...)`, which lets provider code mutate the original query object. Update this call site to pass a defensive copy of the query element instead, using `iq.createCopy()` before invoking `itemsProvider.getItems(...)`, so any provider-side changes cannot leak back into the reply or shared packet state.xmppserver/src/main/java/org/jivesoftware/openfire/muc/MUCRoomConfigExtension.java (1)
34-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider making all hook methods
defaultfor easier partial adoption.Plugins that only want to contribute disco#info features/forms (via the already-default methods) are still forced to implement
contributeConfigForm,populateConfigForm, andprocessConfigSubmitwith no-op bodies. Defaulting these to no-ops lowers the barrier for narrowly-scoped extensions without any behavior change for existing implementers.♻️ Proposed refactor
- void contributeConfigForm(DataForm form, MUCRoom room); + default void contributeConfigForm(DataForm form, MUCRoom room) { + } ... - void populateConfigForm(DataForm form, MUCRoom room); + default void populateConfigForm(DataForm form, MUCRoom room) { + } ... - void processConfigSubmit(DataForm completedForm, MUCRoom room); + default void processConfigSubmit(DataForm completedForm, MUCRoom room) { + }Also applies to: 42-42, 50-50
🤖 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/muc/MUCRoomConfigExtension.java` at line 34, Make the MUCRoomConfigExtension hook methods easier to partially adopt by defaulting the config-form related callbacks to no-ops. Update the interface methods contributeConfigForm, populateConfigForm, and processConfigSubmit so plugins that only implement disco#info hooks are no longer forced to provide empty bodies, while keeping existing behavior unchanged for current implementers.xmppserver/src/test/java/org/jivesoftware/openfire/muc/MUCRoomConfigExtensionManagerTest.java (1)
64-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
populateConfigFormandprocessConfigSubmitdelegation.Only
contributeConfigFormdelegation is asserted here;populateConfigFormandprocessConfigSubmit(both implemented bytestExtension) aren't exercised by any test.✅ Proposed addition
`@Test` void registeredExtensionContributesConfigForm() { MUCRoomConfigExtensionManager.getInstance().register(testExtension); final DataForm form = new DataForm(DataForm.Type.form); final MUCRoom room = mock(MUCRoom.class); MUCRoomConfigExtensionManager.getInstance().contributeConfigForm(form, room); assertNotNull(form.getField("test#field")); + MUCRoomConfigExtensionManager.getInstance().populateConfigForm(form, room); + assertEquals("value", form.getField("test#field").getFirstValue()); + MUCRoomConfigExtensionManager.getInstance().processConfigSubmit(form, room); // no-op, but exercises delegation path }🤖 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/test/java/org/jivesoftware/openfire/muc/MUCRoomConfigExtensionManagerTest.java` around lines 64 - 71, The test currently only verifies MUCRoomConfigExtensionManager.contributeConfigForm delegation, so add coverage for the other two extension callbacks implemented by testExtension as well. Extend MUCRoomConfigExtensionManagerTest with assertions that populateConfigForm is invoked when the manager populates a DataForm, and that processConfigSubmit is invoked when a submit form is processed for a MUCRoom. Use the existing MUCRoomConfigExtensionManager, DataForm, MUCRoom, and testExtension setup to locate the delegation points and verify the expected field/state changes.
🤖 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/muc/MUCRoomConfigExtensionManager.java`:
- Around line 50-78: The MUCRoomConfigExtensionManager delegate methods
currently let plugin exceptions escape, so one bad MUCRoomConfigExtension can
break all room config and disco handling. Update contributeConfigForm,
populateConfigForm, processConfigSubmit, contributeRoomDiscoFeatures, and
contributeRoomDiscoForms to wrap each extension call in a try/catch and isolate
failures per extension, using the same pattern consistently across all five
methods. Refer to MUCRoomConfigExtensionManager and its extension iteration
loops when making the change.
---
Nitpick comments:
In
`@xmppserver/src/main/java/org/jivesoftware/openfire/disco/IQDiscoItemsHandler.java`:
- Around line 159-166: The `IQDiscoItemsHandler` flow is passing the live `iq`
element from `packet.getChildElement()` into `DiscoItemsProvider#getItems(...)`,
which lets provider code mutate the original query object. Update this call site
to pass a defensive copy of the query element instead, using `iq.createCopy()`
before invoking `itemsProvider.getItems(...)`, so any provider-side changes
cannot leak back into the reply or shared packet state.
In
`@xmppserver/src/main/java/org/jivesoftware/openfire/muc/MUCRoomConfigExtension.java`:
- Line 34: Make the MUCRoomConfigExtension hook methods easier to partially
adopt by defaulting the config-form related callbacks to no-ops. Update the
interface methods contributeConfigForm, populateConfigForm, and
processConfigSubmit so plugins that only implement disco#info hooks are no
longer forced to provide empty bodies, while keeping existing behavior unchanged
for current implementers.
In
`@xmppserver/src/test/java/org/jivesoftware/openfire/muc/MUCRoomConfigExtensionManagerTest.java`:
- Around line 64-71: The test currently only verifies
MUCRoomConfigExtensionManager.contributeConfigForm delegation, so add coverage
for the other two extension callbacks implemented by testExtension as well.
Extend MUCRoomConfigExtensionManagerTest with assertions that populateConfigForm
is invoked when the manager populates a DataForm, and that processConfigSubmit
is invoked when a submit form is processed for a MUCRoom. Use the existing
MUCRoomConfigExtensionManager, DataForm, MUCRoom, and testExtension setup to
locate the delegation points and verify the expected field/state changes.
🪄 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: a3a6288c-e252-454a-a14a-99217f979c8e
📒 Files selected for processing (7)
xmppserver/src/main/java/org/jivesoftware/openfire/disco/DiscoItemsProvider.javaxmppserver/src/main/java/org/jivesoftware/openfire/disco/IQDiscoItemsHandler.javaxmppserver/src/main/java/org/jivesoftware/openfire/muc/MUCRoomConfigExtension.javaxmppserver/src/main/java/org/jivesoftware/openfire/muc/MUCRoomConfigExtensionManager.javaxmppserver/src/main/java/org/jivesoftware/openfire/muc/spi/IQOwnerHandler.javaxmppserver/src/main/java/org/jivesoftware/openfire/muc/spi/MultiUserChatServiceImpl.javaxmppserver/src/test/java/org/jivesoftware/openfire/muc/MUCRoomConfigExtensionManagerTest.java
romanbsd
force-pushed
the
muc-room-config-extension-spi
branch
from
bad25c4 to
aace8b4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
xmppserver/src/test/java/org/jivesoftware/openfire/muc/MUCRoomConfigExtensionManagerTest.java (2)
51-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for
populateConfigFormandprocessConfigSubmit.
testExtensionoverridespopulateConfigForm(adds a value totest#field) andprocessConfigSubmit, but no test exercisesMUCRoomConfigExtensionManager.populateConfigForm(...)orprocessConfigSubmit(...)delegation/isolation, unlike the coverage given tocontributeConfigFormand the disco methods.🤖 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/test/java/org/jivesoftware/openfire/muc/MUCRoomConfigExtensionManagerTest.java` around lines 51 - 78, Add test coverage in MUCRoomConfigExtensionManagerTest for the missing delegation paths on testExtension: verify MUCRoomConfigExtensionManager.populateConfigForm(...) calls MUCRoomConfigExtension.populateConfigForm(...) and updates the existing test#field value, and verify processConfigSubmit(...) is invoked with the submitted DataForm without affecting unrelated fields. Reuse the existing MUCRoomConfigExtension anonymous instance and the manager methods to keep the new assertions aligned with the current contributeConfigForm and disco feature/form coverage.
28-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest isolation relies on a live global singleton.
MUCRoomConfigExtensionManager.getInstance()is a shared, process-wide singleton. These tests register/unregister extensions on it directly rather than resetting/injecting a fresh instance. As long as@AfterEachruns, this class is self-consistent, but any other test elsewhere in the suite that registers an extension on this same singleton and fails to unregister (e.g., due to an exception before its own cleanup, or forgettingunregister) will leak state into these assertions (e.g.,assertFalse(forms.isEmpty()), exact field presence checks).Consider exposing a package-private
clear()/reset method on the manager for test use, or asserting on the delta contributed by the registered extension rather than on absolute collection state.🤖 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/test/java/org/jivesoftware/openfire/muc/MUCRoomConfigExtensionManagerTest.java` around lines 28 - 111, The test class relies on the shared MUCRoomConfigExtensionManager singleton, so leaked registrations from other tests can make these assertions flaky. Update the tests to avoid depending on global singleton state by either adding a test-only reset/clear hook on MUCRoomConfigExtensionManager and using it in setup/cleanup, or by asserting only the incremental contribution from testExtension in registeredExtensionContributesDisco and registeredExtensionContributesConfigForm. Keep the focus on MUCRoomConfigExtensionManager.getInstance(), register/unregister, and the testExtension/throwingExtension fixtures.
🤖 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.
Nitpick comments:
In
`@xmppserver/src/test/java/org/jivesoftware/openfire/muc/MUCRoomConfigExtensionManagerTest.java`:
- Around line 51-78: Add test coverage in MUCRoomConfigExtensionManagerTest for
the missing delegation paths on testExtension: verify
MUCRoomConfigExtensionManager.populateConfigForm(...) calls
MUCRoomConfigExtension.populateConfigForm(...) and updates the existing
test#field value, and verify processConfigSubmit(...) is invoked with the
submitted DataForm without affecting unrelated fields. Reuse the existing
MUCRoomConfigExtension anonymous instance and the manager methods to keep the
new assertions aligned with the current contributeConfigForm and disco
feature/form coverage.
- Around line 28-111: The test class relies on the shared
MUCRoomConfigExtensionManager singleton, so leaked registrations from other
tests can make these assertions flaky. Update the tests to avoid depending on
global singleton state by either adding a test-only reset/clear hook on
MUCRoomConfigExtensionManager and using it in setup/cleanup, or by asserting
only the incremental contribution from testExtension in
registeredExtensionContributesDisco and
registeredExtensionContributesConfigForm. Keep the focus on
MUCRoomConfigExtensionManager.getInstance(), register/unregister, and the
testExtension/throwingExtension fixtures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e826187-823c-4d04-88cf-e40db817bff2
📒 Files selected for processing (7)
xmppserver/src/main/java/org/jivesoftware/openfire/disco/DiscoItemsProvider.javaxmppserver/src/main/java/org/jivesoftware/openfire/disco/IQDiscoItemsHandler.javaxmppserver/src/main/java/org/jivesoftware/openfire/muc/MUCRoomConfigExtension.javaxmppserver/src/main/java/org/jivesoftware/openfire/muc/MUCRoomConfigExtensionManager.javaxmppserver/src/main/java/org/jivesoftware/openfire/muc/spi/IQOwnerHandler.javaxmppserver/src/main/java/org/jivesoftware/openfire/muc/spi/MultiUserChatServiceImpl.javaxmppserver/src/test/java/org/jivesoftware/openfire/muc/MUCRoomConfigExtensionManagerTest.java
🚧 Files skipped from review as they are similar to previous changes (6)
- xmppserver/src/main/java/org/jivesoftware/openfire/disco/DiscoItemsProvider.java
- xmppserver/src/main/java/org/jivesoftware/openfire/muc/MUCRoomConfigExtension.java
- xmppserver/src/main/java/org/jivesoftware/openfire/muc/MUCRoomConfigExtensionManager.java
- xmppserver/src/main/java/org/jivesoftware/openfire/disco/IQDiscoItemsHandler.java
- xmppserver/src/main/java/org/jivesoftware/openfire/muc/spi/IQOwnerHandler.java
- xmppserver/src/main/java/org/jivesoftware/openfire/muc/spi/MultiUserChatServiceImpl.java
Introduce a plugin-facing MUCRoomConfigExtension registry so plugins can contribute room owner config fields, handle config submission, and augment room disco#info without modifying MUC internals. - Add MUCRoomConfigExtension interface and MUCRoomConfigExtensionManager - Wire extension hooks into IQOwnerHandler (form build/populate/submit) - Wire extension disco contributions into MultiUserChatServiceImpl - Extend DiscoItemsProvider with a 4-arg getItems() default that receives the full disco#items query element (for XEP-0462/XEP-0499 consumers) - Pass the query element from IQDiscoItemsHandler to providers - Add MUCRoomConfigExtensionManagerTest
romanbsd
force-pushed
the
muc-room-config-extension-spi
branch
from
aace8b4 to
8dd6fd9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduces a plugin-facing
MUCRoomConfigExtensionSPI so plugins can contribute room-owner configuration fields, handle their submission, and augment roomdisco#info— without patching MUC internals. It also threads the fulldisco#itemsquery element through toDiscoItemsProviderimplementations, enabling query-aware providers (e.g. XEP-0462 PubSub Type Filtering, XEP-0499 Pubsub Extended Discovery).This is the core-server groundwork for an out-of-tree XEP-0503 (Server-side Spaces) plugin, but the SPI is deliberately generic and space-agnostic.
Changes
MUCRoomConfigExtension— new interface:contributeConfigForm/populateConfigForm/processConfigSubmit, plusdefaultno-opcontributeRoomDiscoFeatures/contributeRoomDiscoFormshooks.MUCRoomConfigExtensionManager— thread-safe singleton registry (CopyOnWriteArrayList, atomicaddIfAbsent) that fans out to registered extensions.IQOwnerHandler— invokes the extension hooks when building, populating, and processing the room owner configuration form.MultiUserChatServiceImpl— invokes the disco feature/form hooks when answering a room'sdisco#info.DiscoItemsProvider— adds adefault4-arggetItems(name, node, senderJID, query)that receives the fulldisco#itemsquery element and delegates to the existing 3-arg method, so current providers are unaffected.IQDiscoItemsHandler— passes the query element to providers.MUCRoomConfigExtensionManagerTest.Compatibility
Fully backward compatible: the new
DiscoItemsProviderand disco hook methods aredefault, so existing providers and any current callers compile and behave unchanged. No behavior change unless a plugin registers an extension.Testing
./mvnw verify— full suite green (1973 core tests, 0 failures).