Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to SpringBoot 2.6.1, updating unit test and code accordingly #284

Merged
merged 14 commits into from
Dec 18, 2021

Conversation

mr-c
Copy link
Member

@mr-c mr-c commented Nov 5, 2020

(redo of #280 )

The reported issue is related to the following case:

https://stackoverflow.com/questions/49316751/spring-data-jpa-findone-change-to-optional-how-to-use-this

Solution:

[Editing Pom.xml]

update Spring Boot:

org.springframework.boot spring-boot-starter-parent 2.2.2.RELEASE

add the following dependency for ensuring that we use the appropriate spring-data-commons version.

org.springframework.data spring-data-commons 2.0.1.RELEASE

[WorkflowService.java]

Replace any occurrence of the findOne with findById(id).orElse(null) here:

org.commonwl.view.workflow.WorkflowRepository
org.commonwl.view.workflow.QueuedWorkflowRepository

With these changes we should expect to build successfully.

  • upgrade to Spring Boot 2.6.1 (latest)
  • upgrade to JUnit 5 (comes with Spring Boot, unless we enable the JUnit 4 integration layer)
  • Add a javax.validation implementation
  • Upgrade spring-data-commons to 2.6.0
  • Fix unit tests
  • Upgrade Jena
  • Manual testing to confirm everything works
  • Fix GH Actions build

@mr-c
Copy link
Member Author

mr-c commented Nov 5, 2020

Error creating bean with name 'mongoConfig': Unsatisfied dependency expressed through field 'mongoMappingContext'; nested exception is org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'mongoMappingContext' defined in class path resource [org/springframework/boot/autoconfigure/data/mongo/MongoDataConfiguration.class]: Initialization of bean failed; nested exception is java.lang.NoSuchMethodError: org.springframework.data.mapping.context.AbstractMappingContext.setApplicationContext(Lorg/springframework/context/ApplicationContext;)V

https://travis-ci.com/github/common-workflow-language/cwlviewer/builds/198507618#L1273

@mr-c
Copy link
Member Author

mr-c commented Dec 16, 2021

@kinow I assigned you this PR, but feel free to make a new branch

@kinow
Copy link
Member

kinow commented Dec 16, 2021

Rebased.

<groupId>org.hibernate.validator</groupId>
<artifactId>hibernate-validator</artifactId>
<version>6.0.13.Final</version>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

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

Looks like javax.validation implementation is not included, so had to import the Hibernate validator (this is not ORM, just the validation part, which is a reference implementation of JSR-303)

@kinow
Copy link
Member

kinow commented Dec 17, 2021

(checklist moved to first comment, so it is visible on the issues page)

@mr-c
Copy link
Member Author

mr-c commented Dec 17, 2021

Thanks @kinow ! I moved your checklist to the first comment so that it shows up on the issues browser. Let me know if you don't have the permissions to edit it further.

@kinow
Copy link
Member

kinow commented Dec 17, 2021

Thanks @kinow ! I moved your checklist to the first comment so that it shows up on the issues browser. Let me know if you don't have the permissions to edit it further.

That'll be easier to update and for others to see too. Thanks @mr-c !

@kinow
Copy link
Member

kinow commented Dec 18, 2021

Huh, funny, mvnw test failed on GH Actions, but passing locally. Will upgrade the remaining libraries, have a look at miniwdl + types, and revisit this PR later (to give my 🧠 a break 😄 )

@kinow
Copy link
Member

kinow commented Dec 18, 2021

Manual tests passed with docker-compose up --build. Unit tests passing locally. Last part now, fix test in GH Actions, then it should be ready for review! 🎉

@kinow
Copy link
Member

kinow commented Dec 18, 2021

All right. The MongoDB test passed locally, because the tests were run sequentially. On GitHub Actions they were running in parallel, so there was a conflict with ports. Setting the port number to 0 makes SpringData/MongoDB to allocate a random port 👍

@kinow
Copy link
Member

kinow commented Dec 18, 2021

@mr-c this PR should upgrade SpringBoot to 2.6.1, the latest release. The API changed a little from 1.5.x to 2.x, and also to 2.6.1. Removed some deprecated code, and fixed settings and Java annotations so that all tests would pass. Tested with the docker-compose.yml file, after replacing image by the commented build.

I don't know enough about the production environment to say it will work flawlessly when merged & deployed. So, if possible, a backup when upgrading would be recommended IMO.

Let me know if you know what would be the next steps here, or if I need to talk with someone else to see it merged & deployed 🎉

-Bruno

@mr-c
Copy link
Member Author

mr-c commented Dec 18, 2021

@kinow this is fantastic! Deployment is manual, so no worries about merging this PR. Do you want me to squash this down to one commit, or do you want to cleanup these commits and then I'll rebase onto the primary branch?

pom.xml Show resolved Hide resolved
@kinow
Copy link
Member

kinow commented Dec 18, 2021

@kinow this is fantastic! Deployment is manual, so no worries about merging this PR. Do you want me to squash this down to one commit, or do you want to cleanup these commits and then I'll rebase onto the primary branch?

Forgot about squashing commits. Done now 🙂 Thanks!

@mr-c
Copy link
Member Author

mr-c commented Dec 18, 2021

Oh, I can always easily squash down to one commit myself in the web interface. I wondered if you preferred one big commit or a series of fewer commits with cleaned up descriptions.

@mr-c mr-c enabled auto-merge (rebase) December 18, 2021 09:38
@mr-c mr-c disabled auto-merge December 18, 2021 09:39
@mr-c
Copy link
Member Author

mr-c commented Dec 18, 2021

@kinow I force pushed with correct attribution. You wrote a lot of nice commit messages, and some of them should be saved. But since I didn't have a local checkout prior to your squash I can't regroup your original commits. Would you mind doing so? (next week is fine!)

I did make a local backup of view.commonwl.org via the instructions at https:/common-workflow-language/cwlviewer#dumprestore and I did a partial restore until I got bored. Seems fine!

@kinow kinow force-pushed the spring_boot_upgrade branch 2 times, most recently from 1b8a573 to a75d12c Compare December 18, 2021 10:36
etzanis and others added 4 commits December 18, 2021 23:40
Had to add a new dependency for the embedded MongoDB server used for tests.

The version of MongoDB used for tests (embedded) also needs to be defined now. And
MongoDB/SpringData integration appears to be more strict. A query like
{ obj.property: ?0 } failed, and took me a long time to recognize that it was missing
quotes, so { 'obj.property': ?0 } fixed it.

Finally, there was also an issue with serialization. For some reason now the Mongo
repository was failing to serialize a response for Workflow. The object contained a
final String that was used for the base URL of permanent links, and Mongo/Spring/etc
wanted a way to set that value in the constructor. Tried the @PersistenceConstructor
annotation but that didn't work. In the end it was easy to fix it by making the
final String now a constant
@kinow
Copy link
Member

kinow commented Dec 18, 2021

@kinow I force pushed with correct attribution.

Did I replace an author somewhere, or forgot a commit? I thought perhaps you meant the co-author bit that appears after I rebase; but I don't know how to remove it. Hopefully I didn't mess it up now...

You wrote a lot of nice commit messages, and some of them should be saved. But since I didn't have a local checkout prior to your squash I can't regroup your original commits. Would you mind doing so? (next week is fine!)

Thanks! I remember adding some links in the commits to explain where settings/changes were coming from. But there are two commits fixing unit tests which could be combined perhaps. The commit message could be merged into a single message with links & notes.

Feel free to re-organize the commits if you'd like @mr-c. I used git reflog to reset to before my rebase, and cherry-pick your commits. I hope I didn't miss anything.

I did make a local backup of view.commonwl.org via the instructions at https:/common-workflow-language/cwlviewer#dumprestore and I did a partial restore until I got bored. Seems fine!

Hooray! 🎉

Thanks!

@mr-c
Copy link
Member Author

mr-c commented Dec 18, 2021

@kinow I force pushed with correct attribution.

Did I replace an author somewhere, or forgot a commit? I thought perhaps you meant the co-author bit that appears after I rebase; but I don't know how to remove it.

No, the issue was your commit didn't list you as an author! :-)

Hopefully I didn't mess it up now...
Everything looks good, thanks!

Feel free to re-organize the commits if you'd like @mr-c. I used git reflog to reset to before my rebase, and cherry-pick your commits. I hope I didn't miss anything.

I did a local check and all is good; thanks! I'll do some light commit combining with git rebase -i and I'll force push

@mr-c mr-c changed the title Restart spring boot upgrade Upgrade to SpringBoot 2.6.1, updating unit test and code accordingly Dec 18, 2021
@mr-c mr-c enabled auto-merge (rebase) December 18, 2021 10:59
@mr-c mr-c merged commit 9444d7e into main Dec 18, 2021
@mr-c mr-c deleted the spring_boot_upgrade branch December 18, 2021 11:07
@kinow
Copy link
Member

kinow commented Dec 18, 2021

No, the issue was your commit didn't list you as an author! :-)

🤣 ops!

Thanks @mr-c !

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.

3 participants