Skip to content

Create new endpoint to save style editor schema per content type#36070

Open
dario-daza wants to merge 19 commits into
mainfrom
35781-create-new-endpoint-to-save-style-editor-schema-per-content-type
Open

Create new endpoint to save style editor schema per content type#36070
dario-daza wants to merge 19 commits into
mainfrom
35781-create-new-endpoint-to-save-style-editor-schema-per-content-type

Conversation

@dario-daza

@dario-daza dario-daza commented Jun 9, 2026

Copy link
Copy Markdown
Member

Proposed Changes

Create the new PATCH /v1/contenttype/id/{idOrVar}/metadata endpoint to avoid editing other Content Type information when creating and saving a Style Editor Schema for a Content Type.

BE

  • Create the new PATCH /v1/contenttype/id/{idOrVar}/metadata endpoint to handle the metadata of the Content Types, specifically the DOT_STYLE_EDITOR_SCHEMA that is related to the Style Editor Schema.
  • Fix Postman tests using the new endpoint.
  • Create an integration test for the new endpoint.

FE

  • Call the new endpoint when saving Style Editor Schemas per Content Type.
  • Replace the use of DotCrudService with http.
  • Fix tests.

This PR fixes: #35781
This PR fixes: #35979

@claude

claude Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @dario-daza's task in 2m 13s —— View job


Rollback Safety Analysis

  • Read rollback-unsafe categories reference
  • Get full PR diff
  • Analyze diff against unsafe categories
  • Applied label: AI: Not Safe To Rollback

Pull Request Unsafe to Rollback!!!

Category: M-3 — REST / GraphQL / Headless API Contract Change
Risk Level: 🟡 MEDIUM

Why it's unsafe:

This PR introduces two API contract changes that are unsafe to roll back:

1. New additive endpoint (primary risk)
A new PATCH /v1/contenttype/id/{idOrVar}/metadata endpoint is added in the same PR that updates the Angular frontend to call it. If the backend is rolled back to N-1 while a browser or CDN has cached the N-version Angular bundle, any attempt to save a Style Editor Schema will call PATCH .../metadata, which does not exist in N-1, resulting in a 404 error. Users will be unable to save Style Editor Schemas until the browser cache clears.

  • Code: ContentTypeResource.java — new @PATCH @Path("/id/{idOrVar}/metadata") handler
  • Code: dot-style-editor-builder.component.tsthis.#http.patch<DotCMSResponse>(\v1/contenttype/id/${contentType.id}/metadata`, metadataPatch)replaces the priorDotCrudService.putData` PUT call
  • Code: openapi.yaml — new path /v1/contenttype/id/{idOrVar}/metadata added

2. Behavioral change to existing PUT endpoint (secondary risk)
The PUT /v1/contenttype/id/{idOrVar} endpoint's semantics are changed: a PUT body that omits the metadata key now preserves existing metadata, whereas N-1 would wipe it. Any client or script that deployed relying on the new "absent metadata = preserve" contract would lose that protection after rollback — metadata could again be silently overwritten by a bare PUT.

  • Code: ContentTypeResource.java lines 787–793 — new guard checks requestContainsKey(form.getRequestJson(), "metadata") and carries forward existing metadata when absent

Rollback mitigation: Both risks are bounded. The old PUT endpoint is preserved (not removed), so operators can restore Style Editor Schema saves by clearing the CDN/browser cache and forcing a frontend refresh after rollback.

@dario-daza dario-daza marked this pull request as ready for review June 9, 2026 14:57
@dario-daza dario-daza changed the title 35781 create new endpoint to save style editor schema per content type Create new endpoint to save style editor schema per content type Jun 9, 2026
@claude

claude Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Pull Request Unsafe to Rollback!!!

  • Category: M-3 — REST / GraphQL / Headless API Contract Change
  • Risk Level: 🟡 MEDIUM
  • Why it's unsafe: This PR adds a new PATCH /v1/contenttype/id/{idOrVar}/metadata endpoint and updates the Angular frontend component in the same PR to call it (replacing the previous PUT /v1/contenttype/id/{idOrVar} call). If the backend is rolled back to N-1 while a browser or CDN has cached the N-version Angular frontend, any attempt to save a Style Editor Schema will call the new endpoint which does not exist on N-1, resulting in a 404 error. The user will be unable to save Style Editor Schemas until the browser cache clears.
  • Code that makes it unsafe:
    • dotCMS/src/main/java/com/dotcms/rest/api/v1/contenttype/ContentTypeResource.java — new @PATCH @Path("/id/{idOrVar}/metadata") handler added at line 2022+
    • core-web/apps/dotcms-ui/src/app/portlets/shared/dot-content-types-edit/components/style-editor/dot-style-editor-builder.component.ts — line 247: this.#http.patch\<DotCMSResponse\>(v1/contenttype/id/${contentType.id}/metadata, metadataPatch) replaces the prior DotCrudService.putData call to the old PUT endpoint
    • dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml — new path /v1/contenttype/id/{idOrVar}/metadata added (lines 8148+)
  • Alternative (if possible): Since the new endpoint is purely additive (a new path, no existing path removed or changed), the rollback risk is bounded: only the Style Editor Schema save fails with 404, and only while the N-frontend is cached. If the old PUT /v1/contenttype/id/{idOrVar} endpoint is preserved (which it is — this PR does not remove it), operators can restore full functionality by clearing the CDN cache and/or forcing a frontend refresh after rollback.

@github-actions

Copy link
Copy Markdown
Contributor

Security Review

No high-confidence security findings on the changes in this PR.

@fabrizzio-dotCMS

Copy link
Copy Markdown
Member

Question: is metadata preserved when a Content Type is saved through the regular full PUT?

This new endpoint (PATCH /api/v1/contenttype/id/{idOrVar}/metadata) merges into the existing metadata and serializes DOT_STYLE_EDITOR_SCHEMA correctly — that part looks good.

My concern is the interaction with the regular update path (putContentTypeUpdatePUT /api/v1/contenttype/id/{idOrVar}), whose Swagger description states:

⚠️ Destructive semantics. This endpoint treats the request body as the full desired state. Any editable property (including items in fields[] and metadata keys) absent from the body will be removed.

So if any client does a normal full-CT PUT without carrying DOT_STYLE_EDITOR_SCHEMA in metadata, the schema written by this new endpoint would be silently wiped.

  1. Does the CT edit screen (or any existing client) round-trip the full metadata map — including DOT_STYLE_EDITOR_SCHEMA — on a regular save? If not, a normal save would clobber the style editor schema.
  2. Should the style editor schema be protected against the full-PUT's destructive merge (e.g. preserved server-side unless explicitly cleared), or is the contract strictly "GET → mutate → PUT the whole object"?

Mainly want to make sure the schema persisted here survives a subsequent normal CT save.

@github-actions

github-actions Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

❌ Codex Review failed — openai.gpt-5.5

The review job failed before producing output. See the run for details.

Run: #27579601571

@dario-daza

Copy link
Copy Markdown
Member Author

Question: is metadata preserved when a Content Type is saved through the regular full PUT?

This new endpoint (PATCH /api/v1/contenttype/id/{idOrVar}/metadata) merges into the existing metadata and serializes DOT_STYLE_EDITOR_SCHEMA correctly — that part looks good.

My concern is the interaction with the regular update path (putContentTypeUpdatePUT /api/v1/contenttype/id/{idOrVar}), whose Swagger description states:

⚠️ Destructive semantics. This endpoint treats the request body as the full desired state. Any editable property (including items in fields[] and metadata keys) absent from the body will be removed.

So if any client does a normal full-CT PUT without carrying DOT_STYLE_EDITOR_SCHEMA in metadata, the schema written by this new endpoint would be silently wiped.

  1. Does the CT edit screen (or any existing client) round-trip the full metadata map — including DOT_STYLE_EDITOR_SCHEMA — on a regular save? If not, a normal save would clobber the style editor schema.
  2. Should the style editor schema be protected against the full-PUT's destructive merge (e.g. preserved server-side unless explicitly cleared), or is the contract strictly "GET → mutate → PUT the whole object"?

Mainly want to make sure the schema persisted here survives a subsequent normal CT save.

The way we handled it in the frontend, it never happens, but it is possible that any REST API caller (Postman, a migration script, or a third-party integration) that makes a raw PUT and omits metadata from the body would silently wipe the schema. I added a validation that prevents this and preserves the metadata if a user omits it in a PUT request. If you want to wipe the metadata, you'll need to send metadata: null.

@github-actions

github-actions Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

🤖 Bedrock Review — deepseek.v3.2

[🟠 High] dotCMS/src/main/java/com/dotcms/rest/api/v1/contenttype/ContentTypeResource.java:782 — Missing @WrapInTransaction on updateContentTypeMetadata method. This method performs a read-modify-write operation (mergeAndSaveMetadata), which includes database writes. Without a transaction, partial failures could leave metadata in an inconsistent state.

[🟠 High] dotCMS/src/main/java/com/dotcms/rest/api/v1/contenttype/ContentTypeHelper.java:1026 — The mergeAndSaveMetadata method catches Throwable (including Error subclasses like OutOfMemoryError) and wraps it in a DotDataException. This is an anti-pattern; Error types should not be caught and converted, as they indicate unrecoverable JVM conditions. Should catch Exception only.

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/api/v1/contenttype/ContentTypeHelper.java:1063 — The requestContainsKey method logs a warning when JSON parsing fails but returns false. This silently converts a malformed request (which should be a BadRequestException) into an "absent key" assumption, potentially masking client errors. Should either throw BadRequestException or be renamed to reflect its best-effort nature.

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/api/v1/contenttype/ContentTypeResource.java:1102 — In updateContentTypeMetadata, if metadataPatch is null or empty, the method logs a warning and returns the existing content type. This treats an empty PATCH as a no-op, which deviates from standard PATCH semantics (empty body is a valid no-op). The warning log is noisy and should be removed.

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/api/v1/contenttype/ContentTypeResource.java:1103 — Returning HTTP 200 for an empty PATCH is correct, but the warning log "No metadata patch found for ..." is misleading and will clutter logs. Consider removing the log or reducing to DEBUG.

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/api/v1/contenttype/ContentTypeHelper.java:1048 — The normalizeStyleEditorSchemaToString method logs a warning when serialization fails, then throws a BadRequestException. The warning is redundant with the exception and will produce duplicate logs. Should either log at DEBUG or remove.

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/api/v1/contenttype/ContentTypeHelper.java:1069 — In requestContainsKey, the warning log references ContentTypeResource.class but is inside ContentTypeHelper. This is a copy-paste error; the class reference should be ContentTypeHelper.class.

[🟡 Medium] core-web/apps/dotcms-ui/src/app/portlets/shared/dot-content-types-edit/components/style-editor/dot-style-editor-builder.component.ts:247 — The #loadFromMetadata method logs a warning to console.warn. This violates dotCMS conventions; should use Logger instead. However, this is frontend code, so the convention may not apply. Still, consider using a proper logging service.

[🟡 Medium] dotcms-integration/src/test/java/com/dotcms/rest/api/v1/contenttype/ContentTypeResourceUpdateMetadataTest.java:430 — The test file is missing a final newline. This is a minor style issue but violates many code style checks.

[🟢 Note] The PR introduces a new PATCH endpoint for metadata with proper locking (IdentifierStripedLock) to prevent lost updates per content type within a JVM. This is a good design for the concurrency requirement. However, note that the lock is JVM-local, so concurrent updates across cluster nodes are still subject to last-write-wins, as acknowledged in the comment. This is acceptable for the current scope but should be documented as a limitation.


Run: #27644543049 · tokens: in: 19293 · out: 892 · total: 20185

@semgrep-code-dotcms-test

Copy link
Copy Markdown
Contributor

Legal Risk

The following dependencies were released under a license that
has been flagged by your organization for consideration.

Recommendation

While merging is not directly blocked, it's best to pause and consider what it means to use this license before continuing. If you are unsure, reach out to your security team or Semgrep admin to address this issue.

GPL-2.0

MPL-2.0

final ContentType saved = contentTypeHelper.mergeAndSaveMetadata(idOrVar, metadataPatch, contentTypeAPI);
return Response.ok(new ResponseEntityContentTypeDetailView(
new HashMap<>(contentTypeHelper.contentTypeToMap(saved, user)))).build();
} catch (final NotFoundInDbException e) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use ResponseUtil.mapExceptionResponse(e) with a single catch block instead of the different blocks we have right now? That util method delagates all the responses creation

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used ErrorEntity to return responses with errorCode and fieldName to make it easier to read the cause, use mapExceptionResponse would reply plain message strings. Happy to simplify if those fields aren't consumed, but keeping them for now to preserve the API contract

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used ErrorEntity to return responses with errorCode and fieldName to make it easier to read the cause, use mapExceptionResponse would reply plain message strings. Happy to simplify if those fields aren't consumed, but keeping them for now to preserve the API contract

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

Labels

AI: Not Safe To Rollback Area : Backend PR changes Java/Maven backend code Area : Frontend PR changes Angular/TypeScript frontend code

Projects

Status: No status

4 participants