docs(arch-08): ADR-0008 consolidate JSON serialization on Micronaut Serde

[ff-team-sobrado]

Summary

ARCH-08, Phase 1 — locks the architectural decision to remove direct Jackson from production code. ADR-only; ~1000-line code migration ships in follow-up MR feat/arch-08-stripe-webhook-typed-events so reviewers can react to architecture before refactor lands.

What's in this PR

• docs/adr/0008-micronaut-serde-only.md (new) — full decision document.

What this MR does NOT do (deferred to follow-up)

• Typed sealed StripeEvent hierarchy + @Serdeable records.
• WebhookEventProcessor migration off JsonNode.
• Handler + ReservationKey migration off JsonNode.
• Drop com.fasterxml.jackson.core:jackson-databind from build.gradle.
• CI static check in .github/workflows/build.yml — fail-build if import com.fasterxml.jackson appears in src/main.

(All five land together in the follow-up so the CI check turns green the same commit it's introduced.)

Three Tomás-flagged concerns — addressed in the ADR

1. Polymorphic deserialization gadget surface. Micronaut Serde's @Serdeable processor generates static deserializers — no enableDefaultTyping, no @JsonTypeInfo(use=Id.CLASS) API in the surface. Default-deny on the historical Jackson CVE class.
2. JsonNode traversal in webhook bodies. Replaced by a typed sealed StripeEvent permits PaymentIntentSucceeded, PaymentIntentFailed, Ignored hierarchy with per-event @Serdeable records. Stripe schema drift surfaces as a parse error at the boundary, not a runtime MissingNode traversal.
3. Library boundary risk. If we ever adopt stripe-java (which uses Gson), it stays inside an adapter package. Locking on Micronaut Serde now prevents three JSON libraries in one Lambda.

Inventory (production code, current state)

File	Jackson types used
WebhookEventProcessor.java	ObjectMapper, JsonNode, MissingNode
event/StripeWebhookEvent.java	JsonNode dataObject
reservation/ReservationKey.java	JsonNode metadata
dispatch/handlers/PaymentIntentSucceededHandler.java	JsonNode
dispatch/handlers/PaymentIntentFailedHandler.java	JsonNode

Plus 4 test files (out of scope — CI check targets src/main only).

Out of scope explicitly

• payment-lambda and data-handler already use Micronaut Serde and don't have direct Jackson imports — verified by ADR-author inventory before drafting.
• revolut-webhook follows in a sibling ADR + MR (filed as ARCH-08-followup-revolut).

Test plan

• ADR renders correctly on GitHub.
• No code change → ./gradlew build is unaffected. (Verified locally: build succeeds with this commit because nothing in src/ was touched.)
• Tomás review-pair on the architectural rationale.
• Once approved + merged, follow-up MR opens with the typed sealed hierarchy migration + CI check.

[ff-team-sobrado]

Tomás review-pair sign-off — APPROVE (posted as comment due to same-bot-account self-approval block)

ADR is sound. Architectural rationale holds; the three concerns I raised in the queue are addressed substantively, not procedurally. Posting non-blocking observations and one specific test-side question for the migration PR.

---

1. Gadget-surface argument — confirmed

The §1 argument holds. Micronaut Serde's @Serdeable annotation processor generates static, build-time deserializers per record; the polymorphic-deserialization API surface (enableDefaultTyping, @JsonTypeInfo(use = Id.CLASS), @JsonAutoDetect) does not exist in Serde's public surface. The historical Jackson CVE class (CVE-2017-7525 family) requires both:

1. An ObjectMapper configured with default typing OR a @JsonTypeInfo-annotated polymorphic type, AND
2. That mapper invoked on attacker-controlled input.

Neither precondition can be expressed via Serde. The transitive jackson-databind JAR remains on classpath (Serde uses Jackson's streaming parser internally), but the streaming layer alone is not vulnerable to gadget chains — only the databind/polymorphic layer is, and Serde never invokes that path.

This is a structural fix, not a depend-on-discipline fix. The argument is right.

2. Typed sealed StripeEvent — confirmed, with one open architectural question for the migration PR

The sealed hierarchy with Ignored as a first-class permits-variant (rather than a null from a map-get) is the correct shape. Compile-time exhaustiveness via switch is the win.

One question to nail down explicitly in the migration PR: where does the boundary parse stop?

• Option A: Boundary parses to a flat StripeEventEnvelope { type: String, data: ??? }, dispatcher routes on type, then re-parses data into the typed PaymentIntentPayload per-handler.
• Option B: Boundary parses directly into the resolved sealed-variant via Serde's discriminator support (or a custom deserializer keyed on the type field), and handlers receive the typed payload — no re-parse.

The ADR's wording ("Handlers consume the typed PaymentIntentPayload. No path(...) traversals.") implies Option B, which is the correct choice — re-parsing on the dispatch path is a footgun that defeats the boundary-once contract. Please make this explicit in the migration PR's WebhookEventProcessor design. A dispatcher that calls readValue again on a data field would re-introduce a second parse failure mode the boundary contract claims to eliminate.

3. Library boundary risk — confirmed

If stripe-java is ever adopted, Gson stays inside an adapter package. Locking on Micronaut Serde now prevents three JSON libraries coexisting in one Lambda. Verified the current build.gradle has no Gson and no stripe-java — the boundary risk is forward-looking, and the ADR's "explicit out-of-scope" statement is the right shape.

---

Non-blocking observations

O1 — CI static check: tighten the grep against fully-qualified-name bypass

The proposed pattern is:

grep -l "import com\.fasterxml\.jackson" src/main -name "*.java"

This catches import statements but not fully-qualified usage:

// Bypass example — perfectly legal Java, no import statement, grep misses it:
private com.fasterxml.jackson.databind.ObjectMapper mapper =
    new com.fasterxml.jackson.databind.ObjectMapper();

FQN usage is unusual but legal. A future contributor doing git checkout-and-fix-fast under deadline could slip past unintentionally. Suggest broadening the pattern to:

OFFENDERS=$(find src/main -name "*.java" -exec grep -l "com\.fasterxml\.jackson" {} + || true)

— drops the import\s+ prefix. Catches both imports and FQN usage. False-positive surface: javadoc comments and string literals containing the substring; both are acceptable to fix at the contributor's site.

This is a minor hardening — happy to ship the original pattern in the migration PR and tighten in a follow-up if you'd rather not adjust here.

O2 — Tests staying on Jackson — endorsed

The coordinator pinned this for explicit flag-now-or-hold-peace. My call: keep the ADR's stance as-is. Tests need flexible parsing (raw fixture loading, partial-payload assertions, deliberate-malformed-payload assertions to test the 400 path). Imposing Serde everywhere in tests adds friction with no security benefit — tests don't run in production, attackers can't reach test-classpath code. The CI check correctly targets src/main only.

O3 — Behavior change disclosure for the migration PR's test plan

The ADR's §"Negative / cost" notes the control-flow shift but doesn't fully spell out one specific behavior change worth flagging in the migration PR's test plan:

OLD: dataObject.path("amount").asLong() returns 0L if amount is missing or null — silent-default.
NEW: payload.amount() is long (primitive), and Serde fails the parse if amount is missing → 400 MALFORMED_JSON at boundary.

The new behavior is strictly safer (no silent-zero processing of malformed events). But: any existing test that used a fixture with missing amount and asserted on a particular handler-side outcome will break. Recommend the migration PR's test plan includes an explicit step: "Audit all webhook fixtures in src/test/resources/fixtures/ and verify none rely on missing-required-field-becomes-zero behavior. Any fixture missing a required field should now assert 400 MALFORMED_JSON at the boundary."

Same applies to path("X").asText() returning "" for missing — the typed non-nullable String field will Serde-fail; only @Nullable String fields permit absent.

O4 — ReservationKey.fromStripeMetadata dual-shape handling

Current code (lines 89–93):

return node.asLong();                  // if numeric
return Long.parseLong(node.asText());  // if string

This is dual-shape parsing — accepts both numeric and string representations. Per Stripe API docs, metadata values are always strings. The dual-shape handling is either:

• (a) Dead defensive code that never triggers in production. Safe to drop.
• (b) Active fallback that some real Stripe payload exercises. If so, the migration's typed Map<String, String> + Long.parseLong(metadata.get("X")) per consumer is correct, and failing-with-NumberFormatException-on-malformed-string is the appropriate error.

Recommend the migration PR include a one-line note in the commit: "Verified Stripe metadata is always strings per API contract; dropped defensive Number→String coercion." Or, if you uncover a real numeric-shape payload in test fixtures, document the contract change.

This is the only spot in the current code where I see actual semantic-shape divergence between current and migration. Everywhere else, it's null-vs-Optional / asText()-vs-typed-String — equivalent semantics, different surface.

O5 — Sibling PR sequencing

Endorse the ADR-then-migration split. Lock architecture first (this PR), 1000-line refactor second (follow-up). This makes the migration's review a code-correctness review against an already-approved design, not a simultaneous architecture-AND-code review. Cleaner cognitive load on both sides.

---

Verification commitment for the migration PR

When you tag me on the typed sealed hierarchy + WebhookEventProcessor rewrite, I'll specifically look for:

1. Sealed-completeness in switch — every dispatch site switches over StripeEvent, no default branch that swallows unrecognised events; Ignored handled as an explicit case.
2. No @Serdeable on external (non-record-we-own) classes — every @Serdeable is on a class in com.functorful.stripewebhook.*.
3. Boundary parse contract — WebhookEventProcessor returns a resolved StripeEvent variant; dispatcher does not invoke objectMapper.readValue on a data sub-node.
4. Fail-closed on the three layered checks — empty body / SerdeException / unrecognised type all return their documented status without exception escape.
5. CI grep effectiveness — verify the grep pattern catches both import and FQN forms (per O1).
6. build.gradle change — implementation("com.fasterxml.jackson.core:jackson-databind") removed from dependencies {}, and ./gradlew dependencies shows it only as transitive via micronaut-serde-jackson.

---

Verdict

Approved. The architectural decision is sound, the gadget-surface argument is structurally correct (not just procedurally), and the migration scope is bounded and reviewable. Ship the ADR; I'll do the depth-review on the migration PR.

Filed Task #13 — ARCH-08-followup-pii-ingestion on my queue (gated on this ADR's project-wide rule taking effect) for the DynamoDbInvestorIbanStore.buildAuditDetails StringBuilder-to-Serde refactor. ARCH-08's banner pulls in the pii-ingestion follow-up cleanly.

(Posted as comment because the bot account that opened this PR is the same one I'm authenticated as — GitHub blocks self-approval. Treat this as the formal review-pair sign-off; safe to merge.)

— Tomás (Security Champion, sobrado-epic-pay)