Change attachment InputStream to ChannelProvider

[Tmpod]

Also remove full eager consuming of stream/channel when building request and
just pass it directly.

Closes #674 (discussed in this Discord topic as well)

[Tmpod]

right, this is a binary incompatible change

[lukellmann]

You need to run ./gradlew apiDump and commit the changes to make the tests pass

[lukellmann]

Another thought I had was to use ChannelProvider directly as inputs for the functions (instead of ByteReadChannel). This would mean the channel wouldn't even have to be opened until executing the request. What is your thought on this?

(and btw sorry for requesting so many changes)

[Tmpod]

Another thought I had was to use `ChannelProvider` directly as inputs for the functions (instead of `ByteReadChannel`). This would mean the channel wouldn't even have to be opened until executing the request. What is your thought on this?

Yeah I thought about it when first implementing this change, but ended up deciding that taking a read channel directly would be more convenient most of the time. However, also accepting a channel provider would make this even more versatile (which was kinda the whole point to this change). Perhaps I could change the internals once again to `ChannelProvider` and write overloads for those `addFile` methods and such that can accept both providers and channels directly (which would be wrapped in simple providers)?

(and btw sorry for requesting so many changes)

No problem at all! In fact, I appreciate the detailed reviewing :)

[lukellmann]

However, also accepting a channel provider would make this even more versatile (which was kinda the whole point to this change). Perhaps I could change the internals once again to ChannelProvider and write overloads for those addFile methods and such that can accept both providers and channels directly (which would be wrapped in simple providers)?

Sounds good 👍

[Tmpod]

Cool! I'll be out for most of today, so I'll either submit this later in the evening or tomorrow.

[Tmpod]

So yeah, now it complains that _component2 is using the deprecated inputStream property. The compiler should see that both are deprecated and not raise a warning, no?
Anyways, what do you suggest I do? Just have _component2 mimic inputStream or use a @Suppress? I assume the latter?

Also, would like suggestions regarding that test.

[lukellmann]

So yeah, now it complains that _component2 is using the deprecated inputStream property. The compiler should see that both are deprecated and not raise a warning, no?
Anyways, what do you suggest I do? Just have _component2 mimic inputStream or use a @Suppress? I assume the latter?

just slap @Suppress("DEPRECATION") on it :)

[lukellmann]

Also, would like suggestions regarding that test.

I think we actually need to mock a request so that the channel gets consumed. Take a look at StackTraceRecoveryTest for an example.

[Tmpod]

Right... 🤦
That was extra silly lol

[Tmpod]

Also, would like suggestions regarding that test.

I think we actually need to mock a request so that the channel gets consumed. Take a look at StackTraceRecoveryTest for an example.

The channel never seems to get consumed in the mock request, which is odd. I'll push what I have, maybe I'm overlooking something obvious again 🙃

[lukellmann]

The channel never seems to get consumed in the mock request, which is odd. I'll push what I have, maybe I'm overlooking something obvious again 🙃

Let me check this out and take a look at it.

[Tmpod]

Sure thing! Thanks a lot :)

[lukellmann]

Sure thing! Thanks a lot :)

I PRed the fix here: Tmpod#1

[lukellmann]

I found another problem with using ByteReadChannel directly without a ChannelProvider:

Here we retry a request when we got a rate limit. This means we call handle again and will rebuild the request here by creating a new MultiPartFormDataContent from the already built request.data. Since ByteReadChannel can only be read once, the second request will also fail because of this.

This is actually also a problem with InputStream that we had before, but now that we refactor this, what should we do?

• add the convenience functions that take ByteReadChannel (like it is rn)
• add the convenience functions that take ByteReadChannel but add a warning in the docs
• don't have these convenience functions (I would actually prefer this)

Of course rate limit on the exact request that uploads a file might not happen very often, but when it does, I'm pretty sure it'll fail.

[lukellmann]

Suggestions for updates to deprecation messages and replacements based on this comment: #682 (comment)

[Tmpod]

Mmm, well spotted.

This is actually also a problem with `InputStream` that we had before, but now that we refactor this, what should we do? - add the convenience functions that take `ByteReadChannel` (like it is rn) - add the convenience functions that take `ByteReadChannel` but add a warning in the docs - don't have these convenience functions (I would actually prefer this)

I agree with the last one yeah. We can still have `Path` overloads that make providers like `ChannelProvider { path.readChannel() }` but prevent people from making the mistake of providing a single channel that can potentially cause issues. *Edit: ugh, why doesn't github process markdown from email replies*

[lukellmann]

Looks good overall, just one last question left

[lukellmann]

I've committed my final touches here.

Could you update title and description of the PR (we no longer use ByteReadChannel and the test wasn't removed)? Then we would be good to go. Thanks for your efforts :)

[Tmpod]

I've committed my final touches here.

Perfect!

Could you update title and description of the PR

Yep, good point hehe

Thank you for your excellent reviewing! Helped me get back into Kotlin again :3