Javalin: EmbeddedJavalin does not reject all enableStaticFiles overloads

[stevebarham]

Actual behavior (the bug)
Calling .enableStaticFiles("/foo", Location.CLASSPATH) on an EmbeddedJavalin instance does not yield an exception as a call to .enableStaticFiles("/foo") would, but the registration is silently ignored.

Expected behavior
An exception indicating that the call is unsupported in embedded mode.

To Reproduce
Steps to reproduce the behavior

Additional context
Looks like we need an additional override on EmbeddedJavalin of the form:

override fun enableStaticFiles(classpathPath: String, loc: Location) = notAvailable("enableStaticFiles()")

[tipsy]

Thanks! Would you like to submit a PR?

[stevebarham]

Sure; won't be until this evening though unfortunately!

[tipsy]

I'd still be happy to merge this.

[tobiasdrechsel]

should I give it a try? I don't have much experience with Kotlin but I think I can figure it out

[tipsy]

Please go ahead :)

[tobiasdrechsel]

If I'm not wrong the actual change is just one line. I already did that.

But I looked a little bit deeper and it raised some questions for me. Because of the overwrite of the start() method, the effect of ensureActionIsPerformedBeforeServerStart("...") is completely prohibited as start() will never be called when EmbeddedJavalin is used.

I might be wrong but I think we could do the following:

 init {
        Util.noJettyStarted = false // embeddable doesn't use Jetty
        started = true
    }

This would give us some benefits:

1. we can remove some code from EmbeddedJavalin because the ensureActionIsPerformedBeforeServerStart("...") does the job already.

Before:

    override fun enableStaticFiles(path: String, location: Location) = notAvailable("enableStaticFiles()")
    override fun enableStaticFiles(classpathPath: String) = notAvailable("enableStaticFiles()")
    override fun enableWebJars() = notAvailable("enableWebJars()")
    override fun port() = notAvailable("port()")
    override fun port(port: Int) = notAvailable("port(port)")
    override fun server(server: Supplier<Server>) = notAvailable("server()")
    override fun sessionHandler(sessionHandler: Supplier<SessionHandler>) = notAvailable("sessionHandler()")
    override fun start() = notAvailable("start()")
    override fun stop() = notAvailable("stop()")
    override fun ws(path: String, ws: Consumer<WsHandler>) = notAvailable("WebSockets functionality")
    override fun wsLogger(ws: Consumer<WsHandler>) = notAvailable("WebSockets functionality")
    private fun notAvailable(action: String): Nothing = throw RuntimeException("$action is not available in standalone mode")

After:

    override fun stop() = notAvailable("stop()")
    override fun ws(path: String, ws: Consumer<WsHandler>) = notAvailable("WebSockets functionality")
    private fun notAvailable(action: String): Nothing = throw RuntimeException("$action is not available in standalone mode")

1. Preventing future bugs
   2.1 A developer who is unaware of the EmbeddedJavalin implementation might rely on the ensureActionIsPerformedBeforeServerStart("...") and he expects his code gets only executed before the embedded web server starts.
   2.2 When a new method is added to the Javalin and it needs to be executed before javalin starts you also need to add an overwrite to EmbeddedJavalin IMHO this is error prone.

I'm making some assumptions and I'm not really sure if the started = true in the init block would have no side effects as there are no test implementations available (which is reasonable).

What are your thoughts on this @tipsy ?

[tipsy]

If I'm not wrong the actual change is just one line. I already did that.

That's correct.

I'm making some assumptions and I'm not really sure if the started = true in the init block would have no side effects as there are no test implementations available (which is reasonable).

There will be side-effects. For example Javalin#contextPath is used both by JavalinServlet and JettyServerUtil (the util that sets up the Jetty server). If you set started = true, then it's no longer possible to configure JavalinServlet through Javalin#contextPath.

The whole concept of a standalone Javalin was "slapped on" to the existing code-base. The architecture was not supposed to support it, so there is no clean separation between what in Javalin that configures just the JavalinServlet and what configures the embedded Jetty server.

[tobiasdrechsel]

I see, well I have submitted a pull request to fix the actual issue.

The whole concept of a standalone Javalin was "slapped on" to the existing code-base. The architecture was not supposed to support it, so there is no clean separation between what in Javalin that configures just the JavalinServlet and what configures the embedded Jetty server.

Maybe this is something you can approach in Javalin 3.0.

[tipsy]

Yes, it's one of the main things I want to address. There's an issue for Javalin 3 that talks about it briefly.