Conversation
Establishes the target project skeleton for the CBSA COBOL→Java migration: - pom.xml: Spring Boot 3.5.3 parent, Java 25, jOOQ 3.20, Flyway 11, PostgreSQL JDBC driver, Testcontainers BOM. Maven wrapper (script-only). - jOOQ codegen + Flyway Maven plugin wired into generate-sources so that V0__baseline.sql is migrated and table records are generated for each build (output in target/generated-sources/jooq, not committed). - src/main/resources/db/migration/V0__baseline.sql: customer, account, proctran (hash-sharded PK + index, 16 buckets) and control tables derived from the COBOL copybooks. - src/main/resources/application.yaml: env-driven datasource via COCKROACH_URL, virtual threads enabled, actuator exposing health/info. - docs/translation-rules.md: authoritative rulebook for PIC mappings, REDEFINES handling, CICS idioms, jOOQ/CockroachDB conventions, REST contract, error model, and PR conventions. - .github/workflows/ci.yml: installs CockroachDB v24.3.4, starts a single-node in-memory cluster, runs ./mvnw -B verify. - Smoke test: CbsaApplicationTests verifies Spring context loads against the configured CockroachDB. Mockito subclass mock-maker configured for Java 25 compatibility.
The cockroach CLI auto-reads COCKROACH_URL but expects libpq format, while our job-level COCKROACH_URL is the JDBC form Spring/Hikari needs. Unset it inside the DB bootstrap step so the CLI falls back to the explicit --host flag.
![augment-app-staging[bot]](https://avatars.githubusercontent.com/in/1008152?s=60&v=4){kind=small}
| <locations> | ||
| <location>filesystem:src/main/resources/db/migration</location> | ||
| </locations> | ||
| <cleanDisabled>false</cleanDisabled> |
There was a problem hiding this comment.
Setting Flyway cleanDisabled to false enables flyway:clean, which can drop the entire schema against whatever ${cockroach.url} points at (easy to run accidentally in local dev/CI scripts). Consider keeping clean disabled to reduce accidental data loss risk.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| <flyway.version>11.10.0</flyway.version> | ||
| <postgresql.version>42.7.7</postgresql.version> | ||
| <testcontainers.version>1.20.6</testcontainers.version> | ||
| <cockroach.url>jdbc:postgresql://localhost:26257/cbsa?sslmode=disable</cockroach.url> |
There was a problem hiding this comment.
The build-time Flyway/jOOQ configuration uses ${cockroach.url} which is hardcoded to localhost, while runtime/test datasource URLs come from COCKROACH_URL. If someone overrides COCKROACH_URL, codegen/migrations could still target localhost (or vice versa), leading to generated code/schema drift.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| @@ -0,0 +1,3 @@ | |||
| wrapperVersion=3.3.4 | |||
| distributionType=only-script | |||
| distributionUrl=https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.8.7/apache-maven-3.8.7-bin.zip | |||
There was a problem hiding this comment.
The Maven wrapper pins Maven 3.8.7, but README states Maven 3.9+ is required; this inconsistency can confuse setup and may hide Maven/JDK compatibility expectations. Consider aligning the wrapper version and the documented prerequisite.
Severity: low
Other Locations
README.md:20
🤖 Was this useful? React with 👍 or 👎
- pom.xml: flip flyway cleanDisabled to true (drop is destructive; we never
want a stray './mvnw flyway:clean' to wipe a real DB).
- pom.xml: source cockroach.url/user/password from env.COCKROACH_URL etc.,
matching the runtime Spring datasource. A local-defaults profile
activates only when COCKROACH_URL is unset and supplies localhost values
for local builds.
- .mvn/wrapper/maven-wrapper.properties: bump pinned Maven distribution
from 3.8.7 to 3.9.9 to match the README prereq ("Maven 3.9+").
- pom.xml: flip flyway cleanDisabled to true (drop is destructive; we never
want a stray './mvnw flyway:clean' to wipe a real DB).
- pom.xml: source cockroach.url/user/password from env.COCKROACH_URL etc.,
matching the runtime Spring datasource. A local-defaults profile
activates only when COCKROACH_URL is unset and supplies localhost values
for local builds.
- .mvn/wrapper/maven-wrapper.properties: bump pinned Maven distribution
from 3.8.7 to 3.9.9 to match the README prereq ("Maven 3.9+").
|
augment review |
| account_type VARCHAR(8) NOT NULL DEFAULT '', | ||
| interest_rate NUMERIC(6,2) NOT NULL DEFAULT 0, | ||
| opened DATE NOT NULL, | ||
| overdraft_limit INTEGER NOT NULL DEFAULT 0, |
There was a problem hiding this comment.
overdraft_limit is defined as INTEGER, but the project rulebook calls out overdraft limits as money values that should be modeled as BigDecimal (scale 2) in Java/SQL; keeping this as an integer risks truncating cents (and will generate an Integer field in jOOQ).
Severity: medium
🤖 Was this useful? React with 👍 or 👎
Aligns with translation-rules.md §2 which mandates BigDecimal/scale-2 SQL for all money columns (overdraft limits explicitly listed). Matches the existing available_balance / actual_balance shape on the same table.
|
augment review |
| id CHAR(6) NOT NULL PRIMARY KEY DEFAULT 'GLOBAL', | ||
| customer_count BIGINT NOT NULL DEFAULT 0, | ||
| customer_last BIGINT NOT NULL DEFAULT 0, | ||
| account_count INTEGER NOT NULL DEFAULT 0, |
There was a problem hiding this comment.
control.account_count/control.account_last are INTEGER while account.account_number is BIGINT; if these counters are used to allocate account numbers, this type mismatch can lead to overflow or implicit casts when wiring allocation logic.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| -- PIC 9(n) -> integer / bigint sized to fit n digits | ||
| -- PIC 9(n)V99 -> numeric(n+2,2) | ||
| -- PIC X(n) -> varchar(n) (NOT NULL with default '' to match COBOL spaces) | ||
| -- PIC 9(n) date -> date (8-digit DDMMYYYY/YYYYMMDD decoded in app layer) |
There was a problem hiding this comment.
The schema comment says dates are encoded as “DDMMYYYY/YYYYMMDD”, but the translation rulebook specifies PIC 9(8) dates as DDMMYYYY only; this ambiguity risks inconsistent parsing/formatting across services.
Severity: low
Other Locations
docs/translation-rules.md:39
🤖 Was this useful? React with 👍 or 👎
- control.account_count/account_last bumped to BIGINT to match account.account_number's BIGINT widening (we widened account numbers past the COBOL PIC 9(8) cap and the counters that allocate them must follow, otherwise sequential allocation overflows at 10^8). - baseline.sql header comment: PIC 9(8) dates are DDMMYYYY in COBOL (per translation-rules.md §2 and CUSTOMER.cpy / ACCOUNT.cpy). Removed the spurious YYYYMMDD alternative.
|
augment review |
| - name: Install CockroachDB v24.3.4 | ||
| run: | | ||
| set -euo pipefail | ||
| curl -sL https://binaries.cockroachdb.com/cockroach-v24.3.4.linux-amd64.tgz \ |
There was a problem hiding this comment.
The CockroachDB binary is downloaded and executed without any checksum/signature verification, which makes the CI build vulnerable to a compromised upstream artifact or MITM.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| @@ -0,0 +1,3 @@ | |||
| wrapperVersion=3.3.4 | |||
| distributionType=only-script | |||
| distributionUrl=https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.9.9/apache-maven-3.9.9-bin.zip | |||
There was a problem hiding this comment.
distributionUrl is pinned, but there’s no distributionSha256Sum, so the Maven distribution download isn’t integrity-checked (the wrapper scripts will skip verification).
Severity: medium
🤖 Was this useful? React with 👍 or 👎
- maven-wrapper.properties: add distributionSha256Sum for Maven 3.9.9 binary zip (verified locally with re-download). - ci.yml: download CockroachDB to a file, sha256sum -c against the pinned digest before extracting.
|
augment review |
| import org.springframework.boot.test.context.SpringBootTest; | ||
| import org.springframework.test.context.ActiveProfiles; | ||
|
|
||
| @SpringBootTest |
There was a problem hiding this comment.
@SpringBootTest will run Flyway and initialize the datasource during context startup, so this test will fail unless a CockroachDB instance is running/configured in the environment (despite having Testcontainers dependencies on the classpath). If the intent is a self-contained smoke test, consider ensuring the DB is started/managed by the test suite rather than requiring external setup.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
CbsaApplicationTests now excludes DataSource/Flyway/jOOQ autoconfigurations so it just verifies the Spring context wiring. Per translation-rules.md section 9, real persistence tests use Testcontainers CockroachDB and are added per program (starting with INQCUST). This removes the implicit dependency on an externally-provisioned CockroachDB for the smoke test.
|
augment review |
| program throws it with its program-specific abend code (e.g. `CVR1` for | ||
| INQCUST VSAM read failure) only for genuinely unrecoverable errors. | ||
| - A single `@ControllerAdvice` (`CbsaExceptionHandler`) maps: | ||
| - `CbsaNotFoundException` → 404 |
There was a problem hiding this comment.
The rulebook says a “not found” commarea result maps to HTTP 404 not via an exception (§6), but §8 then introduces CbsaNotFoundException → 404; this seems internally inconsistent and could lead to different programs handling not-found differently.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
|
|
||
| ## Local development | ||
|
|
||
| Prereqs: JDK 25, Maven 3.9+, CockroachDB v24.3. |
There was a problem hiding this comment.
The README lists “Maven 3.9+” as a prereq, but the documented workflow uses ./mvnw, which downloads and pins Maven itself; this can confuse setup expectations.
Severity: low
🤖 Was this useful? React with 👍 or 👎
- README: drop "Maven 3.9+" prereq; document that ./mvnw fetches its own pinned Maven 3.9.9 distribution, so a system Maven is not required. - translation-rules.md section 8: remove the stale CbsaNotFoundException -> 404 mapping, which contradicted section 6 (commarea-style fail flag, not an exception). The exception advice now lists only the cases that can actually be thrown; not-found is explicitly called out as a 404 ResponseEntity in the controller.
|
augment review |
| @@ -0,0 +1,5 @@ | |||
| spring: | |||
| datasource: | |||
| url: ${COCKROACH_URL:jdbc:postgresql://localhost:26257/cbsa?sslmode=disable} | |||
There was a problem hiding this comment.
In application-test.yaml, defaulting the datasource URL to localhost means any test that does initialize a datasource could silently connect to a developer’s local CockroachDB when COCKROACH_URL isn’t set (and potentially mutate real local data). Consider making the test profile’s datasource URL only come from Testcontainers/dynamic properties so tests remain hermetic.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
The smoke test now excludes DataSource/Flyway/jOOQ autoconfig, so it needs no test-profile datasource. Removing application-test.yaml also eliminates the localhost default that could silently connect future DB-backed tests to a developer's local CockroachDB. Per-program tests will set explicit Testcontainers properties via @DynamicPropertySource.
|
augment review |
| <profile> | ||
| <id>local-defaults</id> | ||
| <activation> | ||
| <property><name>!env.COCKROACH_URL</name></property> |
There was a problem hiding this comment.
The local-defaults profile only activates when COCKROACH_URL is unset, so if someone sets only COCKROACH_URL (and not COCKROACH_USER/COCKROACH_PASSWORD) the Flyway/jOOQ plugins will get empty credentials and fail even though runtime defaults the username to root.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| <execution> | ||
| <id>flyway-migrate-codegen</id> | ||
| <phase>generate-sources</phase> | ||
| <goals><goal>migrate</goal></goals> |
There was a problem hiding this comment.
Binding Flyway migrate (and jOOQ codegen) to the default generate-sources phase makes mvn test/verify require a reachable CockroachDB even when tests explicitly exclude datasource autoconfig.
That can make the build unexpectedly fail in environments that only want to run the smoke test without a DB.
Severity: medium
Other Locations
pom.xml:178
🤖 Was this useful? React with 👍 or 👎
| program throws it with its program-specific abend code (e.g. `CVR1` for | ||
| INQCUST VSAM read failure) only for genuinely unrecoverable errors. | ||
| - A single `@ControllerAdvice` (`CbsaExceptionHandler`) maps: | ||
| - `MethodArgumentNotValid` → 400 |
There was a problem hiding this comment.
In §8, MethodArgumentNotValid isn’t a Spring exception type; the concrete exception commonly handled is MethodArgumentNotValidException (and/or BindException).
Following this rulebook literally could lead to a handler that never triggers and validation failures not mapping to HTTP 400 as intended.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
- pom.xml properties: split the single local-defaults profile into three per-env-var profiles. Each of COCKROACH_URL/USER/PASSWORD now falls back to its localhost default independently, so setting only the URL no longer leaves USER/PASSWORD blank. - pom.xml plugins: add db.skip property (default false) and wire it to flyway-maven-plugin and jooq-codegen-maven via their <skip>. Running `./mvnw -Ddb.skip=true verify` now builds without contacting any CockroachDB. CI keeps the default behavior. - translation-rules.md section 8: typo fix - the actual exception is MethodArgumentNotValidException (also BindException for non-DTO binding errors), not MethodArgumentNotValid.
|
augment review |
|
|
||
| - All persistence goes through generated jOOQ `DSLContext` + table records | ||
| in `com.augment.cbsa.jooq`. **Never** hand-write JDBC SQL. | ||
| - Wrap multi-statement business operations in `dsl.transaction(c -> { ... })` |
There was a problem hiding this comment.
In §5, dsl.transaction(c -> { ... }) is easy to misapply: the c parameter is a jOOQ Configuration, so any queries need to be executed using a transaction-scoped DSLContext derived from that configuration or they may run outside the transaction.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| transactions — wrap with the **CockroachDB retry helper** | ||
| `CrdbRetry.run(dsl, () -> ...)` (introduced as part of the first program | ||
| that needs it). Never re-throw `40001`. | ||
| - Read-only paths use `dsl.connection(c -> c.setReadOnly(true))` only when |
There was a problem hiding this comment.
The guidance dsl.connection(c -> c.setReadOnly(true)) doesn’t by itself make subsequent dsl.selectFrom(...) calls read-only; it only mutates the borrowed connection within that callback.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
- Spell out that the lambda parameter of dsl.transaction is a jOOQ Configuration and that queries inside the block must be issued via DSL.using(cfg) (or cfg.dsl()), otherwise they run outside the transaction. - Drop the misleading dsl.connection(c -> c.setReadOnly(true)) suggestion. setReadOnly only affects the borrowed connection inside the callback and does not turn subsequent dsl.selectFrom calls into a "read-only mode". Read paths use plain dsl.selectFrom.
|
augment review |
1 participant
Phase 1 bootstrap of the CBSA COBOL→Java migration target.
What is in this PR
pom.xml— Spring Boot 3.5.3 parent, Java 25, jOOQ 3.20, Flyway 11, PostgreSQL JDBC driver, Testcontainers BOM. Maven wrapper (script-only).src/main/resources/db/migration/V0__baseline.sqlwithcustomer,account,proctran,controltables derived from the CBSA COBOL copybooks (CUSTOMER.cpy,ACCOUNT.cpy,PROCTRAN.cpy,CONTROLI.cpy). PROCTRAN uses CockroachDB hash-sharded primary key and date index (16 buckets) since it is the high-write audit log.generate-sources, then jOOQ codegen generates table records intotarget/generated-sources/jooq(not committed).src/main/resources/application.yaml— env-driven datasource viaCOCKROACH_URL, virtual threads enabled, actuatorhealth+infoexposed.docs/translation-rules.md— authoritative rulebook for PIC mappings, REDEFINES handling, CICS idioms (READ/WRITE/LINK/RETURN/HANDLE ABEND/ENQ/DEQ), jOOQ + CockroachDB conventions (retry on40001, hash sharding, control-row UPSERT for sequence allocation), REST contract,@ControllerAdviceerror model, virtual-thread executor policy, and PR conventions. Both Translator and Reviewer read this file..github/workflows/ci.yml— installs CockroachDB v24.3.4, starts a single-node in-memory cluster, setsCOCKROACH_URL, runs./mvnw -B verify.CbsaApplicationTestsverifies the Spring context boots against the configured CockroachDB. Mockito is pinned to the subclass mock-maker (src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker) because Java 25 disallows the agent self-attach used by the default inline maker.README.md— provenance back to the source repo, local dev quick-start, link to the rulebook and the (to-be-pinned) migration ledger.Out of scope (intentionally dropped per instructions)
BNK1*,BNKMENU(BMS handlers — replaced by REST + the existing React UI),BANKDATA(replaced by Flyway seed migrations),ABNDPROC(replaced by@ControllerAdvice).Local verification
Result:
BUILD SUCCESS, 4 jOOQ table records generated, smoke test green.After this merges, the next step is creating the pinned CBSA Migration Ledger issue and starting the per-program migration loop with
INQCUSTas the proof of concept.Pull Request opened by Augment Code | View session