Skip to content

W-23599705: Add external DataWeave module support to Node.js binding - #154

Merged
mlischetti merged 29 commits into
masterfrom
nodejs-external-modules
Aug 6, 2026
Merged

W-23599705: Add external DataWeave module support to Node.js binding#154
mlischetti merged 29 commits into
masterfrom
nodejs-external-modules

Conversation

@mlischetti

@mlischetti mlischetti commented Aug 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Adds support for resolving external DataWeave modules (not compiled into the native image) from the Node.js binding (@dataweave/native), via a resolveModule callback bridged through the C N-API addon down to the Java WeaveResourceResolver SPI.

  • New resolveModule option on DataWeaveOptions, consumed by new DataWeave({ resolveModule })
  • Four resolver factories: modulesFromMap, modulesFromDirectory, modulesFromJars (async), composeResolvers (first-match-wins fallback chain)
  • Native callback infrastructure spanning Java (ScriptRuntime, CallbackWeaveResourceResolver, new @CEntryPoints) and the C addon (resolve_module_callback, napi_run_with_resolver)
  • Six previously excluded TCK conformance cases re-enabled via a committed fixture module + modulesFromDirectory
  • User-facing docs: native-lib/node/docs/external-modules.md (quick start, resolver factories, error handling, trust model) plus a README section

Key constraints and known limitations (documented)

  • One resolver per process — the underlying script engine is a process-wide singleton; a second DataWeave instance with a different resolver logs a warning and keeps the first. Workaround: composeResolvers(). The root cause (hard-singleton ScriptRuntime + single-isolate Node addon) is tracked separately for an architectural fix in W-23692110; this PR documents the limitation and adds isolated-process regression coverage for the current behavior, but does not change the underlying design.
  • Resolver must be synchronous (no async/await inside the callback itself; async setup like modulesFromJars happens before construction).
  • runStreaming()/runTransform() do not honor resolveModule — they execute on a background thread, and wiring a resolver callback there would be an N-API thread-affinity violation (see below). This is called out explicitly in the docs.
  • Resolver code runs with full process permissions — no sandboxing. Documented in external-modules.md, the README, and DataWeaveOptions.resolveModule JSDoc.

Process notes

Built via subagent-driven development: 12 planned tasks, each implemented and independently reviewed with a fix-loop, followed by a final whole-branch review. That final review caught a Critical N-API thread-affinity hazard — resolve_module_callback in addon.c was reachable from the background thread runStreaming/runTransform spawn (since they share the same singleton engine once a resolver is installed by any prior run() call), and would call back into a napi_env/napi_ref captured on a different OS thread — undefined behavior, not the documented clean fallback. Fixed with a thread-identity guard (uv_thread_self()/uv_thread_equal()) that fails closed instead of crashing, plus a regression test and trip-wire comments on the two intentionally-unwired resolver entrypoints for streaming/transform.

Code-review remediation (two independent reviews)

  • Allocator mismatch (native crash/corruption risk): napi_run_with_resolver freed a GraalVM UnmanagedMemory-allocated result with libc free() instead of fn_free_cstring. Fixed to strdup + fn_free_cstring while the thread is still attached, matching the pattern used elsewhere in addon.c.
  • Vulnerable dependency: adm-zip bumped ^0.5.10 → ^0.6.0 (patches GHSA-xcpc-8h2w-3j85, a DoS via crafted archive); npm audit --omit=dev is clean.
  • Symlink escape: modulesFromDirectory now canonicalizes both the base directory and each candidate path (fs.realpathSync) and re-checks containment against the canonical paths, closing a gap where an in-tree symlink could resolve outside baseDir. Covered by a new regression test.
  • Secret-leaking logs: resolver-callback exceptions in addon.c now log a fixed, content-free diagnostic by default; the previous verbose message/stack logging (which could include module source, credentials, or paths) is opt-in via DATAWEAVE_RESOLVER_DEBUG=1.
  • Packaging: added docs/ to package.json's files, so the README's link to docs/external-modules.md resolves in the published tarball (verified via npm pack --dry-run).
  • Minor fixes: modulesFromMap now uses Object.hasOwn instead of in (avoids matching inherited properties like toString); the multi-JAR unit test now uses two distinct archives and asserts both resolve; integration tests now track and clean up every explicit DataWeave instance instead of leaking native references.

Test plan

  • native-lib/node: npm test — 844 passed, 59 skipped (unit + integration + TCK), 0 failed
  • ./gradlew native-lib:test native-cli:test — BUILD SUCCESSFUL, all tests green
  • Native library and Node addon rebuilt from source and exercised by the above
  • Verified the thread-affinity crash reproduces without the guard and is fixed cleanly with it
  • npm audit --omit=dev — 0 vulnerabilities
  • npm pack --dry-run — tarball includes docs/external-modules.md

🤖 Generated with Claude Code

@mlischetti
mlischetti requested a review from a team as a code owner August 5, 2026 18:58
@mlischetti mlischetti changed the title Add external DataWeave module support to Node.js binding @W-23599705: Add external DataWeave module support to Node.js binding Aug 5, 2026
@mlischetti mlischetti changed the title @W-23599705: Add external DataWeave module support to Node.js binding W-23599705: Add external DataWeave module support to Node.js binding Aug 5, 2026
mlischetti and others added 26 commits August 5, 2026 19:06
@mlischetti @claude
feat(native-lib): add callback-based module resolver infrastructure
d7e6eb6 
Add ResolveModuleCallback function pointer interface and
CallbackWeaveResourceResolver implementation that delegates to C callback.
Foundation for external module support in Node.js/Python bindings.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@mlischetti @claude
fix(native-lib): avoid duplicate path conversion in error logging
bb523bc 
Move path variable outside try block to reuse in catch block error message.
Avoids extra work and potential failure if exception was thrown during conversion.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@mlischetti @claude
feat(native-lib): add resolver support to ScriptRuntime
1218bf4 
Add setResolver() method to install callback-based module resolver.
Engine rebuilt with CompositeWeaveResourceResolver(ClassLoader, Callback)
to preserve built-ins while adding user modules. One resolver per process.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@mlischetti @claude
fix(native-lib): address thread-safety and API issues in ScriptRuntime
c076d81 
Critical fixes:
- Add synchronized to setResolver() and volatile to resolver/engine fields
- Fix field ordering: move static resolver before instance engine
- Add synchronized block when rebuilding engine to prevent race conditions
- Copy volatile resolver to local var in compositeResolver() for safe access

Important fixes:
- Use ParserConfigurationBuilder().build() to match native-cli pattern
- Remove incorrect reference to nonexistent composeResolvers() method
- Add javadoc about thread-safe callback requirement
- Update constructor comment for accuracy

Minor improvements:
- Remove unused Seq/HashMap imports
- Improve warning messages

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@mlischetti @claude
feat(native-lib): add resolver-aware FFI entrypoints
0298427 
Add run_script_with_resolver, run_script_callback_with_resolver,
and run_script_input_output_callback_with_resolver. Each accepts
ResolveModuleCallback and installs resolver before delegating to
existing execution logic.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@mlischetti @claude
feat(node): add resolver callback bridge in C addon
1855015 
Implement resolve_module_callback using napi_threadsafe_function pattern.
Background thread blocks while main thread executes JS resolver, returns
source string. Add runWithResolver N-API method. Matches ReadCallback pattern.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@mlischetti @claude
fix(node): resolve memory leaks, race conditions, and resource cleanu…
5e4e9b8 
…p in resolver bridge

Fix 4 Important issues from code review:

1. Memory leak: Free resolver result_source after native copies it
   - Added cleanup in napi_run_with_resolver after fn_run_script_with_resolver call
   - Prevents leak on every module resolution

2. Missing cleanup: Destroy resolver resources on module unload
   - Added g_resolver_data cleanup in napi_cleanup
   - Releases threadsafe function, destroys mutex/cond, frees memory

3. Race condition: Protect g_resolver_data initialization with mutex
   - Wrapped initialization check/alloc in g_mutex lock
   - Added error handling for malloc/threadsafe function creation failures

4. NULL checks: Validate all malloc/strdup returns
   - Added checks after script/inputs/mime_type allocations
   - Added check after strdup(module_path) in resolve_module_callback
   - Free partial allocations on error

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@mlischetti @claude
docs(node): document one-resolver-per-process constraint in addon
63b07d2 
Add comments clarifying that resolver is initialized once per process
lifetime and subsequent calls with different callbacks will reuse the
first resolver. This makes the singleton constraint explicit, matching
the ScriptRuntime.setResolver() enforcement on the native side.

Addresses code review finding #4 about implicit global resolver behavior.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@mlischetti @claude
feat(node): add modulesFromMap resolver factory
bdacf81 
Implement in-memory map-based resolver with ModuleResolver type.
Foundation for resolver module. Returns null on miss, logs at debug level.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@mlischetti @claude
fix(node): remove side effect from modulesFromMap, add empty string test
826b59f 
- Remove console.debug() call to make resolver a pure function
- Use 'in' operator for cleaner existence check
- Add test case for empty string source (valid edge case)

Addresses code review feedback on resolver purity.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@mlischetti @claude
feat(node): add modulesFromDirectory resolver factory
120d13f 
Implement directory-based resolver with recursive namespace support.
Reads from disk on every access (no cache). Throws on read errors,
returns null on not-found.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@mlischetti @claude
fix(node): security and robustness improvements to modulesFromDirectory
3a7a85d 
Security fixes:
- Add path traversal protection (resolve paths and validate within baseDir)
- Replace error type casting with proper Error type guard

Performance and robustness improvements:
- Remove TOCTOU race by checking ENOENT in catch block instead of existsSync
- Remove console.debug on cache misses (expected behavior, too verbose)
- Proper handling of NodeJS.ErrnoException for ENOENT check

Add test case for path traversal prevention.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@mlischetti @claude
feat(node): add modulesFromJars resolver factory
67db800 
Add adm-zip dependency and implement JAR-based resolver. Extracts all
.dwl files from JARs into in-memory map. Returns Promise (async extraction)
but resolver itself is synchronous. Throws on invalid ZIP.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@mlischetti @claude
fix(node): improve error handling in modulesFromJars
5333928 
Use proper type guard for error handling to match modulesFromDirectory
pattern. Prevents undefined in error messages when error lacks .message.
Also add @types/adm-zip dev dependency for TypeScript support.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@mlischetti @claude
feat(node): add composeResolvers factory
8b591fc 
Implement composite resolver with fallback chain. Tries each resolver
in order, returns first non-null. Completes resolver.ts module with all
four factories: modulesFromMap, modulesFromDirectory, modulesFromJars, composeResolvers.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@mlischetti @claude
feat(node): add TypeScript FFI bindings for resolver
4e23dd4 
Add runWithResolver function to ffi.ts that wraps the C addon's
napi_run_with_resolver method. Import ModuleResolver type from resolver.ts.
Binds basic script execution with module resolution callback.

Note: Streaming variants (runStreamingWithResolver, runInputOutputWithResolver)
were not implemented in Task 4's C addon and are deferred to future work.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@mlischetti @claude
feat(node): integrate resolver into DataWeave class
f3b82a2 
Add resolveModule to DataWeaveOptions. Route run() to runWithResolver
when resolver present, standard run otherwise. Backward compatible
(resolver is optional). Export resolver factories from main module.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@mlischetti @claude
fix(node): correct throw test and document resolver singleton caveat
6892fa9 
Test 'throws when module not found' now passes raiseOnError:true, since
run() only throws when explicitly requested (matches existing pattern
elsewhere). Also document that the native layer installs only the first
resolver registered per process; later DataWeave instances with a
different resolveModule silently reuse it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@mlischetti @claude
fix(native-lib): fix native-image build errors and resolver bugs bloc…
f88a70a 
…king verification

ScriptRuntime.setResolver() and CallbackWeaveResourceResolver's constructor
compared a GraalVM Word type (CFunctionPointer) to null with ==, which
native-image rejects at build time ("Should not compare Word to Object in
condition"). Use callback.isNull() instead, blocking all native builds that
exercise the resolver path.

The Node addon's run_script_with_resolver_fn typedef declared 5 parameters
(including an unused mimeType) while the actual @centrypoint signature takes
4, misaligning the call and corrupting the resolver callback function
pointer (observed as a SIGBUS). Fixed the typedef/call site to match.

The resolver callback bridge used napi_threadsafe_function, a pattern for a
background thread handing work to the JS main thread. runWithResolver's
native call is synchronous on the calling (JS) thread, so the callback fired
on that same thread and deadlocked waiting on its own queued work. Replaced
it with a direct napi_ref + napi_call_function bridge, and fixed a related
leak where only one resolver result buffer was tracked/freed per call even
though a single script can trigger multiple resolver invocations.

Finally, NameIdentifierHelper.toWeaveFilePath (Java) always renders module
paths with a leading separator (e.g. "/org/test/lib.dwl"), but the addon
forwarded this to JS unmodified. modulesFromDirectory tolerated it via
path.join, but modulesFromMap's exact-key lookup silently failed. Strip the
leading '/' in resolve_module_callback before invoking the JS resolver.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@mlischetti @claude
feat(node): re-enable TCK module import tests via directory-backed re…
028267b 
…solver

Install a shared resolver on the TCK harness's DataWeave instance backed by
modulesFromDirectory, pointing at a committed fixtures/ tree. The six cases
below import org::mule::weave::v2::libs::lib, a module that lives only in
the private data-weave runtime repo's test resources and isn't in any
published artifact or TCK zip — add a minimal fixture reproducing just the
parts these cases exercise (name binding, dw::Core re-export, a function)
and remove the corresponding ignore-list entries:

  full-qualified-name-ref-out.json
  import-component-alias-lib-out.json
  import-lib-out.json
  import-lib-with-alias-out.json
  import-named-lib-out.json
  import-star-out.json

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@mlischetti @claude
fix(node): log message and stack when resolver callback throws
72e713e 
resolve_module_callback caught the JS resolver's exception and cleared it
(correctly, to avoid leaking a pending exception into the next N-API call),
but only logged a generic "Resolver callback threw exception" string —
message and stack were discarded. The design spec's FFI Boundary Errors
section requires logging the exception with its stack trace, and the
read-callback bridge nearby already does this correctly. Mirror that same
napi_get_named_property extraction of message/stack here so a thrown
resolver error is actually diagnosable instead of silently swallowed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@mlischetti @claude
docs(node): add external module support documentation
1225dc3 
Add external-modules.md with quick start, resolver factories, error
handling, and JAR dependency management. Update main README with
external modules section.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@mlischetti @claude
docs(node): fix external modules documentation accuracy
c4a23bc 
Address reviewer findings:
1. Correct File I/O Errors section: resolver exceptions are caught
   internally and logged to stderr only; result.error is the same
   generic "Unable to resolve module" message as module-not-found,
   not distinguishable via the result object.
2. Add explicit disclosure: module-level singleton functions cannot
   be configured with resolveModule; users must construct their own
   DataWeave instance. Clarify in both docs and README.
3. Fix heading structure: move External Modules under API Reference
   as subsection (###) instead of top-level section (##).

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@mlischetti @claude
fix(native-lib): guard resolver callback against background-thread in…
75cbab0 
…vocation

ScriptRuntime's DataWeave engine is a process-wide singleton: once any run()
call installs a resolveModule callback, that same composite resolver (built-
ins + callback) is shared by every later execution path -- including
runStreaming()/runTransform(), whose native calls execute on a background
uv_thread (see addon.c's streaming_thread_fn/transform_thread_fn), not the JS
thread that registered the resolver. addon.c's resolve_module_callback had no
check that it was running on the thread that owns g_resolver_env/g_resolver_ref,
so a streamed/transformed script importing a non-built-in module could call
back into napi from the wrong OS thread -- undefined behavior, reproduced here
as a fatal V8 HandleScope crash.

Fix: record the OS thread that installs the resolver (g_resolver_thread, set
alongside g_resolver_env in napi_run_with_resolver) and check it at the top of
resolve_module_callback. Off-thread invocations now return NULL ("not found")
instead of touching napi, which matches the already-documented built-ins-only
fallback for streaming/transform but makes it safe instead of merely assumed.

Also in this pass:
- Regression test (dataweave-resolver.test.ts) that installs a resolver via
  run() then exercises runStreaming() against a distinct, never-before-
  resolved module path (DataWeave caches resolved modules by name at the
  process-lifetime engine level, so reusing an already-resolved path would
  silently skip the resolver chain and not exercise the guard). Verified this
  test crashes the process before the addon.c guard and passes cleanly after.
- Trust-model documentation: resolveModule runs with full process permissions
  and no sandboxing (same as the CLI resolving .dwl files from disk) --
  documented in external-modules.md, README.md, and the DataWeaveOptions
  JSDoc.
- Trip-wire comments on the unwired run_script_callback_with_resolver /
  run_script_input_output_callback_with_resolver entrypoints (NativeLib.java
  and their addon.c uv_dlsym lookups), explaining why they're intentionally
  not used by runStreaming()/runTransform().

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@mlischetti @claude
fix(node): skip chmod-based unreadable-file test on Windows
0d81dae 
Windows NTFS ACLs don't respect Unix chmod 000 for denying read access,
so fs.readFileSync succeeds where the test expects it to throw. Skip
this platform-specific test on win32 to fix CI.

Ref: https://github.com/mulesoft/data-weave-cli/actions/runs/31037261526/job/92412368635

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@mlischetti @claude
refactor(native-lib): migrate to modern DataWeave runtime builder API
e8768a3 
- Use DWScriptingEngine.builder() instead of deprecated DataWeaveScriptingEngine constructor
- Use DWModuleComponentsFactory.createSimpleDWModuleComponentsFactoryBuilder() for cleaner resolver setup
- Extract createModuleComponentsFactory() helper to eliminate duplication between constructor and setResolver()
- Clean up CallbackWeaveResourceResolver imports and formatting
- Type-safe Scala empty Seq construction with explicit cast

No behavioral changes — purely API modernization.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@mlischetti
mlischetti force-pushed the nodejs-external-modules branch from 2514126 to e8768a3 Compare August 5, 2026 22:07
mlischetti and others added 3 commits August 6, 2026 12:17
@mlischetti
fix(node): address code-review findings on external-module resolver s…
4aac0d0 
…upport

Fixes the allocator mismatch (libc free() on a GraalVM-allocated result,
addon.c) that could corrupt/crash the process, upgrades adm-zip to patch
GHSA-xcpc-8h2w-3j85, closes a symlink-escape hole in modulesFromDirectory,
stops resolver-error logging from leaking secrets to stderr by default
(opt-in via DATAWEAVE_RESOLVER_DEBUG=1), fixes a prototype-chain lookup bug
in modulesFromMap, includes docs/ in the published npm package so the
README's external-modules link resolves, and cleans up leaking native
references and a no-op multi-JAR test in the test suite. Also documents
(and adds isolated-process regression coverage for) the process-wide
first-resolver-wins behavior — the underlying single-isolate/single-resolver
architecture is tracked separately in W-23692110.
@mlischetti
docs(node): correct stale resolver-error docs, note symlink-swap TOCTOU
f8845f3 
Follow-up to the revalidation review of commit 4aac0d0:

- The "File I/O Errors" section still said resolver failure details are
  always logged to stderr; that's now only true when the caller opts in
  via DATAWEAVE_RESOLVER_DEBUG=1 (addon.c). Update the text and debugging
  note accordingly.
- Document that modulesFromDirectory's symlink-escape check closes the
  stable-symlink case but not a time-of-check/time-of-use race (no
  portable openat2(RESOLVE_BENEATH|RESOLVE_NO_SYMLINKS) equivalent in
  Node), and that the module tree must not be writable by less-trusted
  principals.

Process-wide resolver isolation (the other Medium finding from the
revalidation) is intentionally left as-is: it's the root cause tracked
in W-23692110, and a fail-loudly interim change was considered and
declined for this PR.
@mlischetti @claude
fix(node): harden resolver against chdir/root-dir bugs, fix Worker-lo…
309bac8 
…ad mutex reinit, correct docs

Addresses the follow-on revalidation review of PR #154's external-module
resolver support:

- addon.c: guard the process-global g_mutex init with uv_once so loading
  the addon into multiple worker_threads Workers doesn't re-initialize an
  already-live mutex.
- resolver.ts: modulesFromDirectory() now builds candidate paths from the
  captured absolute base instead of re-resolving the original (possibly
  relative) baseDir on every call, so a later process.chdir() no longer
  breaks lookups. Containment checks switch from a startsWith(base + sep)
  string check to a path.relative()-based isContained() helper, fixing a
  root-directory baseDir (e.g. "/") rejecting every child path.
- first-resolver-wins fixture/test: the fixture now also proves the first
  resolver stays active on the second instance (not just that the second
  instance's own resolver lost), cleans up both instances via try/finally,
  and the isolated child-process test gets a timeout so a native deadlock
  fails the test instead of hanging the suite (same fix applied to
  init-bad-path.test.ts, which had the identical gap).
- Docs + JSDoc: correct "Multiple Resolvers in One Process" to say the
  first resolver-backed run() wins (not the first initialize(), which only
  loads/ref-counts the library), and add an explicit warning that running
  resolver-backed calls concurrently across Workers is memory-unsafe, not
  just unsupported.

The two Critical findings from that review (a resolver-buffer free race
and a resolver N-API reference that can outlive its owning Worker
environment) are confined to the same process-wide resolver architecture
already deferred to W-23692110 and are not addressed here; the Worker
memory-safety warning above documents that limitation explicitly.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@mlischetti
mlischetti merged commit 65b9dc5 into master Aug 6, 2026
5 checks passed
@mlischetti
mlischetti deleted the nodejs-external-modules branch August 6, 2026 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andres-rad andres-rad andres-rad approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mlischetti @andres-rad