feat!: Add pagination support to ListTaskPushNotificationConfig with Result wrapper #552

ehsavoie · 2025-12-18T15:07:08Z

Description

Implements pagination support for push notification config listing by introducing
ListTaskPushNotificationConfigResult record, following the same pattern as
ListTasksResult. This allows the API to return both the list of configs and a
nextPageToken for pagination.

Changes:
- Created ListTaskPushNotificationConfigResult record with configs list and nextPageToken
- Updated ListTaskPushNotificationConfigResponse to use Result type
- Updated all server handlers to return Result wrapper
- Updated ProtoUtils mappers for Result conversion
- Updated all transport implementations (REST, gRPC, JSONRPC) on both client and server
- Updated client interface and implementations
- Fixed test to use Result type

Updated all PushNotificationConfigStore implementations and tests to use the new
ListTaskPushNotificationConfigParams/ListTaskPushNotificationConfigResult API
instead of the old String-based getInfo method.

Changes to implementations:
- InMemoryPushNotificationConfigStore: Fixed bug where pageSize - 1 was used
instead of pageSize, causing one less item to be returned than requested
- JpaDatabasePushNotificationConfigStore: Implemented pagination logic matching
the in-memory implementation
- Added helper methods for pagination: findFirstIndex, convertPushNotificationConfig
- Updated all test files with getInfoAsList helper method to maintain compatibility

Thank you for opening a Pull Request!
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Follow the CONTRIBUTING Guide.
Make your Pull Request title in the https://www.conventionalcommits.org/ specification.
- Important Prefixes for release-please:
  - fix: which represents bug fixes, and correlates to a SemVer patch.
  - feat: represents a new feature, and correlates to a SemVer minor.
  - feat!:, or fix!:, refactor!:, etc., which represent a breaking change (indicated by the !) and will result in a SemVer major.
Ensure the tests pass
Appropriate READMEs were updated (if necessary)

Fixes #535 🦕

…esult wrapper Implements pagination support for push notification config listing by introducing ListTaskPushNotificationConfigResult record, following the same pattern as ListTasksResult. This allows the API to return both the list of configs and a nextPageToken for pagination. Changes: - Created ListTaskPushNotificationConfigResult record with configs list and nextPageToken - Updated ListTaskPushNotificationConfigResponse to use Result type - Updated all server handlers to return Result wrapper - Updated ProtoUtils mappers for Result conversion - Updated all transport implementations (REST, gRPC, JSONRPC) on both client and server - Updated client interface and implementations - Fixed test to use Result type Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>

gemini-code-assist · 2025-12-18T15:07:53Z

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 significantly enhances the push notification configuration listing functionality by integrating pagination support. This change allows clients to retrieve push notification configurations in manageable chunks, improving performance and scalability for tasks with a large number of associated configurations. The update involved introducing a new result record, modifying API interfaces, updating all client and server-side transport layers, and implementing the pagination logic within the data storage components.

Highlights

Pagination Support: Introduced pagination capabilities for listing push notification configurations, mirroring the existing pattern for ListTasksResult.
New Data Structure: A new record, ListTaskPushNotificationConfigResult, was created to encapsulate the list of configurations and a nextPageToken for pagination.
API and Client Updates: The listTaskPushNotificationConfigurations method in the client interface (AbstractClient, Client, ClientTransport) now returns the new ListTaskPushNotificationConfigResult type.
Transport Layer Integration: All transport implementations (REST, gRPC, JSONRPC) have been updated to handle the new pagination parameters (pageSize, pageToken) and return the ListTaskPushNotificationConfigResult.
Storage Layer Enhancements: Both InMemoryPushNotificationConfigStore and JpaDatabasePushNotificationConfigStore were refactored to implement the pagination logic, including handling pageSize and pageToken.
Bug Fix and Helper Methods: A bug in InMemoryPushNotificationConfigStore related to pageSize - 1 was fixed, and helper methods (findFirstIndex, convertPushNotificationConfig) were added to facilitate pagination logic.
Comprehensive Testing: Extensive new test cases were added to JpaDatabasePushNotificationConfigStoreIntegrationTest and InMemoryPushNotificationConfigStoreTest to validate various pagination scenarios, including page size, page tokens, empty results, and full iteration.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

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.

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 introduces pagination support for listing task push notification configurations, which is a great enhancement for the API. The changes are comprehensive, touching all layers from the spec and client to the server handlers and storage implementations. The introduction of ListTaskPushNotificationConfigResult is a clean way to handle paginated results.

My main feedback is on the JpaDatabasePushNotificationConfigStore implementation, where the pagination is currently handled in-memory after fetching all records. This could lead to performance issues with large datasets. I've left a detailed comment with suggestions to move the pagination logic to the database query level for better scalability and to ensure deterministic ordering.

kabir

A few small issues, which would be good to look at :-)

...ificationconfigstore/database/jpa/JpaDatabasePushNotificationConfigStoreIntegrationTest.java

.../a2a/extras/pushnotificationconfigstore/database/jpa/JpaPushNotificationConfigStoreTest.java

server-common/src/main/java/io/a2a/server/tasks/PushNotificationConfigStore.java

kabir · 2025-12-18T16:15:09Z

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


-        List<PushNotificationConfig> pushNotificationConfigList = pushConfigStore.getInfo(params.id());
-        if (pushNotificationConfigList == null || pushNotificationConfigList.isEmpty()) {
+        ListTaskPushNotificationConfigResult listTaskPushNotificationConfigResult = pushConfigStore.getInfo(new ListTaskPushNotificationConfigParams(params.id()));


Wrapping this in params here feels a bit odd. Maybe we could keep the original method and make that handle the wrapping if needed?

the issue is i need to be able to return the new token too, so returning the list is no longer sufficient

server-common/src/test/java/io/a2a/server/tasks/InMemoryPushNotificationConfigStoreTest.java

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

…ginated API Updated all PushNotificationConfigStore implementations and tests to use the new ListTaskPushNotificationConfigParams/ListTaskPushNotificationConfigResult API instead of the old String-based getInfo method. Changes to implementations: - InMemoryPushNotificationConfigStore: Fixed bug where pageSize - 1 was used instead of pageSize, causing one less item to be returned than requested - JpaDatabasePushNotificationConfigStore: Implemented pagination logic matching the in-memory implementation - Added helper methods for pagination: findFirstIndex, convertPushNotificationConfig - Updated all test files with getInfoAsList helper method to maintain compatibility Comprehensive pagination tests added (20 new tests total): - InMemoryPushNotificationConfigStoreTest: 10 pagination tests - JpaDatabasePushNotificationConfigStoreIntegrationTest: 10 pagination tests Test coverage includes: - Basic pagination with pageSize - Multi-page navigation using pageToken - Last page handling (no nextPageToken) - Edge cases: zero/negative pageSize, pageSize larger than available items - Exact pageSize match - Invalid token handling - Empty task pagination - Full iteration across all pages - Explicit overlap detection (verifies no config appears in multiple pages) Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>

@param

- Fix @param error in PushNotificationConfigStore (blocking javadoc build) - Add complete javadoc to EventQueue and all public/protected members - Add javadoc to EventEnqueueHook, EventQueueClosedException - Add javadoc to TaskStore, JSONRPCException, NoTaskQueueException - Add javadoc to InMemoryTaskStore, Internal annotation - Fix typo in PushNotificationConfigStore ("emty" -> "empty") Reduces javadoc errors from 1 to 0 and warnings from 101 to 100. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

kabir · 2025-12-18T18:43:46Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces pagination support for listing task push notification configurations across the client, transport, and server components. The primary change involves updating the return type of listTaskPushNotificationConfigurations methods from List<TaskPushNotificationConfig> to a new ListTaskPushNotificationConfigResult record, which encapsulates both the list of configurations and a nextPageToken for pagination. The ListTaskPushNotificationConfigParams record was also updated to include pageSize and pageToken fields. Pagination logic was implemented in the JpaDatabasePushNotificationConfigStore and InMemoryPushNotificationConfigStore, using the PushNotificationConfig.token() field as the pagination cursor. Corresponding changes were made to client transports (gRPC, JSON-RPC, REST), request handlers, and numerous test cases to reflect the new return type and pagination parameters. A review comment highlighted a potential design flaw in using PushNotificationConfig.token() for pagination, suggesting that a config ID or index would be more appropriate, and also pointed out minor code style issues with extra spaces in a compact constructor declaration.

gemini-code-assist · 2025-12-18T18:47:03Z

server-common/src/main/java/io/a2a/server/tasks/InMemoryPushNotificationConfigStore.java

+    public ListTaskPushNotificationConfigResult getInfo(ListTaskPushNotificationConfigParams params) {
+        List<PushNotificationConfig> configs = pushNotificationInfos.get(params.id());
+        if (configs == null) {
+            return new ListTaskPushNotificationConfigResult(Collections.emptyList());
+        }
+        if (params.pageSize() <= 0) {
+            return new ListTaskPushNotificationConfigResult(convertPushNotificationConfig(configs, params), null);
+        }
+        if (params.pageToken() != null && !params.pageToken().isBlank()) {
+            //find first index
+            int index = findFirstIndex(configs, params.pageToken());
+            if (index < configs.size()) {
+                configs = configs.subList(index, configs.size());
+            }
+        }
+        if (configs.size() <= params.pageSize()) {
+            return new ListTaskPushNotificationConfigResult(convertPushNotificationConfig(configs, params), null);
+        }
+        String newToken = configs.get(params.pageSize()).token();
+        return new ListTaskPushNotificationConfigResult(convertPushNotificationConfig(configs.subList(0, params.pageSize()), params), newToken);
+    }


The pagination implementation uses PushNotificationConfig.token() to generate the nextPageToken. The token field is designed for webhook authentication, not for pagination. Using it as a pagination cursor is a design flaw and could be misleading.

For an in-memory store, a more appropriate pagination token would be based on the item's index or another unique, sortable attribute like the config ID. This would make the pagination logic clearer and avoid misusing the authentication token field.

For example, you could use the config ID as the token:

String newToken = configs.get(params.pageSize()).id();

And then update findFirstIndex to search by ID.

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

…arams.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

…Result wrapper (a2aproject#552) # Description Implements pagination support for push notification config listing by introducing ListTaskPushNotificationConfigResult record, following the same pattern as ListTasksResult. This allows the API to return both the list of configs and a nextPageToken for pagination. Changes: - Created ListTaskPushNotificationConfigResult record with configs list and nextPageToken - Updated ListTaskPushNotificationConfigResponse to use Result type - Updated all server handlers to return Result wrapper - Updated ProtoUtils mappers for Result conversion - Updated all transport implementations (REST, gRPC, JSONRPC) on both client and server - Updated client interface and implementations - Fixed test to use Result type Updated all PushNotificationConfigStore implementations and tests to use the new ListTaskPushNotificationConfigParams/ListTaskPushNotificationConfigResult API instead of the old String-based getInfo method. Changes to implementations: - InMemoryPushNotificationConfigStore: Fixed bug where pageSize - 1 was used instead of pageSize, causing one less item to be returned than requested - JpaDatabasePushNotificationConfigStore: Implemented pagination logic matching the in-memory implementation - Added helper methods for pagination: findFirstIndex, convertPushNotificationConfig - Updated all test files with getInfoAsList helper method to maintain compatibility Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly: - [X] Follow the [`CONTRIBUTING` Guide](../CONTRIBUTING.md). - [X] Make your Pull Request title in the <https://www.conventionalcommits.org/> specification. - Important Prefixes for [release-please](https://github.com/googleapis/release-please): - `fix:` which represents bug fixes, and correlates to a [SemVer](https://semver.org/) patch. - `feat:` represents a new feature, and correlates to a SemVer minor. - `feat!:`, or `fix!:`, `refactor!:`, etc., which represent a breaking change (indicated by the `!`) and will result in a SemVer major. - [X] Ensure the tests pass - [X] Appropriate READMEs were updated (if necessary) Fixes a2aproject#535 🦕 --------- Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Co-authored-by: Kabir Khan <kkhan@redhat.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

ehsavoie changed the title ~~Issue 535~~ feat!: Add pagination support to ListTaskPushNotificationConfig with Result wrappe Dec 18, 2025

ehsavoie changed the title ~~feat!: Add pagination support to ListTaskPushNotificationConfig with Result wrappe~~ feat!: Add pagination support to ListTaskPushNotificationConfig with Result wrapper Dec 18, 2025

gemini-code-assist bot reviewed Dec 18, 2025

View reviewed changes

kabir requested changes Dec 18, 2025

View reviewed changes

ehsavoie force-pushed the issue_535 branch 2 times, most recently from 723027a to b070cbf Compare December 18, 2025 17:26

ehsavoie and others added 2 commits December 18, 2025 18:33

ehsavoie force-pushed the issue_535 branch from b070cbf to 67063da Compare December 18, 2025 17:34

ehsavoie requested a review from kabir December 18, 2025 17:55

gemini-code-assist bot reviewed Dec 18, 2025

View reviewed changes

Update spec/src/main/java/io/a2a/spec/ListTaskPushNotificationConfigP…

f48a9a9

…arams.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

kabir merged commit 6038ee2 into a2aproject:main Dec 18, 2025
11 checks passed

ehsavoie deleted the issue_535 branch December 19, 2025 16:23

feat!: Add pagination support to ListTaskPushNotificationConfig with Result wrapper #552

feat!: Add pagination support to ListTaskPushNotificationConfig with Result wrapper #552

Uh oh!

Conversation

ehsavoie commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

gemini-code-assist bot commented Dec 18, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

kabir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kabir Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

ehsavoie Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kabir commented Dec 18, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ehsavoie commented Dec 18, 2025 •

edited

Loading