Ignore :into collectable for non-200 responses

[jonatanklosko]

Currently when we pass into: collectable, the body is streamed into the collectable, regardless the status. This can be problematics for collectables with side effects. For example, I imagine a common use case may be into: File.stream!(path) to download a file, and in case of an error, we would save error message body instead.

There is one concern, that I'm not sure how to solve, hence a draft (details in separate comment).

[jonatanklosko]

Technically we should call collector.(:halt) in case of an error. It wasn't the case in the existing code, but I assume that's unexpected.

The issue is that Finch.stream doesn't return the accumulator in case of errors, so we have no access to {acc, collector}. In the previous code this could've been fixed, because we call Collectable.into(collectable) at the top, but with the new code it's not possible. However, I do think we should only call Collectable.into(collectable) if we know the status is 200, since this call may have side effects (such as creating a file).

@wojtekmach @josevalim any ideas?

[josevalim]

Maybe we should change Finch to return the accumulator? Or do you mean the Collector never returns the accumulator?

[jonatanklosko]

Maybe we should change Finch to return the accumulator?

Yeah, I think that's exactly what we would need, I'm just not sure if that breaking change is acceptable :D

[josevalim]

It is not v1.0 yet. Plus, who is halting a collectable anyway?

[jonatanklosko]

FTR I will send a PR to finch. I guess it's not blocking this PR, since we hasn't been halting so far either.

[jonatanklosko]

See sneako/finch#295.

[wojtekmach]

cc @acalejos, I think you run into this recently!

[acalejos]

cc @acalejos, I think you run into this recently!

Yes I think I did! When a redirect occurred, if they body of the redirecting URL wasn't empty it would trigger the :into function. Expected behavior IMO would be for it to only trigger after all redirects, but Im not sure if it's possible for it to be aware of any redirects at the point when :into is called since it's called before response steps and redirects aren't always enabled

[jonatanklosko]

it would trigger the :into function

@acalejos you mean into: fn {:data, data}, {req, resp} ->, right? Since you have resp at that point, you could just pattern match on the resp status, and if it's not 200, you ignore the data, so a redirect would just go through. Though one could make an argument that it should only be called on 200 as well, and if needed we can have a option into_on_any_status: true applicable to both. But then the final question is what about :self, perhaps on non-200 req receives all messages itself and doesn't return async body? @wojtekmach I can send PR for any of these, just let me know :)

That said, this PR does fix the redirect case for into: collectable. Previously a redirect could result in the collectable invoked multiple times. Now, it will only happen for the final 200 response.

[acalejos]

@jonatanklosko

Yes, it was :into a function for my case (seen here: https://github.com/acalejos/Reed/blob/64832bc789916e0b2a7ab5347328833846846113/lib/reed/req_plugin.ex#L44)

You're right I should pattern match on the status in the resp. I think for now Im just letting the XML parser error on the data and rescuing it and letting that be the way to ignore it, so I should change that 😅

[wojtekmach]

Thank you!