From c55c86248291a8f637954d9e59540569c10c2311 Mon Sep 17 00:00:00 2001 From: Edward Rudd Date: Mon, 14 Nov 2016 17:06:15 -0500 Subject: [PATCH] un-deprecate stream-like objects - 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 --- CHANGELOG.md | 1 + lib/grape.rb | 4 ++-- lib/grape/dsl/inside_route.rb | 19 +++++++++++++----- lib/grape/middleware/formatter.rb | 6 +++--- .../{serve_file => serve_stream}/file_body.rb | 2 +- .../sendfile_response.rb | 2 +- .../stream_response.rb} | 16 +++++++-------- spec/grape/dsl/inside_route_spec.rb | 20 +++++++++---------- spec/grape/middleware/formatter_spec.rb | 4 ++-- 9 files changed, 42 insertions(+), 32 deletions(-) rename lib/grape/{serve_file => serve_stream}/file_body.rb (96%) rename lib/grape/{serve_file => serve_stream}/sendfile_response.rb (95%) rename lib/grape/{serve_file/file_response.rb => serve_stream/stream_response.rb} (55%) diff --git a/CHANGELOG.md b/CHANGELOG.md index d4806fa86..c237a4d50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ Next Release * [#1507](https://github.com/ruby-grape/grape/pull/1507): Add group attributes for parameter definitions - [@304](https://github.com/304). * [#1512](https://github.com/ruby-grape/grape/pull/1512): Fix: deeply nested parameters are included within `#declared(params)` - [@krbs](https://github.com/krbs). * [#1510](https://github.com/ruby-grape/grape/pull/1510): Fix: inconsistent validation for multiple parameters - [@dgasper](https://github.com/dgasper). +* [#1520](https://github.com/ruby-grape/grape/pull/1520): un-deprecate stream-like objects - [@urkle](https://github.com/urkle). * Your contribution here. #### Fixes diff --git a/lib/grape.rb b/lib/grape.rb index 8a24429fe..00ae82fc2 100644 --- a/lib/grape.rb +++ b/lib/grape.rb @@ -164,9 +164,9 @@ module Presenters autoload :Presenter end - module ServeFile + module ServeStream extend ActiveSupport::Autoload - autoload :FileResponse + autoload :StreamResponse autoload :FileBody autoload :SendfileResponse end diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb index 22f13e017..c03f4fab9 100644 --- a/lib/grape/dsl/inside_route.rb +++ b/lib/grape/dsl/inside_route.rb @@ -189,13 +189,13 @@ def body(value = nil) # GET /file # => "contents of file" def file(value = nil) if value.is_a?(String) - file_body = Grape::ServeFile::FileBody.new(value) - @file = Grape::ServeFile::FileResponse.new(file_body) + file_body = Grape::ServeStream::FileBody.new(value) + @stream = Grape::ServeStream::StreamResponse.new(file_body) elsif !value.is_a?(NilClass) warn '[DEPRECATION] Argument as FileStreamer-like object is deprecated. Use path to file instead.' - @file = Grape::ServeFile::FileResponse.new(value) + @stream = Grape::ServeStream::StreamResponse.new(value) else - @file + @stream end end @@ -218,7 +218,16 @@ def stream(value = nil) header 'Content-Length', nil header 'Transfer-Encoding', nil header 'Cache-Control', 'no-cache' # Skips ETag generation (reading the response up front) - file(value) + if value.is_a?(String) + warn '[DEPRECATION] Use `file file_path` to stream a file instead.' + file(value) + elsif value.respond_to?(:each) + @stream = Grape::ServeStream::StreamResponse.new(value) + elsif !value.is_a?(NilClass) + raise ArgumentError, 'Stream object must respond to :each.' + else + @stream + end end # Allows you to make use of Grape Entities by setting diff --git a/lib/grape/middleware/formatter.rb b/lib/grape/middleware/formatter.rb index ea5705e1f..157fbd17b 100644 --- a/lib/grape/middleware/formatter.rb +++ b/lib/grape/middleware/formatter.rb @@ -34,9 +34,9 @@ def after def build_formatted_response(status, headers, bodies) headers = ensure_content_type(headers) - if bodies.is_a?(Grape::ServeFile::FileResponse) - Grape::ServeFile::SendfileResponse.new([], status, headers) do |resp| - resp.body = bodies.file + if bodies.is_a?(Grape::ServeStream::StreamResponse) + Grape::ServeStream::SendfileResponse.new([], status, headers) do |resp| + resp.body = bodies.stream end else # Allow content-type to be explicitly overwritten diff --git a/lib/grape/serve_file/file_body.rb b/lib/grape/serve_stream/file_body.rb similarity index 96% rename from lib/grape/serve_file/file_body.rb rename to lib/grape/serve_stream/file_body.rb index ba9fa3a1e..99fb212f7 100644 --- a/lib/grape/serve_file/file_body.rb +++ b/lib/grape/serve_stream/file_body.rb @@ -1,5 +1,5 @@ module Grape - module ServeFile + module ServeStream CHUNK_SIZE = 16_384 # Class helps send file through API diff --git a/lib/grape/serve_file/sendfile_response.rb b/lib/grape/serve_stream/sendfile_response.rb similarity index 95% rename from lib/grape/serve_file/sendfile_response.rb rename to lib/grape/serve_stream/sendfile_response.rb index 17312a644..fa148030b 100644 --- a/lib/grape/serve_file/sendfile_response.rb +++ b/lib/grape/serve_stream/sendfile_response.rb @@ -1,5 +1,5 @@ module Grape - module ServeFile + module ServeStream # Response should respond to to_path method # for using Rack::SendFile middleware class SendfileResponse < Rack::Response diff --git a/lib/grape/serve_file/file_response.rb b/lib/grape/serve_stream/stream_response.rb similarity index 55% rename from lib/grape/serve_file/file_response.rb rename to lib/grape/serve_stream/stream_response.rb index 07b98cd21..a76ff5126 100644 --- a/lib/grape/serve_file/file_response.rb +++ b/lib/grape/serve_stream/stream_response.rb @@ -1,20 +1,20 @@ module Grape - module ServeFile - # A simple class used to identify responses which represent files and do not + module ServeStream + # A simple class used to identify responses which represent streams (or files) and do not # need to be formatted or pre-read by Rack::Response - class FileResponse - attr_reader :file + class StreamResponse + attr_reader :stream - # @param file [Object] - def initialize(file) - @file = file + # @param stream [Object] + def initialize(stream) + @stream = stream end # Equality provided mostly for tests. # # @return [Boolean] def ==(other) - file == other.file + stream == other.stream end end end diff --git a/spec/grape/dsl/inside_route_spec.rb b/spec/grape/dsl/inside_route_spec.rb index 8f6ea9723..9da8c3b34 100644 --- a/spec/grape/dsl/inside_route_spec.rb +++ b/spec/grape/dsl/inside_route_spec.rb @@ -185,15 +185,15 @@ def initialize let(:file_path) { '/some/file/path' } let(:file_response) do - file_body = Grape::ServeFile::FileBody.new(file_path) - Grape::ServeFile::FileResponse.new(file_body) + file_body = Grape::ServeStream::FileBody.new(file_path) + Grape::ServeStream::StreamResponse.new(file_body) end before do subject.file file_path end - it 'returns value wrapped in FileResponse' do + it 'returns value wrapped in StreamResponse' do expect(subject.file).to eq file_response end end @@ -202,14 +202,14 @@ def initialize let(:file_object) { Class.new } let(:file_response) do - Grape::ServeFile::FileResponse.new(file_object) + Grape::ServeStream::StreamResponse.new(file_object) end before do subject.file file_object end - it 'returns value wrapped in FileResponse' do + it 'returns value wrapped in StreamResponse' do expect(subject.file).to eq file_response end end @@ -222,7 +222,7 @@ def initialize describe '#stream' do describe 'set' do - let(:file_object) { Class.new } + let(:file_object) { double('StreamerObject', each: nil) } before do subject.header 'Cache-Control', 'cache' @@ -231,12 +231,12 @@ def initialize subject.stream file_object end - it 'returns value wrapped in FileResponse' do - expect(subject.stream).to eq Grape::ServeFile::FileResponse.new(file_object) + it 'returns value wrapped in StreamResponse' do + expect(subject.stream).to eq Grape::ServeStream::StreamResponse.new(file_object) end - it 'also sets result of file to value wrapped in FileResponse' do - expect(subject.file).to eq Grape::ServeFile::FileResponse.new(file_object) + it 'also sets result of file to value wrapped in StreamResponse' do + expect(subject.file).to eq Grape::ServeStream::StreamResponse.new(file_object) end it 'sets Cache-Control header to no-cache' do diff --git a/spec/grape/middleware/formatter_spec.rb b/spec/grape/middleware/formatter_spec.rb index 5a1fd1120..3748b09a9 100644 --- a/spec/grape/middleware/formatter_spec.rb +++ b/spec/grape/middleware/formatter_spec.rb @@ -287,9 +287,9 @@ def to_xml let(:app) { ->(_env) { [200, {}, @body] } } it 'returns Grape::Uril::SendFileReponse' do - @body = Grape::ServeFile::FileResponse.new('file') + @body = Grape::ServeStream::StreamResponse.new('file') env = { 'PATH_INFO' => '/somewhere', 'HTTP_ACCEPT' => 'application/json' } - expect(subject.call(env)).to be_a(Grape::ServeFile::SendfileResponse) + expect(subject.call(env)).to be_a(Grape::ServeStream::SendfileResponse) end end