From 2aca53d8723ccaa1580ad3ae467623c15de6b0aa Mon Sep 17 00:00:00 2001 From: timparsons Date: Tue, 18 Jul 2023 15:41:26 -0400 Subject: [PATCH 1/2] [BI-1842] loosening the validation of observations to not throw an error if there is an existing observation, and a new upload has a blank value for the observationUnit/Phenotype. --- .../importer/services/processors/ExperimentProcessor.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 ddea6296c..69e17c718 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 @@ -618,7 +618,8 @@ private Map fetchExistingObservations(List refe private void validateObservations(ValidationErrors validationErrors, int rowNum, ExperimentObservation importRow, List> phenotypeCols, Map colVarMap, Map existingObservations) { phenotypeCols.forEach(phenoCol -> { - if(existingObservations.containsKey(getImportObservationHash(importRow, phenoCol.name()))) { + var importHash = getImportObservationHash(importRow, phenoCol.name()); + if(existingObservations.containsKey(importHash) && StringUtils.isNotBlank(phenoCol.getString(rowNum)) && !existingObservations.get(importHash).getValue().equals(phenoCol.getString(rowNum))) { addRowError( phenoCol.name(), String.format("Value already exists for ObsUnitId: %s, Phenotype: %s", importRow.getObsUnitID(), phenoCol.name()), From efefd46050b66c37aefb2e28c45df48144884af8 Mon Sep 17 00:00:00 2001 From: timparsons Date: Thu, 20 Jul 2023 15:07:40 -0400 Subject: [PATCH 2/2] [BI-1842] Handling more cases of uploading new data when there's existing data Added more metadata to the creation of an observation to track when/who created it and define an external reference id --- .../ExperimentObservation.java | 38 ++++- .../services/ExternalReferenceSource.java | 3 +- .../processors/ExperimentProcessor.java | 21 ++- .../importer/ExperimentFileImportTest.java | 152 ++++++++++++++++++ 4 files changed, 202 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/breedinginsight/brapps/importer/model/imports/experimentObservation/ExperimentObservation.java b/src/main/java/org/breedinginsight/brapps/importer/model/imports/experimentObservation/ExperimentObservation.java index 778ee85b1..50fcdc745 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/model/imports/experimentObservation/ExperimentObservation.java +++ b/src/main/java/org/breedinginsight/brapps/importer/model/imports/experimentObservation/ExperimentObservation.java @@ -36,6 +36,7 @@ import java.math.BigInteger; import java.time.LocalDateTime; +import java.time.OffsetDateTime; import java.time.format.DateTimeFormatter; import java.util.*; import java.util.function.Supplier; @@ -303,8 +304,15 @@ public BrAPIObservation constructBrAPIObservation( String value, String variableName, String seasonDbId, - BrAPIObservationUnit obsUnit - ) { + BrAPIObservationUnit obsUnit, + boolean commit, + Program program, + User user, + String referenceSource, + UUID trialID, + UUID studyID, + UUID obsUnitID, + UUID id) { BrAPIObservation observation = new BrAPIObservation(); observation.setGermplasmName(getGermplasmName()); if (getEnv() != null) { @@ -320,11 +328,21 @@ public BrAPIObservation constructBrAPIObservation( season.setSeasonDbId(seasonDbId); observation.setSeason(season); + if(commit) { + Map createdBy = new HashMap<>(); + createdBy.put(BrAPIAdditionalInfoFields.CREATED_BY_USER_ID, user.getId()); + createdBy.put(BrAPIAdditionalInfoFields.CREATED_BY_USER_NAME, user.getName()); + observation.putAdditionalInfoItem(BrAPIAdditionalInfoFields.CREATED_BY, createdBy); + observation.putAdditionalInfoItem(BrAPIAdditionalInfoFields.CREATED_DATE, DateTimeFormatter.ISO_OFFSET_DATE_TIME.format(OffsetDateTime.now())); + + observation.setExternalReferences(getObservationExternalReferences(program, referenceSource, trialID, studyID, obsUnitID, id)); + } + return observation; } private List getBrAPIExternalReferences( - Program program, String referenceSourceBaseName, UUID trialId, UUID studyId, UUID obsUnitId) { + Program program, String referenceSourceBaseName, UUID trialId, UUID studyId, UUID obsUnitId, UUID observationId) { List refs = new ArrayList<>(); addReference(refs, program.getId(), referenceSourceBaseName, ExternalReferenceSource.PROGRAMS); @@ -337,23 +355,31 @@ private List getBrAPIExternalReferences( if (obsUnitId != null) { addReference(refs, obsUnitId, referenceSourceBaseName, ExternalReferenceSource.OBSERVATION_UNITS); } + if (observationId != null) { + addReference(refs, observationId, referenceSourceBaseName, ExternalReferenceSource.OBSERVATIONS); + } return refs; } private List getTrialExternalReferences( Program program, String referenceSourceBaseName, UUID trialId) { - return getBrAPIExternalReferences(program, referenceSourceBaseName, trialId, null, null); + return getBrAPIExternalReferences(program, referenceSourceBaseName, trialId, null, null, null); } private List getStudyExternalReferences( Program program, String referenceSourceBaseName, UUID trialId, UUID studyId) { - return getBrAPIExternalReferences(program, referenceSourceBaseName, trialId, studyId, null); + return getBrAPIExternalReferences(program, referenceSourceBaseName, trialId, studyId, null, null); } private List getObsUnitExternalReferences( Program program, String referenceSourceBaseName, UUID trialId, UUID studyId, UUID obsUnitId) { - return getBrAPIExternalReferences(program, referenceSourceBaseName, trialId, studyId, obsUnitId); + return getBrAPIExternalReferences(program, referenceSourceBaseName, trialId, studyId, obsUnitId, null); + } + + private List getObservationExternalReferences( + Program program, String referenceSourceBaseName, UUID trialId, UUID studyId, UUID obsUnitId, UUID observationId) { + return getBrAPIExternalReferences(program, referenceSourceBaseName, trialId, studyId, obsUnitId, observationId); } diff --git a/src/main/java/org/breedinginsight/brapps/importer/services/ExternalReferenceSource.java b/src/main/java/org/breedinginsight/brapps/importer/services/ExternalReferenceSource.java index aa2fd1d75..edfa45a87 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/services/ExternalReferenceSource.java +++ b/src/main/java/org/breedinginsight/brapps/importer/services/ExternalReferenceSource.java @@ -9,7 +9,8 @@ public enum ExternalReferenceSource { STUDIES("studies"), OBSERVATION_UNITS("observationunits"), DATASET("dataset"), - LISTS("lists"); + LISTS("lists"), + OBSERVATIONS("observations"); private String name; 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 69e17c718..a653eb25e 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 @@ -388,9 +388,8 @@ private void prepareDataForValidation(List importRows, List> observations = mappedImportRow.getObservations(); for (Column column : phenotypeCols) { - List> observations = mappedImportRow.getObservations(); - // if value was blank won't be entry in map for this observation observations.add(this.observationByHash.get(getImportObservationHash(importRow, getVariableNameFromColumn(column)))); } @@ -516,7 +515,7 @@ private void initNewBrapiData(List importRows, List> phen } //column.name() gets phenotype name String seasonDbId = this.yearToSeasonDbId(importRow.getEnvYear(), program.getId()); - fetchOrCreateObservationPIO(importRow, column.name(), column.getString(rowNum), dateTimeValue, commit, seasonDbId, obsUnitPIO); + fetchOrCreateObservationPIO(program, user, importRow, column.name(), column.getString(rowNum), dateTimeValue, commit, seasonDbId, obsUnitPIO); } } } @@ -625,6 +624,11 @@ private void validateObservations(ValidationErrors validationErrors, int rowNum, String.format("Value already exists for ObsUnitId: %s, Phenotype: %s", importRow.getObsUnitID(), phenoCol.name()), validationErrors, rowNum ); + } else if(existingObservations.containsKey(importHash) && (StringUtils.isBlank(phenoCol.getString(rowNum)) || existingObservations.get(importHash).getValue().equals(phenoCol.getString(rowNum)))) { + BrAPIObservation existingObs = existingObservations.get(importHash); + existingObs.setObservationVariableName(phenoCol.name()); + observationByHash.get(importHash).setState(ImportObjectState.EXISTING); + observationByHash.get(importHash).setBrAPIObject(existingObs); } else { validateObservationValue(colVarMap.get(phenoCol.name()), phenoCol.getString(rowNum), phenoCol.name(), validationErrors, rowNum); @@ -809,7 +813,9 @@ private PendingImportObject fetchOrCreateObsUnitPIO(Progra } - private PendingImportObject fetchOrCreateObservationPIO(ExperimentObservation importRow, + private PendingImportObject fetchOrCreateObservationPIO(Program program, + User user, + ExperimentObservation importRow, String variableName, String value, String timeStampValue, @@ -821,7 +827,12 @@ private PendingImportObject fetchOrCreateObservationPIO(Experi if (this.observationByHash.containsKey(key)) { pio = observationByHash.get(key); } else { - BrAPIObservation newObservation = importRow.constructBrAPIObservation(value, variableName, seasonDbId, obsUnitPIO.getBrAPIObject()); + PendingImportObject trialPIO = this.trialByNameNoScope.get(importRow.getExpTitle()); + UUID trialID = trialPIO.getId(); + PendingImportObject studyPIO = this.studyByNameNoScope.get(importRow.getEnv()); + UUID studyID = studyPIO.getId(); + UUID id = UUID.randomUUID(); + BrAPIObservation newObservation = importRow.constructBrAPIObservation(value, variableName, seasonDbId, obsUnitPIO.getBrAPIObject(), commit, program, user, BRAPI_REFERENCE_SOURCE, trialID, studyID, obsUnitPIO.getId(), id); //NOTE: Can't parse invalid timestamp value, so have to skip if invalid. // Validation error should be thrown for offending value, but that doesn't happen until later downstream if (timeStampValue != null && !timeStampValue.isBlank() && (validDateValue(timeStampValue) || validDateTimeValue(timeStampValue))) { diff --git a/src/test/java/org/breedinginsight/brapps/importer/ExperimentFileImportTest.java b/src/test/java/org/breedinginsight/brapps/importer/ExperimentFileImportTest.java index d63a0a515..f6927d03b 100644 --- a/src/test/java/org/breedinginsight/brapps/importer/ExperimentFileImportTest.java +++ b/src/test/java/org/breedinginsight/brapps/importer/ExperimentFileImportTest.java @@ -863,6 +863,158 @@ public void importSecondExpAfterFirstExpWithObs() { assertRowSaved(newExpB, program, traits); } + /* + Scenario: + - an experiment was created with observations + - do a second upload with additional observations for the experiment + - verify the second set of observations get uploaded successfully + */ + @ParameterizedTest + @ValueSource(booleans = {true, false}) + @SneakyThrows + public void importNewObsAfterFirstExpWithObs(boolean commit) { + List traits = createTraits(2); + Program program = createProgram("Exp with additional Uploads "+(commit ? "C" : "P"), "EXAU"+(commit ? "C" : "P"), "EXAU"+(commit ? "C" : "P"), BRAPI_REFERENCE_SOURCE, createGermplasm(1), traits); + Map newExp = new HashMap<>(); + newExp.put(Columns.GERMPLASM_GID, "1"); + newExp.put(Columns.TEST_CHECK, "T"); + newExp.put(Columns.EXP_TITLE, "Test Exp"); + newExp.put(Columns.EXP_UNIT, "Plot"); + newExp.put(Columns.EXP_TYPE, "Phenotyping"); + newExp.put(Columns.ENV, "New Env"); + newExp.put(Columns.ENV_LOCATION, "Location A"); + newExp.put(Columns.ENV_YEAR, "2023"); + newExp.put(Columns.EXP_UNIT_ID, "a-1"); + newExp.put(Columns.REP_NUM, "1"); + newExp.put(Columns.BLOCK_NUM, "1"); + newExp.put(Columns.ROW, "1"); + newExp.put(Columns.COLUMN, "1"); + newExp.put(traits.get(0).getObservationVariableName(), "1"); + + importTestUtils.uploadAndFetch(writeDataToFile(List.of(newExp), traits), null, true, client, program, mappingId); + + BrAPITrial brAPITrial = brAPITrialDAO.getTrialsByName(List.of(Utilities.appendProgramKey((String)newExp.get(Columns.EXP_TITLE), program.getKey())), program).get(0); + Optional trialIdXref = Utilities.getExternalReference(brAPITrial.getExternalReferences(), String.format("%s/%s", BRAPI_REFERENCE_SOURCE, ExternalReferenceSource.TRIALS.getName())); + assertTrue(trialIdXref.isPresent()); + BrAPIStudy brAPIStudy = brAPIStudyDAO.getStudiesByExperimentID(UUID.fromString(trialIdXref.get().getReferenceID()), program).get(0); + + BrAPIObservationUnit ou = ouDAO.getObservationUnitsForStudyDbId(brAPIStudy.getStudyDbId(), program).get(0); + Optional ouIdXref = Utilities.getExternalReference(ou.getExternalReferences(), String.format("%s/%s", BRAPI_REFERENCE_SOURCE, ExternalReferenceSource.OBSERVATION_UNITS.getName())); + assertTrue(ouIdXref.isPresent()); + + Map newObservation = new HashMap<>(); + newObservation.put(Columns.GERMPLASM_GID, "1"); + newObservation.put(Columns.TEST_CHECK, "T"); + newObservation.put(Columns.EXP_TITLE, "Test Exp"); + newObservation.put(Columns.EXP_UNIT, "Plot"); + newObservation.put(Columns.EXP_TYPE, "Phenotyping"); + newObservation.put(Columns.ENV, "New Env"); + newObservation.put(Columns.ENV_LOCATION, "Location A"); + newObservation.put(Columns.ENV_YEAR, "2023"); + newObservation.put(Columns.EXP_UNIT_ID, "a-1"); + newObservation.put(Columns.REP_NUM, "1"); + newObservation.put(Columns.BLOCK_NUM, "1"); + newObservation.put(Columns.ROW, "1"); + newObservation.put(Columns.COLUMN, "1"); + newObservation.put(Columns.OBS_UNIT_ID, ouIdXref.get().getReferenceID()); + newObservation.put(traits.get(0).getObservationVariableName(), "1"); + newObservation.put(traits.get(1).getObservationVariableName(), "2"); + + JsonObject result = importTestUtils.uploadAndFetch(writeDataToFile(List.of(newObservation), traits), null, commit, client, program, mappingId); + + JsonArray previewRows = result.get("preview").getAsJsonObject().get("rows").getAsJsonArray(); + assertEquals(1, previewRows.size()); + JsonObject row = previewRows.get(0).getAsJsonObject(); + + assertEquals("EXISTING", row.getAsJsonObject("trial").get("state").getAsString()); + assertEquals("EXISTING", row.getAsJsonObject("location").get("state").getAsString()); + assertEquals("EXISTING", row.getAsJsonObject("study").get("state").getAsString()); + assertEquals("EXISTING", row.getAsJsonObject("observationUnit").get("state").getAsString()); + if(commit) { + assertRowSaved(newObservation, program, traits); + } else { + assertValidPreviewRow(newObservation, row, program, traits); + } + } + + /* + Scenario: + - an experiment was created with observations + - do a second upload with additional observations for the experiment. Make sure the original observations are blank values + - verify the second set of observations get uploaded successfully + */ + @ParameterizedTest + @ValueSource(booleans = {true, false}) + @SneakyThrows + public void importNewObsAfterFirstExpWithObs_blank(boolean commit) { + List traits = createTraits(2); + Program program = createProgram("Exp with additional Uploads (blank) "+(commit ? "C" : "P"), "EXAUB"+(commit ? "C" : "P"), "EXAUB"+(commit ? "C" : "P"), BRAPI_REFERENCE_SOURCE, createGermplasm(1), traits); + Map newExp = new HashMap<>(); + newExp.put(Columns.GERMPLASM_GID, "1"); + newExp.put(Columns.TEST_CHECK, "T"); + newExp.put(Columns.EXP_TITLE, "Test Exp"); + newExp.put(Columns.EXP_UNIT, "Plot"); + newExp.put(Columns.EXP_TYPE, "Phenotyping"); + newExp.put(Columns.ENV, "New Env"); + newExp.put(Columns.ENV_LOCATION, "Location A"); + newExp.put(Columns.ENV_YEAR, "2023"); + newExp.put(Columns.EXP_UNIT_ID, "a-1"); + newExp.put(Columns.REP_NUM, "1"); + newExp.put(Columns.BLOCK_NUM, "1"); + newExp.put(Columns.ROW, "1"); + newExp.put(Columns.COLUMN, "1"); + newExp.put(traits.get(0).getObservationVariableName(), "1"); + + importTestUtils.uploadAndFetch(writeDataToFile(List.of(newExp), traits), null, true, client, program, mappingId); + + BrAPITrial brAPITrial = brAPITrialDAO.getTrialsByName(List.of(Utilities.appendProgramKey((String)newExp.get(Columns.EXP_TITLE), program.getKey())), program).get(0); + Optional trialIdXref = Utilities.getExternalReference(brAPITrial.getExternalReferences(), String.format("%s/%s", BRAPI_REFERENCE_SOURCE, ExternalReferenceSource.TRIALS.getName())); + assertTrue(trialIdXref.isPresent()); + BrAPIStudy brAPIStudy = brAPIStudyDAO.getStudiesByExperimentID(UUID.fromString(trialIdXref.get().getReferenceID()), program).get(0); + + BrAPIObservationUnit ou = ouDAO.getObservationUnitsForStudyDbId(brAPIStudy.getStudyDbId(), program).get(0); + Optional ouIdXref = Utilities.getExternalReference(ou.getExternalReferences(), String.format("%s/%s", BRAPI_REFERENCE_SOURCE, ExternalReferenceSource.OBSERVATION_UNITS.getName())); + assertTrue(ouIdXref.isPresent()); + + assertRowSaved(newExp, program, traits); + + Map newObservation = new HashMap<>(); + newObservation.put(Columns.GERMPLASM_GID, "1"); + newObservation.put(Columns.TEST_CHECK, "T"); + newObservation.put(Columns.EXP_TITLE, "Test Exp"); + newObservation.put(Columns.EXP_UNIT, "Plot"); + newObservation.put(Columns.EXP_TYPE, "Phenotyping"); + newObservation.put(Columns.ENV, "New Env"); + newObservation.put(Columns.ENV_LOCATION, "Location A"); + newObservation.put(Columns.ENV_YEAR, "2023"); + newObservation.put(Columns.EXP_UNIT_ID, "a-1"); + newObservation.put(Columns.REP_NUM, "1"); + newObservation.put(Columns.BLOCK_NUM, "1"); + newObservation.put(Columns.ROW, "1"); + newObservation.put(Columns.COLUMN, "1"); + newObservation.put(Columns.OBS_UNIT_ID, ouIdXref.get().getReferenceID()); + newObservation.put(traits.get(0).getObservationVariableName(), ""); + newObservation.put(traits.get(1).getObservationVariableName(), "2"); + + JsonObject result = importTestUtils.uploadAndFetch(writeDataToFile(List.of(newObservation), traits), null, commit, client, program, mappingId); + + JsonArray previewRows = result.get("preview").getAsJsonObject().get("rows").getAsJsonArray(); + assertEquals(1, previewRows.size()); + JsonObject row = previewRows.get(0).getAsJsonObject(); + + assertEquals("EXISTING", row.getAsJsonObject("trial").get("state").getAsString()); + assertEquals("EXISTING", row.getAsJsonObject("location").get("state").getAsString()); + assertEquals("EXISTING", row.getAsJsonObject("study").get("state").getAsString()); + assertEquals("EXISTING", row.getAsJsonObject("observationUnit").get("state").getAsString()); + + newObservation.put(traits.get(0).getObservationVariableName(), "1"); + if(commit) { + assertRowSaved(newObservation, program, traits); + } else { + assertValidPreviewRow(newObservation, row, program, traits); + } + } + private Map assertRowSaved(Map expected, Program program, List traits) throws ApiException { Map ret = new HashMap<>();