Skip to content

BI-1900 - Update Validation Error Messages #302

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mlm483 merged 3 commits into from
Oct 30, 2023
Merged

BI-1900 - Update Validation Error Messages #302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
38c6910
[BI-1900] - updated validation error messages
mlm483 Oct 13, 2023
c3a53a2
[BI-1900] - added logging
mlm483 Oct 16, 2023
1fc3c81
[BI-1900] - improved naming
mlm483 Oct 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/main/java/org/breedinginsight/brapps/importer/model/imports/BrAPIImportService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,21 @@
public abstract class BrAPIImportService {
public String getImportTypeId() {return null;}
public BrAPIImport getImportClass() {return null;}
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) {
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<BrAPIImport> brAPIImports, Table data, Program program, ImportUpload upload, User user, Boolean commit)
throws UnprocessableEntityException, DoesNotExistException, ValidatorException, ApiException, MissingRequiredInfoException {return null;}
}
5 changes: 5 additions & 0 deletions ...ginsight/brapps/importer/model/imports/experimentObservation/ExperimentImportService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<BrAPIImport> brAPIImports, Table data, Program program, ImportUpload upload, User user, Boolean commit)
throws UnprocessableEntityException, ValidatorException, ApiException, MissingRequiredInfoException {
Expand Down
47 changes: 21 additions & 26 deletions src/main/java/org/breedinginsight/brapps/importer/services/MappingManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -96,7 +90,7 @@ private List<BrAPIImport> map(ImportMapping importMapping, Table data, Map<Strin
// Run through the brapi fields and look for a match
Field[] fields = brAPIImport.getClass().getDeclaredFields();
for (Field field: fields) {
mapField(brAPIImport, field, importMapping.getMappingConfig(), data, rowIndex, userInput, process, validationErrors);
mapField(brAPIImport, field, importMapping.getMappingConfig(), data, rowIndex, userInput, process, validationErrors, importService);
}

brAPIImports.add(brAPIImport);
Expand All @@ -110,7 +104,8 @@ private List<BrAPIImport> map(ImportMapping importMapping, Table data, Map<Strin
}

private void mapField(Object parent, Field field, List<MappingField> mappings, Table importFile, Integer rowIndex,
Map<String, Object> userInput, Boolean process, ValidationErrors validationErrors) throws UnprocessableEntityException {
Map<String, Object> userInput, Boolean process, ValidationErrors validationErrors,
BrAPIImportService importService) throws UnprocessableEntityException {

Row focusRow = importFile.row(rowIndex);
// Process this field
Expand All @@ -131,7 +126,7 @@ private void mapField(Object parent, Field field, List<MappingField> 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;
}
Expand Down Expand Up @@ -167,7 +162,7 @@ private void mapField(Object parent, Field field, List<MappingField> mappings, T
brAPIObject = (BrAPIObject) field.getType().getDeclaredConstructor().newInstance();
List<Field> 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;

Expand Down Expand Up @@ -200,7 +195,7 @@ private void mapField(Object parent, Field field, List<MappingField> mappings, T
// Populate new object
List<Field> 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);
Expand Down Expand Up @@ -270,7 +265,8 @@ private void mapField(Object parent, Field field, List<MappingField> 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()));
log.debug("Expected column missing from file: " + 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?
Expand All @@ -288,11 +284,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);
}

Expand All @@ -306,13 +303,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);
}

Expand All @@ -335,25 +333,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<String, Object> 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, String>
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);
Expand All @@ -365,10 +360,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.getInvalidIntegerMsg(column));
}
}

Expand Down
20 changes: 9 additions & 11 deletions src/test/java/org/breedinginsight/brapps/importer/GermplasmFileImportTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -338,7 +336,7 @@ public void missingRequiredUserInput() {
HttpResponse<String> 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
Expand Down Expand Up @@ -503,7 +501,7 @@ public void nonNumericEntryNumbersError() {
HttpResponse<String> response = call.blockingFirst();
});
assertEquals(HttpStatus.UNPROCESSABLE_ENTITY, e.getStatus());
assertEquals(String.format(MappingManager.wrongDataTypeMsg, "Entry No"), e.getMessage());
assertEquals(importService.getInvalidIntegerMsg("Entry No"), e.getMessage());
}

@Test
Expand Down Expand Up @@ -534,15 +532,15 @@ 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");
assertEquals(1, errors2.size(), "Not enough errors were returned");
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
Expand Down Expand Up @@ -590,7 +588,7 @@ public void missingRequiredFieldHeaderError() {
HttpResponse<String> response = call.blockingFirst();
});
assertEquals(HttpStatus.UNPROCESSABLE_ENTITY, e.getStatus());
assertEquals(String.format(MappingManager.missingColumn, "Source"), e.getMessage());
assertEquals(importService.getMissingColumnMsg("Source"), e.getMessage());
}

@Test
Expand All @@ -611,7 +609,7 @@ public void missingOptionalFieldHeaderError() {
HttpResponse<String> 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
Expand Down