From 34801297382a57ea1d1bf5d2405689fb855b2862 Mon Sep 17 00:00:00 2001 From: Arcadio Quintero Date: Thu, 10 Sep 2026 17:44:47 -0400 Subject: [PATCH 01/10] spec(experiments): creator name on the Experiments API (#37304) Spec for exposing the experiment creator's display name so the Experiments portlet listing can render a Created By column without resolving user IDs itself (consumer: #37307). Additive and non-breaking: a createdByUserName field joins the existing createdBy, which keeps its key, its value and its role as the permission owner. The field is added at the Experiment level, so all 14 endpoints that return an Experiment carry it. lastModifiedBy is deliberately out of scope. Grounded against the code rather than the issue text: - the issue lists DELETE /v1/experiments/{id} as carrying an Experiment; it returns a confirmation message. Two endpoints that do return one were omitted (DELETE .../goals/primary, DELETE .../targetingConditions/{id}). The spec's endpoint table supersedes the issue list (A7). - user lookups are already cache-backed via UserCache (A1), which is what the no-N-lookups requirement rests on. - a @Schema on an Immutables abstract accessor does reach openapi.yaml (verified via AbstractTimestampsView), so no annotation pass over ExperimentsResource is needed (A3). - the immutables mechanism is left to the plan phase and boxed in by four requirements: eager derivation would fire on non-serializing paths, and a memoized lazy value would outlive a rename (A2). 23 FRs, 7 SCs, 3 user stories, 8 assumptions. --- .../spec.md | 349 ++++++++++++++++++ 1 file changed, 349 insertions(+) create mode 100644 specs/37304-experiment-created-by-username/spec.md diff --git a/specs/37304-experiment-created-by-username/spec.md b/specs/37304-experiment-created-by-username/spec.md new file mode 100644 index 00000000000..f09f3a97dbb --- /dev/null +++ b/specs/37304-experiment-created-by-username/spec.md @@ -0,0 +1,349 @@ +# Feature Specification: Creator Name on the Experiments API + +**Feature Branch**: `issue-37304-experiment-created-by-username` + +**Created**: 2026-09-10 + +**Status**: Draft + +**Type**: Task (additive, non-breaking API contract change) + +**Epic**: [#36763 — Experiments: A/B Testing v2](https://github.com/dotCMS/core/issues/36763) + +**Work item**: [dotCMS/core#37304 — Expose the experiment creator's username in the Experiments API](https://github.com/dotCMS/core/issues/37304) + +**Input**: User description: "Expose the experiment creator's username in the Experiments API" — taken from issue #37304. + +--- + +## Scope Note *(read this first)* + +Every Experiment already records who created it, but only as an opaque user ID. The payload says +`"createdBy": "dotcms.org.1"` and nothing else. That ID is not a name: it is the same value the +permission layer uses as the experiment's owner, and it means nothing to the person reading a +listing. + +The new Experiments portlet wants a **Created By** column. With today's contract the portlet has two +bad options: render the raw ID, or issue a second round of requests to translate every ID it sees +into a name. Both push work onto the client that the server can do once, cheaply, from data it has +already loaded. + +This work adds one field beside the existing one: `createdByUserName`, carrying the creator's full +name. It is **additive**. `createdBy` keeps its key, keeps its value and keeps its meaning, and the +owner/permission behaviour that reads it is untouched. Nothing that consumes the current payload has +to change. + +Because the field belongs to the Experiment itself and not to one endpoint's response shape, it +arrives on *every* response that carries an Experiment — the listing, the single fetch, and each of +the lifecycle and variant operations that return the updated experiment. + +Two things this work deliberately does **not** do. It does not touch `lastModifiedBy`, which has the +exact same ID-not-name problem and is left for a follow-up. And it does not store the name: the +value is resolved when the experiment is read, so an experiment created by someone who later changes +their name does not keep serving the old one. + +--- + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - See who created each experiment in the listing (Priority: P1) + +A marketer opens the Experiments portlet and scans the list of experiments for the site. Each row +tells them who created that experiment by name — "Admin User", not "dotcms.org.1" — so they can find +their own work, or find the colleague to ask about someone else's. + +**Why this priority**: This is the whole point of the change and the reason the consumer issue +(#37307, listing Created By column) is blocked. The listing is where an ID is least useful, because +the reader is comparing many rows at once. + +**Independent Test**: Create experiments owned by two different known users, request the experiment +list, and confirm each entry reports the corresponding user's full name. + +**Acceptance Scenarios**: + +1. **Given** an experiment created by a user whose full name is "Admin User", **When** the experiment + list is requested, **Then** that experiment's entry reports `createdBy` unchanged as the user ID + **and** reports the creator's name as "Admin User". +2. **Given** a list containing experiments created by several different users, **When** the list is + requested, **Then** each entry reports the name of its own creator. +3. **Given** a list containing many experiments created by the *same* user, **When** the list is + requested, **Then** every entry reports that user's name and the response is served without a + perceptible slowdown compared with the same list before this change. + +--- + +### User Story 2 - The name travels with the experiment everywhere (Priority: P2) + +A client that has just created, started, ended, archived or otherwise modified an experiment gets the +updated experiment back and can render the creator's name from that response alone, without a +follow-up fetch and without special-casing which operation it just performed. + +**Why this priority**: Without it, a portlet that renders a row from a create/update response and a +row from the listing has to handle two different shapes for the same object. It is also what makes +the change cheap: one place to resolve the name, not fourteen. + +**Independent Test**: Exercise each endpoint whose response carries an Experiment and confirm the +creator's name is present in each one. + +**Acceptance Scenarios**: + +1. **Given** an existing experiment, **When** it is fetched by ID, **Then** the response carries the + creator's name. +2. **Given** an existing draft experiment, **When** it is started, ended, cancelled, archived, + updated, or has a variant added, removed, renamed or promoted, **Then** each of those responses + carries the creator's name for that experiment. +3. **Given** an experiment created through the create endpoint, **When** the create response is read, + **Then** it already carries the creator's name — no second request is needed. + +--- + +### User Story 3 - The column is never blank (Priority: P3) + +An administrator looks at a listing that includes experiments created by a user who has since been +deleted, and by the system itself. Every row still shows something identifying in the Created By +column, and no row is missing or empty. The listing loads normally. + +**Why this priority**: A rare data condition must not produce a broken-looking column or, worse, a +failed request that hides every other experiment in the list. + +**Independent Test**: Point an experiment's `createdBy` at a user ID that no longer resolves, request +both the list and the single fetch, and confirm the response succeeds and reports the raw ID. + +**Acceptance Scenarios**: + +1. **Given** an experiment whose creator ID does not resolve to any user, **When** the experiment is + requested, **Then** the request succeeds and the creator name field carries the raw creator ID. +2. **Given** an experiment created by the system user, **When** the experiment is requested, **Then** + the request succeeds and the creator name field carries a non-empty value. +3. **Given** a list where one experiment has an unresolvable creator and the others do not, **When** + the list is requested, **Then** the request succeeds, the resolvable entries report real names and + only the affected entry falls back to the raw ID. + +--- + +### Edge Cases + +- **Creator no longer exists** (deleted user, orphaned reference): the field falls back to the raw + creator ID. It is never null, never absent and never an empty string. +- **Creator resolves but has no usable name** (first, middle and last name all blank): treated the + same as unresolvable — fall back to the raw creator ID, so the column still identifies something. +- **Creator is the system user**: resolves normally; if the system user has no usable name, the + fallback rule applies. The request never fails because of it. +- **User lookup fails for an infrastructure reason** (database error, cache error): the experiment is + still returned successfully with the fallback value. A failure to decorate must never turn a + successful experiment read into a failed request. +- **Same creator repeated across a long list**: resolving the name must not cost one uncached lookup + per row. +- **Creator renames themselves**: subsequent reads report the new name. The value is resolved from + current user data at read time, not captured when the experiment was created. +- **An experiment payload is read back in** (a client echoing a response, a future import path): the + extra field must not make that payload unreadable. +- **Experiment with no variants / archived / ended**: status has no bearing on the field; it is + present in every state. + +--- + +## Requirements *(mandatory)* + +### Functional Requirements + +**The field** + +- **FR-001**: Every API response that carries an Experiment MUST include a creator-name field, + `createdByUserName`, alongside the existing `createdBy`. +- **FR-002**: `createdByUserName` MUST carry the full name of the user identified by that + experiment's `createdBy` value. +- **FR-003**: `createdByUserName` MUST always be present and non-empty. It is never null, never + omitted, and never an empty string. +- **FR-004**: The field MUST be a single display-ready string. The API is not required to expose + first name and last name separately. + +**Nothing existing changes** + +- **FR-005**: `createdBy` MUST keep its current JSON key and its current value (the user ID). No + consumer of the present contract may break. +- **FR-006**: Owner and permission behaviour MUST be unchanged: the experiment's owner continues to + resolve from `createdBy`, never from the new field. +- **FR-007**: No other existing Experiment field may be removed, renamed, retyped or reordered by + this work. The change is strictly additive. +- **FR-008**: `lastModifiedBy` MUST be left exactly as it is today — same key, same value, no name + companion. It is out of scope (see Out of Scope). + +**Fallback and failure** + +- **FR-009**: When the creator ID does not resolve to a user, `createdByUserName` MUST fall back to + the raw creator ID. +- **FR-010**: When the creator resolves to a user whose name is blank, `createdByUserName` MUST fall + back to the raw creator ID under the same rule as FR-009. +- **FR-011**: A failure to resolve the creator MUST NOT fail the experiment request. The endpoint + still returns its normal success response with the experiment payload and the fallback value. +- **FR-012**: A failure to resolve one experiment's creator inside a list MUST NOT affect the other + entries in that list. +- **FR-013**: Resolution failures MUST be observable to an operator (logged), without the log line + being emitted once per row of a large listing. + +**Cost and freshness** + +- **FR-014**: Serving a list of N experiments MUST NOT perform N uncached user lookups. Resolution is + per distinct creator, and repeated creators cost no additional database work. +- **FR-015**: The change MUST NOT add user lookups to code paths that do not serialize an Experiment + — in particular the running-experiment selection performed during page rendering, and the + push-publish dependency walk. Reading an experiment for those purposes must cost what it costs + today. +- **FR-016**: `createdByUserName` MUST reflect the creator's current name. The name MUST NOT be + captured at experiment-creation time and stored beside the experiment. +- **FR-017**: This work MUST NOT introduce a database schema change. No new column, no migration, no + upgrade task. + +**Contract, docs and tests** + +- **FR-018**: `createdByUserName` MUST be documented in the API schema for the Experiment, with a + description that states it is the creator's display name and that it falls back to the creator ID. + The documented type MUST match what is actually returned. +- **FR-019**: The committed `openapi.yaml` MUST be regenerated from the annotations and committed + together with the code change, so the CI contract check passes. +- **FR-020**: Integration tests MUST cover: the happy path from the list endpoint, the happy path + from the single-fetch endpoint, and the unresolvable-creator fallback returning success plus the + raw ID. +- **FR-021**: Every new integration test class MUST be registered in a `MainSuite*` / `Junit5Suite*` + `@SuiteClasses` list, otherwise it never runs in CI. +- **FR-022**: Existing experiment tests MUST continue to pass unchanged, in particular any asserting + on the shape or value of `createdBy`. +- **FR-023**: An Experiment payload that contains `createdByUserName` MUST still be readable wherever + Experiment JSON is parsed back into an Experiment. The added field must not make a round-tripped + payload fail to parse. + +### Endpoints in scope + +Every endpoint below returns a payload carrying one or more Experiments and is therefore covered by +FR-001. This list is taken from the resource as it exists today, not from the issue body (see +Assumptions, A7): + +| # | Endpoint | +|---|---| +| 1 | `POST /v1/experiments` (create) | +| 2 | `PATCH /v1/experiments/{experimentId}` (partial update) | +| 3 | `PUT /v1/experiments/{experimentId}/_archive` | +| 4 | `GET /v1/experiments/{id}` | +| 5 | `GET /v1/experiments` (list) | +| 6 | `DELETE /v1/experiments/{experimentId}/goals/primary` | +| 7 | `POST /v1/experiments/{experimentId}/_start` | +| 8 | `POST /v1/experiments/{experimentId}/_end` | +| 9 | `POST /v1/experiments/scheduled/{experimentId}/_cancel` | +| 10 | `POST /v1/experiments/{experimentId}/variants` | +| 11 | `DELETE /v1/experiments/{experimentId}/variants/{name}` | +| 12 | `PUT /v1/experiments/{experimentId}/variants/{name}` | +| 13 | `PUT /v1/experiments/{experimentId}/variants/{name}/_promote` | +| 14 | `DELETE /v1/experiments/{experimentId}/targetingConditions/{id}` | + +Endpoints under `/v1/experiments` that do **not** carry an Experiment, and are therefore untouched: +`DELETE /v1/experiments/{experimentId}` (returns a confirmation message), `POST /v1/experiments/isUserIncluded`, +`GET /v1/experiments/{id}/results`, `GET /v1/experiments/health`. + +### Key Entities + +- **Experiment**: the object being serialized. Already carries `createdBy` (the creator's user ID, + also used as the permission owner) and `lastModifiedBy`. Gains one read-time field, + `createdByUserName`, that is derived from `createdBy` and not persisted. +- **User**: the creator behind `createdBy`. Supplies the display name. May not exist any more, and + may have no usable name; both cases resolve to the fallback rule. + +--- + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: A reader of an experiment listing can identify the creator of every row by name, + without the client making any request beyond the one that returned the list. +- **SC-002**: 100% of the 14 endpoints listed under "Endpoints in scope" return the creator name + field; 0% of the endpoints outside that list change shape. +- **SC-003**: 0 existing Experiment fields change key, value, type or meaning — verified by the + existing experiment test suite passing without modification. +- **SC-004**: For a listing of 50 experiments created by a single user, the response time is + indistinguishable from the same listing before this change (within normal run-to-run variation), + and the number of user records read from the database is at most 1. +- **SC-005**: 100% of requests for an experiment whose creator cannot be resolved still succeed and + report a non-empty creator name value. +- **SC-006**: The published API documentation describes the new field, and the repository's generated + contract file matches what the build produces — the CI contract check passes on the first run. +- **SC-007**: The consumer work (#37307, listing Created By column) can be built against the + documented contract with no further backend change. + +--- + +## Assumptions + +- **A1 — User lookups are already cache-backed.** Loading a user by ID consults the user cache before + hitting the database (`UserFactoryLiferayImpl.loadUserById` reads `UserCache` first and populates + it on a miss). A listing whose experiments share a creator therefore costs one database read for + the first row and none afterwards, which is what FR-014 relies on. FR-014 still stands as a + requirement rather than a freebie, because the cache is not infinite and a listing spanning many + distinct creators must not degenerate into one lookup per row. + +- **A2 — The mechanism for a derived JSON field is a plan-phase decision, and it is constrained.** + The Experiment model is an Immutables value type serialized by Jackson. The two obvious immutables + mechanisms behave differently and neither is free: + - an eagerly-derived attribute is computed at build time, so it would fire on *every* Experiment + construction, including the ones that never reach a response (the database transformer, the + push-publish dependency walk, running-experiment selection during page rendering) — which + FR-015 forbids; + - a lazily-computed attribute is memoized inside the instance, and experiment instances are + themselves cached, so a memoized name would outlive a rename — which FR-016 forbids. + + Neither mechanism is settable from JSON, which is where FR-023 comes from. The spec therefore + states the constraints (FR-014, FR-015, FR-016, FR-023) and leaves the mechanism to `/speckit-plan`, + which must verify its choice against all four before committing to it. + +- **A3 — Documenting the field needs no new endpoint annotations.** `ExperimentsResource` today + carries only a `@Tag`; it has no `@Operation`/`@ApiResponse` annotations at all, and the Experiment + schema in `openapi.yaml` is derived from the model. A schema annotation placed on the model's + accessor does reach the generated contract — verified against `AbstractTimestampsView`, whose + per-accessor descriptions and examples appear verbatim under the `TimestampsView` schema. So + FR-018 is satisfied by annotating the model, and adding a full Swagger annotation pass to + `ExperimentsResource` is **not** part of this work. + +- **A4 — "Full name" means the platform's existing notion of a full name**: first, middle and last + name joined as `User.getFullName()` already does, rather than a new formatting rule invented here. + That method returns an empty string when every part is blank, which is exactly the case FR-010 + covers. + +- **A5 — There is no persistence round-trip to break.** Experiments are rebuilt from database columns + by a transformer, not by parsing stored Experiment JSON, and the push-publish wrapper for + experiments has no bundler or handler wired to it today. FR-023 is therefore a guard against future + and client-side round-trips, not a description of a path that runs on every read. + +- **A6 — The consumer needs one display string.** #37307 renders a single Created By column, so a + single pre-joined name is sufficient; separate first/last fields are not required. + +- **A7 — The issue's endpoint list is close but not exact**, and the table above supersedes it. The + issue lists `delete` among the endpoints that carry an Experiment; `DELETE /v1/experiments/{id}` + actually returns a confirmation message and no experiment. The issue also omits two endpoints that + *do* return an Experiment: `DELETE /v1/experiments/{experimentId}/goals/primary` and + `DELETE /v1/experiments/{experimentId}/targetingConditions/{id}`. Both differences are corrections + to the issue text only — they do not change the approach, because the field is added once at the + Experiment level and every experiment-carrying response inherits it. + +- **A8 — Backend only.** No frontend work belongs to this issue; the portlet column is #37307. + +--- + +## Out of Scope + +- **`lastModifiedBy`.** It has the same ID-not-name problem and is deliberately excluded. If the + portlet later surfaces a "last modified by" column, it can be added with the same mechanism as a + follow-up issue. +- **The portlet's Created By column itself** (#37307) — this work only supplies the data. +- **Storing the creator's name.** No new column, no migration, no denormalized copy (FR-016, FR-017). +- **Paging, sorting, counting or permission filtering on the experiment list.** Known gaps in the + list endpoint, tracked separately; this change must not attempt them and must not make them harder. +- **Names for any other user reference** in the experiment payload or elsewhere in the API. +- **A Swagger annotation pass over `ExperimentsResource`.** Only the new field is documented (A3). + +--- + +## Dependencies + +- **Consumer**: [#37307](https://github.com/dotCMS/core/issues/37307) — the Experiments portlet + listing Created By column — is blocked on this field and must not merge before it. +- **Epic**: [#36763](https://github.com/dotCMS/core/issues/36763) — Experiments: A/B Testing v2. From 6774aededde8c0270b999c4c9f63d63d9c1c9928 Mon Sep 17 00:00:00 2001 From: Arcadio Quintero Date: Tue, 15 Sep 2026 12:31:29 -0400 Subject: [PATCH 02/10] docs(experiments): plan artifacts for the creator name contract (#37304) Phase 1 artifacts for the createdByUserName field: the wire/derivation model and the contract the regenerated openapi.yaml must satisfy. plan.md, research.md and quickstart.md stay local per .gitignore. The mechanism is settled against the generated Immutables class rather than argued from docs. Compiling :dotcms-core and reading target/generated-sources/.../Experiment.java shows: - derived members are computed in the constructor, so @Value.Derived would resolve the user on every Experiment construction - the DB transformer, the withTargetingConditions rebuild on find(), the running-experiments cache fill on the page-render path, and the push-publish dependency walk; - the generated Json delegate carries settable attributes only, has no @JsonIgnoreProperties(ignoreUnknown = true), and the REST mapper leaves FAIL_ON_UNKNOWN_PROPERTIES at Jackson's default, so a serialize-only field is an unknown property on the way back in. Hence a plain interface default method annotated @JsonProperty(access = READ_ONLY): not an Immutables attribute, so no field, no memoization and no build() cost - evaluated only on serialization. @JsonAppend is the pre-approved fallback, selected by a round-trip test that runs before any implementation. Two corrections to earlier assumptions, both recorded in research.md: - the per-experiment ExperimentsCache is dead code (commented out in the factory with a TODO); only the running-experiments list cache is live, so the staleness hazard is narrower than spec A2 states while the eager construction hazard is wider. - dotCMS already resolves owner ids to names two different ways (PushedAssetHistoryTransformer -> "Deleted", BrowserAPIImpl.ownerName -> "System"/"unknown"). #37304 chose a third, the raw id. Flagged as a consistency question, not resolved here. --- .../experiment-created-by-username.md | 95 +++++++++++++++ .../data-model.md | 113 ++++++++++++++++++ 2 files changed, 208 insertions(+) create mode 100644 specs/37304-experiment-created-by-username/contracts/experiment-created-by-username.md create mode 100644 specs/37304-experiment-created-by-username/data-model.md diff --git a/specs/37304-experiment-created-by-username/contracts/experiment-created-by-username.md b/specs/37304-experiment-created-by-username/contracts/experiment-created-by-username.md new file mode 100644 index 00000000000..b68d929e72e --- /dev/null +++ b/specs/37304-experiment-created-by-username/contracts/experiment-created-by-username.md @@ -0,0 +1,95 @@ +# Contract: `createdByUserName` on the Experiment payload + +**Feature**: [../spec.md](../spec.md) | **Plan**: [../plan.md](../plan.md) + +The contract is the `Experiment` schema itself, so it is inherited by every response that embeds an +Experiment. `openapi.yaml` is generated from the model annotations — this file states what the +regenerated yaml must contain, it is not a second source of truth. + +--- + +## Schema fragment (expected in `dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml`) + +Under `components.schemas.Experiment.properties`, alphabetically between `createdBy` and +`creationDate`: + +```yaml + Experiment: + type: object + properties: + createdBy: + type: string # unchanged + createdByUserName: + type: string + description: >- + Display name of the user who created the experiment. Falls back to the raw + createdBy user id when the user cannot be resolved or has no name set. + example: Admin User + creationDate: + type: string + format: date-time +``` + +Constraints the generated yaml must satisfy: + +- `createdBy` keeps `type: string` and gains no description change (FR-005). +- No property is removed, renamed or retyped (FR-007). +- `owner`, `identifier`, `permissionId`, `manifestInfo` stay **absent** — they are `@JsonIgnore`d + derived members and must remain so (FR-006). +- `lastModifiedBy` is unchanged, with no name companion (FR-008). + +## Response wrappers (unchanged — listed to make the "no edit" explicit) + +| Wrapper | Shape | Change | +|---|---|---| +| `ResponseEntitySingleExperimentView` | `ResponseEntityView` | **None** — inherits the field | +| `ResponseEntityExperimentView` | `ResponseEntityView>` | **None** — inherits the field | + +## Endpoints that must carry the field + +All 14 inherit it without being edited. Enumerated in [../spec.md](../spec.md#endpoints-in-scope): +create, `PATCH`, `_archive`, `GET /{id}`, `GET` list, `DELETE /goals/primary`, `_start`, `_end`, +`scheduled/{id}/_cancel`, `POST /variants`, `DELETE /variants/{name}`, `PUT /variants/{name}`, +`PUT /variants/{name}/_promote`, `DELETE /targetingConditions/{id}`. + +Explicitly **not** carrying it, because they return no Experiment: +`DELETE /v1/experiments/{experimentId}` (returns the string `"Experiment deleted"`), +`POST /isUserIncluded`, `GET /{id}/results`, `GET /health`. + +## Behavioural contract + +| Condition | `createdBy` | `createdByUserName` | HTTP | +|---|---|---|---| +| Creator resolves, has a name | user id | full name (first + middle + last) | 200 | +| Creator resolves, all name parts blank | user id | the user id | 200 | +| Creator does not resolve (deleted/orphaned) | user id | the user id | 200 | +| User lookup fails (infrastructure) | user id | the user id | 200 | +| Creator is the system user | user id | resolved name, else the user id | 200 | +| One bad creator among many in a list | per entry | only the affected entry falls back | 200 | + +The field is never `null`, never absent and never `""`. + +## Deserialization contract (FR-023) + +`createdByUserName` is **read-only**: serialized on the way out, ignored on the way in. A payload +containing it must still deserialize into an `Experiment` without error. + +This is not free. The generated `Experiment.Json` delegate carries settable attributes only and is +**not** annotated `@JsonIgnoreProperties(ignoreUnknown = true)`, and the REST mapper +(`DotObjectMapperProvider.createDefaultMapper()`) leaves `FAIL_ON_UNKNOWN_PROPERTIES` at Jackson's +default — enabled. `@JsonProperty(access = READ_ONLY)` is the mechanism that makes the property +*known but not bound*; the round-trip test is what proves it, and `@JsonAppend` is the pre-approved +fallback if it does not hold. See [../research.md](../research.md) R1. + +## Verification + +```bash +# Regenerate and inspect the contract +JAVA_HOME="$HOME/.sdkman/candidates/java/25.0.2-ms" \ + ./mvnw compile -pl :dotcms-core --am -DskipTests -Ddocker.skip + +grep -A 8 '^ Experiment:' dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml +git diff --stat dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml # must show the added property +``` + +The committed yaml must match what the build produces, or the CI contract check fails (FR-019). diff --git a/specs/37304-experiment-created-by-username/data-model.md b/specs/37304-experiment-created-by-username/data-model.md new file mode 100644 index 00000000000..65d48e57827 --- /dev/null +++ b/specs/37304-experiment-created-by-username/data-model.md @@ -0,0 +1,113 @@ +# Phase 1 Data Model: Creator Name on the Experiments API + +**Feature**: [spec.md](./spec.md) | **Plan**: [plan.md](./plan.md) + +No persistent data model changes. No table, column, index, migration or upgrade task. This document +describes the **in-memory and wire** shape only. + +--- + +## Entity: Experiment (`com.dotcms.experiments.model.AbstractExperiment`) + +### Existing members that this change must not disturb + +| Member | Kind | JSON | Invariant to preserve | +|---|---|---|---| +| `createdBy()` | abstract attribute, `String` | `createdBy` | Key, value (the creator's **user id**) and meaning unchanged — FR-005. Persisted in `experiment.created_by`. | +| `getOwner()` | `@Value.Derived`, `@JsonIgnore` | *(absent)* | Still returns `createdBy()`. Permission behaviour unchanged — FR-006. Stays out of the payload. | +| `lastModifiedBy()` | abstract attribute, `String` | `lastModifiedBy` | Untouched, no name companion — FR-008. | + +### New member + +| Property | Value | +|---|---| +| **Name** | `createdByUserName` | +| **Java** | `default String createdByUserName()` on `AbstractExperiment` | +| **Immutables kind** | **None** — deliberately not `@Value.Default/Derived/Lazy`, so it is not an attribute: no field, no builder method, no `equals`/`hashCode`/`toString` participation, no `build()` cost | +| **Jackson** | `@JsonProperty(value = "createdByUserName", access = JsonProperty.Access.READ_ONLY)` | +| **Swagger** | `@Schema(description = "...", example = "Admin User")` on the same method | +| **Type** | `String`, **always non-null and non-empty** (FR-003) | +| **Persisted** | No — resolved per serialization (FR-016, FR-017) | +| **Settable** | No — read-only by design; `READ_ONLY` is what keeps an inbound payload parseable (FR-023) | +| **Derivation** | `ExperimentCreatorNameResolver.resolve(createdBy())` | + +### Evaluation timing (the property that makes the design work) + +| Path | Builds an Experiment? | Resolves the name? | +|---|---|---| +| `ExperimentTransformer` (every DB row: `find`, `list`) | Yes | **No** | +| `addTargetingConditions` → `withTargetingConditions` rebuild on `find` | Yes | **No** | +| `cacheRunningExperiments()` → running-experiments list cache (page render) | Yes | **No** — FR-015 | +| Push-publish dependency walk (`DependencyManager`) | Yes | **No** — FR-015 | +| Jackson serialization of a REST response | No | **Yes** — once per experiment per response | + +A `@Value.Derived` member would answer "Yes" in every row of that table; a `@Value.Lazy` member +would answer "Yes" once per instance and then memoize it — including in the shared, long-lived +running-experiments cache entries. See [research.md](./research.md) R1/R2. + +--- + +## Derivation rule: `ExperimentCreatorNameResolver` + +Input: the raw `createdBy` user id. Output: a non-empty display string. + +``` +resolve(createdById): + 1. createdById not set -> return createdById as-is (never null-propagate; FR-003) + 2. user = userAPI.loadUserById(createdById) # UserCache-backed (research R3) + 3. fullName = user.getFullName() # first + middle + last + 4. fullName is set -> return fullName + 5. otherwise -> return createdById # blank-name user; FR-010 + on NoSuchUserException -> return createdById # deleted / orphaned; FR-009 + on DotDataException / any other -> return createdById # never fail the request; FR-011 +``` + +**Per-entry isolation** (FR-012): the rule is applied independently per experiment, so one bad id in +a list degrades exactly one entry. + +**Logging** (FR-013): `Logger.debug` for the expected not-found case — it is a data condition, not +an error, and debug is off by default, so a listing full of orphaned creators cannot flood the log. +`Logger.warn` for an unexpected infrastructure failure, which is rare by nature and worth surfacing. + +**Testability seam**: the resolver accepts a `UserAPI` so every branch above is unit-testable with a +mock; production code goes through `APILocator.getUserAPI()`. + +--- + +## Wire shape + +Before: + +```json +{ + "id": "0e8b8b1e-...", + "name": "Homepage CTA test", + "createdBy": "dotcms.org.1", + "lastModifiedBy": "dotcms.org.1" +} +``` + +After — one added key, nothing else moved: + +```json +{ + "id": "0e8b8b1e-...", + "name": "Homepage CTA test", + "createdBy": "dotcms.org.1", + "createdByUserName": "Admin User", + "lastModifiedBy": "dotcms.org.1" +} +``` + +Unresolvable creator (deleted user, or a user whose name parts are all blank): + +```json +{ + "createdBy": "deleted-user-id-4711", + "createdByUserName": "deleted-user-id-4711" +} +``` + +Property order in real responses is alphabetical — `DotObjectMapperProvider.createDefaultMapper()` +enables `SORT_PROPERTIES_ALPHABETICALLY` when `dotcms.rest.sort.json.properties` is true (the +default), which places `createdByUserName` immediately after `createdBy`. From 63b7b7489b3feaba976d55d0951b02e1498efa0c Mon Sep 17 00:00:00 2001 From: Arcadio Quintero Date: Tue, 15 Sep 2026 13:37:55 -0400 Subject: [PATCH 03/10] feat(experiments): expose the creator's display name on the Experiments API (#37304) Adds createdByUserName beside the existing createdBy, so every response that carries an Experiment reports who created it by name instead of only by an opaque user id. The consumer is #37307 (the portlet listing's Created By column). Additive and inherited: the field is declared once on the model, so all 14 endpoints that return an Experiment carry it without being edited. ExperimentsResource and both response wrappers are untouched, createdBy keeps its key, value and role as the permission owner behind getOwner(), and lastModifiedBy is deliberately left alone. Why a plain default method rather than an Immutables attribute. Reading the generated Experiment class settles it: derived members are assigned in the constructor, so @Value.Derived would resolve a user on every Experiment built from the database - including the page-render and push-publish paths, which never serialize the object - while @Value.Lazy memoizes per instance and would let a name outlive a rename inside the running-experiments cache. An ordinary default method costs nothing until something serializes the Experiment. @JsonProperty(access = READ_ONLY) is load-bearing, not decoration. The generated Experiment.Json delegate binds settable attributes only, carries no @JsonIgnoreProperties(ignoreUnknown = true), and the REST mapper leaves FAIL_ON_UNKNOWN_PROPERTIES at Jackson's default, so a serialize-only field would otherwise make a round-tripped payload unreadable. The round-trip test proved READ_ONLY holds when Jackson binds into the delegate, which is what retired the @JsonAppend fallback the plan had pre-approved. The field is never null, absent or empty: a deleted user, an orphaned reference, a user whose name parts are all blank, and a failed lookup all resolve to the raw id, and nothing thrown by the lookup escapes into the response. Resolution goes through the UserCache-backed APILocator.getUserAPI().loadUserById, so a listing whose experiments share a creator costs one database read. TDD: the 10 unit tests were written and approved first, then confirmed failing on assertions (8 failures, 0 errors) before any implementation. Committed as one commit rather than a Red commit plus a Green commit to avoid leaving a knowingly failing revision in the PR's history. openapi.yaml regenerated from the annotations: 7 insertions, 0 deletions. --- .../experiments/model/AbstractExperiment.java | 26 +++ .../model/ExperimentCreatorNameResolver.java | 82 ++++++++ .../main/webapp/WEB-INF/openapi/openapi.yaml | 7 + .../ExperimentCreatorNameResolverTest.java | 136 +++++++++++++ .../model/ExperimentCreatorNameTest.java | 192 ++++++++++++++++++ 5 files changed, 443 insertions(+) create mode 100644 dotCMS/src/main/java/com/dotcms/experiments/model/ExperimentCreatorNameResolver.java create mode 100644 dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameResolverTest.java create mode 100644 dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java diff --git a/dotCMS/src/main/java/com/dotcms/experiments/model/AbstractExperiment.java b/dotCMS/src/main/java/com/dotcms/experiments/model/AbstractExperiment.java index 1b96a162a3a..4cdbea42469 100644 --- a/dotCMS/src/main/java/com/dotcms/experiments/model/AbstractExperiment.java +++ b/dotCMS/src/main/java/com/dotcms/experiments/model/AbstractExperiment.java @@ -16,6 +16,7 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.databind.annotation.JsonDeserialize; import com.fasterxml.jackson.databind.annotation.JsonSerialize; +import io.swagger.v3.oas.annotations.media.Schema; import io.vavr.control.Try; import java.io.Serializable; import java.time.Instant; @@ -90,6 +91,31 @@ default Instant modDate() { @JsonProperty("createdBy") String createdBy(); + /** + * The display name of the user behind {@link #createdBy()}, resolved when the Experiment is + * serialized and never stored, so a creator who renames themselves is reported under the new + * name. Falls back to the raw user ID when the creator cannot be resolved or has no name set — + * the value is never null, absent or empty. See {@link ExperimentCreatorNameResolver}. + * + *

Deliberately a plain {@code default} method rather than a {@code @Value.Derived} or + * {@code @Value.Lazy} attribute: those are computed at construction or memoized per instance, + * which would resolve a user on every Experiment built from the database — including on the + * page-render and push-publish paths, which never serialize the object — and would let a + * memoized name outlive a rename inside the running-experiments cache. As an ordinary default + * method it costs nothing until something serializes the Experiment. + * + *

{@code READ_ONLY} is load-bearing: the generated {@code Experiment.Json} delegate binds + * settable attributes only, so without it a payload carrying this field would fail to + * deserialize as an unknown property. + */ + @JsonProperty(value = "createdByUserName", access = JsonProperty.Access.READ_ONLY) + @Schema(description = "Display name of the user who created the experiment. Falls back to the " + + "raw createdBy user ID when the user cannot be resolved or has no name set.", + example = "Admin User") + default String createdByUserName() { + return ExperimentCreatorNameResolver.INSTANCE.resolve(createdBy()); + } + @JsonProperty("lastModifiedBy") String lastModifiedBy(); diff --git a/dotCMS/src/main/java/com/dotcms/experiments/model/ExperimentCreatorNameResolver.java b/dotCMS/src/main/java/com/dotcms/experiments/model/ExperimentCreatorNameResolver.java new file mode 100644 index 00000000000..f202ea96104 --- /dev/null +++ b/dotCMS/src/main/java/com/dotcms/experiments/model/ExperimentCreatorNameResolver.java @@ -0,0 +1,82 @@ +package com.dotcms.experiments.model; + +import com.dotmarketing.business.APILocator; +import com.dotmarketing.business.NoSuchUserException; +import com.dotmarketing.business.UserAPI; +import com.dotmarketing.util.Logger; +import com.dotmarketing.util.UtilMethods; +import com.liferay.portal.model.User; +import java.util.function.Supplier; + +/** + * Turns an {@link AbstractExperiment#createdBy()} user ID into the display name published as + * {@code createdByUserName} (#37304). + * + *

The field is never null, absent or empty. Every failure — a deleted user, an orphaned + * reference, a user with no name set, or a lookup that blows up — resolves to the raw user ID + * instead. That is a deliberate product choice: the Experiments portlet renders this in a + * Created By column, and an ID an administrator can search on beats a blank cell. Note that other + * parts of dotCMS answer the same question differently — push-publish history reports + * {@code "Deleted"}, and the Content Drive folder view reports {@code "System"} / {@code "unknown"}. + * + *

Nothing here may throw. The caller is a serializer running inside an already-successful + * API response, so an exception escaping this class would turn a working experiment read into a + * failed request over a field that is only decoration. + * + *

Cost. {@code loadUserById} is served by {@code UserCache}, so a listing whose + * experiments share a creator costs one database read for the first row and an in-memory hit for + * every row after it. This class deliberately adds no memo of its own: the value is resolved per + * serialization so that a user who renames themselves is reported under the new name. + */ +public class ExperimentCreatorNameResolver { + + /** + * The instance used by the model. The {@link UserAPI} is supplied lazily rather than captured, + * so class initialization never depends on {@code APILocator} being ready. + */ + public static final ExperimentCreatorNameResolver INSTANCE = + new ExperimentCreatorNameResolver(APILocator::getUserAPI); + + private final Supplier userAPI; + + /** + * Visible for testing: lets a test drive every fallback branch with a mocked {@link UserAPI} + * and no database. + */ + ExperimentCreatorNameResolver(final Supplier userAPI) { + this.userAPI = userAPI; + } + + /** + * Resolves a creator's display name, falling back to the ID itself. + * + * @param createdById the experiment's {@code createdBy} user ID + * @return the creator's full name, or {@code createdById} when it cannot be resolved to a named + * user. The argument is returned untouched when it is not set — unreachable through the model, + * where {@code createdBy()} is a mandatory attribute. + */ + public String resolve(final String createdById) { + if (!UtilMethods.isSet(createdById)) { + return createdById; + } + + try { + final User creator = this.userAPI.get().loadUserById(createdById); + final String fullName = null != creator ? creator.getFullName() : null; + + return UtilMethods.isSet(fullName) ? fullName : createdById; + } catch (final NoSuchUserException e) { + // A deleted or orphaned creator is a data condition, not an error: debug keeps a + // listing full of them from flooding the log while leaving the detail reachable. + Logger.debug(this, () -> String.format( + "Experiment creator '%s' no longer resolves to a User; reporting the ID", + createdById)); + return createdById; + } catch (final Exception e) { + Logger.warn(this, String.format( + "Failed to resolve the name of Experiment creator '%s': %s", + createdById, e.getMessage())); + return createdById; + } + } +} diff --git a/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml b/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml index 0abb4548e6e..5fcc8432a5c 100644 --- a/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml +++ b/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml @@ -28283,6 +28283,13 @@ components: properties: createdBy: type: string + createdByUserName: + type: string + description: Display name of the user who created the experiment. Falls + back to the raw createdBy user ID when the user cannot be resolved or + has no name set. + example: Admin User + readOnly: true creationDate: type: string format: date-time diff --git a/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameResolverTest.java b/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameResolverTest.java new file mode 100644 index 00000000000..0bd54959665 --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameResolverTest.java @@ -0,0 +1,136 @@ +package com.dotcms.experiments.model; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; +import static org.mockito.Mockito.when; + +import com.dotmarketing.business.NoSuchUserException; +import com.dotmarketing.business.UserAPI; +import com.dotmarketing.exception.DotDataException; +import com.liferay.portal.model.User; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Unit tests for {@link ExperimentCreatorNameResolver}, the rule that turns an Experiment's + * {@code createdBy} user ID into the display name returned as {@code createdByUserName} (#37304). + * + *

The whole value of this field is that the Experiments portlet's Created By column is + * never blank. That promise is not made by the happy path — it is made by the failure + * branches, so each one gets its own test: a user who no longer exists, a user who exists but has + * no name set, and an infrastructure failure. All three collapse to the same answer, the raw user + * ID, and none of them may throw: a decoration failure must never turn a successful experiment + * read into a failed request (FR-009, FR-010, FR-011). + * + *

The resolver takes its {@link UserAPI} through the constructor precisely so these branches can + * be exercised without a database; production code reaches it through {@code APILocator}. + */ +class ExperimentCreatorNameResolverTest { + + private static final String CREATOR_ID = "dotcms.org.1"; + + private UserAPI userAPI; + private ExperimentCreatorNameResolver resolver; + + @BeforeEach + void setUp() { + userAPI = mock(UserAPI.class); + resolver = new ExperimentCreatorNameResolver(() -> userAPI); + } + + private static User userNamed(final String first, final String middle, final String last) { + final User user = new User(); + user.setFirstName(first); + user.setMiddleName(middle); + user.setLastName(last); + return user; + } + + /** + * Method to test: {@link ExperimentCreatorNameResolver#resolve(String)} + * Given Scenario: The creator ID resolves to a User with a first and last name. + * ExpectedResult: The user's full name is returned. + */ + @Test + void resolve_userWithName_returnsFullName() throws Exception { + when(userAPI.loadUserById(CREATOR_ID)).thenReturn(userNamed("Admin", "", "User")); + + assertEquals("Admin User", resolver.resolve(CREATOR_ID)); + } + + /** + * Method to test: {@link ExperimentCreatorNameResolver#resolve(String)} + * Given Scenario: The creator resolves to a User whose first, middle and last name are all + * blank. {@code User.getFullName()} returns an empty string for that user. + * ExpectedResult: The raw creator ID, not the empty string — the field is never empty + * (FR-003, FR-010). + */ + @Test + void resolve_userWithBlankName_fallsBackToRawId() throws Exception { + when(userAPI.loadUserById(CREATOR_ID)).thenReturn(userNamed("", "", "")); + + assertEquals(CREATOR_ID, resolver.resolve(CREATOR_ID)); + } + + /** + * Method to test: {@link ExperimentCreatorNameResolver#resolve(String)} + * Given Scenario: The creator ID points at a user that no longer exists — a deleted user, or an + * orphaned reference left behind by one. + * ExpectedResult: The raw creator ID, and no exception escapes (FR-009). + */ + @Test + void resolve_deletedUser_fallsBackToRawId() throws Exception { + when(userAPI.loadUserById(CREATOR_ID)) + .thenThrow(new NoSuchUserException("No user matches " + CREATOR_ID)); + + assertEquals(CREATOR_ID, resolver.resolve(CREATOR_ID)); + } + + /** + * Method to test: {@link ExperimentCreatorNameResolver#resolve(String)} + * Given Scenario: The user lookup fails for an infrastructure reason rather than a data one. + * ExpectedResult: The raw creator ID. This is the branch that keeps a database hiccup from + * turning {@code GET /v1/experiments} into a 500 (FR-011). + */ + @Test + void resolve_lookupFailure_fallsBackToRawIdAndDoesNotThrow() throws Exception { + when(userAPI.loadUserById(CREATOR_ID)).thenThrow(new DotDataException("boom")); + + assertEquals(CREATOR_ID, resolver.resolve(CREATOR_ID)); + } + + /** + * Method to test: {@link ExperimentCreatorNameResolver#resolve(String)} + * Given Scenario: An unexpected unchecked failure escapes the user layer. + * ExpectedResult: Still the raw creator ID. The catch is deliberately broad because the caller + * is a serializer: anything thrown here would surface as a failed API response + * for a field that is only decoration. + */ + @Test + void resolve_unexpectedRuntimeFailure_fallsBackToRawId() throws Exception { + when(userAPI.loadUserById(CREATOR_ID)).thenThrow(new IllegalStateException("unexpected")); + + assertEquals(CREATOR_ID, resolver.resolve(CREATOR_ID)); + } + + /** + * Method to test: {@link ExperimentCreatorNameResolver#resolve(String)} + * Given Scenario: An unset creator ID. {@code AbstractExperiment.createdBy()} is a mandatory + * attribute, so the model cannot actually produce this — the test pins the + * defensive behaviour rather than a reachable path. + * ExpectedResult: The input is returned unchanged and the user layer is never consulted, so a + * blank ID cannot cost a lookup or raise. + */ + @Test + void resolve_unsetId_returnsInputAndSkipsLookup() throws Exception { + assertEquals("", resolver.resolve("")); + assertNotNull(resolver.resolve("")); + + verify(userAPI, org.mockito.Mockito.never()).loadUserById(anyString()); + verifyNoMoreInteractions(userAPI); + } +} diff --git a/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java b/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java new file mode 100644 index 00000000000..7483e9b4762 --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java @@ -0,0 +1,192 @@ +package com.dotcms.experiments.model; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.when; + +import com.dotcms.rest.api.v1.DotObjectMapperProvider; +import com.dotmarketing.business.APILocator; +import com.dotmarketing.business.UserAPI; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.liferay.portal.model.User; +import org.junit.jupiter.api.Test; +import org.mockito.MockedStatic; + +/** + * Serialization tests for the {@code createdByUserName} field added to the Experiment payload by + * #37304. + * + *

Why the round-trip test carries the most weight. The Experiment model is an Immutables + * value type, and its generated {@code Experiment.Json} delegate holds settable attributes + * only — it has no field for a computed one, and it is not annotated + * {@code @JsonIgnoreProperties(ignoreUnknown = true)}. The REST mapper leaves + * {@code FAIL_ON_UNKNOWN_PROPERTIES} at Jackson's default, which is enabled. So a property + * that serializes but cannot be bound is an unknown property on the way back in, and adding one + * introduces an asymmetry the current payload does not have: today every computed member is + * {@code @JsonIgnore}d, so an Experiment round-trips cleanly. + * + *

{@code @JsonProperty(access = READ_ONLY)} is the mechanism that is supposed to make the + * property known but not bound. {@link #payloadCarryingTheField_stillDeserializes()} is what + * decides whether that holds when Jackson binds into the generated delegate rather than into the + * interface. If it cannot be made green, the agreed fallback is a {@code @JsonAppend} virtual + * property — not disabling {@code FAIL_ON_UNKNOWN_PROPERTIES} on a shared mapper, which + * would weaken every other contract that mapper serves. + * + *

These tests deliberately use {@link DotObjectMapperProvider#createDefaultMapper()} rather than + * a bare {@code ObjectMapper}: the question is what the REST layer actually does, not what Jackson + * does in general. + */ +class ExperimentCreatorNameTest { + + private static final String CREATOR_ID = "dotcms.org.1"; + private static final String CREATOR_NAME = "Admin User"; + + /** + * {@code lookBackWindowExpireTime} is set explicitly on purpose. Leaving it unset makes + * Immutables evaluate its {@code @Value.Default}, which reads {@code ConfigExperimentUtil} and + * therefore {@code Config} — unavailable in a plain unit-test JVM, where it fails with + * {@code ExceptionInInitializerError} long before this feature gets a say. Setting the value + * keeps the test failing (and passing) for reasons that belong to #37304. + */ + private static Experiment anExperiment() { + return Experiment.builder() + .name("Homepage CTA test") + .pageId("2d8b8b1e-0000-0000-0000-000000000001") + .createdBy(CREATOR_ID) + .lastModifiedBy(CREATOR_ID) + .id("0e8b8b1e-0000-0000-0000-000000000002") + .lookBackWindowExpireTime(1_800_000L) + .build(); + } + + private static User adminUser() { + final User user = new User(); + user.setFirstName("Admin"); + user.setMiddleName(""); + user.setLastName("User"); + return user; + } + + /** + * Runs the body with {@code APILocator.getUserAPI()} answering with a stub that resolves + * {@link #CREATOR_ID} to {@link #adminUser()}. + */ + private static void withResolvableCreator(final ThrowingRunnable body) throws Exception { + final UserAPI userAPI = mock(UserAPI.class); + when(userAPI.loadUserById(anyString())).thenReturn(adminUser()); + + try (MockedStatic apiLocator = mockStatic(APILocator.class)) { + apiLocator.when(APILocator::getUserAPI).thenReturn(userAPI); + body.run(); + } + } + + @FunctionalInterface + private interface ThrowingRunnable { + void run() throws Exception; + } + + /** + * Method to test: {@code AbstractExperiment.createdByUserName()} + * Given Scenario: An Experiment whose creator resolves to a named user, serialized with the + * REST mapper. + * ExpectedResult: The payload carries {@code createdByUserName} with the creator's full name + * (FR-001, FR-002). + */ + @Test + void serializedExperiment_carriesCreatorName() throws Exception { + withResolvableCreator(() -> { + final ObjectMapper mapper = DotObjectMapperProvider.createDefaultMapper(); + + final JsonNode payload = mapper.readTree(mapper.writeValueAsString(anExperiment())); + + assertTrue(payload.has("createdByUserName"), + "The payload should carry createdByUserName"); + assertEquals(CREATOR_NAME, payload.get("createdByUserName").asText()); + }); + } + + /** + * Method to test: {@code AbstractExperiment.createdByUserName()} + * Given Scenario: The same serialized payload fed straight back to the same mapper. + * ExpectedResult: It deserializes into an Experiment without raising. This is the gating test + * described in the class Javadoc: a serialize-only property must not make a + * round-tripped payload unreadable (FR-023). + */ + @Test + void payloadCarryingTheField_stillDeserializes() throws Exception { + withResolvableCreator(() -> { + final ObjectMapper mapper = DotObjectMapperProvider.createDefaultMapper(); + final String json = mapper.writeValueAsString(anExperiment()); + + assertTrue(json.contains("createdByUserName"), + "Precondition: the payload under test must actually carry the new field"); + + final Experiment parsed = assertDoesNotThrow( + () -> mapper.readValue(json, Experiment.class), + "A payload carrying createdByUserName must still deserialize"); + + assertEquals(CREATOR_ID, parsed.createdBy(), + "The bound createdBy should survive the round trip unchanged"); + }); + } + + /** + * Method to test: {@code AbstractExperiment.createdByUserName()} + * Given Scenario: An Experiment whose creator cannot be resolved, serialized with the REST + * mapper. + * ExpectedResult: The field is present and carries the raw creator ID, so the column is never + * blank even at the serialization layer (FR-003, FR-009). + */ + @Test + void serializedExperiment_withUnresolvableCreator_carriesRawId() throws Exception { + final UserAPI userAPI = mock(UserAPI.class); + when(userAPI.loadUserById(anyString())) + .thenThrow(new com.dotmarketing.business.NoSuchUserException("gone")); + + try (MockedStatic apiLocator = mockStatic(APILocator.class)) { + apiLocator.when(APILocator::getUserAPI).thenReturn(userAPI); + + final ObjectMapper mapper = DotObjectMapperProvider.createDefaultMapper(); + final JsonNode payload = mapper.readTree(mapper.writeValueAsString(anExperiment())); + + assertEquals(CREATOR_ID, payload.get("createdByUserName").asText()); + } + } + + /** + * Method to test: {@code AbstractExperiment.createdBy()} / {@code getOwner()} + * Given Scenario: The same serialization, inspected for what must NOT have changed. + * ExpectedResult: {@code createdBy} keeps its key and its user-ID value, {@code getOwner()} + * still resolves from it, and the computed permission members stay out of the + * payload. This is the additive-change guarantee (FR-005, FR-006, FR-007). + */ + @Test + void addingTheField_leavesTheExistingContractAlone() throws Exception { + withResolvableCreator(() -> { + final Experiment experiment = anExperiment(); + final ObjectMapper mapper = DotObjectMapperProvider.createDefaultMapper(); + + final JsonNode payload = mapper.readTree(mapper.writeValueAsString(experiment)); + + assertEquals(CREATOR_ID, payload.get("createdBy").asText(), + "createdBy must still be the user ID"); + assertEquals(CREATOR_ID, experiment.getOwner(), + "getOwner() must still resolve from createdBy, not from the new field"); + assertEquals(CREATOR_ID, payload.get("lastModifiedBy").asText(), + "lastModifiedBy is out of scope and must be untouched"); + + for (final String computed : new String[]{"owner", "identifier", "permissionId", + "manifestInfo", "parentPermissionable", "acceptedPermissions"}) { + assertFalse(payload.has(computed), + "The computed member '" + computed + "' must stay out of the payload"); + } + }); + } +} From e9c29c273957d4ccebbc4e8bbfdff617e386eb87 Mon Sep 17 00:00:00 2001 From: Arcadio Quintero Date: Tue, 15 Sep 2026 17:20:55 -0400 Subject: [PATCH 04/10] test(experiments): guard the two invariants the creator-name design rests on (#37304) /speckit-analyze found that FR-015 (no user lookups on paths that never serialize) and FR-016 (the name is never captured on the instance) had zero coverage. Both hold by construction today, which is exactly the problem: they are properties of HOW the accessor is declared, so a plausible refactor would retire them silently. These are regression guards, not TDD - the behaviour is already correct and both tests passed on first run. Their value is what happens when the declaration changes, so that was verified rather than asserted: temporarily annotating createdByUserName() with @Value.Derived fails exactly these two tests and leaves the other ten green. Ten tests that all serialize could not see the regression; these two can. - buildingAnExperimentWithoutSerializing_neverResolvesTheCreator: builds, rebuilds via from(), reads the owner, compares and stringifies an Experiment with no interaction on the user layer, then asserts serialization is what triggers the single lookup. - renamingTheCreator_isReflectedOnTheNextSerialization: serializes the same instance twice across a rename and expects the new name, which a memoized @Value.Lazy value could not produce. Also folds adminUser() into the new userNamed() helper and updates the class Javadoc, which still described the READ_ONLY round-trip as an open question that 353736ef7e settled. --- .../model/ExperimentCreatorNameTest.java | 98 ++++++++++++++++--- 1 file changed, 87 insertions(+), 11 deletions(-) diff --git a/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java b/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java index 7483e9b4762..d5f2b37f101 100644 --- a/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java +++ b/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java @@ -7,6 +7,8 @@ import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; import com.dotcms.rest.api.v1.DotObjectMapperProvider; @@ -31,12 +33,12 @@ * introduces an asymmetry the current payload does not have: today every computed member is * {@code @JsonIgnore}d, so an Experiment round-trips cleanly. * - *

{@code @JsonProperty(access = READ_ONLY)} is the mechanism that is supposed to make the - * property known but not bound. {@link #payloadCarryingTheField_stillDeserializes()} is what - * decides whether that holds when Jackson binds into the generated delegate rather than into the - * interface. If it cannot be made green, the agreed fallback is a {@code @JsonAppend} virtual - * property — not disabling {@code FAIL_ON_UNKNOWN_PROPERTIES} on a shared mapper, which - * would weaken every other contract that mapper serves. + *

{@code @JsonProperty(access = READ_ONLY)} is what makes the property known but not + * bound, and {@link #payloadCarryingTheField_stillDeserializes()} is what established that it + * holds when Jackson binds into the generated delegate rather than into the interface. Should that + * ever stop being true, the answer is a {@code @JsonAppend} virtual property — not disabling + * {@code FAIL_ON_UNKNOWN_PROPERTIES} on a shared mapper, which would weaken every other contract + * that mapper serves. * *

These tests deliberately use {@link DotObjectMapperProvider#createDefaultMapper()} rather than * a bare {@code ObjectMapper}: the question is what the REST layer actually does, not what Jackson @@ -66,11 +68,7 @@ private static Experiment anExperiment() { } private static User adminUser() { - final User user = new User(); - user.setFirstName("Admin"); - user.setMiddleName(""); - user.setLastName("User"); - return user; + return userNamed("Admin", "User"); } /** @@ -189,4 +187,82 @@ void addingTheField_leavesTheExistingContractAlone() throws Exception { } }); } + + /** + * Method to test: {@code AbstractExperiment.createdByUserName()} + * Given Scenario: Experiments are built and handled without ever being serialized — constructed + * from the builder, rebuilt via {@code from(...)} the way + * {@code addTargetingConditions} does on the find path, read for their owner, + * and compared. + * ExpectedResult: The user layer is never consulted. Only serializing the Experiment resolves + * the name (FR-015). + * + *

This is the guard for the decision the whole feature rests on. The accessor is a plain + * {@code default} method precisely so it stays inert on the paths that never serialize — the + * database transformer behind every list row, the running-experiments cache fill on page + * render, and the push-publish dependency walk. Turning it into a {@code @Value.Derived} + * attribute would move the lookup into the constructor and break that silently: every other + * test in this class would still pass, because they all serialize. This one would not. + */ + @Test + void buildingAnExperimentWithoutSerializing_neverResolvesTheCreator() throws Exception { + final UserAPI userAPI = mock(UserAPI.class); + + try (MockedStatic apiLocator = mockStatic(APILocator.class)) { + apiLocator.when(APILocator::getUserAPI).thenReturn(userAPI); + + final Experiment experiment = anExperiment(); + final Experiment rebuilt = Experiment.builder().from(experiment).build(); + experiment.getOwner(); + experiment.equals(rebuilt); + experiment.hashCode(); + experiment.toString(); + + verifyNoInteractions(userAPI); + + DotObjectMapperProvider.createDefaultMapper().writeValueAsString(experiment); + + verify(userAPI).loadUserById(CREATOR_ID); + } + } + + /** + * Method to test: {@code AbstractExperiment.createdByUserName()} + * Given Scenario: The same Experiment instance serialized twice, with the creator renaming + * themselves in between. + * ExpectedResult: The second payload reports the new name (FR-016). + * + *

The guard against memoization. A {@code @Value.Lazy} attribute would compute the name once + * and keep it for the life of the instance — invisible on the REST path, where instances are + * short-lived, but not on the running-experiments list cache, whose entries are long-lived and + * shared. This test fails the moment the value starts being cached on the object. + */ + @Test + void renamingTheCreator_isReflectedOnTheNextSerialization() throws Exception { + final UserAPI userAPI = mock(UserAPI.class); + when(userAPI.loadUserById(CREATOR_ID)) + .thenReturn(adminUser(), userNamed("Renamed", "User")); + + try (MockedStatic apiLocator = mockStatic(APILocator.class)) { + apiLocator.when(APILocator::getUserAPI).thenReturn(userAPI); + + final Experiment experiment = anExperiment(); + final ObjectMapper mapper = DotObjectMapperProvider.createDefaultMapper(); + + final JsonNode before = mapper.readTree(mapper.writeValueAsString(experiment)); + final JsonNode after = mapper.readTree(mapper.writeValueAsString(experiment)); + + assertEquals(CREATOR_NAME, before.get("createdByUserName").asText()); + assertEquals("Renamed User", after.get("createdByUserName").asText(), + "The name must be resolved per serialization, never captured on the instance"); + } + } + + private static User userNamed(final String first, final String last) { + final User user = new User(); + user.setFirstName(first); + user.setMiddleName(""); + user.setLastName(last); + return user; + } } From 4785facef75faed80763ba42ec25c00c0ea037e7 Mon Sep 17 00:00:00 2001 From: Arcadio Quintero Date: Wed, 16 Sep 2026 14:21:22 -0400 Subject: [PATCH 05/10] feat(experiments): align the creator-name fallbacks with Content Drive (#37304) The issue specified the raw user id as the fallback when a creator cannot be resolved. That leaves two listings in the same product labelling the same orphaned owner differently: Content Drive's folder view already answers this exact question through BrowserAPIImpl.ownerName, reporting "System" for the system user and "unknown" for anyone unresolvable. Arcadio's call is to match it, so the rule is now: - system user -> "System", short-circuited before any lookup - deleted, orphaned, blank-named, or a failed lookup -> "unknown" - unset id -> "unknown" (unreachable through the model; createdBy is mandatory) Still never null, never absent, never empty. Note that Content Drive reaches the user through UserLocalManagerUtil and so bypasses UserCache; this resolver deliberately does not, since FR-014 rests on that cache. TDD order kept: the six resolver tests and the serialization test were moved to the new expectations first, then the resolver followed. A seventh test pins the system-user short-circuit, asserting the user layer is never consulted for it. 13/13 green, run with -Dmaven.build.cache.enabled=false - a cached run reports BUILD SUCCESS while skipping surefire entirely. Also in this commit, from /speckit-analyze: - spec A2 said experiment instances are cached in ExperimentsCache. Verified and corrected: the per-experiment cache in ExperimentsFactoryImpl.find has both its read and its write commented out with a TODO, and its commented call does not even match the current interface, so find and list build fresh instances every time. Only the running-experiments list cache is live, which is the one that actually carries the staleness hazard. - openapi.yaml regenerated for the new @Schema description. This diverges from the issue body, which still states the raw-id rule. The issue is not edited from this worktree; reported to the coordinator instead. --- .../experiments/model/AbstractExperiment.java | 11 ++-- .../model/ExperimentCreatorNameResolver.java | 49 +++++++++----- .../main/webapp/WEB-INF/openapi/openapi.yaml | 6 +- .../ExperimentCreatorNameResolverTest.java | 65 ++++++++++++------- .../model/ExperimentCreatorNameTest.java | 9 +-- .../experiment-created-by-username.md | 16 +++-- .../data-model.md | 32 ++++++--- .../spec.md | 43 +++++++----- 8 files changed, 150 insertions(+), 81 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/experiments/model/AbstractExperiment.java b/dotCMS/src/main/java/com/dotcms/experiments/model/AbstractExperiment.java index 4cdbea42469..ce1b59405b1 100644 --- a/dotCMS/src/main/java/com/dotcms/experiments/model/AbstractExperiment.java +++ b/dotCMS/src/main/java/com/dotcms/experiments/model/AbstractExperiment.java @@ -94,8 +94,10 @@ default Instant modDate() { /** * The display name of the user behind {@link #createdBy()}, resolved when the Experiment is * serialized and never stored, so a creator who renames themselves is reported under the new - * name. Falls back to the raw user ID when the creator cannot be resolved or has no name set — - * the value is never null, absent or empty. See {@link ExperimentCreatorNameResolver}. + * name. Reports {@code "System"} for the system user and {@code "unknown"} when the creator + * cannot be resolved or has no name set, matching the labels the Content Drive folder view + * already uses — the value is never null, absent or empty. See + * {@link ExperimentCreatorNameResolver}. * *

Deliberately a plain {@code default} method rather than a {@code @Value.Derived} or * {@code @Value.Lazy} attribute: those are computed at construction or memoized per instance, @@ -109,8 +111,9 @@ default Instant modDate() { * deserialize as an unknown property. */ @JsonProperty(value = "createdByUserName", access = JsonProperty.Access.READ_ONLY) - @Schema(description = "Display name of the user who created the experiment. Falls back to the " - + "raw createdBy user ID when the user cannot be resolved or has no name set.", + @Schema(description = "Display name of the user who created the experiment. Reports \"System\" " + + "for the system user and \"unknown\" when the user cannot be resolved or has no name " + + "set, so the value is never empty.", example = "Admin User") default String createdByUserName() { return ExperimentCreatorNameResolver.INSTANCE.resolve(createdBy()); diff --git a/dotCMS/src/main/java/com/dotcms/experiments/model/ExperimentCreatorNameResolver.java b/dotCMS/src/main/java/com/dotcms/experiments/model/ExperimentCreatorNameResolver.java index f202ea96104..d80631d486e 100644 --- a/dotCMS/src/main/java/com/dotcms/experiments/model/ExperimentCreatorNameResolver.java +++ b/dotCMS/src/main/java/com/dotcms/experiments/model/ExperimentCreatorNameResolver.java @@ -12,12 +12,17 @@ * Turns an {@link AbstractExperiment#createdBy()} user ID into the display name published as * {@code createdByUserName} (#37304). * - *

The field is never null, absent or empty. Every failure — a deleted user, an orphaned - * reference, a user with no name set, or a lookup that blows up — resolves to the raw user ID - * instead. That is a deliberate product choice: the Experiments portlet renders this in a - * Created By column, and an ID an administrator can search on beats a blank cell. Note that other - * parts of dotCMS answer the same question differently — push-publish history reports - * {@code "Deleted"}, and the Content Drive folder view reports {@code "System"} / {@code "unknown"}. + *

The field is never null, absent or empty. The system user is reported as + * {@code "System"}, and everything that cannot be resolved to a named user — a deleted user, an + * orphaned reference, a user with no name set, or a lookup that blows up — is reported as + * {@code "unknown"}. + * + *

Those two labels are not invented here: they are the ones + * {@code BrowserAPIImpl.ownerName} already publishes for the Content Drive folder view, which + * answers exactly this question for a different listing. Two listings in the same product labelling + * the same orphaned owner differently is a worse outcome than either label on its own. (Note that + * the Content Drive implementation reaches the user through {@code UserLocalManagerUtil} and so + * bypasses {@code UserCache}; this one deliberately does not.) * *

Nothing here may throw. The caller is a serializer running inside an already-successful * API response, so an exception escaping this class would turn a working experiment read into a @@ -30,6 +35,12 @@ */ public class ExperimentCreatorNameResolver { + /** Shown for an owner that cannot be resolved to a named user. Matches Content Drive. */ + static final String UNKNOWN = "unknown"; + + /** Shown for the system user. Matches Content Drive. */ + static final String SYSTEM = "System"; + /** * The instance used by the model. The {@link UserAPI} is supplied lazily rather than captured, * so class initialization never depends on {@code APILocator} being ready. @@ -42,41 +53,47 @@ public class ExperimentCreatorNameResolver { /** * Visible for testing: lets a test drive every fallback branch with a mocked {@link UserAPI} * and no database. + * + * @param userAPI supplies the user layer when a lookup is actually needed */ ExperimentCreatorNameResolver(final Supplier userAPI) { this.userAPI = userAPI; } /** - * Resolves a creator's display name, falling back to the ID itself. + * Resolves a creator's display name. * * @param createdById the experiment's {@code createdBy} user ID - * @return the creator's full name, or {@code createdById} when it cannot be resolved to a named - * user. The argument is returned untouched when it is not set — unreachable through the model, - * where {@code createdBy()} is a mandatory attribute. + * @return the creator's full name; {@code "System"} for the system user; {@code "unknown"} when + * the ID is unset, resolves to nobody, or resolves to a user with no name set. Never null, + * never empty. */ public String resolve(final String createdById) { if (!UtilMethods.isSet(createdById)) { - return createdById; + return UNKNOWN; + } + + if (UserAPI.SYSTEM_USER_ID.equalsIgnoreCase(createdById)) { + return SYSTEM; } try { final User creator = this.userAPI.get().loadUserById(createdById); final String fullName = null != creator ? creator.getFullName() : null; - return UtilMethods.isSet(fullName) ? fullName : createdById; + return UtilMethods.isSet(fullName) ? fullName : UNKNOWN; } catch (final NoSuchUserException e) { // A deleted or orphaned creator is a data condition, not an error: debug keeps a // listing full of them from flooding the log while leaving the detail reachable. Logger.debug(this, () -> String.format( - "Experiment creator '%s' no longer resolves to a User; reporting the ID", - createdById)); - return createdById; + "Experiment creator '%s' no longer resolves to a User; reporting '%s'", + createdById, UNKNOWN)); + return UNKNOWN; } catch (final Exception e) { Logger.warn(this, String.format( "Failed to resolve the name of Experiment creator '%s': %s", createdById, e.getMessage())); - return createdById; + return UNKNOWN; } } } diff --git a/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml b/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml index 5fcc8432a5c..5af2553d01a 100644 --- a/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml +++ b/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml @@ -28285,9 +28285,9 @@ components: type: string createdByUserName: type: string - description: Display name of the user who created the experiment. Falls - back to the raw createdBy user ID when the user cannot be resolved or - has no name set. + description: "Display name of the user who created the experiment. Reports\ + \ \"System\" for the system user and \"unknown\" when the user cannot\ + \ be resolved or has no name set, so the value is never empty." example: Admin User readOnly: true creationDate: diff --git a/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameResolverTest.java b/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameResolverTest.java index 0bd54959665..eaaed2a2841 100644 --- a/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameResolverTest.java +++ b/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameResolverTest.java @@ -1,9 +1,9 @@ package com.dotcms.experiments.model; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; @@ -22,9 +22,14 @@ *

The whole value of this field is that the Experiments portlet's Created By column is * never blank. That promise is not made by the happy path — it is made by the failure * branches, so each one gets its own test: a user who no longer exists, a user who exists but has - * no name set, and an infrastructure failure. All three collapse to the same answer, the raw user - * ID, and none of them may throw: a decoration failure must never turn a successful experiment - * read into a failed request (FR-009, FR-010, FR-011). + * no name set, and an infrastructure failure. All three collapse to the same answer, + * {@code "unknown"}, and none of them may throw: a decoration failure must never turn a successful + * experiment read into a failed request (FR-009, FR-010, FR-011). + * + *

The labels deliberately match {@code BrowserAPIImpl.ownerName}, which answers the same question + * for the Content Drive folder view: {@code "System"} for the system user, {@code "unknown"} for + * anyone who cannot be resolved. Two listings in the same product should not label the same + * orphaned owner differently. * *

The resolver takes its {@link UserAPI} through the constructor precisely so these branches can * be exercised without a database; production code reaches it through {@code APILocator}. @@ -32,6 +37,7 @@ class ExperimentCreatorNameResolverTest { private static final String CREATOR_ID = "dotcms.org.1"; + private static final String UNKNOWN = "unknown"; private UserAPI userAPI; private ExperimentCreatorNameResolver resolver; @@ -66,55 +72,70 @@ void resolve_userWithName_returnsFullName() throws Exception { * Method to test: {@link ExperimentCreatorNameResolver#resolve(String)} * Given Scenario: The creator resolves to a User whose first, middle and last name are all * blank. {@code User.getFullName()} returns an empty string for that user. - * ExpectedResult: The raw creator ID, not the empty string — the field is never empty + * ExpectedResult: {@code "unknown"}, not the empty string — the field is never empty * (FR-003, FR-010). */ @Test - void resolve_userWithBlankName_fallsBackToRawId() throws Exception { + void resolve_userWithBlankName_fallsBackToUnknown() throws Exception { when(userAPI.loadUserById(CREATOR_ID)).thenReturn(userNamed("", "", "")); - assertEquals(CREATOR_ID, resolver.resolve(CREATOR_ID)); + assertEquals(UNKNOWN, resolver.resolve(CREATOR_ID)); } /** * Method to test: {@link ExperimentCreatorNameResolver#resolve(String)} * Given Scenario: The creator ID points at a user that no longer exists — a deleted user, or an * orphaned reference left behind by one. - * ExpectedResult: The raw creator ID, and no exception escapes (FR-009). + * ExpectedResult: {@code "unknown"}, and no exception escapes (FR-009). */ @Test - void resolve_deletedUser_fallsBackToRawId() throws Exception { + void resolve_deletedUser_fallsBackToUnknown() throws Exception { when(userAPI.loadUserById(CREATOR_ID)) .thenThrow(new NoSuchUserException("No user matches " + CREATOR_ID)); - assertEquals(CREATOR_ID, resolver.resolve(CREATOR_ID)); + assertEquals(UNKNOWN, resolver.resolve(CREATOR_ID)); } /** * Method to test: {@link ExperimentCreatorNameResolver#resolve(String)} * Given Scenario: The user lookup fails for an infrastructure reason rather than a data one. - * ExpectedResult: The raw creator ID. This is the branch that keeps a database hiccup from + * ExpectedResult: {@code "unknown"}. This is the branch that keeps a database hiccup from * turning {@code GET /v1/experiments} into a 500 (FR-011). */ @Test - void resolve_lookupFailure_fallsBackToRawIdAndDoesNotThrow() throws Exception { + void resolve_lookupFailure_fallsBackToUnknownAndDoesNotThrow() throws Exception { when(userAPI.loadUserById(CREATOR_ID)).thenThrow(new DotDataException("boom")); - assertEquals(CREATOR_ID, resolver.resolve(CREATOR_ID)); + assertEquals(UNKNOWN, resolver.resolve(CREATOR_ID)); } /** * Method to test: {@link ExperimentCreatorNameResolver#resolve(String)} * Given Scenario: An unexpected unchecked failure escapes the user layer. - * ExpectedResult: Still the raw creator ID. The catch is deliberately broad because the caller + * ExpectedResult: Still {@code "unknown"}. The catch is deliberately broad because the caller * is a serializer: anything thrown here would surface as a failed API response * for a field that is only decoration. */ @Test - void resolve_unexpectedRuntimeFailure_fallsBackToRawId() throws Exception { + void resolve_unexpectedRuntimeFailure_fallsBackToUnknown() throws Exception { when(userAPI.loadUserById(CREATOR_ID)).thenThrow(new IllegalStateException("unexpected")); - assertEquals(CREATOR_ID, resolver.resolve(CREATOR_ID)); + assertEquals(UNKNOWN, resolver.resolve(CREATOR_ID)); + } + + /** + * Method to test: {@link ExperimentCreatorNameResolver#resolve(String)} + * Given Scenario: The experiment was created by the system user, whose ID is {@code "system"}. + * ExpectedResult: {@code "System"}, without consulting the user layer at all — the same + * short-circuit {@code BrowserAPIImpl.ownerName} applies, so the two listings + * agree on the label. + */ + @Test + void resolve_systemUser_returnsSystemWithoutLookup() throws Exception { + assertEquals("System", resolver.resolve("system")); + assertEquals("System", resolver.resolve("SYSTEM")); + + verify(userAPI, never()).loadUserById(anyString()); } /** @@ -122,15 +143,15 @@ void resolve_unexpectedRuntimeFailure_fallsBackToRawId() throws Exception { * Given Scenario: An unset creator ID. {@code AbstractExperiment.createdBy()} is a mandatory * attribute, so the model cannot actually produce this — the test pins the * defensive behaviour rather than a reachable path. - * ExpectedResult: The input is returned unchanged and the user layer is never consulted, so a - * blank ID cannot cost a lookup or raise. + * ExpectedResult: {@code "unknown"}, and the user layer is never consulted, so a blank ID + * cannot cost a lookup or raise. */ @Test - void resolve_unsetId_returnsInputAndSkipsLookup() throws Exception { - assertEquals("", resolver.resolve("")); - assertNotNull(resolver.resolve("")); + void resolve_unsetId_returnsUnknownAndSkipsLookup() throws Exception { + assertEquals(UNKNOWN, resolver.resolve("")); + assertEquals(UNKNOWN, resolver.resolve(null)); - verify(userAPI, org.mockito.Mockito.never()).loadUserById(anyString()); + verify(userAPI, never()).loadUserById(anyString()); verifyNoMoreInteractions(userAPI); } } diff --git a/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java b/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java index d5f2b37f101..c79a90044f6 100644 --- a/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java +++ b/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java @@ -139,11 +139,12 @@ void payloadCarryingTheField_stillDeserializes() throws Exception { * Method to test: {@code AbstractExperiment.createdByUserName()} * Given Scenario: An Experiment whose creator cannot be resolved, serialized with the REST * mapper. - * ExpectedResult: The field is present and carries the raw creator ID, so the column is never - * blank even at the serialization layer (FR-003, FR-009). + * ExpectedResult: The field is present and carries {@code "unknown"} — the same label the + * Content Drive folder view uses — so the column is never blank even at the + * serialization layer (FR-003, FR-009). */ @Test - void serializedExperiment_withUnresolvableCreator_carriesRawId() throws Exception { + void serializedExperiment_withUnresolvableCreator_carriesUnknown() throws Exception { final UserAPI userAPI = mock(UserAPI.class); when(userAPI.loadUserById(anyString())) .thenThrow(new com.dotmarketing.business.NoSuchUserException("gone")); @@ -154,7 +155,7 @@ void serializedExperiment_withUnresolvableCreator_carriesRawId() throws Exceptio final ObjectMapper mapper = DotObjectMapperProvider.createDefaultMapper(); final JsonNode payload = mapper.readTree(mapper.writeValueAsString(anExperiment())); - assertEquals(CREATOR_ID, payload.get("createdByUserName").asText()); + assertEquals("unknown", payload.get("createdByUserName").asText()); } } diff --git a/specs/37304-experiment-created-by-username/contracts/experiment-created-by-username.md b/specs/37304-experiment-created-by-username/contracts/experiment-created-by-username.md index b68d929e72e..6afc80d37e9 100644 --- a/specs/37304-experiment-created-by-username/contracts/experiment-created-by-username.md +++ b/specs/37304-experiment-created-by-username/contracts/experiment-created-by-username.md @@ -22,8 +22,9 @@ Under `components.schemas.Experiment.properties`, alphabetically between `create createdByUserName: type: string description: >- - Display name of the user who created the experiment. Falls back to the raw - createdBy user id when the user cannot be resolved or has no name set. + Display name of the user who created the experiment. Reports "System" for the + system user and "unknown" when the user cannot be resolved or has no name set, + so the value is never empty. example: Admin User creationDate: type: string @@ -61,12 +62,15 @@ Explicitly **not** carrying it, because they return no Experiment: | Condition | `createdBy` | `createdByUserName` | HTTP | |---|---|---|---| | Creator resolves, has a name | user id | full name (first + middle + last) | 200 | -| Creator resolves, all name parts blank | user id | the user id | 200 | -| Creator does not resolve (deleted/orphaned) | user id | the user id | 200 | -| User lookup fails (infrastructure) | user id | the user id | 200 | -| Creator is the system user | user id | resolved name, else the user id | 200 | +| Creator resolves, all name parts blank | user id | `unknown` | 200 | +| Creator does not resolve (deleted/orphaned) | user id | `unknown` | 200 | +| User lookup fails (infrastructure) | user id | `unknown` | 200 | +| Creator is the system user | `system` | `System`, short-circuited before any lookup | 200 | | One bad creator among many in a list | per entry | only the affected entry falls back | 200 | +The two fallback labels are the ones `BrowserAPIImpl.ownerName` already publishes for the Content +Drive folder view, so the same orphaned owner reads the same in both listings. + The field is never `null`, never absent and never `""`. ## Deserialization contract (FR-023) diff --git a/specs/37304-experiment-created-by-username/data-model.md b/specs/37304-experiment-created-by-username/data-model.md index 65d48e57827..ac341f2fec9 100644 --- a/specs/37304-experiment-created-by-username/data-model.md +++ b/specs/37304-experiment-created-by-username/data-model.md @@ -26,7 +26,7 @@ describes the **in-memory and wire** shape only. | **Immutables kind** | **None** — deliberately not `@Value.Default/Derived/Lazy`, so it is not an attribute: no field, no builder method, no `equals`/`hashCode`/`toString` participation, no `build()` cost | | **Jackson** | `@JsonProperty(value = "createdByUserName", access = JsonProperty.Access.READ_ONLY)` | | **Swagger** | `@Schema(description = "...", example = "Admin User")` on the same method | -| **Type** | `String`, **always non-null and non-empty** (FR-003) | +| **Type** | `String`, **always non-null and non-empty** (FR-003) — a real name, `System`, or `unknown` | | **Persisted** | No — resolved per serialization (FR-016, FR-017) | | **Settable** | No — read-only by design; `READ_ONLY` is what keeps an inbound payload parseable (FR-023) | | **Derivation** | `ExperimentCreatorNameResolver.resolve(createdBy())` | @@ -53,15 +53,20 @@ Input: the raw `createdBy` user id. Output: a non-empty display string. ``` resolve(createdById): - 1. createdById not set -> return createdById as-is (never null-propagate; FR-003) - 2. user = userAPI.loadUserById(createdById) # UserCache-backed (research R3) - 3. fullName = user.getFullName() # first + middle + last - 4. fullName is set -> return fullName - 5. otherwise -> return createdById # blank-name user; FR-010 - on NoSuchUserException -> return createdById # deleted / orphaned; FR-009 - on DotDataException / any other -> return createdById # never fail the request; FR-011 + 1. createdById not set -> return "unknown" (never null/empty; FR-003) + 2. createdById is "system" -> return "System" (no lookup at all; FR-010a) + 3. user = userAPI.loadUserById(createdById) # UserCache-backed (research R3) + 4. fullName = user.getFullName() # first + middle + last + 5. fullName is set -> return fullName + 6. otherwise -> return "unknown" # blank-name user; FR-010 + on NoSuchUserException -> return "unknown" # deleted / orphaned; FR-009 + on DotDataException / any other -> return "unknown" # never fail the request; FR-011 ``` +The two labels are `BrowserAPIImpl.ownerName`'s, which answers the same question for the Content +Drive folder view (FR-010b). That implementation reaches the user through `UserLocalManagerUtil` and +so bypasses `UserCache`; this one deliberately goes through `APILocator.getUserAPI()` instead. + **Per-entry isolation** (FR-012): the rule is applied independently per experiment, so one bad id in a list degrades exactly one entry. @@ -104,7 +109,16 @@ Unresolvable creator (deleted user, or a user whose name parts are all blank): ```json { "createdBy": "deleted-user-id-4711", - "createdByUserName": "deleted-user-id-4711" + "createdByUserName": "unknown" +} +``` + +Created by the system user: + +```json +{ + "createdBy": "system", + "createdByUserName": "System" } ``` diff --git a/specs/37304-experiment-created-by-username/spec.md b/specs/37304-experiment-created-by-username/spec.md index f09f3a97dbb..ca163637174 100644 --- a/specs/37304-experiment-created-by-username/spec.md +++ b/specs/37304-experiment-created-by-username/spec.md @@ -107,28 +107,28 @@ column, and no row is missing or empty. The listing loads normally. failed request that hides every other experiment in the list. **Independent Test**: Point an experiment's `createdBy` at a user ID that no longer resolves, request -both the list and the single fetch, and confirm the response succeeds and reports the raw ID. +both the list and the single fetch, and confirm the response succeeds and reports `unknown`. **Acceptance Scenarios**: 1. **Given** an experiment whose creator ID does not resolve to any user, **When** the experiment is - requested, **Then** the request succeeds and the creator name field carries the raw creator ID. + requested, **Then** the request succeeds and the creator name field reports `unknown`. 2. **Given** an experiment created by the system user, **When** the experiment is requested, **Then** - the request succeeds and the creator name field carries a non-empty value. + the request succeeds and the creator name field reports `System`. 3. **Given** a list where one experiment has an unresolvable creator and the others do not, **When** the list is requested, **Then** the request succeeds, the resolvable entries report real names and - only the affected entry falls back to the raw ID. + only the affected entry reports `unknown`. --- ### Edge Cases -- **Creator no longer exists** (deleted user, orphaned reference): the field falls back to the raw - creator ID. It is never null, never absent and never an empty string. +- **Creator no longer exists** (deleted user, orphaned reference): the field reports `unknown`. It is + never null, never absent and never an empty string. - **Creator resolves but has no usable name** (first, middle and last name all blank): treated the - same as unresolvable — fall back to the raw creator ID, so the column still identifies something. -- **Creator is the system user**: resolves normally; if the system user has no usable name, the - fallback rule applies. The request never fails because of it. + same as unresolvable — report `unknown`, so the column still says something honest. +- **Creator is the system user**: reported as `System`, short-circuited before any lookup. The + request never fails because of it. - **User lookup fails for an infrastructure reason** (database error, cache error): the experiment is still returned successfully with the fallback value. A failure to decorate must never turn a successful experiment read into a failed request. @@ -171,10 +171,16 @@ both the list and the single fetch, and confirm the response succeeds and report **Fallback and failure** -- **FR-009**: When the creator ID does not resolve to a user, `createdByUserName` MUST fall back to - the raw creator ID. -- **FR-010**: When the creator resolves to a user whose name is blank, `createdByUserName` MUST fall - back to the raw creator ID under the same rule as FR-009. +- **FR-009**: When the creator ID does not resolve to a user, `createdByUserName` MUST report + `unknown`. +- **FR-010**: When the creator resolves to a user whose name is blank, `createdByUserName` MUST + report `unknown` under the same rule as FR-009. +- **FR-010a**: When the creator is the system user, `createdByUserName` MUST report `System`, + without a user lookup. +- **FR-010b**: These labels MUST match the ones the Content Drive folder view already publishes for + the same question (`BrowserAPIImpl.ownerName`): `System` for the system user, `unknown` for an + owner that cannot be resolved. Two listings in the same product must not label the same orphaned + owner differently. - **FR-011**: A failure to resolve the creator MUST NOT fail the experiment request. The endpoint still returns its normal success response with the experiment payload and the fallback value. - **FR-012**: A failure to resolve one experiment's creator inside a list MUST NOT affect the other @@ -203,8 +209,8 @@ both the list and the single fetch, and confirm the response succeeds and report - **FR-019**: The committed `openapi.yaml` MUST be regenerated from the annotations and committed together with the code change, so the CI contract check passes. - **FR-020**: Integration tests MUST cover: the happy path from the list endpoint, the happy path - from the single-fetch endpoint, and the unresolvable-creator fallback returning success plus the - raw ID. + from the single-fetch endpoint, and the unresolvable-creator fallback returning success plus + `unknown`. - **FR-021**: Every new integration test class MUST be registered in a `MainSuite*` / `Junit5Suite*` `@SuiteClasses` list, otherwise it never runs in CI. - **FR-022**: Existing experiment tests MUST continue to pass unchanged, in particular any asserting @@ -288,8 +294,11 @@ Endpoints under `/v1/experiments` that do **not** carry an Experiment, and are t construction, including the ones that never reach a response (the database transformer, the push-publish dependency walk, running-experiment selection during page rendering) — which FR-015 forbids; - - a lazily-computed attribute is memoized inside the instance, and experiment instances are - themselves cached, so a memoized name would outlive a rename — which FR-016 forbids. + - a lazily-computed attribute is memoized inside the instance, and the running-experiments list + is cached whole, so a memoized name on those long-lived shared entries would outlive a rename — + which FR-016 forbids. (The per-experiment cache in `ExperimentsFactoryImpl.find` is commented + out with a TODO, so `find` and `list` build fresh instances on every call; only that list cache + is live.) Neither mechanism is settable from JSON, which is where FR-023 comes from. The spec therefore states the constraints (FR-014, FR-015, FR-016, FR-023) and leaves the mechanism to `/speckit-plan`, From 2b25015c5e9c1b30e41f77aedf94c800aaea54cb Mon Sep 17 00:00:00 2001 From: Arcadio Quintero Date: Wed, 16 Sep 2026 17:14:12 -0400 Subject: [PATCH 06/10] fix(experiments): close the review findings on the creator-name field (#37304) A seven-agent review of this PR found that four of its six important findings were documentation of mine asserting things that are not true, plus one real defect and one test suite that could not prove what it claimed. All of those are fixed here. The tests could not tell createdBy from lastModifiedBy. The fixture used the same id for both and stubbed loadUserById(anyString()), so pointing the accessor at lastModifiedBy() left all 13 tests green - the single mapping this feature exists to get right was unasserted. The fixture now uses distinct ids resolving to distinct users, and the guard verifies the modifier id is never looked up. Same mutation now fails 3 tests instead of 0. A creator with only a first name shipped a trailing space. User.getFullName() joins parts with a space and never trims, so "Admin " went straight to the portlet column; UtilMethods.isSet trims for the check but the untrimmed value was returned. The resolver now trims, and a test pins it. Logging lost the evidence it existed to keep. Logger.warn(Object, String) drops the throwable, so a NullPointerException logged as "...: null" with no type and no stack; and under a failing user layer it fired once per row - the very flood the branch above it avoids on purpose. Now warnEveryAndDebug with the throwable and a 60s throttle. The deleted-user debug line carries its exception too, and the blank-name branch - previously the only route to "unknown" with no trace at any level - has its own debug line. Four corrected claims: - push-publish and starter export DO serialize the whole Experiment. ExperimentBundler lives under dotCMS/src/enterprise/java, which my original search never looked at, and ExperimentHandler reads the bundle back. The Javadoc said these paths never serialize. - getFullName() returns " " for an all-blank user, not "". The fallback works only because UtilMethods.isSet trims first, so a plausible cleanup to !isEmpty() would have shipped a blank column. - the wired factory is UserFactoryImpl, not UserFactoryLiferayImpl. Both cache, so the cost argument stands, but the citation pointed at dead code. - a miss is not negative-cached: every row referencing an orphaned creator pays its own query and its own NoSuchUserException. Content Drive documents the same limitation on warmUpUserCache. Also: Content Drive parity is now described as what it is - exact for the deleted and system cases, deliberately stricter for blank names and unset ids - and "nothing here may throw" is narrowed to checked and runtime exceptions, since Error is not caught. Two new tests cover the padded name and the null user branch. 15/15 green, run with the build cache disabled. Left open for a decision, not fixed here: whether newer-sender to older-receiver push publish is supported, since the field now travels in bundles and an older receiver would reject it as an unknown property; and the two acceptance criteria requiring integration tests. --- .../experiments/model/AbstractExperiment.java | 14 ++-- .../model/ExperimentCreatorNameResolver.java | 67 +++++++++++++------ .../ExperimentCreatorNameResolverTest.java | 39 ++++++++++- .../model/ExperimentCreatorNameTest.java | 38 ++++++++--- .../spec.md | 2 +- 5 files changed, 121 insertions(+), 39 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/experiments/model/AbstractExperiment.java b/dotCMS/src/main/java/com/dotcms/experiments/model/AbstractExperiment.java index ce1b59405b1..6a72e8cfb32 100644 --- a/dotCMS/src/main/java/com/dotcms/experiments/model/AbstractExperiment.java +++ b/dotCMS/src/main/java/com/dotcms/experiments/model/AbstractExperiment.java @@ -101,10 +101,16 @@ default Instant modDate() { * *

Deliberately a plain {@code default} method rather than a {@code @Value.Derived} or * {@code @Value.Lazy} attribute: those are computed at construction or memoized per instance, - * which would resolve a user on every Experiment built from the database — including on the - * page-render and push-publish paths, which never serialize the object — and would let a - * memoized name outlive a rename inside the running-experiments cache. As an ordinary default - * method it costs nothing until something serializes the Experiment. + * which would resolve a user on every Experiment built from the database — including the + * running-experiments cache fill behind page rendering, which serializes only the experiment + * id and never the object — and would let a memoized name outlive a rename inside that cache. + * As an ordinary default method it costs nothing until something serializes the Experiment. + * + *

Two paths do serialize the whole Experiment and therefore do pay one lookup each: + * push-publish bundling ({@code ExperimentBundler}, under {@code dotCMS/src/enterprise/java}) + * and starter export ({@code ExportStarterUtil}). The resolved value is written into the + * bundle and ignored on import by a same-version receiver, since the generated {@code Json} + * delegate binds settable attributes only. * *

{@code READ_ONLY} is load-bearing: the generated {@code Experiment.Json} delegate binds * settable attributes only, so without it a payload carrying this field would fail to diff --git a/dotCMS/src/main/java/com/dotcms/experiments/model/ExperimentCreatorNameResolver.java b/dotCMS/src/main/java/com/dotcms/experiments/model/ExperimentCreatorNameResolver.java index d80631d486e..0e3ee60fee8 100644 --- a/dotCMS/src/main/java/com/dotcms/experiments/model/ExperimentCreatorNameResolver.java +++ b/dotCMS/src/main/java/com/dotcms/experiments/model/ExperimentCreatorNameResolver.java @@ -17,21 +17,30 @@ * orphaned reference, a user with no name set, or a lookup that blows up — is reported as * {@code "unknown"}. * - *

Those two labels are not invented here: they are the ones - * {@code BrowserAPIImpl.ownerName} already publishes for the Content Drive folder view, which - * answers exactly this question for a different listing. Two listings in the same product labelling - * the same orphaned owner differently is a worse outcome than either label on its own. (Note that - * the Content Drive implementation reaches the user through {@code UserLocalManagerUtil} and so - * bypasses {@code UserCache}; this one deliberately does not.) + *

The two labels are borrowed from {@link com.dotcms.browser.BrowserAPIImpl}'s {@code ownerName}, + * which answers the same question for the Content Drive folder view: two listings in the same + * product labelling the same orphaned owner differently is a worse outcome than either label on its + * own. The alignment is exact for the deleted/orphaned and system-user cases and deliberately + * stricter in two others: Content Drive publishes the raw {@code getFullName()} for a blank-named + * user (a bare space) and {@code null} for an unset id, where this resolver reports + * {@code "unknown"} so the field is never blank. Content Drive also reaches the user through + * {@link com.liferay.portal.ejb.UserLocalManagerUtil} and so bypasses {@code UserCache}; this one + * deliberately does not. * - *

Nothing here may throw. The caller is a serializer running inside an already-successful - * API response, so an exception escaping this class would turn a working experiment read into a - * failed request over a field that is only decoration. + *

No checked or runtime exception may escape. The caller is a serializer running inside + * an already-successful API response, so an exception escaping this class would turn a working + * experiment read into a failed request over a field that is only decoration. {@code Error} and its + * subclasses are deliberately not caught. * - *

Cost. {@code loadUserById} is served by {@code UserCache}, so a listing whose - * experiments share a creator costs one database read for the first row and an in-memory hit for - * every row after it. This class deliberately adds no memo of its own: the value is resolved per - * serialization so that a user who renames themselves is reported under the new name. + *

Cost. {@code UserFactoryImpl.loadUserById} consults {@code UserCache} before the + * database, so a listing whose experiments share a resolvable creator costs one database + * read for the first row and an in-memory hit for every row after it. An unresolvable + * creator is the exception worth knowing about: a miss is not negative-cached, so every row + * referencing an orphaned id pays its own query and its own {@code NoSuchUserException}. The same + * residual limitation is documented for Content Drive on + * {@code BrowserAPIImpl#warmUpUserCache}. This class deliberately adds no memo of its own: the + * value is resolved per serialization so that a user who renames themselves is reported under the + * new name. */ public class ExperimentCreatorNameResolver { @@ -41,6 +50,9 @@ public class ExperimentCreatorNameResolver { /** Shown for the system user. Matches Content Drive. */ static final String SYSTEM = "System"; + /** One warning per minute when the user layer is failing, rather than one per experiment. */ + private static final int WARN_THROTTLE_MILLIS = 60_000; + /** * The instance used by the model. The {@link UserAPI} is supplied lazily rather than captured, * so class initialization never depends on {@code APILocator} being ready. @@ -64,9 +76,9 @@ public class ExperimentCreatorNameResolver { * Resolves a creator's display name. * * @param createdById the experiment's {@code createdBy} user ID - * @return the creator's full name; {@code "System"} for the system user; {@code "unknown"} when - * the ID is unset, resolves to nobody, or resolves to a user with no name set. Never null, - * never empty. + * @return the creator's full name, trimmed; {@code "System"} for the system user; + * {@code "unknown"} when the ID is unset, resolves to nobody, resolves to a user with no name + * set, or the lookup itself fails. Never null, never blank. */ public String resolve(final String createdById) { if (!UtilMethods.isSet(createdById)) { @@ -79,20 +91,33 @@ public String resolve(final String createdById) { try { final User creator = this.userAPI.get().loadUserById(createdById); + // getFullName() joins the parts with spaces and never trims, so a user with only a + // first name yields "Admin ". Trim before publishing: the padding would be visible in + // the portlet column, and an all-blank name must collapse to the fallback. final String fullName = null != creator ? creator.getFullName() : null; - return UtilMethods.isSet(fullName) ? fullName : UNKNOWN; + if (UtilMethods.isSet(fullName)) { + return fullName.trim(); + } + + Logger.debug(this, () -> String.format( + "Experiment creator '%s' resolves to a User with no name set; reporting '%s'", + createdById, UNKNOWN)); + return UNKNOWN; } catch (final NoSuchUserException e) { // A deleted or orphaned creator is a data condition, not an error: debug keeps a // listing full of them from flooding the log while leaving the detail reachable. - Logger.debug(this, () -> String.format( + Logger.debug(this, e, () -> String.format( "Experiment creator '%s' no longer resolves to a User; reporting '%s'", createdById, UNKNOWN)); return UNKNOWN; } catch (final Exception e) { - Logger.warn(this, String.format( - "Failed to resolve the name of Experiment creator '%s': %s", - createdById, e.getMessage())); + // Unlike the branch above, this one means something is actually wrong, and under a + // failing user layer it fires once per row. Throttle it and keep the stack trace: + // without the throwable a NullPointerException logs as "...: null" and is undiagnosable. + Logger.warnEveryAndDebug(ExperimentCreatorNameResolver.class, String.format( + "Failed to resolve the name of Experiment creator '%s'", createdById), + e, WARN_THROTTLE_MILLIS); return UNKNOWN; } } diff --git a/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameResolverTest.java b/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameResolverTest.java index eaaed2a2841..a919918a1ff 100644 --- a/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameResolverTest.java +++ b/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameResolverTest.java @@ -71,9 +71,11 @@ void resolve_userWithName_returnsFullName() throws Exception { /** * Method to test: {@link ExperimentCreatorNameResolver#resolve(String)} * Given Scenario: The creator resolves to a User whose first, middle and last name are all - * blank. {@code User.getFullName()} returns an empty string for that user. - * ExpectedResult: {@code "unknown"}, not the empty string — the field is never empty - * (FR-003, FR-010). + * blank. {@code User.getFullName()} joins the blank parts with a space, so it + * returns {@code " "} — a single space, NOT the empty string. + * ExpectedResult: {@code "unknown"} — the field is never blank (FR-003, FR-010). Note what + * makes this work: {@code UtilMethods.isSet} trims before measuring length. + * A non-trimming emptiness check would let the space through to the column. */ @Test void resolve_userWithBlankName_fallsBackToUnknown() throws Exception { @@ -123,6 +125,37 @@ void resolve_unexpectedRuntimeFailure_fallsBackToUnknown() throws Exception { assertEquals(UNKNOWN, resolver.resolve(CREATOR_ID)); } + /** + * Method to test: {@link ExperimentCreatorNameResolver#resolve(String)} + * Given Scenario: The creator has a first name but no last name (or the reverse). + * {@code User.getFullName()} concatenates unconditionally with a space and + * does not trim, so it hands back {@code "Admin "}. + * ExpectedResult: {@code "Admin"} — the padding must not reach the portlet's Created By + * column. This is the branch adjacent to the all-blank one, and the reason + * the resolver trims rather than trusting the source. + */ + @Test + void resolve_userWithOnlyAFirstName_returnsTheNameWithoutPadding() throws Exception { + when(userAPI.loadUserById(CREATOR_ID)).thenReturn(userNamed("Admin", "", "")); + + assertEquals("Admin", resolver.resolve(CREATOR_ID)); + } + + /** + * Method to test: {@link ExperimentCreatorNameResolver#resolve(String)} + * Given Scenario: The user layer returns {@code null} instead of throwing. The production + * {@code UserAPIImpl} throws {@link NoSuchUserException} rather than returning + * null, so this pins a defensive branch reachable through any other + * {@code UserAPI} implementation or decorator. + * ExpectedResult: {@code "unknown"}, with no NullPointerException. + */ + @Test + void resolve_nullUser_fallsBackToUnknown() throws Exception { + when(userAPI.loadUserById(CREATOR_ID)).thenReturn(null); + + assertEquals(UNKNOWN, resolver.resolve(CREATOR_ID)); + } + /** * Method to test: {@link ExperimentCreatorNameResolver#resolve(String)} * Given Scenario: The experiment was created by the system user, whose ID is {@code "system"}. diff --git a/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java b/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java index c79a90044f6..09cbd556110 100644 --- a/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java +++ b/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java @@ -3,10 +3,12 @@ import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; @@ -49,19 +51,28 @@ class ExperimentCreatorNameTest { private static final String CREATOR_ID = "dotcms.org.1"; private static final String CREATOR_NAME = "Admin User"; + /** + * Deliberately different from {@link #CREATOR_ID}. If both ids were the same, every test here + * would still pass with the accessor reading {@code lastModifiedBy()} instead of + * {@code createdBy()} — the one mapping this feature exists to get right would be unasserted. + */ + private static final String MODIFIER_ID = "dotcms.org.2"; + private static final String MODIFIER_NAME = "Other Person"; + /** * {@code lookBackWindowExpireTime} is set explicitly on purpose. Leaving it unset makes - * Immutables evaluate its {@code @Value.Default}, which reads {@code ConfigExperimentUtil} and - * therefore {@code Config} — unavailable in a plain unit-test JVM, where it fails with - * {@code ExceptionInInitializerError} long before this feature gets a say. Setting the value - * keeps the test failing (and passing) for reasons that belong to #37304. + * Immutables evaluate its {@code @Value.Default}, which calls + * {@code ConfigExperimentUtil.lookBackWindowDefaultExpireTime()} and through it + * {@code APILocator.getExperimentsAPI()} — dragging in the CDI/OpenSearch wiring that no plain + * unit-test JVM has, and which is mocked to a stub here anyway. Setting the value keeps the + * test failing (and passing) for reasons that belong to #37304. */ private static Experiment anExperiment() { return Experiment.builder() .name("Homepage CTA test") .pageId("2d8b8b1e-0000-0000-0000-000000000001") .createdBy(CREATOR_ID) - .lastModifiedBy(CREATOR_ID) + .lastModifiedBy(MODIFIER_ID) .id("0e8b8b1e-0000-0000-0000-000000000002") .lookBackWindowExpireTime(1_800_000L) .build(); @@ -72,12 +83,14 @@ private static User adminUser() { } /** - * Runs the body with {@code APILocator.getUserAPI()} answering with a stub that resolves - * {@link #CREATOR_ID} to {@link #adminUser()}. + * Runs the body with {@code APILocator.getUserAPI()} answering a stub that resolves + * {@link #CREATOR_ID} and {@link #MODIFIER_ID} to different users, so an accessor + * reading the wrong field is visible rather than silently equivalent. */ private static void withResolvableCreator(final ThrowingRunnable body) throws Exception { final UserAPI userAPI = mock(UserAPI.class); - when(userAPI.loadUserById(anyString())).thenReturn(adminUser()); + when(userAPI.loadUserById(CREATOR_ID)).thenReturn(adminUser()); + when(userAPI.loadUserById(MODIFIER_ID)).thenReturn(userNamed("Other", "Person")); try (MockedStatic apiLocator = mockStatic(APILocator.class)) { apiLocator.when(APILocator::getUserAPI).thenReturn(userAPI); @@ -107,6 +120,8 @@ void serializedExperiment_carriesCreatorName() throws Exception { assertTrue(payload.has("createdByUserName"), "The payload should carry createdByUserName"); assertEquals(CREATOR_NAME, payload.get("createdByUserName").asText()); + assertNotEquals(MODIFIER_NAME, payload.get("createdByUserName").asText(), + "The name must come from createdBy, never from lastModifiedBy"); }); } @@ -178,8 +193,10 @@ void addingTheField_leavesTheExistingContractAlone() throws Exception { "createdBy must still be the user ID"); assertEquals(CREATOR_ID, experiment.getOwner(), "getOwner() must still resolve from createdBy, not from the new field"); - assertEquals(CREATOR_ID, payload.get("lastModifiedBy").asText(), - "lastModifiedBy is out of scope and must be untouched"); + assertEquals(MODIFIER_ID, payload.get("lastModifiedBy").asText(), + "lastModifiedBy is out of scope: still the raw id, and no name companion"); + assertFalse(payload.has("lastModifiedByUserName"), + "lastModifiedBy must not gain a name companion (FR-008)"); for (final String computed : new String[]{"owner", "identifier", "permissionId", "manifestInfo", "parentPermissionable", "acceptedPermissions"}) { @@ -224,6 +241,7 @@ void buildingAnExperimentWithoutSerializing_neverResolvesTheCreator() throws Exc DotObjectMapperProvider.createDefaultMapper().writeValueAsString(experiment); verify(userAPI).loadUserById(CREATOR_ID); + verify(userAPI, never()).loadUserById(MODIFIER_ID); } } diff --git a/specs/37304-experiment-created-by-username/spec.md b/specs/37304-experiment-created-by-username/spec.md index ca163637174..6ddd39be076 100644 --- a/specs/37304-experiment-created-by-username/spec.md +++ b/specs/37304-experiment-created-by-username/spec.md @@ -314,7 +314,7 @@ Endpoints under `/v1/experiments` that do **not** carry an Experiment, and are t - **A4 — "Full name" means the platform's existing notion of a full name**: first, middle and last name joined as `User.getFullName()` already does, rather than a new formatting rule invented here. - That method returns an empty string when every part is blank, which is exactly the case FR-010 + That method joins the parts with spaces and never trims, so it returns a single space when every part is blank, and leaves padding like "Admin " when only one part is set, which is exactly the case FR-010 covers. - **A5 — There is no persistence round-trip to break.** Experiments are rebuilt from database columns From 7e10bbf066aef3269aff9ec44cc213d94e031e3f Mon Sep 17 00:00:00 2001 From: Arcadio Quintero Date: Wed, 16 Sep 2026 17:22:33 -0400 Subject: [PATCH 07/10] fix(experiments): keep createdByUserName out of push-publish bundles (#37304) The field is resolved at serialization time, so it was travelling inside push-publish bundles and starter exports as well as REST responses. Bundles are read back through a bare ObjectMapper that leaves FAIL_ON_UNKNOWN_PROPERTIES at Jackson's default, so a receiver running a build without the field rejects the experiment file as an unknown property. ExperimentHandler wraps its whole loop in one catch and rethrows, and BundlePublisher runs every handler inside a single transaction - so that rejection rolls back the ENTIRE bundle, taking down the pages, contentlets and templates that shipped alongside it. BundlerUtil now registers a mix-in that ignores the field for Experiment. It never reaches a bundle, those paths perform no user lookup, and a current receiver also becomes tolerant of a bundle that happens to carry it. Nothing is lost: createdBy is still bundled and the receiver resolves the name itself when it serves the experiment over REST. Note what this does NOT fix: receivers already deployed on older builds keep their strict mapper, so the only thing that protects them is the field not being in the bundle. Disabling FAIL_ON_UNKNOWN_PROPERTIES here would have helped future receivers only. Two tests pin both halves, and the first was confirmed failing before the mix-in existed: the bundled shape must not contain createdByUserName, and the REST payload must still contain it - excluding it from bundles must not quietly remove it from the API, which is the whole point of the feature. 17/17 green with the build cache disabled; openapi.yaml unchanged, since the REST contract did not move. --- .../experiments/model/AbstractExperiment.java | 11 ++-- .../com/dotcms/publishing/BundlerUtil.java | 23 +++++++++ .../model/ExperimentCreatorNameTest.java | 51 +++++++++++++++++++ .../experiment-created-by-username.md | 12 +++++ 4 files changed, 92 insertions(+), 5 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/experiments/model/AbstractExperiment.java b/dotCMS/src/main/java/com/dotcms/experiments/model/AbstractExperiment.java index 6a72e8cfb32..b69199f1f5f 100644 --- a/dotCMS/src/main/java/com/dotcms/experiments/model/AbstractExperiment.java +++ b/dotCMS/src/main/java/com/dotcms/experiments/model/AbstractExperiment.java @@ -106,11 +106,12 @@ default Instant modDate() { * id and never the object — and would let a memoized name outlive a rename inside that cache. * As an ordinary default method it costs nothing until something serializes the Experiment. * - *

Two paths do serialize the whole Experiment and therefore do pay one lookup each: - * push-publish bundling ({@code ExperimentBundler}, under {@code dotCMS/src/enterprise/java}) - * and starter export ({@code ExportStarterUtil}). The resolved value is written into the - * bundle and ignored on import by a same-version receiver, since the generated {@code Json} - * delegate binds settable attributes only. + *

Two paths do serialize the whole Experiment — push-publish bundling + * ({@code ExperimentBundler}) and starter export ({@code ExportStarterUtil}) — and both are + * deliberately excluded: {@code BundlerUtil} registers a mix-in that ignores this field, so it + * never reaches a bundle and those paths pay no lookup. That keeps bundles readable by a + * receiver running a build without the field, which would otherwise reject the file as an + * unknown property and roll back the whole bundle. * *

{@code READ_ONLY} is load-bearing: the generated {@code Experiment.Json} delegate binds * settable attributes only, so without it a payload carrying this field would fail to diff --git a/dotCMS/src/main/java/com/dotcms/publishing/BundlerUtil.java b/dotCMS/src/main/java/com/dotcms/publishing/BundlerUtil.java index af9196bb969..067373eb8b5 100644 --- a/dotCMS/src/main/java/com/dotcms/publishing/BundlerUtil.java +++ b/dotCMS/src/main/java/com/dotcms/publishing/BundlerUtil.java @@ -20,6 +20,8 @@ import com.dotmarketing.util.Logger; import com.dotmarketing.util.UtilMethods; import com.dotmarketing.util.XMLUtils; +import com.dotcms.experiments.model.Experiment; +import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.core.JsonGenerator; import com.fasterxml.jackson.core.JsonParseException; import com.fasterxml.jackson.core.JsonParser; @@ -326,10 +328,31 @@ private static ObjectMapper getObjectMapper() { objectMapper.registerModule(javaTimeModule); objectMapper.registerModule(new Jdk8Module()); objectMapper.registerModule(new GuavaModule()); + objectMapper.addMixIn(Experiment.class, ExperimentBundleMixIn.class); } return objectMapper; } + /** + * Keeps read-time derived fields out of the bundled shape of an {@link Experiment}. + * + *

{@code createdByUserName} is resolved when an Experiment is serialized, so it would + * otherwise be written into the bundle and into starter exports. That matters because this + * mapper deliberately leaves {@code FAIL_ON_UNKNOWN_PROPERTIES} at Jackson's default: a + * receiver running a build that predates the field would reject the file as an unknown + * property, and {@link com.dotcms.publisher.receiver.BundlePublisher} runs every handler inside + * one transaction — so that rejection rolls back the entire bundle, not just the experiment. + * + *

The field is a display value derived from {@code createdBy}, which IS bundled, so nothing + * is lost: the receiver resolves the name itself when it serves the experiment over REST. + * Ignoring it here also makes a current receiver tolerant of a bundle that happens to carry it. + */ + private abstract static class ExperimentBundleMixIn { + + @JsonIgnore + abstract String createdByUserName(); + } + private static ObjectMapper getCustomMapper() { if (customMapper == null) { customMapper = DotObjectMapperProvider.getInstance().getDefaultObjectMapper(); diff --git a/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java b/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java index 09cbd556110..12135ec25e2 100644 --- a/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java +++ b/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java @@ -13,12 +13,15 @@ import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; +import com.dotcms.publishing.BundlerUtil; import com.dotcms.rest.api.v1.DotObjectMapperProvider; import com.dotmarketing.business.APILocator; import com.dotmarketing.business.UserAPI; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import com.liferay.portal.model.User; +import java.io.ByteArrayOutputStream; +import java.nio.charset.StandardCharsets; import org.junit.jupiter.api.Test; import org.mockito.MockedStatic; @@ -277,6 +280,54 @@ void renamingTheCreator_isReflectedOnTheNextSerialization() throws Exception { } } + /** + * Method to test: {@code AbstractExperiment.createdByUserName()} as seen by the push-publish + * bundler. + * Given Scenario: An Experiment serialized with the mapper push-publish bundling and starter + * export use ({@link com.dotcms.publishing.BundlerUtil}), rather than the REST + * mapper. + * ExpectedResult: {@code createdByUserName} is absent, while {@code createdBy} is still + * there. + * + *

Why this matters enough to have its own test: the bundle is read back by + * {@code ExperimentHandler} through a bare {@code ObjectMapper}, which fails on unknown + * properties. A receiver on a build without this field would therefore reject the file — and + * {@code BundlePublisher} runs every handler in one transaction, so that rejection rolls back + * the whole bundle, not just the experiment. Keeping the field out of the bundled shape + * is what makes the bundle byte-compatible with older receivers. + */ + @Test + void bundledExperiment_doesNotCarryTheCreatorName() throws Exception { + withResolvableCreator(() -> { + final ByteArrayOutputStream out = new ByteArrayOutputStream(); + BundlerUtil.objectToJSON(anExperiment(), out); + final String bundled = out.toString(StandardCharsets.UTF_8); + + assertFalse(bundled.contains("createdByUserName"), + "The bundled shape must not carry createdByUserName: an older receiver would " + + "reject it as an unknown property and roll back the entire bundle"); + assertTrue(bundled.contains("createdBy"), + "Precondition: the bundled shape should still carry createdBy"); + }); + } + + /** + * Method to test: {@code AbstractExperiment.createdByUserName()} as seen by the REST layer. + * Given Scenario: The same Experiment, serialized with the REST mapper. + * ExpectedResult: The field IS present — excluding it from bundles must not remove it from the + * API response, which is the entire point of the feature. + */ + @Test + void excludingItFromBundles_leavesTheApiResponseIntact() throws Exception { + withResolvableCreator(() -> { + final ObjectMapper mapper = DotObjectMapperProvider.createDefaultMapper(); + + final JsonNode payload = mapper.readTree(mapper.writeValueAsString(anExperiment())); + + assertEquals(CREATOR_NAME, payload.get("createdByUserName").asText()); + }); + } + private static User userNamed(final String first, final String last) { final User user = new User(); user.setFirstName(first); diff --git a/specs/37304-experiment-created-by-username/contracts/experiment-created-by-username.md b/specs/37304-experiment-created-by-username/contracts/experiment-created-by-username.md index 6afc80d37e9..709ab2fb395 100644 --- a/specs/37304-experiment-created-by-username/contracts/experiment-created-by-username.md +++ b/specs/37304-experiment-created-by-username/contracts/experiment-created-by-username.md @@ -73,6 +73,18 @@ Drive folder view, so the same orphaned owner reads the same in both listings. The field is never `null`, never absent and never `""`. +## Bundles and starter exports (excluded) + +The field is a REST-response concern only. `BundlerUtil` registers a mix-in that ignores it, so it +does **not** appear in push-publish bundles or starter exports, and those paths perform no user +lookup. + +The reason is compatibility, not tidiness: bundles are read back through a bare `ObjectMapper` that +fails on unknown properties, and `BundlePublisher` runs every handler in a single transaction — so a +receiver on a build without the field would reject the experiment file and roll back the **entire** +bundle. `createdBy` is bundled as before, and the receiver resolves the name itself when it serves +the experiment over REST. + ## Deserialization contract (FR-023) `createdByUserName` is **read-only**: serialized on the way out, ignored on the way in. A payload From e2451002325c1b2cae57fdd8afaf63a8778c568c Mon Sep 17 00:00:00 2001 From: Arcadio Quintero Date: Wed, 16 Sep 2026 17:24:20 -0400 Subject: [PATCH 08/10] docs(experiments): correct the push-publish assumption in the spec (#37304) Assumption A5 stated that the push-publish wrapper for experiments has no bundler or handler wired to it, and concluded FR-023 only guarded future and client-side round-trips. Both are false, and the error came from a search limited to dotCMS/src/main/java: ExperimentBundler and ExperimentHandler live under dotCMS/src/enterprise/java. A5 now describes the real path - the bundler serializes the whole Experiment through BundlerUtil, the handler reads it back through a bare ObjectMapper that fails on unknown properties, ExperimentHandler catches around its whole loop and rethrows, and BundlePublisher runs every handler in one transaction, so a receiver without the field rolls back the entire bundle rather than the single experiment. Nothing in that path checks that sender and receiver run the same version, and starter export serializes Experiments the same way. FR-023a records the resulting requirement, which 7e10bbf066 already implements and tests: the field must not appear in bundles or starter exports. It is derived from createdBy, which is bundled, so the receiver resolves the name itself over REST and nothing is lost. The model Javadoc that carried the same claim was already corrected in 7e10bbf066; this brings the spec in line with the code and with the fact. --- .../spec.md | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/specs/37304-experiment-created-by-username/spec.md b/specs/37304-experiment-created-by-username/spec.md index 6ddd39be076..bf13eda501b 100644 --- a/specs/37304-experiment-created-by-username/spec.md +++ b/specs/37304-experiment-created-by-username/spec.md @@ -218,6 +218,11 @@ both the list and the single fetch, and confirm the response succeeds and report - **FR-023**: An Experiment payload that contains `createdByUserName` MUST still be readable wherever Experiment JSON is parsed back into an Experiment. The added field must not make a round-tripped payload fail to parse. +- **FR-023a**: `createdByUserName` MUST NOT appear in push-publish bundles or starter exports. Those + artifacts are read back by a receiver that may be running a build without the field, and that + receiver fails the entire bundle rather than the single experiment. The field is derived from + `createdBy`, which is bundled, so the receiver can resolve the name itself when it serves the + experiment over REST — nothing is lost by excluding it. ### Endpoints in scope @@ -317,10 +322,18 @@ Endpoints under `/v1/experiments` that do **not** carry an Experiment, and are t That method joins the parts with spaces and never trims, so it returns a single space when every part is blank, and leaves padding like "Admin " when only one part is set, which is exactly the case FR-010 covers. -- **A5 — There is no persistence round-trip to break.** Experiments are rebuilt from database columns - by a transformer, not by parsing stored Experiment JSON, and the push-publish wrapper for - experiments has no bundler or handler wired to it today. FR-023 is therefore a guard against future - and client-side round-trips, not a description of a path that runs on every read. +- **A5 — The database is not a round-trip, but push publishing is.** Experiments are rebuilt from + database columns by a transformer, not by parsing stored Experiment JSON, so persistence itself + cannot break. Push publishing is a different story and an earlier draft of this spec had it wrong: + `ExperimentBundler` and `ExperimentHandler` are live, they simply live under + `dotCMS/src/enterprise/java` rather than `src/main/java`. The bundler serializes the whole + Experiment through `BundlerUtil`, and the handler reads it back through a bare `ObjectMapper` that + leaves `FAIL_ON_UNKNOWN_PROPERTIES` at Jackson's default. `ExperimentHandler` catches around its + entire loop and rethrows, and `BundlePublisher` runs every handler inside one transaction — so a + receiver on a build without the field would reject the experiment file and roll back the **whole** + bundle, including the pages and contentlets that shipped with it. Nothing in the push-publish path + checks that sender and receiver run the same version. Starter export (`ExportStarterUtil`) + serializes Experiments the same way. FR-023a is the consequence. - **A6 — The consumer needs one display string.** #37307 renders a single Created By column, so a single pre-joined name is sufficient; separate first/last fields are not required. From aec284cc139d82bd58ac5f0cbf6e071e6cf496d9 Mon Sep 17 00:00:00 2001 From: Arcadio Quintero Date: Wed, 16 Sep 2026 19:52:57 -0400 Subject: [PATCH 09/10] refactor(experiments): derive the creator name in the REST layer, not on the model (#37304) Review on #37510 asked for this: AbstractExperiment is a data model, the derivation belongs in the REST layer so only the endpoint response is affected, and then BundlerUtil needs no change at all. Freddy is right, and the evidence this PR accumulated argues his side rather than mine. Putting the accessor on the model meant every serialization of an Experiment resolved a user, and serialization is not only REST: push-publish bundling and starter export serialize the whole object, their receiver rejects unknown properties, and BundlePublisher runs every handler in one transaction - so an older receiver rolled back the entire bundle. My answer to that was a mix-in in BundlerUtil, which coupled a generic publishing utility to one domain model to contain a problem the layering had created. Moving the derivation removes the problem at the source instead of mitigating it. ExperimentView wraps the Experiment with @JsonUnwrapped and adds the field, so the wire shape stays flat and unchanged except for the addition. The wrapping happens inside ResponseEntitySingleExperimentView and ResponseEntityExperimentView, which is why none of the 14 endpoints changed a line - ExperimentsResource is not in this diff. Reverted as a result: the accessor and its Schema on AbstractExperiment, and the BundlerUtil mix-in. The model is a plain data object again. openapi.yaml reflects the move: the Experiment schema is back to what it was, a new ExperimentView schema carries the flattened fields plus createdByUserName, and both response wrappers now $ref the view. FR-023 stops being a hazard rather than being satisfied: with no serialize-only property on the model, the Experiment payload round-trips exactly as before, and the view is a response type that is never deserialized. A test pins that the bare Experiment still round-trips and that serializing one costs no user lookup. 19/19 green with the build cache disabled, including the neighbouring ConfigExperimentUtilTest. Tests moved to the REST package alongside the code. Not replied to on the PR yet, per the user: the two MainSuite comments. These are unit tests, they run in the PR Test / JVM Unit Tests job - verified in this PR's own run - and there is no unit-test suite in the repo to register with. --- .../experiments/model/AbstractExperiment.java | 36 -- .../com/dotcms/publishing/BundlerUtil.java | 23 -- .../ExperimentCreatorNameResolver.java | 4 +- .../api/v1/experiments/ExperimentView.java | 63 ++++ .../ResponseEntityExperimentView.java | 5 +- .../ResponseEntitySingleExperimentView.java | 5 +- .../main/webapp/WEB-INF/openapi/openapi.yaml | 64 +++- .../model/ExperimentCreatorNameTest.java | 338 ------------------ .../ExperimentCreatorNameResolverTest.java | 2 +- .../v1/experiments/ExperimentViewTest.java | 260 ++++++++++++++ .../experiment-created-by-username.md | 28 +- .../spec.md | 31 +- 12 files changed, 413 insertions(+), 446 deletions(-) rename dotCMS/src/main/java/com/dotcms/{experiments/model => rest/api/v1/experiments}/ExperimentCreatorNameResolver.java (97%) create mode 100644 dotCMS/src/main/java/com/dotcms/rest/api/v1/experiments/ExperimentView.java delete mode 100644 dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java rename dotCMS/src/test/java/com/dotcms/{experiments/model => rest/api/v1/experiments}/ExperimentCreatorNameResolverTest.java (99%) create mode 100644 dotCMS/src/test/java/com/dotcms/rest/api/v1/experiments/ExperimentViewTest.java diff --git a/dotCMS/src/main/java/com/dotcms/experiments/model/AbstractExperiment.java b/dotCMS/src/main/java/com/dotcms/experiments/model/AbstractExperiment.java index b69199f1f5f..1b96a162a3a 100644 --- a/dotCMS/src/main/java/com/dotcms/experiments/model/AbstractExperiment.java +++ b/dotCMS/src/main/java/com/dotcms/experiments/model/AbstractExperiment.java @@ -16,7 +16,6 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.databind.annotation.JsonDeserialize; import com.fasterxml.jackson.databind.annotation.JsonSerialize; -import io.swagger.v3.oas.annotations.media.Schema; import io.vavr.control.Try; import java.io.Serializable; import java.time.Instant; @@ -91,41 +90,6 @@ default Instant modDate() { @JsonProperty("createdBy") String createdBy(); - /** - * The display name of the user behind {@link #createdBy()}, resolved when the Experiment is - * serialized and never stored, so a creator who renames themselves is reported under the new - * name. Reports {@code "System"} for the system user and {@code "unknown"} when the creator - * cannot be resolved or has no name set, matching the labels the Content Drive folder view - * already uses — the value is never null, absent or empty. See - * {@link ExperimentCreatorNameResolver}. - * - *

Deliberately a plain {@code default} method rather than a {@code @Value.Derived} or - * {@code @Value.Lazy} attribute: those are computed at construction or memoized per instance, - * which would resolve a user on every Experiment built from the database — including the - * running-experiments cache fill behind page rendering, which serializes only the experiment - * id and never the object — and would let a memoized name outlive a rename inside that cache. - * As an ordinary default method it costs nothing until something serializes the Experiment. - * - *

Two paths do serialize the whole Experiment — push-publish bundling - * ({@code ExperimentBundler}) and starter export ({@code ExportStarterUtil}) — and both are - * deliberately excluded: {@code BundlerUtil} registers a mix-in that ignores this field, so it - * never reaches a bundle and those paths pay no lookup. That keeps bundles readable by a - * receiver running a build without the field, which would otherwise reject the file as an - * unknown property and roll back the whole bundle. - * - *

{@code READ_ONLY} is load-bearing: the generated {@code Experiment.Json} delegate binds - * settable attributes only, so without it a payload carrying this field would fail to - * deserialize as an unknown property. - */ - @JsonProperty(value = "createdByUserName", access = JsonProperty.Access.READ_ONLY) - @Schema(description = "Display name of the user who created the experiment. Reports \"System\" " - + "for the system user and \"unknown\" when the user cannot be resolved or has no name " - + "set, so the value is never empty.", - example = "Admin User") - default String createdByUserName() { - return ExperimentCreatorNameResolver.INSTANCE.resolve(createdBy()); - } - @JsonProperty("lastModifiedBy") String lastModifiedBy(); diff --git a/dotCMS/src/main/java/com/dotcms/publishing/BundlerUtil.java b/dotCMS/src/main/java/com/dotcms/publishing/BundlerUtil.java index 067373eb8b5..af9196bb969 100644 --- a/dotCMS/src/main/java/com/dotcms/publishing/BundlerUtil.java +++ b/dotCMS/src/main/java/com/dotcms/publishing/BundlerUtil.java @@ -20,8 +20,6 @@ import com.dotmarketing.util.Logger; import com.dotmarketing.util.UtilMethods; import com.dotmarketing.util.XMLUtils; -import com.dotcms.experiments.model.Experiment; -import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.core.JsonGenerator; import com.fasterxml.jackson.core.JsonParseException; import com.fasterxml.jackson.core.JsonParser; @@ -328,31 +326,10 @@ private static ObjectMapper getObjectMapper() { objectMapper.registerModule(javaTimeModule); objectMapper.registerModule(new Jdk8Module()); objectMapper.registerModule(new GuavaModule()); - objectMapper.addMixIn(Experiment.class, ExperimentBundleMixIn.class); } return objectMapper; } - /** - * Keeps read-time derived fields out of the bundled shape of an {@link Experiment}. - * - *

{@code createdByUserName} is resolved when an Experiment is serialized, so it would - * otherwise be written into the bundle and into starter exports. That matters because this - * mapper deliberately leaves {@code FAIL_ON_UNKNOWN_PROPERTIES} at Jackson's default: a - * receiver running a build that predates the field would reject the file as an unknown - * property, and {@link com.dotcms.publisher.receiver.BundlePublisher} runs every handler inside - * one transaction — so that rejection rolls back the entire bundle, not just the experiment. - * - *

The field is a display value derived from {@code createdBy}, which IS bundled, so nothing - * is lost: the receiver resolves the name itself when it serves the experiment over REST. - * Ignoring it here also makes a current receiver tolerant of a bundle that happens to carry it. - */ - private abstract static class ExperimentBundleMixIn { - - @JsonIgnore - abstract String createdByUserName(); - } - private static ObjectMapper getCustomMapper() { if (customMapper == null) { customMapper = DotObjectMapperProvider.getInstance().getDefaultObjectMapper(); diff --git a/dotCMS/src/main/java/com/dotcms/experiments/model/ExperimentCreatorNameResolver.java b/dotCMS/src/main/java/com/dotcms/rest/api/v1/experiments/ExperimentCreatorNameResolver.java similarity index 97% rename from dotCMS/src/main/java/com/dotcms/experiments/model/ExperimentCreatorNameResolver.java rename to dotCMS/src/main/java/com/dotcms/rest/api/v1/experiments/ExperimentCreatorNameResolver.java index 0e3ee60fee8..5e83af46cd1 100644 --- a/dotCMS/src/main/java/com/dotcms/experiments/model/ExperimentCreatorNameResolver.java +++ b/dotCMS/src/main/java/com/dotcms/rest/api/v1/experiments/ExperimentCreatorNameResolver.java @@ -1,4 +1,4 @@ -package com.dotcms.experiments.model; +package com.dotcms.rest.api.v1.experiments; import com.dotmarketing.business.APILocator; import com.dotmarketing.business.NoSuchUserException; @@ -9,7 +9,7 @@ import java.util.function.Supplier; /** - * Turns an {@link AbstractExperiment#createdBy()} user ID into the display name published as + * Turns an {@code Experiment.createdBy()} user ID into the display name published as * {@code createdByUserName} (#37304). * *

The field is never null, absent or empty. The system user is reported as diff --git a/dotCMS/src/main/java/com/dotcms/rest/api/v1/experiments/ExperimentView.java b/dotCMS/src/main/java/com/dotcms/rest/api/v1/experiments/ExperimentView.java new file mode 100644 index 00000000000..25000337221 --- /dev/null +++ b/dotCMS/src/main/java/com/dotcms/rest/api/v1/experiments/ExperimentView.java @@ -0,0 +1,63 @@ +package com.dotcms.rest.api.v1.experiments; + +import com.dotcms.experiments.model.Experiment; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonUnwrapped; +import io.swagger.v3.oas.annotations.media.Schema; + +/** + * REST view of an {@link Experiment}: the experiment exactly as it is persisted, plus the display + * name of the user who created it. + * + *

The derivation lives here rather than on the model on purpose. {@link Experiment} is a data + * object, and anything computed on it is computed everywhere it is serialized — which is not only + * REST. Push-publish bundling and starter export serialize the whole Experiment too, and their + * receiver reads it back through a mapper that rejects unknown properties and rolls back the entire + * bundle when it finds one. Keeping the name in the view means only the endpoint response carries + * it, the model stays a plain POJO, and no other serialization path pays a user lookup. + * + *

The experiment is {@link JsonUnwrapped}, so the payload is the Experiment's own fields with + * {@code createdByUserName} beside them rather than nested — the wire contract consumers see is a + * flat object, unchanged except for the added field. + */ +public class ExperimentView { + + private final Experiment experiment; + + /** + * Wraps an Experiment for a REST response. + * + * @param experiment the experiment to publish + * @return a view that serializes the experiment plus its creator's display name + */ + public static ExperimentView of(final Experiment experiment) { + return new ExperimentView(experiment); + } + + private ExperimentView(final Experiment experiment) { + this.experiment = experiment; + } + + @JsonUnwrapped + public Experiment getExperiment() { + return this.experiment; + } + + /** + * The display name of the user behind the experiment's {@code createdBy}, resolved while the + * response is written and never stored, so a creator who renames themselves is reported under + * the new name. Reports {@code "System"} for the system user and {@code "unknown"} when the + * creator cannot be resolved or has no name set, matching the labels the Content Drive folder + * view already uses. Never null, never blank. + * + * @return the creator's display name + */ + @JsonProperty("createdByUserName") + @Schema(description = "Display name of the user who created the experiment. Reports \"System\" " + + "for the system user and \"unknown\" when the user cannot be resolved or has no name " + + "set, so the value is never empty.", + example = "Admin User") + public String getCreatedByUserName() { + return ExperimentCreatorNameResolver.INSTANCE.resolve(this.experiment.createdBy()); + } +} diff --git a/dotCMS/src/main/java/com/dotcms/rest/api/v1/experiments/ResponseEntityExperimentView.java b/dotCMS/src/main/java/com/dotcms/rest/api/v1/experiments/ResponseEntityExperimentView.java index d925663677a..35c2ef74b98 100644 --- a/dotCMS/src/main/java/com/dotcms/rest/api/v1/experiments/ResponseEntityExperimentView.java +++ b/dotCMS/src/main/java/com/dotcms/rest/api/v1/experiments/ResponseEntityExperimentView.java @@ -3,9 +3,10 @@ import com.dotcms.experiments.model.Experiment; import com.dotcms.rest.ResponseEntityView; import java.util.List; +import java.util.stream.Collectors; -public class ResponseEntityExperimentView extends ResponseEntityView> { +public class ResponseEntityExperimentView extends ResponseEntityView> { public ResponseEntityExperimentView(final List entity) { - super(entity); + super(entity.stream().map(ExperimentView::of).collect(Collectors.toList())); } } diff --git a/dotCMS/src/main/java/com/dotcms/rest/api/v1/experiments/ResponseEntitySingleExperimentView.java b/dotCMS/src/main/java/com/dotcms/rest/api/v1/experiments/ResponseEntitySingleExperimentView.java index 6129f74eeba..5ac3fdeb60f 100644 --- a/dotCMS/src/main/java/com/dotcms/rest/api/v1/experiments/ResponseEntitySingleExperimentView.java +++ b/dotCMS/src/main/java/com/dotcms/rest/api/v1/experiments/ResponseEntitySingleExperimentView.java @@ -2,10 +2,9 @@ import com.dotcms.experiments.model.Experiment; import com.dotcms.rest.ResponseEntityView; -import java.util.List; -public class ResponseEntitySingleExperimentView extends ResponseEntityView { +public class ResponseEntitySingleExperimentView extends ResponseEntityView { public ResponseEntitySingleExperimentView(final Experiment entity) { - super(entity); + super(ExperimentView.of(entity)); } } diff --git a/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml b/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml index 5af2553d01a..21690581e53 100644 --- a/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml +++ b/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml @@ -28283,13 +28283,6 @@ components: properties: createdBy: type: string - createdByUserName: - type: string - description: "Display name of the user who created the experiment. Reports\ - \ \"System\" for the system user and \"unknown\" when the user cannot\ - \ be resolved or has no name set, so the value is never empty." - example: Admin User - readOnly: true creationDate: type: string format: date-time @@ -28387,6 +28380,59 @@ components: properties: description: type: string + ExperimentView: + type: object + properties: + createdBy: + type: string + createdByUserName: + type: string + description: "Display name of the user who created the experiment. Reports\ + \ \"System\" for the system user and \"unknown\" when the user cannot\ + \ be resolved or has no name set, so the value is never empty." + example: Admin User + creationDate: + type: string + format: date-time + description: + type: string + goals: + $ref: "#/components/schemas/Goals" + id: + type: string + lastModifiedBy: + type: string + lookBackWindowExpireTime: + type: integer + format: int64 + modDate: + type: string + format: date-time + name: + type: string + pageId: + type: string + runningIds: + $ref: "#/components/schemas/RunningIds" + scheduling: + $ref: "#/components/schemas/Scheduling" + status: + type: string + enum: + - RUNNING + - SCHEDULED + - ENDED + - DRAFT + - ARCHIVED + targetingConditions: + type: array + items: + $ref: "#/components/schemas/TargetingCondition" + trafficAllocation: + type: number + format: float + trafficProportion: + $ref: "#/components/schemas/TrafficProportion" ExportSecretForm: type: object properties: @@ -33910,7 +33956,7 @@ components: entity: type: array items: - $ref: "#/components/schemas/Experiment" + $ref: "#/components/schemas/ExperimentView" errors: type: array items: @@ -35510,7 +35556,7 @@ components: type: object properties: entity: - $ref: "#/components/schemas/Experiment" + $ref: "#/components/schemas/ExperimentView" errors: type: array items: diff --git a/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java b/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java deleted file mode 100644 index 12135ec25e2..00000000000 --- a/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameTest.java +++ /dev/null @@ -1,338 +0,0 @@ -package com.dotcms.experiments.model; - -import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNotEquals; -import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.mockStatic; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoInteractions; -import static org.mockito.Mockito.when; - -import com.dotcms.publishing.BundlerUtil; -import com.dotcms.rest.api.v1.DotObjectMapperProvider; -import com.dotmarketing.business.APILocator; -import com.dotmarketing.business.UserAPI; -import com.fasterxml.jackson.databind.JsonNode; -import com.fasterxml.jackson.databind.ObjectMapper; -import com.liferay.portal.model.User; -import java.io.ByteArrayOutputStream; -import java.nio.charset.StandardCharsets; -import org.junit.jupiter.api.Test; -import org.mockito.MockedStatic; - -/** - * Serialization tests for the {@code createdByUserName} field added to the Experiment payload by - * #37304. - * - *

Why the round-trip test carries the most weight. The Experiment model is an Immutables - * value type, and its generated {@code Experiment.Json} delegate holds settable attributes - * only — it has no field for a computed one, and it is not annotated - * {@code @JsonIgnoreProperties(ignoreUnknown = true)}. The REST mapper leaves - * {@code FAIL_ON_UNKNOWN_PROPERTIES} at Jackson's default, which is enabled. So a property - * that serializes but cannot be bound is an unknown property on the way back in, and adding one - * introduces an asymmetry the current payload does not have: today every computed member is - * {@code @JsonIgnore}d, so an Experiment round-trips cleanly. - * - *

{@code @JsonProperty(access = READ_ONLY)} is what makes the property known but not - * bound, and {@link #payloadCarryingTheField_stillDeserializes()} is what established that it - * holds when Jackson binds into the generated delegate rather than into the interface. Should that - * ever stop being true, the answer is a {@code @JsonAppend} virtual property — not disabling - * {@code FAIL_ON_UNKNOWN_PROPERTIES} on a shared mapper, which would weaken every other contract - * that mapper serves. - * - *

These tests deliberately use {@link DotObjectMapperProvider#createDefaultMapper()} rather than - * a bare {@code ObjectMapper}: the question is what the REST layer actually does, not what Jackson - * does in general. - */ -class ExperimentCreatorNameTest { - - private static final String CREATOR_ID = "dotcms.org.1"; - private static final String CREATOR_NAME = "Admin User"; - - /** - * Deliberately different from {@link #CREATOR_ID}. If both ids were the same, every test here - * would still pass with the accessor reading {@code lastModifiedBy()} instead of - * {@code createdBy()} — the one mapping this feature exists to get right would be unasserted. - */ - private static final String MODIFIER_ID = "dotcms.org.2"; - private static final String MODIFIER_NAME = "Other Person"; - - /** - * {@code lookBackWindowExpireTime} is set explicitly on purpose. Leaving it unset makes - * Immutables evaluate its {@code @Value.Default}, which calls - * {@code ConfigExperimentUtil.lookBackWindowDefaultExpireTime()} and through it - * {@code APILocator.getExperimentsAPI()} — dragging in the CDI/OpenSearch wiring that no plain - * unit-test JVM has, and which is mocked to a stub here anyway. Setting the value keeps the - * test failing (and passing) for reasons that belong to #37304. - */ - private static Experiment anExperiment() { - return Experiment.builder() - .name("Homepage CTA test") - .pageId("2d8b8b1e-0000-0000-0000-000000000001") - .createdBy(CREATOR_ID) - .lastModifiedBy(MODIFIER_ID) - .id("0e8b8b1e-0000-0000-0000-000000000002") - .lookBackWindowExpireTime(1_800_000L) - .build(); - } - - private static User adminUser() { - return userNamed("Admin", "User"); - } - - /** - * Runs the body with {@code APILocator.getUserAPI()} answering a stub that resolves - * {@link #CREATOR_ID} and {@link #MODIFIER_ID} to different users, so an accessor - * reading the wrong field is visible rather than silently equivalent. - */ - private static void withResolvableCreator(final ThrowingRunnable body) throws Exception { - final UserAPI userAPI = mock(UserAPI.class); - when(userAPI.loadUserById(CREATOR_ID)).thenReturn(adminUser()); - when(userAPI.loadUserById(MODIFIER_ID)).thenReturn(userNamed("Other", "Person")); - - try (MockedStatic apiLocator = mockStatic(APILocator.class)) { - apiLocator.when(APILocator::getUserAPI).thenReturn(userAPI); - body.run(); - } - } - - @FunctionalInterface - private interface ThrowingRunnable { - void run() throws Exception; - } - - /** - * Method to test: {@code AbstractExperiment.createdByUserName()} - * Given Scenario: An Experiment whose creator resolves to a named user, serialized with the - * REST mapper. - * ExpectedResult: The payload carries {@code createdByUserName} with the creator's full name - * (FR-001, FR-002). - */ - @Test - void serializedExperiment_carriesCreatorName() throws Exception { - withResolvableCreator(() -> { - final ObjectMapper mapper = DotObjectMapperProvider.createDefaultMapper(); - - final JsonNode payload = mapper.readTree(mapper.writeValueAsString(anExperiment())); - - assertTrue(payload.has("createdByUserName"), - "The payload should carry createdByUserName"); - assertEquals(CREATOR_NAME, payload.get("createdByUserName").asText()); - assertNotEquals(MODIFIER_NAME, payload.get("createdByUserName").asText(), - "The name must come from createdBy, never from lastModifiedBy"); - }); - } - - /** - * Method to test: {@code AbstractExperiment.createdByUserName()} - * Given Scenario: The same serialized payload fed straight back to the same mapper. - * ExpectedResult: It deserializes into an Experiment without raising. This is the gating test - * described in the class Javadoc: a serialize-only property must not make a - * round-tripped payload unreadable (FR-023). - */ - @Test - void payloadCarryingTheField_stillDeserializes() throws Exception { - withResolvableCreator(() -> { - final ObjectMapper mapper = DotObjectMapperProvider.createDefaultMapper(); - final String json = mapper.writeValueAsString(anExperiment()); - - assertTrue(json.contains("createdByUserName"), - "Precondition: the payload under test must actually carry the new field"); - - final Experiment parsed = assertDoesNotThrow( - () -> mapper.readValue(json, Experiment.class), - "A payload carrying createdByUserName must still deserialize"); - - assertEquals(CREATOR_ID, parsed.createdBy(), - "The bound createdBy should survive the round trip unchanged"); - }); - } - - /** - * Method to test: {@code AbstractExperiment.createdByUserName()} - * Given Scenario: An Experiment whose creator cannot be resolved, serialized with the REST - * mapper. - * ExpectedResult: The field is present and carries {@code "unknown"} — the same label the - * Content Drive folder view uses — so the column is never blank even at the - * serialization layer (FR-003, FR-009). - */ - @Test - void serializedExperiment_withUnresolvableCreator_carriesUnknown() throws Exception { - final UserAPI userAPI = mock(UserAPI.class); - when(userAPI.loadUserById(anyString())) - .thenThrow(new com.dotmarketing.business.NoSuchUserException("gone")); - - try (MockedStatic apiLocator = mockStatic(APILocator.class)) { - apiLocator.when(APILocator::getUserAPI).thenReturn(userAPI); - - final ObjectMapper mapper = DotObjectMapperProvider.createDefaultMapper(); - final JsonNode payload = mapper.readTree(mapper.writeValueAsString(anExperiment())); - - assertEquals("unknown", payload.get("createdByUserName").asText()); - } - } - - /** - * Method to test: {@code AbstractExperiment.createdBy()} / {@code getOwner()} - * Given Scenario: The same serialization, inspected for what must NOT have changed. - * ExpectedResult: {@code createdBy} keeps its key and its user-ID value, {@code getOwner()} - * still resolves from it, and the computed permission members stay out of the - * payload. This is the additive-change guarantee (FR-005, FR-006, FR-007). - */ - @Test - void addingTheField_leavesTheExistingContractAlone() throws Exception { - withResolvableCreator(() -> { - final Experiment experiment = anExperiment(); - final ObjectMapper mapper = DotObjectMapperProvider.createDefaultMapper(); - - final JsonNode payload = mapper.readTree(mapper.writeValueAsString(experiment)); - - assertEquals(CREATOR_ID, payload.get("createdBy").asText(), - "createdBy must still be the user ID"); - assertEquals(CREATOR_ID, experiment.getOwner(), - "getOwner() must still resolve from createdBy, not from the new field"); - assertEquals(MODIFIER_ID, payload.get("lastModifiedBy").asText(), - "lastModifiedBy is out of scope: still the raw id, and no name companion"); - assertFalse(payload.has("lastModifiedByUserName"), - "lastModifiedBy must not gain a name companion (FR-008)"); - - for (final String computed : new String[]{"owner", "identifier", "permissionId", - "manifestInfo", "parentPermissionable", "acceptedPermissions"}) { - assertFalse(payload.has(computed), - "The computed member '" + computed + "' must stay out of the payload"); - } - }); - } - - /** - * Method to test: {@code AbstractExperiment.createdByUserName()} - * Given Scenario: Experiments are built and handled without ever being serialized — constructed - * from the builder, rebuilt via {@code from(...)} the way - * {@code addTargetingConditions} does on the find path, read for their owner, - * and compared. - * ExpectedResult: The user layer is never consulted. Only serializing the Experiment resolves - * the name (FR-015). - * - *

This is the guard for the decision the whole feature rests on. The accessor is a plain - * {@code default} method precisely so it stays inert on the paths that never serialize — the - * database transformer behind every list row, the running-experiments cache fill on page - * render, and the push-publish dependency walk. Turning it into a {@code @Value.Derived} - * attribute would move the lookup into the constructor and break that silently: every other - * test in this class would still pass, because they all serialize. This one would not. - */ - @Test - void buildingAnExperimentWithoutSerializing_neverResolvesTheCreator() throws Exception { - final UserAPI userAPI = mock(UserAPI.class); - - try (MockedStatic apiLocator = mockStatic(APILocator.class)) { - apiLocator.when(APILocator::getUserAPI).thenReturn(userAPI); - - final Experiment experiment = anExperiment(); - final Experiment rebuilt = Experiment.builder().from(experiment).build(); - experiment.getOwner(); - experiment.equals(rebuilt); - experiment.hashCode(); - experiment.toString(); - - verifyNoInteractions(userAPI); - - DotObjectMapperProvider.createDefaultMapper().writeValueAsString(experiment); - - verify(userAPI).loadUserById(CREATOR_ID); - verify(userAPI, never()).loadUserById(MODIFIER_ID); - } - } - - /** - * Method to test: {@code AbstractExperiment.createdByUserName()} - * Given Scenario: The same Experiment instance serialized twice, with the creator renaming - * themselves in between. - * ExpectedResult: The second payload reports the new name (FR-016). - * - *

The guard against memoization. A {@code @Value.Lazy} attribute would compute the name once - * and keep it for the life of the instance — invisible on the REST path, where instances are - * short-lived, but not on the running-experiments list cache, whose entries are long-lived and - * shared. This test fails the moment the value starts being cached on the object. - */ - @Test - void renamingTheCreator_isReflectedOnTheNextSerialization() throws Exception { - final UserAPI userAPI = mock(UserAPI.class); - when(userAPI.loadUserById(CREATOR_ID)) - .thenReturn(adminUser(), userNamed("Renamed", "User")); - - try (MockedStatic apiLocator = mockStatic(APILocator.class)) { - apiLocator.when(APILocator::getUserAPI).thenReturn(userAPI); - - final Experiment experiment = anExperiment(); - final ObjectMapper mapper = DotObjectMapperProvider.createDefaultMapper(); - - final JsonNode before = mapper.readTree(mapper.writeValueAsString(experiment)); - final JsonNode after = mapper.readTree(mapper.writeValueAsString(experiment)); - - assertEquals(CREATOR_NAME, before.get("createdByUserName").asText()); - assertEquals("Renamed User", after.get("createdByUserName").asText(), - "The name must be resolved per serialization, never captured on the instance"); - } - } - - /** - * Method to test: {@code AbstractExperiment.createdByUserName()} as seen by the push-publish - * bundler. - * Given Scenario: An Experiment serialized with the mapper push-publish bundling and starter - * export use ({@link com.dotcms.publishing.BundlerUtil}), rather than the REST - * mapper. - * ExpectedResult: {@code createdByUserName} is absent, while {@code createdBy} is still - * there. - * - *

Why this matters enough to have its own test: the bundle is read back by - * {@code ExperimentHandler} through a bare {@code ObjectMapper}, which fails on unknown - * properties. A receiver on a build without this field would therefore reject the file — and - * {@code BundlePublisher} runs every handler in one transaction, so that rejection rolls back - * the whole bundle, not just the experiment. Keeping the field out of the bundled shape - * is what makes the bundle byte-compatible with older receivers. - */ - @Test - void bundledExperiment_doesNotCarryTheCreatorName() throws Exception { - withResolvableCreator(() -> { - final ByteArrayOutputStream out = new ByteArrayOutputStream(); - BundlerUtil.objectToJSON(anExperiment(), out); - final String bundled = out.toString(StandardCharsets.UTF_8); - - assertFalse(bundled.contains("createdByUserName"), - "The bundled shape must not carry createdByUserName: an older receiver would " - + "reject it as an unknown property and roll back the entire bundle"); - assertTrue(bundled.contains("createdBy"), - "Precondition: the bundled shape should still carry createdBy"); - }); - } - - /** - * Method to test: {@code AbstractExperiment.createdByUserName()} as seen by the REST layer. - * Given Scenario: The same Experiment, serialized with the REST mapper. - * ExpectedResult: The field IS present — excluding it from bundles must not remove it from the - * API response, which is the entire point of the feature. - */ - @Test - void excludingItFromBundles_leavesTheApiResponseIntact() throws Exception { - withResolvableCreator(() -> { - final ObjectMapper mapper = DotObjectMapperProvider.createDefaultMapper(); - - final JsonNode payload = mapper.readTree(mapper.writeValueAsString(anExperiment())); - - assertEquals(CREATOR_NAME, payload.get("createdByUserName").asText()); - }); - } - - private static User userNamed(final String first, final String last) { - final User user = new User(); - user.setFirstName(first); - user.setMiddleName(""); - user.setLastName(last); - return user; - } -} diff --git a/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameResolverTest.java b/dotCMS/src/test/java/com/dotcms/rest/api/v1/experiments/ExperimentCreatorNameResolverTest.java similarity index 99% rename from dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameResolverTest.java rename to dotCMS/src/test/java/com/dotcms/rest/api/v1/experiments/ExperimentCreatorNameResolverTest.java index a919918a1ff..e2a4a40f32d 100644 --- a/dotCMS/src/test/java/com/dotcms/experiments/model/ExperimentCreatorNameResolverTest.java +++ b/dotCMS/src/test/java/com/dotcms/rest/api/v1/experiments/ExperimentCreatorNameResolverTest.java @@ -1,4 +1,4 @@ -package com.dotcms.experiments.model; +package com.dotcms.rest.api.v1.experiments; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.mockito.ArgumentMatchers.anyString; diff --git a/dotCMS/src/test/java/com/dotcms/rest/api/v1/experiments/ExperimentViewTest.java b/dotCMS/src/test/java/com/dotcms/rest/api/v1/experiments/ExperimentViewTest.java new file mode 100644 index 00000000000..34b4a36fad1 --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/rest/api/v1/experiments/ExperimentViewTest.java @@ -0,0 +1,260 @@ +package com.dotcms.rest.api.v1.experiments; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +import com.dotcms.experiments.model.Experiment; +import com.dotcms.publishing.BundlerUtil; +import com.dotcms.rest.api.v1.DotObjectMapperProvider; +import com.dotmarketing.business.APILocator; +import com.dotmarketing.business.UserAPI; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.liferay.portal.model.User; +import java.io.ByteArrayOutputStream; +import java.nio.charset.StandardCharsets; +import org.junit.jupiter.api.Test; +import org.mockito.MockedStatic; + +/** + * Serialization tests for {@link ExperimentView}, the REST-layer wrapper that publishes + * {@code createdByUserName} beside an Experiment (#37304). + * + *

Why the field lives here and not on the model. An earlier revision of this work put the + * accessor on {@code AbstractExperiment} itself. That made every serialization of an Experiment + * resolve a user — including push-publish bundling and starter export, which are not REST responses + * at all, and which are read back by a receiver that rejects unknown properties and rolls back the + * whole bundle. Keeping the derivation in the view means the model stays a plain data object and + * only the endpoint response is affected, which is what review asked for. + * + *

The wire shape must not change as a result: {@code createdByUserName} is a sibling of the + * Experiment's own fields, not a nested object. {@link #view_flattensTheExperimentAlongsideTheName()} + * is what pins that. + */ +class ExperimentViewTest { + + private static final String CREATOR_ID = "dotcms.org.1"; + private static final String CREATOR_NAME = "Admin User"; + private static final String MODIFIER_ID = "dotcms.org.2"; + + private static Experiment anExperiment() { + return Experiment.builder() + .name("Homepage CTA test") + .pageId("2d8b8b1e-0000-0000-0000-000000000001") + .createdBy(CREATOR_ID) + .lastModifiedBy(MODIFIER_ID) + .id("0e8b8b1e-0000-0000-0000-000000000002") + .lookBackWindowExpireTime(1_800_000L) + .build(); + } + + private static User userNamed(final String first, final String last) { + final User user = new User(); + user.setFirstName(first); + user.setMiddleName(""); + user.setLastName(last); + return user; + } + + private static void withResolvableCreator(final ThrowingRunnable body) throws Exception { + final UserAPI userAPI = mock(UserAPI.class); + when(userAPI.loadUserById(CREATOR_ID)).thenReturn(userNamed("Admin", "User")); + when(userAPI.loadUserById(MODIFIER_ID)).thenReturn(userNamed("Other", "Person")); + + try (MockedStatic apiLocator = mockStatic(APILocator.class)) { + apiLocator.when(APILocator::getUserAPI).thenReturn(userAPI); + body.run(); + } + } + + @FunctionalInterface + private interface ThrowingRunnable { + void run() throws Exception; + } + + /** + * Method to test: {@link ExperimentView} + * Given Scenario: A view wrapping an Experiment whose creator resolves to a named user. + * ExpectedResult: The payload carries the Experiment's own fields at the top level AND + * {@code createdByUserName} beside them — the same shape the model-level + * revision produced, so the API contract does not move. + */ + @Test + void view_flattensTheExperimentAlongsideTheName() throws Exception { + withResolvableCreator(() -> { + final ObjectMapper mapper = DotObjectMapperProvider.createDefaultMapper(); + + final JsonNode payload = + mapper.readTree(mapper.writeValueAsString(ExperimentView.of(anExperiment()))); + + assertEquals(CREATOR_NAME, payload.get("createdByUserName").asText()); + assertEquals(CREATOR_ID, payload.get("createdBy").asText(), + "createdBy must still be a top-level field, not nested under the experiment"); + assertEquals("Homepage CTA test", payload.get("name").asText(), + "The Experiment's own fields must stay at the top level"); + assertFalse(payload.has("experiment"), + "The Experiment must be unwrapped, not nested under a property"); + }); + } + + /** + * Method to test: {@link ExperimentView} + * Given Scenario: The same Experiment serialized WITHOUT the view, as the push-publish bundler + * and starter export do. + * ExpectedResult: No {@code createdByUserName}, and the user layer is never consulted. This is + * the guarantee that moving the derivation to the REST layer buys, and it needs + * no mix-in in BundlerUtil to hold. + */ + @Test + void bareExperiment_carriesNoNameAndCostsNoLookup() throws Exception { + final UserAPI userAPI = mock(UserAPI.class); + + try (MockedStatic apiLocator = mockStatic(APILocator.class)) { + apiLocator.when(APILocator::getUserAPI).thenReturn(userAPI); + + final ByteArrayOutputStream out = new ByteArrayOutputStream(); + BundlerUtil.objectToJSON(anExperiment(), out); + final String bundled = out.toString(StandardCharsets.UTF_8); + + assertFalse(bundled.contains("createdByUserName"), + "A bare Experiment must not carry the derived name"); + assertTrue(bundled.contains("createdBy"), + "Precondition: the bundled shape should still carry createdBy"); + verifyNoInteractions(userAPI); + } + } + + /** + * Method to test: {@link ExperimentView} + * Given Scenario: The view is built but never serialized. + * ExpectedResult: No user lookup. The name is resolved when the response is written, so the + * value cannot go stale and building a view costs nothing. + */ + @Test + void buildingTheViewWithoutSerializing_costsNoLookup() throws Exception { + final UserAPI userAPI = mock(UserAPI.class); + + try (MockedStatic apiLocator = mockStatic(APILocator.class)) { + apiLocator.when(APILocator::getUserAPI).thenReturn(userAPI); + + ExperimentView.of(anExperiment()); + + verifyNoInteractions(userAPI); + } + } + + /** + * Method to test: {@link ExperimentView} + * Given Scenario: An Experiment whose creator cannot be resolved. + * ExpectedResult: {@code "unknown"} — the column is never blank, and the response still + * serializes (FR-003, FR-009). + */ + @Test + void view_withUnresolvableCreator_reportsUnknown() throws Exception { + final UserAPI userAPI = mock(UserAPI.class); + when(userAPI.loadUserById(anyString())) + .thenThrow(new com.dotmarketing.business.NoSuchUserException("gone")); + + try (MockedStatic apiLocator = mockStatic(APILocator.class)) { + apiLocator.when(APILocator::getUserAPI).thenReturn(userAPI); + + final ObjectMapper mapper = DotObjectMapperProvider.createDefaultMapper(); + final JsonNode payload = + mapper.readTree(mapper.writeValueAsString(ExperimentView.of(anExperiment()))); + + assertEquals("unknown", payload.get("createdByUserName").asText()); + } + } + + /** + * Method to test: {@link ExperimentView} + * Given Scenario: The same view serialized twice, with the creator renaming in between. + * ExpectedResult: The second payload reports the new name. The value is resolved per + * serialization, so it cannot go stale (FR-016). + */ + @Test + void renamingTheCreator_isReflectedOnTheNextSerialization() throws Exception { + final UserAPI userAPI = mock(UserAPI.class); + when(userAPI.loadUserById(CREATOR_ID)) + .thenReturn(userNamed("Admin", "User"), userNamed("Renamed", "User")); + + try (MockedStatic apiLocator = mockStatic(APILocator.class)) { + apiLocator.when(APILocator::getUserAPI).thenReturn(userAPI); + + final ObjectMapper mapper = DotObjectMapperProvider.createDefaultMapper(); + final ExperimentView view = ExperimentView.of(anExperiment()); + + final JsonNode before = mapper.readTree(mapper.writeValueAsString(view)); + final JsonNode after = mapper.readTree(mapper.writeValueAsString(view)); + + assertEquals(CREATOR_NAME, before.get("createdByUserName").asText()); + assertEquals("Renamed User", after.get("createdByUserName").asText(), + "The name must be resolved per serialization, never captured on the view"); + } + } + + /** + * Method to test: the Experiment model itself. + * Given Scenario: A bare Experiment round-tripped through the REST mapper. + * ExpectedResult: It parses back cleanly and keeps createdBy, getOwner() and lastModifiedBy. + * This is the guard that the model stayed a plain data object: with the derived + * field on the model, a serialize-only property made the payload unreadable by + * any strict reader, which is the whole reason the field moved to this view. + */ + @Test + void bareExperiment_stillRoundTripsAndKeepsItsContract() throws Exception { + // The APILocator mock is scaffolding, not subject: AbstractExperiment's pre-existing + // @Value.Derived getParentPermissionable() calls the ContentletAPI at build time, so an + // Experiment cannot be constructed in a bare unit-test JVM. That it fires on build is + // incidentally the very behaviour this feature avoided by not being a derived attribute. + final UserAPI userAPI = mock(UserAPI.class); + + try (MockedStatic apiLocator = mockStatic(APILocator.class)) { + apiLocator.when(APILocator::getUserAPI).thenReturn(userAPI); + + final ObjectMapper mapper = DotObjectMapperProvider.createDefaultMapper(); + final Experiment experiment = anExperiment(); + + final String json = mapper.writeValueAsString(experiment); + final Experiment parsed = mapper.readValue(json, Experiment.class); + + assertFalse(json.contains("createdByUserName"), + "The model must not carry the derived field"); + assertEquals(CREATOR_ID, parsed.createdBy()); + assertEquals(CREATOR_ID, parsed.getOwner(), + "getOwner() must still resolve from createdBy"); + assertEquals(MODIFIER_ID, parsed.lastModifiedBy()); + verifyNoInteractions(userAPI); + } + } + + /** + * Method to test: {@link ExperimentView} + * Given Scenario: An Experiment whose creator resolves, serialized through the view. + * ExpectedResult: The name comes from {@code createdBy} and never from {@code lastModifiedBy}, + * and the modifier is never looked up. + */ + @Test + void view_resolvesTheCreatorAndNotTheModifier() throws Exception { + final UserAPI userAPI = mock(UserAPI.class); + when(userAPI.loadUserById(anyString())).thenReturn(userNamed("Admin", "User")); + + try (MockedStatic apiLocator = mockStatic(APILocator.class)) { + apiLocator.when(APILocator::getUserAPI).thenReturn(userAPI); + + DotObjectMapperProvider.createDefaultMapper() + .writeValueAsString(ExperimentView.of(anExperiment())); + + verify(userAPI).loadUserById(CREATOR_ID); + verify(userAPI, never()).loadUserById(MODIFIER_ID); + } + } +} diff --git a/specs/37304-experiment-created-by-username/contracts/experiment-created-by-username.md b/specs/37304-experiment-created-by-username/contracts/experiment-created-by-username.md index 709ab2fb395..d260e614d4f 100644 --- a/specs/37304-experiment-created-by-username/contracts/experiment-created-by-username.md +++ b/specs/37304-experiment-created-by-username/contracts/experiment-created-by-username.md @@ -73,29 +73,27 @@ Drive folder view, so the same orphaned owner reads the same in both listings. The field is never `null`, never absent and never `""`. -## Bundles and starter exports (excluded) +## Where the field is produced -The field is a REST-response concern only. `BundlerUtil` registers a mix-in that ignores it, so it -does **not** appear in push-publish bundles or starter exports, and those paths perform no user -lookup. +`ExperimentView` (`com.dotcms.rest.api.v1.experiments`) wraps the Experiment for a response and adds +the field; the Experiment is `@JsonUnwrapped`, so the payload stays a flat object and the wire shape +is unchanged except for the addition. `ResponseEntitySingleExperimentView` and +`ResponseEntityExperimentView` do the wrapping, so none of the 14 endpoints changed. -The reason is compatibility, not tidiness: bundles are read back through a bare `ObjectMapper` that -fails on unknown properties, and `BundlePublisher` runs every handler in a single transaction — so a -receiver on a build without the field would reject the experiment file and roll back the **entire** -bundle. `createdBy` is bundled as before, and the receiver resolves the name itself when it serves -the experiment over REST. +The `Experiment` schema in `openapi.yaml` is therefore untouched, and a new `ExperimentView` schema +carries the flattened fields plus `createdByUserName`. Both response wrappers now `$ref` the view. + +Because the model is a plain data object again, the field does **not** reach push-publish bundles or +starter exports, and those paths pay no user lookup — no mix-in or other mitigation is needed. ## Deserialization contract (FR-023) `createdByUserName` is **read-only**: serialized on the way out, ignored on the way in. A payload containing it must still deserialize into an `Experiment` without error. -This is not free. The generated `Experiment.Json` delegate carries settable attributes only and is -**not** annotated `@JsonIgnoreProperties(ignoreUnknown = true)`, and the REST mapper -(`DotObjectMapperProvider.createDefaultMapper()`) leaves `FAIL_ON_UNKNOWN_PROPERTIES` at Jackson's -default — enabled. `@JsonProperty(access = READ_ONLY)` is the mechanism that makes the property -*known but not bound*; the round-trip test is what proves it, and `@JsonAppend` is the pre-approved -fallback if it does not hold. See [../research.md](../research.md) R1. +With the field on the view rather than the model, this is now free: the Experiment payload has no +serialize-only property, so it round-trips exactly as it did before this change. `ExperimentView` is +a response type and is never deserialized. The guard is `bareExperiment_stillRoundTripsAndKeepsItsContract`. ## Verification diff --git a/specs/37304-experiment-created-by-username/spec.md b/specs/37304-experiment-created-by-username/spec.md index bf13eda501b..cb6df07d6c2 100644 --- a/specs/37304-experiment-created-by-username/spec.md +++ b/specs/37304-experiment-created-by-username/spec.md @@ -218,11 +218,9 @@ both the list and the single fetch, and confirm the response succeeds and report - **FR-023**: An Experiment payload that contains `createdByUserName` MUST still be readable wherever Experiment JSON is parsed back into an Experiment. The added field must not make a round-tripped payload fail to parse. -- **FR-023a**: `createdByUserName` MUST NOT appear in push-publish bundles or starter exports. Those - artifacts are read back by a receiver that may be running a build without the field, and that - receiver fails the entire bundle rather than the single experiment. The field is derived from - `createdBy`, which is bundled, so the receiver can resolve the name itself when it serves the - experiment over REST — nothing is lost by excluding it. +- **FR-023a**: `createdByUserName` MUST NOT appear on the `Experiment` model itself, and therefore + MUST NOT reach push-publish bundles or starter exports. It is published by the REST view only, so + the endpoint response carries it and nothing else does. ### Endpoints in scope @@ -322,18 +320,17 @@ Endpoints under `/v1/experiments` that do **not** carry an Experiment, and are t That method joins the parts with spaces and never trims, so it returns a single space when every part is blank, and leaves padding like "Admin " when only one part is set, which is exactly the case FR-010 covers. -- **A5 — The database is not a round-trip, but push publishing is.** Experiments are rebuilt from - database columns by a transformer, not by parsing stored Experiment JSON, so persistence itself - cannot break. Push publishing is a different story and an earlier draft of this spec had it wrong: - `ExperimentBundler` and `ExperimentHandler` are live, they simply live under - `dotCMS/src/enterprise/java` rather than `src/main/java`. The bundler serializes the whole - Experiment through `BundlerUtil`, and the handler reads it back through a bare `ObjectMapper` that - leaves `FAIL_ON_UNKNOWN_PROPERTIES` at Jackson's default. `ExperimentHandler` catches around its - entire loop and rethrows, and `BundlePublisher` runs every handler inside one transaction — so a - receiver on a build without the field would reject the experiment file and roll back the **whole** - bundle, including the pages and contentlets that shipped with it. Nothing in the push-publish path - checks that sender and receiver run the same version. Starter export (`ExportStarterUtil`) - serializes Experiments the same way. FR-023a is the consequence. +- **A5 — The name is derived in the REST layer, not on the model.** An earlier revision put the + accessor on `AbstractExperiment` itself. That made every serialization of an Experiment resolve a + user, and serialization is not only REST: `ExperimentBundler` and `ExperimentHandler` are live — + they simply live under `dotCMS/src/enterprise/java` rather than `src/main/java`, which is why an + earlier draft of this spec wrongly called them absent. The bundler serializes the whole Experiment + and the handler reads it back through a mapper that rejects unknown properties, while + `BundlePublisher` runs every handler in one transaction — so a receiver on a build without the + field would have rolled back the **entire** bundle. Starter export (`ExportStarterUtil`) + serializes Experiments the same way. Review (#37510) asked for the derivation to move to the REST + layer, and it did: `ExperimentView` carries it, the model is a plain data object again, and the + problem is removed at the source rather than mitigated. - **A6 — The consumer needs one display string.** #37307 renders a single Created By column, so a single pre-joined name is sufficient; separate first/last fields are not required. From 45bddc474eff8fbdd2fb4cfa7ffb74dd005a498b Mon Sep 17 00:00:00 2001 From: Arcadio Quintero Date: Thu, 17 Sep 2026 10:18:10 -0400 Subject: [PATCH 10/10] fix(experiments): unwrap the view in the experiments resource IT (#37304) aec284cc13 changed ResponseEntitySingleExperimentView to carry an ExperimentView instead of an Experiment, which broke the compilation of the pre-existing ExperimentsResourceIntegrationTest: its patchPageId helper calls the resource directly and returns .getEntity() typed as Experiment. The helper now unwraps with .getExperiment(). No behaviour changes, and no other usage exists - ExperimentView and both response wrappers are referenced nowhere else in dotcms-integration or dotcms-postman. Worth recording why this reached CI. Every local verification in this branch ran as -pl :dotcms-core --am, which never compiles dotcms-integration, so a green 19/19 said nothing about the module that actually broke. Reproducing the CI build verbatim (clean install -Dprod=true -DskipTests=true) is what found it, and that now passes: 24 modules, 0 errors. --- .../api/v1/experiments/ExperimentsResourceIntegrationTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/experiments/ExperimentsResourceIntegrationTest.java b/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/experiments/ExperimentsResourceIntegrationTest.java index ffc4f014381..98ea4e1113a 100644 --- a/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/experiments/ExperimentsResourceIntegrationTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/experiments/ExperimentsResourceIntegrationTest.java @@ -261,7 +261,8 @@ private Experiment patchPageId(final Experiment experiment, final String pageId) .build(); return resource.update(getHttpRequest(), response, experiment.id().orElseThrow(), form) - .getEntity(); + .getEntity() + .getExperiment(); } /**