Add Post processing action support#1543
Conversation
There was a problem hiding this comment.
Pull request overview
Adds configurable post-processing actions to the FTP listener so files can be automatically deleted or moved after a content handler completes (on success or error), including documentation and new tests.
Changes:
- Extended Ballerina annotations/spec to support
afterProcess/afterErroractions (DELETEorMove). - Implemented annotation parsing and asynchronous execution of post-processing actions in the Java listener runtime.
- Added test coverage and mock-server filesystem setup for post-processing scenarios.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test-utils/src/main/java/io/ballerina/stdlib/ftp/testutils/mockServerUtils/MockFtpServer.java | Pre-creates post-process test directories in the fake FTP filesystem. |
| native/src/main/java/io/ballerina/stdlib/ftp/util/FtpConstants.java | Adds annotation field name constants for post-processing actions. |
| native/src/main/java/io/ballerina/stdlib/ftp/transport/server/connector/contractimpl/RemoteFileSystemServerConnectorImpl.java | Exposes listener path from the server connector implementation. |
| native/src/main/java/io/ballerina/stdlib/ftp/transport/server/RemoteFileSystemConsumer.java | Includes listener path in emitted filesystem events. |
| native/src/main/java/io/ballerina/stdlib/ftp/transport/message/RemoteFileSystemEvent.java | Carries listener path for subdirectory-preserving moves. |
| native/src/main/java/io/ballerina/stdlib/ftp/server/PostProcessAction.java | New model to represent DELETE/MOVE actions with config. |
| native/src/main/java/io/ballerina/stdlib/ftp/server/FtpContentCallbackHandler.java | Executes after-success/after-error file actions after handler invocation. |
| native/src/main/java/io/ballerina/stdlib/ftp/server/FormatMethodsHolder.java | Parses afterProcess/afterError from annotations per handler method. |
| docs/spec/spec.md | Documents the new post-processing feature and configuration. |
| changelog.md | Adds changelog entry for post-processing action support. |
| ballerina/tests/post_process_action_test.bal | Adds integration tests for delete/move post-processing behavior. |
| ballerina/annotations.bal | Extends public annotation API with DELETE, Move, and new fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
native/src/main/java/io/ballerina/stdlib/ftp/server/FormatMethodsHolder.java
Outdated
Show resolved
Hide resolved
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpContentCallbackHandler.java
Outdated
Show resolved
Hide resolved
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpContentCallbackHandler.java
Show resolved
Hide resolved
|
@copilot open a new pull request to apply changes based on the comments in this thread. |
|
@niveathika I've opened a new pull request, #1544, to work on those changes. Once the pull request is ready, I'll request review from you. |
caf45dd to
4fbdfbe
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
native/src/main/java/io/ballerina/stdlib/ftp/transport/server/RemoteFileSystemConsumer.java
Outdated
Show resolved
Hide resolved
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpContentCallbackHandler.java
Show resolved
Hide resolved
81607c6 to
5397545
Compare
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (76.53%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #1543 +/- ##
============================================
- Coverage 76.78% 76.53% -0.25%
- Complexity 769 1043 +274
============================================
Files 58 76 +18
Lines 3928 4833 +905
Branches 653 838 +185
============================================
+ Hits 3016 3699 +683
- Misses 634 760 +126
- Partials 278 374 +96 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5397545 to
181e539
Compare
|
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:
📝 WalkthroughWalkthroughThis PR implements post-processing action support for the Ballerina FTP module. It introduces DELETE and MOVE actions that execute after file processing completes or on errors. The feature includes new type definitions (Move record, DELETE constant), updated FtpFunctionConfig to accept optional afterProcess/afterError actions, service-level configuration support via ServiceConfiguration type and ServiceConfig annotation, corresponding Java implementation for action coordination, and comprehensive test coverage with version bump to 2.17.0. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant FtpListener
participant FtpContentCallbackHandler
participant FormatMethodsHolder
participant PostProcessEngine
participant FtpClient
Client->>FtpListener: File arrives at monitored path
FtpListener->>FtpContentCallbackHandler: processContentBasedCallbacks(fileInfo)
FtpContentCallbackHandler->>FormatMethodsHolder: getAfterProcessAction(methodName)
FormatMethodsHolder-->>FtpContentCallbackHandler: Optional<PostProcessAction>
alt Content Method Success
FtpContentCallbackHandler->>FtpContentCallbackHandler: invokeContentMethodAsync()
FtpContentCallbackHandler->>PostProcessEngine: executePostProcessAction(afterProcess)
PostProcessEngine->>FtpClient: delete(filePath) OR move(filePath, destination)
FtpClient-->>PostProcessEngine: success/error
PostProcessEngine-->>FtpContentCallbackHandler: logged outcome
else Content Method Failure
FtpContentCallbackHandler->>FtpContentCallbackHandler: routeToOnError()
FtpContentCallbackHandler->>FormatMethodsHolder: getAfterErrorAction(methodName)
FormatMethodsHolder-->>FtpContentCallbackHandler: Optional<PostProcessAction>
FtpContentCallbackHandler->>PostProcessEngine: executePostProcessAction(afterError)
PostProcessEngine->>FtpClient: delete(filePath) OR move(filePath, destination)
FtpClient-->>PostProcessEngine: success/error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@ballerina/tests/client_endpoint_test.bal`:
- Around line 1221-1226: resourceNames and fileSizes arrays are out of sync:
resourceNames contains 21 entries while fileSizes only has 20; update the
fileSizes array to include corresponding size entries for the added resource
names ("post-process", "post-process-archive", "post-process-error") so both
arrays align. Locate the resourceNames and fileSizes declarations and append
three size values (use the expected sizes for "post-process" and
"post-process-archive" and set "post-process-error" to 0 if unknown) to the
fileSizes array so its length matches resourceNames.
In `@ballerina/tests/post_process_action_test.bal`:
- Around line 228-294: The test testAfterProcessMoveWithPreserveSubDirsTrue
fails because the destination subdirectory under POST_PROCESS_ARCHIVE_DIR is
never created; before uploading the test file (before check
(<Client>clientEp)->putJson(...)), create the archive subdirectory (e.g., check
(<Client>clientEp)->exists(POST_PROCESS_ARCHIVE_DIR + "/preserve-subdir") and if
missing call check (<Client>clientEp)->mkdir(POST_PROCESS_ARCHIVE_DIR +
"/preserve-subdir")) so the MOVE with preserveSubDirs can succeed; keep cleanup
calls for deleting the archived file and removing the created directories as
already present.
In `@changelog.md`:
- Line 10: Update the changelog entry text "[Add post processing action
support]" to hyphenate the compound modifier by changing "post processing" to
"post-processing" so the line reads "[Add post-processing action support]";
ensure only that phrase is modified and punctuation/links remain unchanged.
🧹 Nitpick comments (4)
native/src/main/java/io/ballerina/stdlib/ftp/transport/message/RemoteFileSystemEvent.java (1)
32-41: Consider constructor chaining to reduce duplication.The new constructor duplicates the field assignments from the original constructor. Using
this(...)would reduce duplication and ensure consistency.♻️ Suggested refactor
public RemoteFileSystemEvent(List<FileInfo> addedFiles, List<String> deletedFiles) { this.addedFiles = addedFiles; this.deletedFiles = deletedFiles; } public RemoteFileSystemEvent(List<FileInfo> addedFiles, List<String> deletedFiles, String listenerPath) { - this.addedFiles = addedFiles; - this.deletedFiles = deletedFiles; + this(addedFiles, deletedFiles); this.listenerPath = listenerPath; }native/src/main/java/io/ballerina/stdlib/ftp/server/FtpListenerHelper.java (1)
166-179: Minor: DuplicateFormatMethodsHolderinstantiation and trailing whitespace.
FormatMethodsHolderis instantiated here for validation and again inFtpListener.dispatchFileEventToService()for dispatch. Consider caching the validated holder in native data to avoid duplicate parsing.Line 179 has trailing whitespace before the
if (!needsCaller)block.♻️ Optional: Cache FormatMethodsHolder to avoid duplicate instantiation
if (contentMethod.isPresent()) { try { FormatMethodsHolder holder = new FormatMethodsHolder(service); if (!needsCaller && holder.hasPostProcessingActions()) { needsCaller = true; } + // Cache for later use in dispatch + service.addNativeData("FORMAT_METHODS_HOLDER", holder); } catch (FtpInvalidConfigException e) { return FtpUtil.createError(e.getMessage(), e, FtpUtil.ErrorType.InvalidConfigError.errorType()); } } - + if (!needsCaller) {native/src/main/java/io/ballerina/stdlib/ftp/server/FtpContentCallbackHandler.java (2)
500-521: Edge case: file path without directory separator.Line 502 extracts the filename using
filePath.lastIndexOf('/'). IffilePathcontains no/(unlikely for FTP paths but possible),lastIndexOfreturns-1andsubstring(0)returns the entire path, which is correct. However, this assumption should be documented.Additionally, the method doesn't handle potential path normalization issues (e.g., double slashes, Windows-style backslashes in edge cases).
♻️ Optional: Add comment documenting the assumption
private String calculateMoveDestination(String filePath, String listenerPath, PostProcessAction action) { String moveTo = action.getMoveTo(); + // Extract filename from path. If no '/' exists, lastIndexOf returns -1, + // and substring(0) returns the entire path which is treated as filename. String fileName = filePath.substring(filePath.lastIndexOf('/') + 1);
333-364: Consider documenting the onError post-processing behavior.The
onErrorhandler can have its ownafterProcess/afterErroractions configured. This means:
- If
onErrorcompletes successfully →afterProcessaction runs- If
onErrorthrows an error →afterErroraction runsThis is semantically correct but may be confusing. Consider adding inline documentation to clarify this behavior for future maintainers.
📝 Optional: Add clarifying comment
+ /** + * Invokes the onError handler method asynchronously with post-processing support. + * Note: The onError handler can have its own post-processing actions configured. + * - If onError completes successfully (no error), afterProcess action is executed. + * - If onError returns/throws an error, afterError action is executed. + */ private void invokeOnErrorMethodAsync(BObject service, String methodName, Object[] methodArguments, FileInfo fileInfo, BObject callerObject, String listenerPath,
181e539 to
c8deea5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/spec/spec.md`:
- Around line 996-1027: Add a documented type alias for the Move record so
readers understand the MOVE identifier used in the FtpFunctionConfig union:
after the Move record definition, add a type alias declaration named MOVE that
aliases Move and include a short doc comment like "Type alias for Move record,
used in union types for post-processing actions" so FtpFunctionConfig's
MOVE|DELETE appearance is clear (referencing the Move record, the MOVE alias,
and the FtpFunctionConfig type).
🧹 Nitpick comments (1)
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpContentCallbackHandler.java (1)
500-521: Consider edge case where file path contains no directory separator.Line 502 uses
lastIndexOf('/')to extract the filename. IffilePathsomehow doesn't contain a/, the result would be-1 + 1 = 0, makingfileNameequal to the entirefilePath. While FTP paths typically include/, this edge case could lead to unexpected behavior.♻️ Suggested defensive improvement
private String calculateMoveDestination(String filePath, String listenerPath, PostProcessAction action) { String moveTo = action.getMoveTo(); - String fileName = filePath.substring(filePath.lastIndexOf('/') + 1); + int lastSlashIndex = filePath.lastIndexOf('/'); + String fileName = lastSlashIndex >= 0 ? filePath.substring(lastSlashIndex + 1) : filePath;
c8deea5 to
c04e082
Compare
f3d9522 to
daad657
Compare
native/src/main/java/io/ballerina/stdlib/ftp/server/FormatMethodsHolder.java
Show resolved
Hide resolved
native/src/main/java/io/ballerina/stdlib/ftp/server/FormatMethodsHolder.java
Outdated
Show resolved
Hide resolved
native/src/main/java/io/ballerina/stdlib/ftp/server/FormatMethodsHolder.java
Outdated
Show resolved
Hide resolved
native/src/main/java/io/ballerina/stdlib/ftp/server/FormatMethodsHolder.java
Outdated
Show resolved
Hide resolved
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpContentCallbackHandler.java
Show resolved
Hide resolved
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpContentCallbackHandler.java
Outdated
Show resolved
Hide resolved
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpContentCallbackHandler.java
Outdated
Show resolved
Hide resolved
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpContentCallbackHandler.java
Outdated
Show resolved
Hide resolved
native/src/main/java/io/ballerina/stdlib/ftp/server/PostProcessAction.java
Show resolved
Hide resolved
3219c74 to
9262845
Compare
f9a25e7 to
7e7e6f2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpContentCallbackHandler.java
Show resolved
Hide resolved
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpListenerHelper.java
Outdated
Show resolved
Hide resolved
native/src/main/java/io/ballerina/stdlib/ftp/util/FtpConstants.java
Outdated
Show resolved
Hide resolved
c909cbb to
7491c09
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@native/src/main/java/io/ballerina/stdlib/ftp/server/FormatMethodsHolder.java`:
- Around line 132-157: In parsePostProcessAction, tighten validation: only treat
actionObj as DELETE when it is a STRING whose value equals the DELETE constant
(don't treat any arbitrary string as delete), verify actionObj is a BMap before
casting to avoid NPEs, validate that moveTo (ANNOTATION_MOVE_TO) is non-null and
non-empty before using it (throw FtpInvalidConfigException with context if
invalid), and implement the preserveSubDirs default to true when
ANNOTATION_PRESERVE_SUB_DIRS is missing or not a boolean (otherwise use the
boolean value); update logic around TypeTags.STRING_TAG, the BMap cast,
ANNOTATION_MOVE_TO and ANNOTATION_PRESERVE_SUB_DIRS, and return
PostProcessAction.delete() or PostProcessAction.move(moveTo, preserveSubDirs)
accordingly.
In `@native/src/main/java/io/ballerina/stdlib/ftp/server/ServiceContext.java`:
- Line 31: The non-volatile field `caller` in ServiceContext can cause
visibility issues between the registration thread and async event handler
threads; make the field declaration for `caller` volatile in the ServiceContext
class so writes from setCaller() are visible to readers calling getCaller() in
dispatchFileEventToService() (the ServiceContext instance published by
FtpListener), ensuring thread-safe visibility across threads.
native/src/main/java/io/ballerina/stdlib/ftp/server/FormatMethodsHolder.java
Show resolved
Hide resolved
native/src/main/java/io/ballerina/stdlib/ftp/server/ServiceContext.java
Outdated
Show resolved
Hide resolved
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpListenerHelper.java
Outdated
Show resolved
Hide resolved
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpListenerHelper.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@native/src/main/java/io/ballerina/stdlib/ftp/server/FtpContentCallbackHandler.java`:
- Around line 451-472: In calculateMoveDestination, handle cases where filePath
ends with '/' by normalizing filePath before extracting fileName and computing
relativePath: trim any trailing slashes from filePath (but preserve a single
leading slash if needed), then compute fileName =
trimmedFilePath.substring(trimmedFilePath.lastIndexOf('/') + 1) and use
trimmedFilePath for normalizedFilePath comparisons; if trimming produces an
empty name (e.g., root path), fall back to using a sensible default (the
original moveTo directory or a placeholder) to avoid returning an empty
filename. Ensure references to action.getMoveTo(), action.isPreserveSubDirs(),
listenerPath, ensureTrailingSlash(moveTo) and normalizedListenerPath remain
consistent.
In `@native/src/main/java/io/ballerina/stdlib/ftp/server/FtpListenerHelper.java`:
- Around line 286-318: The current flow creates a per-service caller (via
createCaller or createCallerWithPath) before calling
listener.addServiceContext(context), which can leak resources if
addServiceContext fails; change the flow so you register the ServiceContext
first (create ServiceContext with caller=null), call
listener.addServiceContext(context) and only on success create the caller (use
createCaller or createCallerWithPath as before), set it on the listener or
context (listener.setCaller(createdCaller) or set into the ServiceContext), and
if caller creation fails return that error; alternatively, if you must create
the caller earlier then ensure you clean it up when addServiceContext returns
false (close/release and do not retain it). Ensure you update references to
ServiceContext, addServiceContext, createCaller, createCallerWithPath and
listener.setCaller accordingly.
🧹 Nitpick comments (1)
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpContentCallbackHandler.java (1)
289-300: Simplify redundant conditional logic.The
hasOnErrorActionscheck is unnecessary since emptyOptionals will behave identically to explicitly passingOptional.empty(). The conditional ternary operators on lines 299-300 don't change the behavior.♻️ Proposed simplification
- Optional<PostProcessAction> onErrorAfterProcess = holder.getAfterProcessAction(onErrorMethod.getName()); - Optional<PostProcessAction> onErrorAfterError = holder.getAfterErrorAction(onErrorMethod.getName()); - boolean hasOnErrorActions = onErrorAfterProcess.isPresent() || onErrorAfterError.isPresent(); - // Prepare arguments for onError method Object[] methodArguments = prepareOnErrorMethodArguments(onErrorMethod, error, callerObject); + Optional<PostProcessAction> onErrorAfterProcess = holder.getAfterProcessAction(onErrorMethod.getName()); + Optional<PostProcessAction> onErrorAfterError = holder.getAfterErrorAction(onErrorMethod.getName()); + // Invoke onError asynchronously and apply afterProcess/afterError actions invokeOnErrorMethodAsync(service, onErrorMethod.getName(), methodArguments, fileInfo, callerObject, - listenerPath, - hasOnErrorActions ? onErrorAfterProcess : Optional.empty(), - hasOnErrorActions ? onErrorAfterError : Optional.empty()); + listenerPath, onErrorAfterProcess, onErrorAfterError);
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpContentCallbackHandler.java
Show resolved
Hide resolved
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpListenerHelper.java
Outdated
Show resolved
Hide resolved
|



Purpose
Part of ballerina-platform/ballerina-library#8604
This pull request introduces support for post-processing actions in the FTP listener, allowing users to automatically move or delete files after they are processed, either on success or error. The changes span the public API, implementation, and documentation, making it easier to configure automated file handling patterns such as archiving or cleanup.
Key changes:
Public API and Annotations
DELETEconstant and newMoverecord type toannotations.bal, and updated theFtpFunctionConfigannotation to supportafterProcessandafterErrorfields for specifying post-processing actions. These fields accept eitherDELETEor aMoverecord, enabling flexible configuration for file handling after processing.Implementation (Java backend)
FormatMethodsHolderto parse and store post-processing actions from annotations, with new methods to retrieve these actions for each content handler. Added logic to handle both delete and move actions, including subdirectory preservation. [1] [2] [3] [4]FtpContentCallbackHandlerto retrieve post-processing actions and invoke content methods asynchronously with support for executing these actions after processing completes, handling both success and error cases. [1] [2] [3] [4]Documentation
afterProcessandafterErrorbehaviors using the new API. [1] [2]Changelog
Examples
Checklist
Summary
This pull request adds post-processing action support to the FTP listener, enabling automatic file handling after processing completes. Users can now configure actions to delete or move files after successful processing or when errors occur.
Key Changes
API Updates:
DELETEconstant andMoverecord type withmoveTo(destination path) andpreserveSubDirs(boolean flag, default true) fieldsFtpFunctionConfigwith optionalafterProcessandafterErrorfields accepting eitherMOVEorDELETEactionsfileNamePatterninFtpFunctionConfigoptionalJava Implementation:
PostProcessActionclass to model and manage delete/move operations with metadataFormatMethodsHolderto parse, validate, and store per-handler post-processing actions with new accessor methods (getAfterProcessAction,getAfterErrorAction,hasPostProcessingActions)FtpContentCallbackHandlerwith post-processing engine: new methods to execute delete actions (removing files via client), move actions (relocating files with optional subdirectory preservation), and calculate move destinations with path normalizationFtpListenerto useServiceContextstructures, replacing per-service maps to better organize service state including service object, configuration, format methods holder, and callerFtpListenerHelperto initialize service contexts and validate post-processing requirements for proper caller setupTesting & Documentation:
post_process_action_test.bal) covering DELETE and MOVE actions, subdirectory preservation modes, error-triggered actions, and combined scenariosVersion & Dependency Updates:
Implementation Details
Post-processing actions execute conditionally after content method invocation:
afterProcessactions on successful processing,afterErroractions when errors occur. The Move action respects thepreserveSubDirssetting to optionally maintain relative subdirectory structures at the destination.