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

Rework InputFiles #77

Merged
merged 17 commits into from
Nov 23, 2021
Merged

Rework InputFiles #77

merged 17 commits into from
Nov 23, 2021

Conversation

KnorpelSenf
Copy link
Member

@KnorpelSenf KnorpelSenf commented Nov 6, 2021

InputFile instances created from streams and iterators may not be reused. This PR adds a protection that will make the request fail if this is attempted. Previously, an empty or a partial file would be sent.

Also see this part of the docs on the issue: https://grammy.dev/advanced/transformers.html#use-cases-of-transformer-functions

Includes #87 and #94.

src/platform.deno.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@wojpawlik wojpawlik left a comment

Choose a reason for hiding this comment

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

Same problem as #28: I don't like putting getters in InputFile. The checks should be moved to Client. Perhaps the consumed field can even be eliminated: https://developer.mozilla.org/en-US/docs/Web/API/ReadableStream/locked

@KnorpelSenf
Copy link
Member Author

I don't like putting getters in InputFile.

Why not?

We can turn the getter into a function toIterator. That way, we could even eliminate all branching over data types in the client.

Perhaps the consumed field can even be eliminated: https://developer.mozilla.org/en-US/docs/Web/API/ReadableStream/locked

I looked into this, but I don't want to convert everything back and forth just to save a flag.

@wojpawlik
Copy link
Contributor

I don't like putting getters in InputFile.

Why not?

Complexity of file uploads should be contained in either Client or InputFile, not spread across both.

We can turn the getter into a function toIterator. That way, we could even eliminate all branching over data types in the client.

👍

@KnorpelSenf
Copy link
Member Author

Agree it would not be spread out as it is now.

I would prefer to have it in InputFile because what is supported can diverge for both platforms.

@KnorpelSenf KnorpelSenf marked this pull request as draft November 8, 2021 04:30
@KnorpelSenf KnorpelSenf requested a review from wojpawlik November 9, 2021 11:16
@KnorpelSenf KnorpelSenf marked this pull request as ready for review November 9, 2021 11:18
@KnorpelSenf
Copy link
Member Author

@wojpawlik what do you think about the SoC between InputFile and client now?

@KnorpelSenf
Copy link
Member Author

KnorpelSenf commented Nov 11, 2021

@KnightNiwrem you're right that the Promise support is superfluous. I'd say we remove it again for now. We can introduce support for asynchronous generation of input data streams in a different PR.

@KnorpelSenf
Copy link
Member Author

@wojpawlik @Loskir @KnightNiwrem can I merge?

@KnorpelSenf KnorpelSenf added the fix Fixes a bug label Nov 22, 2021
@KnorpelSenf KnorpelSenf changed the title Add protection against accidental stream reuse Rework InputFiles Nov 23, 2021
@KnorpelSenf KnorpelSenf merged commit 5638918 into main Nov 23, 2021
@KnorpelSenf KnorpelSenf deleted the stream-reuse-protection branch November 23, 2021 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants