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..5c695aaee --- /dev/null +++ b/src/main/java/org/breedinginsight/brapps/importer/services/processors/germplasm/GermplasmImportIdUtils.java @@ -0,0 +1,110 @@ +/* + * 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; +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 96% 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 aad1720d9..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,9 +14,10 @@ * 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; import io.micronaut.context.annotation.Property; import io.micronaut.context.annotation.Prototype; import io.micronaut.http.HttpStatus; @@ -42,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; @@ -327,10 +329,7 @@ public Map process(ImportUpload upload, List
[ - ]) - // for !commit: Validate for circular pedigree dependencies. - createPostOrder(commit); + createPostOrder(); // Construct our response object return getStatisticsMap(importRows); @@ -560,15 +559,11 @@ private void validatePedigree(Germplasm germplasm, Integer rowNumber, Validation /* 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(GermplasmImportIdUtils::getImportId).collect(Collectors.toSet()); //todo this gets messy @@ -579,7 +574,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(GermplasmImportIdUtils.getImportId(germplasm))) { continue; } @@ -589,20 +584,20 @@ private void createPostOrder(boolean commit) { continue; } - // If both parents have been created already, add it - 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(BrAPIAdditionalInfoFields.FEMALE_PARENT_UNKNOWN).getAsBoolean()) { - if (maleParent == null || created.contains(maleParent) || germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.MALE_PARENT_UNKNOWN).getAsBoolean()) { + String femaleImportId = GermplasmImportIdUtils.getMotherImportId(germplasm); + String maleImportId = GermplasmImportIdUtils.getFatherImportId(germplasm); + + if (created.contains(femaleImportId) || GermplasmImportIdUtils.femaleParentUnknown(germplasm)) { + if (!GermplasmImportIdUtils.maleParentPresent(germplasm) || created.contains(maleImportId) || GermplasmImportIdUtils.maleParentUnknown(germplasm)) { 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(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..13d60a714 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; @@ -787,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 * 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