Conversation
|
augment review |
|
✅ Test coverage looks good. The new/changed behavior in this PR has adequate test coverage. No additional tests needed. What's covered:
The 7 tests provide solid coverage of the INQCUST translation from COBOL to Java. Nice work! 🎉 |
| } | ||
| } | ||
|
|
||
| return InqcustResult.failure(NOT_FOUND_CODE, RANDOM_CUSTOMER_NUMBER, "Unable to find a random customer."); |
There was a problem hiding this comment.
In random mode (customerNumber == 0), exhausting the retry loop returns fail code 1 which the controller maps to HTTP 404, even though no specific customer number was requested. This seems more like a backend failure and could cause clients to treat an internal selection failure as “not found”.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| } | ||
|
|
||
| long highestCustomerNumber = lastCustomer.get().customerNumber(); | ||
| for (int attempt = 0; attempt <= RANDOM_RETRY_LIMIT; attempt++) { |
There was a problem hiding this comment.
The retry loop condition attempt <= RANDOM_RETRY_LIMIT results in RANDOM_RETRY_LIMIT + 1 attempts. If RANDOM_RETRY_LIMIT is intended to be a strict cap, this is an off-by-one.
Severity: low
🤖 Was this useful? React with 👍 or 👎
| String message | ||
| ) { | ||
|
|
||
| public InqcustResult { |
There was a problem hiding this comment.
InqcustResult enforces the presence/absence of customer, but it still allows constructing a failure result with message == null. That can later blow up when building CbsaFailureResponse (which requires a non-null message).
Severity: low
🤖 Was this useful? React with 👍 or 👎
| public ResponseEntity<ProblemDetail> handleUnexpected(Exception exception) { | ||
| ProblemDetail problemDetail = ProblemDetail.forStatus(HttpStatus.INTERNAL_SERVER_ERROR); | ||
| problemDetail.setTitle("Unexpected error"); | ||
| problemDetail.setDetail(exception.getMessage() == null ? "An unexpected error occurred." : exception.getMessage()); |
There was a problem hiding this comment.
handleUnexpected() returns exception.getMessage() directly in the response body, which can leak internal details and varies by exception type. Consider returning a generic message for unexpected errors while logging the exception server-side.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| } | ||
| } | ||
|
|
||
| return InqcustResult.failure(NOT_FOUND_CODE, RANDOM_CUSTOMER_NUMBER, "Unable to find a random customer."); |
There was a problem hiding this comment.
This can return fail code 1 after exhausting random retries, which the controller maps to HTTP 404 even though customers may exist (the retry loop can miss existing customers when customer numbers are sparse). Consider whether random-mode failures should be distinguished from true “not found” results to avoid intermittent 404s.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| if ("1".equals(result.failCode())) { | ||
| return ResponseEntity.status(HttpStatus.NOT_FOUND).body(failureResponse); | ||
| } | ||
| return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(failureResponse); |
There was a problem hiding this comment.
This returns a { failCode, message } body for 500s produced via InqcustResult, but 500s coming from CbsaExceptionHandler (e.g., CbsaAbendException) will serialize as ProblemDetail, so callers may see different JSON schemas for the same endpoint/status. Consider whether the API should guarantee a single 500 error shape to keep client error handling predictable.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
- return retry-exhausted random lookups with a distinct R fail code and map them to 503 - cap random lookup retries at 1000 attempts and add regression coverage - require non-blank failure messages in InqcustResult - log unexpected exceptions server-side and return a generic internal error detail
|
augment review |
| } | ||
|
|
||
| public static InqcustResult success(CustomerDetails customer) { | ||
| return new InqcustResult(true, "0", customer.customerNumber(), customer, null); |
There was a problem hiding this comment.
InqcustResult.success() will throw a NullPointerException if called with a null customer (due to customer.customerNumber()), which is a less clear failure mode than the record's invariant checks.
Severity: low
🤖 Was this useful? React with 👍 or 👎
|
augment review |
|
|
||
| private static final class FallbackCockroachContainer extends CockroachContainer { | ||
|
|
||
| private static final String LOCAL_URL = "jdbc:postgresql://localhost:26257/cbsa?sslmode=disable"; |
There was a problem hiding this comment.
When Docker isn't available, this falls back to jdbc:postgresql://localhost:26257/cbsa...; since the tests delete from tables, it can accidentally wipe a developer's local cbsa database. Consider requiring an explicit opt-in (env var/profile) or using a dedicated test DB name for the fallback.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
|
|
||
| public InqcustResult inquire(InqcustRequest request) { | ||
| try { | ||
| long customerNumber = request.customerNumber(); |
There was a problem hiding this comment.
If inquire() is called with a null request, this will throw a NullPointerException before reaching the DataAccessException handling. Consider defensively null-checking the request at the method boundary to fail with a clearer error.
Severity: low
🤖 Was this useful? React with 👍 or 👎
| public ResponseEntity<ProblemDetail> handleAbend(CbsaAbendException exception) { | ||
| ProblemDetail problemDetail = ProblemDetail.forStatus(HttpStatus.INTERNAL_SERVER_ERROR); | ||
| problemDetail.setTitle("CBSA abend"); | ||
| problemDetail.setDetail(exception.getMessage()); |
There was a problem hiding this comment.
Returning exception.getMessage() in handleAbend() can expose internal details if upstream code includes sensitive context in the exception message. Consider returning a generic client-safe message (similar to handleUnexpected) while logging the full exception server-side.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
…t abend body - require explicit opt-in before using the local Cockroach fallback and move it to cbsa_test - null-check InqcustService request inputs with a clear message and cover it in unit tests - log/redact abend responses and extend MVC coverage for the client-safe error body
|
augment review |
| insertCustomer(10L, "First Customer", "1 Example Road", LocalDate.of(1980, 1, 2), (short) 120, LocalDate.of(2024, 5, 6)); | ||
| insertCustomer(25L, "Last Customer", "99 Example Road", LocalDate.of(1990, 7, 8), (short) 640, LocalDate.of(2025, 3, 4)); | ||
|
|
||
| InqcustResult result = inqcustService.inquire(new InqcustRequest(0L)); |
There was a problem hiding this comment.
This test exercises random-customer mode through the real ThreadLocalRandom, so it’s probabilistic and can (rarely) flake if the retry loop doesn’t hit an existing customer within RANDOM_RETRY_LIMIT.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
|
augment review |
| dsl.deleteFrom(ACCOUNT).execute(); | ||
| dsl.deleteFrom(PROCTRAN).execute(); | ||
| dsl.deleteFrom(CUSTOMER).execute(); | ||
| dsl.deleteFrom(CONTROL).execute(); |
There was a problem hiding this comment.
Deleting from CONTROL here removes the baseline control record that Flyway seeds, which can make later integration tests/programs that rely on that invariant fail in hard-to-debug ways.
Severity: medium
Other Locations
src/test/java/com/augment/cbsa/web/inqcust/InqcustControllerTest.java:36
🤖 Was this useful? React with 👍 or 👎
| return customerRepository.findLastBySortcode(sortcode) | ||
| .map(InqcustResult::success) | ||
| .orElseGet(() -> InqcustResult.failure( | ||
| BACKEND_FAILURE_CODE, |
There was a problem hiding this comment.
When no customers exist, the LAST_CUSTOMER_NUMBER branch returns BACKEND_FAILURE_CODE (9), which the controller maps to HTTP 500 even though this looks like a normal “no customer found” condition.
Severity: medium
Other Locations
src/main/java/com/augment/cbsa/service/InqcustService.java:74
🤖 Was this useful? React with 👍 or 👎
…not-found - keep the Flyway-seeded control baseline row intact in INQCUST integration tests - return not-found for empty-table random/last lookups and cover the 404/fail-code path in tests
|
augment review |
|
|
||
| @Bean | ||
| RandomCustomerNumberGenerator randomCustomerNumberGenerator() { | ||
| return highestCustomerNumber -> ThreadLocalRandom.current().nextLong(1, highestCustomerNumber + 1); |
There was a problem hiding this comment.
highestCustomerNumber + 1 can overflow if the DB ever contains a very large customer_number (there’s no DB constraint on the range), which would make ThreadLocalRandom.nextLong(...) throw and turn random lookups into 500s.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| @Max(9_999_999_999L) | ||
| long customerNumber | ||
| ) { | ||
| InqcustRequestDto requestDto = new InqcustRequestDto(customerNumber); |
There was a problem hiding this comment.
InqcustRequestDto is instantiated here but its validation annotations won’t be evaluated (it’s not a @Valid controller parameter), so the effective constraints are only the method-parameter annotations above.
Severity: low
🤖 Was this useful? React with 👍 or 👎
- Refactor InqcustController to return ProblemDetail on all error paths, matching every other controller (InqacccuController, CrecustController, CreaccController, ...). failCode moves to a ProblemDetail extension; the human-readable message moves to \$.detail. - Update InqcustControllerTest and InqcustControllerWebMvcTest assertions from \$.message to \$.detail accordingly. - Delete CbsaFailureResponse: the legacy error record had only one user and is now obsolete. - Add docs/translation-rules.md \xc2\xa713 codifying the seven recurring patterns from PR #4 (single error wire shape, defensive null-checks, @Valid on \@RequestBody, deterministic randomness, retry off-by-one, control-baseline preservation, empty-table -> 404). Add \xc2\xa714 translator checklist appendix. Closes #5 Closes #6 Co-authored-by: Augment Agent <agent@augmentcode.com>
Translates the COBOL
INQCUSTprogram to Java perdocs/translation-rules.md.What it does
INQCUSTis the read path that returns customer details by customer number. It supports three modes from the source program:1..99999999980returns a random existing customer9999999999returns the highest-numbered customer(The source program defines no
9999999998"first customer" mode, so none was implemented.)Structure (per rulebook)
domain/CustomerDetails,domain/InqcustRequest,domain/InqcustResult— Java recordsrepository/CustomerRepository— jOOQ-backed readsservice/InqcustService—@Servicewith the three-mode logicweb/inqcust/InqcustController—GET /api/v1/inqcust/{customerNumber}returning the response DTOweb/inqcust/dto/Inqcust{Request,Response,Date,Failure}Dto— wire recordserror/CbsaAbendException,error/CbsaExceptionHandler,error/CbsaFailureResponse— shared error model (will grow with later programs)Tests (per rulebook §9)
InqcustServiceTest— success, not-found, validation failure, plus invariant assertionInqcustControllerTest—@SpringBootTest(MOCK)+ MockMvc happy/sad/validation pathssupport/AbstractCockroachIntegrationTest— Testcontainers CockroachDB with a local-cockroach fallback for environments where Docker isn't available; wires JDBC coordinates via@DynamicPropertySource./mvnw -B verifyis green: 7 tests run, 0 failures.Source
Translated from
src/base/cobol_src/INQCUST.cbland copybooksINQCUST.cpy,CUSTOMER.cpy,SORTCODE.cpy.Follow-ups
AbstractCockroachIntegrationTestwill be reused by the next programs (INQACC,INQACCCU, ...). Expect them to evolve in those PRs.