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

Add bolt-jakarta-servlet and bolt-jakarta-jetty modules for Jakarta EE supports #920

Merged
merged 4 commits into from
Jan 27, 2022

Conversation

seratch
Copy link
Contributor

@seratch seratch commented Jan 27, 2022

This pull request partially resolves javax.* dependency transition described #919 by providing the way to switch Web app related dependencies from Java EE Servlet (javax.servlet) to Jakarta EE Servlet (jakarta.servlet). Alsom if we go with this plan, we can resolve #724 in a different approach.

As the majority people in the developer communities are using the Java EE (javax.servlet), we will continue supporting javax.servlet for a long enough time (say, years). That's why we added new dependencies even though these new one are mostly the same with the javax.servlet based ones, except the dependencies and the imports in code.

If you're not familiar with the transition to Jakarta EE, refer to https://blogs.oracle.com/javamagazine/post/transition-from-java-ee-to-jakarta-ee to learn the background.

Code snippets

javax.servlet dependencies (currently available)

dependencies {
  implementation("com.slack.api:bolt:1.18.0-SNAPSHOT")
  implementation("com.slack.api:bolt-servlet:1.18.0-SNAPSHOT")
  implementation("com.slack.api:bolt-jetty:1.18.0-SNAPSHOT")
}
package hello;
import com.slack.api.bolt.App;
import com.slack.api.bolt.jetty.SlackAppServer;
public class MyApp {
  public static void main(String[] args) throws Exception {
    var app = new App();
    var server = new SlackAppServer(app);
    server.start(); // http://localhost:3000/slack/events
  }
}

jakarta.servlet dependencies (will be available in v1.18)

dependencies {
  implementation("com.slack.api:bolt:1.18.0-SNAPSHOT")
  implementation("com.slack.api:bolt-jakarta-servlet:1.18.0-SNAPSHOT")
  implementation("com.slack.api:bolt-jakarta-jetty:1.18.0-SNAPSHOT")
}
package hello;
import com.slack.api.bolt.App;
// This import is the only difference from a user perspective
import com.slack.api.bolt.jakarta_jetty.SlackAppServer;
public class MyApp {
  public static void main(String[] args) throws Exception {
    var app = new App();
    var server = new SlackAppServer(app);
    server.start(); // http://localhost:3000/slack/events
  }
}

Category (place an x in each of the [ ])

  • bolt (Bolt for Java)
  • bolt-{sub modules} (Bolt for Java - optional modules)
  • slack-api-client (Slack API Clients)
  • slack-api-model (Slack API Data Models)
  • slack-api-*-kotlin-extension (Kotlin Extensions for Slack API Clients)
  • slack-app-backend (The primitive layer of Bolt for Java)

Requirements

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to the those rules.

@seratch seratch added enhancement M-T: A feature request for new functionality project:bolt labels Jan 27, 2022
@seratch seratch added this to the 1.18.0 milestone Jan 27, 2022
@seratch seratch self-assigned this Jan 27, 2022
@@ -17,6 +17,7 @@
import java.util.Locale;
import java.util.stream.Collectors;

@Deprecated // Please consider Bolt framework with bolt-servlet adapter instead
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 module is a legacy one, which was added before bolt-java development. I don't think that many people are using this but now it's a good timing to mark this simple module as deprecated and encourage people to use bolt-java for new apps.

I'm not planning to remove this class because we're not in a hurry to remove it as the javax.servlet dependency in this module is optional (=provided scope).

Copy link
Contributor

Choose a reason for hiding this comment

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

This might be good to remember / extract into the changelog for the next release as it is kind of buried deep within this PR (which focusses on providing Jakarta EE support).

@@ -5,6 +5,7 @@

import javax.servlet.http.HttpServletRequest;

@Deprecated // Please consider Bolt framework with bolt-servlet adapter instead
@Slf4j
public class SlackSignatureVerifier {
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 is used only by the above SlackEventsApiServlet class in this SDK. As this class is public, external projects may rely on it. Event after this deprecation, we can keep the class as long as possible (for the reason I mentioned above)

package samples;

import com.slack.api.bolt.App;
import com.slack.api.bolt.jakarta_jetty.SlackAppServer;
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 line of code is the only difference from the existing way using jetty 9.x (=javax.servlet compatible version).


import com.slack.api.bolt.response.Response;

import jakarta.servlet.http.HttpServletRequest;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Imports are changed but they are fully compatible so far. I didn't change any logics this time.

@codecov
Copy link

codecov bot commented Jan 27, 2022

Codecov Report

Merging #920 (830b9c6) into main (caaa7ca) will increase coverage by 0.05%.
The diff coverage is 87.93%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #920      +/-   ##
============================================
+ Coverage     77.27%   77.33%   +0.05%     
- Complexity     3574     3606      +32     
============================================
  Files           388      394       +6     
  Lines         10626    10742     +116     
  Branches       1035     1044       +9     
============================================
+ Hits           8211     8307      +96     
- Misses         1818     1830      +12     
- Partials        597      605       +8     
Impacted Files Coverage Δ
...ent/src/main/java/com/slack/api/rtm/RTMClient.java 39.08% <ø> (ø)
..._backend/events/servlet/SlackEventsApiServlet.java 82.05% <ø> (ø)
...backend/events/servlet/SlackSignatureVerifier.java 66.66% <ø> (ø)
...api/bolt/jakarta_servlet/SlackOAuthAppServlet.java 64.70% <64.70%> (ø)
...k/api/bolt/jakarta_servlet/WebEndpointServlet.java 81.48% <81.48%> (ø)
...olt/jakarta_servlet/WebEndpointServletAdapter.java 88.88% <88.88%> (ø)
...lack/api/bolt/jakarta_servlet/SlackAppServlet.java 94.11% <94.11%> (ø)
...ck/api/bolt/jakarta_servlet/ServletAdapterOps.java 100.00% <100.00%> (ø)
...i/bolt/jakarta_servlet/SlackAppServletAdapter.java 100.00% <100.00%> (ø)
...va/com/slack/api/socket_mode/SocketModeClient.java 70.70% <0.00%> (-3.83%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update caaa7ca...830b9c6. Read the comment docs.

Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Looks good! Lots of code copied around but it is for a good cause: additive support for new runtime without breaking backwards compatibility. Nicely done. Left a couple of questions / comments.

@@ -17,6 +17,7 @@
import java.util.Locale;
import java.util.stream.Collectors;

@Deprecated // Please consider Bolt framework with bolt-servlet adapter instead
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be good to remember / extract into the changelog for the next release as it is kind of buried deep within this PR (which focusses on providing Jakarta EE support).

README.md Outdated
|------------------------------------------------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| [**com.slack.api:bolt**](https://search.maven.org/search?q=g:com.slack.api%20AND%20a:bolt) | Bolt is a framework that offers an abstraction layer to build Slack apps safely and quickly. The most commonly used Servlet environment is supported out-of-the-box. |
| [**com.slack.api:bolt-socket-mode**](https://search.maven.org/search?q=g:com.slack.api%20AND%20a:bolt-socket-mode) | This module offers a handy way to run Bolt apps through [Socket Mode](https://api.slack.com/) connections. |
| [**com.slack.api:bolt-jetty**](https://search.maven.org/search?q=g:com.slack.api%20AND%20a:bolt-jetty) | This module offers a handy way to run Bolt apps on the [Jetty HTTP server](https://www.eclipse.org/jetty/). |
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make explicit that this is Java EE compatible Jetty here? Just like in the line below we make explicit it is Jakarta EE compatible Jetty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I will update these parts before merging this PR!

@seratch seratch merged commit f283e45 into slackapi:main Jan 27, 2022
@seratch seratch deleted the jakarta-servlet-modules branch January 27, 2022 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality project:bolt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jetty upgrades to v11
2 participants