Refactor towards simple data/config classes

[sambostock]

This PR is an alternative to #48, with the same underlying motivations. It explores the idea of having tools and prompts be defined as simple data/config/value objects, and drops support for the current inheritance based approach.

It is a work in progress, but I have opted to open a draft now to get feedback, especially as it relates to other PRs, such as #54.

Motivation and Context

How Has This Been Tested?

Breaking Changes

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the MCP Documentation
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have added or updated documentation as needed

Additional context

[sambostock]

@koic asked the following on #48, which is also relevant here, so I'm transcribing it:

What benefits would this provide to users?

There may be debate over whether an inheritance-based approach is preferable, but if an API incompatibility is introduced, users should receive benefits that justify the cost of the change.

Additionally, the migration path for an API change should follow this approach:

1. Display a deprecation warning to avoid breaking existing code
2. Remove support for the old code in the future (breaking change)

RuboCop is a supporting tool, and as a product, this kind of migration flow is user-friendly.

[sambostock]

What benefits would this provide to users?

My original motivation for this was that as a consumer I found the API confusing: why define a class that will never be instantiated and only have singleton methods?

In addition, the existing approach has (IMO) a higher implementation complexity because of the various "macros" for setting/getting the configured values, as well as the unusual inherited hook.

In this new approach, we maintain a clear separation between the "schema" and the behaviour (isolated in the handler block):

• Instances only hold config, and delegate to a handler (via the supplied block)
• By using a block, which offers built-in optionality of arguments, we can both simplify our implementation, and allow consumers to rely on their existing knowledge of Ruby blocks (rather than needing to know we dynamically change the arguments we pass to the method).
• Validation and determination of required arguments is now consistently handled in MCP::Server for both tools and prompts, making it easier for consumers to swap in their own objects conforming to the simple duck type of a data/schema container (rather than needing to implement behaviour).

Additionally, the migration path for an API change should follow this approach:

1. Display a deprecation warning to avoid breaking existing code
2. Remove support for the old code in the future (breaking change)

Technically, this gem's README says it follows Semantic Versioning, and since we're still on a 0.x version, we're "free to make breaking changes at any time", and perhaps we should consider if it makes more sense to rapidly iterate towards a feature complete 1.0 release.

That said, if we want to go the deprecation warning route, I think we would do something like the following:

1. Emit deprecation warning on inherited¹ recommending the .define approach
2. Emit deprecation warning on .define when no name is provided (as it will become mandatory)
3. Maybe delegate methods like :template to :call, to match the "future" .call based API, with a deprecation.
4. Maybe delegate RequestHandlerError#original_error to Exception#cause with a deprecation
5. Publish a new release.
6. Merge this PR, dropping support for the old API.

Footnotes

1. We'd have to figure out how to ensure it only triggers on named subclassing, as define currently also uses inheritance. ↩

[kfischer-okarin]

One comment I had seeing this PR in general... I think it might be more idiomatic and simpler to just use actual Data classes offered by Ruby

[sambostock]

@kfischer-okarin I flip-flopped on using Data or not, and opted not to on gut feeling.

I gave it a shot based on your feedback (sambostock/mcp-ruby-sdk@data...sambostock:mcp-ruby-sdk:data-define), and based on what I found, I think it's still preferable to stick to POROs:

Exploring using Data.define instead of our own classes. This has a number of pros & cons:

• Marginally less code (depends on each case, < 100 lines across the entire project)
• Increased API surface
  • This is both a pro & con: we get some methods for free, but that means they'll be part of our public API and we'll need to continue supporting them (e.g. .[] to construct instances).
• Loss of ability to subclass Content (AFAICT)
• Violating Liskov Substitution (child changes parent's contract)
  • e.g. Data#to_h has certain semantics and complements initialize.
    We customize Data#to_h to return camelCase values and compact, and omit things like @block.
• Unable to mix positional and keyword arguments: the new method provided by Data expects either all positional or all keyword arguments, and ALWAYS passes them to initialize as keywords. This is incompatible with the API we offer for classes like Content::Text, which expects a positional text argument, and an optional keyword annotations: argument.

To expand on the last point

# frozen_string_literal: true

require "minitest/autorun"

class UsingPORO
  attr_reader :required_positional, :optional_keyword

  def initialize(required_positional, optional_keyword: nil)
    @required_positional = required_positional
    @optional_keyword = optional_keyword
  end
end

UsingData = Data.define(:required_positional, :optional_keyword) do
  def initialize(required_positional:, optional_keyword: nil) = super(required_positional:, optional_keyword:)
end

class MixedPositionalAndKeywordTest < Minitest::Test
  [UsingPORO, UsingData].each do |klass|
    define_method "test_#{klass.name}.new('positional') works" do
      klass.new("positional")
    end

    define_method "test_#{klass.name}.new('positional', 'optional') raises ArgumentError" do
      assert_raises(ArgumentError) { klass.new("positional", "optional") }
    end

    define_method "test_#{klass.name}.new('positional', optional_keyword: 'optional') works" do
      klass.new("positional", optional_keyword: "optional")
    end
  end
end

$ ruby data.rb
Run options: --seed 26182

# Running:

F.E...

Finished in 0.000440s, 13636.3644 runs/s, 4545.4548 assertions/s.

  1) Failure:
MixedPositionalAndKeywordTest#test_UsingData.new('positional', 'optional') raises ArgumentError [data.rb:25]:
ArgumentError expected but nothing was raised.

  2) Error:
MixedPositionalAndKeywordTest#test_UsingData.new('positional', optional_keyword: 'optional') works:
ArgumentError: wrong number of arguments (given 2, expected 0)
    data.rb:29:in 'UsingData.new'
    data.rb:29:in 'block (2 levels) in <class:MixedPositionalAndKeywordTest>'

6 runs, 2 assertions, 1 failures, 1 errors, 0 skips

IMO given the simplicity of the API we're trying to offer, I think it's simplest to stick to POROs.

[kfischer-okarin]

@sambostock Ok, good reasonable points about not choosing the Data class approach - I can get behind that 👍🏻

It would be nice if the examples/docs were also updated to reflect the new API

I was wondering if it would also be a good idea to make resources and resource templates also take blocks as resource/read handler - so that all the features of the MCP server are consistent in their definition interface - (though that would be a slightly wider-scaled change since it also involves updating how the server handles things)

I also think it's okay to be not sooo careful about breaking changes since we're before v1.0.0 - the error message and the necessary rewrite are both straightforward enough. If you wanted to be a bit nicer, you could offer a little mini code rewrite example right inside the error message. Or give those examples in longer form in a Changelog entry and refer to that in the error message.

[sambostock]

I agree with all of the above. Incidentally, if merged, #55 would enforce the examples are updated.

I'll explore the resource changes you mentioned next week. 👍