fix(server): reject duplicate feature names on registration (#883)
Reject duplicate feature registration instead of silently overwriting the existing entry. ## Motivation and Context `FeatureRegistry.add`/`addAll` silently replaced a feature registered under the same key and emitted a spurious `list_changed` notification. Tool/prompt names and resource URIs are unique identifiers in MCP, and the TypeScript SDK already rejects duplicates at registration time (`Tool X is already registered`). - `addTool`/`addPrompt`/`addResource`/`addResourceTemplate` now throw `IllegalArgumentException` on a duplicate name/URI; the existing registration stays intact and no notification is emitted. - `addTools`/`addPrompts`/`addResources` are all-or-nothing: batches with intra-batch duplicates or conflicts with existing keys are rejected without registering anything. - To replace a feature, remove it first (`removeTool` + `addTool`), matching the TypeScript SDK's remove-then-re-register path. No new public API. ## How Has This Been Tested? New `FeatureRegistryTest` unit suite (duplicate rejection, batch atomicity, no notification on rejection, re-add after remove) plus integration tests for tools, prompts, resources, and resource templates. The check also surfaced a real duplicate `addResource` in `AbstractResourceIntegrationTest`, fixed here. `jvmTest` (all modules), `apiCheck`, `ktlintCheck`, and `detekt` pass locally. ## Breaking Changes No public API signature changes (`apiCheck` passes against the existing dump). Behavioral: registering a duplicate name/URI now throws `IllegalArgumentException` instead of silently overwriting; remove the feature first to replace it. ## Types of changes - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [x] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update ## Checklist - [x] I have read the [MCP Documentation](https://modelcontextprotocol.io) - [x] My code follows the repository's style guidelines - [x] New and existing tests pass locally - [x] I have added appropriate error handling - [x] I have added or updated documentation as needed
This commit is contained in:
Pavel Gorgulov
GitHub
parent
737b46f032
commit
9ef4f80f85
2 changed files with 333 additions and 28 deletions
|
|
@ -20,9 +20,12 @@ internal interface FeatureListener {
|
|||
* A generic registry for managing features of a specified type. This class provides thread-safe
|
||||
* operations for adding, removing, and retrieving features from the registry.
|
||||
*
|
||||
* Feature keys are unique: [add] and [addAll] reject keys that are already registered.
|
||||
* To replace a feature, [remove] it first and add the new one.
|
||||
*
|
||||
* @param T The type of the feature, constrained to implement the [Feature] interface.
|
||||
* @param featureType A string description of the type of feature being managed.
|
||||
* Used primarily for logging purposes.
|
||||
* Used for logging and error messages.
|
||||
*/
|
||||
internal class FeatureRegistry<T : Feature>(private val featureType: String) {
|
||||
|
||||
|
|
@ -44,11 +47,11 @@ internal class FeatureRegistry<T : Feature>(private val featureType: String) {
|
|||
private val listeners = atomic(persistentListOf<FeatureListener>())
|
||||
|
||||
internal fun addListener(listener: FeatureListener) {
|
||||
listeners.update { it.add(listener) }
|
||||
listeners.update { it.adding(listener) }
|
||||
}
|
||||
|
||||
internal fun removeListener(listener: FeatureListener) {
|
||||
listeners.update { it.remove(listener) }
|
||||
listeners.update { it.removing(listener) }
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -58,27 +61,42 @@ internal class FeatureRegistry<T : Feature>(private val featureType: String) {
|
|||
*/
|
||||
internal fun add(feature: T) {
|
||||
logger.info { "Adding $featureType: \"${feature.key}\"" }
|
||||
val oldMap = registry.getAndUpdate { current -> current.put(feature.key, feature) }
|
||||
val oldFeature = oldMap[feature.key]
|
||||
registry.update { current ->
|
||||
require(!current.containsKey(feature.key)) {
|
||||
"$featureType \"${feature.key}\" is already registered. Remove it first to replace it."
|
||||
}
|
||||
current.putting(feature.key, feature)
|
||||
}
|
||||
|
||||
logger.info { "Added $featureType: \"${feature.key}\"" }
|
||||
notifyFeatureUpdated(oldFeature, feature)
|
||||
notifyFeatureUpdated(null, feature)
|
||||
}
|
||||
|
||||
/**
|
||||
* Adds the given list of features to the registry. Each feature is mapped by its key
|
||||
* and added to the current registry.
|
||||
* and added to the current registry. The operation is all-or-nothing: if any key is
|
||||
* rejected, no features are added.
|
||||
*
|
||||
* @param features The list of features to add to the registry.
|
||||
*/
|
||||
internal fun addAll(features: List<T>) {
|
||||
logger.info { "Adding ${featureType}s: ${features.size}" }
|
||||
val oldMap = registry.getAndUpdate { current -> current.putAll(features.associateBy { it.key }) }
|
||||
val newEntries = features.associateBy { it.key }
|
||||
require(newEntries.size == features.size) {
|
||||
val duplicated = features.groupingBy { it.key }.eachCount().filterValues { it > 1 }.keys
|
||||
"Duplicate $featureType keys in batch: $duplicated"
|
||||
}
|
||||
registry.update { current ->
|
||||
val conflicting = newEntries.keys.filter { current.containsKey(it) }
|
||||
require(conflicting.isEmpty()) {
|
||||
"${featureType}s already registered: $conflicting. Remove them first to replace them."
|
||||
}
|
||||
current.puttingAll(newEntries)
|
||||
}
|
||||
|
||||
logger.info { "Added ${featureType}s: ${features.size}" }
|
||||
for (feature in features) {
|
||||
val oldFeature = oldMap[feature.key]
|
||||
notifyFeatureUpdated(oldFeature, feature)
|
||||
notifyFeatureUpdated(null, feature)
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -90,7 +108,7 @@ internal class FeatureRegistry<T : Feature>(private val featureType: String) {
|
|||
*/
|
||||
internal fun remove(key: FeatureKey): Boolean {
|
||||
logger.info { "Removing $featureType: \"$key\"" }
|
||||
val oldMap = registry.getAndUpdate { current -> current.remove(key) }
|
||||
val oldMap = registry.getAndUpdate { current -> current.removing(key) }
|
||||
|
||||
val removedFeature = oldMap[key]
|
||||
val removed = removedFeature != null
|
||||
|
|
|
|||
|
|
@ -0,0 +1,106 @@
|
|||
package io.modelcontextprotocol.kotlin.sdk.server
|
||||
|
||||
import io.kotest.matchers.shouldBe
|
||||
import kotlin.test.Test
|
||||
import kotlin.test.assertFailsWith
|
||||
|
||||
private data class TestFeature(override val key: FeatureKey, val payload: String = "") : Feature
|
||||
|
||||
private class RecordingListener : FeatureListener {
|
||||
val updatedKeys = mutableListOf<String>()
|
||||
|
||||
override fun onFeatureUpdated(featureKey: String) {
|
||||
updatedKeys.add(featureKey)
|
||||
}
|
||||
}
|
||||
|
||||
class FeatureRegistryTest {
|
||||
|
||||
private val registry = FeatureRegistry<TestFeature>("Tool")
|
||||
private val listener = RecordingListener()
|
||||
|
||||
init {
|
||||
registry.addListener(listener)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `add should register feature and notify listener`() {
|
||||
val feature = TestFeature("a")
|
||||
|
||||
registry.add(feature)
|
||||
|
||||
registry.values shouldBe mapOf("a" to feature)
|
||||
listener.updatedKeys shouldBe listOf("a")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `add should throw when key is already registered`() {
|
||||
registry.add(TestFeature("a"))
|
||||
|
||||
val exception = assertFailsWith<IllegalArgumentException> {
|
||||
registry.add(TestFeature("a", payload = "override"))
|
||||
}
|
||||
|
||||
exception.message shouldBe "Tool \"a\" is already registered. Remove it first to replace it."
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `add should keep original feature and not notify when duplicate is rejected`() {
|
||||
val original = TestFeature("a", payload = "original")
|
||||
registry.add(original)
|
||||
|
||||
assertFailsWith<IllegalArgumentException> {
|
||||
registry.add(TestFeature("a", payload = "override"))
|
||||
}
|
||||
|
||||
registry.values shouldBe mapOf("a" to original)
|
||||
listener.updatedKeys shouldBe listOf("a")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `addAll should register all features and notify per feature`() {
|
||||
val a = TestFeature("a")
|
||||
val b = TestFeature("b")
|
||||
|
||||
registry.addAll(listOf(a, b))
|
||||
|
||||
registry.values shouldBe mapOf("a" to a, "b" to b)
|
||||
listener.updatedKeys shouldBe listOf("a", "b")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `addAll should throw on duplicate keys within batch and register nothing`() {
|
||||
val exception = assertFailsWith<IllegalArgumentException> {
|
||||
registry.addAll(listOf(TestFeature("a"), TestFeature("b"), TestFeature("a")))
|
||||
}
|
||||
|
||||
exception.message shouldBe "Duplicate Tool keys in batch: [a]"
|
||||
registry.values shouldBe emptyMap<FeatureKey, TestFeature>()
|
||||
listener.updatedKeys shouldBe emptyList<String>()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `addAll should throw when any key is already registered and register nothing`() {
|
||||
val existing = TestFeature("existing")
|
||||
registry.add(existing)
|
||||
|
||||
val exception = assertFailsWith<IllegalArgumentException> {
|
||||
registry.addAll(listOf(TestFeature("new"), TestFeature("existing", payload = "override")))
|
||||
}
|
||||
|
||||
exception.message shouldBe "Tools already registered: [existing]. Remove them first to replace them."
|
||||
registry.values shouldBe mapOf("existing" to existing)
|
||||
listener.updatedKeys shouldBe listOf("existing")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `add should succeed after the existing feature is removed`() {
|
||||
registry.add(TestFeature("a", payload = "original"))
|
||||
registry.remove("a")
|
||||
|
||||
registry.add(TestFeature("a", payload = "replacement"))
|
||||
|
||||
registry.values shouldBe mapOf("a" to TestFeature("a", payload = "replacement"))
|
||||
listener.updatedKeys shouldBe listOf("a", "a", "a")
|
||||
}
|
||||
}
|
||||
Loading…
Reference in a new issue