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

Javalin 7 API redesign #2091

Open
tipsy opened this issue Feb 7, 2024 · 9 comments
Open

Javalin 7 API redesign #2091

tipsy opened this issue Feb 7, 2024 · 9 comments

Comments

@tipsy
Copy link
Member

tipsy commented Feb 7, 2024

Javalin instance and JavalinConfig APIs Test for "full" api: https://github.com/javalin/javalin/commit/eef4dcc39d448e5d912bafa947517bb5b34a56ea

We currently have this API:

val app = Javalin.create { config ->
    // Jetty
    config.jetty.multipartConfig = MultipartConfig()
    config.jetty.defaultHost = "localhost"
    config.jetty.defaultPort = 8080
    config.jetty.threadPool = QueuedThreadPool()
    config.jetty.addConnector { server, httpConfig -> ServerConnector(server) }
    config.jetty.modifyHttpConfiguration { httpConfig -> }
    config.jetty.modifyServer { server -> }
    config.jetty.modifyServletContextHandler { handler -> }
    config.jetty.modifyWebSocketServletFactory { factory -> }
    // Http and compression
    config.http.defaultContentType = "text/plain"
    config.http.asyncTimeout = 10_000L
    config.http.maxRequestSize = 10_000L
    config.http.generateEtags = true
    config.http.prefer405over404 = true
    config.http.customCompression(CompressionStrategy())
    config.http.brotliAndGzipCompression(3)
    config.http.brotliOnlyCompression(3)
    config.http.gzipOnlyCompression(3)
    config.http.disableCompression()
    // Static files
    config.staticFiles.add("/public")
    config.staticFiles.enableWebjars()
    // Router
    config.router.contextPath = "/api"
    config.router.caseInsensitiveRoutes = true
    config.router.ignoreTrailingSlashes = true
    config.router.treatMultipleSlashesAsSingleSlash = true
    config.router.mount { router ->
        router.before("/hello") { ctx -> }
        router.get("/hello") { ctx -> ctx.result("Hello, World!") }
        router.post("/hello") { ctx -> ctx.result("Hello, World!") }
        router.exception(Exception::class.java) { e, ctx -> }
        router.error(404) { ctx -> ctx.result("Not found") }
        router.after("/hello") { ctx -> }
        router.sse("/sse") { client -> }
        router.ws("/ws") { ws -> }
    }
    config.router.apiBuilder {
        get("/hello") { ctx -> ctx.result("Hello, World!") }
    }
    // Context resolver
    config.contextResolver.fullUrl = { "Test" }
    config.contextResolver.host = { "Test" }
    config.contextResolver.ip = { "Test" }
    config.contextResolver.url = { "Test" }
    config.contextResolver.scheme = { "Test" }
    // Bundled plugnis
    config.bundledPlugins.enableDevLogging()
    config.bundledPlugins.enableRouteOverview("/overview")
    config.bundledPlugins.enableSslRedirects()
    config.bundledPlugins // etc etc
    // Events
    config.events { event ->
        event.serverStarting { println("Server is starting") }
        event.serverStartFailed { println("Server start failed") }
        event.serverStarted { println("Server is started") }
        event.serverStopping { println("Server is stopping") }
        event.serverStopFailed { println("Server stop failed") }
        event.serverStopped { println("Server is stopped") }
        event.handlerAdded {}
        event.wsHandlerAdded {}
    }
    // Request logger
    config.requestLogger.http { ctx, ms -> }
    config.requestLogger.ws { ctx -> }
    // Validation
    config.validation.register(Any::class.java) { }
    // Vue
    config.vue.cacheControl = "Test"
    config.vue.enableCspAndNonces = true
    config.vue.isDev = true
    config.vue.vueInstanceNameInJs = "Test"
    config.vue.isDevFunction = { true }
    config.vue.optimizeDependencies = true
    config.vue.stateFunction = { "Test" }
    config.vue.rootDirectory("Test")
    // Spa root
    config.spaRoot.addFile("/", "index.html")
    config.spaRoot.addHandler("/") { ctx -> }
    // Other
    config.showJavalinBanner = false
    config.startupWatcherEnabled = false
    config.useVirtualThreads = true
    config.jsonMapper(JavalinJackson())
    config.appData(Key("Test"), "Test")
    config.fileRenderer { filePath, model, ctx -> "Test" }
    config.registerPlugin(object : Plugin<Any>() {})
}

app.events { event ->
    event.serverStarting { println("Server is starting") }
    event.serverStartFailed { println("Server start failed") }
    event.serverStarted { println("Server is started") }
    event.serverStopping { println("Server is stopping") }
    event.serverStopFailed { println("Server stop failed") }
    event.serverStopped { println("Server is stopped") }
    event.handlerAdded {}
    event.wsHandlerAdded {}
}
app.before("/hello") { ctx -> }
app.get("/hello") { ctx -> ctx.result("Hello, World!") }
app.post("/hello") { ctx -> ctx.result("Hello, World!") }
app.exception(Exception::class.java) { e, ctx -> }
app.error(404) { ctx -> ctx.result("Not found") }
app.after("/hello") { ctx -> }
app.sse("/sse") { client -> }
app.ws("/ws") { ws -> }
app.start()
app.javalinServlet()
app.jettyServer()
app.port()
app.unsafeConfig()
app.stop()

I've left out a few similar methods and overloads, but all categories should be included.

I think we want to move most of the methods on Javalin into JavalinConfig. We want to keep the simplicity of the Hello World Example though:

fun main() {
    val app = Javalin.create(/*config*/)
        .get("/") { ctx -> ctx.result("Hello World") }
        .start(7070)
}

With the current router, we have:

fun main() {
    val app = Javalin.create { config ->
        config.router.mount { router ->
            router.get("/hello") { ctx -> ctx.result("Hello, World!") }
        }
    }
}

Which is not good enough.

Something like:

fun main() {
    val app = Javalin.create { config ->
        config.router.get("/hello") { ctx -> ctx.result("Hello, World!") }
    }
}

Could be enough to get rid of the direct methods.

Changing the naming around makes it a bit less awkward:

fun main() {
    val javalin = Javalin.create { app ->
        app.router.get("/hello") { ctx -> ctx.result("Hello, World!") }
    }
}

Bigger example:

app.router.before("/hello") { ctx -> }
app.router.get("/hello") { ctx -> ctx.result("Hello, World!") }
app.router.post("/hello") { ctx -> ctx.result("Hello, World!") }
app.router.exception(Exception::class.java) { e, ctx -> }
app.router.error(404) { ctx -> ctx.result("Not found") }
app.router.after("/hello") { ctx -> }
app.router.sse("/sse") { client -> }
app.router.ws("/ws") { ws -> }
app.router.options { option ->
    option.contextPath = "/api"
    option.caseInsensitiveRoutes = true
    option.ignoreTrailingSlashes = true
    option.treatMultipleSlashesAsSingleSlash = true
}

With old syntax:

config.router.contextPath = "/api"
config.router.caseInsensitiveRoutes = true
config.router.ignoreTrailingSlashes = true
config.router.treatMultipleSlashesAsSingleSlash = true
config.router.mount { router ->
    router.before("/hello") { ctx -> }
    router.get("/hello") { ctx -> ctx.result("Hello, World!") }
    router.post("/hello") { ctx -> ctx.result("Hello, World!") }
    router.exception(Exception::class.java) { e, ctx -> }
    router.error(404) { ctx -> ctx.result("Not found") }
    router.after("/hello") { ctx -> }
    router.sse("/sse") { client -> }
    router.ws("/ws") { ws -> }
}
MyClass::class.java vs Myclass::class vs reified We have several APIs that accept java classes, but no kotlin class overrides. We do have some reified alternatives. We should try to standardize this. Using `MyClass.class` and `MyClass::class` is probably the best approach. We have some issues with reified on interfaces.
Getters, setters additive methods
  • We should use methodName(value) for setters (including keyed setters).
  • We should use methodName() for getters (including keyed getters).
  • We should use addMethodName() for additive operations (can be called multiple times)
Different Contexts for before/endpoint/after Not everything is available in each stage. We should have a BaseContext, and let each stage extend. BeforeContext, EndpointContext, AfterContext. Look at what WsContext does.
Fix MultiPartUtil.preUploadFunction It should not be accessible through singleton.
Audit class and method visibility Lots of unintentionally public stuff now
Create better exception classes which extend JavalinException Currently we throw a few RuntimeException, which is a bit lazy. We should fix this.
Create wrappers for template engines javalin-rendering goes away, javalin-rendering-velocity (etc) is introduced
Javalin vs StartedJavalin We should separate this.
  • Javalin.create -> Javalin (.start -> StartedJavalin)
  • Javalin.createAndStart -> StartedJavalin
Make RateLimitUtil configurable on instance (or make it a plugin) Current API:
object RateLimitUtil {
    val limiters = ConcurrentHashMap()
    var keyFunction: (Context) -> String = { ip(it) + it.method() + it.matchedPath() }
    val executor: ScheduledExecutorService = Executors.newSingleThreadScheduledExecutor()
    private fun ip(ctx: Context) = ctx.header("X-Forwarded-For")?.split(",")?.get(0) ?: ctx.ip()
    var responseHeaderFunction = { ctx: Context, timeUnit: TimeUnit, numRequests: Int -> } // no-op by default
}
Rework events api @dzikoysk wants to look at this.
Fix "matchedPath" (routerPath?) matchedPath is a bad name, it should be something like routerPath
@tipsy tipsy changed the title Main Javalin and JavalinConfig APIs for Javalin 7 Javalin 7 API redesign Feb 15, 2024
@jbuhacoff
Copy link

Hello, I'm just getting started with Javalin 6. One comment about this:

Changing the naming around makes it a bit less awkward:

fun main() {
    val app = Javalin.create { app ->
        app.router.get("/hello") { ctx -> ctx.result("Hello, World!") }
    }
}

To me that seems more confusing, instead of less awkward. If "Javalin" is "app", and on the next line outside the anonymous function I try "app.router.get(...)" and it doesn't resolve, it's confusing. Especially when using "val" in Kotlin or "var" in Java where the class isn't explicitly stated, and you have to hover or go exploring to find out what something is.

It's less confusing and less awkward if the arbitrary variable names match more closely to their types, like in the earlier example:

fun main() {
    val app = Javalin.create { config ->
        config.router.get("/hello") { ctx -> ctx.result("Hello, World!") }
    }
}

This makes it clear to me (again, I'm new) that the Javalin "app" and the JavalinConfig "config" are not the same class.

@tipsy
Copy link
Member Author

tipsy commented Feb 18, 2024

Thanks for your feeddback @jbuhacoff, how do you feel about

fun main() {
    val javalin = Javalin.create { app ->
        app.router.get("/hello") { ctx -> ctx.result("Hello, World!") }
    }
}

@dzikoysk
Copy link
Member

I didn't notice this before, but for me, calling the config an "app" sounds quite incorrect and confusing as well. If you really don't want to call this config, it could be a setup or something, but it's still an entry point for app config.

@tipsy
Copy link
Member Author

tipsy commented Feb 18, 2024

but it's still an entry point for app config.

I agree, it's "app config", which is why I called it app. This is similar to

config.staticFiles.add(staticFiles -> {
ssl.withTrustConfig(trust->{
config.bundledPlugins.enableCors(cors -> {

and so on. Singling out this particular xyz-config with the generic name config doesn't improve things IMO.

@zugazagoitia
Copy link
Member

but it's still an entry point for app config.

I agree, it's "app config", which is why I called it app. This is similar to

config.staticFiles.add(staticFiles -> {
ssl.withTrustConfig(trust->{
config.bundledPlugins.enableCors(cors -> {

and so on. Singling out this particular xyz-config with the generic name config doesn't improve things IMO.

Yes it does, because for all of these options you wouldn't hold a reference with the same name, as you would do for your app config, which effectively also "shadows" the first.

@tipsy
Copy link
Member Author

tipsy commented Feb 18, 2024

Yes it does, because for all of these options you wouldn't hold a reference with the same name, as you would do for your app config, which effectively also "shadows" the first.

But that's completely arbitrary? It's only called app because that's what the docs call it currently, if we called it javalin (which is a more natural name based on the type), then this problem goes away?

@zugazagoitia
Copy link
Member

Yes it does, because for all of these options you wouldn't hold a reference with the same name, as you would do for your app config, which effectively also "shadows" the first.

But that's completely arbitrary? It's only called app because that's what the docs call it currently, if we called it javalin (which is a more natural name based on the type), then this problem goes away?

People might still call it that way, since it is an instance of the Javalin "application".
I completely support @dzikoysk names, because we are talking about a consumer of a configuration class, not an instance of the application.
Both of them are present in that very same line, and naming the JavalinConfig class something other than config (or similar) to me is unnecessarily confusing.

TL,DR: Javalin.java is an app while JavalinConfig.java isn't, is a config.

@tipsy
Copy link
Member Author

tipsy commented Feb 18, 2024

The old app instance will basically not have any methods anymore, just .start().
I think app reads a lot more natural than config here:

val javalin = Javalin.create { app ->         
    app.jetty
    app.http
    app.staticFiles
    app.router
    app.contextResolver
    app.bundledPlugins
    app.events
    app.requestLogger
    app.validation
    app.spaRoot
    app.showJavalinBanner
    app.startupWatcherEnabled
    app.useVirtualThreads
    app.jsonMapper()
    app.appData()
    app.fileRenderer()
    app.registerPlugin()
}
val app = Javalin.create { config ->         
    config.jetty.multipartConfig
    config.http.defaultContentType
    config.staticFiles
    config.router
    config.contextResolver
    config.bundledPlugins
    config.events
    config.requestLogger
    config.validation
    config.spaRoot
    config.showJavalinBanner
    config.startupWatcherEnabled
    config.useVirtualThreads
    config.jsonMapper()
    config.appData()
    config.fileRenderer()
    config.registerPlugin()
}
val javalin = Javalin.create { it ->         
    it.jetty
    it.http
    it.staticFiles
    it.router
    it.contextResolver
    it.bundledPlugins
    it.events
    it.requestLogger
    it.validation
    it.spaRoot
    it.showJavalinBanner
    it.startupWatcherEnabled
    it.useVirtualThreads
    it.jsonMapper()
    it.appData()
    it.fileRenderer()
    it.registerPlugin()
}
val app = Javalin.create { config ->         
    config.jetty.multipartConfig
    config.http.defaultContentType
    config.staticFiles
    config.router
    config.contextResolver
    config.bundledPlugins
    config.events
    config.requestLogger
    config.validation
    config.spaRoot
    config.showJavalinBanner
    config.startupWatcherEnabled
    config.useVirtualThreads
    config.jsonMapper()
    config.appData()
    config.fileRenderer()
    config.registerPlugin()
}

@jbuhacoff
Copy link

I think any of these 4 options would be fine, because they give different names to the Javalin instance vs the JavalinConfig instance.

This one would be the least changes to existing documentation:

val javalin = Javalin.create { app ->

Because most of the action is in inside the anonymous function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants