Skip to content

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

Open
dario-daza wants to merge 12 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 12 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 5m 23s —— View job


PR Review

  • Gather context
  • Review backend changes (Java)
  • Review frontend changes (Angular)
  • Review tests
  • Review OpenAPI spec
  • Final summary

Overall the design is solid: the race condition fix (striped lock + re-read inside the lock) is correct, the normalization of DOT_STYLE_EDITOR_SCHEMA to a string is well-justified, and the test coverage is thorough. A few issues worth addressing:

Backend

1. Logger.warn fires on a documented no-op — should be debug

ContentTypeResource.java:2094

Logger.warn(this, "No metadata patch found for " + idOrVar);

The OpenAPI description explicitly says an empty body "is treated as a no-op — the current metadata is returned unchanged with HTTP 200." Every health-check or defensive caller that sends {} will spam the warn log. Change to Logger.debug.

Fix this →


2. Lock-timeout (DotConcurrentException) surfaces as HTTP 500

ContentTypeHelper.java:1052-1057 — the catch-all wraps every non-DotDataException/DotSecurityException as DotDataException, including a DotConcurrentException thrown when the striped lock times out. That ends up as a 500. Under load, a locked content type returns the same status code as a database failure, which makes it hard to distinguish in monitoring. Consider catching DotConcurrentException separately in the resource or rethrowing a more specific exception so a 503 can be returned.


3. mergeAndSaveMetadata: no @WrapInTransaction

Other write methods in ContentTypeHelper that call contentTypeAPI.save() (e.g. saveSchemesByContentType) are annotated @WrapInTransaction. The new method does a find + save without a transaction boundary. contentTypeAPI.save() likely opens its own transaction, so this probably works today, but it's inconsistent with the class's existing convention and could surprise a future caller who wraps this in a multi-step operation.


4. contentTypeToMap returns the full Content Type on every metadata PATCH response

ContentTypeResource.java:2101-2102 — the response includes all fields, workflows, and system action mappings, which is significantly heavier than what the FE actually consumes (it only checks for success). Not a correctness issue, but it's an unnecessary DB read and serialization cost on every save. Low priority.


Frontend

5. #schemaLoaded flag is instance-level, not input-level — risky if component is reused

dot-style-editor-builder.component.ts:110,168-174

#schemaLoaded = false;

effect(() => {
    const contentType = this.$contentType();
    if (contentType && !this.#schemaLoaded) {
        this.#schemaLoaded = true;
        untracked(() => this.#loadFromMetadata(contentType));
    }
});

If the parent ever passes a different content type to the same component instance (e.g., in a navigation flow where the component is not destroyed between routes), #schemaLoaded stays true and the new content type's schema is never loaded. The fix is to key the guard on contentType.id rather than a boolean flag:

#loadedForId: string | null = null;

effect(() => {
    const contentType = this.$contentType();
    if (contentType && contentType.id !== this.#loadedForId) {
        this.#loadedForId = contentType.id;
        untracked(() => this.#loadFromMetadata(contentType));
    }
});

Fix this →


6. Spec tests hit the raw relative URL, not the interceptor-prefixed one

dot-style-editor-builder.component.spec.ts:45,274

const METADATA_URL = `v1/contenttype/id/${MOCK_CONTENT_TYPE.id}/metadata`;
// ...
httpController.expectOne(METADATA_URL);  // expects 'v1/...' not '/api/v1/...'

The spec does not include apiPrefixInterceptor, so it tests v1/... while production resolves to /api/v1/.... The interceptor is tested separately, so this is an acceptable trade-off, but the discrepancy is worth documenting in a comment so it doesn't confuse future maintainers.


Tests

7. Concurrency test is non-deterministic

ContentTypeResourceUpdateMetadataTest.java:334 — the test uses a CountDownLatch to release two threads simultaneously, but JVM thread scheduling does not guarantee they will actually interleave in the critical section. The test may pass on a single-core CI runner even without the lock. Consider adding a small artificial delay (e.g. Thread.sleep(1)) inside the lambda to widen the race window, or accept this as a "demonstrates intent" test with a comment explaining it.


No other correctness or security issues found. The lock logic, permission checks, error handling, and OpenAPI spec are all correct.

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

Comment thread dotCMS/src/main/java/com/dotcms/rest/api/v1/contenttype/ContentTypeResource.java Outdated
…ed lock

Moves the read-merge-save logic and DOT_STYLE_EDITOR_SCHEMA normalization
out of ContentTypeResource into ContentTypeHelper#mergeAndSaveMetadata.
The operation is now serialized per Content Type using a JVM-local striped
lock (DotConcurrentFactory.getIdentifierStripedLock), re-reading inside the
lock to prevent lost updates from concurrent PATCH requests on the same
Content Type. Adds a concurrency integration test that asserts both keys
survive when two threads patch simultaneously.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude

claude Bot commented Jun 11, 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
    • core-web/apps/dotcms-ui/src/app/portlets/shared/dot-content-types-edit/components/style-editor/dot-style-editor-builder.component.tsthis.#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
  • 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. The old PUT /v1/contenttype/id/{idOrVar} endpoint is preserved (not removed by this PR), so operators can restore full functionality by clearing the CDN cache and/or forcing a frontend refresh after rollback.

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

2 participants