From fc23e97cf5b5ae672568681b8514b7c6c030bbed Mon Sep 17 00:00:00 2001 From: Nick <53413353+nickpalladino@users.noreply.github.com> Date: Wed, 2 Aug 2023 09:54:01 -0400 Subject: [PATCH 01/15] Updated logic to allow empty pedigrees and only require gids for updates --- .../brapps/importer/model/base/Germplasm.java | 40 ++++++++++++------- .../processors/GermplasmProcessor.java | 26 ++++++++---- 2 files changed, 44 insertions(+), 22 deletions(-) diff --git a/src/main/java/org/breedinginsight/brapps/importer/model/base/Germplasm.java b/src/main/java/org/breedinginsight/brapps/importer/model/base/Germplasm.java index 725bb9e44..b8b63c1fb 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/model/base/Germplasm.java +++ b/src/main/java/org/breedinginsight/brapps/importer/model/base/Germplasm.java @@ -22,6 +22,7 @@ import lombok.Getter; import lombok.NoArgsConstructor; import lombok.Setter; +import org.apache.commons.lang3.StringUtils; import org.brapi.v2.model.BrAPIExternalReference; import org.brapi.v2.model.core.BrAPIListTypes; import org.brapi.v2.model.core.request.BrAPIListNewRequest; @@ -156,12 +157,22 @@ public static String constructGermplasmListName(String listName, Program program return String.format("%s [%s-germplasm]", listName, program.getKey()); } - public void updateBrAPIGermplasm(BrAPIGermplasm germplasm, Program program, UUID listId, boolean commit) { + public void updateBrAPIGermplasm(BrAPIGermplasm germplasm, Program program, UUID listId, boolean commit, boolean updatePedigree) { - germplasm.putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_GID, getFemaleParentDBID()); - germplasm.putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_GID, getMaleParentDBID()); - germplasm.putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_ENTRY_NO, getFemaleParentEntryNo()); - germplasm.putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_ENTRY_NO, getMaleParentEntryNo()); + if (updatePedigree) { + if (!StringUtils.isBlank(getFemaleParentDBID())) { + germplasm.putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_GID, getFemaleParentDBID()); + } + if (!StringUtils.isBlank(getMaleParentDBID())) { + germplasm.putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_GID, getMaleParentDBID()); + } + if (!StringUtils.isBlank(getFemaleParentEntryNo())) { + germplasm.putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_ENTRY_NO, getFemaleParentEntryNo()); + } + if (!StringUtils.isBlank(getMaleParentEntryNo())) { + germplasm.putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_ENTRY_NO, getMaleParentEntryNo()); + } + } // Append synonyms to germplasm that don't already exist // Synonym comparison is based on name and type @@ -208,15 +219,16 @@ public boolean pedigreesEqual(BrAPIGermplasm brAPIGermplasm) { String brapiFemaleGid = femaleGid != null ? femaleGid.getAsString() : null; JsonElement maleGid = brAPIGermplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_GID); String brapiMaleGid = maleGid != null ? maleGid.getAsString() : null; - JsonElement femaleEntryNo = brAPIGermplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_ENTRY_NO); - String brapiFemaleEntryNo = femaleEntryNo != null ? femaleEntryNo.getAsString() : null; - JsonElement maleEntryNo = brAPIGermplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_ENTRY_NO); - String brapiMaleEntryNo = maleEntryNo != null ? maleEntryNo.getAsString() : null; - - return ((getFemaleParentDBID() == null && brapiFemaleGid == null) || (getFemaleParentDBID() != null && getFemaleParentDBID().equals(brapiFemaleGid))) && - ((getMaleParentDBID() == null && brapiMaleGid == null) || (getMaleParentDBID() != null && getMaleParentDBID().equals(brapiMaleGid))) && - ((getFemaleParentEntryNo() == null && brapiFemaleEntryNo == null) || (getFemaleParentEntryNo() != null && getFemaleParentEntryNo().equals(brapiFemaleEntryNo))) && - ((getMaleParentEntryNo() == null && brapiMaleEntryNo == null) || (getMaleParentEntryNo() != null && getMaleParentEntryNo().equals(brapiMaleEntryNo))); + + return ((StringUtils.isBlank(getFemaleParentDBID()) && StringUtils.isBlank(brapiFemaleGid)) || (getFemaleParentDBID() != null && getFemaleParentDBID().equals(brapiFemaleGid))) && + ((StringUtils.isBlank(getMaleParentDBID()) && StringUtils.isBlank(brapiMaleGid)) || (getMaleParentDBID() != null && getMaleParentDBID().equals(brapiMaleGid))); + } + + public boolean pedigreeEmpty() { + return StringUtils.isBlank(getFemaleParentDBID()) && + StringUtils.isBlank(getMaleParentDBID()) && + StringUtils.isBlank(getFemaleParentEntryNo()) && + StringUtils.isBlank(getMaleParentEntryNo()); } public BrAPIGermplasm constructBrAPIGermplasm(ProgramBreedingMethodEntity breedingMethod, User user, UUID listId) { diff --git a/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java b/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java index cc4d8f476..13d98a777 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java +++ b/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java @@ -279,7 +279,7 @@ public Map process(List importRows // Have GID so updating an existing germplasm record if (germplasm.getAccessionNumber() != null) { - if (dbGermplasmByAccessionNo.containsKey(germplasm.getAccessionNumber()) ) { + if (dbGermplasmByAccessionNo.containsKey(germplasm.getAccessionNumber())) { existingGermplasm = dbGermplasmByAccessionNo.get(germplasm.getAccessionNumber()); } else { ValidationError ve = new ValidationError("GID", missingGID, HttpStatus.NOT_FOUND); @@ -287,25 +287,35 @@ public Map process(List importRows continue; } + // Update conditions: + // no existing pedigree and file has different pedigree and not empty + boolean updatePedigree = StringUtils.isBlank(existingGermplasm.getPedigree()) && + !germplasm.pedigreesEqual(existingGermplasm) && + !germplasm.pedigreeEmpty(); + // Error conditions: - // has existing pedigree and file pedigree is different + // has existing pedigree and file pedigree is different and not empty // Valid conditions: - // no existing pedigree and file different pedigree (not blank though, will fail other validations) + // no existing pedigree and file different pedigree // existing pedigree and file pedigree same - - if (!StringUtils.isBlank(existingGermplasm.getPedigree()) && !germplasm.pedigreesEqual(existingGermplasm) ) { + // existing pedigree and file pedigree empty + if (!StringUtils.isBlank(existingGermplasm.getPedigree()) && + !germplasm.pedigreesEqual(existingGermplasm) && + !germplasm.pedigreeEmpty()) { ValidationError ve = new ValidationError("Pedigree", pedigreeAlreadyExists, HttpStatus.UNPROCESSABLE_ENTITY); validationErrors.addError(i+2, ve ); // +2 instead of +1 to account for the column header row. continue; } - validatePedigree(germplasm, i+2, validationErrors); + // only validate if updating pedigree + if (updatePedigree) { + validatePedigree(germplasm, i+2, validationErrors); + } - germplasm.updateBrAPIGermplasm(existingGermplasm, program, importListId, commit); + germplasm.updateBrAPIGermplasm(existingGermplasm, program, importListId, commit, updatePedigree); updatedGermplasmList.add(existingGermplasm); mappedImportRow.setGermplasm(new PendingImportObject<>(ImportObjectState.EXISTING, existingGermplasm)); - importList.addDataItem(existingGermplasm.getGermplasmName()); } else { From 153d2045da23e96c650e57f638271c35b40367aa Mon Sep 17 00:00:00 2001 From: Nick <53413353+nickpalladino@users.noreply.github.com> Date: Wed, 2 Aug 2023 22:55:42 -0400 Subject: [PATCH 02/15] Logic updates --- .../brapps/importer/model/base/Germplasm.java | 6 ++-- .../processors/GermplasmProcessor.java | 33 ++++++++++++++----- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/breedinginsight/brapps/importer/model/base/Germplasm.java b/src/main/java/org/breedinginsight/brapps/importer/model/base/Germplasm.java index b8b63c1fb..b48b9e62b 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/model/base/Germplasm.java +++ b/src/main/java/org/breedinginsight/brapps/importer/model/base/Germplasm.java @@ -214,11 +214,11 @@ public void setUpdateCommitFields(BrAPIGermplasm germplasm, String programKey) { } } - public boolean pedigreesEqual(BrAPIGermplasm brAPIGermplasm) { + public boolean pedigreesEqualGidOnly(BrAPIGermplasm brAPIGermplasm) { JsonElement femaleGid = brAPIGermplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_GID); - String brapiFemaleGid = femaleGid != null ? femaleGid.getAsString() : null; + String brapiFemaleGid = femaleGid != null && !femaleGid.isJsonNull() ? femaleGid.getAsString() : null; JsonElement maleGid = brAPIGermplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_GID); - String brapiMaleGid = maleGid != null ? maleGid.getAsString() : null; + String brapiMaleGid = maleGid != null && !maleGid.isJsonNull() ? maleGid.getAsString() : null; return ((StringUtils.isBlank(getFemaleParentDBID()) && StringUtils.isBlank(brapiFemaleGid)) || (getFemaleParentDBID() != null && getFemaleParentDBID().equals(brapiFemaleGid))) && ((StringUtils.isBlank(getMaleParentDBID()) && StringUtils.isBlank(brapiMaleGid)) || (getMaleParentDBID() != null && getMaleParentDBID().equals(brapiMaleGid))); diff --git a/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java b/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java index 13d98a777..3c730e35f 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java +++ b/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java @@ -289,9 +289,7 @@ public Map process(List importRows // Update conditions: // no existing pedigree and file has different pedigree and not empty - boolean updatePedigree = StringUtils.isBlank(existingGermplasm.getPedigree()) && - !germplasm.pedigreesEqual(existingGermplasm) && - !germplasm.pedigreeEmpty(); + boolean updatePedigree = updatePedigree(existingGermplasm, germplasm); // Error conditions: // has existing pedigree and file pedigree is different and not empty @@ -300,7 +298,7 @@ public Map process(List importRows // existing pedigree and file pedigree same // existing pedigree and file pedigree empty if (!StringUtils.isBlank(existingGermplasm.getPedigree()) && - !germplasm.pedigreesEqual(existingGermplasm) && + !germplasm.pedigreesEqualGidOnly(existingGermplasm) && !germplasm.pedigreeEmpty()) { ValidationError ve = new ValidationError("Pedigree", pedigreeAlreadyExists, HttpStatus.UNPROCESSABLE_ENTITY); validationErrors.addError(i+2, ve ); // +2 instead of +1 to account for the column header row. @@ -387,6 +385,21 @@ public Map process(List importRows return getStatisticsMap(importRows); } + private boolean updatePedigree(BrAPIGermplasm existingGermplasm, Germplasm germplasm) { + // Update conditions: + // no existing pedigree and file has different pedigree and not empty + return StringUtils.isBlank(existingGermplasm.getPedigree()) && + !germplasm.pedigreesEqualGidOnly(existingGermplasm) && + !germplasm.pedigreeEmpty(); + } + + private boolean updatePedigreeNoEqualsCheck(BrAPIGermplasm existingGermplasm, Germplasm germplasm) { + + + return StringUtils.isBlank(existingGermplasm.getPedigree()) && + !germplasm.pedigreeEmpty(); + } + private Map getStatisticsMap(List importRows) { ImportPreviewStatistics germplasmStats = ImportPreviewStatistics.builder() @@ -584,14 +597,16 @@ else if (germplasmIndexByEntryNo.containsKey(germplasm.getFemaleParentEntryNo()) } } - // only update brapi object for new germplasm or update with no previous pedigree + // only update pedigree with the following conditions: + // - new germplasm & pedigree is not empty + // - existing pedigree & existing pedigree is blank & new pedigree is different & not empty BrAPIGermplasm brapiGermplasm = mappedBrAPIImport.get(i).getGermplasm().getBrAPIObject(); - Optional existingPedigree = existingGermplasms.stream() - .filter(g -> g.equals(brapiGermplasm) && StringUtils.isBlank(g.getPedigree())) - .findFirst(); + // no existing pedigree and pedigree not empty + // pedigrees will be equal at this point from prior processing code if being updated so don't check that + boolean updatePedigree = updatePedigreeNoEqualsCheck(brapiGermplasm, germplasm); - if (existingPedigree.isEmpty()) { + if (updatePedigree) { mappedBrAPIImport.get(i).getGermplasm().getBrAPIObject().setPedigree(pedigreeString.length() > 0 ? pedigreeString.toString() : null); //Simpler to just always add boolean, but consider for logic that previous imported values won't have that additional info value mappedBrAPIImport.get(i).getGermplasm().getBrAPIObject().putAdditionalInfoItem(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN, femaleParentUnknown); From bc885a6d4a4ff84d92031b7d99826ee2022697cc Mon Sep 17 00:00:00 2001 From: timparsons Date: Fri, 4 Aug 2023 00:28:27 -0400 Subject: [PATCH 03/15] refactoring some of the logic in GermplasmProcessor --- .../brapi/v2/GermplasmController.java | 5 +- .../brapi/v2/dao/BrAPIGermplasmDAO.java | 4 +- .../brapps/importer/model/base/Germplasm.java | 20 +- .../processors/GermplasmProcessor.java | 341 ++++++++++++------ 4 files changed, 240 insertions(+), 130 deletions(-) diff --git a/src/main/java/org/breedinginsight/brapi/v2/GermplasmController.java b/src/main/java/org/breedinginsight/brapi/v2/GermplasmController.java index d91f4ed57..bed1f4a40 100644 --- a/src/main/java/org/breedinginsight/brapi/v2/GermplasmController.java +++ b/src/main/java/org/breedinginsight/brapi/v2/GermplasmController.java @@ -28,6 +28,7 @@ import org.breedinginsight.api.model.v1.validators.SearchValid; import org.breedinginsight.brapi.v1.controller.BrapiVersion; import org.breedinginsight.brapi.v1.model.request.query.BrapiQuery; +import org.breedinginsight.brapi.v2.constants.BrAPIAdditionalInfoFields; import org.breedinginsight.brapi.v2.dao.BrAPIGermplasmDAO; import org.breedinginsight.brapi.v2.model.request.query.GermplasmQuery; import org.breedinginsight.utilities.Utilities; @@ -223,14 +224,14 @@ public HttpResponse getGermplasmPedigreeInfo( response = pedigreeResponse.getBody(); //Add nodes for unknown parents if applicable - if (germplasm.getAdditionalInfo().has("femaleParentUnknown") && germplasm.getAdditionalInfo().get("femaleParentUnknown").getAsBoolean()) { + if (germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN) && germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN).getAsBoolean()) { BrAPIPedigreeNodeParents unknownFemale = new BrAPIPedigreeNodeParents(); unknownFemale.setGermplasmDbId(germplasm.getGermplasmDbId()+"-F-Unknown"); unknownFemale.setGermplasmName("Unknown"); unknownFemale.setParentType(BrAPIParentType.FEMALE); returnNode.addParentsItem(unknownFemale); } - if (germplasm.getAdditionalInfo().has("maleParentUnknown") && germplasm.getAdditionalInfo().get("maleParentUnknown").getAsBoolean()) { + if (germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.MALE_PARENT_UNKNOWN) && germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.MALE_PARENT_UNKNOWN).getAsBoolean()) { BrAPIPedigreeNodeParents unknownMale = new BrAPIPedigreeNodeParents(); unknownMale.setGermplasmDbId(germplasm.getGermplasmDbId()+"-M-Unknown"); unknownMale.setGermplasmName("Unknown"); diff --git a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java index 14ffdf6b3..aa099b5c8 100644 --- a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java +++ b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java @@ -205,7 +205,7 @@ private Map processGermplasmForDisplay(List ref.getReferenceSource().equals(referenceSource)). map(ref -> ref.getReferenceID()).findFirst().orElse(""); additionalInfo.addProperty(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_GID, femaleParentAccessionNumber); - } else if (additionalInfo.has("femaleParentUnknown") && additionalInfo.get("femaleParentUnknown").getAsBoolean()) { + } else if (additionalInfo.has(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN) && additionalInfo.get(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN).getAsBoolean()) { namePedigreeString = "Unknown"; } } @@ -221,7 +221,7 @@ private Map processGermplasmForDisplay(List> postOrder = new ArrayList<>(); BrAPIListNewRequest importList = new BrAPIListNewRequest(); + public static String missingDbIdsMsg = "The following GIDs were not found in the database: %s."; public static String missingParentalDbIdsMsg = "The following parental GIDs were not found in the database: %s."; public static String missingParentalEntryNoMsg = "The following parental entry numbers were not found in the database: %s."; public static String badBreedMethodsMsg = "Invalid breeding method"; @@ -116,17 +113,20 @@ public GermplasmProcessor(BrAPIGermplasmService brAPIGermplasmService, DSLContex public void getExistingBrapiData(List importRows, Program program) throws ApiException { // Get all of our objects specified in the data file by their unique attributes - Set germplasmDBIDs = new HashSet<>(); + Map germplasmDBIDs = new HashMap<>(); for (int i = 0; i < importRows.size(); i++) { BrAPIImport germplasmImport = importRows.get(i); if (germplasmImport.getGermplasm() != null) { // Retrieve parent dbids to assess if already in db if (germplasmImport.getGermplasm().getFemaleParentDBID() != null) { - germplasmDBIDs.add(germplasmImport.getGermplasm().getFemaleParentDBID()); + germplasmDBIDs.put(germplasmImport.getGermplasm().getFemaleParentDBID(), true); } if (germplasmImport.getGermplasm().getMaleParentDBID() != null) { - germplasmDBIDs.add(germplasmImport.getGermplasm().getMaleParentDBID()); + germplasmDBIDs.put(germplasmImport.getGermplasm().getMaleParentDBID(), true); + } + if (germplasmImport.getGermplasm().getAccessionNumber() != null) { + germplasmDBIDs.put(germplasmImport.getGermplasm().getAccessionNumber(), false); } //Retrieve entry numbers of file for comparison with parent entry numbers @@ -141,20 +141,21 @@ public void getExistingBrapiData(List importRows, Program program) // If parental DBID, should also be in database existingGermplasms = new ArrayList<>(); - List missingDbIds = new ArrayList<>(germplasmDBIDs); + List missingParentalDbIds = germplasmDBIDs.entrySet().stream().filter(Map.Entry::getValue).map(Map.Entry::getKey).collect(Collectors.toList()); + List missingDbIds = germplasmDBIDs.entrySet().stream().filter(Map.Entry::getValue).map(Map.Entry::getKey).collect(Collectors.toList()); if (germplasmDBIDs.size() > 0) { try { - existingParentGermplasms = brAPIGermplasmService.getRawGermplasmByAccessionNumber(new ArrayList<>(germplasmDBIDs), program.getId()); - List existingDbIds = existingParentGermplasms.stream() + existingGermplasms = brAPIGermplasmService.getRawGermplasmByAccessionNumber(new ArrayList<>(germplasmDBIDs.keySet()), program.getId()); + List existingDbIds = existingGermplasms.stream() .map(germplasm -> germplasm.getAccessionNumber()) .collect(Collectors.toList()); + missingParentalDbIds.removeAll(existingDbIds); missingDbIds.removeAll(existingDbIds); - existingParentGermplasms.forEach(existingGermplasm -> { + existingGermplasms.forEach(existingGermplasm -> { germplasmByAccessionNumber.put(existingGermplasm.getAccessionNumber(), new PendingImportObject<>(ImportObjectState.EXISTING, existingGermplasm)); }); - //Since parent germplasms need to be present for check re circular dependencies - existingGermplasms.addAll(existingParentGermplasms); + existingParentGermplasms = existingGermplasms.stream().filter(germplasm -> germplasmDBIDs.get(germplasm.getAccessionNumber())).collect(Collectors.toList()); } catch (ApiException e) { // We shouldn't get an error back from our services. If we do, nothing the user can do about it throw new InternalServerException(e.toString(), e); @@ -163,8 +164,10 @@ public void getExistingBrapiData(List importRows, Program program) // Get existing germplasm names List dbGermplasm = brAPIGermplasmService.getGermplasmByDisplayName(new ArrayList<>(fileGermplasmByName.keySet()), program.getId()); - dbGermplasm.stream().forEach(germplasm -> dbGermplasmByName.put(germplasm.getDefaultDisplayName(), germplasm)); - dbGermplasm.stream().forEach(germplasm -> dbGermplasmByAccessionNo.put(germplasm.getAccessionNumber(), germplasm)); + dbGermplasm.forEach(germplasm -> { + dbGermplasmByName.put(germplasm.getDefaultDisplayName(), germplasm); + dbGermplasmByAccessionNo.put(germplasm.getAccessionNumber(), germplasm); + }); // Check for existing germplasm lists Boolean listNameDup = false; @@ -184,12 +187,20 @@ public void getExistingBrapiData(List importRows, Program program) } //Remove id indicating unknown parent + missingParentalDbIds.remove("0"); missingDbIds.remove("0"); // Parent reference checks - if (missingDbIds.size() > 0) { + if (missingParentalDbIds.size() > 0) { throw new HttpStatusException(HttpStatus.UNPROCESSABLE_ENTITY, String.format(missingParentalDbIdsMsg, + arrayOfStringFormatter.apply(missingParentalDbIds))); + } + + //GID existence check + if (missingDbIds.size() > 0) { + throw new HttpStatusException(HttpStatus.UNPROCESSABLE_ENTITY, + String.format(missingDbIdsMsg, arrayOfStringFormatter.apply(missingDbIds))); } @@ -273,85 +284,14 @@ public Map process(List importRows //todo double check what dbgermplasmbyaccessionNo actually getting //TODO maybe make separate method for cleanliness - if (germplasm != null) { - //Fetch and update existing germplasm - BrAPIGermplasm existingGermplasm; - - // Have GID so updating an existing germplasm record - if (germplasm.getAccessionNumber() != null) { - if (dbGermplasmByAccessionNo.containsKey(germplasm.getAccessionNumber())) { - existingGermplasm = dbGermplasmByAccessionNo.get(germplasm.getAccessionNumber()); - } else { - ValidationError ve = new ValidationError("GID", missingGID, HttpStatus.NOT_FOUND); - validationErrors.addError(i+2, ve ); // +2 instead of +1 to account for the column header row. - continue; - } - - // Update conditions: - // no existing pedigree and file has different pedigree and not empty - boolean updatePedigree = updatePedigree(existingGermplasm, germplasm); - - // Error conditions: - // has existing pedigree and file pedigree is different and not empty - // Valid conditions: - // no existing pedigree and file different pedigree - // existing pedigree and file pedigree same - // existing pedigree and file pedigree empty - if (!StringUtils.isBlank(existingGermplasm.getPedigree()) && - !germplasm.pedigreesEqualGidOnly(existingGermplasm) && - !germplasm.pedigreeEmpty()) { - ValidationError ve = new ValidationError("Pedigree", pedigreeAlreadyExists, HttpStatus.UNPROCESSABLE_ENTITY); - validationErrors.addError(i+2, ve ); // +2 instead of +1 to account for the column header row. - continue; - } - - // only validate if updating pedigree - if (updatePedigree) { - validatePedigree(germplasm, i+2, validationErrors); - } - - germplasm.updateBrAPIGermplasm(existingGermplasm, program, importListId, commit, updatePedigree); - - updatedGermplasmList.add(existingGermplasm); - mappedImportRow.setGermplasm(new PendingImportObject<>(ImportObjectState.EXISTING, existingGermplasm)); - importList.addDataItem(existingGermplasm.getGermplasmName()); - - } else { - // Get the breeding method database object - ProgramBreedingMethodEntity breedingMethod = null; - if (germplasm.getBreedingMethod() != null) { - if (breedingMethods.containsKey(germplasm.getBreedingMethod())) { - breedingMethod = breedingMethods.get(germplasm.getBreedingMethod()); - } else { - List breedingMethodResults = breedingMethodDAO.findByNameOrAbbreviation(germplasm.getBreedingMethod(), program.getId()); - if (breedingMethodResults.size() > 0) { - breedingMethods.put(germplasm.getBreedingMethod(), breedingMethodResults.get(0)); - breedingMethod = breedingMethods.get(germplasm.getBreedingMethod()); - } else { - ValidationError ve = new ValidationError("Breeding Method", badBreedMethodsMsg, HttpStatus.UNPROCESSABLE_ENTITY); - validationErrors.addError(i + 2, ve); // +2 instead of +1 to account for the column header row. - badBreedingMethods.add(germplasm.getBreedingMethod()); - breedingMethod = null; - } - } - } - - validatePedigree(germplasm, i + 2, validationErrors); - - BrAPIGermplasm newGermplasm = germplasm.constructBrAPIGermplasm(program, breedingMethod, user, commit, BRAPI_REFERENCE_SOURCE, nextVal, importListId); - - newGermplasmList.add(newGermplasm); - // Assign status of the germplasm - if (fileGermplasmByName.get(newGermplasm.getDefaultDisplayName()) > 1 || dbGermplasmByName.containsKey(newGermplasm.getDefaultDisplayName())) { - mappedImportRow.setGermplasm(new PendingImportObject<>(ImportObjectState.EXISTING, newGermplasm)); - } else { - mappedImportRow.setGermplasm(new PendingImportObject<>(ImportObjectState.NEW, newGermplasm)); - } - - importList.addDataItem(newGermplasm.getGermplasmName()); + // Have GID so updating an existing germplasm record + if (germplasm.getAccessionNumber() != null) { + if(!processExistingGermplasm(germplasm, validationErrors, importRows, program, importListId, commit, mappedImportRow, i)) { + continue; } + } else { - mappedImportRow.setGermplasm(null); + processNewGermplasm(germplasm, validationErrors, breedingMethods, badBreedingMethods, program, importListId, commit, mappedImportRow, i, user, nextVal); } mappedBrAPIImport.put(i, mappedImportRow); } @@ -385,19 +325,180 @@ public Map process(List importRows return getStatisticsMap(importRows); } - private boolean updatePedigree(BrAPIGermplasm existingGermplasm, Germplasm germplasm) { + private void processNewGermplasm(Germplasm germplasm, ValidationErrors validationErrors, Map breedingMethods, + List badBreedingMethods, + Program program, UUID importListId, boolean commit, PendingImport mappedImportRow, int i, User user, Supplier nextVal) { + // Get the breeding method database object + ProgramBreedingMethodEntity breedingMethod = null; + if (germplasm.getBreedingMethod() != null) { + if (breedingMethods.containsKey(germplasm.getBreedingMethod())) { + breedingMethod = breedingMethods.get(germplasm.getBreedingMethod()); + } else { + List breedingMethodResults = breedingMethodDAO.findByNameOrAbbreviation(germplasm.getBreedingMethod(), program.getId()); + if (breedingMethodResults.size() > 0) { + breedingMethods.put(germplasm.getBreedingMethod(), breedingMethodResults.get(0)); + breedingMethod = breedingMethods.get(germplasm.getBreedingMethod()); + } else { + ValidationError ve = new ValidationError("Breeding Method", badBreedMethodsMsg, HttpStatus.UNPROCESSABLE_ENTITY); + validationErrors.addError(i + 2, ve); // +2 instead of +1 to account for the column header row. + badBreedingMethods.add(germplasm.getBreedingMethod()); + } + } + } + + validatePedigree(germplasm, i + 2, validationErrors); + + BrAPIGermplasm newGermplasm = germplasm.constructBrAPIGermplasm(program, breedingMethod, user, commit, BRAPI_REFERENCE_SOURCE, nextVal, importListId); + + newGermplasmList.add(newGermplasm); + // Assign status of the germplasm + if (fileGermplasmByName.get(newGermplasm.getDefaultDisplayName()) > 1 || dbGermplasmByName.containsKey(newGermplasm.getDefaultDisplayName())) { + mappedImportRow.setGermplasm(new PendingImportObject<>(ImportObjectState.EXISTING, newGermplasm)); + } else { + mappedImportRow.setGermplasm(new PendingImportObject<>(ImportObjectState.NEW, newGermplasm)); + } + + importList.addDataItem(newGermplasm.getGermplasmName()); + } + + private boolean processExistingGermplasm(Germplasm germplasm, ValidationErrors validationErrors, List importRows, Program program, UUID importListId, boolean commit, PendingImport mappedImportRow, int rowIndex) { + BrAPIGermplasm existingGermplasm; + if (germplasmByAccessionNumber.containsKey(germplasm.getAccessionNumber())) { + existingGermplasm = germplasmByAccessionNumber.get(germplasm.getAccessionNumber()).getBrAPIObject(); + } else { + //should be caught in getExistingBrapiData + ValidationError ve = new ValidationError("GID", missingGID, HttpStatus.NOT_FOUND); + validationErrors.addError(rowIndex+2, ve ); // +2 instead of +1 to account for the column header row. + return false; + } + + // Error conditions: + // has existing pedigree and file pedigree is different and not empty + // Valid conditions: + // no existing pedigree and file different pedigree + // existing pedigree and file pedigree same + // existing pedigree and file pedigree empty +// if (!StringUtils.isBlank(existingGermplasm.getPedigree()) && +// !germplasm.pedigreesEqualGidOnly(existingGermplasm) && +// !germplasm.pedigreeEmpty()) { + if(hasPedigree(existingGermplasm) && germplasm.pedigreeExists()) { + if(!arePedigreesEqual(existingGermplasm, germplasm, importRows)) { + ValidationError ve = new ValidationError("Pedigree", pedigreeAlreadyExists, HttpStatus.UNPROCESSABLE_ENTITY); + validationErrors.addError(rowIndex + 2, ve); // +2 instead of +1 to account for the column header row. + return false; + } + } else { + if(germplasm.pedigreeExists()) { + validatePedigree(germplasm, rowIndex + 2, validationErrors); + } + + germplasm.updateBrAPIGermplasm(existingGermplasm, program, importListId, commit, true); + + updatedGermplasmList.add(existingGermplasm); + mappedImportRow.setGermplasm(new PendingImportObject<>(ImportObjectState.MUTATED, existingGermplasm)); + importList.addDataItem(existingGermplasm.getGermplasmName()); + } + + return true; + } + + private boolean canUpdatePedigree(BrAPIGermplasm existingGermplasm, Germplasm germplasm) { // Update conditions: // no existing pedigree and file has different pedigree and not empty - return StringUtils.isBlank(existingGermplasm.getPedigree()) && - !germplasm.pedigreesEqualGidOnly(existingGermplasm) && - !germplasm.pedigreeEmpty(); +// return StringUtils.isBlank(existingGermplasm.getPedigree()) && +// !germplasm.pedigreeEmpty() && +// !germplasm.pedigreesEqualGidOnly(existingGermplasm); //seems unnecessary + + return !hasPedigree(existingGermplasm) && germplasm.pedigreeExists(); + } + + private boolean hasPedigree(BrAPIGermplasm germplasm) { + return StringUtils.isNotBlank(germplasm.getPedigree()) + || germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_GID) + || germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_GID) + || germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN) + || germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.MALE_PARENT_UNKNOWN); + } + + /** + * Compare an existing germplasm's pedigree to the incoming germplasm's pedigree to ensure they are the same.

+ * Assumes that an empty value for a given parent in the incoming germplasm is equal to the existing germplasm.

+ * Assumes that the existing germplasm has pedigree + * @param existingGermplasm the existing germplasm with pedigree + * @param germplasm the germplasm record coming in the file + * @param importRows all records coming in the file. Needed to look up GID of a parent referenced by entry number in the file + * @return true if the two germplasm pedigrees are effectively equal, false otherwise + */ + private boolean arePedigreesEqual(BrAPIGermplasm existingGermplasm, Germplasm germplasm, List importRows) { + if(germplasm.pedigreeExists()) { + StringBuilder existingPedigreeGIDString = new StringBuilder(); + String existingFemalePedigree = getParentId(existingGermplasm, existingPedigreeGIDString, BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_GID, BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN); + existingPedigreeGIDString.append("/"); + String existingMalePedigree = getParentId(existingGermplasm, existingPedigreeGIDString, BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_GID, BrAPIAdditionalInfoFields.MALE_PARENT_UNKNOWN); + + StringBuilder germplasmPedigreeGIDString = new StringBuilder(); + if (StringUtils.isNotBlank(germplasm.getFemaleParentDBID())) { + germplasmPedigreeGIDString.append(germplasm.getFemaleParentDBID()); + } else if (StringUtils.isNotBlank(germplasm.getFemaleParentEntryNo())) { + Integer femaleParentIdx = germplasmIndexByEntryNo.get(germplasm.getFemaleParentEntryNo()); + BrAPIImport femaleParentRow = importRows.get(femaleParentIdx); + BrAPIGermplasm femaleGerm = dbGermplasmByName.get(femaleParentRow.getGermplasm() + .getGermplasmName()); + if (femaleGerm != null) { + germplasmPedigreeGIDString.append(femaleGerm.getAccessionNumber()); + } else { + germplasmPedigreeGIDString.append("-1"); + } + } else { + germplasmPedigreeGIDString.append(existingFemalePedigree); + } + germplasmPedigreeGIDString.append("/"); + if (StringUtils.isNotBlank(germplasm.getMaleParentDBID())) { + germplasmPedigreeGIDString.append(germplasm.getMaleParentDBID()); + } else if (StringUtils.isNotBlank(germplasm.getMaleParentEntryNo())) { + Integer maleParentIdx = germplasmIndexByEntryNo.get(germplasm.getMaleParentEntryNo()); + BrAPIImport maleParentRow = importRows.get(maleParentIdx); + BrAPIGermplasm maleGerm = dbGermplasmByName.get(maleParentRow.getGermplasm() + .getGermplasmName()); + if (maleGerm != null) { + germplasmPedigreeGIDString.append(maleGerm.getAccessionNumber()); + } else { + germplasmPedigreeGIDString.append("-1"); + } + } else { + germplasmPedigreeGIDString.append(existingMalePedigree); + } + + return existingPedigreeGIDString.toString().equals(germplasmPedigreeGIDString.toString()); + } else { + return true; + } + } + + private String getParentId(BrAPIGermplasm existingGermplasm, StringBuilder pedigreeGIDString, String gidAdditionaInfoField, String unknownAdditionalInfoField) { + if (existingGermplasm.getAdditionalInfo() + .has(gidAdditionaInfoField)) { + pedigreeGIDString.append(existingGermplasm.getAdditionalInfo() + .get(gidAdditionaInfoField) + .getAsString()); + return existingGermplasm.getAdditionalInfo() + .get(gidAdditionaInfoField) + .getAsString(); + } else if (existingGermplasm.getAdditionalInfo() + .has(unknownAdditionalInfoField) && existingGermplasm.getAdditionalInfo() + .get(unknownAdditionalInfoField) + .getAsBoolean()) { + pedigreeGIDString.append("0"); + return "0"; + } + return ""; } - private boolean updatePedigreeNoEqualsCheck(BrAPIGermplasm existingGermplasm, Germplasm germplasm) { + private boolean canUpdatePedigreeNoEqualsCheck(BrAPIGermplasm existingGermplasm, Germplasm germplasm) { return StringUtils.isBlank(existingGermplasm.getPedigree()) && - !germplasm.pedigreeEmpty(); + germplasm.pedigreeExists(); } private Map getStatisticsMap(List importRows) { @@ -462,8 +563,8 @@ private void createPostOrder() { List pedigreeArray = List.of(germplasm.getPedigree().split("/")); String femaleParent = pedigreeArray.get(0); String maleParent = pedigreeArray.size() > 1 ? pedigreeArray.get(1) : null; - if (created.contains(femaleParent) || germplasm.getAdditionalInfo().get("femaleParentUnknown").getAsBoolean()) { - if (maleParent == null || created.contains(maleParent) || germplasm.getAdditionalInfo().get("maleParentUnknown").getAsBoolean()) { + if (created.contains(femaleParent) || germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN).getAsBoolean()) { + if (maleParent == null || created.contains(maleParent) || germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.MALE_PARENT_UNKNOWN).getAsBoolean()) { createList.add(germplasm); } } @@ -471,7 +572,7 @@ private void createPostOrder() { totalRecorded += createList.size(); if (createList.size() > 0) { - created.addAll(createList.stream().map(brAPIGermplasm -> brAPIGermplasm.getGermplasmName()).collect(Collectors.toList())); + created.addAll(createList.stream().map(BrAPIGermplasm::getGermplasmName).collect(Collectors.toList())); postOrder.add(createList); } else if (totalRecorded < newGermplasmList.size()) { // We ran into circular dependencies, throw an error @@ -554,13 +655,16 @@ public void constructPedigreeString(List importRows, Map importRows, Map 0 ? pedigreeString.toString() : null); + //Simpler to just always add boolean, but consider for logic that previous imported values won't have that additional info value + mappedBrAPIImport.get(i).getGermplasm().getBrAPIObject().putAdditionalInfoItem(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN, femaleParentUnknown); + mappedBrAPIImport.get(i).getGermplasm().getBrAPIObject().putAdditionalInfoItem(BrAPIAdditionalInfoFields.MALE_PARENT_UNKNOWN, maleParentUnknown); + + if(commit) { + if (femaleParentFound) { + mappedBrAPIImport.get(i).getGermplasm().getBrAPIObject().putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_GID, femaleParent.getAccessionNumber()); + } - // no existing pedigree and pedigree not empty - // pedigrees will be equal at this point from prior processing code if being updated so don't check that - boolean updatePedigree = updatePedigreeNoEqualsCheck(brapiGermplasm, germplasm); + if (maleParent != null) { + mappedBrAPIImport.get(i).getGermplasm().getBrAPIObject().putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_GID, maleParent.getAccessionNumber()); + } - if (updatePedigree) { - mappedBrAPIImport.get(i).getGermplasm().getBrAPIObject().setPedigree(pedigreeString.length() > 0 ? pedigreeString.toString() : null); - //Simpler to just always add boolean, but consider for logic that previous imported values won't have that additional info value - mappedBrAPIImport.get(i).getGermplasm().getBrAPIObject().putAdditionalInfoItem(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN, femaleParentUnknown); - mappedBrAPIImport.get(i).getGermplasm().getBrAPIObject().putAdditionalInfoItem(BrAPIAdditionalInfoFields.MALE_PARENT_UNKNOWN, maleParentUnknown); + mappedBrAPIImport.get(i).getGermplasm().getBrAPIObject().putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_RAW_PEDIGREE, pedigreeString); } } } From ce4bee77160bcfce7d6092416d7366f5521b9d90 Mon Sep 17 00:00:00 2001 From: Nick <53413353+nickpalladino@users.noreply.github.com> Date: Mon, 7 Aug 2023 14:29:13 -0400 Subject: [PATCH 04/15] Fix missing gid message formatting --- .../importer/services/processors/GermplasmProcessor.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java b/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java index dd3c09213..f54547f37 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java +++ b/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java @@ -363,11 +363,12 @@ private void processNewGermplasm(Germplasm germplasm, ValidationErrors validatio private boolean processExistingGermplasm(Germplasm germplasm, ValidationErrors validationErrors, List importRows, Program program, UUID importListId, boolean commit, PendingImport mappedImportRow, int rowIndex) { BrAPIGermplasm existingGermplasm; - if (germplasmByAccessionNumber.containsKey(germplasm.getAccessionNumber())) { - existingGermplasm = germplasmByAccessionNumber.get(germplasm.getAccessionNumber()).getBrAPIObject(); + String gid = germplasm.getAccessionNumber(); + if (germplasmByAccessionNumber.containsKey(gid)) { + existingGermplasm = germplasmByAccessionNumber.get(gid).getBrAPIObject(); } else { //should be caught in getExistingBrapiData - ValidationError ve = new ValidationError("GID", missingGID, HttpStatus.NOT_FOUND); + ValidationError ve = new ValidationError("GID", String.format(missingGID, gid), HttpStatus.NOT_FOUND); validationErrors.addError(rowIndex+2, ve ); // +2 instead of +1 to account for the column header row. return false; } From f22046a0453e2350773b9a4aee7dcbd4a6107916 Mon Sep 17 00:00:00 2001 From: Nick <53413353+nickpalladino@users.noreply.github.com> Date: Mon, 7 Aug 2023 16:06:04 -0400 Subject: [PATCH 05/15] Updated hasPedigree to check for unknown being true, not just keys --- .../importer/services/processors/GermplasmProcessor.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java b/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java index f54547f37..1aff4ba4e 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java +++ b/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java @@ -417,8 +417,10 @@ private boolean hasPedigree(BrAPIGermplasm germplasm) { return StringUtils.isNotBlank(germplasm.getPedigree()) || germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_GID) || germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_GID) - || germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN) - || germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.MALE_PARENT_UNKNOWN); + || (germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN) && + germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN).getAsBoolean()) + || (germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.MALE_PARENT_UNKNOWN) && + germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.MALE_PARENT_UNKNOWN).getAsBoolean()); } /** From 4f432837e0b98574f6d8ebbaa94f9f7c99711e1f Mon Sep 17 00:00:00 2001 From: Nick <53413353+nickpalladino@users.noreply.github.com> Date: Tue, 8 Aug 2023 14:32:09 -0400 Subject: [PATCH 06/15] Update to make sure existing brapi object is populated in all cases --- .../processors/GermplasmProcessor.java | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java b/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java index 1aff4ba4e..df9de7ef0 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java +++ b/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java @@ -286,9 +286,10 @@ public Map process(List importRows //TODO maybe make separate method for cleanliness // Have GID so updating an existing germplasm record if (germplasm.getAccessionNumber() != null) { - if(!processExistingGermplasm(germplasm, validationErrors, importRows, program, importListId, commit, mappedImportRow, i)) { - continue; - } + processExistingGermplasm(germplasm, validationErrors, importRows, program, importListId, commit, mappedImportRow, i); + //if(!processExistingGermplasm(germplasm, validationErrors, importRows, program, importListId, commit, mappedImportRow, i)) { + // continue; + //} } else { processNewGermplasm(germplasm, validationErrors, breedingMethods, badBreedingMethods, program, importListId, commit, mappedImportRow, i, user, nextVal); @@ -388,18 +389,19 @@ private boolean processExistingGermplasm(Germplasm germplasm, ValidationErrors v validationErrors.addError(rowIndex + 2, ve); // +2 instead of +1 to account for the column header row. return false; } - } else { - if(germplasm.pedigreeExists()) { - validatePedigree(germplasm, rowIndex + 2, validationErrors); - } - - germplasm.updateBrAPIGermplasm(existingGermplasm, program, importListId, commit, true); + } - updatedGermplasmList.add(existingGermplasm); - mappedImportRow.setGermplasm(new PendingImportObject<>(ImportObjectState.MUTATED, existingGermplasm)); - importList.addDataItem(existingGermplasm.getGermplasmName()); + if(germplasm.pedigreeExists()) { + validatePedigree(germplasm, rowIndex + 2, validationErrors); } + germplasm.updateBrAPIGermplasm(existingGermplasm, program, importListId, commit, true); + + updatedGermplasmList.add(existingGermplasm); + mappedImportRow.setGermplasm(new PendingImportObject<>(ImportObjectState.MUTATED, existingGermplasm)); + importList.addDataItem(existingGermplasm.getGermplasmName()); + + return true; } From 4c74c6d3ce17e33652ad68e255baa7865410fdd3 Mon Sep 17 00:00:00 2001 From: Nick <53413353+nickpalladino@users.noreply.github.com> Date: Wed, 9 Aug 2023 11:04:57 -0400 Subject: [PATCH 07/15] Workaround for breedbase pedigree strings --- .../brapi/v2/dao/BrAPIGermplasmDAO.java | 31 ++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java index aa099b5c8..1aa61ce6a 100644 --- a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java +++ b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java @@ -187,7 +187,9 @@ private Map processGermplasmForDisplay(List processGermplasmForDisplay(List + private String processBreedbasePedigree(String pedigree) { + + if (pedigree != null) { + if (pedigree.equals("NA/NA")) { + return ""; + } + + // Technically processGermplasmForDisplay should handle ok without stripping these NAs but will strip anyways + // for consistency. + // We only allow the /NA case for single parent as we require a female parent in the pedigree + // keep the leading slash, will be handled by processGermplasmForDisplay + if (pedigree.endsWith("/NA")) { + return pedigree.substring(0, pedigree.length()-2); + } + + // shouldn't have this case in our data but just in case + if (pedigree.startsWith("NA/")) { + return pedigree.substring(2); + } + } + return ""; + } + public List createBrAPIGermplasm(List postBrAPIGermplasmList, UUID programId, ImportUpload upload) { GermplasmApi api = brAPIEndpointProvider.get(programDAO.getCoreClient(programId), GermplasmApi.class); var program = programDAO.fetchOneById(programId); From 1f4291fc3058d68596e734541a2aa54c3c6b0b7f Mon Sep 17 00:00:00 2001 From: Nick <53413353+nickpalladino@users.noreply.github.com> Date: Wed, 9 Aug 2023 12:11:42 -0400 Subject: [PATCH 08/15] Update breedbase string logic --- .../org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java index 1aa61ce6a..9bde0d273 100644 --- a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java +++ b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java @@ -264,7 +264,7 @@ private String processBreedbasePedigree(String pedigree) { return pedigree.substring(2); } } - return ""; + return pedigree; } public List createBrAPIGermplasm(List postBrAPIGermplasmList, UUID programId, ImportUpload upload) { From 09ccfd5fa20dd199d7b00cd8f45516e3d2fa0b14 Mon Sep 17 00:00:00 2001 From: Nick <53413353+nickpalladino@users.noreply.github.com> Date: Thu, 10 Aug 2023 11:39:02 -0400 Subject: [PATCH 09/15] Fixes for issues found testing with Breedbase --- .../brapi/v2/dao/BrAPIGermplasmDAO.java | 6 ++- .../processors/GermplasmProcessor.java | 49 ++++++++++++------- 2 files changed, 34 insertions(+), 21 deletions(-) diff --git a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java index 9bde0d273..07d03e9c0 100644 --- a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java +++ b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java @@ -291,8 +291,10 @@ public List updateBrAPIGermplasm(List putBrAPIGe try { if (!putBrAPIGermplasmList.isEmpty()) { postFunction = () -> { - List postResponse = putGermplasm(putBrAPIGermplasmList, api); - return processGermplasmForDisplay(postResponse, program.getKey()); + putGermplasm(putBrAPIGermplasmList, api); + // Need all program germplasm for processGermplasmForDisplay parents pedigree + List germplasm = getRawGermplasm(programId); + return processGermplasmForDisplay(germplasm, program.getKey()); }; } return programGermplasmCache.post(programId, postFunction); diff --git a/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java b/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java index df9de7ef0..ea5ed19da 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java +++ b/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java @@ -24,6 +24,7 @@ import lombok.extern.slf4j.Slf4j; import org.apache.commons.lang3.StringUtils; import org.brapi.client.v2.model.exceptions.ApiException; +import org.brapi.v2.model.BrAPIAcceptedSearchResponse; import org.brapi.v2.model.core.BrAPIListSummary; import org.brapi.v2.model.core.request.BrAPIListNewRequest; import org.brapi.v2.model.germ.BrAPIGermplasm; @@ -406,19 +407,17 @@ private boolean processExistingGermplasm(Germplasm germplasm, ValidationErrors v } private boolean canUpdatePedigree(BrAPIGermplasm existingGermplasm, Germplasm germplasm) { - // Update conditions: - // no existing pedigree and file has different pedigree and not empty -// return StringUtils.isBlank(existingGermplasm.getPedigree()) && -// !germplasm.pedigreeEmpty() && -// !germplasm.pedigreesEqualGidOnly(existingGermplasm); //seems unnecessary + return !hasPedigreeString(existingGermplasm) && germplasm.pedigreeExists(); + } - return !hasPedigree(existingGermplasm) && germplasm.pedigreeExists(); + private boolean hasPedigreeString(BrAPIGermplasm germplasm) { + return StringUtils.isNotBlank(germplasm.getPedigree()); } private boolean hasPedigree(BrAPIGermplasm germplasm) { return StringUtils.isNotBlank(germplasm.getPedigree()) || germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_GID) - || germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_GID) + || germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_GID) || (germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN) && germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN).getAsBoolean()) || (germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.MALE_PARENT_UNKNOWN) && @@ -706,21 +705,33 @@ else if (germplasmIndexByEntryNo.containsKey(germplasm.getFemaleParentEntryNo()) } } - mappedBrAPIImport.get(i).getGermplasm().getBrAPIObject().setPedigree(pedigreeString.length() > 0 ? pedigreeString.toString() : null); - //Simpler to just always add boolean, but consider for logic that previous imported values won't have that additional info value - mappedBrAPIImport.get(i).getGermplasm().getBrAPIObject().putAdditionalInfoItem(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN, femaleParentUnknown); - mappedBrAPIImport.get(i).getGermplasm().getBrAPIObject().putAdditionalInfoItem(BrAPIAdditionalInfoFields.MALE_PARENT_UNKNOWN, maleParentUnknown); + BrAPIGermplasm brAPIGermplasm = mappedBrAPIImport.get(i).getGermplasm().getBrAPIObject(); - if(commit) { - if (femaleParentFound) { - mappedBrAPIImport.get(i).getGermplasm().getBrAPIObject().putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_GID, femaleParent.getAccessionNumber()); - } + // only allow this when not committing so that display name version can be shown in preview + if (!commit) { + brAPIGermplasm.setPedigree(brAPIGermplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_PEDIGREE_BY_NAME).getAsString()); + } - if (maleParent != null) { - mappedBrAPIImport.get(i).getGermplasm().getBrAPIObject().putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_GID, maleParent.getAccessionNumber()); - } + // no existing pedigree and pedigree not empty + // pedigrees will be equal at this point from prior processing code if being updated so don't check that + if (canUpdatePedigree(brAPIGermplasm, germplasm)) { + + brAPIGermplasm.setPedigree(pedigreeString.length() > 0 ? pedigreeString.toString() : null); + //Simpler to just always add boolean, but consider for logic that previous imported values won't have that additional info value + brAPIGermplasm.putAdditionalInfoItem(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN, femaleParentUnknown); + brAPIGermplasm.putAdditionalInfoItem(BrAPIAdditionalInfoFields.MALE_PARENT_UNKNOWN, maleParentUnknown); - mappedBrAPIImport.get(i).getGermplasm().getBrAPIObject().putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_RAW_PEDIGREE, pedigreeString); + if (commit) { + if (femaleParentFound) { + brAPIGermplasm.putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_GID, femaleParent.getAccessionNumber()); + } + + if (maleParent != null) { + brAPIGermplasm.putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_GID, maleParent.getAccessionNumber()); + } + + //brAPIGermplasm.putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_RAW_PEDIGREE, pedigreeString); + } } } } From 2329ca92371e5829653a6c9a93ccfe7635dad897 Mon Sep 17 00:00:00 2001 From: Nick <53413353+nickpalladino@users.noreply.github.com> Date: Thu, 10 Aug 2023 13:23:33 -0400 Subject: [PATCH 10/15] Cleanup some comments --- .../importer/services/processors/GermplasmProcessor.java | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java b/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java index ea5ed19da..7109fb3ad 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java +++ b/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java @@ -288,10 +288,6 @@ public Map process(List importRows // Have GID so updating an existing germplasm record if (germplasm.getAccessionNumber() != null) { processExistingGermplasm(germplasm, validationErrors, importRows, program, importListId, commit, mappedImportRow, i); - //if(!processExistingGermplasm(germplasm, validationErrors, importRows, program, importListId, commit, mappedImportRow, i)) { - // continue; - //} - } else { processNewGermplasm(germplasm, validationErrors, breedingMethods, badBreedingMethods, program, importListId, commit, mappedImportRow, i, user, nextVal); } @@ -381,9 +377,6 @@ private boolean processExistingGermplasm(Germplasm germplasm, ValidationErrors v // no existing pedigree and file different pedigree // existing pedigree and file pedigree same // existing pedigree and file pedigree empty -// if (!StringUtils.isBlank(existingGermplasm.getPedigree()) && -// !germplasm.pedigreesEqualGidOnly(existingGermplasm) && -// !germplasm.pedigreeEmpty()) { if(hasPedigree(existingGermplasm) && germplasm.pedigreeExists()) { if(!arePedigreesEqual(existingGermplasm, germplasm, importRows)) { ValidationError ve = new ValidationError("Pedigree", pedigreeAlreadyExists, HttpStatus.UNPROCESSABLE_ENTITY); @@ -729,8 +722,6 @@ else if (germplasmIndexByEntryNo.containsKey(germplasm.getFemaleParentEntryNo()) if (maleParent != null) { brAPIGermplasm.putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_GID, maleParent.getAccessionNumber()); } - - //brAPIGermplasm.putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_RAW_PEDIGREE, pedigreeString); } } } From 4bd3006096312dc6a3e9e5ccd292b4db7418b31c Mon Sep 17 00:00:00 2001 From: Nick <53413353+nickpalladino@users.noreply.github.com> Date: Tue, 15 Aug 2023 16:57:21 -0400 Subject: [PATCH 11/15] Null fix --- .../importer/services/processors/GermplasmProcessor.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java b/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java index 7109fb3ad..e9d126d0c 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java +++ b/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java @@ -702,7 +702,10 @@ else if (germplasmIndexByEntryNo.containsKey(germplasm.getFemaleParentEntryNo()) // only allow this when not committing so that display name version can be shown in preview if (!commit) { - brAPIGermplasm.setPedigree(brAPIGermplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_PEDIGREE_BY_NAME).getAsString()); + if (brAPIGermplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.GERMPLASM_PEDIGREE_BY_NAME)) { + brAPIGermplasm.setPedigree(brAPIGermplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_PEDIGREE_BY_NAME).getAsString()); + } + } // no existing pedigree and pedigree not empty From 22bc163e24038662866b4d7276895939712109b4 Mon Sep 17 00:00:00 2001 From: Nick <53413353+nickpalladino@users.noreply.github.com> Date: Wed, 16 Aug 2023 09:41:50 -0400 Subject: [PATCH 12/15] Removed unused import --- .../brapps/importer/services/processors/GermplasmProcessor.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java b/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java index e9d126d0c..341a78f96 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java +++ b/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java @@ -24,7 +24,6 @@ import lombok.extern.slf4j.Slf4j; import org.apache.commons.lang3.StringUtils; import org.brapi.client.v2.model.exceptions.ApiException; -import org.brapi.v2.model.BrAPIAcceptedSearchResponse; import org.brapi.v2.model.core.BrAPIListSummary; import org.brapi.v2.model.core.request.BrAPIListNewRequest; import org.brapi.v2.model.germ.BrAPIGermplasm; From 21eba3dfeabf2be8f63312581a427957cfbf3095 Mon Sep 17 00:00:00 2001 From: Nick <53413353+nickpalladino@users.noreply.github.com> Date: Wed, 16 Aug 2023 09:49:50 -0400 Subject: [PATCH 13/15] Removed unused method --- .../brapps/importer/model/base/Germplasm.java | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/main/java/org/breedinginsight/brapps/importer/model/base/Germplasm.java b/src/main/java/org/breedinginsight/brapps/importer/model/base/Germplasm.java index 106a04c64..9ccd129f1 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/model/base/Germplasm.java +++ b/src/main/java/org/breedinginsight/brapps/importer/model/base/Germplasm.java @@ -218,16 +218,6 @@ public void setUpdateCommitFields(BrAPIGermplasm germplasm, String programKey) { } } - public boolean pedigreesEqualGidOnly(BrAPIGermplasm brAPIGermplasm) { - JsonElement femaleGid = brAPIGermplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_GID); - String brapiFemaleGid = femaleGid != null && !femaleGid.isJsonNull() ? femaleGid.getAsString() : null; - JsonElement maleGid = brAPIGermplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_GID); - String brapiMaleGid = maleGid != null && !maleGid.isJsonNull() ? maleGid.getAsString() : null; - - return (StringUtils.isAllBlank(getFemaleParentDBID(), brapiFemaleGid) || StringUtils.equals(getFemaleParentDBID(), brapiFemaleGid)) - && (StringUtils.isAllBlank(getMaleParentDBID(), brapiMaleGid) || StringUtils.equals(getMaleParentDBID(), brapiMaleGid)); - } - public boolean pedigreeExists() { return StringUtils.isNotBlank(getFemaleParentDBID()) || StringUtils.isNotBlank(getMaleParentDBID()) || From 33a38c97db5d5940a067039e056ea2db16364cc6 Mon Sep 17 00:00:00 2001 From: Nick <53413353+nickpalladino@users.noreply.github.com> Date: Thu, 17 Aug 2023 16:55:11 -0400 Subject: [PATCH 14/15] Added comments --- .../org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java index 07d03e9c0..3adbb99ff 100644 --- a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java +++ b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java @@ -188,6 +188,7 @@ private Map processGermplasmForDisplay(List processGermplasmForDisplay(List From f0f7ec4b3afeda1b7cd6d189646821c7d9393b27 Mon Sep 17 00:00:00 2001 From: Nick <53413353+nickpalladino@users.noreply.github.com> Date: Thu, 17 Aug 2023 23:21:03 -0400 Subject: [PATCH 15/15] Update logic for filtering missing gids --- .../importer/services/processors/GermplasmProcessor.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java b/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java index 341a78f96..98746aab2 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java +++ b/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java @@ -78,7 +78,6 @@ public class GermplasmProcessor implements Processor { List updatedGermplasmList; List existingGermplasms; - List existingParentGermplasms; List> postOrder = new ArrayList<>(); BrAPIListNewRequest importList = new BrAPIListNewRequest(); @@ -142,7 +141,7 @@ public void getExistingBrapiData(List importRows, Program program) // If parental DBID, should also be in database existingGermplasms = new ArrayList<>(); List missingParentalDbIds = germplasmDBIDs.entrySet().stream().filter(Map.Entry::getValue).map(Map.Entry::getKey).collect(Collectors.toList()); - List missingDbIds = germplasmDBIDs.entrySet().stream().filter(Map.Entry::getValue).map(Map.Entry::getKey).collect(Collectors.toList()); + List missingDbIds = germplasmDBIDs.entrySet().stream().filter(entry -> !entry.getValue()).map(Map.Entry::getKey).collect(Collectors.toList()); if (germplasmDBIDs.size() > 0) { try { existingGermplasms = brAPIGermplasmService.getRawGermplasmByAccessionNumber(new ArrayList<>(germplasmDBIDs.keySet()), program.getId()); @@ -155,7 +154,6 @@ public void getExistingBrapiData(List importRows, Program program) existingGermplasms.forEach(existingGermplasm -> { germplasmByAccessionNumber.put(existingGermplasm.getAccessionNumber(), new PendingImportObject<>(ImportObjectState.EXISTING, existingGermplasm)); }); - existingParentGermplasms = existingGermplasms.stream().filter(germplasm -> germplasmDBIDs.get(germplasm.getAccessionNumber())).collect(Collectors.toList()); } catch (ApiException e) { // We shouldn't get an error back from our services. If we do, nothing the user can do about it throw new InternalServerException(e.toString(), e);