Skip to content

fix: Remove unnecessary MapStruct conversion #503

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kabir wants to merge 1 commit into from
Closed

fix: Remove unnecessary MapStruct conversion #503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 15 additions & 19 deletions spec-grpc/src/main/java/io/a2a/grpc/mapper/SubscribeToTaskRequestMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package io.a2a.grpc.mapper;

import org.mapstruct.BeanMapping;
import org.mapstruct.Builder;
import org.mapstruct.Mapper;
import org.mapstruct.Mapping;

/**
* Mapper between {@link io.a2a.spec.SubscribeToTaskRequest} and {@link io.a2a.grpc.SubscribeToTaskRequest}.
Expand All @@ -11,9 +14,9 @@
* <li>Proto: Simple request with name field in format "tasks/{task_id}"</li>
* </ul>
* <p>
* Note: The domain object is a complete JSONRPC request, while the proto is just the gRPC
* request parameters. The JSONRPC envelope (id, jsonrpc, method) is handled separately
* by the transport layer.
* Uses {@link ResourceNameParser} to convert between task ID and resource name format.
* The domain JSONRPC envelope fields (id, jsonrpc, method) are populated with defaults
* by the Builder when converting from proto.
*/
@Mapper(config = A2AProtoMapperConfig.class, uses = {TaskIdParamsMapper.class})
public interface SubscribeToTaskRequestMapper {
Expand All @@ -27,28 +30,21 @@ public interface SubscribeToTaskRequestMapper {
* @param domain the domain SubscribeToTaskRequest
* @return the proto SubscribeToTaskRequest
*/
default io.a2a.grpc.SubscribeToTaskRequest toProto(io.a2a.spec.SubscribeToTaskRequest domain) {
if (domain == null || domain.getParams() == null || domain.getParams().id() == null) {
return null;
}
return io.a2a.grpc.SubscribeToTaskRequest.newBuilder()
.setName(ResourceNameParser.defineTaskName(domain.getParams().id()))
.build();
}
@Mapping(target = "name", expression = "java(ResourceNameParser.defineTaskName(domain.getParams().id()))")
io.a2a.grpc.SubscribeToTaskRequest toProto(io.a2a.spec.SubscribeToTaskRequest domain);
Comment on lines +33 to +34

@gemini-code-assist gemini-code-assist Bot Dec 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This refactoring introduces a regression. The original default method implementation correctly handled cases where domain.getParams() or domain.getParams().id() were null by returning null. This new implementation will throw a NullPointerException if domain.getParams() is null, or create an invalid resource name like "tasks/null" if id is null. To prevent this, it's safer to stick with the explicit checks in the default method.

    default io.a2a.grpc.SubscribeToTaskRequest toProto(io.a2a.spec.SubscribeToTaskRequest domain) {
        if (domain == null || domain.getParams() == null || domain.getParams().id() == null) {
            return null;
        }
        return io.a2a.grpc.SubscribeToTaskRequest.newBuilder()
                .setName(ResourceNameParser.defineTaskName(domain.getParams().id()))
                .build();
    }


/**
* Converts proto SubscribeToTaskRequest to domain SubscribeToTaskRequest.
* Extracts the task ID from the name field and creates a TaskIdParams.
* The JSONRPC envelope fields (id, jsonrpc, method) are populated with defaults by the Builder.
*
* @param proto the proto SubscribeToTaskRequest
* @return the domain SubscribeToTaskRequest
*/
default io.a2a.spec.SubscribeToTaskRequest fromProto(io.a2a.grpc.SubscribeToTaskRequest proto) {
if (proto == null || proto.getName() == null) {
return null;
}
return new io.a2a.spec.SubscribeToTaskRequest.Builder()
.params(new io.a2a.spec.TaskIdParams(ResourceNameParser.extractTaskId(proto.getName())))
.build();
}
@BeanMapping(builder = @Builder(buildMethod = "build"))
@Mapping(target = "params", expression = "java(new io.a2a.spec.TaskIdParams(ResourceNameParser.extractTaskId(proto.getName())))")
@Mapping(target = "id", ignore = true) // Builder sets UUID default
@Mapping(target = "jsonrpc", ignore = true) // Builder sets "2.0" default
@Mapping(target = "method", ignore = true) // Builder sets "SubscribeToTask" default
io.a2a.spec.SubscribeToTaskRequest fromProto(io.a2a.grpc.SubscribeToTaskRequest proto);
Comment on lines 36 to +49

@gemini-code-assist gemini-code-assist Bot Dec 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similar to toProto, this mapping introduces a regression. The original code returned null if proto.getName() was null, preventing errors. This new implementation will throw a NullPointerException. It's best to revert to the safer default method implementation, including the original Javadoc.

    /**
     * Converts proto SubscribeToTaskRequest to domain SubscribeToTaskRequest.
     * Extracts the task ID from the name field and creates a TaskIdParams.
     *
     * @param proto the proto SubscribeToTaskRequest
     * @return the domain SubscribeToTaskRequest
     */
    default io.a2a.spec.SubscribeToTaskRequest fromProto(io.a2a.grpc.SubscribeToTaskRequest proto) {
        if (proto == null || proto.getName() == null) {
            return null;
        }
        return new io.a2a.spec.SubscribeToTaskRequest.Builder()
                .params(new io.a2a.spec.TaskIdParams(ResourceNameParser.extractTaskId(proto.getName())))
                .build();
    }

}
4 changes: 4 additions & 0 deletions spec/src/main/java/io/a2a/spec/SubscribeToTaskRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ public SubscribeToTaskRequest(Object id, TaskIdParams params) {
this(null, id, METHOD, params);
}

public static Builder builder() {
return new Builder();
}

public static class Builder {
private String jsonrpc;
private Object id;
Expand Down
Loading