feat(experiments): expose the experiment creator's display name in the Experiments API (#37304) - #37510
Open
oidacra wants to merge 9 commits into
Open
feat(experiments): expose the experiment creator's display name in the Experiments API (#37304)#37510oidacra wants to merge 9 commits into
oidacra wants to merge 9 commits into
Conversation
14 tasks
Contributor
|
Claude finished @oidacra's task in 3m 10s —— View job Review — REST-layer refactor (commit
|
github-actions
Bot
added
Area : Backend
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.
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.
…ts 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.
…ests 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 353736e settled.
#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.
oidacra
force-pushed
the
issue-37304-experiment-created-by-username
branch
from
September 16, 2026 19:45
feaaa14 to
4785fac
Compare
oidacra
changed the title
oidacra
requested review from
adrianjm-dotCMS,
erickgonzalez,
freddyDOTCMS,
jcastro-dotcms and
nicobytes
September 16, 2026 21:01
…#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.
Contributor
|
Pull Request Unsafe to Rollback!!!
Note: the REST API exposure of this field is additive and safe on its own (guarded by |
…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.
…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 7e10bbf 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 7e10bbf; this brings the spec in line with the code and with the fact.
freddyDOTCMS
requested changes
Sep 16, 2026
| @@ -0,0 +1,190 @@ | |||
| package com.dotcms.experiments.model; | |||
Member
There was a problem hiding this comment.
You must register this class in the MainSuite
| @@ -0,0 +1,338 @@ | |||
| package com.dotcms.experiments.model; | |||
|
|
|||
| import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; | |||
Member
There was a problem hiding this comment.
You must register this class in the MainSuite
| example = "Admin User") | ||
| default String createdByUserName() { | ||
| return ExperimentCreatorNameResolver.INSTANCE.resolve(createdBy()); | ||
| } |
Member
There was a problem hiding this comment.
This class is more like a Data model, so it should be a POJO, I think this set should go out of this class, maybe we should move this logic to the Rest Layer, so we affect just the responde of the endpoint
Member
There was a problem hiding this comment.
Also we are going to hit the UserAPI anytime this method is called
|
|
||
| @JsonIgnore | ||
| abstract String createdByUserName(); | ||
| } |
Member
There was a problem hiding this comment.
If we move the logic to the Rest layer we don't need this change here
… 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
createdByUserNamebeside the existingcreatedBy, 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 Experiments portlet listing's Created By column), which is blocked on this field.Closes #37304. Visual review page (summarizes; the spec decides): https://claude.ai/code/artifact/fdc2cd0a-bf41-4a1f-9b77-3c3ae9e47e85
What changes
The field is declared once on the model, so all 14 endpoints that return an Experiment carry it without being edited —
ExperimentsResourceand both response wrappers are not in the change set.createdBykeeps its key, its value and its role as the permission owner behindgetOwner(), andlastModifiedByis deliberately untouched.The value is never null, absent or empty:
createdByUserNameSystem, short-circuited before any lookupunknownThose two labels are not invented here — they are what
BrowserAPIImpl.ownerNamealready publishes for the Content Drive folder view, which answers this exact 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. (Content Drive reaches the user throughUserLocalManagerUtiland so bypassesUserCache; this resolver deliberately goes throughAPILocator.getUserAPI(), because the cost argument below rests on that cache.)Why a plain
defaultmethod and not an Immutables attributeThis is the decision the whole change rests on, and it was settled by reading the generated
Experimentclass rather than by argument:@Value.Derivedis assigned in the constructor. It would resolve a user on every Experiment ever built — the database transformer behind every list row, thewithTargetingConditionsrebuild insidefind(), the running-experiments cache fill on the page-render path, and the push-publish dependency walk. None of those serialize the object.@Value.Lazymemoizes per instance. The running-experiments list cache holds long-lived shared entries, so a memoized name would outlive a rename.A plain
defaultmethod is not an Immutables attribute at all: no field, no builder entry, noequals/hashCode/toStringparticipation, nobuild()cost. It is evaluated only when something serializes the Experiment.@JsonProperty(access = READ_ONLY)is load-bearing rather than decorative. The generatedExperiment.Jsondelegate binds settable attributes only, carries no@JsonIgnoreProperties(ignoreUnknown = true), and the REST mapper leavesFAIL_ON_UNKNOWN_PROPERTIESat Jackson's default — so a serialize-only field would otherwise make a round-tripped payload unreadable. A test proves it holds when Jackson binds into the delegate; that result is what retired the@JsonAppendfallback the plan had pre-approved.Tests
13 unit tests, written and confirmed failing before any implementation existed.
Two of them guard the invariants above, and their value was verified by mutation, not asserted: temporarily annotating the accessor with
@Value.Derivedfails exactly those two and leaves the other eleven green. Eleven tests that all serialize cannot see that regression.buildingAnExperimentWithoutSerializing_neverResolvesTheCreator— builds, rebuilds, reads the owner, compares and stringifies an Experiment with zero 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.Integration coverage in
ExperimentsResourceIntegrationTest(already registered inMainSuite1a) and the Postman contract assertion are still to come.Corrections to the issue body
The issue's endpoint list is close but not exact; the spec carries a 14-endpoint table that supersedes it. None of this changes the approach.
DELETE /v1/experiments/{experimentId}is listed among the endpoints carrying an Experiment; it returns the confirmation string"Experiment deleted".DELETE /v1/experiments/{experimentId}/goals/primaryandDELETE /v1/experiments/{experimentId}/targetingConditions/{id}.PATCH, notPUT.Checklist
createdByandgetOwner()are untouched, so permission behaviour is unchanged. The field exposes a display name only to callers already authorized to read the experiment, and nothing thrown by the lookup escapes into the response.