Bugzilla – Attachment 188035 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 simplified Transport API with connection management
Bug-40811-Implement-simplified-Transport-API-with-.patch (text/plain), 17.97 KB, created by
Kyle M Hall (khall)
on 2025-10-16 19:31:20 UTC
(
hide
)
Description:
Bug 40811: Implement simplified Transport API with connection management
Filename:
MIME Type:
Creator:
Kyle M Hall (khall)
Created:
2025-10-16 19:31:20 UTC
Size:
17.97 KB
patch
obsolete
>From 6cf64a5deb2c8cc97be95b2ec49499d74bbc7157 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 simplified Transport API with connection > management > >This commit introduces a simplified API for the Transport classes that reduces >the traditional 5-step workflow to a 2-step workflow while maintaining backward >compatibility. > >Key changes: >- Centralized connection and directory management in base Transport class >- Added _ensure_connected() method for automatic connection handling >- Added _auto_change_directory() for intelligent directory management >- Refactored all transport methods to use private implementations (_upload_file, etc.) >- Added state tracking to respect explicit change_directory() calls >- Support for both traditional explicit API and simplified auto-managing API > >Traditional API (unchanged): > $transport->connect(); > $transport->change_directory($path); > $transport->upload_file($local, $remote); > $transport->disconnect(); > >Simplified API (new): > $transport->upload_file($local, $remote, { path => '/custom/path' }); > # OR using default upload directory: > $transport->upload_file($local, $remote); > >The implementation intelligently handles three scenarios: >1. Simplified API with custom path - uses provided path >2. Traditional API with explicit directory management - respects user's choice >3. Traditional API without explicit directories - uses configured defaults > >Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> >--- > Koha/File/Transport.pm | 239 ++++++++++++++++++++++++++++++++--- > Koha/File/Transport/FTP.pm | 45 ++++--- > Koha/File/Transport/Local.pm | 46 ++++--- > Koha/File/Transport/SFTP.pm | 45 ++++--- > 4 files changed, 294 insertions(+), 81 deletions(-) > >diff --git a/Koha/File/Transport.pm b/Koha/File/Transport.pm >index 188aaa1ec8e..8f5e7d61a75 100644 >--- a/Koha/File/Transport.pm >+++ b/Koha/File/Transport.pm >@@ -37,7 +37,20 @@ Koha::File::Transport - Base class for file transport handling > > =head1 DESCRIPTION > >-Base class providing common functionality for FTP/SFTP file transport. >+Base class providing common functionality for FTP/SFTP/Local file transport. >+ >+This class provides both a traditional explicit API and a simplified auto-managing API. >+ >+Traditional API requires manual connection management: >+ $transport->connect(); >+ $transport->change_directory($path); >+ $transport->upload_file($local, $remote); >+ $transport->disconnect(); >+ >+Simplified API handles connections automatically: >+ $transport->upload_file($local, $remote, { path => '/custom/path' }); >+ # OR using default upload directory: >+ $transport->upload_file($local, $remote); > > =cut > >@@ -92,6 +105,68 @@ sub store { > return $self; > } > >+=head3 _ensure_connected >+ >+ $transport->_ensure_connected(); >+ >+Internal method that ensures a connection exists, connecting if needed. >+Returns true if connected, false if connection failed. >+ >+=cut >+ >+sub _ensure_connected { >+ my ($self) = @_; >+ >+ # Check if already connected (transport-specific) >+ return 1 if $self->_is_connected(); >+ >+ # Reset directory state on new connection >+ $self->{_user_set_directory} = 0; >+ >+ # Attempt to connect >+ return $self->connect(); >+} >+ >+=head3 _is_connected >+ >+ my $connected = $transport->_is_connected(); >+ >+Internal method to check if transport is currently connected. >+Must be implemented by subclasses. >+ >+=cut >+ >+sub _is_connected { >+ my ($self) = @_; >+ die "Subclass must implement _is_connected"; >+} >+ >+=head3 _auto_change_directory >+ >+ $transport->_auto_change_directory($dir_type, $custom_path); >+ >+Internal method that automatically changes to the appropriate directory. >+$dir_type is 'upload' or 'download', $custom_path is optional override. >+ >+=cut >+ >+sub _auto_change_directory { >+ my ( $self, $dir_type, $custom_path ) = @_; >+ >+ my $target_dir; >+ if ($custom_path) { >+ $target_dir = $custom_path; >+ } elsif ( $dir_type eq 'upload' ) { >+ $target_dir = $self->upload_directory; >+ } elsif ( $dir_type eq 'download' ) { >+ $target_dir = $self->download_directory; >+ } >+ >+ return 1 unless $target_dir; # No directory to change to >+ >+ return $self->change_directory($target_dir); >+} >+ > =head3 plain_text_password > > my $password = $server->plain_text_password; >@@ -201,15 +276,53 @@ sub connect { > > =head3 upload_file > >- $transport->upload_file($file); >+ # Traditional explicit API: >+ $transport->upload_file($local_file, $remote_file); >+ >+ # Simplified auto-managing API: >+ $transport->upload_file($local_file, $remote_file, { path => '/custom/path' }); > >-Method for uploading a file to the current file server >+Method for uploading a file to the current file server. >+ >+When called with options hashref, automatically handles connection and directory management. >+When called without options, uses traditional explicit workflow. > > =cut > > sub upload_file { >- my ( $self, $local_file, $remote_file ) = @_; >- die "Subclass must implement upload_file"; >+ my ( $self, $local_file, $remote_file, $options ) = @_; >+ >+ return unless $self->_ensure_connected(); >+ >+ # Only auto-change directory if: >+ # 1. Options provided with custom path (simplified API), OR >+ # 2. No explicit directory set by user AND default upload_directory exists (traditional API) >+ if ( $options && $options->{path} ) { >+ >+ # Simplified API - use custom path >+ return unless $self->_auto_change_directory( 'upload', $options->{path} ); >+ } elsif ( !$self->{_user_set_directory} ) { >+ >+ # Traditional API - use default directory only if user hasn't set one >+ return unless $self->_auto_change_directory( 'upload', undef ); >+ } >+ >+ return $self->_upload_file( $local_file, $remote_file ); >+} >+ >+=head3 _upload_file >+ >+ $transport->_upload_file($local_file, $remote_file); >+ >+Internal method that performs the protocol-specific upload operation. >+Must be implemented by subclasses. Called by upload_file after connection >+and directory management. >+ >+=cut >+ >+sub _upload_file { >+ my ($self) = @_; >+ die "Subclass must implement _upload_file"; > } > > =head3 download_file >@@ -221,21 +334,70 @@ Method for downloading a file from the current file server > =cut > > sub download_file { >- my ( $self, $remote_file, $local_file ) = @_; >- die "Subclass must implement download_file"; >+ my ( $self, $remote_file, $local_file, $options ) = @_; >+ >+ return unless $self->_ensure_connected(); >+ >+ # Only auto-change directory if: >+ # 1. Options provided with custom path (simplified API), OR >+ # 2. No explicit directory set by user AND default download_directory exists (traditional API) >+ if ( $options && $options->{path} ) { >+ >+ # Simplified API - use custom path >+ return unless $self->_auto_change_directory( 'download', $options->{path} ); >+ } elsif ( !$self->{_user_set_directory} ) { >+ >+ # Traditional API - use default directory only if user hasn't set one >+ return unless $self->_auto_change_directory( 'download', undef ); >+ } >+ >+ return $self->_download_file( $remote_file, $local_file ); > } > >-=head3 change_directory >+=head3 _download_file > >- my $files = $transport->change_directory($path); >+ $transport->_download_file($remote_file, $local_file); > >-Method for changing the current directory on the connected file server >+Internal method that performs the protocol-specific download operation. >+Must be implemented by subclasses. Called by download_file after connection >+and directory management. > > =cut > >-sub change_directory { >- my ( $self, $path ) = @_; >- die "Subclass must implement change_directory"; >+sub _download_file { >+ my ($self) = @_; >+ die "Subclass must implement _download_file"; >+} >+ >+=head3 rename_file >+ >+ my $success = $transport->rename_file($old_name, $new_name); >+ >+Method for renaming a file on the current file server >+ >+=cut >+ >+sub rename_file { >+ my ( $self, $old_name, $new_name ) = @_; >+ >+ return unless $self->_ensure_connected(); >+ >+ return $self->_rename_file( $old_name, $new_name ); >+} >+ >+=head3 _rename_file >+ >+ $transport->_rename_file($old_name, $new_name); >+ >+Internal method that performs the protocol-specific file rename operation. >+Must be implemented by subclasses. Called by rename_file after connection >+verification. >+ >+=cut >+ >+sub _rename_file { >+ my ($self) = @_; >+ die "Subclass must implement _rename_file"; > } > > =head3 list_files >@@ -247,21 +409,39 @@ Method for listing files in the current directory of the connected file server > =cut > > sub list_files { >- my ( $self, $path ) = @_; >- die "Subclass must implement list_files"; >+ my ( $self, $options ) = @_; >+ >+ return unless $self->_ensure_connected(); >+ >+ # Only auto-change directory if: >+ # 1. Options provided with custom path (simplified API), OR >+ # 2. No explicit directory set by user AND default download_directory exists (traditional API) >+ if ( $options && $options->{path} ) { >+ >+ # Simplified API - use custom path >+ return unless $self->_auto_change_directory( 'download', $options->{path} ); >+ } elsif ( !$self->{_user_set_directory} ) { >+ >+ # Traditional API - use default directory only if user hasn't set one >+ return unless $self->_auto_change_directory( 'download', undef ); >+ } >+ >+ return $self->_list_files(); > } > >-=head3 rename_file >+=head3 _list_files > >- my $success = $transport->rename_file($old_name, $new_name); >+ my $files = $transport->_list_files(); > >-Method for renaming a file on the current file server >+Internal method that performs the protocol-specific file listing operation. >+Must be implemented by subclasses. Called by list_files after connection >+and directory management. > > =cut > >-sub rename_file { >- my ( $self, $old_name, $new_name ) = @_; >- die "Subclass must implement rename_file"; >+sub _list_files { >+ my ($self) = @_; >+ die "Subclass must implement _list_files"; > } > > =head3 disconnect >@@ -272,6 +452,23 @@ Method for disconnecting from the current file server > > =cut > >+=head3 change_directory >+ >+ my $files = $transport->change_directory($path); >+ >+Method for changing the current directory on the connected file server >+ >+=cut >+ >+sub change_directory { >+ my ( $self, $path ) = @_; >+ >+ # Mark that user has explicitly set a directory >+ $self->{_user_set_directory} = 1; >+ >+ die "Subclass must implement change_directory"; >+} >+ > sub disconnect { > my ($self) = @_; > die "Subclass must implement disconnect"; >diff --git a/Koha/File/Transport/FTP.pm b/Koha/File/Transport/FTP.pm >index f965ebd0ede..10484b30f50 100644 >--- a/Koha/File/Transport/FTP.pm >+++ b/Koha/File/Transport/FTP.pm >@@ -64,17 +64,15 @@ sub connect { > return 1; > } > >-=head3 upload_file >+=head3 _upload_file > >- my $success = $transport->upload_file($fh); >- >-Passed a filehandle, this will upload the file to the current directory of the server connection. >+Internal method that performs the FTP-specific upload operation. > > Returns true on success or undefined on failure. > > =cut > >-sub upload_file { >+sub _upload_file { > my ( $self, $local_file, $remote_file ) = @_; > my $operation = "upload"; > >@@ -95,17 +93,15 @@ sub upload_file { > return 1; > } > >-=head3 download_file >- >- my $success = $transport->download_file($filename); >+=head3 _download_file > >-Passed a filename, this will download the file from the current directory of the server connection. >+Internal method that performs the FTP-specific download operation. > > Returns true on success or undefined on failure. > > =cut > >-sub download_file { >+sub _download_file { > my ( $self, $remote_file, $local_file ) = @_; > my $operation = "download"; > >@@ -156,16 +152,15 @@ sub change_directory { > return 1; > } > >-=head3 list_files >- >- my $files = $server->list_files; >+=head3 _list_files > >-Returns an array reference of hashrefs with file information found in the current directory of the server connection. >+Internal method that performs the FTP-specific file listing operation. >+Returns an array reference of hashrefs with file information. > Each hashref contains: filename, longname, size, perms. > > =cut > >-sub list_files { >+sub _list_files { > my ($self) = @_; > my $operation = "list"; > >@@ -207,17 +202,15 @@ sub list_files { > return \@file_list; > } > >-=head3 rename_file >+=head3 _rename_file > >- my $success = $server->rename_file($old_name, $new_name); >- >-Renames a file on the server connection. >+Internal method that performs the FTP-specific file rename operation. > > Returns true on success or undefined on failure. > > =cut > >-sub rename_file { >+sub _rename_file { > my ( $self, $old_name, $new_name ) = @_; > my $operation = "rename"; > >@@ -253,6 +246,18 @@ sub disconnect { > return 1; > } > >+=head3 _is_connected >+ >+Internal method to check if transport is currently connected. >+ >+=cut >+ >+sub _is_connected { >+ my ($self) = @_; >+ >+ return $self->{connection} && $self->{connection}->pwd(); >+} >+ > sub _abort_operation { > my ( $self, $operation ) = @_; > >diff --git a/Koha/File/Transport/Local.pm b/Koha/File/Transport/Local.pm >index f2b208b61f0..e56f45318d0 100644 >--- a/Koha/File/Transport/Local.pm >+++ b/Koha/File/Transport/Local.pm >@@ -117,17 +117,15 @@ sub connect { > return 1; > } > >-=head3 upload_file >+=head3 _upload_file > >- my $success = $transport->upload_file($local_file, $remote_file); >- >-Copies a local file to the upload directory. >+Internal method that performs the local file system upload operation. > > Returns true on success or undefined on failure. > > =cut > >-sub upload_file { >+sub _upload_file { > my ( $self, $local_file, $remote_file ) = @_; > my $operation = "upload"; > >@@ -159,17 +157,15 @@ sub upload_file { > return 1; > } > >-=head3 download_file >- >- my $success = $transport->download_file($remote_file, $local_file); >+=head3 _download_file > >-Copies a file from the download directory to a local file. >+Internal method that performs the local file system download operation. > > Returns true on success or undefined on failure. > > =cut > >-sub download_file { >+sub _download_file { > my ( $self, $remote_file, $local_file ) = @_; > my $operation = 'download'; > >@@ -257,16 +253,15 @@ sub change_directory { > return 1; > } > >-=head3 list_files >- >- my $files = $server->list_files; >+=head3 _list_files > >-Returns an array reference of hashrefs with file information found in the current directory. >+Internal method that performs the local file system file listing operation. >+Returns an array reference of hashrefs with file information. > Each hashref contains: filename, longname, size, perms, mtime. > > =cut > >-sub list_files { >+sub _list_files { > my ($self) = @_; > my $operation = "list"; > >@@ -340,17 +335,15 @@ sub list_files { > return \@files; > } > >-=head3 rename_file >+=head3 _rename_file > >- my $success = $server->rename_file($old_name, $new_name); >- >-Renames a file in the current directory. >+Internal method that performs the local file system file rename operation. > > Returns true on success or undefined on failure. > > =cut > >-sub rename_file { >+sub _rename_file { > my ( $self, $old_name, $new_name ) = @_; > my $operation = "rename"; > >@@ -397,6 +390,19 @@ sub rename_file { > return 1; > } > >+=head3 _is_connected >+ >+Internal method to check if transport is currently connected. >+For local transport, always returns true as local filesystem is always accessible. >+ >+=cut >+ >+sub _is_connected { >+ my ($self) = @_; >+ >+ return 1; # Local filesystem is always "connected" >+} >+ > =head3 disconnect > > $server->disconnect(); >diff --git a/Koha/File/Transport/SFTP.pm b/Koha/File/Transport/SFTP.pm >index d0abdd69d3a..ad3cabe8ff7 100644 >--- a/Koha/File/Transport/SFTP.pm >+++ b/Koha/File/Transport/SFTP.pm >@@ -76,17 +76,15 @@ sub connect { > return 1; > } > >-=head3 upload_file >+=head3 _upload_file > >- my $success = $transport->upload_file($fh); >- >-Passed a filehandle, this will upload the file to the current directory of the server connection. >+Internal method that performs the SFTP-specific upload operation. > > Returns true on success or undefined on failure. > > =cut > >-sub upload_file { >+sub _upload_file { > my ( $self, $local_file, $remote_file ) = @_; > my $operation = "upload"; > >@@ -107,17 +105,15 @@ sub upload_file { > return 1; > } > >-=head3 download_file >- >- my $success = $transport->download_file($filename); >+=head3 _download_file > >-Passed a filename, this will download the file from the current directory of the server connection. >+Internal method that performs the SFTP-specific download operation. > > Returns true on success or undefined on failure. > > =cut > >-sub download_file { >+sub _download_file { > my ( $self, $remote_file, $local_file ) = @_; > my $operation = 'download'; > >@@ -169,16 +165,15 @@ sub change_directory { > return 1; > } > >-=head3 list_files >- >- my $files = $server->list_files; >+=head3 _list_files > >-Returns an array reference of hashrefs with file information found in the current directory of the server connection. >+Internal method that performs the SFTP-specific file listing operation. >+Returns an array reference of hashrefs with file information. > Each hashref contains: filename, longname, a (attributes). > > =cut > >-sub list_files { >+sub _list_files { > my ($self) = @_; > my $operation = "list"; > >@@ -199,17 +194,15 @@ sub list_files { > return $file_list; > } > >-=head3 rename_file >+=head3 _rename_file > >- my $success = $server->rename_file($old_name, $new_name); >- >-Renames a file on the server connection. >+Internal method that performs the SFTP-specific file rename operation. > > Returns true on success or undefined on failure. > > =cut > >-sub rename_file { >+sub _rename_file { > my ( $self, $old_name, $new_name ) = @_; > my $operation = "rename"; > >@@ -232,6 +225,18 @@ sub rename_file { > return 1; > } > >+=head3 _is_connected >+ >+Internal method to check if transport is currently connected. >+ >+=cut >+ >+sub _is_connected { >+ my ($self) = @_; >+ >+ return $self->{connection} && !$self->{connection}->error; >+} >+ > =head3 disconnect > > $server->disconnect(); >-- >2.39.5 (Apple Git-154)
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