Looking at bug 38115, it feels like the code that is required to use a transport is too protocol dependent. The current sequence is: 1) Initialize the transport (->find) 2) Attempt to connect 3) Retrieve the configured base path ($file_server->upload_diectory) 4) Manually set the working dir for uploading $file_server->change_directory) 5) Upload the file I believe we should (keeping only this FTP/SFTP use case in mind) simplify to: 1) Initialize (->find) 2) Upload the file with an optional path parameter Step (2) would attempt to connect if the connection is not active, set the path to the passed path or internally pick the default and use it. Any failed step should raise a specific exception. This schema could work for other protocols like S3, etc.
Interesting idea's there.. I quite like having the ability to manage connection and navigate should I want to, however I do like the idea of having upload/download able to function without having to manage connection.. I thought I'd done that in one of my iterations actually, but perhaps not.
Created attachment 186858 [details] [review] Bug 40811: Implement simplified Transport API with connection management This commit introduces a simplified API for the Transport classes that reduces the traditional 5-step workflow to a 2-step workflow while maintaining backward compatibility. Key changes: - Centralized connection and directory management in base Transport class - Added _ensure_connected() method for automatic connection handling - Added _auto_change_directory() for intelligent directory management - Refactored all transport methods to use private implementations (_upload_file, etc.) - Added state tracking to respect explicit change_directory() calls - Support for both traditional explicit API and simplified auto-managing API Traditional API (unchanged): $transport->connect(); $transport->change_directory($path); $transport->upload_file($local, $remote); $transport->disconnect(); Simplified API (new): $transport->upload_file($local, $remote, { path => '/custom/path' }); # OR using default upload directory: $transport->upload_file($local, $remote); The implementation intelligently handles three scenarios: 1. Simplified API with custom path - uses provided path 2. Traditional API with explicit directory management - respects user's choice 3. Traditional API without explicit directories - uses configured defaults
Let me know what you think of that Tomas
It looks better. I don't love `_ensure_connected` but I get where the terminology comes from. I'd say it is pretty close to what I proposed.
Created attachment 188035 [details] [review] Bug 40811: Implement simplified Transport API with connection management This commit introduces a simplified API for the Transport classes that reduces the traditional 5-step workflow to a 2-step workflow while maintaining backward compatibility. Key changes: - Centralized connection and directory management in base Transport class - Added _ensure_connected() method for automatic connection handling - Added _auto_change_directory() for intelligent directory management - Refactored all transport methods to use private implementations (_upload_file, etc.) - Added state tracking to respect explicit change_directory() calls - Support for both traditional explicit API and simplified auto-managing API Traditional API (unchanged): $transport->connect(); $transport->change_directory($path); $transport->upload_file($local, $remote); $transport->disconnect(); Simplified API (new): $transport->upload_file($local, $remote, { path => '/custom/path' }); # OR using default upload directory: $transport->upload_file($local, $remote); The implementation intelligently handles three scenarios: 1. Simplified API with custom path - uses provided path 2. Traditional API with explicit directory management - respects user's choice 3. Traditional API without explicit directories - uses configured defaults Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 188152 [details] [review] Bug 40811: Implement dual API for file transports with simplified auto-management This patch implements a dual API design for Koha::File::Transport, providing both a simplified auto-managing API and a traditional explicit API for maximum flexibility and ease of use. DUAL API DESIGN =============== Simplified API (Recommended): - Automatic connection management (no connect/disconnect needed) - Flexible per-operation directory control via options hashref - Stateless operations safe for concurrent usage Example: $transport->upload_file($local, $remote, { path => '/custom/' }); $transport->download_file($remote, $local); # Uses download_directory $transport->list_files({ path => '/incoming/' }); Traditional API (Explicit Control): - Manual connection/directory management when needed - Stateful directory operations via change_directory() - Ideal for multiple operations in the same directory Example: $transport->change_directory('/work/'); $transport->upload_file($local, 'file1.txt'); $transport->upload_file($local, 'file2.txt'); my $files = $transport->list_files(); The APIs can be mixed - calling change_directory() explicitly switches to manual mode and disables auto-management for subsequent operations. STANDARDIZED TEMPLATE METHOD PATTERN ===================================== All transport methods now follow a consistent pattern with clear separation between public API (parent class) and protocol implementation (subclasses). Public methods in Koha::File::Transport: - connect() → Resets directory state, calls _connect() - disconnect() → Resets directory state, calls _disconnect() - change_directory() → Sets manual mode flag, calls _change_directory() - upload_file() → Manages connection/directory, calls _upload_file() - download_file() → Manages connection/directory, calls _download_file() - list_files() → Manages connection/directory, calls _list_files() - rename_file() → Ensures connection, calls _rename_file() Subclass responsibilities (SFTP/FTP/Local): - Implement _connect() - Protocol-specific connection logic - Implement _disconnect() - Protocol-specific disconnection logic - Implement _change_directory() - Protocol-specific directory change - Implement _upload_file() - Protocol-specific upload logic - Implement _download_file() - Protocol-specific download logic - Implement _list_files() - Protocol-specific listing logic - Implement _rename_file() - Protocol-specific rename logic - Implement _is_connected() - Protocol-specific connection check Test plan: prove t/db_dependent/Koha/File/Transports.t Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 188313 [details] [review] Bug 40811: (follow-up) Fix list context bug and add comprehensive tests This patch fixes a critical bug where plain_text_password() was called in list context during hash construction. When a transport has no password, the method's bare 'return' statement returns an empty list () in list context rather than undef, causing all subsequent key-value pairs to shift positions. This resulted in malformed connection parameters being passed to Net::SFTP::Foreign and Net::FTP. Example of the bug: my %hash = ( user => 'john', password => $self->plain_text_password, # Returns () when no password timeout => 10 ); # Results in: (user => 'john', timeout => 10) instead of # (user => 'john', password => undef, timeout => 10) Changes to code: - Wrap plain_text_password calls with scalar() to force scalar context in both FTP.pm and SFTP.pm - Make SFTP key and password authentication mutually exclusive using ternary operator - Prefer key-based auth over password when both are available (SFTP) Test plan: 1. Run t/db_dependent/Koha/File/Transport/FTP.t - should pass 2. Run t/db_dependent/Koha/File/Transport/SFTP.t - should pass 3. Verify tests cover: - Scalar context behavior with no password - FTP authentication with proper parameter passing - SFTP password-only authentication - SFTP key-only authentication - SFTP key preference when both key and password exist