BI-2308 - Field Book BrAPI Integration - Importing Fields #402

nickpalladino · 2024-09-18T20:51:16Z

Description

Story: BI-2308

Branch name doesn't match but it's for BI-2308
modified removeProgramKeyAndUnknownAdditionalData as regex seemed incorrect and was removing until the second ] and resulting pedigree string was incorrect. The method is used a bunch if different places so should make sure there aren't regressions. Not sure if issues have been noticed in the past?
Ended up not removing setDbIds methods which was discussed as a possible approach in IT planning. The scope of doing that was much larger because the BrAPI objects in the cache are keyed off the DeltaBreed UUID, not the brapi dbId.

Dependencies

Field Book v5.6.24

Testing

GIVEN have selected a field on the FieldBook Fields page to import
WHEN field is import is started
THEN field should successfully load in a time proportional to the amount of data, not hang indefinitely, and not get an error message

GIVEN have imported a Field with germplasm containing synonyms and pedigree
WHEN the attributes are viewed on the collect page
THEN should see synonyms and pedigree string with program keys & accession numbers stripped out

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>

…or system admins

BI-2274 release/1.0 merge

src/main/java/org/breedinginsight/brapi/v2/BrAPIGermplasmController.java

dmeidlin · 2024-09-19T20:41:00Z

src/main/java/org/breedinginsight/brapi/v2/BrAPIGermplasmController.java

+            // convert dbIds to DeltaBreed UUID
+            BrAPIGermplasmListResponse response = brapiGermplasm.getBody().getLeft().get();
+            List<BrAPIGermplasm> germplasm = response.getResult().getData();
+            germplasm.forEach(g -> setDbIdsAndStripProgramKeys(g, programKey));


Suggested change

germplasm.forEach(g -> setDbIdsAndStripProgramKeys(g, programKey));

batchProcessGermplasm(germplasm, programKey);

src/main/java/org/breedinginsight/brapi/v2/BrAPIGermplasmController.java

dmeidlin · 2024-09-19T21:10:16Z

src/main/java/org/breedinginsight/utilities/Utilities.java

+    /**
+     * \s*: Matches zero or more whitespace characters before the opening bracket.
+     * \[: Matches the opening square bracket [. The backslash is used to escape the special meaning of [ in regex.
+     * .*?: Matches any character (except newline) zero or more times, non-greedily.
+     * . matches any character except newline.
+     * * means "zero or more times".
+     * ? makes the matching non-greedy, so it stops at the first closing bracket.
+     * \]: Matches the closing square bracket ]. Again, the backslash is used to escape it.
+     * \s*: Matches zero or more whitespace characters after the closing bracket.
+     * @param original
+     * @param programKey
+     * @return
+     */


Unit tests for this method covering these cases with specific assertions for the expected format of synonyms and pedigree strings aster stripping would be helpful when testing for regressions later. New edge cases found could be added along with tests for very large fields or complex pedigrees.

Created card BI-2325

dmeidlin

The setDbIdsAndStripProgramKeys method in BrAPIGermplasmController.java processes each germplasm item individually. For large datasets, this could potentially impact performance. I've added a method for processing a germplasm batch that only compiles the regex once and uses parallel streams if possible.

For clarity, I extracted the logic for replacing delta ids with dbIds into a private method to keep the body of the controller method lean.

I also think unit tests for removeProgramKeyAndUnknownAdditionalData are a good idea, but probably exceeds the effort level estimate for this card, but adding it to the card that checks for regressions in other places this method is used should include writing the tests.

[BI-2317] System Admin Must Retain Full Read Access To All Programs

mlm483

I tested locally with Field Book version 5.6.24.

The first test passed, I was able to load fields at around 1000 germplasm/minute which seems fast enough for now.

The second test is less clear to me, I'm not sure where I'm meant to see synonym or pedigree info, but I didn't see any. The germplasm names in the collect screen didn't have the program key which is good.

Also, using wireshark, I see there was one request made from bi-api to brapi-server to the /brapi/v2/search/germplasm POST endpoint with an external reference and a list of germplasmDbIds. A get is then made for the search results (/brapi/v2/search/germplasm/{searchResultDbId}) but what's odd is the results are not ready when this request is made, and the client (bi-api) never seems to make a subsequent request, so I'm not sure what's happening there.

src/main/java/org/breedinginsight/brapi/v2/BrAPIGermplasmController.java

dmeidlin

Just re-submitting initial request

nickpalladino · 2024-09-30T18:08:41Z

I tested locally with Field Book version 5.6.24.

The first test passed, I was able to load fields at around 1000 germplasm/minute which seems fast enough for now.

The second test is less clear to me, I'm not sure where I'm meant to see synonym or pedigree info, but I didn't see any. The germplasm names in the collect screen didn't have the program key which is good.

If you included synonyms and pedigree information in the germplasm you should be able to tap the attributes on the collect page to open a dialog to increase the number of attributes shown and select the attributes you would like displayed. Synonyms and pedigree should be selectable options in the list.

Also, using wireshark, I see there was one request made from bi-api to brapi-server to the /brapi/v2/search/germplasm POST endpoint with an external reference and a list of germplasmDbIds. A get is then made for the search results (/brapi/v2/search/germplasm/{searchResultDbId}) but what's odd is the results are not ready when this request is made, and the client (bi-api) never seems to make a subsequent request, so I'm not sure what's happening there.

From what I was seeing the BrAPI server will give an immediate response if the number of dbIds being searched is below a certain threshold then switch to the searchResultDbId after that point. Field Book should make requests until it gets the result data.

mlm483 · 2024-10-01T14:22:12Z

With a small amount of data, I was able to see synonyms and pedigree. Using wireshark, I could see the response to the POST to the /brapi/v2/search/germplasm endpoint contained a searchResultDbId, and the data was returned in the response to the first GET request to the /brapi/v2/search/germplasm/{searchResultDbId} endpoint.

With a moderate amount of data (5000 Germplasm, ~1000 referenced by Observation Units), Field Book times out after 2 minutes when previewing the field (120 seconds is the default value for BRAPI_TIMEOUT in Field Book). The call that it times out on is the germplasm search request. Looking at the bi-api logs, I can see it's hitting the cache once for each Germplasm record, I think it's important to make that more efficient in this story.

mlm483

Other than the Field Book bug (I see your issue), this is working as expected.

rob-ouser-bi and others added 2 commits September 12, 2024 15:55

[autocommit] bumping build number

3cfd1f5

Updates to germplasm search endpoints to support Field Book integration

e0f22d2

github-actions bot added the bug Something isn't working label Sep 18, 2024

dmeidlin and others added 4 commits September 18, 2024 19:09

fix service method for fetching program user collaborators to allow f…

004179e

…or system admins

refactor for clarity

899576c

Merge pull request #404 from Breeding-Insight/bug/BI-2274

7465477

BI-2274 release/1.0 merge

[autocommit] bumping build number

6551887

nickpalladino marked this pull request as ready for review September 19, 2024 15:29

Merge branch 'develop' into bug/BI-2309

a6e1674

nickpalladino requested review from a team, davedrp, dmeidlin and mlm483 and removed request for a team September 19, 2024 15:55

dmeidlin reviewed Sep 19, 2024

View reviewed changes

src/main/java/org/breedinginsight/brapi/v2/BrAPIGermplasmController.java Outdated Show resolved Hide resolved

dmeidlin reviewed Sep 19, 2024

View reviewed changes

src/main/java/org/breedinginsight/brapi/v2/BrAPIGermplasmController.java Outdated Show resolved Hide resolved

dmeidlin reviewed Sep 19, 2024

View reviewed changes

dmeidlin requested changes Sep 19, 2024

View reviewed changes

nickpalladino assigned mlm483 Sep 20, 2024

nickpalladino requested a review from dmeidlin September 20, 2024 18:15

dmeidlin and others added 2 commits September 20, 2024 14:58

Merge pull request #405 from Breeding-Insight/bug/BI-2317

68609b9

[BI-2317] System Admin Must Retain Full Read Access To All Programs

[autocommit] bumping build number

402926b

davedrp approved these changes Sep 24, 2024

View reviewed changes

mlm483 reviewed Sep 24, 2024

View reviewed changes

src/main/java/org/breedinginsight/brapi/v2/BrAPIGermplasmController.java Outdated Show resolved Hide resolved

dmeidlin requested changes Sep 27, 2024

View reviewed changes

nickpalladino added 2 commits October 1, 2024 11:10

Convert germplasm ids in batch

ad82455

Code review changes

49b365c

nickpalladino requested review from mlm483 and dmeidlin October 1, 2024 18:39

mlm483 approved these changes Oct 2, 2024

View reviewed changes

nickpalladino added 2 commits October 4, 2024 15:58

Merge branch 'release/1.0' into bug/BI-2309

b683b9a

Merge branch 'develop' into bug/BI-2309

d4ef7b5

nickpalladino merged commit e21aaa3 into develop Oct 4, 2024
1 check failed

nickpalladino deleted the bug/BI-2309 branch October 4, 2024 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BI-2308 - Field Book BrAPI Integration - Importing Fields #402

BI-2308 - Field Book BrAPI Integration - Importing Fields #402

nickpalladino commented Sep 18, 2024 •

edited

Loading

dmeidlin Sep 19, 2024

dmeidlin Sep 19, 2024

nickpalladino Sep 20, 2024

dmeidlin left a comment •

edited

Loading

mlm483 left a comment

dmeidlin left a comment

nickpalladino commented Sep 30, 2024

mlm483 commented Oct 1, 2024

mlm483 left a comment

	germplasm.forEach(g -> setDbIdsAndStripProgramKeys(g, programKey));
	batchProcessGermplasm(germplasm, programKey);

BI-2308 - Field Book BrAPI Integration - Importing Fields #402

BI-2308 - Field Book BrAPI Integration - Importing Fields #402

Conversation

nickpalladino commented Sep 18, 2024 • edited Loading

Description

Dependencies

Testing

Checklist:

dmeidlin Sep 19, 2024

Choose a reason for hiding this comment

dmeidlin Sep 19, 2024

Choose a reason for hiding this comment

nickpalladino Sep 20, 2024

Choose a reason for hiding this comment

dmeidlin left a comment • edited Loading

Choose a reason for hiding this comment

mlm483 left a comment

Choose a reason for hiding this comment

dmeidlin left a comment

Choose a reason for hiding this comment

nickpalladino commented Sep 30, 2024

mlm483 commented Oct 1, 2024

mlm483 left a comment

Choose a reason for hiding this comment

nickpalladino commented Sep 18, 2024 •

edited

Loading

dmeidlin left a comment •

edited

Loading