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.65k stars 1.56k forks source link

Mime-type differences between 2.5 and 2.5.1 #684

Closed ph-hs closed 7 years ago

ph-hs commented 8 years ago

[ This is original description is in error - the bug is not before()/mime-type handling related ]

My HTML in static resources doesn't end in .html. I'm adding that with a filter.

public static void main(String[] args) {
    staticFiles.location("/");  // "foo" - a html file is in there.
    before("/", (request, response) -> {
        response.type("text/html"); // "foo" gets its html mime type
    });
}

Was working fine in 2.5. Is not working in 2.5.1

I've looked at https://github.com/perwendel/spark/compare/2.5...2.5.1 and cannot see what broke it.

tipsy commented 8 years ago

We've added mime-type guessing. Have you tried just removing your filter?

ph-hs commented 8 years ago

Removing the filter doesn't change the outcome - no mime types when the HTML arrives in the browser. Chrome, thinks they are candidates for downloads. I have automated Selenium3 testing of the spark app. Each test run yields a couple of dozen failing tests now, and a the same amount of new files in my ~/Downloads/ folder. If if move pasts automated tests, it's exactly the same when I point Chrome at the launched application. I can reproduce at will - 2.5 (all works), 2.5.1 (no mime types).

tipsy commented 8 years ago

Is this when running from a jar?

ph-hs commented 8 years ago

Well the bug is not related to mime-type or before() logic at all.

This - https://github.com/ph-hs/Setting-up-Spark-with-Maven - maven-example project, exhibits the error. Follow these instructions for a reproduction:

0) git clone git@github.com:ph-hs/Setting-up-Spark-with-Maven.git 1) cd Setting-up-Spark-with-Maven 2) Launch as is: mvn install exec:java -Dexec.mainClass="Main" 3) Test by going to http://localhost:4567/hi in Chrome ... it should (and does) display HTML in page (success) 4) Change the POM to use SparkJava 2.5.1 instead of 2.5 5) Repeat #2, #3 but note that Chrome downloads "hi" rather than displays it in page :-(

tipsy commented 8 years ago

In Spark 2.5.1, your hi files are served correctly. hi.html is served as text/html and hi is served as application/octet-stream (the default/least specific mime-type). In Spark 2.5 no content-type header is set. Is serving files without extensions as html (or without content-type) standard behavior in other frameworks?

ph-hs commented 8 years ago

You're saying that a browser access of http://localhost:4567/hi as in the Git repo linked to is working correctly with 2.5.1? That a mime type is being attached, and implicitly the browser is doing the right thing for the URL and displaying it?

Here's me trying it with 2.5.1 - https://www.youtube.com/watch?v=2leNneNDVT4 - where the wrong thing is happening. All the action is in the first half-second: pasting in http://localhost:4567/hi and no display of the page .. and an incorrect download instead.

tipsy commented 8 years ago

the browser is doing the right thing for the URL and displaying it?

Is that the correct thing to do?

Spark serves your extensionless file as application/octet-stream, so Chrome downloads the file instead of rendering it. How is either Spark/Chrome supposed to know that it has HTML content and should be rendered? Isn't this expected behavior? (Why are your HTML-files not .html ?)

I'm not sure what the correct behavior here is. We could remove content-type for files which do not have any extension, then it should work like it used to. Any thoughts @perwendel ?

ph-hs commented 8 years ago

So the change in functionality between 2.5 and 2.5.1 was a bug fix - you're saying.

Fine. Can someone advise me on how to attach mime types to static resources, where it cannot be inferred. I thought that was before(path, {}) logic, but I was wrong :-(

ph-hs commented 8 years ago

Should I bisect through the comments after 2.5, to see when it changed?

tenaciousRas commented 8 years ago

https://github.com/perwendel/spark/blob/19f4d238f966fb49689021899adac93d1da4861f/src/main/java/spark/staticfiles/StaticFilesConfiguration.java#L89

https://github.com/perwendel/spark/blob/4a6a3888d89c0070268d827d060f660209bdc07b/src/main/java/spark/staticfiles/MimeType.java#L100

Please don't take this the wrong way but mime-type guessing is a can of worms. I'm very naive about this codebase but I don't see any way to set an override to handle this issue? It would seem natural to set the mimetype when configuring the static route. Do you wind up serving it as a dynamic route and just read+dump a static file(s)? In your case, can you just stop using empty context rotes as HTML content? If you can't do that due to production exposure then can you put 301/302 redirects to dot-html files?

p.s. - I just want to mention that the mapping in MimeType is incomplete. Take a look at https://www.sitepoint.com/web-foundations/mime-types-complete-list/ -- which is probably incomplete! Does Spark provide a way to inject a custom mapping?

ph-hs commented 8 years ago

I 100% agree about mime-type guessing.

How, in Spark, can I use before() or after() to attach a mime-type to something that's served from staticFiles.location(path) ?

I mean I can clone an amount of Spark code (like so†), and make these same static resources served up as get() end-points and be 100% sure that the right mime type is set as that's normal for get(), but it would suck to HAVE to do that.

https://github.com/perwendel/spark/issues/652#issuecomment-246377764

tipsy commented 8 years ago

@tenaciousRas Do you wind up serving it as a dynamic route and just read+dump a static file(s)?

Yes, though you can specify your own route for a static resources if you wish.

@tenaciousRas p.s. - I just want to mention that the mapping in MimeType is incomplete. Take a look at https://www.sitepoint.com/web-foundations/mime-types-complete-list/ -- which is probably incomplete! Does Spark provide a way to inject a custom mapping?

Yes: https://github.com/perwendel/spark/blob/master/src/main/java/spark/Service.java#L549-L551


Should we include a flag for enabling/disabling mine-type-guessing?

ph-hs commented 8 years ago

OK commit https://github.com/perwendel/spark/commit/4a2b982b39d279350663e6ff926ea4ee26aafdd3 broke the this feature. I've bisected back to that commit, and re-run the Maven test project - https://github.com/perwendel/spark/issues/684#issuecomment-254488975 to show the error happening. I can record of video of the proof of the breaking in functionality change if required.

The commit before (https://github.com/perwendel/spark/commit/f8359c0467560186f56350b4d4373b5105c1955c) works fine. The commit after (https://github.com/perwendel/spark/commit/4a2b982b39d279350663e6ff926ea4ee26aafdd3) feature is broken.

Reminder: I'm perfectly happy to attach a mime type in a before() OR after(), but cannot for statically served content, in SparkJava 2.5.1.

paul-hammant commented 8 years ago

Workaround found. Pull request demonstrating that in the testbase: https://github.com/perwendel/spark/pull/704

tipsy commented 8 years ago

In 2.5.2 (released) you can disable mime type guessing.

paul-hammant commented 8 years ago

Super useful for people who've found this issue ... a link to or copy/paste "how" to turn off mime type guessing, please.

tipsy commented 8 years ago

staticFiles.disableMimeTypeGuessing();