Skip to content

send dotStyleProperties and Style Editor Schemas when Push Publish#35659

Merged
ihoffmann-dot merged 6 commits into
mainfrom
35658-style-editor-data-lost-during-push-publishing
May 12, 2026
Merged

send dotStyleProperties and Style Editor Schemas when Push Publish#35659
ihoffmann-dot merged 6 commits into
mainfrom
35658-style-editor-data-lost-during-push-publishing

Conversation

@dario-daza
Copy link
Copy Markdown
Member

@dario-daza dario-daza commented May 11, 2026

Proposed Changes

This PR fixes data-loss bugs that caused the Style Editor to be non-functional on the receiver after a push publish operation.

  1. DOT_STYLE_EDITOR_SCHEMA not included in the bundle

    • File: ContentTypeBundler.java
    • Fix: Replace the StructureTransformer call with a direct ContentTypeAPI.find() call, which reads the full ContentType from the database including the metadata JSONB column.
  2. dotStyleProperties not included in the bundle

    • File: ContentBundler.java
    • Fix: Read the style properties and add them to the map as a new HashMap<>() copy:
  3. dotStyleProperties ignored on the receiver

    • File: HandlerUtil.java
    • Fix: After reconstructing the MultiTree object, read dotStyleProperties from the map and set it:

Unit test added to check Push Publishing

A pure unit test (BundlerUtilUnitTest) was added to verify that dotStyleProperties and DOT_STYLE_EDITOR_SCHEMA survive the full XStream XML round-trip (serialize → deserialize) that happens between ContentBundler (sender) and ContentHandler (receiver).

This PR fixes: #35658

This PR fixes: #35658

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 11, 2026

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


PR Review — Style Editor data lost during Push Publishing

  • Read changed files
  • Analyze the 3 fix sites + new test
  • Flag risks / bad assumptions / edge cases
  • Post findings

The fix targets the right three sites (sender × 2, receiver × 1) and the unit test demonstrates the wire format survives both legs. A few things I'd want a second look at before merging:

1. Bundle-format compatibility (already raised by the rollback bot — confirm)

The XML bundle has no version marker and no receiver-side "unknown element" warning. Two new payload pieces ride along:

  • dotStyleProperties inside each multiTree entry (ContentBundler.java:408-411)
  • metadata (DOT_STYLE_EDITOR_SCHEMA) on the serialized ContentType (ContentTypeBundler.java:119)

A receiver still on N‑1 ignores both silently — the very failure mode this PR is fixing in reverse. The rollback comment from the bot describes the staged-rollout alternative; if that's not viable for this release, at minimum keep this in mind for the release notes (and for bundles queued in publishing_queue at upgrade/downgrade time).

2. ContentTypeBundler.java:119 — extra DB round-trip per content type

Old code: new StructureTransformer(structure).from() — in-memory transform of the already-cached Structure.
New code: APILocator.getContentTypeAPI(systemUser).find(structure.getInode()) — one DB lookup per content type in the bundle.

For typical bundles this is negligible; for bundles with hundreds of content types it's measurable. Worth checking whether StructureTransformer could be amended to populate metadata, or whether find() is genuinely the only path that returns the JSONB column.

3. MultiTree.setStyleProperties() is a quirky fluent API

MultiTree.java:144-147 both mutates this.styleProperties and returns a new MultiTree instance via the copy constructor. The handler uses it correctly:

multiTree = multiTree.setStyleProperties(new HashMap<>((Map<String, Object>) rawStyleProps));

…but if anyone later drops the reassignment (consistent with the surrounding multiTree.setX(...) style mutators above it), the saved instance would still have style props by accident — until someone fixes the setter. Consider either (a) a brief comment on the reassignment in HandlerUtil.java:141, or (b) fixing the setter to either be pure-fluent or pure-mutating, not both.

4. Receiver-side persistence of ContentType.metadata is implicit, not tested

ContentTypeHandler.saveOrUpdateContentType calls ContentTypeBuilder.builder(contentTypeIn).from(contentTypeIn)...build() and then typeAPI.save(...). The fix assumes .from(contentTypeIn) preserves metadata and that typeAPI.save() writes it to the JSONB column. The new unit test only covers the JSON round-trip (i.e. the wire layer) — it does not exercise the typeAPI.save path. An integration test that pushes a CT with a Style Editor schema and queries it on the receiver would convert this from "trust the immutables generator + save path" to "verified end-to-end."

5. Unit test gaps

The test verifies the happy path but doesn't cover:

  • The new defensive else if (rawStyleProps != null) warn-and-skip branch in HandlerUtil.java:142-146 (non-Map value)
  • Empty stylePropertiesUtilMethods.isSet on the sender side drops empty maps, so the receiver should never see the key. Worth asserting.
  • Non-default variant / non-dot:default personalization
  • MultiTree round-trip through HandlerUtil.setMultiTree(...) (i.e. the actual write path), not just serializer ↔ deserializer

6. Shallow copy of style properties (minor)

new HashMap<>(styleProps) in both ContentBundler.java:410 and HandlerUtil.java:141 is a shallow copy. Today PersonalizedContentlet.styleProperties is a Map.copyOf(...) of flat key/value pairs so this is safe. If the schema ever grows nested maps/lists, both legs would share mutable references with the source map. Probably fine to leave as-is, just be aware.

7. systemUser on find() (note, not a blocker)

The new find call runs as systemUser, which is correct for the bundler context (push-publish is system-level). Flagging only because it widens what the sender pulls — if metadata is ever extended with admin-only fields that shouldn't replicate, that decision needs to happen at the column level, not the API level.


Nothing here is a hard blocker; (1) and (2) are the two worth resolving or explicitly accepting before merge. (4) is the one I'd most like to see a follow-up integration test for.
· Branch: 35658-style-editor-data-lost-during-push-publishing

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 11, 2026

Pull Request Unsafe to Rollback!!!

  • Category: M-2 — Push Publishing Bundle Format Change
  • Risk Level: 🟡 MEDIUM
  • Why it's unsafe: The push-publish bundle XML schema is unversioned and has no receiver-side mismatch detection. This PR adds two new pieces of data to the bundle: dotStyleProperties inside each multiTree entry, and the ContentType metadata field (DOT_STYLE_EDITOR_SCHEMA) on the serialized ContentType. After a rollback (or in any mixed-version setup where sender is on N and a receiver is still on N-1), bundles generated by N will be processed by N-1 code that has no reader for these fields — N-1's HandlerUtil.setMultiTree() never calls setStyleProperties(), and N-1's ContentTypeBundler produces a ContentType with metadata = null because it still uses StructureTransformer. The Style Editor data is silently dropped on the receiver with no error surfaced — exactly the silent-corruption pattern called out in M-2. Bundles already queued in publishing_queue at rollback time are particularly at risk because they were generated by N and will be shipped post-rollback.
  • Code that makes it unsafe:
    • dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/remote/bundler/ContentBundler.java lines 406-409 — multiTreeMap.put("dotStyleProperties", new HashMap<>(styleProps)); adds a new XML element to the multiTree entry.
    • dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/remote/bundler/ContentTypeBundler.java line 119 — switching from new StructureTransformer(structure).from() to APILocator.getContentTypeAPI(APILocator.systemUser()).find(structure.getInode()) causes the serialized ContentType to carry the metadata JSONB field that was previously always null.
    • dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/remote/handler/HandlerUtil.java lines 137-139 — new receiver-side reader for dotStyleProperties. N-1 lacks this reader, so the new payload is silently discarded on rollback.
  • Alternative (if possible): Land the receiver-side handler changes (HandlerUtil.setStyleProperties and ContentTypeHandler metadata support) in an earlier release N-1, with the sender still emitting the old payload. Then in release N flip the sender (ContentBundler / ContentTypeBundler) to start emitting the new fields. That two-phase rollout guarantees that any receiver in the rollback window already knows how to consume the new XML elements, so a rollback or mixed-version push will not lose Style Editor data. If the bundle schema must change in a single release, consider adding an explicit bundle-format version marker and a receiver-side warning when it doesn't match, so silent data loss becomes a loud error.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 11, 2026

Pull Request Unsafe to Rollback!!!

  • Category: M-2 — Push Publishing Bundle Format Change
  • Risk Level: 🟡 MEDIUM
  • Why it's unsafe: The push-publish bundle XML schema is unversioned and has no receiver-side mismatch detection. This PR adds two new pieces of data to the bundle: dotStyleProperties inside each multiTree entry, and the ContentType metadata field (DOT_STYLE_EDITOR_SCHEMA) on the serialized ContentType. After a rollback (or in any mixed-version setup where sender is on N and a receiver is still on N-1), bundles generated by N will be processed by N-1 code that has no reader for these fields — N-1's HandlerUtil.setMultiTree() never calls setStyleProperties(), and N-1's ContentTypeBundler produces a ContentType with metadata = null because it still uses StructureTransformer. The Style Editor data is silently dropped on the receiver with no error surfaced — exactly the silent-corruption pattern called out in M-2. Bundles already queued in publishing_queue at rollback time are particularly at risk because they were generated by N and will be shipped post-rollback.
  • Code that makes it unsafe:
    • dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/remote/bundler/ContentBundler.java lines 406-409 — multiTreeMap.put("dotStyleProperties", new HashMap<>(styleProps)); adds a new XML element to the multiTree entry.
    • dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/remote/bundler/ContentTypeBundler.java line 119 — switching from new StructureTransformer(structure).from() to APILocator.getContentTypeAPI(APILocator.systemUser()).find(structure.getInode()) causes the serialized ContentType to carry the metadata JSONB field that was previously always null.
    • dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/remote/handler/HandlerUtil.java lines 137-139 — new receiver-side reader for dotStyleProperties. N-1 lacks this reader, so the new payload is silently discarded on rollback.
  • Alternative (if possible): Land the receiver-side handler changes (HandlerUtil.setStyleProperties and ContentTypeHandler metadata support) in an earlier release N-1, with the sender still emitting the old payload. Then in release N flip the sender (ContentBundler / ContentTypeBundler) to start emitting the new fields. That two-phase rollout guarantees that any receiver in the rollback window already knows how to consume the new XML elements, so a rollback or mixed-version push will not lose Style Editor data. If the bundle schema must change in a single release, consider adding an explicit bundle-format version marker and a receiver-side warning when it doesn't match, so silent data loss becomes a loud error.

@dario-daza dario-daza marked this pull request as ready for review May 12, 2026 15:20
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 12, 2026

Pull Request Unsafe to Rollback!!!

  • Category: M-2 — Push Publishing Bundle Format Change
  • Risk Level: 🟡 MEDIUM
  • Why it's unsafe: The push-publish bundle XML schema is unversioned and has no receiver-side mismatch detection. This PR adds two new pieces of data to the bundle: dotStyleProperties inside each multiTree entry, and the ContentType metadata field (DOT_STYLE_EDITOR_SCHEMA) on the serialized ContentType. After a rollback (or in any mixed-version setup where sender is on N and a receiver is still on N-1), bundles generated by N will be processed by N-1 code that has no reader for these fields — N-1's HandlerUtil.setMultiTree() never calls setStyleProperties(), and N-1's ContentTypeBundler produces a ContentType with metadata = null because it still uses StructureTransformer. The Style Editor data is silently dropped on the receiver with no error surfaced — exactly the silent-corruption pattern called out in M-2. Bundles already queued in publishing_queue at rollback time are particularly at risk because they were generated by N and will be shipped post-rollback.
  • Code that makes it unsafe:
    • dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/remote/bundler/ContentBundler.java lines 405-409 — multiTreeMap.put(STYLE_PROPERTIES_KEY, new HashMap<>(styleProps)); adds a new XML element to the multiTree entry.
    • dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/remote/bundler/ContentTypeBundler.java line 119 — switching from new StructureTransformer(structure).from() to APILocator.getContentTypeAPI(systemUser).find(structure.getInode()) causes the serialized ContentType to carry the metadata JSONB field that was previously always null.
    • dotCMS/src/enterprise/java/com/dotcms/enterprise/publishing/remote/handler/HandlerUtil.java lines 139-145 — new receiver-side reader for dotStyleProperties. N-1 lacks this reader, so the new payload is silently discarded on rollback.
  • Alternative (if possible): Land the receiver-side handler changes (HandlerUtil.setStyleProperties and ContentTypeHandler metadata support) in an earlier release N-1, with the sender still emitting the old payload. Then in release N flip the sender (ContentBundler / ContentTypeBundler) to start emitting the new fields. That two-phase rollout guarantees that any receiver in the rollback window already knows how to consume the new XML elements, so a rollback or mixed-version push will not lose Style Editor data. If the bundle schema must change in a single release, consider adding an explicit bundle-format version marker and a receiver-side warning when it doesn't match, so silent data loss becomes a loud error.

Copy link
Copy Markdown
Member

@wezell wezell left a comment

Choose a reason for hiding this comment

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

Nice and serializable

@ihoffmann-dot ihoffmann-dot added this pull request to the merge queue May 12, 2026
Merged via the queue into main with commit fbe3c20 May 12, 2026
51 checks passed
@ihoffmann-dot ihoffmann-dot deleted the 35658-style-editor-data-lost-during-push-publishing branch May 12, 2026 20:05
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

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Style Editor data lost during push publishing: schema and dotStyleProperties not transferred to receiver

3 participants