feat!: 553:Adding pagination to JpaDatabasePushNotificationConfigStore

[Sathyvs]

Description

• Added Pagination support to getInfo method in JpaDatabasePushNotificationConfigStore
  -- Follows the same pattern as that of JpaDatabaseTaskStore

• Updated sendNotification(Task) in BasePushNotificationSender to paginate results if a nextToken is present

Fixes #553 🦕

[gemini-code-assist]

Summary of Changes

Hello @Sathyvs, 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 enhances the push notification configuration storage and retrieval system by integrating robust pagination capabilities. The core change involves modifying the JpaDatabasePushNotificationConfigStore to support keyset pagination based on creation timestamps, enabling more scalable and performant fetching of configurations. Consequently, the BasePushNotificationSender has been adapted to leverage this new pagination, ensuring that all associated configurations are retrieved and processed, even when dealing with large datasets.

Highlights

• Pagination for Push Notification Configurations: Added comprehensive pagination support to the getInfo method within JpaDatabasePushNotificationConfigStore, allowing for efficient retrieval of push notification configurations using page size and page tokens. This aligns its behavior with the existing JpaDatabaseTaskStore.
• Timestamp-based Ordering: Introduced a createdAt timestamp field to JpaPushNotificationConfig entities, which is automatically set upon persistence. This timestamp is crucial for implementing keyset pagination, ensuring consistent and deterministic ordering of results.
• Pagination in Notification Sending: The BasePushNotificationSender has been updated to utilize the new pagination mechanism when fetching push notification configurations for a task. It now iteratively retrieves all configurations across multiple pages if a nextToken is present, ensuring all relevant notifications are processed.

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

The pull request successfully introduces pagination support for JpaDatabasePushNotificationConfigStore and updates BasePushNotificationSender to correctly handle paginated results. The implementation utilizes keyset pagination based on createdAt and configId, which is a robust and efficient approach. The addition of the createdAt field to JpaPushNotificationConfig and its automatic population via @PrePersist is well-executed. Test cases have been appropriately updated to reflect the new ordering and ensure deterministic results for timestamp-based pagination, including the introduction of Thread.sleep to guarantee unique timestamps. Overall, the changes are well-structured and address the pagination requirement effectively.

[gemini-code-assist]

The comment on lines 42-43 states that configId is a required field in the spec and should not be empty. However, the code defaults notificationConfig.id() to taskId if it's null or empty. This implies that if a client doesn't provide a unique id for a PushNotificationConfig, it will be assigned the taskId as its configId. If the intention is to allow multiple push notification configurations for a single task, each requiring a unique configId, this defaulting behavior could prevent that by creating duplicate configIds (all equal to taskId). Please clarify if this behavior is intentional for a default configuration or if configId should always be explicitly provided and unique.

[gemini-code-assist]

The comment on line 92 suggests that if the pageToken is invalid, the system should "start from the beginning." However, the current implementation for tokenParts.length != 2 (i.e., the pageToken is not in the expected timestamp:id format) silently ignores the pageToken and proceeds to fetch the first page. For better clarity and consistency with the NumberFormatException handling (lines 112-116), it would be more robust to explicitly throw an InvalidParamsError when the pageToken is malformed or does not adhere to the expected structure. This provides clearer feedback to the client about incorrect usage.

                // Keyset pagination: get tasks where timestamp < tokenTimestamp OR (timestamp = tokenTimestamp AND id > tokenId)
                // All tasks have timestamps (TaskStatus canonical constructor ensures this)
                queryBuilder.append(" AND (COALESCE(c.createdAt, :nullSentinel) < :tokenTimestamp OR (COALESCE(c.createdAt, :nullSentinel) = :tokenTimestamp AND c.id.configId > :tokenId))");
              } else {
                throw new io.a2a.spec.InvalidParamsError(null,
                    "Invalid pageToken format: pageToken must be in 'timestamp_millis:configId' format", null);
              }
            }

[Sathyvs]

@ehsavoie any suggestion on this about the default behavior ? I see many common public APIs ignore pagination token if they are invalid ?

[Sathyvs]

@ehsavoie I have a few questions

• do we need to update the InMemoryPushNotificationConfigStore in the same PR to follow the new pageToken format to use timestamp:config_id ?
• should the config_id be marked mandatory in the operations and assert for them ? currently task id is used as config id if its not present, which I believe would limit to have only one push notification config per task ?
• Also since getInfo method in the PushNotificationConfigStore returns a list, even the DefaultHandler onGetTaskPushNotificationConfig fetches the list to just pick one that matches the config id in the handler. Can we have a method created in the PushNotificationConfigStore to just get a single config based on the id ?

[kabir]

do we need to update the InMemoryPushNotificationConfigStore in the same PR to follow the new pageToken format to use timestamp:config_id ?

I don't think so. I think it is implementation dependent. So as long as the active store understands it, you should be ok

should the config_id be marked mandatory in the operations and assert for them ? currently task id is used as config id if its not present, which I believe would limit to have only one push notification config per task ?

Sorry not sure right now. Maybe Emmanuel will know on Monday, or I can look better then

Also since getInfo method in the PushNotificationConfigStore returns a list, even the DefaultHandler onGetTaskPushNotificationConfig fetches the list to just pick one that matches the config id in the handler. Can we have a method created in the PushNotificationConfigStore to just get a single config based on the id ?

Yes, that sounds reasonable.