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

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 <pedro.amorim@openfifth.co.uk>
This commit is contained in:
Martin Renvoize 2026-07-16 17:17:53 +01:00 committed by Pedro Amorim
parent f8cf2aea42
commit 4deb199163
No known key found for this signature in database
GPG Key ID: 06B75BA7F3894D55
9 changed files with 358 additions and 130 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

59
Koha/File/Transport.pm
View File

@ -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<status> 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<status> themselves, so all three stay in sync.
The persisted C<operations> 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);

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

@ -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<return $self-E<gt>_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<return $self-E<gt>_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<path> key, matching
SFTP and Local, so templates can read C<operation.detail.path> 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,
}
);

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

@ -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<return $self-E<gt>_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.

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

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

2
koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/file_transport_test.inc
View File

@ -14,7 +14,7 @@
[%- CASE 'list' -%]
<span>List files</span> [% status | html %]
[%- CASE 'change_directory' -%]
<span>Change directory ([% operation.detail.directory | html %][% operation.detail.operation_params | html %])</span> [% status | html %]
<span>Change directory ([% operation.detail.path | html %])</span> [% status | html %]
[%- CASE -%]
<span>[% operation.code | html %]</span> [% status | html %]
[%- END -%]

53
t/db_dependent/Koha/File/Transport.t
View File

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

53
t/db_dependent/Koha/File/Transport/FTP.t
View File

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

41
t/db_dependent/Koha/File/Transport/Local.t
View File

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

60
t/db_dependent/Koha/File/Transport/SFTP.t
View File

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