Introduce support to override connector dependencies in descriptor.yml#515
Open
Introduce support to override connector dependencies in descriptor.yml#515
Conversation
WalkthroughAdded a connector configuration subsystem: new JSON-RPC endpoints on SynapseLanguageService delegate to a new ConnectorConfigService that reads/writes project-local connector-config.json, computes effective dependencies by merging descriptor.yml with overrides, and updates download logic to honor overrides/omit flags. Changes
Sequence DiagramsequenceDiagram
actor Client
participant SLS as SynapseLanguageService
participant CCS as ConnectorConfigService
participant FS as Filesystem
participant CH as ConnectorHolder
Client->>SLS: getConnectorDependencies(request)
activate SLS
SLS->>CCS: buildDependencyResponse(projectUri, connectorArtifactId)
activate CCS
CCS->>FS: read connector-config.json
FS-->>CCS: config data (or missing)
CCS->>CH: read descriptor.yml for connector(s)
CH-->>CCS: descriptor dependencies
CCS->>CCS: merge descriptor + overrides, apply omit flags
CCS-->>SLS: ConnectorDependencyResponse
deactivate CCS
SLS-->>Client: CompletableFuture<ConnectorDependencyResponse>
deactivate SLS
Client->>SLS: updateConnectorDependencyOverride(request)
activate SLS
SLS->>CCS: updateDependencyOverride(projectUri, request)
activate CCS
CCS->>FS: read/write connector-config.json (mutation)
FS-->>CCS: success
CCS-->>SLS: Boolean (success)
deactivate CCS
SLS-->>Client: CompletableFuture<Boolean>
deactivate SLS
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new connector-config.json mechanism and corresponding language-service endpoints to let users override connector dependency coordinates (and omit flags) relative to descriptor.yml, so the IDE and build can use consistent effective connector dependencies.
Changes:
- Added a
connector-config.jsonmodel + service to read/write overrides and compute “effective” dependencies (descriptor defaults overlaid with user overrides). - Exposed new JSON-RPC endpoints on
ISynapseLanguageService/SynapseLanguageServiceto query and mutate connector dependency overrides and omit flags. - Applied dependency overrides during driver download so the IDE can use overridden driver coordinates.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/config/ConnectorConfigServiceTest.java | Adds unit tests covering config read/write/init and basic override add/update/reset + lookup behavior. |
| org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/SynapseLanguageService.java | Implements new JSON-RPC endpoints for connector dependency/flag management and config init. |
| org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/ConnectorDownloadManager.java | Applies per-dependency overrides (and omit) while resolving driver coordinates from descriptor.yml. |
| org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/UpdateRootConfigRequest.java | New DTO for root-level omit flags updates. |
| org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/UpdateConnectorFlagsRequest.java | New DTO for connector-level omit flags updates. |
| org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/UpdateConnectorDependencyRequest.java | New DTO for add/update of dependency overrides. |
| org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/ResetConnectorDependencyRequest.java | New DTO for removing dependency overrides. |
| org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/EffectiveDependency.java | New “effective dependency” view for UI rendering (defaults + overrides). |
| org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/DependencyOverride.java | New model for per-dependency override entries persisted in connector-config.json. |
| org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/ConnectorEffectiveData.java | New per-connector effective view (flags + effective dependencies). |
| org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/ConnectorDependencyResponse.java | New response DTO for dependency/flag query endpoint(s). |
| org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/ConnectorDependencyRequest.java | New request DTO for fetching one/all connectors’ effective deps. |
| org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/ConnectorDependencyConfig.java | New persisted per-connector config entry (flags + dependency overrides). |
| org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/ConnectorConfigService.java | Core logic for config read/write, override management, and effective dependency merging. |
| org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/ConnectorConfig.java | Root config model for connector-config.json including global flags. |
| org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/ISynapseLanguageService.java | Adds JSON-RPC API surface for connector dependency override and flag management. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/parser/connectorConfig/ConnectorConfigService.java`:
- Around line 148-190: The updateDependencyOverride method (and the similar
methods resetDependencyOverrides, updateConnectorFlags, updateRootConfig)
perform unsynchronized read-modify-write cycles on the connector-config.json
causing lost updates under concurrent CompletableFuture.supplyAsync calls; wrap
the entire sequence from readConfig(...) through modification to
writeConfig(...) in a single critical section by synchronizing on a dedicated
lock object (e.g., a private static final Object CONNECTOR_CONFIG_LOCK) or using
a ReentrantLock so only one thread at a time executes the read/modify/write for
these methods; apply this same synchronization to resetDependencyOverrides,
updateConnectorFlags, and updateRootConfig to ensure serialized updates and
prevent race conditions.
- Around line 85-96: The current readConfig() in ConnectorConfigService swallows
IO/parse errors (JSON_MAPPER.readValue) and returns a new empty ConnectorConfig,
which risks overwriting a malformed user file on next write; instead, stop
returning a silent fallback: either remove the try/catch so the
IOException/JsonProcessingException bubbles up or catch and rethrow (wrap in an
IOException or UncheckedIOException) after logging via LOGGER, and do not
construct/return the empty ConnectorConfig with CONFIG_VERSION. Update callers
of readConfig() (mutating paths) to handle the propagated exception
appropriately so malformed files are not overwritten.
- Around line 148-160: In updateDependencyOverride (and the similar block in
resetDependencyOverrides) validate the dependency selector before mutating
ConnectorConfig: when request.connectionType is null treat groupId/artifactId as
required and reject (throw or return an error) if either is missing or empty so
you never persist a selector that findMatchingOverride cannot match; for the
reset-all case (when selector is empty/null and you intend to clear all
overrides) only clear connectorCfg.dependencies instead of removing the whole
ConnectorDependencyConfig entry unless both connector-level flags (omit and
omitAllDrivers) are already null, ensuring those flags aren’t accidentally
wiped; reference: updateDependencyOverride, resetDependencyOverrides,
ConnectorDependencyConfig, DependencyOverride, findMatchingOverride.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/ConnectorDownloadManager.java`:
- Around line 202-212: The cache key currently only uses artifactId-version so
changing override.groupId (in the ConnectorDownloadManager block that sets
groupId, artifactId, version from
override.groupId/override.artifactId/override.version) can reuse a wrong jar;
update the cache lookup and storage to include the full Maven coordinates
(groupId:artifactId:version) or, alternatively, evict any entries that match
artifactId-version when override.groupId is present before the exists/check path
is taken so the correct jar is downloaded for the new groupId.
- Around line 197-200: The omit branch in ConnectorDownloadManager currently
returns null (used for errors), which makes callers like
SynapseLanguageService.downloadDriverForConnector unable to distinguish "omitted
by config" from a failure; change the method return to an explicit status/result
(e.g., introduce a DownloadResult enum or a small result class with values like
SUCCESS, OMITTED, FAILURE) and return the OMITTED value from the omit branch in
ConnectorDownloadManager, update
SynapseLanguageService.downloadDriverForConnector to handle the new result type
(check for OMITTED vs FAILURE) and propagate the correct status to callers.
- Around line 183-214: The current validation of groupId, artifactId, and
version runs before applying connector-config.json overrides so a full override
(or omit=true) can still fail; change the flow in ConnectorDownloadManager so
you first read descriptor values into groupId/artifactId/version, apply the
override returned by
ConnectorConfigService.findOverrideByConnectorNameAndConnectionType (handling
DependencyOverride.omit and updating groupId/artifactId/version as shown), and
only then validate the effective coordinates (groupId, artifactId, version) and
log/return null if any are blank; update the LOGGER messages to reference the
final values where appropriate.
🪄 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: 9b588c8f-b394-4dd6-8872-958aaedb938c
📒 Files selected for processing (16)
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/parser/ConnectorDownloadManager.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/ConnectorConfig.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/ConnectorConfigService.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/ConnectorDependencyConfig.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/ConnectorDependencyRequest.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/ConnectorDependencyResponse.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/ConnectorEffectiveData.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/DependencyOverride.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/EffectiveDependency.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/ResetConnectorDependencyRequest.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/UpdateConnectorDependencyRequest.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/UpdateConnectorFlagsRequest.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/UpdateRootConfigRequest.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/config/ConnectorConfigServiceTest.java
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import java.util.Map; | ||
|
|
||
| /** | ||
| * Root model for {@code src/main/wso2mi/connector-config.json}. |
There was a problem hiding this comment.
Javadoc path is incorrect: this model represents src/main/wso2mi/resources/connectors/connector-config.json (per CONFIG_RELATIVE_PATH), not src/main/wso2mi/connector-config.json. Updating the path here will prevent confusion for future maintainers and API consumers.
Suggested change
| * Root model for {@code src/main/wso2mi/connector-config.json}. | |
| * Root model for {@code src/main/wso2mi/resources/connectors/connector-config.json}. |
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.HashMap; | ||
| import java.util.Iterator; |
There was a problem hiding this comment.
java.util.Iterator is imported but not used. If the build uses strict compilation/checkstyle, unused imports can fail CI; please remove it.
Suggested change
| import java.util.Iterator; |
Comment on lines
+478
to
+493
| if (globalOmitAllDrivers) { | ||
| eff.omit = true; | ||
| eff.isOverridden = true; | ||
| } else if (override != null) { | ||
| eff.isOverridden = true; | ||
| eff.omit = Boolean.TRUE.equals(override.omit); | ||
| if (!StringUtils.isBlank(override.groupId)) { | ||
| eff.groupId = override.groupId; | ||
| } | ||
| if (!StringUtils.isBlank(override.artifactId)) { | ||
| eff.artifactId = override.artifactId; | ||
| } | ||
| if (!StringUtils.isBlank(override.version)) { | ||
| eff.overriddenVersion = override.version; | ||
| } | ||
| } |
There was a problem hiding this comment.
mergeDependencies sets eff.isOverridden = true whenever an override entry exists, even if the override doesn't actually change any field (e.g., an override with only connectionType set and all other fields null). This contradicts the EffectiveDependency.isOverridden contract (“when any field differs”) and can cause the UI to show a dependency as overridden when it isn't. Consider computing isOverridden based on actual differences (omit=true, or any non-blank groupId/artifactId/version override) rather than override != null alone.
Comment on lines
+161
to
+190
| boolean hasConnectionType = !StringUtils.isBlank(request.connectionType); | ||
| boolean hasCoordinates = !StringUtils.isBlank(request.groupId) && !StringUtils.isBlank(request.artifactId); | ||
| if (!hasConnectionType && !hasCoordinates) { | ||
| throw new IllegalArgumentException( | ||
| "At least one of connectionType or (groupId + artifactId) must be provided"); | ||
| } | ||
|
|
||
| synchronized (lockFor(projectPath)) { | ||
| ConnectorConfig config = readConfig(projectPath); | ||
| ConnectorDependencyConfig connectorCfg = config.connectors.computeIfAbsent( | ||
| request.connectorArtifactId, k -> new ConnectorDependencyConfig()); | ||
| if (connectorCfg.dependencies == null) { | ||
| connectorCfg.dependencies = new ArrayList<>(); | ||
| } | ||
|
|
||
| // Find existing entry to replace | ||
| DependencyOverride existing = findMatchingOverride( | ||
| connectorCfg.dependencies, request.connectionType, request.groupId, request.artifactId); | ||
|
|
||
| if (existing != null) { | ||
| // Update in-place | ||
| if (request.connectionType != null) { | ||
| existing.connectionType = request.connectionType; | ||
| } | ||
| if (request.groupId != null) { | ||
| existing.groupId = request.groupId; | ||
| } | ||
| if (request.artifactId != null) { | ||
| existing.artifactId = request.artifactId; | ||
| } |
There was a problem hiding this comment.
updateDependencyOverride updates fields whenever they are non-null, but doesn't treat blank strings as “unset”. This can persist connectionType="" (or blank coords) into an existing override, which then breaks matching later because findMatchingOverride requires o.connectionType == null for coordinate-based matches. Consider normalizing blank strings to null (or rejecting them) before updating/creating overrides, and only writing fields when !StringUtils.isBlank(...) unless the caller explicitly wants to clear (in which case prefer null).
Comment on lines
+533
to
+540
| public static void updateConnectorFlags(String projectPath, UpdateConnectorFlagsRequest request) | ||
| throws IOException { | ||
|
|
||
| synchronized (lockFor(projectPath)) { | ||
| ConnectorConfig config = readConfig(projectPath); | ||
| ConnectorDependencyConfig connectorCfg = config.connectors.computeIfAbsent( | ||
| request.connectorArtifactId, k -> new ConnectorDependencyConfig()); | ||
|
|
There was a problem hiding this comment.
updateConnectorFlags will throw a NullPointerException if request.connectorArtifactId is null/blank because computeIfAbsent does not accept a null key. Since this is a JSON-RPC surface, please validate connectorArtifactId up-front (similar to updateDependencyOverride) and either no-op or throw an IllegalArgumentException with a clear message.
Comment on lines
+113
to
+119
| public static void writeConfig(String projectPath, ConnectorConfig config) throws IOException { | ||
|
|
||
| File configFile = configFile(projectPath); | ||
| configFile.getParentFile().mkdirs(); | ||
| JSON_MAPPER.writerWithDefaultPrettyPrinter().writeValue(configFile, config); | ||
| LOGGER.log(Level.INFO, "connector-config.json written to: " + configFile.getAbsolutePath()); | ||
| } |
There was a problem hiding this comment.
writeConfig writes directly to the final JSON file. Because readConfig (and callers like isOmitAllDrivers / findOverrideByConnectorNameAndConnectionType) do not synchronize on the same per-project lock, a read can interleave with a write and observe partially-written JSON, triggering the “malformed” IllegalStateException. Consider writing to a temp file and atomically moving it into place (or synchronizing readConfig with the same lock) to avoid transient parse failures under concurrent access.
Comment on lines
+22
to
+23
| public String projectPath; | ||
|
|
There was a problem hiding this comment.
This request DTO contains projectPath, but the server-side implementation (SynapseLanguageService) always uses its own projectUri and ignores request.projectPath (no references in the codebase). Keeping an unused field in a public JSON-RPC contract is misleading and risks clients assuming they can target other projects. Consider removing projectPath from the request, or actually honoring/validating it.
Suggested change
| public String projectPath; |
Comment on lines
+22
to
+23
| public String projectPath; | ||
|
|
There was a problem hiding this comment.
This request DTO contains projectPath, but the server-side implementation ignores it and always updates the current workspace project (projectUri). As a public JSON-RPC API this is confusing; consider removing projectPath or validating/using it to avoid clients thinking they can modify other projects.
Suggested change
| public String projectPath; |
Comment on lines
+533
to
+580
| public static void updateConnectorFlags(String projectPath, UpdateConnectorFlagsRequest request) | ||
| throws IOException { | ||
|
|
||
| synchronized (lockFor(projectPath)) { | ||
| ConnectorConfig config = readConfig(projectPath); | ||
| ConnectorDependencyConfig connectorCfg = config.connectors.computeIfAbsent( | ||
| request.connectorArtifactId, k -> new ConnectorDependencyConfig()); | ||
|
|
||
| if (request.omit != null) { | ||
| connectorCfg.omit = request.omit ? Boolean.TRUE : null; | ||
| } | ||
| if (request.omitAllDrivers != null) { | ||
| connectorCfg.omitAllDrivers = request.omitAllDrivers ? Boolean.TRUE : null; | ||
| } | ||
|
|
||
| // If all fields on the connector config are now null/empty, remove the entry | ||
| if (connectorCfg.omit == null && connectorCfg.omitAllDrivers == null | ||
| && (connectorCfg.dependencies == null || connectorCfg.dependencies.isEmpty())) { | ||
| config.connectors.remove(request.connectorArtifactId); | ||
| } | ||
|
|
||
| writeConfig(projectPath, config); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Updates root-level flags (omitAllDrivers, omitAllConnectors) in connector-config.json. | ||
| * | ||
| * @param projectPath absolute path to the project root | ||
| * @param request the root config update request | ||
| * @throws IOException if the config file cannot be read or written | ||
| */ | ||
| public static void updateRootConfig(String projectPath, UpdateRootConfigRequest request) | ||
| throws IOException { | ||
|
|
||
| synchronized (lockFor(projectPath)) { | ||
| ConnectorConfig config = readConfig(projectPath); | ||
|
|
||
| if (request.omitAllDrivers != null) { | ||
| config.omitAllDrivers = request.omitAllDrivers ? Boolean.TRUE : null; | ||
| } | ||
| if (request.omitAllConnectors != null) { | ||
| config.omitAllConnectors = request.omitAllConnectors ? Boolean.TRUE : null; | ||
| } | ||
|
|
||
| writeConfig(projectPath, config); | ||
| } | ||
| } |
There was a problem hiding this comment.
ConnectorConfigService adds behavior for root/connector flags (updateRootConfig, updateConnectorFlags) and omit-all-drivers resolution (isOmitAllDrivers), but the new test suite focuses on dependency overrides only. Adding unit tests for these flag update paths (including cleanup when setting flags back to null and global-vs-connector precedence) would help prevent regressions.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/ConnectorConfigService.java (1)
250-257:⚠️ Potential issue | 🟠 MajorReset-all should not wipe connector-level flags.
Line 252 removes the entire
ConnectorDependencyConfig, soresetDependencyOverrides()also clearsomit/omitAllDriversinstead of just dependency overrides. Cleardependenciesfirst, and only remove the connector entry when the flags are also null. This is the same issue raised earlier and it is still present.🩹 Suggested fix
} else { - // Remove all overrides for this connector - config.connectors.remove(request.connectorArtifactId); + // Remove all dependency overrides for this connector, but keep connector-level flags + connectorCfg.dependencies.clear(); } if (connectorCfg.dependencies != null && connectorCfg.dependencies.isEmpty() && connectorCfg.omit == null && connectorCfg.omitAllDrivers == null) { config.connectors.remove(request.connectorArtifactId); }🤖 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/connectorConfig/ConnectorConfigService.java` around lines 250 - 257, The code currently removes the entire ConnectorDependencyConfig (via config.connectors.remove(request.connectorArtifactId)) which causes resetDependencyOverrides() to wipe connector-level flags (omit / omitAllDrivers); change the logic so you clear only connectorCfg.dependencies (e.g., set connectorCfg.dependencies = null or connectorCfg.dependencies.clear()) when resetting overrides, and then remove the connector entry only if connectorCfg.dependencies is null/empty AND connectorCfg.omit == null AND connectorCfg.omitAllDrivers == null; update the branches around the existing config.connectors.remove(request.connectorArtifactId) to implement this conditional removal and ensure resetDependencyOverrides() does not delete omit/omitAllDrivers.
🧹 Nitpick comments (1)
org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/config/ConnectorConfigServiceTest.java (1)
130-310: Add regression coverage for the untested selector branches.Every override/reset test here goes through
connectionType, but the service also has separate logic forgroupId+artifactIdmatching and for preserving connector-level flags when all dependency overrides are reset. Those are important branches in this PR, and the current suite would not catch regressions there.🤖 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/config/ConnectorConfigServiceTest.java` around lines 130 - 310, Add unit tests that exercise the code paths which match overrides by groupId+artifactId and the code that preserves connector-level fields when clearing dependency overrides: create tests that call ConnectorConfigService.updateDependencyOverride with an UpdateConnectorDependencyRequest that sets groupId and artifactId (instead of connectionType) and assert the correct DependencyOverride in config.connectors.get(...).dependencies was updated/added; and create tests that call ConnectorConfigService.resetDependencyOverrides with connectionType == null on a connector that has connector-level flags (e.g., a flag stored alongside dependencies) and assert those connector-level flags remain while dependency entries are removed. Use the existing test helpers (writeConfigFile, readConfig) and request classes (UpdateConnectorDependencyRequest, ResetConnectorDependencyRequest) to target updateDependencyOverride and resetDependencyOverrides behavior.
🤖 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/parser/connectorConfig/ConnectorConfigService.java`:
- Around line 85-103: readConfig() can see a partially-written file because only
mutators use the per-project lock used by writeConfig(); callers like
buildDependencyResponse(), getAllEffectiveDependencies(String),
isOmitAllDrivers(), and findOverrideByConnectorNameAndConnectionType() call
readConfig() without that lock and may hit the malformed-file path. Fix by
making reads use the same per-project lock as writeConfig() (i.e., acquire the
project-specific lock at the start of readConfig() and release it after parsing)
or, alternatively, make writeConfig() atomic (write to a temp file and
move/rename it) and ensure readConfig() opens the file atomically; update
readConfig() to use the lock or rely on atomic replace so callers above no
longer see partial files.
- Around line 127-139: The exists() check in initIfAbsent(String projectPath) is
done outside the project lock so a concurrent writer can race and be
overwritten; acquire the project lock via lockFor(projectPath) before
checking/creating the file (i.e., call lockFor(projectPath).lock()/try-finally
and inside the locked section re-check configFile.exists(), create
ConnectorConfig initial, set version, call writeConfig(projectPath, initial),
and only then log; ensure the lock is released in finally and keep the
IOException catch inside the locked block so you don't overwrite a
concurrently-written config.
- Around line 301-305: The for-loops iterate holder.getConnectors() directly
which can throw ConcurrentModificationException if
addConnector()/removeConnector() run concurrently; take a snapshot first by
creating a new ArrayList from ConnectorHolder.getInstance().getConnectors() and
iterate that snapshot instead (apply this change before the loops that call
connectorArtifactId(...) / readDescriptorDependenciesFromPath(...) and the later
loop around lines ~416–418) so the iterations are stable even if the underlying
list mutates.
---
Duplicate comments:
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/ConnectorConfigService.java`:
- Around line 250-257: The code currently removes the entire
ConnectorDependencyConfig (via
config.connectors.remove(request.connectorArtifactId)) which causes
resetDependencyOverrides() to wipe connector-level flags (omit /
omitAllDrivers); change the logic so you clear only connectorCfg.dependencies
(e.g., set connectorCfg.dependencies = null or
connectorCfg.dependencies.clear()) when resetting overrides, and then remove the
connector entry only if connectorCfg.dependencies is null/empty AND
connectorCfg.omit == null AND connectorCfg.omitAllDrivers == null; update the
branches around the existing
config.connectors.remove(request.connectorArtifactId) to implement this
conditional removal and ensure resetDependencyOverrides() does not delete
omit/omitAllDrivers.
---
Nitpick comments:
In
`@org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/config/ConnectorConfigServiceTest.java`:
- Around line 130-310: Add unit tests that exercise the code paths which match
overrides by groupId+artifactId and the code that preserves connector-level
fields when clearing dependency overrides: create tests that call
ConnectorConfigService.updateDependencyOverride with an
UpdateConnectorDependencyRequest that sets groupId and artifactId (instead of
connectionType) and assert the correct DependencyOverride in
config.connectors.get(...).dependencies was updated/added; and create tests that
call ConnectorConfigService.resetDependencyOverrides with connectionType == null
on a connector that has connector-level flags (e.g., a flag stored alongside
dependencies) and assert those connector-level flags remain while dependency
entries are removed. Use the existing test helpers (writeConfigFile, readConfig)
and request classes (UpdateConnectorDependencyRequest,
ResetConnectorDependencyRequest) to target updateDependencyOverride and
resetDependencyOverrides behavior.
🪄 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: 7adf655f-efcf-44db-86ad-870944686e30
📒 Files selected for processing (4)
org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/SynapseLanguageService.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/connectorConfig/ConnectorConfigService.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/connector/config/ConnectorConfigServiceTest.java
✅ Files skipped from review due to trivial changes (1)
- org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/SynapseLanguageService.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/ConnectorDownloadManager.java
Comment on lines
+85
to
+103
| public static ConnectorConfig readConfig(String projectPath) { | ||
|
|
||
| File configFile = configFile(projectPath); | ||
| if (!configFile.exists()) { | ||
| ConnectorConfig empty = new ConnectorConfig(); | ||
| empty.version = CONFIG_VERSION; | ||
| return empty; | ||
| } | ||
| try { | ||
| ConnectorConfig config = JSON_MAPPER.readValue(configFile, ConnectorConfig.class); | ||
| if (config.connectors == null) { | ||
| config.connectors = new HashMap<>(); | ||
| } | ||
| return config; | ||
| } catch (IOException e) { | ||
| LOGGER.log(Level.SEVERE, "connector-config.json is malformed and cannot be read: " + e.getMessage()); | ||
| throw new IllegalStateException( | ||
| "connector-config.json is malformed: " + e.getMessage(), e); | ||
| } |
There was a problem hiding this comment.
Serialize readers with writeConfig().
The per-project lock only protects the mutators. buildDependencyResponse(), getAllEffectiveDependencies(String), isOmitAllDrivers(), and findOverrideByConnectorNameAndConnectionType() still reach readConfig() without that lock, while Lines 115-117 rewrite the JSON file in place. A read that lands mid-write will hit the malformed-file path on Line 101 even though the file was valid before the write started.
Also applies to: 113-118
🤖 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/connectorConfig/ConnectorConfigService.java`
around lines 85 - 103, readConfig() can see a partially-written file because
only mutators use the per-project lock used by writeConfig(); callers like
buildDependencyResponse(), getAllEffectiveDependencies(String),
isOmitAllDrivers(), and findOverrideByConnectorNameAndConnectionType() call
readConfig() without that lock and may hit the malformed-file path. Fix by
making reads use the same per-project lock as writeConfig() (i.e., acquire the
project-specific lock at the start of readConfig() and release it after parsing)
or, alternatively, make writeConfig() atomic (write to a temp file and
move/rename it) and ensure readConfig() opens the file atomically; update
readConfig() to use the lock or rely on atomic replace so callers above no
longer see partial files.
Comment on lines
+127
to
+139
| public static void initIfAbsent(String projectPath) { | ||
|
|
||
| File configFile = configFile(projectPath); | ||
| if (!configFile.exists()) { | ||
| try { | ||
| ConnectorConfig initial = new ConnectorConfig(); | ||
| initial.version = CONFIG_VERSION; | ||
| writeConfig(projectPath, initial); | ||
| LOGGER.log(Level.INFO, "Created initial connector-config.json at: " + configFile.getAbsolutePath()); | ||
| } catch (IOException e) { | ||
| LOGGER.log(Level.WARNING, "Could not create connector-config.json: " + e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
initIfAbsent() can overwrite a just-written config.
Line 130 does the exists() check outside lockFor(projectPath). If another thread writes a populated config before Line 134, this method will still persist the empty bootstrap file and discard that update.
🔒 Suggested fix
public static void initIfAbsent(String projectPath) {
-
- File configFile = configFile(projectPath);
- if (!configFile.exists()) {
- try {
- ConnectorConfig initial = new ConnectorConfig();
- initial.version = CONFIG_VERSION;
- writeConfig(projectPath, initial);
- LOGGER.log(Level.INFO, "Created initial connector-config.json at: " + configFile.getAbsolutePath());
- } catch (IOException e) {
- LOGGER.log(Level.WARNING, "Could not create connector-config.json: " + e.getMessage());
- }
- }
+ synchronized (lockFor(projectPath)) {
+ File configFile = configFile(projectPath);
+ if (configFile.exists()) {
+ return;
+ }
+ try {
+ ConnectorConfig initial = new ConnectorConfig();
+ initial.version = CONFIG_VERSION;
+ writeConfig(projectPath, initial);
+ LOGGER.log(Level.INFO, "Created initial connector-config.json at: " + configFile.getAbsolutePath());
+ } catch (IOException e) {
+ LOGGER.log(Level.WARNING, "Could not create connector-config.json: " + e.getMessage());
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public static void initIfAbsent(String projectPath) { | |
| File configFile = configFile(projectPath); | |
| if (!configFile.exists()) { | |
| try { | |
| ConnectorConfig initial = new ConnectorConfig(); | |
| initial.version = CONFIG_VERSION; | |
| writeConfig(projectPath, initial); | |
| LOGGER.log(Level.INFO, "Created initial connector-config.json at: " + configFile.getAbsolutePath()); | |
| } catch (IOException e) { | |
| LOGGER.log(Level.WARNING, "Could not create connector-config.json: " + e.getMessage()); | |
| } | |
| } | |
| public static void initIfAbsent(String projectPath) { | |
| synchronized (lockFor(projectPath)) { | |
| File configFile = configFile(projectPath); | |
| if (configFile.exists()) { | |
| return; | |
| } | |
| try { | |
| ConnectorConfig initial = new ConnectorConfig(); | |
| initial.version = CONFIG_VERSION; | |
| writeConfig(projectPath, initial); | |
| LOGGER.log(Level.INFO, "Created initial connector-config.json at: " + configFile.getAbsolutePath()); | |
| } catch (IOException e) { | |
| LOGGER.log(Level.WARNING, "Could not create connector-config.json: " + e.getMessage()); | |
| } | |
| } | |
| } |
🤖 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/connectorConfig/ConnectorConfigService.java`
around lines 127 - 139, The exists() check in initIfAbsent(String projectPath)
is done outside the project lock so a concurrent writer can race and be
overwritten; acquire the project lock via lockFor(projectPath) before
checking/creating the file (i.e., call lockFor(projectPath).lock()/try-finally
and inside the locked section re-check configFile.exists(), create
ConnectorConfig initial, set version, call writeConfig(projectPath, initial),
and only then log; ensure the lock is released in finally and keep the
IOException catch inside the locked block so you don't overwrite a
concurrently-written config.
Comment on lines
+301
to
+305
| ConnectorHolder holder = ConnectorHolder.getInstance(); | ||
| for (Connector connector : holder.getConnectors()) { | ||
| String artifactId = connectorArtifactId(connector); | ||
| List<Map<String, Object>> descriptorDeps = readDescriptorDependenciesFromPath( | ||
| connector.getExtractedConnectorPath()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: inspect ConnectorHolder's backing list and its mutators.
# Expected result: getConnectors() returns an unmodifiable view, while other
# methods mutate the same underlying list.
fd 'ConnectorHolder\.java$' -x sed -n '1,220p' {}Repository: wso2/mi-language-server
Length of output: 14499
🏁 Script executed:
fd 'ConnectorConfigService\.java$' -x wc -l {}Repository: wso2/mi-language-server
Length of output: 199
🏁 Script executed:
fd 'ConnectorConfigService\.java$' -x sed -n '290,310p' {}Repository: wso2/mi-language-server
Length of output: 1206
🏁 Script executed:
fd 'ConnectorConfigService\.java$' -x sed -n '410,425p' {}Repository: wso2/mi-language-server
Length of output: 758
🏁 Script executed:
rg '(addConnector|removeConnector|clearConnectors)\(' -nRepository: wso2/mi-language-server
Length of output: 2125
🏁 Script executed:
rg 'ConnectorHolder\.getInstance\(\)' -n | head -20Repository: wso2/mi-language-server
Length of output: 2339
🏁 Script executed:
fd 'AbstractConnectorLoader\.java$' -x sed -n '140,170p' {}Repository: wso2/mi-language-server
Length of output: 1524
🏁 Script executed:
fd 'ConnectorConfigService\.java$' -x sed -n '285,325p' {}Repository: wso2/mi-language-server
Length of output: 1969
🏁 Script executed:
rg 'ExecutorService|Thread|CompletableFuture|async|@Async' -n -A 2 -B 2 org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/parser/connectorConfig/ConnectorConfigService.java | head -40Repository: wso2/mi-language-server
Length of output: 49
Take a snapshot of connectors before iterating to prevent ConcurrentModificationException.
The getConnectors() method returns an unmodifiable wrapper over a mutable ArrayList. Both iterations (lines 301–305 and 416–418) assume the underlying list remains stable; if addConnector() or removeConnector() executes on another thread during iteration, a ConcurrentModificationException will be thrown. Call new ArrayList<>(holder.getConnectors()) before iterating, or add a snapshot method to ConnectorHolder.
🤖 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/connectorConfig/ConnectorConfigService.java`
around lines 301 - 305, The for-loops iterate holder.getConnectors() directly
which can throw ConcurrentModificationException if
addConnector()/removeConnector() run concurrently; take a snapshot first by
creating a new ArrayList from ConnectorHolder.getInstance().getConnectors() and
iterate that snapshot instead (apply this change before the loops that call
connectorArtifactId(...) / readDescriptorDependenciesFromPath(...) and the later
loop around lines ~416–418) so the iterations are stable even if the underlying
list mutates.
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
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
$subject
Summary by CodeRabbit
New Features
Tests