Repository navigation
fix(rest): include missing query params in listTasks and listTaskPushNotificationConfigs - #1239
Open
sumitvairagar wants to merge 3 commits into
Open
sumitvairagar wants to merge 3 commits into
sumitvairagar wants to merge 3 commits into
Conversation
…NotificationConfigs RestTransport.buildListTasksQueryString() iterated all fields of ListTasksParams except statusTimestampAfter, which was silently dropped from every GET /tasks request, making the server-side timestamp filter unreachable from the Java client. RestTransport.listTaskPushNotificationConfigurations() built its URL as a plain string with no query parameters at all. The pageSize and pageToken fields on ListTaskPushNotificationConfigsParams were ignored, making pagination impossible. Fix: - Add statusTimestampAfter to buildListTasksQueryString(), formatted as ISO-8601 and URL-encoded (same pattern as existing string fields). - Extract buildListPushNotificationConfigsQueryString() for the push notification configs method and append the query string to the URL. - Add regression tests for both bugs using MockServer; each test asserts the query parameter is present in the outbound request. This fixes a2aproject#1233
1 task done
ehsavoie
requested changes
Oct 21, 2026
| if (request.includeArtifacts() != null && request.includeArtifacts()) { | ||
| builder.setIncludeArtifacts(true); | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
It is missing setting statusTimestampAfter
| checkNotNullParam("request", request); | ||
| org.a2aproject.sdk.grpc.ListTaskPushNotificationConfigsRequest.Builder builder | ||
| = org.a2aproject.sdk.grpc.ListTaskPushNotificationConfigsRequest.newBuilder(); | ||
| builder.setTaskId(request.id()); |
Collaborator
There was a problem hiding this comment.
It is missing setting pageSize and pageToken
Collaborator
There was a problem hiding this comment.
This should be URLEncoded
Three issues identified in review by @ehsavoie: 1. listTasks proto builder did not set statusTimestampAfter. Interceptors (auth, tracing) receive the builder before the REST URL is built, so missing fields there are invisible to the interceptor chain. Added the Timestamp conversion using getEpochSecond()/getNano(). 2. listTaskPushNotificationConfigurations proto builder set only taskId. pageSize and pageToken were never set, making pagination invisible to interceptors. Added the two missing fields. 3. status enum value in buildListTasksQueryString was appended raw without URL-encoding. Applied URLEncoder.encode() for consistency with all other string parameters in the same method. Added two regression tests using a capturing ClientCallInterceptor to assert the proto builder carries the correct fields before the HTTP request is sent. This fixes a2aproject#1233
Author
|
Thank you @ehsavoie — all three points addressed in the latest commit (e95a10a):
Also added two regression tests using a capturing |
…portTest - Restore java.util.List<String>/java.util.ArrayList<> FQN style in buildListTasksQueryString and buildListPushNotificationConfigsQueryString to match the pre-existing pattern in the original file - Replace org.a2aproject.sdk.spec.AgentCard FQN with short name in test interceptors (AgentCard is already imported) - Fix AgentInterface import to correct alphabetical position in test file This fixes a2aproject#1233
Author
|
The Windows CI failure ( |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
Two query parameters were silently dropped by the REST transport client, making server-side features unreachable from Java.
Bug 1 —
listTasksdropsstatusTimestampAfterbuildListTasksQueryString()handles every field inListTasksParamsexceptstatusTimestampAfter. The field was added to the record after the query builder was written and was never included. Every call toGET /tasksfrom the Java client omitted the timestamp filter entirely, so the server always returned all tasks regardless of when they were last updated.Bug 2 —
listTaskPushNotificationConfigurationsdropspageSizeandpageTokenThe URL was built as a plain string with no query parameters at all. The
pageSizeandpageTokenfields onListTaskPushNotificationConfigsParamswere never appended, making pagination impossible for push notification configs.What changed
statusTimestampAftertobuildListTasksQueryString(), formatted as ISO-8601 and URL-encoded (same pattern as existing string fields).buildListPushNotificationConfigsQueryString()for the push notification configs method and updatedlistTaskPushNotificationConfigurations()to append the query string to the URL.Fixes #1233