diff --git a/lib/NGCP/Panel/Controller/API/CallListSuppressions.pm b/lib/NGCP/Panel/Controller/API/CallListSuppressions.pm index 79d0b21c4c..09892652e6 100644 --- a/lib/NGCP/Panel/Controller/API/CallListSuppressions.pm +++ b/lib/NGCP/Panel/Controller/API/CallListSuppressions.pm @@ -5,6 +5,8 @@ use Sipwise::Base; use parent qw/NGCP::Panel::Role::Entities NGCP::Panel::Role::API::CallListSuppressions/; +use HTTP::Status qw(:constants); + use NGCP::Panel::Utils::CallList; use NGCP::Panel::Utils::MySQL; @@ -27,7 +29,8 @@ sub api_description { 'In "filter" mode matching calls do not appear at all, in "obfuscate" mode the number is replaced by the given "label", and in "disabled" mode the suppression is not applied. '. 'Admin and reseller users always see the unsuppressed call lists. The combination of "domain", "direction" and "pattern" must be unique. '. 'You can POST suppressions individually one-by-one using json. For bulk uploads specify the Content-Type as "text/csv" and POST the CSV in the request body to the collection with an optional parameter "purge_existing=true". '. - 'The CSV columns are "domain,direction,pattern,mode,label" without a header row. To download all the suppressions in CSV format, GET the collection with an "Accept: text/csv" header.'; + 'The CSV columns are "domain,direction,pattern,mode,label" without a header row. If any row of the CSV is rejected, the upload returns a 422 reporting how many rows were imported and the line numbers which were skipped. '. + 'To download all the suppressions in CSV format, GET the collection with an "Accept: text/csv" header.'; } sub order_by_cols { @@ -104,13 +107,19 @@ sub process_data :Private { ); } - #upload_suppressions_csv returns two values only, unlike the upload_csv of - #the other resources, which also return the list of the accepted records - my ($fails, $text_success) = NGCP::Panel::Utils::CallList::upload_suppressions_csv( + my ($imported, $fails, $text_success) = NGCP::Panel::Utils::CallList::upload_suppressions_csv( c => $c, data => $data_ref, schema => $schema, ); + + if (@{ $fails }) { + $self->error($c, HTTP_UNPROCESSABLE_ENTITY, + sprintf('Call list suppressions upload incomplete, imported %d row(s), skipped the following line numbers: %s', + $imported, join(", ", @{ $fails }))); + return; + } + $c->log->info($$text_success); return; diff --git a/lib/NGCP/Panel/Controller/CallListSuppression.pm b/lib/NGCP/Panel/Controller/CallListSuppression.pm index 429f4e5639..36fe2e3350 100644 --- a/lib/NGCP/Panel/Controller/CallListSuppression.pm +++ b/lib/NGCP/Panel/Controller/CallListSuppression.pm @@ -248,7 +248,7 @@ sub upload :Chained('list') :PathPart('upload') :Args(0) { $end = time; $c->log->debug("Purging call list suppressions took " . ($end - $start) . "s"); } - ( $fails, $text_success ) = NGCP::Panel::Utils::CallList::upload_suppressions_csv( + ( undef, $fails, $text_success ) = NGCP::Panel::Utils::CallList::upload_suppressions_csv( c => $c, data => \$data, schema => $schema, diff --git a/lib/NGCP/Panel/Utils/CallList.pm b/lib/NGCP/Panel/Utils/CallList.pm index c535c712fd..2283caf941 100644 --- a/lib/NGCP/Panel/Utils/CallList.pm +++ b/lib/NGCP/Panel/Utils/CallList.pm @@ -573,22 +573,33 @@ sub upload_suppressions_csv { my @fields ; my @fails = (); my $linenum = 0; + my $imported = 0; my @suppressions = (); open(my $fh, '<:encoding(utf8)', $data); $start = time; my $chunk_size = 2000; - while ( my $line = $csv->getline($fh)) { + #to don't stop on first failed parse - don't use "while($csv->getline)" + while ( my $line = <$fh> ){ ++$linenum; - unless (scalar @{ $line } == scalar @cols) { + #not chomp, to don't leave a \r in the last field of a CRLF file + $line =~ s/\r?\n\z//; + next unless length $line; + unless($csv->parse($line)) { + push @fails, $linenum; + next; + } + @fields = $csv->fields(); + unless (scalar @fields == scalar @cols) { push @fails, $linenum; next; } my $row = {}; - @{$row}{@cols} = @{ $line }; + @{$row}{@cols} = @fields; push @suppressions, [ $row->{domain}, $row->{direction}, $row->{pattern}, $row->{mode}, $row->{label} ]; + ++$imported; - if($linenum % $chunk_size == 0) { + if(scalar @suppressions == $chunk_size) { _insert_suppressions_csv_batch($c, $schema, \@suppressions, $chunk_size); @suppressions = (); } @@ -605,7 +616,7 @@ sub upload_suppressions_csv { $text .= $c->loc(", but skipped the following line numbers: ") . (join ", ", @fails); } - return ( \@fails, \$text ); + return ( $imported, \@fails, \$text ); } diff --git a/t/api-rest/api-calllistsuppressions.t b/t/api-rest/api-calllistsuppressions.t index a7cbc783bd..73f451b36c 100644 --- a/t/api-rest/api-calllistsuppressions.t +++ b/t/api-rest/api-calllistsuppressions.t @@ -116,18 +116,52 @@ EOS_CSV is($res->filename, 'call_list_suppressions.csv', "check downloaded csv filename"); is($res->content, $csv_data, "check downloaded csv content"); - #clear off the uploaded rows, they are out of the Collection control. a one - #field line is skipped by the upload, so this only purges. an empty body - #can't be used for it, as it is rejected with 400 - ($res, $content) = $test_machine->request_post( - 'purgeonly', '/api/calllistsuppressions/?purge_existing=true'); - $test_machine->http_code_msg(201, "check csv purge", $res, $content); + #the uploaded rows are out of the Collection control, the first upload of the + #next block purges them #restore by assigning the value: content_type returns the hash reference #itself, so saving and setting it back again would be a no-op $test_machine->content_type->{POST} = 'application/json'; } +# a malformed csv has to be reported and not silently accepted +{ + $test_machine->content_type->{POST} = 'text/csv'; + + #a row with a wrong number of columns + my ($res, $err) = $test_machine->request_post( + "csvtest3.example.org,outgoing,^433\n", + '/api/calllistsuppressions/?purge_existing=true'); + $test_machine->http_code_msg(422, "check csv upload with a wrong column count", $res, $err); + ok($err->{message} =~ /skipped the following line numbers: 1/, + "check the skipped line number is reported"); + ok($err->{message} =~ /imported 0 row/, "check the imported amount is reported"); + + #a row csv can't parse at all. it used to end the upload silently, dropping + #the rest of the file and still reporting success + my $csv_broken = <<'EOS_CSV'; +csvtest4.example.org,outgoing,^434,obfuscate,csv4 +csvtest5.example.org,outgoing,"^435,obfuscate,csv5 +csvtest6.example.org,outgoing,^436,obfuscate,csv6 +EOS_CSV + ($res, $err) = $test_machine->request_post( + $csv_broken, '/api/calllistsuppressions/?purge_existing=true'); + $test_machine->http_code_msg(422, "check csv upload with an unparseable row", $res, $err); + ok($err->{message} =~ /skipped the following line numbers: 2/, + "check the unparseable line number is reported"); + #the line after the broken one has to be imported anyway + ok($err->{message} =~ /imported 2 row/, "check the lines after the broken one are imported"); + + #leave no rows behind, they are out of the Collection control. the one field + #line is rejected on purpose, this request is only done for the purge. an + #empty body can't be used for it, as it is rejected with 400 + ($res, $err) = $test_machine->request_post( + 'purgeonly', '/api/calllistsuppressions/?purge_existing=true'); + $test_machine->http_code_msg(422, "check csv purge with a rejected row", $res, $err); + + $test_machine->content_type->{POST} = 'application/json'; +} + done_testing; # vim: set tabstop=4 expandtab: