Fix silent message corruption from absent/blank channel scripts (#344)#351
Fix silent message corruption from absent/blank channel scripts (#344)#351e210 wants to merge 2 commits into
Conversation
OpenIntegrationEngine#344) Signed-off-by: Ezio Caffi <ezio.caffi@gmail.com>
Signed-off-by: Ezio Caffi <ezio.caffi@gmail.com>
c0fd2d2 to
7642566
Compare
jonbartels
left a comment
There was a problem hiding this comment.
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
|
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. |
There was a problem hiding this comment.
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
Undefinedreturn values for both global and channel preprocessors. - Update
compileAndAddScriptto treat null/blank preprocessor scripts as absent (cache eviction + no compile). - Add Rhino-path unit tests covering null/blank script compilation behavior and
Undefinedhandling.
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.
| 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); |
| // 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; | ||
| } |
Fixes the root cause behind #344:
POST /api/channelsaccepts a channel without<preprocessingScript>; the channel deploys "healthy" but corrupts every message to the literal stringundefinedon the wire.Root cause
Two independent defects in
JavaScriptUtil(full analysis in #344):JavaScriptBuilder.generateScriptstring-concatenates anullscript into the JS wrapper, producingfunction doScript() { null }.compileAndAddScriptcaches it as a "custom" preprocessor because its body differs from the default script.executePreprocessorScriptsguards results withresult != null, but Rhino'sUndefinedis not Javanull—Context.jsToJavacoerces 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 returnfalse(all callers already handlefalseas "no script").executePreprocessorScripts: both result checks (global + channel) now ignoreUndefined, the same idiomgetPostprocessorResponsealready uses.JavaScriptUtilTest: 5 tests exercising the real Rhino path (TDD — each fix developed against a failing test).Behavior change note
A user preprocessor with no
returnstatement 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 '';andreturn null;behave as before).Verification
:server:testsuite green.\x0bundefined\x1c\rThe 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.