feat: content patches — run-once content transformations in JavaScript - #697
Open
romain-pm wants to merge 7 commits into
Open
feat: content patches — run-once content transformations in JavaScript#697romain-pm wants to merge 7 commits into
romain-pm wants to merge 7 commits into
Conversation
…JavaScript Extracted from the JS server-extensions branch into its own PR for reviewability (part of EPIC #554). registerContentPatch declares versioned, run-once content transformations executed at module start, tracked by core's patch status store (persistent halt-on-failure barrier); guard-railed patch.* operations with low-level jcr.* access. Design: ADR-0006, CONTENT-PATCHES-PLAN.md. - ContentPatchRegistrar implements Registrar (engine SPI); no dependency on the extension-points mechanism, so this branches cleanly from main. - registerContentPatch + jcr/operations/types library API. - java-ts-bind: JCR mutation-method whitelist for the guard-railed operations. - Test-module fixtures (patchTest* CND types) + Cypress spec; registrar unit tests.
📝 Documentation GuidelinesThank you for contributing to our documentation! To ensure your contributions meet our standards, please review these resources:
This comment is posted automatically when changes are detected in the |
🦜 Chachalog
|
romain-pm
added a commit
that referenced
this pull request
Jul 23, 2026
Content patches are an independent feature (ContentPatchRegistrar implements the engine Registrar SPI, no dependency on the extension-points mechanism). Moving them to #697 shrinks this PR by ~2300 LOC and separates two review audiences. This also neutralizes the mislabeled commit 3b6c507: the content-patch files it swept in are no longer part of this branch's net diff. Removed here: registerContentPatch + jcr/operations/types, ContentPatchRegistrar (+support, tests), the content-patch guide/ADR-0006/plan docs, the Cypress spec, and the patchTest* test-module fixtures. Left in place intentionally (not content-patch-exclusive, referenced by other verticals or generally useful): the org.json/test dependencies in the engine-java pom, the JCR mutation-method entries in .java-ts-bind, and JcrHelper.doExecuteAsSystem.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces JavaScript/TypeScript “content patches”: versioned, run-once, ordered content transformations registered by JS modules and executed at module startup, with execution tracked in Jahia’s existing module patch status store.
Changes:
- Added a new library API (
registerContentPatch) with typed patch context (patch.*helpers +jcr.*escape hatch) and batching utilities. - Added a dedicated engine registrar (
ContentPatchRegistrar) that executes pending patches once per module start and records terminal results in/module-management→j:bundlesScripts. - Added end-to-end coverage via a test module fixture + Cypress assertions, and documented the feature (ADR + guide + plan/demo + changelog note).
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/cypress/e2e/engine/contentPatchTest.cy.ts | Cypress E2E spec asserting status-store results and transformed fixture content. |
| javascript-modules-library/src/index.ts | Exposes registerContentPatch and its public types from the library entrypoint. |
| javascript-modules-library/src/framework/contentPatches/types.ts | Defines the public TS types for patch declarations, context, operations, and JCR helpers. |
| javascript-modules-library/src/framework/contentPatches/registerContentPatch.ts | Implements patch registration into the server registry and sync-only execution adapter. |
| javascript-modules-library/src/framework/contentPatches/operations.ts | Implements guard-railed patch.* helpers (bulk property ops + node type changes/removal). |
| javascript-modules-library/src/framework/contentPatches/jcr.ts | Implements jcr.withSystemSession and jcr.forEachNode with batching + dry-run support. |
| javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/registrars/contentpatches/ContentPatchRegistrarTest.java | Unit tests for ordering, once-only behavior, halting on failure, and status persistence. |
| javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/registrars/contentpatches/ContentPatchSupport.java | Java support object exposed to JS patches (logger, module metadata, dry-run, nodetype ops). |
| javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/registrars/contentpatches/ContentPatchRegistrar.java | New registrar that runs pending patches at bundle start and records outcomes. |
| javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/JcrHelper.java | Adds doExecuteAsSystem helper for system-session JCR mutations from JS. |
| javascript-modules-engine-java/pom.xml | Adds test/support dependencies needed by new code/tests (e.g., JSON, Mockito, GraalJS for tests). |
| javascript-modules-engine-java/.java-ts-bind/package.json | Extends the method whitelist to type JCR mutation APIs needed for content patches. |
| jahia-test-module/src/react/server/extensions/contentPatches.ts | Test-module patch fixtures exercising success/skip/fail/halt semantics and operations. |
| jahia-test-module/settings/definitions.cnd | Adds CND definitions used by the content patch fixtures. |
| docs/adr/README.md | Adds ADR index with content-patches ADR entry. |
| docs/adr/0006-javascript-content-patches.md | ADR documenting the design and operational semantics for JS content patches. |
| docs/2-guides/6-content-patches/README.md | User guide describing how to write and run content patches. |
| CONTENT-PATCHES-PLAN.md | Detailed implementation plan and verified core behavior notes. |
| CONTENT-PATCHES-DEMO.md | Contract/demo document showing intended developer experience and API shape. |
| .chachalog/js-content-patches.md | Release note entry for the new content patches feature. |
Comments suppressed due to low confidence (8)
javascript-modules-library/src/framework/contentPatches/operations.ts:108
setPropertyValuespasseslocale.toString()into the user callback, which uses underscores for region locales (e.g. "en_US"). UsingtoLanguageTag()avoids surprising locale formats and matches typical language-tag inputs.
const value = resolveValue(node, locale.toString());
javascript-modules-library/src/framework/contentPatches/operations.ts:232
- In dry-run mode,
changeNodeTypecorrectly avoids unregistering, but it still logs "Node type … unregistered", which is misleading. Log a dry-run message whensupport.isDryRun()is true.
if (options.removeOldDefinition ?? true) {
support.unregisterNodeType(options.from);
log.info(`Node type ${options.from} unregistered`);
}
javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/registrars/contentpatches/ContentPatchRegistrarTest.java:125
- Typo in test method name: "ContentPatchs" → "ContentPatches".
public void alreadyRecordedContentPatchsNeverRunAgain() {
javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/registrars/contentpatches/ContentPatchRegistrarTest.java:135
- Typo in test method name: "ContentPatchs" → "ContentPatches".
public void failedContentPatchsAreRecordedAsTerminal() {
javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/registrars/contentpatches/ContentPatchRegistrarTest.java:145
- Typo in test method name: "ContentPatchs" → "ContentPatches".
public void failureHaltsTheModulesRemainingContentPatchs() {
javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/registrars/contentpatches/ContentPatchRegistrarTest.java:169
- Typo in test method name: "ContentPatchs" → "ContentPatches".
public void clearingAFailedRecordReleasesTheHeldContentPatchs() {
javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/registrars/contentpatches/ContentPatchRegistrarTest.java:200
- Typo in test method name: "ContentPatchs" → "ContentPatches".
public void autoRunDisabledLeavesContentPatchsPending() {
javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/registrars/contentpatches/ContentPatchRegistrarTest.java:233
- Typo in test method name: "ContentPatchs" → "ContentPatches".
public void modulesWithoutContentPatchsAreANoOp() {
Comment on lines
+83
to
+87
| if (result && typeof (result as PromiseLike<unknown>).then === "function") { | ||
| throw new Error( | ||
| `Content patch returned a promise: content patches must be synchronous (do not use an async run function)`, | ||
| ); | ||
| } |
| } catch { | ||
| siteLocales = existingLocales(node); | ||
| } | ||
| return locales ? siteLocales.filter((l) => locales.includes(l.toString())) : siteLocales; |
Comment on lines
+168
to
+170
| support.unregisterNodeType(options.nodeType); | ||
| log.info(`Node type ${options.nodeType} unregistered`); | ||
| return report; |
Comment on lines
+79
to
+99
| NodeTypeRegistry registry = NodeTypeRegistry.getInstance(); | ||
| if (!registry.hasNodeType(name)) { | ||
| getLogger(bundle.getSymbolicName()).info( | ||
| "Node type {} is not registered on this instance, nothing to unregister", name); | ||
| return; | ||
| } | ||
| ExtendedNodeType type; | ||
| try { | ||
| type = registry.getNodeType(name); | ||
| } catch (NoSuchNodeTypeException e) { | ||
| return; // raced away, nothing to do | ||
| } | ||
| if (!bundle.getSymbolicName().equals(type.getSystemId())) { | ||
| throw new IllegalArgumentException("Node type " + name + " is owned by '" + type.getSystemId() | ||
| + "', not by this module ('" + bundle.getSymbolicName() | ||
| + "') — content patches may only remove their own definitions"); | ||
| } | ||
| if (dryRun) { | ||
| getLogger(bundle.getSymbolicName()).info("[dry-run] would unregister node type {}", name); | ||
| return; | ||
| } |
Comment on lines
+40
to
+42
| ## The `migrate.*` helpers | ||
|
|
||
| Every helper iterates both the `default` and `live` workspaces (override with `workspaces`), commits in batches (`batchSize`, default 100), handles internationalized properties on their translation subnodes, logs progress, and no-ops gracefully when the node type was never registered on this instance (fresh installs). |
| javascript-modules: minor | ||
| --- | ||
|
|
||
| New: content patches — versioned, run-once content transformations declared in JavaScript with `registerContentPatch`, tracked by Jahia's patch status store so each runs exactly once per environment. Guard-railed operations (`patch.*`) with low-level `jcr.*` access as an escape hatch. |
| } | ||
|
|
||
| @Test | ||
| public void runsPendingContentPatchsInNameOrderAndRecordsResults() { |
cy.apollo logs query.loc.source.body, so it requires a parsed gql document; passing raw strings made all 5 contentPatchTest specs fail with "Cannot read properties of undefined (reading 'source')". Use queryFile fixtures like the other specs.
GauBen
reviewed
Aug 12, 2026
GauBen
left a comment
GauBen
left a comment
Member
There was a problem hiding this comment.
Interesting design, cool ideas
My biggest take away is that we should make this API in Java and expose it to both JS and Groovy scripts, because we only widen the gap with a JS-first implementation
While I agree there is a need for this it still doesn't make it easier on the CND-side to understand the migration process
…allback skip() throws ContentPatchSkipped, but when called inside jcr.withSystemSession or jcr.forEachNode the throw crosses a host boundary: doExecuteAsSystem wrapped every exception in a new IllegalStateException, destroying the guest exception's identity, so the adapter recorded .failed (a persistent barrier) instead of .skipped — exactly the guide's documented 'nothing to fix on this instance' pattern. Rethrow unchecked exceptions unchanged so GraalVM restores the guest identity, make skip detection flag-based so no wrapping can break it again, and point the 07-skip-me e2e fixture at the nested pattern as a regression test.
…fter removeNodeType and changeNodeType ran their batched (and committed) content mutations first and only hit the ownership check inside unregisterNodeType afterwards — so a patch targeting another module's type destroyed or retyped its content before being refused, and with removeOldDefinition: false the check never ran at all. Extract the systemId check into ContentPatchSupport.assertOwnedNodeType, call it on 'from' and 'to' before any forEachNode, and keep the unregisterNodeType check as defense in depth. Also stop setPropertyValues from persisting empty translation subnodes when the value callback returns undefined: resolve the value before getOrCreateI18N.
…failures isDryRun() was exposed and documented across the API but nothing could ever set it: wire a dryRun property on the existing configuration PID — patches then execute without persisting and without recording, staying pending. And when storing a result fails after a patch executed, log explicitly that the run-once guarantee is broken (the patch will re-run) and halt the module's remaining patches instead of dying silently in the outer catch.
- ADR referenced MIGRATIONS-*.md working docs (renamed CONTENT-PATCHES-*.md) and the registry type 'content patch' (actual: 'content-patch') - the guide compared registerContentPatch to registerNodeLegacyAction, which only exists on the server-extensions branch; document the new dryRun config - drop the unused org.graalvm.js:js test dependency (no test uses it here) - warn that unpadded numbers break lexicographic patch ordering at 10+
8 tasks
…hared by TS and Groovy Per the #697 review and the re-layering plan in #725: the guard-railed operations, the forEachNode batching engine, i18n handling, definition guard rails and dry-run now live once in Java, in the engine's exported contentpatches package (no GraalVM or engine-internal types in its signatures). The TypeScript patch.*/jcr.* surface is unchanged but became a thin typed facade; Groovy patch scripts and Java modules reach the same engine through the new ContentPatchService OSGi service. Parity between the two faces is executable: contentPatchGroovyParityTest runs the same operations as the JS fixtures from a Groovy script (on mirrored patchTestGroovy* types) and asserts the same outcomes. - 22 new unit tests on the operations engine (51 engine-java tests green) - library tsc + publint green; reactor verify green - profiler embedded alongside the explicit Export-Package (same fix as the SDK export on feature/js-server-extensions, whose export list this one will merge with) - fixes the dry-run 'unregistered' log line flagged in review
commit: |
This was referenced Aug 21, 2026
Open
Author
|
@GauBen — the branch has been reworked along your review. Here's the map of what's new, so the re-review can be quick.
Done, in a single commit: Suggested review order:
Deliberately unchanged: the published TS API, the lifecycle semantics (run-once, ordering, halt-on-failure, shared status store), and the original e2e assertions. Fixed in passing: the dry-run "Node type … unregistered" log line.
Agreed — that's now its own workstream instead of a hand-wave: #729 (a CND-change → operation docs matrix; a The full re-layering plan and phase tree live in #725: P1 (Java framework) and P2 (TS façade) are this PR; P3 Groovy ergonomics #727, P4 admin GraphQL API #728 (scope under discussion), P5 CND comprehension #729, optional developer CLI #731, core adoption #730. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of EPIC #554. Split out of #687 into its own PR for reviewability (the two features are independent — this branches cleanly from
main).What
registerContentPatchlets a JS module declare versioned, run-once content transformations executed at module start, tracked by Jahia's patch status store so each patch runs exactly once per environment (persistent halt-on-failure barrier). Guard-railed operations (patch.*) with low-leveljcr.*access as an escape hatch.Design & rationale: ADR-0006 (
docs/adr/0006-javascript-content-patches.md),CONTENT-PATCHES-PLAN.md.Reworked per review: Java-first layering
Following the review feedback (make the API Java-first, expose it to Groovy and JS alike — plan in #725), the operations engine now lives once, in Java: the
forEachNodebatching engine, the fivepatch.*operations, i18n handling, definition guard rails and dry-run moved into the engine-exportedorg.jahia.modules.javascript.modules.engine.contentpatchespackage (no GraalVM or engine-internal types in its signatures — same export discipline as the SDK package in #687, whoseExport-Packagelist this one will merge with,profilerembed included).patch.*/jcr.*surface is byte-for-byte unchanged — the library is now a thin typed façade over the Java engine (callbacks coerce to functional interfaces,undefinednormalizes tonull).ContentPatchServiceOSGi service (BundleUtils.getOsgiService(...), named arguments, trailing closures) — see the new "The same engine from Groovy and Java" section of the guide.contentPatchGroovyParityTest.cy.tsruns the same operations as the JS fixtures from a Groovy script, on mirroredpatchTestGroovy*types, and asserts the same outcomes — the DevEx/feature-parity comparison between the two bridges is a test, not a promise.Notes for reviewers
ContentPatchRegistrar implements Registrar(the pre-existing engine SPI) — no dependency on the extension-points mechanism in feat: JS server extension points, client-callable actions, and the JSServerExtensionInvoker SDK #687, hence a standalone PR frommain.getRealNode().setPrimaryType()retype handling and property remapping around retypes (documented in the plan).JcrHelper.doExecuteAsSystem(system-session helper) and the JCR mutation-method entries in.java-ts-bindare introduced here — they back thepatch.*/jcr.*operations.Verification
ContentPatchRegistrarTest) + 22 operations-engine unit tests (ContentPatchOperationsImplTest) — engine-java suite green (51 tests)tsc+publint, test-module build (content patches register in the server bundle), full reactormvn verifyExport-Package: …contentpatches;uses:="javax.jcr,org.jahia.services.content,org.slf4j"contentPatchTest.cy.ts(TS bridge, lifecycle + operations) andcontentPatchGroovyParityTest.cy.ts(Groovy bridge, same operations)Tracking
Closes #695. Implements the P1/P2 phases (and the parity slice of P3) of #725.