Extract several methods from Rack::File#serving #570

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
@graysonwright
Contributor

graysonwright commented Jun 3, 2013

Extracted methods:

  • mime_type
  • filesize
  • response_body

Motivation

  1. The method is somewhat complicated
  2. Extracting pieces of the method into their own functions really improves your ability to extend the class to customize it.

Example of Customization

Let's say you have a basic Rack::Directory setup.

run Rack::Directory.new(".")

But you have a lot of markdown files in the directory, and you want to compile and render them on the fly. That requires modifying the mime type, the response body, and the filesize.

By extracting the methods out of Rack::File#serving, you only have to overwrite two methods (file_size is calculated based on the new response_body

The config.ru would look something like this:

require 'redcarpet'
require 'rack/mime'

def markdown_compiler
  Redcarpet::Markdown.new(Redcarpet::Render::HTML.new)
end

class Rack::File
  # Overwrite
  def mime_type
    @mime ||= Rack::Mime.mime_type(F.extname(@path), @default_mime)
    markdown? ? 'text/html' : @mime
  end

  # Overwrite
  def response_body
    if markdown?
      @response_body ||= markdown_compiler.render(F.read(@path))
    else
      nil
    end
  end

  def markdown?
    F.extname(@path) == '.md'
  end
end

run Rack::Directory.new(".")

Other stuff

This doesn't change the behavior of Rack, it just makes it easier to customize and to work on in the future. Because of that, I didn't write any tests. Let me know if I should.

Extract several methods from Rack::File#serving
Extracted methods:

  - mime_type
  - filesize
  - response_body
@rkh

This comment has been minimized.

Show comment
Hide comment
@rkh

rkh Jun 3, 2013

Member

Thanks, generally looks good, but won't this break horribly if the file changes?

Member

rkh commented Jun 3, 2013

Thanks, generally looks good, but won't this break horribly if the file changes?

@graysonwright

This comment has been minimized.

Show comment
Hide comment
@graysonwright

graysonwright Jun 3, 2013

Contributor

I don't think so -- I'm playing around with it here and it seems to be working fine.

If the file hasn't been modified since env['HTTP_IF_MODIFIED_SINCE'], it still returns a 304 response.
If it has been modified since then, it uses whatever is in the response_body method to generate a new body, then serves that.

Of course, it would be pretty trivial to extract a method (not sure about the name):

def cached?
  last_modified = F.mtime(@path).httpdate
  env['HTTP_IF_MODIFIED_SINCE'] == last_modified
end

so that whoever's extending it can deal with the problem easier.

Contributor

graysonwright commented Jun 3, 2013

I don't think so -- I'm playing around with it here and it seems to be working fine.

If the file hasn't been modified since env['HTTP_IF_MODIFIED_SINCE'], it still returns a 304 response.
If it has been modified since then, it uses whatever is in the response_body method to generate a new body, then serves that.

Of course, it would be pretty trivial to extract a method (not sure about the name):

def cached?
  last_modified = F.mtime(@path).httpdate
  env['HTTP_IF_MODIFIED_SINCE'] == last_modified
end

so that whoever's extending it can deal with the problem easier.

@rkh

This comment has been minimized.

Show comment
Hide comment
@rkh

rkh Jun 3, 2013

Member

But isn't the file size cached?

Member

rkh commented Jun 3, 2013

But isn't the file size cached?

@graysonwright

This comment has been minimized.

Show comment
Hide comment
@graysonwright

graysonwright Jun 3, 2013

Contributor

Are you talking about the instance variable I set up in the filesize method?

Then yeah, I think you're right. I was only paying attention to the Rack::Directory scenario, where a new Rack::File is created for each request (at least that's how I understand it).

But if your config.ru uses run Rack::File.new('some_file.md') then I think it would break.

Can you confirm that's the problem you're talking about?

Contributor

graysonwright commented Jun 3, 2013

Are you talking about the instance variable I set up in the filesize method?

Then yeah, I think you're right. I was only paying attention to the Rack::Directory scenario, where a new Rack::File is created for each request (at least that's how I understand it).

But if your config.ru uses run Rack::File.new('some_file.md') then I think it would break.

Can you confirm that's the problem you're talking about?

@rkh

This comment has been minimized.

Show comment
Hide comment
@rkh

rkh Jun 3, 2013

Member

Yes. I was referring to using Rack::File directly as an endpoint.

Member

rkh commented Jun 3, 2013

Yes. I was referring to using Rack::File directly as an endpoint.

@graysonwright

This comment has been minimized.

Show comment
Hide comment
@graysonwright

graysonwright Jun 3, 2013

Contributor

Hopefully that takes care of it.

Contributor

graysonwright commented Jun 3, 2013

Hopefully that takes care of it.

lib/rack/file.rb
@@ -134,5 +131,33 @@ def fail(status, body)
]
end
+ # The MIME type for the contents of the file located at @path
+ #
+ # This method can be overridden to specify custom MIME types for certain files,

This comment has been minimized.

@raggi

raggi Jul 12, 2014

Member

I do not like this comment. It seems more sensible to adjust the mime tables.

@raggi

raggi Jul 12, 2014

Member

I do not like this comment. It seems more sensible to adjust the mime tables.

@raggi

This comment has been minimized.

Show comment
Hide comment
@raggi

raggi Jul 12, 2014

Member

I think Rack::File should in future be avoiding setting Content-Length if it cannot determine the size of the body by stat. In the case where it cannot, it should be deferring to Rack::ContentLength and/or Rack::Chunked, to avoid slurping until the last responsible moment.

Member

raggi commented Jul 12, 2014

I think Rack::File should in future be avoiding setting Content-Length if it cannot determine the size of the body by stat. In the case where it cannot, it should be deferring to Rack::ContentLength and/or Rack::Chunked, to avoid slurping until the last responsible moment.

@raggi

This comment has been minimized.

Show comment
Hide comment
@raggi

raggi Jul 12, 2014

Member

In general, these changes are ok, I'm concerned about introducing comments that enforce inheritance as a public API. I would prefer that these are removed.

Member

raggi commented Jul 12, 2014

In general, these changes are ok, I'm concerned about introducing comments that enforce inheritance as a public API. I would prefer that these are removed.

@raggi raggi added this to the Rack 1.6 milestone Jul 12, 2014

Remove caching of response body and filesize
Causes a bug if a file changes between requests.
@graysonwright

This comment has been minimized.

Show comment
Hide comment
@graysonwright

graysonwright Jul 21, 2014

Contributor

Good point -- I didn't know about the MIME tables when I added those comments. Just removed the offending lines.

Contributor

graysonwright commented Jul 21, 2014

Good point -- I didn't know about the MIME tables when I added those comments. Just removed the offending lines.

iamvery added a commit to iamvery/rack that referenced this pull request Apr 9, 2017

Avoid overwriting response body when it's already overridden
It appears that the intention of #570 was to provide a seam on
Rack::File where the response body may be provided by an overriding
class (see their markdown example). However e0ac32 overwrites the value
of response[2] (the body) after this is set.

This is the simplest possible fix, ignore the step of overwriting if a
value is already set. However it's worth noting that the design of the
entire method would probably be served well to improve. At this point, I
don't want to take that on.

Also of note, there may very well be a latent bug in the previous
implementation. Specifically, when response_body is overridden, it
affects the calculation of filesize, therefore if the method is
overridden the filesize of that overridden response_body would be
returned which is probably not the correct size for the response read
from the path. If that is in fact a defect, this PR may very well
address it to, though it is unverified.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment