DO NOT MERGE: fix(permissions): pattern-switch resolvePermissionType and stop it throwing on a typeless contentlet (#34154)

[fabrizzio-dotCMS]

Summary

resolvePermissionType decides the key an asset inherits permissions under — what
permission_reference rows are stored and looked up against. It was an if / else if chain of
thirteen instanceof tests, and the first condition alone cast the same reference three times:

if (permissionable instanceof Host || (permissionable instanceof Contentlet
    && ((Contentlet) permissionable).getStructure() != null
    && ((Contentlet) permissionable).getStructure().getVelocityVarName() != null
    && ((Contentlet) permissionable).getStructure().getVelocityVarName().equals("Host")))

It is now a pattern switch: the type test binds the value once, the extra conditions move into when
guards, the nested Identifier fallback chain moves to its own method, and case null replaces the
if (perm != null) wrapper around it.

The behaviour that deliberately changed

A contentlet with no resolvable content type no longer throws.

Contentlet.getStructure() returns null when getContentType() is null. The first branch checked for
that. The FILEASSET branch right after it did not, and read getStructure().getStructureType()
directly — so a typeless contentlet failed the first guard correctly and then NPE'd on the second.

Verified rather than argued: the original condition, copied verbatim, run against such a contentlet:

java.lang.NullPointerException: Cannot invoke "Structure.getStructureType()"
  because the return value of "Contentlet.getStructure()" is null

The null check now lives in two shared helpers, so both places that read the content type get it.

Two branches removed as provably redundant

Not a judgement call — a checkable fact. Both branches mapped their input to
Contentlet.class.getCanonicalName():

• a FILEASSET contentlet
• an Event

That is exactly what Contentlet.getPermissionType() already returns (Contentlet.java:1186), and
no Contentlet subclass overrides it (verified across the tree). So both branches assigned the value
the variable already held. They now fall through to default and produce the same string.

The FILEASSET branch was also the one that could throw — so it was spending a crash to compute a value
that was already correct.

Case order is load bearing

Documented in the javadoc, because a reviewer will ask: Host extends Contentlet, so it has to be
matched first, and the Host-content-type check has to precede the IHTMLPage one.

Worth noting that getting this wrong is not a silent bug — the compiler rejects a dominated case
(error: this case label is dominated by a preceding case label). But that protection only applies to
unguarded patterns, and several of these cases are guarded, so the comment earns its place.

The Identifier fallback: five cases that could never match

The nested chain that moved out to permissionTypeOfBackingInode held its subject as a
Permissionable, but InodeFactory.getInode can only ever hand back an Inode
(InodeFactory.java:107). Widening the local was the only reason five of its cases compiled at all:

Case	Why it was dead
Contentlet, twice — once guarded on HTMLPAGE	Contentlet implements Permissionable directly; it is not an Inode
Folder	same, and InodeFactory throws DotStateException for a folder row before it returns anything (InodeFactory.java:147)
ContentType	an abstract class with no kinship to Inode

Java has single class inheritance, so an object that is both an Inode and a Folder cannot exist —
sharing the Permissionable interface does not change that. These were dead in the original
if / else if too; the pattern switch is what made them visible, and an IDE inspection is what
reported them.

Typing the local as Inode is the part that lasts, because it moves the guarantee from the IDE to the
compiler:

error: incompatible types: Inode cannot be converted to ContentType
      case Structure _, ContentType _ -> Structure.class.getCanonicalName();

IHTMLPage is kept as a defensive branch, with a comment saying why it is unreachable today: it is an
interface, so a future Inode subclass could implement it — which is also why the compiler still
accepts it while rejecting the four class patterns above. case null folds into the default arm,
which already returned the same value.

Reading the content type off the modern accessor

Both shared helpers now call Contentlet.getContentType() rather than getStructure(), which is
@Deprecated and does nothing but wrap that same call in a StructureTransformer
(Contentlet.java:590). Identical null semantics, one hop and one allocation less. Base types are
compared by enum identity (baseType == contentType.baseType()) instead of through the int behind
them.

Testing

11 new unit tests, all green — PermissionBitFactoryImplResolvePermissionTypeTest. The method had
no direct coverage before this PR; resolvePermissionType widened from private to package-private
@VisibleForTesting so it could get some.

The mocks stub getContentType() — the accessor the code actually reads — with a mocked
ContentType declaring variable() and baseType().

Coverage is one test per branch of the chain plus both trailing overrides (drawn Template →
TemplateLayout, NavResult → enclosing type), and the typeless-contentlet case that used to throw.

Two deliberate choices in the test design:

• Assets are mocked, not constructed. Instantiating a real Contentlet runs a static initialiser
  that reaches CDI (OSIndexAPIImpl), which no plain unit test has. Mocking costs nothing here — every
  assertion is about which branch the resolver takes, not about how the asset was built.
• Where the expected answer is "the asset's own declared type", the mock declares a sentinel rather
  than the real value. Without that, the two redundancy tests above would have asserted a string they
  themselves stubbed. Seeing the sentinel come back proves the resolver reached default instead of
  matching a branch that happened to produce the same value.

Integration tests — stated plainly

Ran PermissionBitFactoryImplTest, PermissionAPITest, PermissionAPIIntegrationTest,
PermissionBitFactoryImplGetPermittedIdsTest, PermissionBitAPIImplFilterCollectionTest:
47 run, 4 errors.

All four appear to be artifacts of running these classes standalone via -Dit.test= rather than in
their suite, and none of them reach the changed code:

Count	Failure	Why it looks unrelated
3	NullPointerException in DwrUtil.getLoggedInUser — DWR WebContext is null	Stack is RoleAjax.saveRolePermission → DwrUtil.getLoggedInUser; it never reaches resolvePermissionType. That web context is only set up by the full suite.
1	issue11850 — FK violation fk_structure_host deleting a test Site	Teardown data-cleanup ordering, not permission resolution.

Confirmed by baseline. The same two classes were run against unmodified main (this branch's source
file reverted, the new test set aside) and the identical four tests fail: issue11850, issue560,
issue781, test_templateLayout_parentPermissionableIsHost. Same set, same causes. They are
pre-existing and independent of this change — not "probably", measured.

Note that test_templateLayout_parentPermissionableIsHost sounds like it would exercise the
TemplateLayout override this PR touches. It does not get that far: it dies in DwrUtil.getLoggedInUser
before any permission type is resolved, on main and on this branch alike. That override is covered by
the two new unit tests instead.

Breaking Changes

None to any signature. One behavioural change, described above: a typeless contentlet resolves to its
declared type instead of throwing a NullPointerException.

Context

Part of the Devoxx Belgium 2025 Lunch and Learn groundwork (#34154). This is the pattern-matching example
for that talk, and it was picked because it demonstrates the three things at once on real code: the type
pattern removing repeated casts, when guards carrying the conditions that used to be &&-chained onto
the instanceof, and a latent defect that the old shape hid and the restructure surfaced.

Note for the talk, and for anyone reading this expecting a Java 25 feature: pattern switch, record
patterns and when guards are all final since Java 21. They are new to dotCMS only because this
codebase went from bytecode 11 straight to 25 in a single commit.

This PR fixes: #34154

🤖 Generated with Claude Code

[claude]

Claude finished @fabrizzio-dotCMS's task in 5m 34s —— View job

---

Code Review

Reviewed the pattern-switch refactor of resolvePermissionType and the new unit suite. I verified each load-bearing claim against the codebase rather than taking the PR description at its word.

Verified facts

• Contentlet.getContentType() returns null when getContentTypeId() is null (Contentlet.java:1756), and the deprecated getStructure() just wraps it (Contentlet.java:590). The old FILEASSET branch read getStructure().getStructureType() with no null guard → the NPE the PR fixes is real, and isOfBaseType/isOfContentType now guard it in both readers. ✅
• Contentlet.getPermissionType() returns Contentlet.class.getCanonicalName() (Contentlet.java:1186). Checked every extends Contentlet subtype (Host, Event, FileAsset, HTMLPageAsset, Persona, DefaultVanityUrl, DefaultKeyValue, …) — none override it. So dropping the FILEASSET and Event branches to default is behavior-preserving. ✅
• InodeFactory.getInode(id, Inode.class) can only return an Inode (InodeFactory.java:157), and Contentlet/Folder implement Permissionable without extending Inode while ContentType is unrelated — so those Identifier-fallback cases were genuinely dead. Typing the local as Inode moves that from IDE lint to compiler enforcement. ✅
• Host.HOST_VELOCITY_VAR_NAME == "Host" (Host.java:64) matches the string literal the old code compared against. ✅
• Case ordering (Host → Host-content-type → IHTMLPage → HTMLPAGE-content-type) preserves the original chain's precedence; no reachable input changes bucket. ✅

Test coverage — the suite exercises every branch including the deliberately-changed typeless-contentlet path, both dropped-redundant branches (with a sentinel DECLARED_TYPE so a fall-through can't be confused with a coincidental match), the case null arm, and the two state overrides. The @VisibleForTesting widening to package-private is the minimal surface change needed.

New Issues

No issues found.

The refactor is behavior-preserving except for the one documented, tested change (typeless contentlet resolves to its declared type instead of throwing). The redundant-branch removals and dead-Identifier-case eliminations are provable from the code, not judgment calls. Correctness, conventions (Logger, @VisibleForTesting, APILocator-backed accessors), and test design all hold up.

One process note: the title is prefixed DO NOT MERGE: — presumably intentional while this rides along with the Devoxx talk. Worth clearing before merge.

• Branch: issue-34154-java25-pattern-switch-permission-type