Skip to content

[BI-693] Upgrade micronaut to 2.5.13#146

Merged
ctucker3 merged 30 commits intofuture/1.0from
2.x-micronaut-upgrade
Jun 3, 2022
Merged

[BI-693] Upgrade micronaut to 2.5.13#146
ctucker3 merged 30 commits intofuture/1.0from
2.x-micronaut-upgrade

Conversation

@ctucker3
Copy link
Contributor

@ctucker3 ctucker3 commented Feb 3, 2022

Description

Includes:

  • Upgrade micronaut to 2.5.13
  • Upgrades all libraries to latest version.
    • Exceptions:
      • Flyway version depends on micronaut version, so that tracks the version used in micronaut.
      • apache-poi libraries are needed by the tika app, so version of these tracks the version used in tika-app.
  • No functionality or tests were removed.
    • Exception:
      • The HttpStatusExceptionHandler was removed because it was not needed anymore.

Dependencies

develop of bi-web.
jooq-3.16.3 of bi-jooq-codegen (Breeding-Insight/bi-jooq-codegen#3) (not needed locally, this just needs to be merged first)

Testing

Minimum testing:

  1. Run bi-api on your local machine and make sure it runs.

Here are some suggestions for what to test:

  1. Test new account creation
  2. Logged out redirect
  3. Unauthorized sign in errors
  4. Pages and tables
  5. End point not found
  6. Launching an app from scratch
  7. Authenticate and load traits with Field Book. (see file below)
  8. BreedBase integration (create program, save germplasm, save traits)

I tested these and all of them passed.

Here is a file with descriptions on how to test a new account and field book.
bi-api_testing_details.md

TAF run returns the same results as the develop branch.

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: https://github.com/Breeding-Insight/taf/suites/5205128227/artifacts/159100342

@ctucker3 ctucker3 changed the base branch from develop to v1.0-working February 4, 2022 13:36
@ctucker3 ctucker3 marked this pull request as ready for review February 7, 2022 18:11
@ctucker3 ctucker3 changed the title Upgrade micronaut to 2.5.13 [BI-693] Upgrade micronaut to 2.5.13 Feb 7, 2022
Copy link
Contributor

@davedrp davedrp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All test passed.

Steps to test

  1. Cleared databases
  2. Ran script to build bi-api
  3. Started docker
  4. Started bi-api server
  5. Added new programs
  6. Snacks
  7. Trail Mix
  8. Added/registered new users
  9. Tested redirect
  10. Imported Ontology
  11. Edit Ontology

Copy link
Contributor

@HMS17 HMS17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested

  1. Unauthorized log in and redirect
  2. Admin login
  3. Log out redirect
  4. Sidebar links direct to proper pages
  5. Tables display data
  6. New account creation
  7. Login with one program
  8. Page does not exist

A couple comments on the tests, but BI appears to be working!

super.stopContainers();
}

//TODO: Fix this test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this and the other todos for a future development or were they accidentally left in?

(Also a+ re: cats)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just accidentally left in. I removed those TODOS

assertEquals(expectedLocation, redirectLocation);

// Check that the jwt cookie was returned

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems there is code missing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was moved below to the end of the test. I moved this comment down there.

@ctucker3 ctucker3 force-pushed the 2.x-micronaut-upgrade branch from 3519f75 to 2563331 Compare April 19, 2022 15:25
@ctucker3 ctucker3 changed the base branch from v1.0-working to future/1.0 April 19, 2022 15:25
Copy link
Member

@timparsons timparsons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested running the application and using the interface to create a program, import ontology terms, import germplasm, view ontology terms, view germplasm, and sign in from Field Book. All of those things worked as expected.

Left a few comments, with some minor changes to make.

import org.breedinginsight.model.ProgramBrAPIEndpoints;
import org.breedinginsight.services.ProgramService;
import org.breedinginsight.services.exceptions.DoesNotExistException;
import org.intellij.lang.annotations.Flow;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unused and can be removed

package org.breedinginsight.api.v1.controller.brapi;

import io.micronaut.context.annotation.Property;
import io.micronaut.core.async.publisher.Publishers;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unused and can be removed

import io.micronaut.http.server.exceptions.InternalServerException;
import io.micronaut.web.router.MethodBasedRouteMatch;
import io.micronaut.web.router.RouteMatch;
import io.micronaut.web.router.RouteMatchUtils;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unused and can be removed


import javax.inject.Inject;
import java.util.Map;
import java.util.Optional;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unused and can be removed

Comment on lines +87 to +83
MutableHttpResponse<?> response = HttpResponse.status(HttpStatus.NOT_FOUND, "Program does not exist");
return Flowable.just(response);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this (and lines 95/96) changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Micronaut was not exiting out of the request to return the failure with the previous way. Needed to use the just so it didn't continue with the request and returns immediately.

Comment on lines +502 to +538
String[] names = traits.stream()
.filter(trait -> trait.getObservationVariableName() != null)
.map(trait -> (RowN) row(trait.getObservationVariableName()))
.collect(Collectors.toList()).toArray(RowN[]::new);
.map(trait -> trait.getObservationVariableName().toLowerCase())
.collect(Collectors.toList()).toArray(String[]::new);

List<Trait> traitResults = new ArrayList<>();
if (valueRows.length > 0){
Table newTraits = dsl.select()
.from(values(valueRows).as("newTraits", "new_trait_name")).asTable("newTraits");
if (names.length > 0){

Result<Record> records = dsl.select()
.from(newTraits)
.join(TRAIT).on(TRAIT.OBSERVATION_VARIABLE_NAME.upper().equalIgnoreCase(newTraits.field("new_trait_name")))
.from(TRAIT)
.join(PROGRAM_ONTOLOGY).on(TRAIT.PROGRAM_ONTOLOGY_ID.eq(PROGRAM_ONTOLOGY.ID))
.join(PROGRAM).on(PROGRAM_ONTOLOGY.PROGRAM_ID.eq(PROGRAM.ID))
.join(SCALE).on(TRAIT.SCALE_ID.eq(SCALE.ID))
.join(METHOD).on(TRAIT.METHOD_ID.eq(METHOD.ID))
.where(PROGRAM.ID.eq(programId))
.and(lower(TRAIT.OBSERVATION_VARIABLE_NAME).in(names))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming this set of changes is from merging in the latest develop/1.0 branches to this branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes were for the migration. The updated JOOQ no longer supported how this was being done. It works the same, but the names are now processed on line 503, and the check was moved from the join statement to the where statement. But, it has the same logic as before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

k! Just wanted to make sure!

@ctucker3 ctucker3 requested a review from timparsons May 26, 2022 17:05
@ctucker3 ctucker3 force-pushed the 2.x-micronaut-upgrade branch from d38c1be to 6950a80 Compare June 3, 2022 14:10
@ctucker3 ctucker3 merged commit 6f82f50 into future/1.0 Jun 3, 2022
@ctucker3 ctucker3 deleted the 2.x-micronaut-upgrade branch June 3, 2022 14:34
timparsons pushed a commit that referenced this pull request Jun 15, 2022
timparsons pushed a commit that referenced this pull request Jul 7, 2022
timparsons pushed a commit that referenced this pull request Jul 19, 2022
timparsons pushed a commit that referenced this pull request Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants