Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add more conversion methods for JVM streams #1121

Merged
merged 2 commits into from
Jul 12, 2024
Merged

Conversation

ianbotsf
Copy link
Contributor

Issue #

Closes awslabs/aws-sdk-kotlin#1352

Description of changes

This change adds new methods for converting to/from ByteStream and JVM streams:

  • InputStream.asByteStream: Wrap an InputStream as a ByteStream
  • ByteStream.fromInputStream: Alias for InputStream.asByteStream
  • ByteStream.writeToOutputStream: Writes the contents of a ByteStream to an OutputStream

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ianbotsf ianbotsf requested a review from a team as a code owner July 11, 2024 21:52

This comment has been minimized.

Copy link

Affected Artifacts

Changed in size
Artifact Pull Request (bytes) Latest Release (bytes) Delta (bytes) Delta (percentage)
runtime-core-jvm.jar 888,946 883,520 5,426 0.61%

* @param inputStream The [InputStream] from which to create a [ByteStream]
* @param contentLength If specified, indicates how many bytes remain in the input stream. Defaults to `null`.
*/
public fun ByteStream.Companion.fromInputStream(
Copy link
Contributor

@0marperez 0marperez Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: just curious, why an alias for InputStream.asByteStream?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a similar alias for File.asByteStream(..) so it seemed prudent to mirror the pattern.

Copy link
Contributor

@lauzadis lauzadis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this PR I think the only operation we're missing is converting an OutputStream to a ByteStream. Is that something you think would be useful, should we add it for the sake of completion?

@ianbotsf
Copy link
Contributor Author

After this PR I think the only operation we're missing is converting an OutputStream to a ByteStream. Is that something you think would be useful, should we add it for the sake of completion?

OutputStreams are meant for writing to, which makes them a natural counterpart to Okio's Sink or our SdkSink components. Using them as a Source may be possible but would be a slight inversion of the typical control flow. ByteStreams typically read from a source as opposed to being written to by a source so the use case is not super clear here. I can imagine users could use machinery like PipedOutputStream/PipedInputStream to bridge into something that could be a ByteStream source but even there the semantics are a little unclear—how does the ByteStream know when the OutputStream is "complete"? If it does know, does a hypothetical operation invocation simply block until the output stream is fully written?

I think we should wait for feature requests or user examples to put something concrete in place for OutputStreamByteStream conversion.

@ianbotsf ianbotsf merged commit b3a6d97 into main Jul 12, 2024
15 checks passed
@ianbotsf ianbotsf deleted the feat-more-jvm-streams branch July 12, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce functionality to wrap a InputStream as a ByteStream
3 participants