[BI-1339] Germplasm Details page by HMS17 · Pull Request #176 · Breeding-Insight/bi-api

HMS17 · 2022-04-05T16:09:08Z

Description

Story: BI-1339 - Germplasm Details page

Updates to enable display of individual germplasm details

Created endpoint in GermplasmController to fetch an individual germplasm record
Refactored ProgramCache to be a Map<K, R> instead of a list in order to more easily have the UUID for each germplasm entry be retrieved
Added logic to fetch individual germplasm record information from the cache
Added an additional info field "pedigreeByName" for germplasm to aid in displaying germplasm pedigree info

Dependencies

bi-web/feature/BI-1339

Testing

See testing for bi-web

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 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: <link to TAF run>

…into feature/BI-1339 � Conflicts: � src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java

ctucker3 · 2022-04-11T14:51:36Z

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

+    @Get("/${micronaut.bi.api.version}/programs/{programId}" + BrapiVersion.BRAPI_V2 + "/germplasm/{germplasmId}")
+    @Produces(MediaType.APPLICATION_JSON)
+    @ProgramSecured(roleGroups = {ProgramSecuredRoleGroup.ALL})
+    public HttpResponse<BrAPIGermplasm> getSingleGermplasm(


Should be HttpResponse<Response<BrAPIGermplasm>> to get the right return type.

src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java

ctucker3 · 2022-04-11T15:14:39Z

src/main/java/org/breedinginsight/brapi/v2/dao/ProgramCache.java

+public class ProgramCache<K, R> {

-    private final FetchFunction<UUID, List<R>> fetchMethod;
+    private final FetchFunction<UUID, Map<K, R>> fetchMethod;


Not clearly explained that K should be the object id, it took me a bit to figure out what K was supposed to be. Maybe a class header comment for explanation?

src/test/java/org/breedinginsight/services/BrAPIGermplasmServiceUnitTest.java

ctucker3 · 2022-04-11T15:19:21Z

src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java

    }
+
+    public BrAPIGermplasm getGermplasmByUUID(String germplasmId, UUID programId) throws ApiException {
+        return programGermplasmCache.get(programId).get(germplasmId);


What if the germplasmId doesn't exist in the returned map?

…-1339

ctucker3 · 2022-04-13T17:31:35Z

src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java

-    public BrAPIGermplasm getGermplasmByUUID(String germplasmId, UUID programId) throws ApiException {
-        return programGermplasmCache.get(programId).get(germplasmId);
+    public BrAPIGermplasm getGermplasmByUUID(String germplasmId, UUID programId) throws ApiException, DoesNotExistException {
+        BrAPIGermplasm germplasm = programGermplasmCache.get(programId).get(germplasmId);


Did you test this out? I thought in java the exception is thrown here on the map access if the key doesn't exist rather than it returning a null.

There probably should be a null check for what's returned from programGermplasmCache.get(programId) as there could be some programs don't have germplasm in the cache...either they don't have any in the system yet, or the cache loading failed, or is in progress.

ctucker3 · 2022-04-13T17:33:00Z

src/main/java/org/breedinginsight/brapi/v2/services/BrAPIGermplasmService.java

        try {
            return germplasmDAO.getGermplasmByUUID(germplasmId, programId);
-        } catch (ApiException e) {
+        } catch (ApiException | DoesNotExistException e) {


If a germplasm is requested that doesn't exist it seems like that is a 404 rather than a 500.

timparsons

Testing passed. Have a few code changes, however

timparsons · 2022-04-14T19:40:05Z

src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java

                germplasm.setPedigree(newPedigreeString);
            }
+
+            BrAPIExternalReference extRef = germplasm.getExternalReferences().stream().filter(reference -> referenceSource.equals(reference.getReferenceSource())).findFirst().orElseThrow();


I think you should specify the error to throw in the .orElseThrow method call, for example: .orElseThrow(() -> new IllegalStateException("No BI external reference found"))

timparsons · 2022-04-14T19:40:28Z

src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java

 import io.micronaut.context.annotation.Context;
 import io.micronaut.context.annotation.Property;
 import io.micronaut.http.server.exceptions.InternalServerException;
+import it.unimi.dsi.fastutil.Hash;


This import is not used and can be removed

timparsons · 2022-04-14T19:53:49Z

src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java

-    public BrAPIGermplasm getGermplasmByUUID(String germplasmId, UUID programId) throws ApiException {
-        return programGermplasmCache.get(programId).get(germplasmId);
+    public BrAPIGermplasm getGermplasmByUUID(String germplasmId, UUID programId) throws ApiException, DoesNotExistException {
+        BrAPIGermplasm germplasm = programGermplasmCache.get(programId).get(germplasmId);


There probably should be a null check for what's returned from programGermplasmCache.get(programId) as there could be some programs don't have germplasm in the cache...either they don't have any in the system yet, or the cache loading failed, or is in progress.

timparsons · 2022-04-14T19:56:02Z

src/main/java/org/breedinginsight/brapi/v2/services/BrAPIGermplasmService.java

        }
    }

+    public BrAPIGermplasm getGermplasmByUUID(UUID programId, String germplasmId) throws ApiException, DoesNotExistException {


Given that you're catching an ApiException, you don't need to specify that an ApiException will be thrown.

timparsons · 2022-04-14T19:56:27Z

src/main/java/org/breedinginsight/brapi/v2/services/BrAPIGermplasmService.java

+        } catch (DoesNotExistException e) {
+            throw e;
+        }


This is redundant and can be removed as the method declares it can throw a DoesNotExistException

[BI-1339] Germplasm Details page

bd85ecc

github-actions bot added the feature label Apr 5, 2022

HMS17 added 5 commits April 6, 2022 16:50

Germplasm Retrieval and Display

0829e5c

Merge branch 'develop' of https://github.com/Breeding-Insight/bi-api …

fee6a2d

…into feature/BI-1339 � Conflicts: � src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java

Cleanup

a2840df

Fix null key in unit test

55a751a

Unit test

2e781eb

HMS17 mentioned this pull request Apr 7, 2022

[BI-1339] Germplasm Details page Breeding-Insight/bi-web#217

Merged

6 tasks

HMS17 requested review from ctucker3 and timparsons April 7, 2022 19:00

HMS17 assigned timparsons Apr 7, 2022

HMS17 marked this pull request as ready for review April 7, 2022 19:01

ctucker3 suggested changes Apr 11, 2022

View reviewed changes

HMS17 added 2 commits April 11, 2022 12:31

Merge branch 'develop' into feature/BI-1339

41518d7

Code review fixes

ceae257

HMS17 requested a review from ctucker3 April 13, 2022 15:51

Merge remote-tracking branch 'origin/feature/BI-1339' into feature/BI…

10dd6d9

…-1339

ctucker3 suggested changes Apr 13, 2022

View reviewed changes

Code review fixes 2

66a2e2e

HMS17 requested a review from ctucker3 April 13, 2022 18:54

ctucker3 approved these changes Apr 14, 2022

View reviewed changes

timparsons requested changes Apr 14, 2022

View reviewed changes

Code review cleanup

f3f5e85

HMS17 requested a review from timparsons April 14, 2022 20:38

timparsons approved these changes Apr 14, 2022

View reviewed changes

Merge branch 'develop' into feature/BI-1339

f588f7c

HMS17 merged commit cc9eb0e into develop Apr 14, 2022

HMS17 deleted the feature/BI-1339 branch April 14, 2022 22:56

Conversation

HMS17 commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Dependencies

Testing

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

timparsons 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!

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

HMS17 commented Apr 5, 2022 •

edited

Loading