Fix silent message corruption from absent/blank channel scripts (#344) by e210 · Pull Request #351 · OpenIntegrationEngine/engine

e210 · 2026-07-16T07:41:30Z

Fixes the root cause behind #344: POST /api/channels accepts a channel without <preprocessingScript>; the channel deploys "healthy" but corrupts every message to the literal string undefined on the wire.

Root cause

Two independent defects in JavaScriptUtil (full analysis in #344):

JavaScriptBuilder.generateScript string-concatenates a null script into the JS wrapper, producing function doScript() { null }. compileAndAddScript caches it as a "custom" preprocessor because its body differs from the default script.
executePreprocessorScripts guards results with result != null, but Rhino's Undefined is not Java null — Context.jsToJava coerces it to the string "undefined", which becomes the processed raw and flows to every destination.

Changes

compileAndAddScript: a null/blank script is now treated as absent — evict from the compiled-script cache and return false (all callers already handle false as "no script").
executePreprocessorScripts: both result checks (global + channel) now ignore Undefined, the same idiom getPostprocessorResponse already uses.
New JavaScriptUtilTest: 5 tests exercising the real Rhino path (TDD — each fix developed against a failing test).

Behavior change note

A user preprocessor with no return statement previously corrupted the message body to "undefined"; it now passes the message through unchanged. This is footgun-removal: returning the literal string "undefined" as content is never intentional (return ''; and return null; behave as before).

Verification

Full :server:test suite green.
E2E on the patched build (the issue's exact repro XML, MLLP sink logging raw bytes):

Channel variant	Bytes on the wire (before)	Bytes on the wire (after)
Issue's incomplete XML as-is	`\x0b` `undefined` `\x1c\r`	intact HL7 message

The broader ask in #344 — structural validation/normalization in POST /api/channels — is deliberately out of scope here; it deserves its own design discussion. This PR removes the data corruption.

OpenIntegrationEngine#344) Signed-off-by: Ezio Caffi <ezio.caffi@gmail.com>

Signed-off-by: Ezio Caffi <ezio.caffi@gmail.com>

jonbartels

Holding my review until I hear from the original issue reporter - https://discord.com/channels/943670759891554316/943670760461987862/1528943487163044094

Looks good at a glance, but I'll let the OP verify it first

github-actions · 2026-07-21T01:59:32Z

Test Results

659 tests +5 659 ✅ +5 3m 38s ⏱️ +55s
109 suites +1 0 💤 ±0
109 files +1 0 ❌ ±0

Results for commit 7642566. ± Comparison against base commit a08c114.

MichaelLeeHobbs · 2026-07-21T04:04:40Z

Thanks for tracking this down — clean fix, and the root cause explains a symptom I couldn't (RAW channels corrupting too, since the preprocessor runs on the raw message before datatype handling). Nice test coverage on the real Rhino path.

Copilot

Pull request overview

Fixes silent message corruption caused by absent/blank channel preprocessor scripts by ensuring such scripts aren’t cached/treated as custom, and by preventing Rhino Undefined results from being coerced into the literal "undefined" message payload.

Changes:

Update preprocessor execution to ignore Rhino Undefined return values for both global and channel preprocessors.
Update compileAndAddScript to treat null/blank preprocessor scripts as absent (cache eviction + no compile).
Add Rhino-path unit tests covering null/blank script compilation behavior and Undefined handling.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
`server/src/main/java/com/mirth/connect/server/util/javascript/JavaScriptUtil.java`	Prevents `Undefined` from corrupting messages and adjusts script compilation/caching behavior for absent scripts.
`server/src/test/java/com/mirth/connect/server/util/javascript/JavaScriptUtilTest.java`	Adds tests to reproduce/guard the null/blank script + `Undefined` scenarios using real Rhino execution paths.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

e210 · 2026-07-25T17:38:13Z

+        ConfigurationController configurationController = mock(ConfigurationController.class);
+        when(controllerFactory.createConfigurationController()).thenReturn(configurationController);
+
+        ExtensionController extensionController = mock(ExtensionController.class);
+        when(controllerFactory.createExtensionController()).thenReturn(extensionController);
+
+        CodeTemplateController codeTemplateController = mock(CodeTemplateController.class);
+        when(controllerFactory.createCodeTemplateController()).thenReturn(codeTemplateController);


Mockito's default answer (ReturnsEmptyValues) returns empty collections — not null — for getCodeTemplates()/getLibraries()/getPluginMetaData(), so appendCodeTemplates() and generateGlobalSealedScript() no-op rather than NPE. The full suite (659 tests, real class ordering) runs green in CI. The @BeforeClass repair covers the ordering concern — now via the setControllersForTesting method added in 42d4c1f.

e210 · 2026-07-25T17:38:14Z

+        // A null or blank script does nothing; treat it as absent instead of
+        // compiling a wrapper whose body is the literal string "null" (MIRTH/OIE #344)
+        if (StringUtils.isBlank(script)) {
+            compiledScriptCache.removeCompiledScript(scriptId);
+            return false;
+        }


True in principle, but the only affected configs were already broken: a blank select script cannot be saved from the Administrator (REST-only — which is how this whole issue started). Before this PR every poll threw DatabaseReceiverException("…: undefined"); now it is an NPE from the same throw line, caught by the same catch. Same loud failure either way. Scoping the behavior per-context and compiling empty wrappers elsewhere would reintroduce the phantom-script executions this PR removes. Happy to add a null-guard to that error message in a follow-up if wanted.

e210 · 2026-07-24T17:10:48Z

Checked both Copilot notes:

Test mocks: Mockito's default returns empty collections, not null — so no NPE, the loops just no-op. The 659 green tests in CI agree.

Blank scripts (DatabaseReceiverScript): fair, but tiny: a blank select script is an already-broken channel (the Administrator won't save it). Before it failed every poll with DatabaseReceiverException("…: undefined"), now with an NPE from the same throw line, caught by the same catch. Happy to add a null-guard to that error message in a follow-up if wanted.

tonygermano

See inline comments. Also, please respond to the co-pilot suggestions.

tonygermano · 2026-07-21T14:08:20Z

            }

-            if (result != null) {
+            if (result != null && !(result instanceof Undefined)) {


This is the correct way to test for javascript undefined in java. There is one other incorrect usage in this file on line 333 which is not currently a part of this PR, but it would be great if you could fix that one, too, for consistency.

Suggested change

if (result != null && !(result instanceof Undefined)) {

if (result != null && !(Undefined.isUndefined(result))) {

Applied in 42d4c1f, including line 333 — and a fourth occurrence of the same pattern at line 132 (attachment script result handling), fixed for the same consistency reason.

tonygermano · 2026-07-21T14:09:18Z

            }

-            if (result != null) {
+            if (result != null && !(result instanceof Undefined)) {


Suggested change

if (result != null && !(result instanceof Undefined)) {

if (result != null && !(Undefined.isUndefined(result))) {

Applied in 42d4c1f.

tonygermano · 2026-07-24T16:09:40Z

+         * mirth.properties isn't on the unit test classpath, but JavaScriptScopeUtil's static init
+         * requires it to be resolvable via the context classloader. Point the context classloader
+         * at the real conf/ dir; restored after the class.
+         */
+        originalContextClassLoader = Thread.currentThread().getContextClassLoader();
+        URL confDir = new File("conf").toURI().toURL();


suggestion: I would prefer to see a copy of mirth.properties added as a test resource

Accessing Files directly from tests can be flaky depending on where the test is run from, and this file being referenced is not guaranteed to be static.

Done in 42d4c1f — mirth.properties copied to test resources, classloader shim removed entirely.

tonygermano · 2026-07-24T17:11:42Z

+         * earlier test class loaded it with a mocked factory that left them null, repair them so
+         * generateGlobalSealedScript/appendCodeTemplates don't NPE.
+         */
+        setJavaScriptBuilderStaticField("extensionController", extensionController);


suggestion: create package-private method for changing these values for testing

It seems a bit hacky to use reflection to change these values, but I understand why you did it. Refactoring JavaScriptBuilder to allow injectable dependencies is probably out of scope for this PR, but I think adding a method would be an acceptable compromise.

// In JavaScriptBuilder.java (from Guava, which should already be on the classpath) import com.google.common.annotations.VisibleForTesting; @VisibleForTesting static void setControllersForTesting(ExtensionController ec, CodeTemplateController ctc) { extensionController = ec; codeTemplateController = ctc; }

Done in 42d4c1f — one tweak: the method had to be public rather than package-private, since the test lives in server.util.javascript while the builder is in server.builders. @VisibleForTesting marks the intent.

@VisibleForTesting

…rationEngine#351) - Use Undefined.isUndefined() instead of instanceof, which also covers SCRIPTABLE_UNDEFINED; apply the same idiom to the postprocessor and attachment-script result handling for consistency - Load mirth.properties from test resources instead of pointing the context classloader at conf/ - Replace reflection on JavaScriptBuilder's static controllers with a @VisibleForTesting setter (public: the test lives in a different package) Signed-off-by: Ezio Caffi <ezio.caffi@gmail.com>

e210 added 2 commits July 16, 2026 09:48

Treat blank channel scripts as absent instead of compiling 'null' body (

86e4476

OpenIntegrationEngine#344) Signed-off-by: Ezio Caffi <ezio.caffi@gmail.com>

Ignore undefined preprocessor script results (OpenIntegrationEngine#344)

7642566

Signed-off-by: Ezio Caffi <ezio.caffi@gmail.com>

e210 force-pushed the fix/issue-344-undefined-preprocessor branch from c0fd2d2 to 7642566 Compare July 16, 2026 07:48

e210 marked this pull request as ready for review July 16, 2026 07:50

e210 requested review from a team, NicoPiel, gibson9583, jonbartels, kayyagari, kpalang, ssrowe and tonygermano July 16, 2026 07:50

jonbartels reviewed Jul 21, 2026

View reviewed changes

This was referenced Jul 21, 2026

REST POST /api/channels accepts a structurally-invalid channel; it deploys "healthy" but sends corrupt (undefined) content #344

Open

docs: add AGENTS.md (AI-contributor guardrails) + AGENTS_USER_EXAMPLE.md #343

Open

jonbartels previously approved these changes Jul 23, 2026

View reviewed changes

NicoPiel previously approved these changes Jul 23, 2026

View reviewed changes

mgaffigan requested a review from Copilot July 23, 2026 21:07

Copilot started reviewing on behalf of mgaffigan July 23, 2026 21:07 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

tonygermano requested changes Jul 24, 2026

View reviewed changes

e210 dismissed stale reviews from NicoPiel and jonbartels via 42d4c1f July 25, 2026 17:37

	if (result != null && !(result instanceof Undefined)) {
	if (result != null && !(Undefined.isUndefined(result))) {

Uh oh!

Conversation

e210 commented Jul 16, 2026

Root cause

Changes

Behavior change note

Verification

Uh oh!

jonbartels left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 21, 2026

Test Results

Uh oh!

MichaelLeeHobbs commented Jul 21, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

e210 commented Jul 24, 2026

Uh oh!

tonygermano left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants