From 38c69100aab6bcc5496660977b3fd6f368e8c796 Mon Sep 17 00:00:00 2001 From: mlm483 <128052931+mlm483@users.noreply.github.com> Date: Fri, 13 Oct 2023 16:18:56 -0400 Subject: [PATCH 1/3] [BI-1900] - updated validation error messages --- .../model/imports/BrAPIImportService.java | 15 ++++++ .../ExperimentImportService.java | 5 ++ .../importer/services/MappingManager.java | 46 ++++++++----------- .../importer/GermplasmFileImportTest.java | 20 ++++---- 4 files changed, 49 insertions(+), 37 deletions(-) diff --git a/src/main/java/org/breedinginsight/brapps/importer/model/imports/BrAPIImportService.java b/src/main/java/org/breedinginsight/brapps/importer/model/imports/BrAPIImportService.java index bd6478265..3a8f56303 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/model/imports/BrAPIImportService.java +++ b/src/main/java/org/breedinginsight/brapps/importer/model/imports/BrAPIImportService.java @@ -33,6 +33,21 @@ public abstract class BrAPIImportService { public String getImportTypeId() {return null;} public BrAPIImport getImportClass() {return null;} + public String getWrongDataTypeMsg(String columnName) { + return String.format("Column name \"%s\" must be integer type, but non-integer type provided.", columnName); + } + public String getBlankRequiredFieldMsg(String fieldName) { + return String.format("Required field \"%s\" cannot contain empty values", fieldName); + } + public String getMissingColumnMsg(String columnName) { + return String.format("Column name \"%s\" does not exist in file", columnName); + } + public String getMissingUserInputMsg(String fieldName) { + return String.format("User input, \"%s\" is required", fieldName); + } + public String getWrongUserInputDataTypeMsg(String fieldName, String typeName) { + return String.format("User input, \"%s\" must be an %s", fieldName, typeName); + } public ImportPreviewResponse process(List brAPIImports, Table data, Program program, ImportUpload upload, User user, Boolean commit) throws UnprocessableEntityException, DoesNotExistException, ValidatorException, ApiException, MissingRequiredInfoException {return null;} } diff --git a/src/main/java/org/breedinginsight/brapps/importer/model/imports/experimentObservation/ExperimentImportService.java b/src/main/java/org/breedinginsight/brapps/importer/model/imports/experimentObservation/ExperimentImportService.java index b12f8ec40..4978ed9b4 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/model/imports/experimentObservation/ExperimentImportService.java +++ b/src/main/java/org/breedinginsight/brapps/importer/model/imports/experimentObservation/ExperimentImportService.java @@ -63,6 +63,11 @@ public String getImportTypeId() { return IMPORT_TYPE_ID; } + @Override + public String getMissingColumnMsg(String columnName) { + return "Column heading does not match template or ontology"; + } + @Override public ImportPreviewResponse process(List brAPIImports, Table data, Program program, ImportUpload upload, User user, Boolean commit) throws UnprocessableEntityException, ValidatorException, ApiException, MissingRequiredInfoException { diff --git a/src/main/java/org/breedinginsight/brapps/importer/services/MappingManager.java b/src/main/java/org/breedinginsight/brapps/importer/services/MappingManager.java index 808784821..e6cd869ad 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/services/MappingManager.java +++ b/src/main/java/org/breedinginsight/brapps/importer/services/MappingManager.java @@ -53,12 +53,6 @@ public class MappingManager { private ImportConfigManager configManager; - public static String wrongDataTypeMsg = "Column name \"%s\" must be integer type, but non-integer type provided."; - public static String blankRequiredField = "Required field \"%s\" cannot contain empty values"; - public static String missingColumn = "Column name \"%s\" does not exist in file"; - public static String missingUserInput = "User input, \"%s\" is required"; - public static String wrongUserInputDataType = "User input, \"%s\" must be an %s"; - @Inject MappingManager(ImportConfigManager configManager) { this.configManager = configManager; @@ -96,7 +90,7 @@ private List map(ImportMapping importMapping, Table data, Map map(ImportMapping importMapping, Table data, Map mappings, Table importFile, Integer rowIndex, - Map userInput, Boolean process, ValidationErrors validationErrors) throws UnprocessableEntityException { + Map userInput, Boolean process, ValidationErrors validationErrors, + BrAPIImportService importService) throws UnprocessableEntityException { Row focusRow = importFile.row(rowIndex); // Process this field @@ -131,7 +126,7 @@ private void mapField(Object parent, Field field, List mappings, T if (type.collectTime().equals(ImportCollectTimeEnum.UPLOAD)) { // User input, check that it was passed if (process) { - mapUserInputField(parent, field, userInput, type, metadata, required); + mapUserInputField(parent, field, userInput, type, metadata, required, importService); } return; } @@ -167,7 +162,7 @@ private void mapField(Object parent, Field field, List mappings, T brAPIObject = (BrAPIObject) field.getType().getDeclaredConstructor().newInstance(); List fields = Arrays.asList(brAPIObject.getClass().getDeclaredFields()); for (Field lowerField: fields) { - mapField(brAPIObject, lowerField, matchedMapping.getMapping(), importFile, rowIndex, userInput, process, validationErrors); + mapField(brAPIObject, lowerField, matchedMapping.getMapping(), importFile, rowIndex, userInput, process, validationErrors, importService); } if (brapiObjectIsEmpty(brAPIObject)) brAPIObject = null; @@ -200,7 +195,7 @@ private void mapField(Object parent, Field field, List mappings, T // Populate new object List fields = Arrays.asList(newObject.getClass().getDeclaredFields()); for (Field lowerField: fields) { - mapField(newObject, lowerField, listField.getMapping(), importFile, rowIndex, userInput, process, validationErrors); + mapField(newObject, lowerField, listField.getMapping(), importFile, rowIndex, userInput, process, validationErrors, importService); } field.setAccessible(true); @@ -270,7 +265,7 @@ private void mapField(Object parent, Field field, List mappings, T if (matchedMapping.getValue().getFileFieldName() != null) { // Check that the file has this name if (!Utilities.containsCaseInsensitive(matchedMapping.getValue().getFileFieldName(), focusRow.columnNames())) { - throw new UnprocessableEntityException(String.format(missingColumn, matchedMapping.getValue().getFileFieldName())); + throw new UnprocessableEntityException(importService.getMissingColumnMsg(matchedMapping.getValue().getFileFieldName())); } // TODO: should handle all types, not sure if better way to do this with tablesaw? @@ -288,11 +283,12 @@ else if (columnType == ColumnType.BOOLEAN) { fileValue = focusRow.getString(matchedMapping.getValue().getFileFieldName()); } - checkFieldType(type.type(), matchedMapping.getValue().getFileFieldName(), fileValue); + checkFieldType(type.type(), matchedMapping.getValue().getFileFieldName(), fileValue, importService); // Check non-null value if (required != null && fileValue.isBlank()) { - ValidationError ve = getMissingRequiredErr(matchedMapping.getValue().getFileFieldName()); + ValidationError ve = new ValidationError(matchedMapping.getValue().getFileFieldName(), + importService.getBlankRequiredFieldMsg(matchedMapping.getValue().getFileFieldName()), HttpStatus.UNPROCESSABLE_ENTITY); validationErrors.addError(getRowNumber(rowIndex), ve); } @@ -306,13 +302,14 @@ else if (columnType == ColumnType.BOOLEAN) { } } else if (matchedMapping.getValue().getConstantValue() != null){ String value = matchedMapping.getValue().getConstantValue(); - checkFieldType(type.type(), metadata.name(), value); + checkFieldType(type.type(), metadata.name(), value, importService); // Check non-null value if (required != null && value.isBlank()) { //throw new UnprocessableEntityException(String.format(blankRequiredField, metadata.name())); - ValidationError ve = getMissingRequiredErr(metadata.name()); + ValidationError ve = new ValidationError(metadata.name(), + importService.getBlankRequiredFieldMsg(metadata.name()), HttpStatus.UNPROCESSABLE_ENTITY); validationErrors.addError(getRowNumber(rowIndex), ve); } @@ -335,25 +332,22 @@ private static int getRowNumber(int row) { return row+2; } - private static ValidationError getMissingRequiredErr(String fieldName) { - return new ValidationError(fieldName, String.format(blankRequiredField, fieldName), HttpStatus.UNPROCESSABLE_ENTITY); - } - private void mapUserInputField(Object parent, Field field, Map userInput, ImportFieldType type, - ImportFieldMetadata metadata, ImportMappingRequired required) throws UnprocessableEntityException { + ImportFieldMetadata metadata, ImportMappingRequired required, + BrAPIImportService importService) throws UnprocessableEntityException { // Only supports user input at the top level of an object at the moment. No nested objects. Map String fieldId = metadata.id(); if (!userInput.containsKey(fieldId) && required != null) { - throw new UnprocessableEntityException(String.format(missingUserInput, metadata.name())); + throw new UnprocessableEntityException(importService.getMissingUserInputMsg(metadata.name())); } else if (required != null && userInput.containsKey(fieldId) && userInput.get(fieldId).toString().isBlank()) { - throw new UnprocessableEntityException(String.format(missingUserInput, metadata.name())); + throw new UnprocessableEntityException(importService.getMissingUserInputMsg(metadata.name())); } else if (userInput.containsKey(fieldId)) { String value = userInput.get(fieldId).toString(); if (!isCorrectType(type.type(), value)) { - throw new UnprocessableEntityException(String.format(wrongUserInputDataType, metadata.name(), type.type().toString().toLowerCase())); + throw new UnprocessableEntityException(importService.getWrongUserInputDataTypeMsg(metadata.name(), type.type().toString().toLowerCase())); } try { field.setAccessible(true); @@ -365,10 +359,10 @@ else if (userInput.containsKey(fieldId)) { } } - private void checkFieldType(ImportFieldTypeEnum expectedType, String column, String value) throws UnprocessableEntityException { + private void checkFieldType(ImportFieldTypeEnum expectedType, String column, String value, BrAPIImportService importService) throws UnprocessableEntityException { //TODO: Do more type checks if (!isCorrectType(expectedType, value)) { - throw new UnprocessableEntityException(String.format(wrongDataTypeMsg, column)); + throw new UnprocessableEntityException(importService.getWrongDataTypeMsg(column)); } } diff --git a/src/test/java/org/breedinginsight/brapps/importer/GermplasmFileImportTest.java b/src/test/java/org/breedinginsight/brapps/importer/GermplasmFileImportTest.java index 8a38da303..505144277 100644 --- a/src/test/java/org/breedinginsight/brapps/importer/GermplasmFileImportTest.java +++ b/src/test/java/org/breedinginsight/brapps/importer/GermplasmFileImportTest.java @@ -15,17 +15,13 @@ import lombok.SneakyThrows; import org.brapi.v2.model.core.BrAPIListTypes; import org.breedinginsight.BrAPITest; -import org.breedinginsight.api.model.v1.request.ProgramRequest; -import org.breedinginsight.api.model.v1.request.SpeciesRequest; -import org.breedinginsight.api.v1.controller.TestTokenValidator; import org.breedinginsight.brapi.v2.constants.BrAPIAdditionalInfoFields; import org.breedinginsight.brapps.importer.model.base.Germplasm; +import org.breedinginsight.brapps.importer.model.imports.germplasm.GermplasmImportService; import org.breedinginsight.brapps.importer.model.response.ImportObjectState; import org.breedinginsight.brapps.importer.services.ExternalReferenceSource; -import org.breedinginsight.brapps.importer.services.MappingManager; import org.breedinginsight.brapps.importer.services.processors.GermplasmProcessor; import org.breedinginsight.dao.db.tables.pojos.BiUserEntity; -import org.breedinginsight.dao.db.tables.pojos.BreedingMethodEntity; import org.breedinginsight.dao.db.tables.pojos.ProgramBreedingMethodEntity; import org.breedinginsight.daos.BreedingMethodDAO; import org.breedinginsight.daos.UserDAO; @@ -64,6 +60,8 @@ public class GermplasmFileImportTest extends BrAPITest { @Property(name = "brapi.server.core-url") private String BRAPI_URL; + @Inject + private GermplasmImportService importService; @Inject private SpeciesService speciesService; @Inject @@ -338,7 +336,7 @@ public void missingRequiredUserInput() { HttpResponse response = call.blockingFirst(); }); assertEquals(HttpStatus.UNPROCESSABLE_ENTITY, e.getStatus()); - assertEquals(String.format(MappingManager.missingUserInput, "List Name"), e.getMessage()); + assertEquals(importService.getMissingUserInputMsg("List Name"), e.getMessage()); } @Test @@ -503,7 +501,7 @@ public void nonNumericEntryNumbersError() { HttpResponse response = call.blockingFirst(); }); assertEquals(HttpStatus.UNPROCESSABLE_ENTITY, e.getStatus()); - assertEquals(String.format(MappingManager.wrongDataTypeMsg, "Entry No"), e.getMessage()); + assertEquals(importService.getWrongDataTypeMsg("Entry No"), e.getMessage()); } @Test @@ -534,7 +532,7 @@ public void emptyRequiredFieldsError() { JsonObject error = errors.get(0).getAsJsonObject(); assertEquals(422, error.get("httpStatusCode").getAsInt(), "Incorrect http status code"); assertEquals("Name", error.get("field").getAsString(), "Incorrect field name"); - assertEquals(String.format(MappingManager.blankRequiredField, "Name"), error.get("errorMessage").getAsString(), "Incorrect error message"); + assertEquals(importService.getBlankRequiredFieldMsg("Name"), error.get("errorMessage").getAsString(), "Incorrect error message"); JsonObject rowError2 = rowErrors.get(1).getAsJsonObject(); JsonArray errors2 = rowError2.getAsJsonArray("errors"); @@ -542,7 +540,7 @@ public void emptyRequiredFieldsError() { JsonObject error2 = errors2.get(0).getAsJsonObject(); assertEquals(422, error2.get("httpStatusCode").getAsInt(), "Incorrect http status code"); assertEquals("Source", error2.get("field").getAsString(), "Incorrect field name"); - assertEquals(String.format(MappingManager.blankRequiredField, "Source"), error2.get("errorMessage").getAsString(), "Incorrect error message"); + assertEquals(importService.getBlankRequiredFieldMsg("Source"), error2.get("errorMessage").getAsString(), "Incorrect error message"); } @Test @@ -590,7 +588,7 @@ public void missingRequiredFieldHeaderError() { HttpResponse response = call.blockingFirst(); }); assertEquals(HttpStatus.UNPROCESSABLE_ENTITY, e.getStatus()); - assertEquals(String.format(MappingManager.missingColumn, "Source"), e.getMessage()); + assertEquals(importService.getMissingColumnMsg("Source"), e.getMessage()); } @Test @@ -611,7 +609,7 @@ public void missingOptionalFieldHeaderError() { HttpResponse response = call.blockingFirst(); }); assertEquals(HttpStatus.UNPROCESSABLE_ENTITY, e.getStatus()); - assertEquals(String.format(MappingManager.missingColumn, "Entry No"), e.getMessage()); + assertEquals(importService.getMissingColumnMsg("Entry No"), e.getMessage()); } @Test From c3a53a2620a90a4e2841bdaaeb79d33ebbb0d468 Mon Sep 17 00:00:00 2001 From: mlm483 <128052931+mlm483@users.noreply.github.com> Date: Mon, 16 Oct 2023 17:23:09 -0400 Subject: [PATCH 2/3] [BI-1900] - added logging --- .../breedinginsight/brapps/importer/services/MappingManager.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/org/breedinginsight/brapps/importer/services/MappingManager.java b/src/main/java/org/breedinginsight/brapps/importer/services/MappingManager.java index e6cd869ad..c643d8c7e 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/services/MappingManager.java +++ b/src/main/java/org/breedinginsight/brapps/importer/services/MappingManager.java @@ -265,6 +265,7 @@ private void mapField(Object parent, Field field, List mappings, T if (matchedMapping.getValue().getFileFieldName() != null) { // Check that the file has this name if (!Utilities.containsCaseInsensitive(matchedMapping.getValue().getFileFieldName(), focusRow.columnNames())) { + log.debug("Expected column missing from file: " + matchedMapping.getValue().getFileFieldName()); throw new UnprocessableEntityException(importService.getMissingColumnMsg(matchedMapping.getValue().getFileFieldName())); } From 1fc3c81eeec3d7698b87203e1394b676905d61d4 Mon Sep 17 00:00:00 2001 From: mlm483 <128052931+mlm483@users.noreply.github.com> Date: Mon, 30 Oct 2023 09:56:48 -0400 Subject: [PATCH 3/3] [BI-1900] - improved naming --- .../brapps/importer/model/imports/BrAPIImportService.java | 2 +- .../brapps/importer/services/MappingManager.java | 2 +- .../brapps/importer/GermplasmFileImportTest.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/breedinginsight/brapps/importer/model/imports/BrAPIImportService.java b/src/main/java/org/breedinginsight/brapps/importer/model/imports/BrAPIImportService.java index 3a8f56303..eb4832b67 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/model/imports/BrAPIImportService.java +++ b/src/main/java/org/breedinginsight/brapps/importer/model/imports/BrAPIImportService.java @@ -33,7 +33,7 @@ public abstract class BrAPIImportService { public String getImportTypeId() {return null;} public BrAPIImport getImportClass() {return null;} - public String getWrongDataTypeMsg(String columnName) { + public String getInvalidIntegerMsg(String columnName) { return String.format("Column name \"%s\" must be integer type, but non-integer type provided.", columnName); } public String getBlankRequiredFieldMsg(String fieldName) { diff --git a/src/main/java/org/breedinginsight/brapps/importer/services/MappingManager.java b/src/main/java/org/breedinginsight/brapps/importer/services/MappingManager.java index c643d8c7e..5a3a2289f 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/services/MappingManager.java +++ b/src/main/java/org/breedinginsight/brapps/importer/services/MappingManager.java @@ -363,7 +363,7 @@ else if (userInput.containsKey(fieldId)) { private void checkFieldType(ImportFieldTypeEnum expectedType, String column, String value, BrAPIImportService importService) throws UnprocessableEntityException { //TODO: Do more type checks if (!isCorrectType(expectedType, value)) { - throw new UnprocessableEntityException(importService.getWrongDataTypeMsg(column)); + throw new UnprocessableEntityException(importService.getInvalidIntegerMsg(column)); } } diff --git a/src/test/java/org/breedinginsight/brapps/importer/GermplasmFileImportTest.java b/src/test/java/org/breedinginsight/brapps/importer/GermplasmFileImportTest.java index 505144277..0110f0d3a 100644 --- a/src/test/java/org/breedinginsight/brapps/importer/GermplasmFileImportTest.java +++ b/src/test/java/org/breedinginsight/brapps/importer/GermplasmFileImportTest.java @@ -501,7 +501,7 @@ public void nonNumericEntryNumbersError() { HttpResponse response = call.blockingFirst(); }); assertEquals(HttpStatus.UNPROCESSABLE_ENTITY, e.getStatus()); - assertEquals(importService.getWrongDataTypeMsg("Entry No"), e.getMessage()); + assertEquals(importService.getInvalidIntegerMsg("Entry No"), e.getMessage()); } @Test