feat(QTDI-2134): dynamic dependencies - nested loading #1135
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements nested JAR loading functionality for dynamic dependencies, removing the hardcoded "MAVEN-INF/repository/" prefix requirement and adding support for alternative layouts like "BOOT-INF/lib/". The changes enable more flexible dependency resolution and improve debugging capabilities.
Key Changes:
- Removed hardcoded "MAVEN-INF/repository/" prefix from nested dependency paths
- Added
getNestedResource()method to support nested JAR resource access - Enhanced debug logging with system information capture
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ConfigurableClassLoader.java | Added getNestedResource() method for nested JAR access and updated path handling |
| Container.java | Modified nested repository detection and parent filter logic |
| ContainerManager.java | Added system information logging and improved nested repository detection |
| MvnDependencyListLocalRepositoryResolver.java | Updated to use new nested resource loading mechanism |
| ComponentManager.java | Enhanced plugin auto-discovery for nested JARs |
| ResolverImpl.java | Removed "MAVEN-INF/repository/" prefix from resource lookup |
| DynamicDependenciesConfiguration.java | New annotation for dynamic dependencies configuration |
| ConfigurableClassLoaderTest.java | Updated test paths to remove "MAVEN-INF/repository/" prefix |
| MvnDependencyListLocalRepositoryResolverTest.java | Updated test paths and added ConfigurableClassLoader usage |
| NestedJarArchiveTest.java | Changed test paths from "MAVEN-INF/repository/" to "BOOT-INF/lib/" |
| ResolverImplTest.java | Updated test paths to remove "MAVEN-INF/repository/" prefix |
| ComponentManagerTest.java | Added system property setup/teardown and adjusted dependency count assertion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try (JarFile innerJar = new JarFile(tempInnerJar.toFile())) { | ||
| final JarEntry resourceEntry = innerJar.getJarEntry(resourcePath); | ||
| if (resourceEntry == null) { | ||
| throw new FileNotFoundException("Resource not found: " + resourcePath); | ||
| } | ||
|
|
||
| // Return a stream that cleans up automatically when closed | ||
| return new FilterInputStream(innerJar.getInputStream(resourceEntry)) { | ||
| @Override | ||
| public void close() throws IOException { | ||
| super.close(); | ||
| Files.deleteIfExists(tempInnerJar); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
The innerJar JarFile is closed in the try-with-resources block before the FilterInputStream is returned, which will cause the stream to be invalid. The JarFile must remain open until the FilterInputStream is closed. Consider creating a custom InputStream that holds references to both the JarFile and the temp file, closing the JarFile before deleting the temp file.
|
|
||
| private boolean isNestedDependencyResource(final String name) { | ||
| return name != null && name.startsWith(NESTED_MAVEN_REPOSITORY); | ||
| return name.startsWith(NESTED_MAVEN_REPOSITORY) || name.endsWith(".jar"); |
There was a problem hiding this comment.
This condition is too broad. Checking name.endsWith(\".jar\") will incorrectly classify any resource path ending with .jar as a nested dependency resource, even if it's a regular classpath resource. This could cause incorrect resource loading behavior for non-nested JAR references.
| return name.startsWith(NESTED_MAVEN_REPOSITORY) || name.endsWith(".jar"); | |
| return name.startsWith(NESTED_MAVEN_REPOSITORY) && name.endsWith(".jar"); |
| final String jarPath = urlFile.substring(0, urlFile.lastIndexOf("!")); | ||
| final String jarFilePath = jarPath.substring(jarPath.lastIndexOf("/") + 1); |
There was a problem hiding this comment.
Using lastIndexOf(\"/\") for extracting the filename is platform-dependent and will fail on Windows where paths use backslashes. Use Paths.get(new URI(jarPath)).getFileName().toString() or similar platform-independent approach.
| final Path tempInnerJar = Files.createTempFile("nested-inner-", ".jar"); | ||
| try (InputStream innerStream = outerJar.getInputStream(innerEntry)) { | ||
| Files.copy(innerStream, tempInnerJar, StandardCopyOption.REPLACE_EXISTING); | ||
|
|
||
| try (JarFile innerJar = new JarFile(tempInnerJar.toFile())) { | ||
| final JarEntry resourceEntry = innerJar.getJarEntry(resourcePath); | ||
| if (resourceEntry == null) { | ||
| throw new FileNotFoundException("Resource not found: " + resourcePath); | ||
| } | ||
|
|
||
| // Return a stream that cleans up automatically when closed | ||
| return new FilterInputStream(innerJar.getInputStream(resourceEntry)) { | ||
| @Override | ||
| public void close() throws IOException { | ||
| super.close(); | ||
| Files.deleteIfExists(tempInnerJar); | ||
| } | ||
| }; | ||
| } | ||
| } catch (IOException e) { | ||
| Files.deleteIfExists(tempInnerJar); | ||
| throw e; |
There was a problem hiding this comment.
If an exception occurs after creating the temp file but before the FilterInputStream's close() is called, the temp file will leak. The temp file should be deleted in a finally block or the exception handler at line 558-561 should be moved outside the inner try block to ensure cleanup.
| final Path tempInnerJar = Files.createTempFile("nested-inner-", ".jar"); | |
| try (InputStream innerStream = outerJar.getInputStream(innerEntry)) { | |
| Files.copy(innerStream, tempInnerJar, StandardCopyOption.REPLACE_EXISTING); | |
| try (JarFile innerJar = new JarFile(tempInnerJar.toFile())) { | |
| final JarEntry resourceEntry = innerJar.getJarEntry(resourcePath); | |
| if (resourceEntry == null) { | |
| throw new FileNotFoundException("Resource not found: " + resourcePath); | |
| } | |
| // Return a stream that cleans up automatically when closed | |
| return new FilterInputStream(innerJar.getInputStream(resourceEntry)) { | |
| @Override | |
| public void close() throws IOException { | |
| super.close(); | |
| Files.deleteIfExists(tempInnerJar); | |
| } | |
| }; | |
| } | |
| } catch (IOException e) { | |
| Files.deleteIfExists(tempInnerJar); | |
| throw e; | |
| Path tempInnerJar = null; | |
| boolean success = false; | |
| try { | |
| tempInnerJar = Files.createTempFile("nested-inner-", ".jar"); | |
| try (InputStream innerStream = outerJar.getInputStream(innerEntry)) { | |
| Files.copy(innerStream, tempInnerJar, StandardCopyOption.REPLACE_EXISTING); | |
| try (JarFile innerJar = new JarFile(tempInnerJar.toFile())) { | |
| final JarEntry resourceEntry = innerJar.getJarEntry(resourcePath); | |
| if (resourceEntry == null) { | |
| throw new FileNotFoundException("Resource not found: " + resourcePath); | |
| } | |
| // Return a stream that cleans up automatically when closed | |
| FilterInputStream fis = new FilterInputStream(innerJar.getInputStream(resourceEntry)) { | |
| @Override | |
| public void close() throws IOException { | |
| super.close(); | |
| Files.deleteIfExists(tempInnerJar); | |
| } | |
| }; | |
| success = true; | |
| return fis; | |
| } | |
| } | |
| } finally { | |
| if (tempInnerJar != null && !success) { | |
| try { | |
| Files.deleteIfExists(tempInnerJar); | |
| } catch (IOException ex) { | |
| // log or ignore, as appropriate | |
| log.warn("Failed to delete temp file: " + tempInnerJar, ex); | |
| } | |
| } |
…s configuration type
…mic dependencies configuration type
|
https://qlik-dev.atlassian.net/browse/QTDI-2134
Related pull requests
Studio build pull request