Conversation
|
augment review |
|
✅ Test coverage looks good. The new/changed behavior in this PR has adequate test coverage. No additional tests needed. This PR includes:
All major code paths, edge cases, and error scenarios are properly tested. |
| .forUpdate() | ||
| .fetchOne(); | ||
| } catch (DataAccessException exception) { | ||
| throw rollbackFailure("3", "Unable to reserve the next customer number."); |
There was a problem hiding this comment.
Catching DataAccessException here and converting it into a business failure will also swallow Cockroach serialization failures (SQLSTATE 40001), preventing CrdbRetry.run(...) from retrying the transaction under contention.
Severity: high
Other Locations
src/main/java/com/augment/cbsa/repository/CrecustRepository.java:95src/main/java/com/augment/cbsa/repository/CrecustRepository.java:108src/main/java/com/augment/cbsa/repository/CrecustRepository.java:125
🤖 Was this useful? React with 👍 or 👎
| int returnedScores = 0; | ||
| for (CompletableFuture<Optional<Integer>> future : futures) { | ||
| try { | ||
| Optional<Integer> maybeScore = future.join(); |
There was a problem hiding this comment.
Using future.join() with no timeout means one stuck credit-agency call can hang the entire request even though individual failures are otherwise tolerated.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| customer.creditScore(), | ||
| toCobolDate(customer.csReviewDate()), | ||
| "Y", | ||
| "" |
There was a problem hiding this comment.
The success response sets CommFailCode to an empty string, but this field looks like a 1-character commarea flag (often a space when unused); returning "" may break clients expecting a single-char value.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| void createsCustomerAuditTrailAndControlRow() { | ||
| when(reviewDateRandom.nextInt(20)).thenReturn(3); | ||
|
|
||
| LocalDate today = LocalDate.now(java.time.Clock.systemUTC()); |
There was a problem hiding this comment.
This test computes today using Clock.systemUTC() separately from the service's injected Clock, so it can flake if the date rolls over between the two calls (e.g., around midnight).
Severity: low
🤖 Was this useful? React with 👍 or 👎
| int returnedScores = 0; | ||
| for (CompletableFuture<Optional<Integer>> future : futures) { | ||
| try { | ||
| Optional<Integer> maybeScore = future.join(); |
There was a problem hiding this comment.
evaluateCredit() uses future.join() without any timeout handling; if a credit-agency call never completes, the request can block indefinitely and never return fail code G. Consider ensuring credit checks complete within a bounded time.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| } | ||
|
|
||
| private String firstToken(String name) { | ||
| if (name.isEmpty() || Character.isWhitespace(name.charAt(0))) { |
There was a problem hiding this comment.
firstToken() returns "" when the name begins with whitespace, which means " Alice Example" bypasses title validation (empty title is allowed) and will be treated as valid. This looks like an input-validation bypass that could allow invalid titles/names to be persisted.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| } | ||
|
|
||
| private CrecustResult invalidDateFailure(int cobolDate) { | ||
| int year = Integer.parseInt(String.format("%08d", cobolDate).substring(4)); |
There was a problem hiding this comment.
invalidDateFailure() derives the year via String.format(...).substring(4) and Integer.parseInt(...); for negative cobolDate values this can throw NumberFormatException and turn a validation issue into a 500. Even if the web layer validates non-negative dates, this method is reachable from direct service calls/tests.
Severity: low
🤖 Was this useful? React with 👍 or 👎
| ) { | ||
|
|
||
| private static final Set<String> VALIDATION_FAIL_CODES = Set.of("T", "O", "Y", "Z"); | ||
| private static final Set<String> CREDIT_FAIL_CODES = Set.of("A", "B", "C", "D", "E", "F", "G", "H"); |
There was a problem hiding this comment.
CREDIT_FAIL_CODES includes E, F, and H, which appear non-credit-related and are treated as “credit failures” (HTTP 503 / title “Credit check unavailable”) via isCreditFailure(). If those codes are ever returned for persistence/audit failures, clients will receive a misleading 503 rather than an internal error.
Severity: medium
Other Locations
src/main/java/com/augment/cbsa/web/crecust/CrecustController.java:58src/main/java/com/augment/cbsa/web/crecust/CrecustController.java:77
🤖 Was this useful? React with 👍 or 👎
… recategorize fail codes - evaluateCredit: future.get(timeout) per agency so a hung credit-agency call cannot block the request indefinitely; cancel on timeout/interrupt. - firstToken: stripLeading before tokenizing so a name with leading whitespace cannot bypass title validation by appearing to have an empty (no-title) prefix. - invalidDateFailure: derive year via abs(cobolDate) % 10000 so negative or out-of-range cobolDate inputs cannot turn a validation issue into a 500. - CrecustResult.CREDIT_FAIL_CODES: drop E (customer READ), F (customer REWRITE) and H (proctran write) — those are persistence failures and must fall through to 5xx, not be reported as 503 "Credit check unavailable". Mirror the change in CrecustController.failureTitle.
|
augment review |
| return CrecustResult.failure("G", "Credit check could not be completed."); | ||
| } | ||
|
|
||
| Instant now = Instant.now(clock); |
There was a problem hiding this comment.
create() reads the injected clock multiple times (e.g., LocalDate.now(clock) and later Instant.now(clock)), so around midnight it can validate against one day but persist transactionDate/reviewDate using the next day. Consider deriving today/reviewDate from the same captured instant to avoid rare off-by-one-day behavior.
Severity: low
Other Locations
src/main/java/com/augment/cbsa/service/CrecustService.java:130src/main/java/com/augment/cbsa/service/CrecustService.java:175
🤖 Was this useful? React with 👍 or 👎
| } catch (ExecutionException | CompletionException exception) { | ||
| // Ignore individual credit-agency failures and average only successful replies. | ||
| } catch (InterruptedException exception) { | ||
| Thread.currentThread().interrupt(); |
There was a problem hiding this comment.
In evaluateCredit(), after catching InterruptedException the thread is re-interrupted but processing continues; if some scores were already collected, the request can still proceed to create a customer even though cancellation was signaled. Consider treating interruption as an immediate overall credit-check failure (or propagating) to avoid persisting work on interrupted threads.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| @@ -0,0 +1,4 @@ | |||
| CREATE SEQUENCE IF NOT EXISTS proctran_counter_seq; | |||
There was a problem hiding this comment.
This migration creates proctran_counter_seq but doesn’t initialize it to at least max(proctran.counter)+1, so applying it to a database with existing proctran rows can cause default counter values to collide with existing primary keys. Consider bumping/initializing the sequence during migration to make it safe on non-empty environments.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
|
|
||
| private String toDescription(CustomerDetails customer) { | ||
| return customer.sortcode() | ||
| + String.format("%010d", customer.customerNumber()) |
There was a problem hiding this comment.
toDescription() assumes customerNumber fits in 10 digits via %010d; if customerNumber ever exceeds 9_999_999_999, the formatted string will exceed the intended 40-char proctran.description layout and may fail inserts. Consider enforcing the 10-digit bound when allocating customer numbers to keep the audit description length stable.
Severity: low
🤖 Was this useful? React with 👍 or 👎
…quence init, customer-number bound - CrecustService.create: capture Instant/today once at the start and thread today through validateDateOfBirth and evaluateCredit so all date/time fields derive from the same instant. Avoids rare off-by-one-day audit records when the request straddles midnight. - CrecustService.evaluateCredit: on InterruptedException, set an 'interrupted' flag, restore the interrupt and short-circuit the remaining futures, then return null so we never persist a customer on a thread where cancellation was signaled. - V1__proctran_counter_default.sql: after creating proctran_counter_seq, setval(seq, max(counter)+1, false) so the DEFAULT cannot collide with existing PROCTRAN rows on a non-empty database while preserving the starting value of 1 on an empty table. - CrecustRepository: enforce 10-digit MAX_CUSTOMER_NUMBER bound when allocating the next customer number so the %010d field in the PROCTRAN.DESCRIPTION layout cannot silently overflow.
|
augment review |
| private static final int REVIEW_DATE_BOUND = 20; | ||
| private static final long CREDIT_AGENCY_TIMEOUT_SECONDS = 6; | ||
| private static final List<String> VALID_TITLES = List.of( | ||
| "Professor", "Mr", "Mrs", "Miss", "Ms", "Dr", "Drs", "Lord", "Sir", "Lady", "" |
There was a problem hiding this comment.
Including "" in VALID_TITLES means an empty/whitespace-only CommName can pass title validation; since the request DTOs only enforce @NotNull (not non-blank), this could allow persisting customers with blank names.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| CustomerDetails customer = Objects.requireNonNull(result.customer(), "Successful response requires a customer"); | ||
| return new CrecustResponseDto(new CrecustCommareaResponseDto( | ||
| EYE_CATCHER, | ||
| new CrecustKeyDto(Integer.parseInt(customer.sortcode()), customer.customerNumber()), |
There was a problem hiding this comment.
Integer.parseInt(customer.sortcode()) will throw at runtime if cbsa.sortcode is ever misconfigured/non-numeric, and it will also drop any leading zeros that might be significant for a 6-digit sortcode.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| .set(PROCTRAN.AMOUNT, new BigDecimal("0.00")) | ||
| .execute(); | ||
| } catch (DataAccessException exception) { | ||
| throw new com.augment.cbsa.error.CbsaAbendException( |
There was a problem hiding this comment.
This maps PROCTRAN insert failures to a CbsaAbendException, but elsewhere (e.g., CrecustResult’s documented fail codes) persistence failures appear intended to surface as a CrecustResult fail code (like H). If clients rely on fail codes for audit-write failures, throwing an abend here may bypass that contract.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
Source program: https://github.com/augment-solutions/cics-banking-sample-application-cbsa/blob/main/src/base/cobol_src/CRECUST.cbl
Mapped REST endpoints
Notable decisions
COBOL fail-code mappings
Test coverage