gitea
/
Koha-clms
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Bug 40811: Implement dual API for file transports with simplified auto-management 

This patch implements a dual API design for Koha::File::Transport, providing
both a simplified auto-managing API and a traditional explicit API for maximum
flexibility and ease of use.

DUAL API DESIGN
===============

Simplified API (Recommended):
  - Automatic connection management (no connect/disconnect needed)
  - Flexible per-operation directory control via options hashref
  - Stateless operations safe for concurrent usage

  Example:
    $transport->upload_file($local, $remote, { path => '/custom/' });
    $transport->download_file($remote, $local);  # Uses download_directory
    $transport->list_files({ path => '/incoming/' });

Traditional API (Explicit Control):
  - Manual connection/directory management when needed
  - Stateful directory operations via change_directory()
  - Ideal for multiple operations in the same directory

  Example:
    $transport->change_directory('/work/');
    $transport->upload_file($local, 'file1.txt');
    $transport->upload_file($local, 'file2.txt');
    my $files = $transport->list_files();

The APIs can be mixed - calling change_directory() explicitly switches to
manual mode and disables auto-management for subsequent operations.

STANDARDIZED TEMPLATE METHOD PATTERN
=====================================

All transport methods now follow a consistent pattern with clear separation
between public API (parent class) and protocol implementation (subclasses).

Public methods in Koha::File::Transport:
  - connect()          → Resets directory state, calls _connect()
  - disconnect()       → Resets directory state, calls _disconnect()
  - change_directory() → Sets manual mode flag, calls _change_directory()
  - upload_file()      → Manages connection/directory, calls _upload_file()
  - download_file()    → Manages connection/directory, calls _download_file()
  - list_files()       → Manages connection/directory, calls _list_files()
  - rename_file()      → Ensures connection, calls _rename_file()

Subclass responsibilities (SFTP/FTP/Local):
  - Implement _connect() - Protocol-specific connection logic
  - Implement _disconnect() - Protocol-specific disconnection logic
  - Implement _change_directory() - Protocol-specific directory change
  - Implement _upload_file() - Protocol-specific upload logic
  - Implement _download_file() - Protocol-specific download logic
  - Implement _list_files() - Protocol-specific listing logic
  - Implement _rename_file() - Protocol-specific rename logic
  - Implement _is_connected() - Protocol-specific connection check

Test plan:
  prove t/db_dependent/Koha/File/Transports.t

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This commit is contained in:
Martin Renvoize 2025-09-24 09:37:29 +01:00 committed by Lucas Gass
parent ac9944f7b0
commit b75d07acd5
No known key found for this signature in database
GPG Key ID: 60B447398B0D36CA
5 changed files with 718 additions and 126 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

556
Koha/File/Transport.pm
View File

@ -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();
Method for connecting the current transport to the file server
Establishes a connection to the remote 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);
Method for uploading a file to the current file server
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');
# 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 _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
$transport->download_file($file);
# Signature:
my $success = $transport->download_file($remote_file, $local_file, \%options);
Method for downloading a file from the current file server
Downloads a file from the remote server. Automatically establishes a connection if needed.
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 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";
}
=head3 list_files
my $files = $transport->list_files($path);
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";
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

63
Koha/File/Transport/FTP.pm
View File

@ -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
=head3 _download_file
my $success = $transport->download_file($filename);
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
=head3 _list_files
my $files = $server->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 ) = @_;

72
Koha/File/Transport/Local.pm
View File

@ -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
=head3 _download_file
my $success = $transport->download_file($remote_file, $local_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
=head3 _list_files
my $files = $server->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
$server->disconnect();
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();
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

63
Koha/File/Transport/SFTP.pm
View File

@ -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
=head3 _download_file
my $success = $transport->download_file($filename);
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
=head3 _list_files
my $files = $server->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
$server->disconnect();
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();
Disconnects from the SFTP server.
=cut
sub disconnect {
sub _disconnect {
my ($self) = @_;
if ( $self->{connection} ) {

90
t/db_dependent/Koha/File/Transports.t
View File

@ -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;
};