[BI-2046] Overwrite observations happening without reference to ObsUnitID by dmeidlin · Pull Request #332 · Breeding-Insight/bi-api

dmeidlin · 2024-02-14T16:32:53Z

Description

Story: BI-2046

Validation errors are now returned for display in the experiment import preview for any rows referring to an existing environment and with the ObsUnitID blank.

Dependencies

bi-web release/0.9

Testing

Verify all tests pass in ExperimentFileImportTest

import an experiment with observations
download the exp data with ObsUnitId populated
select multiple observations for updating and change phenotype values
import and verify updates by viewing experiment details

Checklist:

I have performed a self-review of my own code
I have tested my code and ensured it meets the acceptance criteria of the story
I have tested that my code works with both the brapi-java-server and BreedBase
I have create/modified unit tests to cover this change
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to documentation
I have run TAF: <please include a link to TAF run>

nickpalladino

Did @shawnyarnes want the Observation units to be looked up based on the ObsUnitID? It looks like in fetchOrCreateObsUnitPIO there is dependence on study information. I think ultimately she wanted updates to only rely on the ObsUnitID and none of the other data fields in the file.

If I try to update with a file like this:
exp4_download_obsunitupdate.xls

I get an exception:

java.lang.NumberFormatException: null
	at java.base/java.lang.Integer.parseInt(Integer.java:620)
	at java.base/java.lang.Integer.parseInt(Integer.java:776)
	at org.breedinginsight.brapps.importer.services.processors.ExperimentProcessor.yearToSeasonDbIdFromDatabase(ExperimentProcessor.java:1826)
	at org.breedinginsight.brapps.importer.services.processors.ExperimentProcessor.yearToSeasonDbId(ExperimentProcessor.java:1796)
	at org.breedinginsight.brapps.importer.services.processors.ExperimentProcessor.initNewBrapiData(ExperimentProcessor.java:560)
	at org.breedinginsight.brapps.importer.services.processors.ExperimentProcessor.process(ExperimentProcessor.java:233)
	at org.breedinginsight.brapps.importer.services.processors.ProcessorManager.process(ProcessorManager.java:63)
	at org.breedinginsight.brapps.importer.model.imports.experimentObservation.ExperimentImportService.process(ExperimentImportService.java:74)
	at org.breedinginsight.brapps.importer.services.FileImportService.lambda$processFile$9(FileImportService.java:427)
	at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1771)
	at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.exec(CompletableFuture.java:1763)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1016)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1665)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1598)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:183)

mlm483

Tested, working. Looks good, the only critical change is the error message text. Other than that I had a few thoughts and clarifying questions.

Error Message

There is duplication of "Import Cannot Proceed". See screenshot in one of my comments on ExperimentProcessor.java.
The error is a simple banner error stating "Experimental entities are missing ObsUnitIDs". I don't have an issue with this, but it wasn't clear if the error is intended to indicate row numbers or not. Would defer to you and Shawn on this.

"Reference" Observation Unit

Code and comments use the term Reference Observation Unit, could you clarify what "Reference" means in this context?

mlm483 · 2024-04-02T16:45:47Z

src/main/java/org/breedinginsight/brapps/importer/services/processors/ExperimentProcessor.java


    private static final String NAME = "Experiment";
-    private static final String MISSING_OBS_UNIT_ID_ERROR = "Experimental entities are missing ObsUnitIDs";
+    private static final String MISSING_OBS_UNIT_ID_ERROR = "Experimental entities are missing ObsUnitIDs. Import cannot proceed";


Suggested change

private static final String MISSING_OBS_UNIT_ID_ERROR = "Experimental entities are missing ObsUnitIDs. Import cannot proceed";

private static final String MISSING_OBS_UNIT_ID_ERROR = "Experimental entities are missing ObsUnitIDs";

"Import cannot proceed." is duplicated, because ". Import cannot proceed." is appended by bi-web.

mlm483 · 2024-04-02T18:58:57Z

src/main/java/org/breedinginsight/brapps/importer/services/processors/ExperimentProcessor.java

    }

-    private void processAndCacheTrial(BrAPITrial existingTrial, Program program, String trialRefSource, Map<String, PendingImportObject<BrAPITrial>> trialByNameNoScope) {
+    private void processAndCacheTrial(


Having "Cache" in the name suggests there might be interaction with our Redis cache. I get that it's caching in an instance variable, but if possible I might rename this and other similar methods to remove the word "Cache".

mlm483 · 2024-04-03T16:27:16Z

src/main/java/org/breedinginsight/brapps/importer/services/processors/ExperimentProcessor.java

-        this.obsVarDatasetByName = initializeObsVarDatasetByName(program, experimentImportRows);
-        this.existingGermplasmByGID = initializeExistingGermplasmByGID(program, experimentImportRows);
+        // check for references to Deltabreed-generated observation units
+        referenceOUIds = collateReferenceOUIds(importRows);


Here's an example of instance variables mutated as side effects (collateReferenceOUIds mutates hasAllReferenceUnitIds and hasNoReferenceUnitIds). There is also the assignment which makes it even less obvious that there are side effects.

mlm483 · 2024-04-03T16:28:22Z

src/main/java/org/breedinginsight/brapps/importer/services/processors/ExperimentProcessor.java

+                initializeTrialsForExistingObservationUnits(program, trialByNameNoScope);
+                initializeStudiesForExistingObservationUnits(program, studyByNameNoScope);


Here are examples of mutating passed-by-reference variables.

mlm483 · 2024-04-03T16:31:14Z

src/main/java/org/breedinginsight/brapps/importer/services/processors/ExperimentProcessor.java

+        // check for references to Deltabreed-generated observation units
+        referenceOUIds = collateReferenceOUIds(importRows);
+
+        if (hasAllReferenceUnitIds) {
+            try {
+
+                // get all prior units referenced in import
+                pendingObsUnitByOUId = fetchReferenceObservationUnits(referenceOUIds, program);
+                observationUnitByNameNoScope = mapPendingObservationUnitByName(pendingObsUnitByOUId, program);
+                initializeTrialsForExistingObservationUnits(program, trialByNameNoScope);
+                initializeStudiesForExistingObservationUnits(program, studyByNameNoScope);
+                locationByName = initializeLocationByName(program, studyByNameNoScope);
+                obsVarDatasetByName = initializeObsVarDatasetForExistingObservationUnits(trialByNameNoScope, program);
+                existingGermplasmByGID = initializeGermplasmByGIDForExistingObservationUnits(observationUnitByNameNoScope, program);
+                for (Map.Entry<String, PendingImportObject<BrAPIObservationUnit>>  unitEntry : pendingObsUnitByOUId.entrySet()) {
+                    String unitId = unitEntry.getKey();
+                    BrAPIObservationUnit unit = unitEntry.getValue().getBrAPIObject();
+                    mapPendingTrialByOUId(unitId, unit, trialByNameNoScope, studyByNameNoScope, pendingTrialByOUId, program);
+                    mapPendingStudyByOUId(unitId, unit, studyByNameNoScope, pendingStudyByOUId, program);
+                    mapPendingLocationByOUId(unitId, unit, pendingStudyByOUId, locationByName, pendingLocationByOUId);
+                    mapPendingObsDatasetByOUId(unitId, pendingTrialByOUId, obsVarDatasetByName, pendingObsDatasetByOUId);
+                    mapGermplasmByOUId(unitId, unit, existingGermplasmByGID, pendingGermplasmByOUId);


The mixture of assignments, mutating instance variables as side effects, and mutating passed-by-reference variables confusing. I have to dig into every called method to understand what state is mutated where and the lack of a consistent pattern makes it harder for me to reason about what's happening.

Given it's working and we want to release v0.9 I don't recommend making large changes, but maybe the team can discuss what we want our best practices to be in an IT planning meeting.

Ya, I think we can address this as part of the v0.10 work and refactoring of the ExperimentProcessor

mlm483 · 2024-04-03T18:25:40Z

src/main/java/org/breedinginsight/brapps/importer/services/processors/ExperimentProcessor.java

+        if (hasAllReferenceUnitIds) {
+            String unitName = obsUnitPIO.getBrAPIObject().getObservationUnitName();
+            String studyName = studyPIO.getBrAPIObject().getStudyName();
+            key = getObservationHash(studyName + unitName, variableName, studyName);


Why is studyName prepended to unitName here?

mlm483 · 2024-04-03T18:30:41Z

src/main/java/org/breedinginsight/brapps/importer/services/processors/ExperimentProcessor.java

+                if  (trialPio!=null &&  ImportObjectState.EXISTING==trialPio.getState() &&
+                        (StringUtils.isBlank( importRow.getObsUnitID() )) && (envPio!=null && ImportObjectState.EXISTING==envPio.getState() ) ){
+                    throw new UnprocessableEntityException(PREEXISTING_EXPERIMENT_TITLE);
+                }


This conditional is pretty complex, a comment explaining the intention might be helpful.

mlm483 · 2024-04-03T18:40:59Z

src/main/java/org/breedinginsight/brapps/importer/services/processors/ExperimentProcessor.java

+    /**
+     * This function collates unique ObsUnitID values from a list of BrAPIImport objects.
+     * It iterates through the list and adds non-blank ObsUnitID values to a Set. It also checks for any repeated ObsUnitIDs.
+     *
+     * @param importRows a List of BrAPIImport objects containing ExperimentObservation data
+     * @return a Set of unique ObsUnitID strings
+     * @throws IllegalStateException if a repeated ObsUnitID is encountered
+     */
+    private Set<String> collateReferenceOUIds(List<BrAPIImport> importRows) {


You might want to indicate that the hasAllReferenceUnitIds and hasNoReferenceUnitIds instance variables are set/mutated as well.

dmeidlin · 2024-04-03T19:58:54Z

Updated the error message.
Reference OU ids are the ids supplied by the user via import, and the corresponding units are reference OUs. These are OUs referenced by the user in the import for updating.

nickpalladino

When testing I saw a few things but think they are unrelated to the work in this card. I noticed when doing an updating phenotypes for a single observation unit the statistics were showing 1 for the observation units but I thought it would be 0. I also noticed the changeLog was showing a blank string for originalValue and I was expecting the prior value. I'm good creating new cards for these if they're legitimate issues.
exp4.xls
exp4_no_refs_2.xls

nickpalladino · 2024-04-05T19:04:11Z

src/main/java/org/breedinginsight/brapps/importer/services/processors/ExperimentProcessor.java

+        // check for references to Deltabreed-generated observation units
+        referenceOUIds = collateReferenceOUIds(importRows);
+
+        if (hasAllReferenceUnitIds) {
+            try {
+
+                // get all prior units referenced in import
+                pendingObsUnitByOUId = fetchReferenceObservationUnits(referenceOUIds, program);
+                observationUnitByNameNoScope = mapPendingObservationUnitByName(pendingObsUnitByOUId, program);
+                initializeTrialsForExistingObservationUnits(program, trialByNameNoScope);
+                initializeStudiesForExistingObservationUnits(program, studyByNameNoScope);
+                locationByName = initializeLocationByName(program, studyByNameNoScope);
+                obsVarDatasetByName = initializeObsVarDatasetForExistingObservationUnits(trialByNameNoScope, program);
+                existingGermplasmByGID = initializeGermplasmByGIDForExistingObservationUnits(observationUnitByNameNoScope, program);
+                for (Map.Entry<String, PendingImportObject<BrAPIObservationUnit>>  unitEntry : pendingObsUnitByOUId.entrySet()) {
+                    String unitId = unitEntry.getKey();
+                    BrAPIObservationUnit unit = unitEntry.getValue().getBrAPIObject();
+                    mapPendingTrialByOUId(unitId, unit, trialByNameNoScope, studyByNameNoScope, pendingTrialByOUId, program);
+                    mapPendingStudyByOUId(unitId, unit, studyByNameNoScope, pendingStudyByOUId, program);
+                    mapPendingLocationByOUId(unitId, unit, pendingStudyByOUId, locationByName, pendingLocationByOUId);
+                    mapPendingObsDatasetByOUId(unitId, pendingTrialByOUId, obsVarDatasetByName, pendingObsDatasetByOUId);
+                    mapGermplasmByOUId(unitId, unit, existingGermplasmByGID, pendingGermplasmByOUId);


Ya, I think we can address this as part of the v0.10 work and refactoring of the ExperimentProcessor

nickpalladino · 2024-04-05T19:07:14Z

src/main/java/org/breedinginsight/brapps/importer/services/processors/ExperimentProcessor.java

-                observations.add(this.observationByHash.get(getImportObservationHash(importRow, getVariableNameFromColumn(column))));
-            }
+            String observationHash;
+            if (hasAllReferenceUnitIds) {


I think in v0.10 we can try to make these workflows more modular and break things out a bit more rather than having if-else blocks in a number of different places

github-actions bot added the bug Something isn't working label Feb 14, 2024

dmeidlin requested review from a team, mlm483 and nickpalladino and removed request for a team February 15, 2024 16:14

dmeidlin assigned nickpalladino Feb 15, 2024

mlm483 approved these changes Feb 16, 2024

View reviewed changes

nickpalladino requested changes Feb 19, 2024

View reviewed changes

Base automatically changed from release/0.9 to develop March 12, 2024 16:11

dmeidlin added 21 commits March 29, 2024 11:42

[BI-2046] add validator for exp import on obsUnitID for existing studies

5bda44a

[BI-2046] update ref to obsolete brapi-client field

ba78917

[BI-2046] fix typos and obsolete refs

ef4fea6

[BI-2046] set pendingObsUnitByOUId

b783632

[BI-2046] refactor initializing OUs

9917eb5

[BI-2046] get trial pio from reference obs unit

9db276a

[BI-2046] update fetchOrCreate methods

fd40f55

[BI-2046] initialize brapi maps when OUIds supplied

87cc4bd

[BI-2046] map trials and studies to OU Id

8d3c29a

[BI-2046] map locations by OU id

6cb0251

[BI-2046] initialize dataset maps for existing observation units

7b3eb16

[BI-2046] refactor initialization of dataset map

346b629

[BI-2046] map obsVarDataset by OU id

18ff6e2

[BI-2046] fix NPEs

aa397ce

[BI-2046] map germplasm by observation unit id

9c33f31

[BI-2046] refaactor

e9ef80d

[BI-2046] prepare for validation using reference OU ids

74eeeb5

[BI-2046] validate observations for reference observation units

eaeb423

[BI-2046] add test for updating observations referenced by OU id

bd480f2

[BI-2046] fix studyPIO reference when creating new observation PIO

9704765

[BI-2046] correct observation hash when OU id referenced

885ef53

dmeidlin added 16 commits March 29, 2024 15:52

fix test

3e04828

[BI-2046] remove unused helper method

17b08e3

[BI-2046] refactor

55a0f78

[BI-2046] document exp processhelper method

cc68f28

[BI-2046] document exp process helper method

068f046

[BI-2046] document exp process helper method

e39a02d

[BI-2046] document exp process helper method

a42b98f

[BI-2046] document exp process helper method

b3f819c

[BI-2046] delete unused helper method

2e6161f

[BI-2046] document exp process helper method

a3f16c2

[BI-2046] delete unused helper method

aab081e

[BI-2046] delete unused helper method

b3396c8

[BI-2046] document exp process helper method

ac8072b

[BI-2046] document exp process helper method

4a32877

[BI-2046] clean up

b98da08

[BI-2046] Update expected error message in test

be76c12

dmeidlin force-pushed the bug/BI-2046 branch from 8f60f04 to be76c12 Compare April 1, 2024 19:18

dmeidlin requested review from mlm483 and nickpalladino April 1, 2024 19:53

dmeidlin changed the base branch from develop to release/0.9 April 1, 2024 19:59

mlm483 requested changes Apr 3, 2024

View reviewed changes

[BI-2046] remove redundant part of error message

7ea6dc4

[BI-2046] add comment

8bc7bc5

mlm483 self-requested a review April 4, 2024 17:34

mlm483 approved these changes Apr 4, 2024

View reviewed changes

nickpalladino reviewed Apr 5, 2024

View reviewed changes

nickpalladino approved these changes Apr 5, 2024

View reviewed changes

dmeidlin merged commit 0db3d2f into release/0.9 Apr 5, 2024

dmeidlin deleted the bug/BI-2046 branch April 5, 2024 20:12

	private static final String MISSING_OBS_UNIT_ID_ERROR = "Experimental entities are missing ObsUnitIDs. Import cannot proceed";
	private static final String MISSING_OBS_UNIT_ID_ERROR = "Experimental entities are missing ObsUnitIDs";

		initializeTrialsForExistingObservationUnits(program, trialByNameNoScope);
		initializeStudiesForExistingObservationUnits(program, studyByNameNoScope);

Conversation

dmeidlin commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Dependencies

Testing

Checklist:

Uh oh!

nickpalladino left a comment

Choose a reason for hiding this comment

Uh oh!

mlm483 left a comment

Choose a reason for hiding this comment

Error Message

"Reference" Observation Unit

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmeidlin commented Apr 3, 2024

Uh oh!

nickpalladino left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dmeidlin commented Feb 14, 2024 •

edited

Loading