Skip to content

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

Open
dario-daza wants to merge 22 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 22 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 3m 4s —— View job


Rollback Safety Analysis

  • Read rollback-unsafe categories reference
  • Get full PR diff
  • Analyze diff against unsafe categories
  • Post results and add label

Result: 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 /api/v1/contenttype/id/{idOrVar}/metadata endpoint and simultaneously updates the Angular frontend to call it (replacing the prior PUT /api/v1/contenttype/id/{idOrVar} call for Style Editor Schema saves). If the backend is rolled back to N-1 while a browser or CDN still serves the N-version frontend, any attempt to save a Style Editor Schema will hit the new endpoint — which does not exist on N-1 — returning a 404 error. Users are unable to save Style Editor Schemas until the browser/CDN 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 (lines ~2024–2128)
    • core-web/apps/dotcms-ui/src/app/portlets/shared/dot-content-types-edit/components/style-editor/dot-style-editor-builder.component.ts:213this.#http.patch<DotCMSResponse>(\v1/contenttype/id/${contentType.id}/metadata`, metadataPatch)replaces the priorDotCrudService.putData` call to the existing PUT endpoint
    • dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml — new path /v1/contenttype/id/{idOrVar}/metadata added
  • Alternative (if possible): Since the new endpoint is purely additive (a new path, no existing endpoint removed or changed), the rollback risk is bounded: only Style Editor Schema saves fail with 404, and only while the N-frontend is cached. The existing PUT /api/v1/contenttype/id/{idOrVar} endpoint is preserved (not removed by this PR), so operators can restore full functionality by clearing CDN cache and/or forcing a browser refresh after rollback.


No other unsafe categories apply:

  • No database schema changes (no runonce tasks, no DDL)
  • No Elasticsearch mapping changes
  • No content JSON model version bump (CURRENT_MODEL_VERSION unchanged)
  • No table/column drops, renames, or PK restructuring
  • No push publishing bundle format changes
  • No OSGi public interface changes
  • The WorkflowResource.java change is cosmetic (.collect(Collectors.toList()).toList())
  • The preserveMetadataIfAbsent change on the PUT endpoint changes behavior (preserves metadata instead of wiping it), but this is purely additive/protective and does not break N-1 rollback semantics

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

@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

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/api/v1/contenttype/ContentTypeHelper.java:1056-1058mergeAndSaveMetadata catches Throwable and rethrows as DotDataException, which could swallow unexpected errors like OutOfMemoryError or StackOverflowError. Should catch only specific exceptions (DotDataException, DotSecurityException, RuntimeException).

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/api/v1/contenttype/ContentTypeHelper.java:1070 — The comment mentions "true multi-node safety requires either optimistic locking on modDate or an atomic DB-level JSON merge" but the method doesn't implement it. This is a known limitation, but should be documented in the method's JavaDoc as a caveat.

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/api/v1/contenttype/ContentTypeHelper.java:1088normalizeStyleEditorSchemaToString logs a warning but then throws BadRequestException. The warning is redundant since the exception will be logged upstream; consider removing the Logger.warn line to avoid duplicate logs.

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/api/v1/contenttype/ContentTypeHelper.java:1123-1124preserveMetadataIfAbsent uses ContentTypeBuilder.builder(contentType) which doesn't copy innerFields. The method attempts to re-attach fields via constructWithFields, but this is a fragile workaround. The comment explains it, but this is a design flaw in ContentTypeBuilder that could cause subtle bugs elsewhere.

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/api/v1/contenttype/ContentTypeHelper.java:1149requestContainsKey logs a warning when JSON parsing fails. This could leak sensitive request data into logs. Should sanitize the exception message or log at debug level.

[🟠 High] dotCMS/src/main/java/com/dotcms/rest/api/v1/contenttype/ContentTypeResource.java:2100-2102 — In updateContentTypeMetadata, if metadataPatch is null or empty, it logs a warning and returns the existing content type. This is inconsistent with typical PATCH semantics (no-op is fine, but logging a warning is noisy). Change to debug logging.

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/api/v1/contenttype/ContentTypeResource.java:2114 — The BadRequestException catch block returns a ResponseEntityContentTypeDetailView with a hardcoded field name "DOT_STYLE_EDITOR_SCHEMA". This assumes the error is always about that key, but BadRequestException could be thrown for other reasons. Should extract the field from the exception or use a generic error.

[🟡 Medium] dotcms-integration/src/test/java/com/dotcms/rest/api/v1/contenttype/ContentTypeResourceUpdateMetadataTest.java:430 — The test file is missing a closing brace for the class. The diff ends abruptly; ensure the file is syntactically correct.

[🟡 Medium] core-web/apps/dotcms-ui/src/app/portlets/shared/dot-content-types-edit/components/style-editor/dot-style-editor-builder.component.ts:244#loadFromMetadata logs a warning to console (console.warn). Should use a proper Angular/DotCMS logger service.

[🟡 Medium] core-web/apps/dotcms-ui/src/app/portlets/shared/dot-content-types-edit/components/style-editor/dot-style-editor-builder.component.ts:187-189 — The method #save uses let metadataPatch and reassigns it. Could be simplified to a const with a ternary or immediate assignment.

[🟡 Medium] dotcms-postman/src/main/resources/postman/Define_Contentlets_StyleProperties.postman_collection.json — The Postman collection updates are extensive but mostly correct. However, there are multiple duplicate test steps (e.g., "Save Content Type Schema" appears twice). This is not a code issue but could cause confusion in testing.


Run: #27696239275 · tokens: in: 19215 · out: 938 · total: 20153

@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

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