Skip to content

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

Closed
c-lliope wants to merge 2 commits into
rack:masterfrom
c4lliope:refactor_rack_file_for_easier_customization
Closed

Extract several methods from Rack::File#serving#570
c-lliope wants to merge 2 commits into
rack:masterfrom
c4lliope:refactor_rack_file_for_easier_customization

Conversation

@c-lliope

@c-lliope c-lliope commented Jun 3, 2013

Copy link
Copy Markdown
Contributor

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.

@c-lliope
Extract several methods from Rack::File#serving
0d2138a 
Extracted methods:

  - mime_type
  - filesize
  - response_body
@rkh

rkh commented Jun 3, 2013

Copy link
Copy Markdown
Member

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

@c-lliope

c-lliope commented Jun 3, 2013

Copy link
Copy Markdown
Contributor Author

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

rkh commented Jun 3, 2013

Copy link
Copy Markdown
Member

But isn't the file size cached?

@c-lliope

c-lliope commented Jun 3, 2013

Copy link
Copy Markdown
Contributor Author

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

rkh commented Jun 3, 2013

Copy link
Copy Markdown
Member

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

@c-lliope

c-lliope commented Jun 3, 2013

Copy link
Copy Markdown
Contributor Author

Hopefully that takes care of it.

raggi
raggi reviewed Jul 12, 2014
View reviewed changes
Comment thread lib/rack/file.rb Outdated

@raggi raggi Jul 12, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@raggi

raggi commented Jul 12, 2014

Copy link
Copy Markdown
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.

@raggi

raggi commented Jul 12, 2014

Copy link
Copy Markdown
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.

@raggi raggi added this to the Rack 1.6 milestone Jul 12, 2014
@c-lliope
Remove caching of response body and filesize
a414cc5 
Causes a bug if a file changes between requests.
@c-lliope

c-lliope commented Jul 21, 2014

Copy link
Copy Markdown
Contributor Author

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
@iamvery
Avoid overwriting response body when it's already overridden
bcc2e4e 
It appears that the intention of rack#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.
@iamvery iamvery mentioned this pull request Apr 9, 2017
Closed
iamvery added a commit to iamvery/rack that referenced this pull request Nov 18, 2019
@iamvery
Avoid overwriting response body when it's already overridden
8e866f1 
It appears that the intention of rack#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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v1.6.0

Development

Successfully merging this pull request may close these issues.

3 participants

@c-lliope @rkh @raggi