Skip to content

Bolt micronaut/micronaut 4 upgrade #1294

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.

Sign up for GitHub

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

Closed
hrothwell wants to merge 16 commits into from
Closed

Bolt micronaut/micronaut 4 upgrade #1294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
b9b185b
maybe CI/CD will work but not local, im not familiar with maven
hrothwell Mar 22, 2024
1d3d4e7
remove comment
hrothwell Mar 22, 2024
0800da6
remove unused imports
hrothwell Mar 22, 2024
fa8f6fc
spacing
hrothwell Mar 22, 2024
4d19dd6
cleanup
hrothwell Mar 22, 2024
5121dfe
update pom.xmls
hrothwell Mar 22, 2024
bf07bcb
revert pom.xml
hrothwell Mar 25, 2024
25bc7ac
remove blank lines in micronaut pom.xml
hrothwell Mar 25, 2024
9f296ab
revert slack app micronaut adapter blank line removal
hrothwell Mar 25, 2024
5d6afdc
remove mocking of App bean, not required and fixes test from breaking
hrothwell Mar 26, 2024
0e03acc
update workflow + script file to handle jdk 17
hrothwell Mar 26, 2024
ad42fb4
jdk 17 build
hrothwell Mar 28, 2024
acb7790
remove unneeded blank line changes
hrothwell Mar 28, 2024
ae8c66c
Merge branch 'main' into bolt-micronaut/micronaut-4
hrothwell Mar 28, 2024
734749c
only add opens when running with jdk 17
hrothwell Mar 28, 2024
835d9bb
Merge branch 'main' into bolt-micronaut/micronaut-4
hrothwell Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions .github/workflows/unit-tests-jdk-17.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
name: JDK 17 Build & Tests

on:
push:
branches: [ main ]
pull_request:

jobs:
build:
runs-on: ubuntu-latest
timeout-minutes: 12
strategy:
matrix:
java-version: ['17']

steps:
- uses: actions/checkout@v3
- name: Install JDK
uses: actions/setup-java@v3
with:
java-version: ${{ matrix.java-version }}
distribution: 'adopt'
- name: Run all tests
run: |
./scripts/run_no_prep_tests.sh -ci
env:
SKIP_UNSTABLE_TESTS: 1
- name: Upload coverage to Codecov
uses: codecov/codecov-action@v3
with:
fail_ci_if_error: true
17 changes: 13 additions & 4 deletions bolt-micronaut/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,18 @@
</parent>

<properties>
<!-- TODO: upgrading to v4 -->
<micronaut.version>3.9.4</micronaut.version>
<micronaut-test-junit5.version>3.9.2</micronaut-test-junit5.version>
<micronaut-rxjava3.version>2.4.1</micronaut-rxjava3.version>
<micronaut.version>4.3.12</micronaut.version>
<micronaut-test-junit5.version>4.2.1</micronaut-test-junit5.version>
<micronaut-rxjava3.version>3.2.1</micronaut-rxjava3.version>
<junit5-jupiter.version>5.10.2</junit5-jupiter.version>
<!-- Note that upgrading this library breaks other dependency resolution -->
<mockito-all.version>1.10.19</mockito-all.version>
<jakarta.inject.version>2.0.1.MR</jakarta.inject.version>
<!-- Micronaut 4 requires jdk 17 -->
<jdk.version>17</jdk.version>
<release.version>17</release.version>
<maven.compiler.source>17</maven.compiler.source>
<maven.compiler.target>17</maven.compiler.target>
</properties>

<artifactId>bolt-micronaut</artifactId>
Expand Down Expand Up @@ -57,6 +61,11 @@
<artifactId>micronaut-http-server-netty</artifactId>
<version>${micronaut.version}</version>
</dependency>
<dependency>
<groupId>io.micronaut</groupId>
<artifactId>micronaut-jackson-databind</artifactId>
<version>${micronaut.version}</version>
</dependency>

<dependency>
<groupId>io.micronaut</groupId>
Expand Down
4 changes: 2 additions & 2 deletions bolt-micronaut/src/main/java/com/slack/api/bolt/micronaut/SlackAppController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import io.micronaut.http.HttpRequest;
import io.micronaut.http.HttpResponse;
import io.micronaut.http.MediaType;
import io.micronaut.http.annotation.Body;
import io.micronaut.http.annotation.Controller;
import io.micronaut.http.annotation.Get;
import io.micronaut.http.annotation.Post;
Expand All @@ -29,7 +28,8 @@ public SlackAppController(App slackApp, SlackAppMicronautAdapter adapter) {
}

@Post(value = "/events", consumes = {MediaType.APPLICATION_FORM_URLENCODED, MediaType.APPLICATION_JSON})
public HttpResponse<String> events(HttpRequest<String> request, @Body String body) throws Exception {
public HttpResponse<String> events(HttpRequest<String> request) throws Exception {
String body = request.getBody().orElse(null);
Comment on lines +31 to +32
Copy link
Contributor Author

@hrothwell hrothwell Mar 22, 2024

Choose a reason for hiding this comment

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

issue in MN 4 - micronaut-projects/micronaut-core#9694

Copy link
Contributor

@seratch seratch Mar 22, 2024

Choose a reason for hiding this comment

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

ok, thanks for the workaround

return adapt(request, body);
}

Expand Down
6 changes: 3 additions & 3 deletions bolt-micronaut/src/test/java/test_locally/AdapterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import com.slack.api.bolt.micronaut.SlackAppMicronautAdapter;
import com.slack.api.bolt.request.Request;
import com.slack.api.bolt.response.Response;
import io.micronaut.core.convert.DefaultConversionService;
import io.micronaut.core.convert.DefaultMutableConversionService;
import io.micronaut.http.HttpRequest;
import io.micronaut.http.HttpResponse;
import io.micronaut.http.simple.SimpleHttpHeaders;
Expand Down Expand Up @@ -32,11 +32,11 @@ public void toSlackRequest() {
HttpRequest<String> req = mock(HttpRequest.class);
Map<String, String> rawHeaders = new HashMap<>();
rawHeaders.put("X-Slack-Signature", "xxxxxxx");
SimpleHttpHeaders headers = new SimpleHttpHeaders(rawHeaders, new DefaultConversionService());
SimpleHttpHeaders headers = new SimpleHttpHeaders(rawHeaders, new DefaultMutableConversionService());
when(req.getHeaders()).thenReturn(headers);
Map<CharSequence, List<String>> params = new HashMap<>();
params.put("foo", Arrays.asList("bar", "baz"));
SimpleHttpParameters parameters = new SimpleHttpParameters(params, new DefaultConversionService());
SimpleHttpParameters parameters = new SimpleHttpParameters(params, new DefaultMutableConversionService());
when(req.getParameters()).thenReturn(parameters);

Request<?> slackRequest = adapter.toSlackRequest(req, "token=random&ssl_check=1");
Expand Down
10 changes: 6 additions & 4 deletions bolt-micronaut/src/test/java/test_locally/ControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import com.slack.api.bolt.AppConfig;
import com.slack.api.bolt.micronaut.SlackAppController;
import com.slack.api.bolt.micronaut.SlackAppMicronautAdapter;
import io.micronaut.core.convert.DefaultConversionService;
import io.micronaut.core.convert.DefaultMutableConversionService;
import io.micronaut.http.HttpRequest;
import io.micronaut.http.HttpResponse;
import io.micronaut.http.MutableHttpParameters;
Expand All @@ -20,6 +20,7 @@
import util.AuthTestMockServer;

import java.util.HashMap;
import java.util.Optional;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.IsEqual.equalTo;
Expand Down Expand Up @@ -62,12 +63,13 @@ public void test() throws Exception {
assertNotNull(controller);

HttpRequest<String> req = mock(HttpRequest.class);
SimpleHttpHeaders headers = new SimpleHttpHeaders(new HashMap<>(), new DefaultConversionService());
SimpleHttpHeaders headers = new SimpleHttpHeaders(new HashMap<>(), new DefaultMutableConversionService());
when(req.getHeaders()).thenReturn(headers);
SimpleHttpParameters parameters = new SimpleHttpParameters(new HashMap<>(), new DefaultConversionService());
SimpleHttpParameters parameters = new SimpleHttpParameters(new HashMap<>(), new DefaultMutableConversionService());
when(req.getParameters()).thenReturn(parameters);
when(req.getBody()).thenReturn(Optional.of("token=random&ssl_check=1"));

HttpResponse<String> response = controller.events(req, "token=random&ssl_check=1");
HttpResponse<String> response = controller.events(req);
assertEquals(200, response.getStatus().getCode());
}

Expand Down
9 changes: 5 additions & 4 deletions bolt-micronaut/src/test/java/test_locally/app/CommandsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import io.micronaut.test.annotation.MockBean;
import io.micronaut.test.extensions.junit5.annotation.MicronautTest;
import jakarta.inject.Inject;
import jakarta.inject.Singleton;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

Expand Down Expand Up @@ -51,7 +52,7 @@ public class CommandsTest {
SlackSignature.Generator signatureGenerator = new SlackSignature.Generator(signingSecret);

@Primary
@MockBean(AppConfig.class)
Copy link
Contributor Author

@hrothwell hrothwell Mar 26, 2024

Choose a reason for hiding this comment

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

This seemed to be the issue with the test locally. I debugged and tracked the issue down eventually and I believe there was an issue where this was being treated as a mock but the bean that was returned wasn't actually a mock, so I just made this a normal singleton and it seems to have fixed the issue.

Partially found this as I also just ran the application locally and was able to properly instantiate the controller that was having issues in the test, main difference being the AppConfig bean so figured it had to be around that.

@Singleton
AppConfig mockSlackAppConfig() throws IOException, SlackApiException {
AppConfig config = AppConfig.builder().signingSecret(signingSecret).singleTeamBotToken(botToken).build();
config.setSlack(mockSlack());
Expand All @@ -76,7 +77,7 @@ Slack mockSlack() throws IOException, SlackApiException {
public void command() {
MutableHttpRequest<String> request = HttpRequest.POST("/slack/events", "");
request.header("Content-Type", "application/x-www-form-urlencoded");
String timestamp = "" + (System.currentTimeMillis() / 1000);
String timestamp = String.valueOf(System.currentTimeMillis() / 1000);
Copy link
Contributor

@seratch seratch Mar 22, 2024

Choose a reason for hiding this comment

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

Thanks, looks good to me

request.header(SlackSignature.HeaderNames.X_SLACK_REQUEST_TIMESTAMP, timestamp);
String signature = signatureGenerator.generate(timestamp, helloBody);
request.header(SlackSignature.HeaderNames.X_SLACK_SIGNATURE, signature);
Expand All @@ -90,7 +91,7 @@ public void command() {
public void invalidSignature() {
MutableHttpRequest<String> request = HttpRequest.POST("/slack/events", "");
request.header("Content-Type", "application/x-www-form-urlencoded");
String timestamp = "" + (System.currentTimeMillis() / 1000 - 30 * 60);
String timestamp = String.valueOf(System.currentTimeMillis() / 1000 - 30 * 60);
request.header(SlackSignature.HeaderNames.X_SLACK_REQUEST_TIMESTAMP, timestamp);
String signature = signatureGenerator.generate(timestamp, helloBody);
request.header(SlackSignature.HeaderNames.X_SLACK_SIGNATURE, signature);
Expand Down Expand Up @@ -122,7 +123,7 @@ public void invalidSignature() {
public void regexp_matching() {
MutableHttpRequest<String> request = HttpRequest.POST("/slack/events", "");
request.header("Content-Type", "application/x-www-form-urlencoded");
String timestamp = "" + (System.currentTimeMillis() / 1000);
String timestamp = String.valueOf(System.currentTimeMillis() / 1000);
request.header(SlackSignature.HeaderNames.X_SLACK_REQUEST_TIMESTAMP, timestamp);
String signature = signatureGenerator.generate(timestamp, submissionBody);
request.header(SlackSignature.HeaderNames.X_SLACK_SIGNATURE, signature);
Expand Down
18 changes: 18 additions & 0 deletions bolt-micronaut/src/test/java/test_locally/app/MicronautVersionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package test_locally.app;

import io.micronaut.core.version.VersionUtils;
import io.micronaut.test.extensions.junit5.annotation.MicronautTest;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

@MicronautTest
public class MicronautVersionTest {

@Test
public void expectedMicronautVersion() {
String version = VersionUtils.getMicronautVersion();
Assertions.assertEquals("4.3.12", version);
}

}

1 change: 0 additions & 1 deletion bolt-micronaut/src/test/resources/logback.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
<configuration>
<appender name="STDOUT" class="ch.qos.logback.core.ConsoleAppender">
<withJansi>true</withJansi>
<encoder>
<pattern>%cyan(%d{HH:mm:ss.SSS}) %gray([%thread]) %highlight(%-5level) %magenta(%logger{36}) - %msg%n</pattern>
</encoder>
Expand Down
23 changes: 23 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@
<maven-versions-plugin.version>2.8.1</maven-versions-plugin.version>
<nexus-staging-maven-plugin.version>1.6.8</nexus-staging-maven-plugin.version>
<jacoco-maven-plugin.version>0.8.7</jacoco-maven-plugin.version>
<maven-surefire-plugin.version>3.2.5</maven-surefire-plugin.version>
<dokka.version>1.6.10</dokka.version>
</properties>

Expand Down Expand Up @@ -401,6 +402,28 @@
</plugins>
</build>
</profile>
<profile>
<id>building-with-jdk-17</id>
<activation>
<jdk>17</jdk>
</activation>
<build>
<plugins>
<!-- java 9+ fix for tests, open modules in java.lang -->
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<version>${maven-surefire-plugin.version}</version>
<configuration>
<argLine>
--add-opens java.base/java.lang=ALL-UNNAMED
--add-opens java.base/java.util=ALL-UNNAMED
</argLine>
</configuration>
</plugin>
</plugins>
</build>
</profile>
</profiles>

</project>
18 changes: 16 additions & 2 deletions scripts/run_no_prep_tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ flags()
flags "$@"

is_jdk_8=`echo $JAVA_HOME | grep 8.`
is_jdk_14=`echo $JAVA_HOME | grep 14.`
Copy link
Contributor

@seratch seratch Mar 28, 2024

Choose a reason for hiding this comment

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

Thanks for the improvements! Some tests in slack-api-client project fail due to #892 (this is not a bug, just incompatibility with newer JDK versions, which affects only test code execution). Can you add is_jdk_17 and run tests only for -pl bolt-micronaut?

Copy link
Contributor Author

@hrothwell hrothwell Mar 28, 2024

Choose a reason for hiding this comment

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

At a quick glance, that issue may be similar to what I called out here.

I'll go with checking is_jdk_17 for now like you said, and maybe I can eventually take a look at that other issue as well.

Copy link
Contributor Author

@hrothwell hrothwell Mar 28, 2024

Choose a reason for hiding this comment

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

Actually, since bolt-micronaut depends on slack-api-client I'm not quite sure if that will work, as we will need to build that module in order to build bolt-micronaut if I am understanding correctly. I'm trying a few things locally right now, but my thought is that adding similar opening of jdk modules as I described above may be the simplest solution

Copy link
Contributor

@seratch seratch Mar 28, 2024

Choose a reason for hiding this comment

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

My understanding is that compiling all the modules never fails even with JDK 17 while some of them can fail when executing their test code. Thus, it should be feasible to compile slack-api-client (and others) and run only bolt-micronaut tests for now.

Copy link
Contributor Author

@hrothwell hrothwell Mar 28, 2024

Choose a reason for hiding this comment

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

Ive got the run_no_prep_tests.sh running locally on jdk 17 with very minor changes to the root pom.xml, I will quick push it up in case it is fine to just wrap in with these changes or else can try something else

Copy link
Contributor Author

@hrothwell hrothwell Mar 28, 2024

Choose a reason for hiding this comment

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

tried something new: added a new profile for when building with jdk17 to configure the surefire plugin to add the necessary --add-opens, tested on jdk 8 and jdk 17 locally and ./scripts/run_no_prep_tests.sh ran fine on both

I tried with just changing command line arguments but I could not seem to quite figure out how to get maven to just compile the dependencies needed for bolt-micronaut and not run the tests for them in the process.

Copy link
Contributor

@seratch seratch Mar 29, 2024

Choose a reason for hiding this comment

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

@hrothwell Indeed, there is no simple way to do it, so I came up with a solution to do mvn install first and then run the tests with them: ad0774e

Copy link
Contributor Author

@hrothwell hrothwell Mar 29, 2024

Choose a reason for hiding this comment

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

I thought the changes in the base pom.xml should have resolved the test issues as it worked for me on jdk 8 / 17 locally, I'm unsure exactly why the last few CodeCov tasks failed, but the tests all ran by the looks of it with the initial changes I had. So we could potentially mark #892 as complete also

Copy link
Contributor

@seratch seratch Mar 29, 2024

Choose a reason for hiding this comment

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

Oh I see. Will check if it works for me too later on!

Copy link
Contributor Author

@hrothwell hrothwell Mar 29, 2024

Choose a reason for hiding this comment

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

I did open #1296 which is the same changes I had originally so you wouldn't have to make the same changes again if you wanted to test it at all 👍


is_travis_jdk_8=`echo $TRAVIS_JDK | grep openjdk8`
if [[ "${is_jdk_8}" != "" && "${is_travis_jdk_8}" != "" ]];
then
Expand All @@ -25,20 +27,32 @@ then
-pl !bolt-quarkus-examples \
-pl !bolt-jakarta-servlet \
-pl !bolt-jakarta-jetty \
-pl !bolt-micronaut \
clean \
test-compile \
'-Dtest=test_locally.**.*Test' -Dsurefire.failIfNoSpecifiedTests=false test ${CI_OPTIONS}\
'-Dtest=test_locally.**.*Test' -Dsurefire.failIfNoSpecifiedTests=false test ${CI_OPTIONS} \
-DfailIfNoTests=false \
-Dhttps.protocols=TLSv1.2 \
--no-transfer-progress && \
if git status --porcelain | grep .; then git --no-pager diff; exit 1; fi
else
elif [[ "${is_jdk_14}" != "" ]];
then
./mvnw ${MAVEN_OPTS} \
-pl !bolt-micronaut \
clean \
test-compile \
'-Dtest=test_locally.**.*Test' -Dsurefire.failIfNoSpecifiedTests=false test ${CI_OPTIONS} \
-DfailIfNoTests=false \
-Dhttps.protocols=TLSv1.2 \
--no-transfer-progress && \
if git status --porcelain | grep .; then git --no-pager diff; exit 1; fi
else
./mvnw ${MAVEN_OPTS} \
clean \
test-compile \
'-Dtest=test_locally.**.*Test' -Dsurefire.failIfNoSpecifiedTests=false test ${CI_OPTIONS} \
-DfailIfNoTests=false \
-Dhttps.protocols=TLSv1.2 \
--no-transfer-progress && \
if git status --porcelain | grep .; then git --no-pager diff; exit 1; fi
fi