Don't stream CSV in development

[seanlinsley]

This enables any exceptions to bubble up, instead of waiting until the request times out before sending back any data. This also lets you use in-browser debuggers, like better_errors.

TODO: add tests

[timoschilling]

there should be a option for that streame_csv with default to Rails.env.development? || Rails.env.test?

[seanlinsley]

I'm not sure what you're saying @timoschilling.

[timoschilling]

@seanlinsley I mean we should do it this way:

inheritable_setting :stream_csv, Rails.env.development? || Rails.env.test?

if active_admin_config.stream_csv
  self.response_body = block['']
else
  self.response_body = Enumerator.new &block
end

The main reason is, I want the ability/possibility to test the streaming in dev and test env

Why do you want to change that? What's the problem with streaming in dev / test env?

[seanlinsley]

I only took this approach because I couldn't figure out how to get the Enumerator to return early if an exception occurred while rendering the CSV. If we can figure out how to return early, that would be the better option.

[seanlinsley]

Related: http://stackoverflow.com/questions/25734814

[seanlinsley]

The stack trace after a request times out looks like this:

actionpack-4.1.6/lib/action_dispatch/http/response.rb:95:in `each'
actionpack-4.1.6/lib/action_dispatch/http/response.rb:95:in `each'
actionpack-4.1.6/lib/action_dispatch/http/response.rb:95:in `each'
actionpack-4.1.6/lib/action_dispatch/http/response.rb:50:in `each'
rack-1.5.2/lib/rack/body_proxy.rb:31:in `each'
rack-1.5.2/lib/rack/body_proxy.rb:31:in `each'
rack-1.5.2/lib/rack/body_proxy.rb:31:in `each'
rack-1.5.2/lib/rack/body_proxy.rb:31:in `each'
rack-1.5.2/lib/rack/body_proxy.rb:31:in `each'
rack-1.5.2/lib/rack/body_proxy.rb:31:in `each'
rack-1.5.2/lib/rack/body_proxy.rb:31:in `each'
Pow/Versions/0.5.0/node_modules/nack/lib/nack/server.rb:161:in `handle'
Pow/Versions/0.5.0/node_modules/nack/lib/nack/server.rb:109:in `rescue in block (2 levels) in start'
Pow/Versions/0.5.0/node_modules/nack/lib/nack/server.rb:106:in `block (2 levels) in start'
Pow/Versions/0.5.0/node_modules/nack/lib/nack/server.rb:96:in `each'
Pow/Versions/0.5.0/node_modules/nack/lib/nack/server.rb:96:in `block in start'
Pow/Versions/0.5.0/node_modules/nack/lib/nack/server.rb:76:in `loop'
Pow/Versions/0.5.0/node_modules/nack/lib/nack/server.rb:76:in `start'
Pow/Versions/0.5.0/node_modules/nack/lib/nack/server.rb:12:in `run'
Pow/Versions/0.5.0/node_modules/nack/bin/nack_worker:4:in `<main>'

Maybe the only proper solution is to patch Rails / Rack.

[seanlinsley]

You can emulate the current behavior (returning the exception) except without the wait time with this:

self.response_body = Enumerator.new do |receiver|
  begin
    yield receiver
  rescue => e
    receiver << {
      exception: e.class.to_s, message: e.message, backtrace: e.backtrace
    }.to_json
  end
end

[timoschilling]

I know the problem with stream error handling, and there is no solution for it at the moment.
See this blog post for example, there database connect middleware has the same problem like better errors.

Your PR is a good workaround, but there should be a solution (the stream_csv setting) to switch to stream if needed. Other wise you can't test the streaming behavior.

[seanlinsley]

Another approach is to use ActionController::Live, though then we're limiting ourselves to Rails 4:

$ git diff
diff --git a/lib/active_admin/csv_builder.rb b/lib/active_admin/csv_builder.rb
index 2d5b154..ea09284 100644
--- a/lib/active_admin/csv_builder.rb
+++ b/lib/active_admin/csv_builder.rb
@@ -39,20 +39,18 @@ module ActiveAdmin
       @columns << Column.new(name, @resource, column_transitive_options.merge(options), block)
     end

-    def build(view_context, receiver)
+    def build(view_context, stream)
       options = ActiveAdmin.application.csv_options.merge self.options
       columns = exec_columns view_context

       if options.delete(:column_names) { true }
-        receiver << CSV.generate_line(columns.map{ |c| encode c.name, options }, options)
+        stream.write CSV.generate_line(columns.map{ |c| encode c.name, options }, options)
       end

       view_context.send(:collection).find_each do |resource|
         resource = view_context.send :apply_decorator, resource
-        receiver << CSV.generate_line(build_row(resource, columns, options), options)
+        stream.write CSV.generate_line(build_row(resource, columns, options), options)
       end
-
-      receiver
     end

     def exec_columns(view_context = nil)
diff --git a/lib/active_admin/resource_controller/streaming.rb b/lib/active_admin/resource_controller/streaming.rb
index c9e8cc0..2d04c99 100644
--- a/lib/active_admin/resource_controller/streaming.rb
+++ b/lib/active_admin/resource_controller/streaming.rb
@@ -8,6 +8,10 @@ module ActiveAdmin
     #
     module Streaming

+      def self.included(klass)
+        klass.send :include, ActionController::Live
+      end
+
       def index
         super do |format|
           format.csv { stream_csv }
@@ -17,14 +21,17 @@ module ActiveAdmin

       protected

-      def stream_resource(&block)
+      def stream_resource
         headers['X-Accel-Buffering'] = 'no'
         headers['Cache-Control'] = 'no-cache'

-        if Rails.env.development? || Rails.env.test?
-          self.response_body = block['']
+        yield response.stream
+        response.stream.close
+      rescue
+        if Rails.env.development?
+          raise
         else
-          self.response_body = Enumerator.new &block
+          response.stream.close
         end
       end

[timoschilling]

Using ActionController::Live is a option. Does better_errors works with ActionController::Live?

Do you want to cat of Rails 3.2 in general or only for CSV or only for CSV streaming?

But I think your workaround is good enough, we should just add the config option, so can we set it to `true for specific streaming test.

[timoschilling]

What do you think about a stream_not_in option with default ['development', 'test'], that the user don't need to configured it for his own env names?

[seanlinsley]

I've never heard of teams straying from:

• development
• test
• staging
• production

Are custom environment names really that common?

Also: it seems like we'd only want this behavior in development, not in test.

Also: do you have any tests that expect a certain behavior (streaming or not) from CSV exports? Could you post an example here?

[timoschilling]

Yes custom environment names really happen, some developers use things like env == 'customer name' for whitelabel systems.

I don't have tests for streaming, but in a ActiveAdmin test I want to test the stream and not the download.

# To make debugging easier, by default only stream in staging & production
setting :stream_not_in, ['development']

looks like the best option

[dmitry]

Please also include somekind of documentation.

[timoschilling]

@dmitry whats the best way in your eyes?
setting :stream_not_in, ['development'] or setting :stream_in, ['production', 'staging']

[dmitry]

stream_in is better. Better is to provide ability, than restrict.

[timoschilling]

Sure, but the pro part of that restriction is that the user don't need to change it for other envs

[seanlinsley]

I'm not ready to commit them yet, but here are some changes I had locally:

$ git diff
diff --git a/lib/active_admin/application.rb b/lib/active_admin/application.rb
index 63ebc60..e00f6aa 100644
--- a/lib/active_admin/application.rb
+++ b/lib/active_admin/application.rb
@@ -104,8 +104,8 @@ module ActiveAdmin
                                       :email,
                                       :to_s ]

-    # To make debugging easier, by default only stream in staging & production
-    setting :stream_in, ['production', 'staging']
+    # To make debugging easier, by default don't stream in development
+    setting :disable_streaming, ['development']

     # == Deprecated Settings

diff --git a/lib/active_admin/resource_controller/streaming.rb b/lib/active_admin/resource_controller/streaming.rb
index 356e780..56bd4dc 100644
--- a/lib/active_admin/resource_controller/streaming.rb
+++ b/lib/active_admin/resource_controller/streaming.rb
@@ -21,12 +21,11 @@ module ActiveAdmin
         headers['X-Accel-Buffering'] = 'no'
         headers['Cache-Control'] = 'no-cache'

-        # To make debugging easier, by default only stream in staging/production
-        case Rails.env
-        when *ActiveAdmin.application.stream_in
-          self.response_body = Enumerator.new &block
-        else
+        # To make debugging easier, by default don't stream in development
+        if ActiveAdmin.application.disable_streaming.include? Rails.env
           self.response_body = block['']
+        else
+          self.response_body = Enumerator.new &block
         end
       end

[timoschilling]

👍

[timoschilling]

Maybe we should name it disable_streaming_in to make sure that it is a array property.
Or we should allow false as a value for compelled disabling.

[timoschilling]

@seanlinsley can you finish this PR?

[seanlinsley]

Okay I just revisited this PR. Oddly, in development my CSV exports consist of a single character:

[seanlinsley]

Ah nevermind, I was missing a line after rebasing.

[seanlinsley]

This is ready for review.