diff --git a/docs/experiments/sealed-permissionable/README.md b/docs/experiments/sealed-permissionable/README.md new file mode 100644 index 00000000000..e4e4d32fe0b --- /dev/null +++ b/docs/experiments/sealed-permissionable/README.md @@ -0,0 +1,153 @@ +# Can the `Permissionable` hierarchy be sealed? + +Groundwork for the Java 25 talk (#34154). **Nothing here is production code and nothing here is +built** — these sources live outside every Maven source root on purpose, because two of the three +experiments are supposed to fail to compile. The failures are the result. + +```bash +./run.sh # needs only a JDK 22+ — no Maven, no dotCMS classpath, no network +``` + +Everything quoted below is that script's output. + +## Why the question comes up + +`PermissionBitFactoryImpl.resolvePermissionType` dispatches over `Permissionable` with a pattern +`switch` that ends in a `default`. A `default` is a catch-all: the day someone adds an asset type, +the code keeps compiling and the new type quietly takes the default path. + +Sealing the hierarchy is what would let that `default` go away, and with it the silence — a sealed +type tells the compiler the complete list of subtypes, so it can check that a switch covers them all. + +The hierarchy is not large: **14 direct implementors of `Permissionable`, 7 direct subclasses of +`Inode`.** Perfectly listable. So the question is a fair one. + +## The setup + +`src/` mirrors the real package layout with a handful of the real types, because the package layout +is the whole point: + +| Type | Package | Role | +|---|---|---| +| `Permissionable` | `com.dotmarketing.business` | the sealed root | +| `Contentlet` | `com.dotmarketing.portlets.contentlet.model` | `sealed`, seals further down | +| `Host` | `com.dotmarketing.beans` | `final` — a leaf | +| `Folder` | `com.dotmarketing.portlets.folders.model` | `final` | +| `Identifier` | `com.dotmarketing.beans` | `final` | +| `Inode` | `com.dotmarketing.beans` | `non-sealed` — gives up and reopens | +| `PermissionResolver` | `com.dotmarketing.business` | the switch, with no `default` | + +`Inode` being `non-sealed` does **not** break exhaustiveness downstream: every subclass of `Inode` is +still an `Inode`, so one `case Inode` covers all of them. Sealing reasons about permitted subtypes, +not about leaves. + +## Experiment 1 — seal it where it lives + +Compile those sources **without** `module-info.java`, which is the situation dotCMS is in today: + +``` +src/com/dotmarketing/portlets/contentlet/model/Contentlet.java:7: error: class Contentlet in unnamed module cannot extend a sealed class in a different package +public sealed class Contentlet implements Permissionable permits Host { + ^ +src/com/dotmarketing/business/Permissionable.java:12: error: class Permissionable in unnamed module cannot extend a sealed class in a different package +public sealed interface Permissionable permits Contentlet, Folder, Identifier, Inode { + ^ +... 5 errors +``` + +One error per permitted subtype. The rule: **a sealed type in the unnamed module requires every +permitted subtype to live in the same package.** dotCMS has no `module-info.java`, so everything is +in the unnamed module. + +Running the same attempt against the *real* `Permissionable`, with all 14 implementors in the +`permits` clause, gives **14 errors — one per type, no exceptions**: none of the fourteen lives in +`com.dotmarketing.business`; they are spread across nine packages. + +```bash +# for the record, against the real type (needs the dotcms-core classpath) +javac --release 25 -cp dotCMS/target/classes: Permissionable.java +``` + +### Why "just move them into one package" is not the answer + +Their canonical names are **persisted data**. `permission_reference.permission_type` stores strings +like `com.dotmarketing.portlets.folders.model.Folder`, and those literals appear hardcoded in +`PermissionBitFactoryImpl`'s own SQL (lines 303 and 462). Moving `Folder` to another package is a +data migration on the permissions table, not an import refactor. + +## Experiment 2 — the same sources, inside a named module + +Add six lines and change nothing else: + +```java +module dotcms.permissions { + exports com.dotmarketing.business; +} +``` + +``` +exit: 0 +``` + +It compiles. Sealed across packages, `Contentlet` sealing down to `Host`, `Inode` reopening its +branch with `non-sealed` — and the resolver carries no `default`: + +```java +return switch (permissionable) { + case Host _ -> "Host"; + case Contentlet _ -> "Contentlet"; + case Folder _ -> "Folder"; + case Identifier _ -> "Identifier"; + case Inode _ -> "Inode"; +}; +``` + +(`Host` precedes `Contentlet` because it extends it — the other order is rejected with *"this case +label is dominated by a preceding case label"*.) + +## Experiment 3 — add an asset type, touch nothing else + +A fifteenth permitted type joins the `permits` clause. The resolver is left exactly as it was: + +``` +src/com/dotmarketing/business/PermissionResolver.java:25: error: the switch expression does not cover all possible input values + return switch (permissionable) { + ^ +1 error +``` + +That is the entire payoff, and it only exists because there is no `default`. **Put a `default` back +and this compiles in silence** — sealing does not give you the check; removing `default` gives you +the check, and sealing is what makes removing it possible. Any total pattern (`case Object o`, +`case Permissionable p`) silences it just the same. + +## What this measures + +> Sealing this hierarchy is blocked by neither its design nor its size. **It costs a +> `module-info.java`** — and in exchange the compiler names every place that needs updating when an +> asset type is added. + +Which reframes the modularisation discussion as a trade with a price tag instead of an abstract +preference. The price is real: `module-info` on a WAR carrying OSGi, Hibernate, reflection and split +packages is a project of its own, and JPMS and OSGi are two module systems competing for the same +job. + +## The honest limit, worth knowing before anyone gets excited + +Even fully sealed, this particular resolver would keep most of its shape, because **half its +branches do not dispatch on Java types at all.** From the test suite on #36982: + +> `test_contentletOfHostContentType_resolvesAsHost` — *resolves as a Host, even though the object is +> not a `Host` instance* + +A contentlet "of type Host" is usually a plain `Contentlet` whose content type — a row in the +database — is named `Host`. Hence the two branches for one concept: + +```java +case Host _ -> HOST; // variant is a type +case Contentlet c when isOfContentType(c, HOST_VELOCITY_VAR) -> HOST; // variant is data +``` + +Sealed types verify the variants that live in the type system. dotCMS's variants live in the +database. The `when` guard exists precisely because that variability escaped the type system — and it +is why that method's `default` is honesty rather than laziness. diff --git a/docs/experiments/sealed-permissionable/run.sh b/docs/experiments/sealed-permissionable/run.sh new file mode 100755 index 00000000000..e6f8e07e54c --- /dev/null +++ b/docs/experiments/sealed-permissionable/run.sh @@ -0,0 +1,55 @@ +#!/usr/bin/env bash +# +# Three experiments on sealing the Permissionable hierarchy. Needs nothing but a JDK 22+ — no Maven, +# no dotCMS classpath, no network. Every claim in README.md is this script's output. +# +# 1. Sealed across packages, unnamed module -> fails, and the error is the whole point +# 2. The same sources inside a named module -> compiles, resolver has no `default` +# 3. Experiment 2 plus one more asset type -> the resolver stops compiling on its own +# +# The only delta between 1 and 2 is whether module-info.java is handed to javac. + +set -u + +HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +SRC="${HERE}/src" +WORK="$(mktemp -d)" +trap 'rm -rf "${WORK}"' EXIT + +rule() { printf '\n\033[1m%s\033[0m\n%s\n' "$1" "$(printf '─%.0s' {1..78})"; } + +# ── 1. Unnamed module: every permitted subtype must be in the same package ─────────────────────── +rule "1. Sealed across packages, WITHOUT module-info.java (the unnamed module)" +javac --release 22 -d "${WORK}/out1" \ + $(find "${SRC}" -name '*.java' ! -name 'module-info.java') 2>&1 | sed 's|^.*/src/|src/|' +echo "exit: ${PIPESTATUS[0]} (non-zero expected — this is the wall)" + +# ── 2. Named module: permits may cross packages ────────────────────────────────────────────────── +rule "2. The very same sources, WITH module-info.java" +javac --release 22 -d "${WORK}/out2" \ + $(find "${SRC}" -name '*.java') 2>&1 | sed 's|^.*/src/|src/|' +echo "exit: ${PIPESTATUS[0]} (0 expected — sealed across packages, and the resolver has no default)" + +# ── 3. Add an asset type; nobody touches the resolver ──────────────────────────────────────────── +rule "3. One more permitted type — WorkflowAction — and the resolver is left untouched" +cp -r "${SRC}" "${WORK}/src3" +mkdir -p "${WORK}/src3/com/dotmarketing/portlets/workflows/model" +cat > "${WORK}/src3/com/dotmarketing/portlets/workflows/model/WorkflowAction.java" <<'JAVA' +package com.dotmarketing.portlets.workflows.model; + +import com.dotmarketing.business.Permissionable; + +public final class WorkflowAction implements Permissionable { + + public String getPermissionType() { + return "WorkflowAction"; + } +} +JAVA +# add it to the permits clause, and nothing else +perl -0pi -e 's/permits Contentlet, Folder, Identifier, Inode \{/permits Contentlet, Folder, Identifier, Inode,\n com.dotmarketing.portlets.workflows.model.WorkflowAction {/' \ + "${WORK}/src3/com/dotmarketing/business/Permissionable.java" + +javac --release 22 -d "${WORK}/out3" \ + $(find "${WORK}/src3" -name '*.java') 2>&1 | sed "s|^${WORK}/src3/|src/|" +echo "exit: ${PIPESTATUS[0]} (non-zero expected — the compiler hands you the work list)" diff --git a/docs/experiments/sealed-permissionable/src/com/dotmarketing/beans/Host.java b/docs/experiments/sealed-permissionable/src/com/dotmarketing/beans/Host.java new file mode 100644 index 00000000000..878073455c1 --- /dev/null +++ b/docs/experiments/sealed-permissionable/src/com/dotmarketing/beans/Host.java @@ -0,0 +1,7 @@ +package com.dotmarketing.beans; + +import com.dotmarketing.portlets.contentlet.model.Contentlet; + +/** A leaf: the hierarchy ends here. */ +public final class Host extends Contentlet { +} diff --git a/docs/experiments/sealed-permissionable/src/com/dotmarketing/beans/Identifier.java b/docs/experiments/sealed-permissionable/src/com/dotmarketing/beans/Identifier.java new file mode 100644 index 00000000000..55f0757daf2 --- /dev/null +++ b/docs/experiments/sealed-permissionable/src/com/dotmarketing/beans/Identifier.java @@ -0,0 +1,7 @@ +package com.dotmarketing.beans; + +import com.dotmarketing.business.Permissionable; + +public final class Identifier implements Permissionable { + public String getPermissionType() { return "Identifier"; } +} diff --git a/docs/experiments/sealed-permissionable/src/com/dotmarketing/beans/Inode.java b/docs/experiments/sealed-permissionable/src/com/dotmarketing/beans/Inode.java new file mode 100644 index 00000000000..e2dc883ba44 --- /dev/null +++ b/docs/experiments/sealed-permissionable/src/com/dotmarketing/beans/Inode.java @@ -0,0 +1,15 @@ +package com.dotmarketing.beans; + +import com.dotmarketing.business.Permissionable; + +/** + * Gives up and reopens the branch — Category, Field, WebAsset and UserComment all extend the real + * one. Note this does NOT break exhaustiveness downstream: every subclass of Inode is still an + * Inode, so a single `case Inode` covers all of them. + */ +public non-sealed class Inode implements Permissionable { + + public String getPermissionType() { + return "Inode"; + } +} diff --git a/docs/experiments/sealed-permissionable/src/com/dotmarketing/business/PermissionResolver.java b/docs/experiments/sealed-permissionable/src/com/dotmarketing/business/PermissionResolver.java new file mode 100644 index 00000000000..c35c1406d30 --- /dev/null +++ b/docs/experiments/sealed-permissionable/src/com/dotmarketing/business/PermissionResolver.java @@ -0,0 +1,38 @@ +package com.dotmarketing.business; + +import com.dotmarketing.beans.Host; +import com.dotmarketing.beans.Identifier; +import com.dotmarketing.beans.Inode; +import com.dotmarketing.portlets.contentlet.model.Contentlet; +import com.dotmarketing.portlets.folders.model.Folder; + +/** + * The shape PermissionBitFactoryImpl#resolvePermissionType could take if the hierarchy were sealed. + */ +public final class PermissionResolver { + + private PermissionResolver() { + } + + /** + * The same switch as the real resolver — with no `default`. + * + *

Host precedes Contentlet because Host extends it; the compiler rejects the other order with + * "this case label is dominated by a preceding case label".

+ */ + public static String resolve(final Permissionable permissionable) { + + return switch (permissionable) { + + case Host _ -> "Host"; + + case Contentlet _ -> "Contentlet"; + + case Folder _ -> "Folder"; + + case Identifier _ -> "Identifier"; + + case Inode _ -> "Inode"; + }; + } +} diff --git a/docs/experiments/sealed-permissionable/src/com/dotmarketing/business/Permissionable.java b/docs/experiments/sealed-permissionable/src/com/dotmarketing/business/Permissionable.java new file mode 100644 index 00000000000..bb09a844296 --- /dev/null +++ b/docs/experiments/sealed-permissionable/src/com/dotmarketing/business/Permissionable.java @@ -0,0 +1,15 @@ +package com.dotmarketing.business; + +import com.dotmarketing.beans.Identifier; +import com.dotmarketing.beans.Inode; +import com.dotmarketing.portlets.contentlet.model.Contentlet; +import com.dotmarketing.portlets.folders.model.Folder; + +/** + * Sealed, with its permitted subtypes in OTHER packages — mirroring the real dotCMS layout, where + * fourteen implementors are spread across nine packages. + */ +public sealed interface Permissionable permits Contentlet, Folder, Identifier, Inode { + + String getPermissionType(); +} diff --git a/docs/experiments/sealed-permissionable/src/com/dotmarketing/portlets/contentlet/model/Contentlet.java b/docs/experiments/sealed-permissionable/src/com/dotmarketing/portlets/contentlet/model/Contentlet.java new file mode 100644 index 00000000000..51512ffcffb --- /dev/null +++ b/docs/experiments/sealed-permissionable/src/com/dotmarketing/portlets/contentlet/model/Contentlet.java @@ -0,0 +1,12 @@ +package com.dotmarketing.portlets.contentlet.model; + +import com.dotmarketing.beans.Host; +import com.dotmarketing.business.Permissionable; + +/** Seals further down: the branch stays closed. */ +public sealed class Contentlet implements Permissionable permits Host { + + public String getPermissionType() { + return "Contentlet"; + } +} diff --git a/docs/experiments/sealed-permissionable/src/com/dotmarketing/portlets/folders/model/Folder.java b/docs/experiments/sealed-permissionable/src/com/dotmarketing/portlets/folders/model/Folder.java new file mode 100644 index 00000000000..39ed9a005fd --- /dev/null +++ b/docs/experiments/sealed-permissionable/src/com/dotmarketing/portlets/folders/model/Folder.java @@ -0,0 +1,7 @@ +package com.dotmarketing.portlets.folders.model; + +import com.dotmarketing.business.Permissionable; + +public final class Folder implements Permissionable { + public String getPermissionType() { return "Folder"; } +} diff --git a/docs/experiments/sealed-permissionable/src/module-info.java b/docs/experiments/sealed-permissionable/src/module-info.java new file mode 100644 index 00000000000..49a3215f0ad --- /dev/null +++ b/docs/experiments/sealed-permissionable/src/module-info.java @@ -0,0 +1,10 @@ +/** + * The one thing that unlocks everything below. + * + *

Delete this file and the very same sources stop compiling: a sealed type in the unnamed module + * requires every permitted subtype to live in the same package. Inside a named module, `permits` may + * cross packages. That is the whole difference between experiment 1 and experiment 2.

+ */ +module dotcms.permissions { + exports com.dotmarketing.business; +}