MT#65904 report rejected CSV rows on calllistsuppressions API

A CSV upload to /api/calllistsuppressions/ answered with a success
status even when rows were rejected, so a user had no way to
notice that the import was incomplete.

The rejected rows are reported with a 422 now, whose message tells
how many rows were imported and which line numbers were skipped,
the same information the web interface shows.

Change-Id: I7ab235227a3f69c8739e8ff2b199debf6fa8a304

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;

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,

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

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: