diff --git a/pom.xml b/pom.xml
index 79a4d83d3..53c76ed6e 100644
--- a/pom.xml
+++ b/pom.xml
@@ -289,6 +289,11 @@
junit-jupiter-engine
test
+
+ org.junit.jupiter
+ junit-jupiter-params
+ test
+
io.micronaut.test
micronaut-test-junit5
diff --git a/src/main/java/org/breedinginsight/brapps/importer/services/processors/ExperimentProcessor.java b/src/main/java/org/breedinginsight/brapps/importer/services/processors/ExperimentProcessor.java
index c18bbc3b3..d85b80329 100644
--- a/src/main/java/org/breedinginsight/brapps/importer/services/processors/ExperimentProcessor.java
+++ b/src/main/java/org/breedinginsight/brapps/importer/services/processors/ExperimentProcessor.java
@@ -91,6 +91,8 @@ public class ExperimentProcessor implements Processor {
private static final String BLANK_FIELD_EXPERIMENT = "Field is blank when creating a new experiment";
private static final String BLANK_FIELD_ENV = "Field is blank when creating a new environment";
private static final String BLANK_FIELD_OBS = "Field is blank when creating new observations";
+ private static final String ENV_LOCATION_MISMATCH = "All locations must be the same for a given environment";
+ private static final String ENV_YEAR_MISMATCH = "All years must be the same for a given environment";
@Property(name = "brapi.server.reference-source")
private String BRAPI_REFERENCE_SOURCE;
@@ -228,7 +230,7 @@ public Map process(
prepareDataForValidation(importRows, phenotypeCols, mappedBrAPIImport);
- validateFields(importRows, validationErrors, mappedBrAPIImport, referencedTraits, program, phenotypeCols);
+ validateFields(importRows, validationErrors, mappedBrAPIImport, referencedTraits, program, phenotypeCols, commit);
if (validationErrors.hasErrors()) {
throw new ValidatorException(validationErrors);
@@ -540,7 +542,7 @@ private String getObservationHash(String observationUnitName, String variableNam
}
private ValidationErrors validateFields(List importRows, ValidationErrors validationErrors, Map mappedBrAPIImport, List referencedTraits, Program program,
- List> phenotypeCols) throws MissingRequiredInfoException, ApiException {
+ List> phenotypeCols, boolean commit) throws MissingRequiredInfoException, ApiException {
//fetching any existing observations for any OUs in the import
Map existingObsByObsHash = fetchExistingObservations(referencedTraits, program);
Map colVarMap = referencedTraits.stream().collect(Collectors.toMap(Trait::getObservationVariableName, Function.identity()));
@@ -560,7 +562,7 @@ private ValidationErrors validateFields(List importRows, Validation
throw new MissingRequiredInfoException(MISSING_OBS_UNIT_ID_ERROR);
}
- validateConditionallyRequired(validationErrors, rowNum, importRow);
+ validateConditionallyRequired(validationErrors, rowNum, importRow, program, commit);
validateObservationUnits(validationErrors, uniqueStudyAndObsUnit, rowNum, importRow);
validateObservations(validationErrors, rowNum, importRow, phenotypeCols, colVarMap, existingObsByObsHash);
}
@@ -659,7 +661,7 @@ private void validateUniqueObsUnits(
}
}
- private void validateConditionallyRequired(ValidationErrors validationErrors, int rowNum, ExperimentObservation importRow) {
+ private void validateConditionallyRequired(ValidationErrors validationErrors, int rowNum, ExperimentObservation importRow, Program program, boolean commit) {
ImportObjectState expState = this.trialByNameNoScope.get(importRow.getExpTitle())
.getState();
ImportObjectState envState = this.studyByNameNoScope.get(importRow.getEnv()).getState();
@@ -677,8 +679,21 @@ private void validateConditionallyRequired(ValidationErrors validationErrors, in
validateRequiredCell(importRow.getExpUnit(), Columns.EXP_UNIT, errorMessage, validationErrors, rowNum);
validateRequiredCell(importRow.getExpType(), Columns.EXP_TYPE, errorMessage, validationErrors, rowNum);
validateRequiredCell(importRow.getEnv(), Columns.ENV, errorMessage, validationErrors, rowNum);
- validateRequiredCell(importRow.getEnvLocation(), Columns.ENV_LOCATION, errorMessage, validationErrors, rowNum);
- validateRequiredCell(importRow.getEnvYear(), Columns.ENV_YEAR, errorMessage, validationErrors, rowNum);
+ if(validateRequiredCell(importRow.getEnvLocation(), Columns.ENV_LOCATION, errorMessage, validationErrors, rowNum)) {
+ if(!Utilities.removeProgramKeyAndUnknownAdditionalData(this.studyByNameNoScope.get(importRow.getEnv()).getBrAPIObject().getLocationName(), program.getKey()).equals(importRow.getEnvLocation())) {
+ addRowError(Columns.ENV_LOCATION, ENV_LOCATION_MISMATCH, validationErrors, rowNum);
+ }
+ }
+ if(validateRequiredCell(importRow.getEnvYear(), Columns.ENV_YEAR, errorMessage, validationErrors, rowNum)) {
+ String studyYear = this.studyByNameNoScope.get(importRow.getEnv()).getBrAPIObject().getSeasons().get(0);
+ String rowYear = importRow.getEnvYear();
+ if(commit) {
+ rowYear = this.yearToSeasonDbId(importRow.getEnvYear(), program.getId());
+ }
+ if(!studyYear.equals(rowYear)) {
+ addRowError(Columns.ENV_YEAR, ENV_YEAR_MISMATCH, validationErrors, rowNum);
+ }
+ }
validateRequiredCell(importRow.getExpUnitId(), Columns.EXP_UNIT_ID, errorMessage, validationErrors, rowNum);
validateRequiredCell(importRow.getExpReplicateNo(), Columns.REP_NUM, errorMessage, validationErrors, rowNum);
validateRequiredCell(importRow.getExpBlockNo(), Columns.BLOCK_NUM, errorMessage, validationErrors, rowNum);
@@ -691,10 +706,12 @@ private void validateConditionallyRequired(ValidationErrors validationErrors, in
}
}
- private void validateRequiredCell(String value, String columnHeader, String errorMessage, ValidationErrors validationErrors, int rowNum) {
+ private boolean validateRequiredCell(String value, String columnHeader, String errorMessage, ValidationErrors validationErrors, int rowNum) {
if (StringUtils.isBlank(value)) {
addRowError(columnHeader, errorMessage, validationErrors, rowNum);
+ return false;
}
+ return true;
}
private void addRowError(String field, String errorMessage, ValidationErrors validationErrors, int rowNum) {
@@ -1091,15 +1108,20 @@ private Map> initializeObserva
String refSource = String.format("%s/%s", BRAPI_REFERENCE_SOURCE, ExternalReferenceSource.OBSERVATION_UNITS.getName());
if (existingObsUnits.size() == rowByObsUnitId.size()) {
existingObsUnits.forEach(brAPIObservationUnit -> {
+ processAndCacheObservationUnit(brAPIObservationUnit, refSource, program, ret, rowByObsUnitId);
+
BrAPIExternalReference idRef = Utilities.getExternalReference(brAPIObservationUnit.getExternalReferences(), refSource)
.orElseThrow(() -> new InternalServerException("An ObservationUnit ID was not found in any of the external references"));
+
ExperimentObservation row = rowByObsUnitId.get(idRef.getReferenceID());
- row.setExpUnitId(Utilities.removeProgramKeyAndUnknownAdditionalData(brAPIObservationUnit.getObservationUnitName(), program.getKey()));
- ret.put(createObservationUnitKey(row),
- new PendingImportObject<>(ImportObjectState.EXISTING,
- brAPIObservationUnit,
- UUID.fromString(idRef.getReferenceID())));
+ row.setExpTitle(Utilities.removeProgramKey(brAPIObservationUnit.getTrialName(), program.getKey()));
+ row.setEnv(Utilities.removeProgramKeyAndUnknownAdditionalData(brAPIObservationUnit.getStudyName(), program.getKey()));
+ row.setEnvLocation(Utilities.removeProgramKey(brAPIObservationUnit.getLocationName(), program.getKey()));
});
+ } else {
+ List missingIds = new ArrayList<>(rowByObsUnitId.keySet());
+ missingIds.removeAll(existingObsUnits.stream().map(BrAPIObservationUnit::getObservationUnitDbId).collect(Collectors.toList()));
+ throw new IllegalStateException("Observation Units not found for ObsUnitId(s): " + String.join(COMMA_DELIMITER, missingIds));
}
return ret;
@@ -1138,6 +1160,13 @@ private Map> initializeStudyByNameNoScop
return studyByName;
}
+ try {
+ initializeStudiesForExistingObservationUnits(program, studyByName);
+ } catch (ApiException e) {
+ log.error("Error fetching studies: " + Utilities.generateApiExceptionLogMessage(e), e);
+ throw new InternalServerException(e.toString(), e);
+ }
+
List existingStudies;
Optional> trial = getTrialPIO(experimentImportRows);
@@ -1156,6 +1185,29 @@ private Map> initializeStudyByNameNoScop
return studyByName;
}
+ private void initializeStudiesForExistingObservationUnits(Program program, Map> studyByName) throws ApiException {
+ Set studyDbIds = this.observationUnitByNameNoScope.values()
+ .stream()
+ .map(pio -> pio.getBrAPIObject()
+ .getStudyDbId())
+ .collect(Collectors.toSet());
+
+ List studies = fetchStudiesByDbId(studyDbIds, program);
+ studies.forEach(study -> {
+ processAndCacheStudy(study, program, studyByName);
+ });
+ }
+
+ private List fetchStudiesByDbId(Set studyDbIds, Program program) throws ApiException {
+ List studies = brAPIStudyDAO.getStudiesByStudyDbId(studyDbIds, program);
+ if(studies.size() != studyDbIds.size()) {
+ List missingIds = new ArrayList<>(studyDbIds);
+ missingIds.removeAll(studies.stream().map(BrAPIStudy::getStudyDbId).collect(Collectors.toList()));
+ throw new IllegalStateException("Study not found for studyDbId(s): " + String.join(COMMA_DELIMITER, missingIds));
+ }
+ return studies;
+ }
+
private Map> initializeUniqueLocationNames(Program program, List experimentImportRows) {
Map> locationByName = new HashMap<>();
@@ -1279,17 +1331,24 @@ private Map> initializeExistingGermp
return existingGermplasmByGID;
}
- private void processAndCacheStudy(BrAPIStudy existingStudy, Program program, Map> studyByName) {
- //Swap season DbId with year String
- String seasonId = existingStudy.getSeasons().get(0);
- existingStudy.setSeasons(List.of(this.seasonDbIdToYear(seasonId, program.getId())));
+ private void processAndCacheObservationUnit(BrAPIObservationUnit brAPIObservationUnit, String refSource, Program program, Map> observationUnitByName,
+ Map rowByObsUnitId) {
+ BrAPIExternalReference idRef = Utilities.getExternalReference(brAPIObservationUnit.getExternalReferences(), refSource)
+ .orElseThrow(() -> new InternalServerException("An ObservationUnit ID was not found in any of the external references"));
- existingStudy.setStudyName(Utilities.removeProgramKeyAndUnknownAdditionalData(existingStudy.getStudyName(), program.getKey()));
+ ExperimentObservation row = rowByObsUnitId.get(idRef.getReferenceID());
+ row.setExpUnitId(Utilities.removeProgramKeyAndUnknownAdditionalData(brAPIObservationUnit.getObservationUnitName(), program.getKey()));
+ observationUnitByName.put(createObservationUnitKey(row),
+ new PendingImportObject<>(ImportObjectState.EXISTING,
+ brAPIObservationUnit,
+ UUID.fromString(idRef.getReferenceID())));
+ }
+ private void processAndCacheStudy(BrAPIStudy existingStudy, Program program, Map> studyByName) {
BrAPIExternalReference xref = Utilities.getExternalReference(existingStudy.getExternalReferences(), String.format("%s/%s", BRAPI_REFERENCE_SOURCE, ExternalReferenceSource.STUDIES.getName()))
.orElseThrow(() -> new IllegalStateException("External references wasn't found for study (dbid): " + existingStudy.getStudyDbId()));
studyByName.put(
- existingStudy.getStudyName(),
+ Utilities.removeProgramKeyAndUnknownAdditionalData(existingStudy.getStudyName(), program.getKey()),
new PendingImportObject<>(ImportObjectState.EXISTING, existingStudy, UUID.fromString(xref.getReferenceID())));
}
@@ -1341,12 +1400,7 @@ private void initializeTrialsForExistingObservationUnits(Program program, Map fetchTrialDbidsForStudies(Set studyDbIds, Program program) throws ApiException {
Set trialDbIds = new HashSet<>();
- List studies = brAPIStudyDAO.getStudiesByStudyDbId(studyDbIds, program);
- if(studies.size() != studyDbIds.size()) {
- List missingIds = new ArrayList<>(trialDbIds);
- missingIds.removeAll(studies.stream().map(BrAPIStudy::getStudyDbId).collect(Collectors.toList()));
- throw new IllegalStateException("Study not found for studyDbId(s): " + String.join(COMMA_DELIMITER, missingIds));
- }
+ List studies = fetchStudiesByDbId(studyDbIds, program);
studies.forEach(study -> {
if (StringUtils.isBlank(study.getTrialDbId())) {
throw new IllegalStateException("TrialDbId is not set for an existing Study: " + study.getStudyDbId());
diff --git a/src/main/java/org/breedinginsight/utilities/Utilities.java b/src/main/java/org/breedinginsight/utilities/Utilities.java
index 5384a8241..59a4be0e9 100644
--- a/src/main/java/org/breedinginsight/utilities/Utilities.java
+++ b/src/main/java/org/breedinginsight/utilities/Utilities.java
@@ -80,13 +80,13 @@ public static String appendProgramKey( String original, String programKey ){
* @return
*/
public static String removeProgramKey(String original, String programKey, String additionalKeyData) {
+ String keyValue;
if(StringUtils.isNotBlank(additionalKeyData)) {
- String keyValue = String.format(" [%s-%s]", programKey, additionalKeyData);
- return original.replace(keyValue, "");
+ keyValue = String.format(" [%s-%s]", programKey, additionalKeyData);
} else {
- String keyValue = String.format(" [%s]", programKey);
- return original.replace(keyValue, "");
+ keyValue = String.format(" [%s]", programKey);
}
+ return original.replace(keyValue, "");
}
/**
diff --git a/src/test/java/org/breedinginsight/brapps/importer/ExperimentFileImportTest.java b/src/test/java/org/breedinginsight/brapps/importer/ExperimentFileImportTest.java
index 6cd02e483..d63a0a515 100644
--- a/src/test/java/org/breedinginsight/brapps/importer/ExperimentFileImportTest.java
+++ b/src/test/java/org/breedinginsight/brapps/importer/ExperimentFileImportTest.java
@@ -68,7 +68,10 @@
import org.breedinginsight.utilities.Utilities;
import org.jooq.DSLContext;
import org.junit.jupiter.api.*;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
import org.junit.platform.commons.util.StringUtils;
+import org.opentest4j.AssertionFailedError;
import javax.inject.Inject;
import java.io.ByteArrayOutputStream;
@@ -77,6 +80,8 @@
import java.io.IOException;
import java.time.OffsetDateTime;
import java.util.*;
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsInAnyOrder;
@@ -329,10 +334,11 @@ public void importNewEnvExistingExpNoObsSuccess() {
assertRowSaved(newEnv, program, null);
}
- @Test
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
@SneakyThrows
- public void verifyMissingDataThrowsError() {
- Program program = createProgram("Missing Req Cols", "MISS", "MISS", BRAPI_REFERENCE_SOURCE, createGermplasm(1), null);
+ public void verifyMissingDataThrowsError(boolean commit) {
+ Program program = createProgram("Missing Req Cols "+(commit ? "C" : "P"), "MISS"+(commit ? "C" : "P"), "MISS"+(commit ? "C" : "P"), BRAPI_REFERENCE_SOURCE, createGermplasm(1), null);
Map base = new HashMap<>();
base.put(Columns.GERMPLASM_GID, "1");
@@ -349,43 +355,43 @@ public void verifyMissingDataThrowsError() {
Map noGID = new HashMap<>(base);
noGID.remove(Columns.GERMPLASM_GID);
- uploadAndVerifyFailure(program, writeDataToFile(List.of(noGID), null), Columns.GERMPLASM_GID);
+ uploadAndVerifyFailure(program, writeDataToFile(List.of(noGID), null), Columns.GERMPLASM_GID, commit);
Map noExpTitle = new HashMap<>(base);
noExpTitle.remove(Columns.EXP_TITLE);
- uploadAndVerifyFailure(program, writeDataToFile(List.of(noExpTitle), null), Columns.EXP_TITLE);
+ uploadAndVerifyFailure(program, writeDataToFile(List.of(noExpTitle), null), Columns.EXP_TITLE, commit);
Map noExpUnit = new HashMap<>(base);
noExpUnit.remove(Columns.EXP_UNIT);
- uploadAndVerifyFailure(program, writeDataToFile(List.of(noExpUnit), null), Columns.EXP_UNIT);
+ uploadAndVerifyFailure(program, writeDataToFile(List.of(noExpUnit), null), Columns.EXP_UNIT, commit);
Map noExpType = new HashMap<>(base);
noExpType.remove(Columns.EXP_TYPE);
- uploadAndVerifyFailure(program, writeDataToFile(List.of(noExpType), null), Columns.EXP_TYPE);
+ uploadAndVerifyFailure(program, writeDataToFile(List.of(noExpType), null), Columns.EXP_TYPE, commit);
Map noEnv = new HashMap<>(base);
noEnv.remove(Columns.ENV);
- uploadAndVerifyFailure(program, writeDataToFile(List.of(noEnv), null), Columns.ENV);
+ uploadAndVerifyFailure(program, writeDataToFile(List.of(noEnv), null), Columns.ENV, commit);
Map noEnvLoc = new HashMap<>(base);
noEnvLoc.remove(Columns.ENV_LOCATION);
- uploadAndVerifyFailure(program, writeDataToFile(List.of(noEnvLoc), null), Columns.ENV_LOCATION);
+ uploadAndVerifyFailure(program, writeDataToFile(List.of(noEnvLoc), null), Columns.ENV_LOCATION, commit);
Map noExpUnitId = new HashMap<>(base);
noExpUnitId.remove(Columns.EXP_UNIT_ID);
- uploadAndVerifyFailure(program, writeDataToFile(List.of(noExpUnitId), null), Columns.EXP_UNIT_ID);
+ uploadAndVerifyFailure(program, writeDataToFile(List.of(noExpUnitId), null), Columns.EXP_UNIT_ID, commit);
Map noExpRep = new HashMap<>(base);
noExpRep.remove(Columns.REP_NUM);
- uploadAndVerifyFailure(program, writeDataToFile(List.of(noExpRep), null), Columns.REP_NUM);
+ uploadAndVerifyFailure(program, writeDataToFile(List.of(noExpRep), null), Columns.REP_NUM, commit);
Map noExpBlock = new HashMap<>(base);
noExpBlock.remove(Columns.BLOCK_NUM);
- uploadAndVerifyFailure(program, writeDataToFile(List.of(noExpBlock), null), Columns.BLOCK_NUM);
+ uploadAndVerifyFailure(program, writeDataToFile(List.of(noExpBlock), null), Columns.BLOCK_NUM, commit);
Map noEnvYear = new HashMap<>(base);
noEnvYear.remove(Columns.ENV_YEAR);
- uploadAndVerifyFailure(program, writeDataToFile(List.of(noEnvYear), null), Columns.ENV_YEAR);
+ uploadAndVerifyFailure(program, writeDataToFile(List.of(noEnvYear), null), Columns.ENV_YEAR, commit);
}
@Test
@@ -424,11 +430,88 @@ public void importNewExpWithObsVar() {
assertRowSaved(newExp, program, traits);
}
- @Test
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ @SneakyThrows
+ public void verifyDiffYearSameEnvThrowsError(boolean commit) {
+ Program program = createProgram("Diff Years "+(commit ? "C" : "P"), "YEARS"+(commit ? "C" : "P"), "YEARS"+(commit ? "C" : "P"), BRAPI_REFERENCE_SOURCE, createGermplasm(2), null);
+
+ List