Repository navigation
fix: validate paging parameters of ListTasks - #115
Merged
Merged
Conversation
GET /tasks answered an expired or malformed page_token with 500 and accepted any page_size, wrapping around beyond the range of an int. - Kubernetes refuses a continue token with 410 (expired) or 400 (malformed); with a page_token, both now give 400 Bad Request with a message that names page_token, never its value. - page_size must be between 1 and 2048 (the maximum named by the TES 1.1 API description); other values give 400 Bad Request. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Describe the allowed page_size range and the 400 for an invalid
page_token in the API docs ("Listing tasks") and the upgrade notes.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Spring answered a page_size that is not a whole number with a message that repeats the value. Name the parameter instead, for all request parameters. Also take the page_size limit in the API description from the constant, share the Kubernetes stub of the wrapper tests, and say in the docs which page_token errors give 400. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewer's GuideAPIValidate
Define dedicated client-error exceptions for out-of-range page sizes and rejected page tokens.
Uses of the APITranslate Kubernetes expired or malformed continue-token responses into a sanitized page-token client error while preserving unrelated Kubernetes failures.
ImplementationReplace Spring type-mismatch error text for named request parameters so it identifies the parameter without echoing its invalid value.
DocumentationDocument the page-size range, invalid-token responses, and upgrade implications for clients using paging parameters.
TestsCover token-error classification in the Kubernetes wrapper, including unrelated and insufficiently described Kubernetes errors.
Test named and unnamed type-mismatch error handling and ListTasks paging validation at bounds, invalid input, and rejected tokens.
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #115 +/- ##
==========================================
+ Coverage 62.66% 63.24% +0.57%
==========================================
Files 64 66 +2
Lines 2510 2530 +20
Branches 259 262 +3
==========================================
+ Hits 1573 1600 +27
+ Misses 797 790 -7
Partials 140 140 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Codecov requires full patch coverage; the branch for type mismatches that are not about a named method argument had no test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
uniqueg
marked this pull request as ready for review
November 7, 2026 20:01
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/main/java/uk/ac/ebi/tsc/tesk/k8s/service/KubernetesClientWrapper.java" line_range="203" />
<code_context>
+ try {
+ result = this.listJobs(pageToken, labelSelector, itemsPerPage);
+ } catch (KubernetesException e) {
+ if (pageToken != null && isInvalidContinueToken(e.getApiException())) {
+ // a client error; the message names the parameter, never the token
+ throw new InvalidPageTokenException();
</code_context>
<issue_to_address>
**Other list errors look like bad tokens**
When a list request includes a page token and Kubernetes returns 400 for another parameter, such as a generated label selector or namespace, `isInvalidContinueToken` treats the 400 as an invalid token, so `listAllTaskmasterJobsForUser` throws `InvalidPageTokenException` and callers are told to restart paging even though the underlying request still fails.
Only convert a 400 to `InvalidPageTokenException` when the response identifies the continue token as the cause.
</issue_to_address>
### Comment 2
<location path="src/main/java/uk/ac/ebi/tsc/tesk/tes/api/TasksApiController.java" line_range="100-101" />
<code_context>
@RequestParam(value = "view", required = false, defaultValue = "MINIMAL")
String view) {
+ if (pageSize != null && (pageSize < 1 || pageSize > MAX_PAGE_SIZE)) {
+ throw new InvalidPageSizeException(MAX_PAGE_SIZE);
+ }
TesListTasksResponse response =
</code_context>
<issue_to_address>
**Empty page sizes bypass validation**
When a request explicitly supplies `page_size=`, spring converts an empty optional `Long` request parameter to `null`, so the range check is skipped and `TesService.listTasks` receives no limit; callers see an unbounded listing instead of a 400 for the supplied invalid value.
Distinguish an absent parameter from an explicitly empty value before converting it to `Long`, and reject the empty value with 400.
</issue_to_address>
### Comment 3
<location path="src/test/java/uk/ac/ebi/tsc/tesk/config/CustomErrorHandlerTest.java" line_range="37-39" />
<code_context>
+ HttpStatus.BAD_REQUEST,
+ new ServletWebRequest(request));
+
+ assertEquals(HttpStatus.BAD_REQUEST, response.getStatusCode());
+ assertNull(request.getAttribute(RequestDispatcher.ERROR_MESSAGE));
+ assertEquals(400, ((Map<?, ?>) response.getBody()).get("status"));
+ }
+}
</code_context>
<issue_to_address>
**Fallback message goes unchecked**
When the handler’s response message changes while its status and request attribute stay as asserted, when `handleTypeMismatch` returns an empty or altered message for a plain `TypeMismatchException` but keeps the 400 status and leaves `ERROR_MESSAGE` unset, `handleTypeMismatch_withoutParameterName` checks only the status and attribute, so it passes despite Spring’s fallback message being lost.
Assert that the response retains Spring’s default type-mismatch message.
</issue_to_address>Linked issues
#114 fix: validate paging parameters of ListTasks · 4 of 4 met
| # | Requirement | Status | Notes |
|---|---|---|---|
| 1 | Return 400 for an unusable page_token without echoing its value | ✅ Met | Kubernetes 410 and 400 responses are mapped to InvalidPageTokenException when a token was supplied; its message names page_token and omits the token. |
| 2 | Validate page_size as positive with a documented maximum; return 400 outside the range | ✅ Met | The controller enforces 1–2048 inclusive, and the API and upgrading docs describe the limit. |
| 3 | Add regression tests and update docs/api.md and docs/upgrading.md | ✅ Met | Tests cover invalid tokens, page-size bounds and type mismatches; both requested documentation files are updated. |
| 4 | Write regression tests first | ✅ Met | The PR description states the regression tests were written first and failed against dev. |
Sourcery assessment
Approval pending. 3 findings to address first.
Blocking findings: src/main/java/uk/ac/ebi/tsc/tesk/k8s/service/KubernetesClientWrapper.java:203, src/main/java/uk/ac/ebi/tsc/tesk/tes/api/TasksApiController.java:101, src/test/java/uk/ac/ebi/tsc/tesk/config/CustomErrorHandlerTest.java:39
Kubernetes answers a malformed continue token with 400 and a Status message about the continue key; other 400s of a list request (e.g. for a label selector) now stay server errors instead of being reported as an invalid page_token. Also check that type mismatches without a parameter name keep the message of Spring. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Linked issues
#114 fix: validate paging parameters of ListTasks · 2 of 3 met
| # | Requirement | Status | Notes |
|---|---|---|---|
| 1 | Return 400 for an invalid or expired page_token, with a message naming the parameter. | 🟡 Partly met | Kubernetes 410 responses and 400 responses whose body mentions continue become 400, but unusable tokens that Kubernetes rejects with a 5xx still result in 500. |
| 2 | Validate page_size as positive with a documented maximum, and return 400 outside that range. | ✅ Met | The controller enforces 1–2048 inclusive, and the API and upgrade docs state the limit. |
| 3 | Add regression tests and update docs/api.md and docs/upgrading.md. | ✅ Met | Tests cover invalid tokens and page-size boundaries and malformed values; both requested docs are updated. The PR description says the regression tests were written first. |
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.
Summary
GET /tasksnow validates its paging parameters:page_token(Kubernetes answers410 Gone, or400 Bad Requestwith a Status about the continue key; other 400s stay 500) gives400 Bad Requestinstead of500. The message namespage_tokenbut never its value; the Kubernetes message, which can repeat the token, is dropped.page_sizemust be between 1 and 2048 (inclusive); other numbers give400 Bad Request. Before, any number was accepted, and numbers beyond the range of anintwrapped around. 2048 sits in a constant that also feeds the API description.page_size=abc) still gives400, but the message now names the parameter instead of repeating the value (CustomErrorHandler.handleTypeMismatch). Todaypage_sizeis the only typed request parameter.docs/api.md("Listing tasks") anddocs/upgrading.md.Closes #114
Verification
dev(400 expected, 200 and 500 returned;KubernetesExceptioninstead of the new exception)../mvnw -B spotless:check checkstyle:check clean verify(Temurin 8): BUILD SUCCESS, 7329 unit tests and 53 integration tests, no failures.mkdocs build --strictlocally because the Material theme isn't installed. The Docs job in CI covers it.Declined review findings
KubernetesException.getApiException(): the constructor is private, and the only factory is always given the caughtApiException, so the value can't be null.500. Mapping 5xx to client errors would hide real cluster failures. This is documented indocs/api.md.page_size=2048is accepted, although the TES 1.1 text says "less than 2048". This follows the issue. Clients that follow the spec never send 2048, so accepting it doesn't hurt. The constant's comment says so.page_size=is treated as absent, like the other optional parameters ofGET /tasks(emptyname_prefixandpage_tokenalso mean "not given"); rejecting it would need parsing the raw parameter outside the Spring binding.ApiExceptiontoInvalidPageTokenException; controller: exception to 400 and message).Open questions
None.
🤖 Generated with Claude Code