From a41e184ca688634e10ca85f6fa89aae789edd490 Mon Sep 17 00:00:00 2001 From: Nick <53413353+nickpalladino@users.noreply.github.com> Date: Mon, 7 Apr 2025 17:00:27 -0400 Subject: [PATCH 1/4] Use appropriate ids for both preview and commit --- .../processors/GermplasmProcessor.java | 94 ++++++++++++++----- .../duplicate_names_circular_dependency.csv | 6 ++ 2 files changed, 78 insertions(+), 22 deletions(-) create mode 100644 src/test/resources/files/germplasm_import/duplicate_names_circular_dependency.csv 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 aad1720d9..b6987488c 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 @@ -17,6 +17,7 @@ package org.breedinginsight.brapps.importer.services.processors; import com.google.gson.Gson; +import com.google.gson.JsonElement; import io.micronaut.context.annotation.Property; import io.micronaut.context.annotation.Prototype; import io.micronaut.http.HttpStatus; @@ -58,6 +59,9 @@ import java.util.function.Supplier; import java.util.stream.Collectors; +import static org.apache.commons.lang3.StringUtils.isBlank; +import static org.apache.commons.lang3.StringUtils.isNotBlank; + @Slf4j @Prototype public class GermplasmProcessor implements Processor { @@ -327,10 +331,7 @@ public Map process(ImportUpload upload, List
[ - ]) - // for !commit: Validate for circular pedigree dependencies. - createPostOrder(commit); + createPostOrder(); // Construct our response object return getStatisticsMap(importRows); @@ -428,11 +429,11 @@ private boolean canUpdatePedigree(BrAPIGermplasm existingGermplasm, Germplasm ge } private boolean hasPedigreeString(BrAPIGermplasm germplasm) { - return StringUtils.isNotBlank(germplasm.getPedigree()); + return isNotBlank(germplasm.getPedigree()); } private boolean hasPedigree(BrAPIGermplasm germplasm) { - return StringUtils.isNotBlank(germplasm.getPedigree()) + return isNotBlank(germplasm.getPedigree()) || germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_GID) || germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_GID) || (germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN) && @@ -458,9 +459,9 @@ private boolean arePedigreesEqual(BrAPIGermplasm existingGermplasm, Germplasm ge String existingMalePedigree = getParentId(existingGermplasm, existingPedigreeGIDString, BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_GID, BrAPIAdditionalInfoFields.MALE_PARENT_UNKNOWN); StringBuilder germplasmPedigreeGIDString = new StringBuilder(); - if (StringUtils.isNotBlank(germplasm.getFemaleParentAccessionNumber())) { + if (isNotBlank(germplasm.getFemaleParentAccessionNumber())) { germplasmPedigreeGIDString.append(germplasm.getFemaleParentAccessionNumber()); - } else if (StringUtils.isNotBlank(germplasm.getFemaleParentEntryNo())) { + } else if (isNotBlank(germplasm.getFemaleParentEntryNo())) { Integer femaleParentIdx = germplasmIndexByEntryNo.get(germplasm.getFemaleParentEntryNo()); BrAPIImport femaleParentRow = importRows.get(femaleParentIdx); BrAPIGermplasm femaleGerm = dbGermplasmByName.get(femaleParentRow.getGermplasm() @@ -474,9 +475,9 @@ private boolean arePedigreesEqual(BrAPIGermplasm existingGermplasm, Germplasm ge germplasmPedigreeGIDString.append(existingFemalePedigree); } germplasmPedigreeGIDString.append("/"); - if (StringUtils.isNotBlank(germplasm.getMaleParentAccessionNumber())) { + if (isNotBlank(germplasm.getMaleParentAccessionNumber())) { germplasmPedigreeGIDString.append(germplasm.getMaleParentAccessionNumber()); - } else if (StringUtils.isNotBlank(germplasm.getMaleParentEntryNo())) { + } else if (isNotBlank(germplasm.getMaleParentEntryNo())) { Integer maleParentIdx = germplasmIndexByEntryNo.get(germplasm.getMaleParentEntryNo()); BrAPIImport maleParentRow = importRows.get(maleParentIdx); BrAPIGermplasm maleGerm = dbGermplasmByName.get(maleParentRow.getGermplasm() @@ -518,7 +519,7 @@ private String getParentId(BrAPIGermplasm existingGermplasm, StringBuilder pedig private boolean canUpdatePedigreeNoEqualsCheck(BrAPIGermplasm existingGermplasm, Germplasm germplasm) { - return StringUtils.isBlank(existingGermplasm.getPedigree()) && + return isBlank(existingGermplasm.getPedigree()) && germplasm.pedigreeExists(); } @@ -550,25 +551,62 @@ private void validatePedigree(Germplasm germplasm, Integer rowNumber, Validation String femaleParentGID = germplasm.getFemaleParentAccessionNumber(); String maleParentGID = germplasm.getMaleParentAccessionNumber(); - if(StringUtils.isNotBlank(maleParentEntryNo) && StringUtils.isBlank(femaleParentEntryNo) && StringUtils.isBlank(femaleParentGID)) { + if(isNotBlank(maleParentEntryNo) && isBlank(femaleParentEntryNo) && isBlank(femaleParentGID)) { validationErrors.addError(rowNumber, new ValidationError("Male Parent Entry No", missingFemaleParent, HttpStatus.UNPROCESSABLE_ENTITY)); - } else if(StringUtils.isNotBlank(maleParentGID) && StringUtils.isBlank(femaleParentEntryNo) && StringUtils.isBlank(femaleParentGID)) { + } else if(isNotBlank(maleParentGID) && isBlank(femaleParentEntryNo) && isBlank(femaleParentGID)) { validationErrors.addError(rowNumber, new ValidationError("Male Parent GID", missingFemaleParent, HttpStatus.UNPROCESSABLE_ENTITY)); } } + private String getImportId(BrAPIGermplasm germplasm) { + String gid = germplasm.getAccessionNumber(); + String entryNo = germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_IMPORT_ENTRY_NUMBER).getAsString(); + return generateImportId(gid, entryNo); + } + + private String getMotherImportId(BrAPIGermplasm germplasm) { + JsonElement motherGidElement = germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_GID); + JsonElement motherEntryNoElement = germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_ENTRY_NO); + String motherGid = !motherGidElement.isJsonNull() ? motherGidElement.getAsString() : null; + String motherEntryNo = !motherEntryNoElement.isJsonNull() ? motherEntryNoElement.getAsString() : null; + return generateImportId(motherGid, motherEntryNo); + } + + private String getFatherImportId(BrAPIGermplasm germplasm) { + JsonElement fatherGidElement = germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_GID); + JsonElement fatherEntryNoElement = germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_ENTRY_NO); + String fatherGid = !fatherGidElement.isJsonNull() ? fatherGidElement.getAsString() : null; + String fatherEntryNo = !fatherEntryNoElement.isJsonNull() ? fatherEntryNoElement.getAsString() : null; + return generateImportId(fatherGid, fatherEntryNo); + } + + private String generateImportId(String gid, String entryNo) { + if (gid == null && entryNo == null) return null; + return isNotBlank(gid) ? "GID " + gid : "ENTRY NO " + entryNo; + } + + private boolean maleParentPresent(BrAPIGermplasm germplasm) { + boolean fatherGidNull = germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_GID).isJsonNull(); + boolean fatherEntryNoNull = germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_ENTRY_NO).isJsonNull(); + return !fatherGidNull || !fatherEntryNoNull; + } + + private boolean femaleParentUnknown(BrAPIGermplasm germplasm) { + return germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN).getAsBoolean(); + } + + private boolean maleParentUnknown(BrAPIGermplasm germplasm) { + return germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.MALE_PARENT_UNKNOWN).getAsBoolean(); + } + /* This will set the postOrder and validate for circular pedigree dependencies. */ - private void createPostOrder(boolean commit) { + private void createPostOrder() { + Set created = null; // Construct a dependency tree for POSTing order - if(commit){ - created = existingGermplasm.stream().map(BrAPIGermplasm::getGermplasmName).collect(Collectors.toSet()); - } - else { - created = existingGermplasm.stream().map(BrAPIGermplasm::getDefaultDisplayName).collect(Collectors.toSet()); - } + created = existingGermplasm.stream().map(this::getImportId).collect(Collectors.toSet()); //todo this gets messy @@ -579,7 +617,7 @@ private void createPostOrder(boolean commit) { for (BrAPIGermplasm germplasm : newGermplasmList) { // If we've already planned this germplasm, skip - if ( (commit && created.contains(germplasm.getGermplasmName())) || (!commit && created.contains(germplasm.getDefaultDisplayName())) ) { + if (created.contains(getImportId(germplasm))) { continue; } @@ -589,8 +627,19 @@ private void createPostOrder(boolean commit) { continue; } + String femaleImportId = getMotherImportId(germplasm); + String maleImportId = getFatherImportId(germplasm); + + if (created.contains(femaleImportId) || femaleParentUnknown(germplasm)) { + if (!maleParentPresent(germplasm) || created.contains(maleImportId) || maleParentUnknown(germplasm)) { + createList.add(germplasm); + } + } + + /* // If both parents have been created already, add it List pedigreeArray = List.of(germplasm.getPedigree().split("/")); + // name + gid or name + entry no if no gid String femaleParent = pedigreeArray.get(0); String maleParent = pedigreeArray.size() > 1 ? pedigreeArray.get(1) : null; if (created.contains(femaleParent) || germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN).getAsBoolean()) { @@ -598,11 +647,12 @@ private void createPostOrder(boolean commit) { createList.add(germplasm); } } + */ } totalRecorded += createList.size(); if (createList.size() > 0) { - created.addAll(createList.stream().map(BrAPIGermplasm::getGermplasmName).collect(Collectors.toList())); + created.addAll(createList.stream().map(this::getImportId).collect(Collectors.toList())); postOrder.add(createList); } else if (totalRecorded < newGermplasmList.size()) { // We ran into circular dependencies, throw an error diff --git a/src/test/resources/files/germplasm_import/duplicate_names_circular_dependency.csv b/src/test/resources/files/germplasm_import/duplicate_names_circular_dependency.csv new file mode 100644 index 000000000..a602f6233 --- /dev/null +++ b/src/test/resources/files/germplasm_import/duplicate_names_circular_dependency.csv @@ -0,0 +1,6 @@ +GID,Germplasm Name,Breeding Method,Source,Female Parent GID,Male Parent GID,Entry No,Female Parent Entry No,Male Parent Entry No,External UID,Synonyms +,TestDup,BCR,Test,1,2,1,,,1, +,TestDup,BCR,Test,1,,2,,3,2, +,TestDup,BCR,Test,,,3,,,3, +,TestDup,BCR,Test,,,4,,,4, +,TestDup,BCR,Test,,,5,3,4,5, \ No newline at end of file From 8a134c345160b532096a55cf949af10a7113c9ba Mon Sep 17 00:00:00 2001 From: Nick <53413353+nickpalladino@users.noreply.github.com> Date: Tue, 8 Apr 2025 11:02:39 -0400 Subject: [PATCH 2/4] Refactored to clean up a bit --- .../germplasm/GermplasmImportService.java | 7 +- .../germplasm/GermplasmImportIdUtils.java | 94 +++++++++++++++++++ .../{ => germplasm}/GermplasmProcessor.java | 91 ++++-------------- .../importer/GermplasmFileImportTest.java | 3 +- 4 files changed, 114 insertions(+), 81 deletions(-) create mode 100644 src/main/java/org/breedinginsight/brapps/importer/services/processors/germplasm/GermplasmImportIdUtils.java rename src/main/java/org/breedinginsight/brapps/importer/services/processors/{ => germplasm}/GermplasmProcessor.java (89%) diff --git a/src/main/java/org/breedinginsight/brapps/importer/model/imports/germplasm/GermplasmImportService.java b/src/main/java/org/breedinginsight/brapps/importer/model/imports/germplasm/GermplasmImportService.java index 0caebe65e..64d4c3b23 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/model/imports/germplasm/GermplasmImportService.java +++ b/src/main/java/org/breedinginsight/brapps/importer/model/imports/germplasm/GermplasmImportService.java @@ -18,18 +18,13 @@ package org.breedinginsight.brapps.importer.model.imports.germplasm; import lombok.extern.slf4j.Slf4j; -import org.breedinginsight.brapps.importer.model.ImportUpload; -import org.breedinginsight.brapps.importer.model.imports.BrAPIImport; import org.breedinginsight.brapps.importer.model.imports.BrAPIImportService; import org.breedinginsight.brapps.importer.model.imports.ImportServiceContext; import org.breedinginsight.brapps.importer.model.response.ImportPreviewResponse; import org.breedinginsight.brapps.importer.model.workflow.ImportWorkflow; -import org.breedinginsight.brapps.importer.services.processors.GermplasmProcessor; +import org.breedinginsight.brapps.importer.services.processors.germplasm.GermplasmProcessor; import org.breedinginsight.brapps.importer.services.processors.Processor; import org.breedinginsight.brapps.importer.services.processors.ProcessorManager; -import org.breedinginsight.model.Program; -import org.breedinginsight.model.User; -import tech.tablesaw.api.Table; import javax.inject.Inject; import javax.inject.Provider; diff --git a/src/main/java/org/breedinginsight/brapps/importer/services/processors/germplasm/GermplasmImportIdUtils.java b/src/main/java/org/breedinginsight/brapps/importer/services/processors/germplasm/GermplasmImportIdUtils.java new file mode 100644 index 000000000..4fe10e0c7 --- /dev/null +++ b/src/main/java/org/breedinginsight/brapps/importer/services/processors/germplasm/GermplasmImportIdUtils.java @@ -0,0 +1,94 @@ +package org.breedinginsight.brapps.importer.services.processors.germplasm; + +import com.google.gson.JsonElement; +import org.apache.commons.lang3.StringUtils; +import org.brapi.v2.model.germ.BrAPIGermplasm; +import org.breedinginsight.brapi.v2.constants.BrAPIAdditionalInfoFields; + +/** + * Utility class for managing germplasm import identifiers and pedigree relationships. + */ +public class GermplasmImportIdUtils { + + private GermplasmImportIdUtils() { + // Private constructor to prevent instantiation + } + + /** + * Generates an import ID for a germplasm based on its GID or entry number. + * @param gid The germplasm ID + * @param entryNo The entry number + * @return The generated import ID or null if both parameters are null + */ + public static String generateImportId(String gid, String entryNo) { + if (gid == null && entryNo == null) return null; + return StringUtils.isNotBlank(gid) ? "GID " + gid : "ENTRY NO " + entryNo; + } + + /** + * Gets the import ID for a germplasm. + * @param germplasm The germplasm object + * @return The import ID + */ + public static String getImportId(BrAPIGermplasm germplasm) { + String gid = germplasm.getAccessionNumber(); + String entryNo = germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_IMPORT_ENTRY_NUMBER).getAsString(); + return generateImportId(gid, entryNo); + } + + /** + * Gets the import ID for the mother/female parent of a germplasm. + * @param germplasm The germplasm object + * @return The import ID of the mother/female parent + */ + public static String getMotherImportId(BrAPIGermplasm germplasm) { + JsonElement motherGidElement = germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_GID); + JsonElement motherEntryNoElement = germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_ENTRY_NO); + String motherGid = !motherGidElement.isJsonNull() ? motherGidElement.getAsString() : null; + String motherEntryNo = !motherEntryNoElement.isJsonNull() ? motherEntryNoElement.getAsString() : null; + return generateImportId(motherGid, motherEntryNo); + } + + /** + * Gets the import ID for the father/male parent of a germplasm. + * @param germplasm The germplasm object + * @return The import ID of the father/male parent + */ + public static String getFatherImportId(BrAPIGermplasm germplasm) { + JsonElement fatherGidElement = germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_GID); + JsonElement fatherEntryNoElement = germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_ENTRY_NO); + String fatherGid = !fatherGidElement.isJsonNull() ? fatherGidElement.getAsString() : null; + String fatherEntryNo = !fatherEntryNoElement.isJsonNull() ? fatherEntryNoElement.getAsString() : null; + return generateImportId(fatherGid, fatherEntryNo); + } + + /** + * Checks if a male parent is present for a germplasm. + * @param germplasm The germplasm object + * @return true if a male parent is present, false otherwise + */ + public static boolean maleParentPresent(BrAPIGermplasm germplasm) { + boolean fatherGidNull = germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_GID).isJsonNull(); + boolean fatherEntryNoNull = germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_ENTRY_NO).isJsonNull(); + return !fatherGidNull || !fatherEntryNoNull; + } + + /** + * Checks if the female parent is unknown for a germplasm. + * @param germplasm The germplasm object + * @return true if the female parent is unknown, false otherwise + */ + public static boolean femaleParentUnknown(BrAPIGermplasm germplasm) { + return germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN).getAsBoolean(); + } + + /** + * Checks if the male parent is unknown for a germplasm. + * @param germplasm The germplasm object + * @return true if the male parent is unknown, false otherwise + */ + public static boolean maleParentUnknown(BrAPIGermplasm germplasm) { + return germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.MALE_PARENT_UNKNOWN).getAsBoolean(); + } + +} \ No newline at end of file diff --git a/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java b/src/main/java/org/breedinginsight/brapps/importer/services/processors/germplasm/GermplasmProcessor.java similarity index 89% rename from src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java rename to src/main/java/org/breedinginsight/brapps/importer/services/processors/germplasm/GermplasmProcessor.java index b6987488c..d4d2389a8 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java +++ b/src/main/java/org/breedinginsight/brapps/importer/services/processors/germplasm/GermplasmProcessor.java @@ -14,7 +14,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.breedinginsight.brapps.importer.services.processors; +package org.breedinginsight.brapps.importer.services.processors.germplasm; import com.google.gson.Gson; import com.google.gson.JsonElement; @@ -43,6 +43,7 @@ import org.breedinginsight.brapps.importer.model.response.ImportObjectState; import org.breedinginsight.brapps.importer.model.response.ImportPreviewStatistics; import org.breedinginsight.brapps.importer.model.response.PendingImportObject; +import org.breedinginsight.brapps.importer.services.processors.Processor; import org.breedinginsight.dao.db.tables.pojos.ProgramBreedingMethodEntity; import org.breedinginsight.daos.BreedingMethodDAO; import org.breedinginsight.model.Program; @@ -59,9 +60,6 @@ import java.util.function.Supplier; import java.util.stream.Collectors; -import static org.apache.commons.lang3.StringUtils.isBlank; -import static org.apache.commons.lang3.StringUtils.isNotBlank; - @Slf4j @Prototype public class GermplasmProcessor implements Processor { @@ -429,11 +427,11 @@ private boolean canUpdatePedigree(BrAPIGermplasm existingGermplasm, Germplasm ge } private boolean hasPedigreeString(BrAPIGermplasm germplasm) { - return isNotBlank(germplasm.getPedigree()); + return StringUtils.isNotBlank(germplasm.getPedigree()); } private boolean hasPedigree(BrAPIGermplasm germplasm) { - return isNotBlank(germplasm.getPedigree()) + return StringUtils.isNotBlank(germplasm.getPedigree()) || germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_GID) || germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_GID) || (germplasm.getAdditionalInfo().has(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN) && @@ -459,9 +457,9 @@ private boolean arePedigreesEqual(BrAPIGermplasm existingGermplasm, Germplasm ge String existingMalePedigree = getParentId(existingGermplasm, existingPedigreeGIDString, BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_GID, BrAPIAdditionalInfoFields.MALE_PARENT_UNKNOWN); StringBuilder germplasmPedigreeGIDString = new StringBuilder(); - if (isNotBlank(germplasm.getFemaleParentAccessionNumber())) { + if (StringUtils.isNotBlank(germplasm.getFemaleParentAccessionNumber())) { germplasmPedigreeGIDString.append(germplasm.getFemaleParentAccessionNumber()); - } else if (isNotBlank(germplasm.getFemaleParentEntryNo())) { + } else if (StringUtils.isNotBlank(germplasm.getFemaleParentEntryNo())) { Integer femaleParentIdx = germplasmIndexByEntryNo.get(germplasm.getFemaleParentEntryNo()); BrAPIImport femaleParentRow = importRows.get(femaleParentIdx); BrAPIGermplasm femaleGerm = dbGermplasmByName.get(femaleParentRow.getGermplasm() @@ -475,9 +473,9 @@ private boolean arePedigreesEqual(BrAPIGermplasm existingGermplasm, Germplasm ge germplasmPedigreeGIDString.append(existingFemalePedigree); } germplasmPedigreeGIDString.append("/"); - if (isNotBlank(germplasm.getMaleParentAccessionNumber())) { + if (StringUtils.isNotBlank(germplasm.getMaleParentAccessionNumber())) { germplasmPedigreeGIDString.append(germplasm.getMaleParentAccessionNumber()); - } else if (isNotBlank(germplasm.getMaleParentEntryNo())) { + } else if (StringUtils.isNotBlank(germplasm.getMaleParentEntryNo())) { Integer maleParentIdx = germplasmIndexByEntryNo.get(germplasm.getMaleParentEntryNo()); BrAPIImport maleParentRow = importRows.get(maleParentIdx); BrAPIGermplasm maleGerm = dbGermplasmByName.get(maleParentRow.getGermplasm() @@ -519,7 +517,7 @@ private String getParentId(BrAPIGermplasm existingGermplasm, StringBuilder pedig private boolean canUpdatePedigreeNoEqualsCheck(BrAPIGermplasm existingGermplasm, Germplasm germplasm) { - return isBlank(existingGermplasm.getPedigree()) && + return StringUtils.isBlank(existingGermplasm.getPedigree()) && germplasm.pedigreeExists(); } @@ -551,54 +549,13 @@ private void validatePedigree(Germplasm germplasm, Integer rowNumber, Validation String femaleParentGID = germplasm.getFemaleParentAccessionNumber(); String maleParentGID = germplasm.getMaleParentAccessionNumber(); - if(isNotBlank(maleParentEntryNo) && isBlank(femaleParentEntryNo) && isBlank(femaleParentGID)) { + if(StringUtils.isNotBlank(maleParentEntryNo) && StringUtils.isBlank(femaleParentEntryNo) && StringUtils.isBlank(femaleParentGID)) { validationErrors.addError(rowNumber, new ValidationError("Male Parent Entry No", missingFemaleParent, HttpStatus.UNPROCESSABLE_ENTITY)); - } else if(isNotBlank(maleParentGID) && isBlank(femaleParentEntryNo) && isBlank(femaleParentGID)) { + } else if(StringUtils.isNotBlank(maleParentGID) && StringUtils.isBlank(femaleParentEntryNo) && StringUtils.isBlank(femaleParentGID)) { validationErrors.addError(rowNumber, new ValidationError("Male Parent GID", missingFemaleParent, HttpStatus.UNPROCESSABLE_ENTITY)); } } - private String getImportId(BrAPIGermplasm germplasm) { - String gid = germplasm.getAccessionNumber(); - String entryNo = germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_IMPORT_ENTRY_NUMBER).getAsString(); - return generateImportId(gid, entryNo); - } - - private String getMotherImportId(BrAPIGermplasm germplasm) { - JsonElement motherGidElement = germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_GID); - JsonElement motherEntryNoElement = germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_ENTRY_NO); - String motherGid = !motherGidElement.isJsonNull() ? motherGidElement.getAsString() : null; - String motherEntryNo = !motherEntryNoElement.isJsonNull() ? motherEntryNoElement.getAsString() : null; - return generateImportId(motherGid, motherEntryNo); - } - - private String getFatherImportId(BrAPIGermplasm germplasm) { - JsonElement fatherGidElement = germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_GID); - JsonElement fatherEntryNoElement = germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_ENTRY_NO); - String fatherGid = !fatherGidElement.isJsonNull() ? fatherGidElement.getAsString() : null; - String fatherEntryNo = !fatherEntryNoElement.isJsonNull() ? fatherEntryNoElement.getAsString() : null; - return generateImportId(fatherGid, fatherEntryNo); - } - - private String generateImportId(String gid, String entryNo) { - if (gid == null && entryNo == null) return null; - return isNotBlank(gid) ? "GID " + gid : "ENTRY NO " + entryNo; - } - - private boolean maleParentPresent(BrAPIGermplasm germplasm) { - boolean fatherGidNull = germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_GID).isJsonNull(); - boolean fatherEntryNoNull = germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_ENTRY_NO).isJsonNull(); - return !fatherGidNull || !fatherEntryNoNull; - } - - private boolean femaleParentUnknown(BrAPIGermplasm germplasm) { - return germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN).getAsBoolean(); - } - - private boolean maleParentUnknown(BrAPIGermplasm germplasm) { - return germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.MALE_PARENT_UNKNOWN).getAsBoolean(); - } - /* This will set the postOrder and validate for circular pedigree dependencies. */ @@ -606,7 +563,7 @@ private void createPostOrder() { Set created = null; // Construct a dependency tree for POSTing order - created = existingGermplasm.stream().map(this::getImportId).collect(Collectors.toSet()); + created = existingGermplasm.stream().map(GermplasmImportIdUtils::getImportId).collect(Collectors.toSet()); //todo this gets messy @@ -617,7 +574,7 @@ private void createPostOrder() { for (BrAPIGermplasm germplasm : newGermplasmList) { // If we've already planned this germplasm, skip - if (created.contains(getImportId(germplasm))) { + if (created.contains(GermplasmImportIdUtils.getImportId(germplasm))) { continue; } @@ -627,32 +584,20 @@ private void createPostOrder() { continue; } - String femaleImportId = getMotherImportId(germplasm); - String maleImportId = getFatherImportId(germplasm); + String femaleImportId = GermplasmImportIdUtils.getMotherImportId(germplasm); + String maleImportId = GermplasmImportIdUtils.getFatherImportId(germplasm); - if (created.contains(femaleImportId) || femaleParentUnknown(germplasm)) { - if (!maleParentPresent(germplasm) || created.contains(maleImportId) || maleParentUnknown(germplasm)) { + if (created.contains(femaleImportId) || GermplasmImportIdUtils.femaleParentUnknown(germplasm)) { + if (!GermplasmImportIdUtils.maleParentPresent(germplasm) || created.contains(maleImportId) || GermplasmImportIdUtils.maleParentUnknown(germplasm)) { createList.add(germplasm); } } - /* - // If both parents have been created already, add it - List pedigreeArray = List.of(germplasm.getPedigree().split("/")); - // name + gid or name + entry no if no gid - String femaleParent = pedigreeArray.get(0); - String maleParent = pedigreeArray.size() > 1 ? pedigreeArray.get(1) : null; - 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); - } - } - */ } totalRecorded += createList.size(); if (createList.size() > 0) { - created.addAll(createList.stream().map(this::getImportId).collect(Collectors.toList())); + created.addAll(createList.stream().map(GermplasmImportIdUtils::getImportId).collect(Collectors.toList())); postOrder.add(createList); } else if (totalRecorded < newGermplasmList.size()) { // We ran into circular dependencies, throw an error diff --git a/src/test/java/org/breedinginsight/brapps/importer/GermplasmFileImportTest.java b/src/test/java/org/breedinginsight/brapps/importer/GermplasmFileImportTest.java index d248d42ad..a9985273b 100644 --- a/src/test/java/org/breedinginsight/brapps/importer/GermplasmFileImportTest.java +++ b/src/test/java/org/breedinginsight/brapps/importer/GermplasmFileImportTest.java @@ -20,7 +20,7 @@ import org.breedinginsight.brapps.importer.model.imports.germplasm.GermplasmImportService; import org.breedinginsight.brapps.importer.model.response.ImportObjectState; import org.breedinginsight.brapps.importer.services.ExternalReferenceSource; -import org.breedinginsight.brapps.importer.services.processors.GermplasmProcessor; +import org.breedinginsight.brapps.importer.services.processors.germplasm.GermplasmProcessor; import org.breedinginsight.dao.db.tables.pojos.BiUserEntity; import org.breedinginsight.dao.db.tables.pojos.ProgramBreedingMethodEntity; import org.breedinginsight.daos.BreedingMethodDAO; @@ -32,7 +32,6 @@ import org.junit.jupiter.api.*; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; -import tech.tablesaw.api.Row; import tech.tablesaw.api.Table; import javax.inject.Inject; From aa4a43eec0de4418a717484d4f9dcd96a23efe7e Mon Sep 17 00:00:00 2001 From: Nick <53413353+nickpalladino@users.noreply.github.com> Date: Tue, 8 Apr 2025 11:06:57 -0400 Subject: [PATCH 3/4] Added file header --- .../germplasm/GermplasmImportIdUtils.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/main/java/org/breedinginsight/brapps/importer/services/processors/germplasm/GermplasmImportIdUtils.java b/src/main/java/org/breedinginsight/brapps/importer/services/processors/germplasm/GermplasmImportIdUtils.java index 4fe10e0c7..5c695aaee 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/services/processors/germplasm/GermplasmImportIdUtils.java +++ b/src/main/java/org/breedinginsight/brapps/importer/services/processors/germplasm/GermplasmImportIdUtils.java @@ -1,3 +1,19 @@ +/* + * See the NOTICE file distributed with this work for additional information + * regarding copyright ownership. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ package org.breedinginsight.brapps.importer.services.processors.germplasm; import com.google.gson.JsonElement; From 8610b9a5849e0ce306e07d30abc3fbc1ff0aeaac Mon Sep 17 00:00:00 2001 From: Nick <53413353+nickpalladino@users.noreply.github.com> Date: Tue, 8 Apr 2025 11:45:37 -0400 Subject: [PATCH 4/4] Added test case --- .../importer/GermplasmFileImportTest.java | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/test/java/org/breedinginsight/brapps/importer/GermplasmFileImportTest.java b/src/test/java/org/breedinginsight/brapps/importer/GermplasmFileImportTest.java index a9985273b..13d60a714 100644 --- a/src/test/java/org/breedinginsight/brapps/importer/GermplasmFileImportTest.java +++ b/src/test/java/org/breedinginsight/brapps/importer/GermplasmFileImportTest.java @@ -786,6 +786,32 @@ public void entryNoDescending(boolean commit) { checkEntryNoFields(fileData, previewRows, commit, listName, listDescription, "EntryNoDescGerm 2", "EntryNoDescGerm 1"); } + /** + * Prior to BI-2593 this file would result in a false positive circular dependency error. This was due to germplasm + * references not being unique in the preview and commit phases of the postOrder method. + * + * @param commit controls whether import is preview or commit + */ + @Order(7) // want some existing gids to reference + @ParameterizedTest + @ValueSource(booleans = {false, true}) + @SneakyThrows + public void duplicateNames(boolean commit) { + String pathname = "src/test/resources/files/germplasm_import/duplicate_names_circular_dependency.csv"; + Table fileData = Table.read().file(pathname); + String listName = "DuplicateNames"; + String listDescription = "Duplicate names with pedigree"; + + JsonObject result = importGermplasm(pathname, listName, listDescription, commit); + assertEquals(200, result.getAsJsonObject("progress").get("statuscode").getAsInt()); + + // preview table is sorted by entry number + fileData = fileData.sortAscendingOn("Entry No"); + + JsonArray previewRows = result.get("preview").getAsJsonObject().get("rows").getAsJsonArray(); + checkEntryNoFields(fileData, previewRows, commit, listName, listDescription, "TestDup", "TestDup"); + } + /** * Shared method to perform entry number order tests for preview and commit *