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

Add valuechunk event allow for large values #64

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kahalm
Copy link

@kahalm kahalm commented Jun 24, 2019

Fixes the problem with import for stored files stored in cachedb

Copy link
Collaborator

@evan-king evan-king left a comment

Choose a reason for hiding this comment

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

I am concerned about what will happen with this change for large mal-formed inputs. Specifically, can users who do not expect large values still fail early or will they get stuck waiting until the end of a stream to get an error?

Unless I'm missing a surviving way to support this scenario, I would like to see a new flag allowing users to opt into this non-failing buffer overflow situation. I suggest ALLOW_CHUNKED_VALUES but am open to clearer alternatives.

Additionally, there needs to be test coverage added to test/clarinet.js, implementing support in the generic test suit generator for specifying the new flag and MAX_BUFFER_LENGTH (left at default value when not provided) and introducing test cases that will emit valuechunk events or error respectively, based on the new flag.

And lastly, README.md needs to be updated to include the API changes introduced.

I'll also note that if values exceeding MAX_BUFFER_LENGTH are going to be allowed, the output ought to be predictable (chunks limited in length exactly to MAX_BUFFER_LENGTH). But I suspect that will only create further complexity and new failure cases with splitting multi-byte characters. Thus, I think it's fair to leave that problem for another day - perhaps the same one that addresses the deprecated use of new Buffer.

@kahalm
Copy link
Author

kahalm commented Jun 24, 2019

I will try to find some time to clean up the code, as the fix was not written by me i just hoped it would pass (as it fixed my problem).

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.

2 participants