fix(content): a Content Type's own hostName field is no longer overwritten by the derived one

[nicobytes]

Fixes #37584

What was wrong

Opening a Site in the new Edit Content editor hit two independent defects at once.

A — the Site Key field showed the wrong value. DefaultTransformStrategy.addCommonProperties
writes a derived property under the key hostName — the name of the Site the Contentlet lives
on. hostName is also a real field variable on the Host Content Type: the required Text field
labelled "Site Key". Every Host lives on the System Host by definition, so the derived value was
always the literal System Host, and it overwrote the stored value for every Site.

B — a 500 on load. WorkflowResource.getWorkflowTasksHistoryComments passed the result of
findTaskByContentlet straight into getCommentsAndChangeHistory, which dereferences it. A Site
can never have a workflow task, so it always NPE'd and the editor raised a blocking
"Unknown Error" dialog.

Both are visible side by side in the video on
the issue.

What changed

File	Change
DefaultTransformStrategy.java	Guard the derived hostName write + the shared declaresField predicate
ContentletToMapTransformer.java	The same guard — the legacy transformer carried the identical line
WorkflowResource.java	Empty timeline instead of an NPE when there is no task; Javadoc corrected

if (!declaresField(type, HOST_NAME)) {
    map.put(HOST_NAME, site != null ? site.getHostname() : NOT_APPLICABLE);
}
map.put(HOST_KEY, site != null ? site.getIdentifier() : NOT_APPLICABLE);

Three things worth a reviewer's attention:

1. It is a general rule, not a Host special case. hostName is not in
   FieldFactoryImpl.RESERVED_FIELD_VARS, unlike every sibling derived key on that path (host,
   modUserName, ownerUserName, creationDate, publishUser). So a customer can declare a
   hostName field on a custom Content Type today and hit the identical silent overwrite. The
   guard covers both with one rule. HOST_KEY needs no guard precisely because "host" is
   reserved.
2. The same defect lived in two transformers. Fixing only the modern path made
   Transformer_Backwards_Compatibility_Test fail — it asserts the legacy and modern transformers
   agree, and +basetype:1 pulls Host contentlets because Host's base type is CONTENT. Both are
   fixed from one predicate rather than weakening the parity assertion.
3. declaresField iterates fields(), not fieldMap() — the latter collects into a Guava
   ImmutableMap and throws on a field with a null variable, which is legal in the database.

Only GET /api/v1/content/{inodeOrIdentifier} exposed defect A, because ContentResource:426
hydrates with contentResourceOptions(false) — COMMON_PROPS but not SITE_VIEW, so
SiteViewStrategy never runs to restore the field. /api/content/id/...,
/api/v1/content/_search and /api/v1/site/... were always correct, which is why the Site
selector and Site Browser looked fine.

Answering the issue's open question

The save never reaches the forceExecution guard at HostAPIImpl.java:476. The Host Content
Type has no Workflow scheme, so the editor offers no save action; and forcing a fire through the
API is rejected by WorkflowAPIImpl.fireContentWorkflow. So there was no silent overwrite, no
UpdateContainersPathsJob / UpdatePageTemplatePathJob cascade, and no stored data was ever
corrupted — no repair migration needed, severity stays High.

Recorded in full on the issue. Worth its own ticket: the new editor cannot save a Host at all.

Testing

Every JUnit test below was confirmed failing for the right reason before the fix was written.
The Postman requests are the exception — see the caveat under the table.

• Unit — DefaultTransformStrategyTest: the collision predicate, including a field with a
  null variable.
• Integration — ContentletTransformerTest: a Site keeps its own hostName; the System Host
  still reports its own name (the one Site the broken value got right by accident); a non-Host
  Contentlet still reports its parent Site (regression guard — passes before and after); and a
  Site read through the editor path saves without forceExecution, which is the issue's
  AC-003 and tripped HostAPIImpl:476 before.
• Integration — WorkflowResourceResponseCodeIntegrationTest: empty timeline with no task,
  for a plain Contentlet and for a Site; full timeline preserved when a task exists.
• Postman — contract requests on both endpoints. ⚠️ Authored but not yet executed. The two
  requests added to Workflow_Resource_Tests.json carry no auth block, while that collection
  declares no default auth and 258 of its 274 requests set basic per request — so they will 401
  until that is added. Being fixed before merge; ContentResourceV1 is unaffected (collection-level
  bearer).

Unit          11 tests,  0 failures, 0 errors
Integration   77 tests,  0 failures, 0 errors
openapi.yaml  unchanged

Also verified live against a locally built image — see the before/after video on the issue.

Not in scope

The 2012 hostname / hostName alias in Host.getMap() is untouched, as the issue asks; after
the fix the two simply no longer disagree. Also untouched: contentResourceOptions(),
RESERVED_FIELD_VARS, and the Host workflow prohibition.

Process note for reviewers

Built with the Spec-Kit fix flow. specs/37584-site-key-system-host/ carries spec.md,
data-model.md and the REST contract; the plan, research, tasks, quickstart and checklists stay
local per .gitignore ("Spec-Kit working artifacts — process-only, kept local"), matching every
other spec directory in the repo.

The spec and the implementation are in this single PR rather than the usual two — the spec was
never opened for separate sign-off, so please review it here, or say the word and I will split it.

🤖 Generated with Claude Code

[claude]

Claude finished @nicobytes's task in 2m 18s —— View job

---

Review: PR #37589

• Read changed Java files (transformers, WorkflowResource)
• Analyze the declaresField guard and the NPE fix
• Trace map seeding + blast radius (ES vs. views)
• Review test coverage

I traced both fixes end-to-end. They are correct and well-scoped.

Correctness verification

A — the hostName guard. Both transformers seed their working map from contentlet.getMap() (DotContentletTransformerImpl.java:101, ContentletToMapTransformer.java:95), which already carries stored field values. So skipping the derived map.put(HOST_NAME, …) when declaresField(type, HOST_NAME) is true preserves the stored value rather than dropping the key — the failure mode I checked for (a declared-but-empty field leaving hostName absent) reduces to the field's own stored value, which is the intended behavior. HOST_KEY correctly needs no guard since "host" is in RESERVED_FIELD_VARS.

declaresField iterating fields() over fieldMap() is the right call — the Guava ImmutableMap in fieldMap() would throw on a null-variable field, and the predicate's null != field && … null-guards each element. Covered by the unit test.

Blast radius is bounded. I checked whether this transform path feeds ES indexing (ESMappingAPIImpl / reindex) — it does not; these transformers are read-time view hydration only. So the changed hostName value cannot alter indexed documents or site-filtering (conhost/"host" is untouched). Sharing one public static predicate across both transformers keeps the parity assertion in Transformer_Backwards_Compatibility_Test honest.

B — the workflow NPE. WorkflowResource.java:6292 now returns ResponseEntityWorkflowHistoryCommentsView(List.of()) when findTaskByContentlet is null, matching the method's existing return type in both branches — no @Schema/openapi impact. A null task means no task-bound change history, so an empty timeline is the semantically correct result, not a masked error. The DoesNotExistException for a truly missing contentlet is preserved below.

New Issues

No issues found.

Both defects are addressed at the correct layer with matching unit, integration, and Postman coverage, and the guard is generalized to the real rule (a declared field wins over a derived property) rather than special-casing Host. The reasoning in the PR body — no save path reaches HostAPIImpl:476, so no repair migration is needed — is consistent with the Host workflow prohibition.

One process note, not a code issue: per this repo's Spec-Kit flow the spec normally ships in its own PR gated on separate approval; here spec.md rides along with the implementation. That's called out in the description — reviewers should sign off on specs/37584-site-key-system-host/spec.md in this PR or ask for the split.
· nicobytes/new-edit-content-shows-every-sites-site-key-as-s

[Copilot]

🟡 Changes recommended

Add authentication to the workflow fixtures and resolve or renegotiate the unmet AC-002 alias requirement.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes Site Key overwrites and workflow-history errors when opening Sites.

Changes:

• Preserve declared hostName fields in both transformers.
• Return empty workflow timelines when no task exists.
• Add specifications, tests, and Postman coverage.

Review findings:

• Two workflow Postman requests omit bearer authentication.
• The specification does not satisfy AC-002 for the legacy hostname alias.

File summaries

File	Description
specs/37584-site-key-system-host/spec.md	Defines scope and acceptance criteria; AC-002 remains unmet for new Sites.
specs/37584-site-key-system-host/data-model.md	Documents map precedence and collision handling.
specs/37584-site-key-system-host/contracts/site-key-and-workflow-comments.md	Documents REST contract behavior.
dotCMS/src/test/java/com/dotmarketing/portlets/contentlet/transform/strategy/DefaultTransformStrategyTest.java	Tests collision detection.
dotCMS/src/main/java/com/dotmarketing/portlets/contentlet/transform/strategy/DefaultTransformStrategy.java	Guards derived hostName writes.
dotCMS/src/main/java/com/dotmarketing/portlets/contentlet/transform/ContentletToMapTransformer.java	Applies the collision guard to the legacy transformer.
dotCMS/src/main/java/com/dotcms/rest/api/v1/workflow/WorkflowResource.java	Handles missing workflow tasks safely.
dotcms-postman/src/main/resources/postman/Workflow_Resource_Tests.json	Adds workflow tests; two requests omit bearer authentication.
dotcms-postman/src/main/resources/postman/ContentResourceV1.postman_collection.json	Adds Site Key API tests.
dotcms-integration/src/test/java/com/dotmarketing/portlets/contentlet/transform/ContentletTransformerTest.java	Tests Site and regular content transformations.
dotcms-integration/src/test/java/com/dotcms/rest/api/v1/workflow/WorkflowResourceResponseCodeIntegrationTest.java	Tests empty and populated workflow timelines.

Review details

Suppressed comments (2)

dotcms-postman/src/main/resources/postman/Workflow_Resource_Tests.json:19807

• This request does not set the bearer authentication used by the rest of this collection. Workflow_Resource_Tests.json has no collection-level auth (the JWT is only generated by the collection pre-request script), and existing requests attach {{jwt}} explicitly, so this new fixture will receive an unauthorized response instead of exercising the no-task endpoint. Add the same bearer auth block used by the neighboring workflow requests.

					"request": {
						"method": "GET",
						"header": [],

dotcms-postman/src/main/resources/postman/Workflow_Resource_Tests.json:19851

• This request also omits authentication. Because the collection has no inherited auth and the JWT is only generated by its pre-request script, this endpoint call will be unauthorized and the added assertions cannot validate the workflow fix. Attach the collection's bearer token here, as the existing requests in this file do.

					"request": {
						"method": "GET",
						"header": [],

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jcastro-dotcms]

Code Review — PR #37589

Reviewed the fix for issue #37584 (Site Key showing "System Host" + 500 on workflow comments for Sites). Overall the core fix (hostName guard + workflow null-check) is correct and covered by new tests, but a few gaps and a sibling bug should be addressed before merge.

🔴 Critical

1. WorkflowResource.saveComment() has the same unguarded NPE this PR fixes elsewhere
dotCMS/src/main/java/com/dotcms/rest/api/v1/workflow/WorkflowResource.java:6394-6399

final WorkflowTask task = this.workflowAPI.findTaskByContentlet(currentContentlet.get());
...
taskComment.setWorkflowtaskId(task.getId()); // NPE if task is null

findTaskByContentlet returns null whenever no workflow_task row exists — exactly the condition this PR already fixes at the sibling getWorkflowTasksHistoryComments() endpoint ~100 lines above. Posting a comment against a Host (or any Contentlet whose task was deleted/never created) throws the same NPE/500, one call site away, in the file this PR is already editing.

🟠 High

2. Null-check conflates "no task for this language" with "no task at all"
   WorkflowResource.java:6289

WorkFlowFactoryImpl.findTaskByContentlet queries keyed on both identifier and languageId — a task is per-language. A multi-language Contentlet with comments recorded under language A returns null (empty 200 timeline) when resolved under language B — silently reading as "no history" after a language switch, where it previously errored loudly. Fixes the Host case but changes behavior for the ordinary multi-language case.

3. Field-collision fix is scoped to hostName only, not applied as the general rule the PR describes
   DefaultTransformStrategy.java:108 (addCommonProperties)

The same unguarded collision risk remains for SHORTY_ID (~109), TITLE_IMAGE_KEY/HAS_TITLE_IMAGE_KEY (~118-122), and the urlMap key (~131-132). None of shortyId, titleImage, hasTitleImage, or the urlMap key are in FieldFactoryImpl.RESERVED_FIELD_VARS, so a custom Content Type can legally declare a field with one of these names — and every transform will silently clobber the stored value with the derived one. This is the exact defect class from #37584, left unfixed despite the PR's own stated rationale ("a general rule, not a Host special case").

🟡 Medium

4. New declaresField guard removes the previous fallback value for hostName
   DefaultTransformStrategy.java:100 (duplicated in ContentletToMapTransformer.java:80)

Before this fix, every Contentlet unconditionally got hostName set to at least NOT_APPLICABLE or the derived Site name. Now, if a Content Type declares hostName but leaves it unset on a given Contentlet, the key is absent entirely instead of falling back to a sentinel — a consumer doing map.get(Contentlet.HOST_NAME) (previously always non-null) now gets null.

5. Wasted host lookup when the guard skips the write
   ContentletToMapTransformer.java:101

APILocator.getHostAPI().find(contentlet.getHost(), ...) still runs unconditionally before the new declaresField guard. For every Host contentlet transformed via this path — exactly the case the guard exists for — the lookup executes and its result is discarded, since the if (!declaresField(...)) immediately below is now false.

🔵 Low / Cleanup

6. declaresField duplicates existing field-lookup logic, in the wrong place
   DefaultTransformStrategy.java:128

Reimplements a "does this ContentType declare a field with variable X" scan that already exists in ContentHelper (isCategoryField/isTagField, ContentHelper.java:660-680, and a similar inline check ~767). Living on a transform-strategy class instead of a shared ContentType utility makes it hard to discover for reuse, and risks the two implementations drifting (e.g. if lowercase comparison is added to match RESERVED_FIELD_VARS, it may only be applied to one).

---

🤖 Generated with Claude Code