From bb8562a1ad546b4325c845bdf335363e1ecedd94 Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Fri, 7 Aug 2026 15:40:17 -0600 Subject: [PATCH 1/2] docs(folders): record the shape trade-off on FolderSearchParams (#34154) Documentation only. No behaviour, signature or field changes. FolderSearchParams carries ten flat components. Because the record is public, its canonical constructor cannot be declared less accessible than the record itself, so there is always a callable ten-argument positional entry point and builder() is a convenience rather than a gate. Several components sit adjacent to another of the same type: name/path, recursive/respectFrontendRoles, limit/offset. A transposed argument list therefore compiles and silently searches for the wrong thing. Validating inside the canonical constructor does not help with that, because each transposed value is individually valid and there is nothing for a check to reject. The javadoc records the grouping that would remove the hazard by typing rather than by discipline (FolderCriteria / Requester / PageRequest), together with the reason it is not applied: the type appears in the FolderAPI.searchFolders signature, so reshaping it is a public API change. Written down so that keeping the flat shape stays a deliberate choice rather than an oversight. Also noted, and independent of the shape: the siteId and user null checks live in Builder.build(), which a direct call to the canonical constructor bypasses. Co-Authored-By: Claude Opus 5 (1M context) --- .../folders/business/FolderSearchParams.java | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderSearchParams.java b/dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderSearchParams.java index 6064befa36a..e4f0a7ca672 100644 --- a/dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderSearchParams.java +++ b/dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderSearchParams.java @@ -10,6 +10,38 @@ *

{@code includePermissions} opts into per-folder permission computation: when {@code false} * (the default) the resulting views carry a {@code null} permission list and no extra permission * query is issued. + * + *

Design note: this record carries eleven flat components

+ * + *

A record's canonical constructor is positional, and because this record is {@code public} the + * canonical constructor cannot be declared less accessible than the record itself. So there is always + * a callable eleven-argument entry point, and {@link #builder()} is a convenience rather than a + * gate.

+ * + *

Three pairs of adjacent components share a type — {@code name} / {@code path}, + * {@code limit} / {@code offset}, {@code orderBy} / {@code orderDirection} — and three more components + * are {@code boolean} ({@code recursive}, {@code respectFrontendRoles}, {@code includePermissions}). A + * transposed argument list therefore compiles and silently searches for the wrong thing, or quietly + * flips a permission behaviour. Validating inside the canonical constructor does not help with this: + * each transposed value is individually valid, so there is nothing for a check to reject.

+ * + *

The components do fall into natural groups, which would remove the hazard by typing instead of by + * discipline — a {@code PageRequest} cannot be passed where a {@code FolderCriteria} is expected:

+ * + * 
{@code
+ * public record FolderSearchParams(
+ *         FolderCriteria criteria,   // name, path, recursive
+ *         Requester requester,       // user, respectFrontendRoles, includePermissions
+ *         PageRequest page) {        // limit, offset, sortColumn, sortDirection
+ * }
+ * }
+ * + *

Not applied here: this type appears in the {@link FolderAPI#searchFolders} signature, so + * reshaping it is a public API change. Recorded so that keeping the flat shape stays a deliberate + * choice rather than an oversight — and note that the component count has already grown once.

+ * + *

Related, and independent of the shape: the {@code siteId} and {@code user} null checks currently + * live in {@link Builder#build()}, which a direct call to the canonical constructor bypasses.

*/ public record FolderSearchParams( String name, From 2c869514eb718c33d823ada4e5558dfbd61c9e86 Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Fri, 7 Aug 2026 16:03:56 -0600 Subject: [PATCH 2/2] fix(folders): enforce FolderSearchParams required fields in the canonical constructor (#34154) The siteId and user null checks lived in Builder.build(). A record's canonical constructor cannot be declared less accessible than the record itself, so for a public record there is always a public positional entry point and the builder can only ever be a convenience, never a gate. A check in build() guarded the callers who happened to use the builder; new FolderSearchParams(...) walked past it. Moved both checks into the canonical constructor, which every construction path goes through, the builder's own included. Behaviour is unchanged for existing callers: same exception type (NullPointerException, via Objects.requireNonNull), same messages, and build() still rejects the same inputs because it now delegates instead of carrying its own copy of the check. Nothing in the repository constructs the record directly other than build() itself, so no caller changes. Tests: 15 green. - FolderSearchParamsTest (5 new unit tests). The one that matters is test_directConstruction_enforcesRequiredFields: it exercises the path a check in build() cannot cover, and would have passed silently before this change. The rest pin what must NOT have shifted - the same exception and message from the builder, the builder's defaults, and that the optional components may still be null. - FolderSearchPaginatorTest (10 existing) unchanged and passing. The javadoc design note added earlier in this branch is updated to describe where the invariants now live and why, instead of flagging them as bypassable. Co-Authored-By: Claude Opus 5 (1M context) --- .../folders/business/FolderSearchParams.java | 21 +++- .../business/FolderSearchParamsTest.java | 117 ++++++++++++++++++ 2 files changed, 134 insertions(+), 4 deletions(-) create mode 100644 dotCMS/src/test/java/com/dotmarketing/portlets/folders/business/FolderSearchParamsTest.java diff --git a/dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderSearchParams.java b/dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderSearchParams.java index e4f0a7ca672..e9ea4783e4a 100644 --- a/dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderSearchParams.java +++ b/dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderSearchParams.java @@ -40,8 +40,10 @@ * reshaping it is a public API change. Recorded so that keeping the flat shape stays a deliberate * choice rather than an oversight — and note that the component count has already grown once.

* - *

Related, and independent of the shape: the {@code siteId} and {@code user} null checks currently - * live in {@link Builder#build()}, which a direct call to the canonical constructor bypasses.

+ *

For the same reason, the required-field checks live in the canonical constructor and not in + * {@link Builder#build()}: a check in the builder only guards callers who happen to use the builder, + * while a check in the canonical constructor guards every construction path, including the builder's + * own.

*/ public record FolderSearchParams( String name, @@ -56,6 +58,16 @@ public record FolderSearchParams( String orderDirection, boolean includePermissions) { + /** + * Canonical constructor. The required-field checks live here rather than in + * {@link Builder#build()} because this is the only construction path that cannot be bypassed: the + * builder delegates to it, and so does any direct {@code new FolderSearchParams(...)} call. + */ + public FolderSearchParams { + Objects.requireNonNull(siteId, "siteId is required"); + Objects.requireNonNull(user, "user is required"); + } + public static Builder builder() { return new Builder(); } @@ -87,9 +99,10 @@ private Builder() {} public Builder orderDirection(final String orderDirection) { this.orderDirection = orderDirection; return this; } public Builder includePermissions(final boolean includePermissions) { this.includePermissions = includePermissions; return this; } + /** + * Delegates to the canonical constructor, which enforces the required fields. + */ public FolderSearchParams build() { - Objects.requireNonNull(siteId, "siteId is required"); - Objects.requireNonNull(user, "user is required"); return new FolderSearchParams(name, path, recursive, siteId, user, respectFrontendRoles, limit, offset, orderBy, orderDirection, includePermissions); } diff --git a/dotCMS/src/test/java/com/dotmarketing/portlets/folders/business/FolderSearchParamsTest.java b/dotCMS/src/test/java/com/dotmarketing/portlets/folders/business/FolderSearchParamsTest.java new file mode 100644 index 00000000000..d42c3fae0d4 --- /dev/null +++ b/dotCMS/src/test/java/com/dotmarketing/portlets/folders/business/FolderSearchParamsTest.java @@ -0,0 +1,117 @@ +package com.dotmarketing.portlets.folders.business; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThrows; + +import com.liferay.portal.model.User; +import org.junit.Test; + +/** + * Unit tests for the required-field invariants of {@link FolderSearchParams}. + * + *

The point of these tests is where the checks are enforced, not merely that they exist. A + * record's canonical constructor cannot be declared less accessible than the record itself, so for a + * {@code public record} there is always a public positional entry point and + * {@link FolderSearchParams#builder()} can only ever be a convenience. A check placed in + * {@code Builder.build()} guards the callers who happen to use the builder; a check in the canonical + * constructor guards every construction path, the builder's own included.

+ * + *

{@link #test_directConstruction_enforcesRequiredFields()} is therefore the test that matters: it + * would have passed silently before the checks moved.

+ * + * @author Fabrizio Araya + */ +public class FolderSearchParamsTest { + + private static final String SITE_ID = "48190c8c-42c4-46af-8d1a-0cd5db894797"; + + /** + * Method to test: {@link FolderSearchParams#FolderSearchParams(String, String, boolean, String, User, boolean, int, int, String, String, boolean)} + * Given scenario: the canonical constructor is called directly, bypassing the builder, with a + * missing {@code siteId} and then with a missing {@code user}. + * Expected result: it rejects both. This is the case a check in {@code Builder.build()} cannot + * cover, and the reason the invariant belongs on the constructor. + */ + @Test + public void test_directConstruction_enforcesRequiredFields() { + final NullPointerException noSite = assertThrows(NullPointerException.class, + () -> new FolderSearchParams("name", "/", false, null, new User(), false, + 40, 0, "folder.name", "ASC", false)); + assertEquals("siteId is required", noSite.getMessage()); + + final NullPointerException noUser = assertThrows(NullPointerException.class, + () -> new FolderSearchParams("name", "/", false, SITE_ID, null, false, + 40, 0, "folder.name", "ASC", false)); + assertEquals("user is required", noUser.getMessage()); + } + + /** + * Method to test: {@link FolderSearchParams.Builder#build()} + * Given scenario: the builder is used without setting {@code siteId}. + * Expected result: it still rejects, now because it delegates to the canonical constructor rather + * than because it carries its own copy of the check. Same exception type and message as before, so + * the behaviour callers see is unchanged. + */ + @Test + public void test_builderWithoutSiteId_stillRejects() { + final NullPointerException thrown = assertThrows(NullPointerException.class, + () -> FolderSearchParams.builder().user(new User()).build()); + + assertEquals("siteId is required", thrown.getMessage()); + } + + /** + * Method to test: {@link FolderSearchParams.Builder#build()} + * Given scenario: the builder is used without setting {@code user}. + * Expected result: rejected, with the message unchanged. + */ + @Test + public void test_builderWithoutUser_stillRejects() { + final NullPointerException thrown = assertThrows(NullPointerException.class, + () -> FolderSearchParams.builder().siteId(SITE_ID).build()); + + assertEquals("user is required", thrown.getMessage()); + } + + /** + * Method to test: {@link FolderSearchParams.Builder#build()} + * Given scenario: only the two required fields are set. + * Expected result: the builder's documented defaults survive the move — nothing about them was + * folded into the constructor. + */ + @Test + public void test_builderDefaults_areUnchanged() { + final User user = new User(); + final FolderSearchParams params = FolderSearchParams.builder() + .siteId(SITE_ID) + .user(user) + .build(); + + assertEquals(SITE_ID, params.siteId()); + // assertSame, not assertEquals: User.equals() dereferences a primary key a bare User lacks. + assertSame(user, params.user()); + assertEquals("/", params.path()); + assertEquals(false, params.recursive()); + assertEquals(false, params.respectFrontendRoles()); + assertEquals(40, params.limit()); + assertEquals(0, params.offset()); + assertEquals("folder.name", params.orderBy()); + assertEquals("ASC", params.orderDirection()); + assertEquals(false, params.includePermissions()); + } + + /** + * Method to test: {@link FolderSearchParams#FolderSearchParams(String, String, boolean, String, User, boolean, int, int, String, String, boolean)} + * Given scenario: an optional component is left null. + * Expected result: accepted. Only {@code siteId} and {@code user} are required, so the move must + * not have tightened anything else — {@code name} being null is how "no name filter" is expressed. + */ + @Test + public void test_optionalComponentsMayBeNull() { + final FolderSearchParams params = new FolderSearchParams(null, "/", false, SITE_ID, + new User(), false, 40, 0, "folder.name", "ASC", false); + + assertEquals(null, params.name()); + } +}