MODBULKOPS-451 - Rework errors preview

descriptors/ModuleDescriptor-template.json

@@ -4,7 +4,7 @@
   "provides": [
     {
       "id": "bulk-operations",
-      "version": "1.5",
+      "version": "1.6",
       "handlers": [
         {
           "methods": [ "POST" ],

src/main/java/org/folio/bulkops/client/BulkEditClient.java

@@ -3,13 +3,10 @@
 import java.util.UUID;
 
 import org.folio.bulkops.configs.FeignClientConfiguration;
-import org.folio.bulkops.domain.dto.Errors;
 import org.springframework.cloud.openfeign.FeignClient;
 import org.springframework.http.MediaType;
-import org.springframework.web.bind.annotation.GetMapping;
 import org.springframework.web.bind.annotation.PathVariable;
 import org.springframework.web.bind.annotation.PostMapping;
-import org.springframework.web.bind.annotation.RequestParam;
 import org.springframework.web.bind.annotation.RequestPart;
 import org.springframework.web.multipart.MultipartFile;
 
@@ -20,7 +17,4 @@ public interface BulkEditClient {
 
   @PostMapping(value = "/{jobId}/start")
   void startJob(@PathVariable UUID jobId);
-
-  @GetMapping(value = "/{jobId}/errors")
-  Errors getErrorsPreview(@PathVariable UUID jobId, @RequestParam int limit);
 }

src/main/java/org/folio/bulkops/controller/BulkOperationController.java

@@ -92,8 +92,8 @@ public ResponseEntity<BulkOperationCollection> getBulkOperationCollection(String
   }
 
   @Override
-  public ResponseEntity<Errors> getErrorsPreviewByOperationId(UUID operationId, Integer limit) {
-    return new ResponseEntity<>(errorService.getErrorsPreviewByBulkOperationId(operationId, limit), HttpStatus.OK);
+  public ResponseEntity<Errors> getErrorsPreviewByOperationId(UUID operationId, Integer limit, Integer offset) {
+    return new ResponseEntity<>(errorService.getErrorsPreviewByBulkOperationId(operationId, limit, offset), HttpStatus.OK);
   }
 
   @Override

src/main/java/org/folio/bulkops/service/ErrorService.java

@@ -2,6 +2,7 @@
 
 import static java.util.Objects.isNull;
 import static java.util.Objects.nonNull;
+import static java.util.stream.Collectors.toList;
 import static org.apache.commons.lang3.StringUtils.EMPTY;
 import static org.apache.commons.lang3.StringUtils.LF;
 import static org.folio.bulkops.domain.dto.OperationStatusType.COMPLETED;
@@ -12,7 +13,9 @@
 import static org.folio.bulkops.util.Constants.DATA_IMPORT_ERROR_DISCARDED;
 import static org.folio.bulkops.util.Constants.MSG_NO_CHANGE_REQUIRED;
 
+import java.io.BufferedReader;
 import java.io.ByteArrayInputStream;
+import java.io.InputStreamReader;
 import java.time.LocalDate;
 import java.util.ArrayList;
 import java.util.List;
@@ -29,6 +32,7 @@
 import org.folio.bulkops.domain.bean.JobLogEntry;
 import org.folio.bulkops.domain.bean.StateType;
 import org.folio.bulkops.domain.dto.Error;
+import org.folio.bulkops.domain.dto.ErrorType;
 import org.folio.bulkops.domain.dto.Errors;
 import org.folio.bulkops.domain.dto.IdentifierType;
 import org.folio.bulkops.domain.dto.Parameter;
@@ -56,7 +60,6 @@ public class ErrorService {
   private final BulkOperationRepository operationRepository;
   private final RemoteFileSystemClient remoteFileSystemClient;
   private final BulkOperationExecutionContentRepository executionContentRepository;
-  private final BulkEditClient bulkEditClient;
   private final MetadataProviderClient metadataProviderClient;
 
   public void saveError(UUID bulkOperationId, String identifier,  String errorMessage, String uiErrorMessage, String link) {
@@ -87,17 +90,23 @@ public void deleteErrorsByBulkOperationId(UUID bulkOperationId) {
     log.info("Errors deleted for bulk operation {}", bulkOperationId);
   }
 
-  public Errors getErrorsPreviewByBulkOperationId(UUID bulkOperationId, int limit) {
+  public Errors getErrorsPreviewByBulkOperationId(UUID bulkOperationId, int limit, int offset) {
     var bulkOperation = operationRepository.findById(bulkOperationId)
       .orElseThrow(() -> new NotFoundException("BulkOperation was not found by id=" + bulkOperationId));
     if (Set.of(DATA_MODIFICATION, REVIEW_CHANGES, REVIEWED_NO_MARC_RECORDS).contains(bulkOperation.getStatus()) || COMPLETED_WITH_ERRORS == bulkOperation.getStatus() && noCommittedErrors(bulkOperation)) {
-      var errors = bulkEditClient.getErrorsPreview(bulkOperation.getDataExportJobId(), limit);
-      return new Errors().errors(errors.getErrors().stream()
-          .map(this::prepareInternalErrorRepresentation)
-          .toList())
-        .totalRecords(errors.getTotalRecords());
+      var errors = new BufferedReader(new InputStreamReader(remoteFileSystemClient.get(bulkOperation.getLinkToMatchedRecordsErrorsCsvFile())))
+        .lines()
+        .skip(offset)
+        .limit(limit)
+        .map(message -> {
+          var error = message.split(Constants.COMMA_DELIMETER);
+          return new Error().message(error[1]).parameters(List.of(new Parameter().key(IDENTIFIER).value(error[0])));
+        })
+        .collect(toList());
+      return new Errors().errors(errors)
+        .totalRecords(remoteFileSystemClient.getNumOfLines(bulkOperation.getLinkToMatchedRecordsErrorsCsvFile()));
     } else if (COMPLETED == bulkOperation.getStatus() || COMPLETED_WITH_ERRORS == bulkOperation.getStatus()) {
-      return getExecutionErrors(bulkOperationId, limit);
+      return getExecutionErrors(bulkOperationId, limit, offset);
     } else {
       throw new NotFoundException("Errors preview is not available");
     }
@@ -139,19 +148,8 @@ private boolean noCommittedErrors(BulkOperation bulkOperation) {
     return isNull(bulkOperation.getCommittedNumOfErrors()) || bulkOperation.getCommittedNumOfErrors() == 0;
   }
 
-  private Error prepareInternalErrorRepresentation(Error e) {
-    var error= e.getMessage().split(Constants.COMMA_DELIMETER);
-    return new Error().message(error[1]).parameters(List.of(new Parameter().key(IDENTIFIER).value(error[0])));
-  }
-
-  public String getErrorsCsvByBulkOperationId(UUID bulkOperationId) {
-    return getErrorsPreviewByBulkOperationId(bulkOperationId, Integer.MAX_VALUE).getErrors().stream()
-      .map(error -> String.join(Constants.COMMA_DELIMETER, ObjectUtils.isEmpty(error.getParameters()) ? EMPTY : error.getParameters().get(0).getValue(), error.getMessage()))
-      .collect(Collectors.joining(Constants.NEW_LINE_SEPARATOR));
-  }
-
-  private Errors getExecutionErrors(UUID bulkOperationId, int limit) {
-    var errorPage = executionContentRepository.findByBulkOperationIdAndErrorMessageIsNotNull(bulkOperationId, OffsetRequest.of(0, limit));
+  private Errors getExecutionErrors(UUID bulkOperationId, int limit, int offset) {
+    var errorPage = executionContentRepository.findByBulkOperationIdAndErrorMessageIsNotNull(bulkOperationId, OffsetRequest.of(offset, limit));
     var errors = errorPage.toList().stream()
       .map(this::executionContentToFolioError)
       .toList();

src/main/resources/swagger.api/bulk-operations.yaml

@@ -312,6 +312,12 @@ paths:
           schema:
             type: integer
           description: The numbers of errors to return
+        - in: query
+          name: offset
+          required: true
+          schema:
+            type: integer
+          description: The query offset
       responses:
         '200':
           description: Collection of errors for preview

src/test/java/org/folio/bulkops/service/ErrorServiceTest.java

@@ -18,10 +18,16 @@
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 import static wiremock.org.hamcrest.MatcherAssert.assertThat;
+import static wiremock.org.hamcrest.Matchers.anyOf;
+import static wiremock.org.hamcrest.Matchers.contains;
 import static wiremock.org.hamcrest.Matchers.equalTo;
+import static wiremock.org.hamcrest.Matchers.hasItems;
 import static wiremock.org.hamcrest.Matchers.hasSize;
 
+import java.io.IOException;
 import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Paths;
 import java.time.LocalDate;
 import java.util.Arrays;
 import java.util.List;
@@ -155,19 +161,15 @@ void shouldUploadErrorsAndReturnLinkToFile() {
 
   @ParameterizedTest
   @EnumSource(value = OperationStatusType.class, names = { "DATA_MODIFICATION", "COMPLETED" }, mode = EnumSource.Mode.INCLUDE)
-  void shouldGetErrorsPreviewByBulkOperationId(OperationStatusType statusType) {
+  void shouldGetErrorsPreviewByBulkOperationId(OperationStatusType statusType) throws IOException {
     try (var context =  new FolioExecutionContextSetter(folioExecutionContext)) {
       var operationId = bulkOperationRepository.save(BulkOperation.builder().id(UUID.randomUUID()).dataExportJobId(UUID.randomUUID()).status(statusType).build()).getId();
 
-      var expected = List.of(
-        new Error().message("No match found").parameters(List.of(new Parameter().key("IDENTIFIER").value("123"))),
-        new Error().message("Invalid format").parameters(List.of(new Parameter().key("IDENTIFIER").value("456")))
-      );
-
       mockErrorsData(statusType, operationId);
 
-      var actual = errorService.getErrorsPreviewByBulkOperationId(operationId, 2);
-      assertThat(actual.getErrors(), hasSize(2));
+      var actualWithOffset = errorService.getErrorsPreviewByBulkOperationId(operationId, 2, 1);
+      assertThat(actualWithOffset.getErrors(), hasSize(1));
+      assertThat(actualWithOffset.getTotalRecords(), equalTo(2));
 
       bulkOperationRepository.deleteById(operationId);
     }
@@ -179,48 +181,15 @@ void shouldRejectErrorsPreviewOnWrongOperationStatus(OperationStatusType statusT
     try (var context =  new FolioExecutionContextSetter(folioExecutionContext)) {
       var operationId = bulkOperationRepository.save(BulkOperation.builder().id(UUID.randomUUID()).status(statusType).build()).getId();
 
-      assertThrows(NotFoundException.class, () -> errorService.getErrorsPreviewByBulkOperationId(operationId, 10));
-
-      bulkOperationRepository.deleteById(operationId);
-    }
-  }
-
-  @ParameterizedTest
-  @EnumSource(value = OperationStatusType.class, names = { "DATA_MODIFICATION", "COMPLETED" }, mode = EnumSource.Mode.INCLUDE)
-  void shouldGetErrorsCsvByBulkOperationId(OperationStatusType statusType) {
-    try (var context =  new FolioExecutionContextSetter(folioExecutionContext)) {
-      var operationId = bulkOperationRepository.save(BulkOperation.builder().id(UUID.randomUUID()).dataExportJobId(UUID.randomUUID()).status(statusType).build()).getId();
-
-      var expected = "123,No match found\n456,Invalid format".split(LF);
-
-      mockErrorsData(statusType, operationId);
-
-      var actual = errorService.getErrorsCsvByBulkOperationId(operationId).split(LF);
-      Arrays.sort(expected);
-      Arrays.sort(actual);
-
-      assertArrayEquals(expected, actual);
-      assertThat(actual.length, equalTo(2));
-
-      bulkOperationRepository.deleteById(operationId);
-    }
-  }
-
-  @ParameterizedTest
-  @EnumSource(value = OperationStatusType.class, names = { "DATA_MODIFICATION", "REVIEW_CHANGES", "COMPLETED", "COMPLETED_WITH_ERRORS", "REVIEWED_NO_MARC_RECORDS" }, mode = EnumSource.Mode.EXCLUDE)
-  void shouldRejectErrorsCsvOnWrongOperationStatus(OperationStatusType statusType) {
-    try (var context =  new FolioExecutionContextSetter(folioExecutionContext)) {
-      var operationId = bulkOperationRepository.save(BulkOperation.builder().id(UUID.randomUUID()).status(statusType).build()).getId();
-
-      assertThrows(NotFoundException.class, () -> errorService.getErrorsCsvByBulkOperationId(operationId));
+      assertThrows(NotFoundException.class, () -> errorService.getErrorsPreviewByBulkOperationId(operationId, 10, 1));
 
       bulkOperationRepository.deleteById(operationId);
     }
   }
 
   @ParameterizedTest
   @ValueSource(ints = {0, 1})
-  void shouldReturnErrorsPreviewOnCompletedWithErrors(int committedErrors) {
+  void shouldReturnErrorsPreviewOnCompletedWithErrors(int committedErrors) throws IOException {
     try (var context =  new FolioExecutionContextSetter(folioExecutionContext)) {
       var jobId = UUID.randomUUID();
 
@@ -247,9 +216,10 @@ void shouldReturnErrorsPreviewOnCompletedWithErrors(int committedErrors) {
           .build());
       }
 
-      var errors = errorService.getErrorsPreviewByBulkOperationId(operationId, 10);
+      var errors = errorService.getErrorsPreviewByBulkOperationId(operationId, 10, 0);
 
       assertThat(errors.getErrors(), hasSize(2));
+      assertThat(errors.getTotalRecords(), equalTo(2));
     }
   }
 
@@ -276,7 +246,7 @@ void testOptimisticLockErrorProcessing() {
         .linkToFailedEntity(link)
         .build());
 
-      var errors = errorService.getErrorsPreviewByBulkOperationId(operationId, 1);
+      var errors = errorService.getErrorsPreviewByBulkOperationId(operationId, 1, 0);
 
       assertThat(errors.getErrors(), hasSize(1));
       assertThat(errors.getErrors().get(0).getParameters(), hasSize(2));
@@ -304,7 +274,10 @@ void testSaveErrorsFromDataImport(IdentifierType identifierType, boolean related
     final var instanceInfo = new RelatedInstanceInfo().withIdList(List.of(instanceId)).withHridList(List.of("instance HRID"));
     when(metadataProviderClient.getJobLogEntries(dataImportJobId.toString(), Integer.MAX_VALUE))
       .thenReturn(new JobLogEntryCollection().withEntries(List.of(new JobLogEntry()
-        .withError("some MARC error").withSourceRecordId(sourceRecordId).withRelatedInstanceInfo(
+        .withError("some MARC error #1").withSourceRecordId(sourceRecordId).withRelatedInstanceInfo(
+          relatedInstanceInfo ? instanceInfo : new RelatedInstanceInfo().withIdList(List.of()).withHridList(List.of())
+        ), new JobLogEntry()
+        .withError("some MARC error #2").withSourceRecordId(sourceRecordId).withRelatedInstanceInfo(
           relatedInstanceInfo ? instanceInfo : new RelatedInstanceInfo().withIdList(List.of()).withHridList(List.of())
         ))));
     when(srsClient.getSrsRecordById(sourceRecordId)).thenReturn(new SrsRecord().withExternalIdsHolder(
@@ -315,15 +288,16 @@ void testSaveErrorsFromDataImport(IdentifierType identifierType, boolean related
           .id(UUID.randomUUID())
           .status(COMPLETED_WITH_ERRORS)
           .identifierType(identifierType)
-          .committedNumOfErrors(1)
+          .committedNumOfErrors(2)
           .dataExportJobId(dataExportJobId)
           .build())
         .getId();
       errorService.saveErrorsFromDataImport(operationId, dataImportJobId);
-      var errors = errorService.getErrorsPreviewByBulkOperationId(operationId, 1);
+      var errors = errorService.getErrorsPreviewByBulkOperationId(operationId, 2, 1);
 
       assertThat(errors.getErrors(), hasSize(1));
-      assertEquals("some MARC error", errors.getErrors().get(0).getMessage());
+      assertThat(errors.getTotalRecords(), equalTo(2));
+      assertThat(errors.getErrors().stream().map(Error::getMessage).toList(), anyOf(contains("some MARC error #1"), contains("some MARC error #2")));
     }
   }
 
@@ -346,7 +320,7 @@ void shouldNotSaveError_IfErrorFromDataImportIsEmpty() {
           .build())
         .getId();
       errorService.saveErrorsFromDataImport(operationId, dataImportJobId);
-      var errors = errorService.getErrorsPreviewByBulkOperationId(operationId, 1);
+      var errors = errorService.getErrorsPreviewByBulkOperationId(operationId, 1, 1);
 
       assertThat(errors.getErrors(), hasSize(0));
     }
@@ -375,7 +349,7 @@ void testSaveErrorsFromDataImport_whenDiscardedAndNoMessage() {
           .build())
         .getId();
       errorService.saveErrorsFromDataImport(operationId, dataImportJobId);
-      var errors = errorService.getErrorsPreviewByBulkOperationId(operationId, 1);
+      var errors = errorService.getErrorsPreviewByBulkOperationId(operationId, 1, 0);
 
       assertThat(errors.getErrors(), hasSize(1));
       assertEquals(DATA_IMPORT_ERROR_DISCARDED, errors.getErrors().get(0).getMessage());
@@ -404,12 +378,12 @@ void testDataImportException() {
     }
   }
 
-  private void mockErrorsData(OperationStatusType statusType, UUID operationId) {
+  private void mockErrorsData(OperationStatusType statusType, UUID operationId) throws IOException {
     if (DATA_MODIFICATION == statusType || COMPLETED_WITH_ERRORS == statusType) {
-      when(bulkEditClient.getErrorsPreview(any(UUID.class), anyInt()))
-        .thenReturn(new Errors()
-          .errors(List.of(new Error().type(ErrorType.ERROR).message("123,No match found"),
-            new Error().type(ErrorType.ERROR).message("456,Invalid format"))));
+      when(remoteFileSystemClient.get(any()))
+        .thenReturn(Files.newInputStream(Paths.get("src/test/resources/files/errors.csv")));
+      when(remoteFileSystemClient.getNumOfLines(any()))
+        .thenReturn(2);
     } else {
       executionContentRepository.save(BulkOperationExecutionContent.builder()
         .bulkOperationId(operationId)

src/test/resources/files/errors.csv

@@ -1 +1,2 @@
 12345678,Not Found
+987654321,Bad data