Connector and resource loading improvements for integration project dependencies#529
Open
chathuranga-jayanath-99 wants to merge 35 commits intowso2:mainfrom
Open
Conversation
When an integration project is added as a dependency, its connectors become accessible to the referencing project.
Keep available projects, download new ones, and delete removed ones based on the updated dependencies list.
Clears existing downloaded dependencies and fetches the latest ones.
…endency downloads
…s from dependent projects
…narios Avoids updating or writing to the current USER_HOME during tests, and allow to run tests by writing content to a temp directory.
…on project dependency
…oject dependency If the connector already exists in a dependent integration project, adding it again is skipped and an appropriate message is shown.
Added validation when loading resources for dependent projects to detect conflicts with existing resources. If a conflict is found, the newly added dependency is automatically removed. A clear error message is generated, including a list of the conflicting resources.
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/IntegrationProjectDownloadManager.java (1)
156-181:⚠️ Potential issue | 🟠 MajorReport transitive dependency failures against the failing dependency.
Failures thrown while processing
transitiveDependencyare caught by the outer loop and recorded using the root dependency’s coordinates, so the structured result can point users to the wrong dependency. Preserve the failing dependency identity when propagating recursive errors.Also applies to: 320-322
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/IntegrationProjectDownloadManager.java` around lines 156 - 181, The catch blocks in the outer loop currently attribute any exception from fetchDependencyRecursively(...) to the root DependencyDetails, which misreports transitive failures; modify fetchDependencyRecursively and the custom exceptions (NoDescriptorException, VersioningTypeMismatchException, and the generic Exception wrapper you use) to include the actual failing DependencyDetails (or its coordinates) when rethrowing, and in the outer loop extract the failing dependency coordinates from the exception (instead of using the loop variable) — e.g., call exception.getFailedDependency().getGroupId()/getArtifact()/getVersion() when building failedDependency strings so transitiveDependency failures are recorded against the real failing dependency.
🧹 Nitpick comments (2)
org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/ConnectorDependencyDownloadResult.java (1)
27-45: Consider null-safe list handling.If callers ever pass
nullfor either list, downstream code that iterates will NPE. Consider normalizing to empty lists in the constructor:♻️ Suggested refinement
public ConnectorDependencyDownloadResult(List<String> failedDependencies, List<String> fromIntegrationProjectDependencies) { - this.failedDependencies = failedDependencies; - this.fromIntegrationProjectDependencies = fromIntegrationProjectDependencies; + this.failedDependencies = failedDependencies != null ? failedDependencies : java.util.Collections.emptyList(); + this.fromIntegrationProjectDependencies = fromIntegrationProjectDependencies != null + ? fromIntegrationProjectDependencies : java.util.Collections.emptyList(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/ConnectorDependencyDownloadResult.java` around lines 27 - 45, The constructor of ConnectorDependencyDownloadResult can accept null lists which will cause NPEs when callers iterate getFailedDependencies() or getFromIntegrationProjectDependencies(); update the constructor (ConnectorDependencyDownloadResult(List<String>, List<String>)) to normalize null inputs to non-null empty lists (e.g., Collections.emptyList() or new ArrayList<>()) and assign those normalized lists to the fields so the getter methods always return safe, non-null lists.org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/resourceFinder/pojo/ConflictingDependency.java (1)
19-61: LGTM — optionally consider defensive copies for the lists.The POJO is clean and effectively read-only via the public API. As an optional hardening, consider storing
List.copyOf(...)in the constructor (and/or returning unmodifiable views from the getters) to prevent accidental mutation of the conflict lists via the caller's original reference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/resourceFinder/pojo/ConflictingDependency.java` around lines 19 - 61, The lists assigned in ConflictingDependency's constructor can be mutated by callers; update the constructor to store defensive immutable copies of conflictingArtifacts and conflictingConnectors (e.g., via List.copyOf or Collections.unmodifiableList) and/or make getConflictingArtifacts and getConflictingConnectors return unmodifiable copies to ensure the internal state cannot be changed by external references; modify the ConflictingDependency(String groupId, String artifactId, String version, List<String> conflictingArtifacts, List<String> conflictingConnectors) constructor and the two getter methods accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/ISynapseLanguageService.java`:
- Around line 248-252: The change to
ISynapseLanguageService.loadDependentResources from CompletableFuture<String> to
CompletableFuture<LoadDependentResourcesResponse> breaks the JSON-RPC wire
contract; either revert the signature to CompletableFuture<String> or introduce
a new versioned RPC (e.g., add loadDependentResourcesV2 returning
CompletableFuture<LoadDependentResourcesResponse>) and keep the original
loadDependentResources method so existing clients keep receiving the string
payload; update the `@JsonRequest` annotations accordingly and ensure any consumer
(e.g., mi-vscode) is updated to use the new RPC in the same release if you
choose to change the existing method.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/connectors/entity/Connector.java`:
- Line 37: OldProjectConnectorLoader currently leaves Connector.fromProject as
false because it doesn't call markProjectConnectors(); update
OldProjectConnectorLoader to explicitly mark connectors belonging to the main
project by invoking the same logic used in NewProjectConnectorLoader (call
markProjectConnectors() or set Connector.fromProject=true for project-loaded
connectors) after loading, ensuring connectors originating from the project are
correctly flagged; reference OldProjectConnectorLoader,
NewProjectConnectorLoader, markProjectConnectors(), and the
Connector.fromProject field to locate and apply the change.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/connectors/NewProjectConnectorLoader.java`:
- Around line 187-195: The method setConnectorsZipFolderPath appends to
baseConnectorsZipFolderPaths and connectorsZipFolderPath without clearing them,
causing stale paths to persist across project initializations; update
setConnectorsZipFolderPath (in NewProjectConnectorLoader) to reset/clear
baseConnectorsZipFolderPaths and connectorsZipFolderPath at the start, then set
projectId and add the new Path.of(projectRoot, Constant.SRC, Constant.MAIN,
Constant.WSO2MI, Constant.RESOURCES, Constant.CONNECTORS).toString() and
getConnnectorDownloadPath().toString() into baseConnectorsZipFolderPaths before
adding them into connectorsZipFolderPath to ensure the loader scans only the
current project’s directories.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/IntegrationProjectDownloadManager.java`:
- Around line 94-108: The current cleanup catch block in
IntegrationProjectDownloadManager swallows IOException and proceeds to call
downloadDependencies(), risking reuse of stale cache; change the error handling
so that if Utils.deleteDirectory throws IOException during clearing of
downloadDirectory or extractDirectory you do not continue—either rethrow the
IOException (propagate it) or return an explicit failure result from this method
instead of calling downloadDependencies(projectPath, dependencies,
isVersionedDeploymentEnabled, userHome); update the catch block accordingly and
ensure callers handle the propagated exception or failure result.
- Around line 184-186: The cleanup currently uses fetchedDependencies (populated
before fetch/validation finishes) which can retain stale entries when
fetch/parse fails; modify IntegrationProjectDownloadManager to track two
sets—visited and resolved—and only add a dependency's base name to the resolved
set after the fetch and descriptor validation succeed; change calls that
buildExpectedBaseNames (used before deleteObsoleteDownloadedFiles and
deleteObsoleteExtractedDirs) to accept the resolved set instead of
fetchedDependencies so only successfully resolved dependencies are kept; apply
the same pattern for the other cleanup block covering the logic around lines
292–316 to ensure both Downloaded and Extracted cleanup use the resolved set.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/resourceFinder/AbstractResourceFinder.java`:
- Around line 46-52: The connector-filtering logic in AbstractResourceFinder
uses startsWith(HTTP_CONNECTOR_PREFIX) which incorrectly skips any connector
whose id begins with the prefix; instead, normalize the connector id after
stripping the core name and compare for equality with the exact built-in HTTP
connector id. Update the code paths in AbstractResourceFinder that perform
connector checks (the blocks referencing HTTP_CONNECTOR_PREFIX and the built-in
HTTP connector name) to: compute the stripped/normalized core name once (e.g.,
normalizeConnectorId or similar local variable), then use equals(...) to skip
only the exact bundled HTTP connector; apply the same change to the other
occurrences noted (around the other comparison blocks at the referenced ranges).
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/utils/Utils.java`:
- Around line 511-526: The method stripConnectorVersion currently removes the
final hyphen segment unconditionally; change it to only trim when that trailing
segment looks like a version: find lastHyphen (as done), extract suffix =
zipBaseName.substring(lastHyphen+1), validate suffix against a version pattern
(e.g., matches "\\d+(\\.\\d+)*" or at least startsWith a digit) and only return
zipBaseName.substring(0,lastHyphen) when the suffix passes the check; otherwise
return the original zipBaseName. Use the existing Constant.HYPHEN and the
stripConnectorVersion method name to locate where to add this validation.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/SynapseLanguageService.java`:
- Around line 688-695: The refetchIntegrationProjectDependencies() method
currently re-downloads on-disk integration dependencies but fails to refresh
in-memory connector state; update refetchIntegrationProjectDependencies() to
call updateConnectors() after
DependencyDownloadManager.refetchIntegrationProjectDependencies(projectUri)
completes (similar to updateConnectorDependencies() and
loadDependentResources()) so ConnectorHolder and the generated connectors.xsd
stay in sync with the new baseConnectorsZipFolderPaths and avoid stale
fromProject flags.
- Around line 704-711: Moving resourceFinder.loadDependentResources(projectUri)
and updateConnectors() into supplyAsync introduces concurrent access to shared
mutable state; protect ConnectorHolder's static connectors list by adding
synchronization or switching to a concurrent collection (e.g., replace List with
CopyOnWriteArrayList or synchronize
addConnector/removeConnector/clearConnectors/getConnector/isValidConnector), and
make file writes in SchemaGenerate.generate() atomic (use a dedicated lock
around updateConnectors() and SchemaGenerate.generate(), or write to a temp file
and atomically move/rename to connectors.xsd) so concurrent
updateConnectors()/loadDependentResources() calls cannot corrupt connectors or
the XSD file.
In
`@org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/downloader/ConnectorDownloadManagerTest.java`:
- Around line 91-94: The test currently asserts that "mi-connector-http" appears
in ConnectorDependencyDownloadResult.getFromIntegrationProjectDependencies but
the message still mentions it being "marked as failed"; update the assertion
logic to both verify the connector is present in
getFromIntegrationProjectDependencies (via
ConnectorDownloadManager.downloadDependencies(...)) and explicitly assert it is
NOT present in the failed list (e.g.,
ConnectorDependencyDownloadResult.getFailedDownloads() or equivalent), so the
test confirms the connector is reported as coming from the integration project
and not reported as a failed download.
In
`@org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/loader/NewDependentProjectConnectorLoaderTest.java`:
- Around line 108-114: The teardown does not clear the singleton connector state
left in ConnectorHolder between tests; update tearDown() to reset/clear the
singleton by invoking ConnectorHolder.getInstance() and calling its clear/reset
method (e.g. ConnectorHolder.getInstance().clear() or the appropriate reset
method provided) so all loaded connectors are removed after each test run; keep
the existing deleteRecursively(tempHome) and deleteRecursively(projectRoot)
calls.
In
`@org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/parser/IntegrationProjectDownloadManagerTest.java`:
- Around line 114-116: The static Mockito stub for
Utils.getDependencyFromLocalRepo uses a four-argument signature but
IntegrationProjectDownloadManager.fetchDependencyFile invokes the five-argument
overload (including userHome); update the mock in the test to match that exact
method signature by adding a fifth any() argument to utilsMock.when(...) so the
static mock intercepts the actual call to Utils.getDependencyFromLocalRepo
invoked by IntegrationProjectDownloadManager.fetchDependencyFile.
In
`@org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/resource/finder/LoadDependentResourcesTest.java`:
- Around line 462-478: In testConflictBetweenDependentProjectsDetected (and the
other listed tests), the dependency directories are created back-to-back so they
can have identical mtimes in CI; after creating dep1 and dep2 (via
createDependentProject) set explicit FileTime values on their Paths (use
Files.setLastModifiedTime(dep1, FileTime.fromMillis(<older>)) and
Files.setLastModifiedTime(dep2, FileTime.fromMillis(<newer>))) so dep1 is older
than dep2 before calling
resourceFinder.setProjectResources/loadDependentResources; apply the same fix
pattern to the other tests mentioned (lines 485-501, 741-758, 766-786,
1307-1328) to make order deterministic.
---
Outside diff comments:
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/IntegrationProjectDownloadManager.java`:
- Around line 156-181: The catch blocks in the outer loop currently attribute
any exception from fetchDependencyRecursively(...) to the root
DependencyDetails, which misreports transitive failures; modify
fetchDependencyRecursively and the custom exceptions (NoDescriptorException,
VersioningTypeMismatchException, and the generic Exception wrapper you use) to
include the actual failing DependencyDetails (or its coordinates) when
rethrowing, and in the outer loop extract the failing dependency coordinates
from the exception (instead of using the loop variable) — e.g., call
exception.getFailedDependency().getGroupId()/getArtifact()/getVersion() when
building failedDependency strings so transitiveDependency failures are recorded
against the real failing dependency.
---
Nitpick comments:
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/ConnectorDependencyDownloadResult.java`:
- Around line 27-45: The constructor of ConnectorDependencyDownloadResult can
accept null lists which will cause NPEs when callers iterate
getFailedDependencies() or getFromIntegrationProjectDependencies(); update the
constructor (ConnectorDependencyDownloadResult(List<String>, List<String>)) to
normalize null inputs to non-null empty lists (e.g., Collections.emptyList() or
new ArrayList<>()) and assign those normalized lists to the fields so the getter
methods always return safe, non-null lists.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/resourceFinder/pojo/ConflictingDependency.java`:
- Around line 19-61: The lists assigned in ConflictingDependency's constructor
can be mutated by callers; update the constructor to store defensive immutable
copies of conflictingArtifacts and conflictingConnectors (e.g., via List.copyOf
or Collections.unmodifiableList) and/or make getConflictingArtifacts and
getConflictingConnectors return unmodifiable copies to ensure the internal state
cannot be changed by external references; modify the
ConflictingDependency(String groupId, String artifactId, String version,
List<String> conflictingArtifacts, List<String> conflictingConnectors)
constructor and the two getter methods accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38b3b702-946a-44f0-85eb-f0c96f0dab76
⛔ Files ignored due to path filters (1)
org.eclipse.lemminx/src/test/resources/synapse/connector/zips/mi-connector-csv-3.0.0.zipis excluded by!**/*.zip
📒 Files selected for processing (19)
org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/SynapseLanguageService.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/ISynapseLanguageService.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/connectors/NewProjectConnectorLoader.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/connectors/entity/Connector.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/ConnectorDependencyDownloadResult.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/ConnectorDownloadManager.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/DependencyDownloadManager.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/IntegrationProjectDependencyDownloadResult.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/IntegrationProjectDownloadManager.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/resourceFinder/AbstractResourceFinder.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/resourceFinder/pojo/ConflictingDependency.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/resourceFinder/pojo/LoadDependentResourcesResponse.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/utils/Constant.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/utils/Utils.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/downloader/ConnectorDownloadManagerTest.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/loader/MockConnectorLoader.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/loader/NewDependentProjectConnectorLoaderTest.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/parser/IntegrationProjectDownloadManagerTest.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/resource/finder/LoadDependentResourcesTest.java
… before fresh download
…d dependencies are available at the end
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/parser/IntegrationProjectDownloadManagerTest.java (1)
1024-1033: Close theFiles.walkstream indeleteRecursively.Same concern as in
NewDependentProjectConnectorLoaderTest:Files.walk(path)returns a closeableStream. Wrap it in try-with-resources so directory handles are released deterministically and teardown does not flake on Windows.♻️ Proposed refactor
- Files.walk(path) - .sorted(Comparator.reverseOrder()) - .map(Path::toFile) - .forEach(File::delete); + try (java.util.stream.Stream<Path> walk = Files.walk(path)) { + walk.sorted(Comparator.reverseOrder()) + .map(Path::toFile) + .forEach(File::delete); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/parser/IntegrationProjectDownloadManagerTest.java` around lines 1024 - 1033, The deleteRecursively method uses Files.walk(path) which returns a Closeable Stream but is not closed; update deleteRecursively to open the stream in a try-with-resources (e.g. try (Stream<Path> stream = Files.walk(path)) { stream.sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete); }) so the stream is closed deterministically and directory handles are released (keep the method signature and exception behavior unchanged, referencing deleteRecursively and Files.walk).org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/loader/NewDependentProjectConnectorLoaderTest.java (1)
498-509: Close theFiles.walkstream indeleteRecursively.
Files.walk(path)returns aStreambacked by an open directory handle and should be closed via try-with-resources. As written, the underlying handle is closed only when the stream is GC'd, which can leave file handles open across tests and intermittently prevent directory deletion on Windows.♻️ Proposed refactor
- LOGGER.info("Deleting directory recursively: " + path.toString()); - Files.walk(path) - .sorted(Comparator.reverseOrder()) - .map(Path::toFile) - .forEach(File::delete); + LOGGER.info("Deleting directory recursively: " + path.toString()); + try (java.util.stream.Stream<Path> walk = Files.walk(path)) { + walk.sorted(Comparator.reverseOrder()) + .map(Path::toFile) + .forEach(File::delete); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/loader/NewDependentProjectConnectorLoaderTest.java` around lines 498 - 509, deleteRecursively currently uses Files.walk(path) without closing the returned Stream; wrap the Files.walk(path) call in a try-with-resources (e.g. try (Stream<Path> stream = Files.walk(path)) { ... }) so the directory handle is closed promptly, then perform the .sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete) inside that try block; reference the deleteRecursively method and the Files.walk invocation when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/utils/Utils.java`:
- Around line 522-525: The JUL log call in stripConnectorVersion uses SLF4J `{}`
placeholders so the connector name is printed literally; change the message to
use MessageFormat syntax and pass the parameter: replace logger.log(Level.INFO,
"Stripping version from connector zip base name: {}", zipBaseName) with
logger.log(Level.INFO, "Stripping version from connector zip base name: {0}",
zipBaseName) inside the stripConnectorVersion method so the zipBaseName is
correctly substituted.
---
Nitpick comments:
In
`@org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/loader/NewDependentProjectConnectorLoaderTest.java`:
- Around line 498-509: deleteRecursively currently uses Files.walk(path) without
closing the returned Stream; wrap the Files.walk(path) call in a
try-with-resources (e.g. try (Stream<Path> stream = Files.walk(path)) { ... })
so the directory handle is closed promptly, then perform the
.sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete)
inside that try block; reference the deleteRecursively method and the Files.walk
invocation when making this change.
In
`@org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/parser/IntegrationProjectDownloadManagerTest.java`:
- Around line 1024-1033: The deleteRecursively method uses Files.walk(path)
which returns a Closeable Stream but is not closed; update deleteRecursively to
open the stream in a try-with-resources (e.g. try (Stream<Path> stream =
Files.walk(path)) {
stream.sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete);
}) so the stream is closed deterministically and directory handles are released
(keep the method signature and exception behavior unchanged, referencing
deleteRecursively and Files.walk).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7607256b-270f-4484-8c14-a7a9f3363c02
📒 Files selected for processing (10)
org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/SynapseLanguageService.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/connectors/NewProjectConnectorLoader.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/DependencyDownloadManager.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/IntegrationProjectDownloadManager.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/resourceFinder/AbstractResourceFinder.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/utils/Utils.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/downloader/ConnectorDownloadManagerTest.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/loader/NewDependentProjectConnectorLoaderTest.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/parser/IntegrationProjectDownloadManagerTest.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/resource/finder/LoadDependentResourcesTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/DependencyDownloadManager.java
chathuranga-jayanath-99
force-pushed
the
load-connector-from-deps-main
branch
from
82b6194 to
0741528
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/loader/NewDependentProjectConnectorLoaderTest.java`:
- Around line 347-373: The test
testSameConnectorInProjectAndDependency_isMarkedFromProject must also assert
that only one connector was loaded; after calling loader.init(...) and
loader.loadConnector() and after retrieving the connector via
ConnectorHolder.getInstance().getConnector("http"), fetch all loaded connectors
from ConnectorHolder.getInstance() (e.g. getAllConnectors() or getConnectors())
and assert the collection size is 1 or assert that the stream of connector
artifact IDs / names (Connector#getArtifactId or Connector#getName) contains
exactly one "http" entry so the “loaded only once” behavior is explicitly
enforced.
- Around line 498-508: The deleteRecursively method leaks the Files.walk stream
and hides deletion failures; change it to open the stream in a
try-with-resources around Files.walk(path), iterate the stream in reverse order,
and replace File::delete with Files.deleteIfExists (or explicitly call
Files.delete and catch/log/propagate IOException) so failures are surfaced;
update references inside deleteRecursively to log or rethrow exceptions when
delete operations fail and ensure the stream is closed automatically.
In
`@org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/parser/IntegrationProjectDownloadManagerTest.java`:
- Around line 1024-1032: The deleteRecursively method uses Files.walk(path)
without closing the stream and uses File::delete which silently fails; change it
to open the stream in a try-with-resources (try (Stream<Path> s =
Files.walk(path)) { ... }) so the walk stream is closed, then replace
map(Path::toFile).forEach(File::delete) with a robust deletion that calls
Files.deleteIfExists(path) for each Path (or collect to a list first then
iterate) and surface IOExceptions instead of ignoring failures; update
deleteRecursively to propagate or handle IOExceptions accordingly so test
cleanup reliably reports deletion errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54ab0e12-98dc-444b-a9c0-e17b6d18c730
📒 Files selected for processing (5)
org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/utils/Utils.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/downloader/ConnectorDownloadManagerTest.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/loader/NewDependentProjectConnectorLoaderTest.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/parser/IntegrationProjectDownloadManagerTest.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/resource/finder/LoadDependentResourcesTest.java
chathuranga-jayanath-99
force-pushed
the
load-connector-from-deps-main
branch
from
0741528 to
2e9c151
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/downloader/ConnectorDownloadManagerTest.java (1)
45-49:⚠️ Potential issue | 🟡 MinorUse a partial static mock here, not a full default-value mock.
mockStatic(Utils.class)makes every un-stubbedUtilsmethod return Mockito defaults, so these tests no longer exercise real helpers likeUtils.getHash(...). The code callsUtils.getHash(projectPath)at line 51 ofConnectorDownloadManager.downloadDependencies()to construct the project ID, but this now returns null instead of the actual hash. PreferCALLS_REAL_METHODShere, then stub only the download path you want to isolate.Proposed fix
+import static org.mockito.Mockito.CALLS_REAL_METHODS; import static org.mockito.Mockito.mockStatic; @@ - utilsMock = mockStatic(Utils.class); + utilsMock = mockStatic(Utils.class, CALLS_REAL_METHODS); ConnectorHolder.getInstance().clearConnectors();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/downloader/ConnectorDownloadManagerTest.java` around lines 45 - 49, In setUp() of ConnectorDownloadManagerTest replace the full static mock of Utils with a partial static mock so real helper methods like Utils.getHash(projectPath) are still executed; use mockStatic(Utils.class, CALLS_REAL_METHODS) (or equivalent) and then explicitly stub only the download-related static methods you want isolated, ensuring ConnectorDownloadManager.downloadDependencies() can compute the project ID via Utils.getHash(...) while the download path is mocked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/TestUtils.java`:
- Around line 53-67: The deleteRecursively method declares throws IOException
but currently converts any IOException during deletion into UncheckedIOException
inside the stream lambda (Files.walk(...).sorted(...).forEach(...)), breaking
the checked-exception contract; change the implementation so any
UncheckedIOException thrown from the forEach is caught after the stream closes
and rethrown as an IOException (e.g. wrap the try-with-resources around
Files.walk(path) and catch UncheckedIOException, then rethrow its getCause() if
it's an IOException or wrap it in a new IOException) so callers still receive an
IOException from deleteRecursively.
---
Outside diff comments:
In
`@org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/downloader/ConnectorDownloadManagerTest.java`:
- Around line 45-49: In setUp() of ConnectorDownloadManagerTest replace the full
static mock of Utils with a partial static mock so real helper methods like
Utils.getHash(projectPath) are still executed; use mockStatic(Utils.class,
CALLS_REAL_METHODS) (or equivalent) and then explicitly stub only the
download-related static methods you want isolated, ensuring
ConnectorDownloadManager.downloadDependencies() can compute the project ID via
Utils.getHash(...) while the download path is mocked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8561a4c-4a3c-44e3-9dc0-3e1194853cb3
📒 Files selected for processing (6)
org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/utils/Utils.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/TestUtils.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/downloader/ConnectorDownloadManagerTest.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/loader/NewDependentProjectConnectorLoaderTest.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/parser/IntegrationProjectDownloadManagerTest.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/resource/finder/LoadDependentResourcesTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/loader/NewDependentProjectConnectorLoaderTest.java
Comment on lines
+53
to
+67
| public static void deleteRecursively(Path path) throws IOException { | ||
|
|
||
| if (path == null || !Files.exists(path)) { | ||
| return; | ||
| } | ||
| try (Stream<Path> paths = Files.walk(path)) { | ||
| paths.sorted(Comparator.reverseOrder()) | ||
| .forEach(p -> { | ||
| try { | ||
| Files.deleteIfExists(p); | ||
| } catch (IOException e) { | ||
| throw new UncheckedIOException(e); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Keep deleteRecursively’s checked-exception contract consistent.
This method declares throws IOException, but any delete failure inside the stream escapes as UncheckedIOException instead. That makes teardown failures surface through a different exception type than callers expect.
Proposed fix
public static void deleteRecursively(Path path) throws IOException {
if (path == null || !Files.exists(path)) {
return;
}
try (Stream<Path> paths = Files.walk(path)) {
- paths.sorted(Comparator.reverseOrder())
- .forEach(p -> {
- try {
- Files.deleteIfExists(p);
- } catch (IOException e) {
- throw new UncheckedIOException(e);
- }
- });
+ for (java.util.Iterator<Path> it = paths.sorted(Comparator.reverseOrder()).iterator(); it.hasNext();) {
+ Files.deleteIfExists(it.next());
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/TestUtils.java`
around lines 53 - 67, The deleteRecursively method declares throws IOException
but currently converts any IOException during deletion into UncheckedIOException
inside the stream lambda (Files.walk(...).sorted(...).forEach(...)), breaking
the checked-exception contract; change the implementation so any
UncheckedIOException thrown from the forEach is caught after the stream closes
and rethrown as an IOException (e.g. wrap the try-with-resources around
Files.walk(path) and catch UncheckedIOException, then rethrow its getCause() if
it's an IOException or wrap it in a new IOException) so callers still receive an
IOException from deleteRecursively.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Improve how connectors and resources are loaded from integration project dependencies, and enhance the overall dependency download and management flow.
Porting changes from the below PRs
#516
#517
#521
#522
Changes
Connector loading from dependencies (PR #516)
USER_HOMEin the download flow to support isolated testingConnector origin flag (PR #517)
fromProjectflag to the connector response payload to indicate whether a connector originates from the current project or from a dependent integration projectSkip redundant connector downloads (PR #521)
Conflict detection for dependent resource loading (PR #522)
Tests
fromProjectflag)Related Issues