Skip to content

Commit

Permalink
Fix spec failures in Rack::Deflator under 1.9
Browse files Browse the repository at this point in the history
There were two issues here that

* String#length was used to determine the Content-Length
  resulting in off-by-one failures in expected length
  assertions.

* The specs were using body.to_s to convert bodies to Strings.
  In Ruby 1.8, #to_s is like #join; in Ruby 1.9, #to_s is like
  #inspect
  • Loading branch information
rtomayko committed Feb 6, 2009
1 parent b7939fc commit 3576a34
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 6 deletions.
10 changes: 6 additions & 4 deletions lib/rack/deflater.rb
Expand Up @@ -36,17 +36,19 @@ def call(env)
mtime = headers.key?("Last-Modified") ?
Time.httpdate(headers["Last-Modified"]) : Time.now
body = self.class.gzip(body, mtime)
headers = headers.merge("Content-Encoding" => "gzip", "Content-Length" => body.length.to_s)
size = body.respond_to?(:bytesize) ? body.bytesize : body.size

This comment has been minimized.

Copy link
@dkubb

dkubb Feb 7, 2009

Contributor

This is such a common idiom, perhaps it should be wrapped up into a Rack::Utils.content_length_for method?

headers = headers.merge("Content-Encoding" => "gzip", "Content-Length" => size.to_s)
[status, headers, [body]]
when "deflate"
body = self.class.deflate(body)
headers = headers.merge("Content-Encoding" => "deflate", "Content-Length" => body.length.to_s)
size = body.respond_to?(:bytesize) ? body.bytesize : body.size
headers = headers.merge("Content-Encoding" => "deflate", "Content-Length" => size.to_s)
[status, headers, [body]]
when "identity"
[status, headers, body]
when nil
message = ["An acceptable encoding for the requested resource #{request.fullpath} could not be found."]
[406, {"Content-Type" => "text/plain", "Content-Length" => message[0].length.to_s}, message]
message = "An acceptable encoding for the requested resource #{request.fullpath} could not be found."
[406, {"Content-Type" => "text/plain", "Content-Length" => message.length.to_s}, [message]]
end
end

Expand Down
4 changes: 2 additions & 2 deletions test/spec_rack_deflater.rb
Expand Up @@ -56,7 +56,7 @@ class << body; def each; yield("foo"); yield("bar"); end; end
"Vary" => "Accept-Encoding",
})

io = StringIO.new(response[2].to_s)
io = StringIO.new(response[2].join)
gz = Zlib::GzipReader.new(io)
gz.read.should.equal("foobar")
gz.close
Expand Down Expand Up @@ -105,7 +105,7 @@ class << body; def each; yield("foo"); yield("bar"); end; end
"Last-Modified" => last_modified
})

io = StringIO.new(response[2].to_s)
io = StringIO.new(response[2].join)
gz = Zlib::GzipReader.new(io)
gz.read.should.equal("Hello World!")
gz.close
Expand Down

7 comments on commit 3576a34

@josh
Copy link
Contributor

@josh josh commented on 3576a34 Feb 7, 2009

Choose a reason for hiding this comment

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

+1 dkubb’s suggestion

@rtomayko
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It really is everywhere throughout the codebase.

@sporkmonger
Copy link

Choose a reason for hiding this comment

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

I’ve even found myself using this idiom in other projects. :-P

@rtomayko
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know we don’t extend core classes in Rack but, really, is there any downside to adding something like this in rack/utils.rb:

class String
  alias_method :bytesize, :size unless ''.respond_to? :bytesize
end

@sporkmonger
Copy link

Choose a reason for hiding this comment

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

I’m usually the first to decry monkey-patching, but I don’t have a problem with that suggestion. I’d say that’s perfectly acceptable. It doesn’t break the idiom for anyone else using it, and it simplifies a lot of repetitive code.

@rtomayko
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. It’s different from, say, monkey patching #each in to 1.9. Adding a method like #bytesize that did not exist in 1.8 and that’s supported in 1.9 with the exact same semantics is probably one of the only places I’m okay with a library extending core objects.

@sam-github
Copy link
Contributor

Choose a reason for hiding this comment

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

Its a slippery slope, and its got razors at the bottom.

There’s nothing special about the name bytesize. Just because rails didn’t donkey patch it into String doesn’t mean that some other guy out there didn’t. Maybe he has a “DSL” where he defined bytesize globally to return the number of bytes a string would take when reencoded into utf-8 from jcs, or the number of bytes the string will take when encoded into a protocol header.

He’s going to have problems when he tries to port to 1.9, but those are his problems. If Rack starts silently using his crazy bytesize definition, his problems become yours.

Anyhow, just define Rack::size(s) as a module function, and use it, its only 2 more characters than the ruby1.9 form:
size = body.size # 1.8
size = body.bytesize # 1.9
size = Rack::size body # compatibility wrapper

You can conditionally define it to return either size or bytesize depending on the ruby version, thus avoiding the overhead of continual respond_to? checks, AND having code that works perfectly with ruby 1.8 even when someone does have a weird bytesize method in their String.

Please sign in to comment.