Skip to content

docs(experiments): spec for the experiment creator's name in the API (#37304) - #37510

Open
oidacra wants to merge 1 commit into
mainfrom
issue-37304-experiment-created-by-username
Open

docs(experiments): spec for the experiment creator's name in the API (#37304)#37510
oidacra wants to merge 1 commit into
mainfrom
issue-37304-experiment-created-by-username

Conversation

@oidacra

@oidacra oidacra commented Sep 11, 2026

Copy link
Copy Markdown
Member

Spec-first PR for #37304, using the single-PR flow: the specification lands as the initial commit and the implementation will land in this same PR.

Closes #37304. Visual review page (summarizes; the spec decides): https://claude.ai/code/artifact/fdc2cd0a-bf41-4a1f-9b77-3c3ae9e47e85

What the spec defines

Add an additive createdByUserName to the Experiment serialization, so every response carrying an Experiment reports the creator's display name instead of only an opaque user id. createdBy keeps its key, its value and its role as the permission owner behind getOwner(), so nothing that consumes the current payload changes. Consumer: #37307 (the portlet listing's Created By column).

  • The field is added once at the Experiment level, so all 14 endpoints that return an Experiment carry it: list, single fetch, create, PATCH, _archive, _start, _end, scheduled/{id}/_cancel, the four variant operations, and the two DELETE endpoints for the primary goal and a targeting condition.
  • Never null, never empty. An unresolvable creator (deleted user, orphaned reference) falls back to the raw id — and so does a resolvable user whose name parts are all blank, since User.getFullName() returns "" there. A lookup failure never turns a successful experiment read into a failed request.
  • Resolved at read time, never stored: no new column, no migration, so a creator who later renames themselves is reported under the new name.
  • Documented on the model accessor and regenerated into openapi.yaml. A full @Operation/@ApiResponse pass over ExperimentsResource (which today carries only @Tag) is explicitly out of scope.
  • lastModifiedBy is deliberately excluded — same id-not-name problem, separate follow-up.

23 functional requirements, 7 success criteria, 3 user stories.

Three findings that shaped the spec

The Immutables mechanism is left to the plan phase, deliberately. @Value.Derived computes at build(), so the user lookup would fire on every Experiment construction — including the DB transformer, the push-publish dependency walk and running-experiment selection during page render, none of which serialize anything. @Value.Lazy memoizes inside the instance, and Experiment instances are themselves cached in ExperimentsCache, so a memoized name would outlive a rename. Neither is settable from JSON. The spec states four constraints (FR-014, FR-015, FR-016, FR-023) and leaves the choice to /speckit-plan, which has to check its pick against all four.

User lookups are already cache-backed. UserFactoryLiferayImpl.loadUserById reads UserCache before the database, so a listing whose experiments share a creator costs one read. Kept as a requirement anyway: a listing spanning many distinct creators must not degenerate into one lookup per row.

A @Schema on an Immutables abstract accessor does reach openapi.yaml — verified against AbstractTimestampsView, whose per-accessor descriptions and examples appear verbatim under the generated TimestampsView schema. That is what makes annotating the model sufficient.

Corrections to the issue body

The issue's endpoint list is close but not exact, so the spec carries a 14-endpoint table that supersedes it. None of this changes the approach — the field is added once and every experiment-carrying response inherits it.

  • DELETE /v1/experiments/{experimentId} is listed among the endpoints carrying an Experiment; it returns the confirmation string "Experiment deleted".
  • Two endpoints that do return an Experiment are missing: DELETE /v1/experiments/{experimentId}/goals/primary and DELETE /v1/experiments/{experimentId}/targetingConditions/{id}.
  • The update endpoint is PATCH, not PUT.

Checklist

  • Tests — land with the implementation (FR-020/FR-021: happy path from the list and the single fetch, unresolvable-creator fallback, every new class registered in a MainSuite*)
  • Translations — not applicable, backend only
  • Security Implications Contemplated — createdBy and getOwner() are untouched, so permission behaviour is unchanged. The field exposes a creator's display name only to users who can already read the experiment.

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.
@claude

claude Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @oidacra's task in 1m 42s —— View job


Spec review: creator name on the Experiments API

  • Read the spec
  • Verify factual claims against the codebase
  • Post review

This is a spec-only PR, so I reviewed the spec's factual accuracy against the code it commits to. The endpoint table, the getOwner()/createdBy relationship, the Immutables analysis, and the @Value.Derived vs @Value.Lazy reasoning all check out. One factual claim is wrong in a way that could mislead the plan phase.

New Issues

  • 🟡 Medium: specs/37304-experiment-created-by-username/spec.md:129,177,308User.getFullName() does not return "" for an all-blank name — it returns a single space " ". The spec states in the edge cases (line 129), A4 (line 308: "returns an empty string when every part is blank"), and implies in FR-003/FR-010 that the blank-name case surfaces as an empty string. Tracing the real code: User.getFullName() (com/liferay/portal/model/User.java:164) normalizes each part to "" then calls getFullName("", "", ""). Since Validator.isNull("") is true (com/liferay/util/Validator.java:349, trims then compares to blank), the middle-name branch is skipped and it returns firstName + StringPool.SPACE + lastName"" + " " + """ " (one space), not "".

    Why it matters for the plan: FR-003 requires the field is "never an empty string," and FR-010 requires the blank-name case to fall back to the raw ID. If /speckit-plan takes A4 literally and implements the fallback as fullName.isEmpty() ? id : fullName, the all-blank-name user yields a field of " " — which is non-empty by isEmpty() but still a blank-looking column, silently violating FR-010's intent. The fallback must test blank after trim (e.g. UtilMethods.isSet(name.trim()) / StringUtils.isBlank(name)), not emptiness. Recommend correcting A4's wording and making FR-010 explicit that the trigger is a blank/whitespace-only getFullName(), so the derived requirement can't be mis-implemented. Fix this →

Verified accurate (no action needed)

  • ✅ Endpoint table (lines 222-241) — all 14 in-scope + 4 out-of-scope endpoints match ExperimentsResource.java. Both DELETE .../goals/primary (line 252, returns ResponseEntitySingleExperimentView) and DELETE .../targetingConditions/{id} (line 452, same) do carry an Experiment; DELETE /{experimentId} returns a confirmation. The A7 correction to the issue body is correct.
  • getOwner() returns createdBy() (AbstractExperiment.java:124), a @Value.Derived @JsonIgnore method — FR-006's "owner resolves from createdBy" premise holds.
  • ✅ The @Value.Derived-fires-at-build concern (A2) is well-founded: getOwner, getIdentifier, getManifestInfo etc. are all @Value.Derived on this model, so the mechanism choice genuinely needs the FR-014/015/016/023 gate the spec defers to the plan.

Overall the spec is thorough and the additive approach is sound. The single-space finding is non-blocking but should be corrected before /speckit-plan so the fallback is implemented as a blank-check rather than an empty-check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Expose the experiment creator's username in the Experiments API

1 participant