Feature/ont 48 by ctucker3 · Pull Request #21 · Breeding-Insight/bi-api

ctucker3 · 2020-06-03T18:26:03Z

Includes ONT-48, ONT-47, and INF-91.

This feature sets up infrastructure for interfacing with third party brapi servers. The infrastructure is set up in such a way that allows for different servers for each program in the future. This also implements the trait endpoints for reading lists of traits and individual traits for our system. It also sets up a new way of setting data for tests using fannypack.

Design Choices

The controllers that will call BrAPI services will now be annotated with @BrAPIService. This flags a request filter to get the programId from the request url and retrieve the target url for it.
If a program has no specified url for its services, the default url set in the application.yml is used.
The endpoint for retrieving the list of traits has a query parameter Boolean full that indicates where the information for traits that we have in our system should be returned, or the combined information for our database and our brapi servers should be returned. This should speed up our displaying of lists and make the UI more responsive.
Retrieving of individual traits always returns the combine data from brapi and our server.
Any errors that occur when retrieving from brapi are returned to the user as Internal Server Errors. If we find the trait in our system, but it doesn't exist in the brapi server, that is going to be due to a server error because something went wrong in our storage or retrieval. Returning a 404 would be misleading because it was an error in the code somewhere. Looking for thoughts on this one since this is a different behavior than what is shown in the sequence diagrams.
Fannypack was used in the new tests to insert the test data into the database. The jooq dao (not our dao which extends it) is then used to retrieve that data for use in the tests.

Questions for reviewer

I was thinking to set an external reference source configuration in the application.yml now. If we set a reference source like, breeding-insight then we wouldn't need to worry about accidentally retrieving data that comes from other sources. This allows more specific retrieval of external references. This might also help in retrieving and debugging data issues in our services later on. What are your thoughts on adding the reference source vs. using only UUID?

Review

Do the tests for the BrAPIServerFilter (to get server url) cover the use case? Do you see any ways to improve these tests (they're brittle right now)
Do the tests for the TraitController cover the use case
Were the domain models for trait/scale/method etc. accurately captured?
Does the flow for retrieving data from brapi make sense?
General code review
Tests all pass

ctucker3 · 2020-06-04T19:23:48Z

ONT-48 - API - Fetch trait details from BrAPI Pheno service

ctucker3 · 2020-06-04T19:24:14Z

INF-91 - Explore possibility of using SQL to insert test data

ctucker3 · 2020-06-04T19:24:28Z

ONT-47 - API - Create endpoints to fetch traits

nickpalladino · 2020-06-05T15:53:14Z

Design Choices

I think it could also be that someone deleted the trait in the other system using that system rather than managing through breeding insight, not just an error in code. But I think a server error could still make sense as things would be out of sync unless we have some other type of status code for that case.

Question for reviewer

I think adding the breeding-insight reference source is a good idea. Wonder if a URL to the breeding insight system would better to use as the reference source?

nickpalladino · 2020-06-05T17:11:40Z

src/main/java/org/breedinginsight/services/brapi/BrAPIProvider.java

+    Provider<BrAPIClientProvider> brAPIClientProvider;
+
+    public TraitsAPI getTraitsAPI(BrAPIClientType clientType){
+        BrAPIClient brAPIClient = brAPIClientProvider.get().getClient(clientType);


Does BrAPIClientType need to be passed in here? Wouldn't the various getXAPI methods know what client type to use?

I think there will be cases when you want to use the same API for multiple brapi services. One example that comes to mind is that when we create a study, we are going to need to create that study in the phenotypic service to attach our phenotypic data to it, and in the genotypic service to attach our genotyping data to it. The client type acts a solution to that so the dao can target the services it needs to.

Good point!

nickpalladino · 2020-06-05T17:22:19Z

src/main/java/org/breedinginsight/daos/TraitDAO.java

+            throw new InternalServerException(e.getMessage());
+        }
+
+        Map<String, BrApiVariable> brApiVariableMap = new HashMap<>();


If you end up adding the reference source you could use that as a query parameter in the client call. I guess this checking for a reference id would remain but at least you'd only be iterating over breeding insight variables.

I think the call to the BrAPI backend should make use of /search/variables. You send a ObservationVariableSearchRequest which includes a list of externalReferenceIds and a list of externalReferenceSources.

Yep, agreed! I forgot that those search endpoints existed. Looks like our client library doesn't implement the search endpoints yet though. @nickpalladino is that true? Maybe do it this way for now and create a card for changing it in the future?

That's right, client doesn't currently implement the /search/variables endpoints. I agree with a future improvement to do that.

Sounds good

nickpalladino · 2020-06-05T17:37:48Z

src/main/java/org/breedinginsight/model/Method.java

+    String methodClass;
+    String description;
+    String formula;
+


Guessing we don't care about additionalInfo, bibliographicalReference, externalReferences, ontologyReference?

Yeah that is what I'm thinking. They aren't of much use to our domain model.

timparsons

Sorry for so many comments.

I looked through everything in detail, except BrAPIServerFilterIntegrationTest.java. I want to give that some thought on ways to make it less brittle

timparsons · 2020-06-05T20:38:39Z

src/main/java/org/breedinginsight/api/v1/controller/TraitController.java

+public class TraitController {
+
+    @Inject
+    TraitService traitService;


This should be private

Changed and pushed.

timparsons · 2020-06-05T20:44:09Z

src/main/java/org/breedinginsight/api/v1/controller/brapi/BrAPIServerFilter.java

+import java.util.UUID;
+
+@Filter("/**")
+public class BrAPIServerFilter extends OncePerRequestHttpServerFilter {


Chris and I talked about this earlier. We agreed that this filter should be updated as follows:

Restrict the @Filter pattern to focus on /programs/{programId}

Remove @BrAPIService annotation

Update tests to include tests for using the default BrAPI service and a program specified BrAPI service

Additional change request: Update the name of this filter to be BrAPIServiceFilter

Changed and pushed

timparsons · 2020-06-05T20:44:52Z

src/main/java/org/breedinginsight/api/v1/controller/brapi/BrAPIServerFilter.java

+    String defaultBrAPIUrl;
+    @Inject
+    ProgramService programService;
+    @Inject
+    BrAPIClientProvider brAPIClientProvider;


These should all be private

Changed and pushed.

timparsons · 2020-06-05T20:48:58Z

src/main/java/org/breedinginsight/api/v1/controller/brapi/BrAPIServerFilter.java

+                            Map<String, Object> params = routeMatch.getVariableValues();
+                            if (params.get("programId") != null) {
+                                UUID programId = UUID.fromString(params.get("programId").toString());
+                                ProgramBrAPIEndpoints programBrAPIEndpoints = programService.getBrapiEndpoints(programId);


Given that this is a filter, it will execute before the program-specific endpoint is executed. I think there should be a check that the programsBrAPIEndpoints object isn't null in the case that an invalid program id is passed in.

I'm not sure I am following on this one. If the filter is called on an endpoint, there will always be a programId (with the new filter changes). In the programService we can check if the programId that was sent in exists. If the UUID is invalid, there will be an internal server exception thrown on line 57. I could see us catching that exception and throwing a BadRequestException now that I think of it though. Did that cover your comment, or were you talking about something else?

I think it should throw a 404, not a 500, as the endpoint that's being requested would also throw a 404 if the filter wasn't there (because the programID doesn't exist).

Yep, that makes sense! I'll catch that and throw a 404.

Changed and pushed

timparsons · 2020-06-05T20:50:42Z

src/main/java/org/breedinginsight/daos/TraitDAO.java

+
+    private DSLContext dsl;
+    @Inject
+    BrAPIProvider brAPIProvider;


This should be private

Changed and pushed.

timparsons · 2020-06-06T04:07:42Z

src/test/java/org/breedinginsight/api/v1/controller/TraitControllerIntegrationTest.java

+
+    @Test
+    @SneakyThrows
+    public void getTraitsExistsInBrAPINotInSystem() {


With this test, are you expecting the returned list of traits for the program to be only the two that exist in the system (meaning the third is not included in the response to the client)? If so, I think you should add an assertion that the list returned to the client is == 2

Changed and pushed.

timparsons · 2020-06-06T04:10:36Z

src/test/java/org/breedinginsight/api/v1/controller/TraitControllerIntegrationTest.java

+        assertEquals(HttpStatus.OK, response.getStatus());
+
+        JsonObject result = JsonParser.parseString(response.body()).getAsJsonObject().getAsJsonObject("result");
+        checkTraitFullResponse(result, validTraits.get(0), brApiVariable1);


If the trait exists in BrAPI, but not in the system, shouldn't the response from the system be a 404?

Yeah it should be. Looks like this test needs to be fixed.

I removed this test since it was really a duplicate of getTraitSingleNotExist. Thanks for the catch!

timparsons · 2020-06-06T04:11:58Z

src/test/java/org/breedinginsight/api/v1/controller/TraitControllerIntegrationTest.java

+    public void getTraitSingleBrAPIError() {
+
+        reset(variablesAPI);
+        when(variablesAPI.getVariables()).thenThrow(new HttpNotFoundException("test"));


I think you should have the mocked error be an internal server error, as you've already got a test for the trait existing in the system, but not in BrAPI (aka Not Found).

Changed and pushed.

timparsons · 2020-06-06T04:20:05Z

src/test/java/org/breedinginsight/api/v1/controller/TraitControllerIntegrationTest.java

+
+        List<String> jsonSynonyms = new ArrayList<>();
+        traitJson.get("synonyms").getAsJsonArray().iterator().forEachRemaining(element -> jsonSynonyms.add(element.getAsString()));
+        assertLinesMatch(jsonSynonyms, brApiVariable.getTrait().getSynonyms(), "Synonyms don't match");


Do you know if order is important for this assertion? If so, it may be worth exploring an alternative way to compare if the two lists have the same values (order independent).

Order does matter I believe, but I was trying to be lazy :P I can implement unordered list checking on these.

I think Apache commons may have a utility to do this, so you don't have to write the code

I just sorted the lists before checking them. Changed and pushed.

timparsons · 2020-06-06T04:20:30Z

src/test/java/org/breedinginsight/api/v1/controller/brapi/BrAPIServerFilterIntegrationTest.java

+    ProgramEntity validProgram;
+    @Value("${micronaut.brapi.server.url}")
+    String defaultBrAPIUrl;
+
+    ListAppender<ILoggingEvent> loggingEventListAppender;
+    @Inject
+    ProgramService programService;
+    @MockBean(ProgramService.class)
+    ProgramService programService() { return mock(ProgramService.class);}
+    @Inject
+    BrAPIClientProvider brAPIClientProvider;
+    @MockBean(BrAPIClientProvider.class)
+    BrAPIClientProvider brAPIClientProvider() { return mock(BrAPIClientProvider.class, CALLS_REAL_METHODS); }
+
+    @Inject
+    DSLContext dsl;
+    @Inject
+    ProgramDao programDao;
+
+    @Inject
+    @Client("/${micronaut.bi.api.version}")
+    RxHttpClient client;


These should be private

Agreed. I'm not going to switch over our other tests, both those should probably have private properties too (all public right now).

Changed and pushed.

timparsons · 2020-06-06T04:37:55Z

Question for reviewer

I think adding the breeding-insight reference source is a good idea. Wonder if a URL to the breeding insight system would better to use as the reference source?

I like that idea! Thoughts on just breedinginsight.org or were you thinking more?

Yep I can default to that!

ctucker3 · 2020-06-08T15:26:15Z

Sorry for so many comments.

I looked through everything in detail, except BrAPIServerFilterIntegrationTest.java. I want to give that some thought on ways to make it less brittle

I think I found a better way to test the BrAPIServerFilter. I will push it up with the other changes.

Edit: New way is slightly better in that it doesn't rely on the side effects of error logging in the brapi client. I was able to spy on the brapi client to check the url before it executed and do an assertion there. The only downside is that the assertion is thrown in the micronaut code not in our testing code, so we aren't able to capture it easily. When I call the endpoints I expect them to fail because there will be nothing returned from the brapi server (I think this is fixable. Just a lot of time investment). Because of this, on a successful test, the api will still return an internal server error.

In the end, I'm still checking the logs for my expected error vs. assertion error. These tests could still use improvement. But, they is cleaner and more solid than they were before.

timparsons · 2020-06-08T21:38:32Z

src/test/java/org/breedinginsight/api/v1/controller/brapi/BrAPIServerFilterIntegrationTest.java

                .build();
    }
+
+    public void checkBrAPIClientExecution(String expectedUrl) throws AssertionFailedError {


Just had a thought on this (and if you'd rather create another story for this, I understand). What about using a "Promise"-like approach (in Java they're called "Future"s). Essentially something like this:

In this method, you'd create a Future, typed to return a Boolean. In the Answer object you've created, have it complete the Future instead of using the JUnit assertEquals. Lastly, have this method return the created future. In the test method, you'd have a variable for the future, and after you initiate the API call, you'd have an assertEquals method that would have an expected value of true, and an actual value of future.get(). Here's an example:

public Future<Boolean> checkBrAPIClientExecution(String expectedUrl) throws AssertionFailedError { CompletableFuture<Boolean> future = new CompletableFuture<>(); // Takes advantage of brapi library code to mimic no return results from api. Answer<Optional<Object>> checkBrAPIExecution = new Answer<Optional<Object>>() { @Override public Optional<Object> answer(InvocationOnMock invocation) throws AssertionFailedError { BrAPIClient executingBrAPIClient = (BrAPIClient) invocation.getMock(); // Check that our url is correct future.complete(expectedUrl.equals(executingBrAPIClient.brapiURI())); return Optional.empty(); } }; ... return future; }

and in your test method:

Future<Boolean> brapiUrlCheck = checkBrAPIClientExecution(phenoUrl); Flowable<HttpResponse<String>> call = client.exchange( GET("/programs/" + validProgram.getId() + "/traits?full=true") .contentType(MediaType.APPLICATION_JSON) .cookie(new NettyCookie("phylo-token", "test-registered-user")), String.class ); ... assertEquals(true, brapiUrlCheck.get(), "Expected url was not used");

I think if you went this way, you wouldn't need to look at the logs at all.

Great idea! I implemented this and it worked as expected. The changes are pushed. Thanks for the idea!

nickpalladino · 2020-06-08T16:57:23Z

src/test/java/org/breedinginsight/api/v1/controller/TraitControllerIntegrationTest.java

+
+    public void checkTraitResponse(JsonObject traitJson, Trait trait) {
+
+        assertEquals(traitJson.get("id").getAsString(), trait.getId().toString(), "Ids don't match");


It looks like you pulled a me and swapped the expected and actual parameters, not that it's a big deal.

Changed and pushed

ctucker3 · 2020-06-09T15:40:45Z

Question for reviewer
I think adding the breeding-insight reference source is a good idea. Wonder if a URL to the breeding insight system would better to use as the reference source?

I like that idea! Thoughts on just breedinginsight.org or were you thinking more?

Yep I can default to that!

Changed and pushed

timparsons

Two small comments, but everything else looks great!!

timparsons · 2020-06-09T20:17:29Z

src/main/java/org/breedinginsight/api/v1/controller/brapi/BrAPIServiceFilter.java

+    @Value("${micronaut.brapi.server.url}")
+    private String defaultBrAPIUrl;


Sorry I didn't catch this sooner, we will probably want to have a default BrAPI URL for each of the services (core, pheno, geno)

changed and pushed

timparsons · 2020-06-09T20:20:55Z

src/main/resources/application.yml

  brapi:
    server:
      url: ${brapi.server.url:`https://test-server.brapi.org/`}
+      reference-source: ${brapi.server.reference-source:`breeding-insight.org`}


Sorry I didn't catch this sooner. It may be cleaner to have properties that we've defined NOT be under the micronaut namespace (see web at the bottom of application.yml as an example).

Changed and push

nickpalladino · 2020-06-10T14:57:36Z

src/main/java/org/breedinginsight/api/model/v1/request/query/TraitsQuery.java

@@ -0,0 +1,10 @@
+package org.breedinginsight.api.model.v1.request.query;


Looks like all the newly added files are missing the license header. Also, you can set up intellij so that it will add them automatically when you create a new file.

changed and pushed

timparsons

Well done!! 👏

ctucker3 marked this pull request as ready for review June 4, 2020 19:22

ctucker3 requested review from dmeidlin, eawoods, nickpalladino and timparsons June 4, 2020 19:22

Chris T added 11 commits June 5, 2020 12:02

infrastructure setup for traits and brapi

82316bb

per request brapi urls and tests

819b960

cleanup

05f584a

GET trait controllers complete. Tests written

defbc9c

validate incoming query params

e8b8165

small fixes from self review

0c0bb8b

cleanup and test improvements

bc2b9bd

update api docs

60ae76f

updates to tests and fixes

658c20a

removing unnecessary lombok annotation

adf1d51

update docker build to use settings file. Update readme

e230962

ctucker3 force-pushed the feature/ONT-48 branch from b09a1d7 to e230962 Compare June 5, 2020 16:05

nickpalladino reviewed Jun 5, 2020

View reviewed changes

timparsons requested changes Jun 6, 2020

View reviewed changes

Chris T added 2 commits June 8, 2020 16:27

changes from pull request

b2e280b

changes for pull request

b62aead

timparsons reviewed Jun 8, 2020

View reviewed changes

nickpalladino reviewed Jun 9, 2020

View reviewed changes

update brapi service filter implementation

7095b03

Chris T added 3 commits June 9, 2020 10:10

add reference source search param. Cleanup

a909885

improve brapi service tests

c71a77d

clean up

13d1c53

ctucker3 requested review from nickpalladino and timparsons June 9, 2020 15:41

timparsons requested changes Jun 9, 2020

View reviewed changes

Chris T added 2 commits June 10, 2020 10:26

fix trait controller test. Reorganize brapi configs in config file

c1cb1fc

add new brapi config variables to env file

a76caec

ctucker3 requested a review from timparsons June 10, 2020 14:40

nickpalladino requested changes Jun 10, 2020

View reviewed changes

add copyright header

aa31dd0

ctucker3 requested a review from nickpalladino June 10, 2020 15:22

timparsons approved these changes Jun 10, 2020

View reviewed changes

quick fix for fannypack copyright header

5b2271e

nickpalladino approved these changes Jun 11, 2020

View reviewed changes

ctucker3 merged commit 497690f into develop Jun 11, 2020

ctucker3 deleted the feature/ONT-48 branch June 11, 2020 14:01

nickpalladino added the feature label Aug 24, 2020


		public void checkTraitResponse(JsonObject traitJson, Trait trait) {

		assertEquals(traitJson.get("id").getAsString(), trait.getId().toString(), "Ids don't match");

		@Value("${micronaut.brapi.server.url}")
		private String defaultBrAPIUrl;

		@@ -0,0 +1,10 @@
		package org.breedinginsight.api.model.v1.request.query;

Conversation

ctucker3 commented Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Design Choices

Questions for reviewer

Review

Uh oh!

ctucker3 commented Jun 4, 2020

Uh oh!

ctucker3 commented Jun 4, 2020

Uh oh!

ctucker3 commented Jun 4, 2020

Uh oh!

nickpalladino commented Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

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

ctucker3 commented Jun 3, 2020 •

edited

Loading

nickpalladino commented Jun 5, 2020 •

edited

Loading

timparsons commented Jun 6, 2020 •

edited by ctucker3

Loading

ctucker3 commented Jun 8, 2020 •

edited

Loading