Open wls opened 4 years ago
Yes, wrap-file
and wrap-resource
don't currently guess the media type from the file location, and they probably should do. PRs are welcome.
What would be an acceptable design solution that stays within the philosophy of the library?
For instance, I can see where someone might actually want to get at the unmodified URI for some reason. So conveying an "effective URI" with the fully qualified filename along would be one way to do it. Is it acceptable, then, to add a new, optional, field to the request object (is that the right place) and let wrap-content-type
use it, if present, otherwise fallback to the original URI?
I'm trying to think of minimal, efficient solutions that could correct the behavior, preserve backwards compatibility, and incur only the necessary overhead when needed. I'm typically not a fan of side-effects, so I'm a little hesitant how to pass this along otherwise.
The wrap-file
and wrap-resource
middleware would add their own content-type header, using the functions in ring.util.mimetype
.
Today I ran into this as well. I had a look at the Ring code, and locally I fixed it, but in a different way. I modified the wrap-content-type
middleware so that:
if the body of the request is a file, then use the mime type of that file instead of using the request uri.
@weavejester Does that seem like a good approach to you? If so, I'll make a PR asap.
The request URI extension should take priority, but if there is no URI extension, then it can use the extension of the File object. This ensures that Ring remains backward compatible.
Right, backwards compatibility is indeed important but that way it doesn't fix the issue.
I'll try if I can somehow add the header in file and resource middleware like you initially suggested.
I could also add an option to content-type to retain backward compat, since I feel it is actually a bug...
Or maybe "if req uri points to DIRECTORY and response body is FILE then use mine type of file".
Right, backwards compatibility is indeed important but that way it doesn't fix the issue.
Why doesn't it fix the issue?
Oh wait it probably does, I think you're right. I'll have another go at it. (I misread your comment, the word extension being crucial.)
@weavejester I created the pull request. Please let me know if you want me to change anything.
Consider this simple static file delivery, but observe carefully its delivered Content-Type header:
While this will always serve up
index.html
if it exists, how it gets delivered changes.http://localhost/index.html — correctly serves with a mime type of
text/html
, due to the specified file extension in the URI. Good.http://localhost/ — causes
wrap-file
to deliverindex.html
by default, but as the URI has no file extensionwrap-content-type
, unaware thatwrap-file
automatically selected this resource, delivers a response with a mime type ofapplication/octet-stream
. This is a problem, as it causes browsers, like Firefox, to download the content rather than display it. Bad.Unfortunately, I'm too new to Ring to know if I'm doing this wrong, so apologies in advance should this turn into user education issue, but I didn't see in the documentation how to address it.
It seems that
wrap-content-type
should either be aware of the "effective URI" at best or allow itscontent-type-response
to accept an override – though doing so isn't always reliable, as it may not always be an html index file delivered.The behavior of
wrap-file
(or evenwrap-resource
) seems to be breaking a strong assumption made downstream by the loosely coupledwrap-content-type
about the URI.An aside: workarounds suggesting making a special route for that one off case don't take into consideration that other subdirectories suffer from the same problem; hacking routes to correct a middleware issue is obviously the wrong way to go.