un-deprecate stream-like objects #1520

urkle · 2016-11-14T22:10:33Z

this splits the "stream" and "file" inside_route methods
file is purely for specifying a file (and utilizing sendfile when it can)
stream is purely for stream-like objects.. This allows you to have streaming responses of generated data

This PR is in reference to issue #1392

Pending Questions

why are the headers only set for the "stream" method and not the "file" method in inside_routes ?
if the behavior is different because of those headers then I should update this to NOT deprecate passing a filename to "stream"
what additional tests are needed for this change?

dblock · 2016-11-15T13:15:58Z

So the new code is handling in stream? I think there needs to be tests for any new paths. Maybe explicit deprecation tests that warn is called?

Needs an UPGRADING/README too. Thanks.

milgner · 2017-08-09T14:42:58Z

This looks very promising! @urkle any chance you can adapt this to the current master? Otherwise if anyone is interested, I'd be willing to take this on, too.

urkle · 2017-08-10T01:53:37Z

@milgner it maybe a week or so before I get back around to this. It's been on my TODO list to rebase, add tests and re-submit this

linchus · 2018-04-16T09:11:42Z

@urkle Any news in this PR?
Do you need any help?

urkle · 2018-04-16T12:12:36Z

@linchus this needs to be rebased and tests added (as requested). Right now it has been a matter of finding the time to work on it.

crondaemon · 2020-05-20T17:43:49Z

Can we help in some way this PR to be merged? The deprecation warning is really noisy when used in regression tests and clutters the output. Since we're using a non-deprecated object, it would be really useful to remove the warning.

dblock · 2020-05-20T19:05:10Z

Can we help in some way this PR to be merged? The deprecation warning is really noisy when used in regression tests and clutters the output. Since we're using a non-deprecated object, it would be really useful to remove the warning.

Reopen the PR, fix per comments above, make it green?

urkle · 2020-05-20T20:35:30Z

Right now I'm finishing a project in the next week or so upgrading our apps to using Grape 1.3.x. Once that is done I'll be able to revisit this against grape master (as we are still using this functionality)

And this means I'm actually logging a task in our system for me to work on this :)

crondaemon · 2020-05-21T07:26:28Z

@urkle thanks for your effort!

dblock · 2020-06-02T18:48:59Z

This looks ready code-wise, thanks for following up! We need to document the new behavior in README? Anything to add to UPGRADING?

urkle · 2020-06-02T19:39:39Z

This looks ready code-wise, thanks for following up! We need to document the new behavior in README? Anything to add to UPGRADING?

I just rebased it right now. Going through my changes and refreshing my memory and going to be adding more tests and updating README still. I'll let you know when it's all ready.

@dblock also did you have any thoughts on my first question in the PR? Why the "stream' method sets headers but not the 'file' method?

urkle · 2020-06-02T21:34:34Z

@dblock ok.. I pushed up new tests and updates to UPGRADING and README. Let me know what else you want me to adjust/tweak.

dblock · 2020-06-02T22:14:29Z

why are the headers only set for the "stream" method and not the "file" method in inside_routes ?

I am guessing because for a file you want to let Rack do its thing for content-length and transfer-encoding and generate an e-tag, but for a stream none of these make sense?

if the behavior is different because of those headers then I should update this to NOT deprecate passing a filename to "stream"

I think what you did was right. A file is a file, a stream is a stream. Until someone points out something broken I say leave it. But I would also agree to not deprecate it. Possibly simpler?

what additional tests are needed for this change?

I would ensure tests for those headers, if we made a mistake at least we will have tests to say that it was thought through behavior.

dblock

I'm good with this on a green build. The header specs need to be adjusted to what is going on.

Would like another pair of eyes ideally, maybe @ioquatix or @dm1try ?

dblock · 2020-06-02T22:15:16Z

README.md

@@ -3168,7 +3168,7 @@ end

 Use `body false` to return `204 No Content` without any data or content-type.

-You can also set the response to a file with `file`.
+You can also set the response to a file with `file` and it will be streamed using Rack::Chunked.


Quote Rack::Chunked

dblock · 2020-06-02T22:15:27Z

README.md

@@ -3178,12 +3178,22 @@ class API < Grape::API
 end
 ```

-If you want a file to be streamed using Rack::Chunked, use `stream`.
+If you want to stream non-file data you use the stream method and a Stream object.


Quote classes

dblock · 2020-06-02T22:15:43Z

README.md

@@ -3178,12 +3178,22 @@ class API < Grape::API
 end
 ```

-If you want a file to be streamed using Rack::Chunked, use `stream`.
+If you want to stream non-file data you use the stream method and a Stream object.
+This is simply an object that responds to each and yields for each chunk to send to the client.


Remove "simply", weasel word ;)

Quote each

dblock · 2020-06-02T22:16:13Z

UPGRADING.md

+
+Previously in 0.16 stream-like objects were deprecated. This release restores their functionality for use-cases other than file streaming.
+
+For streaming files, simply use file always.


Remove simply, quote file

dblock · 2020-06-02T22:17:09Z

UPGRADING.md

+end
+```
+
+If you want to stream other kinds of content from a streamer object you may.


Or shorter ...

Use stream to stream other kinds of content. In the following example a streamer class ...

dblock · 2020-06-02T22:17:36Z

UPGRADING.md

+class MyObject
+  def each
+    yield '['
+    # maybe do some paginated DB fetches and return each page


Remove maybe, another weasel word

dm1try

file is purely for specifying a file (and utilizing sendfile when it can)

from the user's point of view, it still sounds ambiguous. at least it should be documented "when it can".

though I would use file only for utilizing sendfile(or even rename it to sendfile) and use stream to produce a chunked response(this could be a file too): those different delivery mechanisms require different changes to the stack, so I would use different interfaces for them to force a user to make an explicit choice.
moreover sendfile is not about streaming, it's about an optimization of static file delivering using web server facilities.

urkle · 2020-06-09T15:52:50Z

file is purely for specifying a file (and utilizing sendfile when it can)

from the user's point of view, it still sounds ambiguous. at least it should be documented "when it can".

though I would use file only for utilizing sendfile(or even rename it to sendfile) and use stream to produce a chunked response(this could be a file too): those different delivery mechanisms require different changes to the stack, so I would use different interfaces for them to force a user to make an explicit choice.
moreover sendfile is not about streaming, it's about an optimization of static file delivering using web server facilities.

@dm1try @dblock
OK.. so how about this.

deprecate fully "file"
add sendfile that does what "file" does today in master
allow "stream" to take a filename (string) or a stream-like object

dblock · 2020-06-09T16:03:40Z

I think what is in this PR is good (enough) as it does what it advertises. Before we go into further renaming - @dm1try what do you think about merging this on 🍏 as here and then we can try the suggested change?

urkle · 2020-06-09T16:30:21Z

@dblock I pushed a fixup that does that rework.. but I can extract to a separate PR if you wish.. Up to you.. just let me know.

urkle · 2020-06-09T16:30:52Z

@dblock I think the rework is definitely cleaner and more obvious.

dm1try · 2020-06-09T16:31:04Z

I think what is in this PR is good (enough) as it does what it advertises. Before we go into further renaming - @dm1try what do you think about merging this on 🍏 as here and then we can try the suggested change?

I’m fine with it)

EDITED: oops, I was late with my comment

dblock

Ok, I'm cool with this update, it's definitely better.

Let's bump version to 1.4 as part of this PR?

Fix the build.

dblock · 2020-06-09T17:02:35Z

UPGRADING.md

+
+Previously in 0.16 stream-like objects were deprecated. This release restores their functionality for use-cases other than file streaming.
+
+This release also renamed `file` to `sendfile` to better document it's purpose.


it's -> its

We're deprecating file, so I would say "this release deprecated file in favor of sendfile"

dblock · 2020-06-09T17:02:57Z

UPGRADING.md

+
+This release also renamed `file` to `sendfile` to better document it's purpose.
+
+To deliver a file via `sendfile` if available do this.


"if available" doesn't make sense to me, I would just remove

to deliver "in chunks" or something similar?

@dblock look at how I re-worded it.. as it is not really "in chunks" as if the web server you are running through supports sendfile then it hands it off to that functionality.

dblock · 2020-06-09T17:03:25Z

UPGRADING.md

+end
+```
+
+Use `stream` to stream files


"stream file conntent"?

dblock

Fix the typo below, squash and I'll merge. Thanks for hanging in here!

dblock · 2020-06-09T18:17:35Z

spec/grape/integration/rack_sendfile_spec.rb

@@ -4,12 +4,16 @@

 describe Rack::Sendfile do
  subject do
-    send_file = file_streamer
+    contet_object = file_object


content_object, typo, missing n

dblock · 2020-06-09T18:17:53Z

UPGRADING.md


-To deliver a file via `sendfile` if available do this.
+To deliver a file via the Sendfile support in your web server and have the Rack::Sendfile middleware enabled. See [`Rack::Sendfile`](https://www.rubydoc.info/gems/rack/Rack/Sendfile)


Add a period in the end.

dblock · 2020-06-09T18:18:10Z

UPGRADING.md

@@ -18,7 +18,7 @@ class API < Grape::API
 end
 ```

-Use `stream` to stream files
+Use `stream` to stream file content in chunks


Add a period.

dblock · 2020-06-09T18:19:39Z

Since we're bumping to 1.4, change version.rb and the version in CHANGELOG too please.

- deprecate file and replace with sendfile - update stream to not deprecate stream-like objects

dblock · 2020-06-09T19:58:48Z

Merged, thank you!

dblock · 2020-06-09T19:59:53Z

I suggest sending an email to the Grape mailing list describing the change (mostly copy from upgrading) and asking people to comment/test/etc. early.

ServeFile was renamed to ServeStream in ruby-grape#1520. Calling Grape.eager_load! would fail with: ``` uninitialized constant Grape::ServeFile (NameError) ```

ServeFile was renamed to ServeStream in #1520. Calling Grape.eager_load! would fail with: ``` uninitialized constant Grape::ServeFile (NameError) ```

The pull request ruby-grape#1520 introduced a regression that always caused the `Cache-Control` HTTP header to be set to `no-cache`, even if the response wasn't a stream. To fix this, we only set HTTP headers if there is an actual stream. Closes ruby-grape#2087

urkle force-pushed the feat-allow-stream-objects branch from b8bfd9a to c55c862 Compare November 14, 2016 22:13

urkle force-pushed the feat-allow-stream-objects branch from c55c862 to 88372d3 Compare January 3, 2019 22:02

urkle force-pushed the feat-allow-stream-objects branch 2 times, most recently from 9e0329e to fdf5398 Compare June 2, 2020 17:56

urkle force-pushed the feat-allow-stream-objects branch from 9b12fe6 to c7da23b Compare June 2, 2020 21:31

dblock approved these changes Jun 2, 2020

View reviewed changes

dm1try reviewed Jun 4, 2020

View reviewed changes

dblock reviewed Jun 9, 2020

View reviewed changes

urkle requested a review from dblock June 9, 2020 18:02

dblock requested changes Jun 9, 2020

View reviewed changes

un-deprecate stream-like objects

3db3b0a

- deprecate file and replace with sendfile - update stream to not deprecate stream-like objects

urkle force-pushed the feat-allow-stream-objects branch from 8f8609c to 3db3b0a Compare June 9, 2020 18:39

urkle requested a review from dblock June 9, 2020 18:41

dblock merged commit 9198f7e into ruby-grape:master Jun 9, 2020

urkle deleted the feat-allow-stream-objects branch June 9, 2020 20:08

stanhu mentioned this pull request Jun 12, 2020

Fix eager loading due to ServeStream rename #2072

Closed

dblock pushed a commit that referenced this pull request Jun 28, 2020

Fix eager loading due to ServeStream rename

b3adbb4

ServeFile was renamed to ServeStream in #1520. Calling Grape.eager_load! would fail with: ``` uninitialized constant Grape::ServeFile (NameError) ```

dblock mentioned this pull request Jul 10, 2020

Deprecation FileStreamer-like object #2083

Closed

stanhu mentioned this pull request Jul 10, 2020

Set Cache-Control only for streamed responses #2088

Merged

dblock mentioned this pull request Nov 8, 2023

How do I respond to stream results via API - Grape #2367

Closed


		Previously in 0.16 stream-like objects were deprecated. This release restores their functionality for use-cases other than file streaming.

		For streaming files, simply use file always.


		Previously in 0.16 stream-like objects were deprecated. This release restores their functionality for use-cases other than file streaming.

		This release also renamed `file` to `sendfile` to better document it's purpose.


		This release also renamed `file` to `sendfile` to better document it's purpose.

		To deliver a file via `sendfile` if available do this.


		To deliver a file via `sendfile` if available do this.
		To deliver a file via the Sendfile support in your web server and have the Rack::Sendfile middleware enabled. See [`Rack::Sendfile`](https://www.rubydoc.info/gems/rack/Rack/Sendfile)

un-deprecate stream-like objects #1520

un-deprecate stream-like objects #1520

Conversation

urkle commented Nov 14, 2016

dblock commented Nov 15, 2016 • edited Loading

milgner commented Aug 9, 2017

urkle commented Aug 10, 2017

linchus commented Apr 16, 2018

urkle commented Apr 16, 2018

crondaemon commented May 20, 2020

dblock commented May 20, 2020

urkle commented May 20, 2020 • edited Loading

crondaemon commented May 21, 2020

dblock commented Jun 2, 2020

urkle commented Jun 2, 2020

urkle commented Jun 2, 2020

dblock commented Jun 2, 2020 • edited Loading

dblock left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dm1try left a comment

Choose a reason for hiding this comment

urkle commented Jun 9, 2020

dblock commented Jun 9, 2020 • edited Loading

urkle commented Jun 9, 2020

urkle commented Jun 9, 2020

dm1try commented Jun 9, 2020 • edited Loading

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Jun 9, 2020

dblock commented Jun 9, 2020

dblock commented Jun 9, 2020

dblock commented Nov 15, 2016 •

edited

Loading

urkle commented May 20, 2020 •

edited

Loading

dblock commented Jun 2, 2020 •

edited

Loading

dblock left a comment •

edited

Loading

dblock commented Jun 9, 2020 •

edited

Loading

dm1try commented Jun 9, 2020 •

edited

Loading