Refactor schema validation to reduce duplication and add additionalProperties support
This refactoring extracts common schema validation logic into a base Schema class, eliminating ~100 lines of duplicate code between InputSchema and OutputSchema. It also adds support for the JSON Schema `additionalProperties` keyword to allow fine-grained control over extra properties in tool arguments. Previously `additionalParameters` could be set only on OutputSchema. Key changes: - Created MCP::Tool::Schema base class consolidating: - Schema validation against JSON Schema Draft 4 metaschema - Deep key transformation (string to symbol conversion) - $ref disallowance checking - Common validation methods - Refactored InputSchema to inherit from Schema: - Simplified constructor to accept full schema hash - Changed required fields storage from symbols to strings - Removed ~40 lines of duplicated validation logic - Refactored OutputSchema to inherit from Schema: - Removed ~54 lines of duplicated validation logic - Now shares all common functionality with InputSchema - Updated Tool.input_schema setter: - Simplified to pass full hash directly to InputSchema - Added additionalProperties support: - By default, tools now allow additional properties (JSON Schema default) - Can explicitly set `additionalProperties: false` to disallow extras - Added comprehensive test coverage for both behaviors
This commit is contained in:
parent
85b81a6a65
commit
a3c24d2bcc
4 changed files with 240 additions and 162 deletions
|
|
@ -1096,6 +1096,62 @@ module MCP
|
|||
assert_equal "OK", response[:result][:content][0][:content]
|
||||
end
|
||||
|
||||
test "tools/call allows additional properties by default" do
|
||||
server = Server.new(
|
||||
tools: [TestTool],
|
||||
configuration: Configuration.new(validate_tool_call_arguments: true),
|
||||
)
|
||||
|
||||
response = server.handle(
|
||||
{
|
||||
jsonrpc: "2.0",
|
||||
id: 1,
|
||||
method: "tools/call",
|
||||
params: {
|
||||
name: "test_tool",
|
||||
arguments: {
|
||||
message: "Hello, world!",
|
||||
other_property: "I am allowed",
|
||||
},
|
||||
},
|
||||
},
|
||||
)
|
||||
|
||||
assert_equal "2.0", response[:jsonrpc]
|
||||
assert_equal 1, response[:id]
|
||||
assert response[:result], "Expected result key in response"
|
||||
assert_equal "text", response[:result][:content][0][:type]
|
||||
assert_equal "OK", response[:result][:content][0][:content]
|
||||
end
|
||||
|
||||
test "tools/call disallows additional properties when additionalProperties set to false" do
|
||||
server = Server.new(
|
||||
tools: [TestToolWithAdditionalPropertiesSetToFalse],
|
||||
configuration: Configuration.new(validate_tool_call_arguments: true),
|
||||
)
|
||||
|
||||
response = server.handle(
|
||||
{
|
||||
jsonrpc: "2.0",
|
||||
id: 1,
|
||||
method: "tools/call",
|
||||
params: {
|
||||
name: "test_tool_with_additional_properties_set_to_false",
|
||||
arguments: {
|
||||
message: "Hello, world!",
|
||||
extra: 123,
|
||||
},
|
||||
},
|
||||
},
|
||||
)
|
||||
|
||||
assert_equal "2.0", response[:jsonrpc]
|
||||
assert_equal 1, response[:id]
|
||||
assert_nil response[:error], "Expected no JSON-RPC error"
|
||||
assert response[:result][:isError]
|
||||
assert_includes response[:result][:content][0][:text], "Invalid arguments"
|
||||
end
|
||||
|
||||
test "tools/call with no args" do
|
||||
server = Server.new(tools: [@tool_with_no_args])
|
||||
|
||||
|
|
@ -1123,7 +1179,19 @@ module MCP
|
|||
input_schema({ properties: { message: { type: "string" } }, required: ["message"] })
|
||||
|
||||
class << self
|
||||
def call(message:, server_context: nil)
|
||||
def call(server_context: nil, **kwargs)
|
||||
Tool::Response.new([{ type: "text", content: "OK" }])
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
class TestToolWithAdditionalPropertiesSetToFalse < Tool
|
||||
tool_name "test_tool_with_additional_properties_set_to_false"
|
||||
description "a test tool with additionalProperties set to false for testing"
|
||||
input_schema({ properties: { message: { type: "string" } }, required: ["message"], additionalProperties: false })
|
||||
|
||||
class << self
|
||||
def call(server_context: nil, **kwargs)
|
||||
Tool::Response.new([{ type: "text", content: "OK" }])
|
||||
end
|
||||
end
|
||||
|
|
|
|||
|
|
@ -5,37 +5,45 @@ require "test_helper"
|
|||
module MCP
|
||||
class Tool
|
||||
class InputSchemaTest < ActiveSupport::TestCase
|
||||
test "required arguments are converted to symbols" do
|
||||
input_schema = InputSchema.new(properties: { message: { type: "string" } }, required: ["message"])
|
||||
assert_equal [:message], input_schema.required
|
||||
test "required arguments are converted to strings" do
|
||||
input_schema = InputSchema.new(properties: { message: { type: "string" } }, required: [:message])
|
||||
assert_equal ["message"], input_schema.schema[:required]
|
||||
end
|
||||
|
||||
test "to_h returns a hash representation of the input schema" do
|
||||
input_schema = InputSchema.new(properties: { message: { type: "string" } }, required: [:message])
|
||||
input_schema = InputSchema.new(properties: { message: { type: "string" } }, required: ["message"])
|
||||
assert_equal(
|
||||
{ type: "object", properties: { message: { type: "string" } }, required: [:message] },
|
||||
{ type: "object", properties: { message: { type: "string" } }, required: ["message"] },
|
||||
input_schema.to_h,
|
||||
)
|
||||
end
|
||||
|
||||
test "to_h returns a hash representation of the input schema with additionalProperties set to false" do
|
||||
input_schema = InputSchema.new(properties: { message: { type: "string" } }, required: ["message"], additionalProperties: false)
|
||||
assert_equal(
|
||||
{ type: "object", properties: { message: { type: "string" } }, required: ["message"], additionalProperties: false },
|
||||
input_schema.to_h,
|
||||
)
|
||||
end
|
||||
|
||||
test "missing_required_arguments returns an array of missing required arguments" do
|
||||
input_schema = InputSchema.new(properties: { message: { type: "string" } }, required: [:message])
|
||||
assert_equal [:message], input_schema.missing_required_arguments({})
|
||||
input_schema = InputSchema.new(properties: { message: { type: "string" } }, required: ["message"])
|
||||
assert_equal ["message"], input_schema.missing_required_arguments({})
|
||||
end
|
||||
|
||||
test "missing_required_arguments returns an empty array if no required arguments are missing" do
|
||||
input_schema = InputSchema.new(properties: { message: { type: "string" } }, required: [:message])
|
||||
input_schema = InputSchema.new(properties: { message: { type: "string" } }, required: ["message"])
|
||||
assert_empty input_schema.missing_required_arguments({ message: "Hello, world!" })
|
||||
end
|
||||
|
||||
test "valid schema initialization" do
|
||||
schema = InputSchema.new(properties: { foo: { type: "string" } }, required: [:foo])
|
||||
assert_equal({ type: "object", properties: { foo: { type: "string" } }, required: [:foo] }, schema.to_h)
|
||||
schema = InputSchema.new(properties: { foo: { type: "string" } }, required: ["foo"])
|
||||
assert_equal({ type: "object", properties: { foo: { type: "string" } }, required: ["foo"] }, schema.to_h)
|
||||
end
|
||||
|
||||
test "invalid schema raises argument error" do
|
||||
assert_raises(ArgumentError) do
|
||||
InputSchema.new(properties: { foo: { type: "invalid_type" } }, required: [:foo])
|
||||
InputSchema.new(properties: { foo: { type: "invalid_type" } }, required: ["foo"])
|
||||
end
|
||||
end
|
||||
|
||||
|
|
@ -46,19 +54,36 @@ module MCP
|
|||
end
|
||||
|
||||
test "validate arguments with valid data" do
|
||||
schema = InputSchema.new(properties: { foo: { type: "string" } }, required: [:foo])
|
||||
schema = InputSchema.new(properties: { foo: { type: "string" } }, required: ["foo"])
|
||||
assert_nil(schema.validate_arguments({ foo: "bar" }))
|
||||
end
|
||||
|
||||
test "validate arguments with invalid data" do
|
||||
schema = InputSchema.new(properties: { foo: { type: "string" } }, required: [:foo])
|
||||
schema = InputSchema.new(properties: { foo: { type: "string" } }, required: ["foo"])
|
||||
assert_raises(InputSchema::ValidationError) do
|
||||
schema.validate_arguments({ foo: 123 })
|
||||
end
|
||||
end
|
||||
|
||||
test "validate arguments with valid data when additionalProperties set to nil (default)" do
|
||||
schema = InputSchema.new(properties: { foo: { type: "string" } }, required: ["foo"])
|
||||
assert_nil(schema.validate_arguments({ foo: "bar", extra: 123 }))
|
||||
end
|
||||
|
||||
test "validate arguments with valid data when additionalProperties set to true (default)" do
|
||||
schema = InputSchema.new(properties: { foo: { type: "string" } }, required: ["foo"], additionalProperties: true)
|
||||
assert_nil(schema.validate_arguments({ foo: "bar", extra: 123 }))
|
||||
end
|
||||
|
||||
test "validate arguments with invalid data when additionalProperties set to false" do
|
||||
schema = InputSchema.new(properties: { foo: { type: "string" } }, required: ["foo"], additionalProperties: false)
|
||||
assert_raises(InputSchema::ValidationError) do
|
||||
schema.validate_arguments({ foo: "bar", extra: 123 })
|
||||
end
|
||||
end
|
||||
|
||||
test "unexpected errors bubble up from validate_arguments" do
|
||||
schema = InputSchema.new(properties: { foo: { type: "string" } }, required: [:foo])
|
||||
schema = InputSchema.new(properties: { foo: { type: "string" } }, required: ["foo"])
|
||||
|
||||
JSON::Validator.stub(:fully_validate, ->(*) { raise "unexpected error" }) do
|
||||
assert_raises(RuntimeError) do
|
||||
|
|
@ -69,29 +94,32 @@ module MCP
|
|||
|
||||
test "rejects schemas with $ref references" do
|
||||
assert_raises(ArgumentError) do
|
||||
InputSchema.new(properties: { foo: { "$ref" => "#/definitions/bar" } }, required: [:foo])
|
||||
InputSchema.new(properties: { foo: { "$ref" => "#/definitions/bar" } }, required: ["foo"])
|
||||
end
|
||||
end
|
||||
|
||||
test "rejects schemas with symbol $ref references" do
|
||||
assert_raises(ArgumentError) do
|
||||
InputSchema.new(properties: { foo: { :$ref => "#/definitions/bar" } }, required: [:foo])
|
||||
InputSchema.new(properties: { foo: { :$ref => "#/definitions/bar" } }, required: ["foo"])
|
||||
end
|
||||
end
|
||||
|
||||
test "== compares two input schemas with the same properties and required fields" do
|
||||
schema1 = InputSchema.new(properties: { foo: { type: "string" } }, required: [:foo])
|
||||
schema2 = InputSchema.new(properties: { foo: { type: "string" } }, required: [:foo])
|
||||
test "== compares two input schemas with the same properties, required fields" do
|
||||
schema1 = InputSchema.new(properties: { foo: { type: "string" } }, required: ["foo"])
|
||||
schema2 = InputSchema.new(properties: { foo: { type: "string" } }, required: ["foo"])
|
||||
assert_equal schema1, schema2
|
||||
|
||||
schema3 = InputSchema.new(properties: { bar: { type: "string" } }, required: [:bar])
|
||||
schema3 = InputSchema.new(properties: { bar: { type: "string" } }, required: ["bar"])
|
||||
refute_equal schema1, schema3
|
||||
|
||||
schema4 = InputSchema.new(properties: { foo: { type: "string" } }, required: [:bar])
|
||||
schema4 = InputSchema.new(properties: { foo: { type: "string" } }, required: ["bar"])
|
||||
refute_equal schema1, schema4
|
||||
|
||||
schema5 = InputSchema.new(properties: { bar: { type: "string" } }, required: [:foo])
|
||||
schema5 = InputSchema.new(properties: { bar: { type: "string" } }, required: ["foo"])
|
||||
refute_equal schema1, schema5
|
||||
|
||||
schema6 = InputSchema.new(properties: { foo: { type: "string" } }, required: ["foo"], additionalProperties: false)
|
||||
refute_equal schema1, schema6
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
|||
|
|
@ -6,7 +6,7 @@ module MCP
|
|||
class Tool
|
||||
class OutputSchemaTest < ActiveSupport::TestCase
|
||||
test "to_h returns a hash representation of the output schema" do
|
||||
output_schema = OutputSchema.new(properties: { result: { type: "string" } }, required: [:result])
|
||||
output_schema = OutputSchema.new(properties: { result: { type: "string" } }, required: ["result"])
|
||||
assert_equal(
|
||||
{ type: "object", properties: { result: { type: "string" } }, required: ["result"] },
|
||||
output_schema.to_h,
|
||||
|
|
@ -14,34 +14,55 @@ module MCP
|
|||
end
|
||||
|
||||
test "validate_result validates result against the schema" do
|
||||
output_schema = OutputSchema.new(properties: { result: { type: "string" } }, required: [:result])
|
||||
output_schema = OutputSchema.new(properties: { result: { type: "string" } }, required: ["result"])
|
||||
assert_nothing_raised do
|
||||
output_schema.validate_result({ result: "success" })
|
||||
end
|
||||
end
|
||||
|
||||
test "validate_result validates result with additional properties against the schema when additionalProperties set to nil (default)" do
|
||||
output_schema = OutputSchema.new(properties: { result: { type: "string" } }, required: ["result"])
|
||||
assert_nothing_raised do
|
||||
output_schema.validate_result({ result: "success", extra: 123 })
|
||||
end
|
||||
end
|
||||
|
||||
test "validate_result validates result with additional properties against the schema when additionalProperties set to true" do
|
||||
output_schema = OutputSchema.new(properties: { result: { type: "string" } }, required: ["result"], additionalProperties: true)
|
||||
assert_nothing_raised do
|
||||
output_schema.validate_result({ result: "success", extra: 123 })
|
||||
end
|
||||
end
|
||||
|
||||
test "validate_result raises error with additional properties when additionalProperties set to false)" do
|
||||
output_schema = OutputSchema.new(properties: { result: { type: "string" } }, required: ["result"], additionalProperties: false)
|
||||
assert_raises(OutputSchema::ValidationError) do
|
||||
output_schema.validate_result({ result: "success", extra: 123 })
|
||||
end
|
||||
end
|
||||
|
||||
test "validate_result raises error for invalid result" do
|
||||
output_schema = OutputSchema.new(properties: { result: { type: "string" } }, required: [:result])
|
||||
output_schema = OutputSchema.new(properties: { result: { type: "string" } }, required: ["result"])
|
||||
assert_raises(OutputSchema::ValidationError) do
|
||||
output_schema.validate_result({ result: 123 })
|
||||
end
|
||||
end
|
||||
|
||||
test "validate_result raises error for missing required field" do
|
||||
output_schema = OutputSchema.new(properties: { result: { type: "string" } }, required: [:result])
|
||||
output_schema = OutputSchema.new(properties: { result: { type: "string" } }, required: ["result"])
|
||||
assert_raises(OutputSchema::ValidationError) do
|
||||
output_schema.validate_result({})
|
||||
end
|
||||
end
|
||||
|
||||
test "valid schema initialization" do
|
||||
schema = OutputSchema.new(properties: { foo: { type: "string" } }, required: [:foo])
|
||||
schema = OutputSchema.new(properties: { foo: { type: "string" } }, required: ["foo"])
|
||||
assert_equal({ type: "object", properties: { foo: { type: "string" } }, required: ["foo"] }, schema.to_h)
|
||||
end
|
||||
|
||||
test "invalid schema raises argument error" do
|
||||
assert_raises(ArgumentError) do
|
||||
OutputSchema.new(properties: { foo: { type: "invalid_type" } }, required: [:foo])
|
||||
OutputSchema.new(properties: { foo: { type: "invalid_type" } }, required: ["foo"])
|
||||
end
|
||||
end
|
||||
|
||||
|
|
@ -52,7 +73,7 @@ module MCP
|
|||
end
|
||||
|
||||
test "unexpected errors bubble up from validate_result" do
|
||||
schema = OutputSchema.new(properties: { foo: { type: "string" } }, required: [:foo])
|
||||
schema = OutputSchema.new(properties: { foo: { type: "string" } }, required: ["foo"])
|
||||
|
||||
JSON::Validator.stub(:fully_validate, ->(*) { raise "unexpected error" }) do
|
||||
assert_raises(RuntimeError) do
|
||||
|
|
@ -63,28 +84,28 @@ module MCP
|
|||
|
||||
test "rejects schemas with $ref references" do
|
||||
assert_raises(ArgumentError) do
|
||||
OutputSchema.new(properties: { foo: { "$ref" => "#/definitions/bar" } }, required: [:foo])
|
||||
OutputSchema.new(properties: { foo: { "$ref" => "#/definitions/bar" } }, required: ["foo"])
|
||||
end
|
||||
end
|
||||
|
||||
test "rejects schemas with symbol $ref references" do
|
||||
assert_raises(ArgumentError) do
|
||||
OutputSchema.new(properties: { foo: { :$ref => "#/definitions/bar" } }, required: [:foo])
|
||||
OutputSchema.new(properties: { foo: { :$ref => "#/definitions/bar" } }, required: ["foo"])
|
||||
end
|
||||
end
|
||||
|
||||
test "== compares two output schemas with the same properties and required fields" do
|
||||
schema1 = OutputSchema.new(properties: { foo: { type: "string" } }, required: [:foo])
|
||||
schema2 = OutputSchema.new(properties: { foo: { type: "string" } }, required: [:foo])
|
||||
schema1 = OutputSchema.new(properties: { foo: { type: "string" } }, required: ["foo"])
|
||||
schema2 = OutputSchema.new(properties: { foo: { type: "string" } }, required: ["foo"])
|
||||
assert_equal schema1, schema2
|
||||
|
||||
schema3 = OutputSchema.new(properties: { bar: { type: "string" } }, required: [:bar])
|
||||
schema3 = OutputSchema.new(properties: { bar: { type: "string" } }, required: ["bar"])
|
||||
refute_equal schema1, schema3
|
||||
|
||||
schema4 = OutputSchema.new(properties: { foo: { type: "string" } }, required: [:bar])
|
||||
schema4 = OutputSchema.new(properties: { foo: { type: "string" } }, required: ["bar"])
|
||||
refute_equal schema1, schema4
|
||||
|
||||
schema5 = OutputSchema.new(properties: { bar: { type: "string" } }, required: [:foo])
|
||||
schema5 = OutputSchema.new(properties: { bar: { type: "string" } }, required: ["foo"])
|
||||
refute_equal schema1, schema5
|
||||
end
|
||||
|
||||
|
|
@ -105,7 +126,7 @@ module MCP
|
|||
required: ["items"],
|
||||
},
|
||||
},
|
||||
required: [:data],
|
||||
required: ["data"],
|
||||
)
|
||||
|
||||
valid_result = {
|
||||
|
|
@ -136,7 +157,7 @@ module MCP
|
|||
type: "array",
|
||||
items: {
|
||||
properties: { foo: { type: "string" } },
|
||||
required: [:foo],
|
||||
required: ["foo"],
|
||||
},
|
||||
})
|
||||
assert_equal(
|
||||
|
|
|
|||
|
|
@ -74,13 +74,13 @@ module MCP
|
|||
class MockTool < Tool
|
||||
tool_name "my_mock_tool"
|
||||
description "a mock tool for testing"
|
||||
input_schema({ properties: { message: { type: "string" } }, required: [:message] })
|
||||
input_schema({ properties: { message: { type: "string" } }, required: ["message"] })
|
||||
end
|
||||
|
||||
tool = MockTool
|
||||
assert_equal "my_mock_tool", tool.name_value
|
||||
assert_equal "a mock tool for testing", tool.description
|
||||
assert_equal({ type: "object", properties: { message: { type: "string" } }, required: [:message] }, tool.input_schema.to_h)
|
||||
assert_equal({ type: "object", properties: { message: { type: "string" } }, required: ["message"] }, tool.input_schema.to_h)
|
||||
end
|
||||
|
||||
test "defaults to class name as tool name" do
|
||||
|
|
@ -103,12 +103,12 @@ module MCP
|
|||
|
||||
test "accepts input schema as an InputSchema object" do
|
||||
class InputSchemaTool < Tool
|
||||
input_schema InputSchema.new(properties: { message: { type: "string" } }, required: [:message])
|
||||
input_schema InputSchema.new(properties: { message: { type: "string" } }, required: ["message"])
|
||||
end
|
||||
|
||||
tool = InputSchemaTool
|
||||
|
||||
expected = { type: "object", properties: { message: { type: "string" } }, required: [:message] }
|
||||
expected = { type: "object", properties: { message: { type: "string" } }, required: ["message"] }
|
||||
assert_equal expected, tool.input_schema.to_h
|
||||
end
|
||||
|
||||
|
|
@ -119,7 +119,7 @@ module MCP
|
|||
properties: {
|
||||
count: { type: "integer", minimum: "not a number" },
|
||||
},
|
||||
required: [:count],
|
||||
required: ["count"],
|
||||
)
|
||||
end
|
||||
end
|
||||
|
|
@ -340,7 +340,7 @@ module MCP
|
|||
|
||||
test "accepts output_schema as a hash" do
|
||||
class HashOutputSchemaTool < Tool
|
||||
output_schema({ properties: { result: { type: "string" } }, required: [:result] })
|
||||
output_schema({ properties: { result: { type: "string" } }, required: ["result"] })
|
||||
end
|
||||
|
||||
tool = HashOutputSchemaTool
|
||||
|
|
@ -350,7 +350,7 @@ module MCP
|
|||
|
||||
test "accepts output_schema as an OutputSchema object" do
|
||||
class OutputSchemaObjectTool < Tool
|
||||
output_schema Tool::OutputSchema.new(properties: { result: { type: "string" } }, required: [:result])
|
||||
output_schema Tool::OutputSchema.new(properties: { result: { type: "string" } }, required: ["result"])
|
||||
end
|
||||
|
||||
tool = OutputSchemaObjectTool
|
||||
|
|
@ -365,7 +365,7 @@ module MCP
|
|||
properties: {
|
||||
count: { type: "integer", minimum: "not a number" },
|
||||
},
|
||||
required: [:count],
|
||||
required: ["count"],
|
||||
)
|
||||
end
|
||||
end
|
||||
|
|
@ -428,7 +428,7 @@ module MCP
|
|||
assert_equal "test_tool_with_output", tool.name_value
|
||||
assert_equal "a test tool with output schema", tool.description
|
||||
|
||||
expected_input = { type: "object", properties: { message: { type: "string" } }, required: [:message] }
|
||||
expected_input = { type: "object", properties: { message: { type: "string" } }, required: ["message"] }
|
||||
assert_equal expected_input, tool.input_schema.to_h
|
||||
|
||||
expected_output = { type: "object", properties: { result: { type: "string" }, success: { type: "boolean" } }, required: ["result", "success"] }
|
||||
|
|
|
|||
Loading…
Reference in a new issue