perwendel / spark

A simple expressive web framework for java. Spark has a kotlin DSL https://github.com/perwendel/spark-kotlin
Apache License 2.0
9.64k stars 1.56k forks source link

Do not use ResourceHandler to serve static files, use DefaultServlet #316

Open joakime opened 9 years ago

joakime commented 9 years ago

See: https://github.com/perwendel/spark/blob/2.2/src/main/java/spark/webserver/SparkServer.java#L238-L242

Back during Jetty 6, this made sense, but the Jetty project has moved on.

The ResourceHandler is inappropriate for production quality serving of static files, it lacks lots of features that the DefaultServlet is better suited for.

Features lacking in ResourceHandler:

See example on stackoverflow for usage of 2 DefaultServlets (the normal one, and an alternate one)

http://stackoverflow.com/a/20223103/775715

tipsy commented 7 years ago

@joakime not sure if you're still around, but I'll try anyway. Static file handling has never really been high priority for Spark, but it's caused us disproportionate amounts of work and trouble. Do you know how much effort it would be to switch to DefaultServlet, and if we could get rid of most of our less-than-stellar resource-handling code?

ruurd commented 7 years ago

Short term solution: put an nginx instance in front of it that serves the static content and puts through any dynamic content to spark. Yes. I know. Hardly the simple solution I always advocate but if you're in a pinch it is one of the alternatives.

tipsy commented 7 years ago

@ruurd That's not really solving any problem. We have about 50kb of resource-handling code in Spark that I would like to get rid of.

joakime commented 7 years ago

@tipsy I'm still here. I can provide a patch, but I'm not 100% familiar with "the spark way" so it might be a few tries before it fits into the structure you have.

tipsy commented 7 years ago

Don't worry about that, I would be happy to review.

joakime commented 7 years ago

I'll whip something up on monday

joakime commented 7 years ago

Started to dig into this a moment ago, but have some thoughts.

Spark.staticFileLocation(String folder) and Spark.externalStaticFileLocation(String location) are the apparent entry points.

If staticFileLocation() is used, then this should be the static resources served from the / context-path. This would be the ServletContextHandler.setBaseResource() setting.

If externalStaticFileLocation() is used, then which context-path should this be served under?

I think the addition of these static resource locations should be delegated to the EmbeddedServer interface.

The parameter for both staticFileLocation(String) and externalStaticFielLocation(String) can be an absolute URI (for file:// and jar:file://), a filesystem path (relative to user.dir, or absolute), or a classpath resource location (to be looked up). This follows the org.eclipse.jetty.util.resource.Resource.newResource() behavior in Jetty as well.

I see you are trying to manipulate the MimeTypes in a generic way, just have to be aware of that when implementing the static file serving too. as the MimeTypes jetty uses has 3+ parts (the extension, the mime-type, the encoding, and soon also the content-disposition setting)

There could be other exposed features of the static file serving too.

All of this makes me wonder if the entire StaticFiles and StaticFilesConfiguration still make sense?

The staticFileLocation() method in particular seems like the wrong place (timing of startup wise).

That is an immutable location associated with the javax.servlet.ServletContext and all embedded servlet servers will have the same requirement, to declare the base resource location for the ServletContext. That location should be defined before you attempt to start/create the ServletContext (or ServletContextHandler) as a proper location is needed for any library wanting to use the following ServletContext methods during the startup lifecycle of the servlet/context/container.

tipsy commented 7 years ago

Let me prefix this with a thanks, and an acknowledgement that you seem to know a lot more about this than I do. I will attempt to answer your questions, but I'm not at all familiar with the current code we have here.

Spark.staticFileLocation(String folder) and Spark.externalStaticFileLocation(String location) are the apparent entry points. If staticFileLocation() is used, then this should be the static resources served from the / context-path. This would be the ServletContextHandler.setBaseResource() setting. If externalStaticFileLocation() is used, then which context-path should this be served under?

Also /. From a Spark-user-perspective the API shouldn't care if the source is in classpath or external. Does that make sense? There is also a wish for supporting multiple calls to staticFileLocation() (#734)

I think the addition of these static resource locations should be delegated to the EmbeddedServer interface.

That would mean static files would only work in embedded mode. I asked Per about this before, and he doesn't like the idea. Why do you think it should be like this?

There could be other exposed features of the static file serving too. LIST The staticFileLocation() method in particular seems like the wrong place (timing of startup wise). That is an immutable location associated with the javax.servlet.ServletContext and all embedded servlet servers will have the same requirement, to declare the base resource location for the ServletContext. That location should be defined before you attempt to start/create the ServletContext (or ServletContextHandler) as a proper location is needed for any library wanting to use the following ServletContext methods during the startup lifecycle of the servlet/context/container.

The static file location must be set before route-mapping, which initializes the server. This sounds correct according to what you just said (?).

All of this makes me wonder if the entire StaticFiles and StaticFilesConfiguration still make sense?

In what way doesn't the API make sense, and what would the alternative be? Spark abstracts away a lot of things, as our main focus is keeping the API clean and understanable. If people have some static resource, they can tell Spark where to find them using staticFileLocation(). I personally like the staticFiles.location() syntax a lot, I would be sad to see it go.

StaticFilesConfiguration, however, I don't really care for. It could hopefully all be removed.