Fix file iterator bytesize fetch (fixes Rack > 2 applications)

[davidalejandroaguilar]

Neither Rack::Files nor Rack::File have ever responded to #map, but they always have responded to #each, so we should use that instead.

Also, on Rack >= 2.1, Rack::Files::Iterator#bytesize was introduced, avoiding the need to calculate it ourselves.

This broken behavior was discovered on a Rack 3 application. The fact that Rack already populated the Content-Length header (content-length now) was hiding this broken fallback implementation. #660 would fix the broken behavior, by appropriately fetching the downcased header coming from Rack 3 applications, but would keep hiding the broken fallback behavior.

This PR fixes the fallback behavior to use the correct method, depending on the Rack version used.

Note

I'm still writing specs for this, but wanted to get the draft up for potential discussion.

[hms]

I was trying this patch out with the hopes it would resolve the issue.

At least for my application, body is an Array and does not support body.bytesize per the >= 2.1 branch of the Rack.release >= "2.1" if test. The else side of the branch works correctly.

I think it might be better to test via respond_to?

Is this something you are open to handling or given I'm in a bit of a time crunch, would you mind if I forked and made a run and seeing if I can get this working (on my codebase) for your review?

Hal

[davidalejandroaguilar]

@hms What exact version of Rack are you on and what endpoint are you using? (derivation, download, etc.)

If you're on a time crunch, try this patch if you're on Rack >= 3: #660.

Rack already populates the Content-Length header, but since Rack 3, it comes downcased as content-length. So this PR fixes Shrine's fallback for calculating it itself, but that other patch would just use the value Rack set.

[davidalejandroaguilar]

Now that I think of it, I think we can just always use the else clause: using each to imperatively sum the chunks and not use the byesize method at all.

Since, as mentioned in the description, this is a fallback for Rack not populating the content length header, which should not really happen.

This way, we'd avoid the ugly conditionals introduced in this PR.

[hms]

Thanks for such a quick response. I'll take a look at #660 to see if that buys me time.

Let me know the best way that I might be able to help and I'll do my best.

Hal

[hms]

Pulled your code and worked on a merge of both of your PRs. I haven't spent enough time to work out all of the permutations, but at least in the presign-endpoint, content-length is nil.

headers["content-length"] ||= body.each.inject(0) { |sum, chunk| sum += chunk.length }.to_s

seems to make everything happy (at least so far).

[tomasc]

@davidalejandroaguilar @hms just ran into this issue as well. Do you know why this has not been merged yet?

[davidalejandroaguilar]

@tomasc Shrine seems to no longer be actively maintained.

I've been using this and #660 in prod since I opened it though, try it and let me know how it goes 👍🏼

[tomasc]

@davidalejandroaguilar oh no, I don't want to believe that!

@janko is this true?

[davidalejandroaguilar]

@tomasc ffc1c04

I'm no longer actively maintaining Shrine, any sponsorship will go
towards the Rodauth ecosystem.

[tomasc]

@davidalejandroaguilar awwwwwwww.

Hope we can get new release (with your PR) at least once in a while … @janko ?

[hms]

Unfortunately, @janko has decided to move on from Shrine. This project was a gift to the community and was a lifesaver for me personally. I can't thank him enough and wish him well.

@davidalejandroaguilar, @tomasc

David, I ended up requiring a small change from your PR #660 in order for things to work for my app (reflective of a comment I made in that thread). I'm happy to push to you / your PR for your review. It was a small change for each of the (derivation, download, presign)_endpoints:

if Rack.release > "2"
-        headers["content-length"] ||= body.map(&:bytesize).inject(0, :+).to_s
+        headers["content-length"] ||= body.each.inject(0) { |sum, chunk| sum += chunk.length }.to_s
       else
         headers["Content-Length"] ||= body.map(&:bytesize).inject(0, :+).to_s
       end

As far as Shrine goes, I'm a bit of a panic as my project has it tightly coupled and switching to ActiveText would be hard if not impossible.

But as we're already seeing small amounts of maintenance is going to be required to keep Shrine working to keep up with Ruby and Rails (and all of its dependencies) bug fixes and new versions. Maybe @janko would be receptive to opening this repo to a new maintainer assuming any one of us would be interested and has the skills/knowledge base to actually successfully keep Shrine functional over time. Unfortunately, I don't have the knowledge base to be lead here, but if someone else was available, I'm open to trying to be part of the solution.

In the meantime, we can always fork. I hope that if that became the direction, that as a shrine community, we'd end up coalescing around a single fork so we have the best chance to keep up with the inevitable maintenance / enhancements required over time.

[tomasc]

@hms @davidalejandroaguilar I made a PR to Shrine which introduces Rack 3 compatibility and passes the test full suite: #682. Please feel free to suggest any updates in case I missed anything.

Too bad that Shrine test suite is not being run on PRs automatically – would be calming to see it pass here explicitly.

Can you please help by testing this in your apps, following which I hope we can have @janko merging this an releasing new version of the gem?

[hms]

@tomasc I'll pull your PR into my application today/tomorrow. Thanks for the help.

hms

[janko]

Closing in favor of #682.