Introduce a new resource to fetch all resources belonging to a project#535
Open
chathuranga-jayanath-99 wants to merge 1 commit intowso2:mainfrom
Open
Conversation
A project can contain resources from both the project itself and its integration project dependencies. This resource fetches all the resources that belong to the project.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis pull request introduces a new resource aggregation feature by adding a Sequence Diagram(s)sequenceDiagram
participant Client
participant SynapseLanguageService
participant NewProjectResourceFinder
participant CurrentProject as Current Project<br/>(findAllResources)
participant DependentProjects as Dependent Projects<br/>(getDependentResourcesMap)
Client->>SynapseLanguageService: getAllResources()
SynapseLanguageService->>NewProjectResourceFinder: getAllResources(projectUri)
NewProjectResourceFinder->>CurrentProject: findAllResources()
CurrentProject-->>NewProjectResourceFinder: Map with artifacts, local, registry resources
NewProjectResourceFinder->>DependentProjects: getDependentResourcesMap()
DependentProjects-->>NewProjectResourceFinder: Dependent resources by type
NewProjectResourceFinder->>NewProjectResourceFinder: For each resource type:<br/>merge dependent into main
NewProjectResourceFinder-->>SynapseLanguageService: Aggregated Map<String, ResourceResponse>
SynapseLanguageService-->>Client: CompletableFuture<Map<String, ResourceResponse>>
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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. Review rate limit: 0/1 reviews remaining, refill in 49 minutes and 36 seconds.Comment |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Comment on lines
+340
to
+343
| @Override | ||
| public CompletableFuture<Map<String, ResourceResponse>> getAllResources() { | ||
|
|
||
| Map<String, ResourceResponse> allResources = resourceFinder.getAllResources(projectUri); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| @Override | |
| public CompletableFuture<Map<String, ResourceResponse>> getAllResources() { | |
| Map<String, ResourceResponse> allResources = resourceFinder.getAllResources(projectUri); | |
| @Override | |
| public CompletableFuture<Map<String, ResourceResponse>> getAllResources() { | |
| log.info("Fetching all available resources for project: {}", projectUri); | |
| Map<String, ResourceResponse> allResources = resourceFinder.getAllResources(projectUri); |
Comment on lines
+410
to
+413
| public Map<String, ResourceResponse> getAllResources(String projectPath) { | ||
|
|
||
| return new HashMap<>(); | ||
| } |
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
Suggested change
| public Map<String, ResourceResponse> getAllResources(String projectPath) { | |
| return new HashMap<>(); | |
| } | |
| public Map<String, ResourceResponse> getAllResources(String projectPath) { | |
| log.debug("Getting all resources for project path: {}", projectPath); | |
| return new HashMap<>(); | |
| } |
Comment on lines
+150
to
+153
| @Override | ||
| public Map<String, ResourceResponse> getAllResources(String projectPath) { | ||
|
|
||
| Map<String, ResourceResponse> allResources = findAllResources(projectPath); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
Suggested change
| @Override | |
| public Map<String, ResourceResponse> getAllResources(String projectPath) { | |
| Map<String, ResourceResponse> allResources = findAllResources(projectPath); | |
| @Override | |
| public Map<String, ResourceResponse> getAllResources(String projectPath) { | |
| log.info("Getting all resources for project: " + projectPath); | |
| Map<String, ResourceResponse> allResources = findAllResources(projectPath); |
Comment on lines
+153
to
+155
| Map<String, ResourceResponse> allResources = findAllResources(projectPath); | ||
| Map<String, ResourceResponse> dependentResources = getDependentResourcesMap(); | ||
| for (Map.Entry<String, ResourceResponse> entry : dependentResources.entrySet()) { |
There was a problem hiding this comment.
Log Improvement Suggestion No: 5
Suggested change
| Map<String, ResourceResponse> allResources = findAllResources(projectPath); | |
| Map<String, ResourceResponse> dependentResources = getDependentResourcesMap(); | |
| for (Map.Entry<String, ResourceResponse> entry : dependentResources.entrySet()) { | |
| Map<String, ResourceResponse> allResources = findAllResources(projectPath); | |
| Map<String, ResourceResponse> dependentResources = getDependentResourcesMap(); | |
| log.debug("Found " + allResources.size() + " main resources and " + dependentResources.size() + " dependent resource types"); | |
| for (Map.Entry<String, ResourceResponse> entry : dependentResources.entrySet()) { |
| ResourceResponse depApiResponse = new ResourceResponse(); | ||
| depApiResponse.setResources(new ArrayList<>(List.of(depApi))); | ||
| finder.addDependentResources("api", depApiResponse); | ||
|
|
There was a problem hiding this comment.
Log Improvement Suggestion No: 6
Suggested change
| Map<String, ResourceResponse> allResources = finder.getAllResources(projectPath); | |
| log.info("Retrieved all resources from finder for project path: {}", projectPath); |
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: 2
🤖 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/resourceFinder/AbstractResourceFinder.java`:
- Around line 410-413: The default implementation of
AbstractResourceFinder.getAllResources currently returns an empty map which can
hide bugs; change getAllResources(String projectPath) to delegate to the
existing findAllResources(projectPath) by returning its result (or alternatively
make getAllResources abstract) so subclasses that don't override it inherit
correct behavior; update the method in AbstractResourceFinder to call
findAllResources(projectPath) (or convert getAllResources to abstract) to ensure
consistent resource discovery.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/resourceFinder/NewProjectResourceFinder.java`:
- Around line 143-159: getAllResources currently merges dependent-project data
(via getDependentResourcesMap and mergeResourceResponses) into the project
result, violating the project-only contract; change getAllResources to return
only the map produced by findAllResources(projectPath) and remove the code that
obtains getDependentResourcesMap() and merges entries (references:
getAllResources, findAllResources, getDependentResourcesMap,
mergeResourceResponses) so the returned payload contains only project-owned
resources.
🪄 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: 5f1600af-619f-484b-825f-6767fc6f60e0
📒 Files selected for processing (5)
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/resourceFinder/AbstractResourceFinder.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/resourceFinder/NewProjectResourceFinder.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/resource/finder/ResourceFinderTest.java
Comment on lines
+410
to
+413
| public Map<String, ResourceResponse> getAllResources(String projectPath) { | ||
|
|
||
| return new HashMap<>(); | ||
| } |
There was a problem hiding this comment.
Avoid an empty default implementation for getAllResources.
Returning an empty map here can silently produce incorrect results when a finder does not override this method. Use findAllResources(projectPath) as the default behavior (or make this method abstract).
Suggested fix
public Map<String, ResourceResponse> getAllResources(String projectPath) {
-
- return new HashMap<>();
+ return findAllResources(projectPath);
}📝 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 Map<String, ResourceResponse> getAllResources(String projectPath) { | |
| return new HashMap<>(); | |
| } | |
| public Map<String, ResourceResponse> getAllResources(String projectPath) { | |
| return findAllResources(projectPath); | |
| } |
🤖 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/AbstractResourceFinder.java`
around lines 410 - 413, The default implementation of
AbstractResourceFinder.getAllResources currently returns an empty map which can
hide bugs; change getAllResources(String projectPath) to delegate to the
existing findAllResources(projectPath) by returning its result (or alternatively
make getAllResources abstract) so subclasses that don't override it inherit
correct behavior; update the method in AbstractResourceFinder to call
findAllResources(projectPath) (or convert getAllResources to abstract) to ensure
consistent resource discovery.
Comment on lines
+143
to
+159
| /** | ||
| * Returns all resources in the project, including artifacts, local entries, and registry | ||
| * resources from both the main project and resolved dependent projects. | ||
| * | ||
| * @param projectPath the root directory of the project | ||
| * @return a map where the key is the resource type and the value is the corresponding ResourceResponse | ||
| */ | ||
| @Override | ||
| public Map<String, ResourceResponse> getAllResources(String projectPath) { | ||
|
|
||
| Map<String, ResourceResponse> allResources = findAllResources(projectPath); | ||
| Map<String, ResourceResponse> dependentResources = getDependentResourcesMap(); | ||
| for (Map.Entry<String, ResourceResponse> entry : dependentResources.entrySet()) { | ||
| allResources.computeIfAbsent(entry.getKey(), k -> new ResourceResponse()); | ||
| mergeResourceResponses(allResources.get(entry.getKey()), entry.getValue()); | ||
| } | ||
| return allResources; |
There was a problem hiding this comment.
getAllResources currently includes dependent-project resources, which conflicts with the endpoint objective.
This method merges getDependentResourcesMap() into the response, so the returned payload is not project-only. If the endpoint contract is project-owned resources only, return just findAllResources(projectPath).
Suggested fix
`@Override`
public Map<String, ResourceResponse> getAllResources(String projectPath) {
-
- Map<String, ResourceResponse> allResources = findAllResources(projectPath);
- Map<String, ResourceResponse> dependentResources = getDependentResourcesMap();
- for (Map.Entry<String, ResourceResponse> entry : dependentResources.entrySet()) {
- allResources.computeIfAbsent(entry.getKey(), k -> new ResourceResponse());
- mergeResourceResponses(allResources.get(entry.getKey()), entry.getValue());
- }
- return allResources;
+ return findAllResources(projectPath);
}🤖 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/NewProjectResourceFinder.java`
around lines 143 - 159, getAllResources currently merges dependent-project data
(via getDependentResourcesMap and mergeResourceResponses) into the project
result, violating the project-only contract; change getAllResources to return
only the map produced by findAllResources(projectPath) and remove the code that
obtains getDependentResourcesMap() and merges entries (references:
getAllResources, findAllResources, getDependentResourcesMap,
mergeResourceResponses) so the returned payload contains only project-owned
resources.
chathuranga-jayanath-99
force-pushed
the
load-connector-from-deps-main
branch
from
dd73ad6 to
2bf64da
Compare
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
A project can contain resources from both the project itself and its integration project dependencies. This resource fetches all the resources that belong to the project itself.