Skip to content

feat(QTDI-693): manage nested jars #969

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

@ozhelezniak-talend ozhelezniak-talend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left comments

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request implements support for managing nested JARs in the Talend SDK component runtime. The changes enable the system to detect and handle components that are packaged within fat JARs with embedded Maven repositories.

  • Adds nested repository detection logic to check for MAVEN-INF/repository resources when default repository paths are used
  • Extends classloader functionality to track and expose nested JAR URLs
  • Updates component scanning to handle nested repositories with proper filtering

Reviewed Changes

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

Show a summary per file
File Description
ContainerManager.java Implements nested repository detection and fallback logic for fat JAR scenarios
Container.java Adds public accessor method for nested repository status
ConfigurableClassLoader.java Tracks nested URLs and overrides getURLs() to include them
ComponentManagerTest.java Updates test expectations to account for nested JAR handling
ComponentManager.java Enhances component scanning to handle nested repositories with proper filtering
Comments suppressed due to low confidence (1)

component-runtime-manager/src/main/java/org/talend/sdk/component/runtime/manager/ComponentManager.java:1328

  • [nitpick] The log message format with manual bracket prefix is inconsistent with typical logging patterns. Consider using standard log formatting without manual prefixes.
                        log.info("[onCreate] Can't read nested scanning.properties: {}", e.getMessage());

if (PathFactory.get(System.getProperty("user.home")).resolve(".m2/repository").equals(rootRepo)) {
final URL nested = classLoaderConfiguration.getParent().getResource("MAVEN-INF/repository");
if (nested != null) {
rootRepo = Paths.get(nested.getFile().replace("file:", ""));
Copy link
Preview

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using simple string replacement for URL handling is unreliable and may fail with encoded URLs or different URL formats. Consider using URI.create(nested.toString()).getPath() or proper URL decoding instead.

Suggested change
rootRepo = Paths.get(nested.getFile().replace("file:", ""));
rootRepo = Paths.get(URI.create(nested.toString()).getPath());

Copilot uses AI. Check for mistakes.

Comment on lines +118 to +120
.orElseGet(() -> PathFactory.get(System.getProperty("user.home")).resolve(".m2/repository"));
// if we've defaulted to user home m2 (fallback), we want to check if we're in running in a fatjar
if (PathFactory.get(System.getProperty("user.home")).resolve(".m2/repository").equals(rootRepo)) {
Copy link
Preview

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed null fallback for System.getProperty("user.home") that was present in the original code. This could cause NullPointerException if the system property is not set.

Suggested change
.orElseGet(() -> PathFactory.get(System.getProperty("user.home")).resolve(".m2/repository"));
// if we've defaulted to user home m2 (fallback), we want to check if we're in running in a fatjar
if (PathFactory.get(System.getProperty("user.home")).resolve(".m2/repository").equals(rootRepo)) {
.orElseGet(() -> PathFactory.get(Optional.ofNullable(System.getProperty("user.home")).orElse("/tmp")).resolve(".m2/repository"));
// if we've defaulted to user home m2 (fallback), we want to check if we're in running in a fatjar
if (PathFactory.get(Optional.ofNullable(System.getProperty("user.home")).orElse("/tmp")).resolve(".m2/repository").equals(rootRepo)) {

Copilot uses AI. Check for mistakes.

@@ -111,6 +111,8 @@ public class ConfigurableClassLoader extends URLClassLoader {
@Getter
private final List<String> cacheableClasses;

private List<URL> nestedURLs = new ArrayList<>();
Copy link
Preview

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The field should be declared as final since it's initialized once and never reassigned. Consider making it final for better immutability.

Suggested change
private List<URL> nestedURLs = new ArrayList<>();
private final List<URL> nestedURLs = new ArrayList<>();

Copilot uses AI. Check for mistakes.

This comment has been minimized.

1 similar comment
Copy link

sonar-eks bot commented Jul 22, 2025

Failed

  • 66.70% Coverage on New Code (is less than 80.00%)

Analysis Details

0 Issues

  • Bug 0 Bugs
  • Vulnerability 0 Vulnerabilities
  • Code Smell 0 Code Smells

Coverage and Duplications

  • Coverage 66.70% Coverage (65.90% Estimated after merge)
  • Duplications 0.00% Duplicated Code (1.40% Estimated after merge)

Project ID: org.talend.sdk.component:component-runtime

View in SonarQube

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants