fix: fixing the handling of historyLength being set to 0 by default in gRPC.

[ehsavoie]

• Using int instead of Integer and setting the default value to 0.
• If historyLength == 0 then we should return the full history
• Removing the handling of null

Issue: #423

Fixes #423 🦕

[gemini-code-assist]

Summary of Changes

Hello @ehsavoie, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses an issue where the historyLength parameter was not being handled consistently, particularly when set to 0 or null. The core change involves refactoring historyLength from a nullable Integer to a primitive int with a default value of 0. This ensures that a historyLength of 0 now explicitly requests the full history, rather than an empty one, and streamlines the logic by removing unnecessary null checks throughout the gRPC and REST transport layers, as well as the server's request handling.

Highlights

• Type Change: The historyLength parameter in TaskQueryParams has been changed from Integer (nullable) to int (primitive), with a default value of 0.
• Full History on Zero: When historyLength is 0, the system will now correctly return the full task history instead of an empty list.
• Null Handling Removal: All explicit null checks for historyLength have been removed across the codebase, simplifying logic.
• API and Transport Updates: Both gRPC and REST transport layers, along with the server-side request handler, have been updated to align with the new int type and 0 default behavior for historyLength.
• Test Adjustments: Corresponding unit tests have been updated to reflect the change from null to 0 when calling methods that accept historyLength.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request correctly refactors the handling of historyLength from a nullable Integer to a primitive int, with a default value of 0. This change is consistently applied across the codebase, including the TaskQueryParams record, gRPC and REST transports, and associated handlers and tests. The new logic, where historyLength = 0 signifies a request for the full task history, is correctly implemented in DefaultRequestHandler. I've added one comment to fix a pre-existing bug in the history truncation logic that is closely related to the changes in this PR. Overall, this is a good fix that improves clarity and correctness.

[ehsavoie]

@gemini-code-assist review

[gemini-code-assist]

@ehsavoie, thanks for tagging me! I've completed a detailed review of your pull request. Overall, the changes are well-implemented and align perfectly with the stated objective of handling historyLength consistently as a primitive int with 0 signifying full history. The refactoring is thorough across the client, server, and test code. Great job!

Here's a breakdown of my observations:

General Feedback

• Correctness: The core logic change where historyLength = 0 now correctly returns the full history (instead of an empty list) is implemented as described and is a significant improvement.
• Maintainability: By switching from Integer to int and removing null checks, the code is cleaner and less prone to NullPointerExceptions related to historyLength.
• Consistency: The changes are consistently applied across all affected layers (gRPC client, REST client, REST server, and tests).

Detailed Review Comments

client/transport/grpc/src/main/java/io/a2a/client/transport/grpc/GrpcTransport.java

• Removed Unused Imports: The removal of A2AServiceBlockingV2Stub, A2AServiceStub, FromProto, and ToProto imports is a good cleanup, assuming they are no longer used elsewhere in the file. (Severity: low)
• Direct historyLength Assignment: The change to directly set historyLength without a null check is correct following the type change. (Severity: none)

client/transport/rest/src/main/java/io/a2a/client/transport/rest/RestTransport.java

• Conditional historyLength Parameter: The update to if (taskQueryParams.historyLength() > 0) correctly ensures that the historyLength parameter is only appended to the URL when it's a positive value, allowing 0 to imply full history without explicitly sending historyLength=0. (Severity: none)

reference/rest/src/main/java/io/a2a/server/rest/quarkus/A2AServerRoutes.java

• historyLength Initialization and Parsing: Changing historyLength to int with a default of 0 and using Integer.parseInt is appropriate. However, it's worth noting that Integer.parseInt will throw a NumberFormatException if the history_length request parameter is present but not a valid integer string. While this is standard Java behavior, ensure that such exceptions are gracefully handled by the framework or an appropriate error handler to prevent unexpected server errors for malformed requests. (Severity: medium)

reference/rest/src/test/java/io/a2a/server/rest/quarkus/A2AServerRoutesTest.java

• Test Updates for anyInt(): The tests were correctly updated to use anyInt() for the historyLength parameter in mockRestHandler.getTask calls, reflecting the new primitive int type. (Severity: none)

server-common/src/main/java/io/a2a/server/requesthandlers/DefaultRequestHandler.java

• Unused Imports Removed: Good cleanup of Instance and TaskStateProvider imports. (Severity: low)
• Full History Logic: The change from history = new ArrayList<>(); to history = task.getHistory(); when params.historyLength() <= 0 is the key behavioral change and correctly implements the requirement for 0 to mean full history. (Severity: none)

spec/src/main/java/io/a2a/spec/TaskQueryParams.java

• Core Type Change: The fundamental change of historyLength from Integer to int in the TaskQueryParams record, along with the updated constructors and validation logic, is well-executed and forms the basis for the entire fix. (Severity: none)

transport/rest/src/main/java/io/a2a/transport/rest/handler/RestHandler.java

• Method Signature Update: The getTask method signature was correctly updated to accept int historyLength. (Severity: none)

transport/rest/src/test/java/io/a2a/transport/rest/handler/RestHandlerTest.java

• Test Parameter Updates: All test calls to handler.getTask were correctly updated to pass 0 instead of null for historyLength, ensuring the tests reflect the new behavior. (Severity: none)

This review was conducted based on common Java best practices, as no specific style guide was provided.

[kabir]

Can you find another name, we already have DefaultRequestHandlerTest so it overlaps a bit