Open svdo opened 2 years ago
Thanks. First, can you remove 34a15b6b, as that's not strictly related to the purpose of the PR.
Second, I think I need to consider the best way to go about this. Pulling the file extension from File response bodies is useful, but in production most statically served pages will be resources.
Thanks. First, can you remove 34a15b6b, as that's not strictly related to the purpose of the PR.
Ok. I am curious as to why you wouldn't want this small improvement in that same function, but it's your call 🙂
Second, I think I need to consider the best way to go about this. Pulling the file extension from File response bodies is useful, but in production most statically served pages will be resources.
What's your hesitation exactly? My thinking is that in general, when you're serving something different than requested for whatever reason, the mime type should be the real mime type of what you're serving, not what was being requested. I mean, the Content-Type
should primarily reflect the response, not the request, right? Maybe this situation doesn't generally apply to resources (as opposed to files), I'm not sure, but conceptually it should still be the right thing to do.
Let me know if I can help! For now I think I'm going to run a modified copy of the middleware, as I need a solution, but I'm of course still motivated to contribute a solution here.
I am curious as to why you wouldn't want this small improvement in that same function
It's good practice to ensure that a pull request contains only code related to the feature or fix being implemented. That way the diff is clean; any line that's changed relates to the pull request.
Formatting changes should be separated out into another commit, and also should ensure that the codebase is consistent. That is, if you decide to replace if
with when
, then do so for all instances in the codebase.
What's your hesitation exactly?
This change produces inconsistent behaviour when dealing with files on the filesystem, and resources located in jars. This can lead to subtle errors between development and production.
For example, suppose you use the wrap-resource
middleware. During development, this will likely draw from the resources
folder, while during production it may instead pull from an uberjar. The problem is that wrap-resource
uses a File
body where possible, which means that in development wrap-content-type
will work with index files, and in production it won't.
The solution I'm considering is to allow URL
instances to be supplied as the response body, to update wrap-resource
to use this, and then to add a protocol to wrap-content-type
to handle both File
and URL
bodies.
For example, suppose you use the
wrap-resource
middleware. During development, this will likely draw from theresources
folder, while during production it may instead pull from an uberjar. The problem is thatwrap-resource
uses aFile
body where possible, which means that in developmentwrap-content-type
will work with index files, and in production it won't.
Maybe that's not actually a problem, as wrap-resource
doesn't do that mapping from "/" to "/index.html", so the situation that we're covering here doesn't apply to wrap-resource
? My change only triggers when the (:uri request)
does not have an extension...
[...] but in production most statically served pages will be resources.
You got me thinking with that remark by the way. I am now approaching the point that I'll be deploying this to production for the first time. I'm now converting my code to use wrap-resource
instead of wrap-file
... I'm adding a simple middleware that makes sure my SPAs can be served, e.g. /login/a/b/c?foo=bar
causes /login/index.html?foo=bar
to be served.
I guess this removes the need for the change for me. I'll leave it up to you if we proceed with it anyway, or close it for now...
Maybe that's not actually a problem, as
wrap-resource
doesn't do that mapping from "/" to "/index.html"
Ah, you're right. Though it might be nice if that were an option. Let me think about it.
Faced the same problem. Does this PR still under consideration? Thanks
Just bumped into this too! I hope this gets fixed because pedestal uses the wrappers around ring's file / resource / content-type middleware as default interceptors
Could you give a little more information on what the exact issue you had? Would it be fixed by looking for File
bodies without content types and deriving the content type from the file name?
My exact issue is that when using content-type
along with file
interceptors in pedestal to serve static content/files, when the uri is an index path ("/") while the file
interceptor searches and serves correctly index-files like index.html
, the content-type
interceptor does not set the correct Content-Type
header and fallbacks to application/octet-stream
.
This happens because content-type
interceptor takes to account the only the uri in the mime-type detection as seen here:
https://github.com/ring-clojure/ring/blob/master/ring-core/src/ring/middleware/content_type.clj#L6
This in turn leads to static files being downloaded and not served by the browser when visiting index paths.
Revisiting the same issue after almost a year, any considerations on merging this?
I think my earlier comment is the best solution I can think of:
The solution I'm considering is to allow
URL
instances to be supplied as the response body, to updatewrap-resource
to use this, and then to add a protocol towrap-content-type
to handle bothFile
andURL
bodies.
Implementing this just for files would produce inconsistent behaviour in wrap-resource
.
Ah yeah, this seems a reasonable solution
When the request points to a directory and the body of the response is a file (i.e. index.html or something like that), take the mime type of that file. This fixes the case where previously requesting "/" would serve "index.html" with content type "application/octet-stream".
This fixes #408.