From 75e8c5bef618572d86fc3283494fd717819db989 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Thu, 26 Sep 2024 17:35:48 +0200 Subject: [PATCH] Remove file method (#2500) * Remove file method and specs A * Add CHANGELOG.md and UPGRADING.md * Remove leaky dummy class in inside_route_spec.rb * Use match operator instead of multiple examples * Use match operator instead of multiple examples * Fix rubocop --- CHANGELOG.md | 1 + UPGRADING.md | 8 ++ lib/grape/dsl/inside_route.rb | 14 --- spec/grape/dsl/inside_route_spec.rb | 149 +++++----------------------- 4 files changed, 33 insertions(+), 139 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aa56c6c119..09b7d2e88e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ #### Features * [#2497](https://github.com/ruby-grape/grape/pull/2497): Update RuboCop to 1.66.1 - [@ericproulx](https://github.com/ericproulx). +* [#2500](https://github.com/ruby-grape/grape/pull/2500): Remove deprecated `file` method - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes diff --git a/UPGRADING.md b/UPGRADING.md index cba4774bbe..acc74fbac8 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -1,6 +1,14 @@ Upgrading Grape =============== +### Upgrading to >= 2.3.0 + +#### Remove deprecated methods + +Deprecated `file` method has been removed. Use `send_file` or `stream`. + +See [#2500](https://github.com/ruby-grape/grape/pull/2500) + ### Upgrading to >= 2.2.0 ### `Length` validator diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb index ba1715de24..72b365bc44 100644 --- a/lib/grape/dsl/inside_route.rb +++ b/lib/grape/dsl/inside_route.rb @@ -305,20 +305,6 @@ def return_no_content body false end - # Deprecated method to send files to the client. Use `sendfile` or `stream` - def file(value = nil) - if value.is_a?(String) - Grape.deprecator.warn('Use sendfile or stream to send files.') - sendfile(value) - elsif !value.is_a?(NilClass) - Grape.deprecator.warn('Use stream to use a Stream object.') - stream(value) - else - Grape.deprecator.warn('Use sendfile or stream to send files.') - sendfile - end - end - # Allows you to send a file to the client via sendfile. # # @example diff --git a/spec/grape/dsl/inside_route_spec.rb b/spec/grape/dsl/inside_route_spec.rb index 8963a73a27..038e203140 100644 --- a/spec/grape/dsl/inside_route_spec.rb +++ b/spec/grape/dsl/inside_route_spec.rb @@ -1,25 +1,21 @@ # frozen_string_literal: true -module Grape - module DSL - module InsideRouteSpec - class Dummy - include Grape::DSL::InsideRoute - - attr_reader :env, :request, :new_settings - - def initialize - @env = {} - @header = {} - @new_settings = { namespace_inheritable: {}, namespace_stackable: {} } - end +describe Grape::Endpoint do + subject { dummy_class.new } + + let(:dummy_class) do + Class.new do + include Grape::DSL::InsideRoute + + attr_reader :env, :request, :new_settings + + def initialize + @env = {} + @header = {} + @new_settings = { namespace_inheritable: {}, namespace_stackable: {} } end end end -end - -describe Grape::Endpoint do - subject { Grape::DSL::InsideRouteSpec::Dummy.new } describe '#version' do it 'defaults to nil' do @@ -202,38 +198,6 @@ def initialize end end - describe '#file' do - describe 'set' do - context 'as file path' do - let(:file_path) { '/some/file/path' } - - it 'emits a warning that this method is deprecated' do - expect(Grape.deprecator).to receive(:warn).with(/Use sendfile or stream/) - expect(subject).to receive(:sendfile).with(file_path) - subject.file file_path - end - end - - context 'as object (backward compatibility)' do - let(:file_object) { double('StreamerObject', each: nil) } - - it 'emits a warning that this method is deprecated' do - expect(Grape.deprecator).to receive(:warn).with(/Use stream to use a Stream object/) - expect(subject).to receive(:stream).with(file_object) - subject.file file_object - end - end - end - - describe 'get' do - it 'emits a warning that this method is deprecated' do - expect(Grape.deprecator).to receive(:warn).with(/Use sendfile or stream/) - expect(subject).to receive(:sendfile) - subject.file - end - end - end - describe '#sendfile' do describe 'set' do context 'as file path' do @@ -248,36 +212,19 @@ def initialize subject.header Rack::CACHE_CONTROL, 'cache' subject.header Rack::CONTENT_LENGTH, 123 subject.header Grape::Http::Headers::TRANSFER_ENCODING, 'base64' - end - - it 'sends no deprecation warnings' do - expect(Grape.deprecator).not_to receive(:warn) - subject.sendfile file_path end it 'returns value wrapped in StreamResponse' do - subject.sendfile file_path - expect(subject.sendfile).to eq file_response end - it 'does not change the Cache-Control header' do - subject.sendfile file_path - - expect(subject.header[Rack::CACHE_CONTROL]).to eq 'cache' - end - - it 'does not change the Content-Length header' do - subject.sendfile file_path - - expect(subject.header[Rack::CONTENT_LENGTH]).to eq 123 - end - - it 'does not change the Transfer-Encoding header' do - subject.sendfile file_path - - expect(subject.header[Grape::Http::Headers::TRANSFER_ENCODING]).to eq 'base64' + it 'set the correct headers' do + expect(subject.header).to match( + Rack::CACHE_CONTROL => 'cache', + Rack::CONTENT_LENGTH => 123, + Grape::Http::Headers::TRANSFER_ENCODING => 'base64' + ) end end @@ -309,42 +256,15 @@ def initialize subject.header Rack::CACHE_CONTROL, 'cache' subject.header Rack::CONTENT_LENGTH, 123 subject.header Grape::Http::Headers::TRANSFER_ENCODING, 'base64' - end - - it 'emits no deprecation warnings' do - expect(Grape.deprecator).not_to receive(:warn) - subject.stream file_path end it 'returns file body wrapped in StreamResponse' do - subject.stream file_path - expect(subject.stream).to eq file_response end - it 'sets Cache-Control header to no-cache' do - subject.stream file_path - - expect(subject.header[Rack::CACHE_CONTROL]).to eq 'no-cache' - end - - it 'does not change Cache-Control header' do - subject.stream - - expect(subject.header[Rack::CACHE_CONTROL]).to eq 'cache' - end - - it 'sets Content-Length header to nil' do - subject.stream file_path - - expect(subject.header[Rack::CONTENT_LENGTH]).to be_nil - end - - it 'sets Transfer-Encoding header to nil' do - subject.stream file_path - - expect(subject.header[Grape::Http::Headers::TRANSFER_ENCODING]).to be_nil + it 'sets only the cache-control header' do + expect(subject.header).to match(Rack::CACHE_CONTROL => 'no-cache') end end @@ -359,36 +279,15 @@ def initialize subject.header Rack::CACHE_CONTROL, 'cache' subject.header Rack::CONTENT_LENGTH, 123 subject.header Grape::Http::Headers::TRANSFER_ENCODING, 'base64' - end - - it 'emits no deprecation warnings' do - expect(Grape.deprecator).not_to receive(:warn) - subject.stream stream_object end it 'returns value wrapped in StreamResponse' do - subject.stream stream_object - expect(subject.stream).to eq stream_response end - it 'sets Cache-Control header to no-cache' do - subject.stream stream_object - - expect(subject.header[Rack::CACHE_CONTROL]).to eq 'no-cache' - end - - it 'sets Content-Length header to nil' do - subject.stream stream_object - - expect(subject.header[Rack::CONTENT_LENGTH]).to be_nil - end - - it 'sets Transfer-Encoding header to nil' do - subject.stream stream_object - - expect(subject.header[Grape::Http::Headers::TRANSFER_ENCODING]).to be_nil + it 'set only the cache-control header' do + expect(subject.header).to match(Rack::CACHE_CONTROL => 'no-cache') end end @@ -403,7 +302,7 @@ def initialize it 'returns default' do expect(subject.stream).to be_nil - expect(subject.header[Rack::CACHE_CONTROL]).to be_nil + expect(subject.header).to be_empty end end