Skip to content

Commit

Permalink
MODINVOICE-523 - After resaving the configurations with empty credent…
Browse files Browse the repository at this point in the history
…ials, an NPE occurs during manual export (#471)

* [MODINVOICE-523] - After resaving the configurations with empty credentials, an NPE occurs during manual export

* [MODINVOICE-523] - After resaving the configurations with empty credentials, an NPE occurs during manual export

* [MODINVOICE-523] - After resaving the configurations with empty credentials, an NPE occurs during manual export

* [MODINVOICE-532] - Improvements for the logic

* [MODINVOICE-532] - Improvements for the logic

* [MODINVOICE-532] - Improvements for the logic

* [MODINVOICE-532] - Improvements for the logic

* [MODINVOICE-532] - Added improvement to check url, directory and remove credentials in case of they are empty

* [MODINVOICE-523] - Moved remove credentials feature to helper method

* [MODINVOICE-523] - After adding avoid saving empty credentials, validation are being reverted

* [MODINVOICE-523] - Covered with test

* [MODINVOICE-523] - Covered with test

* [MODINVOICE-523] - Covered with test

* [MODINVOICE-532] - Improvements for code
  • Loading branch information
azizbekxm authored Feb 27, 2024
1 parent d3c8a9c commit 9f1efe1
Show file tree
Hide file tree
Showing 7 changed files with 188 additions and 28 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@
import java.util.Map;
import java.util.concurrent.CompletionException;

import org.apache.commons.lang3.StringUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.folio.HttpStatus;
import org.folio.exceptions.FtpException;
import org.folio.rest.core.RestClient;
Expand All @@ -29,6 +32,7 @@

public class BatchVoucherExportConfigHelper extends AbstractHelper {

private static final Logger log = LogManager.getLogger();
public static final String GET_EXPORT_CONFIGS_BY_QUERY = resourcesPath(BATCH_VOUCHER_EXPORT_CONFIGS) + SEARCH_PARAMS;
@Autowired
BatchVoucherExportConfigService batchVoucherExportConfigService;
Expand All @@ -55,7 +59,13 @@ public Future<Credentials> getExportConfigCredentials(String id) {
}

public Future<Credentials> createCredentials(String id, Credentials credentials) {
return restClient.post(String.format(resourcesPath(BATCH_VOUCHER_EXPORT_CONFIGS_CREDENTIALS), id), credentials, Credentials.class, buildRequestContext());
log.debug("createCredentials:: Trying to create export config credentials for configId={}", id);
if (StringUtils.isBlank(credentials.getUsername()) || StringUtils.isBlank(credentials.getPassword())) {
log.info("createCredentials:: new username and/or password is empty, so new record '{}' is not being saved to db", id);
return Future.succeededFuture(new Credentials());
}
return restClient.post(String.format(resourcesPath(BATCH_VOUCHER_EXPORT_CONFIGS_CREDENTIALS), id),
credentials, Credentials.class, buildRequestContext());
}

public Future<Void> putExportConfig(ExportConfig exportConfig) {
Expand All @@ -65,6 +75,11 @@ public Future<Void> putExportConfig(ExportConfig exportConfig) {

public Future<Void> putExportConfigCredentials(String id, Credentials credentials) {
String path = String.format(resourcesPath(BATCH_VOUCHER_EXPORT_CONFIGS_CREDENTIALS), id);
log.debug("putExportConfigCredentials:: Trying to update export config credentials for configId={} with path={}", id, path);
if (StringUtils.isBlank(credentials.getUsername()) || StringUtils.isBlank(credentials.getPassword())) {
log.info("putExportConfigCredentials:: new username and/or password is empty, so deleting existing record");
return restClient.delete(path, buildRequestContext());
}
return restClient.put(path, credentials, buildRequestContext());
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
package org.folio.rest.impl;

import static org.folio.invoices.utils.ErrorCodes.BATCH_VOUCHER_NOT_FOUND;
import static org.folio.services.ftp.FtpUploadService.URI_SYNTAX_ERROR;
import static org.folio.services.ftp.FtpUploadService.URL_NOT_FOUND_FOR_FTP;

import java.net.URISyntaxException;
import java.util.List;
import java.util.Map;

Expand Down Expand Up @@ -38,6 +39,7 @@ public class UploadBatchVoucherExportHelper extends AbstractHelper {
private BatchVoucherExportConfigService batchVoucherExportConfigService;
@Autowired
private BatchVoucherExportsService batchVoucherExportsService;

private static final String CREDENTIALS_NOT_FOUND = "Credentials for FTP upload were not found";

private final RequestContext requestContext;
Expand Down Expand Up @@ -135,22 +137,24 @@ private Future<Void> updateHolderWithCredentials(BatchVoucherUploadHolder upload
}

private Future<Void> updateHolderWithBatchVoucher(BatchVoucherUploadHolder uploadHolder) {
return batchVoucherService.getBatchVoucherById(uploadHolder.getBatchVoucherExport().getBatchVoucherId(), buildRequestContext())
.onSuccess(uploadHolder::setBatchVoucher)
.recover(t -> {
var parameter = new Parameter().withKey("batchVoucherId").withValue(uploadHolder.getBatchVoucherExport().getBatchVoucherId());
var causeParam = new Parameter().withKey("cause").withValue(t.getMessage());
var error = BATCH_VOUCHER_NOT_FOUND.toError().withParameters(List.of(parameter, causeParam));
log.error("Failed to fetch batch voucher by id: {}", JsonObject.mapFrom(error).encodePrettily());
throw new HttpException(404, error);
})
.mapEmpty();
}
return batchVoucherService.getBatchVoucherById(uploadHolder.getBatchVoucherExport().getBatchVoucherId(), buildRequestContext())
.onSuccess(uploadHolder::setBatchVoucher)
.recover(t -> {
var parameter = new Parameter().withKey("batchVoucherId").withValue(uploadHolder.getBatchVoucherExport().getBatchVoucherId());
var causeParam = new Parameter().withKey("cause").withValue(t.getMessage());
var error = BATCH_VOUCHER_NOT_FOUND.toError().withParameters(List.of(parameter, causeParam));
log.error("Failed to fetch batch voucher by id: {}", JsonObject.mapFrom(error).encodePrettily());
throw new HttpException(404, error);
})
.mapEmpty();
}

private Future<Void> failUploadUpdate(BatchVoucherExport bvExport, Throwable t) {
if (bvExport != null) {
if (!CREDENTIALS_NOT_FOUND.equals(t.getMessage())
&& !(t instanceof URISyntaxException)) {
boolean isNotFoundError = URL_NOT_FOUND_FOR_FTP.equals(t.getMessage()) ||
URI_SYNTAX_ERROR.equals(t.getMessage()) ||
CREDENTIALS_NOT_FOUND.equals(t.getMessage());
if (!isNotFoundError) {
bvExport.setStatus(BatchVoucherExport.Status.ERROR);
}
bvExport.setMessage(t.getMessage());
Expand Down
14 changes: 11 additions & 3 deletions src/main/java/org/folio/services/ftp/FtpUploadService.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.folio.exceptions.FtpException;
import org.folio.invoices.rest.exceptions.HttpException;
import org.folio.rest.jaxrs.model.ExportConfig;

import io.vertx.core.AsyncResult;
Expand All @@ -26,16 +27,22 @@
public class FtpUploadService implements FileExchangeService {

private static final Logger logger = LogManager.getLogger(FtpUploadService.class);

private static final String DEFAULT_WORKING_DIR = "/files/invoices";
public static final String URL_NOT_FOUND_FOR_FTP = "URI for FTP upload was not found";
public static final String URI_SYNTAX_ERROR = "URI should be valid ftp path";

private final String server;
private final int port;
private final Context ctx;

public FtpUploadService(Context ctx, String uri, Integer portFromConfig) throws URISyntaxException {
if (StringUtils.isBlank(uri)) {
logger.error("FtpUploadService:: URI is not found");
throw new HttpException(400, URL_NOT_FOUND_FOR_FTP);
}
if (!isUriValid(uri)) {
throw new URISyntaxException(uri, "URI should be valid ftp path");
logger.error("FtpUploadService:: URI '{}' is not valid", uri);
throw new URISyntaxException(uri, URI_SYNTAX_ERROR);
}
URI u = new URI(uri);
this.server = u.getHost();
Expand Down Expand Up @@ -109,9 +116,10 @@ public Future<String> upload(Context ctx, String username, String password, Stri
try (InputStream is = new ByteArrayInputStream(content.getBytes())) {
ftpClient.setFileType(FTP.BINARY_FILE_TYPE);
ftpClient.enterLocalPassiveMode();
if (Objects.nonNull(folder)) {
if (StringUtils.isNotBlank(folder)) {
changeWorkingDirectory(folder, ftpClient);
} else {
logger.warn("upload:: folder is empty using default working directory={}", DEFAULT_WORKING_DIR);
changeWorkingDirectory(DEFAULT_WORKING_DIR, ftpClient);
}
if (ftpClient.storeFile(filename, is)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.nullValue;

import java.io.IOException;

Expand All @@ -26,25 +27,27 @@ public class BatchVoucherExportConfigCredentialsTest extends ApiTestBase {

static final String CREDENTIALS_ID = "574f0791-beca-4470-8037-050660cfb73a";
static final String BAD_CREDENTIALS_ID = "badf0791-beca-4470-8037-050660cfb73a";
static final String EMPTY_CREDENTIALS_ID = "ef48e416-3411-48f2-9e60-a0297d385403";
static final String CONFIGURATION_ID = "089b333c-503f-4627-895d-26eaab1e392e";
static final String EMPTY_CONFIGURATION_ID = "a00ae5e7-2aab-44ec-b0e1-f932248cff6f";
static final String BAD_CREDENTIALS_CONFIGURATION_ID = "badb333c-503f-4627-895d-26eaab1e392e";

private static final String BATCH_VOUCHER_EXPORT_CONFIGS_CREDENTIALS_ENDPOINT =
String.format("/batch-voucher/export-configurations/%s/credentials", CONFIGURATION_ID);
private static final String BATCH_VOUCHER_EXPORT_CONFIGS_EMPTY_CREDENTIALS_ENDPOINT =
String.format("/batch-voucher/export-configurations/%s/credentials", EMPTY_CONFIGURATION_ID);
private static final String BATCH_VOUCHER_EXPORT_CONFIGS_CREDENTIALS_TEST_ENDPOINT =
String.format("/batch-voucher/export-configurations/%s/credentials/test", CONFIGURATION_ID);
private static final String BATCH_VOUCHER_EXPORT_CONFIGS_CREDENTIALS_TEST_ERROR_ENDPOINT =
String.format("/batch-voucher/export-configurations/%s/credentials/test", BAD_CREDENTIALS_CONFIGURATION_ID);

static final String BATCH_VOUCHER_EXPORT_CONFIG_SAMPLE_PATH = BASE_MOCK_DATA_PATH + "batchVoucherExportConfigs/";
static final String BATCH_VOUCHER_EXPORT_CONFIG_CREDENTIALS_SAMPLE_PATH = BASE_MOCK_DATA_PATH + "credentials/";

static final String BATCH_VOUCHER_EXPORT_CONFIG_CREDENTIALS_SAMPLE_PATH_WITH_ID =
BATCH_VOUCHER_EXPORT_CONFIG_CREDENTIALS_SAMPLE_PATH + CREDENTIALS_ID + ".json";
static final String BATCH_VOUCHER_EXPORT_CONFIG_BAD_CREDENTIALS_SAMPLE_PATH_WITH_ID =
BATCH_VOUCHER_EXPORT_CONFIG_CREDENTIALS_SAMPLE_PATH + BAD_CREDENTIALS_ID + ".json";
static final String BAD_CREDENTIALS_BATCH_VOUCHER_EXPORT_CONFIG_SAMPLE_PATH_WITH_ID =
BATCH_VOUCHER_EXPORT_CONFIG_SAMPLE_PATH + BAD_CREDENTIALS_CONFIGURATION_ID + ".json";
static final String BATCH_VOUCHER_EXPORT_CONFIG_EMPTY_CREDENTIALS_PATH_WITH_ID =
BATCH_VOUCHER_EXPORT_CONFIG_CREDENTIALS_SAMPLE_PATH + EMPTY_CREDENTIALS_ID + ".json";

@Test
public void testPostExportConfigCredentials() throws IOException {
Expand All @@ -60,6 +63,52 @@ public void testPostExportConfigCredentials() throws IOException {
assertThat(MockServer.serverRqRs.get(BATCH_VOUCHER_EXPORT_CONFIGS_CREDENTIALS, HttpMethod.POST), hasSize(1));
}

@Test
public void testNotPostExportConfigCredentialsWhenEmpty() throws IOException {
logger.info("=== Test post batch voucher export configuration credentials with empty username and password. It shouldn't be saved to db");

var credentials = getMockAsJson(BATCH_VOUCHER_EXPORT_CONFIG_EMPTY_CREDENTIALS_PATH_WITH_ID).mapTo(Credentials.class);
credentials.setUsername("john");
credentials.setPassword("1234");

var res = verifyPostResponse(BATCH_VOUCHER_EXPORT_CONFIGS_EMPTY_CREDENTIALS_ENDPOINT, credentials,
prepareHeaders(X_OKAPI_TENANT), APPLICATION_JSON, 201).as(Credentials.class);

String id = res.getId();
assertThat(id, notNullValue());
assertThat(MockServer.serverRqRs.get(BATCH_VOUCHER_EXPORT_CONFIGS_CREDENTIALS, HttpMethod.POST), hasSize(1));

credentials.setUsername("");
credentials.setPassword("");

var res2 = verifyPostResponse(BATCH_VOUCHER_EXPORT_CONFIGS_EMPTY_CREDENTIALS_ENDPOINT, credentials,
prepareHeaders(X_OKAPI_TENANT), APPLICATION_JSON, 201).as(Credentials.class);

String id2 = res2.getId();
assertThat(id2, nullValue());
assertThat(MockServer.serverRqRs.get(BATCH_VOUCHER_EXPORT_CONFIGS_CREDENTIALS, HttpMethod.POST), hasSize(1));

credentials.setUsername("john");
credentials.setPassword("");

var res3 = verifyPostResponse(BATCH_VOUCHER_EXPORT_CONFIGS_EMPTY_CREDENTIALS_ENDPOINT, credentials,
prepareHeaders(X_OKAPI_TENANT), APPLICATION_JSON, 201).as(Credentials.class);

String id3 = res3.getId();
assertThat(id3, nullValue());
assertThat(MockServer.serverRqRs.get(BATCH_VOUCHER_EXPORT_CONFIGS_CREDENTIALS, HttpMethod.POST), hasSize(1));

credentials.setUsername(" ");
credentials.setPassword("123");

var res4 = verifyPostResponse(BATCH_VOUCHER_EXPORT_CONFIGS_EMPTY_CREDENTIALS_ENDPOINT, credentials,
prepareHeaders(X_OKAPI_TENANT), APPLICATION_JSON, 201).as(Credentials.class);

String id4 = res4.getId();
assertThat(id4, nullValue());
assertThat(MockServer.serverRqRs.get(BATCH_VOUCHER_EXPORT_CONFIGS_CREDENTIALS, HttpMethod.POST), hasSize(1));
}

@Test
public void testGetExportConfigCredentials() {
logger.info("=== Test get batch voucher export configuration credentials ===");
Expand Down
65 changes: 59 additions & 6 deletions src/test/java/org/folio/services/ftp/FtpUploadServiceTest.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
package org.folio.services.ftp;

import static org.folio.services.ftp.FtpUploadService.URL_NOT_FOUND_FOR_FTP;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;

import java.io.IOException;
import java.net.URISyntaxException;
import java.text.SimpleDateFormat;
Expand All @@ -8,9 +12,15 @@
import java.util.TimeZone;
import java.util.UUID;

import io.vertx.core.Context;
import io.vertx.core.Vertx;
import io.vertx.core.json.JsonObject;
import io.vertx.junit5.VertxExtension;
import io.vertx.junit5.VertxTestContext;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.folio.exceptions.FtpException;
import org.folio.invoices.rest.exceptions.HttpException;
import org.folio.rest.jaxrs.model.BatchVoucher;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.Assertions;
Expand All @@ -23,12 +33,6 @@
import org.mockftpserver.fake.filesystem.FileSystem;
import org.mockftpserver.fake.filesystem.UnixFakeFileSystem;

import io.vertx.core.Context;
import io.vertx.core.Vertx;
import io.vertx.core.json.JsonObject;
import io.vertx.junit5.VertxExtension;
import io.vertx.junit5.VertxTestContext;

@ExtendWith(VertxExtension.class)
public class FtpUploadServiceTest {

Expand Down Expand Up @@ -190,4 +194,53 @@ public void testFailedUpload(VertxTestContext vertxTestContext) throws URISyntax
vertxTestContext.assertFailure(future)
.onComplete(result -> vertxTestContext.completeNow());
}

@Test
public void testFailedUploadWhenURLProblem() {
logger.info("=== Test unsuccessful upload ===");

Date end = new Date();
end.setTime(System.currentTimeMillis() - 864000000);

BatchVoucher batchVoucher = new BatchVoucher();
batchVoucher.setId(UUID.randomUUID().toString());
batchVoucher.setStart(end);
batchVoucher.setEnd(end);
batchVoucher.setBatchGroup(UUID.randomUUID().toString());
batchVoucher.setCreated(new Date());

var httpException = assertThrows(HttpException.class, () -> new FtpUploadService(context, "", 0));
assertEquals(400, httpException.getCode());
assertEquals(URL_NOT_FOUND_FOR_FTP, httpException.getMessage());

var urlException = assertThrows(URISyntaxException.class, () -> new FtpUploadService(context, "fsp:/", 0));
assertEquals("fsp:/", urlException.getInput());
assertEquals("URI should be valid ftp path: fsp:/", urlException.getMessage());
}

@Test
public void testUserDefaultFolderWhenDirectoryEmpty(VertxTestContext vertxTestContext) throws URISyntaxException {
logger.info("=== Test successful upload ===");

Date end = new Date();
end.setTime(System.currentTimeMillis() - 864000000);

BatchVoucher batchVoucher = new BatchVoucher();
batchVoucher.setId(UUID.randomUUID().toString());
batchVoucher.setStart(end);
batchVoucher.setEnd(end);
batchVoucher.setBatchGroup(UUID.randomUUID().toString());
batchVoucher.setCreated(new Date());

FtpUploadService helper = new FtpUploadService(context, uri, 0);
var future = helper.upload(context,username_valid, password_valid, "", filename, JsonObject.mapFrom(batchVoucher).encodePrettily())
.onSuccess(logger::info)
.onFailure(t -> {
logger.error(t);
Assertions.fail(t.getMessage());
})
.onComplete(logger::info);
vertxTestContext.assertComplete(future)
.onSuccess(result -> vertxTestContext.completeNow());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import static io.vertx.core.Future.succeededFuture;
import static org.folio.ApiTestSuite.mockPort;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.eq;
Expand All @@ -20,6 +21,7 @@
import io.vertx.junit5.VertxExtension;
import io.vertx.junit5.VertxTestContext;
import org.folio.config.ApplicationConfig;
import org.folio.exceptions.FtpException;
import org.folio.invoices.rest.exceptions.HttpException;
import org.folio.rest.RestConstants;
import org.folio.rest.core.RestClient;
Expand Down Expand Up @@ -49,6 +51,7 @@ public class UploadBatchVoucherExportServiceTest extends ApiTestBase {
private static final String BATCH_VOUCHERS_PATH = BASE_MOCK_DATA_PATH + "batchVouchers/" + BV_ID + ".json";
private static final String BATCH_VOUCHERS_EXPORT_PATH = BASE_MOCK_DATA_PATH + "batchVoucherExports/" + BV_EXPORT_ID + ".json";
private static final String BATCH_VOUCHERS_EXPORT_CONF_PATH = BASE_MOCK_DATA_PATH + "batchVoucherExportConfigs/" + BV_EXPORT_CONF_ID + ".json";
private static final String BATCH_VOUCHERS_EXPORT_CONF_COLLECTION_PATH = BASE_MOCK_DATA_PATH + "batchVoucherExportConfigs/" + "configs.json";
private static final String CRED_PATH = BASE_MOCK_DATA_PATH + "credentials/" + CRED_ID + ".json";


Expand All @@ -57,6 +60,8 @@ public class UploadBatchVoucherExportServiceTest extends ApiTestBase {
@Mock
private BatchVoucherExportConfigHelper bvExportConfigHelper;
@Mock
private BatchVoucherExportConfigService batchVoucherExportConfigService;
@Mock
private BatchVoucherExportsHelper bvExportsHelper;
private Context context;
@Mock
Expand Down Expand Up @@ -166,4 +171,24 @@ public void testFineNameGenerateLogicIdThereNoSeparatorInUUID() {
//Then
Assertions.assertEquals("bv_"+bv.getId()+"_Amherst College (AC)_2019-12-06_2019-12-07.json", actFileName);
}

@Test
public void testChangeStatus(VertxTestContext vertxTestContext) {
//given
UploadBatchVoucherExportHelper serviceSpy = spy(new UploadBatchVoucherExportHelper(okapiHeaders, context));
BatchVoucherExport bv = getMockAsJson(BATCH_VOUCHERS_EXPORT_PATH).mapTo(BatchVoucherExport.class);
bv.setId("xxxyyyzzb58dcd02ee14");

var future = serviceSpy.uploadBatchVoucherExport(bv);

vertxTestContext.assertFailure(future)
.onComplete(event -> {
FtpException exception = (FtpException) event.cause();
assertEquals(BatchVoucherExport.Status.ERROR, bv.getStatus());
assertEquals("Unable to connect to ftp.amherst-lib.edu:22", exception.getMessage());
assertEquals(403, exception.getReplyCode());
vertxTestContext.completeNow();
});
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"id": "ef48e416-3411-48f2-9e60-a0297d385403",
"exportConfigId": "a00ae5e7-2aab-44ec-b0e1-f932248cff6f",
"username": "",
"password": ""
}

0 comments on commit 9f1efe1

Please sign in to comment.