fix(cdc): reject ignored PostgreSQL source SPI config - #239
Conversation
|
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:
📝 WalkthroughWalkthroughPostgreSQL Debezium CDC 소스가 배포 설정만 사용하도록 호출별 설정 제약을 문서화했습니다. 비어 있지 않은 설정을 거부하고, 오류 정보 노출과 ChangesPostgreSQL CDC 구성 검증
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cdc-service/src/test/java/com/xtrmetl/cdc/spi/PostgresDebeziumCdcSourceConfigurationTest.java (1)
21-41: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
null과 빈Map경로를 추가로 테스트하세요.현재 테스트는 비어 있지 않은 호출별 설정의 거부만 검증합니다. 변경 계약에는
null거부와Map.of()전달 시 배포 설정CdcService.start()위임도 포함됩니다.null경로에서CdcService미호출을 확인하고, 빈Map경로에서verify(service).start()를 확인하는 별도 테스트를 추가하세요.추가 테스트 예시
+import static org.mockito.Mockito.verify; + + `@Test` + void rejectsNullCallerConfiguration() { + CdcService service = mock(CdcService.class); + `@SuppressWarnings`("unchecked") + ObjectProvider<CdcService> provider = mock(ObjectProvider.class); + when(provider.getIfAvailable()).thenReturn(service); + PostgresDebeziumCdcSource source = new PostgresDebeziumCdcSource(provider); + + assertThrows(IllegalArgumentException.class, () -> source.start(null)); + verifyNoInteractions(service); + } + + `@Test` + void delegatesEmptyCallerConfiguration() { + CdcService service = mock(CdcService.class); + `@SuppressWarnings`("unchecked") + ObjectProvider<CdcService> provider = mock(ObjectProvider.class); + when(provider.getIfAvailable()).thenReturn(service); + PostgresDebeziumCdcSource source = new PostgresDebeziumCdcSource(provider); + + source.start(Map.of()); + verify(service).start(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cdc-service/src/test/java/com/xtrmetl/cdc/spi/PostgresDebeziumCdcSourceConfigurationTest.java` around lines 21 - 41, Extend PostgresDebeziumCdcSourceConfigurationTest with separate tests for null and empty per-call configuration. Verify source.start(null) throws IllegalArgumentException with the expected contract message and does not interact with CdcService, while source.start(Map.of()) delegates to the mocked service.start() and verifies that invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@cdc-service/src/test/java/com/xtrmetl/cdc/spi/PostgresDebeziumCdcSourceConfigurationTest.java`:
- Around line 21-41: Extend PostgresDebeziumCdcSourceConfigurationTest with
separate tests for null and empty per-call configuration. Verify
source.start(null) throws IllegalArgumentException with the expected contract
message and does not interact with CdcService, while source.start(Map.of())
delegates to the mocked service.start() and verifies that invocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a69bba0-b1a6-4b12-9439-9d780f338c8b
📒 Files selected for processing (2)
cdc-service/src/main/java/com/xtrmetl/cdc/spi/PostgresDebeziumCdcSource.javacdc-service/src/test/java/com/xtrmetl/cdc/spi/PostgresDebeziumCdcSourceConfigurationTest.java
1 participant
Purpose
Advance #238 test-first from exact protected
develop@622e5e6c3d534f230c390f10e3832efadfc01825. The livePostgresDebeziumCdcSourcedelegates lifecycle to deployment-configuredCdcService; caller-supplied per-call maps are not an authoritative Debezium configuration. Protected source accepted and silently ignored non-empty maps, so caller intent (including credential-like values) could appear accepted while having no runtime effect.Exact current identity
develop@622e5e6c3d534f230c390f10e3832efadfc01825;fix/postgres-spi-config-truth-622e5e6;9b98a3b74274c9488422638f2eca7640a34f8057;75065b5c675408f69b0eab918d58fef991950e0f;2a369886794b7d031ba952ac38ad6ceb2375d230;develop;PostgresDebeziumCdcSource.javaand its focused configuration test;Every predecessor-head check, review, base snapshot, or merge preview is historical after head movement.
RCA
validate(Map)rejected onlynull;start(Map)therefore accepted every non-empty map and immediately delegated to deployment-ownedCdcService, ignoring all entries.TDD evidence
RED — exact source
9b98a3b74274c9488422638f2eca7640a34f8057The fail-first commit added only
PostgresDebeziumCdcSourceConfigurationTest. It sends a non-empty map containing a credential-like value through the realstart(config)production boundary and requires rejection before anyCdcServiceinteraction, with a stable failure message that echoes neither key nor value.CI
31391184511, macOS job93463055955, checked out GitHub synthetic mergeab39fd93fae3456cbeb5cbf6d7b55d2a9827f355(Merge 9b98a3b... into 622e5e6...). ETL completed 275 tests green. CDC ran 107 tests with exactly the new test failing and zero test errors/skips: anIllegalArgumentExceptionwas expected but production threw nothing. This reached the intended production adapter boundary rather than failing in setup/import/fixture/dependency code.GREEN — exact current source
75065b5c675408f69b0eab918d58fef991950e0fThe production correction is deliberately narrow:
nullremains invalid;CdcService;All exposed workflow aggregates associated with this exact current source are terminal-success:
31392884219: success;31392881274: success;31392878898: success;31392877894: success;31392877892: success.CI macOS job
93468612634checked out synthetic merge2a369886794b7d031ba952ac38ad6ceb2375d230(Merge 75065b5... into 622e5e6...). The full reactor completedBUILD SUCCESS: ETL 275/275, CDC 107/107 includingPostgresDebeziumCdcSourceConfigurationTest1/1, and gateway 3/3.Evidence boundary
The current CI is strong causal merged-tree proof, but it is not literal-source acceptance evidence: checkout was
refs/pull/239/merge, not literal source75065b5.... The same log still reportsAnalyzed bundle 'etl-service' with 0 classesfollowed by successful JaCoCo checks; #162/#164 owns the non-vacuous coverage repair and #205 owns repository-wide owned-production scope. #196 separately owns dependency-resolution/scanner completeness. No qualifying independent non-author formal approval exists on this head.Merge boundary
Keep Draft. Do not merge until the unchanged exact source head has accepted literal-source deterministic/security evidence, complete same-revision dependency/vulnerability evidence, non-vacuous applicable owned-production coverage, every required CI/security/SBOM/provenance gate, zero valid unresolved findings, current live-base integrity, and qualifying independent non-author formal approval where governance requires it. No predecessor-head, other-PR, incomplete-scanner, status-only, skipped-required, author-only, or synthetic-merge-only evidence transfers.
Summary by CodeRabbit
문서화
Map만 허용되며, 유효하지 않은 입력과 서비스 부재 시 오류 조건을 안내합니다.버그 수정