migrate(CRDTAGY): translate credit agency batch (1..5)#30
Conversation
|
augment review |
|
✅ Test coverage looks good. The new/changed behavior in this PR has adequate test coverage. No additional tests needed. |
![augment-app-staging[bot]](https://avatars.githubusercontent.com/in/1008152?s=60&v=4){kind=small}
| continue; | ||
| } | ||
|
|
||
| Optional<Integer> maybeScore = future.get(remainingNanos, TimeUnit.NANOSECONDS); |
There was a problem hiding this comment.
Because the credit-agency futures are awaited sequentially up to a shared deadline, one slow/hung agency can consume the entire reply window and prevent collecting other agencies that already completed, which can incorrectly trigger the "credit check could not be completed" path.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
|
|
||
| private int awaitCreditScore(CrecustRequest request, int agencyNumber) { | ||
| try { | ||
| return creditAgencyService.requestCreditScore(request, agencyNumber).join().orElse(0); |
There was a problem hiding this comment.
awaitCreditScore() uses join() without any timeout/cancellation, so if requestCreditScore() stalls the HTTP request can block indefinitely (unlike the bounded reply window used by CrecustService).
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| } | ||
|
|
||
| int score = scoreGenerator.nextCreditScore(agency, request); | ||
| if (score < 1 || score > 999) { |
There was a problem hiding this comment.
The score bounds check allows 999, but creditAgencyScoreGenerator() is documented as producing 1..998 to mirror COBOL truncation; if 998 is the intended effective upper bound, allowing 999 here could let injected generators drift from that behavior.
Severity: low
🤖 Was this useful? React with 👍 or 👎
| continue; | ||
| } | ||
|
|
||
| Optional<Integer> maybeScore = future.get(remainingNanos, TimeUnit.NANOSECONDS); |
There was a problem hiding this comment.
This method now enforces a single overall reply deadline via deadlineNanos, but the TimeoutException comment below still describes a per-agency timeout; that mismatch could mislead future changes.
Severity: low
🤖 Was this useful? React with 👍 or 👎
…out; align score bound with COBOL [1..998]
|
augment review |
| boolean interrupted = false; | ||
| for (CompletableFuture<Optional<Integer>> future : futures) { | ||
| if (interrupted) { | ||
| if (!future.isDone() || future.isCompletedExceptionally() || future.isCancelled()) { |
There was a problem hiding this comment.
In evaluateCredit, a future can complete between the !future.isDone() check and the subsequent cancel(true) call, which can cause a valid score to be skipped even though it completed within the reply window. Consider handling the cancel() return value (or reading the result before cancelling) to avoid dropping just-completed replies.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
|
augment review |
| commarea.commDateOfBirth(), | ||
| creditScore, | ||
| defaultInt(commarea.commCsReviewDate()), | ||
| defaultString(commarea.commSuccess()), |
There was a problem hiding this comment.
On successful processing, CommSuccess / CommFailCode are currently echoed from the request (and defaulted), so callers can receive a credit score but still see a blank/incorrect success indicator. Consider populating these fields from the actual outcome (similar to other controllers) so the response consistently signals success/failure.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
1 participant
Source program
Mapped REST endpoints
POST /api/v1/crdtagy/{agencyNumber}whereagencyNumberis1..5Notable decisions
Test coverage
CreditAgencyServiceUnitTestCreditAgencyServiceIntegrationTestCrdtagyControllerWebMvcTestCrecustServiceUnitTest,CrecustServiceIntegrationTest