Bugzilla – Attachment 190460 Details for
Bug 40811
Enhance Koha::File::Transport API
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 40811: Implement dual API for file transports with simplified auto-management
066f5e2.patch (text/plain), 38.40 KB, created by
Tomás Cohen Arazi (tcohen)
on 2025-12-12 13:01:44 UTC
(
hide
)
Description:
Bug 40811: Implement dual API for file transports with simplified auto-management
Filename:
MIME Type:
Creator:
Tomás Cohen Arazi (tcohen)
Created:
2025-12-12 13:01:44 UTC
Size:
38.40 KB
patch
obsolete
>From 066f5e2756d05932b2946ff8f2bc355d7a3aa35a Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >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 <kyle@bywatersolutions.com> >Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> >--- > 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 188aaa1ec8..30375cb24f 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<Automatic Connection Management> >+ >+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<Flexible Directory Management> >+ >+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<No State Maintained> >+ >+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<Explicit Connection Control> >+ >+While connect() and disconnect() are still available and can be called explicitly, >+they are entirely optional. The simplified API manages connections automatically. >+ >+=item * B<Stateful Directory Management> >+ >+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<Session-Based Operations> >+ >+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<Note:> 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<Returns:> 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<Parameters:> >+ >+=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<path> - 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<Usage Patterns:> >+ >+ # 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<Returns:> 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<Parameters:> >+ >+=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<path> - 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<Usage Patterns:> >+ >+ # 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<Returns:> 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<Parameters:> >+ >+=over 4 >+ >+=item * C<\%options> - Optional hashref with keys: >+ >+=over 4 >+ >+=item * C<path> - 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<Usage Patterns:> >+ >+ # 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<Returns:> 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<Note:> 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<Returns:> 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<Important:> 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<Parameters:> >+ >+=over 4 >+ >+=item * C<$path> - Directory path to change to (required) >+ >+=back >+ >+B<Example:> >+ >+ # 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<Returns:> 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 f965ebd0ed..47bc264054 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 f2b208b61f..568f30cf5b 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 d0abdd69d3..0df9c30b75 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 928c48423f..75af7a5055 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.50.1 (Apple Git-155) >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 40811
:
186858
|
188035
|
188152
|
188313
| 190460 |
190461