modelcontextprotocol / modelcontextprotocol/java-sdk

Un-deprecate/add no-arg builders

Đang mở
#1,065 0 bình luận 0 reaction 0 người được giao Xem trên GitHub

Chưa có ai nhận issue này.

enhancement needs confirmation P3
Ngôn ngữ chính
Java
Star
3.7k
Fork
1.1k
Merge trung bình
1 ngày 15 giờ
Pull request đã merge (30 ngày)
9

Mô tả

#928 deprecated no-arg builders for schema types, e.g. McpSchema.Resource

I'd like you to consider reversing that decision, and also to add no-arg versions for types without them (e.g. ProgressNotification).

I understand the motivation that builders not be left in an invalid state. It's worth noting that it might achieve that aim currently (I didn't audit all the classes), but if the schema ever has any fields which are mutually exclusive or conditionally required, then using constructors will only offer partial protection.

The problem is that it makes it undermines one of the main advantages of the builder pattern, which is to make construction clearer.

Here was my attempt to write a CreateMessageRequest with only required properties:

var request = McpSchema.CreateMessageRequest.builder(
        List.of(McpSchema.SamplingMessage.builder(
            McpSchema.Role.USER,
            McpSchema.TextContent.builder("Test Sampling Message").build()).build()
        ),
        50
    )
    .build();

Maybe there's a better way to format/indent this, but I tried several variations and I thought this was the best one.

Compare that with the same thing constructed with named properties

var request = McpSchema.CreateMessageRequest.builder()
    .messages(List.of(
        McpSchema.SamplingMessage.builder()
            .role(McpSchema.Role.USER)
            .content(McpSchema.TextContent.builder("Test Sampling Message").build())
            .build()
    ))
    .maxTokens(50)
    .build();

It's also worth noting that some builders have APIs like ModelPreferences#addHint. If that pattern were applied consistently, it could be simplified a little more by replacing messages(List.of( with addMessage(

Beyond readability

I'm writing a framework and builders enforcing all required params at once makes them inflexible for some possible API designs.

For example, my framework automatically manages progress tokens. I considered a design such as

void sendProgress(Consumer<McpSchema.ProgressNotification.Builder> consumer);

// an example caller. mcp creates the builder and applies the token
mcp.sendProgress(p -> p.progress(5).total(10));

However, it's not possible for the framework to implement this with the current builder methods. The user of the framework knows the progress value, the framework itself knows the progress token, but the builder expects both at once.

So the builder has to be worked around, for example to this

void sendProgress(double progress, Consumer<McpSchema.ProgressNotification.Builder> consumer);
// an example caller
mcp.sendProgress(5, p -> p.total(10));

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Hướng nghiên cứu

Bắt đầu với các điểm vào của McpSchema builder được nêu ở đây, bao gồm CreateMessageRequest, SamplingMessage, TextContent, ProgressNotification và ModelPreferences. So sánh các API có đối số bắt buộc và không có đối số, sau đó kiểm tra xem các giá trị schema có thể được cung cấp từng phần hay không, bao gồm cả các progress token do framework quản lý; hoàn tất khi cách tiếp cận builder nhất quán giữa các kiểu được yêu cầu.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
java
Lĩnh vực
api, backend-api-design
Loại issue
Tính năng
Độ khó
5/5
Thời gian dự kiến
Hơn một tuần
Mức độ hoạt động
Ít trao đổi
Độ rõ ràng
Khá rõ ràng
Mức phù hợp với người mới
42/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.