DO NOT MERGE: feat(velocity): resolve record components from VTL (#34154)

[fabrizzio-dotCMS]

Summary

A record's canonical accessor is named after the component itself — foo(), not getFoo(). Velocity's
property resolution never looked for that. The chain in UberspectImpl.getPropertyGet is:

getFoo()  →  getfoo()  →  Map.get  →  get("foo")  →  isFoo()

An unresolved Velocity reference is not an error — it renders as literal template text. So a
template reading a record printed $rec.id into the page, silently.

That is why the records behind SearchHit — ContentSearchHit and SiteSearchHit — name their
components getId / getIndex / getSourceAsMap instead of id / index / sourceAsMap: they were
deformed to satisfy the template engine. Since dotCMS
carries its own in-tree fork of Velocity (dotCMS/src/main/java/org/apache/velocity/, no pom
declares velocity-engine-core), that tax was never inherent — it was ours to remove.

The change

One new executor, RecordComponentExecutor, plus four lines wiring it into the chain. It is
deliberately narrow, and each restriction is load-bearing:

• Records only, declared components only. It resolves when clazz.isRecord() and the
  identifier names one of getRecordComponents(). It never resolves an arbitrary no-argument method
  — widening resolution to any foo() would silently change the meaning of existing templates across
  the product.
• Last in the chain, after every strategy that could already resolve the reference. So it can only
  add a resolution where there was none: no reference that resolves today changes meaning, without
  exception. Ordering it earlier would have left one theoretical case (a record implementing Map)
  where a working reference changed target; last removes even that.
• Looked up through Introspector.getMethod, not RecordComponent.getAccessor(), so the method
  cache and the checks of the configured introspector (SecureIntrospectorImpl) both still apply.
  SecureUberspector — the uberspect dotCMS actually configures via system.properties
runtime.introspector.uberspect — inherits getPropertyGet unchanged, so one edit covers both.

Records whose components are bean-named keep resolving through PropertyExecutor, so the
SearchHit shapes and every other shipped record are untouched — verified by re-running the VTL
integration families after rebasing onto the main that carries the sealed SearchHit (#36899).

Cost: amortized to nothing. ASTIdentifier caches the VelPropertyGet per AST-node/class in the
introspection cache (ASTIdentifier.java:132-159), so getPropertyGet runs once per pair, not per
render.

A separate trap, pinned by test

Surfaced while writing the tests and unrelated to this fix: a non-public record is invisible to
VTL. ClassMap checks Modifier.isPublic on the class before collecting its methods, so a
package-private or method-local record resolves to nothing — same silent literal-text outcome. This has
always been true of any class, but it bites harder with records, because the natural instinct is to
declare a small record package-private right next to its use.

Kept as an explicit assertion (test_nonPublicRecord_doesNotResolve) rather than deleted, so the
constraint is documented where someone will hit it: a record read from a template must be public,
or nested in a public type.

Testing

111 tests green.

Test	Covers	Result
RecordComponentExecutorTest	the resolution chain, split between what the change adds and what it must not touch	14/14
RecordComponentRenderingTest	the same claims end-to-end through the real engine via VelocityUtil.eval	7/7
10 existing VTL families	regression across the introspection path	90/90

The regression run covers ContentToolTest (23), NavToolTest (19), StoryBlockMapTest (12),
ContentSearchToolTest (11), ContentMapTest (9), VelocityUtilTest (5), ASTMethodTest (4),
DotParseTest (3), VelocityMacroCacheTest (3), SimpleNodeTest (1).

Two deliberate choices in the test design:

• The integration test asserts rendered output, not introspection results. The unit test already
  covers resolution; what matters here is what a page shows, since the failure mode being fixed is
  literal text in the page.
• References are written non-quiet ($rec.id, never $!{rec.id}). Quiet notation renders an
  unresolved reference as the empty string, which would let a broken accessor pass an assertion that
  only checks for absence.

Verified the ITs ran against the change rather than a stale artifact: dotcms-core was reinstalled to
~/.m2 before the run, and test_canonicalRecordComponents_render would fail against the old jar.

Not in this PR

Renaming SearchHit's components back to id / index / sourceAsMap is out of scope. The JSON
contract would survive (explicit @JsonProperty), but the getId() accessors would disappear, which
is source-breaking for any Java consumer. This PR removes the tax for new records; retrofitting the
existing ones is a separate decision.

Breaking Changes

None. The change is strictly additive to property resolution — it only resolves references that
previously resolved to nothing.

Context

Groundwork for the Devoxx Belgium 2025 Lunch and Learn (#34154), whose dotCMS half argues about when
records are and are not a good fit in this codebase. The Velocity naming tax was the sharpest
"when not to" — so it is being removed rather than documented.

This PR fixes: #34154

🤖 Generated with Claude Code

[claude]

Claude finished @fabrizzio-dotCMS's task in 3m 4s —— View job

---

Code Review — PR #36966: resolve record components from VTL

• Read changed files
• Review RecordComponentExecutor + UberspectImpl wiring
• Check conventions, security, tests
• Post review

---

This is a tightly-scoped, load-bearing change with strong test coverage. I traced the resolution chain, the security path, and the record-detection guard against the repo. No bugs found.

Verified

• Security path intact. RecordComponentExecutor.discover resolves via introspector.getMethod(clazz, component, params) rather than RecordComponent.getAccessor(). Under the SecureUberspector that dotCMS actually configures, the introspector is SecureIntrospectorImpl, whose getMethod gates every lookup through checkObjectExecutePermission (SecureIntrospectorImpl.java:62-76). So a record in a restricted package/class is still blocked, exactly as for the other executors. No new reflection surface.
• Ordering is correct. The executor is placed last in getPropertyGet (UberspectImpl.java:271-274), after PropertyExecutor, MapGetExecutor, GetExecutor, and BooleanPropertyExecutor. It only ever adds a resolution where all others returned dead. The "record implementing Map" case the PR body cites is genuinely closed by last-position ordering.
• Guard is narrow and safe. Resolution requires both clazz.isRecord() and a match against getRecordComponents() (RecordComponentExecutor.java:72, 128-156). test_nonRecordNoArgMethod_stillDoesNotResolve and test_nonRecordNoArgMethod_stillRendersAsLiteralText pin the guardrail that arbitrary no-arg methods are not widened into properties.
• Null-safety. flipFirstCharacter reads charAt(0), but discover is only reached when StringUtils.isNotEmpty(property) (RecordComponentExecutor.java:72), so no StringIndexOutOfBounds. The components == null check at line 132 is defensively dead (a record class never returns null here) but harmless.
• Conventions. Uses com.dotmarketing.util.Logger, throws VelocityException, mirrors PropertyExecutor's exception handling exactly. Consistent with the surrounding forked-Velocity code.

Notes (non-blocking)

• 🟡 Medium: RecordComponentExecutor.java:174-179 — execute returns null when !isAlive(). This matches PropertyExecutor.execute, so it's consistent; flagging only because in practice the executor is never invoked when dead (the VelGetterImpl wrapper is created only when isAlive()), so the branch is dead defensive code. Fine to keep for parity.
• The case-flip convenience (test_firstCharacterCaseFlip_resolves) intentionally mirrors PropertyExecutor. For a hypothetical record declaring two components differing only in first-char case, exact match is preferred first (componentNamed line 137-143 before the flip loop 147-153), so there's no ambiguity regression.

Housekeeping

• The PR title is prefixed DO NOT MERGE. Assuming that's intentional pending the Devoxx timing — nothing in the code blocks merge on its own.

Verdict: Clean. Additive-only, correct ordering, security preserved, and the tests assert both what changed and what must not. No changes requested.