BI-1650 - Store and fetch genotype data by timparsons · Pull Request #228 · Breeding-Insight/bi-api

timparsons · 2022-11-16T03:09:49Z

Description

Story: https://breedinginsight.atlassian.net/browse/BI-1650

Created a GenoService interface
Created a GigwaGenoServiceImpl class to handle importing and retrieving genotype data to/from Gigwa
Added Gigwa to the docker-compose file
Introduced dependency to interact with AWS S3
Introduced BrAPIEndpointProvider, and refactored how BrAPI endpoint classes are created from the BrAPI Java Client. This makes the code more testable because the BrAPIEndpointProvider can be mocked in tests
Converted ImportDAO to use the interface/impl pattern
Converted ImportMappingDAO to use the interface/impl pattern
Converted UserDAO to use the interface/impl pattern

Dependencies

bi-web/feature/BI-1650
Gigwa (should be able to run the container specified in the docker-compose file)
AWS keys to connect to S3

Testing

Get AWS keys to connect to S3
Verify that the application can start up, and that Gigwa can start up
Test the card following the acceptance criteria in the card

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: https://github.com/Breeding-Insight/taf/actions/runs/3500841631

src/test/java/org/breedinginsight/services/geno/impl/GigwaGenoServiceImplIntegrationTest.java

HMS17

Importing runs into NullPointerExpection due to addition of dynamic columns. Adding a wrapper to check for null in ImportUpload.java:setDynamicColumnNames fixes it

src/main/java/org/breedinginsight/services/geno/impl/GigwaGenoServiceImpl.java

nickpalladino · 2023-02-17T15:15:01Z

.env

+AWS_REGION=<aws region>
+AWS_ACCESS_KEY_ID=<access key>
+AWS_SECRET_KEY=<secret>
+AWS_GENO_BUCKET=<s3 bucket for genotypic data uploads>


Should this also have the AWS_S3_ENDPOINT= placeholder?

I see this was updated

nickpalladino · 2023-02-17T15:20:42Z

src/main/java/org/breedinginsight/api/v1/controller/geno/GenoDataUploadController.java

+        this.securityService = securityService;
+    }
+
+    @Post("programs/{programId}/experiments/{experimentId}/geno/import")


Not sure how well the api docs have been kept to date as of late but should new api endpoints be added in the docs?

@nickpalladino I don't think very well. I'd be more inclined to create a separate card to handle updating the API docs. Thoughts?

Sounds good

nickpalladino · 2023-02-17T15:27:09Z

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

                BrAPIGermplasm germplasm = germplasmService.getGermplasmByDBID(programId, germplasmId);
                //Forward the pedigree call to the backing BrAPI system of the program passing the germplasmDbId that came in the request
-                GermplasmApi api = new GermplasmApi(programDAO.getCoreClient(programId));
+                GermplasmApi api = brAPIEndpointProvider.get(programDAO.getCoreClient(programId), GermplasmApi.class);


What was the reason for this change?

@nickpalladino To make the code more testable. By changing from using the 'new ()', unit tests can now mock out the creation of certain types of BrAPI endpoint classes that are in the BrAPI Java Client.

Sounds good

nickpalladino · 2023-02-17T16:25:10Z

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

                                                        Map<Integer, PendingImport> mappedBrAPIImport, Table data,
                                                        Program program, User user, boolean commit) throws ValidatorException {

+        log.debug("Processing germplasm import");


Do you still want the debug logging in here?

@nickpalladino Yes. My opinion is the more logging in the code the better. We can always change the log level if it's too noisy in production, but we can't add more without rebuilding code.

src/main/java/org/breedinginsight/services/geno/GenoService.java

…asses, and improve testability

…s in the system before starting import

…riment

…and Localstack

…eam changes

timparsons force-pushed the feature/BI-1650 branch from b2d3e30 to d47c51c Compare November 16, 2022 04:07

timparsons requested review from HMS17, davedrp, dmeidlin and nickpalladino November 19, 2022 01:08

HMS17 reviewed Dec 13, 2022

View reviewed changes

src/test/java/org/breedinginsight/services/geno/impl/GigwaGenoServiceImplIntegrationTest.java Outdated Show resolved Hide resolved

HMS17 requested changes Dec 19, 2022

View reviewed changes

timparsons force-pushed the feature/BI-1650 branch from 01ec3c8 to 16ce165 Compare January 9, 2023 22:30

dmeidlin approved these changes Jan 20, 2023

View reviewed changes

timparsons force-pushed the feature/BI-1650 branch from e41c9bd to feb842d Compare January 23, 2023 15:40

davedrp requested changes Feb 1, 2023

View reviewed changes

src/main/java/org/breedinginsight/services/geno/impl/GigwaGenoServiceImpl.java Show resolved Hide resolved

src/main/java/org/breedinginsight/services/geno/impl/GigwaGenoServiceImpl.java Outdated Show resolved Hide resolved

timparsons requested a review from davedrp February 8, 2023 19:07

timparsons force-pushed the feature/BI-1650 branch from 7562af5 to 5d6a948 Compare February 8, 2023 22:17

davedrp requested changes Feb 9, 2023

View reviewed changes

src/main/java/org/breedinginsight/services/geno/impl/GigwaGenoServiceImpl.java Outdated Show resolved Hide resolved

src/main/java/org/breedinginsight/services/geno/impl/GigwaGenoServiceImpl.java Show resolved Hide resolved

nickpalladino reviewed Feb 17, 2023

View reviewed changes

timparsons added 15 commits February 23, 2023 16:58

[BI-1650] - Adding ability to upload and view genotypic data

9e08eb9

[BI-1650] - cleanup

71a8087

[BI-1650] - Created BrAPIEndpointProvider to create BrAPI endpoint cl…

1068b42

…asses, and improve testability

[BI-1650] Validate vcf file for valid header and that samples have OU…

bca3f60

…s in the system before starting import

[BI-1650] Updating tests, adding gigwa and LocalStacks to tests

93eed87

[BI-1650] Updating docker-compose.yml. Fixing bug with importing expe…

5a0e871

…riment

[BI-1650] Fixing unit test failures

4082b4f

[BI-1650] Fixing unit test failures

17716d1

[BI-1650] Fixing unit test failures

d61f0f6

[BI-1650] Fixing unit test failures

d687272

[BI-1650] Fixing unit test failures

8dfd69d

[BI-1650] Fixing unit test failures

2cb4341

[BI-1650] Fixing unit test failures

b488224

[BI-1650] Fixing unit test failures

aeddea6

Adding localstack support to docker-compose.yml

37f9bb2

timparsons and others added 11 commits February 23, 2023 16:59

[BI-1650] Adding information to the README for running Redis, Gigwa, …

cd1d061

…and Localstack

[BI-1650] Renaming migration

a10e354

[BI-1650] PR changes

7a661f1

[BI-1650] Renaming migration

c29ae45

[BI-1650] Fixing compilation error

729651f

[BI-1650] Fixing compilation error

8773250

[BI-1650] Reorganizing structure to be more readable

9f2a199

Added logging statement to help debugging

b07e9cc

[BI-1650] Changes from review comments

c8c0227

[BI-1650] Updates after rebasing Gigwa. Applying refactoring to upstr…

632ae25

…eam changes

[BI-1650] removing TODO that's been completed

a6eea82

timparsons force-pushed the feature/BI-1650 branch from 683bef4 to a6eea82 Compare February 27, 2023 20:44

[BI-1650] bug fixes

4d215fb

timparsons requested review from HMS17, davedrp and nickpalladino and removed request for HMS17 February 27, 2023 21:13

nickpalladino approved these changes Feb 27, 2023

View reviewed changes

[BI-1650] updating github actions configs

71f15f4

davedrp approved these changes Mar 1, 2023

View reviewed changes

timparsons merged commit ba7927e into develop Mar 1, 2023

timparsons deleted the feature/BI-1650 branch March 1, 2023 20:44

Conversation

timparsons commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Dependencies

Testing

Checklist:

Uh oh!

Uh oh!

HMS17 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

timparsons commented Nov 16, 2022 •

edited

Loading