-
Notifications
You must be signed in to change notification settings - Fork 106
chore: 553: Addressed PR comments from Pull request 564. #599
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| # A2A JPA Database Push Notification Config Store Default Configuration | ||
|
|
||
| # Maximum page size when listing push notification configurations for a task | ||
| # Requested page sizes exceeding this value will be capped to this limit | ||
| # Used as default when pageSize parameter is not specified or is invalid | ||
| a2a.push-notification-config.max-page-size=100 | ||
|
|
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |||||
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||||||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||||||
|
|
||||||
| import io.a2a.spec.InvalidParamsError; | ||||||
| import java.util.List; | ||||||
| import java.util.Queue; | ||||||
| import java.util.concurrent.CountDownLatch; | ||||||
|
|
@@ -293,7 +294,7 @@ public void testPaginationWithZeroPageSize() { | |||||
| ListTaskPushNotificationConfigResult result = pushNotificationConfigStore.getInfo(params); | ||||||
|
|
||||||
| assertNotNull(result); | ||||||
| assertEquals(5, result.configs().size(), "Should return all 5 configs when pageSize=0"); | ||||||
| assertEquals(5, result.configs().size(), "Should return all default capped 5 configs when pageSize=0"); | ||||||
| assertNull(result.nextPageToken(), "Should not have nextPageToken when returning all"); | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -355,12 +356,10 @@ public void testPaginationWithInvalidToken() { | |||||
| // Request with invalid pageToken - JPA implementation behavior is to start from beginning | ||||||
| ListTaskPushNotificationConfigParams params = new ListTaskPushNotificationConfigParams( | ||||||
| taskId, 2, "invalid_token_that_does_not_exist", ""); | ||||||
| ListTaskPushNotificationConfigResult result = pushNotificationConfigStore.getInfo(params); | ||||||
|
|
||||||
| assertNotNull(result); | ||||||
| // When token is not found, implementation starts from beginning | ||||||
| assertEquals(2, result.configs().size(), "Should return first page when token is not found"); | ||||||
| assertNotNull(result.nextPageToken(), "Should have nextPageToken since more items exist"); | ||||||
| assertThrows(InvalidParamsError.class, () -> | ||||||
| pushNotificationConfigStore.getInfo(params), | ||||||
| "Invalid pageToken format: pageToken must be in 'timestamp_millis:configId' format"); | ||||||
| } | ||||||
|
|
||||||
| @Test | ||||||
|
|
@@ -428,12 +427,10 @@ public void testPageTokenWithMissingColon() { | |||||
|
|
||||||
| ListTaskPushNotificationConfigParams params = | ||||||
| new ListTaskPushNotificationConfigParams(taskId, 2, "123456789cfg1", ""); | ||||||
| ListTaskPushNotificationConfigResult result = pushNotificationConfigStore.getInfo(params); | ||||||
|
|
||||||
| assertNotNull(result); | ||||||
| assertEquals(2, result.configs().size(), | ||||||
| "Should return first page when pageToken format is invalid (missing colon)"); | ||||||
| assertNotNull(result.nextPageToken(), "Should have nextPageToken since more items exist"); | ||||||
| assertThrows(InvalidParamsError.class, () -> | ||||||
| pushNotificationConfigStore.getInfo(params), | ||||||
| "Invalid pageToken format: pageToken must be in 'timestamp_millis:configId' format"); | ||||||
| } | ||||||
|
|
||||||
| @Test | ||||||
|
|
@@ -552,6 +549,47 @@ public void testPaginationOrderingConsistency() { | |||||
| assertEquals("cfg0", allConfigIds.get(14), | ||||||
| "Last config should be oldest created"); | ||||||
| } | ||||||
|
|
||||||
| @Test | ||||||
| @Transactional | ||||||
| public void testPageSizeExceedingConfiguredMaxLimit() { | ||||||
| String taskId = "task_max_page_size_" + System.currentTimeMillis(); | ||||||
| // Create 5 configs (more than test max page size of 5) | ||||||
| createSamples(taskId, 7); | ||||||
|
|
||||||
| // Request with pageSize=7 (exceeds configured max of 5 in test application.properties) | ||||||
| // Should be capped to maxPageSize (5) from config | ||||||
| ListTaskPushNotificationConfigParams params = | ||||||
| new ListTaskPushNotificationConfigParams(taskId, 7, "", ""); | ||||||
| ListTaskPushNotificationConfigResult result = pushNotificationConfigStore.getInfo(params); | ||||||
|
|
||||||
| assertNotNull(result); | ||||||
| // Should return 5 configs (capped to maxPageSize from test config), not 7 | ||||||
| assertEquals(5, result.configs().size(), | ||||||
| "Page size should be capped to configured maxPageSize (5 in tests) when requested size exceeds limit"); | ||||||
| assertNotNull(result.nextPageToken(), | ||||||
| "Should have nextPageToken since more configs remain"); | ||||||
|
|
||||||
| // Verify we can iterate through all pages and get all 7 configs | ||||||
| List<String> allConfigIds = new java.util.ArrayList<>(); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fully qualified name
Suggested change
|
||||||
| result.configs().forEach(c -> allConfigIds.add(c.pushNotificationConfig().id())); | ||||||
|
|
||||||
| String nextToken = result.nextPageToken(); | ||||||
| while (nextToken != null) { | ||||||
| ListTaskPushNotificationConfigParams nextParams = | ||||||
| new ListTaskPushNotificationConfigParams(taskId, 7, nextToken, ""); | ||||||
| ListTaskPushNotificationConfigResult nextResult = pushNotificationConfigStore.getInfo(nextParams); | ||||||
|
|
||||||
| nextResult.configs().forEach(c -> allConfigIds.add(c.pushNotificationConfig().id())); | ||||||
| nextToken = nextResult.nextPageToken(); | ||||||
| } | ||||||
|
|
||||||
| assertEquals(7, allConfigIds.size(), | ||||||
| "Should retrieve all 7 configs across multiple pages"); | ||||||
| assertEquals(7, new java.util.HashSet<>(allConfigIds).size(), | ||||||
| "All config IDs should be unique - no duplicates"); | ||||||
| } | ||||||
|
|
||||||
| private void createSamples(String taskId, int size) { | ||||||
| // Create configs with slight delays to ensure unique timestamps for deterministic ordering | ||||||
| for (int i = 0; i < size; i++) { | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| # A2A SDK Default Configuration Values | ||
| # These values are used when no other configuration source provides them | ||
|
|
||
| # DefaultRequestHandler - Blocking call timeouts | ||
| # Timeout for agent execution to complete (seconds) | ||
| # Increase for slow agents: LLM-based, data processing, external APIs | ||
| a2a.blocking.agent.timeout.seconds=30 | ||
|
|
||
| # Timeout for event consumption/persistence to complete (seconds) | ||
| # Ensures TaskStore is fully updated before returning to client | ||
| a2a.blocking.consumption.timeout.seconds=5 | ||
|
|
||
| # AsyncExecutorProducer - Thread pool configuration | ||
| # Core pool size for async agent execution | ||
| a2a.executor.core-pool-size=5 | ||
|
|
||
| # Maximum pool size for async agent execution | ||
| a2a.executor.max-pool-size=50 | ||
|
|
||
| # Keep-alive time for idle threads (seconds) | ||
| a2a.executor.keep-alive-seconds=60 | ||
|
|
||
| # A2A JPA Database Push Notification Config Store Default Configuration | ||
|
|
||
| # A2A Configuration - Override max page size for testing | ||
| # Set to a lower value (2) to make tests faster and verify capping behavior | ||
| a2a.push-notification-config.max-page-size=5 | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catching the generic
Exceptionis overly broad and can hide other unexpected issues. It's better to catch the specific exceptions you expect, such asIllegalArgumentException(if the config value is missing from the provider) andNumberFormatException(if the value is not a valid integer). This makes the error handling more precise and robust.