From 996d7bcb4f0c9c40b4006650fa03496685bc729c Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Wed, 24 Sep 2025 09:37:29 +0100 Subject: [PATCH] Bug 40811: Implement dual API for file transports with simplified auto-management MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- Koha/File/Transport.pm | 546 ++++++++++++++++++++++++-- Koha/File/Transport/FTP.pm | 63 +-- Koha/File/Transport/Local.pm | 72 ++-- Koha/File/Transport/SFTP.pm | 63 +-- t/db_dependent/Koha/File/Transports.t | 90 ++++- 5 files changed, 713 insertions(+), 121 deletions(-) diff --git a/Koha/File/Transport.pm b/Koha/File/Transport.pm index 188aaa1ec8e..30375cb24fb 100644 --- a/Koha/File/Transport.pm +++ b/Koha/File/Transport.pm @@ -35,9 +35,128 @@ use base qw(Koha::Object); Koha::File::Transport - Base class for file transport handling +=head1 SYNOPSIS + + use Koha::File::Transports; + + # Create/retrieve a transport (polymorphic - returns SFTP/FTP/Local subclass) + my $transport = Koha::File::Transports->find($id); + + # SIMPLIFIED API (recommended) - connection and directory management are automatic + # Connections are established on-demand and directories are managed automatically + + # Example 1: Upload to custom path (connection happens automatically) + $transport->upload_file('/local/file.txt', 'remote.txt', { path => '/custom/dir/' }); + + # Example 2: Download using configured download_directory (auto-managed) + $transport->download_file('remote.txt', '/local/file.txt'); + + # Example 3: List files in custom directory (one-shot operation) + my $files = $transport->list_files({ path => '/some/directory/' }); + + # TRADITIONAL API - manual connection and directory management + # Useful when you need fine-grained control or want to perform multiple + # operations in the same directory without repeating the path + + $transport->connect(); # Optional - will auto-connect if omitted + $transport->change_directory('/work/dir/'); # Sets working directory + $transport->upload_file('/local/1.txt', '1.txt'); + $transport->upload_file('/local/2.txt', '2.txt'); # Uses same directory + my $files = $transport->list_files(); # Lists /work/dir/ + $transport->rename_file('1.txt', '1_old.txt'); + $transport->download_file('2.txt', '/local/2.txt'); + $transport->disconnect(); # Optional - cleaned up automatically + + # HYBRID APPROACH - mixing both APIs + # Once you explicitly set a directory, auto-management is disabled + + $transport->change_directory('/work/dir/'); # Explicit directory change + $transport->upload_file('/local/file.txt', 'file.txt'); # Uses /work/dir/ + $transport->list_files(); # Still uses /work/dir/ + # The configured upload_directory/download_directory won't be used anymore + =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 supports two distinct usage patterns: + +=head2 Simplified API (Auto-Managing) + +The simplified API automatically manages connections and directories: + +=over 4 + +=item * B + +You never need to call connect() or disconnect(). Connections are established +on-demand when you call file operation methods and are automatically cleaned +up when the object is destroyed. + +=item * B + +Each file operation (upload_file, download_file, list_files) accepts an optional +options hashref with a 'path' key to specify a custom directory for that operation: + + $transport->upload_file($local, $remote, { path => '/custom/dir/' }); + +If no path is provided, the transport uses its configured upload_directory +(for uploads) or download_directory (for downloads/listings). + +=item * B + +Each operation is independent. The transport doesn't remember which directory +you used in previous operations, making the API stateless and safe for +concurrent usage patterns. + +=back + +=head2 Traditional API (Explicit Control) + +The traditional API provides manual control over connection and directory state: + +=over 4 + +=item * B + +While connect() and disconnect() are still available and can be called explicitly, +they are entirely optional. The simplified API manages connections automatically. + +=item * B + +Once you call change_directory() explicitly, the transport switches to "manual mode" +and remembers your working directory. Subsequent operations will use this directory +and automatic directory management is disabled: + + $transport->change_directory('/work/'); + $transport->upload_file($local, $remote); # Uses /work/, not upload_directory + $transport->list_files(); # Lists /work/, not download_directory + +=item * B + +This is useful when you need to perform multiple operations in the same directory +without repeating the path parameter each time. + +=back + +=head2 How It Works + +The implementation uses a C<_user_set_directory> flag to track which mode is active: + +=over 4 + +=item * When you call change_directory() explicitly, the flag is set to true + +=item * When the flag is true, auto-directory management is disabled + +=item * When the flag is false (default), operations use their path option or fall back to configured directories + +=item * The flag is reset to false when a new connection is established + +=back + +This design allows you to mix both approaches in the same codebase, choosing +the right pattern for each use case. =cut @@ -92,6 +211,67 @@ 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(); + + # Attempt to connect (which will reset directory state) + 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 + + # Call the internal _change_directory directly to avoid setting the flag + # Only explicit user calls to change_directory() should set the flag + return $self->_change_directory($target_dir); +} + =head3 plain_text_password my $password = $server->plain_text_password; @@ -188,67 +368,199 @@ Interface methods that must be implemented by subclasses =head3 connect - $transport->connect(); + my $success = $transport->connect(); + +Establishes a connection to the remote server. -Method for connecting the current transport to the file server +B Calling this method is entirely optional. All file operations +(upload_file, download_file, list_files, etc.) automatically establish +a connection if one doesn't already exist. You only need to call this +explicitly if you want to verify connectivity before performing operations. + +B True on success, undef on failure. Check object_messages() for details. =cut sub connect { my ($self) = @_; - die "Subclass must implement connect"; + + # Reset directory state on new connection + $self->{_user_set_directory} = 0; + + return $self->_connect(); +} + +=head3 _connect + + my $success = $transport->_connect(); + +Internal method that performs the protocol-specific connection operation. +Must be implemented by subclasses. Called by connect() after resetting +directory state. + +=cut + +sub _connect { + my ($self) = @_; + die "Subclass must implement _connect"; } =head3 upload_file - $transport->upload_file($file); + # Signature: + my $success = $transport->upload_file($local_file, $remote_file, \%options); + +Uploads a file to the remote server. Automatically establishes a connection if needed. + +B + +=over 4 + +=item * C<$local_file> - Path to local file to upload (required) + +=item * C<$remote_file> - Remote filename (not a path, just the filename) (required) + +=item * C<\%options> - Optional hashref with keys: + +=over 4 + +=item * C - Directory path to upload to. If provided, uses this directory +for this operation only (simplified API). If omitted, behavior depends on whether +change_directory() has been called explicitly (see DESCRIPTION). + +=back + +=back + +B + + # Pattern 1: Simplified API with custom path + $transport->upload_file('/tmp/data.csv', 'data.csv', { path => '/uploads/' }); + + # Pattern 2: Simplified API with configured upload_directory + $transport->upload_file('/tmp/data.csv', 'data.csv'); -Method for uploading a file to the current file server + # Pattern 3: Traditional API with explicit directory + $transport->change_directory('/uploads/'); + $transport->upload_file('/tmp/data.csv', 'data.csv'); + +B True on success, undef on failure. Check object_messages() for details. =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 download_file +=head3 _upload_file - $transport->download_file($file); + $transport->_upload_file($local_file, $remote_file); -Method for downloading a file from the current file server +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 download_file { - my ( $self, $remote_file, $local_file ) = @_; - die "Subclass must implement download_file"; +sub _upload_file { + my ($self) = @_; + die "Subclass must implement _upload_file"; } -=head3 change_directory +=head3 download_file + + # Signature: + my $success = $transport->download_file($remote_file, $local_file, \%options); - my $files = $transport->change_directory($path); +Downloads a file from the remote server. Automatically establishes a connection if needed. -Method for changing the current directory on the connected file server +B + +=over 4 + +=item * C<$remote_file> - Remote filename (not a path, just the filename) (required) + +=item * C<$local_file> - Path where the downloaded file should be saved (required) + +=item * C<\%options> - Optional hashref with keys: + +=over 4 + +=item * C - Directory path to download from. If provided, uses this directory +for this operation only (simplified API). If omitted, behavior depends on whether +change_directory() has been called explicitly (see DESCRIPTION). + +=back + +=back + +B + + # Pattern 1: Simplified API with custom path + $transport->download_file('data.csv', '/tmp/data.csv', { path => '/downloads/' }); + + # Pattern 2: Simplified API with configured download_directory + $transport->download_file('data.csv', '/tmp/data.csv'); + + # Pattern 3: Traditional API with explicit directory + $transport->change_directory('/downloads/'); + $transport->download_file('data.csv', '/tmp/data.csv'); + +B True on success, undef on failure. Check object_messages() for details. =cut -sub change_directory { - my ( $self, $path ) = @_; - die "Subclass must implement change_directory"; +sub 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 list_files +=head3 _download_file - my $files = $transport->list_files($path); + $transport->_download_file($remote_file, $local_file); -Method for listing files in the current directory of 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 list_files { - my ( $self, $path ) = @_; - die "Subclass must implement list_files"; +sub _download_file { + my ($self) = @_; + die "Subclass must implement _download_file"; } =head3 rename_file @@ -261,20 +573,196 @@ Method for renaming a file on the current file server sub rename_file { my ( $self, $old_name, $new_name ) = @_; - die "Subclass must implement rename_file"; + + 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 + + # Signature: + my $files = $transport->list_files(\%options); + +Lists files in a directory on the remote server. Automatically establishes a connection if needed. + +B + +=over 4 + +=item * C<\%options> - Optional hashref with keys: + +=over 4 + +=item * C - Directory path to list files from. If provided, uses this directory +for this operation only (simplified API). If omitted, behavior depends on whether +change_directory() has been called explicitly (see DESCRIPTION). + +=back + +=back + +B + + # Pattern 1: Simplified API with custom path + my $files = $transport->list_files({ path => '/incoming/' }); + + # Pattern 2: Simplified API with configured download_directory + my $files = $transport->list_files(); + + # Pattern 3: Traditional API with explicit directory + $transport->change_directory('/incoming/'); + my $files = $transport->list_files(); + +B Arrayref of hashrefs on success, undef on failure. Each hashref contains +file metadata (filename, size, permissions, etc.). The exact structure varies by +transport type but always includes a 'filename' key. + +=cut + +sub 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 _list_files + + my $files = $transport->_list_files(); + +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 _list_files { + my ($self) = @_; + die "Subclass must implement _list_files"; } =head3 disconnect $transport->disconnect(); -Method for disconnecting from the current file server +Closes the connection to the remote server. + +B Calling this method is entirely optional. Connections are automatically +cleaned up when the transport object is destroyed (goes out of scope). You only +need to call this explicitly if you want to free resources before the object +is destroyed, such as in long-running processes. + +B True on success, undef on failure. =cut sub disconnect { my ($self) = @_; - die "Subclass must implement disconnect"; + + # Reset directory state when disconnecting + $self->{_user_set_directory} = 0; + + return $self->_disconnect(); +} + +=head3 _disconnect + + $transport->_disconnect(); + +Internal method that performs the protocol-specific disconnection operation. +Must be implemented by subclasses. Called by disconnect() after resetting +directory state. + +=cut + +sub _disconnect { + my ($self) = @_; + die "Subclass must implement _disconnect"; +} + +=head3 change_directory + + my $success = $transport->change_directory($path); + +Changes the current working directory on the remote server. + +B Calling this method explicitly switches the transport to "manual mode" +and disables automatic directory management. After calling this method, all subsequent +file operations will use this directory (or relative paths from it) until you call +change_directory() again or create a new connection. + +B + +=over 4 + +=item * C<$path> - Directory path to change to (required) + +=back + +B + + # After calling change_directory explicitly: + $transport->change_directory('/work/dir/'); + $transport->upload_file($local, $remote); # Uses /work/dir/, not upload_directory + $transport->list_files(); # Lists /work/dir/, not download_directory + +B True on success, undef on failure. Check object_messages() for details. + +=cut + +sub change_directory { + my ( $self, $path ) = @_; + + # Mark that user has explicitly set a directory + # This prevents auto-directory management from interfering + $self->{_user_set_directory} = 1; + + return $self->_change_directory($path); +} + +=head3 _change_directory + + my $success = $transport->_change_directory($path); + +Internal method that performs the protocol-specific directory change operation. +Must be implemented by subclasses. Called by change_directory() after setting +the _user_set_directory flag. + +=cut + +sub _change_directory { + my ($self) = @_; + die "Subclass must implement _change_directory"; } =head3 _post_store_trigger diff --git a/Koha/File/Transport/FTP.pm b/Koha/File/Transport/FTP.pm index f965ebd0ede..47bc264054f 100644 --- a/Koha/File/Transport/FTP.pm +++ b/Koha/File/Transport/FTP.pm @@ -27,15 +27,15 @@ Koha::File::Transport::FTP - FTP implementation of file transport =head2 Class methods -=head3 connect +=head3 _connect - my $success = $self->connect; + my $success = $self->_connect; Start the FTP transport connect, returns true on success or undefined on failure. =cut -sub connect { +sub _connect { my ($self) = @_; my $operation = "connection"; @@ -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"; @@ -126,9 +122,9 @@ sub download_file { return 1; } -=head3 change_directory +=head3 _change_directory - my $success = $server->change_directory($directory); + my $success = $server->_change_directory($directory); Passed a directory name, this will change the current directory of the server connection. @@ -136,7 +132,7 @@ Returns true on success or undefined on failure. =cut -sub change_directory { +sub _change_directory { my ( $self, $remote_directory ) = @_; my $operation = "change_directory"; @@ -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"; @@ -234,15 +227,15 @@ sub rename_file { return 1; } -=head3 disconnect +=head3 _disconnect - $server->disconnect(); + $server->_disconnect(); Disconnects from the FTP server. =cut -sub disconnect { +sub _disconnect { my ($self) = @_; if ( $self->{connection} ) { @@ -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 f2b208b61f0..568f30cf5b4 100644 --- a/Koha/File/Transport/Local.pm +++ b/Koha/File/Transport/Local.pm @@ -28,15 +28,15 @@ Koha::File::Transport::Local - Local file system implementation of file transpor =head2 Class methods -=head3 connect +=head3 _connect - my $success = $self->connect; + my $success = $self->_connect; Validates that the configured directories exist and have appropriate permissions. =cut -sub connect { +sub _connect { my ($self) = @_; my $operation = "connection"; @@ -117,21 +117,19 @@ 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"; - my $upload_dir = $self->upload_directory || $self->{current_directory} || '.'; + my $upload_dir = $self->{current_directory} || $self->upload_directory || '.'; my $destination = File::Spec->catfile( $upload_dir, $remote_file ); unless ( copy( $local_file, $destination ) ) { @@ -159,21 +157,19 @@ 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'; - my $download_dir = $self->download_directory || $self->{current_directory} || '.'; + my $download_dir = $self->{current_directory} || $self->download_directory || '.'; my $source = File::Spec->catfile( $download_dir, $remote_file ); unless ( -f $source ) { @@ -215,9 +211,9 @@ sub download_file { return 1; } -=head3 change_directory +=head3 _change_directory - my $success = $server->change_directory($directory); + my $success = $server->_change_directory($directory); Sets the current working directory for file operations. @@ -225,7 +221,7 @@ Returns true on success or undefined on failure. =cut -sub change_directory { +sub _change_directory { my ( $self, $remote_directory ) = @_; my $operation = 'change_directory'; @@ -257,20 +253,19 @@ 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"; - my $directory = $self->download_directory || $self->{current_directory} || '.'; + my $directory = $self->{current_directory} || $self->download_directory || '.'; unless ( -d $directory ) { $self->add_message( @@ -340,21 +335,19 @@ 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"; - my $directory = $self->download_directory || $self->{current_directory} || '.'; + my $directory = $self->{current_directory} || $self->download_directory || '.'; my $old_path = File::Spec->catfile( $directory, $old_name ); my $new_path = File::Spec->catfile( $directory, $new_name ); @@ -397,15 +390,28 @@ sub rename_file { return 1; } -=head3 disconnect +=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(); + $server->_disconnect(); For local transport, this is a no-op as there are no connections to close. =cut -sub disconnect { +sub _disconnect { my ($self) = @_; # No-op for local transport diff --git a/Koha/File/Transport/SFTP.pm b/Koha/File/Transport/SFTP.pm index d0abdd69d3a..0df9c30b756 100644 --- a/Koha/File/Transport/SFTP.pm +++ b/Koha/File/Transport/SFTP.pm @@ -32,15 +32,15 @@ Koha::File::Transport::SFTP - SFTP implementation of file transport =head2 Class methods -=head3 connect +=head3 _connect - my $success = $self->connect; + my $success = $self->_connect; Start the SFTP transport connect, returns true on success or undefined on failure. =cut -sub connect { +sub _connect { my ($self) = @_; my $operation = "connection"; @@ -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'; @@ -138,9 +134,9 @@ sub download_file { return 1; } -=head3 change_directory +=head3 _change_directory - my $success = $server->change_directory($directory); + my $success = $server->_change_directory($directory); Passed a directory name, this will change the current directory of the server connection. @@ -148,7 +144,7 @@ Returns true on success or undefined on failure. =cut -sub change_directory { +sub _change_directory { my ( $self, $remote_directory ) = @_; my $operation = 'change_directory'; @@ -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,15 +225,27 @@ sub rename_file { return 1; } -=head3 disconnect +=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(); + $server->_disconnect(); Disconnects from the SFTP server. =cut -sub disconnect { +sub _disconnect { my ($self) = @_; if ( $self->{connection} ) { diff --git a/t/db_dependent/Koha/File/Transports.t b/t/db_dependent/Koha/File/Transports.t index 928c48423f0..75af7a5055d 100755 --- a/t/db_dependent/Koha/File/Transports.t +++ b/t/db_dependent/Koha/File/Transports.t @@ -17,8 +17,10 @@ use Modern::Perl; -use Test::More tests => 4; +use Test::More tests => 5; use Test::NoWarnings; +use File::Temp qw(tempdir); +use File::Spec; use Koha::Database; use Koha::File::Transports; @@ -171,3 +173,89 @@ subtest 'find() tests' => sub { $schema->storage->txn_rollback; }; + +subtest 'Dual API functionality (_user_set_directory flag)' => sub { + plan tests => 8; + + $schema->storage->txn_begin; + + # Create temporary directories for testing + my $tempdir = tempdir( CLEANUP => 1 ); + my $download_dir = File::Spec->catdir( $tempdir, 'download' ); + my $upload_dir = File::Spec->catdir( $tempdir, 'upload' ); + my $custom_dir = File::Spec->catdir( $tempdir, 'custom' ); + + mkdir $download_dir or die "Cannot create download_dir: $!"; + mkdir $upload_dir or die "Cannot create upload_dir: $!"; + mkdir $custom_dir or die "Cannot create custom_dir: $!"; + + # Create test files + my $test_file = File::Spec->catfile( $download_dir, 'test.txt' ); + open my $fh, '>', $test_file or die "Cannot create test file: $!"; + print $fh "Test content\n"; + close $fh; + + # Create local transport with default directories + my $local_transport = $builder->build_object( + { + class => 'Koha::File::Transports', + value => { + transport => 'local', + name => 'Dual API Test', + host => 'localhost', + download_directory => $download_dir, + upload_directory => $upload_dir, + } + } + ); + + # TEST 1: Traditional API with explicit change_directory should set flag + $local_transport->connect(); + ok( + !$local_transport->{_user_set_directory}, + 'Flag should be false after connect' + ); + + $local_transport->change_directory($custom_dir); + ok( + $local_transport->{_user_set_directory}, + 'Flag should be true after explicit change_directory' + ); + + # TEST 2: Verify flag prevents auto-directory management + # After setting directory explicitly, list_files should NOT auto-change to download_directory + my $custom_test_file = File::Spec->catfile( $custom_dir, 'custom.txt' ); + open my $fh2, '>', $custom_test_file or die "Cannot create custom test file: $!"; + print $fh2 "Custom content\n"; + close $fh2; + + my $files = $local_transport->list_files(); + ok( $files, 'list_files() works after explicit change_directory' ); + is( scalar(@$files), 1, 'Should find 1 file in custom directory' ); + is( $files->[0]{filename}, 'custom.txt', 'Should find custom.txt, not test.txt' ); + + # TEST 3: Simplified API with options should work + $local_transport = $builder->build_object( + { + class => 'Koha::File::Transports', + value => { + transport => 'local', + name => 'Simplified API Test', + host => 'localhost', + download_directory => $download_dir, + upload_directory => $upload_dir, + } + } + ); + + # Simplified API - no explicit connect or change_directory + my $files_simplified = $local_transport->list_files( { path => $custom_dir } ); + ok( $files_simplified, 'Simplified API list_files() with custom path works' ); + is( scalar(@$files_simplified), 1, 'Should find 1 file via simplified API' ); + is( + $files_simplified->[0]{filename}, 'custom.txt', + 'Simplified API should find custom.txt' + ); + + $schema->storage->txn_rollback; +}; -- 2.51.0