From a5cef5bcdad64f19fd30046965b7e9b53e18d977 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Wed, 24 Sep 2025 09:37:29 +0100 Subject: [PATCH] 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 --- Koha/File/Transport.pm | 239 ++++++++++++++++++++++++++++++++--- Koha/File/Transport/FTP.pm | 45 ++++--- Koha/File/Transport/Local.pm | 46 ++++--- Koha/File/Transport/SFTP.pm | 45 ++++--- 4 files changed, 294 insertions(+), 81 deletions(-) diff --git a/Koha/File/Transport.pm b/Koha/File/Transport.pm index 8a976fd0cd2..96a242d956b 100644 --- a/Koha/File/Transport.pm +++ b/Koha/File/Transport.pm @@ -37,7 +37,20 @@ Koha::File::Transport - Base class for file transport handling =head1 DESCRIPTION -Base class providing common functionality for FTP/SFTP file transport. +Base class providing common functionality for FTP/SFTP/Local file transport. + +This class provides both a traditional explicit API and a simplified auto-managing API. + +Traditional API requires manual connection management: + $transport->connect(); + $transport->change_directory($path); + $transport->upload_file($local, $remote); + $transport->disconnect(); + +Simplified API handles connections automatically: + $transport->upload_file($local, $remote, { path => '/custom/path' }); + # OR using default upload directory: + $transport->upload_file($local, $remote); =cut @@ -92,6 +105,68 @@ sub store { return $self; } +=head3 _ensure_connected + + $transport->_ensure_connected(); + +Internal method that ensures a connection exists, connecting if needed. +Returns true if connected, false if connection failed. + +=cut + +sub _ensure_connected { + my ($self) = @_; + + # Check if already connected (transport-specific) + return 1 if $self->_is_connected(); + + # Reset directory state on new connection + $self->{_user_set_directory} = 0; + + # Attempt to connect + return $self->connect(); +} + +=head3 _is_connected + + my $connected = $transport->_is_connected(); + +Internal method to check if transport is currently connected. +Must be implemented by subclasses. + +=cut + +sub _is_connected { + my ($self) = @_; + die "Subclass must implement _is_connected"; +} + +=head3 _auto_change_directory + + $transport->_auto_change_directory($dir_type, $custom_path); + +Internal method that automatically changes to the appropriate directory. +$dir_type is 'upload' or 'download', $custom_path is optional override. + +=cut + +sub _auto_change_directory { + my ( $self, $dir_type, $custom_path ) = @_; + + my $target_dir; + if ($custom_path) { + $target_dir = $custom_path; + } elsif ( $dir_type eq 'upload' ) { + $target_dir = $self->upload_directory; + } elsif ( $dir_type eq 'download' ) { + $target_dir = $self->download_directory; + } + + return 1 unless $target_dir; # No directory to change to + + return $self->change_directory($target_dir); +} + =head3 plain_text_password my $password = $server->plain_text_password; @@ -201,15 +276,53 @@ sub connect { =head3 upload_file - $transport->upload_file($file); + # Traditional explicit API: + $transport->upload_file($local_file, $remote_file); + + # Simplified auto-managing API: + $transport->upload_file($local_file, $remote_file, { path => '/custom/path' }); -Method for uploading a file to the current file server +Method for uploading a file to the current file server. + +When called with options hashref, automatically handles connection and directory management. +When called without options, uses traditional explicit workflow. =cut sub upload_file { - my ( $self, $local_file, $remote_file ) = @_; - die "Subclass must implement upload_file"; + my ( $self, $local_file, $remote_file, $options ) = @_; + + return unless $self->_ensure_connected(); + + # Only auto-change directory if: + # 1. Options provided with custom path (simplified API), OR + # 2. No explicit directory set by user AND default upload_directory exists (traditional API) + if ( $options && $options->{path} ) { + + # Simplified API - use custom path + return unless $self->_auto_change_directory( 'upload', $options->{path} ); + } elsif ( !$self->{_user_set_directory} ) { + + # Traditional API - use default directory only if user hasn't set one + return unless $self->_auto_change_directory( 'upload', undef ); + } + + return $self->_upload_file( $local_file, $remote_file ); +} + +=head3 _upload_file + + $transport->_upload_file($local_file, $remote_file); + +Internal method that performs the protocol-specific upload operation. +Must be implemented by subclasses. Called by upload_file after connection +and directory management. + +=cut + +sub _upload_file { + my ($self) = @_; + die "Subclass must implement _upload_file"; } =head3 download_file @@ -221,21 +334,70 @@ Method for downloading a file from the current file server =cut sub download_file { - my ( $self, $remote_file, $local_file ) = @_; - die "Subclass must implement download_file"; + my ( $self, $remote_file, $local_file, $options ) = @_; + + return unless $self->_ensure_connected(); + + # Only auto-change directory if: + # 1. Options provided with custom path (simplified API), OR + # 2. No explicit directory set by user AND default download_directory exists (traditional API) + if ( $options && $options->{path} ) { + + # Simplified API - use custom path + return unless $self->_auto_change_directory( 'download', $options->{path} ); + } elsif ( !$self->{_user_set_directory} ) { + + # Traditional API - use default directory only if user hasn't set one + return unless $self->_auto_change_directory( 'download', undef ); + } + + return $self->_download_file( $remote_file, $local_file ); } -=head3 change_directory +=head3 _download_file - my $files = $transport->change_directory($path); + $transport->_download_file($remote_file, $local_file); -Method for changing the current directory on the connected file server +Internal method that performs the protocol-specific download operation. +Must be implemented by subclasses. Called by download_file after connection +and directory management. =cut -sub change_directory { - my ( $self, $path ) = @_; - die "Subclass must implement change_directory"; +sub _download_file { + my ($self) = @_; + die "Subclass must implement _download_file"; +} + +=head3 rename_file + + my $success = $transport->rename_file($old_name, $new_name); + +Method for renaming a file on the current file server + +=cut + +sub rename_file { + my ( $self, $old_name, $new_name ) = @_; + + return unless $self->_ensure_connected(); + + return $self->_rename_file( $old_name, $new_name ); +} + +=head3 _rename_file + + $transport->_rename_file($old_name, $new_name); + +Internal method that performs the protocol-specific file rename operation. +Must be implemented by subclasses. Called by rename_file after connection +verification. + +=cut + +sub _rename_file { + my ($self) = @_; + die "Subclass must implement _rename_file"; } =head3 list_files @@ -247,21 +409,39 @@ Method for listing files in the current directory of the connected file server =cut sub list_files { - my ( $self, $path ) = @_; - die "Subclass must implement list_files"; + my ( $self, $options ) = @_; + + return unless $self->_ensure_connected(); + + # Only auto-change directory if: + # 1. Options provided with custom path (simplified API), OR + # 2. No explicit directory set by user AND default download_directory exists (traditional API) + if ( $options && $options->{path} ) { + + # Simplified API - use custom path + return unless $self->_auto_change_directory( 'download', $options->{path} ); + } elsif ( !$self->{_user_set_directory} ) { + + # Traditional API - use default directory only if user hasn't set one + return unless $self->_auto_change_directory( 'download', undef ); + } + + return $self->_list_files(); } -=head3 rename_file +=head3 _list_files - my $success = $transport->rename_file($old_name, $new_name); + my $files = $transport->_list_files(); -Method for renaming a file on the current file server +Internal method that performs the protocol-specific file listing operation. +Must be implemented by subclasses. Called by list_files after connection +and directory management. =cut -sub rename_file { - my ( $self, $old_name, $new_name ) = @_; - die "Subclass must implement rename_file"; +sub _list_files { + my ($self) = @_; + die "Subclass must implement _list_files"; } =head3 disconnect @@ -272,6 +452,23 @@ Method for disconnecting from the current file server =cut +=head3 change_directory + + my $files = $transport->change_directory($path); + +Method for changing the current directory on the connected file server + +=cut + +sub change_directory { + my ( $self, $path ) = @_; + + # Mark that user has explicitly set a directory + $self->{_user_set_directory} = 1; + + die "Subclass must implement change_directory"; +} + sub disconnect { my ($self) = @_; die "Subclass must implement disconnect"; diff --git a/Koha/File/Transport/FTP.pm b/Koha/File/Transport/FTP.pm index 4107200a16c..17880fb656e 100644 --- a/Koha/File/Transport/FTP.pm +++ b/Koha/File/Transport/FTP.pm @@ -64,17 +64,15 @@ sub connect { return 1; } -=head3 upload_file +=head3 _upload_file - my $success = $transport->upload_file($fh); - -Passed a filehandle, this will upload the file to the current directory of the server connection. +Internal method that performs the FTP-specific upload operation. Returns true on success or undefined on failure. =cut -sub upload_file { +sub _upload_file { my ( $self, $local_file, $remote_file ) = @_; my $operation = "upload"; @@ -95,17 +93,15 @@ sub upload_file { return 1; } -=head3 download_file - - my $success = $transport->download_file($filename); +=head3 _download_file -Passed a filename, this will download the file from the current directory of the server connection. +Internal method that performs the FTP-specific download operation. Returns true on success or undefined on failure. =cut -sub download_file { +sub _download_file { my ( $self, $remote_file, $local_file ) = @_; my $operation = "download"; @@ -156,16 +152,15 @@ sub change_directory { return 1; } -=head3 list_files - - my $files = $server->list_files; +=head3 _list_files -Returns an array reference of hashrefs with file information found in the current directory of the server connection. +Internal method that performs the FTP-specific file listing operation. +Returns an array reference of hashrefs with file information. Each hashref contains: filename, longname, size, perms. =cut -sub list_files { +sub _list_files { my ($self) = @_; my $operation = "list"; @@ -207,17 +202,15 @@ sub list_files { return \@file_list; } -=head3 rename_file +=head3 _rename_file - my $success = $server->rename_file($old_name, $new_name); - -Renames a file on the server connection. +Internal method that performs the FTP-specific file rename operation. Returns true on success or undefined on failure. =cut -sub rename_file { +sub _rename_file { my ( $self, $old_name, $new_name ) = @_; my $operation = "rename"; @@ -253,6 +246,18 @@ sub disconnect { return 1; } +=head3 _is_connected + +Internal method to check if transport is currently connected. + +=cut + +sub _is_connected { + my ($self) = @_; + + return $self->{connection} && $self->{connection}->pwd(); +} + sub _abort_operation { my ( $self, $operation ) = @_; diff --git a/Koha/File/Transport/Local.pm b/Koha/File/Transport/Local.pm index 729a7f4a13b..1822ddaf456 100644 --- a/Koha/File/Transport/Local.pm +++ b/Koha/File/Transport/Local.pm @@ -117,17 +117,15 @@ sub connect { return 1; } -=head3 upload_file +=head3 _upload_file - my $success = $transport->upload_file($local_file, $remote_file); - -Copies a local file to the upload directory. +Internal method that performs the local file system upload operation. Returns true on success or undefined on failure. =cut -sub upload_file { +sub _upload_file { my ( $self, $local_file, $remote_file ) = @_; my $operation = "upload"; @@ -159,17 +157,15 @@ sub upload_file { return 1; } -=head3 download_file - - my $success = $transport->download_file($remote_file, $local_file); +=head3 _download_file -Copies a file from the download directory to a local file. +Internal method that performs the local file system download operation. Returns true on success or undefined on failure. =cut -sub download_file { +sub _download_file { my ( $self, $remote_file, $local_file ) = @_; my $operation = 'download'; @@ -257,16 +253,15 @@ sub change_directory { return 1; } -=head3 list_files - - my $files = $server->list_files; +=head3 _list_files -Returns an array reference of hashrefs with file information found in the current directory. +Internal method that performs the local file system file listing operation. +Returns an array reference of hashrefs with file information. Each hashref contains: filename, longname, size, perms, mtime. =cut -sub list_files { +sub _list_files { my ($self) = @_; my $operation = "list"; @@ -340,17 +335,15 @@ sub list_files { return \@files; } -=head3 rename_file +=head3 _rename_file - my $success = $server->rename_file($old_name, $new_name); - -Renames a file in the current directory. +Internal method that performs the local file system file rename operation. Returns true on success or undefined on failure. =cut -sub rename_file { +sub _rename_file { my ( $self, $old_name, $new_name ) = @_; my $operation = "rename"; @@ -397,6 +390,19 @@ sub rename_file { return 1; } +=head3 _is_connected + +Internal method to check if transport is currently connected. +For local transport, always returns true as local filesystem is always accessible. + +=cut + +sub _is_connected { + my ($self) = @_; + + return 1; # Local filesystem is always "connected" +} + =head3 disconnect $server->disconnect(); diff --git a/Koha/File/Transport/SFTP.pm b/Koha/File/Transport/SFTP.pm index 2424ae21e26..074853d6b5a 100644 --- a/Koha/File/Transport/SFTP.pm +++ b/Koha/File/Transport/SFTP.pm @@ -76,17 +76,15 @@ sub connect { return 1; } -=head3 upload_file +=head3 _upload_file - my $success = $transport->upload_file($fh); - -Passed a filehandle, this will upload the file to the current directory of the server connection. +Internal method that performs the SFTP-specific upload operation. Returns true on success or undefined on failure. =cut -sub upload_file { +sub _upload_file { my ( $self, $local_file, $remote_file ) = @_; my $operation = "upload"; @@ -107,17 +105,15 @@ sub upload_file { return 1; } -=head3 download_file - - my $success = $transport->download_file($filename); +=head3 _download_file -Passed a filename, this will download the file from the current directory of the server connection. +Internal method that performs the SFTP-specific download operation. Returns true on success or undefined on failure. =cut -sub download_file { +sub _download_file { my ( $self, $remote_file, $local_file ) = @_; my $operation = 'download'; @@ -169,16 +165,15 @@ sub change_directory { return 1; } -=head3 list_files - - my $files = $server->list_files; +=head3 _list_files -Returns an array reference of hashrefs with file information found in the current directory of the server connection. +Internal method that performs the SFTP-specific file listing operation. +Returns an array reference of hashrefs with file information. Each hashref contains: filename, longname, a (attributes). =cut -sub list_files { +sub _list_files { my ($self) = @_; my $operation = "list"; @@ -199,17 +194,15 @@ sub list_files { return $file_list; } -=head3 rename_file +=head3 _rename_file - my $success = $server->rename_file($old_name, $new_name); - -Renames a file on the server connection. +Internal method that performs the SFTP-specific file rename operation. Returns true on success or undefined on failure. =cut -sub rename_file { +sub _rename_file { my ( $self, $old_name, $new_name ) = @_; my $operation = "rename"; @@ -232,6 +225,18 @@ sub rename_file { return 1; } +=head3 _is_connected + +Internal method to check if transport is currently connected. + +=cut + +sub _is_connected { + my ($self) = @_; + + return $self->{connection} && !$self->{connection}->error; +} + =head3 disconnect $server->disconnect(); -- 2.51.0