Skip to content

fix(connectors): freeze nested ChangeRecord containers - #228

Closed
seonghobae wants to merge 3 commits into
developfrom
fix/change-record-nested-snapshot-622e5e6
Closed

fix(connectors): freeze nested ChangeRecord containers#228
seonghobae wants to merge 3 commits into
developfrom
fix/change-record-nested-snapshot-622e5e6

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Purpose

Advance #227 test-first from exact protected develop@622e5e6c3d534f230c390f10e3832efadfc01825. etl-service ChangeRecord snapshotted only the top-level row maps, so nested JSON-shaped Map/List values stayed aliased to caller-owned mutable containers. The current candidate recursively snapshots those supported container shapes without claiming arbitrary-object cloning.

Exact current identity

  • protected/live base: develop@622e5e6c3d534f230c390f10e3832efadfc01825;
  • branch: fix/change-record-nested-snapshot-622e5e6;
  • test-only RED head: dba14059d270176c30093c450388b5d45cf1f395;
  • exact current source head: 401bc6833887b2244a3f46438467e7d3dc0b67a3;
  • current synthetic merge preview: f57e7b4588a52487e9ebce941df494e5343a2dc0;
  • Draft / mergeable: true / true.

Hosted RED proof

CI 31377776504, macOS job 93420835710, checked out GitHub synthetic merge 6172376d4a746cb7856578a1d220ad2c91821a26 (Merge dba14059... into 622e5e6...). Production and test compilation succeeded. ChangeRecordTest ran five tests with exactly two failures and zero errors/skips:

  • recursivelySnapshotsNestedJsonContainers: expected nested list size 2 after caller mutation, observed 3;
  • exposesNestedJsonContainersAsUnmodifiable: expected UnsupportedOperationException, but nested mutation succeeded.

The rest of the ETL tests reached 277 total with only those two failures. This is valid RED at the intended public record snapshot boundary, not a setup/import/fixture/dependency failure. The workflow executed a synthetic integration revision, so it is development RED evidence rather than literal-source acceptance proof.

GREEN and current hosted evidence

Current head changes only ChangeRecord:

  • recursively snapshots nested Map<?, ?> and List<?> containers;
  • preserves map/list iteration order and nested null values;
  • exposes copied nested containers as unmodifiable;
  • preserves current scalar/non-container identity semantics;
  • updates the class Javadoc so the supported JSON-shaped deep-snapshot boundary is explicit and does not overclaim arbitrary mutable Java-object cloning.

For exact source head 401bc6833887b2244a3f46438467e7d3dc0b67a3, all exposed aggregates are terminal-success:

  • CI 31378296240: success on Ubuntu/macOS/Windows;
  • Dependency Review 31378296235: success;
  • CycloneDX SBOM 31378296164: success;
  • SAST Semgrep 31378296177: success;
  • Security Scan 31378296221: success.

CI macOS job 93422463428 checked out synthetic merge f57e7b4588a52487e9ebce941df494e5343a2dc0, not literal source. ChangeRecordTest passed 5/5, ETL passed 277/277, CDC passed 106/106, gateway passed 3/3, and the full reactor completed BUILD SUCCESS.

The same job still reproduces the inherited protected coverage defect: JaCoCo reports Analyzed bundle 'etl-service' with 0 classes and then says coverage checks were met. Therefore this aggregate cannot be treated as non-vacuous 100% owned-production coverage evidence; #162/#164 and repository-wide #205 remain separate controls. No reflection, Java serialization, provider change, registry change, migration, schema or wire-contract change is introduced.

Scope / shared gates

This PR does not modify active CDC CanonicalChangeRecord PR #222. Keep Draft because accepted literal-source evidence (#121/central controls), scanner dependency-graph completeness (#196), non-vacuous selected/repository-wide coverage (#162/#164/#205), current review state and independent formal approval remain separate acceptance boundaries. No predecessor/synthetic/other-PR evidence transfers.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 083e4569-ec1e-4978-b598-e10eae2be51f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copy link
Copy Markdown
Collaborator Author

Fresh exact-head CI RCA for current c4dd764ded629aed111131c518acef8a2499d67d (base remains protected develop@622e5e6c3d534f230c390f10e3832efadfc01825): CI run 31378913866 is a real product-test failure on all three hosted OS jobs, not infrastructure. macOS job 93424411143 checked out synthetic merge 1d88589cf38bfcac45bdbf439d5a79fa5a702038 (Merge c4dd764... into 622e5e6...) and ran 278 ETL tests; the sole failure is ChangeRecordTest.rejectsCyclicNestedContainersDeterministically, expected IllegalArgumentException but production recursion in ChangeRecord.snapshotValue throws StackOverflowError. Current source recursively walks Map/List without identity-based cycle tracking, while the newly-added test explicitly requires deterministic cycle rejection. Root-cause-changing remedy: carry an identity-based active-container set through recursive snapshotting (remove a container when unwinding so repeated non-cyclic aliases remain valid), fail with the asserted stable message only when the same container is encountered on the active recursion path, and add a list-cycle case plus a shared-but-acyclic-container case before GREEN. Do not rerun unchanged head: it cannot alter this cause. This branch moved during this invocation, so I am not racing its active writer with a source push.

@seonghobae seonghobae closed this Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant