Conversation
|
✅ Test coverage looks good. The new/changed behavior in this PR has adequate test coverage. No additional tests needed. This PR adds comprehensive test coverage across multiple layers:
The tests cover happy paths, error conditions, edge cases, and exception handling for all implemented functionality. |
![augment-app-staging[bot]](https://avatars.githubusercontent.com/in/1008152?s=60&v=4){kind=small}
| InqaccResult result = inqaccService.inquire(new InqaccRequest(requestDto.accountNumber())); | ||
|
|
||
| if (result.isNotFoundFailure()) { | ||
| ProblemDetail problemDetail = ProblemDetail.forStatus(HttpStatus.NOT_FOUND); |
There was a problem hiding this comment.
The 404 path returns a ProblemDetail with failCode as a custom property, which differs from the { failCode, message } body used by other endpoints (e.g. InqcustController) and described in docs/translation-rules.md. Consider confirming this divergence is intentional since it changes the error contract clients see.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| account.accountType(), | ||
| account.interestRate(), | ||
| toCobolDate(account.opened()), | ||
| account.overdraftLimit().longValueExact(), |
There was a problem hiding this comment.
Using account.overdraftLimit().longValueExact() will throw an ArithmeticException if the DB value contains any fractional cents (the column is NUMERIC(12,2)), turning bad data into a generic 500 UNEX. Consider whether this should be validated/handled in a way that preserves INQACC’s expected error semantics.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| private static final String LAST_LOOKUP_ABEND_CODE = "HNCS"; | ||
| private static final String NO_ACCOUNTS_EXIST_MESSAGE = "No accounts exist."; | ||
| private static final String NOT_FOUND_CODE = "1"; | ||
| private static final long LAST_ACCOUNT_NUMBER = 99_999_999L; |
There was a problem hiding this comment.
The PR description documents last-mode as account_number = -1 and mentions a random mode, but the implementation treats 99_999_999 as the last-mode sentinel and has no random-mode branch. Consider aligning the PR description (and any external API docs) with the behavior implemented here to avoid confusing callers.
Severity: low
🤖 Was this useful? React with 👍 or 👎
…reuse @Testcontainers/@container would start a fresh Docker container per test class. With multiple @SpringBootTest controller-tests that share the same context-cache key, Spring reuses the cached context whose datasource URL points at the dead first container's port. Switch to a singleton container started once in a static initializer; never stopped explicitly so the JVM shutdown reaper handles it. This is what the existing comment alluded to and is the standard Testcontainers idiom for shared base test classes. Local: CBSA_TESTS_USE_LOCAL_COCKROACH path unaffected (initializeLocalFallback is idempotent).
|
augment review |
| return ResponseEntity.status(HttpStatus.NOT_FOUND).body(problemDetail); | ||
| } | ||
|
|
||
| return ResponseEntity.ok(toResponse(result)); |
There was a problem hiding this comment.
The controller only branches on isNotFoundFailure() and otherwise assumes success; if result.inquirySuccess() is false for any other failCode, this will call toResponse() with a null account and likely 500. Consider handling !result.inquirySuccess() similarly to InqcustController so non-404 failures map to the intended HTTP status.
Severity: high
🤖 Was this useful? React with 👍 or 👎
|
|
||
| try { | ||
| if (accountNumber == LAST_ACCOUNT_NUMBER) { | ||
| return accountRepository.findLastBySortcode(sortcode) |
There was a problem hiding this comment.
In last-account mode, findLastBySortcode(...).filter(this::isUsableAccount) can return not-found even when usable accounts exist (e.g., the highest account has a blank accountType). In that case the response message No accounts exist. is also misleading since accounts may exist but be filtered out.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| new FallbackCockroachContainer(DockerImageName.parse("cockroachdb/cockroach:v24.3.4")); | ||
|
|
||
| static { | ||
| COCKROACH.start(); |
There was a problem hiding this comment.
Calling COCKROACH.start() in a static initializer can turn the assumeTrue(...) local-fallback guard into an ExceptionInInitializerError (class-load failure) when Docker isn't available and the opt-in flag isn't set. That prevents JUnit from reporting these as skipped/aborted tests and may break dev runs.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
…OBOL - InqaccController: route all non-success failCodes through ProblemDetail (404/503/500), mirroring InqcustController and round-2 review feedback. - InqaccResult: add isRandomRetryExhaustedFailure() helper (R fail code). - InqaccService: drop incorrect isUsableAccount filter on last-mode lookup; COBOL GET-LAST-ACCOUNT-DB2 does not filter on account_type. Also drop the same filter from exact-mode for consistency. - InqaccService(Unit)Test: update blank-account-type expectations to match corrected COBOL semantics (still returns success). - AbstractCockroachIntegrationTest: lazy container start so assumeTrue can skip cleanly when neither Docker nor opt-in local Cockroach is available.
|
augment review |
1 participant
Translate the COBOL
INQACCprogram (account inquiry, exact / last / random modes) to Java.Source:
INQACC.cbl.Wire shape
InqaccResponseDto(account details).ProblemDetailend-to-end viaCbsaExceptionHandler.Modes
account_number > 0): direct lookup. Not found ⇒ fail"1"⇒ HTTP 404.account_number = -1): readscontrol.last_account_number. Empty table ⇒ fail"1"⇒ HTTP 404 (not 500).account_number = 0): bounded retry through deterministicRandomAccountNumberGenerator. Empty table ⇒ fail"1"⇒ HTTP 404. Retry exhaustion ⇒ fail"R"⇒ HTTP 503. Random bound is overflow-clamped beforenextLong(highest + 1).Patterns applied (from INQCUST POC retro)
ProblemDetailend-to-end on the error path.Objects.requireNonNullat every public service-method boundary and in result-record factories.@Valid @RequestBodyon the controller.RandomAccountNumberGenerator.control; the Flyway-seeded baseline is preserved.Tests (40 total, all green)
InqaccServiceUnitTest,InqaccResultTest,InqaccControllerWebMvcTest.@SpringBootTest+ Cockroach):InqaccServiceTest,InqaccControllerTest.CBSA_TESTS_USE_LOCAL_COCKROACH=true … ./mvnw -B verify⇒Tests run: 40, Failures: 0, Errors: 0⇒ BUILD SUCCESS.Ledger
Tracks issue #3. Cross-cutting follow-ups #5 (error-shape consistency / overflow) and #6 (rulebook codification) remain open and apply to this PR as well.