From 4deb199163ec03a7702365c6d366a7450ff4cbfc Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Thu, 16 Jul 2026 17:17:53 +0100 Subject: [PATCH] Bug 42656: (QA follow-up) Make error status persistence consistent, with a full trace, across FTP, SFTP and Local Two related inconsistencies remained across the three transport backends' error-handling paths: 1) Only Koha::File::Transport::SFTP's _abort_operation() persisted an "errors" status snapshot to the status column on every failed operation. FTP never did, and Local had no _abort_operation() at all - so an SFTP transport's displayed status could reflect a single transient failure from real usage (e.g. an EDI cron job) while the exact same failure on FTP/Local left the stored status untouched (stale, possibly still showing the last explicit test_connection() result). This is exactly the kind of unclear status reporting bug 42656 is about. 2) FTP's _abort_operation() carried its extra detail under an operation_params key, while SFTP (and Local's plain add_message calls) used path. The change_directory() success payload had the same split: FTP used directory, SFTP and Local used path. The admin template's change_directory row display depended on FTP's mismatched key names, which meant it silently failed to show the tested directory for SFTP transports. Adds a shared _record_error($operation, \%payload) to the Koha::File::Transport base class - add_message() plus status persistence - and routes all three backends' _abort_operation() (Local gains one) through it. Renames FTP's operation_params/directory keys to path throughout, matching SFTP and Local, and simplifies the template to read operation.detail.path unconditionally instead of ORing two differently-named keys. _record_error() persists the full trace of operations recorded on the transport object so far this session (every success leading up to the failure), not just the failing operation in isolation - the same "full trace" shape TestTransport::process() already builds for an explicit test_connection() run. A long-lived transport used across a multi-step operation (e.g. an EDI download loop touching several files before one fails) will therefore show every successful step leading up to the failure. It also logs the failure - including the built trace - via Koha::Logger at warn level, so it's visible in the Koha logs without needing to query the transport's status column. Test plan: 1. prove -r t/db_dependent/Koha/File/ t/db_dependent/Koha/BackgroundJob/TestTransport.t t/db_dependent/Koha/Edifact/Transport.t t/db_dependent/api/v1/file_transports.t => All tests successful (48 tests total), including: - new "_abort_operation persists status consistently" subtests in FTP.t, SFTP.t and Local.t that reload the transport from the DB after a simulated failure and confirm the status column was updated with the correct operation code and path detail; - a new "_record_error() tests" subtest in Transport.t proving the persisted trace includes a prior success message plus the failure (not just the failure alone), and that a warn-level log line mentioning the operation and error was emitted. 2. koha-qa.pl -v 2 --more-tests => [PASS], no critic/pod warnings. Signed-off-by: Pedro Amorim --- Koha/File/Transport.pm | 59 +++++++ Koha/File/Transport/FTP.pm | 34 ++-- Koha/File/Transport/Local.pm | 167 ++++++++---------- Koha/File/Transport/SFTP.pm | 19 +- .../background_jobs/file_transport_test.inc | 2 +- t/db_dependent/Koha/File/Transport.t | 53 +++++- t/db_dependent/Koha/File/Transport/FTP.t | 53 +++++- t/db_dependent/Koha/File/Transport/Local.t | 41 ++++- t/db_dependent/Koha/File/Transport/SFTP.t | 60 ++++++- 9 files changed, 358 insertions(+), 130 deletions(-) diff --git a/Koha/File/Transport.pm b/Koha/File/Transport.pm index 8146aa5f41..e2ad840832 100644 --- a/Koha/File/Transport.pm +++ b/Koha/File/Transport.pm @@ -28,6 +28,7 @@ use List::MoreUtils qw( any ); use Koha::Database; use Koha::Exceptions::Object; use Koha::Encryption; +use Koha::Logger; use base qw(Koha::Object); @@ -283,6 +284,64 @@ sub _current_directory { die "Subclass must implement _current_directory"; } +=head3 _record_error + + $transport->_record_error( $operation, \%payload ); + +Records an operation failure consistently across all transport backends: +adds an in-memory 'error' message (as add_message() already does), persists +a snapshot of the failure to the C column, and logs it, so that a +transport's displayed status reflects real usage (not only explicit +test_connection() runs). + +Before this existed, only Koha::File::Transport::SFTP persisted status on +every failed operation; FTP and Local never did, so an SFTP transport's +status could reflect a single transient failure from live usage (e.g. an +EDI cron job) while the same failure on FTP/Local left the stored status +untouched. Subclasses should call this from their C<_abort_operation> +instead of writing to C themselves, so all three stay in sync. + +The persisted C array carries every message recorded on this +transport object so far this session (in order), not just the failing one - +the same "full trace" shape TestTransport::process() builds for an explicit +test_connection() run. A single long-lived transport object (e.g. across an +EDI download loop) will therefore show every prior successful step leading +up to the failure, not just the failure in isolation. + +=cut + +sub _record_error { + my ( $self, $operation, $payload ) = @_; + + $self->add_message( + { + message => $operation || 'operation', + type => 'error', + payload => $payload, + } + ); + + my $operations = + [ map { { code => $_->message, status => $_->type, detail => $_->payload } } @{ $self->object_messages } ]; + + my $status = { + status => 'errors', + operations => $operations, + }; + $self->set( { status => encode_json($status) } )->store(); + + Koha::Logger->get( { category => __PACKAGE__ } )->warn( + sprintf( + "File transport %s (%s) operation '%s' failed: %s. Trace: %s", + $self->id, $self->transport, $operation || 'operation', + $payload->{error} // 'unknown error', + encode_json($operations) + ) + ); + + return; +} + =head3 _auto_change_directory $transport->_auto_change_directory($dir_type, $custom_path); diff --git a/Koha/File/Transport/FTP.pm b/Koha/File/Transport/FTP.pm index b045977954..7c1ebf1058 100644 --- a/Koha/File/Transport/FTP.pm +++ b/Koha/File/Transport/FTP.pm @@ -164,8 +164,8 @@ sub _change_directory { message => $operation, type => 'success', payload => { - directory => $remote_directory, - pwd => $self->_current_directory + path => $remote_directory, + pwd => $self->_current_directory } } ); @@ -387,26 +387,30 @@ sub _is_connected { =head3 _abort_operation - return $self->_abort_operation($operation); + return $self->_abort_operation($operation, $path); -Records an error message for the named operation, aborts any in-progress -transfer on the current connection and returns nothing, so callers can -C_abort_operation($operation)> on failure. +Records an error message for the named operation (persisting a status +snapshot via the shared _record_error(), see Koha::File::Transport), aborts +any in-progress transfer on the current connection and returns nothing, so +callers can C_abort_operation($operation)> on failure. + +C<$path> is optional and carries the directory/file the operation was +acting on, for display purposes (e.g. which directory a failed +change_directory() was trying to reach). Uses the C key, matching +SFTP and Local, so templates can read C the same way +regardless of transport type. =cut sub _abort_operation { - my ( $self, $operation, $operation_params ) = @_; + my ( $self, $operation, $path ) = @_; - $self->add_message( + $self->_record_error( + $operation, { - message => $operation || 'operation', - type => 'error', - payload => { - detail => $self->{connection} ? $self->{connection}->status : '', - error => $self->{connection} ? $self->{connection}->message : $@, - operation_params => $operation_params - } + detail => $self->{connection} ? $self->{connection}->status : '', + error => $self->{connection} ? $self->{connection}->message : $@, + path => $path, } ); diff --git a/Koha/File/Transport/Local.pm b/Koha/File/Transport/Local.pm index 985380c77f..14838b5eaf 100644 --- a/Koha/File/Transport/Local.pm +++ b/Koha/File/Transport/Local.pm @@ -44,62 +44,46 @@ sub _connect { # Check download directory if configured if ( my $download_dir = $self->download_directory ) { unless ( -d $download_dir ) { - $self->add_message( + return $self->_abort_operation( + $operation, { - message => $operation, - type => 'error', - payload => { - error => "Download directory does not exist: $download_dir", - path => $download_dir - } + error => "Download directory does not exist: $download_dir", + path => $download_dir } ); - return; } unless ( -r $download_dir ) { - $self->add_message( + return $self->_abort_operation( + $operation, { - message => $operation, - type => 'error', - payload => { - error => "Download directory is not readable: $download_dir", - path => $download_dir - } + error => "Download directory is not readable: $download_dir", + path => $download_dir } ); - return; } } # Check upload directory if configured if ( my $upload_dir = $self->upload_directory ) { unless ( -d $upload_dir ) { - $self->add_message( + return $self->_abort_operation( + $operation, { - message => $operation, - type => 'error', - payload => { - error => "Upload directory does not exist: $upload_dir", - path => $upload_dir - } + error => "Upload directory does not exist: $upload_dir", + path => $upload_dir } ); - return; } unless ( -w $upload_dir ) { - $self->add_message( + return $self->_abort_operation( + $operation, { - message => $operation, - type => 'error', - payload => { - error => "Upload directory is not writable: $upload_dir", - path => $upload_dir - } + error => "Upload directory is not writable: $upload_dir", + path => $upload_dir } ); - return; } } @@ -167,17 +151,13 @@ sub _upload_file { my $destination = File::Spec->catfile( $upload_dir, $remote_file ); unless ( copy( $local_file, $destination ) ) { - $self->add_message( + return $self->_abort_operation( + $operation, { - message => $operation, - type => 'error', - payload => { - error => $!, - path => $destination - } + error => $!, + path => $destination } ); - return; } $self->add_message( @@ -218,31 +198,23 @@ sub _download_file { my $source = File::Spec->catfile( $download_dir, $remote_file ); unless ( -f $source ) { - $self->add_message( + return $self->_abort_operation( + $operation, { - message => $operation, - type => 'error', - payload => { - error => "File not found: $source", - path => $source - } + error => "File not found: $source", + path => $source } ); - return; } unless ( copy( $source, $local_file ) ) { - $self->add_message( + return $self->_abort_operation( + $operation, { - message => $operation, - type => 'error', - payload => { - error => $!, - path => $source - } + error => $!, + path => $source } ); - return; } $self->add_message( @@ -272,17 +244,13 @@ sub _change_directory { # For local file transport, we just track the current directory if ( $remote_directory && !-d $remote_directory ) { - $self->add_message( + return $self->_abort_operation( + $operation, { - message => $operation, - type => 'error', - payload => { - error => "Directory not found: $remote_directory", - path => $remote_directory - } + error => "Directory not found: $remote_directory", + path => $remote_directory } ); - return; } $self->{current_directory} = $remote_directory; @@ -339,32 +307,24 @@ sub _list_files { } unless ( -d $directory ) { - $self->add_message( + return $self->_abort_operation( + $operation, { - message => $operation, - type => 'error', - payload => { - error => "Directory not found: $directory", - path => $directory - } + error => "Directory not found: $directory", + path => $directory } ); - return; } my $dir_handle = IO::Dir->new($directory); unless ($dir_handle) { - $self->add_message( + return $self->_abort_operation( + $operation, { - message => $operation, - type => 'error', - payload => { - error => "Cannot open directory: $!", - path => $directory - } + error => "Cannot open directory: $!", + path => $directory } ); - return; } my @files; @@ -441,31 +401,23 @@ sub _rename_file { my $new_path = File::Spec->catfile( $directory, $new_name ); unless ( -f $old_path ) { - $self->add_message( + return $self->_abort_operation( + $operation, { - message => $operation, - type => 'error', - payload => { - error => "File not found: $old_path", - path => $old_path - } + error => "File not found: $old_path", + path => $old_path } ); - return; } unless ( move( $old_path, $new_path ) ) { - $self->add_message( + return $self->_abort_operation( + $operation, { - message => $operation, - type => 'error', - payload => { - error => $!, - path => "$old_path -> $new_path" - } + error => $!, + path => "$old_path -> $new_path" } ); - return; } $self->add_message( @@ -479,6 +431,29 @@ sub _rename_file { return 1; } +=head3 _abort_operation + + return $self->_abort_operation( $operation, \%payload ); + +Records an error message for the named operation, persisting a status +snapshot via the shared _record_error() (see Koha::File::Transport), and +returns nothing, so callers can +C_abort_operation($operation, \%payload)> on failure. + +Unlike FTP/SFTP there is no live connection to abort here - this purely +records the failure, but does so the same way as the other two backends so +a Local transport's displayed status reflects real usage too. + +=cut + +sub _abort_operation { + my ( $self, $operation, $payload ) = @_; + + $self->_record_error( $operation, $payload ); + + return; +} + =head3 _is_connected Internal method to check if transport is currently connected. diff --git a/Koha/File/Transport/SFTP.pm b/Koha/File/Transport/SFTP.pm index 6d7ef73b10..8ca66ce360 100644 --- a/Koha/File/Transport/SFTP.pm +++ b/Koha/File/Transport/SFTP.pm @@ -23,7 +23,6 @@ use File::Spec; use IO::File; use Net::SFTP::Foreign; use Try::Tiny; -use JSON qw( decode_json encode_json ); use Fcntl qw( S_ISDIR S_ISREG ); use base qw(Koha::File::Transport); @@ -382,6 +381,10 @@ sub _locate_key_file { Helper method to abort the current operation and return. +Records the failure via the shared _record_error() (see +Koha::File::Transport), which persists a status snapshot the same way for +every transport type. + =cut sub _abort_operation { @@ -397,24 +400,12 @@ sub _abort_operation { error_raw => $stderr }; - $self->add_message( - { - message => $operation, - type => 'error', - payload => $payload - } - ); + $self->_record_error( $operation, $payload ); if ( $self->{connection} ) { $self->{connection}->abort; } - my $status = { - status => 'errors', - operations => [ { code => $operation, status => 'error', detail => $payload } ] - }; - $self->set( { status => encode_json($status) } )->store(); - return; } diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/file_transport_test.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/file_transport_test.inc index 5ca42c1c87..e0ecbbf45f 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/file_transport_test.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/file_transport_test.inc @@ -14,7 +14,7 @@ [%- CASE 'list' -%] List files [% status | html %] [%- CASE 'change_directory' -%] - Change directory ([% operation.detail.directory | html %][% operation.detail.operation_params | html %]) [% status | html %] + Change directory ([% operation.detail.path | html %]) [% status | html %] [%- CASE -%] [% operation.code | html %] [% status | html %] [%- END -%] diff --git a/t/db_dependent/Koha/File/Transport.t b/t/db_dependent/Koha/File/Transport.t index 4e03dc8f34..177dbc118b 100755 --- a/t/db_dependent/Koha/File/Transport.t +++ b/t/db_dependent/Koha/File/Transport.t @@ -17,10 +17,12 @@ use Modern::Perl; -use Test::More tests => 5; +use Test::More tests => 6; use Test::Exception; use Test::NoWarnings; use Test::Warn; +use Test::MockModule; +use JSON qw( decode_json ); use Koha::File::Transports; @@ -171,4 +173,53 @@ subtest 'plain_text_key() tests' => sub { $schema->storage->txn_rollback; }; +subtest '_record_error() tests' => sub { + plan tests => 6; + + $schema->storage->txn_begin; + + my @warnings; + my $logger_module = Test::MockModule->new('Koha::Logger'); + $logger_module->mock( + 'warn', + sub { + shift; + push @warnings, "@_"; + } + ); + + my $transport = $builder->build_object( + { + class => 'Koha::File::Transports', + value => { transport => 'local' } + } + ); + + # Simulate a prior successful step before the failure, as would happen + # during a real multi-step operation (e.g. an EDI download loop + # downloading several files before one fails). + $transport->add_message( { message => 'connection', type => 'success', payload => { status => 'connected' } } ); + + $transport->_record_error( 'download', { error => 'File not found', path => '/missing/file.txt' } ); + + my $reloaded = Koha::File::Transports->find( $transport->id ); + my $persisted_status = decode_json( $reloaded->status ); + + is( $persisted_status->{status}, 'errors', 'status is "errors"' ); + is( + scalar @{ $persisted_status->{operations} }, 2, + 'the full trace is persisted: the prior success plus the failure, not just the failure alone' + ); + is( $persisted_status->{operations}[0]{status}, 'success', 'the prior successful operation is recorded first' ); + is( $persisted_status->{operations}[1]{status}, 'error', 'the failing operation is recorded last' ); + is( $persisted_status->{operations}[1]{code}, 'download', 'the failing operation code is recorded correctly' ); + + ok( + ( grep { /download/ && /File not found/ } @warnings ), + '_record_error() logs the failure, including the operation and error, via Koha::Logger at warn level' + ); + + $schema->storage->txn_rollback; +}; + 1; diff --git a/t/db_dependent/Koha/File/Transport/FTP.t b/t/db_dependent/Koha/File/Transport/FTP.t index 29ff8f57ea..d5c04893c5 100755 --- a/t/db_dependent/Koha/File/Transport/FTP.t +++ b/t/db_dependent/Koha/File/Transport/FTP.t @@ -17,11 +17,12 @@ use Modern::Perl; -use Test::More tests => 9; +use Test::More tests => 10; use Test::Exception; use Test::NoWarnings; use Test::Warn; use Test::MockModule; +use JSON qw( decode_json ); use Koha::File::Transports; @@ -166,6 +167,56 @@ subtest 'current_directory() tests' => sub { $schema->storage->txn_rollback; }; +subtest '_abort_operation persists status consistently (bug 42656 QA follow-up)' => sub { + plan tests => 6; + + $schema->storage->txn_begin; + + my $transport = $builder->build_object( + { + class => 'Koha::File::Transports', + value => { transport => 'ftp', password => 'testpass' } + } + ); + + my $mock_ftp = Test::MockModule->new('Net::FTP'); + $mock_ftp->mock( 'cwd', sub { return 0; } ); # simulate a failed CWD + $mock_ftp->mock( 'abort', sub { return 1; } ); + $mock_ftp->mock( 'status', sub { return 0; } ); + $mock_ftp->mock( 'message', sub { return 'No such directory'; } ); + + $transport->{connection} = bless {}, 'Net::FTP'; + $transport->{_user_set_directory} = 1; + + my $result = $transport->change_directory('/missing'); + is( $result, undef, 'change_directory() returns undef on failure' ); + + my ($error) = grep { $_->type eq 'error' } @{ $transport->object_messages }; + ok( $error, 'an error message was recorded' ); + is( $error->payload->{path}, '/missing', 'error payload uses the "path" key, matching SFTP and Local' ); + + # Before this fix, only SFTP persisted a status snapshot on a failed + # operation; FTP never touched the status column outside test_connection(). + my $reloaded = Koha::File::Transports->find( $transport->id ); + my $persisted_status = decode_json( $reloaded->status ); + is( + $persisted_status->{status}, 'errors', + 'status column now persists an "errors" status after a failed operation, not only after test_connection()' + ); + is( + $persisted_status->{operations}[0]{code}, 'change_directory', + 'persisted status records which operation failed' + ); + is( + $persisted_status->{operations}[0]{detail}{path}, '/missing', + 'persisted status detail also uses the "path" key' + ); + + $transport->{connection} = undef; + + $schema->storage->txn_rollback; +}; + subtest 'list_files() MLSD tests' => sub { plan tests => 16; diff --git a/t/db_dependent/Koha/File/Transport/Local.t b/t/db_dependent/Koha/File/Transport/Local.t index 717c3e609d..c4a83f5d59 100755 --- a/t/db_dependent/Koha/File/Transport/Local.t +++ b/t/db_dependent/Koha/File/Transport/Local.t @@ -17,10 +17,11 @@ 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 JSON qw( decode_json ); use Koha::File::Transports; @@ -183,4 +184,42 @@ subtest 'an unconfigured directory fails cleanly instead of falling back to "." $schema->storage->txn_rollback; }; +subtest '_abort_operation persists status consistently (bug 42656 QA follow-up)' => sub { + plan tests => 5; + + $schema->storage->txn_begin; + + my $transport = $builder->build_object( + { + class => 'Koha::File::Transports', + value => { transport => 'local' } + } + ); + + # Before this fix, Local never persisted a status snapshot on error at + # all (it had no _abort_operation), unlike SFTP. + my $result = $transport->change_directory('/this/does/not/exist'); + is( $result, undef, 'change_directory() returns undef for a missing directory' ); + + my ($error) = grep { $_->type eq 'error' } @{ $transport->object_messages }; + ok( $error, 'an error message was recorded' ); + is( + $error->payload->{path}, '/this/does/not/exist', + 'error payload uses the "path" key, matching FTP and SFTP' + ); + + my $reloaded = Koha::File::Transports->find( $transport->id ); + my $persisted_status = decode_json( $reloaded->status ); + is( + $persisted_status->{status}, 'errors', + 'status column now persists an "errors" status after a failed operation, matching FTP and SFTP' + ); + is( + $persisted_status->{operations}[0]{code}, 'change_directory', + 'persisted status records which operation failed' + ); + + $schema->storage->txn_rollback; +}; + 1; diff --git a/t/db_dependent/Koha/File/Transport/SFTP.t b/t/db_dependent/Koha/File/Transport/SFTP.t index cac5f9d64b..763d7c946b 100755 --- a/t/db_dependent/Koha/File/Transport/SFTP.t +++ b/t/db_dependent/Koha/File/Transport/SFTP.t @@ -17,11 +17,12 @@ use Modern::Perl; -use Test::More tests => 10; +use Test::More tests => 11; use Test::Exception; use Test::NoWarnings; use Test::Warn; use Test::MockModule; +use JSON qw( decode_json ); use Net::SFTP::Foreign::Attributes; @@ -280,6 +281,63 @@ subtest 'current_directory() tests' => sub { $schema->storage->txn_rollback; }; +subtest '_abort_operation persists status consistently (bug 42656 QA follow-up)' => sub { + plan tests => 6; + + $schema->storage->txn_begin; + + my $transport = $builder->build_object( + { + class => 'Koha::File::Transports', + + # key_file must be explicitly undef - _abort_operation() now + # triggers store(), which (via _post_store_trigger) tries to + # decrypt key_file; TestBuilder would otherwise auto-fill it + # with a non-ciphertext value. + value => { transport => 'sftp', password => 'testpass', key_file => undef } + } + ); + + my $mock_sftp = Test::MockModule->new('Net::SFTP::Foreign'); + $mock_sftp->mock( 'setcwd', sub { return 0; } ); # simulate a failed setcwd + $mock_sftp->mock( 'abort', sub { return 1; } ); + $mock_sftp->mock( 'status', sub { return 0; } ); + $mock_sftp->mock( 'error', sub { return 'No such directory'; } ); + $mock_sftp->mock( 'cwd', sub { return '/incoming/'; } ); + + $transport->{connection} = bless {}, 'Net::SFTP::Foreign'; + $transport->{_user_set_directory} = 1; + $transport->{stderr_capture} = ''; + + my $result = $transport->change_directory('/missing'); + is( $result, undef, 'change_directory() returns undef on failure' ); + + my ($error) = grep { $_->type eq 'error' } @{ $transport->object_messages }; + ok( $error, 'an error message was recorded' ); + is( $error->payload->{path}, '/missing', 'error payload uses the "path" key, matching FTP and Local' ); + + # SFTP already persisted status on failure before this refactor; this + # confirms the shared _record_error() preserves that existing behaviour. + my $reloaded = Koha::File::Transports->find( $transport->id ); + my $persisted_status = decode_json( $reloaded->status ); + is( + $persisted_status->{status}, 'errors', + 'status column still persists an "errors" status after a failed operation' + ); + is( + $persisted_status->{operations}[0]{code}, 'change_directory', + 'persisted status records which operation failed' + ); + is( + $persisted_status->{operations}[0]{detail}{path}, '/missing', + 'persisted status detail uses the "path" key' + ); + + $transport->{connection} = undef; + + $schema->storage->txn_rollback; +}; + subtest 'list_files() tests' => sub { plan tests => 12;