Resolve JS module on first use if empty #148
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements automatic resolution of JavaScript modules on first use when they have a URL but no cached script content. This handles a migration scenario where legacy modules stored only URLs need to be resolved and persisted.
Changes:
- Modified the
Moduleinterface to returnFuture<CodeModuleEntity>instead ofFuture<Void>and added atenantparameter to theinitializemethod - Updated
ModuleJavaScriptto detect unresolved modules (URL present, script absent) and automatically fetch, persist, and initialize them - Updated
ModuleJsonPathto conform to the new interface signature - Updated
ModuleCacheImplto use the returned entity for caching
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| server/src/main/java/com/indexdata/reservoir/module/Module.java | Updated interface signature to return the potentially modified CodeModuleEntity and accept tenant parameter |
| server/src/main/java/com/indexdata/reservoir/module/impl/ModuleJavaScript.java | Added migration logic to resolve and persist modules with URLs but no scripts; refactored ES module initialization into separate method |
| server/src/main/java/com/indexdata/reservoir/module/impl/ModuleJsonPath.java | Updated to conform to new Module interface signature |
| server/src/main/java/com/indexdata/reservoir/module/impl/ModuleCacheImpl.java | Updated to cache the entity returned from initialization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return new CodeModuleEntity.CodeModuleBuilder(entity.asJson()) | ||
| .resolve(vertx) | ||
| .compose(newEntity -> new Storage(vertx, tenant, HttpMethod.POST) | ||
| .updateCodeModuleEntity(newEntity).map(newEntity)) |
There was a problem hiding this comment.
The updateCodeModuleEntity method returns Future<Boolean> where false indicates the module was not found in the database. However, the code only maps the result to newEntity without checking if the update was successful. If the database update fails (returns false), the module initialization should fail, but currently it would proceed to initAsEsModule. Consider checking the Boolean result and failing the Future if the update was not successful.
| .updateCodeModuleEntity(newEntity).map(newEntity)) | |
| .updateCodeModuleEntity(newEntity) | |
| .compose(updated -> { | |
| if (Boolean.TRUE.equals(updated)) { | |
| return Future.succeededFuture(newEntity); | |
| } else { | |
| return Future.failedFuture(new IllegalStateException( | |
| "Failed to update code module entity in storage for id: " + newEntity.getId())); | |
| } | |
| })) |
| if (!hasScript) { | ||
| return new CodeModuleEntity.CodeModuleBuilder(entity.asJson()) | ||
| .resolve(vertx) | ||
| .compose(newEntity -> new Storage(vertx, tenant, HttpMethod.POST) | ||
| .updateCodeModuleEntity(newEntity).map(newEntity)) | ||
| .compose(this::initAsEsModule); |
There was a problem hiding this comment.
The migration scenario where a module has a URL but no script is not covered by tests. This is a critical new code path that resolves the module and updates the database. Consider adding test coverage for this scenario to ensure the migration logic works correctly, including success cases and failure cases (e.g., URL resolution failure, database update failure).
| return Future.failedFuture(new IllegalArgumentException( | ||
| "url must end with .mjs to designate ES module")); | ||
| if (hasUrl) { | ||
| //module was never resolved (migration) |
There was a problem hiding this comment.
The inline comment "module was never resolved (migration)" explains the purpose but lacks detail about when this scenario occurs and what the expected behavior is. Consider expanding this comment to explain that this handles legacy modules that were stored with only URLs before automatic resolution was implemented, and that the code will fetch the script and persist it to the database for future use.
| //module was never resolved (migration) | |
| // Handle legacy modules that were stored with only a URL before automatic | |
| // resolution was implemented. In this case the module has never been | |
| // resolved: we fetch the script from the URL, persist the resolved script | |
| // to the database for future use, and then initialize it as an ES module. |
| private Future<CodeModuleEntity> initAsEsModule(CodeModuleEntity entity) { | ||
| defaultFunctionName = entity.getFunction(); | ||
| final boolean isModule = entity.getUrl().endsWith("mjs"); | ||
| if (!isModule) { | ||
| return Future.failedFuture(new IllegalArgumentException( | ||
| "url must end with .mjs to designate ES module")); | ||
| } | ||
| Context.Builder cb = Context.newBuilder("js") | ||
| .allowExperimentalOptions(true) | ||
| .option("js.esm-eval-returns-exports", "true"); | ||
| context = cb.build(); | ||
| String moduleName = entity.getUrl().substring(entity.getUrl().lastIndexOf("/") + 1); | ||
| module = context.eval(Source.newBuilder("js", entity.getScript(), moduleName).buildLiteral()); |
There was a problem hiding this comment.
The initAsEsModule method assumes entity.getScript() and entity.getUrl() are non-null, but this is not validated. While the current call sites ensure this, adding defensive null checks would make the code more robust and prevent potential future bugs if this method is called from other contexts. Consider adding validation at the start of the method.
No description provided.