Skip to content

[BI-2751] - Germplasm entry number weirdness #494

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
HMS17 wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

[BI-2751] - Germplasm entry number weirdness #494

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1620a76
[BI-2751] - Germplasm entry number weirdness
HMS17 Feb 3, 2026
0317e6d
[BI-2751] - fix parental accession no check
HMS17 Feb 9, 2026
3663007
Merge branch 'develop' into bug/BI-2751
HMS17 Feb 9, 2026
e0b7e91
Merge branch 'develop' into bug/BI-2751
nickpalladino Feb 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 63 additions & 48 deletions ...org/breedinginsight/brapps/importer/services/processors/germplasm/GermplasmProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ public class GermplasmProcessor implements Processor {

public static String missingGIDsMsg = "The following GIDs were not found in the database: %s";
public static String missingParentalGIDsMsg = "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 missingParentalEntryNoMsg = "The following parental entry numbers were not found in the file: %s";
public static String badBreedMethodsMsg = "Invalid breeding method";
public static String badGermplasmNameMsg = "Germplasm name cannot contain /";
public static String missingEntryNumbersMsg = "Either all or none of the germplasm must have entry numbers";
Expand Down Expand Up @@ -137,14 +137,6 @@ public void getExistingBrapiData(List<BrAPIImport> importRows, Program program)
BrAPIImport germplasmImport = importRows.get(i);
Germplasm germplasm = germplasmImport.getGermplasm();
if (germplasm != null) {

// Retrieve parent accession numbers to assess if already in db
if (germplasm.getFemaleParentAccessionNumber() != null) {
germplasmAccessionNumbers.put(germplasm.getFemaleParentAccessionNumber(), true);
}
if (germplasm.getMaleParentAccessionNumber() != null) {
germplasmAccessionNumbers.put(germplasm.getMaleParentAccessionNumber(), true);
}
if (germplasm.getAccessionNumber() != null) {
germplasmAccessionNumbers.put(germplasm.getAccessionNumber(), false);
}
Expand All @@ -159,6 +151,31 @@ public void getExistingBrapiData(List<BrAPIImport> importRows, Program program)
}
}

// Get existing germplasm names
List<BrAPIGermplasm> dbGermplasm = brAPIGermplasmService.getGermplasmByDisplayName(new ArrayList<>(fileGermplasmByName.keySet()), program.getId());
dbGermplasm.forEach(germplasm -> {
dbGermplasmByName.put(germplasm.getDefaultDisplayName(), germplasm);
dbGermplasmByAccessionNo.put(germplasm.getAccessionNumber(), germplasm);
});

// Get parental accession nos in file
for (int i = 0; i < importRows.size(); i++) {
BrAPIImport germplasmImport = importRows.get(i);
Germplasm germplasm = germplasmImport.getGermplasm();
if (germplasm != null) {
//Ignore this if germplasm already has a pedigree in the database
// Retrieve parent accession numbers to assess if already in db
if (!databaseGermplasmHasPedigree(germplasm)) {
if (germplasm.getFemaleParentAccessionNumber() != null) {
germplasmAccessionNumbers.put(germplasm.getFemaleParentAccessionNumber(), true);
}
if (germplasm.getMaleParentAccessionNumber() != null) {
germplasmAccessionNumbers.put(germplasm.getMaleParentAccessionNumber(), true);
}
}
}
}

// If a parental accession number is present, it should exist in the database.
existingGermplasm = new ArrayList<>();
List<String> missingParentalAccessionNumbers = germplasmAccessionNumbers.entrySet().stream().filter(Map.Entry::getValue).map(Map.Entry::getKey).collect(Collectors.toList());
Expand All @@ -181,13 +198,6 @@ public void getExistingBrapiData(List<BrAPIImport> importRows, Program program)
}
}

// Get existing germplasm names
List<BrAPIGermplasm> dbGermplasm = brAPIGermplasmService.getGermplasmByDisplayName(new ArrayList<>(fileGermplasmByName.keySet()), program.getId());
dbGermplasm.forEach(germplasm -> {
dbGermplasmByName.put(germplasm.getDefaultDisplayName(), germplasm);
dbGermplasmByAccessionNo.put(germplasm.getAccessionNumber(), germplasm);
});

// Check for existing germplasm lists
Boolean listNameDup = false;
if (importRows.size() > 0 && importRows.get(0).getGermplasm().getListName() != null) {
Expand Down Expand Up @@ -223,27 +233,31 @@ public void getExistingBrapiData(List<BrAPIImport> importRows, Program program)
arrayOfStringFormatter.apply(missingAccessionNumbers)));
}

List<String> missingEntryNumbers = new ArrayList<>();
for (BrAPIImport importRow: importRows) {
Germplasm germplasm = importRow.getGermplasm();
// Check Female Parent
if (germplasm.getFemaleParentEntryNo() != null) {
if ((!germplasmIndexByEntryNo.containsKey(germplasm.getFemaleParentEntryNo())) && !(germplasm.getFemaleParentEntryNo().equals("0"))) {
missingEntryNumbers.add(germplasm.getFemaleParentEntryNo());
List<String> missingEntryNumbers = new ArrayList<>();
for (BrAPIImport importRow : importRows) {
Germplasm germplasm = importRow.getGermplasm();

//If germplasm already has a pedigree, pedigree cannot be overwritten and file values for pedigree will be ignored
boolean pedigreeExists = databaseGermplasmHasPedigree(germplasm);

// Check Female Parent
if (germplasm.getFemaleParentEntryNo() != null && !pedigreeExists) {
if ((!germplasmIndexByEntryNo.containsKey(germplasm.getFemaleParentEntryNo())) && !(germplasm.getFemaleParentEntryNo().equals("0"))) {
missingEntryNumbers.add(germplasm.getFemaleParentEntryNo());
}
}
}
// Check Male Parent
if (germplasm.getMaleParentEntryNo() != null) {
if ((!germplasmIndexByEntryNo.containsKey(germplasm.getMaleParentEntryNo())) && !(germplasm.getMaleParentEntryNo().equals("0"))) {
missingEntryNumbers.add(germplasm.getMaleParentEntryNo());
// Check Male Parent
if (germplasm.getMaleParentEntryNo() != null && !pedigreeExists) {
if ((!germplasmIndexByEntryNo.containsKey(germplasm.getMaleParentEntryNo())) && !(germplasm.getMaleParentEntryNo().equals("0"))) {
missingEntryNumbers.add(germplasm.getMaleParentEntryNo());
}
}
}
}
if (missingEntryNumbers.size() > 0) {
throw new HttpStatusException(HttpStatus.UNPROCESSABLE_ENTITY,
String.format(missingParentalEntryNoMsg,
arrayOfStringFormatter.apply(missingEntryNumbers)));
}
if (missingEntryNumbers.size() > 0) {
throw new HttpStatusException(HttpStatus.UNPROCESSABLE_ENTITY,
String.format(missingParentalEntryNoMsg,
arrayOfStringFormatter.apply(missingEntryNumbers)));
}

if (listNameDup) {
throw new HttpStatusException(HttpStatus.UNPROCESSABLE_ENTITY, listNameAlreadyExists);
Expand Down Expand Up @@ -405,21 +419,8 @@ private boolean processExistingGermplasm(Germplasm germplasm, ValidationErrors v
return false;
}

// Error conditions:
Copy link
Member

@nickpalladino nickpalladino Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing this to silently ignore pedigree conflicts instead of rejecting conflicting updates seems like it could be misleading to the user. Was there a reason for removing this?

// 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(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;
}
}

// if no existing pedigree and file has pedigree then validate and update
// if pedigree exists, file pedigree information should be ignored
if(germplasm.pedigreeExists() && !hasPedigree(existingGermplasm)) {
validatePedigree(germplasm, rowIndex + 2, validationErrors);
updatePedigree = true;
Expand Down Expand Up @@ -460,6 +461,16 @@ private boolean hasPedigree(BrAPIGermplasm germplasm) {
germplasm.getAdditionalInfo().get(BrAPIAdditionalInfoFields.MALE_PARENT_UNKNOWN).getAsBoolean());
}

//Used to check if germplasm already has a pedigree in the database, if so, pedigree information in file should be ignored
private boolean databaseGermplasmHasPedigree(Germplasm germplasm) {
if (germplasm.getAccessionNumber() == null || dbGermplasmByAccessionNo.get(germplasm.getAccessionNumber()) == null) {
return false;
} else {
BrAPIGermplasm dbGermplasm = dbGermplasmByAccessionNo.get(germplasm.getAccessionNumber());
return hasPedigree(dbGermplasm);
}
}

/**
* Compare an existing germplasm's pedigree to the incoming germplasm's pedigree to ensure they are the same.<br><br>
* Assumes that an empty value for a given parent in the incoming germplasm is equal to the existing germplasm.<br><br>
Expand Down Expand Up @@ -793,6 +804,8 @@ else if (germplasmIndexByEntryNo.containsKey(germplasm.getFemaleParentEntryNo())
if (commit) {
if (femaleParentFound) {
brAPIGermplasm.putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_GID, femaleParent.getAccessionNumber());
//entry number no longer needed for figuring out parentage, can remove (since same germplasm can have different entry numbers across multiple lists)
brAPIGermplasm.putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_FEMALE_PARENT_ENTRY_NO, null);
Copy link
Member

@nickpalladino nickpalladino Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw the comment in the PR description about these still showing up without a migration for old records. Is that something we need a card for?

// Add femaleParentUUID to additionalInfo.
Optional<BrAPIExternalReference> femaleParentUUID = Utilities.getExternalReference(femaleParent.getExternalReferences(), BRAPI_REFERENCE_SOURCE);
if (femaleParentUUID.isPresent()) {
Expand All @@ -802,6 +815,8 @@ else if (germplasmIndexByEntryNo.containsKey(germplasm.getFemaleParentEntryNo())

if (maleParent != null) {
brAPIGermplasm.putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_GID, maleParent.getAccessionNumber());
//entry number no longer needed for figuring out parentage, can remove (since same germplasm can have different entry numbers across multiple lists)
brAPIGermplasm.putAdditionalInfoItem(BrAPIAdditionalInfoFields.GERMPLASM_MALE_PARENT_ENTRY_NO, null);
// Add maleParentUUID to additionalInfo.
Optional<BrAPIExternalReference> maleParentUUID = Utilities.getExternalReference(maleParent.getExternalReferences(), BRAPI_REFERENCE_SOURCE);
if (maleParentUUID.isPresent()) {
Expand Down