BI-2593 - Circular Dependency False Positive Error

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;

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();
+    }
+
+}

src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java → 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<String, ImportPreviewStatistics> process(ImportUpload upload, List<Br
 
         // Construct pedigree
         constructPedigreeString(importRows, mappedBrAPIImport, commit);
-
-        // for commit:  Construct a dependency tree for POSTing order. Dependents on unique germplasm name, (<Name> [<Program Key> - <Accession Number>])
-        // 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<String> 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<String> 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

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
      *

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,