DO NOT MERGE: #34154: refactor(java25): scoped values for reindex + config guard, flexible constructor for ISODateParam

[fabrizzio-dotCMS]

What

Applies three Java 25 features to places where the codebase already had the problem they solve. Material came out of the Java 21→25 tech talk (#34154): rather than illustrate the features with invented examples, these are the real call sites the features were designed for.

Each change is small, local, and covered by new unit tests. No behaviour change is intended except the ISODateParam fix.

1. ISODateParam — flexible constructor bodies (JEP 513)

DateUtil.parseISO signals unusable input by returning null, and UtilMethods.isSet rejects null, blank text and the literal four characters "null" — which is what a client sends when it interpolates an unset variable into a URL (/olderthan/null).

The constructor was:

public ISODateParam(final String stringDate) throws ParseException {
    super(DateUtil.parseISO(stringDate).getTime());
}

.getTime() is called on that null inside the argument list of super(...), so those inputs produced a NullPointerException that named no parameter. The author could not have fixed it: before Java 25 nothing was allowed to precede an explicit constructor invocation, so there was no place in the constructor to put the check.

A prologue now validates first and throws a ParseException naming the offending value. This class is a JAX-RS @PathParam binder used by BundleResource.deleteBundlesOlderThan, so the string is untrusted request input.

The prologue is visible in the bytecode — the null check completes before invokespecial Date.<init>:

 1: invokestatic  DateUtil.parseISO
 7: if_acmpne     25
24: athrow                      // ParseException
30: invokespecial Date."<init>":(J)V

2. ThreadContextUtil — scoped value for the reindex flag (JEP 506)

wrapVoidNoReindex / wrapReturnNoReindex were a hand-written dynamically scoped binding — read the old value, rebind, run the delegate, restore in a finally — duplicated across both methods. That is precisely what ScopedValue.where(...).call(...) does, so the two copies collapse into one and the restore can no longer be skipped.

includeDependencies is deliberately not migrated. It travels callee to caller: ESContentletAPIImpl records it deep in the stack and WorkflowAPIImpl.fireWorkflowPostCheckin consumes it afterwards. A scoped binding is immutable by design, so it cannot express an out-channel — it stays in the mutable ThreadContext. Both classes now document why the two pieces of state use different mechanisms.

Verified that no wrap* call site crosses an executor before the flag is read; ScopedValue does not propagate through DotSubmitter.

3. Config — re-entrancy guard via ScopedValue.isBound()

Reading a property can consult the system table, and consulting the system table reads properties in order to reach the database — so the lookup must detect re-entry. The guard was a mutable tag string on the per-thread ThreadContext whose finally assigned null rather than restoring the previous value.

To be clear, this is not a live defect — the nested call returns before the try, so two levels never coexist, and a test in this PR confirms the existing guard blocks recursion correctly. What the change buys:

• isBound() makes that class of mistake unwritable: a single shared slot used as a one-deep stack, with an exit path that assumes it was empty on entry.
• It drops a per-thread allocation. getOrCreateContext() installs a ThreadContext in a thread local for every thread that ever reads a config property, cleared only by ThreadLocalCleanupShutdownTask — which reaches into private static fields by reflection to do it.

This was the sole user of ThreadContext.tag, and the reindex flag was the sole user of ThreadContext.reindex, so both fields are removed. ThreadContext goes from three fields to one.

Testing

22 new unit tests: 9 for ISODateParam, 9 for ThreadContextUtil, 4 for the Config guard.

• The four ISODateParam null-input tests were confirmed failing with NullPointerException before the fix (Red), then green.
• The ThreadContextUtil tests pass before and after by design — they are the regression net proving the rewrite preserves behaviour, including that a throwing delegate still restores the flag and that a nested wrap restores the enclosing value rather than the default.
• The Config guard tests pin that a nested lookup never reaches the source, that the guard is released after the outer lookup returns, and that it is released when the source throws.

Full dotcms-core unit suite green: 3463 tests, 0 failures, 0 errors, 17 skipped.

Integration tests were not run. These are pure unit tests and register in no suite, but ESContentletAPIImpl and WorkflowAPIImpl exercise the reindex flag in ITs — worth a workflow/contentlet battery before merge.

Notes for the reviewer

• Requires the Java 25 compile target, which is already the default (dotcms.core.compiler.release). Neither ScopedValue nor flexible constructor bodies is a preview feature in 25, so no --enable-preview is needed for these.
• Adjacent finding not addressed here: includeDependencies is never cleared between workflow fires, so on a pooled thread it stays true after the first deferred reindex. That over-indexes rather than under-indexes, and changing it could reduce indexing where someone relies on it — flagging rather than folding it in.

🤖 Generated with Claude Code

This PR fixes: #34154

[claude]

Claude finished @fabrizzio-dotCMS's task in 2m 1s —— View job

---

Code Review

• Read PR diff (7 files) against origin/main
• Verify removed ThreadContext methods have no remaining callers
• Check ScopedValue migration for behavioral/compile issues
• Review ISODateParam flexible-constructor fix
• Review Config re-entrancy guard

---

Reviewed the diff end-to-end. This is a careful, well-documented refactor and I found no blocking issues. Notes below.

Verification performed

• Dead-code safety: grepped the whole tree for setReindex / isReindex / getTag / setTag — the removed ThreadContext accessors have no callers outside the deleted code, so removing the two fields is compile-safe. isReindex() and both wrap* helpers are only reached through ThreadContextUtil (ESContentletAPIImpl, WorkflowAPIImpl).
• Direction-of-travel argument holds: includeDependencies is still written via getOrCreateContext().setIncludeDependencies(...) inside ifReindex(delegate, includeDependencies) (ThreadContextUtil.java:99) and read later by WorkflowAPIImpl.fireWorkflowPostCheckin — that callee→caller channel genuinely cannot be a ScopedValue, so keeping it on the thread local is correct.
• Nesting semantics: ScopedValue.where(REINDEX, FALSE).call(...) restores the enclosing binding on exit (return or throw), so the hand-rolled read/rebind/restore-in-finally is faithfully replaced, and the nested-wrap test asserts the right thing.
• wrapVoidNoReindex delegation: collapsing it onto wrapReturnNoReindex preserves the Throwable → DotRuntimeException wrapping.
• ISODateParam: prologue references only the local parsedDate and static DateUtil before super(...) (no this/instance-field access), which is legal under JEP 513; the null case now throws a ParseException naming the value instead of an anonymous NPE. Good.
• Config guard: IN_SYSTEM_TABLE_LOOKUP.isBound() short-circuits recursion identically to the old "ConfigSystemTable" tag, and the binding is scoped to the .call(...) so it's released on return/throw with no finally.

Notes (non-blocking)

• 🟡 Medium: ThreadContextUtil.java:69 — isReindex() now returns REINDEX.orElse(Boolean.TRUE), which is thread-confined. This matches the old ThreadLocal behavior (neither propagates across an executor), so it's not a regression. Assumption: no wrap* scope hands work to a pool thread that then reads the flag. What to verify: the PR states this was checked for DotSubmitter; worth confirming the workflow/contentlet ITs cover a deferred-reindex path before merge, since that's the one runtime behavior a unit suite can't exercise here.
• The PR's own callout stands — includeDependencies is never cleared between fires on a pooled thread — but that's pre-existing and explicitly out of scope, so not flagged as introduced here.

No issues found that block merge.
· branch issue-34154-java25-scoped-values-flexible-ctors

[fabrizzio-dotCMS]

Design note: why includeDependencies stays a ThreadLocal

The PR keeps a ThreadLocal for includeDependencies while migrating reindex to a ScopedValue. That asymmetry looks arbitrary, so here is the trace behind it. Three alternatives were considered and all three are worse than what is in the diff.

First, the framing

ScopedValue is not a general replacement for ThreadLocal. It replaces one use: an immutable binding that descends from caller to callee and dies with the call. Mutable per-thread state, caches, and callee-to-caller channels stay on ThreadLocal — the JDK itself still uses it. So "we still depend on a ThreadLocal" is the correct outcome here, not an unfinished migration.

Where REINDEX is actually bound

Exactly one binding site, reached from exactly two call sites, both inside WorkflowAPIImpl.fireContentWorkflow (:3431):

// isReindex() == true here
final WorkflowProcessor processor = wrapReturnNoReindex(() -> fireWorkflowPreCheckin(...));  // :3449 — false inside
processor.setContentletDependencies(dependencies);                                            // true again
processor.getContentlet().setProperty(WORKFLOW_IN_PROGRESS, TRUE);
wrapVoidNoReindex(() -> fireWorkflowPostCheckin(processor));                                   // :3454 — false inside

The binding covers only those two sub-calls, not the whole method. Outside a workflow fire REINDEX is never bound at all, so isReindex() always falls through to orElse(TRUE).

A consequence worth stating: the else branch of the two-argument ifReindex — the only writer of includeDependencies — is reachable only inside those two wraps, because that is the only window where isReindex() is false. The out-channel always has a reader waiting.

Alternative 1 — make ThreadContext a record

Mechanically feasible: one field, one external consumer (WorkflowAPIImpl:2505), and it never reaches VTL, so record introspection is not a concern.

It is still wrong. The field survived the migration because it needs a mutable cell. A record does not remove the mutability, it moves it into the ThreadLocal slot — the ceremony of immutability with none of the benefit — and introduces a new footgun:

final ThreadContext threadContext = ThreadContextUtil.getOrCreateContext();
final boolean includeDependencies = threadContext.isIncludeDependencies();

With a mutable object that reference stays live and observes a later write. With a record it is a snapshot: a write replaces the ThreadLocal value and the captured reference goes stale. The two lines are adjacent today, but "get the context, pass it around, read it later" becomes silently wrong.

A record is a value. If the object's job is to be a cell that someone else writes into, a record is the wrong tool no matter how few fields it has.

Alternative 2 — bind a mutable holder as a scoped value

ScopedValue<AtomicBoolean>, so the binding is immutable and the holder dies with the scope. This would also fix the "never cleared between fires" leak noted in the PR description.

Rejected: it is a ThreadLocal with extra steps. The mutability is unchanged, and the indirection buys only the scope-bounded lifetime — at the cost of making the mechanism harder to read than the thing it replaces.

Alternative 3 — move the flag onto the Contentlet

This was the attractive one. WorkflowAPIImpl:2502-2506 reads two halves of a single decision from two different channels on adjacent lines — needsReindex rides on the Contentlet, includeDependencies rides on the thread. The Contentlet already carries a sibling field, indexPolicyDependencies (Contentlet:212). And INCLUDE_DEPENDENCIES is a compile-time constant true (ESContentletAPIImpl:278), so the value is not data — it is "something below asked for a dependency-including index."

It does not work, and the trace is decisive. The writer's Contentlet is not the same instance the reader sees:

• SaveContentActionlet:101 and SaveContentAsDraftActionlet:126 call processor.setContentlet(contentletNew) — checkin creates a new version, so a new object.
• ResetTaskActionlet, DeleteContentActionlet and DestroyContentActionlet set it to null outright.

Object identity between writer and reader is not merely unguaranteed, it is deliberately broken on the save path. And the cost of working around it is already visible one line above the replacement:

this.setIndexPolicy(contentlet, contentletNew);       // hand-copy the policy onto the new instance
this.setSpecialVariables(contentlet, contentletNew);
processor.setContentlet(contentletNew);

Moving includeDependencies onto the object means adding one more field to that manual copy, at every replacement point, forever — the same debt indexPolicyDependencies already carries, where forgetting one site is a silent bug.

Conclusion

The ThreadLocal is load-bearing. It survives object replacement, which is exactly the property this signal needs: the flag means "something in this execution asked for dependencies", not "this object asks for them". Not elegant, but not accidental either.

What the PR does keep is the part that genuinely was a scoped binding, and the javadoc on both classes now records why the two pieces of state use different mechanisms — so the next reader does not re-run this trace.

The lifetime issue is untouched and still worth its own ticket: includeDependencies is never cleared between workflow fires, so on a pooled thread it stays true after the first deferred reindex. That over-indexes rather than under-indexes, and fixing it changes behaviour, so it is deliberately not folded in here.