|
Description
Tomás Cohen Arazi (tcohen)
2025-09-15 15:22:37 UTC
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 Created attachment 190460 [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> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 190461 [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 Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> |