fix(content-types): lazy-load field variables to eliminate cold-cache N+1 hydration

[swicken]

Fixes #36125

Proposed Changes

• Lazy field-variable hydration — DbFieldTransformer.fromMap() no longer force-calls newField.fieldVariables() on every field. Variables now load lazily on first access to Field#fieldVariables() and are memoized on the cached Field/ContentType instance. Consumers that actually need them (content-type save/copy/diff, field-variable REST, import/export & push-publish, indexing, unique-per-site/charLimit validation, AI auto-tag, and templates that explicitly read fieldVariablesMap()) still get identical data through the same method — the query just runs when the data is needed instead of for every field on every type load.
• Remove double-transform — DbFieldTransformer.asList() ran fromMap(map) twice per field (the first result was assigned to an unused local and discarded). Combined with the eager load, every field issued two field_variable queries, one entirely wasted. Now each field is transformed once.

Why

On a cold cache, a single POST /api/content/_search for Blogs with render:true on the demo (Costa Rica) starter fired ~298 select ... from field_variable where field_id = ? queries. The render path never reads field variables (warm was already 0), so the eager hydration — doubled by the duplicate fromMap — was pure waste. This is a hot, widely-used legacy path: every cold content hydration across the platform paid the burst.

The change is strictly subtractive: it only removes queries (and the 2× transform). No new cache, no invalidation logic, no API/DB/ES-mapping changes — existing Field/ContentType cache invalidation is untouched. Rollback-safe.

Measurements (local OTel → Tempo rig, cold = first search after restart)

Cold Blog _search (render:true)	Released baseline	This PR
field_variable queries	298	0
total JDBC	632	374
warm field_variable / JDBC	0 / 46	0 / 46

Beats the previously-rejected side-cache spike (which only reached 149 — it was masking the double-transform) and needs none of its cluster/commit/TTL machinery.

Checklist

• Tests — new FieldVariableLazyHydrationTest (asserts 0 eager loads on field load, one lazy load per field on access, warm reads served from cache); existing FieldFactoryImplTest regression suite passes (32 tests, 0 failures).
• Translations — N/A (backend, no user-facing strings)
• Security Implications Contemplated — none; no auth/permission/data-exposure surface changes. Field#fieldVariables() remains @JsonIgnore and @CloseDBIfOpened, so serialization and connection handling are unchanged.

Additional Info

• Behavior of the lazy getter is safe: @Value.Lazy uses synchronized double-checked locking; on failure the memo bit isn't set, so there's no poisoned-empty-list state (it retries next access). @CloseDBIfOpened means the lazy load works on any thread and closes the connection it opens.
• One timing nuance: if a field_variable read fails, the DotStateException now surfaces at first access (e.g. render/serialization) rather than at content-type load — same exception type, narrower blast radius (only the consumer that needs that field's variables is affected, not every type load).
• Verified end-to-end on the rig: all demo Blogs still render with full body content; warm behavior unchanged.

Screenshots

N/A — backend change.

[claude]

Claude finished @swicken's task in 6m 55s —— View job

---

PR Review

• Gather context
• Read changed files
• Review DbFieldTransformer.java
• Review FieldVariableLazyHydrationTest.java
• Post findings

---

Change is correct. The double-transform bug and the eager load removal are both clearly right. A few things worth a second look:

1. innerFieldVariables is not volatile — unguarded public write path

Field.java:234 declares innerFieldVariables as a plain private (non-volatile) field. The @Value.Lazy wrapper generated by Immutables provides synchronized double-checked locking for the fieldVariables() read path, but constructFieldVariables() (Field.java:236) writes to innerFieldVariables without any synchronization. The only caller today (JsonFieldTransformer:143) pre-seeds before any getter access, so the race isn't currently triggerable. The concern is that constructFieldVariables() is public — any future caller that uses it on a field already visible to other threads would have a silent data-race. This PR moves more production traffic through the lazy path, so it's a good time to either declare innerFieldVariables as volatile or add a @GuardedBy annotation as a codebase contract.

2. CountingConnection.getMetaData() proxy swallows exceptions

FieldVariableLazyHydrationTest.java:213:

(proxy, method, args) -> "getConnection".equals(method.getName())
    ? this
    : method.invoke(real, args)

Method.invoke wraps checked exceptions in InvocationTargetException. Any SQLException thrown by a real metadata method becomes InvocationTargetException, which the proxy interface doesn't declare — this will surface as an UndeclaredThrowableException at runtime rather than a SQLException. It won't cause a false green (the test would still fail), but the error message would be confusing in CI. Should unwrap: throw ((InvocationTargetException) e).getCause().

3. Test query-count assertion uses fields.size() — correct but not tight

FieldVariableLazyHydrationTest.java:91:

assertEquals("Each field's variables must load exactly once (no double-load)",
        fields.size(), fieldVarQueries.get());

ContentTypeDataGen may inject additional default/system fields beyond the three explicitly created. The assertion is "one query per whatever fields happen to be in the type," which is the right invariant, but it won't catch a regression where some fields generate 0 or 2 queries unless they affect the total. This is fine as written; just noting that printing fields.size() in the assertion message would help diagnose failures.

4. @Value.Lazy exception-retry claim holds, but only for the Immutables-generated bit

The PR description says "on failure the memo bit isn't set, so there's no poisoned-empty-list state." True for the generated lazy bit. However, if loadVariables throws, innerFieldVariables remains null (it's only set on success at Field.java:210), so the next call re-enters the synchronized block and retries. This is correct — just confirming the two-layer guard is coherent.

5. Minor: unused Proxy import if exception handling is fixed

FieldVariableLazyHydrationTest.java:17 imports java.lang.reflect.Proxy, used only in getMetaData(). Not a functional issue.

---

No correctness bugs introduced. Items 1 and 2 are worth addressing before merge; the rest are observations. Fix the proxy exception handling →

[swicken]

RE: Claude review

Validated all five against the code:

@CloseDBIfOpened (interception skipped) — doesn't hold. It's not proxy-based; it's ByteBuddy bytecode instrumentation (ByteBuddyFactory, RETRANSFORMATION, scope com.dotcms*) on the annotated base method Field.fieldVariables(). The generated ImmutableTextField.fieldVariables() calls super.fieldVariables() (invokespecial into the instrumented body), so the advice fires on the lazy path. And loadVariables reuses the open thread connection anyway, so there's nothing to leak — confirmed by the multi-fork IT run (0 errors).

@Value.Lazy / innerFieldVariables dual-guard — real but pre-existing and not triggered. The only constructFieldVariables() caller is JsonFieldTransformer:143, which pre-seeds a freshly deserialized field before any getter access (fieldVariables() is @JsonIgnore), i.e. the safe order. This PR doesn't touch Field.java or that path — it removes an eager getter, so exposure goes down, not up. Out of scope here.

assertSame on the cached refs — taking it. Turns the identity-cache canary from a comment into a real assertion.

asList/anonymous-class notes: agreed.