fix(etl): reject invalid amounts before persistence - #199
Draft
seonghobae wants to merge 11 commits into
Draft
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Fix #198 test-first from protected
develop@622e5e6c3d534f230c390f10e3832efadfc01825. The protected ETL transformation historically mapped malformed, blank, excessive-precision, and unsupported-scaleAMOUNTinput to valid-looking0.00, making bad upstream data indistinguishable from a genuine zero after persistence.This direct-
developDraft now contains the complete bounded RED → GREEN behavior repair plus source-backed documentation alignment. It does not add a database migration, new public error field, raw-value logging, dependency change, or broad ETL refactor.Exact current identity
develop@622e5e6c3d534f230c390f10e3832efadfc01825;fix/reject-invalid-amount-622e5e6;1b89e62d5f8b30c6dceb0d39f538d283238e7a8a;27707b72ba1865fe86e24dfbecdc740a586a258b;f2d5e2042873b31047afda21b7bfb385e9794c4b;Every check/review from a predecessor head or base is historical and does not transfer.
RED — invalid amounts were accepted and rewritten
Fail-first
1b89e62d5f8b30c6dceb0d39f538d283238e7a8aaddedEtlServiceAmountIntegrityTestbefore production changed. It reachesEtlService.processData(...)with malformed, blank, excessive-precision and unsupported-scale values and requiresEtlRequestError.INVALID_RECORDbefore JDBC; it also requires one bad amount in a multi-record request to reject the entire prevalidated batch before the first write.Hosted CI
31348838751, macOS job93335761349, checked out synthetic mergee292fcd38b3f6af5d2efe9b332cc65ed1fdc8d0a(Merge 1b89e62... into 622e5e6...). Production/test compilation succeeded.EtlServiceAmountIntegrityTestthen ran six tests and all six failed because production threw noEtlRequestException; the rest of the ETL suite remained green before Maven stopped. This is valid RED at the real transformation/admission boundary, not a setup/import/fixture failure.GREEN — fail closed before persistence
The bounded production repair changes the amount transformation so malformed/blank/unsupported numeric input is classified through the existing stable invalid-record path rather than rewritten to zero. Valid values retain deterministic
BigDecimalformatting and HALF_UP scale-2 behavior. Whole-batch prevalidation, transaction boundaries, idempotency, payload limits, response shape and JDBC parameterization remain unchanged.Subsequent tests preserve genuine zero separately from invalid input and cover precision/scale boundaries and whole-batch rejection before JDBC.
Documentation RED → GREEN
After behavior was green,
EtlBatchDocsAlignmentTestrequired the changelog to state the buyer-visible fail-closed amount-integrity contract. CI31358691197, macOS job93363094463, reached that documentation boundary withEtlServiceAmountIntegrityTest6/6 green and failed onlychangelogRecordsFailClosedAmountIntegritybecause the existing sentence did not contain the canonical lowercase phrase required by the contract.Exact current head
27707b72ba1865fe86e24dfbecdc740a586a258bchanges only that changelog wording: invalidAMOUNTvalues fail closed before persistence instead of being rewritten to0.00, preserving the distinction from a genuine zero.Current hosted proof
All exposed workflow aggregates for exact source head
27707b72ba1865fe86e24dfbecdc740a586a258bare terminal-success:31361048737: success on macOS, Ubuntu and Windows;31361048765: success;31361048779: success;31361048735: success;31361048747: success.CI macOS job
93369857424checked out synthetic mergef2d5e2042873b31047afda21b7bfb385e9794c4b, not literal source head. On that integration treeEtlBatchDocsAlignmentTestpassed 5/5,EtlServiceAmountIntegrityTestpassed 6/6, ETL ran 283 tests with zero failures/errors/skips, CDC ran 106 tests, gateway tests passed, and the reactor finishedBUILD SUCCESS.The same CI still reproduces the inherited protected JaCoCo false-green:
Analyzed bundle 'etl-service' with 0 classesfollowed by coverage success. #162/#164 owns selected-bundle non-vacuity, and #205 owns repository-wide coverage scope. Current protected PR workflows also use synthetic merge checkout rather than accepted literal source. #196 separately owns complete Maven dependency-resolution evidence for vulnerability scanning. These controls are independent of this amount-integrity fix.Merge boundary
Keep Draft. Merge only when the unchanged exact source head has accepted literal-source deterministic/security evidence, complete same-revision dependency/vulnerability evidence, non-vacuous owned-production coverage, every required repository/security gate, zero valid unresolved review findings, and qualifying independent non-author formal approval where governance requires it. No predecessor-head, other-PR, status-only, skipped-required, incomplete-scanner, or synthetic-merge-only evidence transfers.