Conditional streaming based on status code

[md5]

I have a scenario where I am streaming large files from AWS S3 with on_data and we occasionally run into S3 errors. When this happens, S3 returns an XML response body that gives more detail on the nature of the error, beyond what the HTTP status code provides on its own.

I was hoping that I could have code in on_data that collects chunks into env[:response_body] for error status codes while still streaming chunks to the appropriate sink in the case of successful responses, but it looks like (most of) the adapters explicitly set the response body to '' or nil after all chunks have been sent to on_data.

I've been able to work around this by putting the error response body into env[:error_body] instead, but this complicates the middleware that I want to parse the error response body because it can't just look at :response_body and needs to look at this other non-standard key as well.

An additional goal I have is to either customize the error class or error message raised by the :raise_error middleware to include the <Code> and <Message> elements from the S3 XML response. I've been able to work around this by rescuing and re-raising the error raised by :raise_error after modifying the message to include the additional data, but it seems very hacky and brittle.

I'd love to get any thoughts that others may have on this use case and see if there is something I've missed.

[md5]

To elaborate a bit on my scenario, this is basically what I have right now:

faraday = Faraday.new do |config|
  config.response :s3_errors
  config.response :raise_error
end

faraday.get(presigned_s3_url) do |req|
  req.options.on_data |chunk, _total_bytes, env|
    if env[:status] == 200
      sink << chunk
    else
      env[:error_body] ||= +''
      env[:error_body] << chunk
    end
  end
end

Then in my :s3_errors middleware, something like this:

def call(env)
  begin
    app.call(env)
  rescue Faraday::Error => e
    s3_detail = process_s3_error(env) if s3_error_response?(env)
    e.message << " #{s3_detail}" if s3_detail
    raise e
  end
end

What I would prefer is:

1. The else branch of my on_data could instead append to env[:response_body]
2. The order of the middlewares could be switched and my :s3_errors middleware could set some key like env[:error_detail]
3. The :raise_error middleware looks for env[:error_detail] and appends it to the message of any raised Faraday::Error or the :raise_error middleware allows for a callback that can override what gets raised

If a callback were supported by raise_error, I would expect something like build_error: ->(error_class, msg, response_values, env) { [error_class, msg, response_values] }

[iMacTia]

Hello @md5, thank you for opening this discussion and for all the details provided 🙏!

Streaming is a fairly recent feature, so this kind of feedback is really appreciated.

I'm glad the current implementation is flexible enough for you to still be able to achieve what you want, but I'm always happy to explore how we can make Faraday smarter and more helpful.

I'm currently away from my laptop and won't be back until later in the week, so apologies if I can't look further into this right now, but I'll make a note to do so once I'm back 👍.

To provide some quick and short feedback to your points:

1. This is not the first time I see people having conditions to the on_data callback to deal with the response status, so maybe the streaming API could accept a list of "streaming statuses", and if the response falls outside of it, it skips streaming and behaves normally
2. IIRC the error raised by raise_error should include the response, so if we manage to do point 1 above, we might not need to do this anymore?

[md5]

I think the idea of having a list of streaming statuses is great. That would definitely work for our use case.

You're also right that the Error raised by raise_error includes the response information, so it's not critical that raise_error itself provide the ability to alter the Error that is raised. My motivation for doing so was that we send these errors to Sentry and ideally the error class and/or message should allow us to distinguish between different types of errors.

The S3 error responses for something like a 400 or 403 error vary, so it would be great if I could raise something like S3Errors::ExpiredTokenError < Faraday::BadRequestError when the <Code> is ExpiredToken or S3Errors::AccessDeniedError < Faraday::ForbiddenError with a message of Request has expired when the <Code> is AccessDenied and the <Message> is Request has expired. I can still do that by rescuing and raising the new error class and maybe this use case is specialized enough that it's not something Faraday itself should do much to support.

[md5]

It seems like it could be useful to be able to specify multiple on_data blocks and have the first one with a matching status be used. I also think the use case of writing all the chunks to a writer IO is probably common. So something like this could be nice:

req.options.on_data(io, status: :all_success)
req.options.on_data($stderr)

I used :all_success as a shorthand for status: 200..299.

If no on_data matches, then it would fall back to using env[:response_body]. And if status: is not specified, it defaults to all statuses.

I think you'd also want to keep supporting the block form for cases where the block won't just be calling io << chunk

Edit: I realized after leaving this comment that the API is #on_data= and that it's not currently possible to set more than one of them unless that API changes... I suppose that #on_data= could be changed to simply replace all callbacks with a single callback with status: :all to preserve existing behavior for the writer, though I'm not sure there's a slick way to preserve the existing reader behavior without changing the method name.