Detect duplicate JSON keys in flow files#78
Conversation
sfc-gh-dchaffelson
left a comment
There was a problem hiding this comment.
This is a clean one, Pierre — and a genuinely nasty failure mode to close off. Validating both files up front with a dedicated strict-duplicate parser is the right call: since JsonParseException is an IOException, relying on the deserialization to surface it would have let a duplicate in the before file get swallowed by the "no original flow" handler, so doing the explicit pre-scan before that try/catch is exactly right. The [!CAUTION] block with file + line/column matches the existing style nicely, and I like that it fails the check (exit 1) so a silently-wrong diff can't give false confidence.
Test coverage is spot on — both flowA and flowB directions, the exit code, and the CAUTION output, with a realistic double-flowContents fixture.
Two small non-blocking notes:
- Heads-up on merge ordering. This, #76, and #66 all touch
FlowDiff.java(and #76 also reworksgetDiff/executeFlowDiffForOneFlow), so they'll conflict with each other on merge, not just withmain. Worth a rebase whichever lands last. - Minor cosmetic: the
### Executing ... for flow:header prints the after path even when the duplicate is in the before file. The message body names the right file, so it's low-impact — just noting it.
Nice fix — LGTM.
sfc-gh-dchaffelson
left a comment
There was a problem hiding this comment.
Approving — clean fix for a nasty silent-failure mode. Notes in my comment (merge ordering, cosmetic header) are non-blocking.
Resolves #77. Before computing a diff, both flow files are now scanned with a streaming JSON parser that has STRICT_DUPLICATE_DETECTION enabled. If a duplicate key is found the action posts a CAUTION callout in the PR comment with the file path and exact line/column, then exits with code 1 so the PR check is blocked until the file is fixed. - Add validateNoDuplicateKeys() which uses a separate JsonFactory with STRICT_DUPLICATE_DETECTION; re-wraps JsonParseException with the file path for a self-contained error message. - Call it at the start of getDiff() for both pathA (skipped when absent) and pathB. - Catch JsonParseException in executeFlowDiffForOneFlow() and print the CAUTION block; set jsonParseError flag for run() to track. - Track jsonParseError per flow in run() and return RETURN_FAILURE if any parse error occurred. - Tighten the catch for snapshotA from Exception to IOException. - Add test fixture flow_v9_duplicate_key.json and four new tests.
9000438 to
1dcb749
Compare
sfc-gh-dchaffelson
left a comment
There was a problem hiding this comment.
Re-approving on the rebased head — duplicate-key detection is intact and the base is now current after the sibling PRs merged. LGTM.
Closes #77.
Summary
STRICT_DUPLICATE_DETECTIONenabled before any POJO deserialization. A dedicatedvalidateNoDuplicateKeys()method handles this using a separateJsonFactoryso the existing parsing configuration is untouched.[!CAUTION]block is posted with the file path and exact line/column, matching the style of the existing checkstyle violation block.catch (Exception e)for the "no original flow" case is narrowed tocatch (IOException e), so duplicate-key errors in flowA are now reported rather than silently treated as a first-version flow.Example PR comment on detection
Test plan
mvn test)testDuplicateKeyInFlowBThrowsException- getDiff() throws JsonParseException when flowB has a duplicate keytestDuplicateKeyInFlowAThrowsException- getDiff() throws JsonParseException when flowA has a duplicate keytestDuplicateKeyReturnsFailureExitCode- run() returns exit code 1 when a duplicate key is presenttestDuplicateKeyOutputContainsCaution- PR comment output contains the CAUTION callout and the duplicate field name