Add FTP integration improvements#718
Merged
NipunaRanasinghe merged 18 commits intoballerina-platform:1.7.xfrom Feb 20, 2026
Merged
Add FTP integration improvements#718NipunaRanasinghe merged 18 commits intoballerina-platform:1.7.xfrom
NipunaRanasinghe merged 18 commits intoballerina-platform:1.7.xfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a deprecated flag to parameter metadata and propagates it through model-generator-commons; integrates FTP support (router wiring, FTPFunctionBuilder, FTPServiceBuilder) with JSON templates and annotation-driven postProcessAction handling; filters deprecated parameters from listener/property generation; updates FTP JSON schemas and re-enables tests to update expected configs on failure. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client/UI
participant Router as FunctionBuilderRouter
participant SBuilder as FTPServiceBuilder
participant FBuilder as FTPFunctionBuilder
participant Template as FTP JSON Template
participant Store as ServiceModelStore
UI->>Router: request build for FTP service
Router->>SBuilder: select or create compatible listener & service config
SBuilder->>Store: query existing listeners (compatibility)
SBuilder->>SBuilder: decide reuse vs new listener, emit `@ftp:ServiceConfig` if needed
Router->>FBuilder: route function build for FTP functions
FBuilder->>Template: load ftp_service.json template
FBuilder->>FBuilder: merge template into function model (params, return, postProcessAction)
FBuilder->>Store: save/update function model and attach annotations
Store-->>UI: return built service model (with postProcessAction + listener config)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
service-model-generator/modules/service-model-generator-ls-extension/src/main/java/io/ballerina/servicemodelgenerator/extension/builder/service/FTPServiceBuilder.java (1)
325-360:⚠️ Potential issue | 🟠 MajorFix function-node lookup to avoid collisions for resource functions sharing the same accessor.
The current map key using
functionName().text()causes collisions when multiple resource functions share the same accessor (e.g.,get("/users")andget("/products")both key to"get"). This can attach post-process annotations to the wrong function or skip them entirely.Use line ranges instead, which are unique identifiers available in both the
FunctionDefinitionNodeand thesourceFunc.getCodedata():🔧 Suggested fix
+import io.ballerina.tools.text.LineRange; @@ - Map<String, FunctionDefinitionNode> functionNodes = new HashMap<>(); - for (FunctionDefinitionNode functionNode : extractFunctionNodesFromSource(serviceNode)) { - functionNodes.put(functionNode.functionName().text().trim(), functionNode); - } + Map<LineRange, FunctionDefinitionNode> functionNodes = new HashMap<>(); + for (FunctionDefinitionNode functionNode : extractFunctionNodesFromSource(serviceNode)) { + functionNodes.put(functionNode.lineRange(), functionNode); + } @@ - FunctionDefinitionNode functionNode = functionNodes.get(sourceFuncName); + LineRange fnRange = sourceFunc.getCodedata() != null + ? sourceFunc.getCodedata().getLineRange() : null; + FunctionDefinitionNode functionNode = fnRange != null + ? functionNodes.get(fnRange) : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service-model-generator/modules/service-model-generator-ls-extension/src/main/java/io/ballerina/servicemodelgenerator/extension/builder/service/FTPServiceBuilder.java` around lines 325 - 360, The function-node lookup uses functionName().text() as the map key which collides for resource functions with the same accessor; change the keying in the population of functionNodes (from extractFunctionNodesFromSource and FunctionDefinitionNode) to use a unique line-range identifier (e.g., functionNode.lineRange().toString()) and when looking up the node for a source Function use the sourceFunc.getCodedata().getLineRange() (or its toString()) to retrieve the correct FunctionDefinitionNode before calling updatePostProcessActionsFromAnnotation; update all references to functionNodes.put(...) and functionNodes.get(...) accordingly so lookups use line ranges instead of functionName().text().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@model-generator-commons/src/main/java/io/ballerina/modelgenerator/commons/PackageUtil.java`:
- Around line 118-124: The code in getSemanticModel currently catches Throwable
from getModulePackage; change that to catch Exception instead and handle
interrupts properly: replace "catch (Throwable ignored)" with "catch (Exception
e)" then if (e instanceof InterruptedException) re-set the interrupt via
Thread.currentThread().interrupt(); finally return Optional.empty(); this keeps
getSemanticModel and getModulePackage references intact while avoiding
swallowing fatal Errors.
In
`@service-model-generator/modules/service-model-generator-ls-extension/src/main/java/io/ballerina/servicemodelgenerator/extension/builder/service/FTPServiceBuilder.java`:
- Around line 688-709: The compatibility scan in
isListenerCompatibleWithNewPattern only iterates the default module (variable
defaultModule) and will miss services in non-default modules; change the logic
to iterate all modules of the current package (or at minimum the listener's
owning module) instead of only defaultModule, and for each Module repeat the
existing DocumentId -> Document -> ModulePartNode -> members() scanning and
hasServiceConfigAnnotation checks so legacy services in non-default modules are
detected.
- Around line 636-643: The current suffix-only matcher in findAnnotationBySuffix
(used for endsWith("FunctionConfig") and endsWith("ServiceConfig")) can falsely
match annotations from other modules; instead resolve the annotation symbol via
the semantic model (or at least inspect the annotation's module
prefix/qualifier) and verify it belongs to the FTP module/namespace before
accepting it. Update findAnnotationBySuffix/its call sites to retrieve the
AnnotationNode's resolved symbol (via the semantic model APIs available in the
project) and confirm the symbol's module name/organization (or compare the
annotation's qualifier/alias) equals the FTP module identifier, returning the
annotation only on a positive match.
---
Outside diff comments:
In
`@service-model-generator/modules/service-model-generator-ls-extension/src/main/java/io/ballerina/servicemodelgenerator/extension/builder/service/FTPServiceBuilder.java`:
- Around line 325-360: The function-node lookup uses functionName().text() as
the map key which collides for resource functions with the same accessor; change
the keying in the population of functionNodes (from
extractFunctionNodesFromSource and FunctionDefinitionNode) to use a unique
line-range identifier (e.g., functionNode.lineRange().toString()) and when
looking up the node for a source Function use the
sourceFunc.getCodedata().getLineRange() (or its toString()) to retrieve the
correct FunctionDefinitionNode before calling
updatePostProcessActionsFromAnnotation; update all references to
functionNodes.put(...) and functionNodes.get(...) accordingly so lookups use
line ranges instead of functionName().text().
---
Duplicate comments:
In
`@service-model-generator/modules/service-model-generator-ls-extension/src/main/resources/services/ftp_service.json`:
- Around line 1044-1321: The JSON contains a duplicated postProcessAction schema
block; remove the redundant copy and keep a single canonical "postProcessAction"
definition (including its nested "onSuccess" and "onError" choices and "moveTo"
properties) so the model isn't duplicated. Locate the second "postProcessAction"
object within the service definition (the block that mirrors the earlier one)
and delete it, ensuring references to "postProcessAction", "onSuccess",
"onError", and the "moveTo" property remain defined only once and the
surrounding JSON remains valid.
- Around line 1349-1632: This block duplicates the existing postProcessAction
schema (properties.postProcessAction with nested onSuccess/onError choices and
moveTo fields); remove the duplicate definition or consolidate by referencing
the original schema instead so only one postProcessAction structure exists,
ensuring the parameter list and returnType remain unchanged and that symbols
like postProcessAction, onSuccess, onError, and moveTo are preserved exactly in
the single canonical definition.
- Around line 733-999: The JSON contains a duplicated postProcessAction schema
block; locate the second "postProcessAction" occurrence (property name
"postProcessAction" inside the service JSON and its nested "onSuccess"/"onError"
choices) and remove or replace it with a reference to the first canonical
definition to avoid duplication — either delete the repeated object entirely or
replace it with a shared schema reference (e.g., reuse the initial
postProcessAction object or point to a common definition key) so only one
authoritative "postProcessAction" structure remains.
- Around line 420-586: The postProcessAction block under properties is
duplicated (same structure as the earlier postProcessAction); remove the
redundant copy or refactor to a single reusable definition and replace the
duplicate with a reference. Locate the "postProcessAction" property object and
either delete the repeated instance or extract its schema into a shared
definition and point the second occurrence to that definition (using a $ref or
common properties) so there is a single authoritative schema for
postProcessAction.
model-generator-commons/src/main/java/io/ballerina/modelgenerator/commons/PackageUtil.java
Outdated
Show resolved
Hide resolved
...ain/java/io/ballerina/servicemodelgenerator/extension/builder/service/FTPServiceBuilder.java
Outdated
Show resolved
Hide resolved
Comment on lines
688
to
709
| Module defaultModule = project.currentPackage().getDefaultModule(); | ||
| for (DocumentId documentId : defaultModule.documentIds()) { | ||
| Document document = defaultModule.document(documentId); | ||
| ModulePartNode modulePartNode = document.syntaxTree().rootNode(); | ||
|
|
||
| for (ModuleMemberDeclarationNode member : modulePartNode.members()) { | ||
| if (member.kind() != SyntaxKind.SERVICE_DECLARATION) { | ||
| continue; | ||
| } | ||
|
|
||
| ServiceDeclarationNode serviceNode = (ServiceDeclarationNode) member; | ||
| if (isServiceAttachedToListener(serviceNode, listenerName)) { | ||
| // Check if this service uses the new pattern (has @ftp:ServiceConfig annotation) | ||
| if (!hasServiceConfigAnnotation(serviceNode)) { | ||
| return false; // Legacy service found | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return true; // No legacy services found | ||
| } |
There was a problem hiding this comment.
Compatibility scan ignores non-default modules.
isListenerCompatibleWithNewPattern only scans the default module. If legacy services or listeners live in non-default modules, they’ll be missed and the listener could be incorrectly classified as compatible. Consider iterating all modules (or the listener’s owning module) when scanning service declarations.
🔧 Possible fix (scan all modules)
- Module defaultModule = project.currentPackage().getDefaultModule();
- for (DocumentId documentId : defaultModule.documentIds()) {
- Document document = defaultModule.document(documentId);
- ModulePartNode modulePartNode = document.syntaxTree().rootNode();
+ for (Module module : project.currentPackage().modules()) {
+ for (DocumentId documentId : module.documentIds()) {
+ Document document = module.document(documentId);
+ ModulePartNode modulePartNode = document.syntaxTree().rootNode();
@@
- }
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@service-model-generator/modules/service-model-generator-ls-extension/src/main/java/io/ballerina/servicemodelgenerator/extension/builder/service/FTPServiceBuilder.java`
around lines 688 - 709, The compatibility scan in
isListenerCompatibleWithNewPattern only iterates the default module (variable
defaultModule) and will miss services in non-default modules; change the logic
to iterate all modules of the current package (or at minimum the listener's
owning module) instead of only defaultModule, and for each Module repeat the
existing DocumentId -> Document -> ModulePartNode -> members() scanning and
hasServiceConfigAnnotation checks so legacy services in non-default modules are
detected.
Contributor
Author
There was a problem hiding this comment.
ATM, we do not expect any projects with non-default module. @samithkavishke Please confirm
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Contributor
There was a problem hiding this comment.
listenerSymbol.get().getLocation().get().lineRange() we can use this to find the document filePath and all the information
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@service-model-generator/modules/service-model-generator-ls-extension/src/main/java/io/ballerina/servicemodelgenerator/extension/builder/service/FTPServiceBuilder.java`:
- Around line 595-609: The applyPostProcessAction method currently only handles
mapping constructors and a fragile suffix check for ACTION_DELETE; update it to
explicitly detect constant reference nodes so qualified and unqualified
MOVE/DELETE values are handled. Inside applyPostProcessAction, add instanceof
branches that check for QualifiedNameReferenceNode and SimpleNameReferenceNode
on valueExpr, extract the referenced identifier text, and call
selectPostProcessChoice(actionProperty, ACTION_MOVE, extractMoveProperties(...))
when the identifier equals ACTION_MOVE and
selectPostProcessChoice(actionProperty, ACTION_DELETE, null) when it equals
ACTION_DELETE; preserve the existing mapping constructor branch (mappingExpr)
and keep using extractMoveProperties for MOVE mapping handling. Ensure you
reference the constants ACTION_MOVE and ACTION_DELETE and reuse
selectPostProcessChoice and extractMoveProperties when applicable.
---
Duplicate comments:
In
`@service-model-generator/modules/service-model-generator-ls-extension/src/main/java/io/ballerina/servicemodelgenerator/extension/builder/service/FTPServiceBuilder.java`:
- Around line 705-732: The compatibility check in
isListenerCompatibleWithNewPattern currently scans only
project.currentPackage().getDefaultModule(), which misses services in other
modules; update the logic to iterate all modules in
project.currentPackage().modules() (or at least the module owning the listener
found via findListenerSymbol) and scan each Module's documentIds for
ServiceDeclarationNode entries, using existing helpers
isServiceAttachedToListener and hasServiceConfigAnnotation to determine
compatibility so legacy services in non-default modules are detected.
...ain/java/io/ballerina/servicemodelgenerator/extension/builder/service/FTPServiceBuilder.java
Show resolved
Hide resolved
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
`@service-model-generator/modules/service-model-generator-ls-extension/src/main/resources/services/ftp_init.json`:
- Around line 625-656: The codedata for the "path" field currently sets
originalName to "ServiceConfig", which is incorrect for the Ballerina FTP
library; update the codedata.originalName value on the "path" field so it reads
"ListenerConfiguration" (i.e., locate the "path" object and change
codedata.originalName from "ServiceConfig" to "ListenerConfiguration" so
templates referencing ftp:ListenerConfiguration will work).
- Around line 8-10: Update the FTP metadata in ftp_init.json by changing the
"version" field value from "2.17.0" to "2.16.0" and update the "icon" URL
accordingly to
"https://bcentral-packageicons.azureedge.net/images/ballerina_ftp_2.16.0.png";
edit the keys "version" and "icon" in the JSON object shown in the diff so
metadata resolution and UI icon rendering use the published 2.16.0 package.
...rator/modules/service-model-generator-ls-extension/src/main/resources/services/ftp_init.json
Show resolved
Hide resolved
...rator/modules/service-model-generator-ls-extension/src/main/resources/services/ftp_init.json
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
service-model-generator/modules/service-model-generator-ls-extension/src/main/java/io/ballerina/servicemodelgenerator/extension/builder/service/FTPServiceBuilder.java (1)
221-274: Consider extracting listener declaration building to a helper method.The listener declaration building logic (lines 221-274) is lengthy and handles multiple conditional paths. While functionally correct, extracting this to a dedicated
buildListenerDeclarationmethod would improve readability and testability ofaddServiceInitSource.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service-model-generator/modules/service-model-generator-ls-extension/src/main/java/io/ballerina/servicemodelgenerator/extension/builder/service/FTPServiceBuilder.java` around lines 221 - 274, Extract the long StringBuilder logic that constructs listenerDeclaration into a new helper method named buildListenerDeclaration that takes the required inputs (listenerVarName, selectedProtocol, host, username, password, privateKey, secureSocket, port) and returns the built String; replace the inlined block in addServiceInitSource with a single call like listenerDeclaration = buildListenerDeclaration(...). Make the helper method private (or private static) inside FTPServiceBuilder, move all conditional branches and StringBuilder manipulation there unchanged, and ensure addServiceInitSource imports/uses the returned String so behavior and variable names (listenerVarName, listenerDeclaration) remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@service-model-generator/modules/service-model-generator-ls-extension/src/main/java/io/ballerina/servicemodelgenerator/extension/builder/service/FTPServiceBuilder.java`:
- Around line 716-737: The compatibility scan in
isListenerCompatibleWithNewPattern only iterates the default module (Module
defaultModule = project.currentPackage().getDefaultModule()), so legacy services
in other modules are ignored; update the method to iterate all modules in the
package (e.g., for each Module module : project.currentPackage().modules()) and
then iterate each module.documentIds() and their ModulePartNode members, using
the same ServiceDeclarationNode checks and hasServiceConfigAnnotation/
isServiceAttachedToListener logic to ensure no legacy services in submodules are
missed.
- Around line 597-611: applyPostProcessAction currently only handles
MappingConstructorExpressionNode (for MOVE) and uses string suffix matching for
DELETE, missing cases where ACTION_MOVE/ACTION_DELETE are referenced as
QualifiedNameReferenceNode or SimpleNameReferenceNode (e.g., ftp:MOVE). Update
applyPostProcessAction to detect when valueExpr is a QualifiedNameReferenceNode
or SimpleNameReferenceNode, resolve the referenced identifier text (trim and
possibly include qualifier), and call selectPostProcessChoice(actionProperty,
ACTION_MOVE, extractMoveProperties(mappingExpr)) when the identifier equals
ACTION_MOVE, or selectPostProcessChoice(actionProperty, ACTION_DELETE, null)
when it equals ACTION_DELETE; keep existing MappingConstructorExpressionNode
handling and string fallback as a last resort. Ensure you reference the
constants ACTION_MOVE and ACTION_DELETE and reuse extractMoveProperties and
selectPostProcessChoice.
---
Nitpick comments:
In
`@service-model-generator/modules/service-model-generator-ls-extension/src/main/java/io/ballerina/servicemodelgenerator/extension/builder/service/FTPServiceBuilder.java`:
- Around line 221-274: Extract the long StringBuilder logic that constructs
listenerDeclaration into a new helper method named buildListenerDeclaration that
takes the required inputs (listenerVarName, selectedProtocol, host, username,
password, privateKey, secureSocket, port) and returns the built String; replace
the inlined block in addServiceInitSource with a single call like
listenerDeclaration = buildListenerDeclaration(...). Make the helper method
private (or private static) inside FTPServiceBuilder, move all conditional
branches and StringBuilder manipulation there unchanged, and ensure
addServiceInitSource imports/uses the returned String so behavior and variable
names (listenerVarName, listenerDeclaration) remain consistent.
Comment on lines
+93
to
+101
| @Override | ||
| public Function getModelFromSource(ModelFromSourceContext context) { | ||
| Function functionModel = super.getModelFromSource(context); | ||
| if (context.node() instanceof FunctionDefinitionNode functionNode) { | ||
| functionModel = mergeWithTemplate(functionNode, functionModel); | ||
| updatePostProcessActionsFromAnnotation(functionNode, functionModel); | ||
| } | ||
| return functionModel; | ||
| } |
Contributor
There was a problem hiding this comment.
Can you check whether this function is being used or not?
samithkavishke
approved these changes
Feb 20, 2026
Contributor
samithkavishke
left a comment
samithkavishke
left a comment
There was a problem hiding this comment.
There are a few places that need to be updated, but it was approved in alpha with a few issues.
| * Represents the FTP function builder of the service model generator. | ||
| * Handles post-processing actions for FTP functions. | ||
| * | ||
| * @since 1.5.0 |
Contributor
There was a problem hiding this comment.
Suggested change
| * @since 1.5.0 | |
| * @since 1.7.0 |
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
4 participants
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.
Summary
Ports FTP integration improvements to
main.Included changes
Validation
./gradlew :service-model-generator:service-model-generator-ls-extension:compileJava -x spotbugsMainFTP Integration Improvements
Overview
Ports FTP integration enhancements to main to improve service-model generation and UI configurability by adding annotation-driven post-processing, excluding deprecated listener configuration members from generated models, and aligning function-node extraction with databinding for consistent annotation handling.
Key changes
FTP post-processing
Deprecation handling
Function & model processing
Templates, resources & versioning
Tests & tooling
Files of note
Intent / Outcome
Improve FTP service-model generation and editor experience by enabling configurable, annotation-driven post-processing, reducing deprecated configuration noise in generated models, and ensuring consistent function extraction and databinding parsing.